amadan
Make the Sheets version contract explicit for native clients
store.Open took a recovery stamp unconditionally, so sequential writes advanced the tab version by two and the native app refetched on every other write and every multi-cell edit (B12). Open now stamps only when cells are stale, write responses carry the stamps they used (1 per cell write, N per batch, absent for format/size/GET), and the native client refetches only on resync or a jump past what it was told. Web grid behaviour is unchanged. Proven against real server responses. Reviewed by Fable 5.1 on #36; supersedes #35. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Squashed from fix/r1-native-version-contract into main.
11 files changed,
+239
−6
internal/sheets/grid.go+10 −1internal/sheets/gridstyle.go+1 −0internal/sheets/handlers_cell_batch.go+1 −1internal/sheets/handlers_grid.go+1 −0internal/sheets/store/handle_open.go+11 −2internal/sheets/store/handle_report.go+7 −1internal/sheets/store/handle_write_batch.go+1 −0internal/sheets/version_contract_test.go+125 −0native/apple/Sources/SheetsKit/SheetModel.swift+7 −1native/apple/Sources/SheetsKit/SheetWire.swift+4 −0native/apple/Tests/SheetsKitTests/VersionGapTests.swift+71 −0
CI passed — run details
CI log
amadan: .amadan/timeout: this job's budget is 15m0s (this runner allows up to 30m0s) === vet === CGO_ENABLED=0 go vet ./... === fmt === === build === CGO_ENABLED=0 go build ./... === migrations === rastrillo migration check rastrillo migration check: models and migrations agree === go-test === CGO_ENABLED=0 go test -count=1 ./... ? amadan.net/oficina/sheets/cmd/sheets [no test files] ok amadan.net/oficina/sheets/internal/guardstest 0.079s ok amadan.net/oficina/sheets/internal/sheets 10.961s ok amadan.net/oficina/sheets/internal/sheets/calc 0.007s ok amadan.net/oficina/sheets/internal/sheets/format 0.004s ok amadan.net/oficina/sheets/internal/sheets/live 0.054s ok amadan.net/oficina/sheets/internal/sheets/seal 0.003s ok amadan.net/oficina/sheets/internal/sheets/sealtest 0.004s ok amadan.net/oficina/sheets/internal/sheets/store 3.402s ok amadan.net/oficina/sheets/internal/sheets/xlsx 0.288s ? amadan.net/oficina/sheets/tools/gencorpus [no test files] === race === go test -race -count=1 ./internal/sheets/store/... ./internal/sheets/live/... ok amadan.net/oficina/sheets/internal/sheets/store 45.917s ok amadan.net/oficina/sheets/internal/sheets/live 1.059s
diff --git a/internal/sheets/grid.go b/internal/sheets/grid.go| index b32d0b1..14f4c18 100644 |
| --- a/internal/sheets/grid.go |
| +++ b/internal/sheets/grid.go |
| @@ -63,7 +63,16 @@ type gridResponse struct { |
| Cols int `json:"cols,omitempty"` |
| Cells []gridCell `json:"cells"` |
| Resync bool `json:"resync,omitempty"` |
| - Client string `json:"client,omitempty"` |
| + // Stamps is how many versions the write behind this response |
| + // consumed (store.Report.Stamps): 1 for a cell, N for an N-cell |
| + // batch, absent for format, size and snapshot responses, which |
| + // consume none. A client holding version H missed an edit iff |
| + // Version > H + max(Stamps, 1); the one of slack is because those |
| + // stampless responses report the NEXT stamp (Handle.Version), not |
| + // the last one issued. Native's SheetModel.accept is the reader; |
| + // grid.js only drops older payloads and ignores it. |
| + Stamps int `json:"stamps,omitempty"` |
| + Client string `json:"client,omitempty"` |
| // Formats is every distinct style the cells above refer to, keyed |
| // by the id each cell's F names. Absent when nothing in the batch |
| // is formatted. |
diff --git a/internal/sheets/gridstyle.go b/internal/sheets/gridstyle.go| index 893514b..a36eb78 100644 |
| --- a/internal/sheets/gridstyle.go |
| +++ b/internal/sheets/gridstyle.go |
| @@ -155,5 +155,6 @@ func (a *App) publishReport(s SheetSummary, h *store.Handle, rep store.Report) { |
| Cells: cells, |
| Formats: styles, |
| Resync: rep.Capped, |
| + Stamps: rep.Stamps, |
| }) |
| } |
diff --git a/internal/sheets/handlers_cell_batch.go b/internal/sheets/handlers_cell_batch.go| index 1cb390a..e794bf6 100644 |
| --- a/internal/sheets/handlers_cell_batch.go |
| +++ b/internal/sheets/handlers_cell_batch.go |
| @@ -58,7 +58,7 @@ func (a *App) handleCellBatchPost(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| cells, styles := fromReport(rep, newPosIndex(h), h) |
| - resp := gridResponse{Version: rep.Version, Cells: cells, Formats: styles, Client: in.Client} |
| + resp := gridResponse{Version: rep.Version, Cells: cells, Formats: styles, Stamps: rep.Stamps, Client: in.Client} |
| a.touchSheet(sheetIDFrom(r)) |
| a.publish(tabIDFrom(r), resp) |
| writeJSON(w, http.StatusOK, resp) |
diff --git a/internal/sheets/handlers_grid.go b/internal/sheets/handlers_grid.go| index 410c930..ad1f098 100644 |
| --- a/internal/sheets/handlers_grid.go |
| +++ b/internal/sheets/handlers_grid.go |
| @@ -177,6 +177,7 @@ func (a *App) handleCellPost(w http.ResponseWriter, r *http.Request) { |
| Cells: cells, |
| Formats: styles, |
| Resync: rep.Capped, |
| + Stamps: rep.Stamps, |
| Client: in.Client, |
| } |
| a.touchSheet(sheetIDFrom(r)) |
diff --git a/internal/sheets/store/handle_open.go b/internal/sheets/store/handle_open.go| index 8f3498b..83d3236 100644 |
| --- a/internal/sheets/store/handle_open.go |
| +++ b/internal/sheets/store/handle_open.go |
| @@ -154,8 +154,17 @@ func buildHandle(db *gorm.DB, sealer *seal.Sealer, tabID uint64) (*Handle, error |
| // A single stamp taken from h.nextVersion is, by construction, greater |
| // than every InputVersion loaded above, so it can never regress |
| // anything it touches. |
| - recoveryVersion := h.nextVersion |
| - h.nextVersion++ |
| + // |
| + // Only when something is stale. Taken unconditionally, every Open |
| + // consumed a stamp nothing was written under, and because a handle |
| + // is evicted at refcount zero (Release), sequential requests each |
| + // re-open it: consecutive writes then jumped by two, and a client |
| + // could not tell that from a write it missed (discussion #35). |
| + var recoveryVersion uint64 |
| + if len(stale) > 0 { |
| + recoveryVersion = h.nextVersion |
| + h.nextVersion++ |
| + } |
| // recoverStale (handle_batch.go) is what used to be a plain loop |
| // recomputing and persisting once per stale key — one transaction (one |
| // WAL fsync) per key. That is fine at the 5,001-cell scale the |
diff --git a/internal/sheets/store/handle_report.go b/internal/sheets/store/handle_report.go| index 835298f..ff7a414 100644 |
| --- a/internal/sheets/store/handle_report.go |
| +++ b/internal/sheets/store/handle_report.go |
| @@ -47,6 +47,10 @@ type Report struct { |
| Version uint64 |
| Cells []ChangedCell |
| Capped bool |
| + // Stamps is how many versions this write consumed: one per typed |
| + // cell, ending at Version. A client that last saw version H and |
| + // receives this report missed nothing iff Version <= H + Stamps. |
| + Stamps int |
| } |
| // Version is the version a snapshot taken right now would carry. |
| @@ -123,7 +127,9 @@ func (h *Handle) SetReport(rowID, colID uint64, raw string, actor uint64) (Repor |
| if err != nil { |
| return Report{}, err |
| } |
| - return h.report(key, rowID, colID, v, touched, capped), nil |
| + rep := h.report(key, rowID, colID, v, touched, capped) |
| + rep.Stamps = 1 |
| + return rep, nil |
| } |
| // report turns a recomputed touched set into the cells a client paints. |
diff --git a/internal/sheets/store/handle_write_batch.go b/internal/sheets/store/handle_write_batch.go| index f3a2378..74f4340 100644 |
| --- a/internal/sheets/store/handle_write_batch.go |
| +++ b/internal/sheets/store/handle_write_batch.go |
| @@ -92,5 +92,6 @@ func (h *Handle) SetBatch(writes []CellWrite, actor uint64) (Report, error) { |
| h.rowPos, h.colPos = staged.rowPos, staged.colPos |
| h.rowByPos, h.colByPos = staged.rowByPos, staged.colByPos |
| h.nextVersion = staged.nextVersion |
| + result.Stamps = len(writes) // one SetReport, so one stamp, per write |
| return result, nil |
| } |
diff --git a/internal/sheets/version_contract_test.go b/internal/sheets/version_contract_test.go| new file mode 100644 |
| index 0000000..f88e83b |
| --- /dev/null |
| +++ b/internal/sheets/version_contract_test.go |
| @@ -0,0 +1,125 @@ |
| +package sheets |
| + |
| +import ( |
| + "fmt" |
| + "net/http" |
| + "net/http/httptest" |
| + "testing" |
| +) |
| + |
| +// versionFollower mirrors native SheetModel.accept (native/apple/Sources/ |
| +// SheetsKit/SheetModel.swift), the one client that detects missed edits |
| +// from the version: drop anything older than what it holds, refetch on |
| +// resync or when the version moved further than the response accounts |
| +// for, otherwise apply. Change the two together. |
| +type versionFollower struct { |
| + have uint64 |
| + refetches int |
| +} |
| + |
| +func (f *versionFollower) missed(g gridResponse) bool { |
| + allowed := uint64(max(g.Stamps, 1)) |
| + return g.Resync || g.Version > f.have+allowed |
| +} |
| + |
| +// accept returns whether the response made the follower refetch. |
| +func (f *versionFollower) accept(t *testing.T, g gridResponse, refetch func() gridResponse) bool { |
| + t.Helper() |
| + if g.Version < f.have { |
| + return false |
| + } |
| + if f.missed(g) { |
| + f.refetches++ |
| + f.have = refetch().Version |
| + return true |
| + } |
| + f.have = g.Version |
| + return false |
| +} |
| + |
| +// B12 and its review (#35): replay REAL server responses — cell writes, |
| +// batches (with a derived formula cell), a format, a size, snapshots — |
| +// through the native rule. With nothing missed, nothing may refetch: an |
| +// Open used to take a stamp every time, and the handle is evicted between |
| +// requests, so sequential writes jumped by two and every other one |
| +// refetched. With one write missed, the next response of every kind must |
| +// refetch, including a format, whose N cells consume no stamp. |
| +func TestNativeVersionContractAgainstRealResponses(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, cells := newSheet(t, app, cookies, "Budget") |
| + snapshot := func() gridResponse { |
| + t.Helper() |
| + rec := do(t, app, as(httptest.NewRequest(http.MethodGet, cells+"?rows=20&cols=10", nil), cookies)) |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET cells = %d: %s", rec.Code, rec.Body.String()) |
| + } |
| + return decodeGrid(t, rec) |
| + } |
| + ok := func(rec *httptest.ResponseRecorder) gridResponse { |
| + t.Helper() |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("write = %d: %s", rec.Code, rec.Body.String()) |
| + } |
| + return decodeGrid(t, rec) |
| + } |
| + cell := func(r, c int, raw string) gridResponse { |
| + return ok(postCell(t, app, cookies, cells, fmt.Sprintf(`{"r":%d,"c":%d,"raw":%q}`, r, c, raw))) |
| + } |
| + batch := func(body string) gridResponse { return ok(postCell(t, app, cookies, cells+"/batch", body)) } |
| + format := func(body string) gridResponse { return ok(postFormat(t, app, cookies, s, body)) } |
| + size := func(body string) gridResponse { return ok(postSize(t, app, cookies, s, body)) } |
| + |
| + t.Run("nothing missed, nothing refetched", func(t *testing.T) { |
| + f := &versionFollower{have: snapshot().Version} |
| + steps := []struct { |
| + name string |
| + resp func() gridResponse |
| + }{ |
| + {"cell", func() gridResponse { return cell(0, 0, "1") }}, |
| + {"cell", func() gridResponse { return cell(1, 0, "2") }}, |
| + {"cell", func() gridResponse { return cell(2, 0, "3") }}, |
| + {"batch of 3 with a derived formula", func() gridResponse { |
| + return batch(`{"cells":[{"r":3,"c":0,"raw":"=SUM(A1:A3)"},{"r":0,"c":1,"raw":"b"},{"r":0,"c":0,"raw":"10"}]}`) |
| + }}, |
| + {"format over 6 cells", func() gridResponse { return format(`{"r1":0,"c1":0,"r2":1,"c2":2,"bold":true}`) }}, |
| + {"cell after format", func() gridResponse { return cell(1, 1, "x") }}, |
| + {"size", func() gridResponse { return size(`{"col":2,"size":220}`) }}, |
| + {"batch of 2", func() gridResponse { return batch(`{"cells":[{"r":5,"c":0,"raw":"5"},{"r":6,"c":0,"raw":"6"}]}`) }}, |
| + {"clear", func() gridResponse { return cell(5, 0, "") }}, |
| + {"snapshot", snapshot}, |
| + {"cell after snapshot", func() gridResponse { return cell(7, 0, "7") }}, |
| + {"batch of 4", func() gridResponse { |
| + return batch(`{"cells":[{"r":8,"c":0,"raw":"8"},{"r":8,"c":1,"raw":"8"},{"r":8,"c":2,"raw":"8"},{"r":8,"c":3,"raw":"8"}]}`) |
| + }}, |
| + } |
| + for _, step := range steps { |
| + before := f.have |
| + g := step.resp() |
| + if f.accept(t, g, snapshot) { |
| + t.Errorf("%s: refetched with nothing missed (held %d, got version %d, stamps %d)", |
| + step.name, before, g.Version, g.Stamps) |
| + } |
| + } |
| + }) |
| + |
| + t.Run("one write missed, the next response refetches", func(t *testing.T) { |
| + for _, next := range []struct { |
| + name string |
| + resp func() gridResponse |
| + }{ |
| + {"cell", func() gridResponse { return cell(10, 0, "n") }}, |
| + {"batch", func() gridResponse { return batch(`{"cells":[{"r":10,"c":1,"raw":"n"},{"r":10,"c":2,"raw":"n"}]}`) }}, |
| + {"format over 20 cells", func() gridResponse { return format(`{"r1":11,"c1":0,"r2":14,"c2":4,"italic":true}`) }}, |
| + {"size", func() gridResponse { return size(`{"col":3,"size":140}`) }}, |
| + } { |
| + // Hold the version of a write this follower saw, then miss one. |
| + f := &versionFollower{have: cell(9, 0, "seen").Version} |
| + cell(9, 1, "missed") |
| + g := next.resp() |
| + if !f.missed(g) { |
| + t.Errorf("%s after a missed write was applied, not refetched (held %d, got version %d, stamps %d)", |
| + next.name, f.have, g.Version, g.Stamps) |
| + } |
| + } |
| + }) |
| +} |
diff --git a/native/apple/Sources/SheetsKit/SheetModel.swift b/native/apple/Sources/SheetsKit/SheetModel.swift| index 2decf12..964f83a 100644 |
| --- a/native/apple/Sources/SheetsKit/SheetModel.swift |
| +++ b/native/apple/Sources/SheetsKit/SheetModel.swift |
| @@ -114,7 +114,13 @@ import RastrilloApple |
| func accept(_ response: GridResponse) async { |
| guard response.version >= version else { return } |
| - if response.resync == true || response.version > version + 1 { |
| + // A write of N cells moves the version by N and says so in `stamps`. |
| + // Stampless responses (format, size, snapshot) report the server's |
| + // NEXT stamp, hence the minimum of one. A wider jump is a missed |
| + // edit. The server's TestNativeVersionContractAgainstRealResponses |
| + // replays real responses through a Go mirror of this rule. |
| + let allowed = UInt64(max(response.stamps ?? 0, 1)) |
| + if response.resync == true || response.version > version + allowed { |
| await refresh() |
| return |
| } |
diff --git a/native/apple/Sources/SheetsKit/SheetWire.swift b/native/apple/Sources/SheetsKit/SheetWire.swift| index a7b6c3b..ec455b4 100644 |
| --- a/native/apple/Sources/SheetsKit/SheetWire.swift |
| +++ b/native/apple/Sources/SheetsKit/SheetWire.swift |
| @@ -66,6 +66,10 @@ struct GridResponse: Decodable, Sendable { |
| var formats: [String: CellStyle]? |
| var sizes: Sizes? |
| var resync: Bool? |
| + /// Versions the write behind this response consumed: 1 for a cell, N |
| + /// for an N-cell batch, absent for format, size and snapshot responses |
| + /// (server grid.go gridResponse.Stamps). See SheetModel.accept. |
| + var stamps: Int? |
| } |
| struct CellWrite: Encodable, Sendable { |
| let r: Int |
diff --git a/native/apple/Tests/SheetsKitTests/VersionGapTests.swift b/native/apple/Tests/SheetsKitTests/VersionGapTests.swift| new file mode 100644 |
| index 0000000..8663aac |
| --- /dev/null |
| +++ b/native/apple/Tests/SheetsKitTests/VersionGapTests.swift |
| @@ -0,0 +1,71 @@ |
| +import Foundation |
| +import RastrilloApple |
| +import Testing |
| + |
| +@testable import SheetsKit |
| + |
| +// B12 and its review (#35). The server says how many versions a write |
| +// consumed (`stamps`); accept() refetches only when the version moved |
| +// further than that, so a multi-cell batch applies in place. Stampless |
| +// responses (format, size, snapshot) allow one. The server-side proof, |
| +// against real responses, is TestNativeVersionContractAgainstRealResponses. |
| +// |
| +// The client points at a closed port, so an attempted refetch shows as a |
| +// connection error and leaves the incremental cells unapplied. |
| +@MainActor private func gapModel(version: UInt64) -> SheetModel { |
| + let model = SheetModel(client: NativeHTTP(origin: URL(string: "http://127.0.0.1:1")!)) |
| + model.page = SheetPage( |
| + Sheet: SheetSummary(ID: 1, Title: "Fixture", TabID: 1, TabName: "Sheet 1"), Rows: 10, Cols: 5, |
| + Truncated: false, MayWrite: true, MayComment: true, MayShare: false, FillSwatches: [], |
| + InkSwatches: [], NumberFormats: [], Warned: nil) |
| + model.version = version |
| + model.error = "" |
| + return model |
| +} |
| + |
| +@MainActor @Test func stampedBatchAppliesWithoutRefetch() async { |
| + let model = gapModel(version: 5) |
| + // Three typed cells (versions 6, 7, 8) plus one formula they changed. |
| + await model.received( |
| + SheetEvent( |
| + name: "cells", |
| + data: #"{"version":8,"stamps":3,"cells":[{"r":0,"c":0,"raw":"1","display":"1"},{"r":1,"c":0,"raw":"2","display":"2"},{"r":2,"c":0,"raw":"3","display":"3"},{"r":3,"c":0,"raw":"=SUM(A1:A3)","display":"6","derived":true}]}"# |
| + ), source: model.grid) |
| + #expect(model.error == "", "a stamped batch attempted a refetch: \(model.error)") |
| + #expect(model.version == 8) |
| + #expect(model.cells[Coordinate(r: 2, c: 0)]?.raw == "3") |
| + #expect(model.cells[Coordinate(r: 3, c: 0)]?.display == "6") |
| +} |
| + |
| +@MainActor @Test func versionGapBeyondTheStampsStillRefetches() async { |
| + let model = gapModel(version: 5) |
| + // Two stamps ending at 9, so versions 6 and 7 were missed. |
| + await model.received( |
| + SheetEvent( |
| + name: "cells", |
| + data: #"{"version":9,"stamps":2,"cells":[{"r":0,"c":0,"raw":"1","display":"1"},{"r":1,"c":0,"raw":"2","display":"2"}]}"# |
| + ), source: model.grid) |
| + #expect(model.error != "", "a real gap was applied incrementally instead of refetched") |
| + #expect(model.version == 5) |
| + #expect(model.cells[Coordinate(r: 0, c: 0)] == nil) |
| +} |
| + |
| +@MainActor @Test func formatCellsDoNotWidenTheAcceptedJump() async { |
| + let model = gapModel(version: 5) |
| + // A format over four cells consumes no stamp; version 7 means a missed |
| + // edit, however many cells the format response lists. |
| + let cells = (0..<4).map { Cell(r: $0, c: 0, raw: "", display: "") } |
| + await model.accept(GridResponse(version: 7, cells: cells)) |
| + #expect(model.error != "") |
| + #expect(model.version == 5) |
| +} |
| + |
| +@MainActor @Test func singleStepsAndStamplessResponsesStillApply() async { |
| + let model = gapModel(version: 5) |
| + await model.accept(GridResponse(version: 6, cells: [Cell(r: 0, c: 0, raw: "x", display: "x")], stamps: 1)) |
| + // A format right after reports the next stamp: one ahead, no stamps. |
| + await model.accept(GridResponse(version: 7, cells: [Cell(r: 0, c: 0, raw: "x", display: "x")])) |
| + #expect(model.error == "") |
| + #expect(model.version == 7) |
| + #expect(model.cells[Coordinate(r: 0, c: 0)]?.raw == "x") |
| +} |