diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 349246d..065fb3a 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -496,7 +496,8 @@ sequenceDiagram CLI->>GS: report(filePath, sheetUrl, credentials, config, limit, all?, silent?) GS->>GS: authentication(credentials)(OAuth2) - GS->>Archive: getArchive(filePath) + GS->>Archive: getArchive(filePath) → { archive, removeSignalHandlers } + Note over GS: try/finally で cleanup を保証 GS->>GS: loadConfig(configPath) GS->>Archive: getPluginReports(archive) @@ -513,6 +514,7 @@ sequenceDiagram Note over GS,Sheets: silent=false 時: Lanes で進捗表示 + レート制限カウントダウン end + GS->>GS: removeSignalHandlers() GS->>Archive: archive.close() ``` diff --git a/packages/@nitpicker/cli/src/commands/crawl.spec.ts b/packages/@nitpicker/cli/src/commands/crawl.spec.ts index f776aef..b00e4b1 100644 --- a/packages/@nitpicker/cli/src/commands/crawl.spec.ts +++ b/packages/@nitpicker/cli/src/commands/crawl.spec.ts @@ -177,6 +177,27 @@ describe('startCrawl', () => { expect(result).toBe('/tmp/test.nitpicker'); }); + it('完了後にシグナルリスナーが蓄積しない', async () => { + const { startCrawl } = await import('./crawl.js'); + const before = process.listenerCount('SIGINT'); + + await startCrawl(['https://example.com'], createFlags()); + await startCrawl(['https://example.com'], createFlags()); + await startCrawl(['https://example.com'], createFlags()); + + expect(process.listenerCount('SIGINT')).toBe(before); + }); + + it('eventAssignments がエラーでもシグナルリスナーが解除される', async () => { + mockEventAssignments.mockRejectedValueOnce(new Error('scrape failed')); + const { startCrawl } = await import('./crawl.js'); + const before = process.listenerCount('SIGINT'); + + await startCrawl(['https://example.com'], createFlags()).catch(() => {}); + + expect(process.listenerCount('SIGINT')).toBe(before); + }); + it('イベントエラー発生時に CrawlAggregateError をスローする', async () => { mockEventAssignments.mockRejectedValueOnce(new Error('scrape failed')); diff --git a/packages/@nitpicker/cli/src/commands/crawl.ts b/packages/@nitpicker/cli/src/commands/crawl.ts index e9e6380..1c654b8 100644 --- a/packages/@nitpicker/cli/src/commands/crawl.ts +++ b/packages/@nitpicker/cli/src/commands/crawl.ts @@ -143,13 +143,16 @@ type LogType = 'verbose' | 'normal' | 'silent'; * crawl via {@link CrawlerOrchestrator.abort}, then kill zombie Chromium * processes and exit. The abort signal propagates through the dealer's * AbortSignal mechanism so no new workers are launched. + * + * Signal handlers are automatically removed in a `finally` block when + * the event assignment pipeline completes or throws. * @param trigger - Display label for the crawl (URL or stub file path) * @param orchestrator - The initialized CrawlerOrchestrator instance * @param config - The resolved archive configuration * @param logType - Output verbosity level - * @returns A promise from the event assignment pipeline. + * @returns A promise that resolves when the event assignment pipeline completes. */ -function run( +async function run( trigger: string, orchestrator: CrawlerOrchestrator, config: Config, @@ -160,16 +163,22 @@ function run( orchestrator.garbageCollect(); process.exit(); }; - process.on('SIGINT', killed); - process.on('SIGBREAK', killed); - process.on('SIGHUP', killed); - process.on('SIGABRT', killed); + const signals: NodeJS.Signals[] = ['SIGINT', 'SIGBREAK', 'SIGHUP', 'SIGABRT']; + for (const signal of signals) { + process.on(signal, killed); + } const head = [ `🐳 ${trigger} (New scraping)`, ...Object.entries(config).map(([key, value]) => ` ${key}: ${value}`), ]; - return eventAssignments(orchestrator, head, logType); + try { + return await eventAssignments(orchestrator, head, logType); + } finally { + for (const signal of signals) { + process.removeListener(signal, killed); + } + } } /** diff --git a/packages/@nitpicker/report-google-sheets/src/archive.spec.ts b/packages/@nitpicker/report-google-sheets/src/archive.spec.ts new file mode 100644 index 0000000..b95085b --- /dev/null +++ b/packages/@nitpicker/report-google-sheets/src/archive.spec.ts @@ -0,0 +1,71 @@ +import { afterEach, beforeEach, describe, it, expect, vi } from 'vitest'; + +const mockOpen = vi.fn(); +const mockClose = vi.fn(); + +vi.mock('@nitpicker/crawler', () => ({ + Archive: { + open: mockOpen, + }, +})); + +vi.mock('./debug.js', () => ({ + archiveLog: vi.fn(), +})); + +describe('getArchive', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockOpen.mockResolvedValue({ close: mockClose }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('archive と removeSignalHandlers を含むオブジェクトを返す', async () => { + const { getArchive } = await import('./archive.js'); + const handle = await getArchive('/tmp/test.nitpicker'); + + expect(handle).toHaveProperty('archive'); + expect(handle).toHaveProperty('removeSignalHandlers'); + expect(typeof handle.removeSignalHandlers).toBe('function'); + }); + + it('シグナルリスナーを登録する', async () => { + const { getArchive } = await import('./archive.js'); + const before = process.listenerCount('SIGINT'); + + await getArchive('/tmp/test.nitpicker'); + + expect(process.listenerCount('SIGINT')).toBe(before + 1); + expect(process.listenerCount('SIGHUP')).toBeGreaterThan(0); + expect(process.listenerCount('SIGABRT')).toBeGreaterThan(0); + }); + + it('removeSignalHandlers を呼ぶとシグナルリスナーが解除される', async () => { + const { getArchive } = await import('./archive.js'); + const before = process.listenerCount('SIGINT'); + + const { removeSignalHandlers } = await getArchive('/tmp/test.nitpicker'); + removeSignalHandlers(); + + expect(process.listenerCount('SIGINT')).toBe(before); + }); + + it('複数回呼び出してもリスナーが蓄積しない(removeSignalHandlers で解除した場合)', async () => { + const { getArchive } = await import('./archive.js'); + const before = process.listenerCount('SIGINT'); + + const handle1 = await getArchive('/tmp/test1.nitpicker'); + handle1.removeSignalHandlers(); + + const handle2 = await getArchive('/tmp/test2.nitpicker'); + handle2.removeSignalHandlers(); + + const handle3 = await getArchive('/tmp/test3.nitpicker'); + handle3.removeSignalHandlers(); + + expect(process.listenerCount('SIGINT')).toBe(before); + }); +}); diff --git a/packages/@nitpicker/report-google-sheets/src/archive.ts b/packages/@nitpicker/report-google-sheets/src/archive.ts index 3888a77..8e9bdc8 100644 --- a/packages/@nitpicker/report-google-sheets/src/archive.ts +++ b/packages/@nitpicker/report-google-sheets/src/archive.ts @@ -2,13 +2,26 @@ import { Archive } from '@nitpicker/crawler'; import { archiveLog } from './debug.js'; +/** + * Result of opening an archive with signal handlers registered. + */ +export interface ArchiveHandle { + /** The opened archive instance. */ + readonly archive: Archive; + /** Removes the registered signal handlers. Call this when the archive is no longer in use. */ + readonly removeSignalHandlers: () => void; +} + /** * Opens a `.nitpicker` archive file and registers process signal handlers * to ensure the archive is closed gracefully on unexpected termination. + * + * The caller must invoke `removeSignalHandlers()` when the archive is no + * longer needed to avoid listener accumulation. * @param filePath - Path to the `.nitpicker` archive file - * @returns The opened `Archive` instance with plugin data loaded + * @returns An {@link ArchiveHandle} containing the archive and a cleanup function. */ -export async function getArchive(filePath: string) { +export async function getArchive(filePath: string): Promise { archiveLog('Open file: %s', filePath); const archive = await Archive.open({ filePath, @@ -16,16 +29,23 @@ export async function getArchive(filePath: string) { }); archiveLog('File open succeeded'); + const signals: NodeJS.Signals[] = ['SIGINT', 'SIGBREAK', 'SIGHUP', 'SIGABRT']; + const close = async () => { await archive.close(); process.exit(); }; archiveLog('Bind close method to SIGINT, SIGBREAK, SIGHUP, SIGABRT events'); - process.on('SIGINT', close); - process.on('SIGBREAK', close); - process.on('SIGHUP', close); - process.on('SIGABRT', close); + for (const signal of signals) { + process.on(signal, close); + } + + const removeSignalHandlers = () => { + for (const signal of signals) { + process.removeListener(signal, close); + } + }; - return archive; + return { archive, removeSignalHandlers }; } diff --git a/packages/@nitpicker/report-google-sheets/src/report.spec.ts b/packages/@nitpicker/report-google-sheets/src/report.spec.ts index f6e3740..1d0ef35 100644 --- a/packages/@nitpicker/report-google-sheets/src/report.spec.ts +++ b/packages/@nitpicker/report-google-sheets/src/report.spec.ts @@ -14,9 +14,15 @@ vi.mock('@d-zero/google-sheets', () => ({ }, })); +const { mockArchiveClose, mockRemoveSignalHandlers } = vi.hoisted(() => ({ + mockArchiveClose: vi.fn(), + mockRemoveSignalHandlers: vi.fn(), +})); + vi.mock('./archive.js', () => ({ getArchive: vi.fn().mockResolvedValue({ - close: vi.fn(), + archive: { close: mockArchiveClose }, + removeSignalHandlers: mockRemoveSignalHandlers, }), })); @@ -87,6 +93,23 @@ describe('report', () => { expect(lanesSpy).toHaveBeenCalled(); }); + it('正常完了時に removeSignalHandlers と archive.close を呼び出す', async () => { + await report({ ...baseParams, all: true }); + + expect(mockRemoveSignalHandlers).toHaveBeenCalledTimes(1); + expect(mockArchiveClose).toHaveBeenCalledTimes(1); + }); + + it('createSheets が例外をスローしても removeSignalHandlers と archive.close を呼び出す', async () => { + const { createSheets } = await import('./sheets/create-sheets.js'); + vi.mocked(createSheets).mockRejectedValueOnce(new Error('sheets error')); + + await expect(report({ ...baseParams, all: true })).rejects.toThrow('sheets error'); + + expect(mockRemoveSignalHandlers).toHaveBeenCalledTimes(1); + expect(mockArchiveClose).toHaveBeenCalledTimes(1); + }); + it('passes all 9 sheets to createSheets when all=true', async () => { const { createSheets } = await import('./sheets/create-sheets.js'); diff --git a/packages/@nitpicker/report-google-sheets/src/report.ts b/packages/@nitpicker/report-google-sheets/src/report.ts index f52d81f..3b9bf2c 100644 --- a/packages/@nitpicker/report-google-sheets/src/report.ts +++ b/packages/@nitpicker/report-google-sheets/src/report.ts @@ -83,171 +83,172 @@ export async function report(params: ReportParams) { log('Authentication succeeded'); log('Opening archive: %s', filePath); - const archive = await getArchive(filePath); + const { archive, removeSignalHandlers } = await getArchive(filePath); log('Archive opened'); - log('Loading config'); - const config = await loadConfig(configPath); - log('Config loaded'); - - const plugins = config.plugins?.analyze - ? Object.keys(config.plugins.analyze) - : undefined; - log('Loaded plugins: %O', plugins); - - log('Loading plugin reports'); - const reports = await getPluginReports(archive /*plugins*/); - log('Plugin reports loaded: %d', reports.length); - - const sheets = new Sheets(sheetUrl, auth); - - log('Reporting starts'); - - const sheetNames = [ - 'Page List' as const, - 'Links' as const, - 'Resources' as const, - 'Images' as const, - 'Violations' as const, - 'Discrepancies' as const, - 'Summary' as const, - 'Referrers Relational Table' as const, - 'Resources Relational Table' as const, - ]; - type SheetNames = typeof sheetNames; - - let selectedSheetNames: SheetNames; - - if (all) { - log('All sheets selected (--all or non-TTY)'); - selectedSheetNames = sheetNames; - } else { - log('Choice creating data'); - const chosenSheets = await enquirer - .prompt<{ sheetName: SheetNames }>([ - { - message: 'What do you report?', - name: 'sheetName', - type: 'multiselect', - choices: sheetNames, - }, - ]) - .catch(() => { - // enquirer v2.4.1: Ctrl+C 後に readline を二重 close して - // ERR_USE_AFTER_CLOSE が unhandled rejection になるため、 - // 即座に終了して回避する - process.exit(0); - }); - - if (!chosenSheets) { + try { + log('Loading config'); + const config = await loadConfig(configPath); + log('Config loaded'); + + const plugins = config.plugins?.analyze + ? Object.keys(config.plugins.analyze) + : undefined; + log('Loaded plugins: %O', plugins); + + log('Loading plugin reports'); + const reports = await getPluginReports(archive /*plugins*/); + log('Plugin reports loaded: %d', reports.length); + + const sheets = new Sheets(sheetUrl, auth); + + log('Reporting starts'); + + const sheetNames = [ + 'Page List' as const, + 'Links' as const, + 'Resources' as const, + 'Images' as const, + 'Violations' as const, + 'Discrepancies' as const, + 'Summary' as const, + 'Referrers Relational Table' as const, + 'Resources Relational Table' as const, + ]; + type SheetNames = typeof sheetNames; + + let selectedSheetNames: SheetNames; + + if (all) { + log('All sheets selected (--all or non-TTY)'); + selectedSheetNames = sheetNames; + } else { log('Choice creating data'); - archiveLog('Closes file'); - await archive.close(); - return; - } + const chosenSheets = await enquirer + .prompt<{ sheetName: SheetNames }>([ + { + message: 'What do you report?', + name: 'sheetName', + type: 'multiselect', + choices: sheetNames, + }, + ]) + .catch(() => { + // enquirer v2.4.1: Ctrl+C 後に readline を二重 close して + // ERR_USE_AFTER_CLOSE が unhandled rejection になるため、 + // 即座に終了して回避する + process.exit(0); + }); + + if (!chosenSheets) { + log('Choice creating data'); + return; + } - selectedSheetNames = chosenSheets.sheetName; - } + selectedSheetNames = chosenSheets.sheetName; + } - log('Chosen sheets: %O', selectedSheetNames); + log('Chosen sheets: %O', selectedSheetNames); - const createSheetList: CreateSheet[] = []; + const createSheetList: CreateSheet[] = []; - if (selectedSheetNames.includes('Page List')) { - createSheetList.push(createPageList); - } + if (selectedSheetNames.includes('Page List')) { + createSheetList.push(createPageList); + } - if (selectedSheetNames.includes('Links')) { - createSheetList.push(createLinks); - } + if (selectedSheetNames.includes('Links')) { + createSheetList.push(createLinks); + } - if (selectedSheetNames.includes('Discrepancies')) { - createSheetList.push(createDiscrepancies); - } + if (selectedSheetNames.includes('Discrepancies')) { + createSheetList.push(createDiscrepancies); + } - if (selectedSheetNames.includes('Violations')) { - createSheetList.push(createViolations); - } + if (selectedSheetNames.includes('Violations')) { + createSheetList.push(createViolations); + } - if (selectedSheetNames.includes('Referrers Relational Table')) { - createSheetList.push(createReferrersRelationalTable); - } + if (selectedSheetNames.includes('Referrers Relational Table')) { + createSheetList.push(createReferrersRelationalTable); + } - if (selectedSheetNames.includes('Resources Relational Table')) { - createSheetList.push(createResourcesRelationalTable); - } + if (selectedSheetNames.includes('Resources Relational Table')) { + createSheetList.push(createResourcesRelationalTable); + } - if (selectedSheetNames.includes('Resources')) { - createSheetList.push(createResources); - } + if (selectedSheetNames.includes('Resources')) { + createSheetList.push(createResources); + } - if (selectedSheetNames.includes('Images')) { - createSheetList.push(createImageList); - } + if (selectedSheetNames.includes('Images')) { + createSheetList.push(createImageList); + } - if (!silent) { - // eslint-disable-next-line no-console - console.log(`\nGenerating ${createSheetList.length} sheet(s)...\n`); - } + if (!silent) { + // eslint-disable-next-line no-console + console.log(`\nGenerating ${createSheetList.length} sheet(s)...\n`); + } - const lanes = silent - ? null - : new Lanes({ verbose: !process.stdout.isTTY, indent: ' ' }); - log('Lanes created (verbose: %s, silent: %s)', !process.stdout.isTTY, !!silent); - - const RATE_LIMIT_LANE = 10_000; - let countdownSeq = 0; - let waitingCount = 0; - - if (lanes) { - sheets.onLog = (message: ErrorHandlerMessage) => { - if (message.waiting && message.waitTime) { - waitingCount++; - const id = `rateLimit_${countdownSeq++}`; - const label = - message.message === 'TooManyRequestError' - ? 'Too Many Requests (429)' - : message.message === 'UserRateLimitExceededError' - ? 'Rate Limit Exceeded (403)' - : 'Connection Reset'; - lanes.update( - RATE_LIMIT_LANE, - c.yellow(`${label}: waiting %countdown(${message.waitTime}, ${id}, s)%s`), - ); - } else { - waitingCount--; - if (waitingCount <= 0) { - waitingCount = 0; - lanes.delete(RATE_LIMIT_LANE); + const lanes = silent + ? null + : new Lanes({ verbose: !process.stdout.isTTY, indent: ' ' }); + log('Lanes created (verbose: %s, silent: %s)', !process.stdout.isTTY, !!silent); + + const RATE_LIMIT_LANE = 10_000; + let countdownSeq = 0; + let waitingCount = 0; + + if (lanes) { + sheets.onLog = (message: ErrorHandlerMessage) => { + if (message.waiting && message.waitTime) { + waitingCount++; + const id = `rateLimit_${countdownSeq++}`; + const label = + message.message === 'TooManyRequestError' + ? 'Too Many Requests (429)' + : message.message === 'UserRateLimitExceededError' + ? 'Rate Limit Exceeded (403)' + : 'Connection Reset'; + lanes.update( + RATE_LIMIT_LANE, + c.yellow(`${label}: waiting %countdown(${message.waitTime}, ${id}, s)%s`), + ); + } else { + waitingCount--; + if (waitingCount <= 0) { + waitingCount = 0; + lanes.delete(RATE_LIMIT_LANE); + } } - } - }; - } + }; + } - log('Reporting starts (limit: %d)', limit); - try { - await createSheets({ - sheets, - archive, - reports, - limit, - createSheetList, - options: lanes ? { lanes } : undefined, - }); - } finally { - lanes?.close(); - } - log('Reporting done'); - if (!silent) { - // eslint-disable-next-line no-console - console.log('\nReport complete.'); - } + log('Reporting starts (limit: %d)', limit); + try { + await createSheets({ + sheets, + archive, + reports, + limit, + createSheetList, + options: lanes ? { lanes } : undefined, + }); + } finally { + lanes?.close(); + } + log('Reporting done'); + if (!silent) { + // eslint-disable-next-line no-console + console.log('\nReport complete.'); + } - if (selectedSheetNames.includes('Summary')) { - await addToSummary(/*sheets, archive, reports*/); + if (selectedSheetNames.includes('Summary')) { + await addToSummary(/*sheets, archive, reports*/); + } + } finally { + archiveLog('Closes file'); + removeSignalHandlers(); + await archive.close(); + log('Done'); } - - archiveLog('Closes file'); - await archive.close(); - log('Done'); }