Archived
fix(sessions): correlate startup progress by token instead of workspace
Startup progress could still be shown on the wrong session's row. Routing by known session id first closed the case where the browser knew the other session, but left open the case where it does not -- which the browser is designed to produce. While a create is pending for a workspace, applyCreatedSession deliberately withholds a session.created event for that workspace and stashes it, to avoid a duplicate row. So during exactly the window this feature exists for, a session created by an agent's spawn or by another tab is intentionally absent from the session list. Its startup events carried an unrecognised id and a matching cwd, and were routed onto the user's pending create row, showing a phase and a label belonging to another session. Workspace path was never evidence of identity; it was the only key both sides happened to share. Give them a real one. The browser already invents a temporary row id for a pending create, so it now sends that id with the create request as an opaque startupToken; the daemon carries it through construction, echoes it on the startup events it publishes for that construction, and the browser matches it exactly. The token is a throwaway label the daemon never interprets. It never becomes the session id: activity.sessionId still carries Pi's SessionManager id, which remains how an open of an already-known session is routed. With exact identity available, the guessing is deleted rather than gated. startupProgressPendingStart goes entirely, and with it the selected-machine comparison, the cwd filter, and the single-match ambiguity rule: a second concurrent create carries a different token, and a foreign workspace or non-selected machine carries no token this browser is waiting on, so those cases stop existing rather than needing detection. One Map lookup replaces a filtered scan. cwd comes off the event, since it existed only as the routing key and nothing else read it. No compatibility path is needed. session.startup is unreleased -- checked against the published tarball, not only git tags -- so no deployed daemon emits these events and no deployed browser parses them. An older daemon ignores the extra request field; a newer daemon talking to an older browser degrades to the pre-existing generic wording, as does any unmatched token. One silent behaviour change to state plainly: startupProgress guarded on `sessionId === "" || cwd === ""`. Removing cwd from the event removes the meaningful half of that guard, and that half had no test. The session-id half is kept, which is the half that actually protects honest reporting. The replaced ambiguity test is rewritten rather than dropped, so the same three scenarios still pin the user-visible guarantee -- no match means the generic wording stays -- now including the reproduced foreign-session case, which fails against the previous code. Session creation ordering, semantics, and queueing are unchanged; the token is a passthrough label read only to build an event.
This commit is contained in:
@@ -77,7 +77,7 @@ describe("PiSessionService session startup progress", () => {
|
||||
// The proof that matters: the user is told what is being waited on before
|
||||
// the wait ends, not after it.
|
||||
expect(startupText(hub)).toEqual(["Creating session: Starting the Pi session"]);
|
||||
expect(startupEvents(hub).at(0)).toMatchObject({ cwd: "/workspace", activity: { sessionId: "session-1", phase: "active" } });
|
||||
expect(startupEvents(hub).at(0)).toMatchObject({ activity: { sessionId: "session-1", phase: "active" } });
|
||||
|
||||
runtimeResult.resolve(fake.runtime);
|
||||
await started;
|
||||
@@ -157,11 +157,42 @@ describe("PiSessionService session startup progress", () => {
|
||||
|
||||
await service.start("/workspace");
|
||||
|
||||
expect(startupEvents(hub).at(-1)).toMatchObject({ cwd: "/workspace", activity: { sessionId: "session-1", phase: "idle", label: "idle" } });
|
||||
expect(startupEvents(hub).at(-1)).toMatchObject({ activity: { sessionId: "session-1", phase: "idle", label: "idle" } });
|
||||
expect(startupEvents(hub).at(-1)?.activity.detail).toBeUndefined();
|
||||
await service.dispose();
|
||||
});
|
||||
|
||||
it("echoes a create's correlation token on every startup report of that construction", async () => {
|
||||
const { hub, service } = startupService();
|
||||
|
||||
await service.start("/workspace", { startupToken: "pending-session-3-k2x9" });
|
||||
|
||||
// The token labels the browser row that is waiting, so it must ride every
|
||||
// report of this construction, the closing idle one included.
|
||||
expect(startupEvents(hub).map((event) => event.startupToken)).toEqual([
|
||||
"pending-session-3-k2x9",
|
||||
"pending-session-3-k2x9",
|
||||
"pending-session-3-k2x9",
|
||||
]);
|
||||
// The token is an opaque throwaway label, never the session's identity.
|
||||
expect(startupEvents(hub).map((event) => event.activity.sessionId)).toEqual(["session-1", "session-1", "session-1"]);
|
||||
await service.dispose();
|
||||
});
|
||||
|
||||
it("publishes no correlation token when a create supplies none, and none for an open", async () => {
|
||||
const created = startupService();
|
||||
await created.service.start("/workspace");
|
||||
const opened = startupService({ sessionRecords: [sessionRecord("session-1")] });
|
||||
await opened.service.status(sessionRef("session-1"));
|
||||
|
||||
for (const hub of [created.hub, opened.hub]) {
|
||||
expect(startupEvents(hub).length).toBeGreaterThan(0);
|
||||
expect(startupEvents(hub).every((event) => event.startupToken === undefined)).toBe(true);
|
||||
}
|
||||
await created.service.dispose();
|
||||
await opened.service.dispose();
|
||||
});
|
||||
|
||||
it("ends the startup window when the runtime construction itself fails", async () => {
|
||||
const failure = new Error("runtime unavailable");
|
||||
const { hub, service } = startupService({ createAgentRuntime: () => Promise.reject(failure) });
|
||||
|
||||
@@ -199,6 +199,11 @@ type SessionCreationProvenance = "tracked-subsession";
|
||||
interface StartSessionOptions {
|
||||
parentSession?: string;
|
||||
initialModel?: AgentModel;
|
||||
/**
|
||||
* Opaque label, echoed on this construction's startup progress so a browser
|
||||
* row with no session id yet can recognise its own.
|
||||
*/
|
||||
startupToken?: string;
|
||||
}
|
||||
|
||||
interface InternalStartSessionOptions extends StartSessionOptions {
|
||||
@@ -390,7 +395,7 @@ interface PendingSessionOpen {
|
||||
promise: Promise<ActiveSession<PiSessionRuntime>>;
|
||||
}
|
||||
|
||||
interface CreateSessionRuntimeOptions extends Pick<InternalStartSessionOptions, "initialModel" | "creationProvenance"> {
|
||||
interface CreateSessionRuntimeOptions extends Pick<InternalStartSessionOptions, "initialModel" | "creationProvenance" | "startupToken"> {
|
||||
notificationGeneration?: SessionNotificationGeneration;
|
||||
notifications?: "enabled" | "disabled";
|
||||
/**
|
||||
@@ -992,6 +997,7 @@ export class PiSessionService implements SessionRouteService {
|
||||
cwd,
|
||||
{
|
||||
startupIntent: "create",
|
||||
...(options.startupToken === undefined ? {} : { startupToken: options.startupToken }),
|
||||
...(options.initialModel === undefined ? {} : { initialModel: options.initialModel }),
|
||||
...(options.creationProvenance === undefined ? {} : { creationProvenance: options.creationProvenance }),
|
||||
},
|
||||
@@ -2353,7 +2359,7 @@ export class PiSessionService implements SessionRouteService {
|
||||
cwd: string,
|
||||
options: CreateSessionRuntimeOptions = {},
|
||||
): Promise<ActiveSession<PiSessionRuntime>> {
|
||||
const startup = this.startupProgress(sessionManager, cwd, options.startupIntent ?? "open");
|
||||
const startup = this.startupProgress(sessionManager, options.startupIntent ?? "open", options.startupToken);
|
||||
try {
|
||||
return await this.createSessionRuntime(sessionManager, cwd, options, startup);
|
||||
} finally {
|
||||
@@ -2999,23 +3005,23 @@ export class PiSessionService implements SessionRouteService {
|
||||
/**
|
||||
* Build the reporter for one session construction.
|
||||
*
|
||||
* The session id and cwd are both known before any await — a `SessionManager`
|
||||
* has its id from construction — so the daemon can name what it is starting
|
||||
* even though the `PiAgentSession` that {@link publishActivity} needs does not
|
||||
* exist yet. When either is missing there is nothing honest to route on, so
|
||||
* the reporter stays silent and the browser keeps its own generic wording.
|
||||
* The session id is known before any await — a `SessionManager` has its id
|
||||
* from construction — so the daemon can name what it is starting even though
|
||||
* the `PiAgentSession` that {@link publishActivity} needs does not exist yet.
|
||||
* Without an id there is nothing to report against, so the reporter stays
|
||||
* silent and the browser keeps its own generic wording.
|
||||
*/
|
||||
private startupProgress(sessionManager: PiSessionManager, cwd: string, intent: "create" | "open"): SessionStartupProgressReporter {
|
||||
private startupProgress(sessionManager: PiSessionManager, intent: "create" | "open", startupToken: string | undefined): SessionStartupProgressReporter {
|
||||
const sessionId = sessionManager.getSessionId();
|
||||
if (sessionId === "" || cwd === "") return { report: noop, end: noop };
|
||||
if (sessionId === "") return { report: noop, end: noop };
|
||||
const label = intent === "create" ? "Creating session" : "Opening session";
|
||||
return {
|
||||
report: (phase) => { this.publishStartupProgress(sessionId, cwd, label, "active", this.startupDetail(phase)); },
|
||||
report: (phase) => { this.publishStartupProgress(sessionId, startupToken, label, "active", this.startupDetail(phase)); },
|
||||
end: () => {
|
||||
// A real activity published during the window (an extension error, say)
|
||||
// is the truth about this session and must survive the clear.
|
||||
if (this.activities.has(sessionId)) return;
|
||||
this.publishStartupProgress(sessionId, cwd, "idle", "idle", undefined);
|
||||
this.publishStartupProgress(sessionId, startupToken, "idle", "idle", undefined);
|
||||
},
|
||||
};
|
||||
}
|
||||
@@ -3027,17 +3033,17 @@ export class PiSessionService implements SessionRouteService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Report startup progress on the global channel only, keyed by `cwd` so a
|
||||
* browser row that has no session id yet can find it.
|
||||
* Report startup progress on the global channel only, echoing the caller's
|
||||
* correlation token so a waiting browser row recognises its own construction.
|
||||
*
|
||||
* 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.
|
||||
*/
|
||||
private publishStartupProgress(sessionId: string, cwd: string, label: string, phase: "active" | "idle", detail: string | undefined): void {
|
||||
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 };
|
||||
this.events.publishGlobal({ type: "session.startup", cwd, activity });
|
||||
this.events.publishGlobal(startupToken === undefined ? { type: "session.startup", activity } : { type: "session.startup", startupToken, activity });
|
||||
}
|
||||
|
||||
private publishActivity(session: PiAgentSession, label: string, phase: "active" | "idle" | "error", detail?: string): void {
|
||||
|
||||
@@ -26,6 +26,7 @@ import { PiSessionService, type PiSessionManagerGateway } from "./piSessionServi
|
||||
import { testModelRuntime } from "./piSessionService.testSupport.js";
|
||||
import { SessionNotificationStore } from "./sessionNotificationStore.js";
|
||||
import type { SessionRouteLookup, SessionRouteService } from "./sessionService.js";
|
||||
import type { ClientSession } from "../types.js";
|
||||
import { registerSessionRoutes } from "./sessionRoutes.js";
|
||||
import type { NormalizedSessionCleanupRequest } from "./sessionCleanup.js";
|
||||
|
||||
@@ -666,6 +667,35 @@ describe("session routes", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("forwards a create's optional correlation token alongside the normalized cwd", async () => {
|
||||
const routeApp = Fastify({ logger: false });
|
||||
await routeApp.register(fastifyWebsocket);
|
||||
const eventHub = new SessionEventHub();
|
||||
const routeService = new CapturingRouteSessionService();
|
||||
registerSessionRoutes(routeApp, routeService, eventHub);
|
||||
|
||||
try {
|
||||
const requestCwd = resolve("/repo");
|
||||
const withToken = await routeApp.inject({ method: "POST", url: "/sessions", payload: { cwd: requestCwd, startupToken: "pending-session-3-k2x9" } });
|
||||
const withoutToken = await routeApp.inject({ method: "POST", url: "/sessions", payload: { cwd: requestCwd } });
|
||||
// An older browser, or any non-browser caller, sends no token; and a
|
||||
// malformed one must not reach the service as a label it would echo.
|
||||
const malformedToken = await routeApp.inject({ method: "POST", url: "/sessions", payload: { cwd: requestCwd, startupToken: 7 } });
|
||||
|
||||
expect(withToken.statusCode).toBe(200);
|
||||
expect(withoutToken.statusCode).toBe(200);
|
||||
expect(malformedToken.statusCode).toBe(400);
|
||||
expect(malformedToken.json()).toEqual({ error: "startupToken field must be a string" });
|
||||
expect(routeService.startCalls).toEqual([
|
||||
{ cwd: requestCwd, startupToken: "pending-session-3-k2x9" },
|
||||
{ cwd: requestCwd, startupToken: undefined },
|
||||
]);
|
||||
} finally {
|
||||
await routeService.dispose();
|
||||
await routeApp.close();
|
||||
}
|
||||
});
|
||||
|
||||
it("rejects malformed bulk mutation bodies before calling the service", async () => {
|
||||
const routeApp = Fastify({ logger: false });
|
||||
await routeApp.register(fastifyWebsocket);
|
||||
@@ -706,6 +736,7 @@ class CapturingRouteSessionService implements SessionRouteService {
|
||||
readonly bulkArchiveCalls: SessionBulkMutationRef[][] = [];
|
||||
readonly bulkDeleteCalls: SessionBulkMutationRef[][] = [];
|
||||
readonly navigateTreeCalls: { lookup: SessionRouteLookup; request: SessionTreeNavigateRequest }[] = [];
|
||||
readonly startCalls: { cwd: string; startupToken: string | undefined }[] = [];
|
||||
reloadError: Error | undefined;
|
||||
clearQueueError: Error | undefined;
|
||||
|
||||
@@ -772,7 +803,11 @@ class CapturingRouteSessionService implements SessionRouteService {
|
||||
}
|
||||
|
||||
list(): never { throw unusedRouteMethod("list"); }
|
||||
start(): never { throw unusedRouteMethod("start"); }
|
||||
|
||||
start(cwd: string, options?: { startupToken?: string }): Promise<ClientSession> {
|
||||
this.startCalls.push({ cwd, startupToken: options?.startupToken });
|
||||
return Promise.resolve({ id: "session-1", path: "/tmp/session-1.jsonl", cwd, created: "2026-06-25T00:00:00.000Z", modified: "2026-06-25T00:00:00.000Z", messageCount: 0, firstMessage: "" });
|
||||
}
|
||||
|
||||
dismissWarning(lookup: SessionRouteLookup, dismissId: string): Promise<SessionStatus> {
|
||||
this.dismissWarningCalls.push({ lookup, dismissId });
|
||||
|
||||
@@ -45,10 +45,14 @@ export function registerSessionRoutes(app: FastifyInstance, sessions: SessionRou
|
||||
}
|
||||
});
|
||||
|
||||
app.post<{ Body: { cwd?: unknown } | undefined }>(`${prefix}/sessions`, async (request, reply) => {
|
||||
app.post<{ Body: { cwd?: unknown; startupToken?: unknown } | undefined }>(`${prefix}/sessions`, async (request, reply) => {
|
||||
try {
|
||||
const body = requireRecord(request.body);
|
||||
return await sessions.start(normalizeRequestCwd(requireString(body, "cwd")));
|
||||
// An opaque label the caller uses to recognise its own construction's
|
||||
// startup reports. Optional: only a browser row waiting for a session id
|
||||
// has anything to correlate.
|
||||
const startupToken = body["startupToken"] === undefined ? undefined : requireNonEmptyString(body, "startupToken");
|
||||
return await sessions.start(normalizeRequestCwd(requireString(body, "cwd")), optionalField("startupToken", startupToken));
|
||||
} catch (error) {
|
||||
return reply.code(400).send({ error: errorMessage(error) });
|
||||
}
|
||||
|
||||
@@ -40,7 +40,12 @@ export type SessionRouteLookup = string | SessionRouteRef;
|
||||
*/
|
||||
export interface SessionRouteService {
|
||||
list(cwd: string): Promise<ClientSession[]>;
|
||||
start(cwd: string): Promise<ClientSession>;
|
||||
/**
|
||||
* Create a session. `startupToken` is an opaque label the caller supplies so
|
||||
* it can recognise this construction's startup progress reports; the service
|
||||
* echoes it and never interprets it.
|
||||
*/
|
||||
start(cwd: string, options?: { startupToken?: string }): Promise<ClientSession>;
|
||||
messages(ref: SessionRouteLookup, page?: { before?: number; limit?: number }): Promise<unknown[] | ClientMessagePage>;
|
||||
status(ref: SessionRouteLookup): Promise<ClientSessionStatus>;
|
||||
streamSnapshot(ref: SessionRouteLookup): Promise<SessionStreamSnapshot>;
|
||||
|
||||
Reference in New Issue
Block a user