fix(server): transfer handoff descriptors in batches - #3411
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe handoff protocol now sends file descriptors in batches of 64. The receiver collects multiple SCM_RIGHTS messages and closes received descriptors when validation fails. Live handoff no longer rejects more than 64 panes. Tests verify session preservation after pane exit and transfer of 70 pane PTY master descriptors. Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Live handoff now supports pane counts beyond a single SCM_RIGHTS message while retaining the existing representation for smaller handoffs. Multi-batch transfer and pane preservation are covered by the added live-handoff test, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
kangal-bot
added
the
ai-review
|
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files ==='
git diff --name-only HEAD^ HEAD
printf '%s\n' '=== PR diff: handoff ==='
git diff HEAD^ HEAD -- src/server/handoff.rs
printf '%s\n' '=== PR diff: headless and test ==='
git diff HEAD^ HEAD -- src/server/headless.rs tests/live_handoff.rs
printf '%s\n' '=== Source outlines ==='
ast-grep outline src/server/handoff.rs --items all
ast-grep outline src/server/headless.rs --items all
ast-grep outline tests/live_handoff.rs --items all
printf '%s\n' '=== Protocol call sites and definitions ==='
rg -n -C 4 -P '\b(send_fds|recv_fds|send_fds_and_wait_restored|recv_fds_and_send_restored|send_fd_batch|recv_fd_batch|close_raw_fds)\s*\(' \
src testsLength of output: 40304 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Handoff setup, manifest exchange, and import ==='
nl -ba src/server/handoff.rs | sed -n '130,320p'
printf '%s\n' '=== Descriptor send and receive implementation ==='
nl -ba src/server/handoff.rs | sed -n '370,530p'
printf '%s\n' '=== Live-handoff sender ownership and rollback ==='
nl -ba src/server/headless.rs | sed -n '1200,1415p'
printf '%s\n' '=== Existing live-handoff failure tests ==='
nl -ba tests/live_handoff.rs | sed -n '1720,1975p'
printf '%s\n' '=== Direct tests of descriptor transfer helpers ==='
rg -n -C 5 -P '\b(send_fds|recv_fds|SCM_RIGHTS|MSG_CTRUNC|handoff fd)\b' src/server/handoff.rs testsLength of output: 1066 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Handoff setup, manifest exchange, and import ==='
sed -n '130,320p' src/server/handoff.rs
printf '%s\n' '=== Descriptor send and receive implementation ==='
sed -n '370,530p' src/server/handoff.rs
printf '%s\n' '=== Live-handoff sender ownership and rollback ==='
sed -n '1200,1415p' src/server/headless.rs
printf '%s\n' '=== Existing live-handoff failure tests ==='
sed -n '1720,1975p' tests/live_handoff.rs
printf '%s\n' '=== Direct tests of descriptor transfer helpers ==='
rg -n -C 5 -P '\b(send_fds|recv_fds|SCM_RIGHTS|MSG_CTRUNC|handoff fd)\b' src/server/handoff.rs testsLength of output: 34819
|
Greptile SummaryThe PR removes the 64-pane live-handoff limit by transferring Unix PTY descriptors through multiple bounded
Confidence Score: 5/5The PR appears safe to merge, with no concrete correctness or security failures identified in the changed handoff path. Descriptor ordering remains aligned with manifest ordering, receive-side failures close acquired handles, and pre-commit incompatibilities retain the existing rollback behavior.
|
| Filename | Overview |
|---|---|
| src/server/handoff.rs | Replaces the single-message descriptor transfer with bounded batching, multi-message receipt, validation, and explicit error cleanup. |
| src/server/headless.rs | Removes the 64-pane preflight guard so the handoff layer can transfer arbitrarily many pane descriptors. |
| tests/live_handoff.rs | Adds an end-to-end 70-pane test that verifies descriptor ownership and pane topology after a multi-batch handoff. |
Sequence Diagram
sequenceDiagram
participant O as Old server
participant N as Replacement server
O->>N: Manifest with expected pane count
loop Until every PTY descriptor is transferred
O->>N: 1 byte + SCM_RIGHTS (up to 64 FDs)
N->>N: Validate and accumulate batch
end
N->>N: Restore pane runtimes in manifest order
N-->>O: restored
O->>N: commit
N-->>O: ownership acknowledged
Reviews (1): Last reviewed commit: "fix(server): transfer handoff descriptor..." | Re-trigger Greptile
122de79 to
359f362
Compare
|
Rebased onto 9e9bc8a to resolve the conflict from the |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Live handoff refused any session with more than 64 panes. The pane count was checked twice against MAX_FDS_PER_HANDOFF, and the transfer itself put every pane's pty master into one SCM_RIGHTS control message, so the guard was the only thing keeping the send inside the kernel's per-message limit. A session past the limit could only be updated by closing panes or by a normal restart, which ends every pane process. Send the descriptors in batches of 64 instead and drop both guards. The receiving side accumulates across recvmsg calls until the expected count arrives, bounds every SCM_RIGHTS payload it reads by the control bytes the kernel returned, rejects a batch that carries more descriptors than it asked for, and closes the descriptors it already holds on any failure. A session of 64 panes or fewer still produces one batch, so the bytes on the wire are unchanged and HANDOFF_VERSION stays at 1. refs herdrdev#3393
359f362 to
0ed1260
Compare
2 participants
Problem
herdr server live-handoffrefuses a session with 65 or more panes:The session then stays on the old build while the CLI moves ahead, and the
only supported way out is a normal restart, which ends the shell and agent
processes in every pane.
#3393 reports the same limit from
herdr update --handoff, where the rejectedhandoff additionally leaves the freshly installed CLI protocol-locked out of
the server that is still running. This change covers only the third option
listed there, chunking the transfer; the install ordering and the protocol
check on recovery commands are untouched, so it does not close that issue on
its own.
Cause
send_fdspacks every pane's pty master descriptor into a singleSCM_RIGHTScontrol message and callssendmsgonce.recv_fdsmirrorsthat with one
recvmsg. A single control message cannot carry an unboundednumber of descriptors, so
MAX_FDS_PER_HANDOFF = 64guards the send, once insend_fds_and_wait_restoredand once inperform_live_handoffbefore anywork starts.
Fix
FDS_PER_MESSAGE(64)min(remaining, 64)descriptors perrecvmsgand accumulates across calls until
expectedhave arrived, iterates everycmsghdrwithCMSG_NXTHDRand bounds eachSCM_RIGHTSpayload bymsg_controllenbefore reading it, rejects a batch that carries moredescriptors than it asked for, treats
MSG_CTRUNCas unrecoverable becausethe kernel has already closed the descriptors that did not fit, and closes
the descriptors it already holds on any failure
A session of 64 panes or fewer still produces exactly one batch, so its wire
representation is unchanged and old and new implementations stay compatible
across the range that works today. A new sender handing more than 64 panes to
an old receiver (a downgrade) still fails, but that pane count is already
unsupported.
HANDOFF_VERSIONstays at 1; raising it would refuse even the64-and-under handoffs that currently succeed.
The sending side of a handoff is always the previously running binary, so the
first update onto a build that carries this change is still refused by the old
server's guard; batching takes effect from the next handoff after that.
64 is kept as the batch width rather than raised: a single
SCM_RIGHTSmessage accepts at most 253 descriptors on Linux and 254 on macOS, measured
with a socketpair program, and 64 leaves room under both.
I'm happy to bump
HANDOFF_VERSIONor widen the batch if you'd rather.Verification
Measured on base e7d8220.
macOS, end-to-end against a real server with an isolated config, runtime
and socket path:
server live-handofforigin/masterat most 64 panesorigin/masterat most 64 panes131 panes splits into three batches (64 + 64 + 3), so this covers more than
the two-batch case. The script creates one pane per tab over the socket API
and then runs
herdr server live-handoff --import-exeagainst the samebinary; "panes after" is
herdr pane liston the replacement server.Linux aarch64:
cargo nextest run --locked -E 'binary(live_handoff)'; thenew test passes, and 5/5 when repeated alongside
live_server_holds_one_pty_master_fd_per_pane.New test
live_handoff_carries_more_panes_than_one_scm_rights_messagebuildsa 70-pane session, hands off, and asserts the replacement server holds 70 pty
master descriptors and lists 70 panes.
refs #3393