diff --git a/cmd/codeaf/app.go b/cmd/codeaf/app.go index 57c0445c79..b13ed7d377 100644 --- a/cmd/codeaf/app.go +++ b/cmd/codeaf/app.go @@ -25,8 +25,7 @@ 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. Open -// views keep theirs only for the [appViewHolds] most recently used. +// without an open view is let go and rebuilt transparently on demand. import ( "cmp" @@ -76,21 +75,12 @@ 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 while -// it is among the [appViewHolds] most recently used. +// it again. An explicitly open view keeps the conversation and its hold. 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 @@ -789,8 +779,7 @@ 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, so - // only views beyond the [appViewHolds] most recently used are let go. + // it would let its next read immediately take back the released hold. viewOpen bool // crewHeld keeps an established crew hold due after its engine closes. crewHeld bool @@ -4754,8 +4743,7 @@ func (b *appBackend) sweep() { } // sweepOnce is one look at the idle rule. A conversation is kept while a turn -// runs or a question waits for the person, whatever the clock says, and so is -// an open view among the [appViewHolds] most recently used. +// runs, a question waits for the person or a view stays open, whatever the clock says. func (b *appBackend) sweepOnce() { var syncing bool b.locked(func() { syncing = b.drives != nil }) @@ -4763,39 +4751,22 @@ func (b *appBackend) sweepOnce() { _, _ = b.SyncStatus() } now := b.now() - type candidate struct { - agent *appAgent - view bool - } - var idle []candidate + var idle []*appAgent b.locked(func() { - var views []*appAgent for _, a := range b.agents { - 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}) + if live := a.live; live != nil && !a.viewOpen && !live.running && now.Sub(live.lastUse) >= appAgentIdle { + idle = append(idle, a) } } }) - for _, c := range idle { - b.sweepAgent(c.agent, now, c.view) + for _, a := range idle { + b.sweepAgent(a, now) } } // sweepAgent lets one idle conversation go, unless something took its gate or -// 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) { +// woke it since the look. +func (b *appBackend) sweepAgent(a *appAgent, now time.Time) { if !a.gate.TryLock() { return } @@ -4804,7 +4775,7 @@ func (b *appBackend) sweepAgent(a *appAgent, now time.Time, view bool) { keep := true b.locked(func() { live = a.live - keep = live == nil || (a.viewOpen && !view) || live.running || now.Sub(live.lastUse) < appAgentIdle + keep = live == nil || a.viewOpen || 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 deleted file mode 100644 index c9d1117c64..0000000000 --- a/cmd/codeaf/app_view_holds_test.go +++ /dev/null @@ -1,102 +0,0 @@ -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 deleted file mode 100644 index b4a4fbb493..0000000000 --- a/docs/changes/unreleased/2472-idle-view-holds-bounded.md +++ /dev/null @@ -1,14 +0,0 @@ ---- -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/docs/changes/unreleased/2473-revert-view-holds.md b/docs/changes/unreleased/2473-revert-view-holds.md new file mode 100644 index 0000000000..e3df482d15 --- /dev/null +++ b/docs/changes/unreleased/2473-revert-view-holds.md @@ -0,0 +1,11 @@ +--- +kind: fixed +title: Desktop chats on screen stay held again, so another process cannot run a turn in one unseen +pr: 2473 +surface: [app, engine, docs] +invalidates: + - "After #2472 the desktop engine let an open chat go once it was idle and outside the three most recently used, so a terminal codeaf on the same home could run a turn in a chat the window still showed without the window seeing it. An open view keeps its conversation and hold until it is closed again, as before #2472." +--- + +This reverts #2472. Per-chat file handles held by the engine (#2471) are +unbounded again until a fix keeps both the hold and a bound. diff --git a/internal/manual/chat/desktop-sync.md b/internal/manual/chat/desktop-sync.md index e9e3c7b722..e8566c2d47 100644 --- a/internal/manual/chat/desktop-sync.md +++ b/internal/manual/chat/desktop-sync.md @@ -194,13 +194,10 @@ 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, 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 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 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.