readExcerpt (brain_context), api.ReadNote (brain_get and read-after-write) and notes.ReadScoped (brain_read) read the Syncthing-shared brain by path. Under a symlink-swap race, each leaked an outside file hundreds of thousands of times in 1.5 s. That includes brain_read: its Lstat check was raced, because the file is then reopened by name. A FIFO blocked os.ReadFile.
What
All three read through brainfs: os.Root, regular files only, a non-blocking open and a type check on the handle.
brainfs.ReadRegularMax checks the size limit on the opened handle. notes keeps its ErrTooLarge.
New internal/brainfs/brainfstest is the shared swap-race harness (a symlink swapper, a held-writer FIFO swapper, awaited before TempDir cleanup, AssertNoLeak/AssertNoHang) for this and later slices.
Tests (red first)
Each reader under a symlink-swap race and a held-writer FIFO race, plus static symlink checks.
ReadRegularMax limit.
Flipped: readExcerpt back to os.ReadFile goes red.
Refs #157, option 1, slice 2.
## Why
`readExcerpt` (brain_context), `api.ReadNote` (brain_get and read-after-write) and `notes.ReadScoped` (brain_read) read the Syncthing-shared brain by path. Under a symlink-swap race, each leaked an outside file **hundreds of thousands of times in 1.5 s**. That includes brain_read: its Lstat check was raced, because the file is then reopened by name. A FIFO blocked `os.ReadFile`.
## What
- All three read through `brainfs`: os.Root, regular files only, a non-blocking open and a type check on the handle.
- `brainfs.ReadRegularMax` checks the size limit on the opened handle. `notes` keeps its `ErrTooLarge`.
- New `internal/brainfs/brainfstest` is the shared swap-race harness (a symlink swapper, a held-writer FIFO swapper, awaited before TempDir cleanup, `AssertNoLeak`/`AssertNoHang`) for this and later slices.
## Tests (red first)
- Each reader under a symlink-swap race and a held-writer FIFO race, plus static symlink checks.
- `ReadRegularMax` limit.
- Flipped: `readExcerpt` back to `os.ReadFile` goes red.
`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_
Option 1 of #157, slice 2. readExcerpt (brain_context), api.ReadNote
(brain_get / read-after-write) and notes.ReadScoped (brain_read) read
the Syncthing-shared brain by path. Under a symlink swap race each
leaked an outside file hundreds of thousands of times in 1.5s, including
brain_read, whose Lstat check was raced by the reopen-by-name; a FIFO
blocked os.ReadFile.
- All three read through brainfs (os.Root, regular files only,
non-blocking open, type check on the handle).
- brainfs.ReadRegularMax checks a size limit on the opened handle;
notes keeps its ErrTooLarge.
- New internal/brainfs/brainfstest: the shared swap-race harness
(symlink and held-writer FIFO swappers, awaited before TempDir
cleanup; AssertNoLeak / AssertNoHang) for this and later slices.
Tests red first: each reader under a symlink-swap race and a held-writer
FIFO race, plus static checks. Flipped: readExcerpt back to os.ReadFile
is red.
Refs #157
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The confinement of the three readers held under every probe. Blocked on the size limit this PR moves into brainfs, and on rule 9.
Blocking
1. ReadRegularMax does not enforce its limit, and dropping the limit entirely stays green.
brainfs.go readOpenedMax checks hfi.Size() > max on the handle, then calls an unbounded io.ReadAll(f). A file growing in place during the read is returned whole.
Run: a 1 MiB limit, with a writer appending 64 KiB chunks up to 100 MiB. One read returned 100 MiB (100x the limit). In the other runs, 8 MiB and 1.4 MiB were returned.
In notes, maxNoteBytes (256 KiB) is the documented cap on brain_read and brain_edit. Syncthing replaces files by rename rather than growing them in place, so the realistic actor is a local writer (uid 1000, a CronJob). It is low-severity memory exhaustion, but the PR claims "a size limit checked on the opened handle", and the bytes returned are not limited.
Separately, I changed readContained from ReadRegularMax(t.Rel, maxNoteBytes) to ReadRegular(t.Rel), dropping the limit, and go test ./internal/notes/ stayed green. Nothing tests the limit at its only caller. That gap existed before this PR, but this PR rewrites the limit.
Fix: read through io.LimitReader(f, max+1) and return ErrTooLarge if len > max. Add a notes test that a note over maxNoteBytes returns ErrTooLarge through ReadScoped.
2. Rule 9: all three new tests are named ...IsConfinedToRegularFilesInTheBrain.
That is the IsContained pattern the rule names: it claims a goal, not the mechanism the test observes. What the tests observe is: a static symlink is refused, no sentinel appears under a symlink-swap race, and nothing hangs under a held-writer FIFO race. TestReadNoteIsConfinedToRegularFilesInTheBrain also has no positive control: a ReadNote that always errors passes it (other api tests catch that today). Suggested names: Test<Reader>RefusesSymlinksAndSurvivesSwapRaces, and add require on one successful read of r.md in the ReadNote test, as the other two have. The same applies to the package doc line "A reader is confined if...": say what the harness observes.
What survived (run in a scratch clone, sentinel counted)
flip
result
readExcerpt → os.ReadFile(path)
red
ReadNote → os.ReadFile(abs)
red
readContained → os.ReadFile(t.Abs)
red
readOpenedMax handle check off
red in all three reader tests plus the brainfs unit test
ReadRegularMax bypassing readOpenedMax (non-blocking open, no handle check)
red (ReadScoped hang test, ReadRegularMax unit test)
ReadRegularMax size check off
red (brainfs unit test only; see finding 1 for the caller)
ReadNote always errors
red (existing api tests)
Flakiness: the three race tests passed -count=10, and -race -count=2 on all four packages. The AssertNoHang timeout path is safe: Cleanup stops the swapper, closing the writers releases a parked reader, and then t.Fatal runs.
notes brainDir derivation:Abs = filepath.Join(brainDir, clean) and Rel = clean (Cleaned, so no ..). The TrimSuffix gives back Clean(brainDir). Run through ReadScoped with a path containing .. and .:
an absolute dir, with a trailing /, with /./ and with /wiki/..: all correct;
a mount-symlink brainDir (Resolve does not replace Abs with the EvalSymlinks result): correct;
a relative dir (../brain, the default): correct;
brainDir == ".": fails closed (open wiki/w/facts/r.md: not a directory), so brain_read and brain_edit break, but nothing escapes.
I found no case producing a root above the brain: a non-matching suffix leaves Abs itself, which is below the brain and fails to open as a root. Non-blocking, but fragile. Carry brainDir in Target instead of re-deriving it.
brain_edit (notes.Edit) write path
It now reads through readContained, so that read is confined. The write (CreateTemp in filepath.Dir(abs), then Rename onto abs) is still by path: a parent directory swapped for a symlink after Resolve sends the temp file and the rename outside. The rename replaces a symlink at abs rather than following it. A separate slice. It belongs with #158 (writes through symlinks), not here.
CI
Run 9008: success. CI=1 task check EXIT=0, 0 FAIL lines. Green, and green does not cover findings 1 or 2.
Not checked
The remaining #157 slices; a live Syncthing round-trip.
## Gatekeeper verdict: BLOCK (head b6f7dac)
The confinement of the three readers held under every probe. Blocked on the size limit this PR moves into `brainfs`, and on rule 9.
### Blocking
**1. `ReadRegularMax` does not enforce its limit, and dropping the limit entirely stays green.**
- `brainfs.go readOpenedMax` checks `hfi.Size() > max` on the handle, then calls an unbounded `io.ReadAll(f)`. A file growing in place during the read is returned whole.
- **Run:** a 1 MiB limit, with a writer appending 64 KiB chunks up to 100 MiB. One read returned **100 MiB (100x the limit)**. In the other runs, 8 MiB and 1.4 MiB were returned.
- In notes, `maxNoteBytes` (256 KiB) is the documented cap on brain_read and brain_edit. Syncthing replaces files by rename rather than growing them in place, so the realistic actor is a local writer (uid 1000, a CronJob). It is low-severity memory exhaustion, but the PR claims "a size limit checked on the opened handle", and the bytes returned are not limited.
- Separately, I changed `readContained` from `ReadRegularMax(t.Rel, maxNoteBytes)` to `ReadRegular(t.Rel)`, dropping the limit, and `go test ./internal/notes/` stayed **green**. Nothing tests the limit at its only caller. That gap existed before this PR, but this PR rewrites the limit.
- **Fix:** read through `io.LimitReader(f, max+1)` and return `ErrTooLarge` if `len > max`. Add a notes test that a note over `maxNoteBytes` returns `ErrTooLarge` through `ReadScoped`.
**2. Rule 9: all three new tests are named `...IsConfinedToRegularFilesInTheBrain`.**
That is the `IsContained` pattern the rule names: it claims a goal, not the mechanism the test observes. What the tests observe is: a static symlink is refused, no sentinel appears under a symlink-swap race, and nothing hangs under a held-writer FIFO race. `TestReadNoteIsConfinedToRegularFilesInTheBrain` also has no positive control: a `ReadNote` that always errors passes it (other api tests catch that today). Suggested names: `Test<Reader>RefusesSymlinksAndSurvivesSwapRaces`, and add `require` on one successful read of `r.md` in the ReadNote test, as the other two have. The same applies to the package doc line "A reader is confined if...": say what the harness observes.
### What survived (run in a scratch clone, sentinel counted)
| flip | result |
|---|---|
| readExcerpt → `os.ReadFile(path)` | red |
| ReadNote → `os.ReadFile(abs)` | red |
| readContained → `os.ReadFile(t.Abs)` | red |
| readOpenedMax handle check off | red in all three reader tests plus the brainfs unit test |
| ReadRegularMax bypassing readOpenedMax (non-blocking open, no handle check) | red (ReadScoped hang test, ReadRegularMax unit test) |
| ReadRegularMax size check off | red (brainfs unit test only; see finding 1 for the caller) |
| ReadNote always errors | red (existing api tests) |
- **Flakiness:** the three race tests passed `-count=10`, and `-race -count=2` on all four packages. The `AssertNoHang` timeout path is safe: Cleanup stops the swapper, closing the writers releases a parked reader, and then t.Fatal runs.
- **notes `brainDir` derivation:** `Abs = filepath.Join(brainDir, clean)` and `Rel = clean` (`Clean`ed, so no `..`). The TrimSuffix gives back `Clean(brainDir)`. Run through `ReadScoped` with a path containing `..` and `.`:
- an absolute dir, with a trailing `/`, with `/./` and with `/wiki/..`: all correct;
- a **mount-symlink brainDir** (Resolve does not replace Abs with the EvalSymlinks result): correct;
- a relative dir (`../brain`, the default): correct;
- `brainDir == "."`: **fails closed** (`open wiki/w/facts/r.md: not a directory`), so brain_read and brain_edit break, but nothing escapes.
I found no case producing a root above the brain: a non-matching suffix leaves Abs itself, which is below the brain and fails to open as a root. Non-blocking, but fragile. Carry `brainDir` in `Target` instead of re-deriving it.
### brain_edit (`notes.Edit`) write path
It now reads through `readContained`, so that read is confined. The write (`CreateTemp` in `filepath.Dir(abs)`, then `Rename` onto abs) is still by path: a parent directory swapped for a symlink after Resolve sends the temp file and the rename outside. The rename replaces a symlink at abs rather than following it. **A separate slice. It belongs with #158 (writes through symlinks), not here.**
### CI
Run 9008: success. `CI=1 task check` EXIT=0, 0 FAIL lines. Green, and green does not cover findings 1 or 2.
### Not checked
The remaining #157 slices; a live Syncthing round-trip.
Review round 1 on PR #165:
- ReadRegularMax checked the size on the opened handle, then read with an
unbounded io.ReadAll, so a file growing after the check returned past the
limit. The read now goes through io.LimitReader(max+1). A test seam grows
the file between the check and the read and counts bytes consumed.
- New: ReadScoped returns ErrTooLarge for a note over maxNoteBytes; red if
readContained drops the limit.
- notes.Target carries the brain root instead of deriving it by TrimSuffix.
- Rule 9: reader tests renamed to <Reader>RefusesSymlinksAndSurvivesSwapRaces;
ReadNote test has a positive control; brainfstest comment states what the
harness observes and what it does not.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Same probe as round 1: a 1 MiB limit, with a writer appending 64 KiB chunks up to 100 MiB, 3 runs. 0 reads returned more than the limit (round 1: 100 MiB). Reads during growth return ErrTooLarge. The largest successful read was 264 KiB, under the limit.
Flips, all red in TestReadRegularMaxRefusesAndStopsReadingAFileThatGrowsPastTheLimit:
drop LimitReader;
drop the len > max check;
LimitReader(r, max) instead of max+1 (silent truncation).
Dropping the Stat-time size check stays green. That is correct: the bounded read alone refuses the file, so the Stat check is now only an early exit.
2. Size limit at the caller.
readContained → ReadRegular (no max): red in TestReadScopedRefusesANoteOverMaxNoteBytes.
readContained → os.ReadFile: red in both that test and the swap-race test.
3. Rule 9. The <Reader>RefusesSymlinksAndSurvivesSwapRaces names match what the tests observe. ReadNote now has a positive control: a ReadNote that always errors fails it, as well as the existing api tests. The brainfstest doc states what the harness observes and what it does not.
4. Target.Brain.Resolve is the only constructor. ReadScoped with brainDir == "." now works (round 1: it failed closed), as does a mount symlink with a trailing /.
Rest of the flip table (all red)
readExcerpt → os.ReadFile
ReadNote → os.ReadFile(abs)
handle check off: all three reader tests, plus the brainfs FIFO unit test
Flakiness
-count=10 and -race -count=2 on notes, mcp, api and brainfs: ok.
CI
Run 9017: success. CI=1 task check EXIT=0, 0 FAIL lines.
Non-blocking notes
wrapAfterStat is a package-level test seam. It is safe while no brainfs test uses t.Parallel; worth a comment saying so.
## Gatekeeper verdict, round 2: PASS (head 02cecb2)
### Round-1 blockers, verified by running them
**1. Bounded read.**
- Same probe as round 1: a 1 MiB limit, with a writer appending 64 KiB chunks up to 100 MiB, 3 runs. **0 reads returned more than the limit** (round 1: 100 MiB). Reads during growth return `ErrTooLarge`. The largest successful read was 264 KiB, under the limit.
- Flips, all red in `TestReadRegularMaxRefusesAndStopsReadingAFileThatGrowsPastTheLimit`:
- drop `LimitReader`;
- drop the `len > max` check;
- `LimitReader(r, max)` instead of `max+1` (silent truncation).
- Dropping the Stat-time size check stays green. That is correct: the bounded read alone refuses the file, so the Stat check is now only an early exit.
**2. Size limit at the caller.**
- `readContained` → `ReadRegular` (no max): red in `TestReadScopedRefusesANoteOverMaxNoteBytes`.
- `readContained` → `os.ReadFile`: red in both that test and the swap-race test.
**3. Rule 9.** The `<Reader>RefusesSymlinksAndSurvivesSwapRaces` names match what the tests observe. ReadNote now has a positive control: a ReadNote that always errors fails it, as well as the existing api tests. The `brainfstest` doc states what the harness observes and what it does not.
**4. `Target.Brain`.** `Resolve` is the only constructor. `ReadScoped` with `brainDir == "."` now works (round 1: it failed closed), as does a mount symlink with a trailing `/`.
### Rest of the flip table (all red)
- readExcerpt → `os.ReadFile`
- ReadNote → `os.ReadFile(abs)`
- handle check off: all three reader tests, plus the brainfs FIFO unit test
### Flakiness
`-count=10` and `-race -count=2` on notes, mcp, api and brainfs: ok.
### CI
Run 9017: success. `CI=1 task check` EXIT=0, 0 FAIL lines.
### Non-blocking notes
- `wrapAfterStat` is a package-level test seam. It is safe while no brainfs test uses `t.Parallel`; worth a comment saying so.
- The brain_edit write path is tracked in #158.
Verdict only. Not merged.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Refs #157, option 1, slice 2.
Why
readExcerpt(brain_context),api.ReadNote(brain_get and read-after-write) andnotes.ReadScoped(brain_read) read the Syncthing-shared brain by path. Under a symlink-swap race, each leaked an outside file hundreds of thousands of times in 1.5 s. That includes brain_read: its Lstat check was raced, because the file is then reopened by name. A FIFO blockedos.ReadFile.What
brainfs: os.Root, regular files only, a non-blocking open and a type check on the handle.brainfs.ReadRegularMaxchecks the size limit on the opened handle.noteskeeps itsErrTooLarge.internal/brainfs/brainfstestis the shared swap-race harness (a symlink swapper, a held-writer FIFO swapper, awaited before TempDir cleanup,AssertNoLeak/AssertNoHang) for this and later slices.Tests (red first)
ReadRegularMaxlimit.readExcerptback toos.ReadFilegoes red.CI=1 task checkgreen.risk/medium: control code (rule 8).
🤖 Generated with Claude Code
Created via git-mcp on behalf of @mathias
Gatekeeper verdict: BLOCK (head
b6f7dac)The confinement of the three readers held under every probe. Blocked on the size limit this PR moves into
brainfs, and on rule 9.Blocking
1.
ReadRegularMaxdoes not enforce its limit, and dropping the limit entirely stays green.brainfs.go readOpenedMaxcheckshfi.Size() > maxon the handle, then calls an unboundedio.ReadAll(f). A file growing in place during the read is returned whole.maxNoteBytes(256 KiB) is the documented cap on brain_read and brain_edit. Syncthing replaces files by rename rather than growing them in place, so the realistic actor is a local writer (uid 1000, a CronJob). It is low-severity memory exhaustion, but the PR claims "a size limit checked on the opened handle", and the bytes returned are not limited.readContainedfromReadRegularMax(t.Rel, maxNoteBytes)toReadRegular(t.Rel), dropping the limit, andgo test ./internal/notes/stayed green. Nothing tests the limit at its only caller. That gap existed before this PR, but this PR rewrites the limit.io.LimitReader(f, max+1)and returnErrTooLargeiflen > max. Add a notes test that a note overmaxNoteBytesreturnsErrTooLargethroughReadScoped.2. Rule 9: all three new tests are named
...IsConfinedToRegularFilesInTheBrain.That is the
IsContainedpattern the rule names: it claims a goal, not the mechanism the test observes. What the tests observe is: a static symlink is refused, no sentinel appears under a symlink-swap race, and nothing hangs under a held-writer FIFO race.TestReadNoteIsConfinedToRegularFilesInTheBrainalso has no positive control: aReadNotethat always errors passes it (other api tests catch that today). Suggested names:Test<Reader>RefusesSymlinksAndSurvivesSwapRaces, and addrequireon one successful read ofr.mdin the ReadNote test, as the other two have. The same applies to the package doc line "A reader is confined if...": say what the harness observes.What survived (run in a scratch clone, sentinel counted)
os.ReadFile(path)os.ReadFile(abs)os.ReadFile(t.Abs)Flakiness: the three race tests passed
-count=10, and-race -count=2on all four packages. TheAssertNoHangtimeout path is safe: Cleanup stops the swapper, closing the writers releases a parked reader, and then t.Fatal runs.notes
brainDirderivation:Abs = filepath.Join(brainDir, clean)andRel = clean(Cleaned, so no..). The TrimSuffix gives backClean(brainDir). Run throughReadScopedwith a path containing..and.:/, with/./and with/wiki/..: all correct;../brain, the default): correct;brainDir == ".": fails closed (open wiki/w/facts/r.md: not a directory), so brain_read and brain_edit break, but nothing escapes.I found no case producing a root above the brain: a non-matching suffix leaves Abs itself, which is below the brain and fails to open as a root. Non-blocking, but fragile. Carry
brainDirinTargetinstead of re-deriving it.brain_edit (
notes.Edit) write pathIt now reads through
readContained, so that read is confined. The write (CreateTempinfilepath.Dir(abs), thenRenameonto abs) is still by path: a parent directory swapped for a symlink after Resolve sends the temp file and the rename outside. The rename replaces a symlink at abs rather than following it. A separate slice. It belongs with #158 (writes through symlinks), not here.CI
Run 9008: success.
CI=1 task checkEXIT=0, 0 FAIL lines. Green, and green does not cover findings 1 or 2.Not checked
The remaining #157 slices; a live Syncthing round-trip.
Gatekeeper verdict, round 2: PASS (head
02cecb2)Round-1 blockers, verified by running them
1. Bounded read.
ErrTooLarge. The largest successful read was 264 KiB, under the limit.TestReadRegularMaxRefusesAndStopsReadingAFileThatGrowsPastTheLimit:LimitReader;len > maxcheck;LimitReader(r, max)instead ofmax+1(silent truncation).2. Size limit at the caller.
readContained→ReadRegular(no max): red inTestReadScopedRefusesANoteOverMaxNoteBytes.readContained→os.ReadFile: red in both that test and the swap-race test.3. Rule 9. The
<Reader>RefusesSymlinksAndSurvivesSwapRacesnames match what the tests observe. ReadNote now has a positive control: a ReadNote that always errors fails it, as well as the existing api tests. Thebrainfstestdoc states what the harness observes and what it does not.4.
Target.Brain.Resolveis the only constructor.ReadScopedwithbrainDir == "."now works (round 1: it failed closed), as does a mount symlink with a trailing/.Rest of the flip table (all red)
os.ReadFileos.ReadFile(abs)Flakiness
-count=10and-race -count=2on notes, mcp, api and brainfs: ok.CI
Run 9017: success.
CI=1 task checkEXIT=0, 0 FAIL lines.Non-blocking notes
wrapAfterStatis a package-level test seam. It is safe while no brainfs test usest.Parallel; worth a comment saying so.Verdict only. Not merged.