amadan

oficina / sheets Public

fix: retain cell version watermark across clear and reopen

Paul Campbell pushed by paul@keymail.dev 6ccb6091e01432328ae3a7a0ef981aa9c2f83ef9 parent 9f6abe2
10 files changed, +268 −9
  • internal/sheets/migrations/0010_alter_tabs.sql +2 −0
  • internal/sheets/migrations/schema.sql +1 −1
  • internal/sheets/store/handle_open.go +8 −1
  • internal/sheets/store/handle_report.go +3 −0
  • internal/sheets/store/handle_set.go +10 −1
  • internal/sheets/store/handle_version.go +27 −0
  • internal/sheets/store/handle_version_export_test.go +14 −0
  • internal/sheets/store/handle_version_limits_test.go +184 −0
  • internal/sheets/store/handle_version_migration_test.go +12 −0
  • internal/sheets/store/models.go +7 −6

CI cancelled — superseded by a newer push — run details

Intent

Humanclaude~paul@keymail.dev

READ-ONLY focused Fable design re-review, no edits/deploys. Repo /home/paulca/amadan.net/oficina/sheets-core/.claude/worktrees/overnight-versions exact HEAD9f6abe2. Read your prior review /tmp/sheets-durable-versions-fable-review.json result, then git diff91e9991..HEAD (insert space after diff). Revision2 spec and regressions now address your four required amendments: MAX SQL + missing-tab Open, snapshot continuity/capped-blank watermark/retained-batch rollback tests, pinned namespace-qualified pre-watermark migration IDs and migration check gate. Capped-blank mutation will run after implementation exists. Explicit APPROVE or REQUEST CHANGES for DESIGN before implementation; current missing-column/version regression tests remain intentionally failing. Inspect exact touched files; ignore separate fixture/tabs/reconciliation designs. Keep response focused on blockers.

full ledger: fix/durable-tab-versions

diff --git a/internal/sheets/migrations/0010_alter_tabs.sql b/internal/sheets/migrations/0010_alter_tabs.sql
new file mode 100644
index 0000000..661e081
--- /dev/null
+++ b/internal/sheets/migrations/0010_alter_tabs.sql
@@ -0,0 +1,2 @@
+ALTER TABLE `tabs` ADD `content_version` integer NOT NULL DEFAULT 0;
+
diff --git a/internal/sheets/migrations/schema.sql b/internal/sheets/migrations/schema.sql
index a15b30a..6d8675c 100644
--- a/internal/sheets/migrations/schema.sql
+++ b/internal/sheets/migrations/schema.sql
@@ -23,7 +23,7 @@ CREATE TABLE `sheets` (`id` integer PRIMARY KEY AUTOINCREMENT,`title_sealed` blo
CREATE TABLE `tab_views` (`id` integer PRIMARY KEY AUTOINCREMENT,`tab_id` integer,`spec_sealed` blob,`updated_by` integer,`updated_at` datetime);
-CREATE TABLE `tabs` (`id` integer PRIMARY KEY AUTOINCREMENT,`sheet_id` integer,`name_sealed` blob,`position` integer,`frozen_rows` integer,`frozen_cols` integer);
+CREATE TABLE `tabs` (`id` integer PRIMARY KEY AUTOINCREMENT,`sheet_id` integer,`name_sealed` blob,`position` integer,`frozen_rows` integer,`frozen_cols` integer, `content_version` integer NOT NULL DEFAULT 0);
CREATE UNIQUE INDEX `idx_col_tab_pos` ON `cols`(`tab_id`,`position`);
diff --git a/internal/sheets/store/handle_open.go b/internal/sheets/store/handle_open.go
index 9e76354..8f3498b 100644
--- a/internal/sheets/store/handle_open.go
+++ b/internal/sheets/store/handle_open.go
@@ -43,6 +43,10 @@ import (
// calc's evaluator to real ids, which is the more invasive but more
// durable fix. This handle does neither because it needs neither yet.
func buildHandle(db *gorm.DB, sealer *seal.Sealer, tabID uint64) (*Handle, error) {
+ var tab Tab
+ if err := db.Select("id", "content_version").First(&tab, tabID).Error; err != nil {
+ return nil, fmt.Errorf("store: load tab version: %w", err)
+ }
var rows []Row
if err := db.Where("tab_id = ?", tabID).Find(&rows).Error; err != nil {
return nil, fmt.Errorf("store: load rows: %w", err)
@@ -78,7 +82,7 @@ func buildHandle(db *gorm.DB, sealer *seal.Sealer, tabID uint64) (*Handle, error
}
var stale []calc.CellKey
- var maxVersion uint64
+ maxVersion := tab.ContentVersion
for _, c := range cells {
key, ok := h.cellKey(c.RowID, c.ColID)
@@ -125,6 +129,9 @@ func buildHandle(db *gorm.DB, sealer *seal.Sealer, tabID uint64) (*Handle, error
stale = append(stale, key)
}
}
+ if maxVersion >= maxContentVersion-1 {
+ return nil, errVersionExhausted
+ }
h.nextVersion = maxVersion + 1
// Crash recovery: any cell whose CacheVersion trails its InputVersion
diff --git a/internal/sheets/store/handle_report.go b/internal/sheets/store/handle_report.go
index 15111d8..835298f 100644
--- a/internal/sheets/store/handle_report.go
+++ b/internal/sheets/store/handle_report.go
@@ -105,6 +105,9 @@ func (h *Handle) SetReport(rowID, colID uint64, raw string, actor uint64) (Repor
return Report{}, err
}
+ if h.nextVersion >= maxContentVersion {
+ return Report{}, errVersionExhausted
+ }
v := h.nextVersion
prevRaw := h.rawText[key]
diff --git a/internal/sheets/store/handle_set.go b/internal/sheets/store/handle_set.go
index f4a8039..c64abbc 100644
--- a/internal/sheets/store/handle_set.go
+++ b/internal/sheets/store/handle_set.go
@@ -52,6 +52,9 @@ func (h *Handle) setBlank(rowID, colID uint64, key calc.CellKey, v uint64, actor
withinCap := len(touched) > 0 && len(touched) <= calc.MaxDirtyPerPass
if err := h.db.Transaction(func(tx *gorm.DB) error {
+ if err := h.advanceContentVersion(tx, v); err != nil {
+ return err
+ }
if err := tx.Where("tab_id = ? AND row_id = ? AND col_id = ?", h.tabID, rowID, colID).
Delete(&Cell{}).Error; err != nil {
return err
@@ -78,6 +81,7 @@ func (h *Handle) setBlank(rowID, colID uint64, key calc.CellKey, v uint64, actor
}); err != nil {
return nil, false, err
}
+ h.nextVersion++
// Test seam: a crash landing exactly here — transaction 1 durably
// committed, nothing after it has run yet — is what
// TestBlankCrashLeavesDependentsRecoverable simulates. Always nil in
@@ -85,7 +89,6 @@ func (h *Handle) setBlank(rowID, colID uint64, key calc.CellKey, v uint64, actor
if err := afterBlankTransactionOneCommits(); err != nil {
return nil, false, err
}
- h.nextVersion++
delete(h.rawText, key)
delete(h.keyToIDs, key)
h.graph.Clear(key)
@@ -124,6 +127,9 @@ func (h *Handle) setContent(rowID, colID uint64, key calc.CellKey, node calc.Nod
return nil, false, err
}
if err := h.db.Transaction(func(tx *gorm.DB) error {
+ if err := h.advanceContentVersion(tx, v); err != nil {
+ return err
+ }
res := tx.Model(&Cell{}).
Where("tab_id = ? AND row_id = ? AND col_id = ?", h.tabID, rowID, colID).
Updates(map[string]any{
@@ -228,6 +234,9 @@ func (h *Handle) recalcTouched(origin calc.CellKey, includeOrigin bool) (map[cal
// caused it — there was never a single-trigger claim to violate.
func (h *Handle) persistTouched(touched map[calc.CellKey]struct{}, v uint64) error {
return h.db.Transaction(func(tx *gorm.DB) error {
+ if err := h.advanceContentVersion(tx, v); err != nil {
+ return err
+ }
for key := range touched {
ids, ok := h.keyToIDs[key]
if !ok {
diff --git a/internal/sheets/store/handle_version.go b/internal/sheets/store/handle_version.go
new file mode 100644
index 0000000..765d787
--- /dev/null
+++ b/internal/sheets/store/handle_version.go
@@ -0,0 +1,27 @@
+package store
+
+import (
+ "errors"
+ "math"
+
+ "gorm.io/gorm"
+)
+
+const maxContentVersion uint64 = math.MaxInt64
+
+var errVersionExhausted = errors.New("store: content version exhausted")
+
+func (h *Handle) advanceContentVersion(tx *gorm.DB, version uint64) error {
+ if version >= maxContentVersion {
+ return errVersionExhausted
+ }
+ result := tx.Model(&Tab{}).Where("id = ?", h.tabID).
+ Select("content_version").Update("content_version", gorm.Expr("MAX(content_version, ?)", version))
+ if result.Error != nil {
+ return result.Error
+ }
+ if result.RowsAffected != 1 {
+ return gorm.ErrRecordNotFound
+ }
+ return nil
+}
diff --git a/internal/sheets/store/handle_version_export_test.go b/internal/sheets/store/handle_version_export_test.go
new file mode 100644
index 0000000..4ea4ba1
--- /dev/null
+++ b/internal/sheets/store/handle_version_export_test.go
@@ -0,0 +1,14 @@
+package store
+
+import "gorm.io/gorm"
+
+func AdvanceContentVersionForTest(db *gorm.DB, tabID, version uint64) error {
+ h := &Handle{tabID: tabID}
+ return db.Transaction(func(tx *gorm.DB) error { return h.advanceContentVersion(tx, version) })
+}
+
+func SetNextVersionForTest(h *Handle, version uint64) {
+ h.mu.Lock()
+ defer h.mu.Unlock()
+ h.nextVersion = version
+}
diff --git a/internal/sheets/store/handle_version_limits_test.go b/internal/sheets/store/handle_version_limits_test.go
new file mode 100644
index 0000000..b5f006f
--- /dev/null
+++ b/internal/sheets/store/handle_version_limits_test.go
@@ -0,0 +1,184 @@
+package store_test
+
+import (
+ "math"
+ "strconv"
+ "testing"
+
+ "amadan.net/oficina/sheets/internal/sheets/store"
+)
+
+func TestVersionWatermarkNeverLowersAndRequiresTab(t *testing.T) {
+ db, _, id := seeded(t)
+ if err := store.AdvanceContentVersionForTest(db, id, 100); err != nil {
+ t.Fatal(err)
+ }
+ if err := store.AdvanceContentVersionForTest(db, id, 4); err != nil {
+ t.Fatal(err)
+ }
+ var mark uint64
+ if err := db.Raw("SELECT content_version FROM tabs WHERE id = ?", id).Scan(&mark).Error; err != nil {
+ t.Fatal(err)
+ }
+ if mark != 100 {
+ t.Fatalf("watermark lowered to %d", mark)
+ }
+ if err := store.AdvanceContentVersionForTest(db, id+1000, 4); err == nil {
+ t.Fatal("missing tab accepted")
+ }
+}
+
+func TestOpenRequiresTabVersionRow(t *testing.T) {
+ db, s, id := seeded(t)
+ h, err := store.Open(db, s, id+1000)
+ if err == nil {
+ h.Release()
+ t.Fatal("missing tab opened")
+ }
+}
+
+func TestMissingTabRefusesRetainedHandleWrite(t *testing.T) {
+ db, s, id := seeded(t)
+ h, err := store.Open(db, s, id)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer h.Release()
+ if err = db.Delete(&store.Tab{}, id).Error; err != nil {
+ t.Fatal(err)
+ }
+ if _, err = h.SetReport(rowA, colA, "9", actor); err == nil {
+ t.Fatal("write accepted without version row")
+ }
+ if h.Value(rowA, colA).Num != 5 {
+ t.Fatal("failed write changed graph")
+ }
+}
+
+func TestHighWatermarkSurvivesEmptyTabOpen(t *testing.T) {
+ db, s, id := seeded(t)
+ if err := db.Where("tab_id = ?", id).Delete(&store.Cell{}).Error; err != nil {
+ t.Fatal(err)
+ }
+ const high = math.MaxInt64 - 10
+ if err := store.AdvanceContentVersionForTest(db, id, high); err != nil {
+ t.Fatal(err)
+ }
+ h, err := store.Open(db, s, id)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer h.Release()
+ rep, err := h.SetReport(rowA, colA, "9", actor)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if rep.Version <= high {
+ t.Fatalf("high watermark lost: %d", rep.Version)
+ }
+}
+
+func TestVersionExhaustionRefusesWithoutMutation(t *testing.T) {
+ for _, mark := range []int64{math.MaxInt64, math.MaxInt64 - 1, -1} {
+ t.Run(strconv.FormatInt(mark, 10), func(t *testing.T) {
+ db, s, id := seeded(t)
+ if err := db.Model(&store.Tab{}).Where("id = ?", id).Update("content_version", mark).Error; err != nil {
+ t.Fatal(err)
+ }
+ h, err := store.Open(db, s, id)
+ if err == nil {
+ h.Release()
+ t.Fatal("invalid watermark opened")
+ }
+ })
+ }
+ for _, batch := range []bool{false, true} {
+ t.Run(map[bool]string{false: "single", true: "batch"}[batch], func(t *testing.T) {
+ db, s, id := seeded(t)
+ h, err := store.Open(db, s, id)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer h.Release()
+ var before store.Tab
+ if err = db.First(&before, id).Error; err != nil {
+ t.Fatal(err)
+ }
+ store.SetNextVersionForTest(h, math.MaxInt64)
+ if batch {
+ _, err = h.SetBatch([]store.CellWrite{{Row: 0, Col: 0, Raw: "9"}}, actor)
+ } else {
+ _, err = h.SetReport(rowA, colA, "9", actor)
+ }
+ if err == nil {
+ t.Fatal("exhausted allocation accepted")
+ }
+ var after store.Tab
+ if err = db.First(&after, id).Error; err != nil {
+ t.Fatal(err)
+ }
+ if after.ContentVersion != before.ContentVersion || h.Value(rowA, colA).Num != 5 {
+ t.Fatal("exhausted write mutated state")
+ }
+ })
+ }
+}
+
+func TestFormulaRecoveryVersionSurvivesClearingNewestCaches(t *testing.T) {
+ db, s, id := seeded(t)
+ h, err := store.Open(db, s, id)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err = h.Set(rowB, colA, "=A1*2", actor); err != nil {
+ t.Fatal(err)
+ }
+ if err = h.Set(rowA, colA, "8", actor); err != nil {
+ t.Fatal(err)
+ }
+ if h.Value(rowB, colA).Num != 16 {
+ t.Fatal("formula did not recalculate")
+ }
+ h.Release()
+ if err = db.Model(&store.Cell{}).Where("tab_id = ? AND row_id = ?", id, rowB).Update("cache_version", 0).Error; err != nil {
+ t.Fatal(err)
+ }
+ h, err = store.Open(db, s, id)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var tab store.Tab
+ if err = db.First(&tab, id).Error; err != nil {
+ t.Fatal(err)
+ }
+ var cell store.Cell
+ if err = db.Where("tab_id = ? AND row_id = ?", id, rowB).First(&cell).Error; err != nil {
+ t.Fatal(err)
+ }
+ if tab.ContentVersion < cell.CacheVersion {
+ t.Fatal("recovery stamp not retained")
+ }
+ if err = h.Set(rowA, colA, "", actor); err != nil {
+ t.Fatal(err)
+ }
+ if h.Value(rowB, colA).Num != 0 {
+ t.Fatal("blank feeder did not recalculate")
+ }
+ last, err := h.SetReport(rowB, colA, "", actor)
+ if err != nil {
+ t.Fatal(err)
+ }
+ h.Release()
+ h, err = store.Open(db, s, id)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer h.Release()
+ next, err := h.SetReport(rowA, colA, "9", actor)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if next.Version <= last.Version {
+ t.Fatal("deleting recovered cells lost watermark")
+ }
+}
diff --git a/internal/sheets/store/handle_version_migration_test.go b/internal/sheets/store/handle_version_migration_test.go
index e33bd8f..9887a6e 100644
--- a/internal/sheets/store/handle_version_migration_test.go
+++ b/internal/sheets/store/handle_version_migration_test.go
@@ -46,10 +46,15 @@ func TestVersionWatermarkMigrationPreservesExistingContent(t *testing.T) {
if err != nil {
t.Fatal(err)
}
+ payload, err := s.Seal("event.payload", []byte(`{"r":100,"c":200,"o":"","n":"5"}`))
+ if err != nil {
+ t.Fatal(err)
+ }
for _, q := range []struct {
sql string
args []any
}{
+ {"INSERT INTO events (id,tab_id,kind,actor_id,payload_sealed) VALUES (?,?,?,?,?)", []any{1, tabID, "input", actor, payload}},
{"INSERT INTO tabs (id,sheet_id,name_sealed,position) VALUES (?,?,?,?)", []any{tabID, 1, name, 0}},
{"INSERT INTO rows (id,tab_id,position) VALUES (?,?,?)", []any{rowA, tabID, 0}},
{"INSERT INTO cols (id,tab_id,position) VALUES (?,?,?)", []any{colA, tabID, 0}},
@@ -62,6 +67,13 @@ func TestVersionWatermarkMigrationPreservesExistingContent(t *testing.T) {
if _, err := migrate.Apply(context.Background(), d, sheets.BootSchema); err != nil {
t.Fatal(err)
}
+ var event store.Event
+ if err := d.G.First(&event, 1).Error; err != nil {
+ t.Fatal(err)
+ }
+ if !bytes.Equal(event.PayloadSealed, payload) || event.ActorID != actor {
+ t.Fatal("migration altered history")
+ }
var mark uint64
if err := d.G.Raw("SELECT content_version FROM tabs WHERE id = ?", tabID).Scan(&mark).Error; err != nil {
t.Fatal(err)
diff --git a/internal/sheets/store/models.go b/internal/sheets/store/models.go
index 0faed88..9bd5195 100644
--- a/internal/sheets/store/models.go
+++ b/internal/sheets/store/models.go
@@ -52,12 +52,13 @@ type Sheet struct {
// Tab is one sheet within a document — the tabs along the bottom.
type Tab struct {
- ID uint64 `gorm:"primaryKey"`
- SheetID uint64 `gorm:"index"`
- NameSealed []byte // seal purpose "tab.name"
- Position int
- FrozenRows int
- FrozenCols int
+ ID uint64 `gorm:"primaryKey"`
+ SheetID uint64 `gorm:"index"`
+ NameSealed []byte // seal purpose "tab.name"
+ Position int
+ FrozenRows int
+ FrozenCols int
+ ContentVersion uint64 `gorm:"not null;default:0"`
}
// Row and Col carry stable identity. Position is a property of the record,