Skip to content

fix: quit handler cleanup ordering and timeout test coverage #689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pedramamini merged 1 commit into from
Mar 29, 2026
+81 −4
Merged

fix: quit handler cleanup ordering and timeout test coverage #689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
78 changes: 77 additions & 1 deletion src/__tests__/main/app-lifecycle/quit-handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,13 @@ describe('app-lifecycle/quit-handler', () => {
expect(mockHistoryManager.stopWatching).toHaveBeenCalled();
expect(deps.stopCliWatcher).toHaveBeenCalled();
expect(deps.stopSessionCleanup).toHaveBeenCalled();
expect(mockPowerManager.clearAllReasons).toHaveBeenCalled();
expect(mockProcessManager.killAll).toHaveBeenCalled();
// clearAllReasons must be called AFTER killAll to prevent late process
// output from re-arming the sleep blocker
expect(mockPowerManager.clearAllReasons).toHaveBeenCalled();
const killOrder = mockProcessManager.killAll.mock.invocationCallOrder[0];
const clearOrder = mockPowerManager.clearAllReasons.mock.invocationCallOrder[0];
expect(killOrder).toBeLessThan(clearOrder);
expect(mockTunnelManager.stop).toHaveBeenCalled();
expect(mockWebServer.stop).toHaveBeenCalled();
expect(deps.closeStatsDB).toHaveBeenCalled();
Expand Down Expand Up @@ -366,6 +371,77 @@ describe('app-lifecycle/quit-handler', () => {
expect(() => beforeQuitHandler!(mockEvent)).not.toThrow();
});

it('should force-quit after safety timeout if renderer never responds', async () => {
vi.useFakeTimers();

const { createQuitHandler } = await import('../../../main/app-lifecycle/quit-handler');

const quitHandler = createQuitHandler(deps as Parameters<typeof createQuitHandler>[0]);
quitHandler.setup();

const mockEvent = { preventDefault: vi.fn() };
beforeQuitHandler!(mockEvent);

// Renderer was asked for confirmation
expect(mockMainWindow.webContents.send).toHaveBeenCalledWith('app:requestQuitConfirmation');
expect(mockQuit).not.toHaveBeenCalled();

// Advance past the 5s timeout without renderer responding
vi.advanceTimersByTime(5000);
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Mar 29, 2026

Choose a reason for hiding this comment

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

P2 Hardcoded timeout value duplicates the source constant

5000 is duplicated in all three new tests (lines 390, 417, 439) but QUIT_CONFIRMATION_TIMEOUT_MS is not exported from quit-handler.ts. If the timeout is ever adjusted, the tests will silently diverge.

Exporting the constant makes the coupling explicit:

// quit-handler.ts
export const QUIT_CONFIRMATION_TIMEOUT_MS = 5000;
// quit-handler.test.ts
import { QUIT_CONFIRMATION_TIMEOUT_MS } from '../../../main/app-lifecycle/quit-handler';
// ...
vi.advanceTimersByTime(QUIT_CONFIRMATION_TIMEOUT_MS);


expect(mockQuit).toHaveBeenCalled();
expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('timed out'), 'Window');

vi.useRealTimers();
});
Comment on lines +375 to +396
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Mar 29, 2026

Choose a reason for hiding this comment

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

P2 Fake timers not cleaned up on assertion failure

vi.useRealTimers() is placed at the end of each fake-timer test, but if any expect between vi.useFakeTimers() and vi.useRealTimers() throws, the process-wide timer state is left in fake mode for all subsequent tests. This can cause mysterious hangs or failures in otherwise unrelated tests.

The same pattern appears in all three new tests (lines 374–395, 398–421, 423–443).

Consider centralising the cleanup in an afterEach:

afterEach(() => {
    vi.useRealTimers(); // idempotent when timers are already real
    vi.restoreAllMocks();
});

Then the individual vi.useRealTimers() calls at the end of each test can be removed.


it('should clear safety timeout when renderer confirms quit', async () => {
vi.useFakeTimers();

const { createQuitHandler } = await import('../../../main/app-lifecycle/quit-handler');

const quitHandler = createQuitHandler(deps as Parameters<typeof createQuitHandler>[0]);
quitHandler.setup();

const mockEvent = { preventDefault: vi.fn() };
beforeQuitHandler!(mockEvent);

// Renderer confirms before timeout
const confirmHandler = ipcHandlers.get('app:quitConfirmed')!;
confirmHandler();

// mockQuit called once from confirmHandler
expect(mockQuit).toHaveBeenCalledTimes(1);

// Advance past timeout — should NOT trigger a second quit
vi.advanceTimersByTime(5000);
expect(mockQuit).toHaveBeenCalledTimes(1);

vi.useRealTimers();
});

it('should clear safety timeout when renderer cancels quit', async () => {
vi.useFakeTimers();

const { createQuitHandler } = await import('../../../main/app-lifecycle/quit-handler');

const quitHandler = createQuitHandler(deps as Parameters<typeof createQuitHandler>[0]);
quitHandler.setup();

const mockEvent = { preventDefault: vi.fn() };
beforeQuitHandler!(mockEvent);

// Renderer cancels
const cancelHandler = ipcHandlers.get('app:quitCancelled')!;
cancelHandler();

// Advance past timeout — should NOT force quit
vi.advanceTimersByTime(5000);
expect(mockQuit).not.toHaveBeenCalled();

vi.useRealTimers();
});

it('should work without stopCliWatcher dependency', async () => {
const depsWithoutCliWatcher = { ...deps };
delete depsWithoutCliWatcher.stopCliWatcher;
Expand Down
7 changes: 4 additions & 3 deletions src/main/app-lifecycle/quit-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,6 @@ export function createQuitHandler(deps: QuitHandlerDependencies): QuitHandler {
stopSessionCleanup();
}

// Clear power save blocker to release OS-level sleep prevention state
powerManager.clearAllReasons();

// Clean up active grooming sessions (context merge/transfer operations)
const processManager = getProcessManager();
const groomingSessionCount = getActiveGroomingSessionCount();
Expand All @@ -225,6 +222,10 @@ export function createQuitHandler(deps: QuitHandlerDependencies): QuitHandler {
logger.info('Killing all running processes', 'Shutdown');
processManager?.killAll();

// Clear power save blocker AFTER killAll() to prevent late process output
// from re-arming the blocker via addBlockReason()
powerManager.clearAllReasons();

// Stop tunnel and web server (fire and forget)
logger.info('Stopping tunnel', 'Shutdown');
tunnelManager.stop().catch((err: unknown) => {
Expand Down
Loading