mathias/hyperguild
1
0
Fork 0
Code Issues 30 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

brain_edit path I/O: snapshot read hangs on a FIFO, lock file and rollback write by path #192

New Issue
Open
opened 2026-10-05 20:55:37 +00:00 by mathias · 3 comments
mathias commented 2026-10-05 20:55:37 +00:00
Owner
Copy Link
Copy Source

Source: PR #189 review (#188), 2026-10-05, verified through MCP brain_edit. This was already the case on main and is not in #158's list.

  • Snapshot read: internal/mcp/tools_notes.go:121 reads the note's prior bytes with os.ReadFile(target.Abs). It hangs on a FIFO held open by a writer.
  • Rollback write: tools_notes.go:134 restores the prior bytes with os.WriteFile(target.Abs), by path.
  • Lock file: internal/notes/edit.go:225 opens <note>.lock with O_CREATE by path. A planted dangling <note>.lock symlink made a successful brain_edit create an empty file outside the brain (verified).

Fix

  • Snapshot through brainfs.ReadRegular, rollback through brainfs.WriteRegular.
  • Lock: open through the brain root with O_CREATE|O_NOFOLLOW plus a SameFile check, or keep the lock outside the Syncthing tree, keyed by note path. A lock file in a synced folder is odd anyway.

Done when: tests with a writer-held FIFO note and a dangling .lock symlink (in-brain and outside targets) show no hang and nothing created at the target, red before the fix.

Created via git-mcp on behalf of @mathias

**Source:** PR #189 review (#188), 2026-10-05, verified through MCP `brain_edit`. This was already the case on main and is not in #158's list. - **Snapshot read:** `internal/mcp/tools_notes.go:121` reads the note's prior bytes with `os.ReadFile(target.Abs)`. It **hangs on a FIFO held open by a writer**. - **Rollback write:** `tools_notes.go:134` restores the prior bytes with `os.WriteFile(target.Abs)`, by path. - **Lock file:** `internal/notes/edit.go:225` opens `<note>.lock` with `O_CREATE` by path. A planted dangling `<note>.lock` symlink made a successful brain_edit **create an empty file outside the brain** (verified). ## Fix - Snapshot through `brainfs.ReadRegular`, rollback through `brainfs.WriteRegular`. - Lock: open through the brain root with `O_CREATE|O_NOFOLLOW` plus a SameFile check, or keep the lock outside the Syncthing tree, keyed by note path. A lock file in a synced folder is odd anyway. **Done when:** tests with a writer-held FIFO note and a dangling `.lock` symlink (in-brain and outside targets) show no hang and nothing created at the target, red before the fix. --- _Created via git-mcp on behalf of @mathias_
mathias added the risk/high label 2026-10-05 20:55:37 +00:00
mathias commented 2026-10-05 21:33:21 +00:00
Author
Owner
Copy Link
Copy Source

pr-gatekeeper on PR #194 (head 47354dc): BLOCKED. The full review is on the PR (review id 16).

Blocking:

  1. Nothing asserts the rollback hardening. Revert it to os.WriteFile and the suite stays green.
  2. The in-brain dangling-lock subtest uses an absolute link, which os.Root refuses on its own. Remove the Lstat pre-check and the suite stays green, while a relative in-brain dangling .lock again creates a file at its target.
  3. OpenLock/RemoveLock have no unit tests. Eight guard mutations survive.

To file separately: flock plus unlink has never serialised processes. This predates the PR and is measured on main as well. The PR adds about 1% spurious lock failures under contention. The branch also conflicts with main in brainfs.go after #189.

Issue is risk/high, so Mathias makes the merge call.

pr-gatekeeper on PR #194 (head 47354dc): **BLOCKED**. The full review is on the PR (review id 16). Blocking: 1. Nothing asserts the rollback hardening. Revert it to `os.WriteFile` and the suite stays green. 2. The in-brain dangling-lock subtest uses an absolute link, which os.Root refuses on its own. Remove the Lstat pre-check and the suite stays green, while a relative in-brain dangling `.lock` again creates a file at its target. 3. `OpenLock`/`RemoveLock` have no unit tests. Eight guard mutations survive. To file separately: flock plus unlink has never serialised processes. This predates the PR and is measured on main as well. The PR adds about 1% spurious lock failures under contention. The branch also conflicts with main in brainfs.go after #189. Issue is `risk/high`, so Mathias makes the merge call.
mathias commented 2026-10-05 21:42:05 +00:00
Author
Owner
Copy Link
Copy Source

pr-gatekeeper round 2 on PR #194 (head 07ea5b0): BLOCKED on one finding. The full review is on the PR (review id 18).

The rollback-through-symlink test still passes when the test seam does not fire. With beforeStampHook() turned into a no-op, the suite stays green. It needs an assertion that the swap happened and the rollback was refused.

Everything else from round 1 is fixed and confirmed by execution. Locking across processes gives 0 overlaps with 2 and 4 processes.

Not blocking: the RollingUpdate that ships this change runs old and new code side by side, and that still overlaps. Exclusion holds from the next rollout.

Issue is risk/high, so Mathias makes the merge call.

pr-gatekeeper round 2 on PR #194 (head 07ea5b0): **BLOCKED** on one finding. The full review is on the PR (review id 18). The rollback-through-symlink test still passes when the test seam does not fire. With `beforeStampHook()` turned into a no-op, the suite stays green. It needs an assertion that the swap happened and the rollback was refused. Everything else from round 1 is fixed and confirmed by execution. Locking across processes gives 0 overlaps with 2 and 4 processes. Not blocking: the RollingUpdate that ships this change runs old and new code side by side, and that still overlaps. Exclusion holds from the next rollout. Issue is `risk/high`, so Mathias makes the merge call.
mathias commented 2026-10-05 21:46:58 +00:00
Author
Owner
Copy Link
Copy Source

pr-gatekeeper round 3 on PR #194 (head cdcff12): PASS. The full review is on the PR.

The round-2 blocker is fixed: turning the stamp hook into a no-op now fails the rollback test. Every earlier flip is still red.

go test -race -count=3 and CI=1 task check pass, CI run #608 is green, and the PR is mergeable.

Follow-ups are tracked in #196 and #197. The issue is risk/high: merging deploys ingestion, so Mathias decides the merge.

pr-gatekeeper round 3 on PR #194 (head cdcff12): **PASS**. The full review is on the PR. The round-2 blocker is fixed: turning the stamp hook into a no-op now fails the rollback test. Every earlier flip is still red. `go test -race -count=3` and `CI=1 task check` pass, CI run #608 is green, and the PR is mergeable. Follow-ups are tracked in #196 and #197. The issue is `risk/high`: merging deploys ingestion, so Mathias decides the merge.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
agent/ready
Meets the agent-ready contract; authorizes unattended execution
 blocked/client
Waiting on the client
 blocked/supplier
Waiting on a supplier
 blocked/upstream
Waiting on an upstream project or vendor
 risk/high
Irreversible, security-relevant, or control plane
 risk/low
Reversible, local, test-covered
 risk/medium
Touches shared state, deploys or data
 size/l
Too big - split it
 size/m
Under two days
 size/s
Under half a day
 size/xs
Under an hour
 status:dispatch
status:dispatched
status:review
status:reviewing
type/chore
Maintenance, no behaviour change
 type/decision
Human decision required; never agent/ready
 type/docs
Documentation only
 type/feat
New capability
 type/fix
Corrects broken behaviour
 type/ops
Infrastructure, deploy, runtime
 type/research
Investigate; output is a finding, not code
 type/test
Test coverage only
No labels risk/high
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate-bot mathias
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mathias/hyperguild#192