diff --git a/cmd/codeaf/app.go b/cmd/codeaf/app.go index b13ed7d377..57c0445c79 100644 --- a/cmd/codeaf/app.go +++ b/cmd/codeaf/app.go @@ -25,7 +25,8 @@ package main // idle conversations stay as indexed records, crew members included. Viewing, // sending or waking opens one through the same per-agent gate. Listing agents, // tasks and crews reads the disk. A conversation left alone for [appAgentIdle] -// without an open view is let go and rebuilt transparently on demand. +// without an open view is let go and rebuilt transparently on demand. Open +// views keep theirs only for the [appViewHolds] most recently used. import ( "cmp" @@ -75,12 +76,21 @@ import ( // appAgentIdle is how long a conversation the app holds may sit with no turn // running, no question waiting and no open view before it is let go. Its // journal stays on disk, its state still reads idle, and the next send opens -// it again. An explicitly open view keeps the conversation and its hold. +// it again. An explicitly open view keeps the conversation and its hold while +// it is among the [appViewHolds] most recently used. const appAgentIdle = 10 * time.Minute // appSweepEvery is how often the idle rule above is asked. const appSweepEvery = 30 * time.Second +// appViewHolds is how many open views keep their idle conversation held: the +// most recently used ones. A window opens every chat it shows and never says +// when it stops showing one, so without a bound each chat shown in a session +// kept its journal, presence watch and timers until the app quit (#2471). An +// older idle view is let go like a background chat; its next read or send +// opens it again. +const appViewHolds = 3 + // appCrewPoll is how often the teams file is looked at for crew.changed, once // the app has asked for the crews at all. It is one stat when nothing moved. const appCrewPoll = 2 * time.Second @@ -779,7 +789,8 @@ type appAgent struct { localNotices map[string]bool windowMoved bool // viewOpen preserves the person's open view through idle sweeps. Retiring - // it would let its next read immediately take back the released hold. + // it would let its next read immediately take back the released hold, so + // only views beyond the [appViewHolds] most recently used are let go. viewOpen bool // crewHeld keeps an established crew hold due after its engine closes. crewHeld bool @@ -4743,7 +4754,8 @@ func (b *appBackend) sweep() { } // sweepOnce is one look at the idle rule. A conversation is kept while a turn -// runs, a question waits for the person or a view stays open, whatever the clock says. +// runs or a question waits for the person, whatever the clock says, and so is +// an open view among the [appViewHolds] most recently used. func (b *appBackend) sweepOnce() { var syncing bool b.locked(func() { syncing = b.drives != nil }) @@ -4751,22 +4763,39 @@ func (b *appBackend) sweepOnce() { _, _ = b.SyncStatus() } now := b.now() - var idle []*appAgent + type candidate struct { + agent *appAgent + view bool + } + var idle []candidate b.locked(func() { + var views []*appAgent for _, a := range b.agents { - if live := a.live; live != nil && !a.viewOpen && !live.running && now.Sub(live.lastUse) >= appAgentIdle { - idle = append(idle, a) + live := a.live + switch { + case live == nil: + case a.viewOpen: + views = append(views, a) + case !live.running && now.Sub(live.lastUse) >= appAgentIdle: + idle = append(idle, candidate{agent: a}) + } + } + sort.Slice(views, func(i, j int) bool { return views[i].live.lastUse.After(views[j].live.lastUse) }) + for _, a := range views[min(len(views), appViewHolds):] { + if !a.live.running && now.Sub(a.live.lastUse) >= appAgentIdle { + idle = append(idle, candidate{agent: a, view: true}) } } }) - for _, a := range idle { - b.sweepAgent(a, now) + for _, c := range idle { + b.sweepAgent(c.agent, now, c.view) } } // sweepAgent lets one idle conversation go, unless something took its gate or -// woke it since the look. -func (b *appBackend) sweepAgent(a *appAgent, now time.Time) { +// woke it since the look. A view is let go only when view says it fell outside +// the most recently used holds. +func (b *appBackend) sweepAgent(a *appAgent, now time.Time, view bool) { if !a.gate.TryLock() { return } @@ -4775,7 +4804,7 @@ func (b *appBackend) sweepAgent(a *appAgent, now time.Time) { keep := true b.locked(func() { live = a.live - keep = live == nil || a.viewOpen || live.running || now.Sub(live.lastUse) < appAgentIdle + keep = live == nil || (a.viewOpen && !view) || live.running || now.Sub(live.lastUse) < appAgentIdle }) // A member of an open crew stays held while the crew is open. if !keep && !b.liveNeedsYou(live) && !b.crewKeeps(a.view.ID) && !b.serviceKeeps(a.view.ID) { diff --git a/cmd/codeaf/app_view_holds_test.go b/cmd/codeaf/app_view_holds_test.go new file mode 100644 index 0000000000..c9d1117c64 --- /dev/null +++ b/cmd/codeaf/app_view_holds_test.go @@ -0,0 +1,102 @@ +package main + +import ( + "fmt" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/appwire" +) + +// AN OPEN VIEW KEEPS ITS CONVERSATION ONLY AMONG THE MOST RECENTLY USED. A +// window opens every chat it shows and never says when it stops showing one, +// so each chat shown in a session used to stay held, with its journal and +// presence watch, until the app quit (#2471). Past the idle rule, only the +// [appViewHolds] most recently used views keep their hold, however many chats +// were shown. +func TestAppIdleViewsBeyondTheRecentHoldsAreLetGo(t *testing.T) { + var mu sync.Mutex + var opens, releases int + open := func(ask appOpenAsk) (*appConn, error) { + mu.Lock() + opens++ + mu.Unlock() + return &appConn{ + Conv: &fakeConversation{}, Workspace: ask.Workspace, Transcript: ask.Transcript, + Release: func() { mu.Lock(); releases++; mu.Unlock() }, + }, nil + } + backend := newFakeBackend(newAppRecorder(), open) + defer backend.Close() + dir := t.TempDir() + minted := 0 + backend.mint = func(string) (string, error) { + minted++ + return filepath.Join(dir, fmt.Sprintf("chat%02d", minted), "transcript.jsonl"), nil + } + clock := time.Now() + backend.now = func() time.Time { return clock } + + var ids []string + view := func() { + t.Helper() + clock = clock.Add(time.Minute) + agent, err := mustCreate(t, backend, appwire.CreateAsk{Workspace: t.TempDir()}) + if err != nil { + t.Fatal(err) + } + if _, err := backend.OpenAgent(agent.ID); err != nil { + t.Fatal(err) + } + ids = append(ids, agent.ID) + } + held := func() map[string]bool { + out := map[string]bool{} + backend.locked(func() { + for id, a := range backend.agents { + if a.live != nil { + out[id] = true + } + } + }) + return out + } + + const chats = appViewHolds + 4 + for range chats { + view() + } + backend.sweepOnce() + if got := len(held()); got != chats { + t.Fatalf("within the idle rule %d of %d views are held", got, chats) + } + + clock = clock.Add(appAgentIdle + time.Second) + backend.sweepOnce() + kept := held() + if len(kept) != appViewHolds { + t.Fatalf("after the idle rule %d views are held, want %d", len(kept), appViewHolds) + } + for i, id := range ids { + if want := i >= len(ids)-appViewHolds; kept[id] != want { + t.Fatalf("view %d held = %v, want only the %d most recently used", i, kept[id], appViewHolds) + } + } + + // Opening more chats and leaving them idle never grows the holds. + for round := 0; round < 3; round++ { + view() + clock = clock.Add(appAgentIdle + time.Second) + backend.sweepOnce() + if got := len(held()); got != appViewHolds { + t.Fatalf("round %d: %d views held, want %d", round, got, appViewHolds) + } + } + mu.Lock() + defer mu.Unlock() + if releases != opens-appViewHolds { + t.Fatalf("opened %d, released %d; want all but %d released", opens, releases, appViewHolds) + } +} diff --git a/docs/changes/unreleased/2472-idle-view-holds-bounded.md b/docs/changes/unreleased/2472-idle-view-holds-bounded.md new file mode 100644 index 0000000000..b4a4fbb493 --- /dev/null +++ b/docs/changes/unreleased/2472-idle-view-holds-bounded.md @@ -0,0 +1,14 @@ +--- +kind: fixed +title: Desktop chats shown in a session no longer stay open in the engine until the app quits +pr: 2472 +surface: [app, engine, docs] +invalidates: + - "A chat opened for viewing in the desktop app kept its conversation and relay hold until it was explicitly closed. Only the three most recently used open chats keep theirs now; an older open chat idle for ten minutes is let go like a background chat and opens again on its next read or send." +--- + +The window opens every chat it shows and does not say when it stops showing +one, so each chat shown in a session stayed held with its journal, its presence +watch (about six file handles on macOS) and its timers. The idle sweep now keeps +at most three open views held; running turns, waiting questions, crews, kept +services and explicit closes are unchanged. diff --git a/internal/manual/chat/desktop-sync.md b/internal/manual/chat/desktop-sync.md index e8566c2d47..e9e3c7b722 100644 --- a/internal/manual/chat/desktop-sync.md +++ b/internal/manual/chat/desktop-sync.md @@ -194,10 +194,13 @@ once if that computer saved again first. ## Does an idle desktop chat keep its hold on the relay -A chat opened for viewing keeps its conversation and relay hold until it is -explicitly closed, including while its tab is hidden. Heartbeats renew the same -hold without releasing it or moving its fence. Reading the chat does not cause -ownership changes or repeat unchanged chat rows. +A chat opened for viewing keeps its conversation and relay hold, including while +its tab is hidden, as long as it is one of the three most recently used open +chats. Heartbeats renew the same hold without releasing it or moving its fence. +Reading the chat does not cause ownership changes or repeat unchanged chat rows. +An older open chat left idle for ten minutes is let go like a background chat, +which frees its files and timers; using it again opens it through the normal +ownership check. Closing a chat lets it go at once. A conversation opened only for background work is let go after ten idle minutes; the sweep checks every 30 seconds. It stays released until work needs it again.