test(write-queue): take the wall clock out of the brain_store retry test, and pin the budget that governs (XS) - #773
Merged
Conversation
…est, and pin the budget that governs `TestStoreRetryOnLock::test_store_retries_busy_error_before_queueing` failed on #762 attempt 1 (ubuntu, py3.13): `assert 1 == 3`, 1 failed / 4524 passed, with nothing from that PR reachable in 0 src lines. The test patched `_retry_delay` to 0.001 but had no control over the WALL-CLOCK deadline: `_store` sets one at now + BRAINLAYER_STORE_BUSY_BUDGET_MS (400ms default). On a loaded runner the budget expired after attempt 1 and the handler deferred instead of retrying three times -- "brain_store BusyError exceeded 400ms busy budget after attempt 1/4; deferring". The retry test now pins BRAINLAYER_STORE_BUSY_BUDGET_MS=60000: 60s cannot expire across three mocked attempts on any runner. It asserts the retry loop, so the clock is out of it. A new sibling asserts the other half -- that the budget, not the retry count, is what decides to defer, and that it is reachable from a test: budget 100ms with `_retry_delay` 0.3s, so the remaining budget cannot cover the next backoff and attempt 1 defers with status DEFERRED and one queue file. No src change: the env reader `_store_busy_budget_ms()` already reads BRAINLAYER_STORE_BUSY_BUDGET_MS on every call, so the budget was already injectable -- the test just never used it. Production defaults untouched. Evidence (mutation, not assertion): - New test with its `setenv` removed (400ms default, same 0.3s delay): `assert 2 == 1` -- it retries. The knob is what makes the deferral deterministic. - Retry test with its pin removed under BRAINLAYER_STORE_BUSY_BUDGET_MS=1: `assert 0 == 3` -- the reported flake, forced. With the pin, green under that same hostile env. - TestStoreRetryOnLock 30/30 green in random order; tests/test_write_queue.py 76 passed. - ruff check + ruff format --check clean. The brief suggested forcing the budget to 1ms; that is too small. At 1ms the deadline is already spent at `_remaining_store_busy_budget_ms` before `store_memory` is ever called (attempts == 0), so the test would pass for the wrong reason. 100ms is the smallest value that still reaches attempt 1. Co-Authored-By: Claude Opus 5 (1M context) <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_2f9faddf-4f38-43b6-9b69-0b64194e3032) |
|
Warning Review limit reachedNext included review available in 14 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Sep 5, 2026 3:21p.m. | Review ↗ |
| Swift | |
Sep 5, 2026 3:21p.m. | Review ↗ |
| JavaScript | |
Sep 5, 2026 3:21p.m. | Review ↗ |
| Shell | |
Sep 5, 2026 3:21p.m. | Review ↗ |
| Secrets | |
Sep 5, 2026 3:21p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
BrainLayer ratchetEvery Value below was measured by this run. A row this machine cannot measure says
🟢 GREEN measured, within budget · 🔴 RED measured, out of budget — a finding to clear before merge · ⚪ n/a not measurable on this machine, never guessed. 1 RED row(s) to clear: Measured on Linux/x86_64 · measured |
…it spends no wall clock at all Round-1 review low on #773 (ACCEPT + optional) — taken, because the PR's whole thesis is taking the wall clock out of this test, and leaving ~100ms of it in the replacement is the same class of defect one size down. With a real clock, a pathological stall between the deadline stamp and the first budget read (store_handler.py:1029-1030) spends the entire 100ms budget there, `_remaining_store_busy_budget_ms` raises before `store_memory` is ever called, and the store defers with attempts == 0 -- a pass for the wrong reason. `_frozen_monotonic` hands store_handler the real `time` module with `monotonic()` pinned to one value, so elapsed time cannot shrink the budget: every read answers 100ms and machine load is irrelevant. Everything else (`time.time`, `time.sleep`) delegates, so asyncio's own clock is untouched -- patching global `time.monotonic` would have frozen that too. Claim kept narrow, because I measured it: the freeze removes the CLOCK dependence, not the arithmetic. The remaining budget is `int((deadline - now) * 1000)`, and at a 1ms budget that float lands on 0 even frozen (int((p+0.001-p)*1000) == 0; at 400ms it is 399). So 100ms remains the floor, and the comment says so rather than implying the freeze licenses a smaller one. Evidence: - Still discriminates: with the budget knob removed (default 400) and the clock frozen, the test fails `assert 2 == 1` -- it retries. The knob is what makes the deferral the asserted behaviour. - TestStoreRetryOnLock 10/10 in random order, 0.60-0.80s; tests/test_write_queue.py 76 passed. - ruff check + ruff format clean. Full CI matrix was green on the previous head (3.11/3.12/3.13). Co-Authored-By: Claude Opus 5 (1M context) <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_3d83a177-b6d9-4f7a-9024-165acbd643df) |
Owner
Author
Round 1 — the low is taken, with the claim kept narrower than "clock-free"Fixed in Your read of the mechanism is right: with a real clock, a stall between the deadline stamp and the
One correction to my own framing, because I measured it rather than assuming it. The freeze Still discriminates: with the budget knob removed (default 400) and the clock frozen, the test
brainlayerClaude-19a28f09 running claude-opus-5. |
EtanHey
added a commit
that referenced
this pull request
Sep 5, 2026
…he PYL-W0404 reimports The DeepSource red on this PR was MINE, and my earlier comment on it was wrong. I inferred the blocking issues were a pre-existing "method doesn't use self" baseline; the run report says otherwise. The three blocking issues are: PYL-W0404 "Reimport 'watcher' (imported line 23)" -- tests/test_jsonl_watcher.py:85, 363, 521 Line 23 was the `import brainlayer.watcher` I added at module scope for the AST pins. Three pre-existing tests already reach the module the way this file has always done it -- `from brainlayer import watcher as watcher_module`, function-local -- and my top-level import turned all three into reimports. Cause: my change. Not a baseline. Both modules the pins parse are now imported inside the test that needs them, matching the file's own convention, and the module-level `import brainlayer.cli` / `import brainlayer.watcher` are gone. `from brainlayer.watcher import (...)` at module scope stays: it does not collide, which is why those three local imports were clean before this PR. What disproved my earlier reasoning: tests/test_write_queue.py carries 56 methods that never use `self` and #773 passed DeepSource Python, so that rule was never the blocker. I should have measured that before posting the inference rather than after. Evidence: tests/test_jsonl_watcher.py 112 passed; the 3 liveness pins green; ruff check + format clean. The three named lines are unchanged by this commit -- only the import that collided with them moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
EtanHey
added a commit
that referenced
this pull request
Sep 5, 2026
…, and pin the stdout/stderr split (XS) (#776) * docs(watcher): name the health file as the canonical liveness surface, and pin the stdout/stderr split The watcher seat's sampler read an empty throughput column because it grepped watch.out.log for "Watcher alive" and got nothing. #759 added logging.basicConfig(stream=sys.stderr), so the heartbeat rides stderr while the startup banner rides rich's stdout. The brief offered two fixes: duplicate the heartbeat onto stdout, or document the split and make the health file canonical. Taking the second. Duplicating it would put two writers on one signal and make the noisier copy the one people grep -- and a prose log line is the wrong liveness surface anyway. watcher-health.json already carries poll_count and updated_at, is JSON, and both fields advance on every poll, which is exactly what a sampler needs. AGENTS.md "Real-time JSONL Watcher" now states three things: read watcher-health.json for liveness (with its BRAINLAYER_WATCHER_HEALTH_PATH override), which stream carries what and why, and that grepping watch.out.log for "Watcher alive" returning nothing IS the split rather than a dead watcher. tests/test_jsonl_watcher.py::TestWatcherLivenessSurface pins both halves so the next reader does not re-derive this from a grep: - the health file advances poll_count 1 -> 2 and moves updated_at (tz-aware) across two polls - the heartbeat's single emit site is `logger.info`, and the watch command's basicConfig names sys.stderr explicitly rather than inheriting a default (AST, not text matching) Evidence: - tests/test_jsonl_watcher.py: 111 passed. - Mutation, both directions: turning the heartbeat's `logger.info` into `print` fails the pin; removing `stream=sys.stderr` alone fails it with "the watcher's log stream must be explicit, not inherited". Sources restored, `git diff src/` empty. - The documented path and fields check out against the real artifact on this machine: ~/.local/share/brainlayer/watcher-health.json, poll_count=2, updated_at 2026-09-05T14:48:53Z. Scope note: com.brainlayer.watch is disabled by Etan's ruling and this lane does not touch it, so there is no live-daemon verification here -- no watcher code changed, only docs and tests. The second test asserts on cli/__init__.py from the watcher's suite, so a changed-only push touching only the CLI will not run it; that mapping is a separate concern, called out in the docstring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(watcher): the health path is database-relative, the clamp is on both streams, and the pins tighten Round-1 bot + review round on #776. All five taken; the two doc findings were real defects in what I wrote. Macroscope medium (AGENTS.md) — CONFIRMED. The documented liveness path was wrong whenever BRAINLAYER_DB is set: `watch` resolves health_path as `watcher-health.json` beside the RESOLVED db (`cli/__init__.py`, `paths.get_db_path()` honours BRAINLAYER_DB), so following the hardcoded `~/.local/share/brainlayer/...` reads a stale or absent file. Now documented as database-relative, with that path named as the canonical-DB DEFAULT, the startup banner as where the real path is printed, and BRAINLAYER_WATCHER_HEALTH_PATH as the outright override. Review low (AGENTS.md) — CONFIRMED. The clamp was attributed to stdout only. It is on BOTH streams: `enforce_min_poll_interval()` logs it at WARNING to stderr (`watcher.py:86`) and the command also rprints it. Operators find it in watch.err.log, which the old wording hid. Review low — `assert second["updated_at"] >= first["updated_at"]` passed on an UNCHANGED timestamp, so only poll_count was really pinned and a sampler reading updated_at alone was unlocked. Now strict `>`, compared as parsed datetimes. Review low — the heartbeat pin filtered `ast.Constant` first args only, so a duplicate spelled `rprint(f"Watcher alive: …")` (an `ast.JoinedStr`) would not have been counted and `len(...) == 1` would still pass. `_leading_string_literal` now reads the leading literal out of either shape. Verified by mutation: adding exactly that f-string print next to the logger call fails the pin with `assert 2 == 1`. Restored; `git diff src/` empty. DeepSource Python (blocking, 4 findings) — all four addressed by splitting the one AST test in two: - cyclomatic complexity 16 "high" -> two focused tests plus three module-level helpers; - two methods not using their instance -> `@staticmethod`; - bare `next()` -> a list comprehension with `assert len(...) == 1`, which also names what was expected when it fails. Evidence: tests/test_jsonl_watcher.py 112 passed (111 before; the split adds one). ruff check + ruff format --check clean. No src change — docs and tests only. Full CI matrix was green on the previous head (3.11/3.12/3.13). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(watcher): import the parsed modules function-locally, clearing the PYL-W0404 reimports The DeepSource red on this PR was MINE, and my earlier comment on it was wrong. I inferred the blocking issues were a pre-existing "method doesn't use self" baseline; the run report says otherwise. The three blocking issues are: PYL-W0404 "Reimport 'watcher' (imported line 23)" -- tests/test_jsonl_watcher.py:85, 363, 521 Line 23 was the `import brainlayer.watcher` I added at module scope for the AST pins. Three pre-existing tests already reach the module the way this file has always done it -- `from brainlayer import watcher as watcher_module`, function-local -- and my top-level import turned all three into reimports. Cause: my change. Not a baseline. Both modules the pins parse are now imported inside the test that needs them, matching the file's own convention, and the module-level `import brainlayer.cli` / `import brainlayer.watcher` are gone. `from brainlayer.watcher import (...)` at module scope stays: it does not collide, which is why those three local imports were clean before this PR. What disproved my earlier reasoning: tests/test_write_queue.py carries 56 methods that never use `self` and #773 passed DeepSource Python, so that rule was never the blocker. I should have measured that before posting the inference rather than after. Evidence: tests/test_jsonl_watcher.py 112 passed; the 3 liveness pins green; ruff check + format clean. The three named lines are unchanged by this commit -- only the import that collided with them moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
EtanHey
added a commit
that referenced
this pull request
Sep 5, 2026
All six sites: pyproject.toml, src/brainlayer/__init__.py, server.json (root + packages[0]), brain-bar/bundle/Info.plist (short, bundle, release). Casks/brainbar.rb stays 1.5.9: no Swift change this release, declared to scripts/brainlayer-version-check.sh via BRAINLAYER_VERSION_CHECK_CASK_LAG_REASON="no BrainBar release for 1.5.15". Why a release. The installed keg on the M4 is 1.5.14 / build_sha 3bbe19f. Verified: `git merge-base --is-ancestor 7369f80 3bbe19f` -> false, so #781 -- the watcher per-poll burst fix, 14.97% -> 2.39% under the LaunchAgent's background QoS -- is NOT in any installed keg. Enabling com.brainlayer.watch today would start the burst-y watcher. 1.5.15 is what carries #781 (7369f80) onto the Mac. Also in: #772 #773 #775 #776. Co-authored-by: brainlayerClaude-ebe88b04 running claude-opus-5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What forced this PR
tests/test_write_queue.py::TestStoreRetryOnLock::test_store_retries_busy_error_before_queueingfailed on #762 attempt 1 (ubuntu, py3.13):
assert 1 == 3, 1 failed / 4524 passed — withnothing in that PR reachable from the failure (0 src lines changed).
Mechanism
The test patches
_retry_delayto0.001but has no control over the wall clock._storesets a deadline of
now + BRAINLAYER_STORE_BUSY_BUDGET_MS(400ms default,src/brainlayer/mcp/store_handler.py:32). On a loaded runner that budget expired after attempt 1,so the handler deferred instead of retrying three times:
assert attempts == 3then reads a slow machine as a broken retry loop.The change (tests only)
BRAINLAYER_STORE_BUSY_BUDGET_MS=60000. That test is aboutthe retry loop, so the clock comes out of it — 60s cannot expire across three mocked attempts
on any runner.
test_store_busy_budget_governs_and_is_injectable— budget 100ms with_retry_delay0.3s, sothe remaining budget cannot cover the next backoff and attempt 1 defers
(
status: DEFERRED,reason: DB_BUSY, one queue file). The deferral stops being the flake andbecomes the asserted behaviour.
No src change, no behaviour change. The brief asked to "make the budget injectable"; it already
is —
_store_busy_budget_ms()reads the env var on every call, so nothing was cached andnothing needed rewiring. The test simply never used the knob. Production defaults untouched.
Evidence — mutation, not assertion
setenvremoved (400ms default, same 0.3s delay)assert 2 == 1— it retries; the knob is what makes the deferral deterministicBRAINLAYER_STORE_BUSY_BUDGET_MS=1assert 0 == 3— the reported flake, forced=1envTestStoreRetryOnLock, 30 consecutive runs, random order ontests/test_write_queue.pyruff check+ruff format --checkOne detail of the brief disproved
The brief suggested forcing the budget to 1ms. That is too small: at 1ms the deadline is
already spent inside
_remaining_store_busy_budget_msatstore_handler.py:1030, beforestore_memoryis ever called —attempts == 0, and the test would pass for the wrong reason(that is exactly what the
=1mutation row above shows). 100ms is the smallest value thatstill reaches attempt 1.
Size
XS — 1 test file: 1 pinned env var, 1 new test.
@coderabbitai review
Opened by brainlayerClaude-19a28f09 running claude-opus-5.
Note
Low Risk
Only test changes in
tests/test_write_queue.py; production store handler logic and defaults are untouched.Overview
Fixes flaky
TestStoreRetryOnLockcoverage where_store’s default 400msBRAINLAYER_STORE_BUSY_BUDGET_MSwall-clock deadline could expire on slow CI before mocked retries finished, so the handler deferred instead of retrying three times.The existing busy-retry test now sets
BRAINLAYER_STORE_BUSY_BUDGET_MS=60000so the case targets the retry loop, not runner timing. A new testtest_store_busy_budget_governs_and_is_injectableasserts the opposite path: with a 100ms budget and 0.3s retry delay, oneBusyErrorleads toDEFERRED/DB_BUSYand a single JSONL queue file. A_frozen_monotonichelper pinsstore_handler.time.monotonicso budget math is deterministic without freezing asyncio’s clock.Tests only — no production behavior or default budget changes.
Reviewed by Cursor Bugbot for commit fff2a8f. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Freeze
monotonic()clock and pin busy budget inbrain_storeretry tests_frozen_monotonictest helper that returns a fixed value frommonotonic()while delegating all other time functions to the real moduleBRAINLAYER_STORE_BUSY_BUDGET_MSto 60s intest_store_queues_on_busy_errorso the three mocked retry attempts complete without early deferraltest_store_busy_budget_governs_and_is_injectable, which injects a 100ms budget, 300ms retry delay, and frozen clock to verify that a single busy attempt defers and queues a JSONL entry deterministicallyMacroscope summarized fff2a8f.