fix: address code review feedback
- Change from absolute timeout to inactivity timeout (reset on each data chunk) to avoid truncating large/slow payloads - Fix race condition: add resolved=true before resolving in catch block - Fix unreliable readable check: just access the property, don't check value - Add cleanup() call in catch block for consistency Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
+28
-14
@@ -1,10 +1,12 @@
|
|||||||
// Stdin reading utility extracted from hook patterns
|
// Stdin reading utility extracted from hook patterns
|
||||||
// See src/hooks/save-hook.ts for the original pattern
|
// See src/hooks/save-hook.ts for the original pattern
|
||||||
|
|
||||||
// Timeout for stdin reading - if Claude Code doesn't close stdin within this time,
|
// Inactivity timeout for stdin reading - if no data arrives within this time,
|
||||||
// we parse whatever data we have. This fixes issue #727 where hooks hang at "1/2 done"
|
// we parse whatever data we have. This fixes issue #727 where hooks hang at "1/2 done"
|
||||||
// because stdin.on('end') never fires.
|
// because stdin.on('end') never fires.
|
||||||
const STDIN_TIMEOUT_MS = 5000;
|
// Using inactivity timeout (reset on each data chunk) instead of absolute timeout
|
||||||
|
// to avoid truncating large/slow payloads.
|
||||||
|
const STDIN_INACTIVITY_TIMEOUT_MS = 5000;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Check if stdin is available and readable.
|
* Check if stdin is available and readable.
|
||||||
@@ -24,11 +26,13 @@ function isStdinAvailable(): boolean {
|
|||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check if we can access basic stdin properties without crashing
|
// Accessing stdin.readable triggers Bun's lazy initialization.
|
||||||
// This triggers Bun's lazy initialization
|
// If we get here without throwing, stdin is available.
|
||||||
const readable = stdin.readable;
|
// Note: We don't check the value since Node/Bun don't reliably set it to false.
|
||||||
return readable !== false;
|
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
|
||||||
} catch (err) {
|
stdin.readable;
|
||||||
|
return true;
|
||||||
|
} catch {
|
||||||
// Bun crashed trying to access stdin (EINVAL from fstat)
|
// Bun crashed trying to access stdin (EINVAL from fstat)
|
||||||
// This is expected when Claude Code doesn't provide valid stdin
|
// This is expected when Claude Code doesn't provide valid stdin
|
||||||
return false;
|
return false;
|
||||||
@@ -45,6 +49,7 @@ export async function readJsonFromStdin(): Promise<unknown> {
|
|||||||
return new Promise((resolve, reject) => {
|
return new Promise((resolve, reject) => {
|
||||||
let input = '';
|
let input = '';
|
||||||
let resolved = false;
|
let resolved = false;
|
||||||
|
let timeoutId: ReturnType<typeof setTimeout>;
|
||||||
|
|
||||||
const cleanup = () => {
|
const cleanup = () => {
|
||||||
try {
|
try {
|
||||||
@@ -59,6 +64,7 @@ export async function readJsonFromStdin(): Promise<unknown> {
|
|||||||
const resolveWithData = () => {
|
const resolveWithData = () => {
|
||||||
if (resolved) return;
|
if (resolved) return;
|
||||||
resolved = true;
|
resolved = true;
|
||||||
|
clearTimeout(timeoutId);
|
||||||
cleanup();
|
cleanup();
|
||||||
try {
|
try {
|
||||||
resolve(input.trim() ? JSON.parse(input) : undefined);
|
resolve(input.trim() ? JSON.parse(input) : undefined);
|
||||||
@@ -67,25 +73,31 @@ export async function readJsonFromStdin(): Promise<unknown> {
|
|||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
// Timeout handler - resolve with whatever data we have
|
// Reset the inactivity timeout - called on each data chunk
|
||||||
// This fixes issue #727 where stdin.on('end') never fires
|
const resetTimeout = () => {
|
||||||
const timeoutId = setTimeout(() => {
|
clearTimeout(timeoutId);
|
||||||
|
timeoutId = setTimeout(() => {
|
||||||
if (!resolved) {
|
if (!resolved) {
|
||||||
resolveWithData();
|
resolveWithData();
|
||||||
}
|
}
|
||||||
}, STDIN_TIMEOUT_MS);
|
}, STDIN_INACTIVITY_TIMEOUT_MS);
|
||||||
|
};
|
||||||
|
|
||||||
|
// Start initial timeout
|
||||||
|
resetTimeout();
|
||||||
|
|
||||||
try {
|
try {
|
||||||
process.stdin.on('data', (chunk) => {
|
process.stdin.on('data', (chunk) => {
|
||||||
input += chunk;
|
input += chunk;
|
||||||
|
// Reset timeout on each data chunk to avoid truncating large/slow payloads
|
||||||
|
resetTimeout();
|
||||||
});
|
});
|
||||||
|
|
||||||
process.stdin.on('end', () => {
|
process.stdin.on('end', () => {
|
||||||
clearTimeout(timeoutId);
|
|
||||||
resolveWithData();
|
resolveWithData();
|
||||||
});
|
});
|
||||||
|
|
||||||
process.stdin.on('error', (err) => {
|
process.stdin.on('error', () => {
|
||||||
if (resolved) return;
|
if (resolved) return;
|
||||||
resolved = true;
|
resolved = true;
|
||||||
clearTimeout(timeoutId);
|
clearTimeout(timeoutId);
|
||||||
@@ -94,9 +106,11 @@ export async function readJsonFromStdin(): Promise<unknown> {
|
|||||||
// This is more graceful for hook execution
|
// This is more graceful for hook execution
|
||||||
resolve(undefined);
|
resolve(undefined);
|
||||||
});
|
});
|
||||||
} catch (err) {
|
} catch {
|
||||||
// If attaching listeners fails (Bun stdin issue), resolve with undefined
|
// If attaching listeners fails (Bun stdin issue), resolve with undefined
|
||||||
|
resolved = true;
|
||||||
clearTimeout(timeoutId);
|
clearTimeout(timeoutId);
|
||||||
|
cleanup();
|
||||||
resolve(undefined);
|
resolve(undefined);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user