Archived
fix(sessions): close extension dialog lifecycle edge cases
Post-review hardening for the extension dialog feature: - dispose() and closeActive() now settle startup-parked session_start dialogs before awaiting pending opens, so daemon shutdown or closing a session whose open is parked on a dialog can no longer block behind the dialog timeout (infinite with extensionDialogsTimeoutMs: 0). - A failed create now drops its dead dialog cards, closes the early-subscribed socket, and ignores late dialog frames instead of leaving an unanswerable card on the failed row. - The pending-start status resync checks its staleness guard before applying the unordered snapshot, so a late response can no longer clobber the post-swap session state. - The dialog countdown no longer queues one screen-reader announcement per second (decorative; the daemon-owned dialog.closed event is the real signal) and no longer renders "1h 60m" near hour boundaries.
This commit is contained in:
@@ -141,13 +141,25 @@ describe("extension-dialog-card countdown", () => {
|
|||||||
vi.setSystemTime(new Date("2026-07-27T10:00:00.000Z"));
|
vi.setSystemTime(new Date("2026-07-27T10:00:00.000Z"));
|
||||||
const card = await mountOpenDialog(openDialog({ timeoutAt: "2026-07-27T10:01:30.000Z" }));
|
const card = await mountOpenDialog(openDialog({ timeoutAt: "2026-07-27T10:01:30.000Z" }));
|
||||||
const root = renderRoot(card);
|
const root = renderRoot(card);
|
||||||
const status = requiredElement(root.querySelector("[role='status']"), "countdown status");
|
const countdown = requiredElement(root.querySelector(".countdown"), "countdown");
|
||||||
|
|
||||||
expect(status.textContent).toBe("Auto-cancels in 1m 30s");
|
expect(countdown.textContent).toBe("Auto-cancels in 1m 30s");
|
||||||
|
|
||||||
await vi.advanceTimersByTimeAsync(30_000);
|
await vi.advanceTimersByTimeAsync(30_000);
|
||||||
await card.updateComplete;
|
await card.updateComplete;
|
||||||
expect(status.textContent).toBe("Auto-cancels in 1m 0s");
|
expect(countdown.textContent).toBe("Auto-cancels in 1m 0s");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("is decorative: no live region announcing every second", async () => {
|
||||||
|
vi.useFakeTimers();
|
||||||
|
vi.setSystemTime(new Date("2026-07-27T10:00:00.000Z"));
|
||||||
|
const card = await mountOpenDialog(openDialog({ timeoutAt: "2026-07-27T10:01:30.000Z" }));
|
||||||
|
const root = renderRoot(card);
|
||||||
|
|
||||||
|
// A ticking live region would queue a screen-reader announcement per
|
||||||
|
// second; the daemon-owned dialog.closed event is the real signal.
|
||||||
|
expect(requiredElement(root.querySelector(".countdown"), "countdown").getAttribute("role")).toBeNull();
|
||||||
|
expect(root.querySelector("[aria-live]")).toBeNull();
|
||||||
});
|
});
|
||||||
|
|
||||||
it("renders no countdown when the dialog waits forever", async () => {
|
it("renders no countdown when the dialog waits forever", async () => {
|
||||||
@@ -155,7 +167,7 @@ describe("extension-dialog-card countdown", () => {
|
|||||||
const card = await mountOpenDialog(openDialog());
|
const card = await mountOpenDialog(openDialog());
|
||||||
const root = renderRoot(card);
|
const root = renderRoot(card);
|
||||||
|
|
||||||
expect(root.querySelector("[role='status']")).toBeNull();
|
expect(root.querySelector(".countdown")).toBeNull();
|
||||||
});
|
});
|
||||||
|
|
||||||
it("stops ticking once the dialog closes", async () => {
|
it("stops ticking once the dialog closes", async () => {
|
||||||
@@ -219,6 +231,11 @@ describe("extensionDialogCountdownText", () => {
|
|||||||
expect(extensionDialogCountdownText("2026-07-27T11:02:00.000Z", now)).toBe("Auto-cancels in 1h 2m");
|
expect(extensionDialogCountdownText("2026-07-27T11:02:00.000Z", now)).toBe("Auto-cancels in 1h 2m");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("never rounds the minutes up to 60 near an hour boundary", () => {
|
||||||
|
expect(extensionDialogCountdownText("2026-07-27T11:59:55.000Z", now)).toBe("Auto-cancels in 1h 59m");
|
||||||
|
expect(extensionDialogCountdownText("2026-07-27T12:59:40.000Z", now)).toBe("Auto-cancels in 2h 59m");
|
||||||
|
});
|
||||||
|
|
||||||
it("stays display-only once the deadline has passed", () => {
|
it("stays display-only once the deadline has passed", () => {
|
||||||
expect(extensionDialogCountdownText("2026-07-27T09:59:59.000Z", now)).toBe("Auto-cancel imminent");
|
expect(extensionDialogCountdownText("2026-07-27T09:59:59.000Z", now)).toBe("Auto-cancel imminent");
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -58,7 +58,8 @@ export function extensionDialogCountdownText(timeoutAt: string | undefined, nowM
|
|||||||
const seconds = Math.ceil(remainingMs / 1000);
|
const seconds = Math.ceil(remainingMs / 1000);
|
||||||
if (seconds >= 3600) {
|
if (seconds >= 3600) {
|
||||||
const hours = Math.floor(seconds / 3600);
|
const hours = Math.floor(seconds / 3600);
|
||||||
const minutes = Math.round((seconds % 3600) / 60);
|
// Floor, not round: rounding yields "1h 60m" in the last half-minute of an hour.
|
||||||
|
const minutes = Math.floor((seconds % 3600) / 60);
|
||||||
return `Auto-cancels in ${String(hours)}h ${String(minutes)}m`;
|
return `Auto-cancels in ${String(hours)}h ${String(minutes)}m`;
|
||||||
}
|
}
|
||||||
if (seconds >= 60) {
|
if (seconds >= 60) {
|
||||||
@@ -129,7 +130,10 @@ export class ExtensionDialogCard extends LitElement {
|
|||||||
<h2 id="extension-dialog-heading">${dialog.title}</h2>
|
<h2 id="extension-dialog-heading">${dialog.title}</h2>
|
||||||
${countdown === undefined
|
${countdown === undefined
|
||||||
? null
|
? null
|
||||||
: html`<span class="header-status countdown" role="status" aria-live="polite" aria-atomic="true">${countdown}</span>`}
|
// Decorative only — no live region: a polite region would queue one
|
||||||
|
// announcement per second. The daemon-owned dialog.closed event is
|
||||||
|
// the real signal, and the settled card announces the outcome.
|
||||||
|
: html`<span class="header-status countdown">${countdown}</span>`}
|
||||||
</header>
|
</header>
|
||||||
${this.renderOpenBody(dialog)}
|
${this.renderOpenBody(dialog)}
|
||||||
</article>
|
</article>
|
||||||
|
|||||||
@@ -298,4 +298,68 @@ describe("SessionController session_start dialog startup reachability", () => {
|
|||||||
resolveBackendSession(harness);
|
resolveBackendSession(harness);
|
||||||
await start;
|
await start;
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("drops a mid-startup status snapshot that lands after the readiness swap", async () => {
|
||||||
|
const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } };
|
||||||
|
const resyncRequest = deferred<SessionStatus>();
|
||||||
|
let resyncIssued = false;
|
||||||
|
const harness = pendingStartController(state, {
|
||||||
|
status: (session) => {
|
||||||
|
// The first backend status call is the subscribe-time resync; hold it
|
||||||
|
// until after the swap. Later calls (the readiness join) answer fresh.
|
||||||
|
if (sessionLookupId(session) === BACKEND_SESSION_ID && !resyncIssued) {
|
||||||
|
resyncIssued = true;
|
||||||
|
return resyncRequest.promise;
|
||||||
|
}
|
||||||
|
return Promise.resolve(status(sessionLookupId(session)));
|
||||||
|
},
|
||||||
|
});
|
||||||
|
const { start, tempId } = beginPendingStart(harness);
|
||||||
|
reportBackendSessionId(harness, tempId);
|
||||||
|
harness.socket.emit({ type: "dialog.opened", dialog: dialog("dialog-1") });
|
||||||
|
expect(harness.state.current.pendingDialogs).toEqual([dialog("dialog-1")]);
|
||||||
|
|
||||||
|
resolveBackendSession(harness);
|
||||||
|
await start;
|
||||||
|
await vi.waitFor(() => { expect(harness.state.current.selectedSession?.id).toBe(BACKEND_SESSION_ID); });
|
||||||
|
|
||||||
|
// The stale snapshot — issued before the swap and claiming dialog-1 is
|
||||||
|
// still open — must not clobber the real session's fresher state.
|
||||||
|
resyncRequest.resolve(statusWithDialogs(BACKEND_SESSION_ID, [dialog("dialog-1")]));
|
||||||
|
await Promise.resolve();
|
||||||
|
await Promise.resolve();
|
||||||
|
|
||||||
|
expect(harness.state.current.pendingDialogs).toEqual([]);
|
||||||
|
expect(harness.state.current.sessionStatuses[BACKEND_SESSION_ID]?.pendingDialogs ?? []).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("drops the dead card, closes the socket, and ignores late frames when the create fails mid-startup", async () => {
|
||||||
|
const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } };
|
||||||
|
let answerCalled = false;
|
||||||
|
const harness = pendingStartController(state, {
|
||||||
|
answerDialog: () => {
|
||||||
|
answerCalled = true;
|
||||||
|
return Promise.resolve(closeResponse(status(BACKEND_SESSION_ID)));
|
||||||
|
},
|
||||||
|
});
|
||||||
|
const closeSpy = vi.spyOn(harness.socket, "close");
|
||||||
|
const { start, tempId } = beginPendingStart(harness);
|
||||||
|
reportBackendSessionId(harness, tempId);
|
||||||
|
harness.socket.emit({ type: "dialog.opened", dialog: dialog("dialog-1") });
|
||||||
|
expect(harness.state.current.pendingDialogs).toEqual([dialog("dialog-1")]);
|
||||||
|
|
||||||
|
closeSpy.mockClear();
|
||||||
|
harness.startRequest.reject(new Error("create exploded"));
|
||||||
|
await start;
|
||||||
|
|
||||||
|
expect(harness.state.current.error).toBe("Failed to start session: create exploded");
|
||||||
|
expect(harness.state.current.pendingDialogs).toEqual([]);
|
||||||
|
expect(closeSpy).toHaveBeenCalled();
|
||||||
|
|
||||||
|
// Late frames from the dead session are dropped, and no answer can leave.
|
||||||
|
harness.socket.emit({ type: "dialog.opened", dialog: dialog("dialog-2") });
|
||||||
|
expect(harness.state.current.pendingDialogs).toEqual([]);
|
||||||
|
await harness.controller.answerDialog("dialog-2", true);
|
||||||
|
expect(answerCalled).toBe(false);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -1214,9 +1214,15 @@ export class SessionController {
|
|||||||
const pending = this.pendingSessionStarts.get(tempId);
|
const pending = this.pendingSessionStarts.get(tempId);
|
||||||
if (pending === undefined) return;
|
if (pending === undefined) return;
|
||||||
this.pendingSessionStarts.delete(tempId);
|
this.pendingSessionStarts.delete(tempId);
|
||||||
|
const wasDiscarded = pending.discarded;
|
||||||
|
// The pending start is dead: stop routing its dialog frames (a card on the
|
||||||
|
// failed row could never be answered) and drop the early-subscribed socket
|
||||||
|
// so it stops reconnecting against a session that may not exist.
|
||||||
|
pending.discarded = true;
|
||||||
|
if (this.getState().selectedSession?.id === tempId) this.socket.close();
|
||||||
const releasedCreatedSessions = this.takeSuppressedCreatedSessionsFor(pending.cwd, pending.machineId);
|
const releasedCreatedSessions = this.takeSuppressedCreatedSessionsFor(pending.cwd, pending.machineId);
|
||||||
const isCurrentPendingStart = this.isCurrentPendingStart(pending);
|
const isCurrentPendingStart = this.isCurrentPendingStart(pending);
|
||||||
if (pending.discarded || !isCurrentPendingStart) {
|
if (wasDiscarded || !isCurrentPendingStart) {
|
||||||
if (isCurrentPendingStart) this.applyReleasedCreatedSessions(releasedCreatedSessions, pending.machineId);
|
if (isCurrentPendingStart) this.applyReleasedCreatedSessions(releasedCreatedSessions, pending.machineId);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
@@ -1228,6 +1234,9 @@ export class SessionController {
|
|||||||
sessions: hasPendingRow ? state.sessions : [pending.session, ...state.sessions],
|
sessions: hasPendingRow ? state.sessions : [pending.session, ...state.sessions],
|
||||||
sessionActivities: { ...state.sessionActivities, [tempId]: activity },
|
sessionActivities: { ...state.sessionActivities, [tempId]: activity },
|
||||||
activity: state.selectedSession?.id === tempId ? activity : state.activity,
|
activity: state.selectedSession?.id === tempId ? activity : state.activity,
|
||||||
|
// Open cards on the failed row are dead: the create is gone, so no
|
||||||
|
// answer could ever reach the daemon. Settled outcomes stay as history.
|
||||||
|
...(state.selectedSession?.id === tempId ? { pendingDialogs: [] } : {}),
|
||||||
error: `Failed to start session: ${message}`,
|
error: `Failed to start session: ${message}`,
|
||||||
});
|
});
|
||||||
this.applyReleasedCreatedSessions(releasedCreatedSessions, pending.machineId);
|
this.applyReleasedCreatedSessions(releasedCreatedSessions, pending.machineId);
|
||||||
@@ -1585,8 +1594,11 @@ export class SessionController {
|
|||||||
if (backendSessionId === undefined) return;
|
if (backendSessionId === undefined) return;
|
||||||
void this.api.status({ id: backendSessionId, cwd: pending.cwd }, pending.machineId).then(
|
void this.api.status({ id: backendSessionId, cwd: pending.cwd }, pending.machineId).then(
|
||||||
(status) => {
|
(status) => {
|
||||||
this.applyStatus(status);
|
// Guard before applying: this unordered snapshot can land after the
|
||||||
|
// readiness swap made the real session selected, and a stale replace
|
||||||
|
// must not clobber the socket's fresher dialog state.
|
||||||
if (pending.discarded || this.getState().selectedSession?.id !== pending.tempId) return;
|
if (pending.discarded || this.getState().selectedSession?.id !== pending.tempId) return;
|
||||||
|
this.applyStatus(status);
|
||||||
const state = this.getState();
|
const state = this.getState();
|
||||||
const knownIds = new Set<string>([
|
const knownIds = new Set<string>([
|
||||||
...state.pendingDialogs.map((pendingDialog) => pendingDialog.dialogId),
|
...state.pendingDialogs.map((pendingDialog) => pendingDialog.dialogId),
|
||||||
|
|||||||
@@ -597,4 +597,37 @@ describe("PiSessionService session_start dialog startup reachability", () => {
|
|||||||
expect(confirmAnswers).toEqual([false]);
|
expect(confirmAnswers).toEqual([false]);
|
||||||
await service.dispose();
|
await service.dispose();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("dispose settles a startup-parked dialog instead of blocking behind its timeout", async () => {
|
||||||
|
const { service, store, events, confirmAnswers } = startupDialogService();
|
||||||
|
// The open flow registers in pendingSessionOpens, which dispose awaits:
|
||||||
|
// without settling the dialog first, disposal would ride its timeout.
|
||||||
|
const opening = service.messages(sessionRef(ACTIVE_SESSION_ID));
|
||||||
|
await parkOnStartupDialog(store);
|
||||||
|
|
||||||
|
await service.dispose();
|
||||||
|
|
||||||
|
expect(confirmAnswers).toEqual([false]);
|
||||||
|
const closedEvents = dialogEvents(events).filter(({ event }) => event.type === "dialog.closed");
|
||||||
|
expect(closedEvents).toHaveLength(1);
|
||||||
|
expect(closedEvents[0]?.event).toMatchObject({ dialogId: "dialog-1", reason: "session-ended" });
|
||||||
|
// The released open completed inside dispose's awaited window; the late
|
||||||
|
// messages read neither hangs nor rejects the test run.
|
||||||
|
await Promise.allSettled([opening]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("closing a session whose open is parked on a session_start dialog settles the dialog first", async () => {
|
||||||
|
const { service, store, events, confirmAnswers } = startupDialogService();
|
||||||
|
const opening = service.messages(sessionRef(ACTIVE_SESSION_ID));
|
||||||
|
await parkOnStartupDialog(store);
|
||||||
|
|
||||||
|
await service.stop(ACTIVE_SESSION_ID);
|
||||||
|
|
||||||
|
expect(confirmAnswers).toEqual([false]);
|
||||||
|
const closedEvents = dialogEvents(events).filter(({ event }) => event.type === "dialog.closed");
|
||||||
|
expect(closedEvents).toHaveLength(1);
|
||||||
|
expect(closedEvents[0]?.event).toMatchObject({ dialogId: "dialog-1", reason: "session-ended" });
|
||||||
|
await Promise.allSettled([opening]);
|
||||||
|
await service.dispose();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -1005,6 +1005,9 @@ export class PiSessionService implements SessionRouteService {
|
|||||||
this.clearUnreadPublicationRetry();
|
this.clearUnreadPublicationRetry();
|
||||||
clearInterval(this.heartbeat);
|
clearInterval(this.heartbeat);
|
||||||
this.clearCompactionDrainTimers();
|
this.clearCompactionDrainTimers();
|
||||||
|
// Same startup-park hazard as closeActive(): settle `session_start` dialogs
|
||||||
|
// of sessions still binding extensions before awaiting their pending opens.
|
||||||
|
for (const sessionId of this.startupSessions.keys()) this.endSessionExtensionDialogs(sessionId);
|
||||||
const pendingOpens = this.pendingSessionOpenPromises();
|
const pendingOpens = this.pendingSessionOpenPromises();
|
||||||
if (pendingOpens.length > 0) await Promise.allSettled(pendingOpens);
|
if (pendingOpens.length > 0) await Promise.allSettled(pendingOpens);
|
||||||
const activeSessions = Array.from(new Set(this.active.values()));
|
const activeSessions = Array.from(new Set(this.active.values()));
|
||||||
@@ -2592,6 +2595,10 @@ export class PiSessionService implements SessionRouteService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private async closeActive(sessionId: string, notificationPolicy: NotificationClosePolicy = CLEAR_RUNTIME_NOTIFICATIONS): Promise<void> {
|
private async closeActive(sessionId: string, notificationPolicy: NotificationClosePolicy = CLEAR_RUNTIME_NOTIFICATIONS): Promise<void> {
|
||||||
|
// A session whose open is parked on a `session_start` dialog holds its
|
||||||
|
// pending open until the dialog settles; settle it first so closing cannot
|
||||||
|
// block behind the dialog timeout (which `0` makes infinite).
|
||||||
|
if (this.startupSessions.has(sessionId)) this.endSessionExtensionDialogs(sessionId);
|
||||||
const pendingOpens = this.pendingSessionOpenPromises(sessionId);
|
const pendingOpens = this.pendingSessionOpenPromises(sessionId);
|
||||||
if (pendingOpens.length > 0) await Promise.allSettled(pendingOpens);
|
if (pendingOpens.length > 0) await Promise.allSettled(pendingOpens);
|
||||||
const active = this.active.get(sessionId);
|
const active = this.active.get(sessionId);
|
||||||
|
|||||||
Reference in New Issue
Block a user