stdiod: deterministic child shutdown — terminal report precedes any respawn ack (T-74)

[Miyamura80]

Stack 3/3 (base: #38). The highest-risk PR of the stack — concurrency change on the reconcile hot path. Please review the shutdown sequence closely.

ChildServer::shutdown() previously killed the child then blindly abort()ed the pump tasks, making it a coin flip whether the old child's terminal server_offline reached the wire before, after, or never relative to a respawn's server_spawn_result. Downstream this forced a receiver-side clearing heuristic and test workarounds in edison-watch.

New sequence: kill the process group (lock released before joining — the pump's child_exit_status takes the same lock), drop the stdin channel, then join the stdout pump with a 2s bound (abort fallback on timeout), then abort stdin/stderr pumps. Every respawn path in the supervisor already awaits shutdown() before try_spawn, so end to end: the old child's server_offline is enqueued before the new child's spawn ack. Specced as PROTOCOL.md T-74; the backend's spawn-ack error clearing becomes a rule instead of a heuristic.

Two new tests pin the guarantee at both seams (pump-join in proc_tests.rs, offline-before-ack on the outbound channel in daemon_tests.rs) — both verified load-bearing by reverting to blind-abort and watching them fail. Also moves daemon.rs's inline test module to sibling daemon_tests.rs per crate convention.

Review checklist / known limits:

• Windows kill path untested — everything ran on Linux; daemon_tests.rs is #[cfg(unix)]-gated and stdiod CI is ubuntu-only. A Windows smoke of kill+respawn (or a windows-latest CI job) before release would close the gap.
• Worst-case teardown latency: a wedged WS writer costs up to 2s per child (bounded, serial across a mass teardown).

Verified: fmt/clippy/cargo test --workspace (114 tests), AI-writing check. Cross-repo: the full edison-watch tests/stdio_tunnel suite (80 tests) ran green against this binary, including 4 consecutive runs of the formerly-flaky pair with the workarounds deleted.

Merge order: edison-watch's fault-e2e PR asserts the T-74 ordering strictly — merge this before it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NgN7YdfLT41kWnWqe1MPZa

---

Generated by Claude Code

---

Summary by cubic

Makes child shutdown deterministic so the old child's terminal server_offline is always queued before any respawn server_spawn_result. This enforces PROTOCOL.md T-74 and removes receiver-side heuristics for spawn-ack error clearing.

• Bug Fixes

  • Updated ChildServer::shutdown to kill the process, mark exit, drop the outbound frame channel to end stdin cleanly, and join both frame pumps under a shared 2s budget; aborts are followed by a 200ms grace; stderr is still aborted.
  • If a pump is aborted and no send completed, shutdown emits the terminal error via OutgoingHandle::try_send (non-blocking), so ordering holds even when the WS writer is wedged.
  • Preserved T-43 at-most-once via a new report_sent flag set immediately after a completed send; fallback only triggers when no send was recorded.
  • Supervisor respawn paths await shutdown() before try_spawn, ensuring server_offline precedes the replacement’s spawn ack; added the ordering contract in try_spawn docs.
  • Expanded PROTOCOL.md T-74 to state ordering (not delivery) and document the fallback.
  • Added tests: shutdown returns only after the terminal report is queued; offline-before-ack ordering on the outbound channel; wedge cases where stdout or stdin holds the report still deliver exactly one terminal error. Note: Windows kill path is implemented but untested.

• Refactors

  • Moved inline tests from daemon.rs to daemon_tests.rs per crate convention.

^{Written for commit 8404f81. Summary will update on new commits.}

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[cubic-dev-ai]

1 issue found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/stdiod/crates/edison-stdiod/src/daemon_tests.rs">

<violation number="1" location="crates/stdiod/crates/edison-stdiod/src/daemon_tests.rs:8">
P2: Windows test runs fail before exercising the supervisor because every spawned fixture hard-codes `/bin/sh`; use a platform-specific shell fixture or gate these Unix process tests with `#[cfg(unix)]`.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: Windows test runs fail before exercising the supervisor because every spawned fixture hard-codes /bin/sh; use a platform-specific shell fixture or gate these Unix process tests with #[cfg(unix)].

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/stdiod/crates/edison-stdiod/src/daemon_tests.rs, line 8:

<comment>Windows test runs fail before exercising the supervisor because every spawned fixture hard-codes `/bin/sh`; use a platform-specific shell fixture or gate these Unix process tests with `#[cfg(unix)]`.</comment>

<file context>
@@ -0,0 +1,152 @@
+    DesiredServer {
+        server_id: server_id.into(),
+        name: server_id.into(),
+        command: "/bin/sh".into(),
+        args: vec!["-c".into(), script.into()],
+        env: Default::default(),
</file context>

[Miyamura80]

Latest push addresses the two ordering findings:

• Timeout-abort stranding the terminal report (P1): report_terminal now records send-completion with no await between send and flag (cancellation can only land at await points, so an aborted pump has either queued the frame and recorded it, or done neither). On a timed-out abort, shutdown joins the aborted handle with a short grace, then emits the server_offline itself via a non-blocking try_send when the pump provably didn't. Semantics: exactly-once in practice, at-most-once guaranteed, with the WS staleness teardown as the backstop for a genuinely wedged channel — documented in code and in PROTOCOL.md T-74.
• Stdin pump winning the latch then being aborted mid-send (P2): both stdin and stdout pumps are now joined under one shared shutdown deadline; only stderr is aborted directly. A dedicated test pins each seam and fails against the previous join-stdout-only behavior.

Declining the /bin/sh finding: the module is compiled only on Unix — daemon.rs wires it as #[cfg(all(test, unix))] #[path = "daemon_tests.rs"] mod tests; — so the fixture never builds on Windows. (Same convention as the crate's other sibling test files.)

---

Generated by Claude Code