Repository navigation
fix(agent-sdk): scope recursion guard to AsyncLocalStorage, memoize SDK import - #782
Conversation
…DK import Reported in #781. mem::summarize fails with 'too_many_chunks_skipped: 4/4 chunks failed to parse after retry' on the agent-sdk provider whenever a session is large enough to split into >=2 chunks. Root cause: the recursion guard from #181 mutated process.env globally: if (process.env.AGENTMEMORY_SDK_CHILD === '1') return '' process.env.AGENTMEMORY_SDK_CHILD = '1' // ...await SDK... // restore #472 then introduced chunked summarize that runs chunks concurrently via Promise.all (SUMMARIZE_CHUNK_CONCURRENCY=6 by default). The first chunk in a batch flips the env synchronously before its first await; siblings in the same batch then enter query(), see the flag, and return ''. An empty string has no <title>, parseSummaryXml returns null, the chunk is counted as skipped, >50% skip ratio throws too_many_chunks_skipped. The guard (cross-process) and the chunk concurrency (in-process) were never reconciled. Each needs the right primitive: - In-process recursion guard: AsyncLocalStorage. Scoped to the async call tree of the SDK query, so concurrent siblings have separate ALS frames and no longer see each other's marker. The check now reads sdkChildContext.getStore() instead of process.env. - Cross-process recursion guard for hook scripts: still process.env.AGENTMEMORY_SDK_CHILD = '1' around the SDK call. Subprocesses spawned by @anthropic-ai/claude-agent-sdk inherit process.env at spawn time; hook scripts (separate processes) still read the env to short-circuit their REST callbacks. The race on the env between concurrent in-process siblings is benign because every sibling wants the same value '1' during its own SDK call. Also memoize the dynamic import of @anthropic-ai/claude-agent-sdk on the provider instance. Production benefit: concurrent callers share one module resolution. Test benefit: vi.mock factories apply uniformly across concurrent imports instead of racing between mock and real module references. Tests cover: - 4 concurrent summarize calls each return the real SDK result (no empty siblings) — the direct #781 regression - Mixed concurrent summarize + compress on the same provider - AGENTMEMORY_SDK_CHILD is set to '1' during the SDK call (child process inheritance) and restored to the prior value on exit - Genuine re-entry inside the same async tree still degrades to '' so the #149 / #181 stop-hook -> summarize recursion stays broken Closes #781.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAgentSDKProvider now uses AsyncLocalStorage to short-circuit in-process re-entry and a reference-counted env marker to preserve cross-process inheritance. The Claude SDK import is memoized via a cached promise. Tests verify concurrent summarize/compress behavior, env preservation/restoration, and genuine re-entry returns "" for inner calls. ChangesRecursion Guard and SDK Memoization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/providers/agent-sdk.ts`:
- Around line 74-100: The current per-call snapshot of
process.env.AGENTMEMORY_SDK_CHILD races across overlapping calls; replace it
with shared active-call accounting: introduce a module-level counter (e.g.,
sdkActiveCount) and a single stored original value (e.g., sdkOriginalEnv) and on
entry (in the method around the existing prev/save block) increment
sdkActiveCount and, if it becomes 1, save process.env.AGENTMEMORY_SDK_CHILD into
sdkOriginalEnv and set process.env.AGENTMEMORY_SDK_CHILD = '1'; in the finally
block decrement sdkActiveCount and only when it reaches 0 restore
process.env.AGENTMEMORY_SDK_CHILD from sdkOriginalEnv (delete if undefined) so
the original value is preserved until the last active SDK call exits; keep the
existing loadSdk/query/result logic intact and ensure the counter and original
value are module-scoped so concurrent invocations of the function share them.
In `@test/agent-sdk-provider.test.ts`:
- Around line 8-30: The mock factory for vi.mock is hoisted and currently
captures top-level bindings queryCalls and mockResult; move those into a hoisted
state by creating a vi.hoisted(() => ({ queryCalls, mockResult })) object and
update the mock factory to reference state.queryCalls and state.mockResult (and
update beforeEach/assertions to use state.queryCalls/state.mockResult) so the
mock does not close over non-hoisted module-scope variables; keep the mock
implementation in the same function (query) but replace direct uses of
queryCalls and mockResult with the hoisted state fields.
🪄 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: bba5211a-4454-4db8-81a0-f6fb07c35660
📒 Files selected for processing (2)
src/providers/agent-sdk.tstest/agent-sdk-provider.test.ts
Address CodeRabbit findings on PR #782. agent-sdk.ts: the per-call prev snapshot races across overlapping calls. Trace: - A enters: prev=undef, env='1' - B enters: prev='1' (already set by A), env='1' - A finishes first, restores prev=undef -> env=undef WHILE B is still mid-flight; any child process B spawns now won't inherit the marker - B finishes, restores prev='1' -> env='1' leaks past the last caller Replace with module-level reference counting: let sdkActiveCount = 0 let sdkOriginalEnv: string | undefined if (sdkActiveCount === 0) { sdkOriginalEnv = process.env.AGENTMEMORY_SDK_CHILD process.env.AGENTMEMORY_SDK_CHILD = '1' } sdkActiveCount++ try { ...SDK call... } finally { sdkActiveCount-- if (sdkActiveCount === 0) { // restore sdkOriginalEnv (delete if undefined) } } Only the first entrant snapshots the original; only the last exit restores it. Env stays '1' for the full overlap. test/agent-sdk-provider.test.ts: vi.mock factories are hoisted above const/let bindings, so the closure can't safely reference non-hoisted module-scope state. Move queryCalls and mockResult into a vi.hoisted state object so the mock factory and tests share an explicitly hoisted reference instead of a closure capture. Also fixed a test-mock bug while adding the overlap regression: the mock yielded the unwrapped Promise from a function-shaped mockResult. The gen now awaits the mockResult inside the generator so async mock results are unwrapped before yielding. New regression test: 3 concurrent calls each observe AGENTMEMORY_SDK_CHILD='1' at enter and exit phases for the full overlap, and env is restored to undefined only after the last call finishes. 122 test files / 1332 tests pass.
Bump to 0.9.25 across 9 files + CHANGELOG. Closes #778 #775 #783 (PR #791), #758 #726 (PR #773), #759 (PR #772), #752 (PR #774), #729 (PR #780), #781 (PR #782), #753 (PR #789), #771 (PR #786), #762 (PR #764). Files bumped: - package.json - packages/mcp/package.json - plugin/.claude-plugin/plugin.json - plugin/.codex-plugin/plugin.json - plugin/plugin.json - src/version.ts - src/types.ts (ExportData.version union) - src/functions/export-import.ts (supportedVersions Set) - test/export-import.test.ts (assertion) 125 test files / 1379 tests pass. npm audit (root + website): 0 vulns.
…DK import (rohitg00#782) * fix(agent-sdk): scope recursion guard to AsyncLocalStorage, memoize SDK import Reported in rohitg00#781. mem::summarize fails with 'too_many_chunks_skipped: 4/4 chunks failed to parse after retry' on the agent-sdk provider whenever a session is large enough to split into >=2 chunks. Root cause: the recursion guard from rohitg00#181 mutated process.env globally: if (process.env.AGENTMEMORY_SDK_CHILD === '1') return '' process.env.AGENTMEMORY_SDK_CHILD = '1' // ...await SDK... // restore rohitg00#472 then introduced chunked summarize that runs chunks concurrently via Promise.all (SUMMARIZE_CHUNK_CONCURRENCY=6 by default). The first chunk in a batch flips the env synchronously before its first await; siblings in the same batch then enter query(), see the flag, and return ''. An empty string has no <title>, parseSummaryXml returns null, the chunk is counted as skipped, >50% skip ratio throws too_many_chunks_skipped. The guard (cross-process) and the chunk concurrency (in-process) were never reconciled. Each needs the right primitive: - In-process recursion guard: AsyncLocalStorage. Scoped to the async call tree of the SDK query, so concurrent siblings have separate ALS frames and no longer see each other's marker. The check now reads sdkChildContext.getStore() instead of process.env. - Cross-process recursion guard for hook scripts: still process.env.AGENTMEMORY_SDK_CHILD = '1' around the SDK call. Subprocesses spawned by @anthropic-ai/claude-agent-sdk inherit process.env at spawn time; hook scripts (separate processes) still read the env to short-circuit their REST callbacks. The race on the env between concurrent in-process siblings is benign because every sibling wants the same value '1' during its own SDK call. Also memoize the dynamic import of @anthropic-ai/claude-agent-sdk on the provider instance. Production benefit: concurrent callers share one module resolution. Test benefit: vi.mock factories apply uniformly across concurrent imports instead of racing between mock and real module references. Tests cover: - 4 concurrent summarize calls each return the real SDK result (no empty siblings) — the direct rohitg00#781 regression - Mixed concurrent summarize + compress on the same provider - AGENTMEMORY_SDK_CHILD is set to '1' during the SDK call (child process inheritance) and restored to the prior value on exit - Genuine re-entry inside the same async tree still degrades to '' so the rohitg00#149 / rohitg00#181 stop-hook -> summarize recursion stays broken Closes rohitg00#781. * fix(agent-sdk): refcount env marker + vi.hoisted mock state Address CodeRabbit findings on PR rohitg00#782. agent-sdk.ts: the per-call prev snapshot races across overlapping calls. Trace: - A enters: prev=undef, env='1' - B enters: prev='1' (already set by A), env='1' - A finishes first, restores prev=undef -> env=undef WHILE B is still mid-flight; any child process B spawns now won't inherit the marker - B finishes, restores prev='1' -> env='1' leaks past the last caller Replace with module-level reference counting: let sdkActiveCount = 0 let sdkOriginalEnv: string | undefined if (sdkActiveCount === 0) { sdkOriginalEnv = process.env.AGENTMEMORY_SDK_CHILD process.env.AGENTMEMORY_SDK_CHILD = '1' } sdkActiveCount++ try { ...SDK call... } finally { sdkActiveCount-- if (sdkActiveCount === 0) { // restore sdkOriginalEnv (delete if undefined) } } Only the first entrant snapshots the original; only the last exit restores it. Env stays '1' for the full overlap. test/agent-sdk-provider.test.ts: vi.mock factories are hoisted above const/let bindings, so the closure can't safely reference non-hoisted module-scope state. Move queryCalls and mockResult into a vi.hoisted state object so the mock factory and tests share an explicitly hoisted reference instead of a closure capture. Also fixed a test-mock bug while adding the overlap regression: the mock yielded the unwrapped Promise from a function-shaped mockResult. The gen now awaits the mockResult inside the generator so async mock results are unwrapped before yielding. New regression test: 3 concurrent calls each observe AGENTMEMORY_SDK_CHILD='1' at enter and exit phases for the full overlap, and env is restored to undefined only after the last call finishes. 122 test files / 1332 tests pass.
Bump to 0.9.25 across 9 files + CHANGELOG. Closes rohitg00#778 rohitg00#775 rohitg00#783 (PR rohitg00#791), rohitg00#758 rohitg00#726 (PR rohitg00#773), rohitg00#759 (PR rohitg00#772), rohitg00#752 (PR rohitg00#774), rohitg00#729 (PR rohitg00#780), rohitg00#781 (PR rohitg00#782), rohitg00#753 (PR rohitg00#789), rohitg00#771 (PR rohitg00#786), rohitg00#762 (PR rohitg00#764). Files bumped: - package.json - packages/mcp/package.json - plugin/.claude-plugin/plugin.json - plugin/.codex-plugin/plugin.json - plugin/plugin.json - src/version.ts - src/types.ts (ExportData.version union) - src/functions/export-import.ts (supportedVersions Set) - test/export-import.test.ts (assertion) 125 test files / 1379 tests pass. npm audit (root + website): 0 vulns.
Problem
Reported in #781.
mem::summarizefails withtoo_many_chunks_skipped: 4/4 chunks failed to parse after retrywhenever the agent-sdk provider is active (AGENTMEMORY_ALLOW_AGENT_SDK=true) and a session is large enough to split into ≥2 chunks.The two PRs interact badly:
The first chunk in a batch flips the env synchronously before its first
await. Sibling chunks in the same batch enterquery(), see the flag, and return"". Empty string has no<title>,parseSummaryXmlreturnsnull, the chunk is counted as skipped, > 50% skip ratio throws.The guard (cross-process) and the chunk concurrency (in-process) were never reconciled.
Fix — split the guard by scope
Each concern uses the right primitive:
AsyncLocalStorageprocess.env.AGENTMEMORY_SDK_CHILD = '1'around the SDK callprocess.envat spawn time. ALS does not cross process boundaries.The check now reads
sdkChildContext.getStore()instead ofprocess.env. The env var stays set+restored around the SDK call so spawned hook subprocesses still see the marker and short-circuit their REST callbacks (the original #149/#181 fix still holds).The race on the env between concurrent in-process siblings is benign because every sibling wants
"1"during its own SDK call anyway.Bonus: memoize the dynamic import
Refactored
await import('@anthropic-ai/claude-agent-sdk')into a per-instance memoized promise. Concurrent callers share one module resolution. Two benefits:vi.mockfactories apply uniformly across concurrent imports (without this, the test mock raced with real module references underPromise.all, see commit body)Tests
5 cases in
test/agent-sdk-provider.test.ts:AGENTMEMORY_SDK_CHILDis set to"1"during the SDK call (verified by reading the env from inside the mocked query) and restored to its prior value on exit""so the 429 rate limit errors when AUTO_COMPRESS uses agent-sdk provider in a heavy Claude Code session #149 / Stop-hook → /summarize → agent-sdk infinite recursion when no provider key is set (extends #149) #181 recursion guard stays armedFull suite: 122 files / 1331 tests pass.
Recommended user action
Users on
0.9.24can remove theSUMMARIZE_CHUNK_CONCURRENCY=1workaround after this lands. No env or config changes required.Closes #781.
Summary by CodeRabbit
Bug Fixes
Performance Improvements
Tests