[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:
  • _run_consolidation records the pending session IDs it sees when it takes the topic snapshot. When the pass succeeds, it removes only those IDs and keeps any added since.
  • after_run registers the session 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.
• What is the impact of these changes? Serial runs behave as before, and the transient-failure path still keeps the window and the queue unchanged. In the interleaved schedules above, the late session stays pending and counts toward the next pass. One side effect of the reordering: if extraction raises a non-transient error (which after_run already propagates), the session is no longer registered. Consolidation was already skipped in that case.
• What do you want reviewers to focus on? The reordering in after_run. I chose it over tracking an "extracting" state because it needs no change to the persisted maintenance state. One case is not covered: the same session ID finishing a second turn while a pass that already counts it is running. Its ID is already in the list, so the pass still retires it.

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_memory_consolidation_retires_only_sessions_in_its_snapshot: a session registers while the pass is consolidating. The pass advances last_consolidated_at and leaves only that session pending.

All three fail on main. The second fails if only the snapshot change is applied, and the third fails if only the reordering is applied.

Checks run locally on Windows with Python 3.13:

• test_harness_memory.py, test_sessions.py and test_harness_background_agents.py: 263 passed, 1 skipped.
• ruff format / ruff check: clean.
• mypy and pyright on _memory.py, and pyright, pyrefly, ty and zuban on the test file: no new diagnostics compared with main.

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

[github-actions]

If extraction persists one topic and then a later item write or rebuild_index() fails, this delayed registration is never reached, so the durable memory loses its pending-session count until unrelated sessions accumulate. Please ensure a run that may have written topics records its pending marker before propagating post-write failures or cancellation, without making the marker visible while extraction is still active.

[Shivam Sharma (ShivamSharma43)]

Agreed, fixed in 480eea4. Registration now runs in a finally around extraction and the index rebuild. It still happens only once extraction is over, so the session is never pending while its extraction is running, and the #9218 ordering holds. But a run that wrote a topic and then failed on a later item write or rebuild_index(), or was cancelled, now records its pending marker before the error propagates. This matches the behavior before this PR, where the session was registered whatever the extraction outcome.

New tests, each with a two-topic extraction where the first topic is written:

• test_memory_session_stays_pending_when_extraction_fails_after_a_topic_write[second_topic_write]: the second write_topic raises.
• test_memory_session_stays_pending_when_extraction_fails_after_a_topic_write[rebuild_index]: the post-extraction rebuild_index raises.
• test_memory_session_stays_pending_when_extraction_is_cancelled_after_a_topic_write: checks the session is not pending mid-extraction, then cancels during the second merge.

All three fail on 4a53aa0 (pending list is empty) and pass now. The three #9218 tests still pass. Memory, sessions and background-agents tests: 264 passed, 1 skipped, 1 failure that also fails on main locally (test_opaque_session_id_round_trips_through_safe_filename with the long id, Windows path length).

[Eduard van Valkenburg (eavanvalkenburg)]

Shivam Sharma (@ShivamSharma43) this still uses a de-duplicated session.session_id as the only pending-work marker. If a consolidation pass snapshots covered_session_ids while session-1 is already pending, then a second overlapping agent.run(..., session=AgentSession(session_id="session-1")) can finish extraction and reach this block while session-1 is still in the list, so line 1435 records no new marker. When the older pass succeeds, lines 1599-1603 remove session-1 because it was in the snapshot, leaving the later turn’s newly written topic out of the pending window even though it was not in that snapshot. Agent.run does not serialize callers per AgentSession, so the marker needs to distinguish completions rather than just membership by session id.

[Shivam Sharma (ShivamSharma43)]

You're right, thanks. Fixed in 069de84. The marker now changes with every finished turn, so it no longer depends on the session id alone.

• Every finished turn stores a new uuid4 id for its session in a new state field, pending_turn_ids ({session_id: id of the latest finished turn}). This happens in the same finally block, under the state lock, even when the session is already pending.
• A pass records each pending session's id when it takes its snapshot. When the pass succeeds, it retires a session only if that id hasn't changed. In your interleaving, the second session-1 turn replaces the id, so the older pass leaves session-1 pending. Ids are never reused, so this also holds when two passes overlap and the other one retires and re-adds the session in between.
• sessions_since_consolidation keeps its format: a de-duplicated list of session ids. consolidation_min_sessions still counts distinct sessions, matching its docstring ("Number of sessions required") and the de-duplication the provider already did. Overlapping turns of one session don't move the threshold forward any faster.
• Existing state still loads. read_state fills in pending_turn_ids as {} when it's missing. A session that's pending without an id is retired by a pass that saw it, unless a newer turn has recorded an id by the time the pass finishes.

Tests:

• test_automatic_consolidation_keeps_overlapping_turn_of_covered_session_pending follows your interleaving. Two overlapping agent.run(..., session=AgentSession(session_id="session-1")) calls run through the real automatic passes. The first pass, which snapshotted session-1, finishes after the second turn wrote and registered its topic, and session-1 stays pending. The second pass then covers the new topic and retires it. On 480eea4 the pending list ends up [] at that point.
• test_memory_consolidation_retires_only_turns_in_its_snapshot is now parametrized. A turn registered during the pass is kept in each case: for a new session, for a pending session without an id (older state), and for a pending session with an id.

Memory, file-memory, sessions and background-agents tests: 302 passed, 1 skipped. The 1 failure is the Windows path-length case (test_opaque_session_id_round_trips_through_safe_filename), which also fails on main. Ruff, pyright (source) and mypy, ty, pyrefly (tests) are clean on the changed files.

[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.