From ab3ef5b208d203d8885915d350698a27a9db08b4 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 18:01:18 +0200 Subject: [PATCH] fix(encode): a worker that died before the Hello landed said EPIPE, not "handshake" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI red on `a_worker_that_exits_immediately_is_a_handshake_failure`: the rung must name the handshake: send Hello: Broken pipe (os error 32) The test was pinning a premise that is only usually true. `spawn_link` execs a binary that exits at once, then races it: the parent writes Hello while the child is exiting. Win the race and the death surfaces as the EOF the recv reads, which carries `.context("encode worker handshake (died on startup?)")`. Lose it and the peer is already closed, so the SEND takes EPIPE — and that arm carried only `.context("send Hello")`, which never contains the word the test looks for. So this is not merely a test problem. Both arms are one cause — a worker that died during startup — and the operator was getting two diagnoses for it. On the EPIPE arm the fallback warn in `open_preferring_worker` degraded to a bare "send Hello: Broken pipe (os error 32)": it names neither the worker nor the stage that failed, on the one code path whose entire job is to explain why the session just fell back to the in-process encoder. The send now names the handshake too, and the race becomes harmless because both outcomes satisfy the same contract. The new deterministic rung is the part worth keeping. The spawn-driven test cannot be trusted to catch this: it did not fail ONCE in 60 runs of the unfixed code in a container here, because an idle box always wins the race — it is CI's load that loses it, which is exactly why this reached main. Closing the peer before the handshake starts reproduces the EPIPE arm with no scheduler dependence at all; backed out, it fails with CI's exact message. Verified with CI's own command, `cargo clippy -p pf-encode --all-targets --locked --features nvenc,vulkan-encode,pyrowave -- -D warnings` + the matching `cargo test`, in the amd64 container: clippy clean, 123 passed / 0 failed, and 100 consecutive passes of each of the two rungs. `cargo fmt --all --check` clean. Non-vacuity confirmed by `Checking pf-encode` / `Compiling pf-encode` in the log. --- .../src/enc/linux/pyrowave_remote.rs | 38 +++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/crates/pf-encode/src/enc/linux/pyrowave_remote.rs b/crates/pf-encode/src/enc/linux/pyrowave_remote.rs index 166c4aa3..f5bdfd34 100644 --- a/crates/pf-encode/src/enc/linux/pyrowave_remote.rs +++ b/crates/pf-encode/src/enc/linux/pyrowave_remote.rs @@ -380,7 +380,15 @@ fn handshake(mut link: Link, p: &Params, bitrate_bps: u64) -> Result // the process boundary. priority_intent: std::env::var("PYROWAVE_QUEUE_PRIORITY").ok(), }; - worker::send_eintr(link.sock.as_fd(), &hello, &[]).context("send Hello")?; + // Naming the handshake here is load-bearing, not decoration. A worker that dies during + // startup — no Vulkan 1.3 device, a missing feature, a half-installed binary that execs and + // exits — races this send: win the race and the death surfaces as the EOF the recv below + // reads, lose it and the socket is already closed, so the send takes EPIPE instead. One + // cause, so the operator must get one diagnosis; without this context the EPIPE side + // degrades the fallback warn to a bare "send Hello: Broken pipe (os error 32)", which names + // neither the worker nor the stage that failed. + worker::send_eintr(link.sock.as_fd(), &hello, &[]) + .context("encode worker handshake: send Hello (died on startup?)")?; let (ready, fds) = worker::recv_eintr::( link.sock.as_fd(), &mut link.rbuf, @@ -1043,8 +1051,16 @@ mod tests { .expect("a `false` binary on PATH") } - /// Ladder rung: the binary exists and runs but is not a worker. It exits at once, so the - /// handshake reads EOF — the same rung a worker that dies during Vulkan bring-up takes. + /// Ladder rung: the binary exists and runs but is not a worker — the same rung a worker that + /// dies during Vulkan bring-up takes. + /// + /// Which HALF of the handshake reports the death is a race this test deliberately does not + /// try to win: the child exits while the parent is still writing, so the send either lands in + /// a socket whose peer is still open (and the death surfaces as the EOF the recv reads) or + /// finds it already closed (EPIPE). CI, being slower and more loaded than a dev box, lands on + /// the EPIPE side often enough that pinning the EOF wording alone flakes — so what is pinned + /// is the operator-visible contract that holds on BOTH sides. The EPIPE half also has a + /// deterministic test of its own below; this one keeps a real spawn+exec in the ladder. #[test] fn a_worker_that_exits_immediately_is_a_handshake_failure() { let err = @@ -1056,6 +1072,22 @@ mod tests { ); } + /// The EPIPE half of the rung above, without the race: the peer is closed BEFORE the + /// handshake starts, so the Hello send cannot succeed. Deterministic, and it fails if anyone + /// trims the send's context back to a bare "send Hello" — which is exactly the drift that + /// made the spawn-driven test flake in CI. + #[test] + fn a_worker_that_died_before_hello_still_names_the_handshake() { + let (host, peer) = ipc::socketpair_seqpacket().unwrap(); + drop(peer); // the worker is gone before the host writes a byte + let err = handshake_on(host).unwrap_err(); + let text = format!("{err:#}"); + assert!( + text.contains("handshake"), + "the rung must name the handshake even when the death beats the Hello: {text}" + ); + } + /// Ladder rung: a binary that does not exist at all — an operator-set `PUNKTFUNK_ENCODE_WORKER` /// typo, or a half-installed package. #[test] -- 2.54.0