Repository navigation
fix(minvmd): reap a stale gvproxy before drawing the node proxy port - #2313
norrietaylor merged 4 commits into
Conversation
A SIGKILLed or aborted supervisor leaves its gvproxy alive, and the leftover keeps the hostname proxy's default port (7654) bound. The supervisor drew the node proxy port before it reaped the stale gvproxy, so the draw saw the default refused and handed the proxy an ephemeral port for no reason; the reap that freed the default ran only later, at the gvproxy spawn. Reap before the draw. The reaper's own guard still fails the start when the matching gvproxy belongs to a live minvmd, and the resolved binary and switch socket are handed to the spawn block so nothing resolves twice. The "assigned the node port" log line now also names the holder of a preferred port skipped for bind refusal, so the supervisor's log says which leftover owned the bind the boot moved off. A run.rs unit test holds the preferred port, draws it away, releases it (the reap's effect), and draws it back. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Review limit reached
This review includes 1 billable file and costs up to $0.25. Or wait 56 minutes for your next included review. View limit details
Comment |
…ignature PathBuf is imported only inside run_foreground, so the module-level helper failed to compile wherever the minvmd_libkrun cfg is set.
The spawn block matches on the switch socket the pre-draw reaper returns instead of calling binary.exists() a second time, so the two checks can no longer disagree. The node-port draw test is renamed: it pins that a held preferred port is skipped and handed back once released, not the reap-before-draw order, which only the e2e lanes check.
|
/review |
|
Approved: no findings at Nothing to do. Clobber: none in this remit: |
port_holder already formats its holder as `pid <pid> (<exe>)`, so wrapping it again logged `pid 1234 (pid 1234 (gvproxy))`. Use the holder as configured_node_port does.
|
/review |
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of 4572aff79558 withheld this approval on the threads open then. Posted as a comment, as the review itself was; the check and the label carry it.
|
/review |
1 similar comment
|
/review |
There was a problem hiding this comment.
Approved on ARCH PASS at 4572aff.
There was a problem hiding this comment.
Read against the branch: the reap now runs after the alive lock is taken (phase 1 of run_foreground) and before the port draw, so the reaper's live-minvmd guard still holds, and the spawn block takes the resolved pair instead of resolving again. The three match arms keep their old meaning (own-IP without gvproxy still fails loudly). The holder lookup only runs on the bind-refused skip, so lsof on macOS stays off the normal path. The unit test pins the draw half; the reap-before-draw order needs the e2e lane, which passed.
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABX3vbpw
A killed supervisor (SIGKILL, abort) leaves its gvproxy alive, and the leftover keeps the hostname proxy's default port bound. The supervisor drew the node proxy port before it reaped the stale gvproxy, so the draw saw the default port refused and handed the proxy an ephemeral port for no reason; the reap that freed the default ran only later, at the gvproxy spawn. After a kill-based restart the hostname proxy never returned to its default port.
The fix reaps a stale gvproxy before the port draw. The reaper's guard still fails the start when the matching gvproxy belongs to a live minvmd, and the resolved gvproxy binary and switch socket are handed to the spawn block, which matches on the returned switch socket, so the "is gvproxy installed" decision is made once.
The "assigned the node port" log line now also names the pid and executable holding a preferred port that was skipped for bind refusal. This is intended: it tells the reader which leftover owns the bind the boot moved off. On macOS the lookup runs
lsof, but only on that bind-refused path (informed by the minvmd gvproxy pidfd spec).A unit test holds the preferred port, checks the draw skips it with
bind refused, releases it, and checks the same draw hands it back. It covers the draw's behaviour only. The order of reap and draw inrun_foregroundneeds a real VM, so only the e2e lanes check it.Verification
just fix,just clippy(macOS scope) — passjust test(macOS scope: minvmd + sessions) — 1234 passedcmd::run::tests::node_port_draw_skips_a_held_preferred_port_and_hands_it_back_once_released— pass