Repository navigation
Conversation
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe changes add boot-session-aware runtime records and shutdown APIs that validate process identity. Lifecycle locks coordinate daemon startup and selected CLI commands. CLI disable and uninstall stop recorded runtimes, and daemon replacement waits for the prior daemon to exit. ChangesRuntime lifecycle and shutdown
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant CLI as CLI disable
participant Environment
participant LaunchAgent
participant DaemonProcess
participant RuntimeStop as stop_recorded_runtimes
participant Supervisor as ProcessSupervisor
CLI->>Environment: daemon_process_for_stop(paths)
CLI->>LaunchAgent: boot out LaunchAgent
CLI->>DaemonProcess: wait up to 10 seconds for exit
CLI->>RuntimeStop: stop recorded runtimes
RuntimeStop->>Supervisor: stop each recorded runtime
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 36.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 156 functions across 37 files. (1 skipped… | 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 describes the main change: stopping managed runtimes before disabling PV. It is concise and directly related to the pull request. |
Full details: Docstring Coverage
Explanation
Docstring coverage is 36.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 156 functions across 37 files. (1 skipped: 1 unsupported.)
- 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
- Autopilot · 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.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
crates/daemon/src/supervisor.rs (1)
1221-1229: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe leader-identity guard removes
SIGKILLescalation but adds no safety.This branch runs only when
wait_for_process_group_exitreported live group members. POSIX does not reuse a process-group ID while the group has members.kill(-pgid, SIGKILL)therefore still targets the original group after the leader is reaped. Thematches_live()check has the same check-to-signal race as the signal it guards.The new behavior has a concrete cost. The leader can exit on
SIGTERMwhile a descendant ignoresSIGTERMfor longer than the 1 s Caddy/FrankenPHP grace. In that case, the function returnsRuntimeCleanupUnprovenand leaves the descendant running. A retry then finds a dead leader with a live group and returnsRuntimeProcessIdentityChanged. Disable and uninstall stay blocked until the descendant exits on its own. The same path affects every caller ofAdoptedProcess::stop_with, not only shutdown.Escalate whenever the group still has live members. Use the identity check only to decide whether the record can be trusted after the stop.
Proposed change
- // An adopted leader can be reaped by its original parent during the grace - // period. Do not signal a group whose recorded leader no longer matches. - if !owned.matches_live()? { - return Err(DaemonError::RuntimeCleanupUnproven { - pid, - reason: "runtime leader changed before escalation; group ownership is unproven" - .to_owned(), - }); - } + // A process-group ID is not reused while the group has members, so a group + // that still had live members at the last check is still the recorded group. + let _ = owned; signal_process_group(pid, ProcessSignal::Kill)?;crates/cli/src/commands/project.rs (1)
26-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWait for
JobsLockcontention inlinkandunlink.A running reconciliation holds
JobsLockuntil all queued and active jobs finish.linkandunlinkacquire this lock with a non-blocking call before database access, so they can fail immediately during a reconciliation. Add a bounded retry forJobsLock::acquirein these mutation paths. The existing reconciliation retry runs too late because it starts only after the CLI releases the lock.
request_project_reconciliationandrequest_system_reconciliationsubmit a job and return its identifier. They do not wait for job completion.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
46c73028-de71-412f-a533-7a9ebca5d540
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
crates/cli/Cargo.tomlcrates/cli/src/commands/daemon.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/project.rscrates/cli/tests/daemon.rscrates/cli/tests/setup.rscrates/cli/tests/support/runtime.rscrates/daemon/src/error.rscrates/daemon/src/lib.rscrates/daemon/src/managed_resources/runtime_contracts.rscrates/daemon/src/runtime_stop.rscrates/daemon/src/supervisor.rscrates/daemon/tests/daemon_foundation.rscrates/daemon/tests/gateway_reconciliation.rscrates/platform/Cargo.tomlcrates/platform/src/lib.rscrates/platform/src/process.rscrates/platform/src/process/macos.rscrates/platform/src/process/unsupported.rscrates/state/src/fs.rscrates/state/src/lib.rscrates/state/src/paths.rscrates/state/src/update_lock.rscrates/state/tests/state_foundation.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.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the complete PR, including shutdown sequencing, lifecycle exclusion, persisted ownership checks, native exit observation, and regression tests.
- Disable and uninstall: Capture the daemon's identity, wait for its process to exit, and stop recorded runtimes before removing registration or installed state.
- Lifecycle coordination: Shared foreground/bootstrap admission and exclusive destructive admission prevent conflicting mutations; daemon identity records survive listener closure and teardown.
- Conservative runtime cleanup: Validate exact PID/metadata pairs without relying on SQLite, retain recovery evidence on failure, and recheck records before removal.
- Native shutdown proof: Observe all live process-group members, revalidate ownership before escalation, and require successful observed PostgreSQL postmaster exit before destructive cleanup.
- Regression coverage: Add lifecycle, descendant, identity-failure, uninstall/prune, and PostgreSQL contract tests; reserve fresh worker ports during fixture restoration.
Validation passed on Linux: workspace check, Clippy with warnings denied, formatting, and all 106 state-crate tests. The macOS shutdown tests could not run on this host; macOS CI was still in progress at the last check.
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: 4c5ab0280c
ℹ️ 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".
| streams: &mut Streams<'_>, | ||
| ) -> Result<ExitCode, ExecuteError> { | ||
| let paths = pv_paths(environment)?; | ||
| let jobs_lock = state::JobsLock::acquire(&paths).map_err(super::coordination_lock_error)?; |
There was a problem hiding this comment.
When daemon reconciliation is active, it holds the nonblocking jobs.lock, so this acquisition makes pv link fail before recording the requested project; the identical addition in unlink has the same problem. DESIGN.md:743 explicitly requires ordinary mutating commands to check only update.lock so reconciliation cannot reject unrelated intent recording. Reserve jobs.lock for the direct resource mutations listed there.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| if let Some(parent) = path.parent() { | ||
| fs::create_dir_all(parent)?; |
There was a problem hiding this comment.
On a fresh home, acquiring a jobs or update lock now creates ~/.pv/run through raw create_dir_all, which uses the process umask—normally producing 0755 under umask 0022—instead of the required owner-only 0700. For example, pv link acquires this lock before validating pv.yml, so a validation failure leaves the insecure layout behind without reaching Database::open to harden it, violating the invariant in DESIGN.md:387. Create state lock parents through the owner-only directory helper, while handling the external lifecycle-lock path separately.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| blocked_request_release_signal: Option<mpsc::Sender<()>>, | ||
| ) -> Result<Self, DaemonError> { | ||
| let _runtime_lifecycle_lock = state::RuntimeLifecycleLock::acquire_shared(&paths)?; | ||
| runtime_stop::require_previous_daemon_exited(&paths)?; |
There was a problem hiding this comment.
When daemon:enable or daemon:restart repairs a stale owned plist, daemon.rs:67-76 performs bootout and immediately bootstraps/kickstarts the replacement, but this new check rejects that replacement while the previous daemon's live identity record remains. The launchctl documentation describes bootout as removing the service definition, not as a process-exit wait; this commit's disable path likewise adds an explicit wait. If old-daemon shutdown plus launchd retry exceeds the 15-second health window, stale repair now reports failure, so capture and wait for the old identity before starting the replacement.
Useful? React with 👍 / 👎.
| CapabilityCheck: FnOnce(&Command) -> Result<(), ExecuteError>, | ||
| { | ||
| capability_check(&cli.command)?; | ||
| let _runtime_lifecycle_lock = acquire_runtime_lifecycle(&cli.command, environment)?; |
There was a problem hiding this comment.
For pv uninstall --prune without --force, dispatch now acquires the exclusive lifecycle lock before setup::uninstall asks for destructive confirmation. Lock acquisition creates and persists ~/.pv-runtime-lifecycle.lock, so declining the prompt—or being unable to prompt without a terminal—still changes the filesystem, contrary to the explicit DESIGN.md:1619 cancellation contract that declining changes nothing. Move confirmation ahead of lock acquisition or make pre-confirmation admission non-creating.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| if !owned.matches_live()? { | ||
| return Err(DaemonError::RuntimeCleanupUnproven { | ||
| pid, | ||
| reason: "runtime leader changed before escalation; group ownership is unproven" | ||
| .to_owned(), | ||
| }); |
There was a problem hiding this comment.
When a verified runtime's leader exits after the graceful signal but a child in the same process group ignores or delays that signal, the preceding wait establishes that the group is still live, while matches_live() necessarily fails because the leader is gone. This branch therefore skips SIGKILL, making daemon:disable or uninstall fail while leaving the child running, contrary to the whole-process-group escalation required by DESIGN.md:269-271 and DESIGN.md:289. Preserve ownership proof for the group across the grace wait instead of requiring the departed leader to re-verify.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| if let Err(source) = runtime_stop::record_daemon_process(&paths) { | ||
| return match daemon.shutdown().await { | ||
| Ok(()) => Err(source), |
There was a problem hiding this comment.
If process identity inspection or writing daemon-process.json fails here, the already-started daemon is shut down and the error is returned without calling write_startup_failure_marker. Inner startup has already cleared any stale marker, so pv update or enable health polling loses the repair-relevant cause and reports only a generic unhealthy-daemon failure, despite DESIGN.md:731 requiring startup failures to be exposed through this marker. Record the identity-publication error, including any cleanup failure, before returning.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed 8572f315 against the prior Pullfrog review at 4c5ab028, with the complete PR diff as context.
- Unified daemon replacement: Routed stale enable/restart and app-update/rollback through verified predecessor exit before replacement startup, preserving managed runtimes.
- Adjusted lifecycle admission: Moved uninstall's exclusive lock after confirmation while retaining it through deletion, and removed project link/unlink's jobs-lock contention.
- Preserved startup diagnostics: Recorded daemon identity-publication failures after shutdown, including any cleanup error.
- Added boot-session recovery: Persisted validated native boot UUIDs and allowed unchanged previous-boot records to be removed without inspecting or signalling potentially reused PIDs; missing, invalid, or current-boot evidence remains insufficient for that shortcut.
- Expanded regression coverage: Added delayed-exit and timeout tests, uninstall lock-lifetime and cancellation checks, jobs-lock intent recording, publication-error snapshots, and boot-evidence/changed-record preservation tests; updated the design and uninstall documentation.
Linux validation passed: workspace compilation, Clippy with warnings denied, formatting, all 106 state tests, and both cross-platform boot-session tests. Native macOS shutdown tests could not run on this host; platform CI remains necessary.
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: 8572f31536
ℹ️ 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 fs::path_is_file(&resource)? && !matches!(resource.extension(), Some("pid" | "json")) { | ||
| continue; | ||
| } | ||
| require_directory(&resource)?; |
There was a problem hiding this comment.
When a real MySQL runtime is active, it creates ~/.pv/run/resources/mysql-<track>.sock via the --socket argument in managed_resources/mysql.rs:142,169-170. A Unix socket is not a regular file, so this condition falls through to require_directory and returns InvalidRuntimeRecord before the record-stop loop runs. Consequently, pv daemon:disable and uninstall unload the daemon but leave MySQL and the other managed runtimes running, contrary to the shutdown behavior required by DESIGN.md:289 and DESIGN.md:773; handle this known ancillary socket entry separately.
AGENTS.md reference: AGENTS.md:L4-L4
Useful? React with 👍 / 👎.
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 · Serialize shim initialization with uninstall, not the PHP process. · mod.rs:156-157
crates/cli/src/commands/mod.rs:156-157
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSerialize shim initialization with uninstall, not the PHP process.
On macOS,
ShimPhpandShimComposerskipRuntimeLifecycleLock, while uninstall holds the exclusive lock and removes the same~/.pvtree. Both shims write database/layout state and PHP defaults before launching PHP. A concurrent shim can therefore recreate directories orphp.iniafter prune deletion begins, leaving PV state after uninstall reports success.Acquire a shared lifecycle lock around each shim’s initialization, including Composer’s initial database open, then release it before
exec_with_env. Do not hold the lock for the long-running PHP process.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
57b5d6b5-a33a-40c9-928b-0af874f1cabc
⛔ Files ignored due to path filters (7)
crates/cli/tests/snapshots/project_open__project_intent_is_recorded_while_reconciliation_holds_jobs_lock.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/setup__uninstall_holds_exclusive_admission_through_prune_and_releases_on_return.snapis excluded by!**/*.snapcrates/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/snapshots/daemon__tests__daemon_publication_and_cleanup_failure.snapis excluded by!**/*.snapcrates/daemon/src/snapshots/daemon__tests__daemon_publication_failure.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 (23)
DESIGN.mdcrates/cli/src/commands/daemon.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/setup.rscrates/cli/src/commands/update.rscrates/cli/src/environment.rscrates/cli/tests/daemon.rscrates/cli/tests/presentation.rscrates/cli/tests/project_open.rscrates/cli/tests/setup.rscrates/cli/tests/update.rscrates/daemon/src/lib.rscrates/daemon/src/managed_resources/mysql_tests.rscrates/daemon/src/supervisor.rscrates/daemon/tests/supervisor_foundation.rscrates/platform/src/ca.rscrates/platform/src/error.rscrates/platform/src/lib.rscrates/platform/src/process.rscrates/platform/src/process/macos.rscrates/platform/src/process/unsupported.rscrates/platform/tests/boot_session.rsdocs/user/README.md
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Merge current main and retain both environment checks in the test conflicts. Skip ancillary MySQL sockets during offline runtime discovery, and hold shared lifecycle admission through PHP and Composer initialization before releasing it for execution.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed 8572f315..3ede0eb0 against the previous Pullfrog review, using the complete PR diff for context.
- Fixed socket discovery: Skipped ancillary
.sockentries before validating resource directories, allowing offline shutdown to proceed with a real MySQL socket present. - Serialized shim initialization: Acquired shared lifecycle admission before PHP/Composer state initialization, transferred the guard through Composer's PHP dispatch, and released it immediately before PHP execution.
- Expanded regression coverage: Added exclusive-shutdown rejection snapshots, initialization and execution lock-lifetime probes, and a bound MySQL socket alongside live runtime groups in the shutdown test; updated the design contract.
Validation passed on Linux: workspace compilation, Clippy with warnings denied, formatting, all 25 portable shim tests, and the lifecycle-admission integration test. The broader Composer run failed nine install/update/uninstall tests (error: test failed, to rerun pass -p cli --test composer); these flows require daemon IPC, which is unsupported on this host. Native shutdown and new shim-admission assertions could not run locally. macOS 14 arm64 CI passed; the remaining three macOS lanes were still running at submission.
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: 3ede0eb0ce
ℹ️ 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".
| io::ErrorKind::NotFound | io::ErrorKind::ConnectionRefused | ||
| ) => | ||
| { | ||
| Ok(None) |
There was a problem hiding this comment.
When enable or restart runs while launchd has spawned the daemon but it has not yet published either its process record or socket, this returns None, so unload_and_wait_for_daemon performs bootout and immediately bootstraps the replacement without calling wait_for_exit. Fresh evidence after the earlier stale-daemon fix is this no-record/no-socket path: the two startups can overlap and race ipc::prepare_endpoint removal/binding, causing the replacement to fail or report health from the departing process, contrary to the exact-process wait required by DESIGN.md:697; preserve a launchd process identity or otherwise wait for bootout completion before bootstrap.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed the missed exit wait in 5d5425d2. When the initial identity check finds no daemon, the shared unload-and-wait operation checks again after bootout. If the daemon published its identity during unload, PV verifies that identity and waits for that exact process to exit before replacement or offline runtime cleanup. An identity captured before unload is preserved.
The integration regression covers records published both before and during mocked bootout, using a real native process and ownership checks. Bootstrap refuses to proceed while that process is alive. The new case failed before the fix and passed after it. All 905 CLI and daemon tests passed, and workspace Clippy and formatting checks passed.
| let grace = if matches!(metadata.resource_name.as_str(), "caddy" | "frankenphp") { | ||
| Duration::from_secs(1) | ||
| } else { | ||
| Duration::from_secs(10) |
There was a problem hiding this comment.
When Caddy or FrankenPHP needs more than one second to drain, such as while completing a long-running request, this special case escalates the offline disable or uninstall to SIGKILL. DESIGN.md:271 gives managed child process groups up to ten seconds for graceful shutdown and specifies only a signal exception for Postgres, so this can terminate requests that would have completed within the required grace period; use the ten-second shutdown grace here as well.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We reviewed this with the maintainer and are keeping the one-second grace for Caddy and FrankenPHP during disable and uninstall. PV is a local development tool; an explicit stop is allowed to interrupt an active web request. We accept that consequence rather than wait up to ten seconds for web requests to finish.
The previous design wording was too broad. 5d5425d2 updates DESIGN.md to state the one-second Gateway/PHP-worker limit, the existing ten-second allowance for other Managed Resources, and the separate ten-second wait for the verified daemon process to exit. Postgres retains SIGINT and its shutdown-verification requirements. No change to the web-runtime grace is intended.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed 3ede0eb0..5d5425d2 against the prior Pullfrog review, using the complete PR diff for context.
- Closed the identity-publication gap: Repeated daemon identity capture after LaunchAgent unload when the initial capture found no daemon, then applied the existing verified-exit wait before cleanup or replacement startup.
- Expanded the replacement regression: Exercised records published both before and during unload, with bootstrap rejected while the native predecessor remains alive.
- Clarified shutdown policy: Documented the existing one-second Gateway/PHP-worker grace and ten-second Managed Resource grace separately from the daemon-exit wait, including the intentional possibility of interrupting local web requests.
Linux validation passed: workspace compilation across all targets/features, Clippy with warnings denied, formatting, all 45 CLI library tests, and all 25 portable shim integration tests. The native publication-during-unload regression could not run on this Linux host; all four macOS CI lanes remain in progress, while Linux and Windows runtime checks passed.
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: 5d5425d200
ℹ️ 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".
| // Startup can publish the daemon identity while bootout is in progress. | ||
| let daemon = match daemon { | ||
| Some(daemon) => Some(daemon), | ||
| None => environment.daemon_process_for_stop(paths)?, |
There was a problem hiding this comment.
When the old daemon is still in pre-publication startup after bootout returns, this second lookup can also return None immediately because neither its record nor socket exists yet; if it publishes just afterward, the caller bootstraps the replacement without waiting, so the replacement can fail require_previous_daemon_exited or race endpoint cleanup and binding. Fresh evidence beyond the prior no-record/no-socket comment is that the added fix performs only this point-in-time recheck, while its regression test publishes synchronously inside bootout rather than after this call returns. Poll through the bounded shutdown window or preserve a launchd process identity that can be awaited.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| let daemon = RunningDaemon::start_with_runtime_catalog(paths, None).await?; | ||
| let daemon = RunningDaemon::start_with_runtime_catalog(paths.clone(), None).await?; | ||
| let daemon = publish_started_daemon(&paths, daemon).await?; | ||
| drop(admission.take()); |
There was a problem hiding this comment.
When disable or uninstall acquires exclusive admission immediately after daemon identity publication, this drops the startup guard before termination_signal() has been polled, so the Tokio SIGTERM handler has not yet been registered. A bootout signal in that window takes the default process-termination path and bypasses wait_for_shutdown, including server-task cancellation, DNS shutdown, and socket cleanup; register the signal listener before releasing lifecycle admission.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
| blocked_request_release_signal: Option<mpsc::Sender<()>>, | ||
| ) -> Result<Self, DaemonError> { | ||
| let _runtime_lifecycle_lock = state::RuntimeLifecycleLock::acquire_shared(&paths)?; | ||
| runtime_stop::require_previous_daemon_exited(&paths)?; |
There was a problem hiding this comment.
When daemon-process.json is malformed, unreadable, or still identifies a live prior daemon, this ? returns before the marker-writing branch below, so the startup failure is not written to daemon-startup-error.json. This is especially visible during self-update, which clears the previous marker immediately before kickstart and therefore reports only a generic daemon-health failure instead of the recovery-relevant identity error; include this check in the marker-wrapped startup path.
AGENTS.md reference: AGENTS.md:L3-L4
Useful? React with 👍 / 👎.
1 participant
daemon:disablecould unload the LaunchAgent while the Gateway, PHP workers, and Managed Resources kept running. Uninstall could then delete their records and binaries. Disable and uninstall now capture the daemon identity, unload it, wait for its exit, and stop each verified recorded runtime before removing installed state. Failed or unproven cleanup preserves the records and files needed for recovery.Lifecycle admission uses a separate lock outside
~/.pv. Reusing the helper lock would deadlock setup and update while they health-check daemon bootstrap; keeping admission outside the state tree lets uninstall hold it through deletion. Uninstall acquires exclusive admission after confirmation. Foreground mutations and short bootstrap hold shared admission. PHP and Composer shims now hold that same admission throughout initialization and release it before executing PHP, so they cannot recreate state during prune or block shutdown for an entire PHP command.Daemon replacement uses one unload-and-wait operation for stale enable, restart, update, and rollback. A missing listener cannot prove that the old daemon exited. If the first identity check finds no daemon, the shared operation checks again after unload and waits for any verified daemon that published its record during unload. A native-process regression covers both publication timings and failed before this fix. Normal restart still preserves managed runtimes. Process-record publication failures now write the startup-failure marker after cleanup, including any cleanup error. Project link and unlink record desired state without taking the jobs lock, so active reconciliation cannot reject unrelated intent.
Disable and uninstall keep a one-second graceful-stop allowance for the Gateway and PHP workers. This is an explicit local-development choice: stopping PV can interrupt an active web request. Other Managed Resources retain ten seconds for shutdown work. The design now states these separate limits and distinguishes them from the daemon exit wait.
Postgres shutdown uses SIGINT and requires observed successful postmaster exit plus process-group cleanup, because backends can leave the postmaster group. Native macOS group observation replaces the leader-only completion check. New records save the native boot-session UUID: an unchanged, valid pair from a previous boot can be removed without inspecting or signalling a potentially reused PID. Missing, invalid, or current-boot evidence cannot authorize that shortcut.
--forceskips confirmation only.The offline scan now skips ancillary
.sockentries. A real MySQL socket previously made discovery fail before any collected runtime could stop. The regression test binds that socket beside live MySQL and other runtimes, then verifies that the groups exit and records disappear. Shim tests verify exclusion before initialization, admission during initialization, and release before execution.Merged current
mainand retained both branches' environment checks in the two test conflicts. This also includes main's coordination-directory permission fix and updated port inspection. The earlier fixture port race is handled by reserving a fresh port for each worker restore.Validation:
cargo shearpassed.This is PR 2 of the five-PR monitor plan in #406. It works without monitor code and targets
mainindependently. PRs 3–5 will form thegh stackstack after this prerequisite lands.Summary by CodeRabbit
Bug Fixes
Documentation