fix(macos): stop cancelled share waits and cursor retries - #122
Merged
Conversation
Integrate the cancellation repair from #114 on current main. Own mailbox expiry tasks with their continuations and close cancellation before waiter registration. Stop cancelled cursor operations while preserving retries for transient errors, with behavioral regressions at the capture owner. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed August 29, 2026, 9:05 AM ET / 13:05 UTC. ClawSweeper reviewWhat this changesThe PR makes Share This Mac cancellation release video-mailbox waits promptly and stop cursor reconciliation retries when cancellation is observed. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readinessKeep open for maintainer landing review: the exact PR delta fixes two reproduced cancellation paths absent from its current-main base, with strong native before/after evidence and no actionable patch finding. The remaining Swift analysis check must complete and the repository’s exact-head gates must be confirmed before landing. Priority: P2 Review scores
Verification
How this fits togetherShare This Mac captures macOS frames for connected viewers. Its capture owner uses a one-frame mailbox and cursor reconciliation task, so stopping a share must unblock both without leaving delayed background work. flowchart LR
A[Share stop or cancellation] --> B[Capture owner]
B --> C[Video mailbox wait]
B --> D[Cursor reconciliation]
C --> E[Timeout task cleanup]
D --> F[Retry decision]
E --> G[Share teardown]
F --> G
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve this bounded cancellation repair, confirm the exact-head gates, and land it so Share This Mac teardown does not retain frame waits or retry cancelled cursor configuration. Do we have a high-confidence way to reproduce the issue? Yes. The supplied exact-head evidence includes a paired real ScreenCaptureKit baseline where an already-cancelled five-second mailbox wait took about five seconds before the change and returned in about 0.00017 seconds after it. Is this the best way to solve the issue? Yes. Owning each timeout alongside its waiter and treating cancellation separately from transient failure is the narrowest repair while preserving the existing bounded retry behavior for ordinary errors. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 830832deb6f3. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
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.
What Problem This Solves
Fixes an issue where stopping or cancelling Share This Mac work could leave a video consumer waiting for its full frame timeout and treat a cancelled cursor configuration operation as a transient failure.
This supersedes #114 and preserves Sebastien Tardif's contribution with a co-author trailer and changelog credit. No linked bug issue exists in #114's closing references or the repository issue list, so this does not close an unrelated issue.
Why This Change Was Made
The mailbox now owns its timeout task alongside the waiter, cancels it on completion/replacement/finish, and rechecks cancellation after registration. That closes the race where the cancellation handler ran before a continuation existed. Cursor reconciliation stops on
CancellationErrorand cancelled backoff; ordinary errors retain the existing exponential backoff. The retry loop remains owned byMacScreenCapture, with a small extraction for behavioral tests.The integration starts from current main
830832deb6f31c43a2eeabb71cc051eb0d147fd3. It preserves the current release history and adds only an Unreleased entry. Listener admission, authentication, permission checks, and capture-configuration serialization are unchanged.Provenance is clear: the mailbox path originated in
ed2354d(#74), and cursor retry in6471f31(#90). The cursor problem is specifically an operation throwingCancellationErrorwhile the retry task remains active; cancelling the retry task itself already reached the loop's cancellation check on main.User Impact
Cancelled video waits return promptly instead of consuming the remaining frame timeout. Timeout tasks are released when their wait ends, and cancelled cursor operations are not retried.
Evidence
Before any production edit, native Swift harnesses built from the unchanged owners on main reproduced both defects: an already-cancelled mailbox wait returned after 5.297 seconds, and the cursor loop made 2 attempts after the first operation threw
CancellationError(expected 1). The larger package baseline build was interrupted during a filesystem stall and is not counted as a test result.After integration, all 39 VideoPipelineTests passed, including seven new cancellation/retry cases. Coverage includes cancellation before registration, 100 cancellation/offer races, mailbox reuse, timeout release, cancellation errors, cancellation during backoff, and preserving transient retries.
Real macOS proof used macOS 26.6.2 (
25G83), arm64, and Xcode 27 beta (27A5237l). A test-only executable linked against the unchanged compiled production capture/mailbox objects was signed with the OpenClaw Foundation Developer ID and run from the stable/Applications/Crabfleet.apppath. It received an actual 2560×1440 ScreenCaptureKit frame, then returnednilfrom a cancelled five-second mailbox wait in 0.00021375 seconds.MacScreenCapture.stop()completed in 0.006607625 seconds, with zero consumers left. No pixels were saved or uploaded; no registry publication, audio, clipboard, remote input, or listener was enabled. The temporary installed probe was removed afterward.A second paired live run used the preserved pre-edit main object files and then the integrated object files with the same signed probe and real captured frames: main took 5.006727 seconds to return
nilfrom the cancelled wait (probe failed its 500ms bound), versus 0.000169542 seconds after the fix (probe passed). Both runs stopped capture cleanly and left zero consumers.A full Share This Mac tailnet session was not exercised: the application's own signed Tailscale executable reported
BackendState: Stopped. The host's network state and TCC permissions were not changed. The ScreenCaptureKit proof above is real capture/teardown evidence, not a claim of a connected remote viewer or a naturally occurring ScreenCaptureKit cancellation error.Validation:
pnpm format,pnpm check,pnpm test(1,003 passed),pnpm build,pnpm build:static, Worker package dry-run,go test ./...,go vet ./..., andgit diff --checkpassed. The Go checks were rerun successfully with an isolated cache after the shared cache lost files during the first run.pnpm macos:testpassed all tiers: 121 vendor, 301 application, and 7 serialized integration tests. The opt-in real-tailnet smoke returned early because it was not enabled; the other six serialized cases exercised real loopback/ARD/lifecycle behavior. The first full run hit a one-second completion deadline in the existing file-sharing fixture; it passed unchanged in isolation (0.296s), and the entire unchanged native gate then passed. No production or test deadlines were increased. The native script retains its normal tests and concurrency settings; a task-local launcher supplies only external scratch directories to avoid host filesystem stalls.Codex autoreview completed with no accepted/actionable findings at the configured default P0 threshold. No merge is requested or performed.