fix: increase systemic timeouts for summarization and consolidation#213
fix: increase systemic timeouts for summarization and consolidation#213xuli500177 wants to merge 3 commits intorohitg00:mainfrom
Conversation
|
@xuli500177 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe pull request increases timeout values across the application's hooks and worker configuration. Fetch call Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/index.ts (1)
107-163:⚠️ Potential issue | 🟡 MinorUpdate the timeout rationale comment to match the new 180s config.
The comment still says 30s, but Line 163 is now 180000ms. Keeping these aligned avoids troubleshooting confusion.
📝 Suggested comment fix
-// projects) `state::set` can occasionally exceed the SDK's 30s timeout. +// projects) `state::set` can occasionally exceed the SDK's timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 107 - 163, The top-level safety-net comment above process.on("unhandledRejection") is stale (mentions the SDK's 30s timeout) while invocationTimeoutMs is now set to 180000 (180s) in the registerWorker call; update that comment to reference 180s (or "180s / 180000ms") so the rationale matches the new invocationTimeoutMs value in main and avoids confusion when reading the registerWorker invocationTimeoutMs setting.src/hooks/session-end.ts (1)
40-71:⚠️ Potential issue | 🟡 MinorRemove inline “increased from …” comments on timeout lines.
These comments are “WHAT” comments and will drift. Prefer named timeout constants instead.
♻️ Suggested cleanup
+const SESSION_END_TIMEOUT_MS = 30_000; +const CRYSTALS_AUTO_TIMEOUT_MS = 60_000; +const CONSOLIDATE_PIPELINE_TIMEOUT_MS = 120_000; +const CLAUDE_BRIDGE_SYNC_TIMEOUT_MS = 30_000; try { await fetch(`${REST_URL}/agentmemory/session/end`, { @@ - signal: AbortSignal.timeout(30000), // Increased from 5s + signal: AbortSignal.timeout(SESSION_END_TIMEOUT_MS), }); @@ - signal: AbortSignal.timeout(60000), // Increased from 15s + signal: AbortSignal.timeout(CRYSTALS_AUTO_TIMEOUT_MS), }); @@ - signal: AbortSignal.timeout(120000), // Increased from 30s + signal: AbortSignal.timeout(CONSOLIDATE_PIPELINE_TIMEOUT_MS), }); @@ - signal: AbortSignal.timeout(30000), // Increased from 5s + signal: AbortSignal.timeout(CLAUDE_BRIDGE_SYNC_TIMEOUT_MS), });As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/session-end.ts` around lines 40 - 71, Replace the inline “Increased from …” comments and magic numeric timeouts with named constants and use those constants in the AbortSignal.timeout calls; for example, add clearly named constants (e.g., TIMEOUT_SHORT_MS = 5000, TIMEOUT_MEDIUM_MS = 30000, TIMEOUT_CONSOLIDATE_MS = 120000) near the top of the module (or import from a shared timeouts/constants module) and then update the three AbortSignal.timeout(...) calls in the session-end.ts fetch blocks (the first generic fetch, the ${REST_URL}/agentmemory/crystals/auto call, the ${REST_URL}/agentmemory/consolidate-pipeline call, and the ${REST_URL}/agentmemory/claude-bridge/sync call) to use those constants and remove the inline “increased from …” comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/stop.ts`:
- Line 47: Remove the inline change-history comment and replace the magic number
passed to AbortSignal.timeout with a named constant (e.g., STOP_TIMEOUT_MS or
STOP_SIGNAL_TIMEOUT) declared near the top of the module; update the call site
from AbortSignal.timeout(120000) to AbortSignal.timeout(STOP_TIMEOUT_MS) so the
timeout value is self-documenting and the comment is no longer needed.
---
Outside diff comments:
In `@src/hooks/session-end.ts`:
- Around line 40-71: Replace the inline “Increased from …” comments and magic
numeric timeouts with named constants and use those constants in the
AbortSignal.timeout calls; for example, add clearly named constants (e.g.,
TIMEOUT_SHORT_MS = 5000, TIMEOUT_MEDIUM_MS = 30000, TIMEOUT_CONSOLIDATE_MS =
120000) near the top of the module (or import from a shared timeouts/constants
module) and then update the three AbortSignal.timeout(...) calls in the
session-end.ts fetch blocks (the first generic fetch, the
${REST_URL}/agentmemory/crystals/auto call, the
${REST_URL}/agentmemory/consolidate-pipeline call, and the
${REST_URL}/agentmemory/claude-bridge/sync call) to use those constants and
remove the inline “increased from …” comments.
In `@src/index.ts`:
- Around line 107-163: The top-level safety-net comment above
process.on("unhandledRejection") is stale (mentions the SDK's 30s timeout) while
invocationTimeoutMs is now set to 180000 (180s) in the registerWorker call;
update that comment to reference 180s (or "180s / 180000ms") so the rationale
matches the new invocationTimeoutMs value in main and avoids confusion when
reading the registerWorker invocationTimeoutMs setting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b615624-faee-4b22-8d5a-51d21c0b491f
📒 Files selected for processing (3)
src/hooks/session-end.tssrc/hooks/stop.tssrc/index.ts
| headers: authHeaders(), | ||
| body: JSON.stringify({ sessionId }), | ||
| signal: AbortSignal.timeout(30000), | ||
| signal: AbortSignal.timeout(120000), // Increased from 30s to 120s |
There was a problem hiding this comment.
Drop the inline change-history comment on this timeout line.
Use a named constant if you want the value to be self-documenting.
As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/stop.ts` at line 47, Remove the inline change-history comment and
replace the magic number passed to AbortSignal.timeout with a named constant
(e.g., STOP_TIMEOUT_MS or STOP_SIGNAL_TIMEOUT) declared near the top of the
module; update the call site from AbortSignal.timeout(120000) to
AbortSignal.timeout(STOP_TIMEOUT_MS) so the timeout value is self-documenting
and the comment is no longer needed.
1 participant
Summary
This PR addresses systemic timeout issues encountered during long coding sessions (e.g., sessions with 300+ observations).
Changes:
registerWorkerinsrc/index.tsto use a 180sinvocationTimeoutMsinstead of the 30s default. This provides sufficient head-room for heavy LLM summarization and consolidation tasks.src/hooks/stop.ts: Increasedfetchtimeout for/agentmemory/summarizeto 120s.src/hooks/session-end.ts: Increased timeouts for session end (30s), crystallization (60s), and consolidation pipeline (120s).Context:
In high-volume sessions, the
mem::summarizeandmem::consolidate-pipelinefunctions often require more than 30s of LLM execution time. Without these adjustments, the hooks fail and the memory system misses critical summary/consolidation steps.Summary by CodeRabbit
Release Notes