Skip to content

fix(watcher): deleted JSONL is not a parse alarm; verify bytes before resuming a same-inode path; name real log files (Refs #732) - #1656

Draft
EtanHey wants to merge 2 commits into
mainfrom
claude/draft-pr-review-vh427p
Draft

EtanHey wants to merge 2 commits into
mainfrom
claude/draft-pr-review-vh427p

Conversation

@EtanHey

@EtanHey EtanHey commented Nov 5, 2026 •

Copy link
Copy Markdown
Owner

Change and evidence

Task marker: cloud-credit-20261105-brainlayer-hammer-watcher732
Requested base: main@d6b82f04f6baa00081c9dc38afca2ada9d114c78 · Actual base: same (fetched inside the cloud container before any edit)
Branch / head: claude/draft-pr-review-vh427p @ 8fc4f2925c8d7f25ff6f048c9ab180004a041930 (2 commits)
Ownership recheck: #732 open, no linked or closing PR, and no open watcher/#732 PR at dispatch (searched open PRs #1636–#1643; none touch this).

What I found on current main (traced before editing)

  • Since the issue was filed, poll_once drops _file_ingestion_failures for paths it no longer discovers. So a deleted file no longer re-alarms on every poll. What is still broken:
    1. Wrong classification (racy discover). When a file is listed by discovery and then deleted before the read, FileNotFoundError goes out as watcher_file_ingestion_failed / "bytes could not be safely parsed" and keeps watcher-health.json alerting.
    2. Silent data loss when an inode is reused. Identity is the inode alone. ext4 hands a freed inode to the next file immediately (shown in this container: 679971 → 679971). A path that is deleted and recreated (or overwritten in place) under the same inode resumes at the old offset. That silently skips the head of the new file, or misparses mid-record and quarantines it.
  • Log paths: the shipped plist, AGENTS.md and BrainBar already name ~/Library/Logs/brainlayer/watch.{out,err}.log. What was still missing was any way for the process itself to say where it actually writes.

Fix (src/brainlayer/watcher.py)

  • Deletion is confirmed, never inferred. Only lstat → ENOENT counts. A permission or I/O error, or a path discovery merely missed, does not. A confirmed-vanished source (at read time, or a tracked tailer whose path disappeared between polls) is recorded once (vanished_source_count_total in health, one INFO line). It never reaches the parse alarm.
  • The source checkpoint shape is unchanged and no checkpoint is deleted. The tailer and registry entry are kept. Startup prune_missing_files stays the only place that retires checkpoints, with mount evidence.
  • Identity check on bytes. Each tailer remembers the last 64 consumed bytes ending at its offset. When it reads a file that changed, it first checks that those bytes (plus the head of its buffer) are still at the offset:
    • match: resume (a rename away and back does not re-ingest)
    • mismatch: start again from 0 under a new registry generation via mark_rewind, exactly like the existing inode-change path. Confirmations still in flight for the old file cannot advance the new one, because the existing generation arbitration rejects them.
    • file shorter than the offset: unchanged. check_rewind still runs first, so checkpoint-restore archival (on_rewind) keeps working.
    • Starting again is safe because the watcher's chunk ids are rt-<stem>-<content_hash> with dedupe. A skip is not safe.
  • Log destinations come from the kernel. describe_stream_destination(fd) uses F_GETPATH on macOS and /proc/self/fd on Linux. The watcher logs Watcher logs: stdout -> …, stderr -> … right after the unchanged JSONL watcher started: line, and writes log_destinations into watcher-health.json.
  • AGENTS.md: tells operators to trust that surface rather than a documented path, records that agents never delete or archive the frozen ~/.local/share/brainlayer/logs/watch.* files (Etan decides), and documents vanished_source_count_total.
  • Not touched: model attribution, Swift, launchd plists, the CLI, registry merge/flush logic.

Proof (synthetic files only, production JSONLWatcher.poll_once path; races forced deterministically)

New tests/test_watcher_vanished_sources.py (10 tests). The last test in tests/test_watch_logging.py (a real brainlayer watch subprocess) now also waits for the stderr line that names its own redirected file.

Test old main this branch
deleted between discovery and read: no parse alarm, not alerting, then reappears and is ingested FAIL (alarm "could not be safely parsed") pass
same inode with new, longer bytes after vanishing: full re-ingest plus generation bump FAIL (['new tail'] only; head silently dropped) pass
recreated inside one poll window with reused inode FAIL ([]; misparsed and quarantined) pass
real brainlayer watch subprocess names its stderr file FAIL (line absent; 90 s deadline) pass
child process with redirected fds / pipes → describe_stream_destination FAIL (new API) pass
deleted tracked file counted once over 5 polls; checkpoint kept FAIL (new counter only; old code had no alarm here either) pass
on disk but missing from discovery is not vanished FAIL (new counter only) pass
health carries log_destinations FAIL (new key) pass
rename away and back resumes without re-ingest (guard) pass pass
new inode after vanishing ingests from 0 (guard) pass pass
shorter after vanishing still fires on_rewind (guard) pass pass

Old main: 8 failed / 3 passed. Branch: 11/11 passed.

Mutants (each run against test_jsonl_watcher.py + the new file). All killed:

  • drop the anchor verify: 2 fail
  • drop the read-time vanish branch: 1 fail
  • drop mark_rewind on replace: 1 fail
  • _source_is_gone always True: 1 fail
  • drop the between-poll vanish detection: 1 fail

Commands run (Linux cloud container, Python 3.13, lightweight venv in .build.noindex/ with no ML deps):

  • pytest tests/test_jsonl_watcher.py: 112 passed on base, 112 passed on branch
  • pytest over test_watcher, test_watcher_bridge, test_watch_logging, test_watcher_heartbeat_{contract,ratchet}, test_watcher_provenance_ingest, test_watch_backfill_cli, test_observability_surface, test_health_facts, test_retirement_reference_docs, test_retire_python_mcp, test_context_pipeline, test_ingest_codex, test_release_tag_contains, test_release_verify_signatures, test_jsonl_watcher and the new file: 487 passed, 4 skipped, 0 failed
  • ruff check + ruff format --check on the touched files: clean

Limitations / UNRUN

  • The full suite was NOT run. The cloud venv omits sentence-transformers, mcp, spacy and others, so some modules cannot even be collected here (for example test_store_response_clarity.py needs mcp).
  • No Mac. The macOS F_GETPATH branch and APFS inode behaviour are UNRUN locally. Hosted macOS CI may exercise it. No installed, live or deployed claim.
  • The repo pre-push hook is not installed in this cloud clone (core.hooksPath unset). It was not bypassed; it simply did not run.
  • Not covered: a same-inode replacement while the watcher is down. After a restart the anchor is captured lazily from whatever file is there now. Startup pruning plus generation tombstones cover deleted paths, but not this case.
  • Edge behaviour change: a same-inode file truncated in place and then re-grown past the old offset between two polls used to go undetected (silent skip). It is now re-ingested from 0, without on_rewind archival.
  • Size: 419 lines added (252 of them tests), slightly over the 400-line target.
  • Cost: one extra 64–128 byte read per poll, only for files whose stat changed (idle files are still skipped by _can_skip_unchanged).

Risks for the reviewer

  • Generation/arbitration: the replace path reuses mark_rewind and also resets the in-loop snapshot to (0, b""), so a later normalize or index failure cannot restore the stale offset. Please check this against concurrent flush confirmations.
  • An unmounted volume makes every tracked file under it lstat ENOENT. Result: one INFO line per file, once. Checkpoints are untouched, and on remount the anchor matches and the file resumes.

Handoff → orc (BrainLayer lead snoozed)

Draft only. Not merged, not marked ready, no tag/release/install/deploy, no production DB, service or log touched, and no old logs deleted or archived. Uses Refs #732, not Fixes: deleting or archiving the stale log files and a "refuse to start" guard are deliberately left to Etan. Next steps: the Codex↔Claude pair review plus CodeRabbit, and macOS CI on the exact head. Peer proof only; this is not final cross-vendor acceptance.

Loop-record: implementer=Anthropic Claude Code cloud/effort high; reviewer=none; rounds=0; defects=0


Generated by Claude Code

Note

Fix watcher: treat deleted JSONL as vanished, verify same-inode source bytes, and name real log destinations

  • A source file deleted between discovery and read is now counted as vanished (once per path), instead of raising a parse alarm or ingestion failure. Absence is confirmed via lstat in _source_is_gone; only FileNotFoundError counts.
  • JSONLTailer now retains up to _SOURCE_ANCHOR_BYTES (64) of consumed bytes as an anchor. Before resuming at a nonzero offset, _verify_anchor compares these bytes to the file; on mismatch, the tailer resets to offset zero, is marked replaced, and the watcher starts a new registry generation.
  • Adds describe_stream_destination, which reports the real stdout/stderr destinations (via F_GETPATH or /proc/self/fd). The watcher logs these at startup and includes them plus the vanished-source count in its health snapshot.
  • Risk: same-inode files whose checked bytes differ from the remembered anchor are fully re-read from offset zero under a new registry generation — see _verify_anchor and the quarantine-clearing logic in poll_once. Registry checkpoints for vanished paths are retained, not removed.
  • Covered by new tests in test_watcher_vanished_sources.py and updated startup-log assertions in test_watch_logging.py.

Macroscope summarized 8fc4f29.

…re resuming a same-inode path

Refs #732.

- FileNotFoundError on a source confirmed gone by lstat ENOENT no longer raises
  watcher_file_ingestion_failed ("bytes could not be safely parsed") or keeps the
  health surface alerting. It is recorded once (vanished_source_count_total) and
  logged at INFO. The tailer and registry checkpoint are kept unchanged.
- The tailer remembers the last 64 consumed bytes ending at its offset and, on each
  read of a changed file, checks they are still there. A mismatch under the same
  inode (delete+recreate given the freed inode, or replaced in place) restarts the
  file at offset 0 with a new registry generation instead of silently skipping
  its head. Re-ingest is idempotent (content-hash ids + dedupe); a skip is loss.
- A rename away and back resumes; a shorter reappearance still takes check_rewind.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L3NoZFRozrUzGpnqqs5EtQ
…ng docs

Refs #732.

The watcher now reads its own stdout/stderr targets from the kernel
(F_GETPATH on macOS, /proc/self/fd on Linux) and states them in a
"Watcher logs: stdout -> ..., stderr -> ..." startup line and in
watcher-health.json as log_destinations. AGENTS.md points operators at that
surface and records that agents never delete or archive the frozen
pre-2026-05-18 ~/.local/share/brainlayer/logs/watch.* files.

The real `brainlayer watch` subprocess test now waits for the stderr line
naming its own redirected file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L3NoZFRozrUzGpnqqs5EtQ
@coderabbitai

coderabbitai Bot commented Nov 5, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@deepsource-io

deepsource-io Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in d6b82f0...8fc4f29 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 Nov 5, 2026 5:18a.m. Review ↗
Swift Nov 5, 2026 5:18a.m. Review ↗
JavaScript Nov 5, 2026 5:18a.m. Review ↗
Shell Nov 5, 2026 5:18a.m. Review ↗
Secrets Nov 5, 2026 5:18a.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.

Comment thread src/brainlayer/watcher.py
return "closed"
if stat.S_ISREG(mode):
try:
import fcntl

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 'fcntl' from outer scope


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


def test_shorter_file_after_vanishing_still_takes_the_rewind_path(tmp_path, monkeypatch):
rewinds: list[tuple[str, int, int]] = []
watcher, sessions, flushed, _alarms = _make_watcher(

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 variable 'flushed'


An unused variable takes up space in the code, and can lead to confusion, and it should be removed. If this variable is necessary, name the variable _ to indicate that it will be unused, or start the name with unused or _unused.

@github-actions

github-actions Bot commented Nov 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.

Hosted success gives no verdict on mapped bytes, search p50/p95, idle CPU, hotlane backlog CPU, or fallback replay debt.

Row Status Value (measured by this run) Method Notes
commit provenance 🟢 GREEN measured 8fc4f2925c8d == PR head · checkout 8fc4f2925c8d 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 41876229562 · main d6b82f04f6ba · 2026-11-04T17:18:27Z) 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. The calibrated socket collector can license p50/p95; every absent measured path stays locked, so missing collection never passes as permission for a hand edit. Boundary: the comparator is this PR's checkout of ci_ratchet_table.py, diff-reviewable, not tamper-proof.
provenance 🟢 GREEN stamped 8fc4f2925c8d == 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 explicit PR head in ratchet.yml — 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 local-only; not enforced by hosted CI. 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.
pre-push: delete-only push runs no suite 🟢 GREEN 0 suite starts · exit 0 real checkout hook · delete-only stdin · sentinel suite runner Bug SHA: 44ecbf5ba6f1ada3b965797559eee90c94b45595 (RED replay, 2026-10-06). Fix: #1084; measured at 8fc4f2925c8d7f25ff6f048c9ab180004a041930. Ceiling: 0 suite starts; hook must exit 0 and announce the delete-only skip.
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 local-only; not enforced by hosted CI. 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 local-only; not enforced by hosted CI. 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 local-only; not enforced by hosted CI. 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.
hotlane backlog CPU ⚪ n/a n/a — live hotlane CPU evidence requires an installed Mac CPU/wall for backlog-encoding cycles · rolling 600 s · installed Mac local-only; not enforced by hosted CI. Default budget 60% of one core; BRAINLAYER_HOTLANE_CPU_BUDGET_PERCENT overrides. At least 30s of active backlog cycles required; yielded time excluded. Measures installed code.
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.
BrainBar renders no enrichment status 🟢 GREEN 19 actual-view captures; active pending-store/replay retained macOS DEBUG binary · AppKit actual views · Vision OCR + icon RGBA comparison · private synthetic state Missing render capability/evidence is RED. Historical data is retained; this is source-build proof, not installed-app proof.
Watcher heartbeat freshness and recovery 🟢 GREEN 13 cases; polls 1 -> 2 private Python producer -> compiled Swift consumer Preserves genuine silence; independent agent stores cannot revive the watcher. Not a load or installed-UI measurement.
maintenance quiesce: services stay down under a reviving watchdog ⚪ n/a n/a — macOS replay trigger-gated: no affected path or ratchet:quiesce label real launchd · GitHub macOS runner · real maintenance + fleet-watchdog Fix: #1076 (bc6d690f4331387485678d0247e19e14e6ef8d97). Ceiling: 0 revivals; supervisor held, both resumed, no hold marker.
no cloud model call reachable anywhere 🟢 GREEN 520 source files; 460 installed imports; 3 native tests; wheel 6da0636a4e50; 0 candidate attempts immutable installed wheel (default + dev) + AST source + actual native BrainBar Synthetic private fixtures; SDK absence gated on merged R10b. Exact-head candidate proof, not deployment.

🟢 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 8fc4f2925c8d · PR head 8fc4f2925c8d · checkout 8fc4f2925c8d · run · updated 2026-11-05 06:04:07 UTC

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants