fix: add timeout to SDK metadata extraction#99
Open
Spirotot wants to merge 3 commits intohappier-dev:devfrom
Open
fix: add timeout to SDK metadata extraction#99Spirotot wants to merge 3 commits intohappier-dev:devfrom
Spirotot wants to merge 3 commits intohappier-dev:devfrom
Conversation
extractSDKMetadata() has no timeout — if the spawned SDK process hangs (e.g. due to inaccessible filesystems or missing credentials), the extraction waits indefinitely, leaking a process for the session lifetime. Add a 10-second timeout via setTimeout + AbortController. If the SDK query doesn't produce an init message within the timeout, the query is aborted and empty metadata is returned gracefully. Normal extraction completes in ~1-2 seconds, so 10 seconds provides ample headroom.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a configurable timeout mechanism for SDK metadata extraction (DEFAULT/MIN/MAX and env parsing), aborting the SDK query on timeout or after capturing init metadata; ensures timeout cleanup on all paths and enhances logging for timeout/abort scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Extractor
participant ClaudeProcess as "Claude\n(child)"
participant Timer
participant OS
Extractor->>ClaudeProcess: spawn process & start read loop
Note right of ClaudeProcess: may emit INIT metadata to stdout
Timer->>Extractor: start timeout
ClaudeProcess-->>Extractor: INIT metadata (optional)
alt INIT received
Extractor->>Timer: clear timeout
Extractor->>ClaudeProcess: send abort (signal/abort controller)
ClaudeProcess->>OS: exit
ClaudeProcess-->>Extractor: exit confirmation
Extractor-->>Extractor: return extracted metadata
else timeout occurs
Timer->>Extractor: trigger timeout
Extractor->>ClaudeProcess: send abort (signal/abort controller)
ClaudeProcess->>OS: exit
ClaudeProcess-->>Extractor: exit confirmation
Extractor-->>Extractor: return empty metadata
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/cli/src/backends/claude/sdk/metadataExtractor.ts (1)
59-71: Consolidate timeout cleanup in afinallyblock.
clearTimeout(timeoutId)is repeated across multiple branches. Afinallyblock keeps cleanup centralized and reduces maintenance risk.Suggested refactor
- try { + try { logger.debug('[metadataExtractor] Starting SDK metadata extraction') @@ - clearTimeout(timeoutId) abortController.abort() @@ - clearTimeout(timeoutId) logger.debug('[metadataExtractor] No init message received from SDK') return {} - } catch (error) { - clearTimeout(timeoutId) // Check if it's an abort error (expected — either from timeout or after capture) if (error instanceof Error && error.name === 'AbortError') { logger.debug('[metadataExtractor] SDK query aborted (timeout or after capturing metadata)') return {} } logger.debug('[metadataExtractor] Error extracting SDK metadata:', error) return {} + } finally { + clearTimeout(timeoutId) }As per coding guidelines: "Apply DRY, SOLID, and KISS principles - avoid speculative or YAGNI code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/backends/claude/sdk/metadataExtractor.ts` around lines 59 - 71, The repeated clearTimeout(timeoutId) (and abortController.abort()) should be centralized in a finally block inside the metadata extraction routine so cleanup always runs regardless of return or thrown error; refactor the function containing the try/catch (e.g., the metadata extraction function in metadataExtractor.ts that returns metadata or {} and currently calls clearTimeout in multiple places) to declare timeoutId and abortController in the outer scope, move clearTimeout(timeoutId) and abortController.abort() into a finally block, and remove the duplicate clearTimeout calls in the normal return paths while keeping existing logging (logger.debug('[metadataExtractor] No init message received from SDK')) and preserving the same return values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/backends/claude/sdk/metadataExtractor.ts`:
- Around line 15-16: The constant METADATA_EXTRACTION_TIMEOUT_MS is hardcoded;
change it to be configurable by reading from environment/config with a safe
default of 10000ms (e.g., use process.env.METADATA_EXTRACTION_TIMEOUT_MS or your
project's config loader to parse an integer and fall back to 10_000). Replace
the current const with code that reads and validates the configured value
(coerce to number, handle NaN) and keep the same exported/used symbol name
METADATA_EXTRACTION_TIMEOUT_MS so existing callers in metadataExtractor.ts
continue to work.
---
Nitpick comments:
In `@apps/cli/src/backends/claude/sdk/metadataExtractor.ts`:
- Around line 59-71: The repeated clearTimeout(timeoutId) (and
abortController.abort()) should be centralized in a finally block inside the
metadata extraction routine so cleanup always runs regardless of return or
thrown error; refactor the function containing the try/catch (e.g., the metadata
extraction function in metadataExtractor.ts that returns metadata or {} and
currently calls clearTimeout in multiple places) to declare timeoutId and
abortController in the outer scope, move clearTimeout(timeoutId) and
abortController.abort() into a finally block, and remove the duplicate
clearTimeout calls in the normal return paths while keeping existing logging
(logger.debug('[metadataExtractor] No init message received from SDK')) and
preserving the same return values.
Comment on lines
+15
to
+16
| /** Maximum time to wait for SDK metadata extraction before giving up. */ | ||
| const METADATA_EXTRACTION_TIMEOUT_MS = 10_000 |
There was a problem hiding this comment.
Make metadata timeout configurable instead of hardcoded.
METADATA_EXTRACTION_TIMEOUT_MS is hardcoded. Please source it from environment/config (with a safe default) so operators can tune behavior per environment.
Suggested change
-/** Maximum time to wait for SDK metadata extraction before giving up. */
-const METADATA_EXTRACTION_TIMEOUT_MS = 10_000
+/** Maximum time to wait for SDK metadata extraction before giving up. */
+const DEFAULT_METADATA_EXTRACTION_TIMEOUT_MS = 10_000
+const METADATA_EXTRACTION_TIMEOUT_MS = (() => {
+ const raw = process.env.HAPPIER_METADATA_EXTRACTION_TIMEOUT_MS
+ const parsed = raw ? Number.parseInt(raw, 10) : NaN
+ return Number.isFinite(parsed) && parsed > 0 ? parsed : DEFAULT_METADATA_EXTRACTION_TIMEOUT_MS
+})()As per coding guidelines: "Avoid hardcoded behavior - load configuration from YAML/environment variables instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/backends/claude/sdk/metadataExtractor.ts` around lines 15 - 16,
The constant METADATA_EXTRACTION_TIMEOUT_MS is hardcoded; change it to be
configurable by reading from environment/config with a safe default of 10000ms
(e.g., use process.env.METADATA_EXTRACTION_TIMEOUT_MS or your project's config
loader to parse an integer and fall back to 10_000). Replace the current const
with code that reads and validates the configured value (coerce to number,
handle NaN) and keep the same exported/used symbol name
METADATA_EXTRACTION_TIMEOUT_MS so existing callers in metadataExtractor.ts
continue to work.
Contributor
Greptile SummaryAdded a 10-second timeout to SDK metadata extraction to prevent indefinite process hangs when the spawned SDK subprocess blocks on inaccessible filesystems or missing credentials. Key changes:
The implementation properly handles all edge cases and follows the codebase's established patterns for error handling and abort controllers. Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| apps/cli/src/backends/claude/sdk/metadataExtractor.ts | Added 10s timeout to prevent indefinite hangs during SDK metadata extraction - proper cleanup in all code paths |
Last reviewed commit: 746c1b7
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/src/backends/claude/sdk/metadataExtractor.test.ts (1)
58-67: AddafterAllto clean up the last test's temporary directory.The
beforeEachhook cleans uptmpRootfrom a previous test, but after the final test runs, its temporary directory is never removed. This leaves test artifacts on disk.🧹 Proposed fix to add final cleanup
import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterAll } from 'vitest';beforeEach(() => { vi.resetModules(); if (tmpRoot) { rmSync(tmpRoot, { recursive: true, force: true }); tmpRoot = ''; } }); + + afterAll(() => { + if (tmpRoot) { + rmSync(tmpRoot, { recursive: true, force: true }); + } + });Based on learnings: "Use temporary directories for filesystem tests with automatic cleanup".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/backends/claude/sdk/metadataExtractor.test.ts` around lines 58 - 67, Add an afterAll cleanup to the describe.sequential('claude sdk metadata extractor') block to remove the final test's temporary directory: check the existing tmpRoot variable and, if set, call rmSync(tmpRoot, { recursive: true, force: true }) (the same cleanup logic as in beforeEach) inside an afterAll hook so the last test's temp directory is removed when the suite finishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/cli/src/backends/claude/sdk/metadataExtractor.test.ts`:
- Around line 58-67: Add an afterAll cleanup to the describe.sequential('claude
sdk metadata extractor') block to remove the final test's temporary directory:
check the existing tmpRoot variable and, if set, call rmSync(tmpRoot, {
recursive: true, force: true }) (the same cleanup logic as in beforeEach) inside
an afterAll hook so the last test's temp directory is removed when the suite
finishes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cli/src/backends/claude/sdk/metadataExtractor.test.tsapps/cli/src/backends/claude/sdk/metadataExtractor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/src/backends/claude/sdk/metadataExtractor.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
extractSDKMetadata()has no timeout — if the spawned SDK process hangs (e.g. due to inaccessible filesystems or missing credentials), the extraction waits indefinitely, leaking a process for the session lifetimesetTimeout+AbortControllerContext
Discovered while debugging daemon-spawned sessions on macOS where the metadata extraction subprocess would hang indefinitely due to filesystem access issues (symlinks to cloud storage). While
extractSDKMetadataAsyncis fire-and-forget and doesn't block session startup, the leaked process persists for the session lifetime.Test plan
extractSDKMetadataAsync: vi.fn()) still passSummary by CodeRabbit
Bug Fixes
Tests