feat: cancel an upload and collect orphaned staged blocks

[FSM1]

Problem

The staged-block lifecycle had two holes on the #813/#825 write-plane line.

A user could not abandon an upload. blueprint/web-client.md requires "cancel is a facade command killing the op before publish", but no Command variant existed and the op queue had no cancel path — the only remedy for a queued upload was a compensating delete, which still pushes the whole file through the network and never returns the staging budget.

And orphan_staging_keys had no production caller, so blocks nothing references — the residue of a crash between staging a version and journaling its op — accumulated against the staging budget forever. #853 recorded the trap that arms the moment GC gets one: the cold-start dead-letter path removes the op record, which is exactly what turns the preserved staged bytes of a dead letter into collectible orphans.

Change

Cancel. Command::CancelUpload { op_id }, content-only — for a metadata op a compensating mutation is already equivalent. Guaranteed until publish entry and refused after with TooLateToCancel, so a cancel can never mutate published state. The drain yields at every block boundary, without which the guarantee collapses to "cancel only works before the op starts". A small interlock decides the race in one non-awaiting borrow each way: either the facade claims the op and the drain abandons its upload, or the drain claims it for publish and the cancel is refused. The claim is sticky across a failed publish attempt — a PUT that did not confirm may still be live at the name. Staged bytes are released, under the rule that splits release from preservation: preserved when the engine gave up on the op, released when the user did. A cancelled create takes every later queued op on its node; a cancelled UpdateContent takes nothing. The host sees OpPhase::UploadCancelled on the existing progress event.

Only the blocks this session's drain confirmed on the network are retired. A block an earlier session sent cannot be told apart from one a version that has since published still names, and unpinning that is loss where leaving the row charged is only a leak (#916).

Orphan GC. First production caller, at cold start and after each drain pass. Roots expand into their leaf sets; open write handles are excluded by CID, recorded before their bytes are staged. A sweep spans many awaits, so it reads a generation counter — bumped by every open and every recorded key — and abandons the pass if the live set moved under it. An unexpandable root freezes the whole pass, self-clearing via the drain's permanent classification.

The four-way staged-byte lifetime is now reconciled in one place, crates/engine/src/sync/staging.rs: preserved on a terminally unrebasable dead letter, released on cancel, on a proven-unopenable blob (#818), and on an unexpandable staged root. Preservation survives the cold start that drops the op record via a durable preserved set GC treats as a second reference source. That set holds the whole op record, not just the root CID — the record is the only carrier of the version's content key, so preserving the blocks without it would keep ciphertext no key ever opens, which is the condition that releases a version rather than the one that preserves it.

Client surface. Command.cancelUpload(opId) across the wasm boundary and packages/client, so #873 has something to call.

Tests

crates/engine/tests/write_plane.rs:

• a_cancel_mid_upload_releases_every_block_and_returns_the_staging_budget — the acceptance case. Fails without the block-boundary yield: the whole version publishes inside one poll and the cancel returns TooLateToCancel.
• a_cancel_after_the_version_published_is_refused, a_cancel_of_a_metadata_op_is_refused
• a_cancelled_create_cascades_onto_its_node_and_a_cancelled_version_does_not
• a_cancel_that_cannot_dequeue_retires_nothing_and_leaves_the_op_publishable — drop the withdraw call and the op is wedged: claimed, still queued, halted forever. Retire before the dequeue instead and the resumed version publishes with its leading leaves unpinned.
• an_undecodable_record_never_authorizes_deleting_the_blocks_its_header_names — the clear header is unauthenticated and carries a public owner tag, so a co-tenant of the origin-shared store can plant one; the test asserts the forgery is dead-lettered and the version it names survives and still publishes.
• orphan_residue_is_collected_and_a_live_handles_blocks_are_not — drop the live-set exclusion and the handle's commit publishes a version whose blocks were swept.
• a_dead_lettered_ops_blocks_survive_a_cold_start_and_a_gc_pass — drop preserve_dead_letter and the first GC pass eats them.

Unit: sync/cancel.rs pins the interlock both ways, the sticky claim, and the session-scoped retire set; sync/staging.rs pins the preserved-record round trip, the prune, the whole-set release, that a preserved entry still reopens to its intent and sealed key, and the fail-closed freeze on a wrong tag, a length prefix past the end, and a zero-length entry.

Verification

All exit 0:

• cargo fmt --all --check
• cargo clippy --workspace --all-targets -- -D warnings
• cargo check --workspace --all-targets
• cargo clippy -p cipherbox-wasm --target wasm32-unknown-unknown --all-targets -- -D warnings
• cargo test --workspace — no failing suites
• pnpm -r --if-present run typecheck, pnpm -r --if-present run test, pnpm exec eslint .
• pnpm --filter @cipherbox/client test:browser — 19 passed

Review gates

/security-review raised two findings, both folded in:

• the GC live-set guard counted only handles opened, so an already-open handle staging its tail and root mid-sweep could have them collected — the counter now moves on every recorded key;
• the cancel retired the version's whole manifest, which in the crash window between an acked record PUT and the op's dequeue would unpin content a live record still names — the retire is now limited to the blocks this session's drain confirmed, and the publish claim is sticky across a failed attempt.

/crypto-privacy-review — run because the diff touches trust-boundary and fail-closed reads — raised two more, both folded in:

• the cold-start undecodable path deleted the blocks named by a record's clear, unauthenticated header. The owner tag on that header is a public key, and the staging store is shared per origin rather than per account, so a co-tenant could plant a record bearing our tag and destroy a queued version whose key was intact. Removing the op record is enough: orphan GC then reclaims on reference evidence, which is also what keeps a Retained record's block set safe.
• a preserved dead letter kept ciphertext nothing could ever open, because the abandonment deletes the op record that carries the version's content key. The preserved set now holds the record.

It confirmed the preserved-record codec is total, panic-free and fail-closed in the safe direction, the encode side release-active per rule 8; that a cancel can only ever address this session's own ops; that no key material is reachable from the shared LiveBlocks cell (rule 7); and that yield_now reads no clock or RNG.

/simplify findings folded in: staged_manifest collapsed into version_leaf_cids, the duplicated yield-then-check folded into cancel_checkpoint, the publish claim and the confirmed-block list collapsed into one in-flight record, the GC scan now enumerates staged keys first and short-circuits on an idle store, and the new visibility widenings and re-exports narrowed back to what is actually called.

Known and deferred: a co-tenant of the origin-shared store can freeze a GC pass with a planted undecodable entry, or delete/pad the preserved-set key. All are budget and availability, never confidentiality or loss, and they sit in the shared-store threat model blueprint/engine.md defers to queue-integrity work — noted here rather than fixed in this slice.

Rebased onto #941

#941 landed the #924 fix first, so this branch is rebased onto it. My earlier note said the conflict surface was "only the loop head" — that was wrong, and worth correcting: #941 inserts its conditional mark_uploaded in exactly the spot where this branch records a confirmed block, so it was a real body conflict in upload_blocks.

Resolved by keeping both of #941's properties intact — mark before release, and the mark written only when it advances — with the confirmed-block record placed immediately after upload_block, which is the moment it describes. All three of #941's tests pass unmodified; nothing in its semantics needed changing.

The cancel-vs-mark window. Neither of the two shapes it could take is reachable:

• Stranding a marked-but-unreleased leaf — the cancel releases every leaf the root manifest lists, mark or no mark, and the root block is staged throughout the upload loop, so the list is always complete. Orphan GC is a second net behind that: the op is dequeued, so nothing references the leaf.
• Releasing a leaf the mark still covers — mechanically yes, but not a hazard. The mark is only ever read back via upload_mark(root_cid), and the cancelled root can never be uploaded again: its op is gone, and a re-upload of the same file seals under a fresh random per-version key and so addresses differently. a_cancelled_versions_upload_mark_never_counts_towards_the_next_one pins that.

A third window was reachable, and #941's reorder is not what opened it: the upload the drain is already awaiting when a cancel lands confirms after the facade has snapshotted its retire batch, leaving one block charged with nothing to reach it — #916's failure mode, bounded to one block per cancel. The drain now retires the complete confirmed set on its Halt::Cancelled arm; retire is idempotent server-side, so the overlap with the facade's batch is a no-op. The cancel test asserts the union of both batches equals exactly the set of blocks that reached the network.

Orphan GC against the new ordering. No change needed. GC classifies by reference, never by the mark: a leaf in a queued op's root manifest is referenced whether or not the mark covers it, and a leaf whose op is gone is residue either way. UPLOAD_MARK_KEY stays in the always-referenced bookkeeping set. a_leaf_a_lost_release_stranded_on_a_cancel_is_reclaimed_by_the_next_sweep covers the residue shape #941 made reachable, on the one path where nothing re-runs the release.

Rebased onto #945 and #943

Rebased again onto main after #945 (focus-window folder resolve) and #943 (SIWE challenge below the facade). One conflict, in crates/engine/src/facade.rs, in two places — both genuine body overlaps, not adjacent insertions:

• the Engine::command match, where feat: resolve the focus window's folders below the scope root #945 replaced the SetFocus arm that previously fell through to Unimplemented and this branch adds CancelUpload. Both arms kept.
• the block after dequeue_op, where feat: move the SIWE challenge below the facade #943 added siwe_challenge and this branch added cancel_upload/discard_upload/collect_staging_orphans. Both kept.

#945's deliberate ordering is preserved: the resolve tick still refreshes the focus window before the drain, and this branch's orphan sweep still runs after it. No test of #945's or #943's was modified.

Does the focus refresh interact with the cancel? No, and the reason is structural rather than incidental: cancel_upload reads and writes only the durable op queue, the in-memory cancel interlock, the staging store and the registry. It never reads or writes self.snapshot, which is the only thing FolderRefresh mutates. So there is no half-updated base for a cancel to observe — and FolderRefresh holds no RefCell borrow across an await, so an interleaving cannot even contend.

The reverse direction is prevented by what the refresh merges: project_folder takes a folder's published children, and a cancel is refused once its record is publishing, so a cancelled node is one the parent's record cannot name. a_focus_refresh_never_renders_back_an_upload_the_user_cancelled holds that line end to end.

Two things that fixture had to get right, and that a future reader will trip on: the refresh needs the folder's own ipnsName, which only a resolved parent ref supplies — so a device that authored the folder itself never descends into it, and the test focuses on a second device. And a first attempt asserted nothing, because the pass the cancel interrupts has already run its focus refresh; the planted second-writer child therefore lands between two passes, and the assertion fails if the refresh never ran.

Closes #869
Closes #853
Part of #655

Summary by CodeRabbit

• New Features

  • Added upload cancellation through client, WebAssembly, and engine interfaces.
  • Cancelled uploads stop safely, clean up staged content, and report progress.
  • Added safeguards preventing cancellation after publishing begins.
  • Added orphaned-content cleanup while preserving data needed for recovery.

• Bug Fixes

  • Improved cleanup and recovery for failed, abandoned, or interrupted uploads.
  • Added clearer errors for invalid or too-late cancellation requests.

• Tests

  • Added comprehensive coverage for cancellation, cleanup, recovery, persistence, and large operation identifiers.

[coderabbitai]

Walkthrough

The engine adds pre-publication upload cancellation, cancellation-aware draining, live staged-block tracking, dead-letter preservation, orphan collection, network retirement, and WASM/TypeScript command support.

Changes

Upload cancellation lifecycle

Layer / File(s)	Summary
Cancellation contract and interlock crates/engine/src/facade.rs, crates/engine/src/sync/cancel.rs, crates/engine/src/sync/mod.rs	Adds Command::CancelUpload, cancellation errors, and UploadCancels to prevent cancellation and publication from claiming the same operation.
Staged-block ownership and orphan collection crates/engine/src/content/write.rs, crates/engine/src/sync/staging.rs, crates/engine/src/facade.rs	Tracks live write handles, preserves dead-letter records, releases complete staged versions, and collects orphaned blocks during startup and drain processing.
Cancellation-aware upload execution crates/engine/src/sync/drain.rs, crates/engine/src/facade.rs, crates/engine/tests/write_plane.rs	Checks cancellation at upload boundaries, retires confirmed network blocks, removes cancelled operations, emits cancellation progress, and tests cancellation and cleanup paths.
WASM and client command propagation crates/wasm/src/*, crates/fuse/src/error.rs, packages/client/src/facade.ts, packages/client/src/worker/*	Exposes cancelUpload, preserves bigint operation IDs, and maps cancellation errors for JavaScript and VFS callers.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant EngineFacade
  participant UploadCancels
  participant Drain
  participant Staging
  participant Network
  Client->>EngineFacade: cancelUpload(op_id)
  EngineFacade->>UploadCancels: request cancellation
  Drain->>UploadCancels: check at upload boundary
  Drain->>Staging: release staged version blocks
  Drain->>Network: retire confirmed uploaded blocks
  EngineFacade-->>Client: UploadCancelled or cancellation error

Loading

Possibly related issues

• A replayed UpdateContent op lets a cancel unpin a published version's content #966: Adds related upload cancellation and retirement handling for replayed UpdateContent operations.

Possibly related PRs

• FSM1/cipher-box#886: Adds the upload drain and staging lifecycle extended by this change.
• FSM1/cipher-box#923: Shares abandonment cleanup and staged-block retirement changes.
• FSM1/cipher-box#925: Shares upload lifecycle and cancellation progress handling.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the two primary changes: upload cancellation and orphaned staged-block collection.
Linked Issues check	✅ Passed	The changes implement the cancellation, staged-byte lifecycle, orphan GC, dead-letter preservation, client APIs, and acceptance-test objectives in [#869] and [#853].
Out of Scope Changes check	✅ Passed	The code, API mappings, client bindings, and tests directly support upload cancellation and orphaned staged-block collection.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/869-cancel-upload-orphan-gc

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

crates/engine/tests/write_plane.rs (1)

3107-3112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert both producers retired, not the exact batch count.

retire chunks its targets at RETIRE_BATCH_MAX, so the number of HTTP calls depends on the version size. A larger fixture would break this assertion without any behavior change. Assert batches.len() >= 2 instead, or assert that the drain's batch contains a CID the facade's batch does not.

♻️ Proposed change

-    assert_eq!(
-        batches.len(),
-        2,
+    assert!(
+        batches.len() >= 2,
         "a block confirming inside the facade's window is only covered by the drain's batch"
     );

🤖 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 `@crates/engine/tests/write_plane.rs` around lines 3107 - 3112, Update the
assertion in the retire_batches test to verify at least two batches are produced
rather than requiring exactly two, accommodating RETIRE_BATCH_MAX chunking while
still confirming both producers retired.

packages/client/src/facade.ts (1)

111-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the notAnUpload rejection.

cancelUpload() accepts any bigint operation ID, and the engine rejects metadata operations with NotAnUpload. The WASM boundary exposes this as notAnUpload. Add it to the public method contract so callers know both refusal outcomes.

Suggested documentation update

-   * with `tooLateToCancel` once the version's record is publishing.
+   * with `notAnUpload` for non-upload operations or `tooLateToCancel`
+   * once the version's record is publishing.

🤖 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 `@packages/client/src/facade.ts` around lines 111 - 117, Update the JSDoc for
cancelUpload() to document that it rejects with notAnUpload when the supplied
operation ID refers to a metadata operation, while preserving the existing
tooLateToCancel rejection description.

crates/fuse/src/error.rs (1)

87-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add regression coverage for both new internal mappings.

TooLateToCancel and NotAnUpload now use the host-side VfsError::Internal arm, but the conversion test does not exercise either variant. Add both variants to the test input so a future change cannot classify them as trust or storage verdicts.

🤖 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 `@crates/fuse/src/error.rs` around lines 87 - 88, Update the error conversion
test input in error.rs to include both EngineError::TooLateToCancel and
EngineError::NotAnUpload, ensuring each is covered by the VfsError::Internal
mapping assertion and cannot regress to a trust or storage classification.

🤖 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/facade.rs`:
- Around line 2107-2121: Update the cascade handling in the operation method
containing the shown discard logic so a failing cascade step emits
Event::SnapshotUpdated before returning the error. Preserve the existing event
emission on the successful exit, ensuring both the error and success paths
notify the host after the primary operation is removed.

In `@crates/engine/src/sync/drain.rs`:
- Around line 510-516: The cancellation path can retire uploaded blocks before
durable queue removal commits, allowing withdraw to restore an unsafe claim. In
crates/engine/src/sync/drain.rs:510-516, gate retire_cancelled in the
Halt::Cancelled handling on durable removal, or mark the in-flight record
terminal so the claim cannot be restored after retirement. In
crates/engine/src/sync/cancel.rs:51-56, update withdraw to reject claims whose
blocks were retired and have the facade translate that rejection into
TooLateToCancel.

In `@crates/engine/tests/write_plane.rs`:
- Around line 3155-3160: In both upload-cancellation tests, including
a_cancelled_versions_upload_mark_never_counts_towards_the_next_one and
a_leaf_a_lost_release_stranded_on_a_cancel_is_reclaimed_by_the_next_sweep,
assert uploads(&alice) > 0 immediately after the four poll_once calls and before
issuing Command::CancelUpload. Preserve the existing cancellation and
stranded-leaf assertions.

---

Nitpick comments:
In `@crates/engine/tests/write_plane.rs`:
- Around line 3107-3112: Update the assertion in the retire_batches test to
verify at least two batches are produced rather than requiring exactly two,
accommodating RETIRE_BATCH_MAX chunking while still confirming both producers
retired.

In `@crates/fuse/src/error.rs`:
- Around line 87-88: Update the error conversion test input in error.rs to
include both EngineError::TooLateToCancel and EngineError::NotAnUpload, ensuring
each is covered by the VfsError::Internal mapping assertion and cannot regress
to a trust or storage classification.

In `@packages/client/src/facade.ts`:
- Around line 111-117: Update the JSDoc for cancelUpload() to document that it
rejects with notAnUpload when the supplied operation ID refers to a metadata
operation, while preserving the existing tooLateToCancel rejection description.

🪄 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: d6a4fa61-847c-48aa-95b6-9056e9c69439

📥 Commits

Reviewing files that changed from the base of the PR and between 4567c33 and c6b5593.

📒 Files selected for processing (15)

• crates/engine/src/content/write.rs
• crates/engine/src/facade.rs
• crates/engine/src/sync/cancel.rs
• crates/engine/src/sync/drain.rs
• crates/engine/src/sync/mod.rs
• crates/engine/src/sync/staging.rs
• crates/engine/tests/write_plane.rs
• crates/fuse/src/error.rs
• crates/wasm/src/host.rs
• crates/wasm/src/lib.rs
• packages/client/src/facade.ts
• packages/client/src/worker/commandCodec.test.ts
• packages/client/src/worker/commandCodec.ts
• packages/client/src/worker/engineWasm.ts
• packages/client/src/worker/protocol.ts

💤 Files with no reviewable changes (1)

• crates/engine/src/content/write.rs

[FSM1]

CodeRabbit body-only nitpicks

The three nitpicks in the review body (no threads attached). All verified against the branch; two applied in 22edb2a, one declined.

crates/fuse/src/error.rs 87-88 — add regression coverage for both new internal mappings. Applied.

TooLateToCancel and NotAnUpload were added to the VfsError::Internal arm but host_side_failures_are_internal_and_never_trust_verdicts did not exercise either, so nothing stopped a later change reclassifying them as a trust or storage verdict. Both are now in the test's input set.

packages/client/src/facade.ts 111-117 — document the notAnUpload rejection. Applied.

cancelUpload() takes any bigint op id, the engine rejects a metadata op with NotAnUpload, and crates/wasm/src/host.rs:406 surfaces that as notAnUpload. Only tooLateToCancel was documented, so one of the two refusals was invisible to callers. Documented both.

Extended beyond the finding to crates/wasm/src/lib.rs:533, which carries the same contract text on Command.cancelUpload and had drifted the same way. The two are one contract stated in two places; fixing one and leaving the other is how they diverge.

crates/engine/tests/write_plane.rs 3107-3112 — assert batches.len() >= 2 instead of == 2. Declined.

The premise is that RETIRE_BATCH_MAX chunking makes the exact count fixture-dependent. RETIRE_BATCH_MAX is 1000 (crates/engine/src/net/retire.rs:14) and the fixture is 200 bytes — a handful of blocks. Reaching a second chunk from one producer needs a version of more than 1000 blocks, which no engine test builds, so the exact count is not fragile in the way described.

Loosening it also costs the assertion its meaning. What it pins is that two distinct producers retire — the facade against what it could see when the cancel landed, and the drain against the complete confirmed set — which is exactly what its message claims. Under >= 2 a single producer chunking twice would satisfy it, and the drain's batch could stop firing without the test noticing. That is the regression the line exists to catch, and it is now load-bearing: the drain's retire is gated on its own dequeue (thread on drain.rs), so an exact count is what keeps that path honest.

CodeRabbit rated this one 🔵 Trivial / 💤 Low value itself. Its stronger alternative — assert the drain's batch carries a CID the facade's does not — is a real improvement over both forms, but the set-equality assertion immediately below already covers the union, so the marginal gain does not carry its own churn.

---

Generated by Claude Code

[FSM1]

Rebased onto e9086c358 after #946 and #944 merged. Three conflicts, all resolved additively — the post-rebase diff against main is identical in shape to the pre-rebase one.

While tracing the two retirement paths that now coexist in drain.rs, a pre-existing hazard on this branch surfaced and is filed as #966: rebase_update_content has no AlreadySatisfied arm, so an UpdateContent op that crashes between its record PUT acking and dequeue_op replays on the next boot — and because UploadCancels is session-scoped, the publish-entry interlock no longer refuses a cancel. retire_cancelled then unpins content a live published record names.

Not fixed here: the fix is publish-state durability, not a rebase concern. #966 is ordered to land after this PR.

[coderabbitai]

🧹 Nitpick comments (1)

crates/engine/src/facade.rs (1)

2259-2270: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Route refused cancel retires into the drain retry set.

discard_upload discards the retire result, but cancelled content CIDs are already encoded as string CIDs. Add retire(api, &uploaded).await.is_err() handling here, or call the existing retire_cancelled pass so refused batches are not dropped with their pin rows charged.

🤖 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 `@crates/engine/src/facade.rs` around lines 2259 - 2270, Update the
cancel-discard flow around discard_upload to handle failed retire calls instead
of ignoring the result. When retire(api, &uploaded).await fails, route the
already encoded uploaded CIDs into the existing drain retry mechanism, or reuse
retire_cancelled, so refused batches remain eligible for retry.

🤖 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.

Nitpick comments:
In `@crates/engine/src/facade.rs`:
- Around line 2259-2270: Update the cancel-discard flow around discard_upload to
handle failed retire calls instead of ignoring the result. When retire(api,
&uploaded).await fails, route the already encoded uploaded CIDs into the
existing drain retry mechanism, or reuse retire_cancelled, so refused batches
remain eligible for retry.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8cdaa3c2-9bb6-41c5-82a5-64bb85221b0f

📥 Commits

Reviewing files that changed from the base of the PR and between c6b5593 and bed26ae.

📒 Files selected for processing (15)

• crates/engine/src/content/write.rs
• crates/engine/src/facade.rs
• crates/engine/src/sync/cancel.rs
• crates/engine/src/sync/drain.rs
• crates/engine/src/sync/mod.rs
• crates/engine/src/sync/staging.rs
• crates/engine/tests/write_plane.rs
• crates/fuse/src/error.rs
• crates/wasm/src/host.rs
• crates/wasm/src/lib.rs
• packages/client/src/facade.ts
• packages/client/src/worker/commandCodec.test.ts
• packages/client/src/worker/commandCodec.ts
• packages/client/src/worker/engineWasm.ts
• packages/client/src/worker/protocol.ts

💤 Files with no reviewable changes (1)

• crates/engine/src/content/write.rs

🚧 Files skipped from review as they are similar to previous changes (13)

• crates/engine/src/sync/mod.rs
• packages/client/src/worker/protocol.ts
• packages/client/src/worker/commandCodec.test.ts
• crates/wasm/src/host.rs
• packages/client/src/worker/commandCodec.ts
• crates/fuse/src/error.rs
• packages/client/src/facade.ts
• packages/client/src/worker/engineWasm.ts
• crates/engine/src/sync/cancel.rs
• crates/wasm/src/lib.rs
• crates/engine/tests/write_plane.rs
• crates/engine/src/sync/drain.rs
• crates/engine/src/sync/staging.rs

[FSM1]

Review of bed26ae70 triaged — one nitpick, body-only, no threads to resolve.

facade.rs:2259-2270 — route refused cancel retires into the drain retry set

Deferred to #966, not fixed here. The observation is accurate: discard_upload does let _ = retire(api, &uploaded).await; and a refused batch leaves pin rows charged. The best-effort behaviour is deliberate and documented at facade.rs:2249-2252, but the comment only rejects failing the cancel over a refused retire — it does not address the queue-and-retry option suggested here, and #944 landed exactly that pattern for head blocks in the session-lived orphan_heads vec. So the suggestion is reasonable on its face.

It is unsafe in isolation, though, because it widens #966. The cancelled set is content-addressed: a replayed UpdateContent op re-uploads byte-identical leaves and produces byte-identical CIDs. Today the retire fires while the op is definitively cancelled and dequeued. Deferring it to a later pass lets it land after a replayed op has republished, targeting CIDs a live record names.

The leak and the unpin pull in opposite directions — retrying harder fixes the first and worsens the second — and both are downstream of the same missing information, whether a queued op has already published. That is #966's subject, so the leak fix should land with whatever guard #966 settles on rather than ahead of it.

No code change on this PR.