fix(macos): stop cursor reconcile and mailbox waits on cancel

[SebTardif]

What Problem This Solves

Fixes an issue where stopping Share This Mac (or cancelling a share task) could leave cursor-reconcile retries and mailbox timeout waits running. Cursor reconcile treated CancellationError as a normal failure and slept with try?, so cancel became another backoff retry. The video mailbox spawned an unstructured timeout task that used try? sleep and was never cancelled when a frame arrived or the waiter went away.

Why This Change Was Made

Honor cancellation in both loops: break when reconcile or its sleep is cancelled, and cancel the mailbox timeout task on offer, finish, replace, and expire. Task.sleep now uses try await so cancel stops the wait.

User Impact

Share teardown no longer waits out a 5s backoff or a leftover mailbox timeout after the user stops sharing.

Evidence

Terminal output from the patched tree. Before the patch, a cancelled mailbox waiter sat for the full 5s timeout:

$ swift test --package-path macos/CrabfleetMac --filter mailboxCancelledWaiter
Expectation failed: (ContinuousClock().now - startedAt -> 5.002 seconds) < 0.5 seconds
Test mailboxCancelledWaiterReturnsPromptlyWithoutResumingTwice() failed after 5.002 seconds

After the patch the same waiter returns immediately:

$ swift test --package-path macos/CrabfleetMac --filter mailboxCancelledWaiter
Test mailboxCancelledWaiterReturnsPromptlyWithoutResumingTwice() passed after 0.001 seconds.
Test run with 1 test in 1 suite passed after 0.001 seconds.

Real behavior proof

• Behavior or issue addressed: Share cancel no longer retries cursor reconcile or leaves mailbox timeout tasks sleeping.

• Real environment tested: macOS, Xcode at /Applications/Xcode.app, swift-testing on arm64, branch fix/macos-cancel-hangs at 0b6a6cc.

• Exact steps or command run after this patch:

  export DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer
  swift test --package-path macos/CrabfleetMac --filter mailboxCancelledWaiter

• Evidence after fix: terminal output from the patched tree:

  $ swift test --package-path macos/CrabfleetMac --filter mailboxCancelledWaiter
  Test mailboxCancelledWaiterReturnsPromptlyWithoutResumingTwice() passed after 0.001 seconds.
  Test run with 1 test in 1 suite passed after 0.001 seconds.

• Observed result after fix: A cancelled mailbox next(timeout: 5s) returns in 0.001s instead of waiting the full 5s. Cursor reconcile treats CancellationError as non-retryable.

• What was not tested: Live ScreenCaptureKit session with a real display share and TCC grants.

Related: introduced in #90 (cursor reconcile) and #74 (mailbox timeout Task).

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed August 29, 2026, 5:09 AM ET / 09:09 UTC.

ClawSweeper review

What this changes

Updates Share This Mac’s cursor-retry loop and video mailbox so cancelling a share stops retry sleeps and pending timeout tasks promptly.

Regression provenance

Possible regression — probable (reviewed change; failure trace; known regression link). No predecessor PR is attributed.

Merge readiness

⛔ Blocked until real behavior proof from a real setup is added - 3 items remain

Keep open: current main still has both cancellation races, and the patch is a focused repair with no discrete source-level correctness finding. The supplied evidence is only an isolated Swift test on an earlier revision and explicitly omits a real ScreenCaptureKit teardown, so real macOS behavior proof is still required before merge.

Priority: P2
Reviewed head: 366c643d09065757834f54526855d6e451a9b69c

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The patch is focused and source-consistent, but merge readiness is capped by test-only proof for a production capture-teardown change.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: The changed production owners are the cursor reconciliation task and VideoMailbox.next; the supplied terminal evidence runs an isolated mailbox Swift test at earlier revision 0b6a6cc and explicitly omits a live signed ScreenCaptureKit teardown. It is useful regression coverage but not real after-fix Share This Mac behavior proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: The changed production owners are the cursor reconciliation task and VideoMailbox.next; the supplied terminal evidence runs an isolated mailbox Swift test at earlier revision 0b6a6cc and explicitly omits a live signed ScreenCaptureKit teardown. It is useful regression coverage but not real after-fix Share This Mac behavior proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	5 items	Current main still needs the repair: Current main retains an unstructured mailbox timeout using try? await Task.sleep and only then expires the waiter, leaving the cancellation-before-registration race addressed by this PR. Current main retries cancellation as an ordinary error: Current main catches every cursor-reconciliation error and ignores cancellation from the retry sleep, whereas the PR breaks on cancellation. Introduced mailbox ownership is race-aware: The PR records the timeout task under the same lock as the waiter, cancels it on offer, finish, replacement, and expiration, and cancels a task that loses the installation race.
Findings	None	None.
Security	None	None.

How this fits together

Share This Mac captures a Mac display and serves its frames to remote viewers. Capture configuration retries and the latest-wins video mailbox sit on the shutdown path, so their cancellation behavior affects how quickly a stopped share releases its work.

flowchart LR
  A[User stops Share This Mac] --> B[Capture shutdown]
  B --> C[Cursor reconciliation task]
  B --> D[Video mailbox wait]
  C --> E[Cancellation decision]
  D --> E
  E --> F[Share teardown completes]

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The changed production owners are the cursor reconciliation task and VideoMailbox.next; the supplied terminal evidence runs an isolated mailbox Swift test at earlier revision 0b6a6cc and explicitly omits a live signed ScreenCaptureKit teardown. It is useful regression coverage but not real after-fix Share This Mac behavior proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Resolve merge risk (P1) - The changed cancellation path has not been demonstrated on the reviewed head in a real signed Share This Mac session; the supplied body explicitly omits the ScreenCaptureKit/TCC scenario.
• Complete next step (P2) - Keep the PR open for contributor-supplied real macOS teardown proof; source review identifies no narrow mechanical repair for an automated worker.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production versus test delta	production +61/-15, tests +24, changelog +2	The concurrency repair is concentrated in two production owners and has focused unit coverage, but no end-to-end teardown proof.

Merge-risk options

Maintainer options:

1. Prove real share teardown before merge (recommended)
   Add redacted after-fix evidence that stopping a signed Share This Mac session promptly ends the capture teardown without a delayed retry or mailbox wait.

Technical review

Best possible solution:

Retain the narrow task-cancellation design and merge after a redacted real Share This Mac stop/cancel trace from the stable, Developer-ID-signed macOS app confirms prompt teardown.

Do we have a high-confidence way to reproduce the issue?

Yes—source inspection gives a high-confidence reproduction shape: cancel a task before VideoMailbox.next has registered its continuation, then observe current main’s unowned timeout wait. The current-main source also shows cursor reconciliation treating cancellation as a retryable error; this review did not execute the path.

Is this the best way to solve the issue?

Yes—the mailbox-owned timeout and explicit cancellation exits are the narrowest maintainable repair for the two observed waits. A real signed Share This Mac teardown trace is still needed to validate the production boundary.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 830832deb6f3.

Labels

Label justifications:

• P2: This is a bounded macOS Share This Mac teardown fix whose failure delays shutdown but does not establish a broader outage.
• merge-risk: 🚨 availability: Changing capture-task cancellation and mailbox timeout ownership can affect whether an active share shuts down promptly.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed production owners are the cursor reconciliation task and VideoMailbox.next; the supplied terminal evidence runs an isolated mailbox Swift test at earlier revision 0b6a6cc and explicitly omits a live signed ScreenCaptureKit teardown. It is useful regression coverage but not real after-fix Share This Mac behavior proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Current main still needs the repair: Current main retains an unstructured mailbox timeout using try? await Task.sleep and only then expires the waiter, leaving the cancellation-before-registration race addressed by this PR. (macos/CrabfleetMac/Sources/CrabfleetMac/VideoMailbox.swift:84, 830832deb6f3)
• Current main retries cancellation as an ordinary error: Current main catches every cursor-reconciliation error and ignores cancellation from the retry sleep, whereas the PR breaks on cancellation. (macos/CrabfleetMac/Sources/CrabfleetMac/MacScreenCapture.swift:421, 830832deb6f3)
• Introduced mailbox ownership is race-aware: The PR records the timeout task under the same lock as the waiter, cancels it on offer, finish, replacement, and expiration, and cancels a task that loses the installation race. (macos/CrabfleetMac/Sources/CrabfleetMac/VideoMailbox.swift:98, 366c643d0906)
• Feature-history provenance: History identifies the merged cursor-pipeline change as the prior major touch on the current reconciliation path. (macos/CrabfleetMac/Sources/CrabfleetMac/MacScreenCapture.swift:408, 6471f31a7c98)
• Proof is test-only and not at the reviewed head: The PR body supplies terminal output for one mailbox unit test at revision 0b6a6cc and states that a live ScreenCaptureKit share with TCC grants was not tested; it does not demonstrate the reviewed head’s signed-app teardown path. (macos/CrabfleetMac/Tests/CrabfleetMacTests/VideoPipelineTests.swift:381, 366c643d0906)

Likely related people:

• steipete: Peter Steinberger authored the merged cursor-pipeline work that history shows as the central predecessor on the reconciliation path, and also authored the related Share This Mac video-pipeline PR. (role: feature owner and recent area contributor; confidence: high; commits: 6471f31a7c98, ed2354d85c14; files: macos/CrabfleetMac/Sources/CrabfleetMac/MacScreenCapture.swift, macos/CrabfleetMac/Sources/CrabfleetMac/VideoMailbox.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Add redacted after-fix evidence from the Developer-ID-signed app installed at the stable /Applications/Crabfleet.app path while stopping Share This Mac.
• Update the PR body with that evidence; if a new review does not start, ask a repository writer to comment @clawsweeper re-review.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (24 earlier review cycles; latest 8 shown)

• reviewed 2026-08-24T14:12:01.814Z sha 0b6a6cc :: needs real behavior proof before merge. :: [P1] Remove the release-owned changelog line
• reviewed 2026-08-24T21:03:05.118Z sha 0b6a6cc :: needs real behavior proof before merge. :: none
• reviewed 2026-08-25T01:15:47.130Z sha 0b6a6cc :: needs real behavior proof before merge. :: [P1] Remove the release-owned changelog line
• reviewed 2026-08-25T07:16:31.353Z sha 0b6a6cc :: needs real behavior proof before merge. :: [P1] Remove the release-owned changelog entry
• reviewed 2026-08-25T09:17:59.122Z sha b3e2bd4 :: needs real behavior proof before merge. :: none
• reviewed 2026-08-25T13:16:35.630Z sha b3e2bd4 :: needs real behavior proof before merge. :: none
• reviewed 2026-08-28T20:11:44.497Z sha 366c643 :: needs real behavior proof before merge. :: none
• reviewed 2026-08-29T06:04:28.287Z sha 366c643 :: needs real behavior proof before merge. :: none

[steipete]

Superseded by #122 (merged) — a reviewed integration of this cancellation work rebased onto current main with your co-author credit preserved. Root causes fixed: cancellation arriving before mailbox waiter registration, unowned timeout tasks, and cursor reconciliation retrying CancellationError. Live signed macOS capture proof: cancellation went from 5.007s on main to 0.17ms, stopping cleanly with zero consumers. Thanks @SebTardif!