fix(embed): define daemon stop() success by port occupancy, not the health probe

[Alan5168]

Implements the contract agreed in #3169 with @koriyoshi2041.

stop() was using is_running() — the 2s /health probe — both as its already-stopped guard and as its final success condition. A daemon that is alive but busy fails that probe, so daemon stop returned success without sending any signal (repro in the issue: nothing killed, port still bound, /health back to 200 once the block cleared). As koriyoshi2041 pointed out, the tail of the function had the same problem: a False from _kill_process() was discarded, and a bound port with no findable PID also fell through to the health-based wait, so both failure paths could exit True.

Every decision is now based on port occupancy:

• nothing bound to the profile's port → already stopped, True
• port bound, no PID found → False
• _kill_process() returns False → False
• after the kill, success means the listener is gone — polling _is_port_in_use(), not /health

Same occupancy/health separation _clear_port() already uses. /health reports responsiveness, not identity or liveness, so it cannot answer either question stop() needs to ask.

Regression tests cover the three cases named in the issue thread — busy listener, failed termination, missing PID — plus already-stopped and lingering-listener-after-kill, and one case pinning stop() and _clear_port() to the same policy for an occupied, unhealthy port. The busy-listener test patches both is_running and _port_health_ok to raise, so it fails if any path in stop() consults the responsiveness probe. Five of the six fail against the previous implementation (verified by reverting just the production change; the already-stopped case passes on both, as expected). Full hindsight-embed suite is green (161 passed), Ruff check and format --check clean on both changed files.

One behavioural note: "port bound but no PID found" previously could report success via the wait loop; per the contract it now reports failure, which callers see as a failed stop instead of a silent no-op.

An earlier revision of this PR added a _port_health_ok gate before the kill, to avoid signalling an unrelated process holding the profile port. That was reverted after review (#3171): a busy Hindsight daemon fails the probe, so the gate refused to stop exactly the process #3169 is about, and it contradicted _clear_port(), which reclaims that same port state on the start path. Guarding against a genuinely foreign listener needs a real ownership receipt — a pidfile written at start, applied to both stop() and _clear_port() — which is left as a follow-up rather than approximated with a health check.

Closes #3169

[koriyoshi2041]

The occupancy guard fixes the false-success path, but it also removes the only process-identity check before sending SIGTERM. If the configured profile port is occupied by an unrelated local service, stop() now finds that service's PID and kills it; previously the failed Hindsight /health probe returned without signaling it.

Please preserve an ownership receipt for the daemon process (for example, a PID file written by the spawned daemon and validated against the listener) and refuse to signal a bound port when ownership cannot be established. A regression with an unrelated listener on the profile port should assert that _kill_process is not called. Port occupancy is the right success condition after termination, but it is not sufficient authorization to choose the process to terminate.

I ran uv run --project hindsight-embed pytest hindsight-embed/tests/test_daemon_client.py -q (29 passed), plus Ruff lint and format checks on both changed files.

[Alan5168]

Good catch - the identity check was load-bearing and I dropped it.

Replaced the bare occupancy gate with a two-tier check:

• _port_health_ok(port) authorizes the kill - it confirms the listener answers with the Hindsight health payload (status == "healthy", database == "connected"), which an unrelated service on the same port will not. This is stricter than the old is_running() probe, which only checked for HTTP 200.
• _is_port_in_use(port) remains the success condition after termination, as you noted it should.

_port_health_ok already exists in the file (it backs _wait_for_port_health on the start path), so this just applies the same identity check to the stop path rather than introducing a PID file. If you would prefer the PID-file route I am happy to switch - the invariant matters more than the mechanism.

One behavior change worth flagging: a busy Hindsight daemon (the original #3169 scenario) fails _port_health_ok, so stop() now returns False rather than killing blind. I think this is the right trade-off - a failed stop is recoverable (retry, or the daemon self-exits on idle), but SIGTERM-ing an unknown process is not - but it does mean the #3169 symptom shifts from "false success" to "honest failure" rather than being silently fixed.

Added test_foreign_listener_is_not_signaled (the regression you asked for): an unrelated listener on the profile port with _port_health_ok -> False asserts _kill_process is never called. Full daemon-client suite: 30 passed.

[koriyoshi2041]

The foreign-listener case is closed, and the focused daemon suite passes locally (30 tests), but this now restores the original #3169 failure: a busy Hindsight daemon fails the health probe, receives no signal, and stop() returns false.

Please do not close #3169 with that behavior. The stop path needs an identity source that does not depend on responsiveness—e.g. persist the spawned daemon PID, validate that PID still owns the configured listener, then signal it. Keep the new health check only as a fallback/refusal boundary when no ownership receipt is available. A regression should cover an identified Hindsight PID whose health request times out and assert it is still terminated.

The occupancy-based post-kill success check and the failed-kill/missing-PID handling remain correct.

[Alan5168]

Quick update: #3260 just merged, so my contributor status is now unlocked. The CI checks on this PR were originally held by the first-time contributor gate — could someone approve/trigger them now? The code is unchanged, just looking to get the test suite green for review. Thanks!

[handnewb]

Good fix! Defining daemon stop() success by port occupancy is more reliable than health probe — avoids false positives when the daemon is busy but port is still bound. Solid test coverage.

[nicoloboschi]

Thanks for this — the diagnosis is spot on and the tests are a nice scaffold. Two things I'd like resolved before merge: one blocking design point, and one CI note.

Blocking: the _port_health_ok gate defeats the fix for the case #3169 is actually about

The core of the PR is right. stop() genuinely had two defects, and both are correctly identified:

• the guard if not self.is_running(...): return True treated a busy daemon as already-stopped, and
• the tail fell through to a health-based success even when _kill_process() returned False or no PID was found.

Pivoting the guard and the success condition from /health to port occupancy (_is_port_in_use) is exactly the right fix.

The problem is the identity gate added on top:

if not self._port_health_ok(port):
    logger.warning(...refusing to signal an unknown process...)
    return False

The whole premise of #3169 is "a daemon that is alive but busy fails that probe." So the one process the user is trying to stop — a busy/wedged Hindsight daemon — is precisely the one this gate refuses to kill. That converts the old bug ("claims success, kills nothing") into "reports failure, kills nothing." The daemon still isn't stopped, so the reported symptom isn't actually fixed for the busy case; a permanently-wedged daemon becomes un-stoppable via stop().

It's also inconsistent with _clear_port(), which on the start path treats an unhealthy listener on the same port as a stale daemon and kills the PID to reclaim it. Same "port occupied but not health-OK" state, opposite policy.

The gate's intent — don't SIGTERM a foreign process that happens to hold the port — is legitimate, but /health is the wrong signal for it: busy ≠ foreign. There's no pidfile recording the owning PID, so health is the only identity check available, and it can't tell a wedged-but-ours daemon from a foreign one. start()/_clear_port() already accept that same risk, and stop is an explicit user command targeting their own profile's port — refusing because the daemon is momentarily busy is user-hostile.

Requested change: drop the health-identity gate and let stop() reclaim the port the way _clear_port() does:

• port not in use → already stopped, True
• port in use, no PID found → False (the one case we genuinely can't act on)
• PID found → kill it; _kill_process() returns False → False
• success = the listener is gone (poll _is_port_in_use, not /health)

That keeps everything the PR fixed (no more false-success, no more fall-through on failed kill / missing PID) while actually stopping the busy daemon. It also means updating test_busy_daemon_refuses_to_kill_without_identity and test_foreign_listener_is_not_signaled to assert the daemon does get signalled — the remaining cases (missing PID, failed kill, already-stopped, lingering listener) stay as-is.

If there's a concrete scenario where a foreign process on the profile port is a real risk we must guard against, let's discuss that here — but if so it should be solved with a real identity signal (e.g. a pidfile written at start) and applied consistently to both stop() and _clear_port(), not via a health check that misfires on the exact bug we're fixing.

CI note (not your change)

The red test-hermes-compat job is a stale base, not this diff: scripts/test-hermes-compat.sh landed on main (in #3265) after this branch was cut, so the workflow can't find it (exit 127). A rebase on main clears it.

[Alan5168]

@nicoloboschi Agreed on all points — the health-identity gate was the wrong call and I've dropped it.

The reasoning I accept: /health reports responsiveness, not identity, and busy ≠ foreign. Using it as the authorization check meant the one process #3169 is about — a wedged daemon holding the port — was precisely the one stop() refused to signal. That is the same no-op the PR set out to fix, just with an honest return code, and it left the daemon un-stoppable. It also split policy from _clear_port(), which reclaims that identical "occupied but not health-OK" state on the start path.

stop() is now occupancy-only, exactly as specified:

• port not in use → already stopped, True
• port in use, no PID found → False
• PID found → kill it; _kill_process() returns False → False
• success = the listener is gone (poll _is_port_in_use, never /health)

Test updates:

• test_busy_daemon_refuses_to_kill_without_identity → test_busy_daemon_is_terminated: asserts _kill_process is called and stop() returns True. Both is_running and _port_health_ok are patched to raise, so the test fails if any path in stop() consults responsiveness.
• test_foreign_listener_is_not_signaled → test_unresponsive_listener_is_reclaimed_like_clear_port: asserts the listener is signalled, and pins stop() and _clear_port() to the same behaviour for an occupied, unhealthy port so the two paths cannot drift apart again.
• Missing-PID, failed-kill, already-stopped and lingering-listener cases are unchanged.

Verification: 5 of the 6 TestStop cases fail against the pre-fix stop() (the already-stopped case passes on both, as expected); the full hindsight-embed suite is green (161 passed), and Ruff check + format --check are clean on both changed files.

On the foreign-process concern that prompted the gate: I agree it needs a real ownership receipt rather than a health probe, and that it should land for stop() and _clear_port() together. That is a separate change from this bug fix — happy to open a follow-up issue for a pidfile written at start if you want it tracked.

For the CI note: I merged the latest main into the branch rather than rebasing, to avoid force-pushing over review history. That brings in scripts/test-hermes-compat.sh from #3265 and should clear the exit-127 failure. If you would prefer a clean rebase before merge, say the word and I will do that instead.

@koriyoshi2041 — your second review called this correctly: identity cannot depend on responsiveness. This resolves it by not gating on identity at all in stop(), matching the existing start-path policy, with the pidfile left as the proper fix if we decide to add one.

[nicoloboschi]

Verified locally against the fork HEAD (f3aaf6a): all 6 TestStop cases pass, and the occupancy-only stop() is exactly the contract requested — health-gate removed, success determined by _is_port_in_use, consistent with _clear_port(). (The 3 test_profile_daemon_config.py failures I saw are an isolated-venv artifact — missing local ML deps forcing a uvx hindsight-api fallback — and pass in a provisioned env; unrelated to this diff.) Foreign-process/pidfile hardening tracked as a follow-up. Thanks for the quick turnaround.