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/brainfs/brainfs_test.go b/ingestion/internal/brainfs/brainfs_test.go index 0f5570b..21d8ee1 100644 --- a/ingestion/internal/brainfs/brainfs_test.go +++ b/ingestion/internal/brainfs/brainfs_test.go @@ -529,3 +529,49 @@ func TestCreateRegularCreatesOnlyNewFilesThroughRealDirectories(t *testing.T) { got, _ = os.ReadFile(filepath.Join(dir, "wiki", "new", "facts", "n.md")) assert.Equal(t, "one", string(got)) } + +// hyperguild#192: OpenLock creates or reuses a regular lock file through the +// root and refuses every other shape without creating anything at a link's +// target or hanging. +func TestOpenLockOpensOnlyARegularLockThroughRealDirectories(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "raw"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "wiki", "w"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "raw", "v.md"), []byte("untouched"), 0o644)) + require.NoError(t, os.Symlink(filepath.Join("..", "..", "raw", "created"), filepath.Join(dir, "wiki", "w", "dangling.lock"))) + require.NoError(t, os.Symlink(filepath.Join("..", "..", "raw", "v.md"), filepath.Join(dir, "wiki", "w", "live.lock"))) + require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "wiki", "w", "fifo.lock"), 0o644)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "wiki", "w", "dir.lock"), 0o755)) + require.NoError(t, os.Symlink(filepath.Join("..", "raw"), filepath.Join(dir, "wiki", "alias"))) + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + + for _, rel := range []string{"wiki/w/dangling.lock", "wiki/w/live.lock", "wiki/w/fifo.lock", "wiki/w/dir.lock", "wiki/alias/x.lock", "wiki/w/../w/x.lock"} { + done := make(chan error, 1) + go func() { + f, err := b.OpenLock(rel) + if f != nil { + _ = f.Close() + } + done <- err + }() + select { + case err := <-done: + assert.ErrorIs(t, err, ErrNotRegular, rel) + case <-time.After(3 * time.Second): + t.Fatalf("OpenLock hung on %s", rel) + } + } + _, err = os.Lstat(filepath.Join(dir, "raw", "created")) + assert.True(t, os.IsNotExist(err), "nothing created at the dangling link's target") + got, _ := os.ReadFile(filepath.Join(dir, "raw", "v.md")) + assert.Equal(t, "untouched", string(got)) + + f, err := b.OpenLock("wiki/w/n.md.lock") + require.NoError(t, err, "positive control: creates a lock") + assert.True(t, b.LockIsCurrent("wiki/w/n.md.lock", f)) + require.NoError(t, b.Remove("wiki/w/n.md.lock")) + assert.False(t, b.LockIsCurrent("wiki/w/n.md.lock", f), "a removed lock is no longer current") + _ = f.Close() +} diff --git a/ingestion/internal/mcp/edit_pathio_test.go b/ingestion/internal/mcp/edit_pathio_test.go new file mode 100644 index 0000000..ab6d35b --- /dev/null +++ b/ingestion/internal/mcp/edit_pathio_test.go @@ -0,0 +1,124 @@ +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/audit" + "github.com/mathiasbq/hyperguild/ingestion/internal/brainstore" + "github.com/mathiasbq/hyperguild/ingestion/internal/capture" + "github.com/mathiasbq/hyperguild/ingestion/internal/capturehttp" + "github.com/mathiasbq/hyperguild/ingestion/internal/classification" + "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") }, + // Relative, as Syncthing carries it: os.Root refuses an absolute + // link on its own, so only OpenLock's pre-check stops this one. + "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) + lock := filepath.Join(dir, "wiki", "homelab", "facts", "n.md.lock") + linkTo := tgt + if name == "in-brain" { + rel, err := filepath.Rel(filepath.Dir(lock), tgt) + require.NoError(t, err) + linkTo = rel + } + require.NoError(t, os.Symlink(linkTo, lock)) + srv := mcp.NewServer(dir, nil, nil, nil) + resp := call(t, srv, "brain_edit", map[string]any{"path": "wiki/homelab/facts/n.md", "old_str": "beta", "new_str": "BETA"}) + require.NotNil(t, resp["error"], "the edit is refused") + assert.Contains(t, resp["error"].(map[string]any)["message"], "lock", "refused because of the lock file") + _, err := os.Lstat(tgt) + assert.True(t, os.IsNotExist(err), "a file was created at the lock symlink's target") + }) + } +} + +// The rollback (when the stamp cannot be verified) writes the prior bytes +// through the brain root: a symlink swapped in for the note before the +// rollback is refused, and its target, in the brain or outside, is +// untouched. +func TestBrainEditRollbackDoesNotWriteThroughASymlinkSwappedInForTheNote(t *testing.T) { + for name, mk := range map[string]func(t *testing.T, dir string) (link, victim string){ + "outside": func(t *testing.T, _ string) (string, string) { + v := filepath.Join(t.TempDir(), "victim.md") + return v, v + }, + "in-brain": func(t *testing.T, dir string) (string, string) { + return filepath.Join("..", "..", "..", "raw", "victim.md"), filepath.Join(dir, "raw", "victim.md") + }, + } { + t.Run(name, func(t *testing.T) { + dir := newBrainRepo(t) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "raw"), 0o755)) + cfg, err := classification.Load(dir) + require.NoError(t, err) + svc := capture.NewService(brainstore.New(dir), capFakeTracker{}, cfg, audit.NewSlogSink(nil)) + srv := mcp.NewServer(dir, nil, nil, nil) + srv.WithCapture(svc, capFakeValidator{subject: "bad\xff"}, capStaticTok, "local-cli", capturehttp.NewOriginResolver(nil)) + out := resultText(t, authCall(t, srv, "Bearer "+capStaticTok, "brain_write", map[string]any{ + "content": "# R\n\nstatic body", "wing": "testing", "hall": "facts", "filename": "rb", + })) + path := out["path"].(string) + note := filepath.Join(dir, path) + link, victim := mk(t, dir) + require.NoError(t, os.WriteFile(victim, []byte("untouched"), 0o644)) + defer mcp.SetBeforeStampHook(func() { + require.NoError(t, os.Remove(note)) + require.NoError(t, os.Symlink(link, note)) + })() + resp := authCall(t, srv, jwtAuthz, "brain_edit", map[string]any{"path": path, "old_str": "static body", "new_str": "injected"}) + require.NotNil(t, resp["error"]) + // The swap happened and the rollback refused it: the error says so, + // and the note path is still the planted symlink. + assert.Contains(t, resp["error"].(map[string]any)["message"], "could NOT be rolled back") + fi, err := os.Lstat(note) + require.NoError(t, err) + assert.NotZero(t, fi.Mode()&os.ModeSymlink, "the note path is still the swapped-in symlink") + got, _ := os.ReadFile(victim) + assert.Equal(t, "untouched", string(got), "the rollback wrote through the swapped-in symlink") + }) + } +} diff --git a/ingestion/internal/mcp/export_test.go b/ingestion/internal/mcp/export_test.go new file mode 100644 index 0000000..5afc51e --- /dev/null +++ b/ingestion/internal/mcp/export_test.go @@ -0,0 +1,9 @@ +package mcp + +// SetBeforeStampHook installs beforeStampHook for a test and returns a +// restore func. +func SetBeforeStampHook(f func()) func() { + old := beforeStampHook + beforeStampHook = f + return func() { beforeStampHook = old } +} diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index bcfa12a..0707045 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) } @@ -130,8 +137,11 @@ func (s *Server) brainEdit(ctx context.Context, args json.RawMessage) (json.RawM // whoever edits a note is the author of what a reader now sees: re-stamp // to the editor after every edit (PR #147 review, escape E1). origin, writtenBy := provenanceFrom(ctx) + if beforeStampHook != nil { + beforeStampHook() + } 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) @@ -141,6 +151,11 @@ func (s *Server) brainEdit(ctx context.Context, args json.RawMessage) (json.RawM return out, nil } +// beforeStampHook is a test seam (nil in production): it runs between the +// edit and the provenance stamp, so a test can swap the note before the +// rollback path runs. +var beforeStampHook func() + // brainEditRoot edits a governance file (CLAUDE.md). De-surfaced AND gated on // the authenticated principal: only the static local-CLI token may call it, so // a claude.ai-origin session — the one that may have just read an untrusted 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 diff --git a/ingestion/internal/notes/lock_xproc_test.go b/ingestion/internal/notes/lock_xproc_test.go new file mode 100644 index 0000000..ce3f2ae --- /dev/null +++ b/ingestion/internal/notes/lock_xproc_test.go @@ -0,0 +1,82 @@ +package notes + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// lockChild runs inside a re-executed test binary: it takes the note's edit +// lock N times and, while holding it, creates an O_EXCL marker. Finding the +// marker already there means another process holds the "same" lock: mutual +// exclusion failed. It prints the overlap count. +func lockChild(brainDir string, rounds int) { + t, err := Resolve(brainDir, "wiki/w/facts/n.md", ScopeNotes) + if err != nil { + fmt.Println("resolve:", err) + os.Exit(2) + } + marker := filepath.Join(brainDir, "marker") + overlaps := 0 + for i := 0; i < rounds; i++ { + unlock, err := flockFile(t) + if err != nil { + fmt.Println("lock:", err) + os.Exit(2) + } + f, err := os.OpenFile(marker, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o644) + if errors.Is(err, os.ErrExist) { + overlaps++ + } else if err == nil { + _ = f.Close() + _ = os.Remove(marker) + } + unlock() + } + fmt.Println(overlaps) + os.Exit(0) +} + +func TestMain(m *testing.M) { + if dir := os.Getenv("NOTES_LOCK_CHILD"); dir != "" { + n, _ := strconv.Atoi(os.Getenv("NOTES_LOCK_ROUNDS")) + lockChild(dir, n) + } + os.Exit(m.Run()) +} + +// hyperguild#192: the edit lock must exclude other processes (two ingestion +// pods share the hostPath brain during a RollingUpdate). Unlock removes the +// lock file, so without re-checking that the locked file is still the one at +// the path, a waiter and the next caller each held a lock (measured on main: +// hundreds of overlaps in 2000 rounds). +func TestEditLockExcludesOtherProcesses(t *testing.T) { + brain := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(brain, "wiki", "w", "facts"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(brain, "wiki", "w", "facts", "n.md"), []byte("x"), 0o644)) + const procs, rounds = 3, 400 + cmds := make([]*exec.Cmd, procs) + outs := make([]*strings.Builder, procs) + for i := range cmds { + outs[i] = &strings.Builder{} + cmds[i] = exec.Command(os.Args[0], "-test.run=^$") + cmds[i].Env = append(os.Environ(), "NOTES_LOCK_CHILD="+brain, "NOTES_LOCK_ROUNDS="+strconv.Itoa(rounds)) + cmds[i].Stdout, cmds[i].Stderr = outs[i], outs[i] + require.NoError(t, cmds[i].Start()) + } + total := 0 + for i, c := range cmds { + require.NoError(t, c.Wait(), "child %d: %s", i, outs[i].String()) + n, err := strconv.Atoi(strings.TrimSpace(outs[i].String())) + require.NoError(t, err, "child %d output: %q", i, outs[i].String()) + total += n + } + require.Zero(t, total, "processes held the edit lock at the same time") +}