Skip to content

fix(index): enforce the runtime cap from a watchdog thread, and correct why the 4h cap missed - #778

Merged
EtanHey merged 5 commits into
mainfrom
wt/index-cap
Sep 5, 2026
Merged

fix(index): enforce the runtime cap from a watchdog thread, and correct why the 4h cap missed#778
EtanHey merged 5 commits into
mainfrom
wt/index-cap

Conversation

@EtanHey

@EtanHey EtanHey commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Size: L — why: 756 lines, but 362 of them are the tests that reproduce the defect, and the watchdog module + its wiring + its tests are one indivisible fix; splitting would ship an unenforced cap or an untested one. The plan asked for S; this is over, flagged rather than quietly split.

The premise in the plan is wrong, and here is the measurement

The plan hypothesised that the apsw progress handler in VectorStore.upsert_chunks cannot fire inside a phase with no transaction boundary. Measured against this apsw (3.51.2.0 / SQLite 3.51.2), it fires freely, and the id=-keyed registration the code uses does register:

statement progress-handler ticks
FTS5 rebuild, 400k rows 7,287 in 0.68s
vec0 bulk DELETE 1,008 in 0.042s
vec0 KNN, 1.5M rows × dim 128 8 in 0.167s (~48/s)
set_progress_handler(..., id=<obj>) 34,180

(An earlier reading of 0 ticks for a vec0 KNN was a scale artifact — a 0.020s statement executes fewer than nsteps=1000 opcodes total. At 1.5M rows it ticks ~48×/s. I am reporting the corrected number, not the first one.)

So a long statement inside upsert_chunks would have been stopped. The plan's "boundary-less phase never hits the check" is not the cause.

What the cause actually is

The cap is enforced only inside upsert_chunks and at index_fast's per-file boundaries. Every other phase of a run has no cap of any kind — notably:

  1. open_writer_store() — schema validation and migrations. Read-heavy, which is exactly what the M1 evidence describes: ~100% CPU, WAL unchanged, only brainlayer.db/-shm/-wal open, no JSONL open.
  2. The first embedding-model loadembed_chunks calls on_progress every 32 chunks, so embedding itself is guarded, but the model load before the first callback is not.
  3. File discovery (rglob) — ran before the clock even started.

Either of (1) or (2) running long explains 12h13m with no alarm, with no opcode blackout needed.

The fix

A cap at the work sites cannot cover sites that have none, so this puts it beside them. IndexWatchdog owns the cap from its own thread, which a blocked main thread cannot delay:

  • Alarms the moment the deadline passes, naming the phase, before the stuck phase unwinds. Unconditional — it holds for a phase issuing no SQL at all, such as a wedged model load.
  • Calls sqlite3_interrupt on the writer connection (apsw documents cross-thread as safe). Measured: aborts a long FTS5 statement 0.22s into a 0.66s rebuild, leaving integrity_check ok, the transaction rolled back, and all 400,000 rows readable. Retries are bounded at 3 so the ROLLBACK that follows survives.
  • Leaves expired set, which the main thread turns into IndexDeadlineExceeded at its next safe point. An exception arriving while expired is now reported as the cap, not as an opaque Error:.
  • Heartbeats phase + last-progress age every BRAINLAYER_INDEX_HEARTBEAT_S (default 300s), so a 12h-silent log cannot recur — and the next real stall names its own phase, which is what would have settled this from the M1 directly.

One cap emits one alarm: the boundary path now carries phase too, and defers when the watchdog already alarmed.

Also: file discovery now runs inside the cap window.

IndexWatchdog binds time.monotonic at import time deliberately — several existing tests monkeypatch the CLI's clock, and a watchdog reading a patched clock would both mis-fire and consume values from those tests' iterators.

Test-first

The cap miss is reproduced before it is closed, with fakes only (no embedding model, no canonical DB) — a write phase that ignores every opcode hook and unwinds only on conn.interrupt():

  • before: 20.99s wall against a 1.0s cap, interrupt_calls == 0 — the run ended only because the test's own 20s ceiling expired.
  • after: 3.88s, interrupted at the cap, alarm carries phase=.

27 tests across the two new files, plus the 7 pre-existing cap tests still green; 110 green across the blast radius (test_cli*.py, context pipeline, source class, path isolation, writer telemetry, ingest-t3).

Live-check (pre-merge, against a copy — never the canonical DB)

APFS clone of the real 16GB DB, real embedder, BRAINLAYER_DB pointed at the clone throughout; canonical DB last written 17:42:57, before every run here.

  • 900s cap: indexed 26 chunks, heartbeat named phase=embed_and_upsert:<file>.
  • 6s cap: tripped at 6.153s, alarm named the phase, exit 1.
  • chunks 805,045 → 805,062 — committed work kept, nothing lost.
  • PRAGMA integrity_checkok; in_transactionFalse.

A duplicate heartbeat line found in that live run (a stderr log handler printing alongside the explicit print) is fixed here; mock-green had not shown it.

Not covered / follow-ups

  • A phase that neither issues SQL nor returns can be alarmed and heartbeaten but not aborted. Killing such a run is a policy call and deserves its own PR + Etan's view.
  • emit_alarm in alarm.py double-prints under a configured stderr log handler (logger.critical + print). Pre-existing, left alone — out of scope.
  • The plan's bonus (no such savepoint: dedupe_merge) is untouched; it is its own PR per canon rule 9.
  • Which phase the M1 was actually in remains unproven from this machine. The heartbeat is the instrument that will name it on the next run.

Reviews

@coderabbitai review


Agent: brainlayerClaude-a9949327 · model claude-opus-5[1m]

🤖 Generated with Claude Code


Note

Medium Risk
Changes indexing runtime control, threading, and SQLite interrupt wiring on the writer open path; behavior is heavily tested but affects long-running production index jobs.

Overview
Fixes index runs that could exceed BRAINLAYER_INDEX_MAX_RUNTIME_S during boundary-less phases (store open, file discovery, model load) where only per-file and upsert_chunks checks applied before.

Adds IndexWatchdog: a daemon thread on wall-clock time that emits INDEX_RUNTIME_EXCEEDED with phase and progress age, prints periodic BRAINLAYER_INDEX_HEARTBEAT lines, calls sqlite3_interrupt on the writer (bounded retries), and sets expired for the main thread to raise IndexDeadlineExceeded.

index-fast arms the watchdog before JSONL discovery (incremental walk with periodic cap checks), wires open_writer_store(..., on_connection=...) so schema probe is interruptible, tracks phases (discover, open_store, parse, embed_and_upsert), and centralizes cap exit via _exit_on_index_deadline while preserving unrelated errors after expiry.

run_tests.sh maps index_watchdog.py to unit, boundaryless, and CLI cap tests only when all three exist (fail-closed partial mapping).

Extensive new/updated pytest coverage for watchdog behavior and changed-only test mapping.

Reviewed by Cursor Bugbot for commit 287a916. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix map_changed_files_to_pytests mapping for index_watchdog.py and remove recursive glob patterns

  • Adds an explicit test-scope mapping for src/brainlayer/index_watchdog.py that requires all three watchdog test files (unit, boundaryless, CLI) before marking the source as mapped; otherwise it falls back to the full suite.
  • Removes recursive tests/**/*.py and src/brainlayer/**/*.py generic case patterns, leaving only top-level tests/*.py and src/brainlayer/*.py matching.
  • Risk: nested test paths and nested source modules no longer match the generic rules in map_changed_files_to_pytests; any future nested modules will need explicit mappings or will fall back to the full suite.

Macroscope summarized 287a916.

@EtanHey EtanHey added the L Large change label Sep 5, 2026
@cursor

cursor Bot commented Sep 5, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_e66fbb67-cb42-48e1-ad3c-825a21c6e4e4)

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 32 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: e45128cb-daa3-4019-b711-2fd0e814335c

📥 Commits

Reviewing files that changed from the base of the PR and between 2448231 and 287a916.

📒 Files selected for processing (8)
  • scripts/run_tests.sh
  • src/brainlayer/cli/__init__.py
  • src/brainlayer/index_watchdog.py
  • src/brainlayer/runtime_store.py
  • tests/test_cli_index_watchdog.py
  • tests/test_index_watchdog_boundaryless.py
  • tests/test_index_watchdog_unit.py
  • tests/test_run_tests_script.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@deepsource-io

deepsource-io Bot commented Sep 5, 2026

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in 2448231...287a916 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade   Security  

Reliability  

Complexity  

Hygiene  

Code Review Summary

Analyzer Status Updated (UTC) Details
Python Sep 5, 2026 5:17p.m. Review ↗
Swift Sep 5, 2026 5:17p.m. Review ↗
JavaScript Sep 5, 2026 5:17p.m. Review ↗
Shell Sep 5, 2026 5:17p.m. Review ↗
Secrets Sep 5, 2026 5:17p.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.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

BrainLayer ratchet

Every Value below was measured by this run. A row this machine cannot measure says n/a — <reason> instead of a number; baselines in Notes name their own machine, method and date and were not measured here.

Row Status Value (measured by this run) Method Notes
commit provenance 🟢 GREEN measured 287a9163fcac == PR head · checkout ef2557d6f49d commit graph + live PR head · in-process · runner Which commit this whole table is about. On a pull_request event the checkout is GitHub's synthetic merge ref, whose sha is not on the PR — #759's table printed 13fa724278bf while that PR's head was 4632f979 — so this row names the PR-head parent instead, the sha a reviewer can actually see. The comparison sha is read live from repos/{owner}/{repo}/pulls/{n} when the table is collected, not taken from the event payload, because the payload cannot know the run has been overtaken. Residual window, stated rather than papered over: a push landing between that read and the comment being posted is not caught here — the run for that push refreshes the table.
baseline attestation 🟢 GREEN baseline f421d1a7c5e6 matches the main attestation (run 33980263749 · main 24482318d82d · 2026-09-05T17:13:16Z) main attestation artifact via Actions API · in-process · runner What every comparison is measured AGAINST, and who says so. The baseline fields of tests/fixtures/sprint_gate/corpus.json (queries, latency_baseline_ms, thresholds) are compared to the ratchet-attestation artifact of the latest successful push or (no-input) workflow_dispatch run of ratchet-attest.yml on main, fetched through the Actions API — a PR run cannot write to another run's artifacts. A field that differs is RED unless that main run measured the new value; today no runner-side collector measures any baseline field, so today the baseline cannot move by PR at all, and this row says so instead of a hand edit passing. Boundary: the comparator is this PR's checkout of ci_ratchet_table.py, diff-reviewable, not tamper-proof.
provenance 🟢 GREEN stamped ef2557d6f49d == HEAD, tree clean wheel stamp · in-process · runner Sha half of #749 keg-mode provenance: a keg built from this wheel can answer __build_sha__. The helper-age and served-process predicates need a running BrainBar and are measured only by scripts/sprint_gate.py on an installed Mac. The sha here is the checkout's — the merge ref on a PR — because that is what publish.yml stamps at release time; the PR-head sha this table describes is the one in commit provenance above.
fallback replay debt ⚪ n/a n/a — no fallback queue on this machine: the pending memories live in ~/Gits/*/docs.local/decisions, and docs.local/ is gitignored, so a runner checkout has no copy of them to count docs.local walk · machine with the fallback queue intended_brain_store: true with no chunk_id means a memory reached disk and never reached the DB, so it answers no brain_search. Budget: 0. Any pending or unparseable file is a finding, never a band -- 122 of these sat from 2026-06-28 to 2026-09-05 because nothing counted them where a reader would look. Measured by walking the tree, so it is only ever measured on a machine that HAS the tree.
mapped bytes ⚪ n/a n/a — no BrainBar daemon at /tmp/brainbar.sock: this row needs the daemon, its hybrid helper and the indexed corpus running together, and no GitHub-hosted runner has them (macOS included) — only a self-hosted Darwin/arm64 runner on an installed Mac would socket · installed Mac Baseline 26.2 GB — installed Mac, socket, 2026-09-03, after R2 drained 15,070 → 0. Up from 16.8 GB because the drain left more vectors mapped under the same cap: the change is the drain, not a leak. Not measured by this run.
search p50/p95 ⚪ n/a n/a — no BrainBar daemon at /tmp/brainbar.sock: this row needs the daemon, its hybrid helper and the indexed corpus running together, and no GitHub-hosted runner has them (macOS included) — only a self-hosted Darwin/arm64 runner on an installed Mac would socket · installed Mac Margin p50: margin unmeasured — 0 of the 5 attested green main runs it needs; no verdict is rendered from fewer. Margin p95: margin unmeasured — 0 of the 5 attested green main runs it needs; no verdict is rendered from fewer. Calibrated on MacBook-Pro.local at 2026-09-01T08:42:22Z under active_sprint_load (tests/fixtures/sprint_gate/corpus.json). Not measured by this run.
idle CPU ⚪ n/a n/a — no BrainBar daemon at /tmp/brainbar.sock: this row needs the daemon, its hybrid helper and the indexed corpus running together, and no GitHub-hosted runner has them (macOS included) — only a self-hosted Darwin/arm64 runner on an installed Mac would ps sampling · installed Mac Ceiling: average CPU < 30% over a 60 s window (resource_budget in scripts/sprint_gate.py), ratified and kept as a hard budget. Margin daemon: margin unmeasured — 0 of the 5 attested green main runs it needs; no verdict is rendered from fewer. Margin helper: margin unmeasured — 0 of the 5 attested green main runs it needs; no verdict is rendered from fewer. Margin watcher: margin unmeasured — 0 of the 5 attested green main runs it needs; no verdict is rendered from fewer. Needs the BrainBar daemon, helper and watcher actually running. Not measured by this run.
signature_valid ⚪ n/a n/a — the macOS signature-parity job is trigger-gated and did not run on this PR: it touches no release or signing path (pyproject.toml, scripts/release-*, scripts/brainlayer-version-check.sh, publish.yml, ratchet.yml) and carries no ratchet:signatures label — a GitHub macOS runner bills at ~10× Linux minutes and rebuilds the keg venv from source codesign · installed keg scripts/release-verify-signatures.sh <keg> codesign-verifies every *.so/*.dylib under libexec/venv. The macOS parity job installs the published tap formula (etanhey/layers/brainlayer), so this row measures the release path — formula, published sdist and Homebrew's relocation — and not this PR's tree. Release-time baseline for the same keg on a different machine: 442 valid / 0 invalid — installed Mac (M4 Max), brew --prefix brainlayer 1.5.11, 2026-09-03.

🟢 GREEN measured, within budget · 🔴 RED measured, out of budget — a finding to clear before merge · ⚪ n/a not measurable on this machine, never guessed.

No RED rows.

Measured on Linux/x86_64 · measured 287a9163fcac · PR head 287a9163fcac · checkout ef2557d6f49d · run · updated 2026-09-05 17:17:44 UTC

Comment thread src/brainlayer/cli/__init__.py Outdated
)
return typer.Exit(1)

context: dict[str, object] = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redefining name 'context' from outer scope


The local variable name hides the variable defined in the outer scope, making it inaccessible and might confuse.

Comment thread src/brainlayer/cli/__init__.py Outdated
import time
from contextlib import ExitStack

from rich.progress import (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reimport 'TaskProgressColumn' (imported line 22)


A module or an import name is reimported multiple times. This can be confusing and should be fixed.
Please refer to the occurrence message to see the reimported name and the line number where it was imported for the first time.

Comment thread src/brainlayer/cli/__init__.py Outdated
while a stuck phase is still unwinding. When it already did, this adds the final
committed counts as a plain line instead of a second alarm -- one cap, one alarm.
"""
import time

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reimport 'time' (imported line 13)


A module or an import name is reimported multiple times. This can be confusing and should be fixed.
Please refer to the occurrence message to see the reimported name and the line number where it was imported for the first time.



@app.command("index-fast", hidden=True)
def index_fast(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`index_fast` has a cyclomatic complexity of 26 with "very-high" risk


A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.

@@ -3556,8 +3602,10 @@ def index_fast(
force: bool = typer.Option(False, "--force", "-f", help="Re-index all files (ignore cache)"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused argument 'force'


An unused argument can lead to confusions. It should be removed. If this variable is necessary, name the variable _ or start the name with unused or _unused.

Comment thread src/brainlayer/cli/__init__.py Outdated
import time
from contextlib import ExitStack

from rich.progress import (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reimport 'Progress' (imported line 22)


A module or an import name is reimported multiple times. This can be confusing and should be fixed.
Please refer to the occurrence message to see the reimported name and the line number where it was imported for the first time.

Comment thread src/brainlayer/cli/__init__.py Outdated
import time
from contextlib import ExitStack

from rich.progress import (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reimport 'MofNCompleteColumn' (imported line 22)


A module or an import name is reimported multiple times. This can be confusing and should be fixed.
Please refer to the occurrence message to see the reimported name and the line number where it was imported for the first time.

Comment thread src/brainlayer/cli/__init__.py Outdated
import time
from contextlib import ExitStack

from rich.progress import (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reimport 'BarColumn' (imported line 22)


A module or an import name is reimported multiple times. This can be confusing and should be fixed.
Please refer to the occurrence message to see the reimported name and the line number where it was imported for the first time.

Comment thread src/brainlayer/cli/__init__.py Outdated

from ..index_new import index_chunks_to_sqlite
from ..index_watchdog import IndexWatchdog
from ..paths import get_db_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redefining name 'get_db_path' from outer scope


The local variable name hides the variable defined in the outer scope, making it inaccessible and might confuse.

assert result.exit_code != 0
codes = [alarm.code for alarm in alarms]
assert codes.count("INDEX_RUNTIME_EXCEEDED") == 1, f"expected exactly one cap alarm, got {codes}"
alarm = next(a for a in alarms if a.code == "INDEX_RUNTIME_EXCEEDED")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to `next()` should be wrapped in `try-except`


Calls to next() should be inside try-except block.

Comment thread src/brainlayer/index_watchdog.py Outdated
Comment thread src/brainlayer/cli/__init__.py
):
# The watchdog is armed before the store opens: opening it can run migrations,
# which is itself a phase with no transaction boundary.
with ExitStack() as stack:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium cli/__init__.py:3663

When the runtime cap expires while ExitStack is closing Progress or the writer store, index_fast still prints the success message and exits with code 0 despite INDEX_RUNTIME_EXCEEDED being emitted. Add a final watchdog.raise_if_expired() immediately after the ExitStack block so shutdown-time expiry is reported as a failed index.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/cli/__init__.py around line 3663:

When the runtime cap expires while `ExitStack` is closing `Progress` or the writer store, `index_fast` still prints the success message and exits with code 0 despite `INDEX_RUNTIME_EXCEEDED` being emitted. Add a final `watchdog.raise_if_expired()` immediately after the `ExitStack` block so shutdown-time expiry is reported as a failed index.

Comment on lines 3715 to 3778
@@ -3645,6 +3714,7 @@ def index_fast(
def progress_callback(embedded_count, total_embed):
pass # Could update sub-progress here

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium cli/__init__.py:3715

During embed_and_upsert, heartbeats report progress only from when the phase started, so a healthy long embedding run is falsely diagnosed as stalled. progress_callback is invoked after each embedding batch but does not call watchdog.note_progress(); update the watchdog there.

Suggested change
pass # Could update sub-progress here
watchdog.note_progress()
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/cli/__init__.py around line 3715:

During `embed_and_upsert`, heartbeats report progress only from when the phase started, so a healthy long embedding run is falsely diagnosed as stalled. `progress_callback` is invoked after each embedding batch but does not call `watchdog.note_progress()`; update the watchdog there.

EtanHey added a commit that referenced this pull request Sep 5, 2026
Round-1 review on #778. Both HIGHs were real; verified in the code, then closed with tests
that fail against the previous commit.

HIGH 1 -- dual deadline. IndexWatchdog computed `own_now + max_runtime_s` at construction, so
arming it after file discovery handed the run a second full budget: a slow rglob plus a stalled
open_store could burn 2 x max_runtime_s, and the PR's claim that discovery is inside the cap
window was false. It now takes `started_at` and the CLI's `monotonic`, so watchdog and CLI
share ONE deadline. Proven: the new test reads 1012.0 against the previous commit where 1007.0
is correct.

HIGH 2 -- open_store, the phase this PR names as the likeliest 12h culprit, could be alarmed
but not aborted, because `interrupt` was attached only after `open_writer_store` returned.
`WriterRuntimeStore._init_runtime_db` creates the apsw connection BEFORE it runs any probe
SQL, so `open_writer_store` now takes an `on_connection` hook and hands the connection over at
that point; the CLI passes `watchdog.set_interrupt`. The whole probe window is interruptible.
Proven: against the previous commit the new test stalls to its 20s ceiling under a 1s cap
("open_store was never interruptible"); with the hook it stops at the cap with
`phase=open_store`. Verified live too -- the hook receives a real apsw.Connection with a
callable `interrupt`, and the store still opens with a valid schema fingerprint.

MEDIUM -- heartbeats stopped at the deadline (`if expired ... elif heartbeat`), so a wedge that
cannot be aborted alarmed once and then went silent again: the second half of the original
12h-silent-log failure. Heartbeats now continue past expiry.

MEDIUM -- `alarm_emitted` was set before the emit succeeded, so a failing emit let the process
exit 1 with no INDEX_RUNTIME_EXCEEDED anywhere, since the CLI defers to that flag.

...and the live-only bug that fixing it introduced: gating solely on "landed" made the alarm
REPEAT. A 6s-cap run against the real DB printed the same alarm 13 times, because
`emit_alarm` returns the Axiom *telemetry* result while its stderr and logging paths are the
documented guaranteed ones -- a falsy return is not a missing record. Now two flags:
`attempted` (set unconditionally, so it can never re-alarm) and `emitted` (only a raising emit
leaves the CLI owing a fallback). Re-checked live: exactly one alarm.

MEDIUM -- tests pinned neither shared-deadline semantics nor an open_store/discovery stall.
Four new tests cover them, each verified to fail against the previous commit.

DeepSource Python (bot round): fixed what is mine -- a redundant `import time` and a `context`
name that shadowed an outer scope. The reimports of `time` and the rich Progress columns, the
`get_db_path` shadow, and unused `force` are pre-existing in `index_fast`; the local reimports
are removed here since they sit in this hunk. **`brainlayer index --force` accepts `--force`
and does nothing with it** -- pre-existing latent bug, its own PR per canon rule 9. On
complexity: `index_fast` measured 19 before this work, 20 after round 1, and **17 now** --
below the baseline I found it at, because one deadline on one clock let three duplicated
`time.monotonic() >= deadline` checks collapse into `watchdog.raise_if_expired()`, which now
checks the clock as well as the flag. Splitting `index_fast` further is a separate refactor.

Live-check (fresh APFS clone of the real 16GB DB, real embedder, BRAINLAYER_DB on the clone
throughout): 900s cap indexed 46 chunks with heartbeats whose `last_progress_age_s` resets per
file; 6s cap tripped at 6.218s naming the phase, exit 1, exactly one alarm, 4 heartbeats;
chunks 805,420 -> 805,466; `integrity_check` ok; no dangling transaction.

Tests: 33 across the two watchdog files, 234 green across the blast radius.

Agent: brainlayerClaude-a9949327 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Sep 5, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_6b026095-5661-40a5-920d-03316a2fe3c4)

@EtanHey

EtanHey commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Round 1 answered — 345fb80c

Both HIGHs were real. I verified each in the code, fixed it, and proved every new test fails against the previous commit — otherwise they are not guards.

HIGH — dual deadline (index_watchdog.py)

Correct. _deadline = own_now + max_runtime_s meant arming the watchdog after discovery handed the run a second full budget, and my claim that discovery is inside the cap window was false. It now takes started_at plus the CLI's monotonic, so there is one deadline on one clock.

Guard: test_discovery_time_is_charged_against_the_one_deadline — discovery burns 5s of a 7s cap, then asserts deadline_monotonic == 1007.0. Against 38938f10 it reads assert 1012.0 == 1007.0 — the second budget, caught exactly.

HIGH — open_store could be alarmed but not aborted (cli/__init__.py)

Correct, and fixable: WriterRuntimeStore._init_runtime_db creates the apsw connection before it runs any probe SQL (runtime_store.py:428-435), so the lever existed — it just wasn't handed out. open_writer_store now takes an on_connection hook invoked at that point; the CLI passes watchdog.set_interrupt. The whole probe window is interruptible.

Guard: test_a_stalling_open_store_is_alarmed_and_interrupted. Against 38938f10 it stalls to its 20.02s ceiling under a 1s cap — "open_store was never interruptible", the M1 shape in miniature. With the hook it stops at the cap with phase=open_store.

Verified live too, on a clone of the real 16GB DB: the hook receives a real apsw.Connection with a callable interrupt, calling it mid-open is safe, and the store still opens with a valid schema fingerprint.

MEDIUM — heartbeats stopped at the deadline

Correct: if expired … elif heartbeat meant a non-abortable wedge alarmed once then went silent — the second half of the original 12h-silent-log failure. Heartbeats now continue past expiry. Guard fails against the old shape with heartbeats stopped after the deadline: [].

MEDIUM — alarm_emitted set before a successful emit

Correct, and fixing it naively introduced a worse bug that only the live run caught. Gating solely on "landed" made the alarm repeat 13 times in a 6s-cap run against the real DB, because emit_alarm returns the Axiom telemetry result while its stderr and logging paths are the documented guaranteed ones (alarm.py:66-99) — a falsy return is not a missing record.

Now two flags: attempted, set unconditionally so it can never re-alarm, and emitted, which only a raising emit leaves unset, so the CLI still owes its fallback. Re-checked live: exactly one alarm. Both shapes have guards.

MEDIUM — tests pinned neither shared-deadline semantics nor an open_store stall

Correct. Four new tests, each verified failing against 38938f10.

DeepSource Python (bot round)

finding disposition
Redefining name 'context' from outer scope Mine — fixed (alarm_context)
Reimport 'time' (line 13) in _exit_on_index_deadline Mine — fixed
Reimports of time + the 8 rich Progress columns in index_fast Pre-existing; removed anyway since they sit in this hunk
Redefining name 'get_db_path' from outer scope Pre-existing; local import removed
Unused argument 'force' Pre-existing and a real latent bugbrainlayer index --force accepts the flag and does nothing with it. Its own PR per canon rule 9.
Call to next() should be wrapped in try-except Not mine — the two next() calls in this file (:1413, :2301) both use the safe 2-arg default form and are far from this diff
index_fast cyclomatic complexity Measured, not argued: 19 before this work → 20 after round 1 → 17 now, below the baseline I found it at. One deadline on one clock let three duplicated time.monotonic() >= deadline checks collapse into watchdog.raise_if_expired(), which now checks the clock as well as the flag. Splitting index_fast further is a separate refactor PR.

Live-check (fresh APFS clone of the real 16GB DB, real embedder, BRAINLAYER_DB on the clone throughout)

  • 900s cap → 46 chunks, heartbeats whose last_progress_age_s correctly resets per file.
  • 6s cap → tripped at 6.218s, phase=embed_and_upsert:<file>, exit 1, exactly one alarm, 4 heartbeats.
  • chunks 805,420 → 805,466; PRAGMA integrity_check ok; no dangling transaction.

33 tests across the two watchdog files; 234 green across the blast radius (now including test_runtime_store.py and the other writer-store consumers).

Note for the lead

My round-1 push used a space-separated BRAINLAYER_CHANGED_FILES, before the lane rule about comma-separation was written. This push used commas. Its first attempt failed the gate and an immediate re-run of the same gate passed (exit 0), as did the retried push — I did not capture the failing step, so I am reporting that as an unexplained one-off, not a diagnosed flake.


Agent: brainlayerClaude-a9949327 · model claude-opus-5[1m]



@app.command("index-fast", hidden=True)
def index_fast(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`index_fast` has a cyclomatic complexity of 23 with "high" risk


A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.

assert store.stalled_s is not None and store.stalled_s < store.ceiling_s
assert wall_s < CAP_S + 10.0, f"open_store ran {wall_s:.1f}s against a {CAP_S}s cap"
assert result.exit_code != 0
alarm = next(a for a in alarms if a.code == "INDEX_RUNTIME_EXCEEDED")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to `next()` should be wrapped in `try-except`


Calls to next() should be inside try-except block.

@cursor

cursor Bot commented Sep 5, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_05f03a2b-5fce-4283-a4a7-8d561697fa4d)

Comment thread tests/test_run_tests_script.py Outdated
env["PYTEST_LOG"] = str(pytest_log)
env["BUN_LOG"] = str(bun_log)

result = subprocess.run(["bash", str(SCRIPT_PATH)], capture_output=True, text=True, env=env)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'subprocess.run' used without explicitly defining the value for 'check'.


subprocess.run uses a default of check=False, which means that a nonzero exit code will be
ignored by default, instead of raising an exception.

You can ignore this issue if this behaviour is intended.

Comment thread tests/test_run_tests_script.py Outdated
env["PYTEST_LOG"] = str(pytest_log)
env["BUN_LOG"] = str(bun_log)

result = subprocess.run(["bash", str(SCRIPT_PATH)], capture_output=True, text=True, env=env)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'subprocess.run' used without explicitly defining the value for 'check'.


subprocess.run uses a default of check=False, which means that a nonzero exit code will be
ignored by default, instead of raising an exception.

You can ignore this issue if this behaviour is intended.

Comment thread tests/test_run_tests_script.py Outdated
env["PYTEST_LOG"] = str(pytest_log)
env["BUN_LOG"] = str(bun_log)

result = subprocess.run(["bash", str(SCRIPT_PATH)], capture_output=True, text=True, env=env)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'subprocess.run' used without explicitly defining the value for 'check'.


subprocess.run uses a default of check=False, which means that a nonzero exit code will be
ignored by default, instead of raising an exception.

You can ignore this issue if this behaviour is intended.

Comment thread src/brainlayer/cli/__init__.py Outdated
cap be checked as the walk proceeds, and keeps `phase=discover` current in the heartbeat.
"""
found: list[Path] = []
for index, entry in enumerate(entries):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redefining name 'index' from outer scope


The local variable name hides the variable defined in the outer scope, making it inaccessible and might confuse.



@app.command("index-fast", hidden=True)
def index_fast(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`index_fast` has a cyclomatic complexity of 24 with "high" risk


A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.

result = CliRunner().invoke(app, ["index", str(source)])

assert result.exit_code != 0
alarm = next(a for a in alarms if a.code == "INDEX_RUNTIME_EXCEEDED")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to `next()` should be wrapped in `try-except`


Calls to next() should be inside try-except block.

result = CliRunner().invoke(app, ["index", str(source)])

assert seen.get("alarms_during_walk"), "the run was silent while discovery was stuck"
alarm = next(a for a in alarms if a.code == "INDEX_RUNTIME_EXCEEDED")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call to `next()` should be wrapped in `try-except`


Calls to next() should be inside try-except block.

with watchdog:
reads_after_start = caller_reads["n"]
deadline = _mono() + 1.0
while _mono() < deadline:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Body doesn't contain any code


In most cases, an empty body of for, while or if implies some piece of code is missing.
Such empty block must be either filled or removed.

Comment on lines +3573 to +3597
if index % 64 == 0:
watchdog.raise_if_expired()
found.append(entry)
watchdog.note_progress()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium cli/__init__.py:3573

A short glob/rglob walk that blocks past the deadline returns without raising, so index_fast can report “No JSONL files found” and exit before the watchdog emits the runtime-cap alarm. Check watchdog.raise_if_expired() after the loop before recording progress.

-    watchdog.note_progress()
+    watchdog.raise_if_expired()
+    watchdog.note_progress()
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/cli/__init__.py around line 3573:

A short `glob`/`rglob` walk that blocks past the deadline returns without raising, so `index_fast` can report “No JSONL files found” and exit before the watchdog emits the runtime-cap alarm. Check `watchdog.raise_if_expired()` after the loop before recording progress.

@cursor

cursor Bot commented Sep 5, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_7b10aa64-c8db-4b29-9b18-947266c1ef26)

@EtanHey

EtanHey commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Round 2 answered — 1e95735a (fixes) + ae3b2467 (prepush mapping)

Both HIGHs and all three mediums were real. Every fix has a guard proven to fail against 345fb80c.

HIGH — discovery charged but not watched

Correct. Arming after the rglob fixed the arithmetic and nothing else. The watchdog is now armed before discovery with phase=discover, and _discover_jsonl_files consumes the generator so the cap is checked as the walk proceeds — every 64 entries, enough to bound a stall and cheap on a 12,000-file tree.

Guard on 345fb80c: "the run was silent while discovery was stuck".

HIGH — every post-expiry Exception rewritten as the cap

Correct, and this was the more dangerous one. An expired watchdog means the cap is part of the story, not that this exception is the cap. The handler now reports the cap alarm and keeps the original type and message — on stderr and in an unwind_error field. Only apsw's InterruptError (and IndexDeadlineExceeded) count as the interrupt we asked for.

Guard on 345fb80c: the original failure was rewritten as the cap: … cause=RuntimeError — the RuntimeError's message was simply gone.

MEDIUM — injected clock raced the poll thread

Correct, and it was the tension between your round-1 HIGH (one deadline) and the module's original wall-clock binding. Resolved with two clocks: the caller's, read exactly once by start() on the calling thread to carry already-spent budget into the wall domain; and this module's import-time wall clock, the only one the thread ever reads. One deadline, no shared-clock race. A guard counts caller-clock reads across a second of polling.

MEDIUM — on_connection failures swallowed

Correct. Both runtime_store paths log at error level, and the CLI hook records note_uninterruptible(reason), which then appears in every heartbeat and in the cap alarm — so abortability can't degrade quietly.

MEDIUM — tests didn't cover the fail-open holes

Four new tests, each verified failing against 345fb80c.

LOW — raise_if_expired could trip without alarming

Fixed rather than argued: it routes through the same _on_deadline() the thread uses, so both halves agree about the cap.


A bug I introduced, that no suite run had caught

tests/test_index_watchdog_boundaryless.py was poisoning brainlayer.index_new for the whole session. index_new binds open_writer_store at its import time; _install patched runtime_store.open_writer_store first and imported index_new second, so index_new captured the fake permanently — monkeypatch restores runtime_store, not index_new. test_context_pipeline then died with '_StallingRuntimeStore' object has no attribute 'upsert_chunks'.

Fixed by importing the module at test-module scope before any patch, and patching every binding through monkeypatch.

The full suite was green on this. The targeted set caught it — which is the opposite of the usual argument for running everything.


The prepush mapping (ae3b2467) — and a correction about my own earlier claim

I previously reported the round-1 gate escalation as a space-vs-comma issue. That was wrong. The real cause, measured:

WARNING: changed-only scope found an unmapped source change; falling back to full pytest unit suite
WARNING: unmapped: src/brainlayer/index_watchdog.py src/brainlayer/cli/__init__.py

4,553 tests in 7m51s — commas never fixed it, because the mapping table was the gap. After: 0 fallbacks, 133 targeted tests, ~22s.

Two mappings, and only one is mine:

Live-check (fresh APFS clone of the real 16GB DB, real embedder, canonical never touched)

  • 900s cap → 38 chunks indexed.
  • 6s cap → tripped at 6.203s, exit 1, exactly one alarm, 3 heartbeats.
  • 0.5s cap over the real ~12,000-file tree → still reached the write phase, so watching discovery costs nothing measurable.
  • chunks 805,636 → 805,674; integrity_check ok; no dangling transaction.

148 green across the blast radius.


Agent: brainlayerClaude-a9949327 · model claude-opus-5[1m]

Comment thread tests/test_run_tests_script.py Outdated
env["PYTEST_LOG"] = str(pytest_log)
env["BUN_LOG"] = str(bun_log)

result = subprocess.run(["bash", str(SCRIPT_PATH)], capture_output=True, text=True, env=env)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'subprocess.run' used without explicitly defining the value for 'check'.


subprocess.run uses a default of check=False, which means that a nonzero exit code will be
ignored by default, instead of raising an exception.

You can ignore this issue if this behaviour is intended.

Comment thread tests/test_run_tests_script.py Outdated
env["PYTEST_LOG"] = str(pytest_log)
env["BUN_LOG"] = str(bun_log)

result = subprocess.run(["bash", str(SCRIPT_PATH)], capture_output=True, text=True, env=env)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'subprocess.run' used without explicitly defining the value for 'check'.


subprocess.run uses a default of check=False, which means that a nonzero exit code will be
ignored by default, instead of raising an exception.

You can ignore this issue if this behaviour is intended.

if sink is not None:
sink(line)
else:
print(line, file=sys.stderr, flush=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High brainlayer/index_watchdog.py:353

A blocked sys.stderr write at this line stalls the sole watchdog thread indefinitely, so it never returns to _run() to set expired, emit the cap alarm, or interrupt SQLite; an undrained stderr=PIPE can therefore let the index exceed its cap. The same fail-open path exists because _on_deadline() calls synchronous _emit_cap_alarm() before setting expired or calling _pull_interrupt(). Make heartbeat and alarm delivery non-blocking/bounded, or set the expiry flag and interrupt before isolating potentially blocking output.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/index_watchdog.py around line 353:

A blocked `sys.stderr` write at this line stalls the sole watchdog thread indefinitely, so it never returns to `_run()` to set `expired`, emit the cap alarm, or interrupt SQLite; an undrained `stderr=PIPE` can therefore let the index exceed its cap. The same fail-open path exists because `_on_deadline()` calls synchronous `_emit_cap_alarm()` before setting `expired` or calling `_pull_interrupt()`. Make heartbeat and alarm delivery non-blocking/bounded, or set the expiry flag and interrupt before isolating potentially blocking output.

EtanHey added a commit that referenced this pull request Sep 5, 2026
Round-1 review on #778. Both HIGHs were real; verified in the code, then closed with tests
that fail against the previous commit.

HIGH 1 -- dual deadline. IndexWatchdog computed `own_now + max_runtime_s` at construction, so
arming it after file discovery handed the run a second full budget: a slow rglob plus a stalled
open_store could burn 2 x max_runtime_s, and the PR's claim that discovery is inside the cap
window was false. It now takes `started_at` and the CLI's `monotonic`, so watchdog and CLI
share ONE deadline. Proven: the new test reads 1012.0 against the previous commit where 1007.0
is correct.

HIGH 2 -- open_store, the phase this PR names as the likeliest 12h culprit, could be alarmed
but not aborted, because `interrupt` was attached only after `open_writer_store` returned.
`WriterRuntimeStore._init_runtime_db` creates the apsw connection BEFORE it runs any probe
SQL, so `open_writer_store` now takes an `on_connection` hook and hands the connection over at
that point; the CLI passes `watchdog.set_interrupt`. The whole probe window is interruptible.
Proven: against the previous commit the new test stalls to its 20s ceiling under a 1s cap
("open_store was never interruptible"); with the hook it stops at the cap with
`phase=open_store`. Verified live too -- the hook receives a real apsw.Connection with a
callable `interrupt`, and the store still opens with a valid schema fingerprint.

MEDIUM -- heartbeats stopped at the deadline (`if expired ... elif heartbeat`), so a wedge that
cannot be aborted alarmed once and then went silent again: the second half of the original
12h-silent-log failure. Heartbeats now continue past expiry.

MEDIUM -- `alarm_emitted` was set before the emit succeeded, so a failing emit let the process
exit 1 with no INDEX_RUNTIME_EXCEEDED anywhere, since the CLI defers to that flag.

...and the live-only bug that fixing it introduced: gating solely on "landed" made the alarm
REPEAT. A 6s-cap run against the real DB printed the same alarm 13 times, because
`emit_alarm` returns the Axiom *telemetry* result while its stderr and logging paths are the
documented guaranteed ones -- a falsy return is not a missing record. Now two flags:
`attempted` (set unconditionally, so it can never re-alarm) and `emitted` (only a raising emit
leaves the CLI owing a fallback). Re-checked live: exactly one alarm.

MEDIUM -- tests pinned neither shared-deadline semantics nor an open_store/discovery stall.
Four new tests cover them, each verified to fail against the previous commit.

DeepSource Python (bot round): fixed what is mine -- a redundant `import time` and a `context`
name that shadowed an outer scope. The reimports of `time` and the rich Progress columns, the
`get_db_path` shadow, and unused `force` are pre-existing in `index_fast`; the local reimports
are removed here since they sit in this hunk. **`brainlayer index --force` accepts `--force`
and does nothing with it** -- pre-existing latent bug, its own PR per canon rule 9. On
complexity: `index_fast` measured 19 before this work, 20 after round 1, and **17 now** --
below the baseline I found it at, because one deadline on one clock let three duplicated
`time.monotonic() >= deadline` checks collapse into `watchdog.raise_if_expired()`, which now
checks the clock as well as the flag. Splitting `index_fast` further is a separate refactor.

Live-check (fresh APFS clone of the real 16GB DB, real embedder, BRAINLAYER_DB on the clone
throughout): 900s cap indexed 46 chunks with heartbeats whose `last_progress_age_s` resets per
file; 6s cap tripped at 6.218s naming the phase, exit 1, exactly one alarm, 4 heartbeats;
chunks 805,420 -> 805,466; `integrity_check` ok; no dangling transaction.

Tests: 33 across the two watchdog files, 234 green across the blast radius.

Agent: brainlayerClaude-a9949327 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Sep 5, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_b9446664-4b4d-426f-913f-dc463fec268f)

@EtanHey

EtanHey commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

DeepSource Python + Shell — one line each, on 2e686002

Shell (red since ae3b2467, my run_tests.sh commit — so mine to answer):

finding line
SC2221/SC2222 ×4 on tests/*.py|tests/**/*.py and src/brainlayer/*.py|src/brainlayer/**/*.py Real, fixed. Both second alternatives are dead code: a bash case glob's * crosses /, which I verified directly — tests/*.py already matches tests/x/y/c.py, src/brainlayer/*.py already matches src/brainlayer/cli/__init__.py. Removed; the nested-path tests still pass. shellcheck on this file: 8 findings on origin/main → 4 now. These were pre-existing lines my mapping commit only shifted (241/271 → 292/322), which is why DeepSource attributed them to my diff — fixed rather than argued.

Python:

finding line
Redefining name 'index' from outer scope Real, fixed — mine. _discover_jsonl_files used index as its enumerate variable, shadowing the module-level index command. Renamed to position.
Call to next() should be wrapped in try-except ×4 Real, fixed — mine. A bare generator next() fishing out the cap alarm raises StopIteration instead of saying what went wrong. Replaced with _cap_alarm(), which also asserts there is exactly one.
subprocess.run used without explicitly defining check ×3 Real, fixed — mine. check=False made explicit at my three call sites (these tests assert on returncode themselves). I deliberately left the 17 pre-existing call sites alone: identical behaviour, and #783 edits this same file.
Body doesn't contain any code Real, fixed — mine. A while …: pass busy-wait, now a 10 ms sleep.
index_fast cyclomatic complexity ("very-high") Metric-only, won't-fix. Metric: cyclomatic complexity of index_fast. Measured with ruff C901: 19 on origin/main → 18 herelower than the baseline this PR inherited, even after adding watchdog wiring, discovery instrumentation and the exception-identity path. DeepSource's own counter reports 23–26 (different algorithm); the direction is what I can hold to, and it is down. Splitting index_fast is a real refactor and its own PR.
Unused argument 'force' Pre-existing on origin/main, not introduced here — and a real defect worth its own PR: brainlayer index --force accepts the flag and does nothing with it. Fixing it means either implementing re-index-ignoring-cache or removing a documented flag; both are behaviour changes that need their own tests, so canon rule 9 says new PR, not another commit here.

Also in this push: I rebased onto main early, and why

Carrying #783's hunk on a branch 16 commits behind main turned out to be actively broken: their rebase replaced the hand-picked list with a derivable one (they found their first cut was itself a fail-open — 8 drivers named, 12 missed, plus one named suite that never touches the app), and that derived list names tests/test_watch_logging.py, which did not exist on my branch. Fail-closed then did exactly what it should: refused to map → and my next push started a full suite.

I killed it mid-run (it was on test_arbitration.py::test_real_concurrent_writers_keep_interactive_store_searchable_under_sla — the same test reported hung earlier). The push was blocked by that kill, so nothing shipped. Then I rebased onto main, which makes the named suites exist.

State now: origin/main is an ancestor, the #783 hunk is byte-identical again (re-verified after their rebase), and the scoped gate runs 427 targeted tests, 0 fallbacks — not 4,553.

One flake worth your attention, now in scope

tests/test_launchd_hygiene.py::test_launchd_env_loader_does_not_evaluate_env_command_substitution failed once on this branch with subprocess.TimeoutExpired … timed out after 1 seconds — a 1-second timeout on a subprocess that spawns a shell, under load from concurrent seats. It passes alone, as a whole file, and on the retry. It is not mine and I have not touched it, but #783's mapping brings test_launchd_hygiene.py into scope for every CLI change, so this will now flake for other seats too. A 1 s budget looks too tight for a loaded M4.


Agent: brainlayerClaude-a9949327 · model claude-opus-5[1m]

if self._emit_cap_alarm():
self._alarm_emitted.set()
self._expired.set()
self._pull_interrupt()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical brainlayer/index_watchdog.py:297

When the main thread closes the writer connection after handling the deadline, _pull_interrupt() still invokes the stale interrupt callback, because the alarm emission delays that call and _interrupt is never cleared or synchronized with connection shutdown. Calling SQLite interrupt on a closed connection is unsafe and can crash or corrupt index shutdown. Coordinate closing the connection with _pull_interrupt() and clear the callback before close.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/index_watchdog.py around line 297:

When the main thread closes the writer connection after handling the deadline, `_pull_interrupt()` still invokes the stale `interrupt` callback, because the alarm emission delays that call and `_interrupt` is never cleared or synchronized with connection shutdown. Calling SQLite `interrupt` on a closed connection is unsafe and can crash or corrupt index shutdown. Coordinate closing the connection with `_pull_interrupt()` and clear the callback before close.

Comment on lines +327 to +333
with self._lock:
interrupt = self._interrupt
if interrupt is None or self.interrupt_attempts >= _MAX_INTERRUPT_ATTEMPTS:
return
self.interrupt_attempts += 1
try:
interrupt()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High brainlayer/index_watchdog.py:327

Concurrent _on_deadline() calls can exceed _MAX_INTERRUPT_ATTEMPTS: when interrupt_attempts is 2, both threads can pass the check and invoke interrupt, producing a fourth SQLite interrupt that may land during rollback. Protect the check and increment with _lock before calling interrupt.

         with self._lock:
-            interrupt = self._interrupt
-        if interrupt is None or self.interrupt_attempts >= _MAX_INTERRUPT_ATTEMPTS:
-            return
-        self.interrupt_attempts += 1
+            interrupt = self._interrupt
+            if interrupt is None or self.interrupt_attempts >= _MAX_INTERRUPT_ATTEMPTS:
+                return
+            self.interrupt_attempts += 1
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/index_watchdog.py around lines 327-333:

Concurrent `_on_deadline()` calls can exceed `_MAX_INTERRUPT_ATTEMPTS`: when `interrupt_attempts` is 2, both threads can pass the check and invoke `interrupt`, producing a fourth SQLite interrupt that may land during rollback. Protect the check and increment with `_lock` before calling `interrupt`.

Comment thread scripts/run_tests.sh
Comment on lines +285 to +254
"$TEST_ROOT/test_cli_direct_sqlite.py"
"$TEST_ROOT/test_cli_enrich.py"
"$TEST_ROOT/test_cli_index_watchdog.py"
"$TEST_ROOT/test_cli_launchd_mode_a.py"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium scripts/run_tests.sh:285

Changes to src/brainlayer/cli/__init__.py run the scoped cli_root_tests list without tests/test_index_watchdog_boundaryless.py, so pre-push skips that CLI integration coverage instead of escalating to the full suite. Add this test to the exhaustive list because it imports app via from brainlayer.cli import app.

           "$TEST_ROOT/test_cli_index_watchdog.py"
+          "$TEST_ROOT/test_index_watchdog_boundaryless.py"
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @scripts/run_tests.sh around line 285:

Changes to `src/brainlayer/cli/__init__.py` run the scoped `cli_root_tests` list without `tests/test_index_watchdog_boundaryless.py`, so pre-push skips that CLI integration coverage instead of escalating to the full suite. Add this test to the exhaustive list because it imports `app` via `from brainlayer.cli import app`.

EtanHey and others added 5 commits September 5, 2026 20:13
…ct why the 4h cap missed

The M1 ran `com.brainlayer.index` for 12h13m against a 4h cap with no alarm and a stdout log
silent for 12h. The plan blamed the apsw progress handler in `upsert_chunks` for being blind
inside a boundary-less phase. Measured against this apsw (3.51.2.0 / SQLite 3.51.2), that is
not true -- the handler ticks freely, and its `id=`-keyed registration works:

  FTS5 rebuild, 400k rows   7,287 ticks / 0.68s
  vec0 bulk DELETE          1,008 ticks / 0.042s
  vec0 KNN, 1.5M rows           8 ticks / 0.167s  (~48/s)
  id= registration          34,180 ticks

So a long statement inside `upsert_chunks` would have been stopped. The real hole is
narrower: the cap is enforced ONLY inside `upsert_chunks` and at `index_fast`'s per-file
boundaries. Opening the writer store (schema validation + migrations -- read-heavy, which
matches "100% CPU, WAL unchanged, only DB files open") and the first embedding-model load
have no cap of any kind. Either running long explains 12h with no alarm, with no opcode
blackout needed.

A cap at the work sites cannot cover sites that have none, so this puts it beside them.
`IndexWatchdog` runs its own thread, which a blocked main thread cannot delay:

- alarms `INDEX_RUNTIME_EXCEEDED` the moment the deadline passes, naming the phase, before
  the stuck phase unwinds -- unconditional, so it holds even for a phase issuing no SQL;
- calls `sqlite3_interrupt` on the writer connection (apsw documents cross-thread as safe).
  Measured: aborts a long FTS5 statement 0.22s into a 0.66s rebuild, leaving
  integrity_check ok, the transaction rolled back and every row readable. Retries are
  bounded at 3 so the rollback that follows survives;
- leaves `expired` set, which the main thread turns into `IndexDeadlineExceeded`;
- heartbeats phase + last-progress age every `BRAINLAYER_INDEX_HEARTBEAT_S` (default 300s),
  so a 12h-silent log cannot recur and the next real stall names its own phase.

Also: file discovery now runs inside the cap window instead of before the clock starts, and
an exception arriving while the watchdog is expired is reported as the cap rather than as an
opaque `Error:`. One cap emits one alarm -- the boundary path adds `phase` and defers to the
watchdog's alarm when it already fired.

The watchdog binds `time.monotonic` at import deliberately: several tests monkeypatch the
CLI's clock, and a watchdog reading a patched clock would both mis-fire and consume values
from those tests' iterators.

Live-checked against an APFS clone of the real 16GB DB, real embedder, never the canonical
path: a 900s-cap run indexed 26 chunks and heartbeated its phase; a 6s cap tripped at 6.153s
naming `embed_and_upsert:<file>` and exited 1; chunks went 805,045 -> 805,062 (committed work
kept), `integrity_check` ok, no dangling transaction.

Not covered: a phase that neither issues SQL nor returns can be alarmed and heartbeaten but
not aborted. Killing such a run is a policy call and its own change.

Tests: the boundary-less cap miss is reproduced first (20.99s wall against a 1.0s cap, zero
interrupts), then closed (3.88s). 27 tests across the two new files plus the 7 pre-existing
cap tests; 110 green over the blast radius.

Agent: brainlayerClaude-a9949327 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-1 review on #778. Both HIGHs were real; verified in the code, then closed with tests
that fail against the previous commit.

HIGH 1 -- dual deadline. IndexWatchdog computed `own_now + max_runtime_s` at construction, so
arming it after file discovery handed the run a second full budget: a slow rglob plus a stalled
open_store could burn 2 x max_runtime_s, and the PR's claim that discovery is inside the cap
window was false. It now takes `started_at` and the CLI's `monotonic`, so watchdog and CLI
share ONE deadline. Proven: the new test reads 1012.0 against the previous commit where 1007.0
is correct.

HIGH 2 -- open_store, the phase this PR names as the likeliest 12h culprit, could be alarmed
but not aborted, because `interrupt` was attached only after `open_writer_store` returned.
`WriterRuntimeStore._init_runtime_db` creates the apsw connection BEFORE it runs any probe
SQL, so `open_writer_store` now takes an `on_connection` hook and hands the connection over at
that point; the CLI passes `watchdog.set_interrupt`. The whole probe window is interruptible.
Proven: against the previous commit the new test stalls to its 20s ceiling under a 1s cap
("open_store was never interruptible"); with the hook it stops at the cap with
`phase=open_store`. Verified live too -- the hook receives a real apsw.Connection with a
callable `interrupt`, and the store still opens with a valid schema fingerprint.

MEDIUM -- heartbeats stopped at the deadline (`if expired ... elif heartbeat`), so a wedge that
cannot be aborted alarmed once and then went silent again: the second half of the original
12h-silent-log failure. Heartbeats now continue past expiry.

MEDIUM -- `alarm_emitted` was set before the emit succeeded, so a failing emit let the process
exit 1 with no INDEX_RUNTIME_EXCEEDED anywhere, since the CLI defers to that flag.

...and the live-only bug that fixing it introduced: gating solely on "landed" made the alarm
REPEAT. A 6s-cap run against the real DB printed the same alarm 13 times, because
`emit_alarm` returns the Axiom *telemetry* result while its stderr and logging paths are the
documented guaranteed ones -- a falsy return is not a missing record. Now two flags:
`attempted` (set unconditionally, so it can never re-alarm) and `emitted` (only a raising emit
leaves the CLI owing a fallback). Re-checked live: exactly one alarm.

MEDIUM -- tests pinned neither shared-deadline semantics nor an open_store/discovery stall.
Four new tests cover them, each verified to fail against the previous commit.

DeepSource Python (bot round): fixed what is mine -- a redundant `import time` and a `context`
name that shadowed an outer scope. The reimports of `time` and the rich Progress columns, the
`get_db_path` shadow, and unused `force` are pre-existing in `index_fast`; the local reimports
are removed here since they sit in this hunk. **`brainlayer index --force` accepts `--force`
and does nothing with it** -- pre-existing latent bug, its own PR per canon rule 9. On
complexity: `index_fast` measured 19 before this work, 20 after round 1, and **17 now** --
below the baseline I found it at, because one deadline on one clock let three duplicated
`time.monotonic() >= deadline` checks collapse into `watchdog.raise_if_expired()`, which now
checks the clock as well as the flag. Splitting `index_fast` further is a separate refactor.

Live-check (fresh APFS clone of the real 16GB DB, real embedder, BRAINLAYER_DB on the clone
throughout): 900s cap indexed 46 chunks with heartbeats whose `last_progress_age_s` resets per
file; 6s cap tripped at 6.218s naming the phase, exit 1, exactly one alarm, 4 heartbeats;
chunks 805,420 -> 805,466; `integrity_check` ok; no dangling transaction.

Tests: 33 across the two watchdog files, 234 green across the blast radius.

Agent: brainlayerClaude-a9949327 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s the cap

Round-2 review on 345fb80. Both HIGHs and all three mediums were real; each fix has a guard
proven to fail against 345fb80.

HIGH -- discovery was charged against the cap but not watched. Arming the watchdog after the
rglob fixed only the arithmetic: a walk stuck on a slow mount stayed silent past the deadline
until it returned. The watchdog is now armed BEFORE discovery with `phase=discover`, and
`_discover_jsonl_files` consumes the generator so the cap is checked as the walk proceeds
(every 64 entries -- enough to bound a stall, cheap on a 12,000-file tree). Guard against
345fb80: "the run was silent while discovery was stuck".

HIGH -- after expiry every Exception was rewritten as INDEX_RUNTIME_EXCEEDED. An expired
watchdog means the cap is part of the story, not that this exception IS the cap: a schema
mismatch, an I/O error or an OOM landing during the unwind lost its only actionable cause. The
handler now reports the cap alarm AND keeps the original type and message, on stderr and in
`unwind_error`. Only apsw's `InterruptError` (and `IndexDeadlineExceeded`) are treated as the
interrupt we asked for. Guard against 345fb80: "the original failure was rewritten as the
cap: … cause=RuntimeError" -- the RuntimeError's message was gone.

MEDIUM -- injecting the CLI's clock let the poll thread consume monkeypatched values. Now two
clocks: the caller's, read exactly once by `start()` on the calling thread to carry the
already-spent budget into the wall domain, and this module's import-time wall clock, the only
one the thread reads. One deadline, no shared-clock race. Guard counts caller-clock reads
during a second of polling.

MEDIUM -- `on_connection` failures were swallowed, so an open could look armed while being
non-abortable. Both `runtime_store` paths now log at error level, and the CLI's hook records
`note_uninterruptible(reason)`, which appears in every heartbeat and in the cap alarm.

MEDIUM -- tests did not cover either fail-open hole. Four new tests do, each verified failing
against 345fb80.

LOW -- `raise_if_expired` could trip on the clock without setting `expired` or pulling the
interrupt. It now routes through the same `_on_deadline()` the thread uses, so both halves
agree about the cap.

Also fixed here, and NOT caught by any suite run so far: `tests/test_index_watchdog_boundaryless.py`
poisoned `brainlayer.index_new` for the whole session. `index_new` binds `open_writer_store` at
ITS import time; `_install` patched `runtime_store.open_writer_store` first and imported
`index_new` second, so index_new captured the fake permanently — monkeypatch restores
runtime_store, not index_new — and `test_context_pipeline` then failed with
`'_StallingRuntimeStore' object has no attribute 'upsert_chunks'`. The module is now imported
at test-module scope before any patch, and `_patch_open_writer_store` patches every binding
through monkeypatch. Worth noting: the FULL suite was green on this; the targeted set caught it.

Live-check (fresh APFS clone of the real 16GB DB, real embedder, canonical never touched):
900s cap indexed 38 chunks; 6s cap tripped at 6.203s, exit 1, exactly one alarm, 3 heartbeats;
a 0.5s cap over the real ~12,000-file tree still reached the write phase, so watching discovery
costs nothing measurable. Chunks 805,636 -> 805,674; integrity_check ok; no dangling transaction.

148 green across the blast radius.

Agent: brainlayerClaude-a9949327 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oot hunk verbatim

Measured, not guessed: `changed-only` was falling open to the FULL suite on every push from
this lane, because both changed source paths are unmapped.

    WARNING: changed-only scope found an unmapped source change; falling back to full pytest unit suite
    WARNING: unmapped: src/brainlayer/index_watchdog.py src/brainlayer/cli/__init__.py

4,553 tests in 7m51s on the M4 -- which LANE-RULES forbids and which hung a sibling seat's
concurrency-SLA test. The comma-separated `BRAINLAYER_CHANGED_FILES` was correct all along; the
mapping table was the real gap, so commas alone never fixed it. After: 0 fallbacks, 133 targeted
tests, whole gate ~22s.

Two mappings, and only one of them is mine.

`src/brainlayer/index_watchdog.py` is mine: the generic `src/brainlayer/X.py -> test_X.py` rule
looks for `test_index_watchdog.py`, which does not exist because the tests are split
unit/boundaryless plus the CLI's own cap suite. Mapped FAIL-CLOSED, per the rule already on main
for `src/brainlayer/__init__.py` (#762): all three must exist to count as mapped, because none
is sufficient alone -- `_unit` drives the watchdog on an injected clock and never starts the
CLI, `_boundaryless` drives the real CLI against stalls that only unwind on interrupt, and
`test_cli_index_watchdog` covers the commit-boundary path the watchdog does not own. A per-file
`mapped=1` would let a renamed suite silently narrow the gate while it still looked mapped.

`src/brainlayer/cli/__init__.py` is NOT mine: PR #783 (`wt/cli-map`, open) already maps it, and
its version is better than the one I first wrote -- mine was per-file `mapped=1`, i.e. the
fail-open that #762 explicitly ruled against. That hunk is carried here BYTE-IDENTICAL to
tests are #783's too and are not duplicated here. When #783 merges, this branch should rebase
onto main and the duplicate disappears with no conflict, since the content matches exactly.

Agent: brainlayerClaude-a9949327 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tually mine

Shell (red since ae3b246, my run_tests.sh commit):
  - `tests/*.py|tests/**/*.py` and `src/brainlayer/*.py|src/brainlayer/**/*.py` each raised an
    SC2221/SC2222 pair. Both second alternatives are dead: a bash `case` glob's `*` crosses `/`,
    verified directly -- `tests/*.py` already matches `tests/x/y/c.py`, and
    `src/brainlayer/*.py` already matches `src/brainlayer/cli/__init__.py`. Removed, behaviour
    identical, and the nested-path tests still pass. shellcheck on this file: 8 findings on
    origin/main -> 4 now. The lines were pre-existing text my mapping commit had only shifted
    (241/271 -> 292/322), so DeepSource attributed them to this diff; fixed rather than argued.

Python:
  - `Redefining name 'index' from outer scope` -- mine: `_discover_jsonl_files` used `index` as
    its enumerate variable, shadowing the module-level `index` command. Renamed to `position`.
  - `Call to next() should be wrapped in try-except` x4 -- mine: fishing the cap alarm out with
    a bare generator `next()` raises StopIteration instead of saying what went wrong. Replaced
    with `_cap_alarm()`, which also asserts there is exactly one.
  - `subprocess.run used without explicitly defining check` x3 -- mine: made `check=False`
    explicit at my three call sites (these tests assert on returncode themselves). Left the 17
    pre-existing call sites alone: identical behaviour, and #783 edits this same file.
  - `Body doesn't contain any code` -- mine: a `while ... : pass` busy-wait. Now sleeps 10ms.

Not fixed, with reasons on the PR: `index_fast` cyclomatic complexity (metric only -- measured
18 here against 19 on origin/main, i.e. LOWER than the baseline this PR inherited) and unused
argument `force` (pre-existing on main; `brainlayer index --force` silently does nothing, which
is a real defect and its own PR per canon rule 9).

Also re-synced the carried #783 CLI-root hunk: its seat's rebase replaced the hand-picked list
with a DERIVABLE one after finding the first cut was itself a fail-open (8 drivers named, 12
missed, and one named suite that never touches the app). My copy was the stale version; it is
byte-identical to #783 again.

93 green across the touched suites.

Agent: brainlayerClaude-a9949327 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Sep 5, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_8f832cb8-6a5a-41ee-9254-4f80849f9d38)

@EtanHey

EtanHey commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto main with #783 merged — 287a9163

origin/main (24482318, which includes #783 0ade8272) is now an ancestor. The duplicate mapping hunk vanished exactly as predicted: the tree has one src/brainlayer/cli/__init__.py) case — main's — and my mapping commit now adds only src/brainlayer/index_watchdog.py).

Where it conflicted I took main's, per your instruction, then reconstructed my side deterministically rather than hand-merging markers: scripts/run_tests.sh = main's file + my index_watchdog case; tests/test_run_tests_script.py = main's file + my three tests (46 test functions, all passing). One earlier attempt at a marker-level merge silently interleaved two test bodies into a NameError, which is why I rebuilt from the two known-good sides instead.

Scoped gate: 0 fallbacks, 429 targeted tests in 80s, gate passed. Not the 4,553-test full suite.

DeepSource lines — already posted

The metric/issue lines are in #778 (comment) and still hold on this head. In short: Shell — 4 × SC2221/SC2222 real and fixed (dead **/*.py alternatives; bash case * crosses /, verified), shellcheck on that file 8 on main → 4 here; Pythonindex shadow, 4 bare next(), 3 implicit subprocess.run checks and an empty loop body all real and fixed; index_fast complexity is metric-only won't-fix (metric: cyclomatic complexity), measured ruff C901 19 on main → 18 here; Unused argument 'force' is pre-existing on main and a real defect for its own PR (brainlayer index --force silently does nothing).

Two disclosures on this push

  1. One commit subject is now stale. 20cc97aa still reads "…and carry fix(prepush): map src/brainlayer/cli/__init__.py to the CLI command suites (XS) #783's CLI-root hunk verbatim", which stopped being true the moment fix(prepush): map src/brainlayer/cli/__init__.py to the CLI command suites (XS) #783 landed — that commit now adds only the index_watchdog mapping, and its body says so. Correcting the subject needed a git reset --hard, which this environment's safety guard blocks; I did not retry it or work around the guard. The commit content is correct and minimal — only the subject line is stale. Say the word if you want it reworded and I will ask for the guard to be lifted, or squash on merge.

  2. tests/test_launchd_hygiene.py::test_launchd_env_loader_does_not_evaluate_env_command_substitution flaked once earlier on this branch with subprocess.TimeoutExpired … timed out after 1 seconds — a 1 s budget for a subprocess that spawns a shell, under concurrent-seat load. It passed alone, as a whole file, on retry, and in both gate runs since. Not mine and untouched by me, but fix(prepush): map src/brainlayer/cli/__init__.py to the CLI command suites (XS) #783's mapping now pulls test_launchd_hygiene.py into scope for every CLI change, so other seats will meet it.


Agent: brainlayerClaude-a9949327 · model claude-opus-5[1m]

def _run(self) -> None:
next_heartbeat = self._wall_started_at + self._heartbeat_interval_s
while not self._stop.wait(self._poll_interval_s):
now = self._wall()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High brainlayer/index_watchdog.py:272

A timed-out wait() continues into _on_deadline() even after stop() has set _stop, so a successful run can still emit INDEX_RUNTIME_EXCEEDED and interrupt its closing connection. Recheck _stop immediately after wait() returns before reading the clock or enforcing the deadline.

Suggested change
now = self._wall()
if self._stop.is_set():
return
now = self._wall()
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/index_watchdog.py around line 272:

A timed-out `wait()` continues into `_on_deadline()` even after `stop()` has set `_stop`, so a successful run can still emit `INDEX_RUNTIME_EXCEEDED` and interrupt its closing connection. Recheck `_stop` immediately after `wait()` returns before reading the clock or enforcing the deadline.

@EtanHey
EtanHey merged commit 78d92bc into main Sep 5, 2026
18 checks passed
@EtanHey
EtanHey deleted the wt/index-cap branch September 5, 2026 17:34
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.16"
(kept in the PR body, never exported into the push env — two tests assert
the check fails without a reason).

Why a release, 22 minutes after the last one. v1.5.15 was tagged at
51a72a0 (20:12:29). #778 — the index runtime watchdog — merged at
78d92bc (20:34:55), so it is NOT in 1.5.15. Verified:
`git merge-base --is-ancestor 78d92bc v1.5.15^{}` -> false, and the
installed keg's python raises
`ModuleNotFoundError: No module named 'brainlayer.index_watchdog'`.
That watchdog is the fix for the M1's nightly `brainlayer index` job,
which ran 14h03m at ~100% CPU on 09-05, 10h past its own 4h cap. Both
Macs' 03:15 index jobs are `launchctl disable`d as a stopgap and come
back ON only after 1.5.16 is installed and the watchdog is proven
present.

Nine commits ride along: #774 #777 #780 #778 #783 #785 #786 #787 #788.

Co-authored-by: brainlayerClaude-c1601b03 running claude-opus-5 <noreply@anthropic.com>
EtanHey added a commit that referenced this pull request Sep 5, 2026
…rovably in the tag (XS)

v1.5.15 was tagged at 51a72a0 (2026-09-05 20:12:29). #778, the index runtime
watchdog, merged at 78d92bc 20:34:55 — 22 minutes LATER. A deploy brief, a
collab post, and a report to Etan all said "1.5.15 carries #778", and a whole M1
deploy was cut for a fix the release did not contain; both Macs' 03:15 index jobs
had to be disabled at 00:50 as a stopgap. Nobody lied — the claim was a MEMORY of
a merge that happened the same evening. So make the claim checkable.

`scripts/release_tag_contains.py <tag> <PRs/SHAs>` resolves each claim (a PR
number through `gh pr view --json mergeCommit`, or a SHA directly), asks
`git merge-base --is-ancestor <sha> <tag>^{}`, prints a markdown `claim | sha |
subject | status` table, and exits non-zero if anything is NOT IN. Unprompted, it
also lists what the tag ACTUALLY carries since the previous tag — so a release
can list its contents instead of someone's recollection. That predecessor must be
a FULL release (`--match 'v*' --exclude '*-*'`), the same policy `.githooks/pre-push`
already applies: a nightly or an rc would start the range short.

`--assert-module <importable>` checks the same claim from the other end, running
the INSTALLED keg's python. That is what would have caught this incident second-hand:
`import brainlayer.index_watchdog` -> ModuleNotFoundError.

Real run, both halves firing on the actual incident:

    $ scripts/release_tag_contains.py v1.5.15 '#778' --assert-module brainlayer.index_watchdog
    | #778 | 78d92bc | fix(index): enforce the runtime cap ... | NOT IN |
    import brainlayer.index_watchdog | FAILED — ModuleNotFoundError
    FAIL: v1.5.15 does NOT contain 1 of 1 claimed items.
    EXIT=1

Fail-closed throughout: a `gh` failure, an unmerged PR, an unfetched commit, and a
missing keg python are each NOT IN / FAILED, never a quiet pass. A 7+ digit claim is
read as a SHA, not handed to `gh pr view` as some unrelated PR number.

Wired in: AGENTS.md "Release safety" now requires the table in every release receipt
and forbids a deploy brief naming a fix that did not pass. `scripts/run_tests.sh`
gains a mapping for the new script — no generic `scripts/*.py` rule exists, and an
unmapped gate script is the same fail-open the gate closes.

18 tests in tests/test_release_tag_contains.py + 1 mapping test.

Agent: brainlayerClaude-74c06b22 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
EtanHey added a commit that referenced this pull request Sep 5, 2026
…rovably in the tag (XS) (#792)

* feat(release): a tag-contains gate — a release may only claim fixes provably in the tag (XS)

v1.5.15 was tagged at 51a72a0 (2026-09-05 20:12:29). #778, the index runtime
watchdog, merged at 78d92bc 20:34:55 — 22 minutes LATER. A deploy brief, a
collab post, and a report to Etan all said "1.5.15 carries #778", and a whole M1
deploy was cut for a fix the release did not contain; both Macs' 03:15 index jobs
had to be disabled at 00:50 as a stopgap. Nobody lied — the claim was a MEMORY of
a merge that happened the same evening. So make the claim checkable.

`scripts/release_tag_contains.py <tag> <PRs/SHAs>` resolves each claim (a PR
number through `gh pr view --json mergeCommit`, or a SHA directly), asks
`git merge-base --is-ancestor <sha> <tag>^{}`, prints a markdown `claim | sha |
subject | status` table, and exits non-zero if anything is NOT IN. Unprompted, it
also lists what the tag ACTUALLY carries since the previous tag — so a release
can list its contents instead of someone's recollection. That predecessor must be
a FULL release (`--match 'v*' --exclude '*-*'`), the same policy `.githooks/pre-push`
already applies: a nightly or an rc would start the range short.

`--assert-module <importable>` checks the same claim from the other end, running
the INSTALLED keg's python. That is what would have caught this incident second-hand:
`import brainlayer.index_watchdog` -> ModuleNotFoundError.

Real run, both halves firing on the actual incident:

    $ scripts/release_tag_contains.py v1.5.15 '#778' --assert-module brainlayer.index_watchdog
    | #778 | 78d92bc | fix(index): enforce the runtime cap ... | NOT IN |
    import brainlayer.index_watchdog | FAILED — ModuleNotFoundError
    FAIL: v1.5.15 does NOT contain 1 of 1 claimed items.
    EXIT=1

Fail-closed throughout: a `gh` failure, an unmerged PR, an unfetched commit, and a
missing keg python are each NOT IN / FAILED, never a quiet pass. A 7+ digit claim is
read as a SHA, not handed to `gh pr view` as some unrelated PR number.

Wired in: AGENTS.md "Release safety" now requires the table in every release receipt
and forbids a deploy brief naming a fix that did not pass. `scripts/run_tests.sh`
gains a mapping for the new script — no generic `scripts/*.py` rule exists, and an
unmapped gate script is the same fail-open the gate closes.

18 tests in tests/test_release_tag_contains.py + 1 mapping test.

Agent: brainlayerClaude-74c06b22 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(release-gate): scrub the ambient GIT_* so the gate answers for its own repo

Found by running the gate's own pre-push: git exports GIT_DIR/GIT_INDEX_FILE into
hooks and into `git rebase --exec`, and they win over `cwd`. Every fixture in
tests/test_release_tag_contains.py died on `git commit` under the hook while
passing bare — it was building its temp repo into the REAL one.

The same hole is in the script: a `release_tag_contains.py` invoked from a hook
would have graded a DIFFERENT repository and said so with a straight face — the
same unfalsifiable claim it exists to close. Both now scrub GIT_* before running
git, pinned by test_an_inherited_git_dir_cannot_redirect_the_gate.

Agent: brainlayerClaude-74c06b22 (claude-opus-5[1m])

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(ci): register the release-gate test in the git-shellout scrub registry

`test_git_shellout_tests_scrub_inherited_git_env` is a CLOSED registry: it globs
`tests/test_*.py`, keeps every file containing the literal `["git",`, and asserts
the set equals a hardcoded allowlist. `tests/test_release_tag_contains.py` shells
out to git -- it must, that is the gate -- so it joined the glob set and the
equality assertion failed with "Extra items in the left set". That, and only
that, was `test (3.11)`.

Two halves, because the guard has two assertions:
- add the file to the expected set;
- rename its `_clean_env` helper to `_clean_git_env`, one of the three names the
  guard's second assertion accepts. The scrub itself was already correct
  (`if not key.startswith("GIT_")`); only the helper's NAME was unrecognized, so
  the file would still have failed after being added to the set. Renaming keeps
  the guard exactly as strict -- widening its accepted-name tuple to admit a
  generic `_clean_env` would have been the fail-open answer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(prepush): escalate when the release gate's own test is missing

The mapping this PR added carries the comment "an unmapped gate script is the
same fail-open the gate itself exists to close". It was only half true. The
`[ -f "$test_path" ]` guard leaves `mapped=0` when
`tests/test_release_tag_contains.py` is deleted or renamed, and the escalation
fallback below only ever looked at `src/brainlayer/*.py` -- so a change to the
release gate script itself mapped to ZERO pytest targets and still printed
"BrainLayer test gate passed." Measured, before the fix:

    ==> pytest unit suite
    SKIP: changed-only scope found no mapped pytest targets
    ...
    BrainLayer test gate passed.

That is the unchecked-claim fail-open this PR exists to close, reintroduced one
level up, in the one mapping whose whole subject is release claims nobody
verified. `scripts/release_tag_contains.py` now joins `src/brainlayer/*.py` in
the escalation case, so a missing test forces the full suite and NAMES what
forced it.

Also answers DeepSource PYL-W1510 (Subprocess run with ignored non-zero exit,
bug-risk/minor) on the mapping test added by this PR: the file's own newer
convention is an explicit `check=False` plus `# noqa: S603` with the reason --
the returncode is what these tests assert on, so raising would hide it. The new
call simply had not followed it. Not a won't-fix; the lint was right.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L Large change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant