amadan

oficina / sheets Public

Mark blank-path dependents dirty even past MaxDirtyPerPass

setBlank (handle_set.go) skipped markDirty entirely once
len(touched) > calc.MaxDirtyPerPass, leaving over-cap dependents with no
marker at all. On the update path that's survivable — the origin cell
keeps its own version gap for Open's recovery to notice. On the blank
path the origin row is deleted, so nothing was left to notice: every
dependent's CacheVersion stayed equal to its own untouched InputVersion,
which Open's crash-recovery loop reads as "already consistent" and never
revisits. A whole-column formula's feeder cell being blanked left every
dependent silently, permanently stale — the failure class the crash
recovery mechanism exists to close, surviving past the cap.

Fix: markDirty now runs whenever touched is non-empty, unconditional on
the cap. withinCap still gates only the second transaction (the
synchronous recompute + persist), matching the finding's framing —
marking is a cheap version bump, the cap exists to bound the recompute.
Measured: marking 5001 dependents took ~103ms in the new test, confirming
that assumption; no fallback was needed.

Test (TDD: written first, watched fail — reopened dependents kept
serving the stale pre-blank value of 10 instead of recovering to 0):
TestBlankFeederOverCapDependentsAreRecoverable seeds calc.MaxDirtyPerPass+1
already-consistent dependents of a feeder cell, blanks the feeder, and
proves each one recovers its correct value on the next Open.

Concern for the record (not fixed here, out of this finding's scope):
Open's crash-recovery loop (handle_open.go) recomputes each stale key in
its own transaction, so recovering 5001 newly-marked dependents took
~25s in this test — a pre-existing inefficiency in that loop, now newly
reachable at this scale via an ordinary blank rather than only via an
actual multi-thousand-cell crash.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
Paul Campbell pushed by paul@keymail.dev 3a3febcfbff7e3acf24a276b574a8a08f3d648a6 parent b996bb2
2 files changed, +177 −2
  • internal/sheets/store/handle_blank_cap_test.go +161 −0
  • internal/sheets/store/handle_set.go +16 −2

Intent

full ledger: core

diff --git a/internal/sheets/store/handle_blank_cap_test.go b/internal/sheets/store/handle_blank_cap_test.go
new file mode 100644
index 0000000..bec5998
--- /dev/null
+++ b/internal/sheets/store/handle_blank_cap_test.go
@@ -0,0 +1,161 @@
+package store
+
+import (
+ "path/filepath"
+ "testing"
+
+ rdb "github.com/carlosframework/rastrillo/db"
+
+ "amadan.net/oficina/sheets/internal/sheets"
+ "amadan.net/oficina/sheets/internal/sheets/calc"
+ "amadan.net/oficina/sheets/internal/sheets/seal"
+)
+
+// TestBlankFeederOverCapDependentsAreRecoverable proves that blanking a
+// feeder cell with MORE dependents than calc.MaxDirtyPerPass still leaves
+// every dependent recoverable on the next Open, rather than silently and
+// permanently stale.
+//
+// Before this fix, setBlank (handle_set.go) skipped markDirty entirely
+// once len(touched) > calc.MaxDirtyPerPass. On the UPDATE path that is
+// survivable because the origin cell keeps its own version gap for
+// Open's recovery pass to notice. On the BLANK path the origin row is
+// deleted, so nothing is left to notice: every dependent's CacheVersion
+// stays equal to its own (untouched) InputVersion, which Open's
+// crash-recovery loop (handle_open.go) reads as "already consistent" and
+// never revisits. Each dependent then keeps serving its old cached value
+// forever, even though the cell it read no longer exists — the exact
+// failure class the crash-recovery mechanism exists to close, surviving
+// past the cap.
+//
+// This lives in package store (not store_test) so it can build its
+// fixture with encodeValue directly — seeding calc.MaxDirtyPerPass+1
+// already-consistent dependent cells through real Set calls would make
+// this test minutes slower for no extra coverage, the same tradeoff
+// handle_test.go's seedOversizedTab makes for TestCellCapIsAnHonestError.
+func TestBlankFeederOverCapDependentsAreRecoverable(t *testing.T) {
+ dir := t.TempDir()
+ d, err := rdb.Open(filepath.Join(dir, "test.db"), nil)
+ if err != nil {
+ t.Fatalf("open db: %v", err)
+ }
+ t.Cleanup(func() {
+ if err := d.Close(); err != nil {
+ t.Errorf("close db: %v", err)
+ }
+ })
+ db := d.G
+ if err := db.AutoMigrate(
+ &sheets.Sheet{}, &sheets.Tab{}, &sheets.Row{}, &sheets.Col{},
+ &sheets.Cell{}, &sheets.Format{}, &Event{},
+ ); err != nil {
+ t.Fatalf("migrate: %v", err)
+ }
+ s, err := seal.New(make([]byte, 32))
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ const localTab, feederRow, colA uint64 = 1, 1, 1
+ const n = calc.MaxDirtyPerPass + 1 // strictly over the cap
+
+ if err := db.Create(&sheets.Tab{ID: localTab, SheetID: 1}).Error; err != nil {
+ t.Fatalf("seed tab: %v", err)
+ }
+ if err := db.Create(&sheets.Col{ID: colA, TabID: localTab, Position: 0}).Error; err != nil {
+ t.Fatalf("seed col: %v", err)
+ }
+ if err := db.Create(&sheets.Row{ID: feederRow, TabID: localTab, Position: 0}).Error; err != nil {
+ t.Fatalf("seed feeder row: %v", err)
+ }
+
+ feederInputSealed, err := s.Seal("cell.input", []byte("5"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ feederCachedSealed, err := s.Seal("cell.cached", encodeValue(calc.Value{Kind: calc.KindNum, Num: 5}))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := db.Create(&sheets.Cell{
+ TabID: localTab, RowID: feederRow, ColID: colA,
+ InputSealed: feederInputSealed, InputVersion: 1,
+ CachedSealed: feederCachedSealed, CacheVersion: 1,
+ }).Error; err != nil {
+ t.Fatalf("seed feeder cell: %v", err)
+ }
+
+ // n dependent rows, positions 1..n, each already holding "=A1*2" with
+ // an ALREADY-CONSISTENT cache (CacheVersion == InputVersion, cached
+ // value 10 — the real value of A1*2 with A1 = 5). No AAD binds a
+ // seal to a specific cell (seal.Sealer.Seal takes no per-call
+ // context beyond the purpose string), so the same two sealed blobs
+ // are reused for every dependent rather than sealing n times.
+ depInputSealed, err := s.Seal("cell.input", []byte("=A1*2"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ depCachedSealed, err := s.Seal("cell.cached", encodeValue(calc.Value{Kind: calc.KindNum, Num: 10}))
+ if err != nil {
+ t.Fatal(err)
+ }
+ rows := make([]*sheets.Row, 0, n)
+ cells := make([]*sheets.Cell, 0, n)
+ for i := 1; i <= n; i++ {
+ rid := feederRow + uint64(i)
+ rows = append(rows, &sheets.Row{ID: rid, TabID: localTab, Position: i})
+ cells = append(cells, &sheets.Cell{
+ TabID: localTab, RowID: rid, ColID: colA,
+ InputSealed: depInputSealed, InputVersion: 1,
+ CachedSealed: depCachedSealed, CacheVersion: 1,
+ })
+ }
+ const batch = 500
+ if err := db.CreateInBatches(&rows, batch).Error; err != nil {
+ t.Fatalf("seed dependent rows: %v", err)
+ }
+ if err := db.CreateInBatches(&cells, batch).Error; err != nil {
+ t.Fatalf("seed dependent cells: %v", err)
+ }
+
+ h, err := Open(db, s, localTab)
+ if err != nil {
+ t.Fatalf("Open: %v", err)
+ }
+
+ // Sanity: the fixture really is over the cap and dependents really
+ // do read 10 to start, so the assertions below are exercising the
+ // over-cap blank path, not something Open's own startup recompute
+ // already fixed for an unrelated reason.
+ sample := []uint64{feederRow + 1, feederRow + n/2, feederRow + n}
+ for _, rid := range sample {
+ if got := h.Value(rid, colA); got.Kind != calc.KindNum || got.Num != 10 {
+ t.Fatalf("dependent row %d before blank = %+v, want KindNum 10", rid, got)
+ }
+ }
+ if touched := h.graph.TransitiveDependents(calc.CellKey{RowID: 1, ColID: 1}); len(touched) <= calc.MaxDirtyPerPass {
+ t.Fatalf("test setup: only %d dependents wired, want more than MaxDirtyPerPass (%d)", len(touched), calc.MaxDirtyPerPass)
+ }
+
+ if err := h.Set(feederRow, colA, "", 7); err != nil {
+ t.Fatalf("blank feeder: %v", err)
+ }
+ h.Release()
+
+ // The real proof: reopen and read the dependents' actual VALUE, not
+ // just a version stamp. A1 is gone, so the correct answer is now 0
+ // (blank reads as 0 in arithmetic). Before this fix they silently
+ // keep serving 10 forever, because nothing ever marked them dirty
+ // and Open's recovery pass has nothing to notice.
+ h2, err := Open(db, s, localTab)
+ if err != nil {
+ t.Fatalf("reopen: %v", err)
+ }
+ defer h2.Release()
+
+ for _, rid := range sample {
+ if got := h2.Value(rid, colA); got.Kind != calc.KindNum || got.Num != 0 {
+ t.Fatalf("dependent row %d after blank+reopen = %+v, want KindNum 0 (recovered, A1 gone)", rid, got)
+ }
+ }
+}
diff --git a/internal/sheets/store/handle_set.go b/internal/sheets/store/handle_set.go
index 6f4316d..218ef36 100644
--- a/internal/sheets/store/handle_set.go
+++ b/internal/sheets/store/handle_set.go
@@ -84,7 +84,17 @@ func (h *Handle) setBlank(rowID, colID uint64, key calc.CellKey, v uint64, actor
Delete(&sheets.Cell{}).Error; err != nil {
return err
}
- if withinCap {
+ // Marking is unconditional on len(touched) > 0 — NOT gated by
+ // withinCap. MaxDirtyPerPass bounds the RECOMPUTE below (a full
+ // Eval of every touched cell), not this marker (one version-bump
+ // UPDATE per touched cell). Past the cap this cell's own row is
+ // gone, so a durable marker on every dependent is the only
+ // recovery signal that will ever exist for them; skipping it
+ // here — as this code once did — left them looking permanently
+ // consistent (CacheVersion == InputVersion, both untouched) while
+ // silently reading a cell that no longer exists. See
+ // TestBlankFeederOverCapDependentsAreRecoverable.
+ if len(touched) > 0 {
if err := h.markDirty(tx, touched, v); err != nil {
return err
}
@@ -111,7 +121,11 @@ func (h *Handle) setBlank(rowID, colID uint64, key calc.CellKey, v uint64, actor
if !withinCap {
// Either no dependents (nothing to do) or too many to recompute
// and persist synchronously — matching recalcAndPersist's own
- // refusal past calc.MaxDirtyPerPass, not a separate cap.
+ // refusal past calc.MaxDirtyPerPass, not a separate cap. Every
+ // dependent was still marked dirty above regardless of this
+ // cap, so this is not silent: each one's own version gap is
+ // there for Open's ordinary per-cell recovery to find and fix,
+ // the same as any other stale cell.
return nil
}