Skip to content

fix(mcp): keep ADR reads nonblocking during reindex - #892

Open
SS-42 wants to merge 1 commit into
DeusData:mainfrom
SS-42:fix/adr-cross-process-lock
Open

fix(mcp): keep ADR reads nonblocking during reindex#892
SS-42 wants to merge 1 commit into
DeusData:mainfrom
SS-42:fix/adr-cross-process-lock

Conversation

@SS-42

@SS-42 SS-42 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep manage_adr(get|sections) on query-only store access while a same-project reindex holds the mutation lease
  • preserve serialized manage_adr(update|store) writes through the existing native project-lock manager
  • return legacy-file ADR content immediately; migration retries only after a nonblocking lease and rechecks the current SQLite generation before writing

Environment observed

  • macOS arm64
  • multiple independent MCP/CLI processes sharing one CBM_CACHE_DIR

Regression coverage

  • stored ADR get and sections bypass a busy blocking mutation guard
  • update remains serialized through the primary mutation guard
  • a busy legacy migration returns readable content without blocking; a later successful try-acquire persists it
  • corrupt-store recovery uses the nonblocking path for ADR reads

Verification

  • git diff --check
  • bash scripts/lint.sh --ci
  • bash scripts/test.sh (6788 passed, 0 failed, 4 skipped across 121 suites)

@SS-42
SS-42 force-pushed the fix/adr-cross-process-lock branch 4 times, most recently from 42b1878 to c52e266 Compare July 5, 2026 23:20
@SS-42
SS-42 marked this pull request as ready for review July 6, 2026 00:47
@SS-42
SS-42 requested a review from DeusData as a code owner July 6, 2026 00:47
@SS-42
SS-42 force-pushed the fix/adr-cross-process-lock branch from c52e266 to ba39ffd Compare July 6, 2026 13:53
@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 7, 2026
@DeusData

DeusData commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Thanks for the process-serialization work. Triage: high-priority stability bug.

Review will focus on lock scope and failure behavior: same-project serialization without blocking unrelated projects, no watcher/self-lock deadlocks, safe stale-store reopen behavior, and ADR migration/write ordering across independent MCP/CLI processes.

@DeusData DeusData added this to the 0.9.1-rc milestone Jul 8, 2026
@DeusData

DeusData commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Thanks — this addresses a real gap (independent MCP/CLI processes sharing one CBM_CACHE_DIR had no cross-process serialization), and the lock hygiene is right: 0600 lock files under the cache dir, FD_CLOEXEC, unlock-before-close, per-project scoping so unrelated indexes don't serialize, and project names are sanitized upstream so the lock filename is traversal-safe. The fork-based cross-process tests are a nice touch. Two changes requested before merge:

  1. Vacuous pass in test setup (tests/test_mcp.c, the new lock tests): if (!cbm_mkdtemp(cache)) { PASS(); } turns a setup failure into a green test. Our suite treats that as a broken guard — a test that cannot establish its preconditions has FAILED, not passed (see the FAIL() macro doc in test_framework.h). Please use FAIL("mkdtemp failed") there (and in any sibling occurrences in the new tests).

  2. Windows is a silent no-opcbm_pipeline_try_lock_project returns true unconditionally under _WIN32, so Windows keeps today's cross-process races. That's a fine incremental scope (no regression), but please make it explicit: a comment at the #ifdef stating the limitation (+ ideally a one-line debug log), so the next Windows-locking pass knows this is intentionally unimplemented rather than accidentally missing. If you're up for it, LockFileEx on the same lock-file path is the natural follow-up — separate PR is fine.

Everything else looks merge-ready: 19/19 checks green, clean mergeable state against current main (impressive, given how much mcp.c/pipeline.c moved this week), scope matches the description exactly, no stray files.

@SS-42
SS-42 force-pushed the fix/adr-cross-process-lock branch from ba39ffd to 0a8b63e Compare July 9, 2026 18:21
@SS-42

SS-42 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both requested changes in 0a8b63e: test setup now fails on mkdtemp errors, and the Windows lock limitation is explicit with a debug log. Also tightened cancellation and test cleanup.

@DeusData

Copy link
Copy Markdown
Owner

Thanks @SS-42 — reviewed in depth and the mechanism is sound: POSIX fcntl advisory locks are the right choice (they auto-release on process death, so there are no stale lock files to garbage-collect), the lock filename is path-traversal-safe, and there's no ABBA/self-deadlock. We'd like to take this, with two changes before merge:

  1. Don't let ADR reads block behind a reindex. Right now manage_adr takes the blocking pipeline lock for all modes, so a get/sections read can hang for minutes behind a same-project full reindex in another process. Please give the read paths a non-blocking/try-lock-with-timeout path (serialize writes, but let reads proceed or fail fast rather than hang).
  2. Rebase onto current main. The base has diverged a lot (the cbm_pipeline_lock() sites and handle_manage_adr have moved), so this needs a rebase + a green re-run before we can merge.

The Windows no-op is fine as documented (it's not a regression — there was no cross-process lock there before). Really appreciate this.

winnyschuster pushed a commit to winnyschuster/codebase-memory-mcp that referenced this pull request Jul 16, 2026
… reclaimed

On-disk write connections set no journal_size_limit, so the -wal file is never
physically shrunk: all our checkpoints are PASSIVE (TRUNCATE is deliberately
avoided because its ftruncate(fd,0) can SIGBUS a sibling mmap'd process on
macOS), and PASSIVE never ftruncates. Under checkpoint starvation the WAL then
grows without bound (reported: 115 GB, filled the drive) and even a later
successful checkpoint leaves the file at its peak size (DeusData#1083).

Set journal_size_limit = 256 MiB on write connections so SQLite truncates the
-wal back to that bound whenever a checkpoint can finally reset the log —
reclaiming disk opportunistically during a session, not only on clean close.
256 MiB is far above the healthy WAL (~4 MiB under the default 1000-page
autocheckpoint), so normal indexing never triggers truncate/regrow churn; the
limit only fires after abnormal growth. No TRUNCATE checkpoint is introduced.

Scope: this is the *reclaim* half of DeusData#1083. It does not prevent the peak under
continuous reader contention (a checkpoint can't reset the log while a reader
holds a mark) — the write-amplification driver (concurrent full re-indexes of
one repo) is serialized by the cross-process pipeline lock in DeusData#892.

Test: a file-backed write store now reports journal_size_limit = 256 MiB
(RED on main: -1 / unlimited).

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@SS-42
SS-42 force-pushed the fix/adr-cross-process-lock branch from 0a8b63e to 918ba1e Compare July 26, 2026 18:43
@SS-42 SS-42 changed the title fix(mcp): serialize ADR access across processes fix(mcp): keep ADR reads nonblocking during reindex Jul 26, 2026
@SS-42

SS-42 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main. get and sections now use read-only access without waiting for a project mutation; legacy migration is nonblocking, while updates remain serialized. Added regression coverage and reran the full suite.

Signed-off-by: SS-42 <noreply@incogni.to>
@SS-42
SS-42 force-pushed the fix/adr-cross-process-lock branch from 918ba1e to df6d61e Compare July 26, 2026 18:45
ycsx pushed a commit to ycsx/codebase-memory-mcp that referenced this pull request Jul 28, 2026
… reclaimed

On-disk write connections set no journal_size_limit, so the -wal file is never
physically shrunk: all our checkpoints are PASSIVE (TRUNCATE is deliberately
avoided because its ftruncate(fd,0) can SIGBUS a sibling mmap'd process on
macOS), and PASSIVE never ftruncates. Under checkpoint starvation the WAL then
grows without bound (reported: 115 GB, filled the drive) and even a later
successful checkpoint leaves the file at its peak size (DeusData#1083).

Set journal_size_limit = 256 MiB on write connections so SQLite truncates the
-wal back to that bound whenever a checkpoint can finally reset the log —
reclaiming disk opportunistically during a session, not only on clean close.
256 MiB is far above the healthy WAL (~4 MiB under the default 1000-page
autocheckpoint), so normal indexing never triggers truncate/regrow churn; the
limit only fires after abnormal growth. No TRUNCATE checkpoint is introduced.

Scope: this is the *reclaim* half of DeusData#1083. It does not prevent the peak under
continuous reader contention (a checkpoint can't reset the log while a reader
holds a mark) — the write-amplification driver (concurrent full re-indexes of
one repo) is serialized by the cross-process pipeline lock in DeusData#892.

Test: a file-backed write store now reports journal_size_limit = 256 MiB
(RED on main: -1 / unlimited).

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants