fix(embed): inherit unset LLM settings instead of overwriting them (#3253) - #3359
Merged
nicoloboschi merged 2 commits intoAug 19, 2026
Merged
Conversation
nicoloboschi
added a commit
that referenced
this pull request
Aug 18, 2026
#3527) (#3585) * fix(embed): harden daemon and UI lifecycle (#3099, #3100, #3517, #3520, #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. * fix(embed): keep the UI probe short and clean up the lock-owner sidecar 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. * fix(embed): scope the long health budget to the reclaim probe only 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.
HindsightEmbedded previously defaulted llm_api_key to an empty string and always forwarded it to the daemon. Omitting the argument therefore replaced a valid key inherited from the parent environment or profile. Use None as the default and omit the config entry only in that case. Keep an explicit empty string as an override, so local LLM services can still run without authentication. Add regression coverage for omitted, empty, and non-empty keys.
Extends the llm_api_key fix to every LLM/daemon setting HindsightEmbedded
forwards. llm_provider, llm_model, log_level and idle_timeout were still sent
unconditionally, so a client constructed without them overwrote whatever the
profile had configured -- and _register_profile then persisted the placeholders
into the profile's .env file. A profile set up for anthropic came back as
groq / openai-gpt-oss-120b on disk after a single keyless HindsightEmbedded
call, which left the newly inherited key pointed at the wrong provider.
All five settings now default to None and are omitted from the daemon config
when not passed, so the daemon resolves them from the profile .env, then the
parent environment, then its own defaults. Those defaults already match the
values that were being hardcoded for log_level ("info") and idle_timeout (0).
Dropping the hardcoded "groq" / "openai/gpt-oss-120b" means a client that
configures nothing anywhere now lands on the server's default provider
(openai); every documented example passes llm_provider explicitly.
Moves the config coverage into test_embedded_config.py, which drives the real
embed-manager start path with Popen stubbed and asserts on the environment the
daemon child actually receives, not just on the intermediate config dict.
nicoloboschi
force-pushed
the
fix/embed-llm-key-inheritance
branch
from
August 19, 2026 10:04
acab7d4 to
5bf76e5
Compare
nicoloboschi
changed the title
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
2 participants
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.
Summary
HindsightEmbeddednow forwards only the settings the caller passes explicitly. Everything omitted is left out of the daemon config so the daemon resolves it from the profile's.env, then the parent environment, then its own default.llm_api_key,llm_provider,llm_model,log_levelandidle_timeoutall default toNone""is still an override, so local LLM services with no authentication can clear an inherited keyProblem
HindsightEmbeddedsent every LLM setting on every construction, using placeholder defaults for the ones the caller never mentioned. Indaemon_embed_manager._start_daemon_lockedthe caller's config is merged over the profile.env, and anyHINDSIGHT_*entry that is notNoneis copied into the daemon's environment — an empty string included. Constructing a client without a key therefore overwrote a valid key inherited from the parent environment or the profile.It did not stop at the process environment:
_register_profilewrites the merged config back to the profile's.env, so one keyless construction persisted the placeholders to disk. A profile configured for Anthropic came back asgroq/openai/gpt-oss-120bin its own.envfile, leaving the (now correctly inherited) Anthropic key pointed at the wrong provider.Fixes #3253.
Behaviour change
Dropping the hardcoded
llm_provider="groq"/llm_model="openai/gpt-oss-120b"defaults means a client that configures a provider nowhere — not in the constructor, not in the profile.env, not in the environment — now falls through to the server's own default (openai) instead of groq. Every example inhindsight/__init__.py, the README and the docs site passesllm_providerexplicitly, so this only affects callers that relied on the undocumented implicit default.log_levelandidle_timeoutare unaffected in practice: the daemon's own fallbacks are already"info"and0, the same values that were previously hardcoded here.Testing
New
hindsight-all/tests/test_embedded_config.pycovers both layers. Asserting onclient.configalone would not catch a regression in how the embed manager merges that config with the profile and the environment, which is where the bug actually surfaced — so the end-to-end tests drive the realensure_runningpath withsubprocess.Popenstubbed and assert on the environment the daemon child receives, plus the contents of the profile.envafter a successful start.pytest hindsight-all/tests/test_embedded_config.py hindsight-all/tests/test_embedded_namespaces.py— 16 passedmain, and 2 still fail against the first commit of this PR./scripts/hooks/lint.sh— all lints passed