fix: deterministic IPC response attribution and whole-group stop wait by disintegrator · Pull Request #606 · jdx/pitchfork

disintegrator · 2026-07-16T18:44:37Z

Summary

Fixes two race conditions observed in production with pitchfork 2.16.0, plus a hardening pass for the system-wide implications of the second fix.

1. Phantom start failure (`fix(ipc)`)

Cold-starting many daemons at once (e.g. 9 in one pitchfork start) occasionally reported a daemon as failed even though the supervisor had marked it ready, aborting its dependents.

Root cause: the IPC wire protocol has no request IDs — responses are attributed purely by read order on a connection. The supervisor handles each request in its own spawned task (conn_watch) and writes responses back in completion order, while all parallel start tasks shared a single Arc<IpcClient> connection and raced on the recv lock. Two concurrent Run requests could each consume the other's response: daemon A's task reads daemon B's DaemonFailedWithCode (which carries no daemon ID) while B's task reads A's DaemonReady. The swap could also silently misroute resolved_ports into dependent daemons' templates.

Fix: each parallel start/adhoc-start/stop task now opens its own dedicated IPC connection, so there is exactly one in-flight request per connection and response attribution is structural rather than timing-dependent — deterministic, with no wire-format change and parallelism within dependency levels preserved. As defense in depth, IpcClient holds a mutex across each send/read pair so shared clients (TUI, web routes) serialize exchanges, and a client whose exchange fails mid-stream (timeout/IO error) marks itself desynced and transparently reconnects on the next request instead of reading the abandoned response. Version-mismatch warnings are deduped on both sides (keyed by peer version, so a different mismatched peer still warns).

2. Restart race: stop returns while the group is still dying (`fix(supervisor)`)

kill_process_group signalled the whole group but only waited for the leader PID to die. Daemons run as sh -c "...", so the leader exits within ~30ms of SIGTERM while children in the group are still shutting down gracefully — e.g. docker compose up waiting for its container to stop. stop() returned early, marked the daemon Stopped, and a force-restart spawned a replacement up that attached to the dying container and exited with it (reproduced killing ClickHouse after every rebuild-restart).

Fix: the wait predicate is now whole-group emptiness (killpg(pgid, 0)) instead of leader death, keeping the existing stop signal → stop_timeout → SIGKILL escalation, with group emptiness verified after SIGKILL (bounded). Members that survive even SIGKILL (uninterruptible sleep) are reported as DaemonStopFailed instead of being silently marked Stopped. killpg EPERM is treated as terminated — only members we may not signal remain (e.g. root-owned helpers), which our signals never reached, so waiting cannot make progress.

3. Hardening: the rest of the system assumed millisecond stops

Making stops wait for the whole group (seconds, bounded by the stop budget) exposed several paths that assumed stops return in milliseconds. These are addressed in the follow-up commits:

Per-daemon stop locks: stop() holds the daemon's lock for the entire stop, and run() acquires it before its already-running check — a start deterministically waits out an in-flight stop instead of racing the (now wider) Stopping window into duplicate instances/port conflicts.
Client stop timeout tracks the server-side budget: IpcClient::stop budgets its request from the daemon's own stop_signal.timeout (or supervisor.stop_timeout) instead of the flat IPC request timeout, so a correct in-progress stop is no longer reported as a timeout failure.
Orphan cleanup off the startup path: it now runs in the background (parallel per orphan, guarded by the stop locks with a PID re-check), so supervisor autostart no longer delays IPC socket creation past the CLI's connect budget; boot daemons start after cleanup completes.
Autostop stops are spawned, keeping the cd shell hook (UpdateShellDir) and the 10s interval watcher responsive during group waits.
Ready-check-exhausted path: the group kill runs as a separate task (the monitor loop keeps draining output, so children logging during SIGTERM cleanup can't deadlock on a full pipe) and the failure notification is deferred until the kill completes, so the retry loop cannot respawn into the dying group.
Zombie reaper installs whenever the supervisor is PID 1 (not only with --container), so orphaned zombies can't keep a process group alive indefinitely.

Note for slow graceful shutdowns: the stop timeout is now the graceful budget for the entire group (like systemd's TimeoutStopSec for a cgroup) — daemons whose teardown legitimately takes longer (draining connections, stopping containers) should raise stop_signal.timeout rather than rely on outliving the budget.

Testing

mise run ci-dev passes on every commit (fmt, clippy -D warnings, full e2e suite 279/279, render — no generated-doc changes)
Bug 1 fix reasoned end-to-end: one in-flight request per connection makes mis-attribution impossible by construction; supervisor-side handling is unchanged
Bug 2 fix verified against all kill_process_group_async call paths (IPC stop, force-restart, readiness-timeout kills, orphan cleanup, supervisor shutdown); the hardening pass came out of an adversarial multi-agent review of the diff (15 verified findings, all addressed)

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved reliability when starting and stopping multiple daemons concurrently.
- Prevented stale or misattributed IPC responses by isolating IPC exchanges per operation.
- Reduced repeated version-mismatch warnings from both client and supervisor.
- Shutdown now waits for the entire process group to exit before reporting success.
- Stronger coordination between run/stop, readiness failures, autostop, and orphan cleanup (including container/PID 1 handling).
Tests
- Added E2E coverage for whole process-group stop/start serialization and multi-daemon readiness attribution.
- Clarified cron startup timing behavior in test documentation.

coderabbitai · 2026-07-16T18:45:12Z

📝 Walkthrough

Walkthrough

IPC exchanges are serialized and recover from failed reads, batch daemon tasks use dedicated connections, and supervisor lifecycle operations coordinate whole-process-group termination with per-daemon locks. Startup orphan cleanup and autostop execution are also updated.

Changes

IPC concurrency and batch execution

Layer / File(s)	Summary
Serialized IPC exchanges and dedicated batch connections `src/ipc/client.rs`, `src/ipc/batch.rs`	IPC request/response exchanges are serialized and reconnected after failures; parallel batch tasks use dedicated connections and extended stop timeouts.

Process termination and lifecycle coordination

Layer / File(s)	Summary
Whole process-group termination `src/procs.rs`	Graceful and forced shutdown now verify that the entire process group disappears, with group-level liveness reporting.
Serialized run, stop, and readiness failure handling `src/supervisor/lifecycle.rs`, `src/supervisor/mod.rs`	Per-daemon stop locks serialize lifecycle operations, and readiness failures wait for asynchronous termination before reporting.
Startup cleanup and orphan reconciliation `src/supervisor/mod.rs`	Orphan cleanup runs before boot daemons start and reconciles candidates concurrently with process identity checks.
Detached autostops and warning deduplication `src/supervisor/autostop.rs`, `src/supervisor/ipc_handlers.rs`, `src/ipc/client.rs`	Autostops run in cancellable detached tasks, and version-mismatch warnings are deduplicated.
End-to-end validation `test/group_stop.bats`, `test/cron.bats`	Tests cover process-group shutdown, stop/start serialization, parallel readiness attribution, and cron timing behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant IpcClient
  participant Supervisor
  participant Procs
  Client->>IpcClient: start or stop request
  IpcClient->>Supervisor: serialized IPC exchange
  Supervisor->>Procs: terminate process group
  Procs-->>Supervisor: group termination status
  Supervisor-->>IpcClient: operation response
  IpcClient-->>Client: result

Possibly related PRs

jdx/pitchfork#632: Overlaps on process identity checks and process-group termination.
jdx/pitchfork#633: Overlaps on supervisor startup and orphan reconciliation.
jdx/pitchfork#665: Overlaps on supervisor stop and process-group termination behavior.

Suggested reviewers: gaojunran, jdx

Poem

A rabbit hops through locks so tight,
Each IPC task gets its own flight.
Warnings speak once, then settle low,
Whole process groups leave in tow.
Cleanup waits before boots take light.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the two main fixes in the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

greptile-apps · 2026-07-16T18:49:31Z

Greptile Summary

This PR fixes two race conditions observed in production with pitchfork 2.16.0, plus a hardening pass for the system-wide implications of the second fix.

IPC response mis-attribution: Each parallel start/stop task now opens its own dedicated IPC connection (one in-flight request per connection), making response attribution structural and deterministic. An exchange mutex and a desynced reconnect flag are added as defense-in-depth for shared clients.
Whole-group stop: kill_process_group now polls killpg(pgid, 0) (not just the leader PID) before declaring termination, with escalation to SIGKILL and a bounded post-kill verification; stop() holds a per-daemon stop lock for its entire duration so concurrent starts wait out the widened Stopping window instead of spawning duplicate instances.
Hardening pass: Autostop stops are spawned as detached tasks; orphan cleanup runs in the background with parallel per-orphan kills; readiness-exhaustion group kill runs as a separate task so the monitor loop can drain output; zombie reaper installs whenever the supervisor is PID 1.

Confidence Score: 5/5

Safe to merge — the two production race conditions are addressed by construction and the hardening changes are consistently applied across all affected code paths.

The core fixes are structurally sound: dedicated IPC connections make response mis-attribution impossible regardless of timing, and killpg(pgid, 0) polling is the correct POSIX primitive for whole-group emptiness. The per-daemon stop lock is acquired before the already-running check and held through PID persistence in run_once, closing the duplicate-spawn window. All four readiness-exhaustion kill sites are updated consistently. The stop/stop_locked split avoids re-entrant lock deadlock on force-restart. The full e2e suite (279/279) passes and three focused new tests cover the precise failure modes.

Files Needing Attention: No files require special attention. The previously-flagged open item about IpcClient::run() client-side timeout not budgeting the post-exhaustion kill delay remains open for tracking.

Important Files Changed

Filename	Overview
src/ipc/batch.rs	Removes the shared Arc from spawn_start_task, spawn_adhoc_start_task, and spawn_stop_task; each task now calls connect_dedicated() to get its own connection, making parallel response attribution deterministic.
src/ipc/client.rs	Adds exchange Mutex (serializes send/recv on shared clients), desynced AtomicBool (triggers reconnect after mid-stream failure), version-mismatch dedup, and a stop-timeout budget for IpcClient::stop that tracks the server-side graceful window.
src/procs.rs	kill_process_group now polls killpg(pgid, 0) for whole-group termination instead of watching only the leader PID; adds process_group_terminated helper and process_group_alive; post-SIGKILL verification loop added with bounded wait and Err on surviving D-state members.
src/supervisor/lifecycle.rs	run() acquires per-daemon stop lock before the already-running check and passes it through run_once (held until PID is persisted); stop/stop_locked split serializes the whole group wait; ready_fail_kill task defers ready_tx until kill completes.
src/supervisor/mod.rs	Adds stop_locks and in_flight_autostops maps; orphan cleanup moved to a background spawn; boot daemons start after cleanup; PID-1 detection always enables zombie reaper.
src/supervisor/autostop.rs	Autostop stops now spawned as detached tasks (spawn_autostop/run_autostop) with cancellation flag + re-validation; in_flight_autostops tracks spawned-but-not-yet-stopping tasks so directory re-entry can cancel them.
src/supervisor/ipc_handlers.rs	Adds supervisor-side version-mismatch dedup (version_mismatch_should_warn) to prevent one warning per parallel dedicated connection.
test/group_stop.bats	New E2E tests covering: whole-group stop waiting for TERM-ignoring child processes, start serializing behind an in-flight slow stop, and parallel multi-daemon start attributing readiness to the correct daemon.
test/cron.bats	Adds a wait_for_logs guard before count assertion in the cron_immediate test to fix a race with log flush on slow CI runners.

_{Reviews (6): Last reviewed commit: "test(supervisor): use run ! for negated ..." | Re-trigger Greptile}

coderabbitai · 2026-07-16T22:41:17Z

Caution

Review failed

An error occurred during the review process. Please try again later.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai · 2026-07-16T22:41:48Z

Caution

Review failed

An error occurred during the review process. Please try again later.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai · 2026-07-16T23:13:53Z

Caution

Review failed

An error occurred during the review process. Please try again later.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

gaojunran · 2026-07-17T14:30:19Z

Good improvements! It'll be better if we add some e2e(bats) tests for these.

github-actions · 2026-07-18T00:38:48Z

This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically.

This is warning day 1 of 7.