refactor(supervisor): fix monitor exit path race + rewrite architecture docs - #620
refactor(supervisor): fix monitor exit path race + rewrite architecture docs#620gaojunran wants to merge 20 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds Mermaid rendering support to the documentation site, expands architecture documentation, redirects the architecture entry point, and revises supervisor monitor shutdown and daemon exit cleanup. ChangesDocumentation rendering and architecture
Daemon lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Greptile SummaryThis PR fixes a real concurrency bug in the monitor task exit path (
Confidence Score: 5/5Safe to merge. The MonitorGuard placement fix is correct and the drain-ordering logic works as intended; no functional regressions were found. The core concurrency fix — moving MonitorGuard acquisition before the drain — is implemented correctly and matches the stated intent. The only finding is a stale block comment in the exit-path header that describes a background-drain architecture (step 5) and a flush-only step 2, neither of which matches the actual inline drain. The code behaviour itself is correct; the comment is misleading to future readers but does not affect runtime. Files Needing Attention: The exit-path header comment in Important Files Changed
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1 @@ | |||
| shamefully-hoist=true | |||
There was a problem hiding this comment.
shamefully-hoist=true masks undeclared peer dependencies
shamefully-hoist=true makes pnpm hoist all packages to the root node_modules, mimicking npm's legacy flat layout. This is sometimes required by packages that don't declare their own peer dependencies. A comment noting which package requires this flag would help future maintainers know whether it can ever be removed.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/.vitepress/theme/custom.css`:
- Around line 183-190: Add a narrowly scoped Stylelint disable around the
Mermaid foreignObject/foreignobject selectors so the intentional SVG selector is
accepted, and re-enable the rule immediately afterward. Keep the existing
overflow and line-height declarations unchanged.
In `@docs/concepts/architecture.md`:
- Around line 318-360: Track the fire-and-forget tasks created by
spawn_output_drain in the supervisor’s shutdown bookkeeping, alongside
active_monitors and hook_tasks. Update close() to await the registered drain
handles with a bounded timeout before shutdown completes, while preserving the
existing 5-second drain limit and ensuring trailing logs can be flushed.
In `@src/supervisor/lifecycle.rs`:
- Around line 170-199: Update the drain loop’s flush logic to retain and await
the `spawn_blocking` handle for each batch, ensuring only one log write is in
flight at a time. Await each completed batch before starting the next flush,
including the final `flush(&mut buffer)`, while preserving the existing batch
size and error logging behavior.
- Around line 1378-1384: Update the active_port cleanup in the lifecycle stop
flow to verify ownership under the state-file lock before clearing it. In the
!stop_intent_observed branch, clear the entry only when the stored PID still
matches pid; otherwise preserve the replacement daemon’s port.
- Around line 1340-1352: Move the active-monitor registration and MonitorGuard
creation from the late exit path to the beginning of the outer monitor task,
before it can observe or act on process shutdown. Keep MonitorGuard’s Drop
implementation decrementing SUPERVISOR.active_monitors and notifying
monitor_done so the guard covers the monitor task’s entire lifetime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a8bf1e00-b43c-41ea-b12d-287a04f9487f
📒 Files selected for processing (12)
AGENTS.mdARCHITECTURE.mddocs/.npmrcdocs/.vitepress/config.mtsdocs/.vitepress/theme/custom.cssdocs/.vitepress/theme/index.tsdocs/aube-lock.yamldocs/concepts/architecture.mddocs/package.jsondocs/pnpm-workspace.yamlmise.tomlsrc/supervisor/lifecycle.rs
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/supervisor/lifecycle.rs (1)
1513-1538: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake PID validation and the state update atomic.
The takeover check at Lines 1462–1466 releases the state lock before this upsert. A concurrent start can install a replacement PID after that check, then this old monitor overwrites it with
Stoppedandpid = None. Perform a PID-conditional update while holding the state lock, as done foractive_port.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/supervisor/lifecycle.rs` around lines 1513 - 1538, The daemon state check and update in the lifecycle monitor are racy because the lock is released before upsert_daemon. Change the flow around the takeover validation and the !already_stopped && !stop_intent_observed branch to retain the state lock through a PID-conditional update, matching the active_port handling, so the old monitor clears the PID and sets Stopped/Errored only when its monitored PID is still current.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/supervisor/lifecycle.rs`:
- Around line 727-740: The monitor registration currently occurs inside the
spawned future, allowing close() to observe active_monitors as zero before the
task is first polled. Move the active_monitors increment and MonitorGuard
construction to the code before tokio::spawn, then move the guard into the
spawned future so cancellation and all exit paths still decrement the count.
- Around line 158-164: Update MonitorGuard::drop to call notify_one() instead of
notify_waiters() after decrementing SUPERVISOR.active_monitors, preserving the
shutdown wakeup permit for the single waiter and avoiding the
missed-notification gap.
In `@src/supervisor/mod.rs`:
- Around line 950-970: Update the drain-task setup around drain_handles and the
JoinSet timeout so each original drain task’s abort handle is retained. On
timeout, abort those original handles directly before reaping the JoinSet
wrapper futures; keep the existing timeout warning and wrapper cleanup behavior.
---
Outside diff comments:
In `@src/supervisor/lifecycle.rs`:
- Around line 1513-1538: The daemon state check and update in the lifecycle
monitor are racy because the lock is released before upsert_daemon. Change the
flow around the takeover validation and the !already_stopped &&
!stop_intent_observed branch to retain the state lock through a PID-conditional
update, matching the active_port handling, so the old monitor clears the PID and
sets Stopped/Errored only when its monitored PID is still current.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 998ebde7-54ec-4d2d-ba13-856ab31dcd58
📒 Files selected for processing (4)
docs/.vitepress/theme/custom.cssdocs/concepts/architecture.mdsrc/supervisor/lifecycle.rssrc/supervisor/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/.vitepress/theme/custom.css
- docs/concepts/architecture.md
| impl Drop for MonitorGuard { | ||
| fn drop(&mut self) { | ||
| SUPERVISOR | ||
| .active_monitors | ||
| .fetch_sub(1, atomic::Ordering::Release); | ||
| SUPERVISOR.monitor_done.notify_waiters(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant symbols and read the surrounding code.
rg -n "MonitorGuard|monitor_done|notify_waiters|notify_one|close\\(" src/supervisor/lifecycle.rs
echo '--- lifecycle.rs excerpt ---'
sed -n '130,210p' src/supervisor/lifecycle.rs
echo '--- more lifecycle.rs around shutdown close path ---'
sed -n '680,780p' src/supervisor/lifecycle.rs
echo '--- any other monitor_done usages ---'
rg -n "monitor_done" srcRepository: jdx/pitchfork
Length of output: 10411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the file size first and then inspect only the relevant nearby sections.
wc -l src/supervisor/lifecycle.rs
sed -n '1,260p' src/supervisor/lifecycle.rsRepository: jdx/pitchfork
Length of output: 10286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '880,960p' src/supervisor/mod.rsRepository: jdx/pitchfork
Length of output: 3647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the monitor task exit path around the comments mentioning monitor_done.
sed -n '1440,1490p' src/supervisor/lifecycle.rs
sed -n '1560,1590p' src/supervisor/lifecycle.rsRepository: jdx/pitchfork
Length of output: 4132
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '880,960p' src/supervisor/mod.rs
sed -n '1440,1490p' src/supervisor/lifecycle.rs
sed -n '1560,1590p' src/supervisor/lifecycle.rsRepository: jdx/pitchfork
Length of output: 7628
Preserve the shutdown wakeup. close() can hit the gap between checking active_monitors and awaiting notified(), so notify_waiters() can drop the wakeup and leave shutdown sitting on the 5s timeout even after all monitors have exited. With a single shutdown waiter, notify_one() keeps that permit for the next poll.
Proposed fix
- SUPERVISOR.monitor_done.notify_waiters();
+ SUPERVISOR.monitor_done.notify_one();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Drop for MonitorGuard { | |
| fn drop(&mut self) { | |
| SUPERVISOR | |
| .active_monitors | |
| .fetch_sub(1, atomic::Ordering::Release); | |
| SUPERVISOR.monitor_done.notify_waiters(); | |
| } | |
| impl Drop for MonitorGuard { | |
| fn drop(&mut self) { | |
| SUPERVISOR | |
| .active_monitors | |
| .fetch_sub(1, atomic::Ordering::Release); | |
| SUPERVISOR.monitor_done.notify_one(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/supervisor/lifecycle.rs` around lines 158 - 164, Update
MonitorGuard::drop to call notify_one() instead of notify_waiters() after
decrementing SUPERVISOR.active_monitors, preserving the shutdown wakeup permit
for the single waiter and avoiding the missed-notification gap.
| // Register with `close()` *before* any lifecycle work so that | ||
| // shutdown reliably waits for this monitor to finish (and to | ||
| // register any drain/hook tasks it spawns) regardless of which | ||
| // exit path is taken. The guard is dropped on every return path, | ||
| // decrementing `active_monitors` and notifying `monitor_done`. | ||
| // | ||
| // Increment first, then construct the guard: if guard | ||
| // construction were to fail (e.g. panic) the count would still | ||
| // be balanced; and Drop cannot run before the matching | ||
| // increment because there is no await point between them. | ||
| SUPERVISOR | ||
| .active_monitors | ||
| .fetch_add(1, atomic::Ordering::Release); | ||
| let _monitor_guard = MonitorGuard; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Register the monitor before publishing the spawned task.
The increment still runs inside the spawned future. run_once() can return and close() can observe active_monitors == 0 before Tokio first polls this task, allowing shutdown to skip its eventual drain registration. Increment synchronously before tokio::spawn, create the guard outside, and move it into the future so cancellation still balances the count.
This is the same monitor-registration race identified previously, but the registration was moved only to the beginning of the future rather than before spawning it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/supervisor/lifecycle.rs` around lines 727 - 740, The monitor registration
currently occurs inside the spawned future, allowing close() to observe
active_monitors as zero before the task is first polled. Move the
active_monitors increment and MonitorGuard construction to the code before
tokio::spawn, then move the guard into the spawned future so cancellation and
all exit paths still decrement the count.
| let mut join_set = tokio::task::JoinSet::new(); | ||
| for handle in drain_handles { | ||
| join_set.spawn(async move { | ||
| let _ = handle.await; | ||
| }); | ||
| } | ||
| let overall_timeout = Duration::from_secs(7); | ||
| let timed_out = tokio::time::timeout(overall_timeout, async { | ||
| while join_set.join_next().await.is_some() {} | ||
| }) | ||
| .await | ||
| .is_err(); | ||
| if timed_out { | ||
| let remaining = join_set.len(); | ||
| warn!( | ||
| "output drain tasks did not complete within {overall_timeout:?} during \ | ||
| shutdown, aborting {remaining} remaining task(s)" | ||
| ); | ||
| join_set.abort_all(); | ||
| // Reap the aborted tasks so their resources are released. | ||
| while join_set.join_next().await.is_some() {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Abort the drain tasks themselves on timeout.
JoinSet::abort_all() only cancels the wrapper futures here; dropping the inner JoinHandles detaches the actual drain tasks, so they can keep running past shutdown. Keep an abort handle for each original drain task and cancel those directly before reaping the wrappers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/supervisor/mod.rs` around lines 950 - 970, Update the drain-task setup
around drain_handles and the JoinSet timeout so each original drain task’s abort
handle is retained. On timeout, abort those original handles directly before
reaping the JoinSet wrapper futures; keep the existing timeout warning and
wrapper cleanup behavior.
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 3 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
Opening the log store from two processes at once fails: switching a database into WAL mode needs an exclusive lock and, unlike ordinary statements, does not consult the busy handler, so the connection that loses the race gets SQLITE_BUSY no matter how long its timeout is. The error surfaced from open() itself, taking the whole store down rather than one statement. WAL is recorded in the database header and persists across connections, so only the first open needs to set it. enable_wal now retries briefly in case a peer is mid-switch and then carries on regardless: a connection that never personally set WAL still works correctly, and refusing to open the log store over a transient pragma race is far worse than running one connection unoptimised. Also states busy_timeout explicitly. rusqlite already defaults to the same 5s, so this changes no behavior today; it records the requirement at the call site instead of resting on a dependency's default, and the new contention test guards it. Prerequisite for moving log writes out of the supervisor process, where several writers open the same store concurrently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Without one the scheduler was free to run the workers in sequence, so the first would enable WAL and the rest would find it already set — never contending for the journal-mode switch the test exists to guard. It caught the bug on this machine but was not guaranteed to anywhere else. Verified by reverting enable_wal: the test now fails on every run rather than by luck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 4 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
The supervisor held the read end of every daemon's output pipe, which put it in the data path. Killing it left the pipe with no reader, so the daemon's next write took SIGPIPE and the daemon usually died with it — undermining the re-adoption added in jdx#633, since a daemon that logs to stdout rarely survived long enough to be adopted. Anything written but not yet read was lost as well. Capture now happens in a sibling process: `pitchfork log-sink`, this binary re-executed, holding the read end and writing to the log store. A supervisor crash is invisible to logging — the daemon keeps writing, the sink keeps recording, and no line is dropped. This is the arrangement runit uses, where runsv starts a service alongside its own log service and stays out of the stream. The supervisor retains a spare read end, which does two things: a sink that dies cannot leave the pipe readerless and kill the daemon, and its replacement can be handed the same pipe. It deliberately keeps no write end, which would stop the sink from ever seeing end of file. A sink that exits while its daemon is still monitored is replaced, mirroring runsv restarting a log service; one that exits cleanly has reached end of file and is not. Failed starts report what the daemon printed by querying the log store, which the in-process path guaranteed by flushing synchronously before signalling. That ordering is restored by waiting for the sink's final write, bounded so a daemon whose descendants hold the pipe open cannot stall the caller. Daemons using ready_output, an on_output hook, or pty = true keep the in-process path, because the supervisor itself has to read the stream to serve them, and so remain vulnerable to a supervisor crash. Teaching the sink to evaluate those and report matches back is left for a follow-up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three problems, two of which could leave a daemon with no reader at all — blocking it once the pipe filled and killing it with SIGPIPE once the sink's own end went. The retry test also failed, on CI and locally: waiting for the sink's final write on every attempt both delayed the backoff and widened the window in which a daemon looks errored and idle. `run` retries inline, so the background retry checker had time to start an attempt of its own alongside it, producing five attempts where four were expected and doubling the elapsed time. The wait now happens only on the attempt that gives up, which is the only one whose output is reported. - a sink that cannot be spawned no longer ends supervision. It dropped the retained read end while the daemon was already running and writing; spawning is now retried for as long as the daemon is monitored. - the sink reads bytes and converts lossily instead of requiring valid UTF-8. A single stray byte used to end the read loop, and because that looked like a clean exit the supervisor read it as end of file and stopped replacing the sink. Genuine read failures now exit non-zero so a replacement starts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both were raised in review: stdout and stderr deliberately share one pipe (as the in-process path effectively did, and as runit does), and an adopted daemon's sink is not covered by a replacement loop, so one that dies after adoption is not replaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI caught `retry zero fails immediately with one attempt`, which requires a failed start to finish in under three seconds. Waiting for the sink process to exit was costing ~766ms of that budget, and the reason is that exiting is a poor proxy for having written: it also pays for spawning a process and opening the log store, hundreds of milliseconds, while the write itself lands in a few dozen. Measured on the same daemon, the output was queryable after 36ms. The failure path now polls for this run's output directly, capped at 400ms, which brings a failed start from ~766ms down to ~33ms. A daemon that fails without printing anything has nothing to wait for and pays the cap, which is why it is short. Sinks are also started before the daemon rather than after it, so a daemon that exits immediately no longer has to wait for its reader to come up before anything can be captured, and the supervision loop now waits on the sink it already holds before consulting the monitor registry. Checking the registry first abandoned the sink of a daemon that had already exited, since its monitor entry was gone by then. This removes the Drained handshake entirely: nothing needs to observe the sink's exit any more. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI's lint step failed on formatting: later edits went in without re-running rustfmt. `mise run lint` now passes locally, which is the same command CI runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rves Four robustness problems, all in the process that is now the only reader of a daemon's pipe — so each one ends with the daemon blocked or its output lost. - reading and writing are separate tasks. The batch write was on the read path, so a contended SQLite write, which can now wait out the store's five-second busy timeout, stopped the pipe being drained. A bounded queue between them keeps backpressure without unbounded growth. - output containing no newline is split at 64 KiB instead of accumulated indefinitely. A daemon emitting an endless stream, or binary data, would have grown the buffer until the sink was killed for memory — and the supervisor would have started another to repeat it. Verified: 300 KB of newline-free output is recorded as bounded lines. - a sink started for a daemon that then fails to spawn is killed and reaped explicitly. Dropping the handle only reaps best-effort, and run_once runs per retry attempt, so repeated spawn failures could leave several behind. - the gap before retrying a failed sink spawn drops from 1s to 200ms. Nothing drains the pipe meanwhile, so a chatty daemon blocks; blocking is the right outcome rather than discarding output, but the gap should be brief. A failed start also waits for its output to settle rather than returning at the first row, since the sink batches: a daemon printing eight lines before failing reported only the first. Now all eight, at a cost of ~120ms rather than ~30ms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five issues, four of them introduced by the previous commit's split of reading from writing. A queue closed by a dead writer was reported as success, which tells the supervisor the sink reached end of file — so it would stop replacing a sink while the daemon was still writing. It is now a read error, which exits non-zero and gets the sink replaced. The wait a failed start does for its output had three problems: - its timeout sat between polls, so a single query waiting out the store's five-second busy timeout blew straight through the 400ms cap. The cap now wraps the whole loop. - it counted every entry since the run began on each poll, materializing a chatty daemon's whole history every 20ms. It now reads one row: the newest entry's id, which is all that growth detection needs. - a query error counted as zero rows, which reads as a settled stream. An unreadable store says nothing about whether the sink has finished writing, so it no longer ends the wait early. Sink spawn failures now back off to five seconds rather than retrying five times a second for the life of the daemon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The branch for a daemon that exits before its PID can be read killed the sink immediately, discarding whatever was still in the pipe or batched in it — and since that daemon ran, its output is the only explanation of why it gave up. Reaping like that is right for a daemon that never started at all, which is the path it was added for. Its write end is already closed, so the sink is on its way to end of file: wait for the output to land, as the other readiness failures do, and reap afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Splitting a line purely by byte count cut multi-byte characters in half at the 64 KiB cap, and converting each half on its own turned one valid character into two replacement characters. The split now stops at the last complete character and carries the partial sequence into the next line. Emitting at the cap also left the buffer empty, so a newline arriving immediately afterwards recorded an extra blank row — that newline terminates the line already written. Blank lines a daemon actually prints are still recorded. Verified: a two-byte character straddling the boundary survives intact, exactly 64 KiB followed by a newline yields two rows rather than three, and consecutive blank lines are still preserved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ectly Two review follow-ups. A sink that has been started but not yet handed to `supervise` is now held by a guard that terminates it when dropped. `run_once` can bail out at several points in between — stdio failing to wire up, the daemon's spawn failing, its PID being unreadable — and each one had to remember to reap. Three findings have now been raised about paths through that gap, so tying cleanup to the value's lifetime retires the class rather than patching the next one. The character-boundary split no longer asks `from_utf8` where the string stops being valid. That reports the first problem, so an invalid byte earlier in the line hid an unfinished character at the end and the character was split anyway. It now inspects the final bytes directly, which cannot be masked. Verified with an invalid byte early and a two-byte character straddling the cap: the injected byte alone becomes a replacement character and the straddling one survives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`tokio::spawn` in a destructor panics when there is no runtime to spawn onto, and silently abandons the task if shutdown has already begun — both reachable while the runtime is going away. `start_kill` needs neither, so the signal is always delivered; collecting the exit status is left to tokio's orphan reaping, since a destructor cannot await. Verified against the case the guard exists for, using a shell binary that does not exist so the daemon's spawn fails after its sink has started: five failed starts leave no sinks behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Move MonitorGuard acquisition before the output drain so close() cannot observe active_monitors == 0 during the 5s drain window and exit(0) before hooks are registered. Make the output drain fire-and-forget via spawn_output_drain() so the 5s drain window no longer blocks the close()/start() critical path. Rename pre_drain_is_stopping to stop_intent_observed and split it into was_stopped_before_cleanup / was_stopping_before_cleanup for clarity. Capture log_format.clone() in parse_line so the monitor task retains ownership of log_format.
Rewrite docs/concepts/architecture.md with the full technical overview: module layout, startup sequence, background tasks, daemon states, state persistence, process spawning, readiness detection, port management, termination, monitor exit path, hooks, logging, proxy, resource limits, and IPC responses. Add ARCHITECTURE.md symlink at repo root pointing to the VitePress source, and reference it prominently in AGENTS.md.
Add vitepress-plugin-mermaid and mermaid 11.16.0 to render flowchart diagrams in the architecture docs. Fix text clipping caused by mermaid 11.15+ bug #7759 (themeCSS lowercases foreignObject selector, breaking XML case-sensitive CSS matching) with three layers: - flowchart.wrappingWidth: 1000 in mermaid config (default 200 clips) - CSS targeting both foreignObject and foreignobject spellings - JS post-processing via MutationObserver to set overflow=visible on asynchronously rendered nodes Add docs/.npmrc (shamefully-hoist=true) and docs/pnpm-workspace.yaml (allowBuilds: esbuild) required by the mermaid plugin install.
Add a comment above the test task explaining that local developers should install GNU parallel (apt install parallel / brew install parallel) to enable bats --jobs 16 parallel execution. Without it, bats falls back to sequential execution.
Track monitor tasks for their full lifetime and register background output drains before their monitor guard drops. Shutdown waits for registered drains concurrently under a single bounded timeout. Await each SQLite drain batch before starting the next one, and clear an active port only while the state record still belongs to the exiting PID. Also add a scoped stylelint exemption for Mermaid SVG selectors and keep the architecture documentation aligned with the shutdown sequence.
Reflect the log-sink design from PR jdx#661: most daemons now have their output captured by a sibling `pitchfork log-sink` process that holds the pipe's read end, surviving supervisor crashes. Daemons using ready_output, on_output hooks, or pty keep the in-process path. Updated sections: - Module layout: add log_sink.rs - Startup sequence: add sink setup step before spawn - Readiness detection: clarify two capture paths - Monitor exit path: note drain applies only to in-process path - Logging: rewrite with sink-path and in-process-path subsections Also includes rustfmt formatting from ci-dev (let-chains, remove unnecessary reference).
# Conflicts: # docs/cli/commands.json # pitchfork.usage.kdl # src/cli/log_sink.rs # src/supervisor/lifecycle.rs # src/supervisor/log_sink.rs # test/logs.bats
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/supervisor/lifecycle.rs (1)
1535-1568: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not block restart state cleanup on the output drain.
Lines 1548-1568 hold the daemon record in
Runningwhile draining for up to five seconds. A natural exit followed by an immediatestartis therefore rejected as already running. Move the drain into a registered background task beforeMonitorGuarddrops; complete port/state cleanup and hooks without waiting for the drain deadline.AI-generated review comment. Based on learnings, GitHub PR comments must note that they are AI-generated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/supervisor/lifecycle.rs` around lines 1535 - 1568, Move the post-exit output drain and final log flush out of the synchronous lifecycle path around MonitorGuard, registering them as a background task before the guard drops. Ensure port/state cleanup and exit hooks proceed without awaiting the five-second drain deadline, while preserving sender removal, local-line buffering, timeout behavior, and final flush within the background task.Source: Learnings
🧹 Nitpick comments (2)
docs/.vitepress/config.mts (1)
177-185: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove
securityLevel: "loose"from the Mermaid config.
wrappingWidthanduseMaxWidthwork with Mermaid’s defaultstrictmode. This docs deploy only contains one Mermaid diagram, and it does not use click callbacks or raw HTML, so keepinglooseunnecessarily exposes diagram content to raw HTML/interactivity in docs that are generated from content behind the docs site.🔒️ Proposed change
mermaid: { - securityLevel: "loose", // wrappingWidth: mermaid 11.16 `#7354` clips long node text at default 200px. // useMaxWidth: scale SVG to container width. flowchart: { wrappingWidth: 1000, useMaxWidth: true, }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/.vitepress/config.mts` around lines 177 - 185, Remove the securityLevel setting from the Mermaid configuration object, leaving the existing flowchart wrappingWidth and useMaxWidth options unchanged so Mermaid uses its default strict mode.src/supervisor/lifecycle.rs (1)
2609-2613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake test database cleanup async and fallible.
These synchronous removals block the runtime and discard all cleanup failures. Use
tokio::fs, ignore onlyNotFound, and returnmiette::Result<()>.AI-generated review comment. As per coding guidelines, “all I/O should be async” and “Return
miette::Resultfor rich error messages.” Based on learnings, GitHub PR comments must note that they are AI-generated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/supervisor/lifecycle.rs` around lines 2609 - 2613, Update cleanup_db to be async and return miette::Result<()>, replacing synchronous remove_file calls with tokio::fs operations. Ignore only NotFound errors for the database, WAL, and SHM paths; propagate all other failures through the miette result, and update callers to await the cleanup.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/concepts/architecture.md`:
- Around line 313-340: Update the `close()` shutdown sequence and Monitor Task
Exit Path to document that output draining runs in a background fire-and-forget
task, with drain handles registered before monitor completion. Include those
handles in shutdown waiting under the bounded timeout, and remove wording that
implies the monitor synchronously drains output or that all trailing output is
flushed before it exits.
- Around line 213-223: Renumber the duplicated spawn-pipeline entries in the
architecture documentation: keep Command construction as step 8, change the
pre_exec hook to step 9, and increment each subsequent Spawn, Spawn monitor
task, and wait_ready entry accordingly.
---
Outside diff comments:
In `@src/supervisor/lifecycle.rs`:
- Around line 1535-1568: Move the post-exit output drain and final log flush out
of the synchronous lifecycle path around MonitorGuard, registering them as a
background task before the guard drops. Ensure port/state cleanup and exit hooks
proceed without awaiting the five-second drain deadline, while preserving sender
removal, local-line buffering, timeout behavior, and final flush within the
background task.
---
Nitpick comments:
In `@docs/.vitepress/config.mts`:
- Around line 177-185: Remove the securityLevel setting from the Mermaid
configuration object, leaving the existing flowchart wrappingWidth and
useMaxWidth options unchanged so Mermaid uses its default strict mode.
In `@src/supervisor/lifecycle.rs`:
- Around line 2609-2613: Update cleanup_db to be async and return
miette::Result<()>, replacing synchronous remove_file calls with tokio::fs
operations. Ignore only NotFound errors for the database, WAL, and SHM paths;
propagate all other failures through the miette result, and update callers to
await the cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 135328ad-3e56-40f8-82ae-c1e27d559ccf
📒 Files selected for processing (13)
AGENTS.mdARCHITECTURE.mddocs/.npmrcdocs/.vitepress/config.mtsdocs/.vitepress/theme/custom.cssdocs/.vitepress/theme/index.tsdocs/aube-lock.yamldocs/concepts/architecture.mddocs/package.jsondocs/pnpm-workspace.yamlmise.tomlsrc/supervisor/lifecycle.rssrc/supervisor/mod.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/.npmrc
- mise.toml
- ARCHITECTURE.md
- docs/pnpm-workspace.yaml
- docs/.vitepress/theme/custom.css
- docs/.vitepress/theme/index.ts
| 8. **Command construction**: | ||
| - `current_dir = opts.dir` | ||
| - `PATH` reset to the original user PATH (so daemons find user tools) | ||
| - Custom `env` from config applied first | ||
| - Pitchfork metadata injected after user env: `PITCHFORK_DAEMON_ID`, `PITCHFORK_DAEMON_NAMESPACE`, `PITCHFORK_RETRY_COUNT` | ||
| - `PORT` / `PORT0` / `PORT1` / ... set from resolved ports | ||
| - Proxy env via `inject_proxy_env` (see Proxy Integration) | ||
| 8. **`pre_exec` hook** (Unix): `setsid()` (so daemon PID == PGID, enabling process-group kills), optional `TIOCSCTTY` for PTY controlling terminal, and `apply_run_identity` for privilege switching. | ||
| 9. **Spawn**, capture PID, `PROCS.refresh_pids`, upsert daemon as `Running`. If a sink was started, hand its read end to `SinkPipe::supervise()` which keeps a sink running for the daemon's lifetime (replacing one that dies unexpectedly). | ||
| 10. **Spawn monitor task** (see Readiness Detection). | ||
| 11. If `wait_ready` is true, block on the readiness oneshot channel and return `DaemonReady` / `DaemonFailedWithCode`; otherwise return `DaemonStart` immediately. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the duplicated spawn-pipeline step number.
pre_exec is also labeled step 8, so the subsequent steps are ambiguous. Renumber pre_exec and the following entries sequentially.
Proposed documentation fix
-8. **`pre_exec` hook** (Unix):
-9. **Spawn**,
-10. **Spawn monitor task**,
-11. If `wait_ready` is true,
+9. **`pre_exec` hook** (Unix):
+10. **Spawn**,
+11. **Spawn monitor task**,
+12. If `wait_ready` is true,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 8. **Command construction**: | |
| - `current_dir = opts.dir` | |
| - `PATH` reset to the original user PATH (so daemons find user tools) | |
| - Custom `env` from config applied first | |
| - Pitchfork metadata injected after user env: `PITCHFORK_DAEMON_ID`, `PITCHFORK_DAEMON_NAMESPACE`, `PITCHFORK_RETRY_COUNT` | |
| - `PORT` / `PORT0` / `PORT1` / ... set from resolved ports | |
| - Proxy env via `inject_proxy_env` (see Proxy Integration) | |
| 8. **`pre_exec` hook** (Unix): `setsid()` (so daemon PID == PGID, enabling process-group kills), optional `TIOCSCTTY` for PTY controlling terminal, and `apply_run_identity` for privilege switching. | |
| 9. **Spawn**, capture PID, `PROCS.refresh_pids`, upsert daemon as `Running`. If a sink was started, hand its read end to `SinkPipe::supervise()` which keeps a sink running for the daemon's lifetime (replacing one that dies unexpectedly). | |
| 10. **Spawn monitor task** (see Readiness Detection). | |
| 11. If `wait_ready` is true, block on the readiness oneshot channel and return `DaemonReady` / `DaemonFailedWithCode`; otherwise return `DaemonStart` immediately. | |
| 8. **Command construction**: | |
| - `current_dir = opts.dir` | |
| - `PATH` reset to the original user PATH (so daemons find user tools) | |
| - Custom `env` from config applied first | |
| - Pitchfork metadata injected after user env: `PITCHFORK_DAEMON_ID`, `PITCHFORK_DAEMON_NAMESPACE`, `PITCHFORK_RETRY_COUNT` | |
| - `PORT` / `PORT0` / `PORT1` / ... set from resolved ports | |
| - Proxy env via `inject_proxy_env` (see Proxy Integration) | |
| 9. **`pre_exec` hook** (Unix): `setsid()` (so daemon PID == PGID, enabling process-group kills), optional `TIOCSCTTY` for PTY controlling terminal, and `apply_run_identity` for privilege switching. | |
| 10. **Spawn**, capture PID, `PROCS.refresh_pids`, upsert daemon as `Running`. If a sink was started, hand its read end to `SinkPipe::supervise()` which keeps a sink running for the daemon's lifetime (replacing one that dies unexpectedly). | |
| 11. **Spawn monitor task** (see Readiness Detection). | |
| 12. If `wait_ready` is true, block on the readiness oneshot channel and return `DaemonReady` / `DaemonFailedWithCode`; otherwise return `DaemonStart` immediately. |
🧰 Tools
🪛 LanguageTool
[grammar] ~223-~223: Ensure spelling is correct
Context: ..._readyis true, block on the readiness oneshot channel and returnDaemonReady/Dae...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/concepts/architecture.md` around lines 213 - 223, Renumber the
duplicated spawn-pipeline entries in the architecture documentation: keep
Command construction as step 8, change the pre_exec hook to step 9, and
increment each subsequent Spawn, Spawn monitor task, and wait_ready entry
accordingly.
| ### `close()` (supervisor shutdown) | ||
|
|
||
| 1. Shut down IPC (stop accepting new requests). | ||
| 2. Force-flush state. | ||
| 3. Stop daemons in **dependency reverse order** (`compute_reverse_stop_order`), concurrent within each dependency layer. | ||
| 4. Wait up to 5s for all monitor tasks to exit (`active_monitors` counter + `monitor_done` Notify), ensuring their hook tasks have been registered. | ||
| 5. Wait for all hook tasks to complete (30s timeout per task). | ||
| 6. Cancel proxy / web / mDNS tasks. | ||
|
|
||
| ## Monitor Task Exit Path | ||
|
|
||
| When the spawned process exits, the monitor task performs cleanup in a specific order to handle concurrent `stop()` / `start()` races correctly: | ||
|
|
||
| 1. **Acquire `MonitorGuard`** (RAII counter via `active_monitors` + `monitor_done` Notify) at the start of the monitor task. It covers every task exit path, so `close()` cannot observe `active_monitors == 0` before any drain or hook task has been registered. | ||
| 2. **Snapshot `stop_intent_observed`** before any cleanup that could race with a concurrent `start()`. Computed as `was_stopped_before_cleanup || was_stopping_before_cleanup` from the pre-cleanup daemon state. This preserves the intentional-stop signal even if `start()` (e.g. from `pitchfork restart`) upserts a new PID with `Running` status during cleanup. | ||
| 3. **Drain in-flight output** synchronously: the `output_rx` channel is drained for up to 5s (or until EOF) and lines are flushed to the log store. The `OutputRelay` is dropped first so its sender doesn't keep the channel open. Sink-relayed lines (`OutputSource::Sink`) are already stored and skipped. Only applies to the in-process path; sink-path daemons are drained by the sink process itself. | ||
| 4. **Clear `active_port`** only if the daemon record still has this monitor's PID, checked while holding the state-file lock. A concurrent `start()` with a replacement PID keeps its new `active_port`. | ||
| 5. **PID-takeover check + exit reason + hooks**: if `stop_intent_observed` is false and a new process has taken over (different PID, not `Stopping`/`Stopped`), spawn the background drain and return without updating state or firing hooks. Otherwise: | ||
| - Combine the pre-cleanup snapshot with the current state to cover both race orders: `stop()` set `Stopping` before cleanup (`stop_intent_observed`), or `stop()` set `Stopped` during cleanup (`already_stopped`). | ||
| - **Determine `exit_reason`**: `"stop"` (intentional), `"exit"` (clean self-exit), or `"fail"` (non-zero). | ||
| - **Update state** unless `stop()` already did it or `stop_intent_observed` is true (avoid undoing a restart that upserted `Running` during cleanup). | ||
| - **Fire hooks** based on `exit_reason`: | ||
| - `"stop"` → `OnStop` + `OnExit` | ||
| - `"exit"` → `OnExit` | ||
| - `"fail"` with retries exhausted → `OnFail` + `OnExit` | ||
| - `"fail"` with retries remaining → no hooks (retry loop handles it) | ||
| 6. **Drain complete**: output was already drained inline in step 3, so no further drain is needed. The `MonitorGuard` at the top of the task drops as the task ends. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document the background output-drain shutdown contract.
These sections still describe the 5-second drain as synchronous and only list monitor/hook-task waiting. That conflicts with the PR objective of moving the drain to a fire-and-forget task; the architecture should state that drain handles are registered and awaited under a bounded shutdown timeout, otherwise readers may assume trailing output is flushed before the monitor exits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/concepts/architecture.md` around lines 313 - 340, Update the `close()`
shutdown sequence and Monitor Task Exit Path to document that output draining
runs in a background fire-and-forget task, with drain handles registered before
monitor completion. Include those handles in shutdown waiting under the bounded
timeout, and remove wording that implies the monitor synchronously drains output
or that all trailing output is flushed before it exits.
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
2 participants
Summary
This PR fixes a concurrency bug in the monitor task exit path, rewrites the architecture documentation to reflect the current codebase, and enables mermaid diagram rendering in the VitePress docs.
Changes
1. Monitor exit path race condition fix (
src/supervisor/lifecycle.rs)Three issues identified and fixed in the monitor task exit path:
MonitorGuard placed after drain (real bug):
close()could observeactive_monitors == 0during the 5s output drain window and callexit(0)before hooks were registered. MovedMonitorGuardacquisition before the drain.Drain blocking critical path: The 5s synchronous drain was on the critical path, blocking
close()andstart(). Moved it to a fire-and-forget background task (spawn_output_drain()) so the 5s window no longer blocks supervisor shutdown or daemon restart.Unclear naming: Renamed
pre_drain_is_stoppingtostop_intent_observed, split intowas_stopped_before_cleanup/was_stopping_before_cleanupfor clarity about what the snapshot represents.2. Architecture documentation rewrite (
docs/concepts/architecture.md)The existing architecture doc was outdated. Rewrote it with the full technical overview:
src/supervisor/submodules)run_once(), privilege switching, PTY allocation)tokio::select!loop, first-ready-wins, timeout exhaustion)stop(),close())Added
ARCHITECTURE.mdsymlink at the repo root pointing to the VitePress source, and referenced it prominently inAGENTS.md.3. Mermaid diagram rendering (
docs/)Enabled mermaid diagrams in VitePress via
vitepress-plugin-mermaid+mermaid 11.16.0.Fixed text clipping caused by mermaid 11.15+ bug #7759 (themeCSS lowercases
foreignObjectselector, breaking XML case-sensitive CSS matching) with three layers:flowchart.wrappingWidth: 1000in mermaid config (default 200 clips long node text)foreignObjectandforeignobjectspellingsMutationObserverto setoverflow="visible"on asynchronously rendered nodesVerified in headless Chromium: all 17 visible
foreignObjectnodes haveoverflow="visible"with no clipping, on both desktop (1440px) and mobile (390px) viewports.4. Test tooling note (
mise.toml)Added a comment above the
testtask documenting that local developers should install GNU parallel to enablebats --jobs 16parallel execution.Verification
cargo nextest run: 476/476 passedmise run test:bats: 279/279 passed (8 slow tests skipped)cargo clippy --all-targets --all-features -- -D warnings: cleanmise run build:docs: builds successfullyThis PR body was generated by Claude Code.
Summary by CodeRabbit
Documentation
Bug Fixes