Repository navigation
fix(*): tell a handed-over memory write from a lost one at shutdown - #544
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: a backend call can be cancelled before persistence but is reported as having reached long-term memory.
I found one concrete blocker, marked inline. I covered the full diff; all drain_backend_stores callers; the generic MemoryBackend contract and bundled EverOS path; cancellation, retry, repeated-drain, and shutdown accounting; branch history; backward compatibility; the changed assertions for weakening; and the repository's AGENTS/CLAUDE and runtime-context architecture rules. I found no separate issue in those areas.
Verification: the first focused run without optional extras stopped during collection because raven_everos was not installed. Re-running through the declared project setup, uv run --frozen --all-extras pytest tests/test_agent_loop_backend_dispatch.py tests/test_providers_factory.py tests/test_generation_swap.py tests/test_agent_loop_everos_pipeline.py tests/test_rpc_bootstrap.py tests/test_core_runtime_swap.py tests/test_cli_tui_commands.py tests/test_everos_backend.py tests/test_agent_loop_run_emit.py tests/test_memory_engine_face.py -x, passed all 402 tests. A minimal compliant cancellable backend reproduced the blocker as DrainOutcome(lost=0, in_flight=1) with no persisted record.
At teardown the store pipeline counted every unsettled record as lost, including the one a worker had already handed to the memory service. Cancelling that worker does not cancel its request, so the service finished the write anyway: on three one-shot runs the extraction landed 32 to 48 seconds after the host had told the user the turn was gone. drain() now returns DrainOutcome(lost, in_flight). A record is marked in flight around the backend call, and the mark is read before the workers are cancelled, because the cancellation unwinds that call and its finally clears exactly what is being counted. The host renders the two apart, so a handed-over turn reads "indexing is still finishing in the background" instead of blaming a service that answered. report_dropped_memory_writes is renamed report_memory_write_outcome: it no longer reports only what was dropped. The 2.0s drain budget is unchanged, and the reasoning now sits beside the constant. It is far shorter than the write it waits on -- one store measured 16.98s end to end, of which the flush leg was 12.1s -- and sizing it to cover that would park every CLI exit for as long while buying nothing, since the service completes the request either way and nothing needs to recall the turn that just ended. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
The comment said the count is taken before the cancellation because the cancellation clears the marks. A mutation moving the read below task.cancel() left every test green: cancel() only schedules, so nothing is unwound until this coroutine yields, and the collect await below is where the finally actually runs. Moving the read below that await does redden two tests, so the constraint is real and the comment named the wrong boundary for it. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
The two new imports reached raven.memory_engine.store_pipeline directly, which test_memory_engine_face pins against for everything outside the engine. lint-imports keeps its ten contracts either way, so the full suite was the only gate that saw it. DrainOutcome joins __all__, the TYPE_CHECKING block and the lazy map beside StorePipeline, and both call sites plus the notice tests now import it from the face. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
The split this branch added reports a record that was inside MemoryBackend.store() when the drain cancelled apart from one that never left the queue. Entering that call is not delivery, so the three surfaces reporting it claimed more than this side can see. Measured: a backend that persists only after an await, cancelled mid-await, leaves DrainOutcome(lost=0, in_flight=1) while the backend's persistence list stays empty. The shutdown notice then told the user the turn had reached long-term memory. A slow connection cancelled before its request body goes out has the same shape. The set itself is sound and stays. "Was inside the backend call" is computable, and an outcome nobody observed is neither a loss nor a win. What changes is every claim derived from it: the shutdown notice, the teardown log, and four docstrings that said the service had already been handed the record. The EverOS measurement that motivated the split is kept and scoped to the case it actually measured. in_flight keeps its name. Across this repo the word already means started and not settled, as in swap_in_flight, outbound.in_flight and a DAG node in flight, and it carries no delivery claim. The delivery claim was in the prose. One existing assertion changed meaning. The test pinning the wording "still finishing in the background", whose docstring stated the write had reached the service, is renamed and rewritten; its two negative assertions were right and are kept. A new sibling pins the other direction, so the notice is now held to the only honest claim from both sides. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
The raven agent teardown drains the memory backend and renders what the drain left, and no test reached those two lines: the shared harness stubs maybe_build_memory_backend to None, so the backend branch guarding them was never true. The same teardown in tui_commands has its own coverage; this copy had none, so dropping the render would have cost nothing red. The harness now takes an optional backend and drain outcome, both defaulting to the previous behaviour, leaving the thirty-three tests already using it unchanged. Proved load-bearing rather than assumed: removing the report_memory_write_outcome call turns the new test red on its stdout assertion, and restoring it turns it green with git diff HEAD empty. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
ad8392e to
9e11dd4
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new revision fixes my prior blocker by reporting a store call that outlives the drain as unsettled, without claiming either loss or delivery. The cancellable-backend reproduction now protects that boundary, and the added one-shot CLI test reaches the host rendering path that was previously unexercised. I found no new findings.
I reviewed the rebased full diff and its new commits; all drain callers; store cancellation and accounting; the generic backend contract and EverOS behavior; branch history and backward compatibility; the changed assertions for weakening; and the AGENTS/CLAUDE, runtime-context, and memory-engine-face constraints. The focused suite passed: uv run --frozen --all-extras pytest tests/test_agent_loop_backend_dispatch.py tests/test_providers_factory.py tests/test_cli_agent_commands.py tests/test_generation_swap.py tests/test_agent_loop_everos_pipeline.py tests/test_rpc_bootstrap.py tests/test_core_runtime_swap.py tests/test_cli_tui_commands.py tests/test_everos_backend.py tests/test_agent_loop_run_emit.py tests/test_memory_engine_face.py -x (438 passed). My prior thread is replied to and resolved.
|
Not a blocker. The split is the right call and it is pinned: reverting 1. The subtraction reads its two operands on opposite sides of an awaitIn Reaching it needs a worker that survives the cancel, and the design deliberately One correction to anyone acting on this: 2. Of the three render sites this PR rewired, one is driven by nothing
Base has the same shape, so this is inherited rather than introduced, and it is 3. One phenomenon, two different measurements
|
|
No blockers; suggestions only, and they are marked inline. The verifier note changes my stance from clean to nonblocking, not the merge decision. I reproduced observation 1 with a contract-conforming backend that suppresses cancellation: after four calls were snapshotted in flight, three workers settled during the collect await, and the drain returned Observation 2 is an inherited gateway-test coverage gap, and observation 3 is a prose measurement inconsistency; neither is a plain error that should hold this change. The focused shutdown and CLI suite remains green: |
## Summary
With a `MemoryBackend` plugin wired, memory lives in the plugin's store
and reaches the
turn as the `# Memory` section. The identity block still named two files
under agent home,
so "remember this" became an `edit_file` against
`user_memory/profile/user.md`. On a
non-interactive turn that write is refused at the ask tier, and the
agent reported the
refusal as the memory system having failed -- while the backend had
already stored the
turn and recalled it in the next session.
`identity_text` takes a `has_memory_backend` flag, threaded from the one
place each caller
already holds the backend: `build_context_engine`, and `AgentLoop` for
the
token-estimation prompt in `ContextBuilder`, which must render what the
turn renders.
With a backend the two file lines become one:
```
- Agent home: <home> -- your own skills, not a place for user artifacts.
- Memory is not yours to write: what is relevant is recalled into the `# Memory`
section each turn, and the host persists the conversation after it ends. You
cannot observe that write, so never report it to the user as done or as failed.
- Custom skills: <home>/skills/{skill-name}/SKILL.md
```
Three clauses, three jobs. Do not write it. It is being saved, so stop
looking for a place
to put it. You cannot see that write, so do not narrate it.
The last clause is the other half of the same defect, and the third item
issue 440 asks to
separate. `backend.store` is dispatched after the turn ends, so a claim
either way is one
the agent never checked -- and whatever it claims is extracted into
long-term memory next
turn as if it were an observation of the system. The issue's own repro
produced exactly
that: an episode titled `Memory Save Failure for Green Brompton Bike
'Pickle' Noted on
2026-09-16`, whose content was right and whose narrative was wrong.
Filtering that
server-side is not available from here, so the prompt is where it is
addressed.
Without a backend the rendered prompt is unchanged, byte for byte.
Whether the episodic
log earns its line at all is issue 122's question, and answering it on
this branch would
couple the two.
The approach is AmirF194's, from the closed PR #216. That branch cannot
be reopened: it
predates the history linearization, so it shares no merge base with
`main`
(`git merge-tree` refuses unrelated histories), its six files no longer
apply, and it did
not allow maintainer edits. It is reimplemented here against current
`main`, with the
author credited as co-author on the commit.
## Type
- [x] Fix
- [ ] Feature
- [ ] Docs
- [ ] CI / tooling
- [ ] Refactor
- [ ] Other
## Verification
Run on this branch's head, rebased onto `main` at c3d2821, Python
3.12, `--extra dev`.
- `pytest` over the 17 test files that construct `ContextBuilder`,
`IdentitySegmentBuilder`,
`build_context_engine` or call `identity_text` (the `git grep -l` list):
756 passed, 0 failed.
`SERPER_API_KEY` is unset for the run; with it set,
`test_agents_code_launcher.py::test_the_products_tool_face_is_the_forks_config_intent_minus_the_ledger`
fails on an extra `web_search` in the tool face, on this branch and on
the base alike.
- The three new tests were each watched failing without the change, by
neutering the
behaviour in place rather than the signature, then restoring the file
and confirming it
byte-identical by checksum:
- forcing the backend branch off reddens
`test_identity_with_a_backend_names_no_memory_file`;
- dropping the flag from `ContextBuilder._get_identity` reddens
`test_identity_with_a_backend_matches_the_estimation_prompt`;
- `test_identity_without_a_backend_still_names_both_memory_files` pins
the unchanged half.
- The no-backend claim is measured, not argued: the whole identity block
rendered from the
base and from this head is 3341 bytes on both sides and `diff` is empty.
- `make lint-python`, `make lint-types`, `make lint-imports`, `make
check-large-files` -- all exit 0.
- `RANGE=github/main..HEAD scripts/check_source_language.py` -- exit 0.
- `commitlint --config commitlint.config.cjs` on the commit body and on
the prospective
squash header -- both exit 0.
Driven against a live EverOS after this description was first written.
All three
symptoms issue 440 describes reproduce on the merge base and none
reproduce here: on
`main` turn 1 calls `read_file` then `edit_file` on
`user_memory/profile/user.md` and
grows it to 897 bytes, at the ask tier that write is refused and the
agent reports the
save as failed, and EverOS extracts that false report into four durable
memory entries.
On this branch turn 1 makes no tool call, agent home is untouched, and
the same grep over
the store returns nothing. Both sides recall the fact in a fresh
session. Full method,
verbatim output and teardown in a comment on this PR.
One run per cell, not a distribution. The host still dumps `user.md`
into `# Memory` with
a backend wired, which is unchanged here and out of scope below.
- [x] Relevant tests pass locally
- [x] Relevant lint / type checks pass locally
- [ ] User-facing docs or screenshots are updated when needed
No docs change: the only user-facing text is the system prompt itself,
and no document
quotes this block.
## Risk
User-visible change is confined to installs that have a memory backend
wired. There, three
prompt lines become three different ones; nothing else in the prompt,
and no behaviour
outside prompt assembly, moves. Installs without a backend render a
byte-identical prompt,
which is the measurement above.
`has_memory_backend` defaults to `False` on all four signatures it is
added to, so every
call site not updated here renders what it rendered before. The three
updated call sites
are the only ones that hold a backend.
Rollback is a revert of the single commit. Nothing persists and no
format changes.
Checked and deliberately not changed:
- `MemorySegmentBuilder` still calls `get_memory_context()`
unconditionally, so `user.md`
content reaches `# Memory` with a backend wired. Turning that off is the
question of
whether two memory mechanisms should coexist, which is larger than this
issue and which
four comparable systems answer by keeping both.
- The episodic log line, under the default no-backend configuration. It
belongs to issue 122.
- [x] Security impact considered
- [x] Backward compatibility considered
- [x] Rollback path is clear for risky changes
## Related Issues
Fixes #440 -- items 1 and 3 of the three the issue asks to separate,
both reproduced on
the merge base and verified gone here. Item 2 landed in #544.
#122 -- untouched, deliberately.
---------
Co-authored-by: gloryfromca <23442919+gloryfromca@users.noreply.github.com>
Co-authored-by: Amir Fathi <amirfathi.me@gmail.com>
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
3 participants
Summary
At teardown the store pipeline counted every unsettled record as lost, including one
a worker had already handed to the memory service. Cancelling that worker does not
cancel its request, so the service finished the write anyway, and the host told the
user the turn was gone while it was being indexed.
Measured on three one-shot CLI runs against a live EverOS: the client gave up at its
budget and
atomic_facts_extractedlanded for the same session 32 to 48 secondslater, every time. The message was wrong twice over -- the turn was written, and the
service was not unavailable.
Review found that the first version of this fix replaced that error with its mirror.
The mark is set on entering
MemoryBackend.store(), which is not a handoff: abackend that persists only after an await, cancelled mid-await, leaves
DrainOutcome(lost=0, in_flight=1)with the backend's persistence list empty, andthe notice then said the turn had reached long-term memory. A slow connection
cancelled before its request body goes out has the same shape.
MemoryBackenddeclares no delivery guarantee, so no evidence from the transport is available
without extending the protocol and asking every backend to implement it. This branch
takes the other route and preserves the uncertainty instead.
What changed:
StorePipeline.drainreturnsDrainOutcome(lost, in_flight)instead of one count.A record is marked in flight around the backend call, and the mark is read before
the collect await that unwinds the cancellation, because that unwinding clears
exactly what is being counted.
AgentLoop.drain_backend_storesreturns the outcome and logs the two apart: awarning for a turn this side watched fail, and for an unsettled one a line saying
the drain stopped waiting and cannot see whether the service finished it.
report_dropped_memory_writesbecomesreport_memory_write_outcome, since it nolonger reports only what was dropped. An unsettled turn reads "were still being
written when shutdown stopped waiting; whether the memory service finished them is
not known here" -- the outcome, not a verdict on it.
DrainOutcomeis published on the memory engine's face besideStorePipeline, socallers reach it the way
test_memory_engine_facerequires. Its docstring nowcarries both cases, with the EverOS measurement scoped to the one it measured.
raven agentteardown that renders the outcome. It had nocoverage at all: the shared harness stubs
maybe_build_memory_backendtoNone,so the branch guarding those two lines never ran.
in_flightkeeps its name deliberately. This repository already uses the word forstarted and not settled --
swap_in_flight,outbound.in_flight, a DAG node inflight -- with no delivery sense, so renaming it would detach the field from a
vocabulary that is already right. The delivery claim lived in the prose, and that is
what changed.
Deliberately unchanged: the 2.0s drain budget. It is far shorter than the write it
waits on -- one store measured 16.98s end to end, of which the flush leg was 12.1s --
and raising it to cover that would park every CLI exit for as long while buying
nothing, since the service completes a request it received either way and nothing
needs to recall the turn that just ended. That reasoning sits beside the constant,
because the gap between the two numbers reads like a defect and the next reader would
otherwise close it.
Type
Verification
Run on this branch's head, Python 3.12.13, all extras synced, rebased onto the
current default branch.
pytest tests/test_agent_loop_backend_dispatch.py tests/test_providers_factory.py tests/test_cli_agent_commands.py tests/test_memory_engine_face.py tests/test_cli_gateway_commands.py-- 171 passed, 0 failed.passed, 117 skipped, 0 failed. A local full run reports one failure,
tests/test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for,which was run again in a clean tree extracted from the default branch at the
commit this branch is based on and fails there with the same assertion and the
same value; the CI runner, which is not root, passes it. 0 introduced either way. The one failure is
tests/test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for.It was run again in a clean tree extracted from the default branch at the commit
this branch is based on, and fails there with the same assertion and the same
value. 0 introduced.
Diff coverage: 93.94% (31/33 executable changed lines),measured by the
coverage gatesjob on this head. The ratchet moved with it:line +1.81pp, branch +2.74pp. The remaining uncovered pair is
the same teardown inside
gateway_commands.run, a 634-line function whose harnesswould cost far more than the one in
agent_commands.make lint-python,make lint-imports,make lint-types-- all exit 0.make check-commits,make check-large-files,make check-source-language--all exit 0.
git range-diff: every pre-existing commit replays=, and none of the commits the base gained touches a file this branch touches, sothe changed-line set the coverage gate measures is unchanged.
Neither round's tests are theatre, and each was checked on its own:
missing function. Two mutants were then run against them -- moving the in-flight
read below the collect await reddens two of them, which is the constraint the
comment beside that read now names; moving it below
cancel()alone leaves themgreen, correctly, because
cancel()only schedules.wording changed, and the end-to-end test was watched failing on the log line.
Removing the
report_memory_write_outcomecall from theagentteardown turns thenew CLI test red on its stdout assertion; restoring it turns it green with
git diff HEADempty.One thing that would have gone unnoticed:
capsyscaptures nothing from loguru here,because the sink holds the real
sys.stderrfrom when the handler was added, so thefirst version of the end-to-end test passed while reading an empty string. It adds
its own sink now and asserts the sink saw something, so the log half cannot pass
unread.
No docs change: the only user-facing text is the shutdown notice itself, and no
document quotes it.
Risk
User-visible change is one line of CLI output at exit, and it now states an unknown
outcome rather than a good one. A user who reads it has nothing to redo -- re-sending
the turn could duplicate a write the service did complete -- so the line is
informational by design, not an alarm.
The exit path is otherwise untouched: the budget constant is byte-identical to the
one on the default branch, and the diff of that file contains no non-comment line, so
exit timing does not move.
drain_backend_storeschanges its return type, which is internal. Five callers:three render the notice and are updated here, two await it and ignore the value as
they already did.
Rollback is a revert of the five commits on this branch. Nothing persists and no
format changes. Reverting only the last two would restore the delivery claim while
keeping the split, which is the state review rejected, so revert all or none.
Checked and deliberately not changed:
diff. The test pinning the wording "still finishing in the background", whose
docstring stated the write had reached the service, is renamed and rewritten; its
two negative assertions were right and are kept, and a new sibling pins the other
direction.
final-flush sweep hit its 5.0s budgetlineto stderr at shutdown. It belongs to a different layer and is factually correct;
folding it into this notice would widen the diff into the plugin.
gateway_commandsstays uncovered. Its enclosing function is634 lines against 60 in
agent_commands, and the gate passes without it.StorePipelineappears inCONTEXT.mdonly as a passingmention with no entry of its own, and
DrainOutcomeis a module-local type ratherthan a concept the map tracks.
Related Issues
N/A