Repository navigation
fix(launcher): eliminate port TOCTOU via child-reported handshake bind; stop forcing --debug (#401) - #480
fix(launcher): eliminate port TOCTOU via child-reported handshake bind; stop forcing --debug (#401)#480ranxianglei wants to merge 1 commit into
Conversation
…OU; stop forcing --debug Parent-side findFreePort probed the preferred port, released it, then let the child re-bind (TOCTOU, #401): another bili grabbing it made probeHealth pass and silently attached client+MCP to a foreign instance; any other grabber burned the full 20s health timeout. No retry either way. - launcher: drop findFreePort; each launch passes a per-launch --handshake-file (BILI_HANDSHAKE_FILE). The child publishes {origin, pid} atomically after binding, so the origin is ours by construction; parent cross-checks pid before attaching and fails fast (<5s) with the child log path if the child exits before reporting/bind/health. - server: in handshake mode, EADDRINUSE on the preferred port falls back to an OS-assigned port (atomic bind, zero probe window) with a clear warn. Plain bili start conflict behavior (hint + exit 1) is unchanged. - launcher: no longer force debug:true on spawned proxies; the child inherits its log level exactly like a plain bili start (--debug flag / ACP_DEBUG env / config file). Per-launch /tmp/bili-proxy-<launchId>.log kept.
[bot] 🏷 Reviewed PR #480 end-to-end (full diff + comparison against current master) before changing anything. Headline: this PR is largely already in master via a different, more complete implementation, so it should not be merged as-is. Details below. What #480 doesOne commit (
Why it's superseded (the important part)Master already contains an independent fix for the same problem, landed ~5h after this branch was cut:
So two of #480's three goals (kill the TOCTOU, debug parity) are already merged, via a different mechanism (shared instance file + token, vs a per-launch handshake file). Merging #480 would introduce a second, competing handshake mechanism and it conflicts with master in The one genuinely-new pieceFast-fail on child exit. Master's spawn path still waits the full Recommendation
Baseline verified on master before all of the above: |
[bot] 🏷 Follow-up — I extracted the one non-duplicated piece into a standalone PR so nothing valuable from #480 is lost: → PR #481: fix(launcher): fail fast when the spawned proxy child exits pre-bind This is the only gap #480 had that master still lacks:
Verified against current master: Recommended disposition for #480 itself: close as superseded by #417 ( |
|
本 PR 基于 v0.1.67(4729609)开发;此后 master 已通过其他 PR 落地了覆盖 #401 全部三点的等价或更完整方案,本 PR 的 per-launch 握手文件设计与之重叠,关闭为 superseded:
验收标准对照:(a) 竞态下只 attach 自家子进程(token match)✅;(b) 默认等级日志量与 若后续发现 master 方案仍有缺口(例如子进程死亡时父进程仍要等满 SPAWN_WAIT_MS 才报错——本 PR 有 <5s fast-fail,可作为小补丁单独提),再开新票。 |
Salvages the one non-duplicated improvement from superseded PR ranxianglei#480 (its handshake-file TOCTOU fix and --debug removal are already in master via ranxianglei#417 instance governance). ensureProxyRunning now watches the spawned child's exit event and breaks the health-poll loop early, so a startup crash reports in ms instead of burning the full SPAWN_WAIT_MS (20s) window. Error carries the exit code/signal and points at the per-launch log.
Problem
The launcher used a two-step probe-release-rebind for proxy ports:
findFreePortbound the preferred port in the parent process, closed it, then passed the number to the child which bound it again — classic TOCTOU:probeHealthpassed against the foreign instance and client + MCP config silently attached to an instance with different routes/windows/compress settings (undetectable to the user);Additionally the launcher forced
debug: trueon every spawned proxy (introduced in 422fc6c for diagnosability), so everybili omp/bili codex/ ... ran at max log level (~12MB/day on one dev machine).Fix
Child-reported bind (kills the race at its root). The parent no longer probes any port. Each launch spawns
bili start --port <preferred> --handshake-file <per-launch tmp file>(new flag; also settable viaBILI_HANDSHAKE_FILE). After binding, the child atomically publishes{origin, pid}(tmp file + rename) to that handshake file. Only our own child knows the per-launch path, so the reported origin is ours by construction — attaching to a foreign instance becomes impossible. The parent cross-checkspidagainst the spawned child before attaching as defense-in-depth (fullinstanceIdhandshake tracked separately in #403).Stolen preferred port → atomic fallback instead of hang. In handshake mode, an
EADDRINUSEon the preferred port makes the server re-listen on port 0 (OS-assigned, atomic — zero probe window), log a clear warn, and report that origin through the handshake file. Plainbili startconflict behavior (hint + exit 1) is unchanged.Fast failure. The parent now watches for child exit during both wait phases (handshake + health). A dead child fails in well under 5s with a clear error pointing at the child log (
/tmp/bili-proxy-<launchId>.log) instead of hanging 20s.Log-level parity. Removed the forced
debug: true; spawned proxies inherit their log level exactly like a plainbili start(--debugflag /ACP_DEBUGenv / config file). The per-launch/tmp/bili-proxy-<launchId>.logredirect (the actual intent of 422fc6c) is kept.Tests
tests/launcher.test.ts:ensureProxyRunningsuite rewritten around the handshake protocol — attach-to-reported-origin (+ cleanup), race simulation (blocker occupies the preferred port; fake child falls back; asserts the parent only ever probes its own child's origin), fast-fail (child exits immediately → rejects in <5s with zero health probes, child killed, error carries the log path), pid ownership-mismatch refusal, debug-flag inheritance (--debugpresent iff requested).tests/handshake-fallback.test.ts(new): integration test driving realstartServerwith the preferred port occupied by a blocker — asserts the handshake file carries the OS-assigned origin + correct pid, health returns 200 there, and the blocker is untouched.Verification
npm run typecheckcleannpm test: 869/870 pass — the single failure (resolveClientCommand: codex/claude resolve to themselves) reproduces identically on pristine master in this environment (sandbox has/usr/bin/codexinstalled; pre-existing, unrelated)npm run buildclean