Skip to content

Commit 3865a74

Browse files
committed
fix: gracefully handle killing errors
Currently WCS seems to be exiting abruptly when a process isn't killed as expected. This should be a non-error. ``` file:///usr/local/google/home/pgschwendtner/projects/llm-codegen-quality/node_modules/.pnpm/[email protected]_5 rejectTimeoutId = setTimeout(() => reject(new Error('Child process did not exit gracefully within the timeout.')), timeoutInMs * 2); ^ Error: Child process did not exit gracefully within the timeout. at Timeout._onTimeout (file:///usr/local/google/home/pgschwendtner/projects/llm-codegen-quality/node_modules/.pnpm/[email protected]_@google-cloud+firest) at listOnTimeout (node:internal/timers:594:17) at process.processTimers (node:internal/timers:529:7) ```
1 parent 1945614 commit 3865a74

File tree

9 files changed

+48
-9
lines changed

9 files changed

+48
-9
lines changed

runner/orchestration/executors/local-executor.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {
1212
RootPromptDefinition,
1313
TestExecutionResult,
1414
} from '../../shared-interfaces.js';
15-
import {killChildProcessGracefully} from '../../utils/kill-gracefully.js';
15+
import {killChildProcessWithSigterm} from '../../utils/kill-gracefully.js';
1616
import {
1717
BuildResult,
1818
BuildWorkerMessage,
@@ -111,11 +111,19 @@ export class LocalExecutor implements Executor {
111111
child.send(buildParams);
112112

113113
child.on('message', async (result: BuildWorkerResponseMessage) => {
114-
await killChildProcessGracefully(child);
114+
try {
115+
await killChildProcessWithSigterm(child);
116+
} catch (e) {
117+
progress.debugLog(`Error while killing build worker: ${e}`);
118+
}
115119
resolve(result.payload);
116120
});
117121
child.on('error', async err => {
118-
await killChildProcessGracefully(child);
122+
try {
123+
await killChildProcessWithSigterm(child);
124+
} catch (e) {
125+
progress.debugLog(`Error while killing build worker: ${e}`);
126+
}
119127
reject(err);
120128
});
121129
}),

runner/orchestration/serve-testing-worker.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import path from 'node:path';
33
import {Environment} from '../configuration/environment.js';
44
import {ProgressLogger} from '../progress/progress-logger.js';
55
import {AssessmentConfig, RootPromptDefinition} from '../shared-interfaces.js';
6-
import {killChildProcessGracefully} from '../utils/kill-gracefully.js';
6+
import {killChildProcessWithSigterm} from '../utils/kill-gracefully.js';
77
import {
88
ServeTestingResult,
99
ServeTestingWorkerMessage,
@@ -59,7 +59,11 @@ export async function serveAndTestApp(
5959

6060
child.on('message', async (result: ServeTestingWorkerResponseMessage) => {
6161
if (result.type === 'result') {
62-
await killChildProcessGracefully(child);
62+
try {
63+
await killChildProcessWithSigterm(child);
64+
} catch (e) {
65+
progress.debugLog(`Error while killing serve testing worker: ${e}`);
66+
}
6367
resolve(result.payload);
6468
} else {
6569
progress.log(
@@ -71,7 +75,11 @@ export async function serveAndTestApp(
7175
}
7276
});
7377
child.on('error', async err => {
74-
await killChildProcessGracefully(child);
78+
try {
79+
await killChildProcessWithSigterm(child);
80+
} catch (e) {
81+
progress.debugLog(`Error while killing serve testing worker: ${e}`);
82+
}
7583
reject(err);
7684
});
7785
}),

runner/progress/dynamic-progress-logger.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,10 @@ export class DynamicProgressLogger implements ProgressLogger {
144144
}
145145
}
146146

147+
debugLog(message: string): void {
148+
// TODO: Implement debug logging in dynamic progress logging.
149+
}
150+
147151
private getColorFunction(type: ProgressType): (value: string) => string {
148152
switch (type) {
149153
case 'success':

runner/progress/noop-progress-logger.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,5 @@ export class NoopProgressLogger implements ProgressLogger {
66
finalize(): void {}
77
log(): void {}
88
evalFinished(): void {}
9+
debugLog(): void {}
910
}

runner/progress/progress-logger.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,4 +58,9 @@ export interface ProgressLogger {
5858
* @param details Additional information about the event.
5959
*/
6060
log(prompt: RootPromptDefinition, type: ProgressType, message: string, details?: string): void;
61+
62+
/**
63+
* Log information for debugging.
64+
*/
65+
debugLog(message: string): void;
6166
}

runner/progress/text-progress-logger.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,10 @@ export class TextProgressLogger implements ProgressLogger {
2424
// It's handy to know how many apps are done when one completes.
2525
console.log(`[${prompt.name}] 🏁 Done (${++this.done}/${this.total})`.trim());
2626
}
27+
28+
debugLog(message: string): void {
29+
if (process.env.DEBUG === '1') {
30+
console.log(`DEBUG: ${message}`);
31+
}
32+
}
2733
}

runner/run-cli.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,7 @@ class ErrorOnlyProgressLogger implements ProgressLogger {
180180
initialize(): void {}
181181
finalize(): void {}
182182
evalFinished(): void {}
183+
debugLog(): void {}
183184

184185
log(_: unknown, type: ProgressType, message: string, details?: string) {
185186
if (type === 'error') {

runner/utils/kill-gracefully.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ function treeKillPromise(pid: number, signal: string): Promise<void> {
1313
});
1414
}
1515

16-
export function killChildProcessGracefully(
16+
export function killChildProcessWithSigterm(
1717
child: ChildProcess,
1818
timeoutInMs = 1000 * 10, // 10s
1919
): Promise<void> {

runner/workers/serve-testing/serve-app.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import {ChildProcess, exec} from 'child_process';
2-
import {killChildProcessGracefully} from '../../utils/kill-gracefully.js';
2+
import {killChildProcessWithSigterm} from '../../utils/kill-gracefully.js';
33
import {cleanupBuildMessage} from '../builder/worker.js';
44
import {ProgressLogger} from '../../progress/progress-logger.js';
55
import {RootPromptDefinition} from '../../shared-interfaces.js';
@@ -96,7 +96,13 @@ export async function serveApp<T>(
9696
'Terminating browser process for app',
9797
`(PID: ${serveProcess.pid})`,
9898
);
99-
await killChildProcessGracefully(serveProcess);
99+
100+
try {
101+
await killChildProcessWithSigterm(serveProcess);
102+
} catch (e) {
103+
progress.debugLog(`Error while killing serve process: ${e}`);
104+
}
105+
100106
serveProcess = null;
101107
}
102108
}

0 commit comments

Comments
 (0)