fix(storage): cross-tab admission for protected writes vs. active migrations - #339
Conversation
CI's docs:check drift gate flagged 4 stale README badges/lines still citing the pre-expansion key count; bring them in line with the current locale bundle so the Node quality gate and its downstream Build/E2E/Storybook/Lighthouse jobs stop skipping.
Addresses the #335 review-thread cluster (CodeAnt/CodeRabbit/Codex/Qodo) converging on services/storage/*Store.ts: - Atomic write-key resolution (resolveProtectedWriteKey/idbEncryptWithKey): saveSlice/createSnapshot/saveImage/saveBinderAsset/saveStoryCodex/ saveRagVectors previously checked assertIdbProtectedWriteAllowed() then re-read isIdbEncryptionReady() after an intervening await (opening the IDB transaction) — Lock Session firing in that gap silently downgraded the write to plaintext. The two checks now collapse into one snapshot. - Fail-closed guards added to every previously-unguarded destructive/listing path: deleteImage, deleteBinderAsset, listBinderAssetIds, deleteStoryCodex, listSnapshots, deleteSnapshot, deleteProject (before its binder-asset cascade); getSnapshotData gets an explicit guard for clarity. - IDBRequest.onsuccess handlers that call idbReadSecure() now catch/reject instead of producing an unhandled rejection that left loadState()/ getImage()/getSnapshotData() pending forever on a locked read. - hasPassphraseSentinel() caches its result (invalidated by clearIdbEncryptionKey()) — was previously an unconditional IDB round trip on every protected read/write, including for users who never configured encryption. - Removed reEncryptAllAppData/reEncryptAllSnapshots: confirmed dead code (zero callers on this branch or #337) per CodeRabbit's own suggested disposition; rotateIdbPassphrase already unconditionally blocks rekeying pending the journal-based implementation. - Fixed two test files whose storageEncryptionService mocks were already missing assertIdbProtectedWriteAllowed (pre-existing gap, now widened by the new guards) and added coverage for every fix above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-closed gap Addresses the remaining #335 review-thread cluster: - storageEncryptionService.ts: getOrCreateSalt() now fails closed with a new IdbEncryptionSaltLostError when the persisted PBKDF2 salt is missing/invalid but a passphrase sentinel already exists — previously it silently derived a new salt, producing a different key and permanently orphaning existing encrypted data instead of surfacing the loss. Applies to initIdbEncryption, setupIdbEncryption, verifyAndInitIdbEncryption, and the (currently unreachable) rotateKey. - README + docs/IDB-ENCRYPTION.md: corrected the false claim that Tauri desktop gets the same IndexedDB-backed at-rest encryption as the web build. Tauri's filesystem-backed store (services/fs/*) writes compressed-but- unencrypted data regardless of the passphrase/unlock state; the unlock screen appearing on desktop was misleading users about actual protection. - locales/*/help.json (en/sv/fi/hu/is/eu/ru/fa): corrected the same overclaim ("all IndexedDB stores") in the in-app feature-flag help article to match the narrower, accurate scope already used elsewhere in the file. - locales/*/settings.json (all 19): writingSurfaceHint previously described the backdrop as "texture-free," contradicting the selector's own Textured option. Reworded to apply to both options; regenerated all 19 runtime bundles (also drops a stale lora.onboarding.selectPython key that had no source definition). - components/settings/GeneralSections.tsx: added aria-pressed to the writing- surface toggle buttons so assistive technology can identify the active option, plus the required QNBS-v3 rationale comment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…advisory metadata; document hooks:install - services/fs/settingsFsStore.ts, services/cloudSync/cloudSyncBackend.ts: loadSettings() used an unchecked `as Settings`/generic-decrypt cast, so settings persisted before writingSurfaceStyle (or any newer required field) existed would violate the Settings contract at runtime. Both now route through the same normalizePersistedSettings() already used by the IDB path. - tests/unit/languageToolClient.test.ts: baseSettings() was missing the required writingSurfaceStyle field. - pnpm-workspace.yaml: corrected several advisory comments that misdescribed their GHSA (wrong summary text for fast-uri/ip-address/joi/undici, an invalid esbuild GHSA id, and two long-standing CVE-2024-XXXX placeholders for uuid/qs). Left override values unchanged — tightening the uuid floor needs a lockfile-only pnpm update this host can't safely run right now; documented as a follow-up rather than silently dropped. - CONTRIBUTING.md: documented `pnpm run hooks:install` as an explicit step after `pnpm install` — pnpm v11's allowBuilds policy denies simple-git-hooks' own install script, so a `prepare` lifecycle script would silently no-op; contributors who skip this step bypass lint-staged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…esktop stack CHANGELOG.md: comprehensive [Unreleased] entry for #335 (fail-closed IDB encryption lifecycle), #336 (desktop AI/Python hardening for #332/#333), #337 (durable resumable migration journal + secondary-store adapters), #339 (cross-tab write-admission closing the migration TOCTOU race), plus the #310 ledger closure and #338 tracking issue. AUDIT.md: appended this session's entry to the follow-up chain and a new quality-gate line; corrected the stale "2869 keys" figure in the current- version summary line to the actual 2904 (already true since an earlier commit this session, just never synced here) — node scripts/check-doc- metrics.mjs now genuinely passes rather than coincidentally matching.
…rness Qodo's review of #339 found real issues: - deleteProject() split its admission guard across two separate lock spans (a guard+cascade, then a later re-acquisition for the final delete), leaving a gap where a migration batch could commit between them — undermining the very race this PR closes. Fixed by giving deleteProject() one continuous admission span and having it call a new unadmitted core (deleteAllBinderAssetsForProjectUnadmitted) instead of the public, self-locking method: nesting two withProtectedWriteAdmission calls for the same lock name can deadlock if an exclusive migration request queues between the outer and inner acquisition. - The tests/setup.ts navigator.locks mock only blocked new shared requests when an exclusive lock was currently held, not when one was queued — letting a stream of readers starve a waiting migration indefinitely, the opposite of the fairness this PR documents. Fixed to track queued waiters by mode and block new shared requests behind an already-queued exclusive one; added a regression test for shared-holder -> queued-exclusive -> later-shared ordering. - pruneAutoSnapshots() called the newly-admitted deleteSnapshot() once per stale snapshot in a loop, performing N separate lock acquisitions and transactions instead of the single-lock/single-transaction batching this PR already uses for deleteAllBinderAssetsForProject. Extracted a shared deleteSnapshotsUnadmitted(ids) core used by both deleteSnapshot() and a now-batched pruneAutoSnapshots(). Also fixes 2 more test files with the same incomplete logger mock found on CI's first full run of this branch (store.test.ts, fileSystemService.test.ts — dbServiceRetry.test.ts was already fixed) and gives dbServiceSnapshots.test.ts's fake IDB store the same .transaction back-reference fix applied earlier to dbServiceBinder.test.ts, now that deleteSnapshot's transaction batching needs it. Collapses 2 more of my own QNBS-v3 comments that got wrapped across multiple lines (tests/setup.ts, protectedWriteAdmission.ts header) back to one physical line each.
qnbs
commented
Aug 12, 2026
@qodo-code-review — dispositions for the 1940fe9 push:
|
qnbs
commented
Aug 12, 2026
@coderabbitai review |
|
The
Overall, the implementation changes align with the stated dispositions; findings 2 and 3 should be treated as non-actionable rather than requiring further code changes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/dbServiceSnapshots.test.ts`:
- Around line 119-120: Update the fake transaction setup around getAllKeys and
the Promise.all(pending) completion logic so every request, including standalone
getAllKeys(), add(), get(), delete(), and cursor operations, is registered in
the transaction-specific pending list before completion is evaluated. Defer
invoking oncomplete until request registration has finished and the final
pending set has settled, ensuring requests added after the initial Promise.all
are awaited.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96263f4f-23cb-46af-bc1c-f2ac05fa2e49
📒 Files selected for processing (9)
services/storage/idbAssetStore.tsservices/storage/idbProjectStore.tsservices/storage/idbSnapshotStore.tsservices/storage/protectedWriteAdmission.tstests/setup.tstests/unit/dbServiceSnapshots.test.tstests/unit/fileSystemService.test.tstests/unit/storage/protectedWriteAdmission.test.tstests/unit/store.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/setup.ts
- services/storage/protectedWriteAdmission.ts
- services/storage/idbProjectStore.ts
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ck, more CodeRabbit's fresh review found real findings beyond the earlier qodo pass: - aiInferenceCacheService.ts's reencryptLegacyEntry() (opportunistic re-encrypt on legacy read) called encodeEntry/persistEntry without withProtectedWriteAdmission — the exact race this PR closes for every other write path. Now shares the same admission boundary. - idbProjectStore.ts's deleteProject() re-checks assertIdbProtectedWriteAllowed immediately before the final project-record delete, not just once at the top — the binder-asset cascade under the same admission hold takes real async time, during which a Lock Session (not a migration, which admission already excludes) could still fire. - protectedWriteAdmission.ts: runtimes without navigator.locks now get a real in-process (same-tab) reader/writer mutex instead of running unguarded — weaker than Web Locks (no cross-tab protection) but strictly better than no admission at all. Same fairness contract (queued exclusive blocks new shared) as the Web Locks path and the test mock. - tests/setup.ts's Web Locks mock now defaults the 2-arg request() form to 'exclusive' per the real LockManager.request() spec (was defaulting to 'shared') — dead code today since production call sites always pass an explicit mode, but closes a latent fidelity gap CodeRabbit's spec lookup caught. - idbSnapshotStore.ts: moved createSnapshot's wordCount computation (pure CPU work) outside the admission window. - Collapsed one more of my own QNBS-v3 comments that stayed wrapped across two lines despite the earlier fix pass (aiInferenceCacheService.ts), and added one that was missing entirely (sceneRevisionService.ts's deleteRevision). - dbServiceSnapshots.test.ts's fake transaction now tracks getAllKeys()'s own request into its pending list too, so oncomplete cannot theoretically fire before it settles, even though it's always a standalone transaction in production code. CodeRabbit's deep dive into tests/setup.ts's fairness fix ran a Node simulation against what turned out to be a stale/cached view of the mock (matching the pre-fix version) — verified the actual current code already has the queued-exclusive-blocks-new-shared fix from the prior commit.
qnbs
commented
Aug 12, 2026
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/storage/protectedWriteAdmission.ts`:
- Around line 31-60: Reserve fallback ownership in fallbackWakeNext before
resolving any waiter: set fallbackExclusiveHeld for an exclusive grant or
increment fallbackActiveShared for a shared batch, and pass each waiter its
pre-reserved release callback so acquireFallback does not re-check admission
after waking. Preserve fairness so queued leading shared waiters proceed
together while a queued exclusive waiter blocks later shared requests. Add a
fallback regression test with explicit barriers covering a writer arriving after
an exclusive request is queued.
In `@tests/unit/storage/protectedWriteAdmission.test.ts`:
- Line 116: Add a single-line QNBS-v3 why-comment directly above the test “still
excludes a writer from a migration batch via the in-process fallback lock,”
explaining that the regression test verifies writers remain excluded from
migration batches when the fallback lock is used.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e8074b2-a500-4812-8503-99c80bacbaad
📒 Files selected for processing (8)
services/ai/aiInferenceCacheService.tsservices/sceneRevisionService.tsservices/storage/idbProjectStore.tsservices/storage/idbSnapshotStore.tsservices/storage/protectedWriteAdmission.tstests/setup.tstests/unit/dbServiceSnapshots.test.tstests/unit/storage/protectedWriteAdmission.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- services/ai/aiInferenceCacheService.ts
- services/storage/idbProjectStore.ts
- tests/unit/dbServiceSnapshots.test.ts
- tests/setup.ts
- services/sceneRevisionService.ts
- services/storage/idbSnapshotStore.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…wake fallbackWakeNext() previously resolved a waiting caller's promise before updating fallbackExclusiveHeld/fallbackActiveShared — the state update happened later, in the woken waiter's own continuation re-checking its while loop. That left a real gap: any other acquireFallback() call landing between release() firing wakeNext() and the woken waiter's continuation actually running would see the lock as free and could be admitted too. Restructured so ownership is always claimed synchronously at the moment a grant decision is made — either immediately in the new fallbackTryClaim() fast path, or inside fallbackWakeNext() itself before it resolves a waiter's promise. A caller can no longer observe a state where the lock looks free but hasn't actually been claimed by anyone.
qnbs
commented
Aug 12, 2026
@coderabbitai review |
|
…ryption stack (#340) * docs: close out PR #310 reconciliation ledger — SUPERSEDED #335/#336/#337 are all now merged into main (4fadbd7), the one condition this ledger's merge decision previously left open. Every commit/behavior reconciliation row already had a final disposition; independently re-verified the deeper PR310-R010-R016 hardening checkpoint's cited test names against main's current tree — all exist and pass. Updates the status banner (was stale, claiming the tables were still outstanding when they were already complete) and the merge-decision section to record closure, with an explicit note that the disable/rotate production wiring B006/B007 describe is separate Phase-4 work (issue #338), not yet done. * docs: housekeeping pass — CHANGELOG + AUDIT sync for the encryption/desktop stack CHANGELOG.md: comprehensive [Unreleased] entry for #335 (fail-closed IDB encryption lifecycle), #336 (desktop AI/Python hardening for #332/#333), #337 (durable resumable migration journal + secondary-store adapters), #339 (cross-tab write-admission closing the migration TOCTOU race), plus the #310 ledger closure and #338 tracking issue. AUDIT.md: appended this session's entry to the follow-up chain and a new quality-gate line; corrected the stale "2869 keys" figure in the current- version summary line to the actual 2904 (already true since an earlier commit this session, just never synced here) — node scripts/check-doc- metrics.mjs now genuinely passes rather than coincidentally matching. * docs: tighten grammar per Sourcery nitpick
Uh oh!
There was an error while loading. Please reload this page.
User description
Summary
Fixes issue #338 items 3 and 4: the encryption-migration-vs-protected-write race that CodeRabbit and Qodo both independently flagged on #337.
assertNoActiveEncryptionMigration()was a standalone read — a migration could claim ownership and commit in the gap between a protected writer's key resolution and its transaction commit, landing ciphertext under a superseded key/generation.ProtectedStoreVerificationShortfallError.What changed
services/storage/protectedWriteAdmission.ts: a Web Locks API (navigator.locks) shared/exclusive reader-writer lock. Ordinary protected writers hold it in shared mode for their full key-resolution-through-transaction-commit span; a migration batch holds it exclusively for one batch (~25 records), not the whole run — bounding writer starvation while closing the race exactly where it matters.idbAssetStore.ts(saveImage,saveBinderAsset,deleteAllBinderAssetsForProject,deleteImage,deleteBinderAsset),idbCodexStore.ts(saveStoryCodex,saveRagVectors,deleteStoryCodex),idbProjectStore.ts(saveSlice,deleteProject),idbSnapshotStore.ts(createSnapshot,deleteSnapshot).protectedStoreMigration.ts's per-batchadapter.migrateNext(...)call in the exclusive variant.aiInferenceCacheService.ts's durable-write section,sceneRevisionService.ts's retention/delete) — this makes the deletion-vs-verification race structurally impossible rather than needing a separate comparison-against-surviving-records fix, closing both Phase 4: wire encryption migration journal into production (disable/rotate flow, secondary-adapter startup, write-admission race, verification-deletion race) #338 items 3 and 4 with one mechanism.navigator.locks; falls back to running the callback directly (today's behavior) with a one-time warning on runtimes that lack it.navigator.locksmock totests/setup.ts(neither jsdom nor Node implement it) and fixes an incompleteloggermock indbServiceRetry.test.tsthat broke once the admission wrapper'screateLogger()call entered its import chain.Test plan
tests/unit/storage/protectedWriteAdmission.test.ts(new) — shared holders run concurrently, exclusive excludes/waits on shared and vice versa, result/error propagation, fallback whennavigator.locksis absent.dbService*.test.ts,sceneRevisionService.test.ts,aiInferenceCacheService.test.ts,protectedStoreMigration.test.ts,tests/unit/services/storage/idb*.test.ts.services/loggerfor a transitive break — onlydbServiceRetry.test.tsneeded the fix above.pnpm run typecheck:singleclean.pnpm run lintclean.Part of the Phase 4 stack for #338; PR B (disable/rotate production flow, primary-store adapters, UI) is based on this branch.
🤖 Generated with Claude Code
Summary by Sourcery
Introduce a cross-tab admission mechanism to coordinate protected IndexedDB writes with encryption migrations, preventing races between key resolution, writes, and migration batches.
New Features:
Bug Fixes:
Enhancements:
Tests:
CodeAnt-AI Description
Prevent encryption migrations from conflicting with protected storage updates
What Changed
Impact
✅ Fewer encryption-migration write conflicts✅ No silent data loss for small codex records✅ Fewer false migration verification failures💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Reliability
Bug Fixes
Tests