From 7f9f74e54f8fafbf4c3e2e4c9d0bfac6428c7188 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 00:50:13 -0400 Subject: [PATCH 01/11] test(session): building a session must not wait on furrow --- internal/session/tools_workspace_test.go | 177 +++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 internal/session/tools_workspace_test.go diff --git a/internal/session/tools_workspace_test.go b/internal/session/tools_workspace_test.go new file mode 100644 index 0000000000..df63d017d0 --- /dev/null +++ b/internal/session/tools_workspace_test.go @@ -0,0 +1,177 @@ +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: they hold furrow's answer +// back with a gate the test owns, so a session that waits for it cannot get past +// the first assertion, and they count the launches rather than time them. +import ( + "fmt" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/furrow" +) + +// installGatedFurrow is a furrow that answers `--version` at once and answers +// `status` with an attached folder only once the test creates the gate file. +// Every launch is appended to the launch log, and the status call's process id +// is written beside it, so the test can count launches and prove one is gone. +func installGatedFurrow(t *testing.T) (launches, gate, pidfile string) { + t.Helper() + dir := t.TempDir() + launches = filepath.Join(dir, "launches") + gate = filepath.Join(dir, "gate") + pidfile = filepath.Join(dir, "pid") + script := filepath.Join(dir, "furrow") + writeFile(t, script, `#!/bin/sh +echo "$*" >> "`+launches+`" +if [ "$1" = "--version" ]; then echo "furrow 0.1.0"; exit 0; fi +echo $$ > "`+pidfile+`" +while [ ! -e "`+gate+`" ]; do sleep 0.02; done +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) + return launches, gate, pidfile +} + +func launchCount(t *testing.T, launches string) int { + t.Helper() + raw, err := os.ReadFile(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 workspaceVerbs(a *Agent) []string { + var names []string + for _, tool := range a.belt() { + if strings.HasPrefix(tool.Name, "workspace_") { + names = append(names, tool.Name) + } + } + return names +} + +// untilFurrow polls a condition. The deadline only turns a hang into a failure; no +// test asserts how long anything took. +func untilFurrow(t *testing.T, what string, ok func() bool) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for !ok() { + if time.Now().After(deadline) { + t.Fatalf("never happened: %s", what) + } + time.Sleep(10 * time.Millisecond) + } +} + +// While furrow has not answered, building the session and assembling its belt +// both return, and the belt carries no workspace verb. The status call is parked +// on the gate and stays parked, so a session that asks furrow itself cannot +// return before furrow's own five-second bound ends the wait; the detector below +// is shorter than that bound and exists only to turn "waited" into a failure. +func TestBuildingASessionDoesNotWaitForFurrow(t *testing.T) { + launches, gate, _ := installGatedFurrow(t) + t.Cleanup(func() { _ = os.WriteFile(gate, nil, 0o644) }) + + type built struct { + agent *Agent + verbs []string + err error + } + done := make(chan built, 1) + go func() { + agent, err := newAgent(Config{Workspace: t.TempDir(), Model: "test/model", System: "SYSTEM"}, &scriptedCompleter{}) + if err != nil { + done <- built{err: err} + return + } + done <- built{agent: agent, verbs: workspaceVerbs(agent)} + }() + select { + case got := <-done: + if got.err != nil { + t.Fatalf("newAgent: %v", got.err) + } + t.Cleanup(func() { _ = got.agent.Close() }) + if len(got.verbs) != 0 { + t.Fatalf("the belt carried %v before furrow had answered; want none", got.verbs) + } + case <-time.After(2 * time.Second): + t.Fatal("building the session waited on furrow") + } + untilFurrow(t, "furrow being asked", func() bool { return launchCount(t, launches) >= 2 }) +} + +// On a folder furrow reports as watched, the four verbs do reach the belt, with +// the description and schema the model is shown. +func TestWatchedFolderVerbsReachTheBelt(t *testing.T) { + _, gate, _ := installGatedFurrow(t) + if err := os.WriteFile(gate, nil, 0o644); err != nil { + t.Fatalf("open the gate: %v", err) + } + agent, _ := newTestAgent(t, &scriptedCompleter{}, nil) + untilFurrow(t, "the belt carrying the workspace verbs", func() bool { + return len(workspaceVerbs(agent)) == len(furrow.Names()) + }) + for _, tool := range agent.belt() { + 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) + } + } +} + +// One session, however many belts it assembles, asks furrow its two questions +// once. +func TestFurrowIsAskedOncePerSession(t *testing.T) { + launches, gate, _ := installGatedFurrow(t) + if err := os.WriteFile(gate, nil, 0o644); err != nil { + t.Fatalf("open the gate: %v", err) + } + agent, _ := newTestAgent(t, &scriptedCompleter{}, nil) + untilFurrow(t, "the belt carrying the workspace verbs", func() bool { return len(workspaceVerbs(agent)) > 0 }) + for i := 0; i < 5; i++ { + _ = agent.belt() + } + if got := launchCount(t, launches); 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) { + _, _, pidfile := installGatedFurrow(t) + agent, _ := newTestAgent(t, &scriptedCompleter{}, nil) + var pid int + untilFurrow(t, "furrow's status call starting", func() bool { + raw, err := os.ReadFile(pidfile) + if err != nil { + return false + } + _, err = fmt.Sscan(string(raw), &pid) + return err == nil && pid > 0 + }) + if err := agent.Close(); err != nil { + t.Fatalf("close: %v", err) + } + if err := syscall.Kill(pid, 0); err == nil { + t.Fatalf("Close returned with furrow's process %d still alive", pid) + } +} From 7a682a284488efa37cc230371f1c5791551c1e39 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 00:51:12 -0400 Subject: [PATCH 02/11] perf(engine): keep furrow detection off the session build path --- PERF.md | 7 ++ .../1952-boot-without-furrow-detect.md | 16 ++++ internal/furrow/furrow.go | 41 ++++++++++ internal/furrow/peek.go | 40 ++++++++++ internal/furrow/peek_test.go | 50 ++++++++++++ internal/session/agent.go | 5 ++ internal/session/session.go | 11 +++ internal/session/tools_workspace.go | 78 ++++++++++++++++--- 8 files changed, 239 insertions(+), 9 deletions(-) create mode 100644 docs/changes/unreleased/1952-boot-without-furrow-detect.md create mode 100644 internal/furrow/peek.go create mode 100644 internal/furrow/peek_test.go diff --git a/PERF.md b/PERF.md index 01c56095a2..bad6f937a7 100644 --- a/PERF.md +++ b/PERF.md @@ -83,6 +83,13 @@ 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`) and the session refreshes it from a goroutine joined +at close. 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.45 s. The gate is +`TestBuildingASessionDoesNotWaitForFurrow`, which holds `furrow status` open and +counts launches instead of timing them. + ## What the gates check `make test-quick` checks compilation, vet, formatting, packed manuals, change diff --git a/docs/changes/unreleased/1952-boot-without-furrow-detect.md b/docs/changes/unreleased/1952-boot-without-furrow-detect.md new file mode 100644 index 0000000000..507d97b2e4 --- /dev/null +++ b/docs/changes/unreleased/1952-boot-without-furrow-detect.md @@ -0,0 +1,16 @@ +--- +kind: fixed +title: Show the first frame without waiting on furrow detection +pr: 1952 +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; they now appear on the first belt built after furrow answers, so a session asked for its belt before then goes without them for that one belt. +--- + +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. +Each session now asks furrow from a goroutine it owns and joins at close, and the +belt reads the remembered answer. A cold engine on a real profile draws its first +frame in about 0.45 s instead of 3.87 s. An unwatched folder still gets none of +the four tools, and a cancelled question is never remembered as an answer. diff --git a/internal/furrow/furrow.go b/internal/furrow/furrow.go index 08178f011a..dbc70fc9cf 100644 --- a/internal/furrow/furrow.go +++ b/internal/furrow/furrow.go @@ -165,6 +165,25 @@ func lockAttach(root string) func() { return lock.Unlock } +// detectLocks is attachLocks for the question instead of the act: one lock per +// root, never removed, for the reason attachLocks gives. +var ( + detectMu sync.Mutex + detectLocks = map[string]*sync.Mutex{} +) + +func lockDetect(root string) func() { + detectMu.Lock() + lock, ok := detectLocks[root] + if !ok { + lock = &sync.Mutex{} + detectLocks[root] = lock + } + detectMu.Unlock() + lock.Lock() + return lock.Unlock +} + // 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 @@ -199,8 +218,30 @@ func Detect(ctx context.Context, root string) Presence { } presenceMu.Unlock() + // ONE ASKER PER FOLDER AT A TIME. Every session on a workspace starts its + // own background refresh ([Peek] is the door that reads without asking), so + // a window that opens several chats at once would otherwise run the same two + // execs once per chat. The second caller waits behind the first and reads + // the answer the first left in the cache. + release := lockDetect(root) + defer release() + presenceMu.Lock() + if cached, ok := presenceCache[root]; ok && time.Since(cached.asked) < detectTTL { + presenceMu.Unlock() + return cached.presence + } + presenceMu.Unlock() + 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 for half a minute. + if ctx.Err() != nil { + return presence + } presenceMu.Lock() presenceCache[root] = cachedPresence{presence: presence, asked: time.Now()} presenceMu.Unlock() diff --git a/internal/furrow/peek.go b/internal/furrow/peek.go new file mode 100644 index 0000000000..02fddd0fbe --- /dev/null +++ b/internal/furrow/peek.go @@ -0,0 +1,40 @@ +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 door for a caller that must not wait on furrow — the +// engine building a session for the first frame — and 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, and a caller that wants a newer one asks +// [Detect] from somewhere it is allowed to wait. +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. +func PeekTools(root string) (tools []bare.Tool, fresh, known bool) { + presence, fresh, known := Peek(root) + if !known || !presence.Available() { + return nil, fresh, known + } + workspace := &Workspace{root: absoluteRoot(root), binary: presence.Path} + return workspace.Tools(), fresh, known +} diff --git a/internal/furrow/peek_test.go b/internal/furrow/peek_test.go new file mode 100644 index 0000000000..359d21fd3b --- /dev/null +++ b/internal/furrow/peek_test.go @@ -0,0 +1,50 @@ +package furrow + +import ( + "context" + "testing" +) + +// 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, _, known := PeekTools(root); known || len(tools) != 0 { + t.Fatalf("PeekTools on an unasked folder = %d tools, known=%v; want none", len(tools), known) + } + 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, _, _ := PeekTools(root) + if len(tools) != len(Names()) { + t.Fatalf("PeekTools on an attached folder = %d tools; want %d", len(tools), len(Names())) + } +} + +func TestPeekToolsOnAnUnattachedFolderIsNothing(t *testing.T) { + installFake(t, "unattached") + root := t.TempDir() + Detect(context.Background(), root) + if tools, _, known := PeekTools(root); !known || len(tools) != 0 { + t.Fatalf("PeekTools on an unwatched folder = %d tools, known=%v; want none from a known answer", len(tools), known) + } +} + +// 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") + } +} diff --git a/internal/session/agent.go b/internal/session/agent.go index 47336a7d80..af5a626c30 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.stopFurrow() // 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/session.go b/internal/session/session.go index 79bd5a4c86..6dc441c4f3 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -2665,6 +2665,17 @@ 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 + // furrowCtx, furrowStop and furrowJobs are the background question to furrow + // about this session's folder (tools_workspace.go). furrowAsking keeps it to + // one at a time and furrowStopped is Close's word that no more may begin; + // all of it is guarded by furrowMu, which is taken by nothing else and held + // across nothing, so it is safe to reach from the belt under any other lock. + furrowMu sync.Mutex + furrowCtx context.Context + furrowStop context.CancelFunc + furrowJobs sync.WaitGroup + furrowAsking bool + furrowStopped bool // 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/tools_workspace.go b/internal/session/tools_workspace.go index ecb03e01ec..0d2b12e3dc 100644 --- a/internal/session/tools_workspace.go +++ b/internal/session/tools_workspace.go @@ -30,17 +30,77 @@ import ( "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, when there is none or it has gone stale, asks +// furrow from a goroutine this session owns ([Agent.askFurrow]) and hands over +// whatever it already knew. +// +// THE COST OF THAT IS ONE BELT, AND IT IS BOUNDED. A session that is asked for +// its belt before the first answer lands has no workspace verbs on it; the +// belt is rebuilt per turn, so the next one has them if the folder is +// attached. The refresh starts when the session is built (see [newAgent]), a +// person has to read the screen and type before the first turn, and an +// unattached folder — the common case — never changes at all. 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, fresh, _ := furrow.PeekTools(a.config.Workspace) + if !fresh { + a.askFurrow() + } + return tools +} + +// askFurrow refreshes the remembered answer about this session's folder from a +// goroutine, once at a time, and returns at once. +// +// THE GOROUTINE IS THE SESSION'S AND CLOSE JOINS IT ([Agent.stopFurrow]). +// Cancelling the context kills a `furrow status` that is still running, and the +// join is what keeps a half-finished exec from outliving the state directory a +// caller is about to remove. A session already closing starts nothing: the +// flag and the group are touched under one lock, so the join cannot miss a +// refresh that begins after it. +func (a *Agent) askFurrow() { + a.furrowMu.Lock() + defer a.furrowMu.Unlock() + if a.furrowStopped || a.furrowAsking { + return + } + if a.furrowCtx == nil { + a.furrowCtx, a.furrowStop = context.WithCancel(context.Background()) + } + a.furrowAsking = true + a.furrowJobs.Add(1) + ctx, root := a.furrowCtx, a.config.Workspace + guard.Go("session/furrow-detect", func() { + defer a.furrowJobs.Done() + defer func() { + a.furrowMu.Lock() + a.furrowAsking = false + a.furrowMu.Unlock() + }() + furrow.Detect(ctx, root) + }) +} + +// stopFurrow ends the refresh and waits for it, on [Agent.stopLaneBeat]'s terms. +func (a *Agent) stopFurrow() { + a.furrowMu.Lock() + a.furrowStopped = true + stop := a.furrowStop + a.furrowMu.Unlock() + if stop != nil { + stop() + } + a.furrowJobs.Wait() } From 87eefdc3639f53e770a532f812cd4f2306e89da6 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 01:40:51 -0400 Subject: [PATCH 03/11] docs: number the change entry for #1954 --- ...hout-furrow-detect.md => 1954-boot-without-furrow-detect.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename docs/changes/unreleased/{1952-boot-without-furrow-detect.md => 1954-boot-without-furrow-detect.md} (99%) diff --git a/docs/changes/unreleased/1952-boot-without-furrow-detect.md b/docs/changes/unreleased/1954-boot-without-furrow-detect.md similarity index 99% rename from docs/changes/unreleased/1952-boot-without-furrow-detect.md rename to docs/changes/unreleased/1954-boot-without-furrow-detect.md index 507d97b2e4..962ae5f40e 100644 --- a/docs/changes/unreleased/1952-boot-without-furrow-detect.md +++ b/docs/changes/unreleased/1954-boot-without-furrow-detect.md @@ -1,7 +1,7 @@ --- kind: fixed title: Show the first frame without waiting on furrow detection -pr: 1952 +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. From 598691cc8017b88e3529d78e5e4067560af3eefd Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 01:48:55 -0400 Subject: [PATCH 04/11] test: furrow answers must reach the model and waiters must be able to leave --- internal/furrow/ctx_test.go | 101 +++++++++ internal/session/tools_workspace_test.go | 276 +++++++++++++++-------- 2 files changed, 289 insertions(+), 88 deletions(-) create mode 100644 internal/furrow/ctx_test.go 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/session/tools_workspace_test.go b/internal/session/tools_workspace_test.go index df63d017d0..b4205997cd 100644 --- a/internal/session/tools_workspace_test.go +++ b/internal/session/tools_workspace_test.go @@ -1,12 +1,17 @@ +//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: they hold furrow's answer -// back with a gate the test owns, so a session that waits for it cannot get past -// the first assertion, and they count the launches rather than time them. +// 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" @@ -19,22 +24,38 @@ import ( "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 creates the gate file. -// Every launch is appended to the launch log, and the status call's process id -// is written beside it, so the test can count launches and prove one is gone. -func installGatedFurrow(t *testing.T) (launches, gate, pidfile string) { +// `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() - launches = filepath.Join(dir, "launches") - gate = filepath.Join(dir, "gate") - pidfile = filepath.Join(dir, "pid") + 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 "$*" >> "`+launches+`" +echo "$*" >> "`+g.launches+`" if [ "$1" = "--version" ]; then echo "furrow 0.1.0"; exit 0; fi -echo $$ > "`+pidfile+`" -while [ ! -e "`+gate+`" ]; do sleep 0.02; done +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 { @@ -43,12 +64,58 @@ echo '{"workspace":"'"$2"'","head":"aaaabbbbcccc0001","watcher_running":true}' t.Setenv(furrow.BinaryEnvVar, script) furrow.Forget() t.Cleanup(furrow.Forget) - return launches, gate, pidfile + // 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 launchCount(t *testing.T, launches string) int { +func (g *gatedFurrow) launchCount(t *testing.T) int { t.Helper() - raw, err := os.ReadFile(launches) + raw, err := os.ReadFile(g.launches) if os.IsNotExist(err) { return 0 } @@ -58,98 +125,114 @@ func launchCount(t *testing.T, launches string) int { return strings.Count(string(raw), "\n") } -func workspaceVerbs(a *Agent) []string { - var names []string - for _, tool := range a.belt() { - if strings.HasPrefix(tool.Name, "workspace_") { - names = append(names, tool.Name) +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 } -// untilFurrow polls a condition. The deadline only turns a hang into a failure; no -// test asserts how long anything took. -func untilFurrow(t *testing.T, what string, ok func() bool) { +func furrowSession(t *testing.T, workspace string) *Agent { t.Helper() - deadline := time.Now().Add(10 * time.Second) - for !ok() { - if time.Now().After(deadline) { - t.Fatalf("never happened: %s", what) - } - time.Sleep(10 * time.Millisecond) + 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 and assembling its belt -// both return, and the belt carries no workspace verb. The status call is parked -// on the gate and stays parked, so a session that asks furrow itself cannot -// return before furrow's own five-second bound ends the wait; the detector below -// is shorter than that bound and exists only to turn "waited" into a failure. +// 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) { - launches, gate, _ := installGatedFurrow(t) - t.Cleanup(func() { _ = os.WriteFile(gate, nil, 0o644) }) - - type built struct { - agent *Agent - verbs []string - err error - } - done := make(chan built, 1) + 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 { - done <- built{err: err} - return + t.Errorf("newAgent: %v", err) } - done <- built{agent: agent, verbs: workspaceVerbs(agent)} + built <- agent }() + var agent *Agent select { - case got := <-done: - if got.err != nil { - t.Fatalf("newAgent: %v", got.err) - } - t.Cleanup(func() { _ = got.agent.Close() }) - if len(got.verbs) != 0 { - t.Fatalf("the belt carried %v before furrow had answered; want none", got.verbs) - } - case <-time.After(2 * time.Second): + case agent = <-built: + case <-time.After(furrowTestSafety): t.Fatal("building the session waited on furrow") } - untilFurrow(t, "furrow being asked", func() bool { return launchCount(t, launches) >= 2 }) + 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 do reach the belt, with -// the description and schema the model is shown. -func TestWatchedFolderVerbsReachTheBelt(t *testing.T) { - _, gate, _ := installGatedFurrow(t) - if err := os.WriteFile(gate, nil, 0o644); err != nil { - t.Fatalf("open the gate: %v", err) - } - agent, _ := newTestAgent(t, &scriptedCompleter{}, nil) - untilFurrow(t, "the belt carrying the workspace verbs", func() bool { - return len(workspaceVerbs(agent)) == len(furrow.Names()) - }) - for _, tool := range agent.belt() { +// 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) + waitForVerbs(t, agent, 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())) + } +} + +// waitForVerbs waits for the background answer to land on the belt. +func waitForVerbs(t *testing.T, a *Agent, want int) { + t.Helper() + deadline := time.Now().Add(furrowTestSafety) + for len(workspaceVerbs(carried(a))) < want { + if time.Now().After(deadline) { + t.Fatalf("the belt never carried the workspace verbs; has %v", workspaceVerbs(carried(a))) + } + time.Sleep(5 * time.Millisecond) + } } // One session, however many belts it assembles, asks furrow its two questions // once. func TestFurrowIsAskedOncePerSession(t *testing.T) { - launches, gate, _ := installGatedFurrow(t) - if err := os.WriteFile(gate, nil, 0o644); err != nil { - t.Fatalf("open the gate: %v", err) - } - agent, _ := newTestAgent(t, &scriptedCompleter{}, nil) - untilFurrow(t, "the belt carrying the workspace verbs", func() bool { return len(workspaceVerbs(agent)) > 0 }) + g := installGatedFurrow(t) + agent := furrowSession(t, t.TempDir()) + g.started(t) + g.release(t) + waitForVerbs(t, agent, len(furrow.Names())) for i := 0; i < 5; i++ { _ = agent.belt() } - if got := launchCount(t, launches); got != 2 { + if got := g.launchCount(t); got != 2 { t.Fatalf("furrow was launched %d times for one session; want 2 (--version, status)", got) } } @@ -157,21 +240,38 @@ func TestFurrowIsAskedOncePerSession(t *testing.T) { // Close cuts a question furrow is still answering and waits for it: when Close // returns the process is gone. func TestClosingJoinsAFurrowThatIsStillAnswering(t *testing.T) { - _, _, pidfile := installGatedFurrow(t) - agent, _ := newTestAgent(t, &scriptedCompleter{}, nil) - var pid int - untilFurrow(t, "furrow's status call starting", func() bool { - raw, err := os.ReadFile(pidfile) - if err != nil { - return false - } - _, err = fmt.Sscan(string(raw), &pid) - return err == nil && pid > 0 - }) + g := installGatedFurrow(t) + agent := furrowSession(t, t.TempDir()) + pid := g.started(t) if err := agent.Close(); err != nil { t.Fatalf("close: %v", err) } - if err := syscall.Kill(pid, 0); err == nil { + 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) + waitForVerbs(t, first, len(furrow.Names())) +} From 05a03165b1461ce59af98da2d2f9ec67e83f24b9 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 01:51:19 -0400 Subject: [PATCH 05/11] perf(engine): arm furrow verbs late, ctx-aware detection, settle on the first request --- PERF.md | 11 +- .../1954-boot-without-furrow-detect.md | 19 +- internal/furrow/furrow.go | 125 +++++++------ internal/furrow/peek.go | 28 ++- internal/furrow/peek_test.go | 73 ++++++-- internal/session/agent.go | 2 +- internal/session/loop.go | 3 + internal/session/session.go | 15 +- internal/session/tools_workspace.go | 170 +++++++++++++----- internal/session/tools_workspace_test.go | 65 +++++-- 10 files changed, 354 insertions(+), 157 deletions(-) diff --git a/PERF.md b/PERF.md index bad6f937a7..6ab1bb369a 100644 --- a/PERF.md +++ b/PERF.md @@ -83,12 +83,13 @@ 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`) and the session refreshes it from a goroutine joined -at close. 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.45 s. The gate is +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. The gate is `TestBuildingASessionDoesNotWaitForFurrow`, which holds `furrow status` open and -counts launches instead of timing them. +asserts the build came back while that process was still alive; no clock decides it. ## What the gates check diff --git a/docs/changes/unreleased/1954-boot-without-furrow-detect.md b/docs/changes/unreleased/1954-boot-without-furrow-detect.md index 962ae5f40e..5bfc2c197c 100644 --- a/docs/changes/unreleased/1954-boot-without-furrow-detect.md +++ b/docs/changes/unreleased/1954-boot-without-furrow-detect.md @@ -5,12 +5,21 @@ 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; they now appear on the first belt built after furrow answers, so a session asked for its belt before then goes without them for that one belt. + - 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. -Each session now asks furrow from a goroutine it owns and joins at close, and the -belt reads the remembered answer. A cold engine on a real profile draws its first -frame in about 0.45 s instead of 3.87 s. An unwatched folder still gets none of -the four tools, and a cancelled question is never remembered as an answer. +Each session now asks furrow from a goroutine it owns and joins at close. A cold +engine on a real profile draws its first frame in about 0.5 s instead of 3.9 s. + +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/furrow.go b/internal/furrow/furrow.go index dbc70fc9cf..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,25 +169,14 @@ func lockAttach(root string) func() { return lock.Unlock } -// detectLocks is attachLocks for the question instead of the act: one lock per -// root, never removed, for the reason attachLocks gives. +// 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 ( - detectMu sync.Mutex - detectLocks = map[string]*sync.Mutex{} + detectingMu sync.Mutex + detecting = map[string]chan struct{}{} ) -func lockDetect(root string) func() { - detectMu.Lock() - lock, ok := detectLocks[root] - if !ok { - lock = &sync.Mutex{} - detectLocks[root] = lock - } - detectMu.Unlock() - lock.Lock() - return lock.Unlock -} - // 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 @@ -210,42 +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 - } - presenceMu.Unlock() - - // ONE ASKER PER FOLDER AT A TIME. Every session on a workspace starts its - // own background refresh ([Peek] is the door that reads without asking), so - // a window that opens several chats at once would otherwise run the same two - // execs once per chat. The second caller waits behind the first and reads - // the answer the first left in the cache. - release := lockDetect(root) - defer release() - presenceMu.Lock() - 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) - - // 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 for half a minute. - if ctx.Err() != nil { - return presence - } - 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 { @@ -370,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 index 02fddd0fbe..3bda859e38 100644 --- a/internal/furrow/peek.go +++ b/internal/furrow/peek.go @@ -6,16 +6,13 @@ import ( "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 door for a caller that must not wait on furrow — the -// engine building a session for the first frame — and it is the other half of -// [Detect], which is the door that asks. +// 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, and a caller that wants a newer one asks -// [Detect] from somewhere it is allowed to wait. -func Peek(root string) (presence Presence, fresh, known bool) { +// 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() @@ -27,14 +24,15 @@ func Peek(root string) (presence Presence, fresh, known bool) { } // 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. -func PeekTools(root string) (tools []bare.Tool, fresh, known bool) { - presence, fresh, known := Peek(root) +// 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, known + return nil, fresh } - workspace := &Workspace{root: absoluteRoot(root), binary: presence.Path} - return workspace.Tools(), fresh, known + return presence.workspace(root).Tools(), fresh } diff --git a/internal/furrow/peek_test.go b/internal/furrow/peek_test.go index 359d21fd3b..f705ca6f78 100644 --- a/internal/furrow/peek_test.go +++ b/internal/furrow/peek_test.go @@ -2,28 +2,32 @@ 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. +// 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 _, _, known := peek(root); known { + t.Fatal("peek knew a folder nobody had asked about") } - if tools, _, known := PeekTools(root); known || len(tools) != 0 { - t.Fatalf("PeekTools on an unasked folder = %d tools, known=%v; want none", len(tools), known) + 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) + 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) + t.Fatalf("peek after Detect = %+v fresh=%v known=%v; want the fresh available answer", presence, fresh, known) } - tools, _, _ := PeekTools(root) - if len(tools) != len(Names()) { - t.Fatalf("PeekTools on an attached folder = %d tools; want %d", len(tools), len(Names())) + 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())) } } @@ -31,8 +35,8 @@ func TestPeekToolsOnAnUnattachedFolderIsNothing(t *testing.T) { installFake(t, "unattached") root := t.TempDir() Detect(context.Background(), root) - if tools, _, known := PeekTools(root); !known || len(tools) != 0 { - t.Fatalf("PeekTools on an unwatched folder = %d tools, known=%v; want none from a known answer", len(tools), known) + 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) } } @@ -44,7 +48,48 @@ func TestACancelledDetectionIsNotRemembered(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() Detect(ctx, root) - if _, _, known := Peek(root); known { + 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 af5a626c30..3c7efa05a5 100644 --- a/internal/session/agent.go +++ b/internal/session/agent.go @@ -2513,7 +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.stopFurrow() + 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/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 6dc441c4f3..35daffdd76 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -2665,17 +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 - // furrowCtx, furrowStop and furrowJobs are the background question to furrow - // about this session's folder (tools_workspace.go). furrowAsking keeps it to - // one at a time and furrowStopped is Close's word that no more may begin; - // all of it is guarded by furrowMu, which is taken by nothing else and held - // across nothing, so it is safe to reach from the belt under any other lock. - furrowMu sync.Mutex - furrowCtx context.Context - furrowStop context.CancelFunc - furrowJobs sync.WaitGroup - furrowAsking bool - furrowStopped 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/tools_workspace.go b/internal/session/tools_workspace.go index 0d2b12e3dc..04f864921c 100644 --- a/internal/session/tools_workspace.go +++ b/internal/session/tools_workspace.go @@ -27,6 +27,8 @@ package session import ( "context" + "sync" + "time" "github.com/Agent-Field/codeaf/internal/exec/bare" "github.com/Agent-Field/codeaf/internal/furrow" @@ -41,66 +43,144 @@ import ( // `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, when there is none or it has gone stale, asks -// furrow from a goroutine this session owns ([Agent.askFurrow]) and hands over -// whatever it already knew. +// ([furrow.PeekTools]) and nothing more. // -// THE COST OF THAT IS ONE BELT, AND IT IS BOUNDED. A session that is asked for -// its belt before the first answer lands has no workspace verbs on it; the -// belt is rebuilt per turn, so the next one has them if the folder is -// attached. The refresh starts when the session is built (see [newAgent]), a -// person has to read the screen and type before the first turn, and an -// unattached folder — the common case — never changes at all. 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. +// 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 { - tools, fresh, _ := furrow.PeekTools(a.config.Workspace) - if !fresh { - a.askFurrow() - } + tools, _ := furrow.PeekTools(a.config.Workspace) return tools } -// askFurrow refreshes the remembered answer about this session's folder from a -// goroutine, once at a time, and returns at once. -// -// THE GOROUTINE IS THE SESSION'S AND CLOSE JOINS IT ([Agent.stopFurrow]). -// Cancelling the context kills a `furrow status` that is still running, and the -// join is what keeps a half-finished exec from outliving the state directory a -// caller is about to remove. A session already closing starts nothing: the -// flag and the group are touched under one lock, so the join cannot miss a -// refresh that begins after it. +// 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() { - a.furrowMu.Lock() - defer a.furrowMu.Unlock() - if a.furrowStopped || a.furrowAsking { + if _, fresh := furrow.PeekTools(a.config.Workspace); fresh { + return + } + a.furrow.ask(a.config.Workspace, a.armFurrowVerbs) +} + +// armFurrowVerbs puts the verbs on the belt once furrow has said the folder is +// attached. The belt of a model with no tools stays empty, as it was built. +func (a *Agent) armFurrowVerbs() { + if a.config.promptProfile().chat() { + return + } + if tools := a.workspaceTools(); len(tools) > 0 { + _, _ = 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 one background question 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 taken +// by nothing else and no caller holds another lock into it, so it is safe to +// reach from the belt or the request path under any other lock. +type furrowRefresher struct { + mu sync.Mutex + ctx context.Context + cancel context.CancelFunc + jobs sync.WaitGroup + running chan struct{} + stopped bool + settled bool +} + +func (r *furrowRefresher) ask(root string, landed func()) { + r.mu.Lock() + defer r.mu.Unlock() + if r.stopped || r.running != nil { return } - if a.furrowCtx == nil { - a.furrowCtx, a.furrowStop = context.WithCancel(context.Background()) + if r.ctx == nil { + r.ctx, r.cancel = context.WithCancel(context.Background()) } - a.furrowAsking = true - a.furrowJobs.Add(1) - ctx, root := a.furrowCtx, a.config.Workspace + running := make(chan struct{}) + r.running = running + r.jobs.Add(1) + ctx := r.ctx guard.Go("session/furrow-detect", func() { - defer a.furrowJobs.Done() + defer r.jobs.Done() defer func() { - a.furrowMu.Lock() - a.furrowAsking = false - a.furrowMu.Unlock() + r.mu.Lock() + r.running = nil + r.mu.Unlock() + close(running) }() 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() + } }) } -// stopFurrow ends the refresh and waits for it, on [Agent.stopLaneBeat]'s terms. -func (a *Agent) stopFurrow() { - a.furrowMu.Lock() - a.furrowStopped = true - stop := a.furrowStop - a.furrowMu.Unlock() - if stop != nil { - stop() +// idle is closed when no question is running: the channel of the one in flight, +// or an already-closed one. +func (r *furrowRefresher) idle() <-chan struct{} { + r.mu.Lock() + defer r.mu.Unlock() + if r.running == nil { + done := make(chan struct{}) + close(done) + return done + } + return r.running +} + +// settle waits up to max for the question in flight, once per session. +func (r *furrowRefresher) settle(max time.Duration) { + r.mu.Lock() + if r.settled || r.running == nil { + r.mu.Unlock() + return + } + r.settled = true + running := r.running + r.mu.Unlock() + timer := time.NewTimer(max) + defer timer.Stop() + select { + case <-running: + case <-timer.C: + } +} + +// close cancels the question and waits for it. +func (r *furrowRefresher) close() { + r.mu.Lock() + r.stopped = true + cancel := r.cancel + r.mu.Unlock() + if cancel != nil { + cancel() } - a.furrowJobs.Wait() + r.jobs.Wait() } diff --git a/internal/session/tools_workspace_test.go b/internal/session/tools_workspace_test.go index b4205997cd..7a98b68fad 100644 --- a/internal/session/tools_workspace_test.go +++ b/internal/session/tools_workspace_test.go @@ -197,7 +197,10 @@ func TestWatchedFolderVerbsReachTheModel(t *testing.T) { agent := furrowSession(t, t.TempDir()) g.started(t) g.release(t) - waitForVerbs(t, agent, len(furrow.Names())) + 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) @@ -209,15 +212,14 @@ func TestWatchedFolderVerbsReachTheModel(t *testing.T) { } } -// waitForVerbs waits for the background answer to land on the belt. -func waitForVerbs(t *testing.T, a *Agent, want int) { +// 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() - deadline := time.Now().Add(furrowTestSafety) - for len(workspaceVerbs(carried(a))) < want { - if time.Now().After(deadline) { - t.Fatalf("the belt never carried the workspace verbs; has %v", workspaceVerbs(carried(a))) - } - time.Sleep(5 * time.Millisecond) + select { + case <-a.furrow.idle(): + case <-time.After(furrowTestSafety): + t.Fatal("furrow's question never ended") } } @@ -228,7 +230,7 @@ func TestFurrowIsAskedOncePerSession(t *testing.T) { agent := furrowSession(t, t.TempDir()) g.started(t) g.release(t) - waitForVerbs(t, agent, len(furrow.Names())) + answered(t, agent) for i := 0; i < 5; i++ { _ = agent.belt() } @@ -273,5 +275,46 @@ func TestClosingOneSessionDoesNotWaitBehindAnothersFurrow(t *testing.T) { t.Fatal("the first session's furrow was cut by the second session closing") } g.release(t) - waitForVerbs(t, first, len(furrow.Names())) + 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) } From 2c588121c52f9e4d2295b6a9944fdf380ec22eb4 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 02:27:07 -0400 Subject: [PATCH 06/11] fix(session): arm furrow verbs under the session lock and only for the folder asked about --- internal/session/tools_workspace.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/internal/session/tools_workspace.go b/internal/session/tools_workspace.go index 04f864921c..c516c192f3 100644 --- a/internal/session/tools_workspace.go +++ b/internal/session/tools_workspace.go @@ -77,12 +77,20 @@ func (a *Agent) askFurrow() { } // armFurrowVerbs puts the verbs on the belt once furrow has said the folder is -// attached. The belt of a model with no tools stays empty, as it was built. -func (a *Agent) armFurrowVerbs() { - if a.config.promptProfile().chat() { +// attached. It runs on the refresher's goroutine, so it reads the session's +// configuration under the session lock like any other background reader, and 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) { + a.mu.Lock() + chat := a.config.promptProfile().chat() + same := a.config.Workspace == root + a.mu.Unlock() + if chat || !same { return } - if tools := a.workspaceTools(); len(tools) > 0 { + if tools, _ := furrow.PeekTools(root); len(tools) > 0 { _, _ = a.armFamily(tools) } } @@ -111,7 +119,7 @@ type furrowRefresher struct { settled bool } -func (r *furrowRefresher) ask(root string, landed func()) { +func (r *furrowRefresher) ask(root string, landed func(root string)) { r.mu.Lock() defer r.mu.Unlock() if r.stopped || r.running != nil { @@ -137,7 +145,7 @@ func (r *furrowRefresher) ask(root string, landed func()) { // caller released by it sees them on the belt. A cancelled question // answered nothing and arms nothing. if ctx.Err() == nil { - landed() + landed(root) } }) } From fd8851bb9b08c483a7f74d9b24633da77323ff01 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 02:40:32 -0400 Subject: [PATCH 07/11] fix(session): read the session config from a build-time copy in the furrow arming goroutine --- internal/session/tools_workspace.go | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/internal/session/tools_workspace.go b/internal/session/tools_workspace.go index c516c192f3..9b67a2cc18 100644 --- a/internal/session/tools_workspace.go +++ b/internal/session/tools_workspace.go @@ -73,21 +73,26 @@ func (a *Agent) askFurrow() { if _, fresh := furrow.PeekTools(a.config.Workspace); fresh { return } - a.furrow.ask(a.config.Workspace, a.armFurrowVerbs) + 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 reads the session's -// configuration under the session lock like any other background reader, and 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) { +// 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) { + if built.promptProfile().chat() { + return + } a.mu.Lock() - chat := a.config.promptProfile().chat() same := a.config.Workspace == root a.mu.Unlock() - if chat || !same { + if !same { return } if tools, _ := furrow.PeekTools(root); len(tools) > 0 { From 649e53eda5ee1b00bb06ca9083507d2d7aa8ecfc Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 02:54:08 -0400 Subject: [PATCH 08/11] fix(session): decide there is nothing to arm before touching the session config; test writes take the lock --- internal/session/task_divide_wip_test.go | 4 ++++ internal/session/tools_workspace.go | 10 +++++----- 2 files changed, 9 insertions(+), 5 deletions(-) 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_workspace.go b/internal/session/tools_workspace.go index 9b67a2cc18..c519920ed4 100644 --- a/internal/session/tools_workspace.go +++ b/internal/session/tools_workspace.go @@ -86,16 +86,16 @@ func (a *Agent) askFurrow() { // 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) { - if built.promptProfile().chat() { + // 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 { - return - } - if tools, _ := furrow.PeekTools(root); len(tools) > 0 { + if same { _, _ = a.armFamily(tools) } } From 13b62b79afd84a191b781828deb48c3f6d4d128d Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 08:50:23 -0400 Subject: [PATCH 09/11] test(session): anchoring must ask furrow about the new folder --- internal/session/anchor_furrow_test.go | 69 ++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 internal/session/anchor_furrow_test.go 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())) + } +} From a7883136de60ce99aa95467be17ea248a4075849 Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 08:51:13 -0400 Subject: [PATCH 10/11] fix(session): anchoring asks furrow about the new folder from the background --- internal/session/tools_anchor_workspace.go | 5 +++ internal/session/tools_workspace.go | 49 +++++++++++++--------- 2 files changed, 35 insertions(+), 19 deletions(-) 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 c519920ed4..27ce7c0f81 100644 --- a/internal/session/tools_workspace.go +++ b/internal/session/tools_workspace.go @@ -105,45 +105,56 @@ func (a *Agent) armFurrowVerbs(root string, built Config) { // first time. Every later request reads the belt as it stands. func (a *Agent) settleFurrow() { a.furrow.settle(firstBeltWait) } -// furrowRefresher owns the one background question a session asks furrow, on the +// 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 taken -// by nothing else and no caller holds another lock into it, so it is safe to -// reach from the belt or the request path under any other lock. +// [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 - running chan struct{} + 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.running != nil { + if r.stopped || r.asking[root] { return } if r.ctx == nil { r.ctx, r.cancel = context.WithCancel(context.Background()) + r.asking = map[string]bool{} } - running := make(chan struct{}) - r.running = running + 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() - r.running = nil + delete(r.asking, root) + if len(r.asking) == 0 { + close(busy) + } r.mu.Unlock() - close(running) }() furrow.Detect(ctx, root) // The verbs are armed BEFORE the question is reported finished, so a @@ -155,38 +166,38 @@ func (r *furrowRefresher) ask(root string, landed func(root string)) { }) } -// idle is closed when no question is running: the channel of the one in flight, -// or an already-closed one. +// 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 r.running == nil { + if len(r.asking) == 0 { done := make(chan struct{}) close(done) return done } - return r.running + return r.busy } -// settle waits up to max for the question in flight, once per session. +// 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 || r.running == nil { + if r.settled || len(r.asking) == 0 { r.mu.Unlock() return } r.settled = true - running := r.running + busy := r.busy r.mu.Unlock() timer := time.NewTimer(max) defer timer.Stop() select { - case <-running: + case <-busy: case <-timer.C: } } -// close cancels the question and waits for it. +// close cancels the questions and waits for them. func (r *furrowRefresher) close() { r.mu.Lock() r.stopped = true From 2d0db483312021f50bb2b86ebda97e32db1c735b Mon Sep 17 00:00:00 2001 From: santoshkumarradha Date: Mon, 26 Oct 2026 09:38:10 -0400 Subject: [PATCH 11/11] docs: state the first-frame gain against the furrow that was embedded then --- PERF.md | 4 +++- .../changes/unreleased/1954-boot-without-furrow-detect.md | 8 +++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/PERF.md b/PERF.md index 068fa25e36..c7ed2cb4c7 100644 --- a/PERF.md +++ b/PERF.md @@ -87,7 +87,9 @@ 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. The gate is +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. diff --git a/docs/changes/unreleased/1954-boot-without-furrow-detect.md b/docs/changes/unreleased/1954-boot-without-furrow-detect.md index 5bfc2c197c..657f352fe8 100644 --- a/docs/changes/unreleased/1954-boot-without-furrow-detect.md +++ b/docs/changes/unreleased/1954-boot-without-furrow-detect.md @@ -10,9 +10,11 @@ invalidates: --- 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. -Each session now asks furrow from a goroutine it owns and joins at close. A cold -engine on a real profile draws its first frame in about 0.5 s instead of 3.9 s. +`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