Repository navigation
feat(daemon): run production runtimes through monitors - #414
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
Merge Risk | 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 67.03% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 364 functions across 28 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: production daemon runtimes now run through per-runtime monitors. 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.
- 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.
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. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The cutover can discard ownership/recovery evidence without a verified stop. Three lifecycle issues need fixing before merge, including PostgreSQL cleanup and prior-boot recovery.
Reviewed changes Reviewed the production monitor cutover and the helper-only follow-up that arrived during this review.
- Runtime supervision: Gateway, PHP workers, and Managed Resources now start, adopt, stop, and record config proofs through per-runtime monitors.
- Lifecycle transitions: Startup migrates legacy runtimes; reconciliation retires orphan monitors; disable/uninstall enumerate monitors and retain unproven PostgreSQL exits.
- Cleanup and coverage: Reviewed the EPERM group-exit handling, held-exit logging, fixture lifelines, migrated tests, new regressions, and platform-specific helper cleanup.
- Validation: Formatting and the portable readiness test pass. The broader Linux library run was not usable for macOS lifecycle validation (unsupported platform/capabilities and missing fixture examples); artifact E2E failed before runtime execution because
artifacts.prvious.testdid not resolve.
gpt-6.1-sol ｜ 𝕏
| MonitorStopped::Stopped(state) => { | ||
| structured_log::runtime_exited(paths, subject, &exit_description(state.exit)); | ||
| monitor::release_monitor(paths, subject).await | ||
| } | ||
| if started_at.elapsed() >= PROCESS_IDENTITY_TIMEOUT { | ||
| return Err(DaemonError::MissingProcessIdentity { | ||
| name: spec.name.clone(), | ||
| pid, | ||
| MonitorStopped::Unavailable => monitor::recover_monitor(paths, subject).await, |
There was a problem hiding this comment.
[P1] Reconciliation can discard the PostgreSQL evidence that offline shutdown deliberately retains. A monitor holding Signal(9) or a nonzero exit is released here after group cleanup, and an unavailable monitor is recovered without the PostgreSQL check; resource replacement/undemanded cleanup then deletes its records even though escaped backends have not been proven gone.
Technical details
# Preserve PostgreSQL cleanup proof across all retirement paths
## Affected sites
- `crates/daemon/src/supervisor.rs:1299-1303`: generic monitored stop releases any completed exit or recovers an unavailable monitor.
- `crates/daemon/src/supervisor.rs:432-452`: offline shutdown correctly retains unsuccessful/unavailable PostgreSQL exits.
- `crates/daemon/src/managed_resources/mod.rs:2458-2473`: replacement uses generic stop, deletes metadata, and starts a replacement.
- `crates/daemon/src/managed_resources/mod.rs:2661-2675`: undemanded cleanup uses generic stop before deleting records/releasing ports.
## Required outcome
- Current-boot PostgreSQL retirement must not discard a held unsuccessful exit or unavailable-monitor evidence without proving escaped backends gone.
- A failed offline stop must not become removable merely by re-enabling the daemon and running resource reconciliation.
- Add replacement and undemanded-resource integration regressions for a killed/nonzero PostgreSQL runtime, asserting retained monitor/metadata and no replacement or successful stopped observation.
## Suggested approach
- Share the PostgreSQL clean-exit admission rule between generic and offline retirement, retaining the prior-boot exception separately.Related paths: replacement, undemanded cleanup.
There was a problem hiding this comment.
Skipping this one: it isn't new with monitors. Before this PR, reconciliation restarted a crashed Postgres the same way: adopt_recorded found the dead pid and a fresh start replaced the records. Undemanded cleanup removed them too. Postgres also guards itself: a new postmaster refuses to start while old backends still hold its shared memory. The keep-the-records rule exists for pv daemon:disable and pv uninstall, which go on to delete binaries and data. Those paths still require a clean exit 0.
| if let Err(error) = runtime_stop::stop_legacy_runtimes(&paths).await { | ||
| structured_log::legacy_runtime_stop_failed(&paths, &error.to_string()); | ||
| } |
There was a problem hiding this comment.
[P1] A failed legacy stop is logged, but the following reconciliation can erase the failed runtime’s ownership records. For an installed resource with no remaining demand, stop_resource_runtime finds no monitor, treats stop_subject(false) as success, and deletes the legacy PID/JSON while its process may still be alive, leaving later disable/uninstall unable to stop it safely.
Technical details
# Keep failed legacy cutover subjects blocked
## Affected sites
- `crates/daemon/src/jobs.rs:490-500`: legacy-stop failure does not constrain subsequent System reconciliation.
- `crates/daemon/src/supervisor.rs:243-355`: legacy stop deliberately preserves records on identity mismatch or unproven cleanup.
- `crates/daemon/src/supervisor.rs:469-470`: stop_subject returns false when no monitor is recorded, without checking retained legacy records.
- `crates/daemon/src/managed_resources/mod.rs:2625-2705`: undemanded resource cleanup records Stopped, deletes legacy records/config, and releases ports despite that false result.
## Required outcome
- Failed legacy stops must retain their exact ownership records and prevent replacement/undemanded cleanup for that subject, while independent reconciliation may continue.
- Add a startup integration test with an undemanded legacy resource whose verified stop fails while the runtime remains alive; assert retained records, a reported failure, and no successful stopped observation.
## Suggested approach
- Treat a remaining legacy PID record as an unresolved lifecycle transition before monitor-only start/cleanup paths can proceed, or carry the failed subjects into reconciliation.Related path: resource cleanup.
There was a problem hiding this comment.
Skipping this one. Logging a failed cutover and carrying on was a deliberate decision for this PR. The harm needs an old runtime that is both still running and no longer demanded at upgrade time, and whose verified stop then fails. Reconciliation stops undemanded resources promptly, so a running old runtime is demanded in practice. After cf5b6fd the cutover also runs at the start of every job, so it always comes before any cleanup. Blocking every subject, or failing every job, over one bad record would be worse for the common case.
![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: b6b0e9f075
ℹ️ 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".
| supervisor | ||
| .stop_subject( | ||
| &paths.resource_runtime_metadata(&track.resource_name, &track.track), | ||
| stop_signal, | ||
| RESOURCE_STOP_GRACE_PERIOD, |
There was a problem hiding this comment.
When startup cutover fails to stop a still-live pre-monitor Managed Resource, startup intentionally logs the error and continues into reconciliation. Because a legacy runtime has no monitor, this call returns false, but the result is ignored; the code then records the runtime as stopped, deletes its PID/JSON ownership evidence, and releases its ports while the process may remain alive. The same pattern affects stale workers in gateway.rs. Preserve the records and report the stop failure whenever a legacy PID record remains.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one, for the same reason as the Pullfrog thread on jobs.rs. It needs a verified stop of an old runtime to fail at cutover, and that runtime to be no longer demanded. Logging and continuing was a deliberate decision, and after cf5b6fd the cutover runs before any job's cleanup. The stale-worker path in gateway.rs only stops runtimes that the routing plan no longer contains, which has the same precondition.
| if let Err(error) = write_runtime_metadata(&spec, process.pid, &process.instance) { | ||
| let _stop_result = | ||
| stop_monitored_runtime(&self.paths, &process.subject, &process.instance, stop) | ||
| .await; |
There was a problem hiding this comment.
If writing the runtime metadata fails because of a filesystem or permission error, stopping the already-started monitor/runtime can also fail due to monitor unavailability, timeout, unproven cleanup, or release failure. Discarding _stop_result reports only the write error even though a runtime without its normal runtime record may remain alive and occupy its ports. Preserve both errors, as the existing monitor-start cleanup path does, so callers know cleanup was not completed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one. If the record write fails and the compensating stop also fails, the monitor's own record still names the runtime. The next reconciliation's stop_subject finds that monitor even though it has no runtime JSON beside it, which is the orphan case it handles, so nothing is left running unmanaged. The only cost is the second error message, after a record write fails on a disk PV already writes to.
| // Runtimes from a release before monitors restart under monitors. Reconciliation reports any | ||
| // that couldn't be stopped, since it can't start a replacement over them. | ||
| if let Err(error) = runtime_stop::stop_legacy_runtimes(&paths).await { |
There was a problem hiding this comment.
The daemon starts its request server and config watcher concurrently with the startup task, and all of them enqueue into the same queue. A foreground or watcher-triggered reconciliation can therefore be queued and run before the startup job obtains its turn, while this cutover only runs afterward. During an upgrade, that earlier reconciliation treats healthy pre-monitor runtimes as unmonitored—for example, rejecting a demanded Gateway as a foreign listener or cleaning an undemanded legacy runtime without stopping it. Run the cutover before opening reconciliation admission, or otherwise gate every non-startup job until it completes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in cf5b6fd. The queue runs jobs in arrival order, and on the update that installs monitors, pv update releases jobs.lock and immediately re-execs to queue its own update job. The new daemon's startup job is still retrying the lock every 50 ms, so the update job could run first and reconcile before the old runtimes were stopped. The cutover no longer lives only in the startup job: every reconciliation and update job runs it first (stop_legacy_runtimes_before_reconciliation), so whichever job goes first does it. Once nothing is left it only lists the run directory. New test: any_first_job_restarts_a_gateway_from_before_monitors_under_a_monitor runs a plain system reconciliation job with a pid-file Gateway present. It fails without the change.
| let supervisor = ProcessSupervisor::new(paths.clone()); | ||
| let mut failures = Vec::new(); | ||
| // Every runtime started since monitors, including one whose record never got written. | ||
| for subject in monitor::recorded_monitor_subjects(&paths)? { |
There was a problem hiding this comment.
recorded_monitor_subjects trusts the subject embedded in each monitor record without verifying that it hashes to the directory containing that record. If a record is corrupted or copied from another subject, shutdown attempts to stop the embedded subject, never visits the actual monitor, and then retains its runtime JSON because that monitor directory still contains a record; with no accumulated failure, disable or uninstall can report success while the original runtime and monitor remain running. Validate the subject-to-directory association while enumerating records and fail closed on a mismatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one. Only PV writes monitor records, into ~/.pv/run, which is 0700. A record would have to be a valid record for one subject, sitting in another subject's directory. That needs someone copying files by hand inside PV's private state, and nothing in PV does that.
| runtime | ||
| .block_on(crate::stop_recorded_runtimes(paths.clone())) | ||
| .map_err(anyhow::Error::from) |
There was a problem hiding this comment.
If a test returns early or unwinds while it still owns JobsLock, this Drop path calls stop_recorded_runtimes, which immediately tries to acquire that same exclusive lock and fails. The error is only printed, leaving the test's monitors and runtime processes alive until the entire test binary exits; this can retain listeners and contaminate later tests. The fallback teardown must stop its exact recorded monitors without depending on a lock that the unwinding test may still hold.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one, since it's test-only. The guard is created before any JobsLock in these tests, so it is dropped after the lock, during unwinding too. Nextest also runs each test in its own process. If cleanup does fail, the test lifeline stops those monitors when that test's process exits, so nothing carries over into other tests.
![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 (2)
DESIGN.md (1)
267-267: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the Postgres "exited before the stop" rule with
stop_monitored_for_shutdown.Line 267 says PV keeps the monitor and records if Postgres "exited before the stop". The code does not apply that rule.
ProcessSupervisor::stop_monitored_for_shutdownincrates/daemon/src/supervisor.rs(Lines 431-446) checks onlystate.exit != Some(MonitorExit::Code(0)). It does not look atstate.stop. A Postgres runtime that exited by itself with code 0 is therefore released, and its records are removed.A held exit 0 is valid proof that the backends are gone, so the code is probably correct. The document describes a stricter rule than the code applies. That rule only holds for legacy pid-file runtimes, which have no exit status. Reword the monitored-path sentence so that it matches the code.
📝 Proposed wording
-`pv daemon:disable` and `pv uninstall` stop and release every monitor, then remove records that no monitor backs. Only a successful Postgres exit status proves that its escaped backends are gone. If Postgres exited before the stop, needed a force-kill, exited unsuccessfully, or its monitor is unavailable, PV keeps its monitor and records; `--force` does not bypass this proof. +`pv daemon:disable` and `pv uninstall` stop and release every monitor, then remove records that no monitor backs. Only a successful Postgres exit status proves that its escaped backends are gone. If Postgres needed a force-kill, exited unsuccessfully, or its monitor is unavailable, PV keeps its monitor and records; `--force` does not bypass this proof. A held exit status of 0 counts as proof even when Postgres exited before the stop.crates/pv-fake/src/install.rs (1)
58-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBuild all daemon examples before running the CLI package tests.
The CLI fixtures install fakes below
.pv/resources, so the newinstall_withbranch callsinstall_monitor. After the existingpv-fakeexample is available, a package-scopedcargo nextest run -p clican still fail because it does not buildpv-monitor.Build both examples before the package-scoped run:
cargo build --locked --all-features -p daemon --examples cargo nextest run --locked --all-features -p cliThe
pv-fakecontract tests use temporary paths outside.pvand do not trigger this branch.
🧹 Nitpick comments (1)
crates/daemon/src/supervisor.rs (1)
114-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse [
TypeName] links in the new doc comments.The new doc comments refer to the monitor and the supervisor in plain prose only. Link the types so the docs resolve:
- Line 114: link [
ProcessSupervisor].- Line 136: link [
monitor::live_runtime], which decides whether the monitor "still runs it as recorded".📝 Proposed doc links
-/// A runtime this supervisor has just started under a monitor. +/// A runtime that [`ProcessSupervisor::start`] has just started under a monitor.-/// A runtime whose monitor still runs it as its record names. +/// A runtime whose monitor still runs it as its record names; see [`monitor::live_runtime`].As per coding guidelines: "PREFER [
TypeName] references when writing Rust doc comments".Also applies to: 136-136
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
65094ce9-dc63-4da7-b2f2-85e2554c56ba
⛔ Files ignored due to path filters (10)
crates/daemon/src/managed_resources/snapshots/daemon__managed_resources__mysql_tests__mysql_project_demand_installs_missing_fixture_track_before_start.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__mysql_tests__mysql_reconciliation_creates_database_allocation_and_renders_env.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__demanded_resource_cleans_runtime_files_when_process_exits_after_readiness.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__demanded_resource_records_failed_runtime_when_readiness_fails_before_env_rendering.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__postgres_preload_configuration_rejects_missing_library.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__postgres_preload_configuration_rejects_unsafe_combination.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__postgres_reconciliation_records_generated_env_when_readiness_fails_after_initdb.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__system_resource_reconciliation_stops_unlinked_project_runtime.snapis excluded by!**/*.snapcrates/daemon/src/snapshots/daemon__jobs__tests__partial_update_reconciliation_and_reporting.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/supervisor_foundation__supervisor_captures_logs_and_runtime_metadata_then_stops_child.snapis excluded by!**/*.snap
📒 Files selected for processing (28)
DESIGN.mdcrates/cli/tests/daemon.rscrates/cli/tests/setup.rscrates/cli/tests/support/runtime.rscrates/daemon/src/gateway.rscrates/daemon/src/health.rscrates/daemon/src/jobs.rscrates/daemon/src/lib.rscrates/daemon/src/managed_resources/mod.rscrates/daemon/src/managed_resources/mysql_tests.rscrates/daemon/src/managed_resources/runtime_contracts.rscrates/daemon/src/managed_resources/tests.rscrates/daemon/src/monitor.rscrates/daemon/src/monitor/macos.rscrates/daemon/src/runtime_stop.rscrates/daemon/src/structured_log.rscrates/daemon/src/supervisor.rscrates/daemon/tests/daemon_foundation.rscrates/daemon/tests/gateway_reconciliation.rscrates/daemon/tests/gateway_runtime_contracts.rscrates/daemon/tests/monitor.rscrates/daemon/tests/real_artifact_gateway_e2e.rscrates/daemon/tests/supervisor_foundation.rscrates/pv-fake/src/events.rscrates/pv-fake/src/fake.rscrates/pv-fake/src/install.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.
The Gateway, PHP workers and Managed Resource tracks now start through per-runtime monitors instead of as daemon children. The supervisor keeps its API. start launches a monitor and writes the runtime's JSON record with the monitor instance and no pid file. A runtime is PV's while its record names a monitor whose runtime still runs as recorded. stop asks the monitor, logs the held exit to daemon.log, then releases it. Config proofs stay in the JSON records. Before reconciliation starts a runtime, stop_subject clears whatever monitor its subject still has, including one a cancelled start left without a record, so that monitor no longer looks like a foreign listener. At startup, before the first reconciliation, the daemon stops runtimes that a release from before monitors left running and keeps their JSON proofs, so reconciliation restarts them under monitors. Disable and uninstall stop every monitor, remove records that no monitor backs, and still stop pid-file runtimes. Postgres counts as stopped only after a clean exit. A worker or Gateway that exits right after it starts now reports that exit instead of lost ownership.
Only the macOS monitor strips the PHP ini environment and reaps a runtime, so the constant moves into the monitor and the unused non-macOS reap stub goes.
9b12810 to
314c0f2
Compare
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/supervisor.rs (1)
1344-1351: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow the Postgres recovery hint only for Postgres runtimes.
stop_monitor_instancehandles every monitored subject. Its incomplete-cleanup error always adds{POSTGRES_RECOVERY}. When a Caddy, FrankenPHP, Redis or other runtime hasMonitorCleanupother thanComplete, the user gets Postgres recovery steps that do not apply to that runtime. Add the hint only whensubject_resource_name(subject) == "postgres". For other runtimes, use a generic recovery message.Proposed fix
if state.cleanup != MonitorCleanup::Complete { + let recovery = if subject_resource_name(subject) == "postgres" { + format!(" {POSTGRES_RECOVERY}") + } else { + String::new() + }; return Err(DaemonError::RuntimeCleanupUnproven { pid: state.runtime_pid, reason: format!( - "PV could not prove that the runtime's processes exited, so it kept its monitor and records. {POSTGRES_RECOVERY}" + "PV could not prove that the runtime's processes exited, so it kept its monitor and records.{recovery}" ), }); }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
bf24ddac-b681-4fe6-ad6c-58e6979e0878
⛔ Files ignored due to path filters (10)
crates/daemon/src/managed_resources/snapshots/daemon__managed_resources__mysql_tests__mysql_project_demand_installs_missing_fixture_track_before_start.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__mysql_tests__mysql_reconciliation_creates_database_allocation_and_renders_env.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__demanded_resource_cleans_runtime_files_when_process_exits_after_readiness.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__demanded_resource_records_failed_runtime_when_readiness_fails_before_env_rendering.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__postgres_preload_configuration_rejects_missing_library.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__postgres_preload_configuration_rejects_unsafe_combination.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__postgres_reconciliation_records_generated_env_when_readiness_fails_after_initdb.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__system_resource_reconciliation_stops_unlinked_project_runtime.snapis excluded by!**/*.snapcrates/daemon/src/snapshots/daemon__jobs__tests__partial_update_reconciliation_and_reporting.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/supervisor_foundation__supervisor_captures_logs_and_runtime_metadata_then_stops_child.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
crates/daemon/src/supervisor.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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 314c0f2762
ℹ️ 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".
| let instance = | ||
| monitor::recorded_monitor_instance(&self.paths, &subject)?.ok_or_else(|| { |
There was a problem hiding this comment.
If start_monitor succeeds but rereading its record fails (for example, due to a filesystem/JSON error), this ? returns while the newly started monitor and runtime keep running; the later process-identity lookup has the same problem. Because start returns no ManagedProcess, the caller cannot compensate, leaving a runtime without its normal runtime record and potentially occupying its ports after reconciliation reports failure. Apply the same compensating stop used for metadata persistence failures to every fallible post-start step.
AGENTS.md reference: AGENTS.md:L4-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one, for the same reason as the record-write thread. Either way, the monitor has published its record, so the next reconciliation's stop_subject clears it before anything else starts for that subject. The two steps are re-reading a record the monitor has just written, and looking up the identity of a process that is running. Neither fails in practice, so I'd rather not add a third compensation path.
| MonitorStopped::Stopped(state) | ||
| if postgres && state.exit != Some(MonitorExit::Code(0)) => |
There was a problem hiding this comment.
When Postgres already exited with code 0 before disable/uninstall, or its leader exits 0 but cleanup later escalates to SIGKILL for remaining group members, the monitor still reports Code(0) with Complete cleanup. This guard therefore releases the monitor and allows the following pass to delete its records, although DESIGN.md requires retaining them for pre-stop exits and force-killed shutdowns. The stop outcome must record that this request caused a non-escalated shutdown before accepting it as clean.
AGENTS.md reference: AGENTS.md:L4-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping the code as it is, and I've fixed the doc in cf5b6fd. The postmaster exits 0 only after its backends have exited, so a held exit 0 proves they're gone whoever asked it to stop; the approved plan settled on exactly that rule. The SIGKILL case can't leave a Postgres process behind either: by the time the leader has exited 0, it has no children left in the group. DESIGN.md still had the stricter wording carried over from the pid-file rules ("exited before the stop"). It now says a held exit 0 counts whoever stopped it.
| let supervisor = ProcessSupervisor::new(paths.clone()); | ||
| let mut failures = Vec::new(); | ||
| // Every runtime started since monitors, including one whose record never got written. | ||
| for subject in monitor::recorded_monitor_subjects(&paths)? { |
There was a problem hiding this comment.
If the daemon dies after spawning a monitor but before that monitor publishes monitor.json, a subsequent disable/uninstall can miss it because recorded_monitor_subjects explicitly skips directories without records. The monitor is independent of the daemon and can still publish and start its runtime after this one-time loop reports success, leaving a live listener after the LaunchAgent is removed. Drain or coordinate with in-flight monitor reservations before declaring recorded-runtime cleanup complete.
AGENTS.md reference: AGENTS.md:L4-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one. A monitor starts nothing until it receives its start request over the private startup socket. If the daemon dies before sending it, the monitor sees EOF and exits. Once it has the request, it publishes its record within about 20 ms. Disable only stops runtimes after the LaunchAgent bootout and a confirmed daemon exit, which takes far longer than that window.
The real-artifact suites run PV in a temporary home with no installed pv binary, so every monitor spawn failed. They now install the test monitor there, as the contract suites do, and the Gateway E2E cleans up with stop_recorded_runtimes.
There was a problem hiding this comment.
Important
No new issues found in this delta. Approval remains withheld because the existing lifecycle review threads are unresolved.
Reviewed changes Reviewed d849e5c8 against the previously examined 314c0f27 tree, focusing on real-artifact test startup and teardown.
- Installed test monitors: Added monitor installation to the Gateway E2E and resource-matrix temporary homes so real runtimes can start through the monitor-backed supervisor.
- Replaced Gateway cleanup: Switched from adopting only live Gateway/worker records to
stop_recorded_runtimes, which also handles exited runtimes and monitors without runtime metadata. - Validated changed targets: Both integration targets compile on Linux, all five portable helper tests pass, and
cargo fmt --all --checkpasses. The three opt-in artifact tests were not run locally; macOS artifact verification remains incomplete, with the Gateway E2E step failed and later steps still running at the last observation.
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: d849e5c812
ℹ️ 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".
| if let Err(error) = runtime_stop::stop_legacy_runtimes(&paths).await { | ||
| structured_log::legacy_runtime_stop_failed(&paths, &error.to_string()); | ||
| } |
There was a problem hiding this comment.
When a failed legacy record belongs to a removed Managed Resource track, logging this error and continuing allows the startup job to report success while the pre-monitor runtime remains alive: subsequent reconciliation only visits tracks present in the database, so it never performs the promised retry for this orphaned record. Propagate the cutover failure through startup rather than assuming reconciliation will report it.
AGENTS.md reference: AGENTS.md:L4-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one. A track is removed through PV, which stops its runtime first, so a legacy runtime for a removed track would need a failed earlier stop as well as a failed cutover. That's two independent failures for one rare record. Failing startup, and with it every job, until a reboot would cost more than the case it covers. pv daemon:disable and pv uninstall still stop every remaining pid-file runtime with the verified stop and fail if they can't.
| if process_group_has_exited(self.owned.pid)? { | ||
| return Ok(()); |
There was a problem hiding this comment.
When RuntimeFixture::cleanup calls this method, the runtime group disappears but its monitor is never released; the fixture then discards its ownership handle, leaving the monitor process, reservation, socket, and record alive until the entire test binary exits. Repeated CLI fixtures can therefore accumulate monitors and violate the repository's fixture-cleanup isolation contract, so ordinary cleanup must release the exact monitor after the verified kill.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one, since it's test-only. The fixture's monitor is released by the test lifeline when that test's process exits, and nextest runs each test in its own process. Some tests also depend on the monitor still holding the exit after this kill. For example, daemon_disable_keeps_dead_postgres_records_when_cleanup_is_unproven checks that disable refuses a Postgres that ended by signal 9.
…r-boot Postgres monitors The legacy cutover ran only in the startup job, but the queue runs jobs in arrival order. On the update that installs monitors, `pv update` releases jobs.lock and queues its own update job right away, while the new daemon's startup job retries the lock every 50 ms. When the update job won, it reconciled before the old runtimes were stopped: it saw the old Caddy as a foreign listener, and could delete the records of an old runtime that was still running. Every reconciliation and update job now runs the cutover first. Once nothing is left it only lists the run directory. After a reboot, a Postgres monitor is gone, so offline disable and uninstall refused its record and told the user to restart their Mac, which could never help. A record from an earlier boot names nothing that still runs, so recovery now removes it without signalling, as pid-file records already allowed. DESIGN.md now says a held Postgres exit 0 counts whoever stopped it, as the code and the approved plan do.
|
@coderabbitai on the two notes in your review summaries:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf5b6fdf07
ℹ️ 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".
| timing.queue_wait(), | ||
| &[], | ||
| ); | ||
| stop_legacy_runtimes_before_reconciliation(paths).await; |
There was a problem hiding this comment.
When the first post-upgrade job has a Project or resource scope, this call stops every pre-monitor runtime, but the branch below reconciles only the requested scope; an obsolete-Project job skips reconciliation entirely. For example, a Redis-scoped job can stop an unrelated legacy Gateway, worker, or MySQL runtime and leave it unavailable until a later system job, potentially after a long scoped install. Promote any job that performs the global cutover to system reconciliation, or stop only the legacy runtimes covered by its scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipping this one: the gap closes itself quickly. In-process jobs share one queue and one jobs.lock. Once any job is queued, the startup job, which retries every 50 ms, enqueues right behind it without waiting for the lock. That startup job is a full System reconciliation, so it restarts every runtime the cutover stopped. Unrelated runtimes are only down for the length of that one narrow job. Pullfrog's review of cf5b6fd checked the same thing. For a narrow job to go first at all, it has to land in the ~50 ms after pv update releases the lock, ahead of pv update's own job, which is itself System-wide. And that only happens once, at the upgrade to monitors. Limiting the cutover to a job's scope, or widening such a job to System, would add code for a gap the queue already closes.
| let Some(start_identity) = self.start_identity else { | ||
| return Ok(true); | ||
| }; | ||
|
|
||
| Ok(platform::inspect_process_start_identity(self.pid)? != Some(start_identity)) |
There was a problem hiding this comment.
When a Managed Resource becomes ready and then exits during the post-readiness confirmation window, this identity check returns true, but PendingManagedResourceReadiness::wait at managed_resources/mod.rs:2518-2522 only deletes the runtime files and returns an error. The monitor process, reservation, socket, and monitor record therefore remain until a later reconciliation or shutdown; stop and release the ManagedProcess before deleting its files while preserving the primary exit error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in b4fb654. The exits-just-after-ready branch in PendingManagedResourceReadiness::wait now calls stop_with before deleting the runtime files, as the readiness-failure branch already does. The monitor is released right away and logs the exit it was holding. demanded_resource_cleans_runtime_files_when_process_exits_after_readiness now also checks that the monitor directory is gone. It fails without the change and passed 20 out of 20 stressed runs with it.
There was a problem hiding this comment.
Important
No new issues found in this delta. Approval remains withheld while the two existing lifecycle review threads remain unresolved.
Reviewed changes Reviewed cf5b6fdf against the prior Pullfrog review at d849e5c8, focusing on legacy cutover ordering and boot-aware PostgreSQL recovery.
- Moved legacy cutover before every job: Reconciliation and update jobs now stop pre-monitor runtimes before doing their work, including when an update job runs before startup reconciliation. Verified that the independently queued startup System job subsequently covers unrelated demanded runtimes.
- Enabled prior-boot PostgreSQL cleanup: Offline shutdown now routes validated earlier-boot monitor records through recovery without signalling their recorded runtime PIDs, while unavailable current-boot PostgreSQL monitors remain fail-closed. Resolved the corresponding review thread.
- Clarified clean-exit proof and added regressions: Documented that a held PostgreSQL exit 0 counts regardless of who stopped it, and added coverage for ordinary-job cutover and prior-boot recovery with an unrelated reused PID.
- Validated the changed targets: Formatting, daemon library test compilation, CLI daemon test compilation, and the portable readiness regression pass locally. The new macOS lifecycle behavior was not executed locally on Linux; current-head CI has passed macOS 14, 26, and 27 arm64 lanes, while macOS 15 Intel was still running at the last check.
gpt-6.1-sol ｜ 𝕏
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd '^DESIGN\.md$'
rg -n -C5 'held|earlier.boot|Postgres|PostgreSQL' DESIGN.md
ast-grep outline crates/daemon/src/supervisor.rs --match 'stop_monitored_for_shutdown' --view expanded
rg -n -C8 'POSTGRES_RECOVERY|async fn stop_monitored_for_shutdown|monitor_from_earlier_boot|require_successful_exit|exit_success' crates/daemon/src/supervisor.rsLength of output: 36326
✏️ Learnings added
You are interacting with an AI system. |
…eadiness A Managed Resource that exits during the post-readiness check had its runtime files removed, but its monitor kept running and holding the exit until a later reconciliation released it. That branch now stops the runtime through its monitor first, as the readiness-failure branch already does, so the monitor is released and its exit logged right away.
There was a problem hiding this comment.
ℹ️ No new issues found in this delta. Approval remains withheld while the two existing lifecycle review threads remain unresolved.
Reviewed changes Reviewed b4fb654d against the prior Pullfrog review at cf5b6fdf, focusing on resource cleanup immediately after readiness.
- Released fast-exit monitors before deleting runtime files when a newly started Managed Resource exits during the post-readiness check. Verified that this logs the held exit and preserves runtime records if monitor cleanup fails.
- Strengthened regression coverage by asserting that the fast-exit Mailpit monitor directory is absent before fixture teardown, alongside the existing runtime-file and failed-observation assertions.
- Validated portable checks with
cargo fmt --all --check, daemon library test compilation, and the bounded-readiness regression, all passing. The monitor-dependent regression was not executed locally on Linux; current-head CI has passed its Linux and Windows runtime lanes, with all four macOS lanes still running at the last check.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve legacy shutdown before releasing resource state. · mod.rs:2664-2670
crates/daemon/src/managed_resources/mod.rs:2664-2670
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve legacy shutdown before releasing resource state.
When
stop_subjectreturnsfalseand a legacy PID/JSON record exists, usestop_legacy_runtimebefore recordingStopped, deleting runtime files, or releasing ports. Keep the records and port assignments if verified shutdown fails.
stop_legacy_runtimes_before_reconciliationlogs legacy-stop errors and continues. An undemanded resource track can then reach this function while its legacy process is still running. Since that process has no monitor,stop_subjectreturnsfalse; the current code ignores that result and releases the runtime state.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
99978b9c-898b-4090-a16a-1ab1a82a8eec
📒 Files selected for processing (2)
crates/daemon/src/managed_resources/mod.rscrates/daemon/src/managed_resources/tests.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
1 participant
PR 4b of the monitor stack, on top of #413. This is the cutover: the Gateway, PHP workers and Managed Resource tracks now run under per-runtime monitors instead of as daemon children.
It follows your four decisions from 2026-10-10:
daemon.log, then releases the monitor.What changes
supervisor.rs) now run on monitors. Its API is unchanged, sogateway.rsandmanaged_resources/barely change.startlaunches a monitor through the activepvbinary. It writes the runtime's JSON record with the monitor instance and no pid file. If that write fails, it stops the monitor.live_runtimefrom feat(daemon): let supervisors check monitor runtimes without a request #413, which is synchronous.stopasks the monitor, logs the exit, then releases the monitor. If cleanup is unproven, PV keeps the monitor and its record, which blocks that runtime.SpawnedProcessGroup), identity stabilisation and pid-file writing.stop_subjectruns before reconciliation starts a runtime and before the foreign-listener check. It clears any monitor the subject still has, including one that a cancelled start left without a record. Before this, such an orphan blocked its own replacement as "something else is listening".jobs.lock, before the first reconciliation, the daemon stops runtimes that an older release left running. It removes their pid files and keeps their JSON proofs, so reconciliation restarts them under monitors. Failures are logged and startup continues. After the first run this does nothing.Bugs found while stress testing
A runtime whose children were still exiting could be blocked until a reboot. The monitor's fallback cleanup could mark cleanup unproven because of how macOS answers
killpgwhile a group member is still exiting. That fix and its regression test now live in #412 (f9a9f59), because the bug is in the monitor's own cleanup.A worker that exited right after starting reported the wrong error. Staging the config proof right after
startfound the runtime gone and reportedRuntimeOwnershipChanged. It now reports "exited before readiness". Monitor startup takes slightly longer than the old direct spawn, sotargeted_project_reconciliation_preserves_old_route_until_new_worker_is_readyhit this under load.Tests
pv_fake::install_monitor), and the test lifeline cleans up runtimes when a test process dies. Guards no longer stop runtimes inDrop.signal 9and starts a replacement with a new pid.start_replaces_a_monitor_left_without_a_recordNot in this PR
mismatched_peer_process_group_receives_no_http_request_bytesalready covers that rejection, and every gateway test runs the monitor-backed verifier.pid_pathfield, the legacy stop code andboot_session_idhandling stay until PR 5.Open risks
read_recordrefuses their records. The next bump needs a stop-before-update step.recover_monitorafter a monitor dies.Test plan
--stress-count 10with the full suite running alongside as load, all passed.cargo clippy --workspace --all-targets --all-features --locked -- -D warnings,cargo fmt --all --checkandcargo shearare clean. No leftover processes..envexpectation that has been on main since 2026-07-29 and stopped the matrix early. That run used a temporary branch, now deleted.Summary by CodeRabbit