fix: harden rotation soundness under concurrency and crash-resume#596
Merged
FSM1 merged 43 commits intoJul 8, 2026
Merged
Conversation
…ncurrency Entire-Checkpoint: bccfa5fe2c9d
Entire-Checkpoint: 83cbbccad208
Entire-Checkpoint: 91dda672b133
Entire-Checkpoint: 086bedc26077
Author RED-phase unit tests for the rotation-only three-way merge covering local-wins-on-conflict, remote-only-add-included, base-only-omission-dropped, and the documented concurrent-delete resurrection residual accepted in T-70-02. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 589efbb59e0f
Add the rotation-only three-way merge as an isolated pure function so the local-wins conflict policy is syntactically impossible to invoke from a non-rotation call site, closing the merge-downgrade Elevation-of-Privilege gap (T-70-01). Re-export from the rotation barrel; folder/merge.ts's remote-wins mergeChildren is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 3adad415a36f
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: f88550437dd6
Entire-Checkpoint: f30e4b1b3738
…decarFloorStore Adds three tokio tests against the current unsynchronized floor store: two concurrency tests spawning N tasks against a shared Arc-wrapped store to prove no lost updates on same and different node_ids, and a corrupt-sidecar test proving load_map currently degrades a present-but-corrupt sidecar to a silent empty map instead of failing closed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 1041d54f87c6
…p_floor JsonSidecarFloorStore::get/put now hold a tokio::sync::Mutex around the whole load-modify-write critical section, with all blocking filesystem I/O moved into tokio::task::spawn_blocking so the executor never blocks while the lock is held. put computes max(existing, candidate) inside that locked section (mirroring the already-correct TS idbPut pattern), so concurrent puts on the same or different node_ids can never lost-update each other. A present-but-unparseable sidecar now fails closed: get reports every node under that store as maximally floored (a bounded i64::MAX sentinel, never u64::MAX which would wrap negative and defeat the regression check) instead of silently degrading to an empty cold-start map, and put refuses to write over the corrupt file rather than clobbering other nodes' floors. This required no HighWaterStore trait signature change, so listing.rs/adapter.rs consumers are unaffected. high_water.rs's bump_floor call sites (bump_generation, bump_seq, seed_from_grant, and enforce_resolved's step 4) are now guarded by a per-instance tokio::sync::Mutex so this orchestration layer's own read-compare-write window cannot interleave against a concurrent bump, on top of the store's own authoritative atomicity. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 74943bdeb759
…igh-water Documents that this orchestration layer's bumpFloor is intentionally non-atomic on both the TS and Rust twins, and that atomicity is owned by the concrete HighWaterStore adapter -- idbPut already reads the existing value back inside the same IndexedDB readwrite transaction and writes Math.max(existing, value), matching the just-hardened JsonSidecarFloorStore::put (SC#5). No functional TS change was needed; idbPut was already correct. Also records the cross-store bump sequencing in enforceResolved (generationStore then seqStore, awaited sequentially) as an explicit doc-only residual: both floors only ever rise, so a partial failure between the two writes is under-protective, never rollback-accepting, and does not need an atomic multi-store transaction to satisfy this phase's SC#5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: d734ed785a7c
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: d1d9629f3efe
Entire-Checkpoint: 7a9f762e0c62
Replaces the module-global activeRootNodeId scalar with a Set so concurrent rotations on different root node ids no longer clobber each other's badge state — the badge now resets only when the per-root tracking set drains. Also caches the job-checkpoint IndexedDB connection instead of opening a fresh one on every persistJob call, invalidating the cache on onversionchange/close. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: da28f60842d9
Adds a web-e2e case driving the real rotation-driver.service.ts persistJob callback for two distinct root node ids overlapping in time, proving the badge stays active until BOTH roots finish rather than resetting when the first root completes. Static-typechecked only per SC#6; full Playwright run is deferred to the stack-gated wave/phase verification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 57b9f3ee670b
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 858ee460f2ca
Entire-Checkpoint: ce55e4e35d9c
… sites
Extends the CAS-409 concurrent-add merge describe block with three
assertions that fail against the current remote-wins mergeChildren
implementation:
- local wins on conflict (rotated child new-key seal survives a merge
against a remote still holding the pre-rotation seal)
- rotateOne returns the CAS-merged children (including a remote-added
child), not the pre-merge node.children snapshot
- mergeConcurrentChildren exposes { published, mergedChildren }
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Entire-Checkpoint: 4d392e4bf0c7
mergeConcurrentChildren now delegates to mergeRotatedChildren (local
wins on conflict) instead of the generic remote-wins mergeChildren,
and returns { published, mergedChildren } so rotateOne can capture the
plaintext merged set. rotateOne's CAS-409 merge closure captures the
result into an outer-scope mergedChildrenForReturn (mirroring
registration.ts's currentWriteChildren idiom), and the final return
now hands back the merged children instead of the pre-merge
node.children snapshot, so a concurrently-added child is available to
the BFS caller.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Entire-Checkpoint: 90bfe0dbcf23
updateFolderMetadataAndPublish gains an optional mergeChildrenFn param (defaults to the existing remote-wins mergeChildren so every non-rotation caller is byte-unchanged). ParentTrackingState in engine.ts gains a baseChildrenSnapshot field, captured at parentTracking.set time before any child-driven mutation. Both D-09 batched-republish call sites in rotateReadFromNode now pass mergeChildrenFn: mergeRotatedChildren and the captured snapshot as baseChildren, and diff the republish's returned publishedChildren against the pre-call snapshot to enqueue any concurrently-added child onto the BFS frontier for its own rotateOne pass. folder/merge.ts's mergeChildren remote-wins default is unchanged (verified via git diff --stat). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: c5f8debd4990
Entire-Checkpoint: fe6b5fa7f885
Adds a depth-2 fixture (root -> subfolder -> grandchild) asserting a dirty grandchild edge is detected with a usable engine-derived nodeReadKey, a missing-root record surfaces as dirty rather than silently clean, and a fully-clean multi-level tree returns no dirty frontier. Fails against the current depth-1-only implementation. Also adjusts two pre-existing depth-1 verifySubtreeClean fixtures (their clean child edges default to kind folder) to mark the child as kind file, since the upcoming recursive implementation will otherwise attempt to recurse into them against a static unsealNode mock that returns the same root node for every call, hanging the suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 52b95cfc040a
verifySubtreeClean now walks the FULL subtree instead of only the
root's immediate children: it recurses into every clean folder edge to
find dirty edges at any depth, and treats a missing root IPNS record
as dirty rather than silently returning clean. Recursion stops below a
dirty edge since the derived key there is the child's stale
pre-rotation key and cannot unseal its current body — there is no
cryptographic recovery path for a key genuinely lost to an
interrupted prior run.
The returned frontier item shape is now { ipnsName, nodeId,
parentIpnsName, nodeReadKey, childPubKind, enqueuedGeneration } so a
dirty node discovered at any depth carries everything a BFS consumer
needs to seed its queue directly, without re-deriving keys under a
depth-1 assumption.
Extracts two shared key-chain-walk helpers (resolveAndFetchNode,
resolveChildKeyAndEnvelope) used by both verifySubtreeClean and the
main BFS in rotateReadFromNode, replacing four previously-duplicated
resolve+unseal call sites with a single implementation. The stale
docstring claiming fresh-record resume depends on an unwired Phase-68
durable floor is corrected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Entire-Checkpoint: c3c478c03bea
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 4717cc9b3472
Entire-Checkpoint: 3ba5156f940c
…, grant reachability, fail-closed accounting, and fresh-copy dirty-resume - Extends engine.test.ts with 5 new behaviors for rotateReadFromNode's entry gate - Rewrites the stale 64-07 no-double-bump convergence-guard test into a safe double-rotation test (design 4.5) since the old skip semantics are being replaced - Adjusts two pre-existing dirty-resume fixtures whose static unsealNode mocks collide node identity once the child is genuinely re-entered via rotateOne Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 99f52da47a53
…leError, grant threading, and fresh-copy dirty-resume result - rotateReadFromNode's entry gate now probes root-unseal viability with the supplied rootReadKey BEFORE deciding fresh rotateOne(root) vs dirty-tail recovery, regardless of completedNodeIds.size; a stale key throws a distinct RootKeyStaleError (re-exported via rotation/index.ts and sdk-core index.ts) instead of a generic AEAD failure, documenting the genuinely-lost-root-key window as an accepted residual with no cryptographic recovery - verifySubtreeClean-driven dirty-tail detection now runs unconditionally (not gated on completedNodeIds.size) using the pre-rotation rootReadKey, consuming plan 70-05's key-bearing DirtyFrontierItem frontier to seed the BFS queue directly, even below a would-be convergence-skipped ancestor - Removed the ROT-06 no-double-bump convergence guard from the BFS dequeue loop; every queued item is rotated via the normal rotateOne call (design 4.5 safe double-rotation) instead of being skipped when already ahead of its enqueued baseline generation - Added grantCallbacks/innerGrants to RotationParams, threaded to every rotateOne call site (root and BFS loop) so reMintGrantsRootedAt is reachable from the real walk, not only via direct rotateOne injection - A missing IPNS/envelope record on the frontier (root children enqueue, dirty-resume seeding, grandchildren enqueue) is now fail-closed accounted via a shared decrementPendingAndMaybeRepublish helper instead of a silent continue that desynced pendingChildCount and stalled the parent's D-09 batched republish forever - The dirty-resume-republish path returns a truthy RotateReadResult whose readKey is a fresh Uint8Array copy of the caller-supplied rootReadKey, never an alias of the caller-owned buffer - collectDirtyFrontier zeroes a clean edge's derived readKey once fully processed (D-09 terminal-owner hygiene); a dirty edge's key is left untouched since it survives into the returned frontier Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 016a3d7afcc0
Entire-Checkpoint: a86120c1152e
…ScopeExitRotation - performScopeExitRotation now zeroes rotationResult.readKey after the folderTree defensive copy is taken, closing the SC#6 gap where the engine-returned readKey was never zeroed by its caller - safe because plan 70-06 guarantees rotateReadFromNode always returns a fresh copy, never an alias of the caller-owned rootReadKey - extends client-rotation.test.ts with paired zeroization assertions (owner-zeroed, folderTree-copy-unchanged, caller-rootReadKey-unchanged) and fixes the pre-existing folderTree-refresh test to snapshot expected bytes before the call, since rotatedReadKey is now zeroed in place Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: c129af71e03c
…igation - performScopeExitRotation's rotate() now catches RootKeyStaleError from rotateReadFromNode specifically (typed via the sdk-core barrel, not a string match) instead of letting it fail the already-published mutation - on catch, drops the stale folderTree entry and re-navigates top-down from the vault root via ensureFolderLoaded to rediscover the current key via the parent chain (Open Question 1's recommended recovery path) -- no cryptographic key recovery is claimed, only re-derivation via the chain - when re-navigation also fails to recover the root (Open Question 2's pure-revoke ancestor-mirror residual can block this one hop earlier), surfaces a clear, actionable error instead of a generic AEAD failure - adds client-rotation.test.ts coverage for the recovered, unrecoverable, and non-RootKeyStaleError-passthrough cases Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 8a7fbdffdfde
…d child Test 3 previously only checked child IPNS names survived the CAS-409 merge, which masks the merge-downgrade bug where remote-wins would leave subfolder3's SealedChildRef pointing at its stale pre-rotation key. Now derives subfolder3's key via unsealChildReadKey against the new root key and unseals its actual published body, plus asserts the concurrent-write injection callback genuinely ran so the test cannot silently pass without exercising the CAS-409 merge path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: bc0cc5a3cdc4
Adds a new e2e case proving SC#3's fresh-record resume: rotation crashes immediately after the root's own commit (earlier than the existing final-persist crash), then resumes with a brand-new RotationJobRecord (empty completedNodeIds, not seeded from crash-time state) and the current valid rootReadKey. The root is a single file with no children, since any node whose own D-09 batched parent-republish has not fired leaves its children's SealedChildRef entries wrapped under a pre-rotation key that cannot be re-derived from the node's post-rotation key - a childless root avoids that unrecoverable window while still proving the entry gate converges via safe double-rotation and cuts the pre-rotation grant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: b3bcd4718723
…me e2e plan Entire-Checkpoint: a34ada8e34e8
Plan 70-04's enqueueConcurrentlyAddedChildren over-reached ROT-05 by pushing a concurrently-added child onto the BFS queue for its own rotateOne pass, which required an IPNS write key the rotating party may not hold, and ran after parentTracking teardown so the parent's SealedChildRef readKeySealed was never repointed at the new key. Replace it with createConcurrentAddResealingMerge, an async mergeChildrenFn wrapper invoked inside the D-09 batched republish's CAS-409 merge. It merges the concurrent child via mergeRotatedChildren and re-seals only its readKeySealed wrapper under the parent's new readKey, trying both the old and already-current key since a concurrent writer may have used either depending on race timing. publishWithCas and updateFolderMetadataAndPublish already support an async merge callback; widen mergeChildrenFn's type to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 6fb006865be5
Documents the 70-04 over-reach diagnosis and correction (commit 7faa0e82835d56368ea87f969d57b083d43ea9a3), plus the live sdk-e2e rotation-crash-safety gate result (4/4) and sdk-core unit result (355/355). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 40313205f74b
Phase 70 rotation soundness verified 6/6 (VERIFICATION.md). Live sdk-e2e rotation-crash-safety gate 4/4; unit sdk-core rotation 86/86, sdk client-rotation 29/29, Rust floor_store 9/9 + high_water 10/10. VALIDATION.md status flipped to passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: e6ebdea39e2a
WalkthroughThis PR implements Phase 70 "rotation soundness" fixes: a rotation-only local-wins merge policy for concurrent CAS-409 conflicts, a fully recursive dirty-subtree detection with richer resume frontier items, ChangesRotation Soundness (sdk-core, Rust, web, sdk client)
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
release:sdk-core:feat
Contributor
Release Preview
Cascade Details
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
==========================================
+ Coverage 65.89% 73.81% +7.91%
==========================================
Files 153 193 +40
Lines 14551 28412 +13861
Branches 1700 1731 +31
==========================================
+ Hits 9589 20971 +11382
- Misses 4717 7196 +2479
Partials 245 245
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…covery error CodeRabbit findings on Phase 70 branch. engine.ts leaked the nodeReadKey buffer for a dirty-frontier item that got deduped in enqueueDirtyFrontierItem, and left the dirty-resume loop's frontierItem.nodeReadKey unzeroed on every exit path since that buffer is never adopted into queue (a freshly re-derived childReadKey is adopted instead). client.ts swallowed the real ensureFolderLoaded recovery error behind .catch(() => null), so every RootKeyStaleError recovery failure was misreported as the known stale-key residual instead of surfacing the actual underlying error as cause. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 2004e720d51b
Crypto security audit: SECURED (0 critical/high/medium, 5 low residuals). Concurrent-add re-seal fix verified sound; local-wins confirmed load-bearing for revocation. Adds 70-LEARNINGS.md; reconciles STATE.md current-focus drift flagged by review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 6a37b1a18c0c
Greptile SummaryThis PR hardens read-key rotation across merge, resume, and durable floor handling. The main changes are:
Confidence Score: 4/5This is close, but the nested resume path should be fixed before merging.
packages/sdk-core/src/rotation/engine.ts
|
| Filename | Overview |
|---|---|
| packages/sdk-core/src/rotation/engine.ts | Expands rotation resume and merge handling, but the skipped-root resume branch can still drop dirty descendants below depth one. |
| packages/sdk-core/src/folder/registration.ts | Adds an async merge-policy hook for rotation-specific CAS conflict handling. |
| packages/sdk-core/src/rotation/merge.ts | Adds a rotation-only local-wins merge helper for child references. |
| crates/sdk/src/floor_store.rs | Serializes sidecar floor updates and fails closed on corrupt floor files. |
| crates/sdk/src/rotation/high_water.rs | Serializes high-water read, check, and bump operations. |
| packages/sdk/src/client.ts | Handles stale rotation keys with folder-tree recovery and zeroizes returned rotation keys. |
| apps/web/src/services/rotation-driver.service.ts | Tracks active rotation roots with a set and caches the job IndexedDB connection. |
Reviews (2): Last reviewed commit: "chore(70): defer depth>=2 crash-resume s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-RESEARCH.md (1)
428-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefresh the gap table or mark this note historical.
The test map and Wave 0 gap list still read as pre-implementation (
❌,Wave 0) even though the roadmap/state now mark Phase 70 complete. That leaves the same work described as both done and pending. Please either update the status or archive the note as historical research.🤖 Prompt for 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. In @.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-RESEARCH.md around lines 428 - 455, The Phase 70 research note still lists several tests and gaps as pending with Wave 0 markers even though the phase is now marked complete. Update the test map and Wave 0 gap list in 70-RESEARCH.md to reflect the current completed status, or move/archive the note as historical so it no longer conflicts with the roadmap; focus on the status markers and gap items rather than the test descriptions themselves..planning/STATE.md (1)
5-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one completion scope per progress block.
total_plans/completed_plansandpercentlook like they come from different denominators, so this state snapshot now mixes plan counts with a phase-completion percentage. Please split phase-level and plan-level progress, or derive both renderings from the same source field.🤖 Prompt for 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. In @.planning/STATE.md around lines 5 - 17, The progress snapshot in STATE.md is mixing phase-level and plan-level tracking, so update the progress block to use a single completion scope per block. Adjust the state representation around current_phase/current_phase_name and progress so total_plans/completed_plans and percent are derived from the same source field, or split them into separate phase-progress and plan-progress fields to avoid mismatched denominators.
🤖 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 `@crates/sdk/src/floor_store.rs`:
- Around line 62-75: The `load_map_blocking` fallback is too broad: it currently
treats every `std::fs::read` failure as `LoadOutcome::Empty`, which lets
unreadable existing sidecars bypass the fail-closed behavior. Update the `match
std::fs::read(path)` branch in `load_map_blocking` so only
`std::io::ErrorKind::NotFound` returns `LoadOutcome::Empty`; for other read
errors such as permission or I/O failures, log an error with the path and error
details and return `LoadOutcome::Corrupt` (or the appropriate fail-closed
outcome used by `load_map_blocking`/`LoadOutcome`).
- Around line 212-223: JsonSidecarFloorStore::put currently only logs failures
from the blocking write path, so callers like HighWaterStore::put and bump_floor
can still succeed even when the floor never reaches disk. Update put to
propagate the persistence error instead of swallowing it (including both the
Ok(Err(e)) and panic cases), and make enforce_resolved/floor update paths fail
closed if the write fails so the caller can observe the error.
In `@crates/sdk/src/rotation/high_water.rs`:
- Around line 266-272: The `enforce_resolved` flow in `high_water.rs` only locks
`bump_lock` around the final `bump_floor` calls, which still allows concurrent
reads of stale floors and can accept a rollback. Move the `bump_lock`
acquisition to cover the floor checks and the validation path for the
`generation_store` and `seq_store`, so the read-compare-write sequence is
serialized within `enforce_resolved`. Keep the existing final monotonic bumps
under the same guard so the whole check-and-bump operation stays atomic for a
given instance.
In `@packages/sdk-core/src/__tests__/rotation/engine.test.ts`:
- Around line 1798-1800: The mocks in `collectDirtyFrontier`-related tests are
returning shared module-level key buffers, which can be zeroized by the
implementation and corrupt the expected fixture. Update the `unsealChildReadKey`
mock in the `engine.test.ts` dirty-frontier and clean-tree cases to return fresh
copies for each call, and keep assertions against an immutable snapshot of the
expected key values. Reference the `unsealChildReadKey` mock setup and the
affected `collectDirtyFrontier` test cases to apply the same pattern
consistently.
In `@packages/sdk-core/src/rotation/engine.ts`:
- Around line 1395-1411: The dirty-frontier handling in verifySubtreeClean is
only looking up frontierItem.ipnsName against rootNode.children, which lets
depth-2+ dirty items be treated as missing and can finish without repairing the
real stale parent mirror. Update the dirty-resume flow around
verifySubtreeClean, frontier processing, and decrementPendingAndMaybeRepublish
so items are queued/released by parentIpnsName and only processed once the
matching parent tracking state exists; for non-root parents, carry or
reconstruct the needed ParentTrackingState instead of assuming root-level
children. Also adjust the normal frontier ordering so dirty items are not
rotated before their parent’s parentTracking entry has been created.
- Around line 655-697: The clean-edge childReadKey cleanup in
collectDirtyFrontier can be skipped if unsealNode or the recursive
collectDirtyFrontier call throws, leaving the derived key unzeroed. Move the
zeroing logic for childReadKey into a finally block around the clean-edge
processing in collectDirtyFrontier so it always runs after
resolveChildKeyAndEnvelope, regardless of whether the folder unseal or recursion
succeeds. Keep the existing dirty-edge early continue behavior intact, and
preserve the defensive Uint8Array check when zeroing the key.
In `@tests/sdk-e2e/src/suites/rotation-crash-safety.test.ts`:
- Around line 779-789: Clear all captured key material in the rotation
crash-safety test paths, including on assertion failures. Update the
unseal/assertion flow around unsealChildReadKey, unsealNode, and the
fresh-resume logic so owned buffers like sub3ReadKey, readKeyPrimeRoot4,
resumeResult4.readKey, and any entries in capturedReadKeys are zeroed in
try/finally blocks after use. Keep the existing assertions, but ensure every
buffer that stores sensitive read keys is always wiped even if a test fails.
---
Nitpick comments:
In
@.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-RESEARCH.md:
- Around line 428-455: The Phase 70 research note still lists several tests and
gaps as pending with Wave 0 markers even though the phase is now marked
complete. Update the test map and Wave 0 gap list in 70-RESEARCH.md to reflect
the current completed status, or move/archive the note as historical so it no
longer conflicts with the roadmap; focus on the status markers and gap items
rather than the test descriptions themselves.
In @.planning/STATE.md:
- Around line 5-17: The progress snapshot in STATE.md is mixing phase-level and
plan-level tracking, so update the progress block to use a single completion
scope per block. Adjust the state representation around
current_phase/current_phase_name and progress so total_plans/completed_plans and
percent are derived from the same source field, or split them into separate
phase-progress and plan-progress fields to avoid mismatched denominators.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 337ad58a-2e1a-4359-a670-7d6ee9caf9ea
📒 Files selected for processing (40)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-01-PLAN.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-01-SUMMARY.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-02-PLAN.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-02-SUMMARY.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-03-PLAN.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-03-SUMMARY.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-04-PLAN.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-04-SUMMARY.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-05-PLAN.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-05-SUMMARY.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-06-PLAN.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-06-SUMMARY.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-07-PLAN.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-07-SUMMARY.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-08-PLAN.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-08-SUMMARY.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-LEARNINGS.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-PATTERNS.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-RESEARCH.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-SECURITY.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-VALIDATION.md.planning/phases/70-rotation-soundness-deep-merge-fresh-record-resume-and-durabl/70-VERIFICATION.mdapps/web/src/services/rotation-driver.service.tscrates/sdk/src/floor_store.rscrates/sdk/src/rotation/high_water.rspackages/sdk-core/src/__tests__/rotation/engine.test.tspackages/sdk-core/src/__tests__/rotation/merge.test.tspackages/sdk-core/src/folder/registration.tspackages/sdk-core/src/index.tspackages/sdk-core/src/rotation/engine.tspackages/sdk-core/src/rotation/index.tspackages/sdk-core/src/rotation/merge.tspackages/sdk/src/__tests__/client-rotation.test.tspackages/sdk/src/client.tspackages/sdk/src/state/rotation-high-water.tstests/sdk-e2e/src/suites/rotation-crash-safety.test.tstests/web-e2e/tests/rotation-ux.spec.ts
RR-01 (concurrent-add downgrade), RR-02 (fresh-record resume/SC#4), floor-store atomicity, and the coderabbit rotation-soundness followups are closed by Phase 70. hardening-followups kept in pending with items 1 (cross-store atomicity) and 5 (reconcile cached generation) still open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 75291945dd52
- widen enforce_resolved's bump_lock to cover the whole floor read-check-bump sequence so two concurrent calls cannot both read a stale floor and accept a rollback under concurrency - fail closed on non-NotFound floor sidecar read errors instead of silently treating a present-but-unreadable file as cold-start, which bypassed the anti-rollback regression check - zero collectDirtyFrontier's locally-derived clean-edge read key in a finally block so a throw from unsealNode or the recursive call no longer leaves it live - return fresh copies from the engine.test.ts unsealChildReadKey mock so collectDirtyFrontier zeroing a derived key cannot mutate the shared fixture and mask a regression via a self-referential toEqual - wrap the sdk-e2e rotation-crash-safety key-material assertions in try/finally so an assertion failure still zeroes owned key buffers Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: cf513607cd76
PR #596 review (greptile P1 + CodeRabbit) found the dirty-frontier consumption path is depth-1-only and reuses a stale child key at dirty edges. Real but large/structural (needs new depth>=2 e2e + a key-source design decision entangled with the known Pitfall-4 limitation). SC#2/SC#3 are proven for depth-1/childless-root as shipped; tracked for a follow-on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: f759f48de47c
FSM1
deleted the
feat/rotation-soundness-deep-merge-fresh-record-resume-and-durabl
branch
FSM1
added a commit
that referenced
this pull request
Jul 8, 2026
docs(roadmap): insert Phase 70.1 for rotation read-plane durability Deferred rotation read-plane debt from Phase 70 ship (PR #596 review): depth>=2 crash-resume soundness gap, floor-store write-failure propagation + cross-store atomicity, and the reconcile cached-generation gate. Retargets the depth-gap todo (resolves_phase 70.1) and folds hardening-followups items 1 + 5 into the phase. Depends on Phase 70. Entire-Checkpoint: b65e7c88bc74 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Phase 70 — Rotation Soundness: Deep Merge, Fresh-Record Resume, Durable Floor Concurrency
Closes the read-key rotation-soundness debt deferred across Phases 64/68/69. All six Success Criteria delivered and verified.
What changed
packages/sdk-core/src/rotation/merge.ts,engine.ts): a concurrent-add CAS-409 re-merge no longer downgrades a rotated child'sreadKeySealed. Wired at both merge sites (mergeConcurrentChildren+updateFolderMetadataAndPublish's inline D-09 merge).verifySubtreeClean: now recurses the full subtree (not just immediate children); missing root ⇒ dirty; key-bearing frontier at any depth.completedNodeIds.size; a freshRotationJobRecord+ current root key converges via safe double-rotation. DistinctRootKeyStaleErrorsurfaced (client falls back to top-down re-nav). No key recovery is attempted — a genuinely-lost root key has no cryptographic recovery by design (documented residual).grantCallbacks/innerGrantsreach bothrotateOnecall sites so inner-grant re-mint fires.crates/sdk/src/floor_store.rs): mutex held acrossspawn_blockingfs I/O, max-preserving, fail-closed on corrupt sidecar, atomic temp-rename, 0600 perms. TS/Rust parity asserted.packages/sdk/src/client.ts):activeRootNodeIds: Set<string>, cached IDB connection.Bug caught by the phase gate and fixed in this PR
The live
sdk-e2egate caught a real over-reach in the initial SC#1 implementation: enqueuing a concurrently-added child for its ownrotateOnepass required that child's IPNS write key (which the rotating party may not hold) and orphaned the parent pointer. Fixed to merge + re-seal the concurrent child'sSealedChildRefunder the parent's new readKey instead of rotating it — matching design section 4.5 ("picked up, full re-key is a follow-on"). See commitreseal not rotate.Gates
70-VERIFICATION.md, status passed).rotation-crash-safety: 4/4 against the docker + API stack (the SC#1/SC#3 round-trip proof).client-rotation29/29, Rustfloor_store9/9 +high_water10/10.Notes
.planningbookkeeping swept in during parallel planning) — harmless docs, Phase 69 code already shipped in#594..planning/todos/pending/for triage — the two "followups/hardening" bundles may hold residual items and were intentionally not auto-closed.Post-review disposition (PR review resolved)
Two reviewers (greptile + CodeRabbit) raised 10 threads, all triaged and resolved:
Fixed in
98a6265b7:high_water.rs— CRITICAL:bump_locknow wraps the whole read/check/bump (was bump-only) — closes a concurrent-resolve anti-rollback TOCTOU.floor_store.rs— fail closed on unreadable sidecars (onlyNotFound-> empty; other read errors no longer become cold-start).engine.ts— zero the clean-edgechildReadKeyin afinally.try/finallyaround e2e key captures.Deferred (tracked todos, noted here for transparency):
.planning/todos/pending/2026-07-08-rotation-crash-resume-depth2-soundness-gap.md. Reviewer note before merge.HighWaterStore::putsignature change; folded into the existing rotation-hardening-followups todo.🤖 Generated with Claude Code