diff --git a/PERF.md b/PERF.md index 01bd6d4275..c7ed2cb4c7 100644 --- a/PERF.md +++ b/PERF.md @@ -83,6 +83,16 @@ migration flag skips that check. `BenchmarkOpenSettledSessions` measures the whole public open/close operation. `TestSettledStoreTerminalStartup` separates fixture construction from the first interactive frame of the running terminal. +Furrow detection stays off the hello path. The belt reads the remembered answer +(`furrow.PeekTools`); the session asks furrow from a goroutine joined at close +and appends the four workspace tools through `armFamily` if the folder is +watched. Measured on Spark linux-arm64 with a cold engine per workspace and the +owner profile, first frame fell from 3.87 s to about 0.5 s with the furrow +that was embedded then; with furrow 0.3.3's fast status the unfixed tree is already +at about 0.43 s, and this change keeps the build from depending on furrow's speed. The gate is +`TestBuildingASessionDoesNotWaitForFurrow`, which holds `furrow status` open and +asserts the build came back while that process was still alive; no clock decides it. + Native peer acknowledgements reuse the validated routing view only for the same store and durably applied index. Every applied change, including revocation, invalidates that view. `TestR11R27UnchangedPeerAcknowledgementWork` exercises the diff --git a/docs/changes/unreleased/1954-boot-without-furrow-detect.md b/docs/changes/unreleased/1954-boot-without-furrow-detect.md new file mode 100644 index 0000000000..657f352fe8 --- /dev/null +++ b/docs/changes/unreleased/1954-boot-without-furrow-detect.md @@ -0,0 +1,27 @@ +--- +kind: fixed +title: Show the first frame without waiting on furrow detection +pr: 1954 +surface: [chat, engine, docs] +invalidates: + - Building a session ran furrow to find out whether the folder is watched; it now starts that question in the background and the session never waits for it. + - The workspace snapshot, restore, fork and merge tools were decided when the belt was built; on a watched folder they now join the end of the belt when furrow answers. + - A session closing while another session on the same folder was still waiting for furrow used to wait for it too; it now leaves at once. +--- + +A cold launch used to block the engine hello behind `furrow --version` and +`furrow status`, two to three seconds on a folder furrow was never pointed at +(with the furrow that shipped before 0.3.3). Each session now asks furrow from a +goroutine it owns and joins at close, so first-frame time no longer depends on +how fast furrow is: on a cold engine and a real profile it was about 0.5 s +instead of 3.9 s then, and it stays about 0.4 s whatever furrow does. + +An unwatched folder still gets none of the four tools. On a watched folder they +are appended to the end of the tool list when furrow answers, like any other +late capability, so the prompt cache is invalidated once at the tail and never +moves anything ahead of it. The first request of a session waits up to 200 ms for +a question that is already running, so a message sent the instant the session +exists usually carries the tools and pays no invalidation. Tools never leave the +list mid-conversation: a folder that stops being watched later keeps them, and +they answer with furrow's own refusal. A furrow that fails or hangs is +remembered as not attached for thirty seconds rather than asked again. diff --git a/internal/furrow/ctx_test.go b/internal/furrow/ctx_test.go new file mode 100644 index 0000000000..5f2a6ede3e --- /dev/null +++ b/internal/furrow/ctx_test.go @@ -0,0 +1,101 @@ +//go:build unix + +package furrow + +import ( + "context" + "fmt" + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +// installParkedFurrow is a furrow whose `status` announces itself on the ready +// pipe and then parks on the gate pipe, so a test can hold one question open for +// as long as it likes and prove a second asker did not wait for it. +func installParkedFurrow(t *testing.T) (pid func() int, release func()) { + t.Helper() + carryNothing(t) + dir := t.TempDir() + ready, gate, pidfile := filepath.Join(dir, "ready"), filepath.Join(dir, "gate"), filepath.Join(dir, "pid") + for _, fifo := range []string{ready, gate} { + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Fatalf("mkfifo: %v", err) + } + } + script := filepath.Join(dir, "furrow") + body := `#!/bin/sh +if [ "$1" = "--version" ]; then echo "furrow 0.1.0"; exit 0; fi +echo $$ > "` + pidfile + `" +echo started > "` + ready + `" +cat "` + gate + `" > /dev/null +echo '{"head":"aaaabbbbcccc0001","watcher_running":true}' +` + if err := os.WriteFile(script, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+"/usr/bin"+string(os.PathListSeparator)+"/bin") + t.Setenv(BinaryEnvVar, "") + Forget() + t.Cleanup(Forget) + t.Cleanup(func() { + if f, err := os.OpenFile(gate, os.O_WRONLY|syscall.O_NONBLOCK, 0); err == nil { + _ = f.Close() + } + }) + pid = func() int { + done := make(chan struct{}) + go func() { _, _ = os.ReadFile(ready); close(done) }() + select { + case <-done: + case <-time.After(time.Minute): + t.Fatal("furrow's status call never started") + } + raw, _ := os.ReadFile(pidfile) + var n int + _, _ = fmt.Sscan(string(raw), &n) + return n + } + release = func() { + done := make(chan struct{}) + go func() { _ = os.WriteFile(gate, nil, 0o600); close(done) }() + select { + case <-done: + case <-time.After(time.Minute): + t.Fatal("furrow was not waiting at the gate") + } + } + return pid, release +} + +// A caller whose context is cancelled stops waiting behind another caller's +// question. The other caller's furrow is untouched and still answers. +func TestAWaiterCanAbandonAnotherAskersQuestion(t *testing.T) { + pid, release := installParkedFurrow(t) + root := t.TempDir() + leader := make(chan Presence, 1) + go func() { leader <- Detect(context.Background(), root) }() + leaderPid := pid() + + ctx, cancel := context.WithCancel(context.Background()) + waiter := make(chan Presence, 1) + go func() { waiter <- Detect(ctx, root) }() + cancel() + select { + case got := <-waiter: + if got.Available() { + t.Fatalf("an abandoned wait answered %+v", got) + } + case <-time.After(time.Minute): + t.Fatal("a cancelled caller kept waiting behind another caller's furrow") + } + if syscall.Kill(leaderPid, 0) != nil { + t.Fatal("abandoning the wait cut the other caller's furrow") + } + release() + if got := <-leader; !got.Available() { + t.Fatalf("the question that was waited behind answered %+v; want available", got) + } +} diff --git a/internal/furrow/furrow.go b/internal/furrow/furrow.go index 08178f011a..c0a52e820c 100644 --- a/internal/furrow/furrow.go +++ b/internal/furrow/furrow.go @@ -65,7 +65,11 @@ const ( // mount must cost a session a bounded pause rather than a hang; a detection // that times out reads as "not attached", which is the answer that leaves the // belt honest. -const detectTimeout = 5 * time.Second +// +// It is a variable for the reason [embedded] is: a test has to be able to say +// "a furrow that hangs" without waiting out the real bound. Nothing in the +// product reassigns it. +var detectTimeout = 5 * time.Second // readTimeout bounds the read-only questions — the timeline, the fork list, a // restore preview. They touch the store and nothing else, so they are quick, @@ -165,6 +169,14 @@ func lockAttach(root string) func() { return lock.Unlock } +// detecting holds the questions that are being asked right now, one per root. +// A caller that finds its root here waits for that question instead of asking +// its own, and can leave the wait when its own context ends ([Detect]). +var ( + detectingMu sync.Mutex + detecting = map[string]chan struct{}{} +) + // Forget drops the cached answer for every workspace. It exists for tests, // which change what is on PATH between cases and would otherwise read a // neighbour's answer, and for a caller that has just watched the person attach @@ -191,20 +203,67 @@ func Forget() { // CAN act on are carried in the fields instead. func Detect(ctx context.Context, root string) Presence { root = absoluteRoot(root) + for { + if presence, ok := fresh(root); ok { + return presence + } + // ONE ASKER PER FOLDER AT A TIME, AND A WAITER CAN LEAVE. Every session on + // a workspace starts its own background question, so a window that opens + // several chats at once would otherwise run the same two execs once per + // chat. The later callers wait for the first one's answer, but on their + // OWN context: a session closing, or a ground-ladder rung whose deadline + // passed, stops waiting at once and leaves the first caller's furrow + // running for whoever still wants it. (A plain mutex here made a Close + // wait out another session's five-second bound.) + detectingMu.Lock() + if running, ok := detecting[root]; ok { + detectingMu.Unlock() + select { + case <-running: + // The asker finished. Its answer is in the cache, or it was + // cut short and wrote nothing, in which case this caller asks. + continue + case <-ctx.Done(): + return Presence{} + } + } + running := make(chan struct{}) + detecting[root] = running + detectingMu.Unlock() + defer func() { + detectingMu.Lock() + delete(detecting, root) + detectingMu.Unlock() + close(running) + }() + + presence := detect(ctx, root) + + // AN ANSWER A CANCELLATION WROTE IS NOT FURROW'S. A caller that was cut + // mid-question (a session closing while its background refresh was + // still in `furrow status`) got "not attached" because the exec was + // killed, not because the folder is unwatched, and leaving that in the + // cache would tell every other session on the folder the same untruth. + // A furrow that FAILED or TIMED OUT on its own is different: that is an + // answer, it is cached like any other, and it is what keeps a hanging + // furrow to two launches per [detectTTL] however many belts are built. + if ctx.Err() == nil { + presenceMu.Lock() + presenceCache[root] = cachedPresence{presence: presence, asked: time.Now()} + presenceMu.Unlock() + } + return presence + } +} +// fresh is the cached answer for a root when it is still inside [detectTTL]. +func fresh(root string) (Presence, bool) { presenceMu.Lock() + defer presenceMu.Unlock() if cached, ok := presenceCache[root]; ok && time.Since(cached.asked) < detectTTL { - presenceMu.Unlock() - return cached.presence + return cached.presence, true } - presenceMu.Unlock() - - presence := detect(ctx, root) - - presenceMu.Lock() - presenceCache[root] = cachedPresence{presence: presence, asked: time.Now()} - presenceMu.Unlock() - return presence + return Presence{}, false } func detect(ctx context.Context, root string) Presence { @@ -329,7 +388,14 @@ func Open(ctx context.Context, root string) *Workspace { if !presence.Available() { return nil } - return &Workspace{root: absoluteRoot(root), binary: presence.Path} + return presence.workspace(root) +} + +// workspace is the one place a Workspace is made from an answer, shared by +// [Open] and [PeekTools] so the two cannot disagree about what holding one +// claims. +func (p Presence) workspace(root string) *Workspace { + return &Workspace{root: absoluteRoot(root), binary: p.Path} } // wholeWorkspaceTimeout bounds the two calls whose cost is the person's project diff --git a/internal/furrow/peek.go b/internal/furrow/peek.go new file mode 100644 index 0000000000..3bda859e38 --- /dev/null +++ b/internal/furrow/peek.go @@ -0,0 +1,38 @@ +package furrow + +import ( + "time" + + "github.com/Agent-Field/codeaf/internal/exec/bare" +) + +// peek is the last answer about one folder, read from memory and NEVER from a +// subprocess. It is the other half of [Detect], which is the door that asks. +// +// known is false when nobody has asked about this folder yet. fresh is false +// when the answer is older than [detectTTL]; the stale answer is still returned +// because it is the best one there is. +func peek(root string) (presence Presence, fresh, known bool) { + root = absoluteRoot(root) + presenceMu.Lock() + defer presenceMu.Unlock() + cached, ok := presenceCache[root] + if !ok { + return Presence{}, false, false + } + return cached.presence, time.Since(cached.asked) < detectTTL, true +} + +// PeekTools is [Tools] for a caller that may not wait: the four verbs when the +// remembered answer says this folder is attached, and nothing when it says it is +// not or when there is no answer yet. The absence law holds either way — a verb +// is only ever handed over on the strength of an answer furrow gave. fresh says +// whether that answer is young enough to rely on; a caller that wants a newer one +// asks [Detect] from somewhere it is allowed to wait. +func PeekTools(root string) (tools []bare.Tool, fresh bool) { + presence, fresh, known := peek(root) + if !known || !presence.Available() { + return nil, fresh + } + return presence.workspace(root).Tools(), fresh +} diff --git a/internal/furrow/peek_test.go b/internal/furrow/peek_test.go new file mode 100644 index 0000000000..f705ca6f78 --- /dev/null +++ b/internal/furrow/peek_test.go @@ -0,0 +1,95 @@ +package furrow + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// peek reads what was last asked and never asks: with nothing remembered it says +// so, and a furrow that would answer is not launched by looking. +func TestPeekNeverAsks(t *testing.T) { + installFake(t, "ok") + root := t.TempDir() + if _, _, known := peek(root); known { + t.Fatal("peek knew a folder nobody had asked about") + } + if tools, _ := PeekTools(root); len(tools) != 0 { + t.Fatalf("PeekTools on an unasked folder = %d tools; want none", len(tools)) + } + Detect(context.Background(), root) + presence, fresh, known := peek(root) + if !known || !fresh || !presence.Available() { + t.Fatalf("peek after Detect = %+v fresh=%v known=%v; want the fresh available answer", presence, fresh, known) + } + tools, fresh := PeekTools(root) + if len(tools) != len(Names()) || !fresh { + t.Fatalf("PeekTools on an attached folder = %d tools fresh=%v; want %d fresh", len(tools), fresh, len(Names())) + } +} + +func TestPeekToolsOnAnUnattachedFolderIsNothing(t *testing.T) { + installFake(t, "unattached") + root := t.TempDir() + Detect(context.Background(), root) + if tools, fresh := PeekTools(root); len(tools) != 0 || !fresh { + t.Fatalf("PeekTools on an unwatched folder = %d tools fresh=%v; want none from a fresh answer", len(tools), fresh) + } +} + +// A question cut short is not an answer: it must not be remembered as "not +// attached" for the next session on the folder. +func TestACancelledDetectionIsNotRemembered(t *testing.T) { + installFake(t, "ok") + root := t.TempDir() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + Detect(ctx, root) + if _, _, known := peek(root); known { + t.Fatal("a cancelled detection was cached as furrow's answer") + } +} + +// A furrow that fails or hangs on its own IS an answer, and it is remembered for +// the TTL: however many times the question is asked, furrow is launched for it +// once. Launches are counted; nothing is timed. +func TestAFailingOrHangingFurrowIsAskedOncePerTTL(t *testing.T) { + for name, status := range map[string]string{ + "failing": "echo boom >&2; exit 1", + "hanging": "sleep 30", + } { + t.Run(name, func(t *testing.T) { + carryNothing(t) + dir := t.TempDir() + log := filepath.Join(dir, "launches") + script := "#!/bin/sh\necho \"$*\" >> " + log + "\nif [ \"$1\" = \"--version\" ]; then echo furrow 0.1.0; exit 0; fi\n" + status + "\n" + if err := os.WriteFile(filepath.Join(dir, Binary), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+"/usr/bin"+string(os.PathListSeparator)+"/bin") + t.Setenv(BinaryEnvVar, "") + Forget() + t.Cleanup(Forget) + restore := detectTimeout + detectTimeout = 200 * time.Millisecond + t.Cleanup(func() { detectTimeout = restore }) + + root := t.TempDir() + for i := 0; i < 8; i++ { + if Detect(context.Background(), root).Available() { + t.Fatal("a furrow that cannot answer reported available") + } + } + raw, err := os.ReadFile(log) + if err != nil { + t.Fatal(err) + } + if got := strings.Count(string(raw), "\n"); got != 2 { + t.Fatalf("furrow was launched %d times for eight questions; want 2 (--version, status)", got) + } + }) + } +} diff --git a/internal/session/agent.go b/internal/session/agent.go index 47336a7d80..3c7efa05a5 100644 --- a/internal/session/agent.go +++ b/internal/session/agent.go @@ -431,6 +431,10 @@ func newAgent(config Config, client Completer) (*Agent, error) { // is why it is the thing most carefully gated: see [Agent.startLaneBeat] for // the three sessions that run no beat at all. agent.startLaneBeat() + // AND FURROW IS ASKED ABOUT THIS FOLDER, from the background. Opening the + // conversation must not wait on a subprocess, so the answer is started here + // and read later by the belt (tools_workspace.go). + agent.askFurrow() // AND ONLY NOW MAY IT SPEAK UNPROMPTED. Recovery turns the frontier, and a // cascade over the dependents of an interrupted node settles them right here, // inside New — before the caller holds the agent, before any surface has @@ -2509,6 +2513,7 @@ func (a *Agent) Close() error { // its fetch and join any cache write before the state directory can be // handed back to the caller. a.stopLaneBeat() + a.furrow.close() // AND THE BELT RUN, on the adaptive runs' own terms above: it holds a context // of its own precisely because the turn that proposed it ended, so this is // the only thing that can reach it. A RUN'S LIFE IS THE CONVERSATION'S. Until diff --git a/internal/session/anchor_furrow_test.go b/internal/session/anchor_furrow_test.go new file mode 100644 index 0000000000..369b33d3e2 --- /dev/null +++ b/internal/session/anchor_furrow_test.go @@ -0,0 +1,69 @@ +//go:build unix + +package session + +import ( + "os" + "path/filepath" + "testing" + + "github.com/Agent-Field/codeaf/internal/furrow" +) + +// installFurrowWatching is a furrow that reports only the folder named by the +// marker file as attached, and refuses every other folder in furrow's own words. +func installFurrowWatching(t *testing.T, watched string) { + t.Helper() + dir := t.TempDir() + script := filepath.Join(dir, "furrow") + writeFile(t, script, `#!/bin/sh +if [ "$1" = "--version" ]; then echo "furrow 0.1.0"; exit 0; fi +if [ "$2" = "`+watched+`" ]; then + echo '{"workspace":"'"$2"'","head":"aaaabbbbcccc0001","watcher_running":true}' + exit 0 +fi +echo "this repository is not watched" >&2 +exit 1 +`) + if err := os.Chmod(script, 0o755); err != nil { + t.Fatal(err) + } + t.Setenv(furrow.BinaryEnvVar, script) + furrow.Forget() + t.Cleanup(furrow.Forget) +} + +// A conversation that starts in its own scratch folder and is anchored to a +// project afterwards gets the project's furrow verbs: the anchor asks furrow +// about the NEW folder from the background and arms the verbs when it answers. +func TestAnchoringAsksFurrowAboutTheNewFolder(t *testing.T) { + project, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + installFurrowWatching(t, project) + dir := filepath.Join(t.TempDir(), "0123456789abcdef") + place := Place{Dir: dir, Owned: true} + place.Workspace = place.Work() + if err := os.MkdirAll(place.Work(), 0o700); err != nil { + t.Fatal(err) + } + agent, err := newAgent(Config{ + Workspace: place.Work(), Place: place, SessionFile: place.Transcript(), Model: "test/model", System: "SYSTEM", + }, &scriptedCompleter{}) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = agent.Close() }) + answered(t, agent) + if verbs := workspaceVerbs(carried(agent)); len(verbs) != 0 { + t.Fatalf("the scratch folder carried %v", verbs) + } + if _, err := agent.AnchorWorkspace(project); err != nil { + t.Fatal(err) + } + answered(t, agent) + if got := len(workspaceVerbs(carried(agent))); got != len(furrow.Names()) { + t.Fatalf("after anchoring to a watched project the belt carries %d workspace verbs; want %d", got, len(furrow.Names())) + } +} diff --git a/internal/session/loop.go b/internal/session/loop.go index 789b3482e6..f9ff0cdd2d 100644 --- a/internal/session/loop.go +++ b/internal/session/loop.go @@ -2065,6 +2065,9 @@ func (a *Agent) completeWithRetryReasoning(ctx context.Context, hub *eventHub, m attemptCtx = provider.WithContextBudget(attemptCtx, provider.ContextBudget{ Window: a.window(), Reserve: ctxbudget.CompletionReserve(), PromptFloor: promptFloor, }) + // THE FIRST REQUEST LOOKS ONCE AT FURROW'S QUESTION, and only here: the + // session was built without waiting for it (tools_workspace.go). + a.settleFurrow() attemptCtx, generation := a.beginGeneration(attemptCtx, reached) response, err := a.completeWithModel(attemptCtx, purpose, messages, model, ai.WithTools(a.beltDefinitions())) diff --git a/internal/session/session.go b/internal/session/session.go index 79bd5a4c86..35daffdd76 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -2665,6 +2665,10 @@ type Agent struct { // have a lane context, and "did a beat start" is the question the gate is // about (agent.go's [Agent.startLaneBeat]). laneBeating bool + // furrow is the background question to furrow about this session's folder + // (tools_workspace.go): started at construction, settled once by the first + // request, cancelled and joined by Close. + furrow furrowRefresher // turnLane is what THIS agent's own last request was served by, and it is // the only honest source there is for the lane half of a usage row diff --git a/internal/session/task_divide_wip_test.go b/internal/session/task_divide_wip_test.go index bb5f312ff0..b97145ed1e 100644 --- a/internal/session/task_divide_wip_test.go +++ b/internal/session/task_divide_wip_test.go @@ -490,7 +490,11 @@ func TestAFamilyWorkingInThePersonsOwnRepositoryIsNeverCommittedInto(t *testing. dirty := gitOut(t, repo, "status", "--porcelain") nest := newDivideNest(t, wideBrief, 0) + // Under the session lock: the furrow question started at construction reads + // this field from its own goroutine. + nest.node.mu.Lock() nest.node.config.Workspace = repo + nest.node.mu.Unlock() nest.parent.setTree(taskTree{dir: repo, ground: repo, merge: mergeInPlace, mode: TaskModeInPlace, rung: GroundRungHere}) nest.parent.noteWrote("repro.txt") diff --git a/internal/session/tools_anchor_workspace.go b/internal/session/tools_anchor_workspace.go index 91d045e02c..3f6ad6ce67 100644 --- a/internal/session/tools_anchor_workspace.go +++ b/internal/session/tools_anchor_workspace.go @@ -117,6 +117,11 @@ func (a *Agent) AnchorWorkspace(path string) (string, error) { a.jobs.place = place a.jobs.mu.Unlock() } + // AND FURROW IS ASKED ABOUT THE NEW FOLDER, from the background and never + // waited for: the belt rebuilt above read only what was already remembered, + // and the verbs arrive at the tail when furrow answers + // (tools_workspace.go's [Agent.askFurrow]). + a.askFurrow() a.rerenderSystemLocked(time.Now()) return resolved, nil } diff --git a/internal/session/tools_workspace.go b/internal/session/tools_workspace.go index ecb03e01ec..27ce7c0f81 100644 --- a/internal/session/tools_workspace.go +++ b/internal/session/tools_workspace.go @@ -27,20 +27,184 @@ package session import ( "context" + "sync" + "time" "github.com/Agent-Field/codeaf/internal/exec/bare" "github.com/Agent-Field/codeaf/internal/furrow" + "github.com/Agent-Field/codeaf/internal/guard" ) -// workspaceTools is the family, or nothing. +// workspaceTools is the family, or nothing, from what is ALREADY KNOWN. // -// THE DETECTION IS PAID HERE AND NOT ON THE DRAW PATH. This runs once per belt -// assembly — a turn, not a frame — and internal/furrow keeps its own answer for -// a while behind that, so the cost is one short-lived subprocess per workspace -// per half-minute rather than one per question the screen asks. A seam that -// shelled out while a terminal was repainting would be the surface stopping to -// ask another program a question, which is the mistake internal/tui3's -// hostlink.go documents for its own seam. +// IT NEVER WAITS FOR FURROW. The first version of this asked furrow on the spot, +// and the belt is assembled while the engine answers a window's hello, so a +// cold launch held its first frame behind `furrow --version` and then +// `furrow status` — two to three seconds on a folder furrow was never pointed +// at, because even that answer opens furrow's global store. Nothing the first +// frame draws depends on these four verbs, so this reads the remembered answer +// ([furrow.PeekTools]) and nothing more. +// +// THE BELT IS BUILT ONCE, so a verb that is not on it now has to come later by +// the road every other late capability takes: [Agent.armFamily], which appends +// at the tail. [Agent.askFurrow] asks from the background when the session is +// built and arms the verbs when furrow says the folder is attached +// ([Agent.armFurrowVerbs]). An unattached folder — the common case — never +// changes at all, and the model is never told about a verb on a guess: the +// absence law holds with a cold cache exactly as it does with a missing binary. func (a *Agent) workspaceTools() []bare.Tool { - return furrow.Tools(context.Background(), a.config.Workspace) + tools, _ := furrow.PeekTools(a.config.Workspace) + return tools +} + +// firstBeltWait is how long the first request of a session waits for a furrow +// question that is already running. It is a courtesy to the doors that send a +// message the instant the session exists (`--once`, a resumed conversation with a +// pending prompt, a spawned worker): without it their only request would go out +// before the answer and carry no workspace verbs, and the arming that follows +// would cost a second prompt-cache write. It is a cap and not a promise — the +// request goes out without the verbs when it passes — and it is paid only where a +// request is made, never while the session is built. +const firstBeltWait = 200 * time.Millisecond + +// askFurrow starts the background question about this session's folder unless +// the remembered answer is already fresh. +func (a *Agent) askFurrow() { + if _, fresh := furrow.PeekTools(a.config.Workspace); fresh { + return + } + built := a.config + a.furrow.ask(built.Workspace, func(root string) { a.armFurrowVerbs(root, built) }) +} + +// armFurrowVerbs puts the verbs on the belt once furrow has said the folder is +// attached. It runs on the refresher's goroutine, so it touches the session's +// configuration as little as a background reader can: the profile comes from the +// copy taken when the session was built (whose live-profile pointer still follows +// a change of model), and the one live field it reads, the folder, is read under +// the session lock. It arms nothing if the session has since been anchored to +// another folder (the answer was about the old one) or belongs to a model with no +// tools, whose belt was built empty. +func (a *Agent) armFurrowVerbs(root string, built Config) { + // Nothing to arm is the common answer, and it is decided before the session's + // configuration is touched at all. + tools, _ := furrow.PeekTools(root) + if len(tools) == 0 || built.promptProfile().chat() { + return + } + a.mu.Lock() + same := a.config.Workspace == root + a.mu.Unlock() + if same { + _, _ = a.armFamily(tools) + } +} + +// settleFurrow is the first request's one look at the question: if furrow is +// being asked right now, wait for it for at most [firstBeltWait], and only the +// first time. Every later request reads the belt as it stands. +func (a *Agent) settleFurrow() { a.furrow.settle(firstBeltWait) } + +// furrowRefresher owns the background questions a session asks furrow, on the +// terms of [Agent.startLaneBeat]: the goroutine is the session's, Close cancels +// it and joins it, and a session that has begun closing starts nothing. +// +// Its mutex guards the flags and the group together, and IS held across +// guard.Go and the group's Add — both are non-blocking — so that the join in +// [furrowRefresher.close] cannot miss a refresh that begins after it. It is the +// LAST lock in any order: nothing is taken while it is held, so it is safe to +// reach from the belt, the request path or an anchor under the session lock. +type furrowRefresher struct { + mu sync.Mutex + ctx context.Context + cancel context.CancelFunc + jobs sync.WaitGroup + asking map[string]bool + busy chan struct{} + stopped bool + settled bool +} + +// ask starts a question about root unless one about that folder is already in +// flight. A question about ANOTHER folder runs beside it: an owned conversation +// anchored to a project after it was built asks about the project while the +// scratch folder's question may still be running ([Agent.AnchorWorkspace]). +func (r *furrowRefresher) ask(root string, landed func(root string)) { + r.mu.Lock() + defer r.mu.Unlock() + if r.stopped || r.asking[root] { + return + } + if r.ctx == nil { + r.ctx, r.cancel = context.WithCancel(context.Background()) + r.asking = map[string]bool{} + } + if len(r.asking) == 0 { + r.busy = make(chan struct{}) + } + r.asking[root] = true + busy := r.busy + r.jobs.Add(1) + ctx := r.ctx + guard.Go("session/furrow-detect", func() { + defer r.jobs.Done() + defer func() { + r.mu.Lock() + delete(r.asking, root) + if len(r.asking) == 0 { + close(busy) + } + r.mu.Unlock() + }() + furrow.Detect(ctx, root) + // The verbs are armed BEFORE the question is reported finished, so a + // caller released by it sees them on the belt. A cancelled question + // answered nothing and arms nothing. + if ctx.Err() == nil { + landed(root) + } + }) +} + +// idle is closed when no question is running: the channel of the batch in +// flight, or an already-closed one. +func (r *furrowRefresher) idle() <-chan struct{} { + r.mu.Lock() + defer r.mu.Unlock() + if len(r.asking) == 0 { + done := make(chan struct{}) + close(done) + return done + } + return r.busy +} + +// settle waits up to max for the questions in flight, once per session. +func (r *furrowRefresher) settle(max time.Duration) { + r.mu.Lock() + if r.settled || len(r.asking) == 0 { + r.mu.Unlock() + return + } + r.settled = true + busy := r.busy + r.mu.Unlock() + timer := time.NewTimer(max) + defer timer.Stop() + select { + case <-busy: + case <-timer.C: + } +} + +// close cancels the questions and waits for them. +func (r *furrowRefresher) close() { + r.mu.Lock() + r.stopped = true + cancel := r.cancel + r.mu.Unlock() + if cancel != nil { + cancel() + } + r.jobs.Wait() } diff --git a/internal/session/tools_workspace_test.go b/internal/session/tools_workspace_test.go new file mode 100644 index 0000000000..7a98b68fad --- /dev/null +++ b/internal/session/tools_workspace_test.go @@ -0,0 +1,320 @@ +//go:build unix + +package session + +// THE BELT NEVER WAITS FOR FURROW (tools_workspace.go). +// +// A cold launch used to hold its first frame behind `furrow --version` and +// `furrow status`, two to three seconds on a folder furrow was never pointed at. +// The tests here pin the property and not the seconds. Furrow is a script whose +// `status` call announces that it has started on one named pipe and then parks +// on another until the test lets it go, so "the session did not wait" is the +// observable fact that the build, or a Close, came back while that process was +// still alive. Nothing here measures how long anything took; the timeouts are +// safety nets that turn a hang into a loud failure. +import ( + "fmt" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/furrow" +) + +// safety is how long a test waits for something that must happen before it +// gives up and fails. It is never an assertion about speed. +const furrowTestSafety = 60 * time.Second + +type gatedFurrow struct { + launches, pidfile, ready, gate string +} + +// installGatedFurrow is a furrow that answers `--version` at once and answers +// `status` with an attached folder only once the test releases the gate. Every +// launch is appended to the launch log. +func installGatedFurrow(t *testing.T) *gatedFurrow { + t.Helper() + dir := t.TempDir() + g := &gatedFurrow{ + launches: filepath.Join(dir, "launches"), + pidfile: filepath.Join(dir, "pid"), + ready: filepath.Join(dir, "ready"), + gate: filepath.Join(dir, "gate"), + } + for _, fifo := range []string{g.ready, g.gate} { + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Fatalf("make the pipe %s: %v", fifo, err) + } + } + script := filepath.Join(dir, "furrow") + writeFile(t, script, `#!/bin/sh +echo "$*" >> "`+g.launches+`" +if [ "$1" = "--version" ]; then echo "furrow 0.1.0"; exit 0; fi +echo $$ > "`+g.pidfile+`" +echo started > "`+g.ready+`" +cat "`+g.gate+`" > /dev/null +echo '{"workspace":"'"$2"'","head":"aaaabbbbcccc0001","watcher_running":true}' +`) + if err := os.Chmod(script, 0o755); err != nil { + t.Fatalf("chmod the gated furrow: %v", err) + } + t.Setenv(furrow.BinaryEnvVar, script) + furrow.Forget() + t.Cleanup(furrow.Forget) + // A test that fails with furrow parked must not leave a process behind: a + // non-blocking open succeeds only if the script is already reading. + t.Cleanup(func() { + if f, err := os.OpenFile(g.gate, os.O_WRONLY|syscall.O_NONBLOCK, 0); err == nil { + _ = f.Close() + } + }) + return g +} + +// started blocks until furrow's status call is running and answers its pid. +func (g *gatedFurrow) started(t *testing.T) int { + t.Helper() + done := make(chan struct{}) + go func() { + _, _ = os.ReadFile(g.ready) + close(done) + }() + select { + case <-done: + case <-time.After(furrowTestSafety): + t.Fatal("furrow's status call never started") + } + raw, err := os.ReadFile(g.pidfile) + if err != nil { + t.Fatalf("read furrow's pid: %v", err) + } + var pid int + if _, err := fmt.Sscan(string(raw), &pid); err != nil { + t.Fatalf("furrow's pid %q: %v", raw, err) + } + return pid +} + +// release lets the parked status call answer. +func (g *gatedFurrow) release(t *testing.T) { + t.Helper() + done := make(chan struct{}) + go func() { + _ = os.WriteFile(g.gate, nil, 0o600) + close(done) + }() + select { + case <-done: + case <-time.After(furrowTestSafety): + t.Fatal("furrow was not waiting at the gate") + } +} + +func (g *gatedFurrow) launchCount(t *testing.T) int { + t.Helper() + raw, err := os.ReadFile(g.launches) + if os.IsNotExist(err) { + return 0 + } + if err != nil { + t.Fatalf("read the launch log: %v", err) + } + return strings.Count(string(raw), "\n") +} + +func processAlive(pid int) bool { return syscall.Kill(pid, 0) == nil } + +func workspaceVerbs(tools []string) []string { + var verbs []string + for _, name := range tools { + if strings.HasPrefix(name, "workspace_") { + verbs = append(verbs, name) + } + } + return verbs +} + +// carried is what the model is actually shown: the belt as the turn reads it. +func carried(a *Agent) []string { + var names []string + for _, tool := range a.beltTools() { + names = append(names, tool.Name) + } + return names +} + +func furrowSession(t *testing.T, workspace string) *Agent { + t.Helper() + agent, err := newAgent(Config{Workspace: workspace, Model: "test/model", System: "SYSTEM"}, &scriptedCompleter{}) + if err != nil { + t.Fatalf("newAgent: %v", err) + } + t.Cleanup(func() { _ = agent.Close() }) + return agent +} + +// While furrow has not answered, building the session returns, and the belt +// carries no workspace verb. The status call is parked on the gate for the whole +// test, so a session that asked furrow itself could only come back after furrow +// was killed; here it comes back while furrow is still alive. +func TestBuildingASessionDoesNotWaitForFurrow(t *testing.T) { + g := installGatedFurrow(t) + built := make(chan *Agent, 1) + go func() { + agent, err := newAgent(Config{Workspace: t.TempDir(), Model: "test/model", System: "SYSTEM"}, &scriptedCompleter{}) + if err != nil { + t.Errorf("newAgent: %v", err) + } + built <- agent + }() + var agent *Agent + select { + case agent = <-built: + case <-time.After(furrowTestSafety): + t.Fatal("building the session waited on furrow") + } + if agent == nil { + return + } + t.Cleanup(func() { _ = agent.Close() }) + pid := g.started(t) + if !processAlive(pid) { + t.Fatal("furrow's status call was already over when the session came back; it should still be parked") + } + if verbs := workspaceVerbs(carried(agent)); len(verbs) != 0 { + t.Fatalf("the belt carried %v before furrow had answered; want none", verbs) + } + g.release(t) +} + +// On a folder furrow reports as watched, the four verbs reach the belt THE MODEL +// READS, even though the session was built before furrow answered. +func TestWatchedFolderVerbsReachTheModel(t *testing.T) { + g := installGatedFurrow(t) + agent := furrowSession(t, t.TempDir()) + g.started(t) + g.release(t) + answered(t, agent) + if got := len(workspaceVerbs(carried(agent))); got != len(furrow.Names()) { + t.Fatalf("the belt carries %d workspace verbs; want %d", got, len(furrow.Names())) + } + for _, tool := range agent.beltTools() { + if strings.HasPrefix(tool.Name, "workspace_") && (tool.Description == "" || len(tool.Schema) == 0 || tool.Execute == nil) { + t.Errorf("%s reached the belt without a description, schema or hand", tool.Name) + } + } + // The request's definitions travel with the tools, one for one. + if len(agent.beltDefinitions()) != len(agent.beltTools()) { + t.Fatalf("the request carries %d definitions for %d tools", len(agent.beltDefinitions()), len(agent.beltTools())) + } +} + +// answered waits for the background question to end, which is also the moment +// the verbs are armed. +func answered(t *testing.T, a *Agent) { + t.Helper() + select { + case <-a.furrow.idle(): + case <-time.After(furrowTestSafety): + t.Fatal("furrow's question never ended") + } +} + +// One session, however many belts it assembles, asks furrow its two questions +// once. +func TestFurrowIsAskedOncePerSession(t *testing.T) { + g := installGatedFurrow(t) + agent := furrowSession(t, t.TempDir()) + g.started(t) + g.release(t) + answered(t, agent) + for i := 0; i < 5; i++ { + _ = agent.belt() + } + if got := g.launchCount(t); got != 2 { + t.Fatalf("furrow was launched %d times for one session; want 2 (--version, status)", got) + } +} + +// Close cuts a question furrow is still answering and waits for it: when Close +// returns the process is gone. +func TestClosingJoinsAFurrowThatIsStillAnswering(t *testing.T) { + g := installGatedFurrow(t) + agent := furrowSession(t, t.TempDir()) + pid := g.started(t) + if err := agent.Close(); err != nil { + t.Fatalf("close: %v", err) + } + if processAlive(pid) { + t.Fatalf("Close returned with furrow's process %d still alive", pid) + } +} + +// Two sessions on one folder share one question. The second one closing must not +// wait behind the first one's furrow: it is not the one asking. +func TestClosingOneSessionDoesNotWaitBehindAnothersFurrow(t *testing.T) { + g := installGatedFurrow(t) + workspace := t.TempDir() + first := furrowSession(t, workspace) + pid := g.started(t) + second := furrowSession(t, workspace) + closed := make(chan error, 1) + go func() { closed <- second.Close() }() + select { + case err := <-closed: + if err != nil { + t.Fatalf("close: %v", err) + } + case <-time.After(furrowTestSafety): + t.Fatal("closing the second session waited on the first session's furrow") + } + if !processAlive(pid) { + t.Fatal("the first session's furrow was cut by the second session closing") + } + g.release(t) + answered(t, first) +} + +// The first request of a session waits for a question that is already running, +// and gets the verbs because of it; with a cap too short to matter it goes out +// without them and the arming that follows is still correct. +func TestTheFirstRequestSettlesAQuestionAlreadyRunning(t *testing.T) { + g := installGatedFurrow(t) + agent := furrowSession(t, t.TempDir()) + g.started(t) + settled := make(chan struct{}) + go func() { + agent.furrow.settle(furrowTestSafety) + close(settled) + }() + g.release(t) + select { + case <-settled: + case <-time.After(furrowTestSafety): + t.Fatal("the first request was not released by furrow's answer") + } + if got := len(workspaceVerbs(carried(agent))); got != len(furrow.Names()) { + t.Fatalf("the request that waited carries %d workspace verbs; want %d", got, len(furrow.Names())) + } +} + +// The wait is a cap: with furrow parked the request goes out without the verbs, +// and it only ever waits the first time. +func TestTheFirstRequestDoesNotWaitForAParkedFurrow(t *testing.T) { + g := installGatedFurrow(t) + agent := furrowSession(t, t.TempDir()) + pid := g.started(t) + agent.furrow.settle(time.Nanosecond) + if !processAlive(pid) { + t.Fatal("settling cut furrow") + } + if verbs := workspaceVerbs(carried(agent)); len(verbs) != 0 { + t.Fatalf("the request carried %v with furrow still parked", verbs) + } + // Spent: a second look returns at once even with a long cap. + agent.furrow.settle(furrowTestSafety) + g.release(t) +}