Skip to content

fix(capture): honor stop during watch transient backoff#193

Merged
steipete merged 5 commits into
openclaw:mainfrom
SebTardif:fix/watch-capture-transient-stop
Jun 23, 2026
Merged

fix(capture): honor stop during watch transient backoff#193
steipete merged 5 commits into
openclaw:mainfrom
SebTardif:fix/watch-capture-transient-stop

Conversation

@SebTardif

@SebTardif SebTardif commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Watch capture sessions could ignore requestStop() for up to 350ms while backing off from a transient ScreenCaptureKit failure. The transient retry path used raw Task.sleep, unlike the other sleeps in the loop.

This routes transient backoff through the existing stop-aware sleep helper with a retry-local timestamp, preserving the full retry delay while allowing a stop request to wake it immediately.

Regression coverage

The focused test now:

  1. captures one valid frame so normal session finalization remains meaningful;
  2. waits for the next capture to enter the transient-failure backoff;
  3. requests stop inside that 350ms window; and
  4. requires the session to finish within 80ms.

Maintainer review removed the standalone 198-line source-mutating proof harness; the production-matched Swift regression is the durable proof.

Verification

swift test --package-path Core/PeekabooCore --no-parallel --filter 'Stop request wakes transient capture backoff'
PROOF transient_stop_elapsed_ms=9
1 test passed

swift test --package-path Core/PeekabooCore --no-parallel --filter WatchCaptureSessionTests
PROOF transient_stop_elapsed_ms=12
15 tests passed
  • SwiftFormat: clean on the changed Swift files
  • git diff --check: clean
  • Codex autoreview of the maintainer fixup: clean
  • Codex autoreview of the full final branch against origin/main: clean

Caveat

A live ScreenCaptureKit transient denial is not deterministic enough for a stable test. The regression injects the same transient error only after a valid frame and synchronizes on entry to the retry path.

Related

@clawsweeper

clawsweeper Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 23, 2026, 6:54 PM ET / 22:54 UTC.

Summary
The PR changes watch-capture transient ScreenCaptureKit retry backoff to use the existing stop-aware sleep helper, adds a focused Swift regression test, and includes a release-note line.

Reproducibility: yes. Current main and the latest release show raw Task.sleep in the transient retry branch, and the PR body/comment provide terminal timing proof plus a focused injected regression.

Review metrics: 2 noteworthy metrics.

  • Diff surface: 3 files changed, +136/-1. The runtime change is small, but the PR also carries regression coverage and a release-note line.
  • Production behavior: 1 sleep call changed. The behavior change is limited to watch-capture transient retry backoff.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Wait for the remaining macOS CI Peekaboo CLI build & tests check to complete before merge.

Risk before merge

  • [P1] The Peekaboo CLI build & tests macOS CI check was still in progress at review time, so merge should wait for that result.
  • [P1] A live ScreenCaptureKit transient denial is nondeterministic; review confidence comes from source inspection, terminal timing proof, and an injected regression path rather than a live TCC-denial recording.

Maintainer options:

  1. Decide the mitigation before merge
    Land the stop-aware retry sleep change after the remaining macOS CI job completes and maintainer review accepts the bounded validation.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] No repair lane is needed because no actionable patch defect was found; the remaining action is CI completion and maintainer review.

Security
Cleared: The final diff changes local Swift source, Swift tests, and a changelog line; it does not add dependency, secret, permission, download, publishing, or code-execution surface.

Review details

Best possible solution:

Land the stop-aware retry sleep change after the remaining macOS CI job completes and maintainer review accepts the bounded validation.

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

Yes. Current main and the latest release show raw Task.sleep in the transient retry branch, and the PR body/comment provide terminal timing proof plus a focused injected regression.

Is this the best way to solve the issue?

Yes. Reusing the existing sleep(ns:since:) helper is the narrowest maintainable repair because other watch cadence sleeps already use that stop-aware path.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 1771d7db349b.

Label changes

Label justifications:

  • P2: This fixes a bounded watch-capture responsiveness bug with limited blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body and comment include terminal proof showing the after-fix stop-aware path waking promptly during the transient backoff window, with focused Swift test output.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and comment include terminal proof showing the after-fix stop-aware path waking promptly during the transient backoff window, with focused Swift test output.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: Git blame and GitHub path history tie the watch-capture loop, transient retry branch, stop-aware helper, and recent test fixup to this author. (role: feature-history owner and recent area contributor; confidence: high; commits: 1fa8eead7eea, 7e610180194e, 6b2554a6ad01; files: Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/Capture/WatchCaptureSession+Loop.swift, Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/Capture/WatchCaptureSession.swift, Core/PeekabooCore/Tests/PeekabooAutomationTests/WatchCaptureSessionTests.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jun 19, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper[bot]

Proof: 🧂 unranked krab — blocked until real behavior proof is added.

Added real terminal behavior proof and tightened the regression.

Proof command (any macOS host with Swift):

bash scripts/prove-watch-transient-stop.sh

Observed output on this machine:

watch_transient_backoff_delay_ms=350
stop_requested_after_backoff_start_ms=5
raw_task_sleep_stop_elapsed_ms=369
stop_aware_sleep_stop_elapsed_ms=6

Same stop timing (5ms into the 350ms backoff window): unfixed raw Task.sleep still runs ~369ms; the stop-aware helper exits in ~6ms. That is the behavioral difference this PR wires into WatchCaptureSession+Loop.swift.

Also tightened Stop request wakes transient capture backoff to assert stop within 80ms (was 1s) and to synchronize stop during the backoff window. The proof script step 2 runs red/green against that test when full Xcode is available.

PR body now includes a ## Real behavior proof section with the captured output.

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 19, 2026
@steipete steipete merged commit db5192b into openclaw:main Jun 23, 2026
5 checks passed
@steipete

Copy link
Copy Markdown
Collaborator

Landed as db5192bb370bfef279153b6d4d431bd381424208.

Maintainer verification:

  • swift test --package-path Core/PeekabooCore --no-parallel --filter 'Stop request wakes transient capture backoff' — passed; measured stop wakeup 9ms
  • swift test --package-path Core/PeekabooCore --no-parallel --filter WatchCaptureSessionTests — 15 tests passed; measured stop wakeup 12ms
  • SwiftFormat on both changed Swift files — clean
  • git diff --check — clean
  • Codex autoreview of the maintainer fixture/proof cleanup — clean, no accepted/actionable findings
  • Codex autoreview of the full final branch against origin/main — clean, no accepted/actionable findings
  • GitHub Actions run 28062294051 — PeekabooCore, CLI, Tachikoma, macOS apps, and SwiftLint all passed

The maintainer fixup changed the regression fixture to capture one valid frame before injecting the transient error, avoiding the zero-frame contact-sheet failure seen in the original test. It also removed the standalone 198-line source-mutating proof harness; the production-matched Swift regression is now the durable proof.

Caveat: no live ScreenCaptureKit transient denial was forced; the test injects the same transient error and synchronizes on entry to the actual retry path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants