Opt-in optimistic concurrency for record mutations (#48)#96
Merged
Conversation
version was documented as being for conflict detection but no such mechanism existed anywhere in the stack — updates were unconditional read-modify-write, and the SQLite adapters silently dropped colliding version snapshots via INSERT OR IGNORE, corrupting rollback history exactly when a conflict happened. - Stack/ScopedStack: ifVersion option on update/delete/undelete/ associate/dissociate/setPermissions/restoreVersion. Throws StackVersionConflictError (a StackConflictError subtype carrying recordId/expectedVersion/actualVersion) on a mismatch; omitting it keeps last-writer-wins. - StackRecordAdapter contract: expectedVersion threaded through the same methods. The check is atomic inside each adapter's write (UPDATE ... WHERE id = ? AND version = ?), not a read-then-write in Stack, so the race can't just move down a layer. - sqlite-shared: saveVersion's INSERT OR IGNORE becomes a plain INSERT mapped to StackConflictError, so a version collision fails loudly instead of silently dropping a snapshot — this also protects last-writer-wins callers who never opted into ifVersion. - adapter-api: If-Match header carries expectedVersion over the wire; wire-types gained a version_conflict code that round-trips the conflict's recordId/expectedVersion/actualVersion, sharing 409 with the existing conflict code. - Fixed a real bug found while wiring this up: records_fts is an external-content FTS4/FTS5 table, so removing a stale index entry requires reading the record's content before it changes — same constraint for both engines despite fts.ts's comment claiming otherwise. patchContent/restoreVersion now check the precondition before touching FTS, then mutate in the original safe order. - docs/spec.md: corrected the version field's doc comment and adapter wire format, documented ifVersion/If-Match and the 409 response shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Np76L1sQPMSvgFYNcN9nDT
…tackConflictError subtype 409 was borrowed from StackConflictError under the assumption that reusing its class (and status) was simplest. Two problems with that: inheriting from StackConflictError meant either every version conflict had to share 409 or a subtype had to silently override its parent's status, and sharing 409 with the generic 'conflict' code meant status-only wire-error reconstruction (no parseable body — a legacy server, or a proxy that strips bodies) could only ever recover the generic StackConflictError, never the precise version_conflict. StackVersionConflictError is now a standalone error class (own static code, no shared base with StackConflictError) mapped to 412 — RFC 7232's status for a failed If-Match precondition — so every wire code keeps an unambiguous 1:1 status mapping, including in the status-only fallback path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Np76L1sQPMSvgFYNcN9nDT
Sat right next to the 412 row's version-conflict description, reading as if the doc described the same ifVersion mechanism twice under two different statuses. The saveVersion snapshot-collision safety net is already documented in full, in the right place, under the "Never silently dropped" paragraph in ## Versions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Np76L1sQPMSvgFYNcN9nDT
…Conflict instead of flat top-level fields recordId/expectedVersion/actualVersion sat directly on WireError.error next to code/message, unrelated to any other code and only meaningful for 'version_conflict'. Reusing the existing details field instead (as originally considered) would have made it a union of ValidationError[] and this new shape, forcing code-based narrowing everywhere it's read. Instead: a new versionConflict object, scoped and typed like details already is for validation — each error code that carries structured data gets its own uniquely-named, uniquely-typed field rather than flat fields sprawling across the shared error object or one field doing double duty. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Np76L1sQPMSvgFYNcN9nDT
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #48.
versionwas documented as "for conflict detection," but no such mechanism existed anywhere in the stack:Stack.update()was unconditional read-modify-write, the adapter contract had no precondition parameter, and the SQLite adapters'saveVersionusedINSERT OR IGNORE, silently dropping a colliding version snapshot exactly when a real conflict happened — punching a hole in rollback history at the one moment you'd want to roll back.Stack/ScopedStack: newifVersionoption onupdate,delete,undelete,associate,dissociate,setPermissions,restoreVersion. On a mismatch it throwsStackVersionConflictError(aStackConflictErrorsubtype carryingrecordId/expectedVersion/actualVersionso a caller can decide whether to retry). Omitting it keeps today's last-writer-wins behavior — apps that don't care don't pay.expectedVersionthreaded through the same methods onStackRecordAdapter. The check happens atomically inside each adapter's write (UPDATE ... WHERE id = ? AND version = ?, inspecting the affected-row count) rather than as a read-then-write inStack, so the race can't just move down a layer.saveVersion:INSERT OR IGNORE→ plainINSERT, mapped toStackConflictErroron the resultingUNIQUEcollision. A version-number collision is now loud instead of silently dropping a snapshot — this also protects last-writer-wins callers who never opted intoifVersion, since the losing write's own snapshot attempt fails before it can touch the actual mutation.adapter-api:If-Matchheader carriesexpectedVersionover the wire.wire-typesgained aversion_conflicterror code that round-tripsrecordId/expectedVersion/actualVersion, sharing HTTP 409 with the existingconflictcode (kept as one status sinceStackVersionConflictError extends StackConflictError; see spec for the status-only-fallback tradeoff this implies).docs/spec.md: corrected theversionfield's doc comment (it was never really "for conflict detection"), documentedifVersion/If-Match, and updated the wire error table/body shape.Bug found along the way
Wiring the CAS check into
patchContent/restoreVersioninitially reorderedfts.remove()to run after the content-mutatingUPDATE, which broke full-text search:records_ftsis an external-content FTS4/FTS5 table (content='records'), so unindexing a row requires reading the record's content before it changes — true for both engines, despite an existing code comment claiming FTS4 didn't need this. Fixed by checking theifVersionprecondition first (using the record already fetched for the operation) and keeping the originalremove → mutate → insertordering afterward, with a regression test locking in the FTS4 case specifically.Test plan
pnpm -r build/pnpm -r typecheck/pnpm -r lint/pnpm -r testall clean across the monorepoifVersionunit tests inpackages/core/tests/stack.test.ts(update/associate/dissociate/setPermissions/delete/undelete/restoreVersion, conflict + success + not-found paths)expectedVersionadapter-level tests in bothrecord-adapter-sqliteandrecord-adapter-sqljs, including the FTS-index-consistency regression testadapter-apitests:If-Matchheader sent/omitted correctly,StackVersionConflictErrorreconstructed from aversion_conflictwire bodysaveVersionidempotency test updated to assert the new loud-failure behavior instead of the old silent-drop behavior🤖 Generated with Claude Code
https://claude.ai/code/session_01Np76L1sQPMSvgFYNcN9nDT
Generated by Claude Code