amadan

oficina / sheets Public

fix: bound batch reports and verify recovery and large-tab costs

Paul Campbell pushed by paul@keymail.dev 37c8055cd36812d19e9f98a0be14bf3106241565 parent 50914fb
3 files changed, +102 −1
  • docs/superpowers/specs/2026-09-07-atomic-cell-batch.md +9 −1
  • internal/sheets/store/handle_write_batch.go +4 −0
  • internal/sheets/store/handle_write_batch_test.go +89 −0

CI passed — run details

Intent

Humanclaude~paul@keymail.dev

Independent Claude Fable spec and branch review, READ ONLY. Review revision 2 spec docs/superpowers/specs/2026-09-07-atomic-cell-batch.md plus git diff origin/main (tracked working tree changes included) against AGENTS.md. Prior REQUEST CHANGES required transaction-scoped recovery, large tab measurements, immediate postcommit swap, merged publication cap, stale recovery regression. Author addressed in revision 2 with tests and measured 660ms/2.326s for 50k/200k tabs; 500-cell input cap stays. Capped batches reject atomically, explicit bounded first implementation. Review code correctness, authorization, rollback, JS queue integration and any serious untested case. APPROVE or REQUEST CHANGES, concrete file/line findings. Canary review is a separate gate still pending root provisioning; do not claim deployment tested. Do not edit files or run mutation/deploy commands. Read tools and readonly git allowed.

full ledger: feat/atomic-cell-batch

diff --git a/docs/superpowers/specs/2026-09-07-atomic-cell-batch.md b/docs/superpowers/specs/2026-09-07-atomic-cell-batch.md
index 8fb268f..bc4614d 100644
--- a/docs/superpowers/specs/2026-09-07-atomic-cell-batch.md
+++ b/docs/superpowers/specs/2026-09-07-atomic-cell-batch.md
@@ -1,6 +1,6 @@
# Atomic cell batch implementation contract
-Status: proposed; Fable review pending. Implements the overnight suite readiness request, under the existing Sheets design and next-three batch-route item.
+Status: revision 2; Fable re-review pending. Revision 1 reviewed by Claude Fable (claude-fable-5-1): REQUEST CHANGES; answers below. Implements the overnight suite readiness request, under the existing Sheets design and next-three batch-route item.
- POST /api/sheets/{sheetID}/tabs/{tabID}/cells/batch accepts {cells: [{r,c,raw,fromR?,fromC?}], client?}. Require an editor using openTab, preserving sheet/tab pairing and 404 isolation.
- Limit to 500 distinct destinations and 1 MiB body. Validate every coordinate, copy origin, raw length, translated raw length, and formula before any committed writes. Duplicate destinations, empty batches, and trailing JSON are invalid. Reuse existing fixed response strings; add no UI copy.
@@ -10,3 +10,11 @@ Status: proposed; Fable review pending. Implements the overnight suite readiness
- No format, row/column semantics, tab lifecycle, formula grammar, or permission changes. Clipboard provenance is a separate next slice; this changes only transmission, leaving clipboard.js untouched.
- Cost: a second tab graph is built per batch. Measure a 500-cell batch and document this bounded first implementation. Avoid adding a graph clone API or redesigning calculation before evidence demands it.
- Tests: dependent formulas and clears, overlapping formulas, encryption/history, invalid late formula, injected persistence failure after earlier writes, axis rollback, shared live handle unchanged on failure, viewer/stranger/cross-sheet isolation, request bounds, one publication, client queue one-request and stop-on-failure behavior. Run make test and exact-head CI. Build with make release, deploy isolated canary, run browser checks, obtain Fable branch review, then coordinate squash merge and production deployment with root.
+
+## Revision 2 review answers
+
+1. The independent handle is built with `buildHandle(tx, ...)`; its `db` is exactly `tx`. Recovery, count, axis allocation, input/event writes, and nested transactions all use that handle. No Open/registry path runs inside the transaction. TestCellBatchRecoversStaleCacheInsideTransaction deliberately seeds a stale cell after opening the live handle, then requires recovery and batch completion within three seconds. Building from the outer DB deadlocks that test.
+2. Measured with CELL_BATCH_BENCH_N and the committed TestCellBatchLargeTabTiming fixture: 500 changed cells on a 50,000-cell tab took 660ms; on a 200,000-cell tab took 2.326s on this development machine. These are transaction duration including rebuild, and therefore instance-wide writer blocking durations. The fixture has one feeder and many dependent formulas; paste changes leaf cells. Production measurements remain required. This initial bounded implementation accepts this cost to preserve atomic rollback without inventing graph cloning. The trigger for moving the rebuild out of the writer transaction is measured unacceptable production/pilot latency; it must preserve recovery and handle consistency. Each SetReport uses nested savepoints, which GORM retains until outer commit (roughly 1,000 per 500-cell batch).
+3. The first non-error operation after outer Transaction returns is swapping graph and mutable maps into the live handle under its mutex. Report assembly occurs inside the transaction before commit; response formatting happens after the live state is updated. There is no fallible operation between commit and the swap. Failure to encode or deliver the HTTP response does not invalidate the now-current live handle; reconnect/snapshot recovers delivery.
+4. A merged report may contain at most calc.MaxDirtyPerPass (5,000) distinct cells. Exceeding either an individual recalc cap or the merged report cap rejects and rolls back the whole batch. This deliberately differs from the single-cell path: the batch success contract guarantees complete final values without depending on the pre-existing resync behavior. No new user copy is introduced; the existing write failure string is reused. Supporting larger dependency fanouts is a later slice with dependable resync and explicit recovery semantics. TestCellBatchRejectsMergedReportPastCalculationCap proves the aggregate limit is applied even when each individual edit is within its cap.
+5. Both the implementation and failure-injection tests exist. The live handle is preserved on rollback and updated immediately on success. Local make test and remote CI were green at initial implementation 50914fb; revision 2 requires fresh checks and review.
diff --git a/internal/sheets/store/handle_write_batch.go b/internal/sheets/store/handle_write_batch.go
index a1f0fa8..f3a2378 100644
--- a/internal/sheets/store/handle_write_batch.go
+++ b/internal/sheets/store/handle_write_batch.go
@@ -1,6 +1,7 @@
package store
import (
+ "amadan.net/oficina/sheets/internal/sheets/calc"
"errors"
"gorm.io/gorm"
@@ -67,6 +68,9 @@ func (h *Handle) SetBatch(writes []CellWrite, actor uint64) (Report, error) {
for _, cell := range rep.Cells {
changed[CellCoord{cell.RowID, cell.ColID}] = cell
}
+ if len(changed) > calc.MaxDirtyPerPass {
+ return errCellBatch
+ }
}
var count int64
if err := tx.Model(&Cell{}).Where("tab_id = ?", h.tabID).Count(&count).Error; err != nil {
diff --git a/internal/sheets/store/handle_write_batch_test.go b/internal/sheets/store/handle_write_batch_test.go
index 4e64012..59fec8f 100644
--- a/internal/sheets/store/handle_write_batch_test.go
+++ b/internal/sheets/store/handle_write_batch_test.go
@@ -4,7 +4,9 @@ import (
"amadan.net/oficina/sheets/internal/sheets/seal"
"amadan.net/oficina/sheets/internal/sheets/sealtest"
"errors"
+ "os"
"path/filepath"
+ "strconv"
"testing"
"time"
@@ -160,3 +162,90 @@ func TestCellBatchKeepsCellsAndHistorySealed(t *testing.T) {
}
sealtest.AssertNoPlaintext(t, path)
}
+
+func TestCellBatchRecoversStaleCacheInsideTransaction(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 := h.Set(rowB, colA, "=A1*2", actor); err != nil {
+ t.Fatal(err)
+ }
+ sealed, err := s.Seal("cell.input", []byte("9"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := db.Model(&store.Cell{}).Where("tab_id = ? AND row_id = ? AND col_id = ?", id, rowA, colA).Updates(map[string]any{"input_sealed": sealed, "input_version": h.Version() + 1}).Error; err != nil {
+ t.Fatal(err)
+ }
+ done := make(chan error, 1)
+ go func() { _, err := h.SetBatch([]store.CellWrite{{Row: 0, Col: 1, Raw: "=A2"}}, actor); done <- err }()
+ select {
+ case err := <-done:
+ if err != nil {
+ t.Fatal(err)
+ }
+ case <-time.After(3 * time.Second):
+ panic("batch recovery deadlocked")
+ }
+ if h.Value(rowB, colA).Num != 18 {
+ t.Fatal("stale dependent not recovered")
+ }
+}
+
+func TestCellBatchLargeTabTiming(t *testing.T) {
+ raw := os.Getenv("CELL_BATCH_BENCH_N")
+ if raw == "" {
+ t.Skip("set CELL_BATCH_BENCH_N to measure full-tab rebuild")
+ }
+ n, err := strconv.Atoi(raw)
+ if err != nil || n < 501 || n > store.MaxCellsPerTab {
+ t.Fatal("invalid CELL_BATCH_BENCH_N")
+ }
+ db := openTestDB(t, filepath.Join(t.TempDir(), "test.db"))
+ s, err := seal.New(make32())
+ if err != nil {
+ t.Fatal(err)
+ }
+ id := tabID
+ seedBlankRecoveryFixture(t, db, s, id, rowA, colA, n-1)
+ h, err := store.Open(db, s, id)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer h.Release()
+ writes := make([]store.CellWrite, 500)
+ for i := range writes {
+ writes[i] = store.CellWrite{Row: i + 1, Col: 0, Raw: "8"}
+ }
+ start := time.Now()
+ _, err = h.SetBatch(writes, actor)
+ elapsed := time.Since(start)
+ if err != nil {
+ t.Fatal(err)
+ }
+ t.Logf("%d-cell tab, 500 writes with transaction rebuild: %s", n, elapsed)
+}
+
+func TestCellBatchRejectsMergedReportPastCalculationCap(t *testing.T) {
+ db := openTestDB(t, filepath.Join(t.TempDir(), "test.db"))
+ s, err := seal.New(make32())
+ if err != nil {
+ t.Fatal(err)
+ }
+ seedBlankRecoveryFixture(t, db, s, tabID, rowA, colA, 4999)
+ h, err := store.Open(db, s, tabID)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer h.Release()
+ _, err = h.SetBatch([]store.CellWrite{{Row: 0, Col: 0, Raw: "7"}, {Row: 0, Col: 1, Raw: "8"}}, actor)
+ if err == nil {
+ t.Fatal("oversized merged report accepted")
+ }
+ if h.Value(rowA, colA).Num != 5 {
+ t.Fatal("oversized report committed partial write")
+ }
+}