[Bug]: MCP auto-watch can close GraphStore while its watcher is still active #1121

code-review-graph version

2.3.9

Operating system

macOS

Python version

3.11.16

AI platform

claude-code

Output of code-review-graph status

Nodes: 4
Edges: 4
Files: 2
Languages: python
Last updated: 2026-10-18T19:57:24
Built on branch: master
Built at commit: c8e9fa27671e

Steps to reproduce

1. Check out staging (or PR fix(mcp): run post-processing during auto-watch updates #1108's head) and set up the dev environment: uv sync --extra dev.
2. Save the attached test_mcp_watch_store_shutdown_repro.py under tests/.
3. Run:

   uv run pytest -p no:cacheprovider -q tests/test_mcp_watch_store_shutdown_repro.py

The test drives the real main(auto_watch=True), start_watch_thread(), watch(), the watch handler, batch processor
and debouncer thread. Only the boundaries are replaced: GraphStore (an instrumented object that counts active users,
records close(), and owns no connection; sqlite3.connect is patched to fail), incremental_update /
run_post_processing (they hold the store at an explicit gate), the watchdog Observer (in-memory), and mcp.run
(it returns once a watcher thread is confirmed inside the held operation). The ordering is forced with threading.Event
gates, not sleeps. Every thread is released and joined in teardown.

Minimal reproduction: See the attached repro_mcp_watch_store_shutdown.zip.

It contains a standalone pytest regression test, execution instructions, and recorded RED results against both staging and PR #1108. A local stop → join → close prototype passes the same four scenarios.

The test uses an instrumented store and deterministic synchronization; it does not open or close a real SQLite connection.

repro_mcp_watch_store_shutdown.zip

Expected vs actual behavior

Expected: when the transport returns, main() signals the watcher to stop, waits until the watcher thread and its
debouncer have exited (so no store operation is in progress), and only then calls GraphStore.close(). The four cases
pass.

Actual: main() closes the store immediately, while a watcher thread is still inside an operation on it, and
returns with the watcher still running.

On PR #1108's head, all four cases fail (three identical runs, Python 3.11.16; once more on 3.13.12):

FAILED ...::test_mcp_waits_for_watcher_before_closing_store[startup_reconciliation]
FAILED ...::test_mcp_waits_for_watcher_before_closing_store[startup_post_processing]
FAILED ...::test_mcp_waits_for_watcher_before_closing_store[event_batch]
FAILED ...::test_mcp_waits_for_watcher_before_closing_store[batch_post_processing]
4 failed in 0.63s

AssertionError: MCP closed GraphStore while its background watcher was still active:
close() saw {'active': 1, 'watcher_threads_alive': ['crg-watch', 'crg-watch-debouncer']}
during startup_reconciliation (held on thread crg-watch);
order=['mcp_transport_started', 'watcher_entered_store_operation', 'mcp_transport_returned',
       'graphstore_close_called', 'main_returned']

On staging d72e8fbf: startup_reconciliation and event_batch fail the same way. The two post-processing cases are
skipped because MCP auto-watch runs no post-processing before #1108.

Additional context

Platform: not client-specific. It affects any MCP client using code-review-graph serve --auto-watch (or mcp --auto-watch). Found while working on #1103 / #1108, by code inspection and the test above.

Root cause. main() owns the store but not the thread lifetime:

• It creates the store and starts the watcher, keeping the thread handle only for an is None check:
  main.py#L1490-L1496 (staging)
  · main.py#L1462-L1472 (#1108 head)
• It closes the store unconditionally once the transport returns:
  main.py#L1533-L1536 (staging)
  · main.py#L1509-L1512 (#1108 head)
• start_watch_thread() has no stop parameter and calls watch() without stop_event, so the watch loop never ends on
  request:
  incremental.py#L3337-L3367 (staging)
  · incremental.py#L3401-L3436 (#1108 head)
• watch() already supports stop_event, and on exit it stops the observer and joins the debouncer
  (#L3239-L3244,
  #L3323-L3328,
  #L3393-L3397).
  The MCP path just never uses it.
• GraphStore.close() is a bare self._conn.close()
  (graph.py#L792-L793).

The store is used from two watcher threads: crg-watch (startup reconciliation, then post-processing) and the debouncer
thread (each event batch, then post-processing). The processor lock added in #1108 serialises those two against each
other. main() and close() never take it, so the lock does not protect the store's lifetime. This is a use-after-close
ordering problem, not a concurrent-write problem.

Impact. What was demonstrated is the invariant violation: close() runs while a watcher operation is in progress, and
main() returns before the watcher stops. Possible consequences, not observed by this test (the store is a fake):
the watcher's next statement on a closed connection fails (for example sqlite3.ProgrammingError), a batch or its
FTS/flow/community post-processing near disconnect is lost or left partial, and noisy or unclean shutdown. Closing a
SQLite connection while another thread is executing on it is unsafe in general. I'm not claiming a crash or data
corruption from MCP shutdown. The defect predates #1108, but #1108 also runs post-processing on the watcher threads,
which likely widens the window (not measured).

Suggested repair contract (not implemented here):

1. Give start_watch_thread() a stop signal (for example an optional stop_event, forwarded to watch()), keeping its
   current positional/keyword API.
2. In main()'s finally: set the stop signal, then join the watcher thread. watch() already joins its debouncer
   before returning.
3. Close the store only after the join has confirmed the thread exited.
4. Decide what happens when the watcher does not exit within a deadline (startup reconciliation is not interruptible).
   For example: keep waiting and log, rather than closing under a live thread.
5. Keep the existing behaviour when watchdog is unavailable or the thread fails to start, and keep watcher-failure
   logging.

As a sensitivity check, a local prototype of that contract (stop event, join, then close) makes all four cases pass. The
prototype is not part of #1108.

Regression tests worth having: transport returning (and transport raising) during (a) startup reconciliation, (b)
startup post-processing, (c) an event batch, (d) batch post-processing. Each should assert that close() happens
exactly once, after the watcher and debouncer have exited, plus one case for a watcher slower than the join timeout. The
attached test covers (a)–(d).

Relation to #1103 / #1108. Separate, pre-existing defect discovered during that work. #1108 does not fix it and is
not intended to. Keeping it out of that PR keeps the FTS correction focused.

Attachment: repro_mcp_watch_store_shutdown.zip, containing the test, a README, and the RED logs from PR #1108's
head, from staging, and from the prototype comparison.

repro_mcp_watch_store_shutdown.zip