diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go index d8406e05b..1abb51ce1 100644 --- a/authbridge/authlib/observe/claude/harvest.go +++ b/authbridge/authlib/observe/claude/harvest.go @@ -19,14 +19,54 @@ import ( // tree. Read here so a user who has moved it is not told there are no sessions. const ConfigDirEnv = "CLAUDE_CONFIG_DIR" -// ErrCorruptMetadata reports that an existing metadata file could not be trusted, so a -// merge refused rather than rebuilding from scratch and dropping its entries. +// ErrCorruptMetadata reports that an existing metadata file was there and could not be +// read, so a merge refused rather than replacing a file whose contents are unknown. +// +// NARROWER THAN THE NAME SUGGESTS: a file that reads fine but does not parse no longer +// comes back here, because Harvest rebuilds it instead. What is left is every file Harvest +// refuses because a rebuild might replace good entries — one that could not be read (a +// permission or I/O failure) and one too large to read whole (ErrMetadataTooLarge). The two +// need different advice, so a caller printing a remedy must check the narrower sentinel +// first rather than assuming this one means permissions. // // Exported because the repair is a CLI affordance: `abctl experimental // read-claude-sessions` names --merge=false as the way past, and only the command layer // knows its own flags. Callers discriminate with errors.Is. var ErrCorruptMetadata = errors.New("corrupt session metadata") +// ErrLockTimeout reports that the metadata lock could not be acquired before its deadline, +// so the harvest proceeded UNLOCKED rather than not at all. +// +// Its own error so a caller can tell "this filesystem cannot flock" from "another process +// is holding it": both proceed unlocked, but only the second means a concurrent harvest is +// real and a lost update is possible. +// +// Declared here rather than in lock_unix.go so it is part of the package API on every +// platform — an errors.Is against it must compile where the lock is a no-op too, which is +// the Windows cross-check lock_other.go exists to keep working. +var ErrLockTimeout = errors.New("timed out waiting for the session metadata lock") + +// ErrMetadataTooLarge reports that the metadata file is larger than ReadMetadata's cap, so it +// could not be read whole and was refused rather than rebuilt over. +// +// Exported, unlike errMetadataNotJSON, because the remedy differs and callers must not give the +// wrong one: this file is intact and its permissions are fine, so "fix the permissions" is wrong +// advice. It is also permanent — every launch refuses again — which a caller may want to say +// before a full-screen viewer hides it. +var ErrMetadataTooLarge = errors.New("session metadata file is too large to read") + +// errMetadataNotJSON marks the one read failure Harvest can recover from by rebuilding. +// +// A SENTINEL RATHER THAN A STRING MATCH on ReadMetadata's message, and rather than +// treating every non-permission error as a parse failure: io.ReadAll can fail with EIO +// mid-file, which reads exactly like a truncated document and must not be rebuilt over. +// Classifying on what the error IS, not on what it is not, keeps the destructive branch +// reachable only from the one case that is provably a parse failure. +// +// Unexported: callers outside this package have no decision to make with it, since +// Harvest acts on it before they see anything. +var errMetadataNotJSON = errors.New("metadata is not valid JSON") + // Options selects how much work one harvest does. type Options struct { // ConfigDir is the agent config directory to read. Empty resolves via @@ -47,8 +87,11 @@ type Options struct { // file is a cache with no eviction, and `--merge=false` is the only thing that prunes it. // - Merge:false DROPS every entry this harvest did not see, which includes entries from any // OTHER config dir and any session pruned since. That is what makes it the way to rebuild a - // wrong file, and also why it is not the default: run it with a --dir narrower than the one - // that produced the file and it discards the difference without asking. + // file whose entries are WRONG — a file that parses but says the wrong thing, which nothing + // else can fix — and also why it is not the default: run it with a --dir narrower than the + // one that produced the file and it discards the difference without asking. It is no longer + // needed for a file that does not parse; Harvest rebuilds that one itself, and reports it + // on Result.Rebuilt. Merge bool // Incremental skips transcripts no newer than the entry already recorded for // them, so only recently-touched sessions are parsed. @@ -95,6 +138,31 @@ type Result struct { // grow, so counting it as recovered drove Kept negative and the subcommand printed // "-1 kept from the existing file". Not part of the Total identity below for the same reason. Replaced int + // Rebuilt reports that the existing file did not parse, so this harvest replaced it from + // the transcripts instead of merging into it. + // + // Worth a field because the counts alone cannot show it: a rebuild looks exactly like an + // ordinary first run — everything Harvested, nothing Kept — and the entries it dropped + // (sessions whose transcripts are gone) leave no trace anywhere for a caller to notice. + Rebuilt bool + // LockTimedOut reports that the metadata lock could not be taken before its deadline, so + // this harvest ran unlocked and a concurrent run's rename may erase its entries. + // + // A field because the alternative is silence: without it, the one failure mode this lock + // exists to prevent is invisible from outside, and "some titles vanished" has no diagnosis. + LockTimedOut bool + // LockFailed carries a lock failure that was NOT a timeout — flock refused for some other + // reason, e.g. the lock file could not be created or the filesystem rejected the call. + // + // Separate from LockTimedOut because the two need different words: a timeout means someone + // else holds it, while this means locking did not work at all, and reporting "timed out" for + // an EIO would send the reader looking for a process that does not exist. Empty when locking + // succeeded, was not attempted (no Merge), or is a no-op on this platform. + // + // Same consequence as a timeout, which is why it is reported at all: the harvest proceeds + // UNLOCKED, so a concurrent run's rename can erase everything it wrote. Before this, that + // state was the only one where wholesale loss was possible and nothing recorded it. + LockFailed string // Meta is what the run wrote: the merged whole under Merge, or just this harvest // otherwise. Keyed by session id. // @@ -147,9 +215,21 @@ func Harvest(opts Options) (Result, error) { // Best-effort: a filesystem that cannot flock still harvests, unlocked, on the footing every // platform had before this. Not taken without Merge, where there is nothing to lose — that path // replaces the file by definition. + // The error is recorded, not returned: proceeding unlocked is the deliberate fallback, so a + // caller that wants to report the risk can, and one that does not still harvests. if opts.Merge { - if unlock, lerr := lockMetadata(path); lerr == nil { + unlock, lerr := lockMetadata(path) + switch { + case lerr == nil: defer unlock() + case errors.Is(lerr, ErrLockTimeout): + res.LockTimedOut = true + default: + // ARMED, because the fallback is the same as a timeout's — harvest unlocked — but + // nothing said so. A flock that fails for any other reason left LockTimedOut false + // and no other trace, so the one state where a concurrent rename can erase every + // entry this run wrote was also the one state a caller could not report. + res.LockFailed = lerr.Error() } } @@ -159,16 +239,38 @@ func Harvest(opts Options) (Result, error) { var existing map[string]SessionMetadata if opts.Merge { if existing, err = ReadMetadata(path); err != nil { - // Wrapped in a sentinel rather than returned bare. A corrupt file read as - // absent would silently rebuild from scratch under the flag whose whole - // purpose is not losing entries — the same trap readState exists to close - // for claude-code-state.json. The caller names the repair. - return res, fmt.Errorf("%w: %w", ErrCorruptMetadata, err) + // A FILE THAT DOES NOT PARSE IS REBUILT; a file that could not be READ is + // refused. Refusing both is what this used to do, on the reasoning that a + // rebuild drops entries the flag exists to keep. What that reasoning missed is + // where the entries actually go: the only ones a rebuild loses are those whose + // transcripts are gone, and refusing did not preserve those either — it just + // deferred the choice onto a user who had to know --merge=false to make it, + // while `abctl observe` showed no titles at all until they did. Since every + // launch read the same bad file, that state never cleared itself. + // + // The read failure stays a refusal, and the distinction is the whole safety + // argument: a permission or I/O error says nothing about the contents, so + // replacing the file there would destroy entries that are very likely intact. + if !errors.Is(err, errMetadataNotJSON) { + return res, fmt.Errorf("%w: %w", ErrCorruptMetadata, err) + } + existing = map[string]SessionMetadata{} + res.Rebuilt = true } } var since map[string]SessionMetadata - if opts.Incremental { + if opts.Incremental && !res.Rebuilt { + // NOT INCREMENTAL OVER A REBUILD, though today nothing observable turns on it: the + // rebuild replaced existing with an empty map, so every transcript looks new and the + // scan reads them all either way. NO TEST PINS THIS — a mutation removing the + // !res.Rebuilt term passes the suite, deliberately recorded here rather than guarded + // with a test that would only be asserting the coincidence. + // + // Kept because the equivalence is a property of the line above, not of this one: give + // the rebuild any non-empty baseline — salvaged entries, a defaulted map — and a set + // since starts skipping transcripts the rebuild exists to read. Saying "a rebuild is + // not incremental" directly costs one term and cannot come apart. since = existing } @@ -1728,10 +1830,16 @@ type contentBlock struct { // ReadMetadata reads the existing file, distinguishing absent from unreadable. // // An empty map with a nil error means genuinely no file yet — the first run, which is -// not a problem. A non-nil error means a file was there and could not be trusted, and -// the caller must say so out loud rather than proceeding: under merge, treating a -// corrupt file as empty would discard exactly the entries the flag exists to keep. -// Same distinction, for the same reason, as readState in cmd_claudecode.go. +// not a problem. A non-nil error means a file was there and could not be trusted; what +// the caller should DO about it depends on which error, and this function's job is only +// to keep them apart. Same distinction, for the same reason, as readState in +// cmd_claudecode.go. +// +// Three outcomes a caller can discriminate, because Harvest treats them differently: +// errMetadataNotJSON for a file whose bytes are not JSON, which Harvest rebuilds over; +// ErrMetadataTooLarge for a valid file past the read cap, which it refuses because +// rebuilding would destroy intact entries; and a bare os/io error — permission, EIO — +// which it also refuses, since nothing there says the contents are bad. // // A file holding JSON `null` decodes to a nil map, which is indistinguishable from an // empty object for merging purposes, so it is normalised rather than refused. @@ -1744,24 +1852,44 @@ func ReadMetadata(path string) (map[string]SessionMetadata, error) { return nil, err } defer f.Close() //nolint:errcheck // read-only - // BOUNDED, the same 16 MiB the viewer's own reader of this file uses. Both now cap it, and - // for the same reason: a stray large file at this path would otherwise be read whole and - // decoded before the viewer starts — `abctl observe` calls this synchronously to check the - // file is readable, so an unbounded read stalls startup with nothing on screen to say why. - // Far past any real metadata file: the measured 192-session file is 74 KB. + // BOUNDED at the same 16 MiB the viewer's own reader of this file uses, for the same reason: + // a stray large file at this path would otherwise be read whole and decoded before the viewer + // starts — `abctl observe` calls this synchronously to check the file is readable, so an + // unbounded read stalls startup with nothing on screen to say why. Far past any real metadata + // file: the measured 192-session file is 74 KB. + // + // THE CAP IS SHARED; THE CONTRACT AT IT IS NOT, and the difference is deliberate rather than + // an oversight to be unified. This function has a caller that can act — it returns an error, + // so the CLI and the pre-flight both name a remedy — and refuses loudly with + // ErrMetadataTooLarge. tui.LoadSessionMetadata has nowhere to report anything (it runs while + // the model is built, before tea.NewProgram owns the screen) and so returns an empty map, + // costing the TITLE column. What both must agree on is WHICH files are too large, which is + // why the constant and the read-one-past-it shape are duplicated there rather than eyeballed: + // they once disagreed, and the viewer silently decoded a truncated prefix of a file this one + // refused whole. // - // Truncation surfaces as a JSON error rather than as silent data loss, which is the right - // outcome here: this reader's caller refuses to merge over a file it cannot parse, so a - // file too large to read is treated like any other unreadable one instead of quietly - // becoming a smaller map. + // Truncation is reported as ITS OWN failure, not left to surface as a JSON error. A valid + // file over the cap decodes as a parse failure, and Harvest rebuilds over parse failures — + // which would replace a good 17 MB file with a 400-byte one. So the cap is checked before + // the decode, and the error deliberately carries its own sentinel: not errMetadataNotJSON, + // so it cannot be rebuilt over; not a bare read error either, because nothing is wrong with the + // bytes — it carries ErrMetadataTooLarge, which the CLI and the viewer's pre-flight each + // branch on to give the right remedy. Read one over the cap to tell "exactly at the cap" + // from "larger". const maxMetadataBytes = 16 << 20 - b, err := io.ReadAll(io.LimitReader(f, maxMetadataBytes)) + b, err := io.ReadAll(io.LimitReader(f, maxMetadataBytes+1)) if err != nil { return nil, err } + if len(b) > maxMetadataBytes { + return nil, fmt.Errorf("%w: %s is larger than the %d byte read limit", ErrMetadataTooLarge, path, maxMetadataBytes) + } var m map[string]SessionMetadata if uerr := json.Unmarshal(b, &m); uerr != nil { - return nil, fmt.Errorf("%s is not valid JSON: %w", path, uerr) + // Sentinel-wrapped so Harvest can tell a parse failure from a file it could not + // read: only the former is safe to rebuild over. The message keeps the path and + // the decoder's own detail, both of which reach the user. + return nil, fmt.Errorf("%w: %s is not valid JSON: %w", errMetadataNotJSON, path, uerr) } if m == nil { return map[string]SessionMetadata{}, nil @@ -1834,6 +1962,14 @@ func recoverConcurrentEntries(path string, meta map[string]SessionMetadata) (add return added, replaced, nil } +// writeAll writes body to w. A var, not a call, purely as a test seam. +// +// The bug it guards is the `err :=` shadow in SaveMetadata: a scoped error would be dropped, +// and Close and Rename would then rename a TRUNCATED file over the good one. Nothing a test +// can provoke on a working filesystem — a short write without an error is not something a +// real file does — so the injection point is the only way to reach that path. +var writeAll = func(w io.Writer, body []byte) (int, error) { return w.Write(body) } + // SaveMetadata writes the map atomically, creating ~/.cortex if needed. // // Same mechanics as saveUserConfig, for the same reasons: CreateTemp rather than a @@ -1879,16 +2015,11 @@ func SaveMetadata(path string, meta map[string]SessionMetadata) error { // would then see success, renaming a truncated file over the good one. The bug // saveUserConfig's comment records having made. // - // TRIPWIRE LOST IN THE MOVE, recorded rather than left silent: in package main this was - // `writeAll(f, body)`, an indirected io.Writer.Write that a test could swap for a failing - // one — added because the shadowing bug above is invisible to every test that writes to a - // working filesystem. That var stays in cmd/abctl for saveUserConfig, which is what its - // own test swaps, and it cannot travel here without duplicating it across two modules. No - // test ever reached it through the harvest path (the write-failure test uses an - // unwritable directory instead), so nothing regressed today — but the injection point is - // gone, so a future edit that reintroduces the shadowing has one fewer way to be caught. - // Restoring it is three lines if that ever feels too thin. - _, err = f.Write(body) + // Through writeAll, not f.Write directly, so a test can make the write fail while Close + // and Rename still succeed — the only shape that catches the shadowing above, and one no + // real filesystem produces on demand. cmd/abctl has its own copy of this seam for + // saveUserConfig; the duplication is two lines and buys each module its own tripwire. + _, err = writeAll(f, body) if cerr := f.Close(); err == nil { err = cerr } diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go index 92c911758..3d7d747d1 100644 --- a/authbridge/authlib/observe/claude/harvest_test.go +++ b/authbridge/authlib/observe/claude/harvest_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -222,13 +223,91 @@ func TestHarvest_IncrementalRequiresMerge(t *testing.T) { } } -// A corrupt existing file is refused under Merge, distinguishably, so the command layer -// can name --merge=false as the way past. -func TestHarvest_CorruptMetadataIsDistinguishable(t *testing.T) { +// writeMetadataFile puts raw bytes at the metadata path, creating the directory. +// +// Its own helper because every corrupt-file test needs the same four lines, and the mode +// matters: SaveMetadata writes 0o600, so a fixture that differs would test a file the +// product never produces. +func writeMetadataFile(t *testing.T, body string) string { + t.Helper() + path, err := SessionMetadataPath() + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + return path +} + +// A file that does not parse is rebuilt from the transcripts rather than refused, so the +// titles come back on their own instead of waiting for a hand-run --merge=false. +func TestHarvest_CorruptMetadataIsRebuilt(t *testing.T) { metadataHome(t) cfg := filepath.Join(t.TempDir(), "claude") writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl", `{"type":"ai-title","aiTitle":"t"}`) + path := writeMetadataFile(t, "{not json") + + // Incremental too, which is how `abctl observe` calls it: the rebuild has to clear the + // baseline, or a skip test against an empty map is the only thing making this work. + res, err := Harvest(Options{ConfigDir: cfg, Merge: true, Incremental: true}) + if err != nil { + t.Fatalf("Harvest: %v", err) + } + if !res.Rebuilt { + t.Error("Rebuilt = false, want true: the caller cannot tell a rebuild from a first run") + } + if got := res.Meta["s1"].Title; got != "t" { + t.Errorf("Meta[s1].Title = %q, want the harvested title", got) + } + if got := readMetadataFile(t, path)["s1"].Title; got != "t" { + t.Errorf("on disk Title = %q, want the file replaced with the rebuild", got) + } +} + +// An UNREADABLE file is still refused, and left alone. This is the destructive case the +// rebuild must not reach: a permission or I/O failure says nothing about the contents, so +// replacing the file there would drop entries that are very likely intact. +func TestHarvest_UnreadableMetadataIsRefusedNotRebuilt(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the mode bits this test relies on") + } + metadataHome(t) + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl", + `{"type":"ai-title","aiTitle":"t"}`) + // VALID JSON, so the only thing making this unreadable is the mode. A corrupt body here + // would let the test pass for the wrong reason if the classification were inverted. + path := writeMetadataFile(t, `{"old":{"title":"keep me"}}`) + if err := os.Chmod(path, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(path, 0o600) }) + + res, err := Harvest(Options{ConfigDir: cfg, Merge: true}) + if !errors.Is(err, ErrCorruptMetadata) { + t.Fatalf("err = %v, want it to wrap ErrCorruptMetadata", err) + } + if res.Rebuilt { + t.Error("Rebuilt = true: an unreadable file must not be rebuilt over") + } + if err := os.Chmod(path, 0o600); err != nil { + t.Fatal(err) + } + if got := readMetadataFile(t, path)["old"].Title; got != "keep me" { + t.Errorf("Title = %q, want the untouched entry: the file was replaced", got) + } +} + +// An oversized but VALID file is refused, not rebuilt over. The read cap makes such a file +// fail json.Unmarshal, which would classify it as a parse failure and destroy it: measured at +// 18.5MB/17000 entries replaced by 412 bytes/1 entry. +func TestHarvest_OversizedValidMetadataIsRefusedNotRebuilt(t *testing.T) { + metadataHome(t) path, err := SessionMetadataPath() if err != nil { t.Fatal(err) @@ -236,13 +315,78 @@ func TestHarvest_CorruptMetadataIsDistinguishable(t *testing.T) { if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { t.Fatal(err) } - if err := os.WriteFile(path, []byte("{not json"), 0o600); err != nil { + // Built as text rather than by marshalling a map: cheaper, and it keeps the fixture's size + // the point of the test rather than a side effect of the struct. + var b []byte + b = append(b, '{') + pad := strings.Repeat("x", 4<<10) + for i := 0; len(b) <= 16<<20; i++ { + if i > 0 { + b = append(b, ',') + } + b = append(b, fmt.Sprintf("%q:{\"title\":%q}", fmt.Sprintf("sess-%06d", i), pad)...) + } + b = append(b, '}') + if !json.Valid(b) { + t.Fatal("fixture is not valid JSON, so this would not test the rebuild path") + } + if err := os.WriteFile(path, b, 0o600); err != nil { t.Fatal(err) } - _, err = Harvest(Options{ConfigDir: cfg, Merge: true}) + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "fresh.jsonl", + `{"type":"ai-title","aiTitle":"new"}`) + + res, err := Harvest(Options{ConfigDir: cfg, Merge: true}) + if err == nil { + t.Fatalf("Harvest succeeded (Rebuilt=%v); want it refused rather than rebuilding", res.Rebuilt) + } if !errors.Is(err, ErrCorruptMetadata) { - t.Fatalf("err = %v, want it to wrap ErrCorruptMetadata", err) + t.Errorf("err = %v, want it to wrap ErrCorruptMetadata", err) + } + // Distinguishable through both wraps, so a caller can give the right remedy: this file's + // permissions are fine and "fix the permissions" would be wrong advice. + if !errors.Is(err, ErrMetadataTooLarge) { + t.Errorf("err = %v, want it to wrap ErrMetadataTooLarge", err) + } + if res.Rebuilt { + t.Error("Rebuilt is true for a file that was merely too large to read") + } + // The assertion that matters: the bytes are still there. + st, serr := os.Stat(path) + if serr != nil { + t.Fatal(serr) + } + if got := st.Size(); got != int64(len(b)) { + t.Errorf("file is %d bytes, was %d — it was rewritten", got, len(b)) + } +} + +// A rebuild is still a merge otherwise: it replaces the unparseable file, and the next +// harvest keeps what it wrote. +func TestHarvest_RebuildThenMergeKeepsEntries(t *testing.T) { + metadataHome(t) + cfg := filepath.Join(t.TempDir(), "claude") + dir := filepath.Join(cfg, "projects", "-p") + writeSessionTranscript(t, dir, "s1.jsonl", `{"type":"ai-title","aiTitle":"one"}`) + writeMetadataFile(t, "{not json") + + if _, err := Harvest(Options{ConfigDir: cfg, Merge: true}); err != nil { + t.Fatalf("rebuild: %v", err) + } + writeSessionTranscript(t, dir, "s2.jsonl", `{"type":"ai-title","aiTitle":"two"}`) + res, err := Harvest(Options{ConfigDir: cfg, Merge: true}) + if err != nil { + t.Fatalf("second harvest: %v", err) + } + if res.Rebuilt { + t.Error("Rebuilt = true on a file this package just wrote") + } + for id, want := range map[string]string{"s1": "one", "s2": "two"} { + if got := res.Meta[id].Title; got != want { + t.Errorf("Meta[%s].Title = %q, want %q", id, got, want) + } } } @@ -2709,3 +2853,42 @@ func TestTitleFromTranscript_CwdIsBounded(t *testing.T) { got[max(0, len(got)-20):], "/the-leaf") } } + +// A short write must not be renamed over the good file. This is the seam writeAll exists for: +// the failure is a write error that Close and Rename both survive, which no real filesystem +// produces, and the bug it guards — `err :=` shadowing inside SaveMetadata — is invisible to +// every test that writes to a working disk. +func TestSaveMetadata_AFailedWriteKeepsTheOldFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "session-metadata.json") + const keep = `{"old":{"title":"keep me"}}` + if err := os.WriteFile(path, []byte(keep), 0o600); err != nil { + t.Fatal(err) + } + + was := writeAll + writeAll = func(io.Writer, []byte) (int, error) { return 0, errors.New("disk on fire") } + t.Cleanup(func() { writeAll = was }) + + if err := SaveMetadata(path, map[string]SessionMetadata{"new": {Title: "t"}}); err == nil { + t.Fatal("SaveMetadata returned nil after the write failed") + } + // The point of the test: the old file, not a truncated new one. + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(b) != keep { + t.Errorf("file = %q, want the original %q", b, keep) + } + // And no temp file left behind to accumulate one per failure. + ents, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, e := range ents { + if strings.HasSuffix(e.Name(), ".tmp") { + t.Errorf("temp file %s was left behind", e.Name()) + } + } +} diff --git a/authbridge/authlib/observe/claude/lock_unix.go b/authbridge/authlib/observe/claude/lock_unix.go index 8ad314e08..7c6edbf93 100644 --- a/authbridge/authlib/observe/claude/lock_unix.go +++ b/authbridge/authlib/observe/claude/lock_unix.go @@ -3,11 +3,52 @@ package claude import ( + "errors" + "fmt" "os" "path/filepath" "syscall" + "time" ) +// lockTimeout bounds how long a harvest waits for the metadata lock before proceeding +// without it. +// +// THE TWO COSTS ARE NOT SYMMETRIC, which is what sets the size. Too long only delays a +// harvest that was going to be wrong anyway — the wedged case this bound exists for, where +// no wait of any length succeeds. Too short corrupts a HEALTHY run: expiry means proceeding +// unlocked, and an unlocked harvest can have its whole contribution erased by another run's +// rename. So the deadline has to sit far above the slowest legitimate wait, and the penalty +// for overshooting is measured in seconds of a background goroutine nobody is watching. +// +// SIZED AGAINST A QUEUE, not a single scan, because the wait is the queue ahead of you and +// not the holder alone. That was the sizing error in the first version of this: 2s was chosen +// as "many times one scan" from a ~3ms one-file measurement, and it fell over as soon as +// several harvests contended on a slow machine. Six concurrent harvests of a padded tree took +// ~190ms serialized on a developer laptop and blew straight past 2s on a shared CI runner — +// the package's own concurrency test failed, with five of six children losing every entry. +// A runner is not an exotic environment; it is the slowest machine this code routinely runs on +// and therefore the one that sets the number. +// +// A VAR RATHER THAN A CONST only so the tests can shrink it: waiting the real deadline twice +// would add a minute to the package for no extra coverage, and a test that asserts the logic at +// 40ms asserts exactly the same logic. Nothing outside the tests assigns it. +// +// 30s is deliberately far past any queue this file can produce. `abctl observe` harvests one +// tree per tick, `read-claude-sessions` is one process, and the realistic worst case is a +// handful of viewers plus a manual run — a queue of seconds, not minutes, even derated for a +// loaded runner. What 30s buys is that reaching it means no wait would have worked. +var lockTimeout = 30 * time.Second + +// lockPoll is how often acquisition is retried inside lockTimeout. +// +// Polled rather than blocking because the two are exclusive in this API: syscall.Flock +// either blocks forever (LOCK_EX) or returns at once (LOCK_NB), and there is no +// deadline variant. 20ms adds at most 20ms to an uncontended handoff, and costs 1500 +// cheap syscalls across a full 30s timeout — paid only by a harvest that is already +// losing, since a lock that frees up is acquired on the next tick. +const lockPoll = 20 * time.Millisecond + // lockMetadata takes an exclusive advisory lock covering the read-modify-write of the metadata // file, and returns the release. // @@ -24,13 +65,33 @@ import ( // replaces the metadata file itself. Locking the metadata file would lock an inode the rename is // about to detach, protecting nothing. // -// Blocking, with no timeout. A harvest holds this for the length of one scan, and the failure mode -// of a timeout here is the lost update this exists to prevent; a caller that cannot wait should not -// be harvesting. A crashed holder releases on process exit, since the kernel owns the lock. +// BOUNDED BY lockTimeout, then it gives up and lets the caller proceed unlocked. This used to +// block indefinitely, on the reasoning that a caller who cannot wait should not be harvesting. +// The case that reasoning did not cover is a holder that never releases — not a crash, which the +// kernel cleans up on process exit, but a process still alive and stuck. `abctl observe` harvests +// on a timer, so every later attempt queued behind the same lock and the viewer showed no titles +// at all, indefinitely, with nothing on screen to say why. +// +// What the bound costs, and why the deadline is generous rather than tight: past it two harvests +// interleave, and the loser's entries can be erased wholesale by the winner's rename. That is the +// very lost update this lock exists to prevent, so expiry must mean "no wait would have worked" +// and never "this machine is slow today". recoverConcurrentEntries is a weaker backstop than it +// looks — a SINGLE re-read after saving, which by its own documentation cannot converge when more +// than one rename lands inside its window, i.e. exactly the many-writer case a short deadline +// creates. It does not cover for a deadline that fires under ordinary contention. +// +// So: unlocked is strictly better than wedged, and strictly worse than locked — which makes the +// bound worth having and worth sizing so that only the wedged case ever reaches it. See +// lockTimeout for the measurements that set the number. // // Errors are returned rather than swallowed so the caller can proceed UNLOCKED instead of refusing // to harvest: a filesystem that cannot flock (some network mounts) should still get titles, on the -// same best-effort footing as before this existed. +// same best-effort footing as before this existed. A timeout joins that path, reported as +// ErrLockTimeout so the two are distinguishable. +// +// The release func is nil on every error return, which the caller relies on to decide whether to +// defer it — see Harvest's `if lerr == nil` gate. Returning a no-op func alongside an error would +// make that gate look optional, and it is not. func lockMetadata(path string) (func(), error) { lockPath := filepath.Join(filepath.Dir(path), "."+filepath.Base(path)+".lock") if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil { @@ -40,9 +101,33 @@ func lockMetadata(path string) (func(), error) { if err != nil { return nil, err } - if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { - _ = f.Close() - return nil, err + // LOCK_NB polled to a deadline. EWOULDBLOCK (EAGAIN on Linux, where they are the same errno) + // is the one retryable answer: it means held, not broken. Every other errno is a real failure + // and returns at once, exactly as the blocking call used to. + // + // UNTESTED BRANCH, said out loud rather than left as a silent hole: no test distinguishes this + // discrimination from "retry on every errno", because provoking a non-EWOULDBLOCK flock error + // needs a filesystem that refuses flock outright (some network mounts) — not something a unit + // test can conjure. A mutation that retries every errno passes the whole suite. What it would + // cost in production is the timeout spent sleeping against an error that will never change, + // then reported as a lock timeout rather than as the ENOLCK it was. + deadline := time.Now().Add(lockTimeout) + for { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + break + } + if !errors.Is(err, syscall.EWOULDBLOCK) { + _ = f.Close() + return nil, err + } + if !time.Now().Before(deadline) { + // Closed here too. The caller proceeds unlocked and never sees this descriptor, so + // leaking it would leak one per harvest — and `abctl observe` harvests on a timer. + _ = f.Close() + return nil, fmt.Errorf("%w after %s", ErrLockTimeout, lockTimeout) + } + time.Sleep(lockPoll) } return func() { // Unlock before close, though closing the descriptor would release it anyway: being diff --git a/authbridge/authlib/observe/claude/lock_unix_test.go b/authbridge/authlib/observe/claude/lock_unix_test.go new file mode 100644 index 000000000..7f666d39d --- /dev/null +++ b/authbridge/authlib/observe/claude/lock_unix_test.go @@ -0,0 +1,231 @@ +//go:build unix + +package claude + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +// metadataLockPath is where lockMetadata puts its sibling lock file. +// +// Duplicated from lock_unix.go on purpose: a test that derived the path by calling the +// product code could not catch the path moving, which is the thing that would silently +// stop the lock from serialising anything. +func metadataLockPath(t *testing.T, path string) string { + t.Helper() + return filepath.Join(filepath.Dir(path), "."+filepath.Base(path)+".lock") +} + +// holdMetadataLock takes the lock from the test process and releases it on cleanup. +// +// Flock is per-open-file-description, not per-process, so a second descriptor here does +// contend with the product's — which is what makes an in-process test of this possible at +// all, without the subprocess machinery TestHarvest_ConcurrentRunsLoseNothing needs. +func holdMetadataLock(t *testing.T, path string) { + t.Helper() + lockPath := metadataLockPath(t, path) + if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil { + t.Fatal(err) + } + f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatal(err) + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + _ = f.Close() + t.Fatalf("could not take the lock the test depends on holding: %v", err) + } + t.Cleanup(func() { + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + _ = f.Close() + }) +} + +// shortenLockTimeout shrinks the deadline for one test and restores it after. +// +// The deadline's production VALUE is a judgement about slow machines (see lockTimeout); what +// these tests check is the behaviour at it — that expiry happens, is reported, and returns no +// unlock func. That behaviour is identical at 40ms, and waiting the real 30s twice would add a +// minute to the package to re-learn nothing. +func shortenLockTimeout(t *testing.T) { + t.Helper() + was := lockTimeout + lockTimeout = 40 * time.Millisecond + t.Cleanup(func() { lockTimeout = was }) +} + +// A held lock is waited for, then given up on — not waited for forever. This is the +// wedged-holder case: before the timeout, `abctl observe` queued every harvest behind a +// lock nobody would release and showed no titles at all, indefinitely. +func TestLockMetadata_TimesOutAndReportsIt(t *testing.T) { + shortenLockTimeout(t) + path := filepath.Join(t.TempDir(), "session-metadata.json") + holdMetadataLock(t, path) + + start := time.Now() + unlock, err := lockMetadata(path) + elapsed := time.Since(start) + + if !errors.Is(err, ErrLockTimeout) { + if unlock != nil { + unlock() + } + t.Fatalf("err = %v, want it to wrap ErrLockTimeout", err) + } + // Nil, not a no-op: Harvest's `if lerr == nil` gate is what keeps this from being + // called, and a non-nil func here would make that gate look optional. + if unlock != nil { + t.Error("unlock is non-nil alongside an error") + } + if elapsed < lockTimeout { + t.Errorf("gave up after %s, before the %s deadline", elapsed, lockTimeout) + } + // Generous: this asserts it is bounded at all, not that the sleep is precise. + if elapsed > 4*lockTimeout { + t.Errorf("took %s, far past the %s deadline", elapsed, lockTimeout) + } +} + +// An uncontended harvest does not claim a timeout. Guards the field against being set +// unconditionally, which would make it useless as a signal. +func TestHarvest_UncontendedDoesNotReportALockTimeout(t *testing.T) { + metadataHome(t) + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl", + `{"type":"ai-title","aiTitle":"t"}`) + + res, err := Harvest(Options{ConfigDir: cfg, Merge: true}) + if err != nil { + t.Fatalf("Harvest: %v", err) + } + if res.LockTimedOut { + t.Error("LockTimedOut is true with nothing holding the lock") + } +} + +// An uncontended lock is still taken, and released, at once. +func TestLockMetadata_UncontendedIsFast(t *testing.T) { + path := filepath.Join(t.TempDir(), "session-metadata.json") + + start := time.Now() + unlock, err := lockMetadata(path) + if err != nil { + t.Fatalf("lockMetadata: %v", err) + } + if unlock == nil { + t.Fatal("unlock is nil with no error") + } + if elapsed := time.Since(start); elapsed >= lockTimeout { + t.Errorf("uncontended acquisition took %s, want well under %s", elapsed, lockTimeout) + } + unlock() + + // Released, so it can be taken again. Without this the test would pass against a + // release that does nothing. + again, err := lockMetadata(path) + if err != nil { + t.Fatalf("second lockMetadata after release: %v", err) + } + again() +} + +// THE POINT OF THE TIMEOUT, at the level the bug was reported at: a wedged holder no +// longer stops the titles from appearing. The harvest proceeds unlocked and writes. +func TestHarvest_ProceedsWhenTheLockIsHeld(t *testing.T) { + shortenLockTimeout(t) + metadataHome(t) + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl", + `{"type":"ai-title","aiTitle":"t"}`) + path, err := SessionMetadataPath() + if err != nil { + t.Fatal(err) + } + holdMetadataLock(t, path) + + res, err := Harvest(Options{ConfigDir: cfg, Merge: true}) + if err != nil { + t.Fatalf("Harvest: %v", err) + } + if got := res.Meta["s1"].Title; got != "t" { + t.Errorf("Meta[s1].Title = %q, want the harvest to have run unlocked", got) + } + if got := readMetadataFile(t, path)["s1"].Title; got != "t" { + t.Errorf("on disk Title = %q, want the harvest to have written", got) + } + // Reported, not silent: an unlocked harvest can lose its entries to a concurrent rename, and + // without this the only failure the lock exists to prevent has no diagnosis. + if !res.LockTimedOut { + t.Error("LockTimedOut is false after the harvest ran unlocked") + } +} + +// A flock failure that is NOT a timeout is recorded too, and the harvest still runs. +// +// The default: arm this covers was missing: a lock that failed for any reason other than the +// deadline left LockTimedOut false and nothing else set, so the one state where a concurrent +// rename can erase every entry a run wrote was also the one state a caller could not report. +// +// Provoked through a metadata directory that cannot be written, which makes the lock FILE +// uncreatable — a real case (read-only home, restrictive mode) and the only non-timeout lock +// failure a unit test can produce without a filesystem that refuses flock outright. +func TestHarvest_NonTimeoutLockFailureIsRecorded(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the directory mode this test relies on") + } + metadataHome(t) + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl", + `{"type":"ai-title","aiTitle":"t"}`) + path, err := SessionMetadataPath() + if err != nil { + t.Fatal(err) + } + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + // 0o500: readable and traversable, so the harvest can still try, but no new file can be + // created in it — which is what the lock needs. + if err := os.Chmod(dir, 0o500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) + + res, _ := Harvest(Options{ConfigDir: cfg, Merge: true}) + // The save fails too — the directory is unwritable, that is the point — so the error is not + // asserted. What must hold is that the lock failure was RECORDED rather than swallowed. + if res.LockFailed == "" { + t.Error("LockFailed is empty after the lock could not be created") + } + // Not mislabelled as a timeout: nobody was holding it, and saying so would send a reader + // looking for a process that does not exist. + if res.LockTimedOut { + t.Error("LockTimedOut is true for a failure that was not a timeout") + } +} + +// The mirror: a lock that works reports neither field. Without this the pair above would pass +// against code that set LockFailed unconditionally. +func TestHarvest_SuccessfulLockReportsNeitherFailure(t *testing.T) { + metadataHome(t) + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl", + `{"type":"ai-title","aiTitle":"t"}`) + + res, err := Harvest(Options{ConfigDir: cfg, Merge: true}) + if err != nil { + t.Fatalf("Harvest: %v", err) + } + if res.LockFailed != "" { + t.Errorf("LockFailed = %q with a working lock", res.LockFailed) + } + if res.LockTimedOut { + t.Error("LockTimedOut is true with a working lock") + } +} diff --git a/authbridge/cmd/abctl/README.md b/authbridge/cmd/abctl/README.md index 892acb96f..26d00d4f9 100644 --- a/authbridge/cmd/abctl/README.md +++ b/authbridge/cmd/abctl/README.md @@ -153,10 +153,12 @@ simply not be touched. It suppresses only the *scan*: the viewer still reads only sessions new or renamed since the last scan show as bare ids. There is no flag that hides titles already on disk — delete the file for that. -A harvest that cannot run is never fatal — a missing, unreadable or corrupt file -costs the `TITLE` column and nothing else, and the viewer still opens. The failures -that can be known before the viewer starts, such as an unreadable metadata file, print -one line to stderr with the repair; success says nothing. +A harvest that cannot run is never fatal — the worst a missing or unreadable file +costs is the `TITLE` column, and the viewer still opens. A file that does not parse is +rebuilt from the transcripts rather than costing anything; the entries a rebuild cannot +recover are sessions whose transcripts Claude Code has already pruned. The failures that +need a human, such as an unreadable metadata file, print one line to stderr with the +repair before the viewer starts; success says nothing. The config directory is `CLAUDE_CONFIG_DIR` when set, and `~/.claude` otherwise. To read a different directory, or to force a full re-read of every diff --git a/authbridge/cmd/abctl/cmd_experimental.go b/authbridge/cmd/abctl/cmd_experimental.go index ea0c36ff6..14f65b435 100644 --- a/authbridge/cmd/abctl/cmd_experimental.go +++ b/authbridge/cmd/abctl/cmd_experimental.go @@ -144,13 +144,28 @@ func runReadClaudeSessions(args []string, stdout, stderr io.Writer) int { // `abctl observe` is the incremental caller. res, err := claude.Harvest(claude.Options{ConfigDir: *dir, Merge: *merge}) if err != nil { + // Two refusals, two remedies. Checked narrowest first: the oversized file also wraps + // ErrCorruptMetadata, and its permissions are fine, so the permission advice is wrong + // for it. + // + // Both DO offer --merge=false, because it genuinely repairs both: Harvest reads the + // file only under Merge, and the save renames over the path, needing the directory + // writable rather than the old file readable. Verified on an unreadable file and on a + // 17.7 MB one. It is also the gentler of the two remedies — it leaves no .bad file to + // clean up — so it goes first, with mv kept for whoever wants the old bytes preserved. + if errors.Is(err, claude.ErrMetadataTooLarge) { + // The path is already in the error text, so it is not repeated here. + fmt.Fprintf(stderr, "abctl: %v\n"+ + " Re-run with --merge=false to rebuild it, or move the file aside first\n"+ + " to keep the old entries.\n", err) + return 1 + } if errors.Is(err, claude.ErrCorruptMetadata) { - // Refused rather than treated as empty. A corrupt file read as absent would - // silently rebuild from scratch under the flag whose whole purpose is not - // losing entries — the same trap readState exists to close for - // claude-code-state.json. Name the repair, and name the way past it. + // A file that does not parse is rebuilt by Harvest itself, so what reaches here + // could not be read at all. Name what a human can do. fmt.Fprintf(stderr, "abctl: %v\n"+ - " Fix or move the file, or re-run with --merge=false to rebuild it.\n", err) + " Re-run with --merge=false to rebuild it, or fix the file's permissions\n"+ + " to keep the old entries.\n", err) return 1 } fmt.Fprintf(stderr, "abctl: %v\n", err) @@ -175,6 +190,26 @@ func runReadClaudeSessions(args []string, stdout, stderr io.Writer) int { fmt.Fprintf(stderr, "abctl: merged %d entry(s) written concurrently by another run\n", res.Recovered) } + // Said out loud because the entries it may have lost are gone without a trace: an unlocked + // harvest can have its whole contribution erased by a concurrent run's rename. + if res.LockTimedOut { + fmt.Fprintf(stderr, "abctl: timed out waiting for the lock on %s; harvested anyway, so a concurrent run may have overwritten this one\n", res.Path) + } + + // Same risk, different cause, so different words: a timeout means another run holds the lock, + // this means locking did not work at all. Reported for the same reason — the harvest ran + // unlocked either way. + if res.LockFailed != "" { + fmt.Fprintf(stderr, "abctl: could not lock %s (%s); harvested anyway, so a concurrent run may have overwritten this one\n", res.Path, res.LockFailed) + } + + // Said out loud because the counts cannot show it: a rebuild reports everything harvested + // and nothing kept, which is exactly what a first run reports. The entries it dropped — + // sessions whose transcripts are gone — leave no trace for the operator to notice. + if res.Rebuilt { + fmt.Fprintf(stderr, "abctl: the existing file could not be parsed; rebuilt it from %s\n", res.ConfigDir) + } + // Reported rather than silent: the count is the only way to notice that a wrong // --dir found nothing, and zero is not an error — a machine that has never run // Claude Code legitimately has no transcripts. diff --git a/authbridge/cmd/abctl/cmd_experimental_test.go b/authbridge/cmd/abctl/cmd_experimental_test.go index 04914b346..e0a7671f3 100644 --- a/authbridge/cmd/abctl/cmd_experimental_test.go +++ b/authbridge/cmd/abctl/cmd_experimental_test.go @@ -3,6 +3,7 @@ package main import ( "bytes" "encoding/json" + "fmt" "os" "path/filepath" "runtime" @@ -30,6 +31,40 @@ func writeSessionTranscript(t *testing.T, dir, name string, lines ...string) { } } +// oversizedMetadata builds a VALID metadata file just over Harvest's 16 MiB read cap. +// +// Raw text rather than marshalling a map: 4000-odd entries through encoding/json took seconds, +// and what the test needs is the size and the validity, not realistic content. +func oversizedMetadata(t *testing.T) []byte { + t.Helper() + var b []byte + b = append(b, '{') + pad := strings.Repeat("x", 4<<10) + for i := 0; len(b) <= 16<<20; i++ { + if i > 0 { + b = append(b, ',') + } + b = append(b, fmt.Sprintf("%q:{\"title\":%q}", fmt.Sprintf("sess-%06d", i), pad)...) + } + b = append(b, '}') + // Asserted, because an INVALID oversized file would exercise the rebuild path instead and + // the test would pass for the wrong reason. + if !json.Valid(b) { + t.Fatal("fixture is not valid JSON, so this would not test the refusal") + } + return b +} + +// fileSize reports the file's size, for before/after comparison. +func fileSize(t *testing.T, path string) int64 { + t.Helper() + st, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s: %v", path, err) + } + return st.Size() +} + // readMetadataFile decodes what the command wrote. func readMetadataFile(t *testing.T, path string) map[string]tui.SessionMetadata { t.Helper() @@ -381,10 +416,9 @@ func TestReadClaudeSessions_MergePrefersTheFreshHarvest(t *testing.T) { } } -// A corrupt existing file must not be read as empty under --merge: that would rebuild -// from scratch under the flag whose purpose is not losing entries. Refuse, and name -// the way past it. -func TestReadClaudeSessions_MergeRefusesACorruptFile(t *testing.T) { +// A file that does not parse is rebuilt in place, without --merge=false and without the +// operator having to know that flag exists. Reported, because the counts cannot show it. +func TestReadClaudeSessions_MergeRebuildsACorruptFile(t *testing.T) { home := prefsHome(t) path := filepath.Join(home, tui.SessionMetadataRel) if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { @@ -394,6 +428,42 @@ func TestReadClaudeSessions_MergeRefusesACorruptFile(t *testing.T) { t.Fatal(err) } + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "fresh.jsonl", + `{"type":"user","cwd":"/w"}`) + + var out, errb bytes.Buffer + if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg}, &out, &errb); code != 0 { + t.Fatalf("exit = %d, want 0: %s", code, errb.String()) + } + if !strings.Contains(errb.String(), "rebuilt it") { + t.Errorf("stderr does not report the rebuild: %q", errb.String()) + } + if _, ok := readMetadataFile(t, path)["fresh"]; !ok { + t.Error("the corrupt file was not rebuilt") + } +} + +// A file that could not be READ is still refused, and left alone. The rebuild must not +// reach this case: a permission failure says nothing about the contents. +func TestReadClaudeSessions_MergeRefusesAnUnreadableFile(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the mode bits this test relies on") + } + home := prefsHome(t) + path := filepath.Join(home, tui.SessionMetadataRel) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + // Valid JSON, so the mode is the only thing making it unreadable. + if err := os.WriteFile(path, []byte(`{"old":{"title":"keep me"}}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(path, 0o600) }) + cfg := filepath.Join(t.TempDir(), "claude") writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "fresh.jsonl", `{"type":"user","cwd":"/w"}`) @@ -402,22 +472,94 @@ func TestReadClaudeSessions_MergeRefusesACorruptFile(t *testing.T) { if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg}, &out, &errb); code != 1 { t.Errorf("exit = %d, want 1", code) } - if !strings.Contains(errb.String(), "--merge=false") { - t.Errorf("stderr does not name the way past it: %q", errb.String()) + // Asserted before the mode is restored, so the message is the one a user with a genuinely + // unreadable file sees. + if got := errb.String(); !strings.Contains(got, "--merge=false") { + t.Errorf("stderr does not name the remedy that repairs this:\n%s", got) + } + if err := os.Chmod(path, 0o600); err != nil { + t.Fatal(err) } - // The bad file is left alone rather than overwritten, so it can still be repaired. - b, err := os.ReadFile(path) - if err != nil || string(b) != "{not json" { - t.Errorf("the corrupt file was modified: %q, %v", b, err) + if got := readMetadataFile(t, path)["old"].Title; got != "keep me" { + t.Errorf("Title = %q, want the untouched entry: the file was replaced", got) } +} - // --merge=false is that way past it. - var out2, errb2 bytes.Buffer - if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg, "--merge=false"}, &out2, &errb2); code != 0 { - t.Fatalf("--merge=false exit = %d: %s", code, errb2.String()) +// AND THE REMEDY THE REFUSAL NAMES ACTUALLY WORKS. Without this, the advice in the message +// above is only asserted to be PRESENT, not to be true — which is how it came to be deleted +// as impossible in the first place: --merge=false skips the read entirely (Harvest reads the +// file only under Merge) and the save renames over the path, so the old file never has to be +// readable. Same fixture as the refusal, one flag different. +func TestReadClaudeSessions_MergeFalseRepairsAnUnreadableFile(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the mode bits this test relies on") } - if _, ok := readMetadataFile(t, path)["fresh"]; !ok { - t.Error("--merge=false did not rebuild the file") + home := prefsHome(t) + path := filepath.Join(home, tui.SessionMetadataRel) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(`{"old":{"title":"keep me"}}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(path, 0o600) }) + + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "fresh.jsonl", + `{"type":"ai-title","aiTitle":"new"}`) + + var out, errb bytes.Buffer + if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg, "--merge=false"}, &out, &errb); code != 0 { + t.Fatalf("exit = %d, want 0; stderr:\n%s", code, errb.String()) + } + if got := readMetadataFile(t, path)["fresh"].Title; got != "new" { + t.Errorf("Meta[fresh].Title = %q, want the rebuild to have written it", got) + } + // The old entry is gone, which is what --merge=false MEANS. Asserted so the test cannot be + // read as claiming the unreadable entries were somehow recovered. + if _, ok := readMetadataFile(t, path)["old"]; ok { + t.Error("the old entry survived a --merge=false rebuild") + } +} + +// An oversized but valid file is refused with its OWN remedy — not the permission advice, which +// would be wrong: the file is intact and readable, just over the read cap. +func TestReadClaudeSessions_OversizedFileNamesTheRightRepair(t *testing.T) { + home := prefsHome(t) + path := filepath.Join(home, tui.SessionMetadataRel) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, oversizedMetadata(t), 0o600); err != nil { + t.Fatal(err) + } + size := fileSize(t, path) + + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "fresh.jsonl", + `{"type":"user","cwd":"/w"}`) + + var out, errb bytes.Buffer + if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg}, &out, &errb); code != 1 { + t.Errorf("exit = %d, want 1", code) + } + if got := errb.String(); !strings.Contains(got, "too large") { + t.Errorf("stderr does not say what is wrong:\n%s", got) + } + if got := errb.String(); strings.Contains(got, "permissions") { + t.Errorf("stderr gives the permission remedy for an intact file:\n%s", got) + } + // The remedy that actually works, asserted because it was once removed on the theory that + // --merge=false "would hit the same read". It does not: Harvest reads the file only under + // Merge, and the save renames over the path. Measured against a 17.7 MB file. + if got := errb.String(); !strings.Contains(got, "--merge=false") { + t.Errorf("stderr does not name the remedy that repairs this without deleting anything:\n%s", got) + } + if got := fileSize(t, path); got != size { + t.Errorf("file is %d bytes, was %d — it was rewritten", got, size) } } diff --git a/authbridge/cmd/abctl/cmd_observe_test.go b/authbridge/cmd/abctl/cmd_observe_test.go index 2e7bc071e..ac9faa796 100644 --- a/authbridge/cmd/abctl/cmd_observe_test.go +++ b/authbridge/cmd/abctl/cmd_observe_test.go @@ -319,16 +319,13 @@ func TestClaudeHarvester_SurvivesAnUnreadableConfigDir(t *testing.T) { } } -// A corrupt metadata file names the repair, and disables the harvest rather than running it. +// A file that does not parse no longer disables the harvest: Harvest rebuilds it, and the +// titles arrive on their own. // -// Checked BEFORE the TUI starts precisely so the message can be printed at all: the harvest now -// runs after the alt screen goes up, where a warning would corrupt the frame. It is also the one -// failure that cannot clear itself, since every launch reads the same bad file. observe has to -// name a different repair from the subcommand's --merge=false, which is not a flag it has. -// -// (This test existed before the async change and was lost in an edit; restored here in the -// factory's shape, which is also where it belongs now that the check moved.) -func TestClaudeHarvester_CorruptFileNamesTheRepair(t *testing.T) { +// This is the reported bug at the level the user sees it. The pre-flight used to return nil +// here, so `abctl observe` showed no titles at all for the whole run, and every later launch +// read the same bad file — the state never cleared itself without a hand-run subcommand. +func TestClaudeHarvester_CorruptFileIsRebuiltNotFatal(t *testing.T) { home := prefsHome(t) cfg := filepath.Join(t.TempDir(), "claude") writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl", @@ -343,22 +340,97 @@ func TestClaudeHarvester_CorruptFileNamesTheRepair(t *testing.T) { t.Fatal(err) } + var warn bytes.Buffer + h := claudeHarvester(&warn) + if h == nil { + t.Fatal("a file that does not parse must not disable the harvest: Harvest rebuilds it") + } + // Asserted through the harvester rather than the file, because a harvester that runs but + // yields nothing would leave the TITLE column exactly as empty as no harvester at all. + got, err := h() + if err != nil { + t.Fatalf("harvest: %v", err) + } + if got["s1"].Title != "t" { + t.Errorf("Title = %q, want %q: the rebuild did not reach the transcripts", got["s1"].Title, "t") + } +} + +// A file that cannot be READ still disables the harvest, and still names a repair. Harvest +// refuses that one — a permission failure says nothing about the contents — so the pre-flight +// is the only place it can be said before the alt screen goes up. +func TestClaudeHarvester_UnreadableFileNamesTheRepair(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the mode bits this test relies on") + } + home := prefsHome(t) + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl", + `{"type":"ai-title","aiTitle":"t"}`) + t.Setenv("CLAUDE_CONFIG_DIR", cfg) + + path := filepath.Join(home, tui.SessionMetadataRel) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(`{"old":{"title":"keep me"}}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(path, 0o600) }) + var warn bytes.Buffer if h := claudeHarvester(&warn); h != nil { - t.Error("a corrupt file must disable the harvest, not be silently merged over") + t.Error("an unreadable file must disable the harvest, not be rebuilt over") } got := warn.String() - if !strings.Contains(got, "read-claude-sessions --merge=false") { + if !strings.Contains(got, "mv ") { t.Errorf("the warning does not name the repair:\n%s", got) } // The command it names must be runnable as printed, on its own line. for _, line := range strings.Split(got, "\n") { - if strings.Contains(line, "abctl experimental") && !strings.HasPrefix(strings.TrimSpace(line), "abctl experimental") { + if strings.Contains(line, "mv ") && !strings.HasPrefix(strings.TrimSpace(line), "mv ") { t.Errorf("the repair command is not on a line of its own: %q", line) } } } +// An oversized-but-valid file also disables the harvest — Harvest refuses it, so running the +// harvester anyway would fail on every launch with nothing said before the alt screen. The +// remedy printed must be its own, not the permission advice: this file is readable and intact. +func TestClaudeHarvester_OversizedFileNamesItsOwnRepair(t *testing.T) { + home := prefsHome(t) + cfg := filepath.Join(t.TempDir(), "claude") + writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl", + `{"type":"ai-title","aiTitle":"t"}`) + t.Setenv("CLAUDE_CONFIG_DIR", cfg) + + path := filepath.Join(home, tui.SessionMetadataRel) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, oversizedMetadata(t), 0o600); err != nil { + t.Fatal(err) + } + + var warn bytes.Buffer + if h := claudeHarvester(&warn); h != nil { + t.Error("a file Harvest refuses must disable the harvest rather than fail every launch") + } + got := warn.String() + if !strings.Contains(got, "too large") { + t.Errorf("the warning does not say what is wrong:\n%s", got) + } + if strings.Contains(got, "permission") { + t.Errorf("the warning gives the permission remedy for an intact file:\n%s", got) + } + if !strings.Contains(got, "mv ") { + t.Errorf("the warning does not name the repair:\n%s", got) + } +} + // `abctl observe` wires the harvest to the flag: on by default, off with // --skip-claude-metadata. // diff --git a/authbridge/cmd/abctl/main.go b/authbridge/cmd/abctl/main.go index fc9910198..3aeedf1d0 100644 --- a/authbridge/cmd/abctl/main.go +++ b/authbridge/cmd/abctl/main.go @@ -13,9 +13,11 @@ package main import ( "context" + "errors" "flag" "fmt" "io" + "io/fs" "os" "os/exec" "os/signal" @@ -239,14 +241,52 @@ func claudeHarvester(warn io.Writer) tui.HarvestFunc { fmt.Fprintf(warn, "abctl: not naming sessions from Claude Code: %v\n", err) return nil } else if _, err := claude.ReadMetadata(path); err != nil { - // A corrupt file is the one failure that cannot clear itself: every launch reads the - // same bad file. ErrCorruptMetadata is exported so a caller can name the fix, and this - // caller has to name a different one from the subcommand's --merge=false, which is not - // a flag `abctl observe` has. - fmt.Fprintf(warn, "abctl: not naming sessions from Claude Code: %v\n"+ - " Fix or move the file, or rebuild it:\n"+ - " abctl experimental read-claude-sessions --merge=false\n", err) - return nil + // A FILE THAT DOES NOT PARSE IS NO LONGER CHECKED FOR HERE, because Harvest rebuilds it + // on the background goroutine and the titles arrive on their own. What is left is the + // file Harvest still refuses: one that could not be READ. That one repeats every launch + // and needs a human — a permission fixed, a disk looked at — so it is worth the one + // thing this position can still do, which is print before the alt screen goes up. + // + // Read-and-discard rather than a stat: the failure being checked for is the read itself, + // and Harvest's own read is the one that decides. Not a wasted read either way — it was + // already here, and what changed is only which of its errors is fatal. + if errors.Is(err, fs.ErrPermission) { + fmt.Fprintf(warn, "abctl: not naming sessions from Claude Code: %v\n"+ + " Fix the file's permissions, or move it aside:\n"+ + " mv %s %s.bad\n", err, path, path) + return nil + } + // Too large is the same shape of problem: Harvest refuses it, so it repeats every launch + // and needs a human. Different remedy — the file is intact and readable, just over the + // cap — so it gets its own line rather than the permission advice. + if errors.Is(err, claude.ErrMetadataTooLarge) { + fmt.Fprintf(warn, "abctl: not naming sessions from Claude Code: %v\n"+ + " Move it aside to start a fresh file:\n"+ + " mv %s %s.bak\n", err, path, path) + return nil + } + // EVERY OTHER READ ERROR FALLS THROUGH SILENTLY rather than disabling titles or warning + // here, because this position cannot tell which of them Harvest will recover from. The + // sentinel that says "this one does not parse" is unexported, and exporting it would + // widen authlib's API to let this pre-flight re-derive a decision Harvest makes a few + // lines later anyway. + // + // So the split is by what a human can DO, not by what went wrong: the two cases above + // name an action and repeat every launch, which is worth printing before the alt screen + // goes up. Not warning about the rest is the deliberate half — the common case among them + // is the file that does not parse, which heals itself moments later, so a line here would + // tell the operator titles are in trouble and then hand them titles. + // + // WHAT THAT COSTS, stated plainly because there is no other record of it: the rarer + // errors here — EIO, EISDIR, ELOOP — are ones Harvest also refuses, and nothing + // downstream reports them. harvestCmd does `meta, _ := h()` (tui/session_metadata.go) + // and drops the error on purpose, so those launch with no titles and no explanation, + // every time, exactly the shape of bug this fix exists to remove. It is narrower than + // what was fixed — a directory or a symlink loop where the metadata file belongs is not + // a state a user reaches by accident, whereas a truncated write is — and closing it + // properly means either exporting the parse sentinel or giving the TUI somewhere to put + // a late error. Both are bigger than this change; #1110 is about the file that does not + // parse and the wedged lock. } return func() (map[string]tui.SessionMetadata, error) { // Incremental, unlike `abctl experimental read-claude-sessions`: that command's subject diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go index 7756ec1ff..c2464c527 100644 --- a/authbridge/cmd/abctl/tui/session_metadata.go +++ b/authbridge/cmd/abctl/tui/session_metadata.go @@ -58,16 +58,40 @@ func LoadSessionMetadata(path string) map[string]SessionMetadata { // before the TUI starts, stalling startup with nothing on screen to say why — the load // happens while the model is built, before tea.NewProgram, so there is nowhere to report // it. 16 MiB is far past any real metadata file: the measured 109-session file is 42 KB. + // + // READ ONE PAST THE CAP, and bail on over-cap rather than decoding. Without the +1 the file + // was read to exactly the cap and the truncated buffer handed to Unmarshal, and what that + // produced depended on where the cut landed. Measured both ways: + // + // - Cut mid-token, the common case: Unmarshal fails with "unexpected end of JSON input" + // and the result is this same empty map. No behaviour change from the fix. + // - Cut where the prefix is INDEPENDENTLY VALID JSON — trailing whitespace past the cap + // does it — the prefix decoded and this function returned a PARTIAL map as though it + // were the whole file, while claude.ReadMetadata refused the identical bytes. A + // 16,777,238-byte file loaded 1 entry before the fix and 0 after. + // + // The second is what the +1 is for: not the volume of titles lost, but that a partial read + // was indistinguishable from a complete one. + // + // Still returns the empty map, because this function has nowhere to report anything and + // says so above — but claude.ReadMetadata applies the SAME cap and DOES distinguish it, + // carrying ErrMetadataTooLarge, and `abctl observe`'s pre-flight calls that before the alt + // screen goes up. So the operator gets the one line naming the remedy from there; what + // this bail-out buys is that the two readers agree on which files are too large, instead + // of one silently reading a prefix the other refuses whole. const maxMetadataBytes = 16 << 20 f, err := os.Open(path) //nolint:gosec // operator-supplied path if err != nil { return out } defer f.Close() //nolint:errcheck // read-only - b, err := io.ReadAll(io.LimitReader(f, maxMetadataBytes)) + b, err := io.ReadAll(io.LimitReader(f, maxMetadataBytes+1)) if err != nil { return out } + if len(b) > maxMetadataBytes { + return out + } var m map[string]SessionMetadata if err := json.Unmarshal(b, &m); err != nil { return out diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 534be3827..0b8a8aa4e 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -2120,3 +2120,51 @@ func TestRefreshTick_FreshRowStillWaitsToSettle(t *testing.T) { "exists because the title tiers read a transcript the agent has not finished writing") } } + +// An oversized file must be REFUSED WHOLE, not read as a valid prefix of itself. +// +// What the cap does and does not protect against, measured rather than assumed. A file over the +// limit was read to exactly maxMetadataBytes with no +1, and what happened next depended entirely +// on where the cut landed: +// +// - Cut mid-token — the overwhelmingly common case — Unmarshal fails with "unexpected end of +// JSON input" and the result is the empty map. Identical before and after this fix, and not +// data loss beyond the TITLE column this function is contracted to lose on any failure. +// - Cut where the prefix is INDEPENDENTLY VALID JSON, which trailing whitespace past the cap +// produces: the prefix decoded and the viewer loaded a partial map, believing it complete, +// while claude.ReadMetadata refused the very same file with ErrMetadataTooLarge. Measured: a +// 16,777,238-byte file loaded 1 entry before the fix and 0 after. +// +// The second is the one worth a test, because it is the only shape where the two readers of this +// file disagreed about its CONTENTS rather than merely about how loudly to fail. +func TestLoadSessionMetadata_AtTheReadCap(t *testing.T) { + const cap = 16 << 20 + path := filepath.Join(t.TempDir(), "session-metadata.json") + + // A valid object followed by enough whitespace to push the file past the cap. The first + // cap bytes are therefore valid JSON on their own — which is exactly what makes a truncated + // read decode successfully and silently drop whatever followed. + body := `{"a":{"title":"real"}}` + over := []byte(body + strings.Repeat(" ", cap)) + if len(over) <= cap { + t.Fatalf("fixture is %d bytes, within the %d cap: it cannot test the over case", len(over), cap) + } + if err := os.WriteFile(path, over, 0o600); err != nil { + t.Fatal(err) + } + // Zero, not one. One means a prefix decoded and the viewer is now showing a file it only + // partly read, with no error anywhere and claude.ReadMetadata refusing the same bytes. + if got := LoadSessionMetadata(path); len(got) != 0 { + t.Errorf("loaded %d entries from a %d byte file: a valid prefix of an oversize file decoded", + len(got), len(over)) + } + + // And the cap still admits everything under it, so the guard is a limit and not a wall. + under := []byte(body + strings.Repeat(" ", 1024)) + if err := os.WriteFile(path, under, 0o600); err != nil { + t.Fatal(err) + } + if got := LoadSessionMetadata(path); len(got) != 1 || got["a"].Title != "real" { + t.Errorf("loaded %d entries from a %d byte file, want the 1 real entry", len(got), len(under)) + } +}