Skip to content

fix(embed): verify process ownership before termination - #3526

Open
Beverly621 wants to merge 3 commits into
vectorize-io:mainfrom
Beverly621:agent/guard-managed-process-termination
Open

fix(embed): verify process ownership before termination#3526
Beverly621 wants to merge 3 commits into
vectorize-io:mainfrom
Beverly621:agent/guard-managed-process-termination

Conversation

@Beverly621

Copy link
Copy Markdown

Closes #3520

Summary

Prevent hindsight-embed from 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(), and stop_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 managed hindsight-api deployment.

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

  • pass a profile-specific PID receipt path to the embedded API daemon
  • write and conditionally clean up the receipt from the actual API process after daemonization
  • write the actual Next.js server PID from control-plane instrumentation
  • require listener PID and receipt PID to match before every termination path
  • retain the UI receipt if termination fails, so a later stop can retry safely
  • cover owned, foreign, missing, and failed-termination cases with regression tests

Validation

53 passed: hindsight-embed/tests/test_daemon_client.py + test_embed_manager.py
12 relevant profile daemon tests passed (3 unrelated local-ML command-selection tests require optional sentence-transformers)
ruff check: passed for all changed Python source files
ruff format --check: passed for all changed Python files
python compileall: passed
git diff --check: passed

The repository-wide lint hook could not complete in this checkout because its absolute directory name contains :, which uv rejects 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.

@Beverly621
Beverly621 marked this pull request as ready for review August 16, 2026 11:23

@koriyoshi2041 koriyoshi2041 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Beverly621

Copy link
Copy Markdown
Author

Thanks for catching this ownership edge. I addressed it in 498a78ad and the follow-up 7b733572.

Ownership receipts are now versioned JSON records containing both the PID and a process birth marker. Linux uses /proc/<pid>/stat starttime, Windows uses the process creation date from CIM, and other POSIX platforms fall back to ps lstart. _owned_pid_on_port() now requires both the listener PID and the current birth marker to match the receipt; if either is missing or mismatched, it fails closed.

Both the unhealthy-port cleanup path and stop() go through this shared ownership check. I added focused regressions for a matching PID with a mismatched birth marker in both paths, asserting that _kill_process is not called.

I also replaced the receipt tuple with a frozen dataclass to comply with the project's typed-data conventions.

Validation completed:

  • hindsight-embed: 56 tests passed
  • daemon receipt lifecycle: 5 tests passed
  • control-plane instrumentation receipt test: passed
  • Ruff, formatting, ty, Prettier, ESLint, and git diff --check: passed

The latest CI run is currently awaiting maintainer approval. Please take another look when convenient.

@koriyoshi2041 koriyoshi2041 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hindsight-embed: _clear_port kills any PID holding the port with no ownership check, terminating unrelated processes

2 participants