fix(init): bound the whole tailscale query, not just the child's exit

Merged
BN
6d ago
+125-9

Bound the whole tailscale query, not just the child's exit

Main's shard 2 is red on every PR, including ones that do not touch this code:

FAIL rust-test (2) — init::tests::a_tailscale_query_that_does_not_answer_is_no_suggestion
panicked at crates/preloop-cli/src/init.rs:4147:9:
an unresponsive daemon must not hang the wizard, took 30.024862264s

30.02s is the test's own fixture (sh -c "sleep 30"), so the child was awaited
to completion although bounded_stdout had been handed a 200 ms deadline.

Root cause

bounded_stdout watched the clock only while waiting for the child to exit. It
then blocked in reader.join() — that is, in read_to_end on the child's
stdout — until the pipe reached EOF. Child::kill reaches the direct child
alone
, so a child that forked the command it was given leaves the pipe's write
end open in a grandchild that outlives the signal; the read then waits for the
grandchild and the deadline buys nothing.

Whether that happens is the host's /bin/sh:

  • engine (ubuntu:24.04 runner, ci/runner.Dockerfile) → /bin/sh is dash,
    which forks -c rather than replacing itself: the panic. Checked directly,
    not inferred: the runner image is FROM ubuntu:24.04, and /bin/sh in it is a
    symlink to dash.
  • macOS → /bin/sh execs the single command, so the pipe closes with the
    child and the test passes locally.

Measured with the function lifted verbatim from main (a98e769e9, no workspace
build needed) in a rust:1.99-slim-bookworm container, where
/bin/sh -> dash (200 ms deadline, child sleeps 30 s):

probemainthis branch
sh -c 'sleep 30'30.00983369s213.458463ms
dash -c 'sleep 30'30.014565131s216.580719ms
bash -c 'sleep 30'213.291085ms212.277528ms

The healthy shapes are unchanged: echo hello → Some("hello") in 53.5ms,
exit 3 → None, a binary that is not on PATH → None, a 200 000-byte answer
→ Some(200000 bytes) in 50.6ms.

Fix

The drain hands its bytes back over a channel, and this thread waits for them
only for what is left of the deadline:

let (sender, answer) = std::sync::mpsc::channel();
std::thread::spawn(move || {
    let _ = stdout.read_to_end(&mut bytes);
    let _ = sender.send(bytes);
});
…
let remaining = deadline.saturating_duration_since(std::time::Instant::now());
let bytes = answer.recv_timeout(remaining).ok()?;

timeout now bounds the whole call. A child that answered has closed its own
stdout and sends at once, so nothing healthy waits longer than it used to. The
kill still happens and the child is still reaped — only the read stops waiting
for whatever the child left behind, which a double-forked descendant would
escape a process-group kill for anyway.

Reproduced locally, on the real test binary

fakebin/sh first on PATH forks the command instead of exec-ing it, exactly as
dash does, so a macOS host can hold the engine's condition:

binaryconditionresult
mainforking shFAIL — an unresponsive daemon must not hang the wizard, took 30.051644917s at init.rs:4147:9
this branchforking shpass — 0.262s wall, of which 0.050s is process start

10 runs each with 8 CPU burners running (8 cores, one burner per core):

  • forking sh (engine condition): 10/10 pass —
    0.228 0.228 0.228 0.226 0.228 0.230 0.237 0.229 0.255 0.235 s
  • host sh: 10/10 pass —
    0.287 0.311 0.235 0.237 0.248 0.242 0.238 0.239 0.245 0.248 s

The assertion is untouched: elapsed >= 200ms still holds (the deadline is
waited out, not short-circuited) and elapsed < 10s is met with two orders of
magnitude of room. No sleep, no widened threshold.

Wizard path audit

There is no unbounded tailscale call site to route. Both queries the wizard
makes go through tailscale_json → bounded_stdout (init.rs:2584-2588),
including the funnel status --json → serve status --json fallback, and
TAILSCALE_TIMEOUT is a 5 s const (init.rs:2549) — not an env default above
30 s. The unbounded wait was inside the helper the probe already used.

For completeness, the file's other process spawns: docker build/save/image inspect (init.rs:3210-3248, legitimately long — a base image build), the
same via status()/output() in run_container, and a macOS-only
sysctl -n kern.hv_support (init.rs:2918). None is on the tailscale probe.

cargo fmt on main

The first commit reformats init.rs:4259, which the pinned rustfmt
(rustfmt 1.10.0-stable (58cf8f9c35), the toolchain's, confirmed in the same
container) already rejects on main — cargo fmt --all --check cannot pass
without it. Formatting only, no behaviour.

The second commit is the fix plus a CHANGELOG.md entry under
[Unreleased] → Fixed.

Gates

  • cargo fmt --all --check — clean (and red on main without the first commit).
  • cargo clippy --locked -p preloop-cli --all-targets -- -D warnings — clean,
    Finished dev profile … in 14.54s, no warnings.
  • the failing test, 10× with the slow fixture and 8 burners — 20/20 pass
    (10 with a forking sh, 10 with the host's).
  • cargo test -p preloop-cli init::tests — 31 passed, 0 failed.
  • cargo test -p preloop-cli — all targets pass, 0 failed.

Summary by CodeRabbit

  • Bug Fixes
    • The wizard’s Tailscale query now respects a single five-second deadline while waiting for a response. It no longer waits indefinitely when a child process leaves the output stream open.
    • Responses are limited to 4 MiB, preventing excessive output from growing without bound. If a complete response isn’t received before the deadline, the query returns no result.