Skip to content

Python: serialize todo mutations across providers sharing a store - #9920

Open
Lushiyao5 wants to merge 2 commits into
microsoft:mainfrom
Lushiyao5:fix/todo-clean
Open

Lushiyao5 wants to merge 2 commits into
microsoft:mainfrom
Lushiyao5:fix/todo-clean

Conversation

@Lushiyao5

Copy link
Copy Markdown

Motivation & Context

TodoProvider serializes its read-modify-write todo operations with a lock owned by the provider instance and keyed only by session. Two agents that each use their own provider over one shared TodoStore take different locks: both load the same state and next_id, and the last save silently drops the other write while both tool calls report success for the same item id. A session reused across asyncio.run boundaries can also raise from a lock bound to the previous event loop.

Description & Review Guide

  • What are the major changes? The lock is now looked up from a process-wide registry keyed by (id(store), running loop, id(session)) and held weakly, so providers sharing a store serialize with each other, unrelated sessions of one store still mutate concurrently, and a session is safe across event loops. Values live only while a mutation holds or awaits the lock. TodoProvider no longer owns a WeakKeyDictionary of locks.
  • What is the impact of these changes? Concurrent mutations over one shared store now behave like mutations from a single provider (distinct ids, no dropped items). Coordination remains process-local; other processes and other stores over the same backing storage are not serialized.
  • What do you want reviewers to focus on? The lock key (ids rather than objects, so an unhashable store still works and the map cannot keep objects alive) and the per-loop scoping, which mirrors the file-access layer's existing per-loop lock keys.

Related Issue

Fixes #9868

TodoProvider kept its read-modify-write lock on the provider instance, so two
providers over one store loaded the same state and next_id and one write was
silently dropped while both calls reported success. The per-session lock was
also not keyed by the event loop, so a session reused across asyncio.run
boundaries could raise a loop-bound-lock RuntimeError.

Move the lock to a module-level WeakValueDictionary keyed by store identity,
session identity, and the running loop, mirroring the file-access store lock.
Unrelated stores and sessions keep separate locks and are not serialized.

Fixes microsoft#9868

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Outstanding concurrency defects allow shared-file mutations to bypass locking or outlive it after cancellation, causing duplicate IDs and lost updates.

3 open issues
What changed in this PR

This PR addresses #9868 by moving Python todo mutation locks from individual providers to a shared registry, coordinating providers that use the same store and session object within an event loop.

Changes:

  • Keys locks by store identity, running event loop, and session identity.
  • Holds locks weakly so unused entries can be released.
  • Adds regression coverage for shared-store mutations, lock cleanup, and event-loop isolation.
File Description
python/​packages/​core/​tests/​core/​test_harness_todo.py Tests shared-provider serialization, lock lifetime, and store/loop scoping.
python/​packages/​core/​agent_framework/​_harness/​_todo.py Replaces provider-owned locks with a shared weak-value lock registry.

Review effort: Balanced

The lock is process-local: other processes, and other stores over the same backing
storage, are not coordinated.
"""
key = (id(store), asyncio.get_running_loop(), id(session))
The lock is process-local: other processes, and other stores over the same backing
storage, are not coordinated.
"""
key = (id(store), asyncio.get_running_loop(), id(session))
self._mutation_locks[session] = lock
return lock
"""Return the store-scoped lock for read-modify-write todo operations."""
return _todo_mutation_lock(self.store, session)
@Lushiyao5

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

… cancellation

- Lock keys now come from the store's mutation identity instead of object
  ids, so two sessions resolving to the same todo file (or two stores over
  one directory) share a lock instead of overwriting each other.
- Saves run under a shield, so a cancelled mutation keeps the lock until
  its in-flight write finishes and the cancellation is re-raised after.
@Lushiyao5

Copy link
Copy Markdown
Author

Addressed the Copilot review comments (two fixed, one remains a documented limitation):

  1. Lock by shared store state identity — fixed. Lock keys now come from a new TodoStore._mutation_identity(session, source_id) hook instead of id(store)/id(session). The default identity is the session object (in-memory stores keep state on the session itself); TodoFileStore overrides it with the resolved state file path, so two sessions with the same session ID (and owner scope) — or two store instances over the same directory — now share one lock. As a side effect, two TodoSessionStore instances over one session object also share a lock, which is correct since both write session.state. Covered by new tests test_todo_mutation_lock_shares_one_file_identity_for_equivalent_sessions and test_todo_providers_serialize_equivalent_sessions_over_one_file (all four new/updated tests fail without the fix).
  2. Hold the mutation lock until cancelled writes finish — fixed. Saves inside the locked section now run through _save_state_holding_lock, which shields the save: on cancellation the lock is held until the in-flight write completes, then the cancellation is re-raised. Covered by test_todo_mutation_keeps_lock_until_cancelled_write_finishes.
  3. Coordinate mutations across event loops — not addressed; the lock remains per-loop and process-local by design (documented in the _todo_mutation_lock docstring). Cross-loop coordination would need a blocking primitive across loops, which changes the concurrency contract more than this fix intends; happy to follow the maintainers' direction here.

All 25 tests in test_harness_todo.py pass (40 passed together with test_harness_file_memory.py).

This branch was previously deployed

1 inactive deployment
github-app-auth — 9bec05da Deployed Nov 1, 2026 by Lushiyao5 via add_label #26773
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: todo mutations race when two providers share one todo store

2 participants