[Shivam Sharma (ShivamSharma43)]

Motivation & Context

With automatic consolidation, MemoryContextProvider keeps the IDs of sessions that finished since the last pass in sessions_since_consolidation and consolidates once there are consolidation_min_sessions of them. When a pass finished, it cleared that whole list, including sessions whose memories it never saw. Two orderings lose a session this way:

• after_run registered the session before extracting its memories. If another session's pass took its topic snapshot while this session was still extracting, the pass covered the ID but not the memories, then cleared the ID. This is the schedule in Python: [Bug]: Automatic memory consolidation clears sessions still extracting memories #9218: B is registered, its extraction of a New topic is still running, and the Old pass clears B, so C leaves only ["C"] pending and no consolidation is triggered.
• A session that registered after the snapshot, while the pass was still consolidating, was cleared when the pass finished.

The facts themselves are saved either way. What gets lost is the count, so these memories wait for unrelated sessions before they are consolidated.

Description & Review Guide

• What are the major changes? In _memory.py:
  • Per-turn pending markers. The maintenance state now also persists pending_turn_ids, which maps each pending session ID to a fresh uuid4 marker for its latest finished turn. after_run records a new marker on every turn, even when the session is already pending.
  • Snapshot-scoped retirement. _run_consolidation records the (session ID, turn marker) pairs it sees when it takes the topic snapshot. When the pass succeeds, it removes only the sessions whose marker is unchanged. A session that registered after the snapshot stays pending, and so does a session whose marker changed during the pass. The pass then keeps markers only for the sessions that remain. The empty-topics path clears both fields, as before.
  • Registration after extraction, in finally. after_run registers the session (and its turn marker) after _extract_memories and the index rebuild instead of before. A pass that sees a session as pending has therefore also seen the topics that session wrote. Registration sits in a finally, so a run that fails or is cancelled after writing a topic is still counted. The exception or cancellation then propagates unchanged.
  • State written by older code or custom stores. _normalize_state fills in missing or mistyped fields, including state saved before pending_turn_ids existed. MemoryFileStore.read_state and the provider's own reads use it, so custom MemoryStore implementations that return the old shape keep working. A session that is pending without a marker is retired by the next pass that sees it, which matches the previous behavior.
• What is the impact of these changes? Serial runs behave as before, apart from the extra pending_turn_ids field in the state file. The transient-failure path still leaves the window and the queue unchanged. In the interleaved schedules above, the late session or late turn stays pending and counts toward the next pass. Two behavior changes:
  • A run whose extraction raises after writing a topic, or is cancelled after writing one, is now registered. Before this PR, a failed run was also registered, because registration came first.
  • A run that fails before writing anything is registered too. This adds at most one pending entry, so it can bring a consolidation pass forward by one session. It never drops a session.
• What do you want reviewers to focus on?
  • The finally registration in after_run.
  • The marker comparison in _run_consolidation, which also handles overlapping turns of the same session. Agent.run doesn't serialize runs per session, so a second turn can register while a pass that already counts the session is running. That second turn now stays pending; it is no longer an uncovered case.
  • I chose markers over tracking an "extracting" state, because a marker only needs one extra persisted field and is backward compatible.

Tests in tests/core/test_harness_memory.py:

• test_automatic_consolidation_keeps_session_extracting_during_pass: the schedule from the issue, with real Agent.run calls on a gated client. B stays pending after the Old pass, and C then consolidates New together with Old.
• test_automatic_consolidation_keeps_session_extracting_before_snapshot: B starts extracting before another session's pass takes its snapshot. B stays pending, and the next pass consolidates B's fact.
• test_automatic_consolidation_keeps_overlapping_turn_of_covered_session_pending: a second session-1 turn writes a topic and registers while a pass that snapshotted session-1 is running. The turn stays pending.
• test_memory_consolidation_retires_only_turns_in_its_snapshot: a turn registers while the pass is consolidating. It covers three cases: a new session, a session pending without a marker (legacy state) and a session pending with a marker. The pass advances last_consolidated_at and leaves only the late turn pending.
• test_memory_session_stays_pending_when_extraction_fails_after_a_topic_write: a run fails on the second topic write or on the index rebuild, after one topic was persisted. The error propagates and the session is pending.
• test_memory_session_stays_pending_when_extraction_is_cancelled_after_a_topic_write: the same check, for a run cancelled after one topic was written.
• test_memory_provider_accepts_custom_store_state_without_turn_ids and test_memory_consolidation_accepts_custom_store_state_without_turn_ids: a custom store that returns state without pending_turn_ids still registers sessions and retires them.

With _memory.py from current main and this test file, the race tests fail: the during-pass, before-snapshot and overlapping-turn tests, all three cases of the retire test, and the cancellation test. The tests for failures after a topic write and for custom store state pass on main too, because main registers before extracting and has no markers. They guard against regressions in the new ordering.

Checks run locally on Windows with Python 3.14, on head c92fc7d:

• test_harness_memory.py, test_sessions.py and test_harness_background_agents.py: 277 passed, 1 skipped. I ran them with a short --basetemp, because one test_sessions.py case uses a filename longer than Windows' path limit under the default temp directory. That case is unrelated to this PR.
• ruff format / ruff check: clean.
• mypy and pyright on _memory.py: no new diagnostics compared with main, re-run on this head. Earlier heads were also checked with pyright, pyrefly, ty and zuban on the test file, with no new diagnostics.

Related Issue

Fixes #9218

The issue is reproduced by the triage workflow and has no other open PR. It doesn't have a maintainer comment agreeing with the direction yet, so I've left that checklist item unchecked. I'm happy to adjust the approach if you'd prefer a different one.

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: analysis of the race, implementation, tests, and this PR description.

Contribution Checklist

• The code builds clean without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
• This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Eduard van Valkenburg (eavanvalkenburg)]

/review

[github-actions]

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): 4a53aa064987
Model: gpt-5.6-sol

Overview

The change closes two distinct-session races by registering sessions only after extraction and retiring only IDs covered by a consolidation snapshot. Owner-scoped state locks, per-topic locks, and transient-failure retention provide strong concurrency guardrails, and the new tests exercise the intended interleavings. However, delaying registration until after all extraction and index work creates a failure window where persisted memories can lose their consolidation count.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/core/agent_framework/_harness/_memory.py

[Eduard van Valkenburg (eavanvalkenburg)]

Shivam Sharma (@ShivamSharma43) One parser regression remains in python/packages/core/agent_framework/_harness/_memory.py: MemoryTopicRecord.from_markdown() now interprets Updated: and Sessions: as record metadata even inside ## Summary. A persisted summary line such as Sessions: keep the troubleshooting context. is removed from the summary and can overwrite session_ids. The base behavior limited those labels to the pre-section metadata area. Please restore the current_section is None guard and add round-trip coverage for metadata-looking summary lines.

[Shivam Sharma (ShivamSharma43)]

Eduard van Valkenburg (@eavanvalkenburg) thanks. That regression came from the branch predating #9079, not from a change here: this PR never touched from_markdown, so its diff against current main showed the current_section is None guard being removed. Merged main in 0b7c363, which brings the guard back unchanged, along with #9079's round-trip coverage: test_memory_file_store_preserves_metadata_like_summary_text (covering Sessions: keep the troubleshooting context.) and the parametrized Updated:/Sessions: summary-line parse test. test_harness_memory.py: 42 passed on the merged head; ruff clean.

[Eduard van Valkenburg (eavanvalkenburg)]

Shivam Sharma (@ShivamSharma43) The implementation concerns raised on this head look substantively addressed, but before approval we need to define the intended scope and design for cross-agent file-based memory—particularly ownership, isolation, and concurrency semantics when multiple agents share file-backed memory. I requested a plan on #9218 so we can understand and agree on that behavior first. Please hold this PR while that design is clarified.

[Shivam Sharma (ShivamSharma43)]

Thanks, holding this until the plan on #9218 settles. For reference, the PR doesn't change who owns the memory or how it is isolated. It only changes which pending session IDs a successful pass clears from the shared maintenance state: the IDs it saw when it took its topic snapshot. It also registers a session after its extraction rather than before. Once the design is agreed, I'll rework the PR to match.

[Eduard van Valkenburg (eavanvalkenburg)]

Shivam Sharma (@ShivamSharma43) The current-head review is clean under the accepted narrow scope. Before approval, please update the Description & Review Guide to reflect the implemented persisted pending_turn_ids markers, failure/cancellation registration in finally, and same-session overlap handling; those are no longer uncovered limitations.

[Shivam Sharma (ShivamSharma43)]

Eduard van Valkenburg (@eavanvalkenburg) updated the Description & Review Guide for head c92fc7d. It now covers the persisted pending_turn_ids markers and snapshot-scoped retirement, registration in finally on failure or cancellation, same-session overlap (now handled, no longer listed as uncovered), and _normalize_state for older or custom-store state. It also lists every new test, which of them fail against main's _memory.py, and refreshed local results (277 passed, 1 skipped).