Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/tui/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,10 @@ the e2e harness drives a real run, and `tuiProgramFlow(programId)` returns the
program's flow steps, for walking a flow in tests.

`runTui` and `runTuiTool` resolve an exit code and never exit: the CLI applies
it. A screen ends the run with `store.requestExit(code)`: `runTui` resolves it
as is, and `runTuiTool` resolves it once the tool's analytics events flush. A
decided failure ends it through `abortOnScreens`, or through `wizardAbort` with
it. A screen ends the run with `store.requestExit(code)`: `runTui` unmounts,
reports the run's end to analytics within two seconds and resolves it, and
`runTuiTool` resolves it once the tool's analytics events flush. A decided
failure ends it through `abortOnScreens`, or through `wizardAbort` with
`printAbortOutro` before the TUI mounts; Ctrl+C or a signal ends it 130 or 143.

A flow id names a program or a tool. The core finds its flow, screens and deck
Expand Down
46 changes: 44 additions & 2 deletions src/tui/__tests__/run.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ it.each([
},
);

it("resolves a screen's exit request with its code and starts no end shutdown", async () => {
it("resolves a screen's exit request with its code and starts no stream shutdown", async () => {
const { store, unmount } = mountedStore();
vi.mocked(runProgram).mockResolvedValue({
outcome: RunOutcome.Success,
Expand All @@ -210,7 +210,49 @@ it("resolves a screen's exit request with its code and starts no end shutdown",
await expect(exited).resolves.toBe(0);
await flush();
expect(streamShutdown).not.toHaveBeenCalled();
expect(unmount).not.toHaveBeenCalled();
expect(unmount).toHaveBeenCalledOnce();
});

it.each([
[0, 'cancelled'],
[1, 'error'],
] as const)(
'ends a screen exit request %i before the run with a %s shutdown, delivered before the exit',
async (code, status) => {
const { store, unmount } = mountedStore();
// The intro never settles: the user leaves from its menu.
vi.spyOn(store, 'getGate').mockReturnValue(new Promise(() => undefined));
let delivered = false;
vi.mocked(analytics.flush).mockImplementation(async () => {
// The screen is gone before the wait, so it takes no more input.
expect(unmount).toHaveBeenCalledOnce();
await flush();
delivered = true;
});
const exited = runTui(posthogIntegration, launch('/tmp/intro-exit-test'));
await vi.waitFor(() => expect(store.getGate).toHaveBeenCalled());
store.requestExit(code);
await expect(exited).resolves.toBe(code);
expect(delivered).toBe(true);
expect(analytics.shutdown).toHaveBeenCalledExactlyOnceWith(status);
expect(runProgram).not.toHaveBeenCalled();
},
);

it('exits on a screen exit request within the report budget when analytics hang', async () => {
vi.useFakeTimers({ toFake: ['setTimeout'] });
try {
const { store } = mountedStore();
vi.spyOn(store, 'getGate').mockReturnValue(new Promise(() => undefined));
vi.mocked(analytics.shutdown).mockReturnValue(new Promise(() => undefined));
const exited = runTui(posthogIntegration, launch('/tmp/hung-exit-test'));
await vi.waitFor(() => expect(store.getGate).toHaveBeenCalled());
store.requestExit(0);
await vi.advanceTimersByTimeAsync(2000);
await expect(exited).resolves.toBe(0);
} finally {
vi.useRealTimers();
}
});

it('resolves an abort with its code once its outro is dismissed', async () => {
Expand Down
25 changes: 22 additions & 3 deletions src/tui/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ import type { WizardStore } from './store.js';
import type { TuiLaunch } from './launch.js';
import { tuiWorkflow } from './workflow.js';

/** How long a screen's exit request waits for analytics before it exits anyway. */
const EXIT_REPORT_BUDGET_MS = 2000;

/**
* Run `config` in the TUI. Resolves with the exit code: the run's, a screen's
* exit request, a decided failure's through `wizardAbort`, or 130 or 143 on a signal.
Expand Down Expand Up @@ -118,9 +121,25 @@ export async function runTui(
tui = startTUI(VERSION, config.id, () => onSignal('SIGINT'));
const activeTui = tui;
const { store } = activeTui;
// A screen's exit request ends the run with no shutdown; start-tui's exit listener unmounts.
// A screen's exit request unmounts, reports the run's end within a bounded wait, then exits with no stream shutdown.
store.subscribe(() => {
if (store.exitRequest !== null && !handedOff) exit.end(store.exitRequest);
const code = store.exitRequest;
if (code === null || handedOff || exitInProgress || signalled) return;
exitInProgress = true;
launch.signal.removeEventListener('abort', onAbort);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggested

Here's the potential issue: Cancel used to exit at once. Now it waits on two analytics shutdowns with no time limit, and Ctrl-C is ignored for the whole wait.

network that drops PostHog requests -> user picks Cancel -> exit waits about 30 s or more -> Ctrl-C does nothing -> user kills the terminal

Suggested fix: Cap the shutdown and flush with a short timeout, like the 2 s cap on the stream shutdown.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, NotVincent here :disguise-emoji:, I will do the deeds

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done, NotVincent will tell Vincent to double check.

activeTui.unmount();
const report = async (): Promise<void> => {
try {
await analytics.shutdown(code === 0 ? 'cancelled' : 'error');
} catch {
logToFile('[run-wizard] exit request shutdown failed');
}
await flushAnalytics();
};
void Promise.race([
report(),
new Promise((resolve) => setTimeout(resolve, EXIT_REPORT_BUDGET_MS)),
]).then(() => exit.end(code));
});

const session = buildSession(launch.session);
Expand Down Expand Up @@ -244,7 +263,7 @@ export async function runTui(
await activeStream.finishRun(runFailed ? 'failed' : 'completed');
await store.waitUntil((s) => s.mintHandoff === 'exit' || s.skillsComplete);
// A screen already ended the run (KeepSkills after a success): start no flush it would cut off.
if (exit.ended) return;
if (exit.ended || exitInProgress) return;

exitInProgress = true;
await activeStream.shutdown(2000);
Expand Down
Loading