fix: fall back to FTS when search embeddings stall#524
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThe PR adds configurable embedding timeout and per-process concurrency locks to both the hook script and MCP search handler. When embedding exceeds the timeout or a concurrent embed is already running, both paths fall back to FTS-only search. ChangesEmbedding timeout and FTS-only fallback
Sequence Diagram(s)sequenceDiagram
participant Client
participant _search
participant _EMBED_IN_FLIGHT as _EMBED_IN_FLIGHT (Lock)
participant EmbeddingModel
participant hybrid_search
participant search_profile
Client->>`_search`: query
rect rgba(100, 180, 255, 0.5)
Note over _search,_EMBED_IN_FLIGHT: Concurrency guard
`_search`->>`_EMBED_IN_FLIGHT`: acquire(blocking=False)
end
alt Lock acquired + embedding succeeds within timeout
`_search`->>EmbeddingModel: embed(query)
EmbeddingModel-->>`_search`: vector
`_search`->>search_profile: emit embed_success
`_search`->>hybrid_search: query_embedding=vector
else Lock busy
`_search`->>search_profile: emit embed_busy
`_search`->>hybrid_search: query_embedding=None
else Timeout exceeded
`_search`->>search_profile: emit embed_timeout
`_search`->>hybrid_search: query_embedding=None
else Embedding raises
`_search`->>search_profile: emit embed_error
`_search`->>hybrid_search: query_embedding=None
end
hybrid_search-->>`_search`: results
`_search`-->>Client: {search_mode, fallback_reason?, results}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
@codex review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and list of changed files
gh pr view 524 --repo EtanHey/brainlayer --json title,body,files,additions,deletions,changedFiles
echo "=== CHANGED FILES ==="
gh pr diff 524 --repo EtanHey/brainlayer --name-onlyLength of output: 4748 🏁 Script executed: #!/bin/bash
# Get the full diff
gh pr diff 524 --repo EtanHey/brainlayerLength of output: 23977
Review:
|
| Severity | Issue |
|---|---|
| 🔴 Bug | except TimeoutError misses asyncio.TimeoutError on Python < 3.11 |
| 🟡 Minor | CancelledError leaves lock held until background thread finishes |
| 🟡 Minor | results.get_nowait() edge case in hook |
| 🟡 Minor | 1 ms test timeout may be flaky under CI load |
…fallback-pr2 # Conflicts: # hooks/brainlayer-prompt-search.py # src/brainlayer/mcp/search_handler.py # tests/test_adaptive_injection.py # tests/test_search_handler.py
![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: 07abd6df82
ℹ️ 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".
| except Exception: | ||
| _EMBED_IN_FLIGHT.release() | ||
| raise | ||
| query_embedding = await asyncio.wait_for(embed_future, timeout=embed_timeout_ms / 1000.0) |
There was a problem hiding this comment.
If the default executor is saturated, run_in_executor can leave this embed job queued until wait_for times out; wait_for then cancels the queued future before _embed_query_and_release() ever runs, so its finally never releases _EMBED_IN_FLIGHT. After that, every later brain_search in this MCP process sees embed_busy and permanently falls back to FTS-only until restart, even after the executor drains.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![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 `@hooks/brainlayer-prompt-search.py`:
- Around line 16-20: The embed_timeout_ms() function at line 57 does not
properly validate non-finite timeout values like NaN, which causes thread.join()
to fail later at line 78 with a ValueError. Add the math module to the imports
at the top of the file, then update the validation logic in embed_timeout_ms()
to use math.isfinite() to reject non-finite values (NaN, infinity) at validation
time rather than allowing them to pass and fail later during thread.join()
execution.
In `@tests/test_adaptive_injection.py`:
- Around line 183-184: Replace the fixed time.sleep(0.06) calls in the finally
block on line 184 and the similar call on line 220 with a bounded polling loop
that explicitly checks for the HYBRID_IN_FLIGHT flag to be unlocked. This
ensures proper synchronization on actual lock state rather than relying on
timing assumptions, eliminating flakiness caused by variable scheduler timing.
Use a reasonable timeout limit for the polling loop to prevent indefinite waits.
In `@tests/test_search_handler.py`:
- Around line 216-217: Replace the fixed `await asyncio.sleep(0.06)` calls in
the finally blocks (at both the first occurrence and the second one also
mentioned at lines 288-289) with deterministic polling logic that waits for the
`_EMBED_IN_FLIGHT` lock or semaphore to be released with a deadline instead of
sleeping for a constant duration. This ensures that cleanup waits for the actual
lock release event rather than hoping a fixed sleep is long enough, preventing
test flakiness on slower CI systems where `_EMBED_IN_FLIGHT` might leak into
following tests.
🪄 Autofix (Beta)
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
Run ID: 943c6e16-1046-45a2-96dd-8d733750e0c6
📒 Files selected for processing (6)
hooks/brainlayer-prompt-search.pysrc/brainlayer/mcp/search_handler.pysrc/brainlayer/search_repo.pytests/test_adaptive_injection.pytests/test_hybrid_search.pytests/test_search_handler.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Macroscope - Correctness Check
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Flag risky DB or concurrency changes explicitly and do not hand-wave lock behavior
Enforce one-write-at-a-time concurrency constraint; reads are safe but brain_digest is write-heavy and must not run in parallel with other MCP work
Run pytest before claiming behavior changed safely; current test suite has 929 tests
Files:
tests/test_hybrid_search.pyhooks/brainlayer-prompt-search.pytests/test_adaptive_injection.pysrc/brainlayer/search_repo.pysrc/brainlayer/mcp/search_handler.pytests/test_search_handler.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Run tests with
pytestand useBRAINLAYER_PREPUSH=1for pre-push validation
Files:
tests/test_hybrid_search.pytests/test_adaptive_injection.pytests/test_search_handler.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Run linting with
ruff check src/ && ruff format src/on all Python source files
Files:
src/brainlayer/search_repo.pysrc/brainlayer/mcp/search_handler.py
src/brainlayer/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Resolve database path using
paths.py:get_db_path()with env var override; canonical path is~/.local/share/brainlayer/brainlayer.db
Files:
src/brainlayer/search_repo.pysrc/brainlayer/mcp/search_handler.py
src/brainlayer/**/*search*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Exclude lifecycle-managed chunks from default search; support
include_archived=Trueto show history
Files:
src/brainlayer/search_repo.pysrc/brainlayer/mcp/search_handler.py
🪛 ast-grep (0.43.0)
tests/test_adaptive_injection.py
[info] 173-173: Do not hardcode temporary file or directory names
Context: "/tmp/test.db"
Note: [CWE-377].
(hardcoded-tmp-file)
[info] 202-202: Do not hardcode temporary file or directory names
Context: "/tmp/test.db"
Note: [CWE-377].
(hardcoded-tmp-file)
[info] 208-208: Do not hardcode temporary file or directory names
Context: "/tmp/test.db"
Note: [CWE-377].
(hardcoded-tmp-file)
[info] 231-231: Do not hardcode temporary file or directory names
Context: "/tmp/test.db"
Note: [CWE-377].
(hardcoded-tmp-file)
🔇 Additional comments (5)
tests/test_hybrid_search.py (1)
220-236: LGTM!src/brainlayer/search_repo.py (1)
1714-1714: LGTM!Also applies to: 1813-1825
hooks/brainlayer-prompt-search.py (1)
39-40: LGTM!Also applies to: 62-85, 988-988
src/brainlayer/mcp/search_handler.py (2)
38-39: LGTM!Also applies to: 77-81, 1792-1840, 1861-1889, 1917-1923, 1953-1975, 1977-1979, 2050-2061
10-10: No action required. The current_embed_timeout_ms()validation at lines 66-74 already correctly rejects NaN. The conditionif not value or value < 0:evaluates toFalsewhenvalueis NaN (sincenot float('nan')isFalseandfloat('nan') < 0isFalse), preventing NaN from being passed toasyncio.wait_for(). The code does not have the vulnerability described in the review comment.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@hooks/brainlayer-prompt-search.py`:
- Around line 16-20: The embed_timeout_ms() function at line 57 does not
properly validate non-finite timeout values like NaN, which causes thread.join()
to fail later at line 78 with a ValueError. Add the math module to the imports
at the top of the file, then update the validation logic in embed_timeout_ms()
to use math.isfinite() to reject non-finite values (NaN, infinity) at validation
time rather than allowing them to pass and fail later during thread.join()
execution.
In `@tests/test_adaptive_injection.py`:
- Around line 183-184: Replace the fixed time.sleep(0.06) calls in the finally
block on line 184 and the similar call on line 220 with a bounded polling loop
that explicitly checks for the HYBRID_IN_FLIGHT flag to be unlocked. This
ensures proper synchronization on actual lock state rather than relying on
timing assumptions, eliminating flakiness caused by variable scheduler timing.
Use a reasonable timeout limit for the polling loop to prevent indefinite waits.
In `@tests/test_search_handler.py`:
- Around line 216-217: Replace the fixed `await asyncio.sleep(0.06)` calls in
the finally blocks (at both the first occurrence and the second one also
mentioned at lines 288-289) with deterministic polling logic that waits for the
`_EMBED_IN_FLIGHT` lock or semaphore to be released with a deadline instead of
sleeping for a constant duration. This ensures that cleanup waits for the actual
lock release event rather than hoping a fixed sleep is long enough, preventing
test flakiness on slower CI systems where `_EMBED_IN_FLIGHT` might leak into
following tests.
🪄 Autofix (Beta)
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
Run ID: 943c6e16-1046-45a2-96dd-8d733750e0c6
📒 Files selected for processing (6)
hooks/brainlayer-prompt-search.pysrc/brainlayer/mcp/search_handler.pysrc/brainlayer/search_repo.pytests/test_adaptive_injection.pytests/test_hybrid_search.pytests/test_search_handler.py
📜 Review details
🔇 Additional comments (5)
tests/test_hybrid_search.py (1)
220-236: LGTM!src/brainlayer/search_repo.py (1)
1714-1714: LGTM!Also applies to: 1813-1825
hooks/brainlayer-prompt-search.py (1)
39-40: LGTM!Also applies to: 62-85, 988-988
src/brainlayer/mcp/search_handler.py (2)
38-39: LGTM!Also applies to: 77-81, 1792-1840, 1861-1889, 1917-1923, 1953-1975, 1977-1979, 2050-2061
10-10: No action required. The current_embed_timeout_ms()validation at lines 66-74 already correctly rejects NaN. The conditionif not value or value < 0:evaluates toFalsewhenvalueis NaN (sincenot float('nan')isFalseandfloat('nan') < 0isFalse), preventing NaN from being passed toasyncio.wait_for(). The code does not have the vulnerability described in the review comment.
🛑 Comments failed to post (3)
hooks/brainlayer-prompt-search.py (1)
16-20:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash python - <<'PY' import math import threading value = float("nan") print("current_checks_allow_nan=", bool(value) and not (value < 0)) print("isfinite=", math.isfinite(value)) thread = threading.Thread(target=lambda: None) thread.start() try: thread.join(value) print("join accepted nan") except Exception as exc: print(type(exc).__name__, str(exc)) PYRepository: EtanHey/brainlayer
Length of output: 153
🏁 Script executed:
cat -n hooks/brainlayer-prompt-search.py | sed -n '16,20p; 39,60p; 62,90p; 985,995p'Repository: EtanHey/brainlayer
Length of output: 2652
🏁 Script executed:
rg "BRAINLAYER_EMBED_TIMEOUT_MS|thread\.join" hooks/brainlayer-prompt-search.py -n -A 2 -B 2Repository: EtanHey/brainlayer
Length of output: 487
Reject non-finite timeout values in
embed_timeout_ms().The validation at line 57 allows
BRAINLAYER_EMBED_TIMEOUT_MS=nanto pass (sincenot float('nan')is False andfloat('nan') < 0is False), butthread.join()at line 78 will raiseValueError: Invalid value NaN (not a number). Although the exception is caught and the search degrades to FTS-only, this should be rejected at validation time like other malformed values.Use
math.isfinite()to reject non-finite values:Proposed fix
+import math import queue import re import sqlite3 import sys import threadingdef embed_timeout_ms(): raw = os.environ.get("BRAINLAYER_EMBED_TIMEOUT_MS", str(DEFAULT_EMBED_TIMEOUT_MS)) try: value = float(raw) except (TypeError, ValueError): return DEFAULT_EMBED_TIMEOUT_MS - if not value or value < 0: + if not math.isfinite(value) or value <= 0: return DEFAULT_EMBED_TIMEOUT_MS return min(value, 30_000.0)🤖 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 `@hooks/brainlayer-prompt-search.py` around lines 16 - 20, The embed_timeout_ms() function at line 57 does not properly validate non-finite timeout values like NaN, which causes thread.join() to fail later at line 78 with a ValueError. Add the math module to the imports at the top of the file, then update the validation logic in embed_timeout_ms() to use math.isfinite() to reject non-finite values (NaN, infinity) at validation time rather than allowing them to pass and fail later during thread.join() execution.tests/test_adaptive_injection.py (1)
183-184:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid fixed
sleepfor lock synchronization in timeout tests.Line 184 and Line 220 use fixed delays to “let the worker finish”; this is brittle under variable scheduler timing and can make timeout/concurrency assertions flaky. Wait for
HYBRID_IN_FLIGHTto unlock with a bounded polling loop.As per coding guidelines, "Flag risky DB or concurrency changes explicitly and do not hand-wave lock behavior."
Suggested patch
@@ import time @@ class TestSearchFallback: + def _wait_for_hybrid_release(self, prompt_search, timeout_s: float = 1.0): + deadline = time.monotonic() + timeout_s + while prompt_search.HYBRID_IN_FLIGHT.locked() and time.monotonic() < deadline: + time.sleep(0.01) + assert not prompt_search.HYBRID_IN_FLIGHT.locked() + @@ - finally: - time.sleep(0.06) + finally: + self._wait_for_hybrid_release(prompt_search) @@ - finally: - time.sleep(0.25) + finally: + self._wait_for_hybrid_release(prompt_search)Also applies to: 219-220
🤖 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_adaptive_injection.py` around lines 183 - 184, Replace the fixed time.sleep(0.06) calls in the finally block on line 184 and the similar call on line 220 with a bounded polling loop that explicitly checks for the HYBRID_IN_FLIGHT flag to be unlocked. This ensures proper synchronization on actual lock state rather than relying on timing assumptions, eliminating flakiness caused by variable scheduler timing. Use a reasonable timeout limit for the polling loop to prevent indefinite waits.Source: Coding guidelines
tests/test_search_handler.py (1)
216-217:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace fixed cleanup sleeps with deterministic lock-release waiting.
On Line 217 and Line 289, fixed
await asyncio.sleep(...)can still race on slower CI and leak_EMBED_IN_FLIGHTinto following tests, causing flakyembed_busyoutcomes. Poll lock release with a deadline instead of sleeping a constant duration.As per coding guidelines, "Flag risky DB or concurrency changes explicitly and do not hand-wave lock behavior."
Suggested patch
@@ import asyncio import time @@ from brainlayer.mcp import call_tool from brainlayer.mcp.search_handler import _brain_search, _search +import brainlayer.mcp.search_handler as search_handler_module + + +async def _wait_for_embed_release(timeout_s: float = 1.0) -> None: + deadline = time.monotonic() + timeout_s + while search_handler_module._EMBED_IN_FLIGHT.locked() and time.monotonic() < deadline: + await asyncio.sleep(0.01) + assert not search_handler_module._EMBED_IN_FLIGHT.locked() @@ - finally: - await asyncio.sleep(0.06) + finally: + await _wait_for_embed_release() @@ - finally: - await asyncio.sleep(0.25) + finally: + await _wait_for_embed_release()Also applies to: 288-289
🤖 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_search_handler.py` around lines 216 - 217, Replace the fixed `await asyncio.sleep(0.06)` calls in the finally blocks (at both the first occurrence and the second one also mentioned at lines 288-289) with deterministic polling logic that waits for the `_EMBED_IN_FLIGHT` lock or semaphore to be released with a deadline instead of sleeping for a constant duration. This ensures that cleanup waits for the actual lock release event rather than hoping a fixed sleep is long enough, preventing test flakiness on slower CI systems where `_EMBED_IN_FLIGHT` might leak into following tests.Source: Coding guidelines
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?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: 275aa88f61
ℹ️ 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 not _EMBED_IN_FLIGHT.acquire(blocking=False): | ||
| search_mode = "fts_only" | ||
| fallback_reason = "embed_timeout" | ||
| fallback_reason = "embed_busy" |
There was a problem hiding this comment.
When two brain_search/_search calls overlap, the second call hits this non-blocking acquire while the first embedding is still running, even if that first embed is fast and still within BRAINLAYER_EMBED_TIMEOUT_MS. It immediately marks fts_only/embed_busy and passes query_embedding=None to hybrid_search, so concurrent read traffic loses semantic retrieval instead of waiting within its own timeout; only downgrade after the in-flight embed has actually timed out/stalled or after a bounded wait.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Latest follow-up pushed in Verification after that commit:
Review notes:
@codex review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check Python version requirement in pyproject.toml
grep -n "python" pyproject.toml | head -20
echo "=== Latest commit on PR branch ==="
git log --oneline -5
echo "=== search_handler.py - key sections ==="
grep -n "TimeoutError\|asyncio\|release_if_embed_never_started\|CancelledError\|EMBED_IN_FLIGHT\|non.finite\|finite\|math\.isfinite\|isinf\|isnan" src/brainlayer/mcp/search_handler.py
echo "=== hook file - key sections ==="
grep -n "TimeoutError\|HYBRID_IN_FLIGHT\|release\|get_nowait\|Empty\|non.finite\|isfinite" hooks/brainlayer-prompt-search.py
echo "=== New test for lock release ==="
grep -n "release_if_embed_never_started\|executor_timeout_cancels\|lock_state\|acquire\|locked" tests/test_search_handler.py | head -30Length of output: 3075
All four issues are resolved or correctly invalidated. The implementation looks solid. [approve] |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 93a3fec. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93a3fec71c
ℹ️ 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".
| except Exception: | ||
| release_embed_lock_once() | ||
| raise | ||
| query_embedding = await asyncio.wait_for(embed_future, timeout=embed_timeout_ms / 1000.0) |
There was a problem hiding this comment.
When an MCP _search task is cancelled while run_in_executor has accepted the work but the default executor has not started embed_query_guarded yet (for example, a client disconnect/request timeout while the executor is saturated), asyncio.wait_for exits via CancelledError, which is not caught by except Exception on Python 3.11+. This path never calls release_if_embed_never_started(), and because the cancelled queued future will not run the worker finally, _EMBED_IN_FLIGHT remains locked so every subsequent search degrades to embed_busy/FTS-only until the process restarts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is addressed in the current 93caf0e diff: _search now has an explicit except asyncio.CancelledError: branch immediately after the wait_for, calls release_if_embed_never_started(), then re-raises. The regression test_brain_search_releases_embed_lock_when_cancelled_before_executor_starts reproduces exactly the queued-future cancellation case and failed before this change. Full pre-push passed after the fix: 3025 passed, 9 skipped, 61 deselected, 1 xfailed, plus MCP registration, isolated eval/hook routing, bun, and FTS determinism.
|
Follow-up pushed in What changed:
Verification after this commit:
@codex review |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify the changes in commit 93caf0e
gh pr view 524 --repo EtanHey/brainlayer --json headRefName,headRefOid
echo "=== Recent commits ==="
git log --oneline -5
echo "=== search_handler.py - cancellation handling ==="
grep -n "CancelledError\|release_if_embed_never_started\|BaseException\|finally\|cancel" src/brainlayer/mcp/search_handler.py | head -40
echo "=== New cancellation test ==="
grep -n "cancelled_before_executor_starts\|CancelledError\|cancel" tests/test_search_handler.py | head -30Length of output: 1755 🏁 Script executed: #!/bin/bash
echo "=== Cancellation handling block in search_handler.py ==="
sed -n '1810,1870p' src/brainlayer/mcp/search_handler.py
echo "=== New cancellation test implementation ==="
sed -n '335,395p' tests/test_search_handler.pyLength of output: 5405
All four issues from the initial review are fully resolved. The implementation looks solid — nice work iterating on this. [approve] |
|
Codex Review: Didn't find any major issues. Swish! 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
Summary
Tests
Supersedes #523 because GitHub did not synchronize #523 after the follow-up commit even though the branch ref advanced.
Note
Medium Risk
Changes core brain_search and prompt-hook retrieval concurrency and response shape (
search_mode,fallback_reason); lock bugs could degrade all searches to FTS-only, but behavior is covered by new timeout/busy/cancel tests.Overview
Prevents stacked embedding work when hybrid search or MCP
brain_searchis slow or times out. A process-wide lock means a second concurrent request skips vector embedding and uses FTS-only immediately (embed_busy) instead of starting another embedder call.The MCP
_searchpath wraps executor-based embedding with lock acquire/release on timeout, errors, and cancellation so a timed-out embed cannot leave the lock held and block later searches.BRAINLAYER_EMBED_TIMEOUT_MSnow rejects non-finite or non-positive values (defaults to 1000 ms). Empty no-hit responses includesearch_modeandfallback_reason, with user-facing text explaining FTS-only fallback when embedding was skipped.The prompt hook applies the same pattern via
HYBRID_IN_FLIGHTaround timed hybrid search so overlapping hook invocations do not spawn parallel hybrid workers. Tests cover parallel suppression, lock release edge cases, and timeout env validation.Reviewed by Cursor Bugbot for commit 93caf0e. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fall back to FTS when embedding is busy or times out in search handler
_EMBED_IN_FLIGHTlock insearch_handler.pyso only one embedding can run at a time; concurrent requests immediately fall back to FTS-only withfallback_reason='embed_busy'.asyncio.TimeoutError, the handler falls back to FTS-only withfallback_reason='embed_timeout'; on general exceptions, falls back withfallback_reason='embed_error:<ExceptionClass>'.BRAINLAYER_EMBED_TIMEOUT_MSvalidation to reject NaN/Inf and non-positive values, defaulting to 1000ms instead of using invalid values.search_modeandfallback_reasonmetadata; empty-result text reflects FTS-only fallback when applicable.Macroscope summarized 93caf0e.
Summary by CodeRabbit
Release Notes
New Features
Tests