Repository navigation
fix(reverse-attach): answer runtime pings during long tool calls (liveness) - #69
Merged
Merged
Conversation
The openab-pty runtime pings every 20 s and drops a Mac that sends no frame for more than three intervals. dialOnce handled one frame at a time, so no receive() was outstanding while a tool ran and URLSession answered no pings: any call over ~60-80 s evicted the Mac (#68). - Keep ws.receive() always outstanding. Each inbound frame is answered in its own task; replies go out through one writer task per socket, so calls also run concurrently. Our own cancellation still reaches in-flight calls. - Ping the peer every 15 s while attached (keepaliveInterval). - Treat close code 4011 (runtime liveness close, openab-pty#86) as redial, with a unit test. - Reset the redial backoff after a stable connection on openab-pty attaches too, as switchboard already did. - On openab-pty attaches, refuse exec timeout_secs over 100 s (the runtime fails calls after 120 s) with a tool error pointing to exec_start + exec_poll. - Tell every grant profile, owner included, to keep calls under ~60 s and never sleep inside exec. Tested on macmini (rsync to ~/src/oab-instance-mcp-pr): - swift build -c release: OK, 0 warnings - swift test: 146 tests, 0 failures. New: ReverseAttachLivenessTests.testSlowToolCallSurvivesTheRuntimePingBudget (2.5 s tool vs a 0.8 s ping budget, keepalive off) passed, testClientSendsItsOwnKeepalivePings passed, testRuntimeAttachResetsBackoffAfterAStableConnection passed, testLivenessCloseRedials, testLongExecIs*, testEveryGrantProfile* passed. - The slow-tool test on unfixed origin/main code fails: evicted, 0 pongs answered, replies serialized [2, 3]. That reproduces the inferred URLSession behaviour. Fixes #68
…rs cancellation Review round 1 on #69: - Every dialOnce exit (stop code, retryable close, send failure, our own cancel) now cancels the calls still running on that socket, so none outlives it or reaches the next connection. cancel() also cancels tracked calls directly, which covers a revoke during redial backoff. - ExecTool.run kills its process group from a task cancellation handler (osascript goes through it too). The kill is guarded by a waitid(WNOWAIT) so it can never hit a reaped, recycled pid. - osascript gets the same 100 s limit on openab-pty attaches as exec. - exec / osascript timeout_secs descriptions mention the attach limit and exec_start + exec_poll. - Comment in dialOnce on why there is no per-socket concurrency cap and that pipelined calls may finish out of order. Tested on macmini (rsync to ~/src/oab-instance-mcp-pr): - swift build -c release: OK, no warnings - swift test: 149 tests, 0 failures. New: ReverseAttachLivenessTests.testCallsRunningWhenTheSocketClosesAreCancelled, ReverseAttachLivenessTests.testRevokingTheGrantCancelsCallsInFlight, ExecToolTests.testCancellingTheCallKillsTheProcessGroup passed; testSlowToolCallSurvivesTheRuntimePingBudget still passes.
Contributor
Author
Review round 1: findings → fixesAll fixed in 8b47951.
Behaviour note: a synchronous On an M4 Mac mini (macOS 15.7): |
…ll agents to serialize UI steps - ExecTool: the pipe-drain wait is now cancellable, so a cancelled exec returns even when a child backgrounded with `cmd &` still holds stdout/stderr after the shell leader exited (review O5). That child is not killed: once the leader is reaped its pid may be recycled. - desktop and owner instructions now say calls run concurrently and to issue mouse/key/osascript steps one at a time (review O4). Tested on an M4 Mac mini (macOS 15.7) from a fresh rsync: swift build -c release OK; swift test 150 tests, 0 failures (new: testCancelledCallDoesNotWaitForABackgroundedChildHoldingThePipes).
Contributor
Author
|
Review round 2 fixes (HEAD
Sol raised no findings. Build: |
Contributor
Author
Review round 3: findings → fixesNo code change; HEAD stays at 71b3e8e.
|
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
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.
Fixes #68. Runtime side: openabdev/openab-pty#86.
Problem
The openab-pty runtime pings the Mac every 20 s and closes the tools attach after more than three intervals with no inbound frame.
ReverseAttachClient.dialOncehandled frames one at a time (receive → answer → send → receive). While a tool ran, noreceive()was outstanding, andURLSessionWebSocketTaskanswered no pings. Any tool call longer than about 60–80 s evicted the Mac, and the agent sawattached=false, grant_pending=true.Changes
ws.receive()is now always outstanding. Each inbound frame is answered in its own task, and replies go out through one writer task per socket, so sends never interleave. Calls also run concurrently now. When a socket ends for any reason (a stop code, a retryable close, a send failure, or a revoke), the calls still running on it are cancelled, andcancel()also reaches calls directly during a redial backoff.exec(andosascript, which runs through it) kills its process group when its task is cancelled, and a cancelled call no longer waits for a child backgrounded withcmd &that still holds its pipes (that child is not killed, because the leader's pid may already be recycled). Long work started withexec_startis not tied to a socket and keeps running.Config.keepaliveInterval). The runtime counts any inbound frame as alive.disposition(closeCode: 4011)returns.redialfor both kinds. 4011 was picked because 4001–4010 are taken.execlimit on openab-pty attaches. Anexecwithtimeout_secsover 100 s is refused with a tool error pointing toexec_start+exec_poll, because the runtime fails any call still running after 120 s (REQUEST_TIMEOUT). Switchboard attaches and the local HTTP endpoint keep the 600 s cap.osascriptgets the same 100 s limit on openab-pty attaches, and both tools'timeout_secsdescriptions mention it.ownerincluded, now gets one line saying to keep calls under ~60 s, to useexec_start+exec_pollfor long work, and never tosleepinsideexec. Thedesktopandownerprofiles also get a line saying calls run concurrently, somouse,keyandosascriptsteps should be issued one at a time.stableAfter(60 s), as switchboard attaches already did. A drop after a long attach redials in about 1 s instead of 30 s.Coordination with openab-pty
exec's 100 s limit is tied to the runtime's 120 sREQUEST_TIMEOUT. If that changes,ReverseAttachClient.maxAttachedExecTimeoutshould follow.Test results
Run on an M4 Mac mini (macOS 15.7) from a fresh rsync of the branch:
swift build -c release: OK, no warnings.swift test: 150 tests, 0 failures (head after review round 2).ReverseAttachLivenessTests.testSlowToolCallSurvivesTheRuntimePingBudget: a fake runtime pings every 0.2 s and closes 1000 after more than 3 missed intervals, while a tool takes 2.5 s and the client's own keepalive is off. The attach survives, ≥5 pings are answered during the call, and a fastechosent after the slow call is answered first. Passed.ReverseAttachLivenessTests.testClientSendsItsOwnKeepalivePings: passed.ReverseAttachEndToEndTests.testRuntimeAttachResetsBackoffAfterAStableConnection: passed.ReverseAttachPolicyTests.testLivenessCloseRedials,testLongExecIsRefusedOnARuntimeAttachWithAPointerToExecStart,testLongExecIsNotRefusedOnASwitchboardAttach,testEveryGrantProfileIsToldToKeepCallsShort: passed.ReverseAttachLivenessTests.testCallsRunningWhenTheSocketClosesAreCancelled: a fake runtime starts a 3 s cancellation-aware call, closes 1000 after 0.3 s, and the client redials. The call is cancelled, never completes, and only the newinitializeis answered on the second connection. Passed.ReverseAttachLivenessTests.testRevokingTheGrantCancelsCallsInFlight:cancel()during a slow call cancels it. Passed.ExecToolTests.testCancellingTheCallKillsTheProcessGroup: cancellingexecofsleep 3; touch <marker>returns at once with exit 137, and the marker is never written. Passed.ExecToolTests.testCancelledCallDoesNotWaitForABackgroundedChildHoldingThePipes: cancellingexecofsleep 4 & exit 0returns within 2.5 s instead of waiting for the child. Passed.mainclient fails three ways: the runtime evicts the Mac, 0 pings are answered, and replies arrive serialized ([2, 3]). This confirms the inference in Reverse attach drops during long tool calls: receive loop is serial, so runtime pings go unanswered #68 that URLSession answers pings only while areceive()is outstanding.Not deployed anywhere; that is a separate step.