fix(*): start, find and reuse the everos memory service correctly - #310
fix(*): start, find and reuse the everos memory service correctly#310gloryfromca wants to merge 17 commits into
Conversation
…k to PATH
`uv tool install` exposes only the requested package's entry points, so a
released install puts `raven` in ~/.local/bin and leaves `everos` inside the
tool venv. `shutil.which("everos")` therefore returned None and every attempt
to start the memory server failed with "everos not found", even though everos
was installed as a pinned dependency in the very same environment.
Look in the running interpreter's own directory first. That fixes more than the
lookup failure: when PATH carries an everos from an unrelated environment,
which() would hand back a build whose version does not match raven's pin, and
that CLI would then operate on raven's data directory.
No Windows branch: the EverOS path is gated off on native Windows by both
callers, and a never-executed branch would only offer false assurance. A test
now locks the wizard's guard so that assumption cannot silently lapse.
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…fault The onboard wizard called ensure_everos_server() with no argument, so it probed the 18791 default while the memory backend connected to whatever plugins.config named. On a machine whose base_url had been seeded with an older default the wizard therefore concluded nothing was running, spawned a second instance, and that instance died acquiring the OME jobstore lock the first one already held. The user saw a 30s timeout telling them to check whether everos was installed, while a healthy server was serving requests on another port. Read the configured address in the wizard, pass it to both the first attempt and the retry, and name the real address in the failure hint instead of a hardcoded 18791. ensure_everos_server loses its base_url default as well. Requiring the argument turns "forgot to read the config" from a silent runtime bug into a signature error at the call site. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…entials
EverOS builds its LLM client eagerly during startup and raises
LLMNotConfiguredError when credentials are missing, which fails FastAPI startup
outright. raven spawned the server without checking, so the caller then waited
out the whole poll timeout on a process that had already exited, and the real
reason stayed in the server log where nobody looks.
This is reachable out of the box rather than only after a misconfiguration:
memory.backend defaults to "everos" in the schema, while the everos.toml
template ships [llm] with an empty api_key. A fresh install therefore hit this
on every session.
Check everos_role_configured("llm") before spawning and raise
EverosNotConfigured, a RuntimeError subclass so existing handlers still treat it
as "server unavailable" while a caller that wants a better message can narrow
on it. The check runs only on the spawn path: a /health 200 already proves the
LLM client was built.
Two pre-existing tests read the developer's own ~/.everos to get past the new
gate, which would have passed locally and failed on a clean CI runner. Both now
seed an isolated toml. Verified with the full unit suite under a throwaway HOME:
6157 passed, 33 skipped (all pre-existing provider-matrix skips).
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
_start_server_if_unlocked discarded the Popen handle, so the poll loop had no way to tell "still booting" from "already dead". A child that failed to start exited in well under a second, yet the caller kept polling for the full 30s budget and then blamed a missing install or an occupied port. On a broken setup that cost every single session 30 seconds of silence. Return the handle and check proc.poll() each round. A dead child now fails in about a second and the error carries the last exception line from the server log, so the reason arrives with the failure instead of waiting in a file the user has to know to open. The timeout itself is unchanged. It is the budget for a slow first boot, and shortening it would trade one wrong behaviour for another; a test pins that a live-but-unhealthy child still gets the whole budget. A None handle means another process holds the startup lock, which is not a dead child and keeps polling. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…emory The wizard offered "Retry" or "Skip (memory disabled)", and a second failed retry disabled memory without asking. Both paths called _set_memory_backend(None), so one transient startup problem silently cost the user long-term memory -- with nothing in the summary explaining that memory was now off. By this point the models are already written to everos.toml, and the runtime starts the service on demand anyway, so a failed start says nothing about whether the user wants memory. Offer three outcomes instead: retry, leave the settings in place for the runtime to retry next session, or explicitly turn long-term memory off. Retry now loops instead of getting one attempt. The failure line drops its guesses about a missing install and an occupied port. The exception already names the actual reason now that the caller can tell a dead child from a slow one. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…rvice The memory service starts on demand at the beginning of every session, and both the wait and its failures were invisible. A cold start printed nothing, so seconds of silence read as the agent having hung; a failure went only to the log file the caller writes, so a backend that had never worked looked like an agent that was merely vague. ensure_everos_server takes an on_wait callback that fires once, and only when a boot is actually about to be waited on. Passing it a print keeps the common case -- a server that is already answering -- as quiet as it is today, which existing tests pin. Missing credentials get their own message naming `raven onboard`, and degrade to the no-op adapter instead of raising: the session simply has no long-term memory rather than reporting a failure the user cannot act on from there. Other failures still raise, now after saying what went wrong. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Two facts about EverOS were being re-derived at every call site instead of recorded once: which root holds the memories, and whether raven may write to it. Both are decisions, not derivable properties, and the code was guessing. The root is now recorded in plugins.config["everos-memory"]["root"], and configure_everos_env writes EVEROS_ROOT from it rather than deferring to an ambient value. Following the environment was quiet data loss waiting to happen: run raven once with EVEROS_ROOT set and the memories land there, then run it without and raven reports none while they sit on disk. That reverses a documented contract, so the test asserting the old direction now asserts the new one rather than being deleted. Fresh installs get <raven data dir>/everos. The old ~/.everos/raven squatted on a scope slot of the user's own root -- that is exactly the directory EverOS assigns an app_id of "raven" -- so anything enumerating scopes there would walk into raven's config files. Existing installs keep the old location: resolution prefers it when it holds a config, so nothing is moved. "owned" gates every write. A root raven created is raven's to configure and serve; a root the user manages is read-only, which now includes template files. ensure_everos_home was dropping everos.toml / ome.toml into whatever root was active, and "those files are usually already there" is not a basis for a read-only promise. Cold start follows the same rule: an unowned root that is not answering is reported, not started, because starting it would take the OME jobstore lock the user has not offered. The address moves into <root>/everos.toml [api] and the child is spawned with --root instead of --port. A command-line port override left the file describing an address nobody listened on -- the drift that made the wizard probe 18791 while the backend talked to 1995. Writing it makes the root self-describing, and --root makes a running server identifiable from `ps` when a stale one has to be found. A pidfile records what raven started, and stopping verifies the command line first: a pidfile is stale information, and a reused pid would otherwise send SIGTERM to an unrelated process. Two wizard tests were passing for the wrong reason. The old fixture redirected only the config path, not the root, so ensure_everos_home wrote into the developer's real ~/.everos/raven during unit runs and the assertions were reading a file with no template merged into it. The fixture now redirects the root; both tests assert what actually matters -- that no role reads as configured -- rather than that the file is absent. Verified with the full unit suite under a throwaway HOME: 6185 passed, 33 skipped (all pre-existing provider-matrix skips). Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…uming one raven assumed exactly one EverOS root at one address, and started a server whenever that address did not answer. Both assumptions were wrong in ways that cost users their memory: a root can already be served on another port, and a root can belong to the user rather than to raven. Discovery reports four facts per candidate root, kept apart because they fail independently: whether [llm] is configured at all, what address the root declares, whether that address answers, and whether the OME jobstore lock is taken. The lock is the only reliable answer to "is this data already being served" -- it is keyed on the data directory, not on a port, which is why a second server on a different port dies there. That gives a name to the state raven could not previously see: data served somewhere other than where it declares. A --port override, an EVEROS_API__PORT in the environment, or a non-server holder of the lock all land in it, and none can be fixed by starting something, since one directory admits one engine. It used to surface as a 30s timeout blaming a missing install. Candidate order is preference, not health: a recorded root wins even over a healthier one, because switching roots behind the user's back changes which memories raven has. The user's own ~/.everos is offered only when raven has no configured root of its own -- suggesting it earlier would invite abandoning raven's own memories, and adopting it costs the user exclusive use of their data. Nothing here writes, signals, or starts anything. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…tarting one Step 4 assumed there was nothing running and configured from scratch, so a machine that already had an EverOS ended up with a second one -- which then died on the jobstore lock the first one held, reported as a 30s timeout. The step now discovers first and branches on ownership. A root raven owns is converged onto the standard address. An install seeded with an older default kept its port forever, so support began with "which port is yours"; the old server is stopped and restarted on the standard one. Nothing is asked: the process is raven's own, crash recovery replays interrupted strategy runs, and SIGTERM drains what is in flight before releasing the lock, so there is no decision for the user to make. A process raven did not start is never signalled -- its address stays in use and the user is told why. Data held by something raven cannot identify stops the step rather than walking into role configuration. One memory directory admits one engine, so there is nothing that starting could achieve. A root the user manages is read-only: raven records its address and reports what its server can actually do, then leaves it alone. It never writes those models and keys, never drops template files in, and never starts or stops the process -- starting takes the jobstore lock exclusively, which is the user's to grant. When it is not running the user is shown the command and asked to start it, and the address is probed again; declining hands the step back to building raven its own memory rather than leaving the user with none. Ownership and the root are recorded in plugins.config, and "configured" now has a single definition shared by discovery, the wizard and doctor -- an architectural guard caught the second copy this change introduced, which is the same class of disagreement that once made the wizard's Back loop on itself. Verified with the full unit suite under a throwaway HOME: 6206 passed, 33 skipped (all pre-existing provider-matrix skips). Also confirmed the suite no longer creates anything under $HOME/.everos. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Coverage of _server.py sat at 76%, and the gap was not incidental: the health probe, the `ps` verification, the OME lock probe and the stop-and-wait loop were stubbed out by every test that used them. That left the layer where a wrong assumption about flock or ps output would go unnoticed, and the lock probe is what the whole "served somewhere other than where it declares" state rests on. Six tests now run them for real: an untouched root reads as free, a held lock is detected and stops being held when its holder leaves, ps declines to mistake the test process for a server, ps recognises one whose command line says it is, a dead pid is not a server, and the health probe reads 200 / 503 / connection refused off an actual socket. The held-lock case also pins the documented flock caveat -- the lock lives on the open file description, so a holder inside the same process collides with itself, which is why only the wizard and doctor may ask. The ps-recognises case first passed alone and failed in a full run: `sh -c "sleep 5 # marker"` gets exec-optimised, so the shell replaces its own command line and the marker disappears. Spawning a python interpreter instead is deterministic -- it never rewrites its argv. Confirmed stable across repeated runs rather than retried until green. _server.py 76% -> 85%; the remaining misses are logging and OSError branches. Full suite: 6212 passed, 33 skipped. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Adversarial review of the branch, four findings. The serious one: declining to reuse a user-managed EverOS handed it over anyway. Session one records their root with owned=false; session two discovers it, the user says "no, give Raven its own", and the fall-through asked everos_root() for somewhere to build -- which returns the recorded root, theirs. raven then claimed ownership of it, seeded it with templates and overwrote its models and keys. The exact outcome the ownership model exists to prevent, on the one path that says out loud it does not want it. owned_everos_root() now answers the question the self-build branch is actually asking. "The active root" and "a root raven may build in" diverge precisely after a read-only reuse, and one function was answering both. The test that should have caught it asserted only that ownership flipped to true -- which is equally true when raven has taken over the user's root. It now asserts which root was chosen, and drops the fixture that pinned ownership to true, since that pinning was hiding the condition under test. Confirmed by reverting the fix and watching it fail. Ownership becomes a gate on the write primitives rather than a rule each caller remembers. set_everos_section, clear_everos_section and ensure_everos_home now refuse on a root raven does not own, raising EverosRootNotOwnedError -- not PermissionError, which callers catch as a filesystem condition and would swallow. No existing test broke, which says the current call sites were all checking; the point is that the next one cannot forget. This is the same shape as the drift the branch is about: one rule enforced in several places, one of them wrong. Config migration no longer re-reads raven's config. _migrate_config holds the dict it is migrating, but called everos_root(), which reads whatever path is globally current -- so loading a config by explicit path could stamp it with another file's root. It now asks fallback_everos_root(), which derives the answer without reading anything. stop_recorded_server returned one bool for three situations, and the wizard printed "Raven did not start this process" for all of them. A server still draining memory work is not a foreign process, and telling a user that sends them looking for the wrong thing. StopOutcome names the three, and each gets its own sentence. Not changed, on purpose: the 2s discovery probe stays. Shortening it would halve the worst-case wizard pause and risk misjudging a slow server as dead -- which starts a second instance into the jobstore lock, the exact failure this branch fixes. 6222 passed, 33 skipped. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
|
Claude finished @gloryfromca's task in 3m 50s —— View job ReviewNo blockers; this can merge. Scope of this pass: the author's own adversarial self-review found six defects against Verified still fixed (no regression from the new commits):
New commits: no new issues found.
|
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Adversarial review
I read this as three separable pieces and attacked each one on its own terms:
- the three original defects (address split,
everoslookup, spawning without credentials), - the discovery / ownership model layered on top,
- the port-convergence step, which is the only part that touches a running process.
Held up under attack. The everos sibling lookup is right, and the reason given for preferring it over PATH (version pinning, not just a missing entry point) is the stronger reason. _require_llm_configured sits on the spawn path only, which is correct -- a server that answers /health has already proven its credentials. The dead-child detection via the retained Popen handle closes a real hole. ome_lock_held was worth checking against the other side rather than trusting the docstring: everos acquires the same lock with portalocker.LOCK_EX | LOCK_NB on <jobstore_path>.lock (infra/ome/engine.py:546), MemoryRoot.ome_db is derived from the root and not configurable, and memory/cascade/_backfill.py:1031 already probes it acquire-and-release exactly this way. Same lock, same mechanism, same path -- that probe is sound. Making configure_everos_env assign rather than setdefault is the right direction, and owned_everos_root() splitting "the active root" from "a root raven may build in" is the correct fix for the bug the description records.
Did not hold up. Six findings below, four of them reproduced by running the wizard against this branch (worktree at cf49c4b, throwaway HOME). The first two are the serious ones, and they share a shape with the bug this PR already caught in itself: a decision is recorded correctly and then a later branch returns before anything acts on it.
Findings
| # | Severity | Where | What |
|---|---|---|---|
| 1 | High | onboard_everos.py:1613 |
Convergence stops the server, then Keep it enabled returns without restarting it -- memory is down when the wizard exits |
| 2 | High | onboard_everos.py:1609 |
Declining to share a user-managed root is discarded whenever memory already reads as enabled |
| 3 | Medium | onboard_everos.py:1525 |
busy_elsewhere discards the StopOutcome the sibling branch handles carefully |
| 4 | Medium | _server.py:320 |
The spawned child inherits EVEROS_API__HOST / EVEROS_API__PORT, which outrank the toml the branch made authoritative |
| 5 | Low-Med | onboard_everos.py:1446 |
An owned root whose server is simply not running never converges; the legacy port persists |
| 6 | Low | backend.py:418 |
Reusing a user-managed server returns before the capability warning, on the one root raven cannot fix |
Reproductions
All four run against cf49c4b with the branch's own fixtures (tmp_env, everos_isolated, _found, _root_state), HOME thrown away. They are written to fail on the current code:
test_A_converged_server_is_never_restarted_on_keep FAILED STOPPED=[.../everos] STARTED=[] base_url=http://localhost:18791
test_B_declining_is_discarded_when_memory_is_enabled FAILED root=.../theirs (expected .../mine)
test_C_busy_elsewhere_ignores_a_failed_stop FAILED _converge_owned_root -> True after STILL_DRAINING
test_D_a_stopped_owned_root_never_converges FAILED base_url stayed http://localhost:1995
Finding 4 is not reproduced as a test; it is read off everos' own source (config/settings.py:466, "Source order: init_args > env_vars > everos.toml > default.toml").
On the test suite
test_a_legacy_port_is_moved_to_the_standard_one currently locks finding 1 in: it answers keep, asserts stopped == [root] and base_url == 18791, and never asks whether anything is listening there. test_declining_falls_through_to_ravens_own_root passes only because its fixture config carries no memory.backend, which makes _memory_enabled() false -- the same "the fixture hid the condition under test" pattern the description calls out one paragraph earlier. Both would catch their bug with one added assertion; see the inline comments.
Default event is a comment, not request-changes -- that call is yours.
|
|
||
| if found is not None and found.owned: | ||
| _record_root(found.root, owned=True) | ||
| if not _converge_owned_root(found): |
There was a problem hiding this comment.
[High] Convergence stops the server and the Keep it enabled branch returns before anything restarts it.
Failure sequence, on the exact install this step exists for (owned root, recorded address 1995, server up):
_converge_owned_root(found)->state.serving and declared_url != target, so it printsMoving it to the standard address http://localhost:18791...stop_recorded_server(state.root)->STOPPED. The memory service is now down._set_base_url(target)-> raven's config now says 18791;<root>/everos.toml[api]still says 1995; nothing is listening on either.- returns
True, falls to_memory_enabled()at line 1616 -> true (backend iseveros, the llm role is on disk) Keep it enabledis the first choice, so it is the default answer on exactly this install- line 1631:
return None # backend already "everos" + models on disk; leave as-is
The wizard exits having announced a move that never completed, with the service stopped. ensure_everos_server at line 1707 is only reached through the role screens, i.e. only if the user picks Reconfigure.
It does self-heal on the next raven agent session (backend.start -> ensure_everos_server(18791) -> spawn -> set_everos_api rewrites [api]), so this is not data loss. But it leaves the wizard printing a half-finished sentence, and it recreates the branch's own headline symptom -- config and toml naming two different addresses, neither served -- for the whole window in between. Manual check 3 in the description ("confirm 18791 is listening") fails on the keep path.
Reproduced (fails on cf49c4b):
def test_A_converged_server_is_never_restarted_on_keep(tmp_env, everos_isolated, monkeypatch):
monkeypatch.setattr(onboard_everos, "_report_everos_capabilities", lambda: None)
monkeypatch.setattr(onboard_everos, "_memory_enabled", lambda: True)
root = tmp_env.parent / "everos"
_found(monkeypatch, _root_state(root, declared_url="http://localhost:1995"))
stopped = []
monkeypatch.setattr(_server, "stop_recorded_server",
lambda r, **_kw: (stopped.append(r), _server.StopOutcome.STOPPED)[1])
started = []
async def _spy(url, **_kw): started.append(url)
monkeypatch.setattr(_server, "ensure_everos_server", _spy)
monkeypatch.setattr(questionary, "select", lambda *a, **kw: _Answer("keep"))
onboard_everos._step4_memory(skip=False, non_interactive=False,
main_model="openai/gpt-4o-mini", warnings=[])
assert stopped == [root]
assert started != [] # -> AssertionError: assert [] != []Fix. Convergence is the one thing in this step that leaves a side effect behind, so it should finish what it started rather than depend on a later branch it does not control. Have _converge_owned_root restart the server itself after a successful stop -- it already knows the target and it already had one running, so asyncio.run(ensure_everos_server(target)) there closes the loop and lets it report the move as done (or report it as failed, which is also more honest than silence). Failing that, the keep branch at line 1631 has to run the start block before returning. Whichever you pick, test_a_legacy_port_is_moved_to_the_standard_one should assert something is serving target at the end, not just that base_url was rewritten -- as written it passes on the broken behaviour.
| outcome = _reuse_unowned_root(found) | ||
| if outcome is not _OWN_ROOT_INSTEAD: | ||
| return outcome | ||
| found = None # declined the reuse -> build raven its own root below |
There was a problem hiding this comment.
[High] Declining to share a user-managed root is silently discarded whenever memory already reads as enabled.
This is the same bug the description records finding by reviewing the branch against itself, one level up. owned_everos_root() is the right fix and it is correct -- but it lives at line 1663, past a branch that returns first.
Failure sequence -- session 2 of a read-only reuse, which is the only way to reach this screen twice:
- config on disk:
memory.backend = "everos",everos-memory.root = <theirs>,owned = false,base_url = http://localhost:8000(written by session 1's reuse) discover()puts the recorded root first; it is configured, sopick()returns it withowned=False_reuse_unowned_root-> user picks "No, give Raven its own memory" ->_OWN_ROOT_INSTEAD- line 1609:
found = None - line 1616
_memory_enabled()->oc._load_raw_config()["memory"]["backend"] == "everos"and_everos_role_configured("llm")readseveros_root()-- still theirs, and theirs is configured (that is why it was offered for reuse). So: true Keep it enabled-> line 1631return None
owned_everos_root() at 1663 is never reached. The config still says root = <theirs>, owned = false, base_url still points at their server, and raven goes on using the root the user just declined to share. The user's answer changed nothing.
Reproduced (fails on cf49c4b):
def test_B_declining_is_discarded_when_memory_is_already_enabled(tmp_env, monkeypatch):
theirs, mine = tmp_env.parent / "theirs", tmp_env.parent / "mine"
theirs.mkdir(parents=True, exist_ok=True)
(theirs / "everos.toml").write_text('[llm]\nmodel = "gpt-4o-mini"\napi_key = "sk-user"\n')
monkeypatch.setattr(ue, "default_everos_root", lambda: mine)
tmp_env.write_text(json.dumps({
"memory": {"backend": "everos"},
"plugins": {"config": {"everos-memory": {
"root": str(theirs), "owned": False, "base_url": "http://localhost:8000"}}},
}))
assert onboard_everos._memory_enabled() is True # precondition
_found(monkeypatch, _root_state(theirs, owned=False, declared_url="http://localhost:8000"))
answers = iter(["own", "keep"]) # reuse screen, then keep/reconfigure
monkeypatch.setattr(questionary, "select", lambda *a, **kw: _Answer(next(answers)))
onboard_everos._step4_memory(skip=False, non_interactive=False,
main_model="openai/gpt-4o-mini", warnings=[])
slice_ = json.loads(tmp_env.read_text())["plugins"]["config"]["everos-memory"]
assert Path(slice_["root"]) == mine # -> AssertionError: got .../theirsFix. The decline is a decision, so record it where it is made rather than letting a later branch decide whether it survives. At line 1609, alongside found = None, do the ownership switch immediately:
found = None
root = ue.default_everos_root()
_record_root(root, owned=True)After that everos_root() points at raven's own root, so the _memory_enabled() check at 1616 reads raven's toml (unconfigured on a first switch) and correctly falls through to the role screens. owned_everos_root() at 1663 then agrees by construction rather than by luck.
On the test that covers this. test_declining_falls_through_to_ravens_own_root passes today only because its fixture config carries no memory key, so _memory_enabled() is false and the keep/reconfigure branch never runs. That is the same failure mode the description flags two paragraphs earlier -- "a fixture that pinned ownership to true, hiding the condition under test". Adding "memory": {"backend": "everos"} plus a configured theirs/everos.toml to that test's seed reproduces the bug, and is the assertion that would have caught it.
| highlight=False, | ||
| ) | ||
| return False | ||
| stop_recorded_server(state.root) |
There was a problem hiding this comment.
[Medium] The busy_elsewhere branch discards the StopOutcome, and then walks on to spawn into the lock it failed to free.
The branch 60 lines up handles all three non-STOPPED outcomes with an explanation each, on the grounds that "saying the wrong one is worse than saying nothing". This one drops the value on the floor:
stop_recorded_server(state.root)
_set_base_url(target)
return TrueFailure sequence: the OME lock is held, find_recorded_server says it is raven's process, stop_recorded_server returns STILL_DRAINING (the server is draining a strategy run -- documented as up to 30s, and the stop waits 35s, so this is not exotic). The lock is still held. The step returns True, the user goes through four role screens, and ensure_everos_server(target) spawns a second instance into that same jobstore lock. That is the failure this branch exists to fix, reached by the branch that fixes it. SIGNAL_FAILED (EPERM) is the same story with no drain to wait out.
Reproduced (fails on cf49c4b):
monkeypatch.setattr(_server, "find_recorded_server", lambda _r: {"pid": 1, "root": str(root)})
monkeypatch.setattr(_server, "stop_recorded_server", lambda _r, **_kw: _server.StopOutcome.STILL_DRAINING)
state = _root_state(root, alive=False, lock_held=True, declared_url="http://localhost:1995")
assert onboard_everos._converge_owned_root(state) is False # -> got TrueFix. Bind the result and reuse the message map the sibling branch already builds -- lifting that dict to module scope makes it one call for both sites:
outcome = stop_recorded_server(state.root)
if outcome is not StopOutcome.STOPPED:
oc.console.print(... _STOP_REASON[outcome] ...)
return False # nothing can serve this root right now
_set_base_url(target)False is the right answer here specifically: unlike the serving-at-the-wrong-port case there is no working server to fall back on, and test_data_held_by_an_unidentifiable_process_stops_the_step already asserts that this branch must not walk into role configuration on data it cannot serve. It just does not cover the case where raven did start the holder and still could not stop it.
| # EVEROS_ROOT: a server that names its own root is one `ps` | ||
| # away from being identified, which matters when a stale | ||
| # instance has to be found and stopped. | ||
| [everos, "server", "start", "--root", str(root)], |
There was a problem hiding this comment.
[Medium] The child inherits EVEROS_API__HOST / EVEROS_API__PORT, which outrank the toml this branch just made authoritative.
The design here is "the root is self-describing; [api] in <root>/everos.toml is the authority, and both the child and any later reader agree by construction". --port was dropped to make that true. But Popen is called without env=, so the child inherits raven's whole environment, and everos resolves its bind address as:
# everos/config/settings.py:466
"""Source order: init_args > env_vars > everos.toml > default.toml."""
env_settings sits above the everos.toml source, with env_prefix="EVEROS_" and env_nested_delimiter="__". So a shell that exports EVEROS_API__PORT=8123 makes the child bind 8123 while set_everos_api has just written 18791 into the toml and raven proceeds to probe 18791.
Failure sequence: probe 18791 -> silent -> spawn -> child binds 8123 and takes the OME lock -> raven polls 18791 for the full 30s (the dead-child check does not fire; the child is alive and healthy) -> RuntimeError: did not become healthy within 30s. On the next run _describe reads declared_url = 18791, alive = False, lock_held = True -> busy_elsewhere, which is precisely the state _discover's own docstring attributes to "an EVEROS_API__PORT in its environment". The branch names this cause and then leaves the door open to producing it.
Not reproduced as a test -- read off everos' source rather than run, since it needs a real spawn.
Fix. The child's address must come from one place. Either strip the overrides so the toml genuinely wins:
env = {k: v for k, v in os.environ.items() if not k.startswith("EVEROS_API__")}
env["EVEROS_ROOT"] = str(root)
proc = subprocess.Popen([...], env=env, ...)or pass --host/--port from base_url (init_args outrank env) and keep set_everos_api as the record. The first keeps the "root is self-describing" story intact; the second reintroduces the command line but with the toml written to match, which is a different arrangement from the one that caused the original bug. Same argument applies to EVEROS_ROOT: --root already wins there, so only [api] is exposed.
Smaller thing on the same line: set_everos_api runs at line 308, before the startup lock is attempted. When another process holds the lock this rewrites [api] and spawns nothing, so a running server's declared address can be changed out from under it by a process that then does not start anything. Moving it inside the with file_lock(...) block costs nothing.
| from raven.plugin.memory.everos._server import StopOutcome, find_recorded_server, stop_recorded_server | ||
|
|
||
| target = _discover.default_new_root_url() | ||
| _set_base_url(state.declared_url or target) |
There was a problem hiding this comment.
[Low-Med] An owned root whose server is simply not running never converges -- the legacy port persists indefinitely.
_set_base_url(state.declared_url or target) is the first statement, and the only paths that later overwrite it with target require state.serving or state.busy_elsewhere. A root that is owned, configured, and just not running right now (the machine was rebooted; the user stopped it; the wizard is being run before the first start) falls through both and returns True with base_url still at the legacy address. ensure_everos_server then starts a server there and set_everos_api writes [api].port = 1995 back into the toml, so the legacy port is re-affirmed rather than retired.
Reproduced (fails on cf49c4b):
state = _root_state(root, alive=False, lock_held=False, declared_url="http://localhost:1995")
onboard_everos._converge_owned_root(state)
slice_ = json.loads(tmp_env.read_text())["plugins"]["config"]["everos-memory"]
assert slice_["base_url"] == "http://localhost:18791" # -> got http://localhost:1995The description's stated goal is that support "no longer starts with 'which port is yours'", and that only holds for installs whose server happened to be up during the one wizard run. Whether that matters is a product call, but the code reads as if convergence is unconditional and it is not.
Fix. If a stopped owned root should converge -- and it is the cheapest case, since there is nothing to stop -- the opening line becomes _set_base_url(target) and the serving-but-cannot-stop branch keeps its explicit _set_base_url(state.declared_url) where it already prints Keeping {declared_url}. If it deliberately should not, the docstring should say which states converge, because "bring a root raven owns onto the standard address" reads as all of them.
| # which is theirs to grant, not raven's to assume. | ||
| from raven.plugin.memory.everos._server import _probe_health | ||
|
|
||
| if await asyncio.to_thread(_probe_health, base_url): |
There was a problem hiding this comment.
[Low] The unowned-and-alive path returns before _warn_if_recall_cannot_work, on the one root raven cannot fix for the user.
This bare return skips the await asyncio.to_thread(self._warn_if_recall_cannot_work, base_url) that closes the owned path. _warn_if_recall_cannot_work's own docstring argues the warning is worth having because a degraded server "looks like an agent that is merely vague rather than one running on a lesser search -- the hardest kind of fault to attribute", and that raven doctor finds it only if the user thinks to ask.
That argument is stronger for a user-managed server, not weaker: raven cannot repair its embedding config, so telling the user is the only remedy available -- and the wizard already prints a capability line for exactly this root at _reuse_unowned_root, so the information is considered worth showing once. A server that was healthy at wizard time and lost its embedding provider on a later restart gets no signal at all.
Fix. Fall through to the shared warning instead of returning:
if await asyncio.to_thread(_probe_health, base_url):
await asyncio.to_thread(self._warn_if_recall_cannot_work, base_url)
returnNo other behaviour changes -- the function only reads /health and prints.
…stopped Six findings from an external review of this branch, all reproduced before being fixed and each confirmed by reverting the fix. Convergence stopped the running server and nobody started it again. The user picks "Keep it enabled" -- the first option, and the answer an existing install gives -- and _step4_memory returns before reaching any spawn. The wizard exits with the memory service down, raven's config naming 18791 and the root's [api] still naming the old port, nothing listening on either. Convergence is stop -> write -> start, and the design has said so all along; the implementation did the first two and left the third to a branch it does not control. _restart_here now completes the sequence in the function that began it, and an owned root that is simply not running converges too rather than re-affirming its legacy port forever. Declining to share a user-managed root was discarded whenever memory already read as enabled. owned_everos_root() -- added earlier on this branch for exactly this -- sits downstream of the _memory_enabled() branch, which reads the still-recorded user root, finds their configured llm, offers keep/reconfigure, and returns. The decision is now written the moment it is made, so everything downstream is right by construction rather than by reaching the right line. The busy_elsewhere branch dropped the StopOutcome and spawned anyway. When the stop returns STILL_DRAINING the jobstore lock is still held, so that spawn walks a second instance into the lock -- the failure this entire step exists to prevent. It now reports which of the three outcomes happened and stops. The spawned child inherited EVEROS_API__HOST / PORT, which outrank everos.toml in everos's own source order -- the [api] section this branch made authoritative was one env var away from being ignored. Those keys are stripped from the child's environment. set_everos_api also moves inside the startup lock: losing that race means another process is already spawning, and rewriting the declared address on the way out moved the goalposts for a server that was already starting. Reusing a user-managed server skipped the capability warning. The argument for it is stronger there, not weaker: raven cannot repair someone else's embedding config, so saying so is the only move it has. Two existing tests had locked two of these in. The convergence test asserted that the service stopped and the config moved, never that anything was listening afterwards. The decline test seeded no memory.backend and no llm in the user's root, so _memory_enabled() was false and the branch under test was never entered -- it passed for the wrong reason twice more while being fixed, because a single stubbed answer stood in for two screens. Both now assert the precondition that the branch is reachable before asserting what it does, which is the check that was missing when this branch mutation-tested a fix and confirmed only that the function was right, not that anything reached it. 6222 passed, 33 skipped. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Branch coverage on the new step-4 code found four decision paths with no test at all, two of them the fixes from the previous commit -- the review's repro script proved they were broken, and only two of its four cases were carried over into the permanent suite when the fixes landed. Now covered: a root already on the standard address is left alone (the steady state, and the one where doing anything is wrong); an owned root that is simply down still converges rather than re-affirming its legacy port forever; a lock that could not be freed stops the step instead of spawning a second instance into it; and a restart that fails reports why rather than swallowing it. Each asserts the absence that matters -- nothing stopped, nothing spawned, the role screens never reached -- since "it printed something" would pass for an implementation that walked on anyway. 6226 passed, 33 skipped. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
|
Claude finished @0xKT's task in 2m 8s —— View job Claude's review
The trigger comment carried no text beyond All six findings are fixed in
I also checked two things not called out explicitly in the fix commit, since they're easy places for this kind of change to break:
The new tests in One note, not a blocker: I couldn't execute I didn't find any new issues introduced by |
The previous commit ran ruff check but not ruff format, so pre-commit's ruff-format hook rewrapped four calls that fit on one line and CI failed on the diff. No behaviour change. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
f6b7c14 to
eb3cf83
Compare
The 30s was a guess with no measurement behind it, and it stopped meaning what it used to. Once the poll loop watches the child's exit code, a boot that cannot succeed is reported in about a second whatever the timeout is -- so this value no longer detects failure, it only decides how long a user waits. That reframes the size. Overrunning the budget is cheap: the child keeps booting, this session goes without long-term memory, and the next session finds a healthy server. So a small budget costs at most one session and heals itself, while a large one blocks the first session of every machine's uptime. Measured cold start against a real everos 1.2.1 is 2.1s on one store and 2.7s on another, both with a small index -- 10s keeps roughly a fourfold margin over that. The timeout message stops calling it a failure. The process is up and still booting, which is what the user should hear and what the next session will find. A test pins the default: the number reads like a safety margin, which invites raising it back, and the reason it can be small is not visible from the call site. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Cutting the startup budget to 10s left two docstrings narrating the symptom as "a mysterious 30s startup timeout" and "reported to the user as a 30s timeout" -- a number that no longer appears anywhere in this code. A reader who greps for it finds nothing and then has to decide whether the comment or the code is lying. The mechanism is what those paragraphs are for: a second instance dies on the jobstore lock, and the user saw a startup timeout that blamed the install. Both keep saying that, without pinning a figure that moved. The remaining 30s in stop_recorded_server stays: it is not this module's budget but EverOS's own drain limit (`ome/engine.py` wait_idle(timeout=30.0)), and the 35s stop timeout is sized against it. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The EverOS root had no reader. The wizard printed it once while converging and no command showed it again, so "where are my memories, and is this one mine" had no answer short of reading config.json by hand -- for the one question users ask after memory looks wrong. Doctor now reports the root and the address, and says outright when the root is one the user manages rather than raven's to write. That is the command where someone is asking about state rather than being walked through a decision, so identity belongs there. The convergence screen drops the same path. Nothing on that screen is the user's to decide -- the port move is automatic -- so a path they never chose and cannot act on is noise; the address and the reason it is moving stay, because they are what justifies stopping a running service. The reuse screens keep it: there the user is choosing whether to share their own EverOS, and the path is how they recognise which one it is. Same string, opposite value, decided by whether anything is being asked of them. Also folds the loopback spelling back to one place. set_everos_api restated "127.0.0.1" as a fallback while the default URL says "localhost"; it now derives both host and port from that URL, so one constant decides how loopback is spelled and the address written into [api] is the address that gets probed. (The 127.0.0.1-vs-localhost mismatch observed while smoke testing came from the test harness seeding a root, not from raven -- raven already wrote back whatever hostname its base_url named.) 6229 passed, 33 skipped. Convergence re-verified end to end against a real everos after the copy change. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
2 participants
Summary
Starting the EverOS memory service was built on two assumptions that turn out to
be wrong on real machines: that there is exactly one root at one address, and
that a root not answering means one should be started. Together they produced a
failure a user cannot diagnose -- a 30-second silence followed by advice to check
whether everos is installed, on a machine where a healthy server was already
serving requests on another port.
Traced from a report of that exact symptom. Three independent defects were on the
path, and the shape underneath all of them is the same: one fact recorded in two
places, drifting apart.
The wizard probed a different address than the backend used. It called
ensure_everos_server()with no argument, so it checked the 18791 default whilethe memory backend connected to whatever
plugins.confignamed. An installseeded with an older default therefore looked idle to the wizard, which spawned a
second instance; that instance died acquiring the OME jobstore lock the first one
held. The lock is keyed on the data directory, not the port, so no port choice
could have saved it.
everoswas looked up only on PATH.uv tool installexposes only therequested package's entry points, so a released install has
ravenin~/.local/binandeverosonly inside the tool venv. Every start failed with"everos not found" although everos was installed, pinned, in the same
environment. The lookup now prefers the interpreter's own directory, which also
avoids picking up an everos from an unrelated environment whose version does not
match raven's pin.
A server was spawned without checking it could start. EverOS builds its LLM
client eagerly and fails startup outright without credentials.
memory.backenddefaults to
"everos"while the shipped everos.toml carries an empty[llm]api_key, so a fresh install hit this on every session: a doomed process, a full
poll timeout, and the reason left in a log file. The credentials are now checked
before spawning, and the Popen handle is no longer discarded -- a child that dies
is noticed in about a second instead of costing the whole 30s budget, and the
failure carries the last exception line from the log.
On top of the fixes, the step now discovers before it configures.
The EverOS root and whether raven owns it become recorded decisions rather than
values re-derived at each call site.
configure_everos_envwritesEVEROS_ROOTfrom the record instead of deferring to an ambient value -- following the
environment was quiet data loss waiting to happen, since a run without the
variable reported no memories while they sat on disk. Fresh installs get
<raven data dir>/everos; the old~/.everos/ravensquatted on a scope slot ofthe user's own root, and existing installs keep it, so nothing is migrated.
Ownership decides what raven may do. A root raven owns is converged onto the
standard address: the old server is stopped with SIGTERM (crash recovery replays
interrupted strategy runs, and shutdown drains what is in flight) and restarted,
so support no longer starts with "which port is yours". A process raven did not
start is never signalled. A root the user manages is read-only throughout --
raven records its address and reports what the server can do, and never writes
its models, drops template files in, or starts and stops it.
The address itself moves into
<root>/everos.toml[api]and the child isspawned with
--rootrather than--port. A command-line override was what letthe file describe an address nobody was listening on.
Two smaller things fixed along the way, both user-visible: a failed start no
longer decides that the user has given up on memory (retry, keep the settings for
the runtime to retry, or explicitly turn memory off), and a session that is
waiting on a cold start now says so instead of sitting silent.
Type
Verification
Full unit suite under a throwaway
HOME, so nothing depends on the developer'sown
~/.everos:The 33 skips are pre-existing provider-matrix parametrisations, unrelated to this
change.
Coverage of the modules this change owns:
raven/plugin/memory/everos/_discover.pyraven/config/update_everos.pyraven/plugin/memory/everos/_server.py_server.pywas at 76% until the four OS-touching helpers got tests that do notmock them -- the health probe against a real socket,
psverification againstreal
psoutput, and the OME lock against a realflock. That gap mattered morethan the number suggests: the lock probe is what the "served somewhere other than
where it declares" state rests on, and it had no unmocked coverage at all. What
remains uncovered is logging and
OSErrorfallbacks; no decision path isuntested.
Exercised against the real everos 1.2.1 binary, not only mocks:
LLMNotConfiguredErrorfrom the server log, where it previously waited 30s andblamed a missing install
/health200 in 2.08s, which is the firstmeasurement of EverOS cold-start time in this repo and shows the 30s budget has
ample headroom
Three assertions were mutation-checked by breaking the source and confirming the
test fails: the native-Windows guard, the configured-address read, and the
dead-child detection.
Assertions are written against what must not happen where that is the actual
risk --
Popennever called when credentials are missing, zero writes and zerosignals on a root the user manages -- because a test that only checks the error
message would pass an implementation that still spawned the doomed process.
Left to a human
Four checks cannot be made deterministic and are listed here rather than
committed as tests that skip, which would read as coverage that does not exist.
HOME, runraven agent -m "..."and time it. Expect one line within about a secondsaying the memory LLM is not configured, and no new
LLMNotConfiguredErrorin~/.raven/logs/everos-server.log. Run it three times; every run should bequick, since there is no failure cache.
uv tool installlayout finds everos. On a released install, confirm~/.local/bin/everosdoes not exist while~/.local/share/uv/tools/raven/bin/everosdoes, then runraven doctorandconfirm "everos not found" is gone.
is not 18791, note the memory count, run the wizard, then confirm 18791 is
listening, the old port is not,
[api].portin the root's toml reads 18791,and the memory count is unchanged.
against raven's own root on some other port, run the wizard, and confirm it is
still running afterwards with no signal sent -- and that the step says why it
could not take over.
Item 1 is the one worth doing before merge; it is the symptom users reported.
Risk
Behaviour changes users will notice. An install whose recorded address is not
18791 has its memory service stopped and restarted once, on the standard address;
memories are untouched, and the restart is skipped when raven did not start the
process. A failed start no longer disables long-term memory on its own. A cold
start prints one line while it waits.
A reversed contract.
configure_everos_envno longer honours an ambientEVEROS_ROOT; the recorded root wins. The test asserting the old direction nowasserts the new one rather than being deleted. Anyone relying on the environment
variable should record the root in
plugins.configinstead.Config migration.
plugins.config["everos-memory"]gainsrootandownedand loses the dead
modekey. Read-time normalisation only -- a pure dicttransform plus one existence check, no toml reads and no probes on the config
load path -- and the shape persists on the next write. Older raven versions
ignore the added keys, so rolling back the code does not require touching the
config.
Not covered here, and required at merge time. The GitLab trunk carries an
EverOS config surface (
raven.everos.set/clearand a web page) that does notexist on this branch. It writes through the same ops layer, so once this work
reaches the trunk those handlers must reject writes when
ownedis false, andmust reject the
apisection outright since the address is owned by theconvergence step. There is no exposure before that merge:
ownedcannot be falseon the trunk today, because nothing there records it.
Found by reviewing this branch against itself, and fixed here. Recording it
because the reason it was invisible is more useful than the bug.
Declining to reuse a user-managed EverOS handed it over anyway. One session
records their root with
owned=false; the next discovers it, the user chooses"no, give Raven its own", and the fall-through asked
everos_root()forsomewhere to build -- which returns the recorded root, theirs. raven then claimed
ownership, seeded templates, and overwrote their models and keys: the exact
outcome the ownership model exists to prevent, on the one path that says out loud
it does not want it. "The active root" and "a root raven may build in" diverge
precisely after a read-only reuse, and one function was answering both;
owned_everos_root()now answers the second.The test covering that path asserted only that ownership flipped to true -- which
is equally true when raven has taken over the user's root. It also used a fixture
that pinned ownership to true, hiding the condition under test. An assertion that
cannot distinguish the fix from the bug is not coverage, and the mutation check
that would have exposed it was not run on that one. It now asserts which root was
chosen, resolves ownership for real, and was confirmed by reverting the fix and
watching it fail.
Three smaller findings from the same pass: ownership is now a gate on the write
primitives rather than a rule each caller remembers (no existing test broke, so
the current callers were all checking -- the point is the next one cannot forget);
config migration no longer re-reads raven's config, which could stamp a config
loaded by explicit path with a different file's root; and
stop_recorded_serverreturned one bool for three situations, so a server still draining memory work was
reported as a process raven had not started.
Deliberately not changed: the 2s discovery probe. Shortening it would halve the
worst-case wizard pause and risk misjudging a slow server as dead, which starts a
second instance into the jobstore lock -- the exact failure this branch fixes.
Rollback. Revert the branch. The eleven commits are independent enough to
revert individually; the port convergence is the only one that touched a running
process, and re-running
raven onboardre-establishes whatever address the confignames.
Also fixed: unit tests were writing into the developer's own
~/.everos/ravenbecause the fixture redirected the config path but not the root, which left two
assertions passing for the wrong reason. Verified after the fix that a full run
creates nothing under
$HOME/.everos. One test added here was flaky on the firstattempt --
sh -c "sleep 5 # marker"is exec-optimised, so the shell replaces itsown command line and the marker vanishes, which passed alone and failed in a full
run. Rewritten to spawn a python interpreter, which never rewrites its argv, and
confirmed stable across repeated runs rather than retried until green.
Related Issues
N/A