Repository navigation
feat: add per-service runtime monitor core - #412
Conversation
…open ProcessExitWatch::with_exec reports a watched process's exec as well as its exit, without reaping it, and exposes its kqueue descriptor so a Tokio AsyncFd can await it. PvPaths gains run/m/<12 hex> monitor directories and their reservation locks, sharing the 12-digit SHA-256 digest worker admin sockets use. MonitorReservation takes the exclusive lock, hands the same locked file to a child as stdin, and lets that child adopt and verify it. open_private_log_file opens a runtime log without following symlinks and checks owner, type and link count on the opened descriptor.
A monitor process owns one runtime for its whole life. It starts the runtime through a gate that execs only once the monitor has written its recovery record, so the runtime keeps the gate's pid and birth identity. The monitor learns the exit status from kqueue without reaping, so the zombie leader pins the process group until cleanup is proven, and it reaps the leader exactly once after that. Controllers use start_monitor, monitor_state, stop_monitor, release_monitor and recover_monitor over a bounded NDJSON protocol (version 1) that checks the peer and the instance on both sides. Release removes records only once the reservation proves the monitor exited. Recovery stops a dead monitor's runtime only by its exact identity from this boot. The monitor rotates its runtime's log by copy and truncate on the retained descriptor and keeps five archives that `pv logs` lists. Signalling a group whose only members are zombies now counts as done, because macOS answers EPERM instead of ESRCH for one. Nothing in production starts a monitor yet.
`pv monitor:run` and `pv monitor:gate` run the monitor engine. Both need process containment and skip runtime lifecycle admission, which a monitor holding it for its whole life would keep `daemon:disable` from taking. A smoke test drives the real binary through start, stop and release.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The stop command no longer has the deadline that could prematurely report healthy cleanup as unavailable. The remaining concern is targeted regression-test coverage, not a demonstrated runtime failure. Security Architecture Review
Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 46.63% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 178 functions across 27 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the primary change: adding the per-service runtime monitor core. It is concise and directly related to the changeset.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
-
📝 Generate docstrings 💡
-
-
-
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
-
🧪 Generate unit tests (beta)
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
crates/daemon/src/monitor/macos.rs (1)
208-209: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBase the
stop_monitordeadline on the stop that the monitor actually runs.
stop_monitorsets its deadline from the caller'sstop.grace()plusSTOP_MARGIN. The monitor does not always use that stop:
Command::Stoprunsstart_cleanup. If an executor already runs,start_cleanupreturns early. This happens after a self-exit withfallback_stopor after an earlier stop request.- The test
monitor_stop_outlives_its_client_and_keeps_its_first_deadlineconfirms that the first deadline wins.Example: the runtime exits by itself with a 10 s
fallback_stopand a surviving child. A controller then callsstop_monitorwith a 1 s grace. After about 6 s,stop_monitorreturnsMonitorUnavailable("the runtime's cleanup did not finish"). The cleanup is still running and later reachesComplete. The caller then treats a healthy monitor as failed.Take the deadline from the longest grace that can apply. One option is to make the monitor report its active stop in
MonitorStateand extend the deadline from that value. A simpler option is to use the maximum of the requested grace andrecord.fallback_stop.grace(), plus the margin and the 1 s kill wait.Proposed fix
- request(paths, subject, Operation::Stop(stop)).await?; - let deadline = Instant::now().checked_add(stop.grace().saturating_add(STOP_MARGIN)); + let (record, _accepted) = request(paths, subject, Operation::Stop(stop)).await?; + // The monitor keeps the first stop, or its fallback, if a cleanup already runs. + let grace = stop.grace().max(record.fallback_stop.grace()); + let deadline = Instant::now().checked_add(grace.saturating_add(STOP_MARGIN));This fix does not cover an earlier controller stop with a longer grace. That case still needs the active stop in
MonitorState.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
3ca5b064-0c50-474b-80f8-70ffd2889768
⛔ Files ignored due to path filters (7)
Cargo.lockis excluded by!**/*.lockcrates/daemon/tests/snapshots/monitor__monitor_requests_follow_the_frozen_contract.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/monitor__monitor_runs_a_runtime_through_stop_and_release.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/monitor__monitor_start_reports_a_missing_command_and_leaves_no_instance.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_bash_script.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_fish_script.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_zsh_script.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
Cargo.tomlcrates/cli/src/args.rscrates/cli/src/commands/daemon.rscrates/cli/src/commands/logs.rscrates/cli/src/commands/mod.rscrates/daemon/examples/pv-monitor.rscrates/daemon/src/error.rscrates/daemon/src/lib.rscrates/daemon/src/monitor.rscrates/daemon/src/monitor/macos.rscrates/daemon/src/monitor/rotation.rscrates/daemon/src/supervisor.rscrates/daemon/tests/monitor.rscrates/platform/src/lib.rscrates/platform/src/process.rscrates/platform/src/process/macos.rscrates/platform/src/process/unsupported.rscrates/platform/tests/process_identity.rscrates/pv-fake/src/install.rscrates/pv-fake/src/lib.rscrates/state/src/fs.rscrates/state/src/lib.rscrates/state/src/paths.rscrates/state/src/update_lock.rsit/cli.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two gaps need attention: test-process death during early startup pauses can leak monitor fixtures indefinitely, and normal controller requests omit reply-version validation.
Reviewed changes Full review of the per-service monitor core, its platform/state primitives, hidden CLI entry points, and tests; production daemon integration remains intentionally deferred.
- Runtime lifecycle: Record-before-exec gate startup, kqueue exec/exit observation, unreaped-leader group cleanup, and first-stop deadline ownership.
- Controller protocol: Authenticated state/stop/release requests, reservation-based release, and identity-checked recovery after monitor death.
- State and logging: Per-subject monitor paths and inherited reservations, private log descriptors, copy/truncate rotation, and archive retention.
- CLI and tests: Hidden command routing, platform capability mapping, process-contract fixtures and snapshots, and log-follow regression coverage.
Validation: both targeted portable CLI tests and cargo fmt --all --check passed. Used cargo test because cargo nextest is unavailable; macOS process-contract tests could not run in this Linux environment.
gpt-6.1-sol ｜ 𝕏
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 259602b0cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stop: MonitorStop, | ||
| ) -> Result<MonitorState, DaemonError> { | ||
| request(paths, subject, Operation::Stop(stop)).await?; | ||
| let deadline = Instant::now().checked_add(stop.grace().saturating_add(STOP_MARGIN)); |
There was a problem hiding this comment.
When a stop is already underway, the monitor deliberately preserves only the first request and its deadline, but this macOS controller computes the polling timeout from the new caller's stop. If an initial caller requests a 60-second grace and disconnects, then a retry requests zero grace, the retry reports MonitorUnavailable after roughly five seconds even though cleanup is correctly proceeding under the accepted 60-second policy; derive the timeout from the monitor's recorded first stop instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 049fcff by removing the controller-side deadline instead of recomputing it. Whichever request started the monitor's stop (the first stop, or the fallback after the runtime exited on its own), it ends within that stop's grace period plus the 1 s SIGKILL wait. Each state request also has its own 5 s timeout, which covers a monitor that stops answering. A shorter retry now waits for the stop that is actually running. The repeated-stop test now calls stop_monitor for its second stop.
| tokio::select! { | ||
| events = next_optional_events(&mut self.watch) => self.observe(events?, hooks), | ||
| accepted = self.listener.accept() => { | ||
| if let Ok((stream, _address)) = accepted { |
There was a problem hiding this comment.
On macOS, every listener error is silently discarded. A persistent error such as EMFILE or ENFILE can leave this select arm repeatedly ready, causing the monitor to busy-loop while clients merely time out and the underlying failure is never recorded; handle terminal errors or retain the error and back off retryable failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 049fcff: an accept error now waits 50 ms before retrying, the same ACCEPT_ERROR_BACKOFF the daemon's IPC and DNS servers use.
| if reply.instance != record.instance { | ||
| return Err(unavailable(subject, "the reply came from another instance")); | ||
| } |
There was a problem hiding this comment.
When a monitor from another application release returns a reply whose schema still deserializes, this path validates only the instance and accepts state/accepted even if reply.version differs. That defeats the version field on every message and can let a new controller perform stop or release using incompatible monitor semantics instead of reporting a protocol mismatch; validate reply.version before consuming the result, as startup already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same fix as the pullfrog thread above, in 049fcff: request() now checks the reply's version along with its instance before using the result.
| "[year][month][day]-[hour][minute][second]-[subsecond digits:3]" | ||
| ))?; | ||
| fs::rename(&temporary, &parent.join(format!("{stem}-{stamp}.log")))?; | ||
| log.set_len(0)?; |
There was a problem hiding this comment.
When a runtime refills the truncated file beyond a pv logs --follow reader's previous offset before linemux handles the filesystem event, linemux 0.3 sees a size that never decreased and resumes from the stale offset, silently dropping the beginning of the new log. This is reproducible with an 11-byte old file, where a longer post-rotation line is emitted only as its suffix; the added test cannot catch this because its replacement line is shorter than the previous offset, so rotation needs a mechanism that lets followers unambiguously reset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this one. linemux 0.3 does detect truncation only when the size has shrunk below the reader's offset (reader.rs:496). But a monitor rotates only once a log passes 10 MiB, so a follower that is caught up sits at 10 MiB or more. To miss the truncation, the runtime would have to write over 10 MiB before linemux handles one file event, and the effect is only on pv logs --follow output, not on stored logs. If it ever matters, the fix belongs in the follower (for example, checking the inode or file size on each event), not in rotation.
| let written = write_line(&mut connection, &Reply::new(instance, result)).await; | ||
| if released { | ||
| send(commands, Command::Exit).await?; | ||
| } | ||
| written?; |
There was a problem hiding this comment.
If a release client disconnects or is canceled after submitting its request, write_line can fail, but this path still sends Command::Exit before propagating that failure. The monitor exits while release_monitor never reaches the reservation wait and directory removal, leaving monitor.json behind so the next start_monitor returns MonitorUnresolved until an explicit recovery; make accepted release cleanup self-contained or avoid committing the exit when the response was not delivered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 049fcff: the monitor now removes its own records when it accepts a release. It still holds the reservation at that point, so no replacement can own those records; the path it takes is the one it already used to release itself when the lifeline closes. release_monitor now only waits for the reservation to prove the monitor exited, and its re-read-and-compare code is gone. A test now releases with a bare request and no controller-side cleanup, then checks the records are gone.
A monitor now removes its own records when it accepts a release, while it still holds the reservation, so a controller that disconnects after releasing no longer leaves records that block the next start. release_monitor only waits for the reservation. stop_monitor no longer times out on the caller's grace period. The monitor's stop is bounded by the stop it actually runs, and each state request has its own timeout. Controllers reject replies from another protocol version, as startup already did. The monitor backs off after an accept error instead of spinning, as the daemon's servers do. Test pause points give up after 150 s, so a test that dies at one leaves no paused monitor behind.
|
@coderabbitai Thanks. On your two review-body notes:
I'm not acting on the docstring-coverage warning. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Re-reviewed 049fcffc against the prior Pullfrog review at 259602b0, with the full PR diff as context.
- Fixed reply validation so normal controller requests reject incompatible protocol versions as well as mismatched instances.
- Bounded fixture pauses so early startup holds no longer prevent eventual cleanup after the test process dies.
- Moved release cleanup into the monitor while its reservation is held, and updated the raw-release contract test to verify record removal without controller-side cleanup.
- Removed the competing stop timeout so repeated calls wait for the monitor's original stop executor, and updated the repeated-stop test to use the controller API.
- Added accept-error backoff to prevent a persistent listener failure from causing a busy loop.
Validation: cargo fmt --all --check and the portable monitor CLI capability test passed. The initial test build exceeded the tool's 120-second limit; retry passed. The changed macOS process-contract tests could not run on this Linux runner, and macOS CI was still in progress. Separate-controller death during early pauses and incompatible authenticated replies remain untested directly.
gpt-6.1-sol ｜ 𝕏
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 049fcffc28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// The monitor removes its own records before it exits; its reservation proves it has exited. | ||
| pub async fn release_monitor(paths: &PvPaths, subject: &str) -> Result<(), DaemonError> { | ||
| request(paths, subject, Operation::Release).await?; | ||
| drop(wait_for_reservation(paths, subject).await?); |
There was a problem hiding this comment.
If removing the monitor directory fails during Command::Exit—for example, because its permissions changed or the filesystem reports an I/O error—the monitor exits and releases its reservation, so this wait succeeds and release_monitor returns Ok(()) even though monitor.json remains and the next start fails with MonitorUnresolved. The disconnect case is now handled, but the current deletion result is still lost across process exit; verify record removal after acquiring the reservation or acknowledge the deletion result before reporting release success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as is. Getting here takes a failed delete of PV's own 0700 directory, which means a disk or permissions fault. Nothing is lost when it happens: the next start_monitor reports MonitorUnresolved, and recover_monitor finds no live group member for the recorded runtime, so it removes the leftover records without sending any signal. Having the controller remove records again after release would also need the instance comparison we just dropped, so that a release never deletes a replacement's evidence; that isn't worth it for this case.
|
|
||
| fn read_record(paths: &PvPaths, subject: &str) -> Result<Option<MonitorRecord>, DaemonError> { | ||
| match fs::read_to_string(&record_path(paths, subject)) { | ||
| Ok(content) => Ok(Some(serde_json::from_str(&content)?)), |
There was a problem hiding this comment.
When a dead monitor leaves a record from another application version whose JSON remains structurally deserializable, recover_monitor trusts its process identity and fallback stop policy without checking record.version, potentially signaling the recorded group and deleting recovery evidence under incompatible semantics. Validate the persisted version before any recovery action; this is distinct from validating the live monitor's reply version.
AGENTS.md reference: AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 388e7a9. read_record is the one loader for start, requests and recovery, and it now refuses a record whose version isn't this protocol's with InvalidRuntimeRecord, before any signal or deletion. The recovery test now covers a foreign-version record that has the exact runtime identity from this boot: recovery refuses it, sends no signal and keeps the record.
Starting, requesting and recovering a monitor all read its record through one loader, which now refuses a record written under another protocol version instead of trusting its identities and stop policy.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Re-reviewed 388e7a97 against the prior Pullfrog review at 049fcffc, with the complete PR diff as context.
- Rejected incompatible monitor records in the shared reader before startup, controller requests, or recovery can act on their identities or stop policy.
- Extended recovery regression coverage with an exact-identity, current-boot runtime recorded under protocol version 2, checking typed rejection, record preservation, and that the runtime remains alive.
Validation: cargo fmt --all --check passed. The changed implementation and regression test are macOS-only, so behavioral tests could not run on this Linux runner; all four macOS CI lanes were still in progress when checked.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 388e7a9782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// The monitor removes its own records before it exits; its reservation proves it has exited. | ||
| pub async fn release_monitor(paths: &PvPaths, subject: &str) -> Result<(), DaemonError> { | ||
| request(paths, subject, Operation::Release).await?; | ||
| drop(wait_for_reservation(paths, subject).await?); |
There was a problem hiding this comment.
When another controller starts the same subject immediately after the accepted release, the replacement can acquire the reservation before this poll does. wait_for_reservation cannot distinguish the replacement's lock from the released monitor's lock, so it waits five seconds and returns MonitorUnavailable even though the requested instance exited successfully; use the MonitorRecord returned by request to recognize that handoff and add concurrent process-level coverage for it.
AGENTS.md reference: AGENTS.md:L9-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. Today only tests call release_monitor. In PR 4 the only controller that starts and releases monitors is the daemon, and DESIGN.md has it run reconciliation jobs one at a time through jobs.lock, so a release and a start for the same subject never overlap. The other path that stops runtimes, daemon:disable and uninstall, stops the daemon first and holds the runtime lifecycle lock exclusively. If it ever did happen, the cost is a spurious MonitorUnavailable after 5 s with no state damage. A free reservation is also a stronger proof that the monitor exited than a pid and birth-identity check, which depends on how macOS reports exiting processes.
| // A dot file, so `pv logs` never shows a partial archive. | ||
| let temporary = parent.join(format!(".{name}.rotating")); | ||
| fs::remove_file_if_exists(&temporary)?; | ||
| let mut archive = fs::create_new_file(&temporary)?; |
There was a problem hiding this comment.
When a runtime reaches the rotation threshold under a typical 022 umask, create_new_file creates this temporary archive as 0644, and neither this path nor the generic helper subsequently applies secure_sensitive_file; the rename preserves that mode. The archive contains the same potentially sensitive output as the active log, which open_private_log_file explicitly forces to owner-only 0600, so create the temporary archive through a sensitive-file helper or explicitly secure it and assert the archive mode in rotation coverage.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. The boundary here is the directory: DESIGN.md requires ~/.pv to be 0700, pv setup creates it that way and pv doctor checks it, so other users can't reach anything under ~/.pv/logs whatever each file's mode. DESIGN.md's list of 0600 files is pv.db, CA private keys and generated secrets, and logs aren't on it. Changing the archive mode wouldn't close any real exposure.
| let stamp = OffsetDateTime::now_utc().format(format_description!( | ||
| "[year][month][day]-[hour][minute][second]-[subsecond digits:3]" | ||
| ))?; | ||
| fs::rename(&temporary, &parent.join(format!("{stem}-{stamp}.log")))?; |
There was a problem hiding this comment.
Although copy_prefix syncs the temporary archive's contents, this rename changes the parent directory without syncing it before set_len(0) truncates the active log. If the machine or kernel crashes after rotation, the filesystem may persist the truncation while losing the unsynced archive directory entry, leaving neither durable copy of those log bytes; call sync_parent_directory after the rename and before truncation, as the existing atomic-copy helper already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. PV never syncs log data: runtimes append to their active logs without fsync, and sync_parent_directory is used only for atomic config and state writes. A crash already loses the tail of any active log, so making the archive's directory entry durable would protect data that wasn't durable to begin with.
… cleanup On macOS, killpg answers EPERM when no member of a group can take a signal: each is an unreaped zombie or already exiting. A killed runtime's children are briefly in that state, often for far longer when they hold a lot of memory. signal_process_group accepted EPERM only once the group had already exited, so a monitor's fallback stop that started while a child was still tearing down failed, and the monitor marked its cleanup unproven, which blocks the runtime until a reboot. EPERM now leaves the decision to the group-exit wait that every caller already runs. A pv-fake descendant can now hold resident memory, which makes the test reproduce this every time.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/daemon/tests/monitor.rs (1)
267-270: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftMake the
EPERMregression test exerciseEPERM.The test can reach monitor cleanup after the direct group kill, but
DescendantResidentonly proves that the descendant allocated memory. It does not prove thatsignal_process_groupreceivesPermissionDeniedwhile the descendant is still exiting. The cleanup assertions can therefore pass without distinguishing the changed branch. Add a targeted signal seam or assertion that confirmsEPERMoccurred before asserting cleanup.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
877d2328-c028-4da5-8d60-52c0da57b454
📒 Files selected for processing (6)
crates/daemon/src/supervisor.rscrates/daemon/tests/monitor.rscrates/pv-fake/src/events.rscrates/pv-fake/src/fake.rscrates/pv-fake/src/lib.rscrates/pv-fake/tests/contracts.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Re-reviewed f9a9f593 against the prior Pullfrog review at 388e7a97, with the complete PR diff as context.
- Fixed exiting-group cleanup by allowing
EPERMto proceed to the existing group-exit wait instead of immediately marking cleanup unproven; successful cleanup still requires verified absence of live members. - Added resident-memory fixture support so a fake descendant can retain memory and report when it is ready for the teardown scenario.
- Extended monitor regression coverage by killing a runtime group with a memory-resident descendant and requiring signal-9 exit, complete cleanup, and successful release.
Validation: cargo fmt --all --check and the portable descendant lifecycle test passed. Used cargo test because nextest is unavailable. The new macOS teardown regression could not run on this Linux runner; Linux and Windows portability checks passed, while all four macOS CI lanes remained in progress.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9a9f593c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match platform::inspect_process_start_identity(runtime.pid)? { | ||
| Some(identity) if identity == runtime.start_identity => { | ||
| stop_process_group( |
There was a problem hiding this comment.
When a current-boot runtime retains its PID and start identity but re-execs to a different executable, this branch still treats it as owned and signals its entire process group. That is weaker than the persisted-runtime ownership invariant in CONTRIBUTING.md, and it leaves the recorded command and arguments unused during recovery. Validate the full live process identity against the record before the initial signal and again before escalation, otherwise recovery can terminate a group that no longer matches the recorded runtime.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one. The pid plus its start time already rule out pid reuse, and that's what this check guards against. A runtime that re-execs keeps that identity because it's still the process PV started, in PV's own process group, so stopping its group is still correct. Checking the recorded command would also be wrong here: the recorded runtime begins as the pv monitor:gate process and only becomes the command after exec. If a monitor dies before that exec, a command check would refuse to stop the gate and leak it.
| let rotation_period = hooks | ||
| .rotation_interval_ms | ||
| .map_or(ROTATION_INTERVAL, Duration::from_millis); | ||
| let mut rotation_tick = interval_at(Instant::now() + rotation_period, rotation_period); |
There was a problem hiding this comment.
When test-hook JSON supplies rotation_interval_ms: 0, this passes Duration::ZERO to Tokio's interval_at, which asserts that the period is nonzero. The runtime has already started and the successful startup reply was sent before this code runs, so the monitor process panics and leaves the runtime running with recovery records instead of supervising its lifeline; reject zero during hook loading/startup or represent the interval as a nonzero type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one. Only test hook files can set the rotation interval. Production pv monitor:run always uses MonitorHooks::default() (crates/cli/src/commands/daemon.rs), and only the pv-monitor test example reads pv-monitor-hooks.json. The one test that sets it uses 20 ms.
| // A held rotation or the lifeline reader must not keep the monitor alive. | ||
| runtime.shutdown_background(); |
There was a problem hiding this comment.
When release or lifeline cleanup completes while the blocking rotation task is copying, the monitor returns without joining that task and this shutdown lets the process exit immediately. rotate_if_needed has already created .NAME.rotating, while its removal path only runs when copy_prefix returns an error normally, so process termination leaves a partial archive behind; if the runtime is not restarted, that hidden file persists indefinitely and can duplicate much of an arbitrarily large active log. Unlink the temporary archive as part of intentional monitor shutdown or otherwise make cancellation clean it up without waiting for the copy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one. The next rotation of that log deletes any leftover .NAME.rotating file before creating a new one (rotation.rs:49). So it's at most one hidden file per log, about the 10 MiB rotation threshold. It also needs a release or a closed lifeline to land inside the few milliseconds the copy takes. I don't think that's worth a cancellation path.
|
@coderabbitai on the nitpick that |
|
Tip For best results, initiate chat on the files or code changes.
I withdraw the nitpick. No production signal seam is needed for this test. ✏️ Learnings added
You are interacting with an AI system. |
1 participant
PR 3 of the per-service monitor stack. It adds the monitor engine, the hidden
pv monitor:runandpv monitor:gatecommands, and process-level contract tests. Nothing in production starts a monitor yet: PR 4 switches the daemon over, and PR 5 removes legacy adoption.How a monitor runs a runtime
run/m/<12 hex>.lockand hands the same locked file to the monitor as stdin, so the lock is held with no gap until the monitor dies. Release and recovery use that lock as proof that the monitor has exited.run/m/<12 hex>/monitor.json(no secrets) before the gate may exec. Exec is confirmed by the kqueue exec event or the gate's own error report; the socket closing alone never counts.Protocol v1
One request per connection over bounded NDJSON (
protocol::transport, 64 KiB). The monitor checks the peer's UID. The controller checks the peer pid, start identity and boot againstmonitor.json, and both sides checkversionandinstance. Unknown fields are ignored.statestop,exit(codeorsignal),cleanup(pending,completeorunproven),log_rotation_errorstop {signal, grace_ms}acceptedimmediatelyreleaseacceptedonly once the exit is known and cleanup is complete; otherwisenot_releasableError kinds:
unsupported_version,instance_mismatch,not_releasable,invalid_requestandstartup_failed. The frozen replies are snapshotted inmonitor__monitor_requests_follow_the_frozen_contract.snap.The controller API, which PR 4 reuses, is
start_monitor,monitor_state,stop_monitor,release_monitorandrecover_monitor. Recovery refuses a live monitor because the reservation is held. A record from a previous boot is removed without sending any signal. A runtime with its exact recorded identity is stopped. A changed identity is refused.Log rotation: the monitor copies and truncates the retained append descriptor once the log passes 10 MiB (checked every 60 s), and keeps five
<stem>-YYYYMMDD-HHMMSS-mmm.logarchives thatpv logsalready lists. Output written between the copy and the truncation is lost; that window is accepted.Two macOS behaviors found while testing
killpganswersEPERM, notESRCH, when no member can take a signal. That covers a group whose only members are zombies, and also members that are still exiting.EPERMfor more than 10,000 polls during its teardown.signal_process_groupnow treatsEPERMas "signalled" and lets the group-exit wait that every caller runs decide the stop.supervisor.rs; everything else there is visibility, plus a closure-basedstop_process_groupthat the old by-pid stop now calls.EPERMonly once the group had already exited. A monitor's fallback stop could then start while a child was still tearing down, fail, and mark cleanup unproven, blocking the runtime until a reboot.monitor_cleans_up_a_killed_group_whose_member_is_still_exitingfailed 5/5 with that version and passes 20/20 now.Changes from the approved plan
versionop. Every message carriesversion.watch), explicit stop on cancellation, directory listing, the Postgres evidence policy, real-artifact tests, choosing the installed release executable, and the lifeline during readiness and daemon restarts.pv-monitor-hooks.jsonin the test's PV home, not aPV_MONITOR_TEST_HOOKSvariable. The controller already sets the monitor'sHOME, so no environment handling is needed. Productionpvalways uses the defaults.monitor/rotation.rsif you'd rather review it on its own.Footprint
Measured on a release build, idle, with
/bin/sleepas the runtime:topphysical footprint: 3.2 MB)Test plan
cargo nextest run --profile ci --workspace --all-features --locked: 1654 passed, also with a stress run going alongside. After f9a9f59: 1655 passed.cargo nextest run -p daemon --profile ci -E 'binary(monitor) | test(process_watch)' --stress-count 20under load: 20/20.cargo clippy --workspace --all-targets --all-features --locked -- -D warnings,cargo fmt --all --check,cargo shear.ring, which needs a cross C compiler.Tests added:
crates/daemon/tests/monitor.rs(18 contract tests):descendant_resident_mibsettingSIGSTOPped monitorpv logsfollow-across-truncation test.pvsmoke test init/cli.rs.Summary by CodeRabbit