From 497cd5b6f31044ed0d1fcadc48409103eb1a80f3 Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 23:24:41 +0200 Subject: [PATCH] fix(brain_edit): snapshot, rollback and lock file go through the brain root (#192) 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 ".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 --- ingestion/internal/brainfs/brainfs.go | 52 ++++++++++++++++++ ingestion/internal/mcp/edit_pathio_test.go | 61 ++++++++++++++++++++++ ingestion/internal/mcp/tools_notes.go | 13 +++-- ingestion/internal/notes/edit.go | 54 ++++++++++++++----- 4 files changed, 164 insertions(+), 16 deletions(-) create mode 100644 ingestion/internal/mcp/edit_pathio_test.go diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go index f7f423d..87163a9 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/ingestion/internal/brainfs/brainfs.go @@ -22,6 +22,10 @@ import ( // ErrNotRegular is returned for an entry that is not a regular file. var ErrNotRegular = errors.New("not a regular file") +// ErrLockChanged is returned by OpenLock when the lock file was replaced +// while it was being opened. +var ErrLockChanged = errors.New("lock file replaced while opening") + // ErrTooLarge is returned by ReadRegularMax for a file over the limit. var ErrTooLarge = errors.New("file too large") @@ -270,6 +274,54 @@ func (b *Brain) mkdirReal(rel string) error { return 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, so the Lstat pre-check is +// what refuses a dangling in-brain link). A link swapped in between the +// pre-check and the open can still have an empty file created at an in-brain +// target before the refusal: the #167 residual. ErrLockChanged reports a +// lock replaced while opening (a peer removed and recreated it); callers +// retry. +func (b *Brain) OpenLock(rel string) (*os.File, error) { + if err := b.RealParent(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 || !hfi.Mode().IsRegular() { + _ = f.Close() + return nil, fmt.Errorf("%s: %w", rel, ErrNotRegular) + } + if lerr != nil || !os.SameFile(lfi, hfi) { + _ = f.Close() + if lerr == nil && !lfi.Mode().IsRegular() { + return nil, fmt.Errorf("%s: %w", rel, ErrNotRegular) + } + return nil, fmt.Errorf("%s: %w", rel, ErrLockChanged) + } + return f, nil +} + +// LockIsCurrent reports whether f is still the file at rel: after taking a +// flock, a lock file removed and recreated by a peer means the flock is on +// an orphaned inode and does not exclude anyone (hyperguild#192). +func (b *Brain) LockIsCurrent(rel string, f *os.File) bool { + hfi, herr := f.Stat() + lfi, lerr := b.root.Lstat(rel) + return herr == nil && lerr == nil && os.SameFile(lfi, hfi) +} + // 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) { diff --git a/ingestion/internal/mcp/edit_pathio_test.go b/ingestion/internal/mcp/edit_pathio_test.go new file mode 100644 index 0000000..21ee56f --- /dev/null +++ b/ingestion/internal/mcp/edit_pathio_test.go @@ -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 ".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") + }) + } +} diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index bcfa12a..e906308 100644 --- a/ingestion/internal/mcp/tools_notes.go +++ b/ingestion/internal/mcp/tools_notes.go @@ -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) diff --git a/ingestion/internal/notes/edit.go b/ingestion/internal/notes/edit.go index c289db8..8436760 100644 --- a/ingestion/internal/notes/edit.go +++ b/ingestion/internal/notes/edit.go @@ -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,21 +220,49 @@ 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 ".lock" link made a successful +// edit create a file at its target. +// +// Unlock removes the lock file while still holding the flock, so a waiter +// can wake holding a lock on an orphaned inode while the next caller locks a +// fresh file: two editors at once (measured on main). After taking the +// flock, the file must still be the one at the path; otherwise retry. +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) } - 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) + for attempt := 0; attempt < 1000; attempt++ { + f, err := bfs.OpenLock(lockRel) + if errors.Is(err, brainfs.ErrLockChanged) { + continue + } + if err != nil { + _ = bfs.Close() + return nil, fmt.Errorf("notes: lock %s: %w", lockRel, err) + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { + _ = f.Close() + _ = bfs.Close() + return nil, fmt.Errorf("notes: flock %s: %w", lockRel, err) + } + if !bfs.LockIsCurrent(lockRel, f) { + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + _ = f.Close() + continue + } + return func() { + _ = bfs.Remove(lockRel) + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + _ = f.Close() + _ = bfs.Close() + }, nil } - return func() { - _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) - _ = f.Close() - _ = os.Remove(abs + ".lock") - }, nil + _ = bfs.Close() + return nil, fmt.Errorf("notes: lock %s: kept changing while locking", lockRel) } // writeAtomic replaces the file via a same-directory temp + rename, then