fix(notes): the edit lock excludes other processes; pin OpenLock and the rollback (#192)
Review round 1 on PR #194: - The edit lock did not exclude other processes (also on main): unlock removes the lock file, so a waiter could wake holding a flock on the orphaned inode while the next caller locked a fresh file. flockFile now re-checks after flock that the locked file is still the one at the path (brainfs LockIsCurrent) and retries otherwise; OpenLock reports a lock replaced mid-open as ErrLockChanged, which is retried instead of failing the edit. New cross-process test (3 re-executed processes x 400 rounds, O_EXCL marker under the lock): 0 overlaps; red without the re-check. - OpenLock unit test: relative dangling link (only the Lstat pre-check stops it; os.Root refuses absolute links on its own), live link, FIFO, directory, behind a symlinked dir, unclean path, positive control. - Rollback test: a symlink swapped in for the note before the rollback (test seam beforeStampHook) is refused, target untouched in and outside the brain. - The in-brain dangling-lock case uses a relative link and asserts the refusal names the lock. OpenLock reuses #189's RealParent; RemoveLock is dropped for brainfs.Remove. The doc comment names the swap residual. Rebased onto main after #189. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -10,6 +10,11 @@ import (
|
||||
"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"
|
||||
)
|
||||
|
||||
@@ -45,17 +50,69 @@ func TestBrainEditReturnsPromptlyOnAFIFONoteWithAWriter(t *testing.T) {
|
||||
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)
|
||||
require.NoError(t, os.Symlink(tgt, filepath.Join(dir, "wiki", "homelab", "facts", "n.md.lock")))
|
||||
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)
|
||||
_ = call(t, srv, "brain_edit", map[string]any{"path": "wiki/homelab/facts/n.md", "old_str": "beta", "new_str": "BETA"})
|
||||
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"])
|
||||
got, _ := os.ReadFile(victim)
|
||||
assert.Equal(t, "untouched", string(got), "the rollback wrote through the swapped-in symlink")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user