fix(embed): verify process ownership before termination - #3526
fix(embed): verify process ownership before termination#3526Beverly621 wants to merge 3 commits into
Conversation
koriyoshi2041
left a comment
There was a problem hiding this comment.
The fail-closed behavior for missing/mismatched receipts is a good improvement. One ownership edge remains: a receipt containing only a PID can outlive the daemon, and after PID reuse it can authorize signaling an unrelated listener that happens to acquire both that PID and the configured port. The cleanup handler covers orderly exit, but SIGKILL/crash/power loss leaves the receipt behind. Could the receipt also include a process birth marker and have _owned_pid_on_port compare it (for example Linux /proc//stat starttime, with a platform fallback), or could startup invalidate stale receipts before treating them as authority? A focused regression could write a matching PID with a mismatched birth marker and assert _kill_process is not called. I checked the current stop and unhealthy-port paths; both currently rely only on listener_pid == owned_pid.
|
Thanks for catching this ownership edge. I addressed it in Ownership receipts are now versioned JSON records containing both the PID and a process birth marker. Linux uses Both the unhealthy-port cleanup path and I also replaced the receipt tuple with a frozen dataclass to comply with the project's typed-data conventions. Validation completed:
The latest CI run is currently awaiting maintainer approval. Please take another look when convenient. |
koriyoshi2041
left a comment
There was a problem hiding this comment.
Re-reviewed at 7b733572. The receipt now binds both the listener PID and an OS process-birth marker, and both _clear_port() and stop() fail closed when either value differs. The Linux /proc/<pid>/stat parsing reads field 22 correctly even when comm contains spaces, and the receipt cleanup avoids removing a replacement record.
Local validation passed: 56/56 embed manager/client tests, 5/5 daemon receipt lifecycle tests, the control-plane instrumentation test, and git diff --check. The focused PID-reuse regressions cover both termination paths and assert _kill_process is not called. This resolves my earlier ownership concern.
Closes #3520
Summary
Prevent
hindsight-embedfrom terminating an arbitrary process merely because it listens on the configured daemon or UI port.The embedded API and control-plane server now write PID ownership receipts from the actual listener processes.
_clear_port(),stop(), andstop_ui()only signal a listener when its PID matches the receipt for that profile. Missing or mismatched ownership fails safely with the port left untouched.Why
The previous unhealthy-port recovery path selected a victim solely through
_find_pid_on_port(). A failed health probe could therefore SIGTERM an unrelated service, including a separately managedhindsight-apideployment.This keeps stale-process recovery for daemons started by the embed manager while changing the ambiguous case from destructive recovery to a recoverable “port in use” failure.
Implementation
Validation
The repository-wide lint hook could not complete in this checkout because its absolute directory name contains
:, whichuvrejects as a path separator. The changed Python files were checked directly with the same Ruff rules; CI runs from a normal checkout path.AI assistance
AI model: GPT-5.6Sol
Coding agent: Codex
The contributor reviewed and accepted the changes.