Archived
fix(sessions): attribute session startup progress by id before workspace
Startup progress resolved its target row by workspace path first and only fell back to a known session id, which let one row be shown another row's phase. While a create is pending in a workspace, an existing session in that same workspace can also be opened -- by selecting another row, by another tab, or by a subsession open -- and that open publishes the same cwd. The cwd-first order rewrote such an event onto the pending create row, so a user watching a session being created could be told a phase that belonged to a different session. That is exactly the dishonest attribution this work set out to avoid. A known session id is the strongest available proof of the target, so it is now checked first; workspace routing is used only when the id is unknown, which is precisely the pre-session case it exists for. No wording changed and no event changed; only which row an event is applied to. Two tests were added where behavior was asserted but not proved. The controller test fails against the previous order, so the misattribution is now pinned. The service test covers a startup whose extension binding rejects, proving the window still ends with an idle report rather than leaving a waiting row labelled with a phase the service has left.
This commit is contained in:
@@ -127,6 +127,32 @@ describe("SessionController session startup progress", () => {
|
||||
expect(state.activity).toMatchObject({ sessionId: oldSession.id, label: "Opening session", detail: "Starting the Pi session" });
|
||||
});
|
||||
|
||||
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] } };
|
||||
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");
|
||||
|
||||
// Opening an existing session in the same workspace publishes the same cwd as
|
||||
// the pending create. The known id is the proof of which row it belongs to, so
|
||||
// the pending row must keep its own wording instead of the other row's phase.
|
||||
controller.applyGlobalEvent({
|
||||
type: "session.startup",
|
||||
cwd: workspace.path,
|
||||
activity: startupActivity({ sessionId: existing.id, label: "Opening session" }),
|
||||
});
|
||||
runPendingAnimationFrames();
|
||||
|
||||
expect(state.current.sessionActivities[existing.id]).toMatchObject({ label: "Opening session", detail: "Starting the Pi session" });
|
||||
expect(state.current.sessionActivities[temporaryId]?.detail).toBe("Waiting for the backend session to be ready");
|
||||
|
||||
startRequest.resolve({ ...oldSession, id: "backend-session", path: "/tmp/backend-session.jsonl" });
|
||||
await start;
|
||||
});
|
||||
|
||||
it("keeps the generic wording when the startup progress cannot be attributed to one row", async () => {
|
||||
const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } };
|
||||
const { controller, startRequest } = pendingStartController(state);
|
||||
|
||||
@@ -1309,26 +1309,31 @@ export class SessionController {
|
||||
}
|
||||
|
||||
// Session startup progress arrives while the daemon is still constructing the
|
||||
// session, so it is routed by workspace path: a pending start knows its cwd
|
||||
// but not the session id the daemon is creating. Once the target row is
|
||||
// resolved the progress goes through the normal activity buffer, so it renders
|
||||
// exactly like any other activity and stays batched per frame.
|
||||
// session, so the target row is resolved by session id when the browser knows
|
||||
// it and by workspace path when it does not: a pending start knows its cwd but
|
||||
// not the session id the daemon is creating. Once the row is resolved the
|
||||
// progress goes through the normal activity buffer, so it renders exactly like
|
||||
// any other activity and stays batched per frame.
|
||||
private queueStartupProgress(event: SessionStartupProgressEvent): void {
|
||||
// A known session id is the strongest possible proof of the target, so it is
|
||||
// checked first: while a create is pending in a workspace, an *existing*
|
||||
// session in that same workspace can be opened too (another row selected,
|
||||
// another tab, a subsession), and that open publishes the same cwd. Matching
|
||||
// on cwd first would paint the pending row with another session's phase.
|
||||
if (this.getState().sessions.some((session) => session.id === event.activity.sessionId)) {
|
||||
this.queueActivityUpdate(event.activity);
|
||||
return;
|
||||
}
|
||||
// The id is unknown, so this can only be a create whose id the browser has
|
||||
// not been told yet. Route it by workspace path, the one key both sides share.
|
||||
const pending = this.startupProgressPendingStart(event.cwd);
|
||||
if (pending !== undefined) {
|
||||
if (pending === undefined) return;
|
||||
// An idle startup phase means the daemon has nothing left to attribute, so
|
||||
// restore this row's own generic wording rather than clearing the text of a
|
||||
// creation request that has not returned yet.
|
||||
this.queueActivityUpdate(event.activity.phase === "idle"
|
||||
? creatingPendingSessionActivity(pending.tempId, pending.queuedSends.length)
|
||||
: { ...event.activity, sessionId: pending.tempId });
|
||||
return;
|
||||
}
|
||||
// Opening a session the browser already knows the id of: the event applies as
|
||||
// published. Anything else (a foreign workspace, or several pending starts in
|
||||
// one workspace, where the row this belongs to cannot be proved) is dropped
|
||||
// rather than attributed to a guess.
|
||||
if (this.getState().sessions.some((session) => session.id === event.activity.sessionId)) this.queueActivityUpdate(event.activity);
|
||||
}
|
||||
|
||||
private startupProgressPendingStart(cwd: string): PendingSessionStart | undefined {
|
||||
|
||||
@@ -187,6 +187,23 @@ describe("PiSessionService session startup progress", () => {
|
||||
await service.dispose();
|
||||
});
|
||||
|
||||
it("ends the startup window when extension binding fails, leaving no stale phase label", async () => {
|
||||
const failure = new Error("extension refused to load");
|
||||
const { hub, fake, service } = startupService();
|
||||
fake.session.bindExtensions = () => Promise.reject(failure);
|
||||
|
||||
await expect(service.start("/workspace")).rejects.toBe(failure);
|
||||
|
||||
// The last word on this startup must not be an "active" phase the service is
|
||||
// no longer inside; otherwise a waiting row keeps a label that is now false.
|
||||
expect(startupText(hub)).toEqual([
|
||||
"Creating session: Starting the Pi session",
|
||||
"Creating session: Loading session extensions",
|
||||
"idle",
|
||||
]);
|
||||
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");
|
||||
|
||||
Reference in New Issue
Block a user