Extract shared SQLite layer; close deleteAttachment race; enforce FKs (#46 phase 1)#84
Open
cuibonobo wants to merge 1 commit into
Open
Extract shared SQLite layer; close deleteAttachment race; enforce FKs (#46 phase 1)#84cuibonobo wants to merge 1 commit into
cuibonobo wants to merge 1 commit into
Conversation
…ce; enforce FKs First phase of #46 (SQLite split), plus the two follow-up work items from its comments: - New internal package @haverstack/sqlite-shared: schema DDL, WHERE/ ORDER building, cursor codec, row mappers, and the FTS4 sanitizer, extracted verbatim from record-adapter-sqljs so a future native SQLite adapter shares this SQL instead of drifting from a second copy. Cursor encoding moves from Node's Buffer to btoa/atob so the codec is portable to a browser adapter without change. - record-adapter-sqljs enables PRAGMA foreign_keys = ON and maps FK constraint violations to StackNotFoundError in associate(), so associating a nonexistent record fails loudly instead of silently creating an orphan row. Also fixes an sql.js quirk where db.export() (called by persist() after every write) silently resets connection pragmas, which was quietly turning FK enforcement back off after the first write. - StackRecordAdapter gains an optional deleteUnreferencedAttachmentRecords(fileId, metadataTypeId), and record-adapter-sqljs implements it as a single atomic SQL transaction. Stack.deleteAttachment() uses it when available, closing the check-then-act race where a concurrent associate() could add a reference between the reference check and the metadata delete; adapters without it fall back to the previous non-atomic sequence. Byte deletion still runs after the metadata step commits, so a crash in between leaves orphaned bytes (GC-reclaimable) rather than a dangling reference. - MemoryAdapter gains attachmentFileId filtering, needed to actually exercise deleteAttachment()'s reference check in core's test suite (previously untested). Native adapter creation, sqljs's browser-only cleanup, the adapter-local swap, and the token-store-out-of-file default are left for a follow-up pass on #46. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRLiGUZC344E3pgNEGAe5W
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
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
First phase of #46 (SQLite record-adapter split), plus the two follow-up work items filed as comments on that issue. #46's own suggested order puts "§3 shared layer extraction (pure refactor, current tests keep passing)" first — this PR does that, plus the two comment items that fit naturally on top of it:
@haverstack/sqlite-shared(not aStackAdapter— not published/public API): schema DDL,WHERE/ORDERbuilding, cursor codec, row mappers, and the FTS4 sanitizer, extracted verbatim fromrecord-adapter-sqljs. This is what a future native SQLite adapter would consume too, so a cursor minted by one SQLite-backed adapter stays decodable by another instead of the two copies drifting. Cursor encoding moves from Node'sBuffertobtoa/atobso the codec is portable to a browser adapter without change.record-adapter-sqljsnow runsPRAGMA foreign_keys = ONand maps constraint violations toStackNotFoundErrorinassociate(), so associating a nonexistent record fails loudly instead of silently creating an orphan row. This surfaced an sql.js quirk along the way —db.export()(called bypersist()after every write) silently resets connection-level pragmas, which was quietly turning FK enforcement back off after the very first write. Fixed by reapplying the pragma insidepersist(), with a regression test.deleteAttachment()'s check-then-act race (comment item 3):StackRecordAdaptergains an optionaldeleteUnreferencedAttachmentRecords(fileId, metadataTypeId), implemented inrecord-adapter-sqljsas a single atomic SQL transaction.Stack.deleteAttachment()uses it when the adapter provides it, closing the race where a concurrentassociate()could add a reference between the reference check and the metadata delete; adapters without it fall back to the previous non-atomic sequence. Byte deletion still runs only after the metadata step commits — a crash in between leaves orphaned bytes (harmless, GC-reclaimable later) rather than a dangling reference (harmful).MemoryAdapter(core's test double) gainsattachmentFileIdfiltering — needed to actually exercisedeleteAttachment()'s reference check, which had no test coverage before this PR.Left for a follow-up on #46
Native adapter creation (
record-adapter-sqliteonnode:sqlite, FTS5, WAL), sqljs's browser-only cleanup, theadapter-localswap, and moving the default token store out of the portable stack file (comment item 2) — these are larger, separable chunks of #46 and are better done once this shared layer is in place to build on.Test plan
pnpm typecheck/pnpm lint/pnpm test— full monorepo, clean, 492 tests passing (no regressions)@haverstack/sqlite-sharedunit tests: cursor codec roundtrip + malformed-input cases, FTS4 sanitizerrecord-adapter-sqljstests: FK violation onassociate()→StackNotFoundError; FK enforcement survives multiplepersist()calls (regression test for the sql.js pragma-reset quirk);deleteUnreferencedAttachmentRecords— referenced throwsStackConflictError, unreferenced deletes and returns ids, dedup case deletes every metadata record sharing afileId, rollback leaves records intact on conflict, version history is cleaned up toocoretests:deleteAttachment()fallback path (conflict, success, not-found) and confirmation that the atomic adapter path is preferred when availableRefs #46.
https://claude.ai/code/session_01RRLiGUZC344E3pgNEGAe5W
Generated by Claude Code