fix(search): walk and read the brain through an os.Root (#157, slice 1) #161

Merged
mathias merged 3 commits from fix/search-root-157 into main 2026-10-03 19:34:46 +00:00
Owner

Refs #157, option 1, slice 1 (search). Later slices cover vectorstore, the context excerpt, ReadNote/brain_get, graphsync, the wiki inventory, wing indexes, the ingest merge and the provenance re-stamp.

Why

search.Query and hydrate read the brain by path:

  • A symlink planted in the Syncthing-shared brain dir put an outside file in brain_query results (reproduced in the #156 review).
  • A FIFO swapped into the wiki hung the query forever. The race test blocked for its full 6 s timeout before this change. The tripwire doesn't catch that, since a FIFO isn't a symlink.

What

  • New package internal/brainfs:
    • Open: an os.Root on the brain dir.
    • ReadRegular: Lstat, then a non-blocking open, then a type check on the handle.
    • WalkRegular: walks the root's FS, yields regular files only, never follows a link.
  • search.Query and hydrate use it.

Tests (red first)

  • a wiki symlink isn't followed, and regular notes are still found
  • a symlinked dir isn't descended
  • hydrate refuses a symlink
  • symlink-swap and FIFO-swap races while Query loops for 1.5 s (any leaked byte or a hang fails)

CI=1 task check green.

risk/medium: control code (rule 8).

🤖 Generated with Claude Code


Created via git-mcp on behalf of @mathias

Refs #157, option 1, slice 1 (search). Later slices cover vectorstore, the context excerpt, ReadNote/brain_get, graphsync, the wiki inventory, wing indexes, the ingest merge and the provenance re-stamp. ## Why `search.Query` and `hydrate` read the brain by path: - A symlink planted in the Syncthing-shared brain dir put an outside file in `brain_query` results (reproduced in the #156 review). - **A FIFO swapped into the wiki hung the query forever.** The race test blocked for its full 6 s timeout before this change. The tripwire doesn't catch that, since a FIFO isn't a symlink. ## What - New package `internal/brainfs`: - `Open`: an os.Root on the brain dir. - `ReadRegular`: Lstat, then a non-blocking open, then a type check on the handle. - `WalkRegular`: walks the root's FS, yields regular files only, never follows a link. - `search.Query` and `hydrate` use it. ## Tests (red first) - a wiki symlink isn't followed, and regular notes are still found - a symlinked dir isn't descended - `hydrate` refuses a symlink - symlink-swap and FIFO-swap races while Query loops for 1.5 s (any leaked byte or a hang fails) `CI=1 task check` green. risk/medium: control code (rule 8). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- _Created via git-mcp on behalf of @mathias_
mathias added 1 commit 2026-10-03 16:19:10 +00:00
fix(search): walk and read the brain through an os.Root (#157, slice 1)
CI / Lint / Test / Vet (pull_request) Successful in 48s
CI / Mirror to GitHub (pull_request) Skipped
9c9ba31770
Option 1 of #157, first slice. Search walked and read the brain by
path, so a symlink planted in the Syncthing-shared brain dir put an
outside file's contents in brain_query results, and a FIFO swapped into
the wiki hung the query forever (reproduced: the FIFO race test blocked
for the full 6s timeout before this change).

New package internal/brainfs: Open (os.Root on the brain dir),
ReadRegular (Lstat + non-blocking open + type check on the handle) and
WalkRegular (walks the root's FS, yields regular files only, never
follows a link). search.Query and hydrate use it.

Tests (red first): a wiki symlink is not followed, a symlinked dir is not
descended, hydrate refuses a symlink, and race tests swap a symlink or a
FIFO into a wiki note while Query loops; any leaked byte or a hang fails.

Refs #157

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mathias added the risk/medium label 2026-10-03 16:19:21 +00:00
Author
Owner

Gatekeeper verdict: BLOCK (head 9c9ba31)

The control held under every probe. One load-bearing check has no test.

What survived (run in a scratch clone, sentinel counted, contents never printed)

  • Static symlinks: symlinks to an outside file at knowledge/l.md, knowledge/a/b/l.md, and as a relative ../../.. link in wiki/w/facts/: Query results 0, leaks 0.
  • Symlinked directories:
    • a symlinked wing passed as the filter (Wing: "evil", where wiki/evil -> outside dir with a sentinel note in it): 0 results;
    • the same for a symlinked hall (Wing: "w", Hall: "facts");
    • a symlinked knowledge/ root and a symlinked wiki/ root: 0 results. fs.WalkDir stats the start dir through the root, and the root refuses the escape.
  • The brain dir itself as a symlink: followed by design ("mount symlink"); a note inside is still found. The Syncthing peer cannot replace the folder root.
  • Static FIFO: skipped, no hang.
  • hydrate: refuses ../x/secret.md, an absolute outside path and wiki/../../x.md. Under a mixed regular/symlink/FIFO swap race on wiki/w/facts/r.md: 402k iterations in 2s, 0 leaks, no hang.
  • Rel: <B> → .; <B>/..foo → ..foo (correctly inside); <B>/../x, <B>-evil/x, /etc/passwd, <B>/wiki/../../x → error; a relative wiki/rel against an absolute dir → error. Correct.
  • Flakiness: the race tests passed -count=10.
  • Hard links: read (results 1, leaks 1). Same accepted residual as #156, but brainfs's doc comment does not state it. Please add the same assumptions note as in rawroot.go.

Blocking: the type check on the opened handle in ReadRegular has no test, and it is load-bearing

In brainfs.go, changing if !hfi.Mode().IsRegular() to a no-op leaves go test ./internal/search/ green. The FIFO race test does not catch this because its FIFOs have no writer. A non-blocking open succeeds, io.ReadAll returns EOF straight away, and nothing hangs.

Now give each swapped-in FIFO a writer that is held open (os.OpenFile(fifo, O_RDWR) before the rename, which a local process can do):

  • head: no hang (14k Query iterations in 1.5s);
  • handle check removed: Query hangs after 16 iterations. The fd is non-blocking, so Go's poller parks ReadAll on EAGAIN for as long as the writer stays open.

So this check is what prevents the hang, and nothing asserts it. Suggested fix: hold a writer open in TestQueryDoesNotHangUnderAFIFOSwapRace's swapper, or add a brainfs unit test asserting that ReadRegular returns ErrNotRegular for a FIFO reached past the Lstat. An error assertion, like TestOpenRegularRefusesAFIFOWithoutBlocking in #156, is the deterministic version.

Flips

flip result
ReadRegular → os.ReadFile(by name) red (hydrate, leak race, FIFO race)
ReadRegular → root.ReadFile red (FIFO race)
drop O_NONBLOCK, handle check kept red (FIFO race)
Query → os.ReadFile(path) red (leak race, FIFO race)
hydrate → os.ReadFile red (TestHydrateRefusesASymlink)
drop the handle check green; load-bearing (see above)
drop the Lstat check green. Redundant: the root refuses escapes and the handle check refuses non-regular files. Acceptable.
WalkRegular: drop the regular-type filter green. ReadRegular refuses anyway. Acceptable defence in depth.
WalkRegular → filepath.WalkDir by path green. Behaviourally equivalent today: neither follows symlinks, and every read goes through the root. A dir swapped mid-walk would at most list names, never read content. Acceptable.
Rel: drop the .. check green. Every caller passes brainDir-joined paths. Acceptable, but brainfs has no unit tests at all, so Rel's edge cases are unpinned. Non-blocking.

Performance

1,560 notes of about 5 KB each (matching the live brain's 1,559 .md files in wiki/ and knowledge/), BenchmarkQuery, 30x3 runs:

  • base: about 15.7 ms/query;
  • PR: about 26.1 ms/query.

That is +66%, about +10 ms in absolute terms, from Lstat + openat + fstat per file through the root (one Open per Query, which is fine). Acceptable for a BM25 scan; noting it so later slices (vectorstore sync) budget for it. hydrate opens one root per vector hit, which is negligible.

Rule 9

The test names match their assertions. TestQueryDoesNotHangUnderAFIFOSwapRace is honest, but it only exercises writer-less FIFOs, which is the gap above.

CI

Run 8913: success. CI=1 task check EXIT=0, 0 FAIL lines. Green, and green does not cover the blocking finding.

Not checked

The later #157 slices (vectorstore, context excerpt, ReadNote, graphsync and the rest); a live Syncthing round-trip; hybridMerge end to end with a stub vector searcher (I called hydrate directly).

## Gatekeeper verdict: BLOCK (head 9c9ba31) The control held under every probe. One load-bearing check has no test. ### What survived (run in a scratch clone, sentinel counted, contents never printed) - **Static symlinks:** symlinks to an outside file at `knowledge/l.md`, `knowledge/a/b/l.md`, and as a relative `../../..` link in `wiki/w/facts/`: Query results 0, leaks 0. - **Symlinked directories:** - a symlinked wing passed as the filter (`Wing: "evil"`, where `wiki/evil -> outside dir` with a sentinel note in it): 0 results; - the same for a symlinked hall (`Wing: "w", Hall: "facts"`); - a symlinked `knowledge/` root and a symlinked `wiki/` root: 0 results. `fs.WalkDir` stats the start dir through the root, and the root refuses the escape. - **The brain dir itself as a symlink:** followed by design ("mount symlink"); a note inside is still found. The Syncthing peer cannot replace the folder root. - **Static FIFO:** skipped, no hang. - **hydrate:** refuses `../x/secret.md`, an absolute outside path and `wiki/../../x.md`. Under a mixed regular/symlink/FIFO swap race on `wiki/w/facts/r.md`: 402k iterations in 2s, 0 leaks, no hang. - **`Rel`:** `<B>` → `.`; `<B>/..foo` → `..foo` (correctly inside); `<B>/../x`, `<B>-evil/x`, `/etc/passwd`, `<B>/wiki/../../x` → error; a relative `wiki/rel` against an absolute dir → error. Correct. - **Flakiness:** the race tests passed `-count=10`. - **Hard links:** read (results 1, leaks 1). Same accepted residual as #156, but `brainfs`'s doc comment does not state it. Please add the same assumptions note as in `rawroot.go`. ### Blocking: the type check on the opened handle in `ReadRegular` has no test, and it is load-bearing In `brainfs.go`, changing `if !hfi.Mode().IsRegular()` to a no-op leaves `go test ./internal/search/` **green**. The FIFO race test does not catch this because its FIFOs have no writer. A non-blocking open succeeds, `io.ReadAll` returns EOF straight away, and nothing hangs. Now give each swapped-in FIFO a writer that is held open (`os.OpenFile(fifo, O_RDWR)` before the rename, which a local process can do): - **head:** no hang (14k Query iterations in 1.5s); - **handle check removed:** **Query hangs after 16 iterations.** The fd is non-blocking, so Go's poller parks `ReadAll` on EAGAIN for as long as the writer stays open. So this check is what prevents the hang, and nothing asserts it. Suggested fix: hold a writer open in `TestQueryDoesNotHangUnderAFIFOSwapRace`'s swapper, or add a `brainfs` unit test asserting that `ReadRegular` returns `ErrNotRegular` for a FIFO reached past the Lstat. An error assertion, like `TestOpenRegularRefusesAFIFOWithoutBlocking` in #156, is the deterministic version. ### Flips | flip | result | |---|---| | ReadRegular → `os.ReadFile(by name)` | red (hydrate, leak race, FIFO race) | | ReadRegular → `root.ReadFile` | red (FIFO race) | | drop O_NONBLOCK, handle check kept | red (FIFO race) | | Query → `os.ReadFile(path)` | red (leak race, FIFO race) | | hydrate → `os.ReadFile` | red (`TestHydrateRefusesASymlink`) | | **drop the handle check** | **green; load-bearing (see above)** | | drop the Lstat check | green. Redundant: the root refuses escapes and the handle check refuses non-regular files. Acceptable. | | WalkRegular: drop the regular-type filter | green. ReadRegular refuses anyway. Acceptable defence in depth. | | WalkRegular → `filepath.WalkDir` by path | green. Behaviourally equivalent today: neither follows symlinks, and every read goes through the root. A dir swapped mid-walk would at most list names, never read content. Acceptable. | | `Rel`: drop the `..` check | green. Every caller passes `brainDir`-joined paths. Acceptable, but `brainfs` has **no unit tests at all**, so `Rel`'s edge cases are unpinned. Non-blocking. | ### Performance 1,560 notes of about 5 KB each (matching the live brain's 1,559 `.md` files in wiki/ and knowledge/), `BenchmarkQuery`, 30x3 runs: - base: about 15.7 ms/query; - PR: about 26.1 ms/query. That is +66%, about +10 ms in absolute terms, from Lstat + openat + fstat per file through the root (one `Open` per Query, which is fine). Acceptable for a BM25 scan; noting it so later slices (vectorstore sync) budget for it. `hydrate` opens one root per vector hit, which is negligible. ### Rule 9 The test names match their assertions. `TestQueryDoesNotHangUnderAFIFOSwapRace` is honest, but it only exercises writer-less FIFOs, which is the gap above. ### CI Run 8913: success. `CI=1 task check` EXIT=0, 0 FAIL lines. Green, and green does not cover the blocking finding. ### Not checked The later #157 slices (vectorstore, context excerpt, ReadNote, graphsync and the rest); a live Syncthing round-trip; `hybridMerge` end to end with a stub vector searcher (I called `hydrate` directly).
mathias added 1 commit 2026-10-03 16:26:28 +00:00
test(brainfs): pin the handle check with a FIFO held open by a writer
CI / Lint / Test / Vet (pull_request) Successful in 47s
CI / Mirror to GitHub (pull_request) Skipped
ffe46c7077
PR #161 review: removing ReadRegular's type check on the opened handle
stayed green, because the race test's FIFOs had no writer and read EOF
at once; with a writer holding the FIFO open, search hung after 16
iterations. The open-and-check step is split into readOpened, and
TestReadOpenedRefusesAFIFOHeldOpenByAWriter reaches it past the Lstat
with a writer open: it must return ErrNotRegular promptly (the flip is
red).

Also unit tests for ReadRegular (symlink, escape), Rel (inside incl.
..foo, escapes) and WalkRegular (regular files only), and the hard-link
residual documented where it is accepted, as in api/rawroot.go.

Refs #157

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
Owner

Gatekeeper verdict, round 2: BLOCK (head ffe46c7)

Close. One gap is left, with the same shape as #156 round 2: the check has a unit test, but nothing checks that its caller uses it.

Fixed, verified by running it

flip result
handle check disabled red (TestReadOpenedRefusesAFIFOHeldOpenByAWriter)
WalkRegular: drop the regular-type filter red (TestWalkRegularYieldsOnlyRegularFiles)
Rel: drop the .. check red (TestRelAcceptsPathsInsideAndRefusesEscapes)
ReadRegular: drop the Lstat check red (TestReadRegularRefusesSymlinksAndReadsRegularFiles)
ReadRegular → root.ReadFile red (FIFO race)
ReadRegular → os.ReadFile(by name) red (leak race, FIFO race)
  • Probes at head: no change from round 1. Every symlink case returns 0. hydrate under the swap race: 482k iterations, 0 leaks, no hang. The held-writer FIFO race against Query: no hang (12.4k iterations).
  • Hard-link note: present on readOpened.

Blocking: nothing checks that ReadRegular goes through readOpened

I replaced return b.readOpened(rel) in ReadRegular with the same non-blocking open plus io.ReadAll, but without the handle check, so readOpened is bypassed:

  • go test ./internal/brainfs/ ./internal/search/: green. The unit test calls readOpened directly. The search FIFO race uses FIFOs with no writer, so a missing handle check cannot show up there.
  • The held-writer FIFO race against Query: hangs after 5 iterations.

So the round-1 hang can come back through a refactor of ReadRegular (or a future second reader in brainfs that inlines its own open), and the suite stays green.

Fix (one line of intent): in swapInto in search/brainroot_test.go, open each FIFO O_RDWR before the rename and keep the fd. That makes TestQueryDoesNotHangUnderAFIFOSwapRace exercise the handle check through the public reader. I ran exactly this as a probe: green at head, hangs with the bypass.

CI

Run 8917: success. CI=1 task check EXIT=0, 0 FAIL lines. Green, and green does not cover the blocking finding.

## Gatekeeper verdict, round 2: BLOCK (head ffe46c7) Close. One gap is left, with the same shape as #156 round 2: the check has a unit test, but nothing checks that its caller uses it. ### Fixed, verified by running it | flip | result | |---|---| | handle check disabled | red (`TestReadOpenedRefusesAFIFOHeldOpenByAWriter`) | | WalkRegular: drop the regular-type filter | red (`TestWalkRegularYieldsOnlyRegularFiles`) | | Rel: drop the `..` check | red (`TestRelAcceptsPathsInsideAndRefusesEscapes`) | | ReadRegular: drop the Lstat check | red (`TestReadRegularRefusesSymlinksAndReadsRegularFiles`) | | ReadRegular → `root.ReadFile` | red (FIFO race) | | ReadRegular → `os.ReadFile(by name)` | red (leak race, FIFO race) | - **Probes at head:** no change from round 1. Every symlink case returns 0. hydrate under the swap race: 482k iterations, 0 leaks, no hang. **The held-writer FIFO race against Query: no hang** (12.4k iterations). - **Hard-link note:** present on `readOpened`. ### Blocking: nothing checks that `ReadRegular` goes through `readOpened` I replaced `return b.readOpened(rel)` in `ReadRegular` with the same non-blocking open plus `io.ReadAll`, but without the handle check, so `readOpened` is bypassed: - `go test ./internal/brainfs/ ./internal/search/`: **green**. The unit test calls `readOpened` directly. The search FIFO race uses FIFOs with no writer, so a missing handle check cannot show up there. - The held-writer FIFO race against `Query`: **hangs after 5 iterations.** So the round-1 hang can come back through a refactor of `ReadRegular` (or a future second reader in brainfs that inlines its own open), and the suite stays green. **Fix (one line of intent):** in `swapInto` in `search/brainroot_test.go`, open each FIFO `O_RDWR` before the rename and keep the fd. That makes `TestQueryDoesNotHangUnderAFIFOSwapRace` exercise the handle check through the public reader. I ran exactly this as a probe: green at head, hangs with the bypass. ### CI Run 8917: success. `CI=1 task check` EXIT=0, 0 FAIL lines. Green, and green does not cover the blocking finding.
mathias added 1 commit 2026-10-03 16:32:41 +00:00
test(search): hold a writer on swapped-in FIFOs; await the swapper
CI / Mirror to GitHub (pull_request) Skipped
CI / Lint / Test / Vet (pull_request) Successful in 39s
fd33e04467
PR #161 round 2: ReadRegular bypassing readOpened (non-blocking open,
no handle check) stayed green, because the search FIFO race used FIFOs
with no writer, which read EOF at once. The swapper now opens each FIFO
O_RDWR and keeps every writer open until it exits, as any local process
could, so a reader that skips the handle check parks forever; the bypass
flip is red. The swapper is stopped and awaited in a t.Cleanup before
t.TempDir removal (hyperguild#162).

Refs #157

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
Owner

Gatekeeper verdict, round 3: PASS (head fd33e04)

Only search/brainroot_test.go changed since ffe46c7, so the round-2 probes of the production code stand: every symlink case returns 0, hydrate under the swap race leaks 0 times with no hang, and the held-writer FIFO race against Query does not hang.

The round-2 finding is closed, verified by running it

  • ReadRegular bypassing readOpened (non-blocking open plus ReadAll, no handle check): TestQueryDoesNotHangUnderAFIFOSwapRace red 10/10 at -count=10.
  • The rest of the flip table is red:
    • handle check off: the search FIFO race and the brainfs unit test;
    • ReadRegular → root.ReadFile: the FIFO race;
    • ReadRegular → os.ReadFile(by name): the leak and FIFO races;
    • hydrate → os.ReadFile: TestHydrateRefusesASymlink;
    • Query → os.ReadFile(path): the leak and FIFO races.

Flakiness and fd leaks

  • Whole search package, -count=10: 140 test runs, 0 FAIL. -race -count=3 on search and brainfs: ok.
  • fd usage: peaks at about 20k during each FIFO race test, because every held writer stays open until cleanup. It drops back to about 8 between iterations, so startSwap's cleanup closes them and nothing leaks across tests.
  • Low fd limits: at hard limits of 1024, 4096 and 65536, the head passes 3/3 and the bypass is caught 3/3 at each limit. The hang shows up within the first few iterations, before the limit is reached, so a low-ulimit runner does not make the test vacuous.

CI

Run 8920: success. CI=1 task check EXIT=0, 0 FAIL lines.

Notes (non-blocking)

  • Performance cost: about +10 ms per Query at the live brain size (round 1). Budget for it in the later slices.
  • Hard links: accepted residual, documented on readOpened.
  • Not checked: the later #157 slices; a live Syncthing round-trip; hybridMerge end to end (I called hydrate directly).

Verdict only. Not merged.

## Gatekeeper verdict, round 3: PASS (head fd33e04) Only `search/brainroot_test.go` changed since ffe46c7, so the round-2 probes of the production code stand: every symlink case returns 0, hydrate under the swap race leaks 0 times with no hang, and the held-writer FIFO race against Query does not hang. ### The round-2 finding is closed, verified by running it - **ReadRegular bypassing readOpened** (non-blocking open plus ReadAll, no handle check): `TestQueryDoesNotHangUnderAFIFOSwapRace` red **10/10** at `-count=10`. - **The rest of the flip table is red:** - handle check off: the search FIFO race and the brainfs unit test; - ReadRegular → `root.ReadFile`: the FIFO race; - ReadRegular → `os.ReadFile(by name)`: the leak and FIFO races; - hydrate → `os.ReadFile`: `TestHydrateRefusesASymlink`; - Query → `os.ReadFile(path)`: the leak and FIFO races. ### Flakiness and fd leaks - **Whole search package, `-count=10`:** 140 test runs, 0 FAIL. `-race -count=3` on search and brainfs: ok. - **fd usage:** peaks at about 20k during each FIFO race test, because every held writer stays open until cleanup. It drops back to about 8 between iterations, so `startSwap`'s cleanup closes them and nothing leaks across tests. - **Low fd limits:** at hard limits of 1024, 4096 and 65536, the head passes 3/3 and the bypass is caught 3/3 at each limit. The hang shows up within the first few iterations, before the limit is reached, so a low-ulimit runner does not make the test vacuous. ### CI Run 8920: success. `CI=1 task check` EXIT=0, 0 FAIL lines. ### Notes (non-blocking) - Performance cost: about +10 ms per Query at the live brain size (round 1). Budget for it in the later slices. - Hard links: accepted residual, documented on `readOpened`. - Not checked: the later #157 slices; a live Syncthing round-trip; `hybridMerge` end to end (I called hydrate directly). Verdict only. Not merged.
mathias merged commit eb77305580 into main 2026-10-03 19:34:46 +00:00
Sign in to join this conversation.