From bc3c374e23fb732212153844b7f05c8cc00a7478 Mon Sep 17 00:00:00 2001 From: project820 <239489026+project820@users.noreply.github.com> Date: Mon, 10 Aug 2026 15:02:12 +0900 Subject: [PATCH] fix: acknowledge document save before session flush --- scripts/close-dialog-smoke-runner.mjs | 63 +++++++++++++++++++---- src/__tests__/app-windows-discard.test.ts | 27 +++++++++- src/main/app-windows.ts | 18 +++++-- src/renderer/close-save-handler.test.ts | 41 +++++++++++++++ src/renderer/close-save-handler.ts | 50 ++++++++++++++++++ src/renderer/main.ts | 20 ++++--- 6 files changed, 194 insertions(+), 25 deletions(-) create mode 100644 src/renderer/close-save-handler.test.ts create mode 100644 src/renderer/close-save-handler.ts diff --git a/scripts/close-dialog-smoke-runner.mjs b/scripts/close-dialog-smoke-runner.mjs index 3aa30f3..c7ced77 100644 --- a/scripts/close-dialog-smoke-runner.mjs +++ b/scripts/close-dialog-smoke-runner.mjs @@ -54,6 +54,16 @@ async function replaceEditorText(win, content) { }, 60_000); } +async function appendEditorText(win, marker) { + await waitFor('editor', () => win.webContents.executeJavaScript(`Boolean(document.querySelector('.cm-content'))`)); + await win.webContents.executeJavaScript(`document.querySelector('.cm-content')?.focus(); true`); + win.focus(); + win.webContents.focus(); + await win.webContents.executeJavaScript(`document.querySelector('.cm-content')?.focus(); true`); + win.webContents.insertText(`\n${marker}\n`); + await waitFor('CodeMirror append', async () => (await editorText(win)).includes(marker), 60_000); +} + function createFixture(userData) { const doc = join(userData, 'close-smoke.md'); const secondDoc = join(userData, 'close-smoke-second.md'); @@ -68,6 +78,17 @@ function createFixture(userData) { return { doc, secondDoc, largeDoc }; } +function assertPersistedDocument(filePath, expected) { + const actual = readFileSync(filePath, 'utf8'); + if (expected.kind === 'large') { + if (!actual.includes(expected.marker) || actual.length <= expected.originalLength) { + throw new Error(`large save did not persist the edited marker for ${filePath}`); + } + return; + } + if (actual !== expected.content) throw new Error(`save did not persist ${filePath}`); +} + async function worker() { const { app, BrowserWindow, Menu } = electron; require(resolve(REPO, 'dist/main/main.js')); @@ -79,18 +100,27 @@ async function worker() { const dirtyWindows = base.startsWith('quit') ? await waitFor('two windows', () => BrowserWindow.getAllWindows().filter((candidate) => !candidate.isDestroyed()).length === 2 ? BrowserWindow.getAllWindows() : null) : [win]; + const expectedByPath = new Map(); await Promise.all(dirtyWindows.map((dirtyWindow) => waitFor('editor', () => dirtyWindow.webContents.executeJavaScript(`Boolean(document.querySelector('.cm-content'))`), ))); - for (const dirtyWindow of dirtyWindows) { + if (scenario.endsWith('-large')) { + await Promise.all(dirtyWindows.map((dirtyWindow) => waitFor('large document content', async () => + (await editorText(dirtyWindow)).includes('# Close smoke large'), + ))); + } + for (const [index, dirtyWindow] of dirtyWindows.entries()) { + const marker = `close smoke ${scenario} ${dirtyWindow.id}`; + const path = index === 0 ? documentPath : secondDocumentPath; + const originalLength = scenario.endsWith('-large') ? readFileSync(path, 'utf8').length : null; if (scenario.endsWith('-large')) { - dirtyWindow.webContents.insertText(`\nclose smoke ${scenario} ${dirtyWindow.id}\n`); + await appendEditorText(dirtyWindow, marker); + expectedByPath.set(path, { kind: 'large', marker, originalLength }); } else { - dirtyWindow.webContents.sendInputEvent({ type: 'keyDown', keyCode: 'A', modifiers: ['meta'] }); - dirtyWindow.webContents.sendInputEvent({ type: 'keyUp', keyCode: 'A', modifiers: ['meta'] }); - await delay(30); - dirtyWindow.webContents.insertText(`close smoke ${scenario} ${dirtyWindow.id}`); + await replaceEditorText(dirtyWindow, marker); + expectedByPath.set(path, { kind: 'exact', content: marker }); } + if (!(await editorText(dirtyWindow)).includes(marker)) throw new Error(`${scenario} edit marker was not applied`); } await delay(100); @@ -120,12 +150,27 @@ async function worker() { console.log('[close-dialog-smoke] quit-discard-closed-two-dirty-windows'); return; } + if (base === 'quit-save') { + const closed = new Promise((resolveClosed) => app.once('window-all-closed', resolveClosed)); + app.quit(); + await Promise.race([ + closed, + delay(20_000).then(() => { throw new Error('quit save did not close both dirty windows'); }), + ]); + for (const [path, expected] of expectedByPath) assertPersistedDocument(path, expected); + console.log('[close-dialog-smoke] quit-save-closed-two-dirty-windows'); + return; + } const closed = new Promise((resolveClosed) => win.once('closed', resolveClosed)); win.close(); await Promise.race([ closed, delay(20_000).then(() => { throw new Error(`${scenario} did not close the window`); }), ]); + if (base === 'save') { + const expected = expectedByPath.get(documentPath); + if (expected != null) assertPersistedDocument(documentPath, expected); + } console.log(`[close-dialog-smoke] ${scenario}-closed-window`); app.exit(0); } @@ -282,7 +327,7 @@ if (scenario) { } else { void (async () => { if (!existsSync(resolve(REPO, 'dist/main/main.js'))) throw new Error('dist/main/main.js missing; build the app before smoke execution'); - // The legacy seven scenarios intentionally share ONE userData in their + // The legacy scenarios intentionally share ONE userData in their // historical order, exactly as before the shutdown pairs were added: a // per-scenario cold userData exposes a pre-existing cold-start save-close // delay (>20s on some machines) that made `save`/`save-large` flaky. @@ -290,13 +335,13 @@ if (scenario) { // pairs below, which each use their own pair-local userData. const userData = mkdtempSync(join(tmpdir(), 'notepad-ai-close-smoke-')); const { doc, secondDoc, largeDoc } = createFixture(userData); - for (const choice of ['discard', 'save', 'cancel', 'quit-cancel', 'quit-discard', 'discard-large', 'save-large']) { + for (const choice of ['discard', 'save', 'cancel', 'quit-cancel', 'quit-discard', 'quit-save', 'discard-large', 'save-large']) { const base = choice.replace(/-large$/, ''); await waitForWorker(choice, { NOTEPAD_AI_CLOSE_SMOKE_SCENARIO: choice, NOTEPAD_AI_CLOSE_SMOKE_DOCUMENT: choice.endsWith('-large') ? largeDoc : doc, NOTEPAD_AI_CLOSE_SMOKE_SECOND_DOCUMENT: secondDoc, - NOTEPAD_AI_CLOSE_DIALOG_CHOICE: base === 'quit-discard' ? 'discard' : base === 'quit-cancel' ? 'cancel' : base, + NOTEPAD_AI_CLOSE_DIALOG_CHOICE: base === 'quit-discard' ? 'discard' : base === 'quit-cancel' ? 'cancel' : base === 'quit-save' ? 'save' : base, NOTEPAD_AI_USERDATA: userData, }); console.log(`[close-dialog-smoke] ${choice}=PASS`); diff --git a/src/__tests__/app-windows-discard.test.ts b/src/__tests__/app-windows-discard.test.ts index 903bc21..d11436b 100644 --- a/src/__tests__/app-windows-discard.test.ts +++ b/src/__tests__/app-windows-discard.test.ts @@ -91,7 +91,12 @@ const electron = vi.hoisted(() => { }; }); +const appLog = vi.hoisted(() => ({ + logWarn: vi.fn(), +})); + vi.mock('electron', () => electron); +vi.mock('../main/app-log', () => appLog); import type { WindowRecord } from '../main/window-registry'; @@ -203,7 +208,10 @@ async function setup( } describe('discard close IPC waiters', () => { - beforeEach(() => electron.reset()); + beforeEach(() => { + electron.reset(); + appLog.logWarn.mockReset(); + }); it('approves a save only after a fresh matching post-save revision', async () => { let stateQueries = 0; const commitQuitSession = vi.fn(async () => {}); @@ -508,6 +516,23 @@ describe('discard close IPC waiters', () => { vi.useRealTimers(); } }); + it('logs a renderer state timeout while keeping close approval denied', async () => { + vi.useFakeTimers(); + try { + const { appWindows } = await setup(() => {}, async () => 'cancel', 1, () => true, async () => {}, async () => {}, async () => {}, () => {}); + const approval = appWindows.approveAllForQuit('quit'); + + await vi.advanceTimersByTimeAsync(400); + await expect(approval).resolves.toBe(false); + expect(appLog.logWarn).toHaveBeenCalledWith( + 'close', + 'renderer state response timed out', + expect.objectContaining({ webContentsId: 1001, timeoutMs: 400 }), + ); + } finally { + vi.useRealTimers(); + } + }); it('times out a missing rollback ACK, releases the session fence, and denies teardown', async () => { vi.useFakeTimers(); try { diff --git a/src/main/app-windows.ts b/src/main/app-windows.ts index 5ebfe6f..3738090 100644 --- a/src/main/app-windows.ts +++ b/src/main/app-windows.ts @@ -143,10 +143,12 @@ export function createAppWindows({ webContentsId: number, send: () => void, ) => new Promise((resolve) => { + const timeoutMs = 400; const timer = setTimeout(() => { pendingState.delete(id); + void logWarn('close', 'renderer state response timed out', { requestId: id, webContentsId, timeoutMs }); resolve(null); - }, 400); + }, timeoutMs); pendingState.set(id, { webContentsId, resolve: (value) => { @@ -315,7 +317,11 @@ export function createAppWindows({ }; pendingSave.set(id, pending); win.once('closed', onDestroyed); - timer = setTimeout(() => pending.resolve({ saved: false, committedRevision: null }), Math.max(0, deadline - Date.now())); + const timeoutMs = Math.max(0, deadline - Date.now()); + timer = setTimeout(() => { + void logWarn('close', 'renderer save response timed out', { requestId: id, webContentsId: pending.webContentsId, timeoutMs }); + pending.resolve({ saved: false, committedRevision: null }); + }, timeoutMs); win.webContents.send('close:save', { requestId: id, revision }); }); }; @@ -350,10 +356,12 @@ export function createAppWindows({ if (!activeLease(win, leaseId)) return Promise.resolve(false); if (leaseId && mainOwnedShutdownLeases.has(leaseId)) return Promise.resolve(true); return new Promise((resolve) => { + const timeoutMs = 400; const timer = setTimeout(() => { pendingAuthorize.delete(leaseId!); + void logWarn('close', 'renderer authorize response timed out', { requestId: leaseId, webContentsId: win.webContents.id, timeoutMs }); resolve(false); - }, 400); + }, timeoutMs); pendingAuthorize.set(leaseId!, { webContentsId: win.webContents.id, resolve: (valid) => { @@ -369,10 +377,12 @@ export function createAppWindows({ if (!activeLease(win, leaseId)) return Promise.resolve(false); if (leaseId && mainOwnedShutdownLeases.has(leaseId)) return Promise.resolve(true); return new Promise((resolve) => { + const timeoutMs = 400; const timer = setTimeout(() => { pendingConsume.delete(leaseId!); + void logWarn('close', 'renderer consume response timed out', { requestId: leaseId, webContentsId: win.webContents.id, timeoutMs }); resolve(false); - }, 400); + }, timeoutMs); pendingConsume.set(leaseId!, { webContentsId: win.webContents.id, resolve: (consumed) => { diff --git a/src/renderer/close-save-handler.test.ts b/src/renderer/close-save-handler.test.ts new file mode 100644 index 0000000..b2f5fb1 --- /dev/null +++ b/src/renderer/close-save-handler.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, it, vi } from 'vitest'; +import { handleCloseSaveRequest } from './close-save-handler'; + +describe('handleCloseSaveRequest', () => { + it('reports document save before waiting for session snapshot persistence', async () => { + let releaseSnapshot!: () => void; + const snapshotFlush = new Promise((resolve) => { releaseSnapshot = resolve; }); + const send = vi.fn(); + const request = handleCloseSaveRequest({ + requestId: 'save:1', + requestedRevision: 4, + save: async () => 4, + isDirty: () => false, + flushSessionSnapshot: () => snapshotFlush, + onSnapshotFlushError: vi.fn(), + send, + }); + + await vi.waitFor(() => expect(send).toHaveBeenCalledWith('save:1', { saved: true, committedRevision: 4 })); + releaseSnapshot(); + await request; + }); + + it('keeps a snapshot flush failure after a successful document ACK', async () => { + const send = vi.fn(); + const onSnapshotFlushError = vi.fn(); + await handleCloseSaveRequest({ + requestId: 'save:2', + requestedRevision: 4, + save: async () => 4, + isDirty: () => false, + flushSessionSnapshot: async () => { throw new Error('session write failed'); }, + onSnapshotFlushError, + send, + }); + + expect(send).toHaveBeenCalledTimes(1); + expect(send).toHaveBeenCalledWith('save:2', { saved: true, committedRevision: 4 }); + expect(onSnapshotFlushError).toHaveBeenCalledOnce(); + }); +}); diff --git a/src/renderer/close-save-handler.ts b/src/renderer/close-save-handler.ts new file mode 100644 index 0000000..dc2197a --- /dev/null +++ b/src/renderer/close-save-handler.ts @@ -0,0 +1,50 @@ +type CloseSaveResult = { + saved: boolean; + committedRevision: number | null; +}; + +export type CloseSaveRequest = { + requestId: string; + requestedRevision: number; + save: () => Promise; + isDirty: () => boolean; + flushSessionSnapshot?: () => Promise; + onSnapshotFlushError?: (error: unknown) => void; + send: (requestId: string, result: CloseSaveResult) => void; +}; + +/** + * Complete the authoritative document save handshake before doing best-effort + * session persistence. The main process can then finish the close transaction + * without waiting on the recovery snapshot write. + */ +export async function handleCloseSaveRequest({ + requestId, + requestedRevision, + save, + isDirty, + flushSessionSnapshot, + onSnapshotFlushError, + send, +}: CloseSaveRequest): Promise { + let committedRevision: number | null; + try { + committedRevision = await save(); + } catch { + send(requestId, { saved: false, committedRevision: null }); + return; + } + + const saved = committedRevision !== null && committedRevision >= requestedRevision; + send(requestId, { + saved, + committedRevision: saved ? committedRevision : null, + }); + + if (!saved || isDirty() || !flushSessionSnapshot) return; + try { + await flushSessionSnapshot(); + } catch (error) { + onSnapshotFlushError?.(error); + } +} diff --git a/src/renderer/main.ts b/src/renderer/main.ts index 06ec830..f31ab43 100644 --- a/src/renderer/main.ts +++ b/src/renderer/main.ts @@ -30,6 +30,7 @@ import { folderFromFilePath, initProjectWizardFlow } from './project-wizard-flow import { initSessionSnapshot } from './session-snapshot'; import { initUpdateBanner } from './update-banner'; import { handleCloseQueryState } from './close-query-state'; +import { handleCloseSaveRequest } from './close-save-handler'; const workspace = document.querySelector('.workspace') as HTMLElement; const editorHost = document.getElementById('editor-host') as HTMLDivElement; @@ -308,17 +309,14 @@ window.api.onCloseQuiesceRollback(({ requestId }) => { }); window.api.sendCloseQuiesceReady(); window.api.onCloseSave((requestId, requestedRevision) => { - void (async () => { - const committedRevision = await docLifecycle.save(); - const saved = committedRevision !== null && committedRevision >= requestedRevision; - if (saved && !ctx.dirty && sessionSnapshot) await sessionSnapshot.flushSessionSnapshot(); - window.api.sendCloseSaveResult(requestId, { - saved, - // This is the actual document revision written, not merely the request. - committedRevision: saved ? committedRevision : null, - }); - })().catch(() => { - window.api.sendCloseSaveResult(requestId, { saved: false, committedRevision: null }); + void handleCloseSaveRequest({ + requestId, + requestedRevision, + save: () => docLifecycle.save(), + isDirty: () => ctx.dirty, + flushSessionSnapshot: () => sessionSnapshot.flushSessionSnapshot(), + onSnapshotFlushError: (error) => console.warn('[session] close snapshot flush failed:', error), + send: window.api.sendCloseSaveResult, }); }); window.api.onCloseAuthorize((requestId) => {