amadan
app,core: classify update errors correctly, close the seq race
Fix round 1 on Task 5's review. Two LOW findings, both reachable once Task 6's editor is posting real traffic: 1. updatesPost collapsed every Store.ApplyUpdate error into 400/Info, so a genuine failure on our own end (a DB error, a replay failure, a failed instance rebuild) was reported to the client as "your bytes were bad." internal/core gains RejectedError/Rejected, mirroring the existing RecoveredError/Recovered, so a clean core rejection is positively identifiable rather than merely "not a RecoveredError." store.go wraps only the new-update Apply call in an unexported *updateRejection so a replay failure (which calls the same core.Handle.Apply) can never be mistaken for one. Everything else is now a 500 logged at Error with the document id. Covered by TestInternalApplyFailureReturns500NotBadUpdate, which induces a real failure (a corrupted historical row fed through the real wasm core) rather than a mock. 2. ApplyUpdate's MAX(seq)->INSERT was two separate statements: two concurrent POSTs to one document could compute the same next seq and both write it. Fixed by moving both statements inside one gorm.DB.Transaction — rastrillo/db's write pool caps at a single SQLite connection, so a second concurrent call cannot even begin its transaction until the first commits and releases it, which closes the race without a bounded retry's failure mode under load (verified: an earlier bounded-retry version of this fix was genuinely exhausted by the 21-way concurrent test below on a real run). Migration 0003 adds UNIQUE(doc_id, seq) as a backstop. TestConcurrentUpdatesDoNotCollideOnSeq drives real concurrent goroutines through the router and confirmed it failed against the prior two-statement code (0/3 clean runs) before this fix (15/15 clean runs after).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA
5 files changed,
+349
−34
internal/core/core.go+41 −7internal/docs/handlers.go+43 −6internal/docs/migrations/0003_doc_updates_seq_unique.sql+27 −0internal/docs/store.go+74 −21internal/docs/updates_test.go+164 −0
Intent
Human
<task-notification> <task-id>af1d2dca782186aa3</task-id> <tool-use-id>toolu_011S3LDwEqViCiHv5r2Ghbmd</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-carlos-docs/70f302d1-9d5d-4eb2-8b38-e09a15fdd1c3/tasks/af1d2dca782186aa3.output</output-file> <status>completed</status> <summary>Agent "Editing Task 5 update endpoints" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>## Summary Status: done. Commit `d1d0884` on branch `editing`. `.amadan/ci.d/10-gate` exits 0. Implemented `GET /d/{sqid}/updates` (streams the concatenated update log, owner-scoped, capped at `maxLogBytes = 8<<20`, fails loudly with a logged sqid on overflow rather than truncating) and `POST /d/{sqid}/updates` (wraps the body in `http.MaxBytesReader` at `maxUpdateBytes = 1<<20` before reading, applies via `Store.ApplyUpdate`, maps any core rejection to 400 without persisting). Both use `Store.Get`'s existing 404-not-403 owner scoping. CSRF origin-checking is not exempted for the route. **Owner-scoping verification**: ran the four tests before routes existed — the scoping test passed for the wrong reason (unrouted 404), the other three failed correctly. Wired the routes, then temporarily patched `getDocumentForUpdates` to bypass ownership (raw sqid-only lookup); reran the scoping test and it failed for real (`200`/`204` instead of `404`). Reverted to the real `Store.Get(ctx, owner, sqid)` implementation (diffed byte-identical against my backup before committing) and it passed. **Parked Task 1 findings**, fixed in `core/src/fragment.rs`'s `node_text`: (a) a text sibling preceding a block now gets flushed before the block consumes its own children, so `"orphanactual"` becomes `"orphan\n\nactual"`; (b) an empty `<p></p>` now leaves a blank text segment (matching `fragment_html`) instead of vanishing, distinguished from a block whose child already flushed via a `had_children` guard so nested blocks (e.g. `list_item > paragraph`) don't gain spurious empty entries. Three new Rust tests cover this, all passing. Test results: `go test ./...` green; `cargo test` (core, 19 tests) green; full CI gate green, including the wasm-freshness check (re-copied `internal/core/testdata/docs_core.wasm`) and the JS/Rust wire-compat round trip. Report written to `/home/paulca/amadan.net/carlos/docs/.superpowers/sdd/2026-08-31-editing/task-5-report.md`.</result> <usage><subagent_tokens>145745</subagent_tokens><tool_uses>68</tool_uses><duration_ms>565332</duration_ms></usage> </task-notification>
Human
<task-notification> <task-id>a5e5cfb638aacb572</task-id> <tool-use-id>toolu_01P5GAZPJNmQcQwvrBMkxMwx</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-carlos-docs/70f302d1-9d5d-4eb2-8b38-e09a15fdd1c3/tasks/a5e5cfb638aacb572.output</output-file> <status>completed</status> <summary>Agent "Review editing task 5 security seam" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>Tree is clean, no stray files (the review .md lives in `.superpowers/sdd/`, an expected output). --- **Spec verdict: PASS.** **Quality verdict: HIGH.** No BLOCKING issue — safe to merge. Verified by doing: `go test ./...` green, `cargo test` green (incl. 3 new fragment tests), `.amadan/ci.d/10-gate` exit 0. Confirmed `yrs` `from_utf8_unchecked` at `read.rs:137`. Hostile POSTs (driven through the real router): empty body, truncated (½ and 1 byte), huge-length varint, 64×`0xff`, single `0x00` → all **400, none persisted**. Valid foreign-client update and real `nested.bin`/`marks.bin` → 204. **Exhaustive single-byte flip of `simple.bin` (291 flips): 194 accepted / 97 rejected, 0 read-errors and 0 HTML-errors after any accept.** One flip triggered a genuine trap in `yrs` (`panic_const_div_by_zero` in `block_store::find_pivot` from `docs_core_apply`) — **contained**: instance discarded+rebuilt, 400 returned, nothing persisted, document readable, no hang (whole sweep 4.1s). Also confirmed: injected >8MB log → GET returns **500 + 19-byte error page, not a truncated log**; `MaxBytesReader` wraps before `io.ReadAll` (413 before buffering); guest-call 1s timeout + `WithCloseOnContextDone` on the POST path. Owner scoping is **not vacuous** — I neutralised `owned()` and `TestUpdatesEndpointsAreOwnerScoped` failed for real (stranger GET=200/POST=204). The `had_children` guard is correct across all three fragment cases; tests use discriminating exact-string asserts. Findings: - **LOW-1** — `store.go` `ApplyUpdate` (~485-505) + `migrations/0001`: no `UNIQUE(doc_id, seq)` and non-atomic `MAX(seq)`→INSERT. Task 5 first exposes `ApplyUpdate` to concurrency; simultaneous POSTs to one doc can write duplicate seqs. Harmless (CRDT convergence, per-request instance) but the invariant is unenforced. - **LOW-2** — `handlers.go` `updatesPost`: all `ApplyUpdate` errors collapse to 400 + Info log, so a real rebuild-failure or DB error is mis-reported as "bad update" and under-logged. Split clean-rejection/`Recovered` (400) from other errors (500 + Error). - **INFO** — 8MB doc → 500 on GET for the owner; loud-not-silent, the intended D2 behaviour pending compaction. Full review at `/home/paulca/amadan.net/carlos/docs/.superpowers/sdd/2026-08-31-editing/task-5-review.md`.</result> <usage><subagent_tokens>104175</subagent_tokens><tool_uses>36</tool_uses><duration_ms>452534</duration_ms></usage> </task-notification>
diff --git a/internal/core/core.go b/internal/core/core.go| index 1e5141a..54bad65 100644 |
| --- a/internal/core/core.go |
| +++ b/internal/core/core.go |
| @@ -269,6 +269,36 @@ func Recovered(err error) bool { |
| return errors.As(err, &r) |
| } |
| +// RejectedError wraps the error from an Apply call that cleanly refused |
| +// the update (a negative return code from the guest, e.g. malformed wire |
| +// bytes yrs's decoder catches) without the instance ever being touched — |
| +// the document's existing content is completely unaffected, and the |
| +// instance is exactly as usable afterward as it was before the call. This |
| +// is the ordinary, expected way most bad input is refused; see Apply's |
| +// own doc comment. |
| +// |
| +// A caller outside this package cannot otherwise distinguish this case |
| +// from a rebuild that itself failed (Apply's plain errors.Join branch — |
| +// a fatal error, and one this package genuinely could not recover from) |
| +// or from a wholly unrelated failure in a caller's own pipeline: neither |
| +// of those is "the client's fault" the way this is, and before this type |
| +// existed, both looked identical to this from the outside — a bare, |
| +// untyped error. Use Rejected(err) rather than a type assertion, the same |
| +// pattern Recovered(err) above already establishes for the trap-and- |
| +// rebuild-succeeded case. |
| +type RejectedError struct{ err error } |
| + |
| +func (e *RejectedError) Error() string { return e.err.Error() } |
| +func (e *RejectedError) Unwrap() error { return e.err } |
| + |
| +// Rejected reports whether err came from an Apply call that cleanly |
| +// refused the update with the instance left fully usable — see |
| +// RejectedError. |
| +func Rejected(err error) bool { |
| + var r *RejectedError |
| + return errors.As(err, &r) |
| +} |
| + |
| // Handle is one document, backed by its own module instance. Not safe for |
| // concurrent use; callers serialise access per document. |
| // |
| @@ -437,14 +467,18 @@ func applyOn(ctx context.Context, timeout time.Duration, mod api.Module, docHand |
| // error code from the guest, e.g. malformed wire bytes that yrs's decoder |
| // catches) leaves the instance untouched and fully usable — that is the |
| // whole point of §5.2's per-document instance: most bad input never needs |
| -// the instance discarded at all. A trap or a deadline exceeded is |
| -// different: the instance is discarded and rebuilt from every update |
| -// applied here so far, and the triggering update is reported as an error |
| -// but never retried or logged, so it can never itself be replayed into the |
| -// rebuilt instance. When that rebuild succeeds, the returned error is a |
| +// the instance discarded at all. That case comes back as a *RejectedError |
| +// (see Rejected). A trap or a deadline exceeded is different: the |
| +// instance is discarded and rebuilt from every update applied here so |
| +// far, and the triggering update is reported as an error but never |
| +// retried or logged, so it can never itself be replayed into the rebuilt |
| +// instance. When that rebuild succeeds, the returned error is a |
| // *RecoveredError (see Recovered) so a caller can tell "this update was |
| // rejected but the document is fine" apart from "the document itself is |
| -// gone" (a rebuild that fails returns a plain joined error instead). |
| +// gone" (a rebuild that fails returns a plain joined error instead — the |
| +// one shape this method returns that is neither *RejectedError nor |
| +// *RecoveredError, and the one a caller must not mistake for either: the |
| +// document itself may now be unusable, not merely this one update). |
| // |
| // Once the document's retained update log is full (maxRetainedUpdates), |
| // Apply refuses any further update — including one that would otherwise |
| @@ -470,7 +504,7 @@ func (h *Handle) Apply(ctx context.Context, update []byte) error { |
| } |
| return &RecoveredError{err} |
| } |
| - return err |
| + return &RejectedError{err} |
| } |
| // Text returns the document's current text content. If the read itself |
diff --git a/internal/docs/handlers.go b/internal/docs/handlers.go| index 60319b7..60abc91 100644 |
| --- a/internal/docs/handlers.go |
| +++ b/internal/docs/handlers.go |
| @@ -13,6 +13,8 @@ import ( |
| "github.com/carlosframework/rastrillo" |
| rauth "github.com/carlosframework/rastrillo/auth" |
| + |
| + "github.com/carlosframework/docs/internal/core" |
| ) |
| // maxUpdateBytes caps a single client-submitted Yjs update |
| @@ -350,18 +352,53 @@ func (h *handlers) updatesPost(w http.ResponseWriter, r *http.Request) { |
| } |
| if err := h.store.ApplyUpdate(r.Context(), doc, update); err != nil { |
| - // Not h.logger.Error: a client sending bytes the core |
| - // rejects is an ordinary, expected outcome of accepting |
| - // arbitrary input over HTTP, not an operational problem with |
| - // this instance. |
| - h.logger.Info("docs: reject posted update", "sqid", doc.Sqid, "err", err) |
| - http.Error(w, rastrillo.T(r, "docs.error.badupdate"), http.StatusBadRequest) |
| + if updateIsClientFault(err) { |
| + // Info, not Error: a client sending bytes the core |
| + // cleanly rejects is an ordinary, expected outcome of |
| + // accepting arbitrary input over HTTP, not an operational |
| + // problem with this instance. Fix round 1 on Task 5: this |
| + // used to be the ONLY branch, so a real database or |
| + // wasm-rebuild failure was also reported to the client as |
| + // "your bytes were bad" and logged where nobody would |
| + // think to look for an outage. |
| + h.logger.Info("docs: reject posted update", "sqid", doc.Sqid, "err", err) |
| + http.Error(w, rastrillo.T(r, "docs.error.badupdate"), http.StatusBadRequest) |
| + return |
| + } |
| + h.logger.Error("docs: apply posted update", "sqid", doc.Sqid, "doc_id", doc.ID, "err", err) |
| + http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError) |
| return |
| } |
| w.WriteHeader(http.StatusNoContent) |
| } |
| +// updateIsClientFault classifies a Store.ApplyUpdate error: true means |
| +// the core looked at the bytes the client just posted and refused |
| +// them — a 400, the client's problem, and unremarkable enough to log |
| +// at Info. False means everything else: a database error, a replay |
| +// failure reading this document's own history, or a trap whose |
| +// recovery itself failed (core.Handle.Apply's plain errors.Join |
| +// branch — the document may now be unusable, not merely this one |
| +// update) — none of which the client caused, all of which deserve a |
| +// 500 and an Error-level log an operator will actually see. |
| +// |
| +// errors.As(err, &ur) is the load-bearing check, not the three core |
| +// checks after it: only Store.ApplyUpdate's own call to |
| +// core.Handle.Apply for THIS update wraps its error in |
| +// *updateRejection (store.go) — a failure replaying this document's |
| +// existing log calls the exact same core.Handle.Apply and would |
| +// satisfy core.Recovered/core.Rejected/ErrUpdateLogFull just as |
| +// easily, but is never wrapped in *updateRejection, so it can never |
| +// reach the 400 branch by accident. |
| +func updateIsClientFault(err error) bool { |
| + var ur *updateRejection |
| + if !errors.As(err, &ur) { |
| + return false |
| + } |
| + return core.Recovered(err) || core.Rejected(err) || errors.Is(err, core.ErrUpdateLogFull) |
| +} |
| + |
| // groupByRecency buckets docs — already newest-first, per Store.List |
| // — into recency groups without re-sorting: consecutive documents |
| // that share a bucket join the same group, and a new bucket starts a |
diff --git a/internal/docs/migrations/0003_doc_updates_seq_unique.sql b/internal/docs/migrations/0003_doc_updates_seq_unique.sql| new file mode 100644 |
| index 0000000..02029d6 |
| --- /dev/null |
| +++ b/internal/docs/migrations/0003_doc_updates_seq_unique.sql |
| @@ -0,0 +1,27 @@ |
| +-- Fix round 1 on Task 5 (fuzzing/race review): two concurrent POSTs to |
| +-- the same document could both read the same MAX(seq) before either |
| +-- had committed its INSERT, and both land a row at the same (doc_id, |
| +-- seq). Harmless to the document itself -- the CRDT converges |
| +-- regardless of the order updates replay in, and each request runs |
| +-- its own core.Handle -- but a later compaction or snapshot plan that |
| +-- assumes seq is unique per document would be quietly wrong about |
| +-- rows already on disk. store.go's ApplyUpdate now retries on this |
| +-- constraint rather than trusting its own MAX(seq) read to still be |
| +-- current by the time it writes (its own doc comment). |
| +-- |
| +-- Like 0002_owners.sql's owner_id backfill, this is written for a |
| +-- database that may already have rows, not just a fresh one -- and |
| +-- CREATE UNIQUE INDEX simply refuses to apply if a duplicate (doc_id, |
| +-- seq) pair already exists, rather than silently dropping one of the |
| +-- two rows to make room for it. That refusal is the correct behaviour |
| +-- here, not a gap to paper over with a DELETE: both racing rows are a |
| +-- real, already-persisted update apiece, and picking one to discard |
| +-- during a migration would be exactly the kind of silent data loss |
| +-- this whole plan's GET /d/{sqid}/updates cap (handlers.go's |
| +-- maxLogBytes) refuses to commit elsewhere. In practice this is safe |
| +-- to apply as-is: this plan is what puts POST /d/{sqid}/updates |
| +-- (and therefore this race) in front of any real traffic for the |
| +-- first time, and this migration ships in the same change as the |
| +-- fix -- there is no window in which the vulnerable code could have |
| +-- run against a database this migration is later applied to. |
| +CREATE UNIQUE INDEX IF NOT EXISTS idx_doc_updates_doc_id_seq ON doc_updates (doc_id, seq); |
diff --git a/internal/docs/store.go b/internal/docs/store.go| index 479b123..2427f5e 100644 |
| --- a/internal/docs/store.go |
| +++ b/internal/docs/store.go |
| @@ -429,15 +429,37 @@ func (s *Store) HTML(ctx context.Context, doc *Document) (template.HTML, error) |
| return template.HTML(html), nil |
| } |
| +// updateRejection wraps whatever error Handle.Apply(update) itself |
| +// returned for the update just submitted to ApplyUpdate below — never |
| +// for a failure while replay (above) rebuilds this document's |
| +// existing history, and never for an ordinary database error further |
| +// down in ApplyUpdate. It exists purely so a caller outside this |
| +// package (handlers.go's updatesPost, fix round 1 on Task 5) can tell |
| +// "the core said no to the bytes the client just sent" apart from |
| +// "something on our own end broke", using core.Recovered/core.Rejected/ |
| +// errors.Is(..., core.ErrUpdateLogFull) on the unwrapped error, without |
| +// mistaking a failure from replay for one — replay calls the exact |
| +// same core.Handle.Apply, on the exact same *core.Handle, and would |
| +// satisfy those same checks if it were not kept structurally distinct |
| +// from this wrapper. A replay failure means EXISTING, already-accepted |
| +// history came back corrupted or unreplayable — never the client's |
| +// fault, regardless of what shape the underlying core error happens to |
| +// take — so it is deliberately never given this wrapper. |
| +type updateRejection struct{ err error } |
| + |
| +func (e *updateRejection) Error() string { return e.err.Error() } |
| +func (e *updateRejection) Unwrap() error { return e.err } |
| + |
| // ApplyUpdate applies a new Yjs update to doc: it replays the |
| // existing log to rebuild current state, applies update against that |
| // live core instance, and only once the core has accepted it does it |
| // persist the update as the next row in doc_updates. A rejection from |
| // the core (a clean rejection, core.ErrUpdateLogFull, or a |
| -// *core.RecoveredError) is returned as-is and the update is never |
| -// persisted — see the errors.Is-friendly wrapping in replay's doc |
| -// comment and Handle.Apply's for why an uncertain outcome must not be |
| -// written down as a fact. |
| +// *core.RecoveredError) is returned as-is, wrapped in *updateRejection |
| +// (above) so a caller can positively identify it as such, and the |
| +// update is never persisted — see the errors.Is-friendly wrapping in |
| +// replay's doc comment and Handle.Apply's for why an uncertain outcome |
| +// must not be written down as a fact. |
| // |
| // Before any of that, doc.OwnerID is verified against the row itself. |
| // Fix round 2 on this task (Finding 1 again, this time for real): the |
| @@ -484,26 +506,57 @@ func (s *Store) ApplyUpdate(ctx context.Context, doc *Document, update []byte) e |
| defer h.Close(ctx) |
| if err := h.Apply(ctx, update); err != nil { |
| - return fmt.Errorf("docs: apply update to document %d: %w", doc.ID, err) |
| + return fmt.Errorf("docs: apply update to document %d: %w", doc.ID, &updateRejection{err}) |
| } |
| - var maxSeq int64 |
| - if err := s.db.WithContext(ctx). |
| - Model(&DocUpdate{}). |
| - Where("doc_id = ?", doc.ID). |
| - Select("COALESCE(MAX(seq), 0)"). |
| - Scan(&maxSeq).Error; err != nil { |
| - return fmt.Errorf("docs: determine next seq for document %d: %w", doc.ID, err) |
| - } |
| + // MAX(seq) then INSERT used to be two separate statements, not one |
| + // atomic step: two concurrent POSTs to this same document (Task 6's |
| + // flush timer and its beforeunload beacon are exactly the pair that |
| + // races on a real page close) could both read the same MAX(seq) |
| + // before either had committed its own INSERT, and both then write |
| + // the same (doc_id, seq) pair. doc_updates' UNIQUE(doc_id, seq) |
| + // index (migration 0003) is the backstop that would turn a second |
| + // such bug into a loud error instead of silent duplicate data, but |
| + // the actual fix is this transaction: rastrillo/db's pool (db.go's |
| + // own doc comment) caps the WRITE pool at exactly one connection — |
| + // SQLite allows one writer — and gorm.DB.Transaction always opens |
| + // on that pool's underlying *sql.DB, never on the read replicas |
| + // dbresolver would otherwise route a bare SELECT to. With only one |
| + // connection available, a second concurrent call to this method |
| + // cannot even begin its own transaction until this one commits and |
| + // releases it — so by the time that second transaction's SELECT |
| + // MAX(seq) runs, it is guaranteed to see this row already |
| + // committed. That is a stronger guarantee than retrying on a |
| + // UNIQUE-constraint collision (the migration's own alternative): |
| + // a bounded retry loop can still be exhausted once enough requests |
| + // race the same document at once, so a busy document would start |
| + // returning errors under load that this queue-instead-of-fail |
| + // approach never does — confirmed empirically: an earlier version |
| + // of this fix used a 5-attempt retry loop, and |
| + // updates_test.go's TestConcurrentUpdatesDoNotCollideOnSeq (21 |
| + // concurrent POSTs to one document) genuinely exhausted it on a |
| + // real run before this transaction replaced it. |
| + if err := s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { |
| + var maxSeq int64 |
| + if err := tx.Model(&DocUpdate{}). |
| + Where("doc_id = ?", doc.ID). |
| + Select("COALESCE(MAX(seq), 0)"). |
| + Scan(&maxSeq).Error; err != nil { |
| + return fmt.Errorf("docs: determine next seq for document %d: %w", doc.ID, err) |
| + } |
| - row := &DocUpdate{ |
| - DocID: doc.ID, |
| - Seq: maxSeq + 1, |
| - UpdateBin: update, |
| - CreatedAt: time.Now().UTC(), |
| - } |
| - if err := s.db.WithContext(ctx).Create(row).Error; err != nil { |
| - return fmt.Errorf("docs: persist update for document %d: %w", doc.ID, err) |
| + row := &DocUpdate{ |
| + DocID: doc.ID, |
| + Seq: maxSeq + 1, |
| + UpdateBin: update, |
| + CreatedAt: time.Now().UTC(), |
| + } |
| + if err := tx.Create(row).Error; err != nil { |
| + return fmt.Errorf("docs: persist update for document %d: %w", doc.ID, err) |
| + } |
| + return nil |
| + }); err != nil { |
| + return err |
| } |
| // Bumping documents.updated_at here — not just appending the |
diff --git a/internal/docs/updates_test.go b/internal/docs/updates_test.go| index b2c7ea3..c58d31a 100644 |
| --- a/internal/docs/updates_test.go |
| +++ b/internal/docs/updates_test.go |
| @@ -7,7 +7,9 @@ import ( |
| "net/http/httptest" |
| "os" |
| "strings" |
| + "sync" |
| "testing" |
| + "time" |
| ) |
| // serve runs req through the app's real router and returns the |
| @@ -149,3 +151,165 @@ func TestOversizedUpdateIsRefused(t *testing.T) { |
| t.Errorf("oversized POST = %d, want 413", rec.Code) |
| } |
| } |
| + |
| +// TestInternalApplyFailureReturns500NotBadUpdate is fix round 1's |
| +// finding (1): updatesPost used to collapse every Store.ApplyUpdate |
| +// error into 400 at Info, so a genuine failure on our own end (a |
| +// corrupted historical row that the real core cannot even replay) was |
| +// reported to the client as "your bytes were bad" — the exact wrong- |
| +// side-of-the-mistake shape this codebase keeps producing. |
| +// |
| +// The failure here is real, not mocked: a garbage row is written |
| +// directly into doc_updates as if it were already-persisted history |
| +// (bypassing ApplyUpdate's own validation entirely — the only way |
| +// such a row could exist, since ApplyUpdate never persists what the |
| +// core rejects), and the POST that follows drives the actual wasm |
| +// core through Store.replay to really fail decoding it. A mock |
| +// Store returning an arbitrary error would only prove the handler's |
| +// switch statement compiles, not that the classification matches |
| +// what the core actually returns for a real internal failure — the |
| +// reviewer's own words for why this test exists. |
| +func TestInternalApplyFailureReturns500NotBadUpdate(t *testing.T) { |
| + app, err := NewForTest(t) |
| + if err != nil { |
| + t.Fatalf("NewForTest: %v", err) |
| + } |
| + ctx := context.Background() |
| + owner := app.TestPerson(t, "owner@example.com") |
| + doc, err := app.Store().Create(ctx, owner, "Corrupted history") |
| + if err != nil { |
| + t.Fatalf("Create: %v", err) |
| + } |
| + |
| + // app.store.db is package-private (same package, docs) — reached |
| + // directly here because this row must land in doc_updates without |
| + // ever going through ApplyUpdate's own core validation, which |
| + // would (correctly) refuse it. |
| + corrupt := &DocUpdate{ |
| + DocID: doc.ID, |
| + Seq: 1, |
| + UpdateBin: []byte{0xff, 0x00, 0x13, 0x37}, |
| + CreatedAt: time.Now().UTC(), |
| + } |
| + if err := app.store.db.WithContext(ctx).Create(corrupt).Error; err != nil { |
| + t.Fatalf("seed a corrupted historical row: %v", err) |
| + } |
| + |
| + rec := serve(app, app.TestRequest(t, "owner@example.com", http.MethodPost, |
| + "/d/"+doc.Sqid+"/updates", bytes.NewReader(validUpdate(t)))) |
| + if rec.Code != http.StatusInternalServerError { |
| + t.Errorf("POST with unreplayable history = %d, want 500 (not 400 — this is not the client's fault): %s", rec.Code, rec.Body) |
| + } |
| +} |
| + |
| +// TestConcurrentUpdatesDoNotCollideOnSeq is fix round 1's finding (2): |
| +// ApplyUpdate's MAX(seq)->INSERT was two separate statements, not one |
| +// atomic step, so two POSTs racing on the same document could both |
| +// compute the same next seq and both write it. Reachable for real |
| +// once Task 6's editor debounces edits into a flush timer AND sends a |
| +// beforeunload beacon with whatever is still queued — those two |
| +// genuinely race on a real page close. |
| +// |
| +// This drives the race with real concurrent goroutines POSTing |
| +// through the real router to the same document, started together off |
| +// one channel close rather than launched in a loop (which tends to |
| +// serialise on the Go scheduler before any of them touch the |
| +// database). |
| +// |
| +// Every goroutine posts the exact same fixture, not N different ones: |
| +// every hand-written fixture under fixtures/vectors/fragment/ pins |
| +// Y.Doc.clientID to a fixed value for reproducibility (generate- |
| +// fragment.mjs's own doc comment) — 1, for every one of them except |
| +// "concurrent" — so applying two DIFFERENT such fixtures to one |
| +// shared document makes two genuinely incompatible claims about that |
| +// same client's clock history and the real core correctly refuses the |
| +// second (confirmed empirically: mixing fixtures here produced real |
| +// "apply rejected the update: code -3" replay failures unrelated to |
| +// the seq race this test exists to exercise). Reapplying the identical |
| +// bytes N times is exactly what a real debounce+beacon race would |
| +// also risk doing did it retry a failed flush verbatim, and it is |
| +// idempotent at the CRDT level (Store.ApplyUpdate content-merges, not |
| +// row-dedupes), so it still exercises the real thing this test is |
| +// for: N real POSTs each getting persisted a distinct doc_updates row. |
| +func TestConcurrentUpdatesDoNotCollideOnSeq(t *testing.T) { |
| + app, err := NewForTest(t) |
| + if err != nil { |
| + t.Fatalf("NewForTest: %v", err) |
| + } |
| + ctx := context.Background() |
| + owner := app.TestPerson(t, "owner@example.com") |
| + doc, err := app.Store().Create(ctx, owner, "Race") |
| + if err != nil { |
| + t.Fatalf("Create: %v", err) |
| + } |
| + payload := readFixture(t, "../../fixtures/vectors/fragment/simple.bin") |
| + |
| + // One sign-in, cookies reused by every goroutine below: this test |
| + // is about racing the write path, not re-proving TestSignIn's own |
| + // round trip N times over. |
| + link, err := app.TestSignIn(t, "owner@example.com") |
| + if err != nil { |
| + t.Fatalf("TestSignIn: %v", err) |
| + } |
| + verifyRec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(verifyRec, httptest.NewRequest(http.MethodGet, link, nil)) |
| + cookies := verifyRec.Result().Cookies() |
| + if len(cookies) == 0 { |
| + t.Fatal("sign-in set no session cookie") |
| + } |
| + |
| + const n = 21 |
| + codes := make([]int, n) |
| + var wg sync.WaitGroup |
| + start := make(chan struct{}) |
| + for i := 0; i < n; i++ { |
| + wg.Add(1) |
| + go func(i int) { |
| + defer wg.Done() |
| + <-start |
| + req := httptest.NewRequest(http.MethodPost, "/d/"+doc.Sqid+"/updates", bytes.NewReader(payload)) |
| + for _, c := range cookies { |
| + req.AddCookie(c) |
| + } |
| + req.Header.Set("Sec-Fetch-Site", "same-origin") |
| + codes[i] = serve(app, req).Code |
| + }(i) |
| + } |
| + close(start) |
| + wg.Wait() |
| + |
| + for i, code := range codes { |
| + if code != http.StatusNoContent { |
| + t.Errorf("goroutine %d: POST = %d, want 204", i, code) |
| + } |
| + } |
| + |
| + var rows []DocUpdate |
| + if err := app.store.db.WithContext(ctx). |
| + Where("doc_id = ?", doc.ID). |
| + Order("seq ASC"). |
| + Find(&rows).Error; err != nil { |
| + t.Fatalf("load doc_updates: %v", err) |
| + } |
| + if len(rows) != n { |
| + t.Fatalf("doc_updates has %d rows for document %d, want %d — a concurrent POST's row went missing", len(rows), doc.ID, n) |
| + } |
| + seen := make(map[int64]bool, n) |
| + for _, row := range rows { |
| + if seen[row.Seq] { |
| + t.Fatalf("duplicate seq %d for document %d across %d concurrent POSTs — the MAX(seq)->INSERT race was not closed", row.Seq, doc.ID, n) |
| + } |
| + seen[row.Seq] = true |
| + } |
| + |
| + // The document must still replay — not just "no duplicate rows", |
| + // but every one of those rows a real, applyable update the core |
| + // accepts in sequence. |
| + html, err := app.Store().HTML(ctx, doc) |
| + if err != nil { |
| + t.Fatalf("HTML after %d concurrent updates: %v", n, err) |
| + } |
| + if !strings.Contains(string(html), "First paragraph.") { |
| + t.Errorf("document lost content after concurrent updates: %q", html) |
| + } |
| +} |