fix(watcher): refusals end in a terminal state; pin every converted writer (#158)
Review round 1 on PR #195: - A refused marker or archive write left no marker, so the note was reprocessed every interval (archive copies piling up; an LLM call per round on the failure path). The marker check now uses Lstat (any entry at the marker path counts, so a dangling marker link skips the note instead of looping), and a note whose archive copy or marker write is refused is remembered (path, size, mtime) and not retried until it changes. - cleanRel no longer refuses a backslash: it is a legal filename byte on Linux, not a traversal, and a raw note named with one was never archived or marked. - Tests now reach every converted writer: the log.md append (real raw/failed so the log is reached, symlinked log.md), a dangling marker link, the refused-retry loop with a positive control, a backslash name, a buildAndWrite swap race, and the claude-session archive with in-brain and outside symlinked archive dirs. The stale lint allowlist entry for appendWatcherLog is removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -3,12 +3,14 @@ package watcher
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"log/slog"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
"unicode"
|
||||
|
||||
@@ -110,14 +112,28 @@ func processDir(ctx context.Context, cfg Config, date string) []error {
|
||||
}
|
||||
|
||||
// Skip files that have already been processed or permanently failed.
|
||||
if _, err := os.Stat(path + ".processed"); err == nil {
|
||||
// Any entry at the marker path counts (Lstat, not Stat): a dangling
|
||||
// marker symlink made os.Stat report nothing and the note was
|
||||
// reprocessed every interval (hyperguild#158).
|
||||
if _, err := os.Lstat(path + ".processed"); err == nil {
|
||||
return nil
|
||||
}
|
||||
if _, err := os.Stat(path + ".failed"); err == nil {
|
||||
if _, err := os.Lstat(path + ".failed"); err == nil {
|
||||
return nil
|
||||
}
|
||||
// A note whose archive copy or marker was refused is not retried
|
||||
// until it changes: a refusal has no marker to stop the next round,
|
||||
// and each round can cost an LLM call (hyperguild#158).
|
||||
info, ierr := d.Info()
|
||||
if ierr == nil && refusedUnchanged(path, info) {
|
||||
return nil
|
||||
}
|
||||
|
||||
if err := processFile(ctx, cfg, root, rawDir, path, date); err != nil {
|
||||
if ierr == nil && errors.Is(err, brainfs.ErrNotRegular) {
|
||||
rememberRefused(path, info)
|
||||
slog.Error("watcher: note refused; not retried until it changes", "file", filepath.Base(path), "error", err)
|
||||
}
|
||||
errs = append(errs, fmt.Errorf("process %s: %w", filepath.Base(path), err))
|
||||
}
|
||||
return nil
|
||||
@@ -152,6 +168,9 @@ func processFile(ctx context.Context, cfg Config, root *os.Root, rawDir, path, d
|
||||
return fmt.Errorf("copy to failed: %w", cpErr)
|
||||
}
|
||||
if mkErr := putBrainFile(cfg.BrainDir, "raw/"+filepath.ToSlash(rel)+".failed", []byte(runErr.Error())); mkErr != nil {
|
||||
if errors.Is(mkErr, brainfs.ErrNotRegular) {
|
||||
return fmt.Errorf("write .failed marker: %w", mkErr)
|
||||
}
|
||||
slog.Error("watcher: failed to write .failed marker", "error", mkErr)
|
||||
}
|
||||
|
||||
@@ -177,6 +196,9 @@ func processFile(ctx context.Context, cfg Config, root *os.Root, rawDir, path, d
|
||||
return fmt.Errorf("copy to processed: %w", err)
|
||||
}
|
||||
if err := putBrainFile(cfg.BrainDir, "raw/"+filepath.ToSlash(rel)+".processed", []byte(date)); err != nil {
|
||||
if errors.Is(err, brainfs.ErrNotRegular) {
|
||||
return fmt.Errorf("write .processed marker: %w", err)
|
||||
}
|
||||
slog.Error("watcher: failed to write .processed marker", "error", err)
|
||||
}
|
||||
|
||||
@@ -184,6 +206,32 @@ func processFile(ctx context.Context, cfg Config, root *os.Root, rawDir, path, d
|
||||
return nil
|
||||
}
|
||||
|
||||
// refused remembers notes whose archive copy or marker write was refused,
|
||||
// by path, with the size and mtime they had, for the life of the process.
|
||||
var refused sync.Map
|
||||
|
||||
type refusedStamp struct {
|
||||
size int64
|
||||
mtime time.Time
|
||||
}
|
||||
|
||||
func rememberRefused(path string, info os.FileInfo) {
|
||||
refused.Store(path, refusedStamp{size: info.Size(), mtime: info.ModTime()})
|
||||
}
|
||||
|
||||
func refusedUnchanged(path string, info os.FileInfo) bool {
|
||||
v, ok := refused.Load(path)
|
||||
if !ok {
|
||||
return false
|
||||
}
|
||||
st := v.(refusedStamp)
|
||||
if st.size == info.Size() && st.mtime.Equal(info.ModTime()) {
|
||||
return true
|
||||
}
|
||||
refused.Delete(path)
|
||||
return false
|
||||
}
|
||||
|
||||
// deriveSource turns a filename into a human-readable source name.
|
||||
// "shape-up-book.md" → "Shape Up Book"
|
||||
func deriveSource(filename string) string {
|
||||
|
||||
@@ -33,7 +33,7 @@ func failingPipeline() pipeline.Config {
|
||||
// directory or log redirected the write elsewhere in the brain or outside
|
||||
// it. (A pre-planted marker symlink makes the watcher skip the note, so the
|
||||
// marker write itself is reachable only by a race.)
|
||||
func TestProcessDirDoesNotWriteThroughASymlinkedArchiveDirOrLog(t *testing.T) {
|
||||
func TestProcessDirDoesNotWriteThroughASymlinkedArchiveDir(t *testing.T) {
|
||||
for name, pcfg := range map[string]pipeline.Config{"processed": okPipeline(), "failed": failingPipeline()} {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
brainDir := setupBrainDir(t)
|
||||
@@ -42,12 +42,9 @@ func TestProcessDirDoesNotWriteThroughASymlinkedArchiveDirOrLog(t *testing.T) {
|
||||
require.NoError(t, os.WriteFile(victim, []byte("untouched"), 0o644))
|
||||
require.NoError(t, os.WriteFile(filepath.Join(brainDir, "raw", "n.md"), []byte("note"), 0o644))
|
||||
require.NoError(t, os.Symlink(outside, filepath.Join(brainDir, "raw", name)))
|
||||
if name == "failed" {
|
||||
require.NoError(t, os.Symlink(filepath.Join("wiki", "concepts", "v.md"), filepath.Join(brainDir, "log.md")))
|
||||
}
|
||||
_ = processDir(context.Background(), Config{BrainDir: brainDir, Interval: time.Hour, Pipeline: pcfg}, "2026-10-05")
|
||||
got, _ := os.ReadFile(victim)
|
||||
assert.Equal(t, "untouched", string(got), "the log was written through a symlink")
|
||||
assert.Equal(t, "untouched", string(got))
|
||||
ents, _ := os.ReadDir(outside)
|
||||
assert.Empty(t, ents, "the archive copy was written outside the brain")
|
||||
})
|
||||
@@ -64,3 +61,70 @@ func TestProcessDirWritesMarkersAndArchiveInRealDirectories(t *testing.T) {
|
||||
_, err = os.Stat(filepath.Join(brainDir, "raw", "processed", "2026-10-05", "n.md"))
|
||||
assert.NoError(t, err, "positive control: archive copy written")
|
||||
}
|
||||
|
||||
// The log.md entry for a failed note is appended through the brain root: with
|
||||
// a real raw/failed (so the archive copy succeeds and the log is reached), a
|
||||
// symlinked log.md is not written through.
|
||||
func TestProcessDirDoesNotAppendTheLogThroughASymlink(t *testing.T) {
|
||||
brainDir := setupBrainDir(t)
|
||||
victim := filepath.Join(brainDir, "wiki", "concepts", "v.md")
|
||||
require.NoError(t, os.WriteFile(victim, []byte("untouched"), 0o644))
|
||||
require.NoError(t, os.WriteFile(filepath.Join(brainDir, "raw", "n.md"), []byte("note"), 0o644))
|
||||
require.NoError(t, os.Symlink(filepath.Join("wiki", "concepts", "v.md"), filepath.Join(brainDir, "log.md")))
|
||||
_ = processDir(context.Background(), Config{BrainDir: brainDir, Interval: time.Hour, Pipeline: failingPipeline()}, "2026-10-05")
|
||||
_, err := os.Stat(filepath.Join(brainDir, "raw", "failed", "n.md"))
|
||||
require.NoError(t, err, "positive control: the archive copy was written, so the log append was reached")
|
||||
got, _ := os.ReadFile(victim)
|
||||
assert.Equal(t, "untouched", string(got), "the log was appended through a symlink")
|
||||
}
|
||||
|
||||
// A marker path holding anything (here a dangling symlink) counts as a
|
||||
// marker: the note is skipped and nothing is created at the link's target.
|
||||
// (os.Stat followed the link, saw nothing, and processed the note again.)
|
||||
func TestProcessDirTreatsAnyEntryAtTheMarkerPathAsAMarker(t *testing.T) {
|
||||
brainDir := setupBrainDir(t)
|
||||
outside := filepath.Join(t.TempDir(), "created")
|
||||
require.NoError(t, os.WriteFile(filepath.Join(brainDir, "raw", "n.md"), []byte("note"), 0o644))
|
||||
require.NoError(t, os.Symlink(outside, filepath.Join(brainDir, "raw", "n.md.processed")))
|
||||
calls := 0
|
||||
cfg := Config{BrainDir: brainDir, Interval: time.Hour, Pipeline: pipeline.Config{
|
||||
Complete: func(context.Context, string, string) (string, error) { calls++; return "[]", nil }, Schema: "# Schema"}}
|
||||
_ = processDir(context.Background(), cfg, "2026-10-05")
|
||||
assert.Zero(t, calls, "the note was processed despite an entry at its marker path")
|
||||
_, err := os.Lstat(outside)
|
||||
assert.True(t, os.IsNotExist(err), "a file was created at the marker link's target")
|
||||
}
|
||||
|
||||
// A refused archive copy must not retry every interval: the note is
|
||||
// remembered and skipped until it changes (a symlinked raw/failed cost one
|
||||
// LLM call per round before).
|
||||
func TestProcessDirDoesNotRetryANoteWhoseArchiveCopyIsRefused(t *testing.T) {
|
||||
brainDir := setupBrainDir(t)
|
||||
require.NoError(t, os.WriteFile(filepath.Join(brainDir, "raw", "n.md"), []byte("note"), 0o644))
|
||||
require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(brainDir, "raw", "failed")))
|
||||
calls := 0
|
||||
cfg := Config{BrainDir: brainDir, Interval: time.Hour, Pipeline: pipeline.Config{
|
||||
Complete: func(context.Context, string, string) (string, error) { calls++; return "", errors.New("model down") }, Schema: "# Schema"}}
|
||||
for i := 0; i < 3; i++ {
|
||||
_ = processDir(context.Background(), cfg, "2026-10-05")
|
||||
}
|
||||
assert.Equal(t, 1, calls, "the refused note was retried every round")
|
||||
|
||||
// Once the note changes it is tried again.
|
||||
require.NoError(t, os.WriteFile(filepath.Join(brainDir, "raw", "n.md"), []byte("note, edited"), 0o644))
|
||||
future := time.Now().Add(2 * time.Second)
|
||||
require.NoError(t, os.Chtimes(filepath.Join(brainDir, "raw", "n.md"), future, future))
|
||||
_ = processDir(context.Background(), cfg, "2026-10-05")
|
||||
assert.Equal(t, 2, calls, "positive control: a changed note is tried again")
|
||||
}
|
||||
|
||||
// A note whose name has a backslash is legal on Linux and must be processed
|
||||
// (cleanRel refused it, so it was never archived or marked).
|
||||
func TestProcessDirProcessesANoteWithABackslashInItsName(t *testing.T) {
|
||||
brainDir := setupBrainDir(t)
|
||||
require.NoError(t, os.WriteFile(filepath.Join(brainDir, "raw", "a\\b.md"), []byte("note"), 0o644))
|
||||
errs := processDir(context.Background(), Config{BrainDir: brainDir, Interval: time.Hour, Pipeline: okPipeline()}, "2026-10-05")
|
||||
assert.Empty(t, errs)
|
||||
_, err := os.Stat(filepath.Join(brainDir, "raw", "a\\b.md.processed"))
|
||||
assert.NoError(t, err)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user