From 4940eda352f0241dbc957102fd069fca6e60b3ac Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 22:54:27 +0200 Subject: [PATCH] fix(sessions): stop counting session startup as work in progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Startup progress rides the per-session activity channel with an "active" phase, because a startup phase really is in progress. But isSessionActive() treated any active activity as work, so a session that was merely *opening* enabled "Stop Active Work", disabled "Reload from disk" with the misleading "Stop current session activity before reloading" tooltip, showed the row's active-work indicator, and — for any caller that hands a startup activity to WorkspaceActivityService — reported the whole workspace as busy. Selecting an archived, read-only session reported active work while it opened. Starting is not working. publishStartupProgress now marks its reports with a new optional SessionActivity.startup field, and isSessionActive() does not count a marked activity. Every affected consumer — the session list, the core actions, the app's activity-transition handling, and the server's workspace aggregation — reads that one helper, so the correction lands in all of them at once. The marker is a new field rather than a new phase on purpose: six readers test phase === "active" directly, including the pending row's "creating · " prefix, the chat dock's active styling, and the daemon's own heartbeat re-publication. It can only ever remove the activity-phase reason for being active, so streaming, bash, compaction, and queued prompts still report as active through the status even while a startup report is the latest activity. The chat dock still shows the startup text; this changes what counts as work, not what is shown. The browser's own pending-create row keeps its previous appearance: it borrows only the daemon's phase text and drops the marker, since that row stands for a create the user is waiting on rather than a session the daemon is opening. --- src/client/src/api/parsers.test.ts | 9 ++++ src/client/src/api/parsers.ts | 13 +++++ src/client/src/components/SessionList.test.ts | 8 ++++ .../sessionController.startupProgress.test.ts | 47 +++++++++++++++++++ .../src/controllers/sessionController.ts | 20 +++++++- src/client/src/plugins/registry.test.ts | 19 ++++++++ src/client/src/sessionSocket.test.ts | 10 ++++ .../activity/workspaceActivityService.test.ts | 13 +++++ .../piSessionService.startupProgress.test.ts | 29 ++++++++++++ src/server/sessions/piSessionService.ts | 7 ++- src/shared/activity.test.ts | 25 +++++++++- src/shared/activity.ts | 10 +++- src/shared/apiTypes.ts | 8 ++++ 13 files changed, 214 insertions(+), 4 deletions(-) diff --git a/src/client/src/api/parsers.test.ts b/src/client/src/api/parsers.test.ts index a05ec35..10ccf61 100644 --- a/src/client/src/api/parsers.test.ts +++ b/src/client/src/api/parsers.test.ts @@ -305,6 +305,15 @@ describe("API parsers", () => { }); }); + it("carries the startup marker so an opening session is not mistaken for a working one", () => { + const activity = { sessionId: "session-1", phase: "active", label: "Opening session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z", startup: true }; + + expect(parseSessionStartupProgressEvent({ type: "session.startup", activity })).toEqual({ type: "session.startup", activity }); + // A malformed marker is dropped like any other malformed field rather than + // being coerced into "this is startup" or "this is work". + expect(() => parseSessionStartupProgressEvent({ type: "session.startup", activity: { ...activity, startup: "yes" } })).toThrow("Expected optional boolean field: startup"); + }); + it("rejects session startup progress that cannot be routed or rendered honestly", () => { const activity = { sessionId: "session-1", phase: "active", label: "Creating session", at: "2026-07-20T00:00:01.000Z" }; diff --git a/src/client/src/api/parsers.ts b/src/client/src/api/parsers.ts index 4621037..c9d54e0 100644 --- a/src/client/src/api/parsers.ts +++ b/src/client/src/api/parsers.ts @@ -300,9 +300,22 @@ function parseSessionActivity(value: unknown): SessionActivity { label: requireNonEmptyString(record, "label"), ...optionalField("detail", optionalString(record, "detail")), at: requireNonEmptyString(record, "at"), + ...optionalField("startup", optionalActivityStartupMarker(record)), }; } +/** + * The startup marker says the activity is a session opening rather than work in + * progress, which decides whether "Stop Active Work" is offered and whether a + * reload is blocked. A malformed marker is rejected rather than guessed at. + */ +function optionalActivityStartupMarker(record: Record): boolean | undefined { + const value = record["startup"]; + if (value === undefined) return undefined; + if (typeof value !== "boolean") throw new Error("Expected optional boolean field: startup"); + return value; +} + function requireSessionActivityPhase(record: Record, key: string): SessionActivity["phase"] { const value = requireString(record, key); if (value !== "active" && value !== "idle" && value !== "error") throw new Error(`Expected session activity phase field: ${key}`); diff --git a/src/client/src/components/SessionList.test.ts b/src/client/src/components/SessionList.test.ts index dd3953e..fcda1d7 100644 --- a/src/client/src/components/SessionList.test.ts +++ b/src/client/src/components/SessionList.test.ts @@ -16,6 +16,14 @@ describe("sessionRowActivityKind", () => { expect(sessionRowActivityKind(session("s"), { ...idle, isStreaming: true }, undefined, false)).toBe("session"); }); + it("shows no active-work indicator for a session that is only starting up", () => { + const startup = { sessionId: "s", phase: "active" as const, label: "Opening session", detail: "Starting the Pi session", at: "now", startup: true }; + + expect(sessionRowActivityKind(session("s"), idle, startup, false)).toBeUndefined(); + // Ordinary activity is work and keeps its indicator. + expect(sessionRowActivityKind(session("s"), idle, { sessionId: "s", phase: "active", label: "running tool", at: "now" }, false)).toBe("session"); + }); + it("reports unread only while the session is idle", () => { expect(sessionRowActivityKind(session("s"), idle, undefined, false, true)).toBe("unread"); expect(sessionRowActivityKind(session("s"), { ...idle, isStreaming: true }, undefined, false, true)).toBe("session"); diff --git a/src/client/src/controllers/sessionController.startupProgress.test.ts b/src/client/src/controllers/sessionController.startupProgress.test.ts index 8b59c8d..74e0e62 100644 --- a/src/client/src/controllers/sessionController.startupProgress.test.ts +++ b/src/client/src/controllers/sessionController.startupProgress.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { isSessionActive } from "../../../shared/activity"; import { initialAppState } from "../appState"; import { SessionController } from "./sessionController"; import { defaultApi, deferred, emptyPage, FakeSocket, oldSession, runPendingAnimationFrames, sessionLookupId, status, workspace, type AppState, type SessionActivity, type SessionInfo } from "./sessionController.testSupport"; @@ -10,6 +11,7 @@ function startupActivity(patch: Partial = {}): SessionActivity label: "Creating session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z", + startup: true, ...patch, }; } @@ -76,6 +78,28 @@ describe("SessionController session startup progress", () => { await start; }); + it("keeps a pending create row's own appearance while it borrows the daemon's phase text", async () => { + const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } }; + const { controller, startRequest } = pendingStartController(state); + + const start = controller.startSession(); + const temporaryId = state.current.selectedSession?.id; + if (temporaryId === undefined) throw new Error("Expected temporary session id"); + const beforeStartupProgress = isSessionActive(state.current.status, state.current.activity); + + controller.applyGlobalEvent({ type: "session.startup", startupToken: temporaryId, activity: startupActivity() }); + runPendingAnimationFrames(); + + // This row is the browser's own pending create, not a session being opened. + // Substituting the daemon's phase text into it must not silently change what + // the row reports about itself, or its indicator would blink off mid-create. + expect(state.current.activity?.phase).toBe("active"); + expect(isSessionActive(state.current.status, state.current.activity)).toBe(beforeStartupProgress); + + startRequest.resolve({ ...oldSession, id: "backend-session", path: "/tmp/backend-session.jsonl" }); + await start; + }); + it("restores the generic wording when the daemon has nothing left to attribute", async () => { const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } }; const { controller, startRequest } = pendingStartController(state); @@ -136,6 +160,29 @@ describe("SessionController session startup progress", () => { expect(state.activity).toMatchObject({ sessionId: oldSession.id, label: "Opening session", detail: "Starting the Pi session" }); }); + it("shows an archived session's opening progress without reporting it as active work", () => { + const archived = { ...oldSession, id: "archived-session", archived: true, archivedAt: "2026-05-16T00:00:00.000Z" }; + let state: AppState = { ...initialAppState(), selectedSession: archived, sessions: [archived] }; + const controller = new SessionController( + () => state, + (patch) => { state = { ...state, ...patch }; }, + () => undefined, + undefined, + { socket: new FakeSocket() }, + ); + + controller.applyGlobalEvent({ + type: "session.startup", + activity: startupActivity({ sessionId: archived.id, label: "Opening session" }), + }); + runPendingAnimationFrames(); + + // A read-only session cannot be worked on, so opening one must not make it + // look busy — while the text a waiting user reads still arrives. + expect(state.activity).toMatchObject({ sessionId: archived.id, label: "Opening session", detail: "Starting the Pi session" }); + expect(isSessionActive(state.status, state.activity)).toBe(false); + }); + it("gives an existing session's startup its own row rather than a pending start in the same workspace", async () => { const existing = { ...oldSession, id: "existing-session", cwd: workspace.path }; const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [existing] } }; diff --git a/src/client/src/controllers/sessionController.ts b/src/client/src/controllers/sessionController.ts index 46fbaa0..76ebcd9 100644 --- a/src/client/src/controllers/sessionController.ts +++ b/src/client/src/controllers/sessionController.ts @@ -1328,7 +1328,7 @@ export class SessionController { // creation request that has not returned yet. this.queueActivityUpdate(event.activity.phase === "idle" ? creatingPendingSessionActivity(pending.tempId, pending.queuedSends.length) - : { ...event.activity, sessionId: pending.tempId }); + : pendingStartActivity(event.activity, pending.tempId)); } private schedulePendingFlush(): void { @@ -1429,6 +1429,24 @@ function isClientPendingStartSessionInfo(session: SessionInfo | undefined): sess return session !== undefined && "clientPendingStart" in session && session.clientPendingStart === true; } +/** + * Re-label a startup report onto the browser's own pending create row. + * + * The daemon's startup marker is dropped: this row stands for a create the user + * asked for and is waiting on, which the browser has always reported as active + * work in progress, rather than for a session the daemon is opening. Only the + * phase text is borrowed, so the row keeps its own `creating · ` appearance. + */ +function pendingStartActivity(activity: SessionActivity, sessionId: string): SessionActivity { + return { + sessionId, + phase: activity.phase, + label: activity.label, + ...(activity.detail === undefined ? {} : { detail: activity.detail }), + at: activity.at, + }; +} + function creatingPendingSessionActivity(sessionId: string, queuedCount = 0): SessionActivity { return { sessionId, diff --git a/src/client/src/plugins/registry.test.ts b/src/client/src/plugins/registry.test.ts index bdb1de3..7c74fda 100644 --- a/src/client/src/plugins/registry.test.ts +++ b/src/client/src/plugins/registry.test.ts @@ -249,6 +249,25 @@ describe("PluginRegistry", () => { expect(busy.find((action) => action.id === "core:session.reload")?.enabled).toBe(false); }); + it("treats a session that is only starting up as having no work to stop or block", () => { + const registry = new PluginRegistry(); + registry.register({ id: "core", plugin: corePlugin }); + const reloadRuntime = { local: { machineId: "local", ok: true as const, checkedAt: "now", capabilities: [PI_WEB_CAPABILITIES.sessionsReload, PI_WEB_CAPABILITIES.sessionsPersistedState] } }; + const startupActivity = { sessionId: "s1", phase: "active" as const, label: "Opening session", detail: "Starting the Pi session", at: "now", startup: true }; + + const opening = registry.getActions(createContext({ selectedSession: testSession({ persisted: true }), status: testStatus({ persisted: true }), activity: startupActivity, machineRuntimes: reloadRuntime }).context); + + // Nothing is being worked on, so there is nothing to stop and no reason to + // block a reload with "Stop current session activity before reloading". + expect(opening.find((action) => action.id === "core:session.stop")?.enabled).toBe(false); + expect(opening.find((action) => action.id === "core:session.reload")?.enabled).toBe(true); + + // Real work is still real work, whatever else the session is doing. + const working = registry.getActions(createContext({ selectedSession: testSession({ persisted: true }), status: testStatus({ persisted: true, isStreaming: true }), activity: startupActivity, machineRuntimes: reloadRuntime }).context); + expect(working.find((action) => action.id === "core:session.stop")?.enabled).toBe(true); + expect(working.find((action) => action.id === "core:session.reload")?.enabled).toBe(false); + }); + it("routes session reload through the runtime context", () => { const registry = new PluginRegistry(); registry.register({ id: "core", plugin: corePlugin }); diff --git a/src/client/src/sessionSocket.test.ts b/src/client/src/sessionSocket.test.ts index 0337316..c96cdd1 100644 --- a/src/client/src/sessionSocket.test.ts +++ b/src/client/src/sessionSocket.test.ts @@ -90,6 +90,16 @@ describe("notification socket guards", () => { })).toBeUndefined(); }); + it("carries the startup marker through the socket boundary, marker and all", () => { + const activity = { sessionId: "session-1", phase: "active", label: "Opening session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z", startup: true }; + + // The marker is what stops an opening session being treated as a working + // one, so dropping it in transit would restore the defect for every frame, + // including those relayed from a remote machine. + expect(parseRealtimeSocketEvent({ type: "session.startup", activity })).toMatchObject({ type: "session.startup", activity: { startup: true } }); + expect(parseRealtimeSocketEvent({ type: "session.startup", activity: { ...activity, startup: 1 } })).toBeUndefined(); + }); + it("accepts validated session startup progress and drops malformed frames", () => { const activity = { sessionId: "session-1", phase: "active", label: "Creating session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z" }; diff --git a/src/server/activity/workspaceActivityService.test.ts b/src/server/activity/workspaceActivityService.test.ts index 976ec0a..4378016 100644 --- a/src/server/activity/workspaceActivityService.test.ts +++ b/src/server/activity/workspaceActivityService.test.ts @@ -32,6 +32,19 @@ describe("WorkspaceActivityService", () => { expect(events.at(-1)).toMatchObject({ type: "workspace.activity", activity: { cwd: "/repo", hasSessionActivity: false, hasTerminalActivity: false } }); }); + it("does not report a workspace active for a session that is only starting up", () => { + const events: RealtimeEvent[] = []; + const service = new WorkspaceActivityService({ publishRealtime: (event) => events.push(event) }); + + // Startup progress names a phase the daemon is inside; it is not work, so the + // workspace (and the project indicators and remote machines that read it) + // must not be reported as busy because of it. + service.applySessionActivity("/repo", { sessionId: "s1", phase: "active", label: "Opening session", detail: "Starting the Pi session", at: "now", startup: true }); + + expect(service.snapshot().workspaces).toEqual([]); + expect(events.at(-1)).toMatchObject({ type: "workspace.activity", activity: { cwd: "/repo", hasSessionActivity: false } }); + }); + it("clears stale active activity when an idle status arrives", () => { const events: RealtimeEvent[] = []; const service = new WorkspaceActivityService({ publishRealtime: (event) => events.push(event) }); diff --git a/src/server/sessions/piSessionService.startupProgress.test.ts b/src/server/sessions/piSessionService.startupProgress.test.ts index aa55a8d..f4a404f 100644 --- a/src/server/sessions/piSessionService.startupProgress.test.ts +++ b/src/server/sessions/piSessionService.startupProgress.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { PiSessionService, type PiSessionRuntime } from "./piSessionService.js"; import { CapturingSessionEventHub, emptyArchiveStore, fakeRuntime, sessionGateway, sessionRecord, sessionRef, testModelRuntime } from "./piSessionService.testSupport.js"; +import { isSessionActive } from "../../shared/activity.js"; import type { SessionActivity, SessionStartupProgressEvent } from "../../shared/apiTypes.js"; const TEST_AGENT_DIR = "/tmp/pi-web-test-agent"; @@ -235,6 +236,34 @@ describe("PiSessionService session startup progress", () => { await service.dispose(); }); + it("reports startup progress as starting rather than as work in progress", async () => { + const { hub, service } = startupService(); + + await service.start("/workspace"); + + // Startup phases are published with an "active" phase so the waiting user + // sees them, but opening a session is not work: nothing that decides whether + // work is in progress may count them. + const phases = startupEvents(hub).filter((event) => event.activity.phase === "active"); + expect(phases).toHaveLength(2); + expect(phases.map((event) => isSessionActive(undefined, event.activity))).toEqual([false, false]); + await service.dispose(); + }); + + it("still reports a real activity published during startup as work", async () => { + const { hub, fake, service } = startupService(); + + await service.start("/workspace"); + fake.emit({ type: "tool_execution_start", toolName: "bash" }); + + // The marker belongs to the startup channel alone; an ordinary activity for + // the same session still counts, or the fix would hide real work. + const running = activityUpdates(hub).filter((activity) => activity.phase === "active"); + expect(running.length).toBeGreaterThan(0); + expect(running.every((activity) => isSessionActive(undefined, activity))).toBe(true); + await service.dispose(); + }); + it("keeps startup reporting event-only, writing no session or workspace activity state", async () => { const recorder = recordingWorkspaceActivity(); const failure = new Error("runtime unavailable"); diff --git a/src/server/sessions/piSessionService.ts b/src/server/sessions/piSessionService.ts index 8ccc346..b6cbd80 100644 --- a/src/server/sessions/piSessionService.ts +++ b/src/server/sessions/piSessionService.ts @@ -3039,10 +3039,15 @@ export class PiSessionService implements SessionRouteService { * Unlike {@link publishActivity} this deliberately records nothing: no * `activities` entry, no workspace activity, no unread observation. There is * no session to own that state, and a failed creation would leave it stranded. + * + * Every report is marked `startup`, which is what keeps a session that is + * merely opening from counting as one doing work. This is the only publisher + * that sets the marker, and because it writes no `activities` entry no later + * heartbeat re-publication can carry it. */ private publishStartupProgress(sessionId: string, startupToken: string | undefined, label: string, phase: "active" | "idle", detail: string | undefined): void { const at = new Date().toISOString(); - const activity = detail === undefined ? { sessionId, phase, label, at } : { sessionId, phase, label, detail, at }; + const activity = detail === undefined ? { sessionId, phase, label, at, startup: true } : { sessionId, phase, label, detail, at, startup: true }; this.events.publishGlobal(startupToken === undefined ? { type: "session.startup", activity } : { type: "session.startup", startupToken, activity }); } diff --git a/src/shared/activity.test.ts b/src/shared/activity.test.ts index badf315..b99b659 100644 --- a/src/shared/activity.test.ts +++ b/src/shared/activity.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { isSessionActive, isWorkspaceActivityActive } from "./activity"; -import type { SessionStatus, WorkspaceActivity } from "./apiTypes"; +import type { SessionActivity, SessionStatus, WorkspaceActivity } from "./apiTypes"; const idleStatus: SessionStatus = { sessionId: "s1", @@ -20,6 +20,29 @@ describe("activity helpers", () => { expect(isSessionActive({ ...idleStatus, pendingMessageCount: 2 })).toBe(true); }); + it("does not count a session that is only starting up as doing work", () => { + // Startup is reported on the activity channel with an "active" phase because + // a phase really is in progress, but opening a session is not work a user + // can stop, so the marker is what separates starting from working. + const startup: SessionActivity = { sessionId: "s1", phase: "active", label: "Opening session", detail: "Starting the Pi session", at: "now", startup: true }; + + expect(isSessionActive(undefined, startup)).toBe(false); + expect(isSessionActive(idleStatus, startup)).toBe(false); + }); + + it("still reports genuine work happening while a session starts up", () => { + const startup: SessionActivity = { sessionId: "s1", phase: "active", label: "Opening session", at: "now", startup: true }; + + // The marker only removes the activity-phase reason for being active, so + // work the status proves is unaffected by it. + expect(isSessionActive({ ...idleStatus, isStreaming: true }, startup)).toBe(true); + expect(isSessionActive({ ...idleStatus, isBashRunning: true }, startup)).toBe(true); + expect(isSessionActive({ ...idleStatus, isCompacting: true }, startup)).toBe(true); + expect(isSessionActive({ ...idleStatus, pendingMessageCount: 1 }, startup)).toBe(true); + // An unmarked active activity is ordinary work and keeps counting. + expect(isSessionActive(idleStatus, { sessionId: "s1", phase: "active", label: "running tool", at: "now" })).toBe(true); + }); + it("detects workspace activity presence without exposing details", () => { const idle: WorkspaceActivity = { cwd: "/repo", hasSessionActivity: false, hasTerminalActivity: false, updatedAt: "now" }; expect(isWorkspaceActivityActive(idle)).toBe(false); diff --git a/src/shared/activity.ts b/src/shared/activity.ts index 657b34b..4994562 100644 --- a/src/shared/activity.ts +++ b/src/shared/activity.ts @@ -1,7 +1,15 @@ import type { SessionActivity, SessionStatus, WorkspaceActivity } from "./apiTypes.js"; +/** + * Whether a session has work in progress a user could stop or must wait for. + * + * A startup activity is excluded: the session is being opened, not worked in. + * The exclusion can only ever remove the activity-phase reason for being + * active, so streaming, bash, compaction, and queued prompts still count even + * while a startup report is the latest activity. + */ export function isSessionActive(status?: SessionStatus, activity?: SessionActivity): boolean { - return activity?.phase === "active" + return (activity?.phase === "active" && activity.startup !== true) || status?.isStreaming === true || status?.isBashRunning === true || status?.isCompacting === true diff --git a/src/shared/apiTypes.ts b/src/shared/apiTypes.ts index 308f15f..93839d9 100644 --- a/src/shared/apiTypes.ts +++ b/src/shared/apiTypes.ts @@ -426,6 +426,14 @@ export interface SessionActivity { label: string; detail?: string; at: string; + /** + * Set only on the startup window's own reports. A startup phase is genuinely + * in progress, so it is published as `active` and rendered like any other + * activity, but *starting* a session is not *working* in it: there is nothing + * to stop, nothing that blocks reloading from disk, and no workspace-level + * work to report. `isSessionActive()` reads this to keep the two apart. + */ + startup?: boolean; } export interface QueuedSessionMessage {