[Yufeng He (he-yufeng)]

Summary

MemoryFileStore.read_state parsed state.json with no guard, so a corrupt state file (interrupted editor, partial external write, hand edit) raised JSONDecodeError out of the per-turn memory bookkeeping path and kept raising on every later turn, since nothing ever removed the bad file.

This follows the session store's existing corrupt-snapshot convention (_quarantine_corrupt_snapshot in _sessions.py): an unreadable state file is moved aside to .<name>.<uuid>.corrupt, a warning is logged, and the store restarts from a clean default state. The next write_state recreates a clean file. The deliberate ValueError for a well-formed non-object document is unchanged.

Tests

• New test_memory_file_store_quarantines_corrupt_state_file: writes state through the real path, corrupts the file, asserts a clean default comes back, the corrupt file was moved aside with its content preserved, and a later write/read round-trips normally.
• New test_memory_file_store_quarantines_non_utf8_state_file: writes non-UTF-8 bytes, asserts the same clean quarantine behavior, and verifies the original bytes are preserved. It failed with UnicodeDecodeError before the byte-wise read/decode change.
• Mutation check: dropping the guard fails the test (JSONDecodeError propagates); restored green.
• pytest tests/core/test_harness_memory.py: 36 passed.
• ruff format and ruff check on the changed files: passed.
• pyright agent_framework/_harness/_memory.py and the test file: 0 errors.
• Verified against the worktree checkout (PYTHONPATH pinning confirmed by agent_framework.__file__), not the shared editable install.

Concurrency boundary

File-backed maintenance state is single-process/single-writer. Quarantine recovery is best-effort when another process or an external editor replaces the file concurrently. Cross-process or independently hosted shared-memory scenarios should use a service-backed MemoryStore rather than sharing these files.

Related Issue

Refs #9997. The tracking issue requests maintainer agreement on the quarantine-versus-fail-closed recovery policy before further implementation. It is not yet labelled ready-for-implementation.

AI Assistance

• No material AI assistance was used.
• This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details: implementation, regression tests, and this PR description. The author reviewed the final diff and ran the targeted memory tests, Ruff, and Pyright checks.

[Copilot]

🟡 Changes recommended

Quarantine can move aside a concurrently repaired file and reset valid maintenance metadata.

1 open issue

• Avoid quarantining valid state replacements during concurrent repair · New

What changed in this PR

Improves Python memory-store recovery so corrupt state files do not repeatedly fail per-turn bookkeeping.

Changes:

• Quarantines unreadable state files, logs a warning, and returns defaults.
• Adds regression coverage for quarantine, content preservation, and subsequent writes.

File	Description
python/​packages/​core/​tests/​core/​test_harness_memory.py	Tests corrupt-state recovery and later write/read behavior.
python/​packages/​core/​agent_framework/​_harness/​_memory.py	Adds quarantine and default-state recovery.

_{Review effort: Balanced}

[Yufeng He (he-yufeng)]

You raised this first and I only answered the restatement, so let me close it here too.

Addressed in 06edcafb9, "only quarantine a memory state file that is still the failing payload". It does what you asked and takes the shape from _quarantine_corrupt_snapshot as you suggested: read_state keeps the text that failed to parse, re-reads the path before renaming, and only quarantines when the content is byte-identical to the payload that failed. If the content moved, it retries with the new text instead of moving it aside. If the file is gone it returns defaults without quarantining anything.

The regression you asked for is there: test_memory_file_store_does_not_quarantine_a_valid_replacement. A stubbed loader writes the valid replacement mid-flight on the first, failing parse, and the test asserts the repaired sessions_since_consolidation survives, the file is left in place, and no .corrupt sibling appears.

On the last part of your comment, coordinating with store writes: I did not add a lock around the check-and-rename. The read-verify-rename sequence is the same one _quarantine_corrupt_snapshot relies on, and it closes the case you described. What is still theoretically open is two readers both seeing the same corrupt payload and both attempting the rename. The loser gets OSError, warns, and falls back to defaults, so it cannot move a valid file aside twice or lose a quarantine, but it also cannot see the winner's quarantine. If you want that last case closed properly it needs the write side to take the same lock, which is a bigger change than this PR and I would rather not fold it in here.

Happy to open that as a follow-up if you think the double-reader window is worth closing now.

[Somay kousis (Somay-kousis)]

Reviewed at 68bc218 (merge base 08851e6).

What I ran

• New test on the PR head: passes. With _harness/_memory.py reverted to the merge base: fails with JSONDecodeError. So the test catches the bug.
• tests/core/test_harness_memory.py on head: 34 passed. tests/core on head: 6788 passed, 133 skipped.
• End to end: two agent.run turns with a MemoryContextProvider and a truncated state.json, using the test file's _MemoryHarnessClient. Merge base: both turns raise JSONDecodeError and the bad file stays. PR head: both turns succeed, one .corrupt file is created, and state.json is rewritten clean. The bug and the fix both hold up beyond the unit test.
• Non-UTF-8 bytes in state.json: the merge base raises UnicodeDecodeError. The PR quarantines the file, since UnicodeDecodeError is a ValueError. Good, but untested.
• ruff check 0.17.0: clean. ruff format --check 0.17.0: fails, see 3.

1. A valid file written between the failed parse and the rename gets quarantined (worth considering)

This confirms the bot comment on _memory.py:905 with a repro. I used a custom loads that writes a valid state through write_state before parsing the corrupt text, to simulate another writer winning the race. On the PR head:

read: returned {'last_consolidated_at': None, 'sessions_since_consolidation': []}
state.json exists after read: False
quarantined file content: {"sessions_since_consolidation": ["written-by-other-writer"]}

The valid state ends up in the .corrupt file and later reads return defaults. In one process the per-owner asyncio.Lock (_state_async_lock) serializes read_state/write_state, so this needs a second process or event loop on the same directory. _sessions.py:_quarantine_corrupt_snapshot, which the PR says it mirrors, re-reads the file and skips the move if the bytes changed. Doing the same here (keep the text you failed to parse, compare before rename, re-read if it changed) would close most of the window.

2. A state file that is valid JSON but not an object still fails every turn (worth considering)

_memory.py:923 still raises ValueError("Memory state file must contain a JSON object."). With state.json set to [], both reads raise on the PR head, the same "fails every later turn" problem the PR fixes for unparseable files. The description says this is on purpose. A hand edit to [] or null is a realistic way to hit it though, and recovery would be the same quarantine call. If it stays a hard error, a test pinning that behaviour would make the choice explicit.

3. ruff format --check fails on the new warning (nit)

Run from python/ with the repo config (line-length 120), ruff@0.17.0 format --check wants the two-part string at _memory.py:908-909 joined onto one line. The diff is in the evidence. The Python CI checks have not run on this PR yet, so this would likely show up there.

Not checked

Real multi-process access (finding 1 is a simulation), Windows rename semantics (the target name is a fresh uuid, so I don't expect a clash), custom loads callables that raise something other than ValueError, and pyright/mypy.

[Eduard van Valkenburg (eavanvalkenburg)]

One quarantine race should be addressed before merge.

[Eduard van Valkenburg (eavanvalkenburg)]

Please retain the exact text that failed to parse and verify the path still contains that payload before renaming it. Another process can atomically replace state.json with a valid file after the read but before this rename; this code then quarantines the valid replacement and returns defaults, losing current maintenance metadata. _sessions.py::_quarantine_corrupt_snapshot already re-reads and skips quarantine when the contents changed. Please mirror that behavior, retry the read when a replacement wins, and add an interleaving regression test.

[Yufeng He (he-yufeng)]

Good catch — that race was real. Fixed in 06edcaf: read_state now keeps the exact text that failed to parse, and before quarantining it re-reads the file and compares — the same shape as _sessions.py::_quarantine_corrupt_snapshot. If the content differs (a valid replacement landed), it re-parses the new payload instead of moving it aside; if the file is gone entirely, it returns the default state without touching anything. Added test_memory_file_store_does_not_quarantine_a_valid_replacement, which drives the interleave through a scripted loads (replace the file mid-parse, raise once): it fails against the pre-fix code and passes now. test_harness_memory.py is 35/35, and the memory/sessions regression set (test_harness_memory, test_harness_file_memory, test_in_memory, test_sessions) is green.

[Yufeng He (he-yufeng)]

Looked at the Public API Compatibility failure: it is not this diff.

The job crashes while analyzing the base tree, before it ever reaches the PR side. The traceback ends in griffe's AliasResolutionError — "Could not resolve alias agent_framework._workflows._edge.EdgeGroup.builtin_type pointing at builtins.type (in python/packages/core/agent_framework/_workflows/_edge.py:315)". That line is the class-body from builtins import type as builtin_type alias used as builtin_type[EdgeGroup], which griffe does not resolve. This PR only touches _harness/_memory.py and its tests, so the failure comes from the tooling meeting that alias on main, and it will hit any PR that triggers the comparison until griffe handles the pattern or _edge.py spells the annotation differently.

Everything else on this head is green. A re-run will clear it the same way it did for the earlier checks, or the alias can be rewritten as type[EdgeGroup] in a separate change if the maintainers want the gate healthy again.

[Somay kousis (Somay-kousis)]

Re-reviewed at f0e42a7 (fix in 06edcaf). The race is closed: with a valid state written between the failed parse and the rename, the read now returns that state and state.json stays in place (my earlier repro case 4). ruff format --check is clean now and test_harness_memory.py passes (35). One regression from the restructure.

A non-UTF-8 state.json fails every turn again

state_path.read_text(encoding="utf-8") moved out of the try (_memory.py:900), so a UnicodeDecodeError escapes before the quarantine branch. At 68bc218 it was inside the try and got quarantined, since UnicodeDecodeError is a ValueError. Same repro as before (state.json = b'\xff\xfe garbage'):

68bc218: read returned the default state, 1 file quarantined
f0e42a7: RAISED UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0, 0 files quarantined

So a binary or mis-encoded file now breaks every later turn, the problem this PR fixes for unparseable JSON. The re-read at :911-912 has the same gap: it only catches OSError, so a file replaced by non-UTF-8 bytes mid-read would raise there too.

Reading bytes and comparing bytes would cover both, the way _sessions.py:_quarantine_corrupt_snapshot does (read_bytes() at :2190): keep failing = state_path.read_bytes(), decode and loads inside the try, and compare state_path.read_bytes() against it before the rename. A test with non-UTF-8 bytes would pin it; none exists yet.

Not rechecked: real multi-process access (still a simulation) and Windows rename semantics.

[Yufeng He (he-yufeng)]

Somay kousis (@Somay-kousis) Thanks for catching the regression from the race fix. 85c2fd256 now reads bytes for both the initial load and the concurrent-replacement check. UTF-8 decoding and loads are inside the same ValueError handler, and the quarantine decision compares the original and current bytes before moving anything.

I added test_memory_file_store_quarantines_non_utf8_state_file, which writes b"\xff\xfe garbage", verifies recovery to the default state, and checks that the original bytes are preserved in exactly one .corrupt file. It fails with UnicodeDecodeError on the previous implementation and passes now.

The complete test_harness_memory.py file is 36 passed, Ruff format/check pass, and pyright reports 0 errors and 0 warnings. The checks currently reported on this head are green as well.

[Somay kousis (Somay-kousis)]

Reran on 85c2fd2: state.json = b'\xff\xfe garbage' now returns the default state with one .corrupt file, and a valid state written between the failed parse and the rename still survives. Thanks.

[Eduard van Valkenburg (eavanvalkenburg)]

Yufeng He (@he-yufeng) The body explains the recovery change and validation, but it does not provide the required Related Issue answer or an explicit, justified trivial-change exception. Please link the agreed tracking issue, or state N/A - trivial change with the reason this qualifies as an obvious correction whose desired behavior is not in dispute. Equivalent prose is fine; this is about the missing answer, not the headings or AI-assistance disclosure.

[Yufeng He (he-yufeng)]

Added Refs #9997 to the Related Issue section. The issue includes a fresh two-read reproduction on current main, the non-UTF-8 case, and the recovery-policy alternatives. I have not represented it as agreed or ready-for-implementation; further implementation is paused until the quarantine-versus-fail-closed direction is confirmed.

[Eduard van Valkenburg (eavanvalkenburg)]

Yufeng He (@he-yufeng) The remaining race is limited to writers that do not share the same in-process store/loop lock: separate store instances or processes pointed at the same files, or external repair of state.json. Normal concurrent turns using one shared MemoryContextProvider and MemoryStore instance are serialized. For this v1, please document file-backed memory maintenance state as single-process/single-writer, note that quarantine recovery is best-effort under external concurrent replacement, and direct cross-process or independently hosted multi-agent shared-memory scenarios to a service-backed MemoryStore. With that boundary explicit, we can defer cross-process coordination.

[Yufeng He (he-yufeng)]

Eduard van Valkenburg (@eavanvalkenburg) Documented in faef9b618, both in the read_state contract and the PR description: file-backed maintenance state is single-process/single-writer, quarantine is best-effort under external concurrent replacement, and cross-process or independently hosted shared-memory scenarios should use a service-backed MemoryStore instead of sharing these files.

The focused test_harness_memory.py run remains green at 36 tests. Ruff check and formatting pass on the changed file.