Skip to content

Commit b0fbf45

Browse files
authored
Merge pull request #57 from d-zero-dev/fix/process-signal-listener-leak
fix: remove process signal listeners to prevent MaxListenersExceededWarning
2 parents 607d06b + 2377a93 commit b0fbf45

7 files changed

Lines changed: 310 additions & 163 deletions

File tree

ARCHITECTURE.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -496,7 +496,8 @@ sequenceDiagram
496496
497497
CLI->>GS: report(filePath, sheetUrl, credentials, config, limit, all?, silent?)
498498
GS->>GS: authentication(credentials)(OAuth2)
499-
GS->>Archive: getArchive(filePath)
499+
GS->>Archive: getArchive(filePath) → { archive, removeSignalHandlers }
500+
Note over GS: try/finally で cleanup を保証
500501
GS->>GS: loadConfig(configPath)
501502
GS->>Archive: getPluginReports(archive)
502503
@@ -513,6 +514,7 @@ sequenceDiagram
513514
Note over GS,Sheets: silent=false 時: Lanes で進捗表示 + レート制限カウントダウン
514515
end
515516
517+
GS->>GS: removeSignalHandlers()
516518
GS->>Archive: archive.close()
517519
```
518520

packages/@nitpicker/cli/src/commands/crawl.spec.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,27 @@ describe('startCrawl', () => {
177177
expect(result).toBe('/tmp/test.nitpicker');
178178
});
179179

180+
it('完了後にシグナルリスナーが蓄積しない', async () => {
181+
const { startCrawl } = await import('./crawl.js');
182+
const before = process.listenerCount('SIGINT');
183+
184+
await startCrawl(['https://example.com'], createFlags());
185+
await startCrawl(['https://example.com'], createFlags());
186+
await startCrawl(['https://example.com'], createFlags());
187+
188+
expect(process.listenerCount('SIGINT')).toBe(before);
189+
});
190+
191+
it('eventAssignments がエラーでもシグナルリスナーが解除される', async () => {
192+
mockEventAssignments.mockRejectedValueOnce(new Error('scrape failed'));
193+
const { startCrawl } = await import('./crawl.js');
194+
const before = process.listenerCount('SIGINT');
195+
196+
await startCrawl(['https://example.com'], createFlags()).catch(() => {});
197+
198+
expect(process.listenerCount('SIGINT')).toBe(before);
199+
});
200+
180201
it('イベントエラー発生時に CrawlAggregateError をスローする', async () => {
181202
mockEventAssignments.mockRejectedValueOnce(new Error('scrape failed'));
182203

packages/@nitpicker/cli/src/commands/crawl.ts

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -143,13 +143,16 @@ type LogType = 'verbose' | 'normal' | 'silent';
143143
* crawl via {@link CrawlerOrchestrator.abort}, then kill zombie Chromium
144144
* processes and exit. The abort signal propagates through the dealer's
145145
* AbortSignal mechanism so no new workers are launched.
146+
*
147+
* Signal handlers are automatically removed in a `finally` block when
148+
* the event assignment pipeline completes or throws.
146149
* @param trigger - Display label for the crawl (URL or stub file path)
147150
* @param orchestrator - The initialized CrawlerOrchestrator instance
148151
* @param config - The resolved archive configuration
149152
* @param logType - Output verbosity level
150-
* @returns A promise from the event assignment pipeline.
153+
* @returns A promise that resolves when the event assignment pipeline completes.
151154
*/
152-
function run(
155+
async function run(
153156
trigger: string,
154157
orchestrator: CrawlerOrchestrator,
155158
config: Config,
@@ -160,16 +163,22 @@ function run(
160163
orchestrator.garbageCollect();
161164
process.exit();
162165
};
163-
process.on('SIGINT', killed);
164-
process.on('SIGBREAK', killed);
165-
process.on('SIGHUP', killed);
166-
process.on('SIGABRT', killed);
166+
const signals: NodeJS.Signals[] = ['SIGINT', 'SIGBREAK', 'SIGHUP', 'SIGABRT'];
167+
for (const signal of signals) {
168+
process.on(signal, killed);
169+
}
167170

168171
const head = [
169172
`🐳 ${trigger} (New scraping)`,
170173
...Object.entries(config).map(([key, value]) => ` ${key}: ${value}`),
171174
];
172-
return eventAssignments(orchestrator, head, logType);
175+
try {
176+
return await eventAssignments(orchestrator, head, logType);
177+
} finally {
178+
for (const signal of signals) {
179+
process.removeListener(signal, killed);
180+
}
181+
}
173182
}
174183

175184
/**
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import { afterEach, beforeEach, describe, it, expect, vi } from 'vitest';
2+
3+
const mockOpen = vi.fn();
4+
const mockClose = vi.fn();
5+
6+
vi.mock('@nitpicker/crawler', () => ({
7+
Archive: {
8+
open: mockOpen,
9+
},
10+
}));
11+
12+
vi.mock('./debug.js', () => ({
13+
archiveLog: vi.fn(),
14+
}));
15+
16+
describe('getArchive', () => {
17+
beforeEach(() => {
18+
vi.clearAllMocks();
19+
mockOpen.mockResolvedValue({ close: mockClose });
20+
});
21+
22+
afterEach(() => {
23+
vi.restoreAllMocks();
24+
});
25+
26+
it('archive と removeSignalHandlers を含むオブジェクトを返す', async () => {
27+
const { getArchive } = await import('./archive.js');
28+
const handle = await getArchive('/tmp/test.nitpicker');
29+
30+
expect(handle).toHaveProperty('archive');
31+
expect(handle).toHaveProperty('removeSignalHandlers');
32+
expect(typeof handle.removeSignalHandlers).toBe('function');
33+
});
34+
35+
it('シグナルリスナーを登録する', async () => {
36+
const { getArchive } = await import('./archive.js');
37+
const before = process.listenerCount('SIGINT');
38+
39+
await getArchive('/tmp/test.nitpicker');
40+
41+
expect(process.listenerCount('SIGINT')).toBe(before + 1);
42+
expect(process.listenerCount('SIGHUP')).toBeGreaterThan(0);
43+
expect(process.listenerCount('SIGABRT')).toBeGreaterThan(0);
44+
});
45+
46+
it('removeSignalHandlers を呼ぶとシグナルリスナーが解除される', async () => {
47+
const { getArchive } = await import('./archive.js');
48+
const before = process.listenerCount('SIGINT');
49+
50+
const { removeSignalHandlers } = await getArchive('/tmp/test.nitpicker');
51+
removeSignalHandlers();
52+
53+
expect(process.listenerCount('SIGINT')).toBe(before);
54+
});
55+
56+
it('複数回呼び出してもリスナーが蓄積しない(removeSignalHandlers で解除した場合)', async () => {
57+
const { getArchive } = await import('./archive.js');
58+
const before = process.listenerCount('SIGINT');
59+
60+
const handle1 = await getArchive('/tmp/test1.nitpicker');
61+
handle1.removeSignalHandlers();
62+
63+
const handle2 = await getArchive('/tmp/test2.nitpicker');
64+
handle2.removeSignalHandlers();
65+
66+
const handle3 = await getArchive('/tmp/test3.nitpicker');
67+
handle3.removeSignalHandlers();
68+
69+
expect(process.listenerCount('SIGINT')).toBe(before);
70+
});
71+
});

packages/@nitpicker/report-google-sheets/src/archive.ts

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,30 +2,50 @@ import { Archive } from '@nitpicker/crawler';
22

33
import { archiveLog } from './debug.js';
44

5+
/**
6+
* Result of opening an archive with signal handlers registered.
7+
*/
8+
export interface ArchiveHandle {
9+
/** The opened archive instance. */
10+
readonly archive: Archive;
11+
/** Removes the registered signal handlers. Call this when the archive is no longer in use. */
12+
readonly removeSignalHandlers: () => void;
13+
}
14+
515
/**
616
* Opens a `.nitpicker` archive file and registers process signal handlers
717
* to ensure the archive is closed gracefully on unexpected termination.
18+
*
19+
* The caller must invoke `removeSignalHandlers()` when the archive is no
20+
* longer needed to avoid listener accumulation.
821
* @param filePath - Path to the `.nitpicker` archive file
9-
* @returns The opened `Archive` instance with plugin data loaded
22+
* @returns An {@link ArchiveHandle} containing the archive and a cleanup function.
1023
*/
11-
export async function getArchive(filePath: string) {
24+
export async function getArchive(filePath: string): Promise<ArchiveHandle> {
1225
archiveLog('Open file: %s', filePath);
1326
const archive = await Archive.open({
1427
filePath,
1528
openPluginData: true,
1629
});
1730
archiveLog('File open succeeded');
1831

32+
const signals: NodeJS.Signals[] = ['SIGINT', 'SIGBREAK', 'SIGHUP', 'SIGABRT'];
33+
1934
const close = async () => {
2035
await archive.close();
2136
process.exit();
2237
};
2338

2439
archiveLog('Bind close method to SIGINT, SIGBREAK, SIGHUP, SIGABRT events');
25-
process.on('SIGINT', close);
26-
process.on('SIGBREAK', close);
27-
process.on('SIGHUP', close);
28-
process.on('SIGABRT', close);
40+
for (const signal of signals) {
41+
process.on(signal, close);
42+
}
43+
44+
const removeSignalHandlers = () => {
45+
for (const signal of signals) {
46+
process.removeListener(signal, close);
47+
}
48+
};
2949

30-
return archive;
50+
return { archive, removeSignalHandlers };
3151
}

packages/@nitpicker/report-google-sheets/src/report.spec.ts

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,15 @@ vi.mock('@d-zero/google-sheets', () => ({
1414
},
1515
}));
1616

17+
const { mockArchiveClose, mockRemoveSignalHandlers } = vi.hoisted(() => ({
18+
mockArchiveClose: vi.fn(),
19+
mockRemoveSignalHandlers: vi.fn(),
20+
}));
21+
1722
vi.mock('./archive.js', () => ({
1823
getArchive: vi.fn().mockResolvedValue({
19-
close: vi.fn(),
24+
archive: { close: mockArchiveClose },
25+
removeSignalHandlers: mockRemoveSignalHandlers,
2026
}),
2127
}));
2228

@@ -87,6 +93,23 @@ describe('report', () => {
8793
expect(lanesSpy).toHaveBeenCalled();
8894
});
8995

96+
it('正常完了時に removeSignalHandlers と archive.close を呼び出す', async () => {
97+
await report({ ...baseParams, all: true });
98+
99+
expect(mockRemoveSignalHandlers).toHaveBeenCalledTimes(1);
100+
expect(mockArchiveClose).toHaveBeenCalledTimes(1);
101+
});
102+
103+
it('createSheets が例外をスローしても removeSignalHandlers と archive.close を呼び出す', async () => {
104+
const { createSheets } = await import('./sheets/create-sheets.js');
105+
vi.mocked(createSheets).mockRejectedValueOnce(new Error('sheets error'));
106+
107+
await expect(report({ ...baseParams, all: true })).rejects.toThrow('sheets error');
108+
109+
expect(mockRemoveSignalHandlers).toHaveBeenCalledTimes(1);
110+
expect(mockArchiveClose).toHaveBeenCalledTimes(1);
111+
});
112+
90113
it('passes all 9 sheets to createSheets when all=true', async () => {
91114
const { createSheets } = await import('./sheets/create-sheets.js');
92115

0 commit comments

Comments
 (0)