Fix: isolate Remote L3 daemon per-connection send failures - #1427
Conversation
|
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 (2)
📝 WalkthroughWalkthroughChangesRunner lifecycle and connection isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Parent
participant _serve_loop
participant _serve_connection
participant _start_session
participant RunnerReaper
Parent->>_serve_loop: connect and send handshake
_serve_loop->>_serve_connection: accept connection
_serve_connection->>_start_session: validate and start runner
_start_session-->>_serve_connection: reply and runner handle
_serve_connection-->>Parent: send success reply
_serve_connection->>RunnerReaper: hand off runner
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors connection handling in the remote L3 worker by introducing a dedicated _serve_connection function. This isolates individual sessions, ensuring that a broken pipe or send failure on a reply does not crash the main accept loop, and properly reclaims orphaned runner processes. Corresponding unit tests have been added to verify these behaviors. The review feedback points out a critical issue where catching BaseException in _serve_connection could swallow system-terminating exceptions like KeyboardInterrupt, and suggests catching Exception instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
857dfc8 to
6bfa992
Compare
The shared Remote L3 control daemon's accept loop treated a send failure on one parent connection as fatal to the whole daemon. When a timed-out parent closed its socket before the daemon replied, the success-reply send raised BrokenPipe, the except handler issued a second bare send on the same dead connection which also raised, and that exception escaped the accept loop into `finally: server.close()`. The daemon is the control plane for *starting* sessions; already-ready sessions talk to their parent over their own direct command/health sockets, so its death does not tear those down. What it does kill is the current, still-undelivered runner and every subsequent session-start request to that host — a shared-fate failure of the host's control plane. - `_start_session` now returns `(reply, proc)`: the live ready runner is handed back to the caller instead of being reaped internally, so the caller can reclaim it if the reply is never delivered. A failed handshake is still killed + reaped in place and returns `proc=None`. A successful send only means the bytes were queued locally, not that the parent read them (unobserved receipt would need an ACK / lease). - New `_serve_connection` owns one connection with full isolation and a closed ownership transaction: any `Exception` affects only that session; `KeyboardInterrupt`/`SystemExit` (BaseException) propagate for clean shutdown instead of becoming a spurious error reply; and a live runner is disposed of exactly once on every exit path — handed to the reaper only once its send returns, otherwise reclaimed by a `finally`. This covers the send failing, the reaper thread failing to launch, and a control exception unwinding mid-connection. - `serve` delegates to a `_serve_loop` that stops when the listener closes, so the loop can be driven and torn down deterministically. Tests reproduce each gap first (all fail against the pre-fix source and against interim versions lacking the finally/Exception narrowing): deterministic `_serve_connection` unit tests for hand-off, reclaim on send failure, reclaim on reaper-launch failure, error-reply isolation, truncated-frame/EOF, real-error reporting, and control-exception propagation (with exactly-once reclaim); a `_serve_loop` test for multi-connection survival and clean stop; and two bounded real-socket tests (dead parent, truncated frame) that serve the next connection and join their server thread with no listener leak or port race.
1 participant
Summary
The shared Remote L3 control daemon (
remote_l3_worker.serve) treated asend failure on one parent connection as fatal to the whole
daemon. When a timed-out parent closed its socket before the daemon
replied:
_send_jsonraisedBrokenPipeError,excepthandler issued a second, bare_send_jsonon the samedead connection, which also raised,
with connblock and thewhile Trueaccept loop intofinally: server.close().Blast radius (corrected)
The daemon is the control plane for starting sessions. Already-ready
sessions talk to their parent over their own direct command/health
sockets, so the daemon's death does not tear those down. What it does
kill is the current, still-undelivered runner and every subsequent
session-start request to that host — a shared-fate failure of the host's
control plane, plus an orphaned runner that lingered until its own
session_timeout_s.Changes (
python/simpler/remote_l3_worker.py)_start_sessionnow returns(reply, proc). The live, ready runnerhandle is handed back to the caller instead of being reaped internally,
so the caller can reclaim it if the reply is never delivered. A failed
handshake is still killed + reaped in place and returns
proc=None.(A successful
sendallonly means the bytes were queued locally, notthat the parent read them; unobserved receipt would need an ACK / lease,
which sim does not have.)
_serve_connectionowns one parent connection with fullisolation and a closed ownership transaction:
Exceptionon the socket affects only that session, never theaccept loop;
KeyboardInterrupt/SystemExitareBaseException, notException, so they propagate for a clean daemon shutdown insteadof becoming a spurious error reply;
handed to the reaper only once its send returns, otherwise reclaimed
by a
finally. This covers the send failing, the reaper threadfailing to launch, and a control exception unwinding mid-connection.
servedelegates to_serve_loopwhich stops when the listenercloses, so the loop is deterministically testable and tearable-down.
Testing
Repro-first (
.claude/rules/discipline.md§3): every new/updated test wasconfirmed to fail against the pre-fix source and against interim
versions lacking the
finally/Exception-narrowing._serve_connectionunit tests: hand-off after sendreturns, reclaim on send failure, reclaim on reaper-launch failure,
error-reply isolation, truncated-frame / EOF, real-error reporting,
control-exception propagation (with exactly-once reclaim).
_serve_loopunit test: multi-connection survival + clean stop onlistener close.
next connection is still served; the server thread is joined with no
listener leak and no port-rebind race.
Device-free; runs under the py UT suite.
test_remote_l3_lifecycle.py+test_remote_startup_budget.py;ruff+pyrightclean)