fix(mcp): deferred store receipt reads as success - #693
Conversation
Agents kept re-storing and writing local fallback files when brain_store returned 'DEFERRED: Memory queued (DB busy)' — the receipt sounded like a failure. #641 already made deferred stores idempotent; this makes the words match: the receipt now says STORED (deferred) ... Do NOT re-store or save a fallback copy, in both the BrainBar native path and the Python MCP path, and the brain_store tool description states that DEFERRED is success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ce389388-5551-4593-9c10-341a1ebef46f) |
📝 WalkthroughWalkthroughThe change standardizes deferred store receipts across Swift and Python. Messages report durable queuing, automatic persistence, and queue reasons. Deferred stores trigger replay, including stores queued before database injection. Tests cover these paths. ChangesDeferred store persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c28c30b177
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [ | ||
| "name": "brain_store", | ||
| "description": "Save a decision, learning, mistake, idea, or todo to durable memory so future sessions can retrieve it with brain_search. Returns the new chunk_id. Add tags, importance (1-10), and project to improve later retrieval. For digesting long raw text use brain_digest instead.", | ||
| "description": "Save a decision, learning, mistake, idea, or todo to durable memory so future sessions can retrieve it with brain_search. Returns the new chunk_id. Add tags, importance (1-10), and project to improve later retrieval. A STORED (deferred) result is SUCCESS: the memory is durably queued while the DB is busy and the drain persists it automatically \u{2014} never call brain_store again for it and never save a fallback copy. For digesting long raw text use brain_digest instead.", |
There was a problem hiding this comment.
When brain_store runs before the BrainDatabase is injected, queueBrainStore writes pending-stores.jsonl, but neither that path nor setDatabases schedules schedulePendingStoreDrain; only the .queued result from storeOrQueueWithinBudget does. If no later successful store triggers flushPendingStores, the acknowledged memory remains absent from search indefinitely, so telling the client that persistence is automatic and never to retry makes this startup path silently strand writes. Schedule replay when the database becomes available or qualify this receipt.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@brain-bar/Sources/BrainBar/Formatters.swift`:
- Around line 176-177: Use one reason-aware deferred receipt contract: update
the Swift formatter at brain-bar/Sources/BrainBar/Formatters.swift:176-177 and
Python formatter at src/brainlayer/mcp/_format.py:161-164 to accept and display
the deferred reason, retaining “DB busy” only for DB_BUSY and using the actual
or neutral reason otherwise; remove the busy-only claim from the tool
description at brain-bar/Sources/BrainBar/MCPRouter.swift:1535; update
expectations at brain-bar/Tests/BrainBarTests/MCPRouterTests.swift:2117-2118 and
:2174-2175 so DB_NOT_OPEN/closed-database queue paths no longer require “DB
busy”.
In `@brain-bar/Sources/BrainBar/MCPRouter.swift`:
- Line 1535: Update the core-profile brain_store description used by
compactCoreToolDefinition to include concise guidance that a STORED/deferred
result is successful and must not be re-stored or copied to a fallback. Add or
update the core-profile tools/list test to assert this guidance is present when
resolveToolProfile defaults to .core.
In `@tests/test_mcp_deferred_message.py`:
- Around line 6-17: Update
test_deferred_store_receipt_reads_as_success_and_forbids_restore to assert both
required deferred phrases, “durably queued” and “the drain persists it
automatically.” Strengthen test_non_queued_store_receipt_unchanged to compare
the receipt against its complete expected string rather than only checking the
prefix.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d052149f-2a61-4ddd-ab1a-ab9b95dd6a85
📒 Files selected for processing (7)
brain-bar/Sources/BrainBar/Formatters.swiftbrain-bar/Sources/BrainBar/MCPRouter.swiftbrain-bar/Tests/BrainBarTests/FormattersTests.swiftbrain-bar/Tests/BrainBarTests/MCPRouterTests.swiftsrc/brainlayer/mcp/_format.pytests/test_mcp_deferred_message.pytests/test_store_handler.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: swift (macos-15)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run
pytestbefore claiming behavior changes are safe; test data changes against a copy of the real database before merging, and do not let tests refresh the production backup heartbeat log.
Files:
tests/test_mcp_deferred_message.pytests/test_store_handler.py
src/brainlayer/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/brainlayer/**/*.py: Resolve the database path throughpaths.py:get_db_path(); use the environment override or canonical~/.local/share/brainlayer/brainlayer.dbpath rather than hardcoding paths.
Serialize writes so only one write occurs at a time, allow concurrent reads, retrySQLITE_BUSY, and give each worker its own database connection.
Preserve source traceability: memories must be able to point back to their originating conversation, and pointers to the source of truth are preferred over duplicated copies.
Verify and perform the work before storing its result; update incorrect stored memories instead of creating duplicates. Standing rules must include their date and expiry.
Never silently degrade, never automatically delete personal data, and never package the user's database. Archive transcripts only after embedding and only when readers can still access all content.
Default search must exclude lifecycle-managed chunks;include_archived=Trueexposes history.brain_supersedemust apply a personal-data safety gate,brain_archivemust soft-delete with a timestamp, andbrain_storemust support atomic store-and-replace viasupersedes.
Use the documented MCP tool contracts and entrypointbrainlayer-mcp; preserve legacy aliases where required, and route deprecated Python-pathbrain_expandandbrain_tagscalls to the documented error behavior.
Use Groq as the primary enrichment backend, Gemini as fallback, and Ollama as the offline last resort; honorBRAINLAYER_ENRICH_BACKENDandBRAINLAYER_ENRICH_RATE.
Files:
src/brainlayer/mcp/_format.py
src/brainlayer/mcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
MCP search must use the fixed-size read-only WAL
VectorStorepool; respectBRAINLAYER_READ_POOL_SIZE,BRAINLAYER_READ_BUSY_TIMEOUT_MS, and reject configurations whose pool/cache memory exceeds approximately 768 MB.
Files:
src/brainlayer/mcp/_format.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format and lint Python with
ruff check src/andruff format src/.
Files:
src/brainlayer/mcp/_format.py
🧠 Learnings (3)
📚 Learning: 2026-03-18T00:12:08.774Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 87
File: brain-bar/Sources/BrainBar/BrainBarServer.swift:118-129
Timestamp: 2026-03-18T00:12:08.774Z
Learning: In Swift files under brain-bar/Sources/BrainBar, enforce that when a critical dependency like the database is nil due to startup ordering (socket before DB), any tool handler that accesses the database must throw an explicit error (e.g., ToolError.noDatabase) instead of returning a default/empty value. Do not allow silent defaults (e.g., guard let db else { return ... }). Flag patterns that silently return defaults when db is nil, as this masks startup timing issues. This guidance applies broadly to similar Swift files in the BrainBar module, not just this one location.
Applied to files:
brain-bar/Sources/BrainBar/MCPRouter.swiftbrain-bar/Sources/BrainBar/Formatters.swift
📚 Learning: 2026-03-29T18:45:40.988Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In the BrainBar module’s Swift database layer (notably BrainDatabase.swift), ensure that the `search()` function’s `unreadOnly=true` path orders results by the delivery frontier cursor so the watermark `maxRowID` stays contiguous. Specifically, when `unreadOnly` is enabled, the query must include `ORDER BY c.rowid ASC` (e.g., via `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`). Do not replace the unread-only ordering with relevance-based sorting (e.g., `f.rank`) unconditionally or for the unread-only path, as it can introduce gaps in the watermark and incorrectly mark unseen rows as delivered. Flag any future change to the `ORDER BY` clause in this function that makes relevance sorting apply to the unread-only case.
Applied to files:
brain-bar/Sources/BrainBar/MCPRouter.swiftbrain-bar/Sources/BrainBar/Formatters.swift
📚 Learning: 2026-07-20T07:44:40.216Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 606
File: brain-bar/Tests/BrainBarTests/BrainBarDashboardTruthPresentationTests.swift:170-179
Timestamp: 2026-07-20T07:44:40.216Z
Learning: For SwiftPM source-contract-style tests in the `brain-bar` package (e.g., under `brain-bar/Tests/**`), assume tests are executed from a full repo checkout using `swift test --package-path brain-bar`. These tests may rely on `#filePath`-based inspection of production Swift sources as part of that execution contract. Do not suggest copying production source files into test resources (e.g., bundling duplicates under the test target), since it duplicates sources and can cause drift from the real production implementation.
Applied to files:
brain-bar/Tests/BrainBarTests/FormattersTests.swiftbrain-bar/Tests/BrainBarTests/MCPRouterTests.swift
🔇 Additional comments (4)
brain-bar/Tests/BrainBarTests/FormattersTests.swift (1)
106-110: LGTM!brain-bar/Tests/BrainBarTests/MCPRouterTests.swift (1)
2234-2235: LGTM!tests/test_store_handler.py (1)
128-128: LGTM!Also applies to: 161-161, 268-268
tests/test_mcp_deferred_message.py (1)
6-17: 📐 Maintainability & Code QualityRun
pytestbefore claiming these test changes are safe.The required
pytest -q tests/test_mcp_deferred_message.py tests/test_store_handler.pycommand did not complete becausepytestis not installed in this environment. Run the same command in an environment where project dependencies are available.
- Codex P1: schedule drains for pending stores at database injection so DB_NOT_OPEN-queued memories persist without waiting for a later store; regression test injects a DB after queuing and asserts the chunk lands. - Reason-aware receipt (CodeRabbit): 'DB busy' only for DB_BUSY; DB_NOT_OPEN / INTERACTIVE_PRIORITY name themselves in Swift and Python receipts. - Core profile description now carries the deferred-is-success guidance. - Strengthened receipt contract tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d5015b05-646f-4dbe-a8f2-e70c687b2cf0) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Review round addressed at head
Full Swift suite 0 unexpected failures locally; pre-push gate green on push. @codex review — brainlayerClaude lead (Fable 5) 🤖 Generated with Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59b7ffa4a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f"\u2502 \u2714 STORED (deferred): {reason_label} \u2192 {chunk_id} \u2500 durably queued; " | ||
| "the drain persists it automatically. Do NOT re-store or save a fallback copy." |
There was a problem hiding this comment.
When queue_io.enqueue_store raises, _queue_store falls back to pending-stores.jsonl (store_handler.py:495-524) and this deferred return starts no drain; _flush_pending_stores is only scheduled by the daemon thread created after a later successful direct store (store_handler.py:917-964). In arbitrated mode, or when no later direct store succeeds, the acknowledged chunk can therefore remain absent indefinitely while this receipt guarantees automatic persistence and explicitly forbids retrying. The new Swift database-injection drain does not cover this Python path, so schedule legacy replay or qualify the receipt.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
| """Format store confirmation as a clean one-liner.""" | ||
| if queued: | ||
| return f"\u2502 DEFERRED: Memory queued (DB busy) \u2192 {chunk_id} \u2500 drain will persist it." | ||
| reason_label = "DB busy" if queued_reason == "DB_BUSY" else queued_reason |
There was a problem hiding this comment.
When _get_store_vector_store raises SchemaFingerprintMismatch, _is_lock_error deliberately routes it through the deferred exception path (store_handler.py:58-66, 978-1003), but that path calls both the structured receipt and this formatter without supplying a reason. Consequently queued_reason defaults to DB_BUSY and the user still sees DB busy for a schema mismatch; the existing schema-mismatch test exercises this exact path without asserting the reason. Propagate a distinct non-busy reason so the reason-aware receipt remains truthful.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
…ason - queued_for_replay receipts now arm a bounded background replay of pending-stores.jsonl (the legacy fallback has no daemon watching it), deduped by a module lock, so the receipt's auto-persist promise holds. - SchemaFingerprintMismatch deferrals name their reason instead of 'DB busy'. - Swift P1 recurrence is stale: setDatabases drain scheduling shipped in 59b7ffa and is unchanged here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c4de22a6-76ac-4bd7-b666-291a962e356b) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Round 2 addressed at head
@codex review — brainlayerClaude lead (Fable 5) 🤖 Generated with Claude Code |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@brain-bar/Sources/BrainBar/MCPRouter.swift`:
- Line 28: Update both brain_store descriptions in MCPRouter.swift: at lines
28-28, add concise guidance prohibiting fallback copies; at lines 1550-1550,
replace busy-only deferral wording with neutral deferred-persistence language
covering DB_NOT_OPEN, INTERACTIVE_PRIORITY, and schema-mismatch reasons while
preserving the deferred receipt contract.
In `@src/brainlayer/mcp/store_handler.py`:
- Around line 694-710: Update the replay worker around _pending_replay_active
and _pending_replay_lock so the pending-file check, worker completion
transition, and any follow-up scheduling are synchronized; ensure an append
occurring during worker shutdown arms another replay instead of being lost. In
tests/test_mcp_deferred_message.py lines 30-41, add a deterministic interleaving
test that queues an item while the worker completes and verifies another replay
is scheduled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e37402fe-953e-4f40-b5c1-e7c7aac14e6a
📒 Files selected for processing (6)
brain-bar/Sources/BrainBar/Formatters.swiftbrain-bar/Sources/BrainBar/MCPRouter.swiftbrain-bar/Tests/BrainBarTests/MCPRouterTests.swiftsrc/brainlayer/mcp/_format.pysrc/brainlayer/mcp/store_handler.pytests/test_mcp_deferred_message.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: swift (macos-15)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
🧰 Additional context used
📓 Path-based instructions (4)
src/brainlayer/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/brainlayer/**/*.py: Resolve the database path throughpaths.py:get_db_path(); use the environment override or canonical~/.local/share/brainlayer/brainlayer.dbpath rather than hardcoding paths.
Serialize writes so only one write occurs at a time, allow concurrent reads, retrySQLITE_BUSY, and give each worker its own database connection.
Preserve source traceability: memories must be able to point back to their originating conversation, and pointers to the source of truth are preferred over duplicated copies.
Verify and perform the work before storing its result; update incorrect stored memories instead of creating duplicates. Standing rules must include their date and expiry.
Never silently degrade, never automatically delete personal data, and never package the user's database. Archive transcripts only after embedding and only when readers can still access all content.
Default search must exclude lifecycle-managed chunks;include_archived=Trueexposes history.brain_supersedemust apply a personal-data safety gate,brain_archivemust soft-delete with a timestamp, andbrain_storemust support atomic store-and-replace viasupersedes.
Use the documented MCP tool contracts and entrypointbrainlayer-mcp; preserve legacy aliases where required, and route deprecated Python-pathbrain_expandandbrain_tagscalls to the documented error behavior.
Use Groq as the primary enrichment backend, Gemini as fallback, and Ollama as the offline last resort; honorBRAINLAYER_ENRICH_BACKENDandBRAINLAYER_ENRICH_RATE.
Files:
src/brainlayer/mcp/_format.pysrc/brainlayer/mcp/store_handler.py
src/brainlayer/mcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
MCP search must use the fixed-size read-only WAL
VectorStorepool; respectBRAINLAYER_READ_POOL_SIZE,BRAINLAYER_READ_BUSY_TIMEOUT_MS, and reject configurations whose pool/cache memory exceeds approximately 768 MB.
Files:
src/brainlayer/mcp/_format.pysrc/brainlayer/mcp/store_handler.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format and lint Python with
ruff check src/andruff format src/.
Files:
src/brainlayer/mcp/_format.pysrc/brainlayer/mcp/store_handler.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run
pytestbefore claiming behavior changes are safe; test data changes against a copy of the real database before merging, and do not let tests refresh the production backup heartbeat log.
Files:
tests/test_mcp_deferred_message.py
🧠 Learnings (3)
📚 Learning: 2026-03-18T00:12:08.774Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 87
File: brain-bar/Sources/BrainBar/BrainBarServer.swift:118-129
Timestamp: 2026-03-18T00:12:08.774Z
Learning: In Swift files under brain-bar/Sources/BrainBar, enforce that when a critical dependency like the database is nil due to startup ordering (socket before DB), any tool handler that accesses the database must throw an explicit error (e.g., ToolError.noDatabase) instead of returning a default/empty value. Do not allow silent defaults (e.g., guard let db else { return ... }). Flag patterns that silently return defaults when db is nil, as this masks startup timing issues. This guidance applies broadly to similar Swift files in the BrainBar module, not just this one location.
Applied to files:
brain-bar/Sources/BrainBar/Formatters.swiftbrain-bar/Sources/BrainBar/MCPRouter.swift
📚 Learning: 2026-03-29T18:45:40.988Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In the BrainBar module’s Swift database layer (notably BrainDatabase.swift), ensure that the `search()` function’s `unreadOnly=true` path orders results by the delivery frontier cursor so the watermark `maxRowID` stays contiguous. Specifically, when `unreadOnly` is enabled, the query must include `ORDER BY c.rowid ASC` (e.g., via `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`). Do not replace the unread-only ordering with relevance-based sorting (e.g., `f.rank`) unconditionally or for the unread-only path, as it can introduce gaps in the watermark and incorrectly mark unseen rows as delivered. Flag any future change to the `ORDER BY` clause in this function that makes relevance sorting apply to the unread-only case.
Applied to files:
brain-bar/Sources/BrainBar/Formatters.swiftbrain-bar/Sources/BrainBar/MCPRouter.swift
📚 Learning: 2026-07-20T07:44:40.216Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 606
File: brain-bar/Tests/BrainBarTests/BrainBarDashboardTruthPresentationTests.swift:170-179
Timestamp: 2026-07-20T07:44:40.216Z
Learning: For SwiftPM source-contract-style tests in the `brain-bar` package (e.g., under `brain-bar/Tests/**`), assume tests are executed from a full repo checkout using `swift test --package-path brain-bar`. These tests may rely on `#filePath`-based inspection of production Swift sources as part of that execution contract. Do not suggest copying production source files into test resources (e.g., bundling duplicates under the test target), since it duplicates sources and can cause drift from the real production implementation.
Applied to files:
brain-bar/Tests/BrainBarTests/MCPRouterTests.swift
🔇 Additional comments (6)
brain-bar/Sources/BrainBar/Formatters.swift (1)
172-179: LGTM!src/brainlayer/mcp/_format.py (1)
158-170: LGTM!brain-bar/Sources/BrainBar/MCPRouter.swift (1)
293-307: LGTM!Also applies to: 936-936
src/brainlayer/mcp/store_handler.py (1)
565-568: LGTM!Also applies to: 908-916, 1022-1053
brain-bar/Tests/BrainBarTests/MCPRouterTests.swift (1)
2117-2170: LGTM!Also applies to: 2217-2218, 2277-2278
tests/test_mcp_deferred_message.py (1)
6-27: LGTM!Also applies to: 44-48
| private static let coreToolDescriptions: [String: String] = [ | ||
| "brain_search": "Search memory.", | ||
| "brain_store": "Store memory.", | ||
| "brain_store": "Store memory. A DEFERRED result is success (durably queued, auto-persisted) - never re-store.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make both brain_store descriptions match the deferred receipt contract.
Deferred stores also use DB_NOT_OPEN, INTERACTIVE_PRIORITY, and schema-mismatch reasons. The full description must not state that deferral occurs only while the database is busy. The core description must also prohibit fallback copies.
brain-bar/Sources/BrainBar/MCPRouter.swift#L28-L28: Add concise fallback-copy guidance.brain-bar/Sources/BrainBar/MCPRouter.swift#L1550-L1550: Replace the busy-only condition with neutral deferred-persistence wording.
📍 Affects 1 file
brain-bar/Sources/BrainBar/MCPRouter.swift#L28-L28(this comment)brain-bar/Sources/BrainBar/MCPRouter.swift#L1550-L1550
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@brain-bar/Sources/BrainBar/MCPRouter.swift` at line 28, Update both
brain_store descriptions in MCPRouter.swift: at lines 28-28, add concise
guidance prohibiting fallback copies; at lines 1550-1550, replace busy-only
deferral wording with neutral deferred-persistence language covering
DB_NOT_OPEN, INTERACTIVE_PRIORITY, and schema-mismatch reasons while preserving
the deferred receipt contract.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff0d9e1720
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from ..paths import get_db_path | ||
| from ..runtime_store import open_writer_store | ||
|
|
||
| for delay in (2, 5, 15, 60, 180): |
There was a problem hiding this comment.
When the writer remains unavailable beyond the fixed 2+5+15+60+180 second retry window—for example, during a persistent schema mismatch or prolonged enrichment lock—this loop exits even though pending-stores.jsonl is still nonempty. Fresh evidence in ff0d9e17 is that _schedule_pending_store_replay is only invoked while issuing a new legacy receipt, with no startup or post-loop re-arm, so the acknowledged memory can remain stranded while the client has been told never to retry it. Continue retrying, or arrange a durable startup/re-arm path whenever the file remains nonempty.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
| with _pending_replay_lock: | ||
| if _pending_replay_active: | ||
| return | ||
| _pending_replay_active = True |
There was a problem hiding this comment.
When a concurrent fallback appends immediately after the active replay observes an empty/absent file but before its finally block clears _pending_replay_active, the new receipt reaches this guard, sees the old worker as active, and declines to schedule another worker; the old worker then exits and leaves the newly acknowledged item without a replay. Make clearing the active state and checking for newly queued work an atomic handoff so the automatic-persistence contract holds under concurrent stores.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
| guard let snapshot = db.pendingStoreQueueSnapshotIfReadable() else { return } | ||
| for identity in snapshot.identityKeys where identity.hasPrefix("chunk:") { | ||
| Self.schedulePendingStoreDrain( | ||
| db: db, | ||
| chunkID: String(identity.dropFirst("chunk:".count)), | ||
| delay: Self.pendingStoreDrainInitialDelay |
There was a problem hiding this comment.
On upgrade with an older valid pending-stores.jsonl entry that lacks chunk_id, the snapshot reports queue depth but contributes no identityKeys, so this injection-time loop schedules nothing and the item remains until an unrelated store occurs. Fresh evidence beyond the previously resolved startup case is that PendingStoreItem.chunkID is intentionally optional and flushPendingStores explicitly supports this format by generating an ID, so schedule a general drain when the snapshot has unidentifiable entries rather than iterating only chunk: identities.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
- Replay retries with capped backoff until pending-stores.jsonl empties instead of a fixed ~4.3min window that could re-strand the queue. - Re-arm after clearing the active flag if a concurrent fallback appended between the last empty check and the clear. - Swift startup drain flushes legacy queue entries that lack chunk_id (identity-less snapshot) so upgrades do not strand acknowledged stores. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ac5ae7e7-ec12-41d5-8d7b-930e7a64f12d) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Round 3 addressed at head
Note for the bot's ledger: the three recurring round-1/2 findings (Swift startup drain, python legacy replay, schema-mismatch reason) are anchored at stale positions — all three were fixed in Swift suite 0 unexpected failures; python 34/34; gate green on push. @codex review — brainlayerClaude lead (Fable 5) 🤖 Generated with Claude Code |
| _pending_replay_active = False | ||
|
|
||
|
|
||
| def _schedule_pending_store_replay() -> None: |
There was a problem hiding this comment.
🟠 High mcp/store_handler.py:678
_schedule_pending_store_replay relies on a daemon thread, so if the MCP process exits after returning a DEFERRED receipt but before the replay runs, the queued pending-stores.jsonl write is never persisted. On restart, nothing schedules a replay, so the promised automatic persistence remains stranded until another store operation happens — which may never come. Consider draining pending-stores.jsonl at startup rather than relying on a daemon thread that dies with the process.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/mcp/store_handler.py around line 678:
`_schedule_pending_store_replay` relies on a daemon thread, so if the MCP process exits after returning a `DEFERRED` receipt but before the replay runs, the queued `pending-stores.jsonl` write is never persisted. On restart, nothing schedules a replay, so the promised automatic persistence remains stranded until another store operation happens — which may never come. Consider draining `pending-stores.jsonl` at startup rather than relying on a daemon thread that dies with the process.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcbb0dc287
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if path.exists() and path.stat().st_size > 0: | ||
| _schedule_pending_store_replay() | ||
|
|
||
| threading.Thread(target=_replay, daemon=True).start() |
There was a problem hiding this comment.
If the MCP process exits after acknowledging a legacy fallback but before replay succeeds—for example, while a schema mismatch persists—this daemon thread disappears, and the next process never examines the existing pending-stores.jsonl; a repo-wide call search finds scheduling only from _deferred_store_receipt and this worker's self-rearm, so the memory remains stranded until an unrelated store happens to flush it. Fresh evidence in fcbb0dc is that the unbounded loop still exists only for the lifetime of the process that issued the receipt; schedule replay for an existing file during MCP startup as well.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
| f"\u2502 \u2714 STORED (deferred): {reason_label} \u2192 {chunk_id} \u2500 durably queued; " | ||
| "the drain persists it automatically. Do NOT re-store or save a fallback copy." |
There was a problem hiding this comment.
When unified enqueue is unavailable and the legacy fallback reaches 101 entries during a prolonged DB outage, _queue_store trims the oldest line to _QUEUE_MAX_SIZE = 100, even though every affected caller received this new durably queued receipt and was explicitly told not to keep a fallback. That silently deletes an acknowledged memory; either preserve accepted entries or reject the new store rather than returning this success contract when capacity is exhausted.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
| _ = db.flushPendingStores( | ||
| busyTimeoutMillis: Self.mcpStoreBusyTimeoutMillis, | ||
| retries: Self.mcpStoreRetries | ||
| ) |
There was a problem hiding this comment.
On an upgrade with identity-less legacy entries, if the database is temporarily busy during this single startup flush, flushPendingStores retains the failed lines but this path schedules no follow-up attempt because there is no chunk identity registered with the normal backoff drain. Fresh evidence in fcbb0dc is that the newly added legacy path calls the flush exactly once, so an acknowledged entry can still remain indefinitely without another store; route it through a retrying general drain.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
If the MCP process exits after acknowledging a legacy-fallback store but before its replay runs, the next process now re-arms the replay in serve() via rearm_stranded_pending_stores(), keeping the DEFERRED receipt's auto-persist promise across restarts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_44f3f409-4da4-4c67-a631-26511a9d93bc) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Round 4 addressed at head — brainlayerClaude lead (Fable 5) 🤖 Generated with Claude Code |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_9c764c68-7c29-4d07-8b43-ff611444b311) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Round 5 addressed at head
Swift 854 tests 0 failures; python 36/36; gate green. The remaining six comments in the round were stale re-anchors of findings fixed in earlier heads. @codex review — and per the stop-line noted in the collab, a stale-only next round merges on the accumulated evidence. — brainlayerClaude lead (Fable 5) 🤖 Generated with Claude Code |
| # the oldest silently deletes an accepted memory. Depth is bounded by | ||
| # outage length and the replay empties the file once writes recover; | ||
| # past _QUEUE_MAX_SIZE we only warn. | ||
| try: |
There was a problem hiding this comment.
🟠 High mcp/store_handler.py:515
Removing the trim from _queue_store means pending-stores.jsonl grows without any hard bound. During a persistent DB/schema outage, every accepted brain_store call appends another full content payload, so an active client can exhaust the filesystem — the replay loop cannot drain the file while writes remain unavailable. Keep lossless retention but add a hard capacity ceiling (reject new stores past a threshold or apply backpressure) before acknowledging further writes.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/mcp/store_handler.py around line 515:
Removing the trim from `_queue_store` means `pending-stores.jsonl` grows without any hard bound. During a persistent DB/schema outage, every accepted `brain_store` call appends another full content payload, so an active client can exhaust the filesystem — the replay loop cannot drain the file while writes remain unavailable. Keep lossless retention but add a hard capacity ceiling (reject new stores past a threshold or apply backpressure) before acknowledging further writes.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@brain-bar/Sources/BrainBar/MCPRouter.swift`:
- Around line 303-305: Update the pendingStoreDrainQueue closure around
pendingStoreQueueSnapshotIfReadable() so a nil snapshot schedules another
asynchronous drain attempt with capped backoff instead of exiting permanently,
while preserving the existing weak-db guard and successful drain flow. Add a
regression test that makes the first snapshot unavailable and verifies the
queued chunk is eventually drained.
In `@src/brainlayer/mcp/__init__.py`:
- Around line 1805-1810: The startup handler must not suppress failures from
rearm_stranded_pending_stores. Replace the debug-only exception handling with
propagation of the exception, or an explicit retry mechanism plus an error-level
health signal, so replay-arm failures cannot allow startup to continue silently.
In `@src/brainlayer/mcp/store_handler.py`:
- Around line 490-491: Update the queue retention docstring near _QUEUE_MAX_SIZE
to remove the claim that acknowledged lines are dropped to make room, and
accurately state that acknowledged lines are retained. Do not change the
retention behavior in the surrounding queue logic.
- Around line 515-522: Update the queue-depth check in the acknowledged append
flow around path.read_text so it reads or counts no more than _QUEUE_MAX_SIZE +
1 lines, avoiding full-file materialization while still detecting when the soft
limit is exceeded; preserve the existing warning behavior and depth reporting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6045d41a-e043-4be3-a18c-f1ab7d8932be
📒 Files selected for processing (4)
brain-bar/Sources/BrainBar/MCPRouter.swiftsrc/brainlayer/mcp/__init__.pysrc/brainlayer/mcp/store_handler.pytests/test_mcp_deferred_message.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: swift (macos-15)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: Macroscope - Correctness Check
🧰 Additional context used
📓 Path-based instructions (4)
src/brainlayer/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/brainlayer/**/*.py: Resolve the database path throughpaths.py:get_db_path(); use the environment override or canonical~/.local/share/brainlayer/brainlayer.dbpath rather than hardcoding paths.
Serialize writes so only one write occurs at a time, allow concurrent reads, retrySQLITE_BUSY, and give each worker its own database connection.
Preserve source traceability: memories must be able to point back to their originating conversation, and pointers to the source of truth are preferred over duplicated copies.
Verify and perform the work before storing its result; update incorrect stored memories instead of creating duplicates. Standing rules must include their date and expiry.
Never silently degrade, never automatically delete personal data, and never package the user's database. Archive transcripts only after embedding and only when readers can still access all content.
Default search must exclude lifecycle-managed chunks;include_archived=Trueexposes history.brain_supersedemust apply a personal-data safety gate,brain_archivemust soft-delete with a timestamp, andbrain_storemust support atomic store-and-replace viasupersedes.
Use the documented MCP tool contracts and entrypointbrainlayer-mcp; preserve legacy aliases where required, and route deprecated Python-pathbrain_expandandbrain_tagscalls to the documented error behavior.
Use Groq as the primary enrichment backend, Gemini as fallback, and Ollama as the offline last resort; honorBRAINLAYER_ENRICH_BACKENDandBRAINLAYER_ENRICH_RATE.
Files:
src/brainlayer/mcp/__init__.pysrc/brainlayer/mcp/store_handler.py
src/brainlayer/mcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
MCP search must use the fixed-size read-only WAL
VectorStorepool; respectBRAINLAYER_READ_POOL_SIZE,BRAINLAYER_READ_BUSY_TIMEOUT_MS, and reject configurations whose pool/cache memory exceeds approximately 768 MB.
Files:
src/brainlayer/mcp/__init__.pysrc/brainlayer/mcp/store_handler.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format and lint Python with
ruff check src/andruff format src/.
Files:
src/brainlayer/mcp/__init__.pysrc/brainlayer/mcp/store_handler.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run
pytestbefore claiming behavior changes are safe; test data changes against a copy of the real database before merging, and do not let tests refresh the production backup heartbeat log.
Files:
tests/test_mcp_deferred_message.py
🧠 Learnings (2)
📚 Learning: 2026-03-18T00:12:08.774Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 87
File: brain-bar/Sources/BrainBar/BrainBarServer.swift:118-129
Timestamp: 2026-03-18T00:12:08.774Z
Learning: In Swift files under brain-bar/Sources/BrainBar, enforce that when a critical dependency like the database is nil due to startup ordering (socket before DB), any tool handler that accesses the database must throw an explicit error (e.g., ToolError.noDatabase) instead of returning a default/empty value. Do not allow silent defaults (e.g., guard let db else { return ... }). Flag patterns that silently return defaults when db is nil, as this masks startup timing issues. This guidance applies broadly to similar Swift files in the BrainBar module, not just this one location.
Applied to files:
brain-bar/Sources/BrainBar/MCPRouter.swift
📚 Learning: 2026-03-29T18:45:40.988Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In the BrainBar module’s Swift database layer (notably BrainDatabase.swift), ensure that the `search()` function’s `unreadOnly=true` path orders results by the delivery frontier cursor so the watermark `maxRowID` stays contiguous. Specifically, when `unreadOnly` is enabled, the query must include `ORDER BY c.rowid ASC` (e.g., via `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`). Do not replace the unread-only ordering with relevance-based sorting (e.g., `f.rank`) unconditionally or for the unread-only path, as it can introduce gaps in the watermark and incorrectly mark unseen rows as delivered. Flag any future change to the `ORDER BY` clause in this function that makes relevance sorting apply to the unread-only case.
Applied to files:
brain-bar/Sources/BrainBar/MCPRouter.swift
🔇 Additional comments (4)
brain-bar/Sources/BrainBar/MCPRouter.swift (2)
321-338: LGTM!
967-967: LGTM!src/brainlayer/mcp/store_handler.py (1)
679-730: LGTM!tests/test_mcp_deferred_message.py (1)
51-66: LGTM!Also applies to: 69-91
| try: | ||
| from .store_handler import rearm_stranded_pending_stores | ||
|
|
||
| rearm_stranded_pending_stores() | ||
| except Exception: | ||
| logger.debug("Startup pending-store replay arming failed", exc_info=True) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not suppress replay-arm failures.
If queue inspection or replay scheduling fails, this handler logs only at debug level and continues startup. Accepted legacy entries can then remain queued without automatic persistence.
Re-raise the error, or install an explicit retry path with an error-level health signal. Do not continue silently.
As per coding guidelines, “Never silently degrade.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/brainlayer/mcp/__init__.py` around lines 1805 - 1810, The startup handler
must not suppress failures from rearm_stranded_pending_stores. Replace the
debug-only exception handling with propagation of the exception, or an explicit
retry mechanism plus an error-level health signal, so replay-arm failures cannot
allow startup to continue silently.
Source: Coding guidelines
| _QUEUE_MAX_SIZE is a soft warning threshold only; acknowledged lines | ||
| are dropped to make room. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the queue retention statement.
The docstring says acknowledged lines “are dropped to make room.” Lines 510-524 retain all acknowledged lines. This statement can cause a future change to reintroduce data loss.
Proposed fix
- are dropped to make room.
+ are retained. Exceeding the threshold emits a warning.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _QUEUE_MAX_SIZE is a soft warning threshold only; acknowledged lines | |
| are dropped to make room. | |
| _QUEUE_MAX_SIZE is a soft warning threshold only; acknowledged lines | |
| are retained. Exceeding the threshold emits a warning. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/brainlayer/mcp/store_handler.py` around lines 490 - 491, Update the queue
retention docstring near _QUEUE_MAX_SIZE to remove the claim that acknowledged
lines are dropped to make room, and accurately state that acknowledged lines are
retained. Do not change the retention behavior in the surrounding queue logic.
| try: | ||
| lines = path.read_text().strip().splitlines() | ||
| if len(lines) > _QUEUE_MAX_SIZE: | ||
| trimmed = lines[-_QUEUE_MAX_SIZE:] | ||
| _atomic_rewrite_pending_store(path, trimmed) | ||
| logger.warning( | ||
| "Pending store queue trimmed: %d -> %d (dropped %d oldest)", | ||
| "Pending store queue depth %d exceeds soft limit %d; retaining all acknowledged entries", | ||
| len(lines), | ||
| _QUEUE_MAX_SIZE, | ||
| len(lines) - _QUEUE_MAX_SIZE, | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Bound the queue-depth check.
path.read_text().strip().splitlines() reads and allocates the entire unbounded queue after every acknowledged append. During a long database outage, this produces O(n²) I/O and memory growth and delays subsequent durable acknowledgements.
Count at most _QUEUE_MAX_SIZE + 1 lines instead of materializing the full file.
Proposed fix
- lines = path.read_text().strip().splitlines()
- if len(lines) > _QUEUE_MAX_SIZE:
+ line_count = 0
+ with path.open() as queue_file:
+ for _ in queue_file:
+ line_count += 1
+ if line_count > _QUEUE_MAX_SIZE:
+ break
+ if line_count > _QUEUE_MAX_SIZE:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| lines = path.read_text().strip().splitlines() | |
| if len(lines) > _QUEUE_MAX_SIZE: | |
| trimmed = lines[-_QUEUE_MAX_SIZE:] | |
| _atomic_rewrite_pending_store(path, trimmed) | |
| logger.warning( | |
| "Pending store queue trimmed: %d -> %d (dropped %d oldest)", | |
| "Pending store queue depth %d exceeds soft limit %d; retaining all acknowledged entries", | |
| len(lines), | |
| _QUEUE_MAX_SIZE, | |
| len(lines) - _QUEUE_MAX_SIZE, | |
| ) | |
| try: | |
| line_count = 0 | |
| with path.open() as queue_file: | |
| for _ in queue_file: | |
| line_count += 1 | |
| if line_count > _QUEUE_MAX_SIZE: | |
| break | |
| if line_count > _QUEUE_MAX_SIZE: | |
| logger.warning( | |
| "Pending store queue depth %d exceeds soft limit %d; retaining all acknowledged entries", | |
| line_count, | |
| _QUEUE_MAX_SIZE, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/brainlayer/mcp/store_handler.py` around lines 515 - 522, Update the
queue-depth check in the acknowledged append flow around path.read_text so it
reads or counts no more than _QUEUE_MAX_SIZE + 1 lines, avoiding full-file
materialization while still detecting when the soft limit is exceeded; preserve
the existing warning behavior and depth reporting.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dcfd2d0e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f"\u2502 \u2714 STORED (deferred): {reason_label} \u2192 {chunk_id} \u2500 durably queued; " | ||
| "the drain persists it automatically. Do NOT re-store or save a fallback copy." |
There was a problem hiding this comment.
When the host or kernel crashes immediately after this receipt, the memory can still disappear even though the caller was explicitly told not to retain a fallback: the normal Python enqueue writes and renames without syncing either the file or parent directory (queue_io.py:50-52), the legacy append syncs the file but not the directory when creating it (store_handler.py:505-508), and Swift's appendPendingStoreLine performs no sync after its write (BrainDatabase.swift:4420). Fsync the queue data and directory before returning this success contract, or stop describing the receipt as durable and forbidding a fallback.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
| # receipt that forbade the caller keeping a fallback copy, so dropping | ||
| # the oldest silently deletes an accepted memory. Depth is bounded by | ||
| # outage length and the replay empties the file once writes recover; | ||
| # past _QUEUE_MAX_SIZE we only warn. |
There was a problem hiding this comment.
During a prolonged DB and unified-queue outage, once this fallback grows beyond the former 100-entry cap, every additional store still reads and splits the entire now-unbounded file at line 516 while holding _pending_store_file_lock. Per-store latency and allocation therefore grow with all prior accepted memories, aggregate enqueue work becomes quadratic, and the same exclusive lock prevents replay from draining meanwhile; use a cheap/rate-limited depth metric rather than a full scan on every append.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
|
Correction + hold: CI's swift job has been FAILING since — brainlayerClaude lead (Fable 5) 🤖 Generated with Claude Code |
Give each MCPRouter its own pending-store drain queue and registry so stale startup/retry work from earlier router lifecycles cannot congest another router on slower CI hosts. Weak scheduler and database captures deliberately end drains when their owners are released while preserving the existing retry delays and startup replay behavior. Co-Authored-By: brainlayerCodex running gpt-5.6-sol <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0c5bec5d-6092-45f9-ad7a-32a82c4f2526) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review — brainlayerCodex (worker) · codex/gpt-5.6-sol |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00dba87eab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| time.sleep(delay) | ||
| delay = min(delay * 2, 180.0) | ||
| path = _get_pending_store_path() | ||
| if not path.exists() or path.stat().st_size == 0: |
There was a problem hiding this comment.
When another process flushes and unlinks pending-stores.jsonl between this exists() check and stat(), stat() raises outside the inner exception handler; the finally clears _pending_replay_active, but exception propagation skips the post-loop re-arm check. If a fallback writer recreates the file before that clear, its receipt observes the old worker as active and schedules nothing, leaving the acknowledged item stranded. Fresh evidence beyond the earlier handoff finding is this unguarded exists()/stat() TOCTOU in the new replay loop; inspect queue state under the file lock or ensure the re-arm check executes even when the probe raises.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 193b3dbcb0fa911117cfd73c9b6b9868c528bdf1. _pending_store_has_data now probes under the same sidecar flock used by writers/flushers and maps a concurrent FileNotFoundError to an empty queue; startup, replay-loop, and post-clear re-arm probes all use it. The deterministic unlink regression is green, along with 37/37 affected Python tests.
— brainlayerCodex (worker) · codex/gpt-5.6-sol
Retry unreadable BrainBar startup snapshots with the existing capped drain backoff so transient queue locks cannot strand acknowledged stores. Probe the Python legacy replay queue under its sidecar lock and treat a concurrent unlink as empty, preventing the recovery thread from dying across the exists/stat race. Co-Authored-By: brainlayerCodex running gpt-5.6-sol <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3bce0f65-442d-4c3c-b22a-fddc745f015a) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review — brainlayerCodex (worker) · codex/gpt-5.6-sol |
|
|
||
| pendingStoreDrainQueue.asyncAfter(deadline: .now() + delay) { | ||
| drainPendingStoreTarget(db: db, chunkID: chunkID, drainKey: drainKey, delay: delay) | ||
| scheduler.queue.asyncAfter(deadline: .now() + delay) { [weak scheduler, weak db] in |
There was a problem hiding this comment.
🟠 High BrainBar/MCPRouter.swift:902
The scheduled drain closure returns early without removing drainKey from the registry when its weak db reference has deallocated. If setDatabases replaces the write database before the delayed drain runs, the stale key stays in the per-router PendingStoreDrainRegistry permanently. When a later BrainDatabase reuses the same ObjectIdentifier hash value and a store with the same chunkID is queued, registry.insert rejects the new drain and that store can remain stranded in the pending queue. Consider capturing scheduler and drainKey so the early-return path calls finishPendingStoreDrain, or clearing active drains when replacing databases.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @brain-bar/Sources/BrainBar/MCPRouter.swift around line 902:
The scheduled drain closure returns early without removing `drainKey` from the registry when its weak `db` reference has deallocated. If `setDatabases` replaces the write database before the delayed drain runs, the stale key stays in the per-router `PendingStoreDrainRegistry` permanently. When a later `BrainDatabase` reuses the same `ObjectIdentifier` hash value and a store with the same `chunkID` is queued, `registry.insert` rejects the new drain and that store can remain stranded in the pending queue. Consider capturing `scheduler` and `drainKey` so the early-return path calls `finishPendingStoreDrain`, or clearing active drains when replacing databases.
| if _pending_replay_active: | ||
| return | ||
| _pending_replay_active = True | ||
|
|
There was a problem hiding this comment.
🟠 High mcp/store_handler.py:711
_schedule_pending_store_replay sets _pending_replay_active = True under the lock, but if threading.Thread(...).start() raises (e.g., the runtime cannot create another thread), the flag is never reset. Every later call returns early at the if _pending_replay_active check, so acknowledged fallback entries stay stranded for the rest of the process lifetime. Move the threading.Thread(...).start() call inside a try that resets _pending_replay_active on failure.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/mcp/store_handler.py around line 711:
`_schedule_pending_store_replay` sets `_pending_replay_active = True` under the lock, but if `threading.Thread(...).start()` raises (e.g., the runtime cannot create another thread), the flag is never reset. Every later call returns early at the `if _pending_replay_active` check, so acknowledged fallback entries stay stranded for the rest of the process lifetime. Move the `threading.Thread(...).start()` call inside a `try` that resets `_pending_replay_active` on failure.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@brain-bar/Sources/BrainBar/MCPRouter.swift`:
- Around line 360-387: Update scheduleIdentitylessLegacyFlush to use
pendingStoreDrainMaxDelay instead of the literal 60-second cap. Track the prior
queue depth across retries and only re-arm when the latest flush reduces depth;
when consecutive attempts leave depth unchanged, stop scheduling and emit a
single diagnostic log. Preserve the existing handling for unreadable snapshots
and successful queue draining.
In `@brain-bar/Tests/BrainBarTests/MCPRouterTests.swift`:
- Around line 2552-2558: The test setup around startupDrainQueue must explicitly
wait for the first pending-store scan attempt before restoring queuePath.
Coordinate the scan with a semaphore or equivalent hook signaled after the
directory replacement, release the queue, and wait until the drain-attempt
counter exceeds one before rewriting queuePath; apply the same deterministic
ordering to testDeferredBrainStoreDrainRetriesAfterTransientQueueReadFailure.
In `@tests/test_mcp_deferred_message.py`:
- Around line 75-89: In the UnlinkedPendingStorePath test stub, remove the
unused exists(), parent, and name members, and update any outdated docstring
wording about “between existence and size probes” to describe the current
stat-only probe. Keep the stat() behavior that raises FileNotFoundError and the
existing lock stub unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f56c206f-e266-49e3-b4f5-8e65123a5a3e
📒 Files selected for processing (4)
brain-bar/Sources/BrainBar/MCPRouter.swiftbrain-bar/Tests/BrainBarTests/MCPRouterTests.swiftsrc/brainlayer/mcp/store_handler.pytests/test_mcp_deferred_message.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: swift (macos-15)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: Macroscope - Correctness Check
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run
pytestbefore claiming behavior changes are safe; test data changes against a copy of the real database before merging, and do not let tests refresh the production backup heartbeat log.
Files:
tests/test_mcp_deferred_message.py
src/brainlayer/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/brainlayer/**/*.py: Resolve the database path throughpaths.py:get_db_path(); use the environment override or canonical~/.local/share/brainlayer/brainlayer.dbpath rather than hardcoding paths.
Serialize writes so only one write occurs at a time, allow concurrent reads, retrySQLITE_BUSY, and give each worker its own database connection.
Preserve source traceability: memories must be able to point back to their originating conversation, and pointers to the source of truth are preferred over duplicated copies.
Verify and perform the work before storing its result; update incorrect stored memories instead of creating duplicates. Standing rules must include their date and expiry.
Never silently degrade, never automatically delete personal data, and never package the user's database. Archive transcripts only after embedding and only when readers can still access all content.
Default search must exclude lifecycle-managed chunks;include_archived=Trueexposes history.brain_supersedemust apply a personal-data safety gate,brain_archivemust soft-delete with a timestamp, andbrain_storemust support atomic store-and-replace viasupersedes.
Use the documented MCP tool contracts and entrypointbrainlayer-mcp; preserve legacy aliases where required, and route deprecated Python-pathbrain_expandandbrain_tagscalls to the documented error behavior.
Use Groq as the primary enrichment backend, Gemini as fallback, and Ollama as the offline last resort; honorBRAINLAYER_ENRICH_BACKENDandBRAINLAYER_ENRICH_RATE.
Files:
src/brainlayer/mcp/store_handler.py
src/brainlayer/mcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
MCP search must use the fixed-size read-only WAL
VectorStorepool; respectBRAINLAYER_READ_POOL_SIZE,BRAINLAYER_READ_BUSY_TIMEOUT_MS, and reject configurations whose pool/cache memory exceeds approximately 768 MB.
Files:
src/brainlayer/mcp/store_handler.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format and lint Python with
ruff check src/andruff format src/.
Files:
src/brainlayer/mcp/store_handler.py
🧠 Learnings (3)
📚 Learning: 2026-07-20T07:44:40.216Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 606
File: brain-bar/Tests/BrainBarTests/BrainBarDashboardTruthPresentationTests.swift:170-179
Timestamp: 2026-07-20T07:44:40.216Z
Learning: For SwiftPM source-contract-style tests in the `brain-bar` package (e.g., under `brain-bar/Tests/**`), assume tests are executed from a full repo checkout using `swift test --package-path brain-bar`. These tests may rely on `#filePath`-based inspection of production Swift sources as part of that execution contract. Do not suggest copying production source files into test resources (e.g., bundling duplicates under the test target), since it duplicates sources and can cause drift from the real production implementation.
Applied to files:
brain-bar/Tests/BrainBarTests/MCPRouterTests.swift
📚 Learning: 2026-03-18T00:12:08.774Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 87
File: brain-bar/Sources/BrainBar/BrainBarServer.swift:118-129
Timestamp: 2026-03-18T00:12:08.774Z
Learning: In Swift files under brain-bar/Sources/BrainBar, enforce that when a critical dependency like the database is nil due to startup ordering (socket before DB), any tool handler that accesses the database must throw an explicit error (e.g., ToolError.noDatabase) instead of returning a default/empty value. Do not allow silent defaults (e.g., guard let db else { return ... }). Flag patterns that silently return defaults when db is nil, as this masks startup timing issues. This guidance applies broadly to similar Swift files in the BrainBar module, not just this one location.
Applied to files:
brain-bar/Sources/BrainBar/MCPRouter.swift
📚 Learning: 2026-03-29T18:45:40.988Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In the BrainBar module’s Swift database layer (notably BrainDatabase.swift), ensure that the `search()` function’s `unreadOnly=true` path orders results by the delivery frontier cursor so the watermark `maxRowID` stays contiguous. Specifically, when `unreadOnly` is enabled, the query must include `ORDER BY c.rowid ASC` (e.g., via `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`). Do not replace the unread-only ordering with relevance-based sorting (e.g., `f.rank`) unconditionally or for the unread-only path, as it can introduce gaps in the watermark and incorrectly mark unseen rows as delivered. Flag any future change to the `ORDER BY` clause in this function that makes relevance sorting apply to the unread-only case.
Applied to files:
brain-bar/Sources/BrainBar/MCPRouter.swift
🪛 SwiftLint (0.65.0)
brain-bar/Sources/BrainBar/MCPRouter.swift
[Warning] 138-138: Classes should have an explicit deinit method
(required_deinit)
🔇 Additional comments (13)
brain-bar/Sources/BrainBar/MCPRouter.swift (7)
28-28: 🗄️ Data Integrity & Integration | ⚡ Quick winCore description still omits the no-re-store guidance.
The core
brain_storedescription statesDEFERRED = stored.but does not prohibit re-storing or saving a fallback copy. A previous review requested that guidance at this exact line. The coretools/listbudget test asserts 1,500 bytes, so a short clause still fits.Proposed description update
- "brain_store": "Store memory; DEFERRED = stored.", + "brain_store": "Store memory; DEFERRED = stored, do not retry or copy.",
138-153: Static analysis reportsrequired_deinitforPendingStoreDrainScheduler. Sibling nested classes in the same file (PaletteSession,HybridSearchResultBox,PendingStoreDrainRegistry) also have nodeinit, so the rule appears unenforced for this file. No change requested.
1657-1657: 🗄️ Data Integrity & Integration | ⚡ Quick winThe full description still limits deferral to a busy database.
The text states the memory is queued "while the DB is busy". Deferral also occurs for
DB_NOT_OPEN(Lines 821 and 879) and, in the Python handler, forINTERACTIVE_PRIORITYandSCHEMA_FINGERPRINT_MISMATCH. A client that reads this description can conclude a non-busy deferral is not covered by the no-re-store rule. A previous review requested neutral wording at this line.Proposed wording
-A STORED (deferred) result is SUCCESS: the memory is durably queued while the DB is busy and the drain persists it automatically \u{2014} never call brain_store again for it and never save a fallback copy. +A STORED (deferred) result is SUCCESS: the memory is durably queued (the receipt names the reason) and the drain persists it automatically \u{2014} never call brain_store again for it and never save a fallback copy.
186-198: LGTM!
311-355: LGTM!
884-1001: LGTM!
1043-1043: LGTM!src/brainlayer/mcp/store_handler.py (3)
725-726: LGTM!
739-743: LGTM!
679-699: 🩺 Stability & AvailabilityStartup already tolerates pending-store probe failures.
src/brainlayer/mcp/__init__.pycallsrearm_stranded_pending_stores()inside atry/exceptblock, so probe failures do not abort MCP startup.brain-bar/Tests/BrainBarTests/MCPRouterTests.swift (3)
2115-2119: LGTM!Also applies to: 2215-2219, 2275-2279
2129-2171: LGTM!
2577-2628: LGTM!
| private static func scheduleIdentitylessLegacyFlush( | ||
| scheduler: PendingStoreDrainScheduler, | ||
| db: BrainDatabase, | ||
| delay: TimeInterval | ||
| ) { | ||
| scheduler.queue.asyncAfter(deadline: .now() + delay) { [weak scheduler, weak db] in | ||
| guard let scheduler, let db, db.isOpen else { return } | ||
| _ = db.flushPendingStores( | ||
| busyTimeoutMillis: mcpStoreBusyTimeoutMillis, | ||
| retries: mcpStoreRetries | ||
| ) | ||
| guard let after = db.pendingStoreQueueSnapshotIfReadable() else { | ||
| scheduleIdentitylessLegacyFlush( | ||
| scheduler: scheduler, | ||
| db: db, | ||
| delay: min(delay * 2, 60) | ||
| ) | ||
| return | ||
| } | ||
| if after.depth > 0 { | ||
| scheduleIdentitylessLegacyFlush( | ||
| scheduler: scheduler, | ||
| db: db, | ||
| delay: min(delay * 2, 60) | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Bound the identityless legacy flush loop, and reuse the named delay cap.
Two issues in this retry path:
min(delay * 2, 60)uses a literal cap. Every other backoff in this file usespendingStoreDrainMaxDelay(30.0). Use the constant so the delay policy stays consistent.- The loop re-arms whenever
after.depth > 0.flushPendingStoresretains lines it cannot decode (remaining.append(line)in the decodecatch, BrainDatabase.swift Lines 1638-1642). One permanently malformed legacy line therefore keeps this recursion alive for the process lifetime. Each attempt takes the cross-process queue lock and rewrites the queue file.
Stop re-arming when consecutive attempts do not reduce depth, and log once at that point.
♻️ Proposed change
private static func scheduleIdentitylessLegacyFlush(
scheduler: PendingStoreDrainScheduler,
db: BrainDatabase,
- delay: TimeInterval
+ delay: TimeInterval,
+ lastDepth: Int = Int.max
) {
scheduler.queue.asyncAfter(deadline: .now() + delay) { [weak scheduler, weak db] in
guard let scheduler, let db, db.isOpen else { return }
_ = db.flushPendingStores(
busyTimeoutMillis: mcpStoreBusyTimeoutMillis,
retries: mcpStoreRetries
)
guard let after = db.pendingStoreQueueSnapshotIfReadable() else {
scheduleIdentitylessLegacyFlush(
scheduler: scheduler,
db: db,
- delay: min(delay * 2, 60)
+ delay: min(delay * 2, pendingStoreDrainMaxDelay),
+ lastDepth: lastDepth
)
return
}
- if after.depth > 0 {
+ guard after.depth > 0 else { return }
+ if after.depth >= lastDepth {
+ NSLog("[BrainBar] Legacy pending store queue stalled at depth %d; stopping retries", after.depth)
+ return
+ }
scheduleIdentitylessLegacyFlush(
scheduler: scheduler,
db: db,
- delay: min(delay * 2, 60)
+ delay: min(delay * 2, pendingStoreDrainMaxDelay),
+ lastDepth: after.depth
)
- }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static func scheduleIdentitylessLegacyFlush( | |
| scheduler: PendingStoreDrainScheduler, | |
| db: BrainDatabase, | |
| delay: TimeInterval | |
| ) { | |
| scheduler.queue.asyncAfter(deadline: .now() + delay) { [weak scheduler, weak db] in | |
| guard let scheduler, let db, db.isOpen else { return } | |
| _ = db.flushPendingStores( | |
| busyTimeoutMillis: mcpStoreBusyTimeoutMillis, | |
| retries: mcpStoreRetries | |
| ) | |
| guard let after = db.pendingStoreQueueSnapshotIfReadable() else { | |
| scheduleIdentitylessLegacyFlush( | |
| scheduler: scheduler, | |
| db: db, | |
| delay: min(delay * 2, 60) | |
| ) | |
| return | |
| } | |
| if after.depth > 0 { | |
| scheduleIdentitylessLegacyFlush( | |
| scheduler: scheduler, | |
| db: db, | |
| delay: min(delay * 2, 60) | |
| ) | |
| } | |
| } | |
| } | |
| private static func scheduleIdentitylessLegacyFlush( | |
| scheduler: PendingStoreDrainScheduler, | |
| db: BrainDatabase, | |
| delay: TimeInterval, | |
| lastDepth: Int = Int.max | |
| ) { | |
| scheduler.queue.asyncAfter(deadline: .now() + delay) { [weak scheduler, weak db] in | |
| guard let scheduler, let db, db.isOpen else { return } | |
| _ = db.flushPendingStores( | |
| busyTimeoutMillis: mcpStoreBusyTimeoutMillis, | |
| retries: mcpStoreRetries | |
| ) | |
| guard let after = db.pendingStoreQueueSnapshotIfReadable() else { | |
| scheduleIdentitylessLegacyFlush( | |
| scheduler: scheduler, | |
| db: db, | |
| delay: min(delay * 2, pendingStoreDrainMaxDelay), | |
| lastDepth: lastDepth | |
| ) | |
| return | |
| } | |
| guard after.depth > 0 else { return } | |
| if after.depth >= lastDepth { | |
| NSLog("[BrainBar] Legacy pending store queue stalled at depth %d; stopping retries", after.depth) | |
| return | |
| } | |
| scheduleIdentitylessLegacyFlush( | |
| scheduler: scheduler, | |
| db: db, | |
| delay: min(delay * 2, pendingStoreDrainMaxDelay), | |
| lastDepth: after.depth | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@brain-bar/Sources/BrainBar/MCPRouter.swift` around lines 360 - 387, Update
scheduleIdentitylessLegacyFlush to use pendingStoreDrainMaxDelay instead of the
literal 60-second cap. Track the prior queue depth across retries and only
re-arm when the latest flush reduces depth; when consecutive attempts leave
depth unchanged, stop scheduling and emit a single diagnostic log. Preserve the
existing handling for unreadable snapshots and successful queue draining.
| let startupDrainQueue = DispatchQueue(label: "test.pending-store-startup-retry") | ||
| let router = MCPRouter(profile: "full", pendingStoreDrainQueue: startupDrainQueue) | ||
| router.setDatabase(db) | ||
| startupDrainQueue.sync {} | ||
|
|
||
| try FileManager.default.removeItem(at: queuePath) | ||
| try queuedText.write(to: queuePath, atomically: true, encoding: .utf8) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
startupDrainQueue.sync {} does not guarantee the first scan ran.
scheduleExistingPendingStoreScan submits the first attempt with asyncAfter(deadline: .now() + 0) (MCPRouter.swift Lines 316 and 324). A zero-delay asyncAfter is delivered through a timer source, so it is not guaranteed to be enqueued before the sync barrier at Line 2555 executes. If sync returns first, Lines 2557-2558 restore a readable queue before any snapshot attempt, the first scan succeeds, and the retry path this test names is never exercised. The assertion at Line 2570 still passes.
The PR notes a CI failure for the sibling test testDeferredBrainStoreDrainRetriesAfterTransientQueueReadFailure, which depends on the same kind of timing assumption. Make the ordering explicit instead of relying on queue scheduling.
Gate the restore on an observed first attempt. For example, block startupDrainQueue with a semaphore that you signal only after the directory replacement is in place, then release it and wait for a drain-attempt counter to exceed one before restoring the file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@brain-bar/Tests/BrainBarTests/MCPRouterTests.swift` around lines 2552 - 2558,
The test setup around startupDrainQueue must explicitly wait for the first
pending-store scan attempt before restoring queuePath. Coordinate the scan with
a semaphore or equivalent hook signaled after the directory replacement, release
the queue, and wait until the drain-attempt counter exceeds one before rewriting
queuePath; apply the same deterministic ordering to
testDeferredBrainStoreDrainRetriesAfterTransientQueueReadFailure.
| class UnlinkedPendingStorePath: | ||
| parent = tmp_path | ||
| name = "pending-stores.jsonl" | ||
|
|
||
| @staticmethod | ||
| def exists() -> bool: | ||
| return True | ||
|
|
||
| @staticmethod | ||
| def stat(): | ||
| raise FileNotFoundError("concurrent replay removed pending-stores.jsonl") | ||
|
|
||
| calls = [] | ||
| monkeypatch.setattr(store_handler, "_get_pending_store_path", UnlinkedPendingStorePath) | ||
| monkeypatch.setattr(store_handler, "_pending_store_file_lock", lambda _path: nullcontext()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove the stub members the probe no longer uses.
_pending_store_has_data calls path.stat() directly; it never calls exists() (store_handler.py Lines 685-689). With _pending_store_file_lock stubbed to nullcontext(), parent and name are also unused. The three unused members and the docstring phrase "between existence and size probes" describe the older two-step probe.
♻️ Proposed cleanup
-def test_startup_replay_probe_tolerates_concurrent_queue_unlink(monkeypatch, tmp_path) -> None:
- """A queue removed between existence and size probes must not kill replay recovery."""
+def test_startup_replay_probe_tolerates_concurrent_queue_unlink(monkeypatch) -> None:
+ """A queue removed before the size probe must not kill replay recovery."""
from contextlib import nullcontext
from brainlayer.mcp import store_handler
class UnlinkedPendingStorePath:
- parent = tmp_path
- name = "pending-stores.jsonl"
-
- `@staticmethod`
- def exists() -> bool:
- return True
-
`@staticmethod`
def stat():
raise FileNotFoundError("concurrent replay removed pending-stores.jsonl")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class UnlinkedPendingStorePath: | |
| parent = tmp_path | |
| name = "pending-stores.jsonl" | |
| @staticmethod | |
| def exists() -> bool: | |
| return True | |
| @staticmethod | |
| def stat(): | |
| raise FileNotFoundError("concurrent replay removed pending-stores.jsonl") | |
| calls = [] | |
| monkeypatch.setattr(store_handler, "_get_pending_store_path", UnlinkedPendingStorePath) | |
| monkeypatch.setattr(store_handler, "_pending_store_file_lock", lambda _path: nullcontext()) | |
| def test_startup_replay_probe_tolerates_concurrent_queue_unlink(monkeypatch) -> None: | |
| """A queue removed before the size probe must not kill replay recovery.""" | |
| from contextlib import nullcontext | |
| from brainlayer.mcp import store_handler | |
| class UnlinkedPendingStorePath: | |
| `@staticmethod` | |
| def stat(): | |
| raise FileNotFoundError("concurrent replay removed pending-stores.jsonl") | |
| calls = [] | |
| monkeypatch.setattr(store_handler, "_get_pending_store_path", UnlinkedPendingStorePath) | |
| monkeypatch.setattr(store_handler, "_pending_store_file_lock", lambda _path: nullcontext()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_mcp_deferred_message.py` around lines 75 - 89, In the
UnlinkedPendingStorePath test stub, remove the unused exists(), parent, and name
members, and update any outdated docstring wording about “between existence and
size probes” to describe the current stat-only probe. Keep the stat() behavior
that raises FileNotFoundError and the existing lock stub unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 193b3dbcb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| from .store_handler import rearm_stranded_pending_stores | ||
|
|
||
| rearm_stranded_pending_stores() |
There was a problem hiding this comment.
When a second Python MCP process starts while another process is flushing pending-stores.jsonl, this synchronous call blocks on _pending_store_file_lock; the flusher holds that same exclusive flock across every database write in the queue. Because stdio_server() is not entered until afterward, a large or contended replay can prevent the new process from answering the MCP initialization handshake and cause client startup timeouts. Run this probe in the background or use a nonblocking lock attempt.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
Keep router instances alive for the full assertion window in async drain tests. The production scheduler is intentionally weakly captured so retries end with their router; tests must model the long-lived server owner explicitly. Co-Authored-By: brainlayerCodex running gpt-5.6-sol <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d8baed90-3a70-46ee-a029-6883ab75dad9) |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Agents kept re-storing and writing local fallback files when brain_store returned
DEFERRED: Memory queued (DB busy)— the receipt sounded like a failure (live specimen: maintenanceCodex today wrote a fallback decisions .md and set retry_attempted). #641 made deferred stores idempotent; this makes the words match the semantics.Changes
Formatters.swift+ Pythonmcp/_format.py): now✔ STORED (deferred): DB busy → <id> ─ durably queued; the drain persists it automatically. Do NOT re-store or save a fallback copy.brain_storetool description (MCPRouter.swift): states a DEFERRED result is SUCCESS.status: "DEFERRED"field unchanged (machine-readable contract intact — the retry-idempotency tests still pass unmodified).tests/test_mcp_deferred_message.pycontract test; updated 3 store-handler + 4 Swift assertion sites.Verification: full pre-push gate green on push; Swift suite 853 tests, 0 unexpected failures; ruff clean.
Deploy note (merged ≠ deployed): BrainBar must be rebuilt+restarted for the Swift path; the Python MCP server picks it up on next restart.
Tiny lead self-edit, disclosed in the wave25 collab. @codex review
🤖 Generated with Claude Code
Note
Medium Risk
Changes touch durable-write semantics, queue retention, and async drain/replay across Swift and Python MCP paths; mis-scheduling could strand or duplicate stores, though coverage was expanded for injection, startup retry, and scheduler isolation.
Overview
Agents were re-storing and writing fallback files because deferred
brain_storetext sounded like failure. Human-readable receipts in BrainBar (Formatters.swift) and Python (mcp/_format.py) now use✔ STORED (deferred): <reason>, propagate reasons likeDB_BUSY,DB_NOT_OPEN, andSCHEMA_FINGERPRINT_MISMATCH, and tell callers not to re-store or keep a fallback copy.brain_storetool copy inMCPRoutermatches that contract; structuredstatus: "DEFERRED"is unchanged.Pending-store reliability gets a parallel set of fixes. Swift
MCPRoutergives each router its own drain scheduler (no shared static queue), and onsetDatabasesscans the queue to schedule drains for stores that queued before the DB opened, with backoff when snapshots are unreadable and a legacy flush for identity-less entries. Pythonstore_handlerstops trimmingpending-stores.jsonlpast the soft limit (acknowledged lines must not be dropped), arms background replay for the legacy queue path, andrearm_stranded_pending_stores()at MCPserve()startup.Tests lock the new receipt strings and drain/replay behavior in Swift and Python.
Reviewed by Cursor Bugbot for commit 03396fb. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix deferred
brain_storereceipts to read as success and add background replay drain✔ STORED (deferred): <reason>instead ofDEFERRED: Memory queued (DB busy), signaling to callers that the write is durably queued and will be persisted automatically.DB_BUSYorSCHEMA_FINGERPRINT_MISMATCH, surfaced in both structured receipts and user-facing text via updatedformat_store_result/Formatters.formatStoreResult.pending-stores.jsonlqueue with exponential backoff until empty, self-rearming if new entries appear after it finishes.rearm_stranded_pending_storesto re-arm replay if the legacy queue has content from a previous session.MCPRouteris moved from global static state to a per-routerPendingStoreDrainScheduler, and drains are automatically armed when databases are injected viasetDatabases.Macroscope summarized 03396fb.
Summary by CodeRabbit