fix: bound osascript wait with the shared process timeout - #197
fix: bound osascript wait with the shared process timeout#197SebTardif wants to merge 4 commits into
Conversation
Extract ProcessTimeout from the ffmpeg conversion wait and apply the same monotonic deadline to MessageSender and ReactCommand osascript paths so hung Messages automation cannot block indefinitely. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
Codex review: needs maintainer review before merge. Reviewed July 27, 2026, 3:48 PM ET / 19:48 UTC. ClawSweeper reviewWhat this changesThe PR extracts the existing external-process timeout behavior into a shared helper, applies it to message-send and reaction AppleScript calls plus launcher helpers, and adds timeout coverage using real macOS tools. Merge readinessThis is a focused reliability fix with sufficient real-process proof and no remaining line-level correctness finding. It should stay open for maintainer review because it deliberately changes the compatibility contract: previously unbounded Priority: P2 Review scores
Verification
How this fits togetherThe iMessage CLI and RPC paths invoke macOS subprocesses to send messages, add reactions, convert attachments, and prepare Messages.app. Their exit results feed back into command handling, so a hung subprocess can otherwise stall a user operation indefinitely. flowchart LR
Request[CLI or RPC request] --> Command[Send, react, conversion, or launcher]
Command --> Helper[External macOS helper]
Helper --> Wait[Shared bounded process wait]
Wait --> Outcome[Exit status or timeout error]
Outcome --> Request
Decision needed
Why: The implementation is mechanically coherent and follows the existing attachment-conversion policy, but applying that policy to established interactive command paths intentionally changes what slow existing workflows observe. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Adopt the shared bounded-wait helper if maintainers accept the explicit 60-second AppleScript and 15-second launcher-helper contracts, preserving the existing attachment-conversion timeout policy and its caller-specific errors. Do we have a high-confidence way to reproduce the issue? Yes for the bounded-subprocess behavior: the supplied test launches real macOS Is this the best way to solve the issue? Unclear: extracting the already-merged conversion timeout behavior is the narrowest maintainable implementation, but maintainers must decide whether its fixed deadline is the right compatibility contract for send, react, and launcher operations. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e22dfad8e54e. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
Import Darwin or Glibc conditionally like AttachmentResolver so linux-read-core does not fail on unconditional import Darwin. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
@clawsweeper re-review Updated Real behavior proof with exact-head live evidence (not mock-only unit assertions only). |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Remove Unreleased CHANGELOG entry (release-owned). Add a ProcessTimeout test that launches real /usr/bin/osascript with delay 30 and reaps it under a short bound (same launch shape as MessageSender/ReactCommand). Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
@clawsweeper re-review
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Use ProcessTimeout for short helper processes so hung killall/csrutil cannot stall launcher setup (same policy as osascript/ffmpeg). Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
@clawsweeper re-review Also bound MessagesLauncher killall/csrutil via ProcessTimeout (15s helper timeout) plus processTimeoutAllowsCsrutilStatus live success proof. |
|
🦞👀 Command router queued. I will update this comment with the next step. |
What Problem This Solves
MessageSender.runOsascript(NSAppleScript authorization fallback) andReactCommand.runAppleScriptstart/usr/bin/osascriptand block on unboundedprocess.waitUntilExit(). A hung Messages automation stalls send/react CLI or RPC work indefinitely. Attachment conversion already fixed the same hang class for ffmpeg in #176; osascript was left unbounded.Evidence
Patch
ProcessTimeout(monotonic deadline, default 60s, SIGTERM then SIGKILL process/group).Live osascript route shape (head
c0243a6)Production launch shape:
/usr/bin/osascript -l AppleScript -with AppleScript on stdin (same as MessageSender/ReactCommand). Source isdelay 30; helper timeout 0.6s:status=15is SIGTERM from the shared terminate path. Full suite remains green on macOS; linux-read-core green after Glibc import fix.Real behavior proof
Behavior or issue addressed: Unbounded
waitUntilExiton osascript send-fallback and react automation; now deadline-bounded.Real environment tested: macOS arm64, real
/usr/bin/osascript, branchfix/osascript-wait-timeoutatc0243a6.Exact steps or command run after this patch:
Evidence after fix: real osascript child running
delay 30reaped in under 1s with timedOut=true; helper unit tests still pass.Observed result after fix: same ProcessTimeout used by MessageSender/ReactCommand terminates hung osascript instead of blocking ~30s.
What was not tested: Full Messages.app Accessibility deadlock (requires intentional UI hang); NSAppleScript success path without osascript fallback.
MessagesLauncher helpers
killall Messagesandcsrutil statusalso used unboundedwaitUntilExit. They now useProcessTimeoutwith a 15s helper bound. Live success path:Test plan
swift test --filter processTimeoutReapsHungOsascript(real osascript)swift test --filter processTimeoutKillsHungProcessmake test/make lint