fix(embed): harden daemon and UI lifecycle (#3099, #3100, #3517, #3520, #3527) - #3585
Merged
Conversation
#3527) Five open hindsight-embed issues all sit in the same two files and share one root theme: the manager decides who to talk to, and who to kill, from evidence that isn't good enough. #3520 — `_clear_port`/`stop`/`stop_ui` picked their victim purely by "who holds the port" and SIGTERMed it. On a host where an unrelated service shared the port, that service died with no indication of what killed it. A listener is now only signalled once its command line identifies it as our daemon (or our control plane); otherwise we log and refuse, and startup fails with "port in use" instead. A failed start is recoverable; killing someone else's service is not. #3517 — `_find_pid_on_port` shelled out to `lsof` only, so on Linux hosts without it (minimal containers, Arch-based distros) every daemon stop logged "Could not find PID for port" and stopped nothing. PID discovery now falls back to `ss` (iproute2). It also returns every listener rather than an arbitrary first PID, which is what lets the ownership check above pick the right one. #3527 — `is_ui_running` health-checked 127.0.0.1 regardless of the bind hostname. Next.js started with `--hostname localhost` binds ::1 only, so `ui start` always timed out after 30s on a UI that was up and serving, and `ui status` reported it as down. Both loopback families are now probed, in `_is_port_in_use` and the Windows netstat parse as well, and user-facing URLs say `localhost` so they resolve whichever way the server bound. #3099 — the 2s /health client timeout classified a busy daemon as dead. /health is served from the same event loop as the daemon's LLM calls, so a slow provider stalls it. The probe budget is now 10s by default (aligned with the worker-side liveness threshold) and configurable via HINDSIGHT_EMBED_HEALTH_PROBE_TIMEOUT. The 30s reclaim grace window is unchanged. #3100 — the Windows lock used `msvcrt.LK_LOCK`, which retries exactly 10 times internally and then raises, so a concurrent start of the same profile failed non-deterministically with an opaque OSError. Both platforms now drive the non-blocking primitive from one bounded retry loop with backoff; on timeout the error names the lock file and the PID holding it, and `_start_daemon` turns that into a normal startup failure. Not included: #3253 (empty llm_api_key clobbering an inherited env var) already has a fix in the open PR #3359.
Two defects from the previous commit, found in review. The UI health probe inherited HEALTH_PROBE_TIMEOUT (10s). That budget exists for the daemon, whose /health sits behind the event loop its LLM calls run on (#3099); the control plane's /api/health has nothing blocking behind it. Since start_ui polls the probe inside a 30s budget and now probes two loopback families, a listener that binds but does not answer would burn the whole budget in two probes and report a false "UI failed to start (timeout)" — the exact symptom #3527 is about. The UI keeps its own 2s probe. delete_profile removed <name>.lock but not the <name>.lock.owner sidecar the new locking writes, so a crash while holding the lock orphaned a file that outlived the profile. Also adds direct coverage for _process_command_line, which decides every kill but was only reached through tests that patch it out, and documents that _wait_for_port_health bounds when the last probe starts rather than when it returns.
test-embed-windows failed on test_delete_profile_over_http with a client-side ReadTimeout. The control center's delete handler asks is_running once and the UI probe once per loopback family; at the 10s budget those three serial probes could reach 14s against httpx's 5s default client timeout. The same path was ~4s before, so a slow connect that Windows already had was being masked. The budgets are now split by what a wrong answer costs. HEALTH_PROBE_TIMEOUT (10s, configurable) applies only to _port_health_ok — the probe whose false negative gets the listener killed, which is what #3099 is actually about. is_running and the UI probe use LIVENESS_PROBE_TIMEOUT (2s, the pre-existing value): they only answer "is it up?", and a false negative there costs a re-run of ensure_running, which consults the long probe before doing anything destructive. #3099's real harm — a busy daemon being reclaimed as stale — stays fixed. Connect is capped separately at 1s. An address that swallows the SYN hangs in connect rather than read, so this is what actually bounds the handler: three probes at 1s is 3s, below both the 5s client default and the 4s the two uncapped probes could reach before this branch.
This was referenced Aug 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five open
hindsight-embedissues sit in the same two files and share one roottheme: the manager decides who to talk to, and who to kill, from evidence that
isn't good enough. Three of them compound each other, which is why they are
worth doing in one pass — #3517 means no PID is found, #3099 makes a healthy
daemon look dead, and #3520 means that false negative can take out an unrelated
process.
Fixes
#3520 —
_clear_portkilled any PID holding the port_clear_port/stop/stop_uipicked their victim purely by "who holds theport" and SIGTERMed it. On the reporter's host that terminated a separate,
legitimately-owned
hindsight-apiservice; the embed side logged"Stale process stopped" and carried on, so the operator had no indication of
what killed it.
A listener is now only signalled once its command line identifies it as our
daemon (or our control plane). Otherwise we log which PIDs hold the port and
refuse, and startup fails with "port in use" instead. A failed start is
recoverable; killing someone else's service is not.
#3517 —
_find_pid_on_portrequiredlsofOn Linux hosts without
lsof(minimal containers, Arch-based distros) theFileNotFoundErrorwas swallowed with no fallback, so every daemon stop loggedCould not find PID for portand stopped nothing. PID discovery now falls backto
ss(iproute2). It also returns every listener rather than an arbitraryfirst PID — which is what lets the ownership check above pick the right one.
#3527 —
is_ui_running()hardcoded 127.0.0.1Next.js started with
--hostname localhostbinds::1only, so the IPv4 probegot ECONNREFUSED:
ui startalways timed out after 30s on a UI that was up andserving,
ui statusreported it as down, andui stopcould orphan theprocess. Both loopback families are now probed — in
is_ui_running,_is_port_in_use, and the Windows netstat parse, which matched an IPv4 literaland so missed
0.0.0.0/[::1]listeners too. User-facing URLs saylocalhostso they resolve whichever way the server bound.
#3099 — 2s
/healthtimeout classified a busy daemon as dead/healthis served from the same event loop as the daemon's LLM calls, so aslow provider stalls it. The probe budget is now 10s by default (aligned with
the worker-side liveness threshold) and configurable via
HINDSIGHT_EMBED_HEALTH_PROBE_TIMEOUT.Note the issue's mechanism does not hold up as written:
_clear_porthas had a30s grace window since #1858, so a 17s stall never reaches the kill, and #2906
— cited there as the merged worker-side fix — was closed unmerged. The related
stop()bug from the same thread was already fixed as #3169. So this PR fixesthe part that is real (the probe misreporting a busy daemon) and leaves the 30s
reclaim window alone rather than adding the separate liveness server.
#3100 — Windows lock had no timeout
msvcrt.locking(..., LK_LOCK, 1)retries exactly 10 times internally and thenraises, so a concurrent start of the same profile failed non-deterministically
with an opaque
OSError. Both platforms now drive the non-blocking primitivefrom one bounded retry loop with exponential backoff; on timeout the error names
the lock file and the PID holding it, and
_start_daemonturns that into anormal startup failure. Budget is 300s (the start path holds the lock for the
whole startup sequence), overridable via
HINDSIGHT_EMBED_LOCK_TIMEOUT.Not included
#3253 (an empty
llm_api_keyclobbering an inherited env var) already has afix in the open PR #3359 — left alone rather than duplicated.
Testing
hindsight-embedsuite: 180 passed. New regression coverage for each issue,including a family of tests asserting that a foreign listener survives both
_clear_portandstop. Beyond the mocked tests, PID discovery, thessfallback, the ownership refusal and IPv6-only UI detection were each verified
against real listeners.
Tradeoffs worth a look
next-serverbecause Next.js rewrites argv once it isserving, so the package name is not always still visible. A different Next
app squatting the UI port would therefore still be killed; command line alone
cannot do better without walking the process tree.
command-line lookup fails entirely,
daemon stopstops working rather thankilling blind. That is the tradeoff hindsight-embed: _clear_port kills any PID holding the port with no ownership check, terminating unrelated processes #3520 explicitly asks for.
Closes #3099
Closes #3100
Closes #3517
Closes #3520
Closes #3527