amadan

oficina / sheets Public

Fix round 1: close the blank/delete crash window, dedupe the dependents walk, document SeedCache's contract

Critical: Set's blank path deleted a cell's row in one transaction and
recomputed/persisted its dependents in a separate one. A crash between
them left every dependent serving a value computed against a
now-deleted precedent, forever — the origin row was gone (nothing to
flag as stale) and the dependents' own version fields were untouched
(self-consistent, so Open's per-cell check found nothing wrong).
setBlank now marks every transitive dependent's own InputVersion
durably in the SAME transaction as the delete (its CacheVersion left
behind, exactly the marker shape Set's update path already relies on),
so a crash there leaves each dependent self-flagged and recoverable by
Open's existing per-cell recovery — no new detection logic needed.

Verified with a real crash simulation, not a fabricated end-state: a
test-only seam (afterBlankTransactionOneCommits) interrupts the actual
Set/setBlank code path immediately after transaction 1 commits, before
transaction 2 ever runs. Confirmed TestBlankCrashLeavesDependentsRecoverable
fails against the pre-fix code (dependent left self-consistent, no
marker) before restoring the fix.

Important: exported Graph.TransitiveDependents (the full BFS Recalc
already used internally) instead of the one-hop Dependents getter, and
deleted handle_set.go's hand-written copy of the same walk — the
duplication the earlier export was supposed to avoid happened anyway,
just at the wrong granularity.

Important: documented SeedCache's caller obligation — Graph does no
verification against the installed formula or any version concept
(versioning lives entirely in store); a caller must seed only a value
it can vouch is the cell's true current answer, and treat it as
provisional until its own staleness check has run.

Full detail in task-10-report.md's fix-round addendum.
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 33441d2ccc52bf8fc41390426fec82c2a0ec9705 parent a03d91b
4 files changed, +293 −89
  • internal/sheets/calc/graph.go +29 −26
  • internal/sheets/calc/graph_recalc.go +1 −1
  • internal/sheets/store/handle_blank_crash_test.go +130 −0
  • internal/sheets/store/handle_set.go +133 −62

Intent

full ledger: core

diff --git a/internal/sheets/calc/graph.go b/internal/sheets/calc/graph.go
index 3835a63..c960eb3 100644
--- a/internal/sheets/calc/graph.go
+++ b/internal/sheets/calc/graph.go
@@ -116,12 +116,23 @@ func collectRefs(n Node) []CellKey {
return out
}
-// transitiveDependents returns every cell reachable from origin by
+// TransitiveDependents returns every cell reachable from origin by
// following dependent edges — the set that could change because origin
// did. origin itself is never included by this walk (callers that also
// need to recompute a changed formula cell's own value add it
// separately; see Recalc).
-func (g *Graph) transitiveDependents(origin CellKey) map[CellKey]struct{} {
+//
+// Exported so a caller that needs to know WHICH cells a change affects —
+// Task 10's handle, deciding which cells' persisted cache rows to
+// rewrite after a Set or during Open's crash recovery — can ask Graph
+// directly rather than re-deriving the same edges. Recalc itself
+// deliberately returns only a touched count (it feeds the recalc
+// marker's derived-count field, §4.3 — not a cell list), and this is
+// the one walk this package already gets right, including everything
+// SetFormula/collectRefs capped when the edges were built; a second,
+// hand-written copy of it elsewhere would be exactly the kind of
+// duplicated truth this project's own review history keeps finding.
+func (g *Graph) TransitiveDependents(origin CellKey) map[CellKey]struct{} {
seen := make(map[CellKey]struct{})
queue := []CellKey{origin}
for len(queue) > 0 {
@@ -138,30 +149,6 @@ func (g *Graph) transitiveDependents(origin CellKey) map[CellKey]struct{} {
return seen
}
-// Dependents returns the direct dependents of key — the cells whose
-// installed formula reads key, one hop, not transitive. Order is
-// unspecified.
-//
-// This exists for Task 10's handle: after a Recalc, the handle has to
-// know which cells' persisted cache rows to rewrite, and Recalc itself
-// deliberately returns only a touched count (it feeds the recalc
-// marker's derived-count field, §4.3 — not a cell list). Re-deriving
-// "which cells changed" by re-running collectRefs/rangeCells in the
-// store package would be a second implementation of edge-building this
-// package already gets right, including the maxRangeCells cap — exactly
-// the failure shape this project's own review history keeps finding. A
-// getter over the edges Graph already maintains avoids that: the handle
-// walks Dependents itself (see its own transitiveDependents) using
-// edges this package built and capped once.
-func (g *Graph) Dependents(key CellKey) []CellKey {
- deps := g.dependents[key]
- out := make([]CellKey, 0, len(deps))
- for d := range deps {
- out = append(out, d)
- }
- return out
-}
-
// Clear removes key's formula, its cached value, and its precedent
// edges, exactly as SetFormula's replacement prologue does — but installs
// nothing new. It is what a blanked cell uses: SetFormula only knows how
@@ -202,6 +189,22 @@ func (g *Graph) CachedValue(key CellKey) (Value, bool) {
// a fresh Open — SetFormula always drops any existing cache — turning an
// ordinary restart into a full-tab recompute instead of the targeted
// dirty-forward one crash recovery actually needs.
+//
+// What the caller owes: Graph does no checking of its own here — no
+// verification against key's installed formula (or that one even
+// exists), and no version bookkeeping at all, because versioning is not
+// a concept this package has; it lives entirely in the store package
+// that calls this. It is safe in Task 10's own use because buildHandle
+// only ever seeds a value it just decrypted from that SAME cell's own
+// stored cache, immediately followed by the CacheVersion/InputVersion
+// check that decides whether a real Recalc is still owed. A caller that
+// seeds a value NOT actually derived from key's current formula — or
+// skips that staleness check afterward — gets a Graph that will hand
+// CachedValue's answer back indefinitely, no differently from a value
+// Recalc legitimately computed. Seed only a value you can vouch is
+// key's true current answer as of the moment you call this, and treat
+// it as provisional until whatever staleness check your caller relies
+// on has run.
func (g *Graph) SeedCache(key CellKey, v Value) {
g.cache[key] = v
g.cached[key] = true
diff --git a/internal/sheets/calc/graph_recalc.go b/internal/sheets/calc/graph_recalc.go
index eab4402..3966053 100644
--- a/internal/sheets/calc/graph_recalc.go
+++ b/internal/sheets/calc/graph_recalc.go
@@ -12,7 +12,7 @@ import "time"
// Past MaxDirtyPerPass, Recalc evaluates nothing and returns the count it
// would have touched; see the constant's comment for why.
func (g *Graph) Recalc(origin CellKey, env Env) int {
- dirty := g.transitiveDependents(origin)
+ dirty := g.TransitiveDependents(origin)
if _, hasFormula := g.formulas[origin]; hasFormula {
dirty[origin] = struct{}{}
}
diff --git a/internal/sheets/store/handle_blank_crash_test.go b/internal/sheets/store/handle_blank_crash_test.go
new file mode 100644
index 0000000..dbbe519
--- /dev/null
+++ b/internal/sheets/store/handle_blank_crash_test.go
@@ -0,0 +1,130 @@
+package store
+
+import (
+ "errors"
+ "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"
+)
+
+// crashSentinel is what afterBlankTransactionOneCommits returns while
+// this test is simulating an interrupted setBlank.
+var crashSentinel = errors.New("store: simulated crash for test")
+
+// TestBlankCrashLeavesDependentsRecoverable proves that a process crash
+// landing between setBlank's two transactions — the delete commits, the
+// recompute that would give a dependent its real new value never runs —
+// leaves that dependent recoverable rather than silently wrong forever.
+//
+// This lives in package store (not store_test) because it needs
+// afterBlankTransactionOneCommits, the test-only seam that lets it
+// interrupt REAL Set/setBlank code exactly between the two transactions
+// — the only way to prove this fix's actual code path writes the
+// dependents' dirty marker atomically with the delete, as opposed to a
+// hand-fabricated DB state that Open's pre-existing per-cell recovery
+// would already know how to fix regardless of whether Set ever wrote it.
+func TestBlankCrashLeavesDependentsRecoverable(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, rowA, rowB, colA uint64 = 1, 1, 2, 1
+ if err := db.Create(&sheets.Tab{ID: localTab, SheetID: 1}).Error; err != nil {
+ t.Fatalf("seed tab: %v", err)
+ }
+ if err := db.Create([]*sheets.Row{
+ {ID: rowA, TabID: localTab, Position: 0},
+ {ID: rowB, TabID: localTab, Position: 1},
+ }).Error; err != nil {
+ t.Fatalf("seed rows: %v", err)
+ }
+ if err := db.Create(&sheets.Col{ID: colA, TabID: localTab, Position: 0}).Error; err != nil {
+ t.Fatalf("seed col: %v", err)
+ }
+
+ h, err := Open(db, s, localTab)
+ if err != nil {
+ t.Fatalf("Open: %v", err)
+ }
+ if err := h.Set(rowA, colA, "5", 7); err != nil {
+ t.Fatalf("Set A1: %v", err)
+ }
+ if err := h.Set(rowB, colA, "=A1*2", 7); err != nil {
+ t.Fatalf("Set B1: %v", err)
+ }
+ h.Release()
+
+ h, err = Open(db, s, localTab)
+ if err != nil {
+ t.Fatalf("reopen: %v", err)
+ }
+ if got := h.Value(rowB, colA); got.Kind != calc.KindNum || got.Num != 10 {
+ t.Fatalf("B1 before blanking = %+v, want 10", got)
+ }
+
+ // Interrupt setBlank exactly between its two transactions.
+ old := afterBlankTransactionOneCommits
+ afterBlankTransactionOneCommits = func() error { return crashSentinel }
+ err = h.Set(rowA, colA, "", 7)
+ afterBlankTransactionOneCommits = old
+ if !errors.Is(err, crashSentinel) {
+ t.Fatalf("Set A1 blank (interrupted): got %v, want the simulated crash sentinel", err)
+ }
+ h.Release()
+
+ // Transaction 1 must have really committed: A1's row is gone.
+ var count int64
+ db.Model(&sheets.Cell{}).Where("row_id = ? AND col_id = ?", rowA, colA).Count(&count)
+ if count != 0 {
+ t.Fatal("A1's row survived the interrupted blank; transaction 1 should have deleted it")
+ }
+
+ // B1 must have been left with a durable dirty marker: its own
+ // InputVersion leading its own CacheVersion, written atomically with
+ // the delete in transaction 1, per this fix.
+ var b sheets.Cell
+ db.Where("row_id = ? AND col_id = ?", rowB, colA).First(&b)
+ if b.CacheVersion >= b.InputVersion {
+ t.Fatalf("B1 was not left with a durable dirty marker: CacheVersion %d, InputVersion %d (want CacheVersion < InputVersion)", b.CacheVersion, b.InputVersion)
+ }
+
+ // The tab must be recoverable on the very next Open, exactly as
+ // TestStaleCacheIsRecomputedOnOpen requires for any dirty marker.
+ h2, err := Open(db, s, localTab)
+ if err != nil {
+ t.Fatalf("Open after interrupted blank: %v", err)
+ }
+ defer h2.Release()
+
+ if got := h2.Value(rowB, colA); got.Kind != calc.KindNum || got.Num != 0 {
+ t.Fatalf("B1 after recovery = %+v, want KindNum 0 (A1 is gone; blank reads as 0 in arithmetic)", got)
+ }
+ var b2 sheets.Cell
+ db.Where("row_id = ? AND col_id = ?", rowB, colA).First(&b2)
+ if b2.CacheVersion < b2.InputVersion {
+ t.Fatalf("B1 still stale after Open's recovery: CacheVersion %d < InputVersion %d", b2.CacheVersion, b2.InputVersion)
+ }
+}
diff --git a/internal/sheets/store/handle_set.go b/internal/sheets/store/handle_set.go
index 010fdf5..6f4316d 100644
--- a/internal/sheets/store/handle_set.go
+++ b/internal/sheets/store/handle_set.go
@@ -9,18 +9,32 @@ import (
"amadan.net/oficina/sheets/internal/sheets/calc"
)
+// afterBlankTransactionOneCommits is a test-only seam: called
+// immediately after setBlank's first transaction (the delete, the
+// dependents' dirty-marker bump, and the input event) commits, before
+// anything else runs. It always returns nil in production. A test
+// overrides it to return a non-nil error, simulating a process crash
+// landing exactly between setBlank's two transactions — see
+// TestBlankCrashLeavesDependentsRecoverable.
+var afterBlankTransactionOneCommits = func() error { return nil }
+
// Set writes a cell's new raw input and recalculates forward from it.
//
-// The input write, plus the version bump that IS the dirty marker
-// (models.go: CacheVersion records the InputVersion its cache was
-// derived from — bumping InputVersion here without yet touching
-// CacheVersion is exactly what makes this cell look dirty), lands in one
-// transaction. Recalculation — and persisting what it computes — happens
-// afterward, in a second transaction. A crash between the two leaves
-// precisely the state Open's crash-recovery pass exists to detect and
-// fix: no separate marker column is needed because the version gap IS
-// the marker, and clearing it is writing a CacheVersion that has caught
-// back up.
+// For an edit to a surviving cell, the input write — plus the version
+// bump that IS the dirty marker (models.go: CacheVersion records the
+// InputVersion its cache was derived from — bumping InputVersion here
+// without yet touching CacheVersion is exactly what makes this cell look
+// dirty) — lands in one transaction. Recalculation, and persisting what
+// it computes, happens afterward in a second transaction. A crash
+// between the two leaves precisely the state Open's crash-recovery pass
+// exists to detect and fix: no separate marker column is needed because
+// the version gap IS the marker, and clearing it is writing a
+// CacheVersion that has caught back up.
+//
+// Blanking a cell cannot use that mechanism on the cell itself — its row
+// is deleted, so there is no surviving row to carry a marker — so it
+// marks its DEPENDENTS dirty instead, atomically with the delete. See
+// the blank branch below and TestBlankCrashLeavesDependentsRecoverable.
func (h *Handle) Set(rowID, colID uint64, raw string, actor uint64) error {
h.mu.Lock()
defer h.mu.Unlock()
@@ -39,28 +53,77 @@ func (h *Handle) Set(rowID, colID uint64, raw string, actor uint64) error {
prevRaw := h.rawText[key]
if blank {
- if err := h.db.Transaction(func(tx *gorm.DB) error {
- if err := tx.Where("tab_id = ? AND row_id = ? AND col_id = ?", h.tabID, rowID, colID).
- Delete(&sheets.Cell{}).Error; err != nil {
+ return h.setBlank(rowID, colID, key, v, actor, prevRaw)
+ }
+ return h.setContent(rowID, colID, key, node, raw, v, actor, prevRaw)
+}
+
+// setBlank deletes a cell's row (cells are sparse — models.go) and
+// propagates the change to its dependents.
+//
+// A crash between "the row is gone" and "dependents have been
+// recomputed" is exactly the scenario this whole task exists to close —
+// reached here via delete instead of update. The origin row cannot carry
+// a dirty marker once it no longer exists, so this durably marks every
+// DEPENDENT dirty (bumping its own InputVersion to v, its CacheVersion
+// untouched) in the SAME transaction as the delete. If the process never
+// reaches the recompute below, each dependent's own InputVersion now
+// leads its own CacheVersion, and Open's ordinary per-cell recovery —
+// already proven by TestStaleCacheIsRecomputedOnOpen — finds and fixes
+// it without needing to know anything about deletes specifically.
+func (h *Handle) setBlank(rowID, colID uint64, key calc.CellKey, v uint64, actor uint64, prevRaw string) error {
+ // TransitiveDependents reads g.dependents[key], which Clear (called
+ // below, after this transaction commits) never touches — only key's
+ // own precedent edges are rewired by Clear — so this is safe to
+ // compute before the delete.
+ touched := h.graph.TransitiveDependents(key)
+ withinCap := len(touched) > 0 && len(touched) <= calc.MaxDirtyPerPass
+
+ if err := h.db.Transaction(func(tx *gorm.DB) error {
+ if err := tx.Where("tab_id = ? AND row_id = ? AND col_id = ?", h.tabID, rowID, colID).
+ Delete(&sheets.Cell{}).Error; err != nil {
+ return err
+ }
+ if withinCap {
+ if err := h.markDirty(tx, touched, v); err != nil {
return err
}
- return AppendInput(tx, h.sealer, InputEvent{
- TabID: h.tabID, RowID: rowID, ColID: colID, ActorID: actor,
- OldRaw: prevRaw, NewRaw: raw,
- })
- }); err != nil {
- return err
}
- h.nextVersion++
- delete(h.rawText, key)
- delete(h.keyToIDs, key)
- h.graph.Clear(key)
- // Blanked: no row survives to carry a cache version, so origin
- // itself is excluded from the persist pass below — only its
- // dependents (still referencing an now-empty cell) need one.
- return h.recalcAndPersist(key, v, false)
+ return AppendInput(tx, h.sealer, InputEvent{
+ TabID: h.tabID, RowID: rowID, ColID: colID, ActorID: actor,
+ OldRaw: prevRaw, NewRaw: "",
+ })
+ }); err != nil {
+ return err
+ }
+ // Test seam: a crash landing exactly here — transaction 1 durably
+ // committed, nothing after it has run yet — is what
+ // TestBlankCrashLeavesDependentsRecoverable simulates. Always nil in
+ // production.
+ if err := afterBlankTransactionOneCommits(); err != nil {
+ return err
+ }
+ h.nextVersion++
+ delete(h.rawText, key)
+ delete(h.keyToIDs, key)
+ h.graph.Clear(key)
+
+ 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.
+ return nil
}
+ // Transaction 2: recompute the real values and persist them,
+ // clearing the marker transaction 1 just wrote.
+ h.graph.Recalc(key, newHandleEnv())
+ return h.persistTouched(touched, v)
+}
+
+// setContent writes a formula or literal cell's new input and
+// recalculates forward from it.
+func (h *Handle) setContent(rowID, colID uint64, key calc.CellKey, node calc.Node, raw string, v uint64, actor uint64, prevRaw string) error {
sealedInput, err := h.sealer.Seal("cell.input", []byte(raw))
if err != nil {
return err
@@ -100,12 +163,33 @@ func (h *Handle) Set(rowID, colID uint64, raw string, actor uint64) error {
return h.recalcAndPersist(key, v, true)
}
+// markDirty bumps each cell in touched's own InputVersion to v within
+// tx, leaving its CacheVersion untouched — the same "input advances
+// without yet touching cache" shape setContent uses on the one cell it
+// directly edits, applied here to every cell whose precedent is about to
+// disappear rather than to the cell whose content actually changed.
+func (h *Handle) markDirty(tx *gorm.DB, touched map[calc.CellKey]struct{}, v uint64) error {
+ for key := range touched {
+ ids, ok := h.keyToIDs[key]
+ if !ok {
+ continue // defensive: every touched key was registered via SetFormula
+ }
+ if err := tx.Model(&sheets.Cell{}).
+ Where("tab_id = ? AND row_id = ? AND col_id = ?", h.tabID, ids.RowID, ids.ColID).
+ Update("input_version", v).Error; err != nil {
+ return err
+ }
+ }
+ return nil
+}
+
// recalcAndPersist propagates a change at origin to its transitive
-// dependents, evaluates every touched cell exactly once (via Graph.Recalc),
-// and writes each one's new CachedSealed/CacheVersion in a single
-// transaction — the step that closes the dirty marker Set (or Open's
-// recovery pass) opened. includeOrigin is false only for a cell that was
-// just blanked: its row no longer exists to receive a cache write.
+// dependents, evaluates every touched cell exactly once (via
+// Graph.Recalc), and persists each one's new CachedSealed/CacheVersion —
+// the step that closes the dirty marker setContent (or Open's recovery
+// pass) opened. includeOrigin is true whenever origin's own row survives
+// to receive a cache write (every caller except setBlank, which persists
+// dependents directly once its own recompute step is reached).
//
// Past calc.MaxDirtyPerPass, this deliberately does nothing: the touched
// set is computed BEFORE Recalc is called specifically so this can be
@@ -113,19 +197,31 @@ func (h *Handle) Set(rowID, colID uint64, raw string, actor uint64) error {
// Recalc's own cap — see calc.MaxDirtyPerPass's comment for why an
// over-wide pass is refused rather than run.
func (h *Handle) recalcAndPersist(origin calc.CellKey, v uint64, includeOrigin bool) error {
- touched := h.transitiveDependents(origin)
+ touched := h.graph.TransitiveDependents(origin)
if includeOrigin {
touched[origin] = struct{}{}
}
- if len(touched) == 0 {
- return nil
- }
- if len(touched) > calc.MaxDirtyPerPass {
+ if len(touched) == 0 || len(touched) > calc.MaxDirtyPerPass {
return nil
}
h.graph.Recalc(origin, newHandleEnv())
+ return h.persistTouched(touched, v)
+}
+// persistTouched writes each cell in touched's current graph-cached
+// value — sealed as CachedSealed, stamped CacheVersion=v — in one
+// transaction, plus a recalc marker recording how wide the pass was.
+// Callers are expected to have already run Graph.Recalc (or otherwise
+// ensured every touched key has a current cached value) before calling
+// this; a key with nothing cached persists as KindEmpty rather than
+// erroring, matching decodeValue's own empty-input convention.
+//
+// triggerID 0 in the AppendRecalc call below: AppendInput does not hand
+// back the id of the event row it just created, and correlating one
+// would cost a second query for bookkeeping only — the marker's real
+// job, the derived-count, is unaffected. See events.go.
+func (h *Handle) persistTouched(touched map[calc.CellKey]struct{}, v uint64) error {
return h.db.Transaction(func(tx *gorm.DB) error {
for key := range touched {
ids, ok := h.keyToIDs[key]
@@ -146,31 +242,6 @@ func (h *Handle) recalcAndPersist(origin calc.CellKey, v uint64, includeOrigin b
return err
}
}
- // triggerID 0: AppendInput does not hand back the id of the event
- // row it just created, and correlating one would cost a second
- // query for bookkeeping only — the marker's real job, the
- // derived-count, is unaffected. See events.go.
return AppendRecalc(tx, 0, len(touched))
})
}
-
-// transitiveDependents mirrors calc.Graph's own private BFS of the same
-// name, walking Graph.Dependents instead of an unexported edge map — see
-// that method's doc for why the handle asks Graph for edges rather than
-// rebuilding them.
-func (h *Handle) transitiveDependents(origin calc.CellKey) map[calc.CellKey]struct{} {
- seen := make(map[calc.CellKey]struct{})
- queue := []calc.CellKey{origin}
- for len(queue) > 0 {
- cur := queue[0]
- queue = queue[1:]
- for _, dep := range h.graph.Dependents(cur) {
- if _, ok := seen[dep]; ok {
- continue
- }
- seen[dep] = struct{}{}
- queue = append(queue, dep)
- }
- }
- return seen
-}