Repository navigation
Conversation
One monitor process per supervised runtime becomes the runtime's parent for its whole life, and the daemon talks to monitors instead of runtime PIDs. The proposal records the decisions already made: the containerd-style split of mechanics in the monitor and policy in the daemon, exit status kept until acknowledged, pushed events plus state pulled on reconnect, start and delete commands, one state directory per runtime with a short socket path, and a frozen version-and-stop core that restarts a runtime on a protocol mismatch. Three decisions remain open: how long a runtime outlives its daemon, where the monitor code lives, and the state directory name.
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 (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds an accepted macOS design for per-runtime monitors and a five-PR implementation plan. The documents specify monitor lifecycle, control and recovery, production integration, testing, cutover, and rollback. They do not implement the design. ChangesPer-service monitor design and plan
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Merge Risk: ⚪ Minimal · up to This change publishes a monitor design and implementation plan without changing runtime behavior. The reviewed text defines recovery cleanup and keeps Linux unsupported, so no outstanding documented risk blocks merging. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to No introduced or worsened security risk was identified. The design requires instance-specific identity, verified cleanup, and separate Caddy peer checks. This PR changes documentation, not production runtime behavior. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
DESIGN.md describes what PV does and what's been decided, and agents treat it as settled. The proposal still has open decisions, so it lives in docs/superpowers/specs like other designs; DESIGN.md changes when the monitor is built.
clvsh
changed the title
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Two crash-recovery gaps to clarify in the proposal; no executable behavior changes in this PR.
Reviewed changes Reviewed the complete monitor proposal against current supervision, descendant cleanup, daemon lifecycle, and fixture ownership behavior; tests were skipped because this PR changes documentation only.
- Runtime ownership and lifecycle: Proposes one persistent monitor per runtime, daemon-owned policy, retained exit status, and state recovery on reconnect.
- Protocol and cleanup: Defines directory discovery, UID-checked sockets, frozen
version/stopoperations, protocol-mismatch replacement, and crashed-monitor deletion. - Operations and rollout: Covers log rotation, disable/uninstall, owner-disconnection behavior, three open decisions, manual cutover, and future Linux containment; the latest commit moves the proposal to a standalone spec and leaves
DESIGN.mdunchanged.
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: 98bdfa50eb
ℹ️ 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".
| ### Lifecycle | ||
|
|
||
| 1. **Start.** The daemon runs `pv monitor start` with the runtime's command, arguments, private environment, log path, and stop policy. The command starts the long-lived monitor in a new session, so it survives the daemon's LaunchAgent restarts as runtimes do today. It returns only once the monitor's socket is listening, and prints `{version, address}` or an error, so the daemon gets a clear success or failure without polling for a socket. | ||
| 2. **Run.** The monitor starts the runtime in its own process group, with the runtime's stdout and stderr connected to the monitor through pipes. |
There was a problem hiding this comment.
When a monitor crashes while its runtime is still active, the monitor's pipe read ends close, so the runtime's next stdout/stderr write can receive SIGPIPE or EPIPE; depending on the runtime, that can terminate it or disable logging. This contradicts the later claim that monitor failure merely leaves a parentless runtime for pv monitor delete to recover. Either intentionally fate-share the runtime with the monitor or use a logging arrangement that remains valid after monitor death.
Useful? React with 👍 / 👎.
|
|
||
| - One extra process per runtime. | ||
| - The frozen core has to be right the first time. | ||
| - A monitor runs the code it started with until its runtime restarts. After two self-updates its release may have been pruned from `~/.pv/bin/releases/`. That is harmless because a monitor never re-runs its own binary, and cleanup uses the current `pv monitor delete`. |
There was a problem hiding this comment.
This is not harmless when a monitor survives enough same-protocol updates for its originating release to be pruned and then crashes: the current pv monitor delete must parse that old monitor's state file to identify the runtime, but the proposal freezes only the socket framing, version, and stop while allowing everything else to change freely. Freeze or explicitly version the on-disk cleanup schema with compatible readers, or retain a binary capable of reading it; otherwise cleanup and uninstall can leave the runtime orphaned.
Useful? React with 👍 / 👎.
|
|
||
| 1. **Start.** The daemon runs `pv monitor start` with the runtime's command, arguments, private environment, log path, and stop policy. The command starts the long-lived monitor in a new session, so it survives the daemon's LaunchAgent restarts as runtimes do today. It returns only once the monitor's socket is listening, and prints `{version, address}` or an error, so the daemon gets a clear success or failure without polling for a socket. | ||
| 2. **Run.** The monitor starts the runtime in its own process group, with the runtime's stdout and stderr connected to the monitor through pipes. | ||
| 3. **Exit.** As the parent, the monitor learns the runtime's exact exit status the moment it exits. It reports the runtime stopped only once the runtime's whole process group is gone. Because the monitor reaps the runtime itself, a group that still answers `EPERM` can only mean another member is still exiting, which removes the ambiguity behind #394. The monitor pushes an `exited` event to a connected daemon and keeps the exit status until the daemon acknowledges it, then removes its state directory and exits. A crash during a daemon restart or update is therefore never lost. |
There was a problem hiding this comment.
If the daemon sends the acknowledgment and crashes before durably recording the exit status and restart bookkeeping, the monitor removes its state directory and the next daemon has no way to recover the promised exact exit information. The protocol needs to require a durable commit before acknowledgment, with idempotent redelivery when an acknowledgment is lost, for the stated never-lost guarantee to hold.
Useful? React with 👍 / 👎.
|
|
||
| 1. **Start.** The daemon runs `pv monitor start` with the runtime's command, arguments, private environment, log path, and stop policy. The command starts the long-lived monitor in a new session, so it survives the daemon's LaunchAgent restarts as runtimes do today. It returns only once the monitor's socket is listening, and prints `{version, address}` or an error, so the daemon gets a clear success or failure without polling for a socket. | ||
| 2. **Run.** The monitor starts the runtime in its own process group, with the runtime's stdout and stderr connected to the monitor through pipes. | ||
| 3. **Exit.** As the parent, the monitor learns the runtime's exact exit status the moment it exits. It reports the runtime stopped only once the runtime's whole process group is gone. Because the monitor reaps the runtime itself, a group that still answers `EPERM` can only mean another member is still exiting, which removes the ambiguity behind #394. The monitor pushes an `exited` event to a connected daemon and keeps the exit status until the daemon acknowledges it, then removes its state directory and exits. A crash during a daemon restart or update is therefore never lost. |
There was a problem hiding this comment.
After the monitor reaps the group leader, EPERM is not proof that an owned group member is merely exiting: the numeric PID/PGID may have been reused by a process group the monitor cannot signal. The existing supervisor deliberately treats EPERM as insufficient to identify the owned group, so the monitor still needs a containment or identity mechanism for remaining group members rather than using reaping alone to remove that ambiguity.
Useful? React with 👍 / 👎.
|
|
||
| ### Cutover | ||
|
|
||
| There is no backward compatibility (decided 2026-09-29). The first release with monitors does not adopt runtimes started from pid files. Stop the old runtimes once by hand on each machine before updating, and the new daemon starts every runtime under a monitor. Roll it out on one machine first and on the second a few days later. |
There was a problem hiding this comment.
Stopping the old runtimes by hand before updating leaves the old KeepAlive daemon active, and its health/reconciliation loop is explicitly responsible for restarting desired runtimes that stop. It can therefore recreate a runtime before or during pv update, after which the new daemon starts a monitor-owned replacement that collides on ports or the same data directory. The cutover must atomically disable or quiesce the old daemon and stop its runtimes as part of activation rather than relying on manual process kills.
Useful? React with 👍 / 👎.
| ### Lifecycle | ||
|
|
||
| 1. **Start.** The daemon runs `pv monitor start` with the runtime's command, arguments, private environment, log path, and stop policy. The command starts the long-lived monitor in a new session, so it survives the daemon's LaunchAgent restarts as runtimes do today. It returns only once the monitor's socket is listening, and prints `{version, address}` or an error, so the daemon gets a clear success or failure without polling for a socket. | ||
| 2. **Run.** The monitor starts the runtime in its own process group, with the runtime's stdout and stderr connected to the monitor through pipes. |
There was a problem hiding this comment.
The proposal starts the runtime before defining any crash-safe publication handshake for its PID and start identity. If the monitor dies after spawning the child but before atomically publishing a complete state record, directory scanning cannot discover or verify that live orphan and pv monitor delete cannot clean it up. Gate the child before exec or resume until its durable identity record exists, and only then report startup success.
Useful? React with 👍 / 👎.
| → {"op":"version"} | ||
| ← {"protocol":1,"pv":"0.3.0","runtime":"postgres:18"} | ||
| → {"op":"stop"} | ||
| ← {"stopped":true,"exit_code":0} |
There was a problem hiding this comment.
The response permanently models termination as an integer exit_code, but the frozen stop operation escalates to SIGKILL when its grace period expires. A Unix process terminated by a signal has no exit code, so this schema cannot report the promised exact status without inventing a lossy shell-style value. Define a signal-aware or optional status representation before freezing the protocol.
Useful? React with 👍 / 👎.
|
|
||
| ### What it replaces | ||
|
|
||
| Once monitors run every runtime, PV no longer writes pid files or runtime metadata, adopts runtimes by checking their identity, or keeps the supervisor's script-identity fallback. `pv monitor delete` keeps the only identity check on macOS. The 30-second health tick stays for readiness probes, while crash detection becomes immediate through `exited` events. |
There was a problem hiding this comment.
Removing runtime metadata also removes the durable command, arguments, private-environment/config fingerprints, replacement marker, and applied/desired/staged Caddy fingerprints currently used during adoption and reload recovery. The proposed state record contains none of these, so after a daemon restart a same-protocol monitor can be retained despite running a stale artifact or environment, and an unresolved POST /load can lose the marker that prevents an unsafe second load. Preserve a non-secret launch-spec fingerprint and the config transaction state in durable monitor or authoritative database state before removing runtime metadata.
Useful? React with 👍 / 👎.
|
|
||
| 1. **Start.** The daemon runs `pv monitor start` with the runtime's command, arguments, private environment, log path, and stop policy. The command starts the long-lived monitor in a new session, so it survives the daemon's LaunchAgent restarts as runtimes do today. It returns only once the monitor's socket is listening, and prints `{version, address}` or an error, so the daemon gets a clear success or failure without polling for a socket. | ||
| 2. **Run.** The monitor starts the runtime in its own process group, with the runtime's stdout and stderr connected to the monitor through pipes. | ||
| 3. **Exit.** As the parent, the monitor learns the runtime's exact exit status the moment it exits. It reports the runtime stopped only once the runtime's whole process group is gone. Because the monitor reaps the runtime itself, a group that still answers `EPERM` can only mean another member is still exiting, which removes the ambiguity behind #394. The monitor pushes an `exited` event to a connected daemon and keeps the exit status until the daemon acknowledges it, then removes its state directory and exits. A crash during a daemon restart or update is therefore never lost. |
There was a problem hiding this comment.
The monitor removes its state directory only after the daemon acknowledges the exit, but the daemon may immediately restart a desired runtime after sending that acknowledgment. Because the replacement uses the same deterministic <id>, it can create its socket and state before the old monitor executes its removal, allowing the old monitor to delete the replacement's live directory. Make cleanup completion part of the stop/ack handshake, wait for the old monitor to exit before reuse, or use generation-specific directories.
Useful? React with 👍 / 👎.
|
|
||
| ### Commands that stop every runtime | ||
|
|
||
| `pv daemon:disable` and `pv uninstall` stop every runtime by asking each monitor found under `~/.pv/run/m/` to stop it. This also closes a gap: Daemon Lifecycle says `pv daemon:disable` stops PV-managed processes first, but today it only unloads the LaunchAgent, without asking the daemon to stop any runtime. |
There was a problem hiding this comment.
pv daemon:disable is still specified to stop children before stopping the daemon, so an active reconciliation can create another monitor after the command takes its directory snapshot. The command can then unload the LaunchAgent while the missed monitor and runtime remain alive indefinitely under the recommended production owner-loss policy. Enter a quiescing state that rejects new starts and drains active mutation work before enumerating monitors, then verify the directory set is empty before completing shutdown.
Useful? React with 👍 / 👎.
![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 (1)
docs/superpowers/specs/2026-10-08-per-service-monitor-design.md (1)
97-97: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDefine Linux cleanup when cgroups are unavailable.
Line 56 defines Linux recovery as killing the runtime cgroup, while Line 97 makes cgroup placement optional. If the monitor crashes before cleanup, descendants that left the process group may be re-parented to init and survive. Either mark Linux hosts without cgroups as unsupported or define a safe fallback that can identify and terminate those descendants.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
2e632a9e-b8d4-48b4-b753-6e3feb6050fe
📒 Files selected for processing (1)
docs/superpowers/specs/2026-10-08-per-service-monitor-design.md
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
|
|
||
| **Everything else is versioned.** The daemon uses the other requests only when the monitor reports the daemon's own protocol version: `state`, `signal` (a signal sent to the runtime or to its process group), the `exited` event, acknowledging an exit, and log settings. These can change freely between versions. | ||
|
|
||
| **A version mismatch restarts that runtime once.** If a monitor reports a different protocol version, the daemon sends the frozen `stop`, then starts a new monitor and runtime with its own version. Self-updates that don't change the protocol keep runtimes running, as today. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Define cleanup for the frozen stop path.
When a protocol mismatch triggers frozen stop, specify that the daemon removes the old state directory after the process group exits, before starting the replacement monitor for the same <id>. The current lifecycle only defines directory removal after an exited acknowledgment, while frozen stop does not define that cleanup. A stale directory can prevent recovery.
clvsh
changed the title
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed d5bc48c against the prior Pullfrog review at f8ecaf2f, with the complete current spec and plan as context.
- Clarified monitor ownership and policy: Defined indefinite production lifetime, daemon-selected stop parameters, monitor-completed escalation, and fallback cleanup after spontaneous leader exit.
- Separated exit reporting from cleanup: Required prompt leader-exit state, non-reaping observation, explicit instance-scoped release, database commit before daemon consumption, and conservative recovery when ownership or Postgres cleanup cannot be proven.
- Specified startup and replacement safety: Added the exec gate, published birth/boot identities, transferred subject reservation, authenticated monitor peers, and controller-owned record removal after verified process death.
- Revised logging and configuration contracts: Selected direct append files with bounded copy-and-truncate rotation and accepted diagnostic loss, while preserving instance-bound configuration proof and separate Caddy admin peer verification.
- Added lifecycle and rollout requirements: Defined the shutdown fix, external admission-lock scopes, five-PR dependency structure, updater-enforced cutover, rollback/recovery constraints, and process-level acceptance matrix.
Whitespace validation passed. Runtime tests were skipped because this PR changes documentation only; the plan assigns behavioral verification to the implementation PRs.
gpt-6.1-sol ｜ 𝕏
1 participant
PV keeps runtimes alive across daemon restarts, but the daemon cannot retain an exit result or finish an accepted stop after its own process dies. This design gives each runtime a stable monitor while keeping readiness and restart policy in the daemon.
The approved design keeps one result in monitor memory, writes output directly to append files, and accepts the diagnostic loss window of copy-and-truncate rotation. Review made startup publication, instance ownership, clean Postgres shutdown, and explicit result consumption part of the contract. macOS non-parent exit observation alone does not meet the retention requirement.
The plan uses five PRs. A standalone shutdown fix comes first. Monitor core, production integration, and obsolete-code removal form the three-PR stack linked with
gh stack. Integration must remain safe if final removal is delayed.Validation: checked the revised spec and plan for conflicting requirements and whitespace. Runtime tests are not required for this documentation-only PR; each implementation stage has its own acceptance checks.
Summary by CodeRabbit