diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go index 97bf8aa..f8c323a 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/ingestion/internal/brainfs/brainfs.go @@ -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) { 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..d5fe7c5 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,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 ".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 }