Compare commits

...
Author SHA1 Message Date
mathiasandClaude Opus 5.5 47354dc4fc fix(brain_edit): snapshot, rollback and lock file go through the brain root (#192)
CI / Lint / Test / Vet (pull_request) Successful in 1m7s
CI / Mirror to GitHub (pull_request) Skipped
brain_edit did its own I/O by path around notes.Edit:
- the pre-edit snapshot used os.ReadFile, which hung on a FIFO note held
  open by a writer;
- the rollback (when the provenance stamp fails) used os.WriteFile;
- the edit lock "<note>.lock" was opened with O_CREATE by path, so a
  planted dangling .lock symlink made a successful edit create a file at
  the link's target, in the brain or outside it.

Snapshot is brainfs ReadRegular, rollback WriteRegular, and the lock is
opened and removed through new brainfs OpenLock/RemoveLock: clean path,
real parent directories, an existing lock must be regular, and the opened
handle must be the file at the path (SameFile).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 23:24:41 +02:00
4 changed files with 133 additions and 10 deletions
+43
View File
@@ -136,6 +136,49 @@ func (b *Brain) ReadDirDirs(relDir string) ([]string, error) {
return names, nil
}
// OpenLock opens (creating if absent) a regular lock file at a clean
// brain-relative path for flock, through the root (hyperguild#192): every
// directory above it must be real, an existing lock must be a regular file,
// and the opened handle must be the file at the path (os.Root follows an
// in-root final symlink even with O_NOFOLLOW). A dangling or swapped-in
// symlink is refused; nothing is created at its target.
func (b *Brain) OpenLock(rel string) (*os.File, error) {
if err := cleanRel(rel); err != nil {
return nil, err
}
if err := b.realDir(path.Dir(rel)); err != nil {
return nil, err
}
if fi, err := b.root.Lstat(rel); err == nil && !fi.Mode().IsRegular() {
return nil, fmt.Errorf("%s: %w", rel, ErrNotRegular)
} else if err != nil && !errors.Is(err, fs.ErrNotExist) {
return nil, err
}
f, err := b.root.OpenFile(rel, os.O_RDWR|os.O_CREATE|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0o644)
if err != nil {
return nil, err
}
hfi, herr := f.Stat()
lfi, lerr := b.root.Lstat(rel)
if herr != nil || lerr != nil || !lfi.Mode().IsRegular() || !os.SameFile(lfi, hfi) {
_ = f.Close()
return nil, fmt.Errorf("%s: changed while opening: %w", rel, ErrNotRegular)
}
return f, nil
}
// RemoveLock removes the lock file at rel through the root (a symlink there
// is removed itself, never its target).
func (b *Brain) RemoveLock(rel string) error {
if err := cleanRel(rel); err != nil {
return err
}
if err := b.realDir(path.Dir(rel)); err != nil {
return err
}
return b.root.Remove(rel)
}
// ReadRegular reads a brain-relative path, refusing anything that is not a
// regular file, both before the open and on the opened handle.
func (b *Brain) ReadRegular(rel string) ([]byte, error) {
@@ -0,0 +1,61 @@
package mcp_test
import (
"os"
"path/filepath"
"syscall"
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/mathiasbq/hyperguild/ingestion/internal/mcp"
)
// hyperguild#192: brain_edit snapshotted the note with os.ReadFile by path,
// which blocks on a FIFO held open by a writer, and hung the call.
func TestBrainEditReturnsPromptlyOnAFIFONoteWithAWriter(t *testing.T) {
dir := newBrainRepo(t)
fifo := filepath.Join(dir, "wiki", "homelab", "facts", "f.md")
require.NoError(t, syscall.Mkfifo(fifo, 0o644))
w, err := os.OpenFile(fifo, os.O_RDWR, 0)
require.NoError(t, err)
t.Cleanup(func() { _ = w.Close() })
srv := mcp.NewServer(dir, nil, nil, nil)
done := make(chan map[string]any, 1)
go func() {
done <- call(t, srv, "brain_edit", map[string]any{"path": "wiki/homelab/facts/f.md", "old_str": "a", "new_str": "b"})
}()
select {
case resp := <-done:
assert.Contains(t, resp, "error", "a FIFO note is refused")
case <-time.After(3 * time.Second):
t.Fatal("brain_edit hung on a FIFO note")
}
resp := call(t, srv, "brain_edit", map[string]any{"path": "wiki/homelab/facts/n.md", "old_str": "beta", "new_str": "BETA"})
require.NotContains(t, resp, "error", "positive control: a regular note is edited: %v", resp["error"])
}
// hyperguild#192: the edit lock "<note>.lock" was opened by path with
// O_CREATE, so a planted dangling .lock symlink made a successful edit create
// a file at the link's target, outside the brain or elsewhere in it.
func TestBrainEditDoesNotCreateAFileThroughADanglingLockSymlink(t *testing.T) {
for name, target := range map[string]func(t *testing.T, dir string) string{
"outside": func(t *testing.T, _ string) string { return filepath.Join(t.TempDir(), "created") },
"in-brain": func(t *testing.T, dir string) string { return filepath.Join(dir, "raw", "created") },
} {
t.Run(name, func(t *testing.T) {
dir := newBrainRepo(t)
require.NoError(t, os.MkdirAll(filepath.Join(dir, "raw"), 0o755))
tgt := target(t, dir)
require.NoError(t, os.Symlink(tgt, filepath.Join(dir, "wiki", "homelab", "facts", "n.md.lock")))
srv := mcp.NewServer(dir, nil, nil, nil)
_ = call(t, srv, "brain_edit", map[string]any{"path": "wiki/homelab/facts/n.md", "old_str": "beta", "new_str": "BETA"})
_, err := os.Lstat(tgt)
assert.True(t, os.IsNotExist(err), "a file was created at the lock symlink's target")
})
}
}
+10 -3
View File
@@ -6,8 +6,8 @@ import (
"errors"
"fmt"
"github.com/mathiasbq/hyperguild/ingestion/internal/api"
"github.com/mathiasbq/hyperguild/ingestion/internal/brainfs"
"log/slog"
"os"
"strings"
"time"
@@ -118,7 +118,14 @@ func (s *Server) brainEdit(ctx context.Context, args json.RawMessage) (json.RawM
return nil, err
}
// No snapshot, no edit: without one a failed stamp could not be undone.
prior, err := os.ReadFile(target.Abs)
// Read through the brain root, regular files only (hyperguild#192): a
// FIFO held open by a writer hung os.ReadFile.
bfs, err := brainfs.Open(s.brainDir)
if err != nil {
return nil, fmt.Errorf("snapshot before edit: %w", err)
}
defer func() { _ = bfs.Close() }()
prior, err := bfs.ReadRegular(target.Rel)
if err != nil {
return nil, fmt.Errorf("snapshot before edit: %w", err)
}
@@ -131,7 +138,7 @@ func (s *Server) brainEdit(ctx context.Context, args json.RawMessage) (json.RawM
// to the editor after every edit (PR #147 review, escape E1).
origin, writtenBy := provenanceFrom(ctx)
if serr := api.StampProvenanceFile(s.brainDir, res.Path, origin, writtenBy); serr != nil {
if werr := os.WriteFile(target.Abs, prior, 0o644); werr != nil {
if werr := bfs.WriteRegular(target.Rel, prior); werr != nil {
// Say exactly what state the note is in: edited, unverified.
return nil, fmt.Errorf("stamp provenance failed (%v) AND the edit could NOT be rolled back: %w; "+
"%s carries the edit under its previous attribution", serr, werr, res.Path)
+19 -7
View File
@@ -127,7 +127,7 @@ func Edit(brainDir, path, oldStr, newStr string, scope Scope) (Result, error) {
mu.Lock()
defer mu.Unlock()
unlock, err := flockFile(t.Abs)
unlock, err := flockFile(t)
if err != nil {
return Result{}, err
}
@@ -220,20 +220,32 @@ func lockFor(abs string) *sync.Mutex {
}
// flockFile takes an advisory lock covering other processes sharing the
// hostPath — during a RollingUpdate two ingestion pods do.
func flockFile(abs string) (func(), error) {
f, err := os.OpenFile(abs+".lock", os.O_CREATE|os.O_RDWR, 0o644)
// hostPath — during a RollingUpdate two ingestion pods do. The lock file is
// opened and removed through the brain root, never through a symlink
// (hyperguild#192): a planted dangling "<note>.lock" link made a
// successful edit create a file at its target.
func flockFile(t Target) (func(), error) {
lockRel := t.Rel + ".lock"
bfs, err := brainfs.Open(t.Brain)
if err != nil {
return nil, fmt.Errorf("notes: lock %s: %w", filepath.Base(abs), err)
return nil, fmt.Errorf("notes: lock %s: %w", lockRel, err)
}
f, err := bfs.OpenLock(lockRel)
_ = bfs.Close()
if err != nil {
return nil, fmt.Errorf("notes: lock %s: %w", lockRel, err)
}
if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil {
_ = f.Close()
return nil, fmt.Errorf("notes: flock %s: %w", filepath.Base(abs), err)
return nil, fmt.Errorf("notes: flock %s: %w", lockRel, err)
}
return func() {
_ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN)
_ = f.Close()
_ = os.Remove(abs + ".lock")
if b, err := brainfs.Open(t.Brain); err == nil {
_ = b.RemoveLock(lockRel)
_ = b.Close()
}
}, nil
}