fix(engine): retire the head block a publish orphaned before the transport - #944
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThe engine now tracks head CIDs from eligible failed publishes and retires them after drain processing. Tests cover retry-generated charged heads, registration refusal, live-head preservation, retirement failures, and quota cleanup. ChangesOrphan head cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 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 |
d179aca to
bba88f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/engine/src/sync/drain.rs`:
- Around line 1733-1752: Recheck the live held set in retire_orphan_heads
immediately before each destructive retire call, skipping any queued CID whose
head_cid is currently held. Update record_orphan_head’s doc comment to describe
only its enqueue-time live-set check, while preserving the existing orphan queue
cap and enqueue behavior.
🪄 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 Plus
Run ID: 65c5b663-f25a-4b3d-8dca-2e8e3342c0a3
📒 Files selected for processing (6)
blueprint/engine.mdcrates/contract/tests/contract.rscrates/engine/src/facade.rscrates/engine/src/net/retire.rscrates/engine/src/sync/drain.rscrates/engine/tests/write_plane.rs
Merge-order constraint with #946Verified by building the pairwise merge: this PR and #946 are each green against #946 deletes There is also a test-fake collision: both branches add Land #946 first, then rebase this PR. The rebase is small: repoint the import at After a correct hand-resolution the pair is fully green: Do not merge the two back-to-back on green checkmarks alone. |
…sport Every record publish uploads its head block through the same charged POST /content/upload a content block goes through, so each attempt creates its own accountable pin row. A publish that failed before the record reached the transport left that row charged against an account that could never reach the block again - and because the drain draws a fresh seal nonce per pass, every retry orphaned another one. Nothing retired any of them, for any op kind: #916 taught abandonment to retire the write name and the content blocks, and folder creates, deletes, renames and moves have no content blocks at all. The drain now notes the head CID of any publish that stopped short of the PUT fan-out - register-first, the floor read, the head-CID echo, a refused upload aside - and retires the set at the end of the pass that orphaned it, independent of the op's fate. An op that retries and then succeeds charged those rows just as surely as one that abandons. A fan-out that acknowledged nothing is deliberately not one of them: AllEndpointsFailed says no endpoint acked, not that none stored, and unpinning the head of a record a lost ack left resolvable is the loss #916 refuses, where leaving the row charged is only a leak. Closes #921
A refused upload charged no pin row, but a dropped connection or an unreadable 2xx body may have left one behind: the server committed the pin and the client never learned the address was live. The retry authors a fresh head, so nothing revisits that CID and the row is charged forever. Only a status answer is the server's own refusal. Transport and decode failures now orphan their head like every other pre-transport failure, and a unit test pins each arm - the AllEndpointsFailed arm above all, where retiring would be loss rather than a leak.
The orphan queue's only consumer physically unpins at refcount zero, so deciding from the publish error alone put a destructive step one careless caller away from unpinning a head a resolvable record names. The check now runs where the damage would be done: a CID the held set carries is refused outright, whatever the error said. Also drops EmptyHeadCid from the orphan set - it means nothing was ever addressed, so there is no CID to retire - and bounds the post-retire drain by the queue's own length. The contract leg declared its stand-in head blocks under the dag-cbor codec, which the ingress could not pin because 96 bytes of filler is not valid CBOR. It uses the same raw address the neighbouring version-set leg uses; the byte accounting the test asserts does not turn on the codec.
The doc comment claimed the destructive step checks the live set. It does not: the check runs at enqueue, in record_orphan_head. State the guard the code performs and why refusing a live head matters, and drop the claim about a check that lives nowhere.
d376272 to
275fda6
Compare
1 participant
Problem
Every record publish uploads its head block through
POST /content/upload— the same charged ingress a content block goes through — so each attempt creates its own accountablePinnedCidrow, sized and counted againstsumHostedBytes.A publish that failed before the record reached the transport left that row charged against an account that can never reach the block again. Nothing retired it, for any op kind: #916 taught abandonment to retire the write name and every content block, but
Drain::registered_bynames only those two, and folder creates, deletes, renames and moves have no content blocks at all.It compounded per attempt. The drain draws a fresh seal nonce per pass, so each retry re-authored byte-different head bytes under a different CID — an op that burned
ATTEMPT_BUDGETcould leave five orphaned charged rows, and an op that retried three times and then succeeded leaked three, with no abandonment ever coming to clean up.Change
crates/engine/src/sync/drain.rsnotes the head CID of any publish that stopped short of the PUT fan-out and retires the set at the end of the pass that orphaned it, independent of the op's fate.orphaned_head(&RecordPublishError)is the predicate — an exhaustive match, so a newPublishErrorvariant is a compile error rather than a silent default.Publish(Register | FloorRead)andHeadCidMismatch→ orphan. All three are raised with the head block already uploaded and no record on the wire.Publish(EmptyHeadCid)→ not an orphan: nothing was ever addressed, so there is no CID to retire.Upload(Transport | Decode)→ orphan. A dropped connection or an unreadable 2xx may have left a pin the server committed and the client never learned about; no record was PUT, so retiring is safe.Upload(Status | Unauthorized | …)→ not an orphan: a status answer is the server's own refusal, so it charged no row.Publish(AllEndpointsFailed)→ not an orphan. This is the decision the issue asked for on the acked-PUT arm.AllEndpointsFailedsays no endpoint acked, not that none stored; a lost ack leaves a record resolvable at the name pointing at that head, and unpinning it is the loss engine: retire every uploaded content block on abandonment, not only the version root #916 refuses, where leaving the row charged is only a leak. The security gate caught this — the first cut retired on that arm and would have turned a quota leak into an unreadable node.record_orphan_headrefuses any CID the live held set still names, whatever the error said. The queue's only consumer physically unpins, so the destructive step checks the live set itself rather than trusting its caller to hand it a freshly authored head.Engine::orphan_heads), session-lived, so a retire the registry refused goes out again on a later pass. It is capped atRETIRE_BATCH_MAXso a session whose retires keep failing bounds its leak, not its memory.Drain::runsplits intodrain_queue+retire_orphan_heads, so the retire fires on every exit from a pass, including the early ones.blueprint/engine.md"Retirement" gains the rule.Retiring eagerly, per pass, rather than accumulating a durable per-op list for the abandonment batch, is a deliberate divergence from the issue's literal wording. It covers strictly more: an op that never abandons still orphaned rows, and there is no new staging key, format tag, or pruning pass to keep correct. The observable outcome the issue asks for — every head CID an op's retries minted leaves the inventory, plus the write name on abandonment — is what the tests assert.
The issue's third bullet, "consider whether re-authoring per attempt is necessary at all", was evaluated and not taken: an unconfirmed retry re-PUTting its earlier bytes would need the authored record bytes made durable and the whole publish plan pinned across passes, while the drain re-derives its plan from the current gate-passing base each pass. That is a publish-pipeline change, not a retirement one.
Tests
crates/engine/tests/write_plane.rs::every_head_block_a_retrying_op_orphaned_leaves_the_inventory— a create whose register-first is refused orphans a distinct head per pass; each leaves the inventory on its own pass, every attempt's CID differs, and the abandonment then owes back the write name on top. Fails without the fix: the retire batch is only[write_name].crates/engine/tests/write_plane.rs::a_publish_that_reached_the_transport_never_retires_its_head— with every endpoint's PUT refused, the head block uploads and nothing is retired. Fails ifAllEndpointsFailedis flipped to orphan, which is the destructive direction.crates/engine/src/sync/drain.rs::only_a_publish_that_never_reached_the_transport_orphans_its_head— a unit test over every arm of the predicate, including the twoApiErrorshapes of a failed upload.crates/engine/tests/write_plane.rs::a_publish_that_never_confirms_dead_letters_once_its_attempt_budget_runs_out(pre-existing) still asserts an empty retire batch — the acked arm is unchanged.crates/contract/tests/contract.rs::every_head_block_a_retrying_publish_orphaned_retires_back_to_the_pre_upload_figure— the live leg: three attempts' head blocks upload and register under one name against the real API/Postgres/Kubo, each charging on its own; retiring only the last leaves the earlier two charged, and retiring the whole set returns the account to its pre-upload figure.Blocks::refuse_registeris new test scaffolding: the registration half of register-first refuses while retirement keeps answering, which is what puts a charged head block behind a publish that never reached the transport.Verification
All run in the worktree, all exit 0:
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo check --workspace --all-targetscargo check -p cipherbox-wasm --target wasm32-unknown-unknown --all-targetscargo test --workspace— 32 suites ok, 0 failedpnpm -r --if-present run typecheckpnpm -r --if-present run test— client 111, api 177, web 67, all passingnpx eslint .npx markdownlint-cli2 blueprint/engine.mdThe first Contract Suite run failed: the leg declared its stand-in head blocks under the
dag-cborcodec, which the ingress could not pin because 96 bytes of filler is not valid CBOR. It now uses the same raw address the neighbouring version-set leg uses — the byte accounting the test asserts does not turn on the codec.Review gates
/security-review— one MEDIUM: the first cut retired onAllEndpointsFailed, which would have unpinned the head of a record a lost ack left resolvable, turning a quota leak into an unreadable node. Folded in as theorphaned_headcarve-out plusa_publish_that_reached_the_transport_never_retires_its_head, which fails if the arm is flipped back./simplify— four comment/doc corrections (a cap justification that contradictedretire's own chunking, a doc block re-deriving the blueprint prose, a fourth restatement of the per-attempt fact, and a field doc describing the bug rather than the fix) and a merge of two near-duplicate write_plane tests. All applied.Entropyseam, prefixed into the sealed blob, so a repeat CID needs a 192-bit nonce collision), that both matches are exhaustive so a new error variant is a compile error rather than a silenttrue, that no key or plaintext material is retained, and that the retire batch tells the server nothing it did not already learn from the upload header. Three findings applied: the live-set guard above, droppingEmptyHeadCid, and bounding the post-retire drain by the queue's own length. One deferred as engine: retire the settings-record head block a failed publish orphaned #947.Deferred
#947 —
publish_settingspublishes the vault settings head through the same charged ingress and orphans it the same way, but runs outside a drain pass and holds none of its session state. Filed with a depends-on edge both ways: it reusesorphaned_head/record_orphan_head/retire_orphan_headsrather than adding a second mechanism.Parallel work
#920 is being implemented concurrently in the same registration/retire area and also extends #916. This branch does not touch
registered_by,registry_cids, or the registration batch shape; it addsorphaned_head/record_orphan_head/retire_orphan_headsbeside them and widensRETIRE_BATCH_MAXtopub(crate). Whichever of the two merges second should re-checkcrates/engine/src/sync/drain.rsandcrates/engine/src/net/retire.rsfor a semantic conflict, not just a textual one.Closes #921
Summary by CodeRabbit