Skip to content

fix(ios): guard websocket ping continuation#88231

Merged
ngutman merged 1 commit into
mainfrom
fix/ios-websocket-ping-continuation-crash
May 30, 2026
Merged

fix(ios): guard websocket ping continuation#88231
ngutman merged 1 commit into
mainfrom
fix/ios-websocket-ping-continuation-crash

Conversation

@ngutman
Copy link
Copy Markdown
Member

@ngutman ngutman commented May 30, 2026

Summary

  • Guard iOS/macOS WebSocket ping continuations so duplicate ping callbacks cannot resume a checked continuation twice.
  • Add regression coverage for duplicate success callbacks and error-first duplicate callbacks.

Verification

  • cd apps/shared/OpenClawKit && swift test --filter GatewayNodeSessionTests/websocketPingIgnoresDuplicateSuccessCallbacks (reproduced pre-fix crash: SWIFT TASK CONTINUATION MISUSE: sendPing() tried to resume its continuation more than once)
  • cd apps/shared/OpenClawKit && swift test --filter GatewayNodeSessionTests/websocketPingIgnoresDuplicateSuccessCallbacks
  • cd apps/shared/OpenClawKit && swift test --filter GatewayNodeSessionTests/websocketPingIgnoresDuplicateCallbacksAfterFirstError
  • cd apps/shared/OpenClawKit && swift test --filter GatewayNodeSessionTests
  • git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode local

Real behavior proof

Behavior addressed: Latest shared App Store Connect TestFlight crash (2026-05-30, build 2026.5.25 (4)) showed EXC_BREAKPOINT from CheckedContinuation.resume(throwing:) via WebSocketTaskBox.sendPing() / ThrowingContinuationSupport.resumeVoid.

Real environment tested: Local macOS Swift Package test runner for apps/shared/OpenClawKit on branch fix/ios-websocket-ping-continuation-crash.

Exact steps or command run after this patch: cd apps/shared/OpenClawKit && swift test --filter GatewayNodeSessionTests.

Evidence after fix: Regression tests simulate duplicate WebSocket ping callbacks; the full GatewayNodeSessionTests suite passed with 14 tests.

Observed result after fix: Duplicate ping callbacks no longer trap from checked-continuation double resume; first callback result wins.

What was not tested: A live iOS TestFlight build against App Store Connect crash reporting was not run for this branch.

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels May 30, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 30, 2026

Codex review: needs maintainer review before merge. Reviewed May 30, 2026, 12:42 AM ET / 04:42 UTC.

Summary
Adds an NSLock-backed at-most-once gate around WebSocketTaskBox.sendPing() continuation resumption and regression tests for duplicate ping callbacks.

PR surface: Other +76. Total +76 across 2 files.

Reproducibility: yes. at source level: current main resumes the checked continuation directly from the sendPing callback, and a duplicate callback would hit Swift's double-resume trap. The PR body also reports a pre-fix Swift test crash and App Store Connect crash signature, though I did not run Swift tests in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🦞 diamond lobster
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:

  • none.

Next step before merge

  • [P2] No repair lane is needed because the PR already contains the focused runtime change and tests; protected maintainer landing should stay with maintainers.

Security
Cleared: No security or supply-chain issue found; the diff only changes Swift continuation guarding and local tests.

Review details

Best possible solution:

Land the focused ping-continuation guard with its regression tests after normal maintainer and CI gates so duplicate WebSocket ping callbacks cannot crash the native app.

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

Yes at source level: current main resumes the checked continuation directly from the sendPing callback, and a duplicate callback would hit Swift's double-resume trap. The PR body also reports a pre-fix Swift test crash and App Store Connect crash signature, though I did not run Swift tests in this read-only review.

Is this the best way to solve the issue?

Yes: a local at-most-once gate in WebSocketTaskBox.sendPing() is the narrow maintainable fix and avoids changing keepalive timing, reconnect policy, or the WebSocket task API.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against d13c8b03c901.

Label changes

Label changes:

  • add P1: The PR addresses an iOS/macOS checked-continuation trap in WebSocket keepalive that can crash a real native app or node workflow.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This is a maintainer-labeled member PR, so the external contributor proof gate does not apply; the body still records crash provenance and Swift regression commands.

Label justifications:

  • P1: The PR addresses an iOS/macOS checked-continuation trap in WebSocket keepalive that can crash a real native app or node workflow.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This is a maintainer-labeled member PR, so the external contributor proof gate does not apply; the body still records crash provenance and Swift regression commands.
Evidence reviewed

PR surface:

Other +76. Total +76 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 2 77 1 +76
Total 2 77 1 +76

What I checked:

Likely related people:

  • steipete: git blame on WebSocketTaskBox.sendPing() and the keepalive ping caller points to Peter Steinberger's commit that added the current unguarded continuation path in this checkout. (role: introduced current ping wrapper; confidence: high; commits: 14795dc0cca5; files: apps/shared/OpenClawKit/Sources/OpenClawKit/GatewayChannel.swift, apps/shared/OpenClawKit/Sources/OpenClawKit/ThrowingContinuationSupport.swift, apps/shared/OpenClawKit/Tests/OpenClawKitTests/GatewayNodeSessionTests.swift)
  • ngutman: Nimrod Gutman has recent merged OpenClawKit gateway/auth work on current main, including gateway connection error UX and QR bootstrap changes, in addition to authoring this focused fix. (role: recent area contributor; confidence: medium; commits: 6380c872bcc2, a9140abea6d4; files: apps/shared/OpenClawKit/Sources/OpenClawKit/GatewayChannel.swift, apps/shared/OpenClawKit/Sources/OpenClawKit/GatewayConnectionProblem.swift, apps/shared/OpenClawKit/Tests/OpenClawKitTests/GatewayErrorsTests.swift)
  • mbelinky: Mariano/Mariano Belinky appears in the iOS gateway reconnect and receive-loop history around GatewayChannel.swift and GatewayNodeSessionTests.swift. (role: adjacent iOS gateway contributor; confidence: medium; commits: e98ccc8e17a4, 42d11a3ec5f4, 84e115834fa2; files: apps/shared/OpenClawKit/Sources/OpenClawKit/GatewayChannel.swift, apps/shared/OpenClawKit/Tests/OpenClawKitTests/GatewayNodeSessionTests.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: 🐚 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. P1 High-priority user-facing bug, regression, or broken workflow. labels May 30, 2026
@ngutman ngutman force-pushed the fix/ios-websocket-ping-continuation-crash branch from c6f3f22 to b4cee97 Compare May 30, 2026 04:55
@ngutman ngutman merged commit b352cb2 into main May 30, 2026
16 of 17 checks passed
@ngutman ngutman deleted the fix/ios-websocket-ping-continuation-crash branch May 30, 2026 04:56
@ngutman
Copy link
Copy Markdown
Member Author

ngutman commented May 30, 2026

Merged via squash.

Thanks @ngutman!

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 30, 2026
Merged via squash.

Prepared head SHA: b4cee97
Co-authored-by: ngutman <1540134+ngutman@users.noreply.github.com>
Co-authored-by: ngutman <1540134+ngutman@users.noreply.github.com>
Reviewed-by: @ngutman
steipete pushed a commit that referenced this pull request May 30, 2026
Merged via squash.

Prepared head SHA: b4cee97
Co-authored-by: ngutman <1540134+ngutman@users.noreply.github.com>
Co-authored-by: ngutman <1540134+ngutman@users.noreply.github.com>
Reviewed-by: @ngutman
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer Maintainer-authored PR P1 High-priority user-facing bug, regression, or broken workflow. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S 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.

1 participant