Refs #157. Step 1 of Mathias's "both, auto-remove first" decision.
Why
The live tripwire (#159) only alerted. A symlink planted in the Syncthing-shared brain dir stayed readable to search, vectorstore and the other readers until a human removed it.
What
symlinkguard.Watch now unlinks every symlink it finds, through an os.Root on the resolved brain dir. It re-checks with Lstat that the entry is still a symlink and removes only the link, never its target.
DefaultInterval drops from 5 min to 30 s.
Alerts: each removal is an alert naming the paths. A persistent finding (a link it couldn't remove, an unreadable dir, a scan error) alerts once per distinct state.
Alerts queue until Notify succeeds. The next scan of a cleaned brain is clean, so without the queue an ntfy outage would lose the removal alert.
Startup still refuses on any symlink (unchanged).
Tests (red first)
TestWatchRemovesSymlinksButNeverTheirTargetsOrRegularFiles: the link is gone, and the target and a regular file are byte-identical. The alert says "removed" and names the path.
TestDefaultIntervalIsThirtySeconds.
The existing retry test now also covers a queued alert after the link is already gone (notifier fails twice).
-race -count=3 green.
Known limits
TOCTOU between Lstat and Remove: a symlink swapped for a regular file in that window would delete the regular file. Exploiting that needs write access to the brain dir, and anyone with that can delete files directly.
Exposure is now at most one interval (30 s). Option 1 (brain-wide os.Root readers) follows as defense in depth.
Refs #157. Step 1 of Mathias's "both, auto-remove first" decision.
## Why
The live tripwire (#159) only alerted. A symlink planted in the Syncthing-shared brain dir stayed readable to search, vectorstore and the other readers until a human removed it.
## What
- `symlinkguard.Watch` now unlinks every symlink it finds, through an `os.Root` on the resolved brain dir. It re-checks with Lstat that the entry is still a symlink and removes only the link, never its target.
- `DefaultInterval` drops from 5 min to **30 s**.
- **Alerts:** each removal is an alert naming the paths. A persistent finding (a link it couldn't remove, an unreadable dir, a scan error) alerts once per distinct state.
- Alerts queue until Notify succeeds. The next scan of a cleaned brain is clean, so without the queue an ntfy outage would lose the removal alert.
- Startup still refuses on any symlink (unchanged).
## Tests (red first)
- `TestWatchRemovesSymlinksButNeverTheirTargetsOrRegularFiles`: the link is gone, and the target and a regular file are byte-identical. The alert says "removed" and names the path.
- `TestDefaultIntervalIsThirtySeconds`.
- The existing retry test now also covers a queued alert after the link is already gone (notifier fails twice).
- `-race -count=3` green.
## Known limits
- **TOCTOU between Lstat and Remove:** a symlink swapped for a regular file in that window would delete the regular file. Exploiting that needs write access to the brain dir, and anyone with that can delete files directly.
- Exposure is now at most one interval (30 s). Option 1 (brain-wide `os.Root` readers) follows as defense in depth.
risk/medium: control code (rule 8).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---
_Created via git-mcp on behalf of @mathias_
Decision (#157): the tripwire only alerted, so a symlink planted in the
Syncthing-shared brain dir stayed readable until a human removed it.
Watch now unlinks every symlink it finds through an os.Root on the brain
dir (re-checking it is still a symlink; the target is never touched),
and the default scan interval drops from 5 min to 30 s, so a planted
link is readable for at most one interval.
Each removal is an alert naming what was removed; a persistent finding
(a link it could not remove, an unreadable dir, a scan error) alerts
once per distinct state. Alerts queue until Notify succeeds, so an ntfy
outage delays them without losing them, even though the next scan of
the cleaned brain is clean.
The startup check still refuses to start on any symlink.
Refs #157
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pr-gatekeeper verdict: BLOCK (PR #160 @ 504feb3). Not merged.
The removal itself is correct, and I verified that by execution (table below). Two things block. CI is red, and the alert path can be wedged by the very party it is meant to report. One claimed guard also has no assertion.
Blocking
CI run 8912 is red.
The failure is TestRawReadersLeakNothingUnderASymlinkSwapRace, from #156, not this PR.
Its assertion passed (0 leaks). The t.TempDir cleanup failed with "directory not empty" because the test never waits for its swapper goroutine.
Red is still an automatic block. Filed as #162 with the cause and the fix.
Locally CI=1 task check was EXIT=0, so this is a flake, but it is the estate's gate.
symlinkguard.go Watch/pending: the alert queue is unbounded, and one rejected message wedges it permanently.
The paths in the alert are chosen by whoever plants the links, and nothing bounds them.
The message is a single string with no limit on path count or length.
Notify is retried head-first with break. A message the server will never accept therefore blocks every later alert forever, while pending keeps growing by one per removal event.
Repro (executed): plant 2000 symlinks with 200-byte names, using a notifier that rejects bodies over a size limit, as a stand-in for ntfy's limits. All links were removed, but delivered=0, including 20 later, ordinary single-link incidents; rejected attempts=70.
Live ntfy (v2.11) has attachments enabled. Bodies over 4 KB become attachments (an alert of about 30 paths already arrives as "attachment.txt"), and bodies over the attachment size limit are refused.
Separately, an ntfy outage with re-planting queued 300 messages, then flushed all 300 at once on recovery. At 30 s that is 2880 messages per outage-day, inside a pod limited to 128Mi.
Fix (a correction):
Cap the paths per message (for example, the first 10 plus "and N more").
Bound the queue by coalescing it into a single "N removal events, M paths since " summary when it exceeds a small cap.
Optionally, truncate or drop a message the server rejects with a 4xx, rather than retrying it forever.
symlinkguard.go removeSymlinks: the "still a symlink" Lstat re-check has no test.
The PR body names this re-check as the guard against deleting a regular file.
Replacing the check with fi == nil keeps the suite green.
Routing removal through os.RemoveAll(filepath.Join(resolved, rel)) instead of root.Remove also keeps it green, so the os.Root confinement is unasserted too.
A direct removeSymlinks test fixes both. It should pass a regular file, a path that goes through an in-root dir symlink, and a ../ path, and assert that each is reported failed and nothing was deleted.
Verified by execution (holds)
Case
Result
File target outside the brain
link removed, target intact
Dir target outside the brain, plus a symlink inside that dir
link removed; dir contents and the inner link (outside the brain) intact
In-brain targets: file, dir, a link-to-link chain, ../../..
all links removed, every regular file intact; 0 symlinks remain
Dangling link
removed
removeSymlinks called directly on a regular file, a path through an in-root dir link, a ../ escape, a missing path
all reported failed, nothing deleted
Link under an unreadable (0100) dir
not reachable; alerted once as an unreadable dir
Re-planting every tick (ntfy up)
98 alerts over about 100 ticks, one per removal event; no duplicate alert for a state that persists
Swap race (Lstat to Remove): I accept it. os.Root confines removal to inside the brain, and the only file that can be hit is the one at the very path the peer just replaced. A sendreceive peer can already delete any brain file outright by syncing a deletion. The race gives a remote party nothing it doesn't already have.
Non-blocking, for Mathias's attention
The removal propagates. The brain is a sendreceive folder, so unlinking on koala syncs the deletion to flamingo. A deliberate symlink there (an Obsidian or iCloud link) gets deleted on flamingo too. It is link only, never the target. That follows from the auto-remove decision, but the decision record does not state it.
Risk label: risk/medium, while #157 is risk/high and this code deletes files on a shared folder. #159 ended up risk/high.
The ntfy Title header is hard-coded to "brain-capture audit degraded", so symlink alerts arrive under that title (pre-existing in audit/ntfy.go).
A link that disappears between Scan and Lstat is reported as "failed", which gives one spurious persistent alert.
Rule 9: TestWatchRemovesSymlinksButNeverTheirTargetsOrRegularFiles checks one outside file target. Dir targets and in-brain targets are not asserted. They hold by execution (above), but the name is broader than what the test proves.
Flip tests
Mutation
Suite
no removal alert queued
red (4 tests)
drop a message on notify failure
red
drop the Lstat symlink re-check
green
os.RemoveAll outside os.Root
green
drop the rest of the queue after the first send
green
Deploy safety (read-only, re-checked)
Live /var/lib/supervisor/brain: 0 symlinks, 0 dirs unreadable by uid 1000, 0 find errors. Rollout removes nothing.
No BRAIN_SYMLINK_SCAN_INTERVAL is set on the deployment, so the new 30 s default applies.
Not checked
A real Syncthing peer, including the deletion propagating to flamingo.
Live ntfy's actual reject threshold (inferred from config; I did not post to it).
Readers other than search.
## pr-gatekeeper verdict: **BLOCK** (PR #160 @ 504feb3). Not merged.
The removal itself is correct, and I verified that by execution (table below). Two things block. CI is red, and the alert path can be wedged by the very party it is meant to report. One claimed guard also has no assertion.
### Blocking
1. **CI run 8912 is red.**
- The failure is `TestRawReadersLeakNothingUnderASymlinkSwapRace`, from #156, not this PR.
- Its assertion passed (0 leaks). The `t.TempDir` cleanup failed with "directory not empty" because the test never waits for its swapper goroutine.
- Red is still an automatic block. Filed as **#162** with the cause and the fix.
- Locally `CI=1 task check` was EXIT=0, so this is a flake, but it is the estate's gate.
2. **`symlinkguard.go` Watch/`pending`: the alert queue is unbounded, and one rejected message wedges it permanently.**
- The paths in the alert are chosen by whoever plants the links, and nothing bounds them.
- The message is a single string with no limit on path count or length.
- Notify is retried head-first with `break`. A message the server will never accept therefore blocks every later alert forever, while `pending` keeps growing by one per removal event.
- Repro (executed): plant 2000 symlinks with 200-byte names, using a notifier that rejects bodies over a size limit, as a stand-in for ntfy's limits. All links were removed, but **delivered=0**, including 20 later, ordinary single-link incidents; rejected attempts=70.
- Live ntfy (v2.11) has attachments enabled. Bodies over 4 KB become attachments (an alert of about 30 paths already arrives as "attachment.txt"), and bodies over the attachment size limit are refused.
- Separately, an ntfy outage with re-planting queued 300 messages, then flushed all 300 at once on recovery. At 30 s that is 2880 messages per outage-day, inside a pod limited to 128Mi.
- Fix (a correction):
- Cap the paths per message (for example, the first 10 plus "and N more").
- Bound the queue by coalescing it into a single "N removal events, M paths since <t>" summary when it exceeds a small cap.
- Optionally, truncate or drop a message the server rejects with a 4xx, rather than retrying it forever.
3. **`symlinkguard.go` removeSymlinks: the "still a symlink" Lstat re-check has no test.**
- The PR body names this re-check as the guard against deleting a regular file.
- Replacing the check with `fi == nil` keeps the suite green.
- Routing removal through `os.RemoveAll(filepath.Join(resolved, rel))` instead of `root.Remove` also keeps it green, so the `os.Root` confinement is unasserted too.
- A direct `removeSymlinks` test fixes both. It should pass a regular file, a path that goes through an in-root dir symlink, and a `../` path, and assert that each is reported failed and nothing was deleted.
### Verified by execution (holds)
| Case | Result |
|---|---|
| File target outside the brain | link removed, target intact |
| Dir target outside the brain, plus a symlink inside that dir | link removed; dir contents and the inner link (outside the brain) intact |
| In-brain targets: file, dir, a link-to-link chain, `../../..` | all links removed, every regular file intact; 0 symlinks remain |
| Dangling link | removed |
| `removeSymlinks` called directly on a regular file, a path through an in-root dir link, a `../` escape, a missing path | all reported failed, nothing deleted |
| Link under an unreadable (0100) dir | not reachable; alerted once as an unreadable dir |
| Re-planting every tick (ntfy up) | 98 alerts over about 100 ticks, one per removal event; no duplicate alert for a state that persists |
**Swap race (Lstat to Remove):** I accept it. `os.Root` confines removal to inside the brain, and the only file that can be hit is the one at the very path the peer just replaced. A sendreceive peer can already delete any brain file outright by syncing a deletion. The race gives a remote party nothing it doesn't already have.
### Non-blocking, for Mathias's attention
- **The removal propagates.** The brain is a sendreceive folder, so unlinking on koala syncs the deletion to flamingo. A deliberate symlink there (an Obsidian or iCloud link) gets deleted on flamingo too. It is link only, never the target. That follows from the auto-remove decision, but the decision record does not state it.
- **Risk label:** risk/medium, while #157 is risk/high and this code deletes files on a shared folder. #159 ended up risk/high.
- The ntfy `Title` header is hard-coded to "brain-capture audit degraded", so symlink alerts arrive under that title (pre-existing in `audit/ntfy.go`).
- A link that disappears between Scan and Lstat is reported as "failed", which gives one spurious persistent alert.
- Rule 9: `TestWatchRemovesSymlinksButNeverTheirTargetsOrRegularFiles` checks one outside file target. Dir targets and in-brain targets are not asserted. They hold by execution (above), but the name is broader than what the test proves.
### Flip tests
| Mutation | Suite |
|---|---|
| no removal alert queued | red (4 tests) |
| drop a message on notify failure | red |
| drop the Lstat symlink re-check | **green** |
| `os.RemoveAll` outside `os.Root` | **green** |
| drop the rest of the queue after the first send | **green** |
### Deploy safety (read-only, re-checked)
- Live `/var/lib/supervisor/brain`: 0 symlinks, 0 dirs unreadable by uid 1000, 0 find errors. Rollout removes nothing.
- No `BRAIN_SYMLINK_SCAN_INTERVAL` is set on the deployment, so the new 30 s default applies.
### Not checked
- A real Syncthing peer, including the deletion propagating to flamingo.
- Live ntfy's actual reject threshold (inferred from config; I did not post to it).
- Readers other than search.
PR #160 review:
- The planter chooses the paths, and one alert listed them all, while the
queue retried oldest-first forever: a message ntfy refuses (too large)
blocked every later alert (2000 links: 0 alerts delivered), and an
outage queued one message per removal, unbounded. Alerts now name at
most 10 paths and count the rest; the queue holds at most 5, further
alerts become one suppression summary; a message failing 5 times is
dropped so it cannot block the rest.
- removeSymlinks' Lstat re-check had no test: a direct test passes a
regular file, a path through an in-brain dir link, a ../ escape and a
missing path, and asserts all fail with nothing deleted. A Watch test
covers dir and in-brain targets. root.Remove (not RemoveAll) is
commented: it refuses a dir swapped in after the Lstat.
- Every queued alert is delivered after an outage (truncating the queue
after the first send is now red).
- hyperguild#162: race tests did not wait for their swapper goroutines
before t.TempDir cleanup ("directory not empty" in CI run 8912). Each
swapper is now stopped and awaited in a t.Cleanup.
Fixes#162
Refs #157
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Round-1 findings 1, 3 and 4 are closed, and I verified each by execution. CI run 8916 is green. The fix for finding 2 introduces a new silent loss: an ntfy outage longer than about 2.5 minutes loses alerts without any trace in the alert channel. That is the same class as #159 round-1 finding 3.
Blocking
symlinkguard.goalertQueue.flush: the drop-after-5-attempts rule can't tell a poison message from an outage, and drops are never counted.
When ntfy is down, each tick tries the head message once. After maxAttempts (5) ticks the head is dropped, then the next message, and so on.
At the 30 s default, an outage longer than 2.5 minutes, such as an ntfy pod restart or a node blip, discards the oldest alerts.
A drop only increments a log line. It does not increment suppressed, so the recovery summary undercounts and says nothing about the drops.
The summary is itself a queued item, so it can be dropped after 5 failures too.
Long outage, single incident: plant one link, keep the notifier down for about 30 ticks (15 minutes at the default), then recover. The link is removed, and delivered=0, msgs=[]. The only incident is never reported, and nothing says an alert was lost.
Outage with re-planting: 300 plants, then recovery. 6 messages delivered. The summary says "221 further alert(s) were suppressed", so 74 removal events are missing even from the count.
The Watch doc comment still says "an outage delays them but loses none", which is now false. TestWatchDeliversEveryQueuedAlertAfterAnOutage passes only because its outage is shorter than 5 ticks. Rule 9: the name claims "every" alert after "an outage", but the test only proves it for outages under 5 attempts.
Fix (a correction):
Count every drop into suppressed.
Keep the summary as a counter that is retried every tick and never dropped. Don't make it a queued item.
Correct the doc comment.
Add a test with an outage longer than maxAttempts ticks that asserts the summary reports the dropped count.
Optionally, drop only on a permanent 4xx and keep retrying on transport errors or 5xx. That would need Notify to expose the status code.
Verified by execution (holds)
Check
Result
2000-link poison repro (200-byte names, notifier rejects bodies over 4096)
all links removed; 21 delivered, 0 rejected; largest message 2217 bytes. The 20 later incidents all flowed.
Queue bound during an outage
6 delivered for 300 plants: 5 queued plus the summary. Bounded, but the count is wrong (above).
Removal guards
TestRemoveSymlinksRefusesNonSymlinksEscapesAndPathsThroughLinks covers a regular file, a path through an in-brain dir link, a ../ escape and a missing path
-race -count=20 on the api and watcher race tests, and -race -count=30 on symlinkguard: all green. Swappers are joined in a t.Cleanup that runs before TempDir removal.
Gate
CI=1 task check EXIT=0, 0 FAILs
Deploy safety
the live brain has 0 symlinks and 0 unreadable dirs
On the RemoveAll question: I agree that a dir swapped in after the Lstat can only be reached through a race, and that root.Remove refuses a non-empty dir. The code comment is enough; I don't need a race test.
Flip tests
Mutation
Suite
never drop the poison message
red
unbounded queue
red
no path cap
red
no suppressed summary
red
drop the Lstat check (round-1 gap)
red, per the author; covered by the new direct test
drops not counted in the summary
no test exists; the behaviour is absent
Not checked
A real Syncthing peer.
Live ntfy's actual reject threshold.
Readers other than search.
## pr-gatekeeper verdict, round 2: **BLOCK** (PR #160 @ 01de163). Not merged.
Round-1 findings 1, 3 and 4 are closed, and I verified each by execution. CI run 8916 is green. The fix for finding 2 introduces a new silent loss: **an ntfy outage longer than about 2.5 minutes loses alerts without any trace in the alert channel.** That is the same class as #159 round-1 finding 3.
### Blocking
**`symlinkguard.go` `alertQueue.flush`: the drop-after-5-attempts rule can't tell a poison message from an outage, and drops are never counted.**
- When ntfy is down, each tick tries the head message once. After `maxAttempts` (5) ticks the head is dropped, then the next message, and so on.
- At the 30 s default, an outage longer than 2.5 minutes, such as an ntfy pod restart or a node blip, discards the oldest alerts.
- A drop only increments a log line. It does not increment `suppressed`, so the recovery summary undercounts and says nothing about the drops.
- The summary is itself a queued item, so it can be dropped after 5 failures too.
Repro (executed at 01de163):
- **Long outage, single incident:** plant one link, keep the notifier down for about 30 ticks (15 minutes at the default), then recover. The link is removed, and **delivered=0, msgs=[]**. The only incident is never reported, and nothing says an alert was lost.
- **Outage with re-planting:** 300 plants, then recovery. 6 messages delivered. The summary says "221 further alert(s) were suppressed", so **74 removal events are missing even from the count.**
The Watch doc comment still says "an outage delays them but loses none", which is now false. `TestWatchDeliversEveryQueuedAlertAfterAnOutage` passes only because its outage is shorter than 5 ticks. Rule 9: the name claims "every" alert after "an outage", but the test only proves it for outages under 5 attempts.
Fix (a correction):
- Count every drop into `suppressed`.
- Keep the summary as a counter that is retried every tick and never dropped. Don't make it a queued item.
- Correct the doc comment.
- Add a test with an outage longer than `maxAttempts` ticks that asserts the summary reports the dropped count.
Optionally, drop only on a permanent 4xx and keep retrying on transport errors or 5xx. That would need Notify to expose the status code.
### Verified by execution (holds)
| Check | Result |
|---|---|
| 2000-link poison repro (200-byte names, notifier rejects bodies over 4096) | all links removed; **21 delivered, 0 rejected**; largest message 2217 bytes. The 20 later incidents all flowed. |
| Queue bound during an outage | 6 delivered for 300 plants: 5 queued plus the summary. Bounded, but the count is wrong (above). |
| Removal guards | `TestRemoveSymlinksRefusesNonSymlinksEscapesAndPathsThroughLinks` covers a regular file, a path through an in-brain dir link, a `../` escape and a missing path |
| #162 deflake | `-race -count=20` on the api and watcher race tests, and `-race -count=30` on symlinkguard: all green. Swappers are joined in a `t.Cleanup` that runs before TempDir removal. |
| Gate | `CI=1 task check` EXIT=0, 0 FAILs |
| Deploy safety | the live brain has 0 symlinks and 0 unreadable dirs |
On the RemoveAll question: I agree that a dir swapped in after the Lstat can only be reached through a race, and that `root.Remove` refuses a non-empty dir. The code comment is enough; I don't need a race test.
### Flip tests
| Mutation | Suite |
|---|---|
| never drop the poison message | red |
| unbounded queue | red |
| no path cap | red |
| no suppressed summary | red |
| drop the Lstat check (round-1 gap) | red, per the author; covered by the new direct test |
| drops not counted in the summary | **no test exists**; the behaviour is absent |
### Not checked
- A real Syncthing peer.
- Live ntfy's actual reject threshold.
- Readers other than search.
PR #160 round 2: dropping a message after 5 failures could not tell a
poison message from an ntfy outage. A 15-minute outage dropped the only
incident with nothing reported, and drops were not counted (74 of 300
removals were neither delivered nor counted).
- audit.NtfyNotifier returns *RejectedError for non-2xx; Permanent() is
true for 4xx other than 429.
- The tripwire queue drops a message only on a permanent rejection and
counts it; every other failure is an outage and is retried, however
long. Each flush tries every queued message, so an undeliverable one
cannot block the rest.
- Overflow and drops go into one counter, sent as a summary retried
every tick and never dropped. Every removal is delivered or counted.
Tests (red first): an outage of ~100 ticks still delivers the only
incident; delivered + counted equals the number of removals; a
permanently rejected message is dropped and counted. The short-outage
test is renamed to say it is short. The Watch doc no longer claims
'loses none' unconditionally.
Refs #157
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Flush runs in the scan goroutine, and it tries every queued message plus the summary, each with a 10 s HTTP timeout.
So an ntfy that accepts connections but never answers can stall scanning for about 60 s, and a link then lives about 90 s instead of 30 s.
Scaled repro (executed): interval 50 ms, notifier latency 40 ms, queue full. The worst link lifetime was 244 ms.
It is bounded and needs a hung connection rather than a refusal, so I don't block on it. The fix is a correction: stop the flush at the first transient failure, or deliver from a separate goroutine.
Other open items: the ntfy title header predates this PR (the author will file it), and #157 records that deletions propagate to flamingo.
Not checked
A real Syncthing peer.
Live ntfy's actual thresholds (I did not post to it).
Readers other than search.
## pr-gatekeeper verdict, round 3: **PASS** (PR #160 @ 4ba9d20). Not merged.
This is a verdict only. The PR is risk/high and CD deploys on merge, so the merge is Mathias's call.
### Round-2 blocker: closed, verified by execution
| Probe | Result |
|---|---|
| Long outage, a single incident, ~100 ticks down, with **503** | delivered after recovery (1 message, names `only.md`) |
| Same, with **429** | delivered after recovery; nothing dropped |
| 300 plants during a 503 outage | 5 removal messages delivered + 295 counted in the summary = **300** |
| 300 plants during a 429 storm | 5 + 295 = **300** |
| Poison message: 2000 long-named links against a notifier limit forced down to 1000 bytes, so the alert gets a permanent 413 | dropped; the summary says "1 further alert(s) were suppressed or undeliverable", and the next incident (`s.md`) is delivered |
At the real ntfy limits, the capped alert (largest 2217 bytes in round 2) does not get a 413 at all.
### Flip tests: all red
| Mutation | Suite |
|---|---|
| never drop a permanent rejection | red (`TestWatchDropsAPoisonMessageAndCountsIt`) |
| drop on any failure | red (`...AfterAShortOutage`, `...LosesNoAlertThroughALongOutage`) |
| a permanent drop is not counted | red |
| 429 classified as permanent | red (`TestNotifyClassifiesRejectionsAsPermanentOrNot`) |
| ntfy returns an untyped error | red (same) |
| the summary is dropped when its send fails | red (`...QueueIsBoundedDuringAnOutage`, `...AccountsForEveryRemovalDuringAnOutage`) |
**Rule 9:** I read the new and renamed test names against their assertions, and they match.
### Gate and CI
- CI run 8919 on 4ba9d20: success.
- Local `CI=1 task check`: EXIT=0, 0 FAILs.
- `-race -count=10` on symlinkguard and audit: green.
### Deploy safety (read-only, re-checked)
- Live `/var/lib/supervisor/brain`: 0 symlinks, 0 unreadable dirs, 0 find errors. Rollout removes nothing.
- The 30 s default applies, since no override is set on the deployment.
### Non-blocking, filed
**#164: a hung ntfy delays removal.**
- Flush runs in the scan goroutine, and it tries every queued message plus the summary, each with a 10 s HTTP timeout.
- So an ntfy that accepts connections but never answers can stall scanning for about 60 s, and a link then lives about 90 s instead of 30 s.
- Scaled repro (executed): interval 50 ms, notifier latency 40 ms, queue full. The worst link lifetime was 244 ms.
- It is bounded and needs a hung connection rather than a refusal, so I don't block on it. The fix is a correction: stop the flush at the first transient failure, or deliver from a separate goroutine.
Other open items: the ntfy title header predates this PR (the author will file it), and #157 records that deletions propagate to flamingo.
### Not checked
- A real Syncthing peer.
- Live ntfy's actual thresholds (I did not post to it).
- Readers other than search.
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. Step 1 of Mathias's "both, auto-remove first" decision.
Why
The live tripwire (#159) only alerted. A symlink planted in the Syncthing-shared brain dir stayed readable to search, vectorstore and the other readers until a human removed it.
What
symlinkguard.Watchnow unlinks every symlink it finds, through anos.Rooton the resolved brain dir. It re-checks with Lstat that the entry is still a symlink and removes only the link, never its target.DefaultIntervaldrops from 5 min to 30 s.Tests (red first)
TestWatchRemovesSymlinksButNeverTheirTargetsOrRegularFiles: the link is gone, and the target and a regular file are byte-identical. The alert says "removed" and names the path.TestDefaultIntervalIsThirtySeconds.-race -count=3green.Known limits
os.Rootreaders) follows as defense in depth.risk/medium: control code (rule 8).
🤖 Generated with Claude Code
Created via git-mcp on behalf of @mathias
pr-gatekeeper verdict: BLOCK (PR #160 @
504feb3). Not merged.The removal itself is correct, and I verified that by execution (table below). Two things block. CI is red, and the alert path can be wedged by the very party it is meant to report. One claimed guard also has no assertion.
Blocking
CI run 8912 is red.
TestRawReadersLeakNothingUnderASymlinkSwapRace, from #156, not this PR.t.TempDircleanup failed with "directory not empty" because the test never waits for its swapper goroutine.CI=1 task checkwas EXIT=0, so this is a flake, but it is the estate's gate.symlinkguard.goWatch/pending: the alert queue is unbounded, and one rejected message wedges it permanently.break. A message the server will never accept therefore blocks every later alert forever, whilependingkeeps growing by one per removal event.symlinkguard.goremoveSymlinks: the "still a symlink" Lstat re-check has no test.fi == nilkeeps the suite green.os.RemoveAll(filepath.Join(resolved, rel))instead ofroot.Removealso keeps it green, so theos.Rootconfinement is unasserted too.removeSymlinkstest fixes both. It should pass a regular file, a path that goes through an in-root dir symlink, and a../path, and assert that each is reported failed and nothing was deleted.Verified by execution (holds)
../../..removeSymlinkscalled directly on a regular file, a path through an in-root dir link, a../escape, a missing pathSwap race (Lstat to Remove): I accept it.
os.Rootconfines removal to inside the brain, and the only file that can be hit is the one at the very path the peer just replaced. A sendreceive peer can already delete any brain file outright by syncing a deletion. The race gives a remote party nothing it doesn't already have.Non-blocking, for Mathias's attention
Titleheader is hard-coded to "brain-capture audit degraded", so symlink alerts arrive under that title (pre-existing inaudit/ntfy.go).TestWatchRemovesSymlinksButNeverTheirTargetsOrRegularFileschecks one outside file target. Dir targets and in-brain targets are not asserted. They hold by execution (above), but the name is broader than what the test proves.Flip tests
os.RemoveAlloutsideos.RootDeploy safety (read-only, re-checked)
/var/lib/supervisor/brain: 0 symlinks, 0 dirs unreadable by uid 1000, 0 find errors. Rollout removes nothing.BRAIN_SYMLINK_SCAN_INTERVALis set on the deployment, so the new 30 s default applies.Not checked
pr-gatekeeper verdict, round 2: BLOCK (PR #160 @
01de163). Not merged.Round-1 findings 1, 3 and 4 are closed, and I verified each by execution. CI run 8916 is green. The fix for finding 2 introduces a new silent loss: an ntfy outage longer than about 2.5 minutes loses alerts without any trace in the alert channel. That is the same class as #159 round-1 finding 3.
Blocking
symlinkguard.goalertQueue.flush: the drop-after-5-attempts rule can't tell a poison message from an outage, and drops are never counted.maxAttempts(5) ticks the head is dropped, then the next message, and so on.suppressed, so the recovery summary undercounts and says nothing about the drops.Repro (executed at
01de163):The Watch doc comment still says "an outage delays them but loses none", which is now false.
TestWatchDeliversEveryQueuedAlertAfterAnOutagepasses only because its outage is shorter than 5 ticks. Rule 9: the name claims "every" alert after "an outage", but the test only proves it for outages under 5 attempts.Fix (a correction):
suppressed.maxAttemptsticks that asserts the summary reports the dropped count.Optionally, drop only on a permanent 4xx and keep retrying on transport errors or 5xx. That would need Notify to expose the status code.
Verified by execution (holds)
TestRemoveSymlinksRefusesNonSymlinksEscapesAndPathsThroughLinkscovers a regular file, a path through an in-brain dir link, a../escape and a missing path-race -count=20on the api and watcher race tests, and-race -count=30on symlinkguard: all green. Swappers are joined in at.Cleanupthat runs before TempDir removal.CI=1 task checkEXIT=0, 0 FAILsOn the RemoveAll question: I agree that a dir swapped in after the Lstat can only be reached through a race, and that
root.Removerefuses a non-empty dir. The code comment is enough; I don't need a race test.Flip tests
Not checked
pr-gatekeeper verdict, round 3: PASS (PR #160 @
4ba9d20). Not merged.This is a verdict only. The PR is risk/high and CD deploys on merge, so the merge is Mathias's call.
Round-2 blocker: closed, verified by execution
only.md)s.md) is deliveredAt the real ntfy limits, the capped alert (largest 2217 bytes in round 2) does not get a 413 at all.
Flip tests: all red
TestWatchDropsAPoisonMessageAndCountsIt)...AfterAShortOutage,...LosesNoAlertThroughALongOutage)TestNotifyClassifiesRejectionsAsPermanentOrNot)...QueueIsBoundedDuringAnOutage,...AccountsForEveryRemovalDuringAnOutage)Rule 9: I read the new and renamed test names against their assertions, and they match.
Gate and CI
4ba9d20: success.CI=1 task check: EXIT=0, 0 FAILs.-race -count=10on symlinkguard and audit: green.Deploy safety (read-only, re-checked)
/var/lib/supervisor/brain: 0 symlinks, 0 unreadable dirs, 0 find errors. Rollout removes nothing.Non-blocking, filed
#164: a hung ntfy delays removal.
Other open items: the ntfy title header predates this PR (the author will file it), and #157 records that deletions propagate to flamingo.
Not checked