Repository navigation
fix(tools): close a ping-silent Mac with 4011 and report last_detach_reason - #87
Merged
Merged
Conversation
…reason The tools attach closed a Mac that missed the ping budget with plain 1000, which reads like a normal close on both sides, and instance_status then showed only grant_pending=true. Agents read that as a lost lease and asked the human to re-lend a Mac that was about to redial on its own. - close_code::LIVENESS_TIMEOUT = 4011 (4001-4010 are taken); the ping budget now closes with it. - The hub records the last detach per session (current socket going away, or a revoke/expiry). instance_status while detached reports last_detach_reason / last_detach_close_code / last_detach_secs_ago and a hint: wait for the redial while the grant is valid, ask the human only when there is no grant. A tool call while detached with a valid grant says the same instead of "ask the human". - Missed pings still count while a request is in flight (decision and reason documented on PING_INTERVAL). - ServerConfig.tools_ping_interval (default PING_INTERVAL) so the e2e suite can observe a liveness close in under a second. - CLIENT-CONTRACT.md: 4011 in the §5 and §9 tables, §9.2 liveness rule (keep a receive outstanding during calls; redial on 4011), §9.3 status shape. Fixes #86 Verified on macmini (rsync to ~/src/openab-pty-pr, runtime/): - cargo check --locked --all-targets: ok - cargo clippy --locked --all-targets -- -D warnings: ok - cargo test --locked: 186 + 29 passed, 0 failed - cargo test --locked -- --ignored --test-threads=1: 2 + 16 + 9 passed (incl. new a_ping_silent_mac_is_closed_with_liveness_timeout_and_may_redial) - cargo fmt --check: ok - cargo build --locked: ok
CI (ubuntu-24.04, macos-14) failed the new liveness e2e test with BrokenPipe: reading through tungstenite queued a pong for the first buffered ping and flushed it into a socket the runtime had already closed, before the close frame was returned. The stuck Mac now reads the raw TCP stream until EOF and looks for the 0x88 close frame carrying 4011, which is also closer to what a blocked receive loop does. Verified on macmini (~/src/openab-pty-pr/runtime): - cargo fmt --check, cargo check --locked --all-targets, cargo clippy --locked --all-targets -- -D warnings: ok - cargo test --locked: 186 + 29 passed - cargo test --locked -- --ignored --test-threads=1: 2 + 16 + 9 passed - new test alone 5x in a row: 5/5 passed
detach_reason() also yields runtime_replaced (close_all during a drain) and takeover (reserved); the §9 list named only five. Verified on macmini: cargo test --locked (all suites ok, 186 unit passed) and cargo test --locked --test tools_e2e -- --ignored --test-threads=1 (9 passed, incl. a_ping_silent_mac_is_closed_with_liveness_timeout_and_may_redial).
Collaborator
Author
|
First fix pass (no reviewer findings yet).
Verified: |
close_all drains the attached map before closing each socket, so a drain never records a detach (review O3). Doc-only change; no build or test run.
Collaborator
Author
|
Review round 2 fixes (HEAD
Sol raised no findings. No code changed in this round, so the Rust suites were not re-run locally; CI runs them on the new HEAD. |
The minimum-viable-lender checklist said to stop on every 4xxx except 4006, contradicting §9.2, which requires a redial on 4011 LIVENESS_TIMEOUT. The retry list is now 1000/4006/4011. Verification: docs-only change; grep of both PR worktrees finds no other retry list that omits 4011. No build or test run (no code changed).
Collaborator
Author
|
Review round 5 findings:
|
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 #86. Client side: openabdev/openab-computer#68.
Problem
The tools attach pings the Mac every 20 s and, after more than three silent intervals, closed it with plain WS
1000. That reads like a normal close in both logs.instance_statusthen reported onlyattached=false, grant_pending=true, so the PTY agent took it as a lost lease and asked the human to lend the Mac again, while the Mac was about to redial on its own. The Mac went silent because instance-mcp's reverse-attach client handles one frame at a time and does not answer pings during a long tool call (fixed in openab-computer#68).Change
4011LIVENESS_TIMEOUT. The ping-budget path now closes with4011instead of1000, and the log line carries the code. Codes 4001–4010 are all taken.last_detach_reasonininstance_status. The hub records the last detach per session: when the current socket goes away (liveness timeout, Mac closed or dropped) and when a grant is revoked or expires. While nothing is attached,instance_statusreturnslast_detach_reason(liveness_timeout,closed,revoked,ttl_expired,session_ended, ornull),last_detach_close_code,last_detach_secs_agoand ahint. With a valid grant the hint says the Mac redials on its own within about 30 s, so the agent should wait and check again. With no grant it says to ask the human. A tool call made while detached with a valid grant returns the same "wait for the redial" text instead of "ask the human to lend one". The record is forgotten with the session.ServerConfig.tools_ping_interval(defaultPING_INTERVAL, 20 s) lets the e2e suite see a liveness close in under a second. Production keeps 20 s.4011is added to the §5 and §9.2 close-code tables, and the retry rule changes to "give up on any 4xxx except 4006 and 4011". §9.2 item 3 now states the liveness rule: keep a receive outstanding during a call, missed pings count even while a request is in flight, and redial on4011. §9.3 documents the new status fields.Decision: missed pings still count while a request is in flight
I did not suspend the ping budget while a request younger than
REQUEST_TIMEOUT(120 s) is in flight. Reasons:-32003at 120 s anyway.execcalls lasting up to 600 s, so suspension would only move the eviction from about 80 s to about 200 s, and the call would still fail with-32003at 120 s.1000.Coordination with openab-computer
openab-computer#68 (PR A) treats exactly
4011as redial inReverseAttachClient's close-code disposition for the openab-pty attach. Today's client already redials on any code it does not list as terminal (4001, 4002, 4004, 4010), so this PR also works with the currently deployed client. The two can merge in either order.Test results
Run on a macOS build host (
runtime/, same commands asci.yml):cargo check --locked --all-targets: okcargo clippy --locked --all-targets -- -D warnings: okcargo test --locked: 186 + 29 passed, 0 failedcargo test --locked -- --ignored --test-threads=1: 2 + 16 + 9 passed, 0 failedcargo fmt --check: okBrokenPipe: reading through tungstenite queued a pong into the already-closed socket. The stuck Mac now reads the raw TCP stream and finds the4011close frame on the wire (5/5 local reruns pass).New tests:
tools_e2e::a_ping_silent_mac_is_closed_with_liveness_timeout_and_may_redial: a Mac that answers pings outlives the budget. A Mac that never reads is closed with4011.instance_statusreportsliveness_timeoutwithgrant_pending=trueand the redial hint, and a tool call says to wait for the redial. The same secret then redials and serves calls. A later revoke reads asrevokedwith no grant.tools::tests::detached_status_names_the_last_detach_and_whether_to_waitandliveness_has_its_own_close_code_and_reason.