Repository navigation
fix: exit min proxy bridge when the daemon socket closes - #1282
Conversation
min proxy bridges stdin/stdout to the daemon socket with try_join!, which waits for both directions to finish. When min stop tears the socket down, the socket->stdout copy sees EOF but the stdin->socket copy blocks forever on the ssh-held stdin, so the proxy never exits and the ssh driving it is orphaned. Switch to select!: return as soon as the socket side closes, while still draining trailing daemon output when stdin closes first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthrough
ChangesProxy bridge lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The proxy now exits when the daemon socket closes while preserving trailing output handling; the change is localized and covered by passing checks and a regression test, so no actionable merge-blocking risk remains. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the bug, the implementation change, and the regression test. It also provides verification results. The required Checklist section is missing, but the description is otherwise complete. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
minimal-review — advisory review
Verdict COMMENT — COMMENT: policy.actions.request_changes is off; policy.actions.approve is off.
Scope: single file, single commit (dd3db9f), matches the PR description exactly — replace try_join! with select! in cmd_proxy's stdio↔socket bridge, plus a regression test. The PR head is a merge of unrelated main commits (ci, mfile/graph/mctx, diagnostics) that don't touch this file, so nothing outside the stated intent is included.
Clobber: none. git log on the touched file shows this is the only recent change to it; the merge doesn't revert or overwrite prior work.
Correctness: the new proxy_bridge correctly fixes the described hang. from_sock is pinned so it can be raced in select! and then polled again afterward; when it completes first (daemon closed the socket) the function returns immediately instead of blocking on ssh-held stdin — that's the actual fix. When to_sock completes first (stdin EOF), the code shuts down the socket write half and explicitly awaits the pinned from_sock to drain trailing daemon output, preserving the old behavior for a clean-exit stdin close. I checked for the same shape elsewhere (copy_bidirectional relays in minimald) — those bridge two live network peers where EOF propagates on close, not analogous to a pipe held open by a parent process, so this isn't a repeating bug. The new test (proxy_exits_when_the_daemon_closes_the_socket) is a genuine regression test: it uses tokio::io::duplex to keep the stdin side open (write half held by _ssh_stdin) while the mock daemon drops the accepted connection, and asserts completion within a timeout — this would hang under the old try_join! and passes under the new select!. No build was run (out of bounds per instructions); nothing here required one to verify.
No findings to report.
What ran
4 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. rg for select!/try_join!/copy_bidirectional bridge patterns repo-wide — exit 0 · self-reported
Found bidirectional relays in crates/minimald/src/connection.rs (relay_streams, direct-tcpip) and crates/minimald/src/net/proxy.rs (handle_connection_io, CONNECT tunnel), both using tokio::io::copy_bidirectional. Both bridge two live network endpoints where EOF/close propagates naturally on the peer side; neither holds one side open indefinitely the way an ssh ProxyCommand holds stdin open. This fix's specific bug (a pipe held open by a parent process that never sends EOF) does not repeat elsewhere in the tree.
2. git log -15 -- crates/minimal/src/lib.rs (scope/clobber check) — exit 0 · self-reported
dd3db9f (fix(minimal): exit proxy bridge when the daemon socket closes) is the only commit touching this file on the PR branch. The PR head eca6b59 is a merge of main bringing in unrelated commits (529040b ci, bff10a7 mfile/graph/mctx, e9fcb2f diagnostics) that don't touch lib.rs. No revert or overwrite of recent work.
3. rg -i for key/secret/token/password/private-key markers in pr.diff — exit 0 · self-reported
No matches; no secrets in the diff.
4. grep Cargo.toml for tempfile dependency used by the new test — exit 0 · self-reported
crates/minimal/Cargo.toml already declares tempfile.workspace = true; the new test introduces no new dependency.
minimal-review · schema minimal-review/v1 · persona generalist · model claude-sonnet-5 via claude-code · prompt review-prompt/v2 · head eca6b59380bb · verdict COMMENT · 0 finding(s), 0 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABKmg4iA
min proxybridges an ssh ProxyCommand's stdio to the daemon's Unix socket, joining both copy directions withtry_join!— which returns only once both directions finish. When the socket is torn down, the socket→stdout copy hits EOF but the stdin→socket copy blocks forever on the ssh-held stdin, so the proxy never exits and lingers (together with its parent ssh process) against a dead socket, surviving daemon restarts. The bridge now usesselect!: it exits as soon as the socket side closes, while still draining any trailing daemon output when stdin closes first. A regression test hangs on the old code and passes on the new.Verification
cargo fmt --all --check — clean
cargo clippy --workspace --locked -- -D warnings — no warnings
cargo build --workspace --locked — ok
cargo test --workspace --locked — passed (exit 0), including the new proxy_exits_when_the_daemon_closes_the_socket test
Summary by CodeRabbit