refactor(server): detach PR lookup stacks instead of a custom cache - #13311
Conversation
The PR lookup leak came from the caller's lazy stack frame, which the Effect Cache lookup fiber inherits. Materialize that frame before Cache.get and go back to Effect's Cache, dropping the hand-rolled replacement and its expiry/eviction/cancellation tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| @@ -0,0 +1,31 @@ | |||
| import { Cache, Effect } from "effect"; | |||
| import { detachStackFrame } from "../detachStackFrame.ts"; | |||
There was a problem hiding this comment.
🟡 Medium testing/StackRetention.fixture.mjs:2
On Node 22.16/22.17, the spawned process exits while loading ../detachStackFrame.ts because it is invoked only with --expose-gc; those Node versions still require --experimental-strip-types for .ts imports, so the fixture never reaches its assertions. Add that flag to the child-process invocation or use a JavaScript fixture/built output.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/git/testing/StackRetention.fixture.mjs around line 2:
On Node 22.16/22.17, the spawned process exits while loading `../detachStackFrame.ts` because it is invoked only with `--expose-gc`; those Node versions still require `--experimental-strip-types` for `.ts` imports, so the fixture never reaches its assertions. Add that flag to the child-process invocation or use a JavaScript fixture/built output.
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This is a focused PR-lookup cache refactor that preserves lookup, TTL, eviction, and invalidation behavior while preventing cached fibers from retaining request snapshots. The supplied Medium finding identifies a supported-Node test-fixture compatibility problem, which remains a separate correctness issue. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: unavailable · PR result: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
cde4737
into
t3code/codex-turn-mapping
…13311) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…13311) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…13311) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…13311) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…13311) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…13311) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Follow-up to #13304.
#13304 stopped cached PR lookups from retaining caller thread snapshots by replacing Effect's
Cachewith a hand-rolledLookupResultCache. The leak comes only from the caller's lazy stack frame, which theCachelookup fiber inherits. Materializing that frame beforeCache.getis enough.This adds a 20-line
detachStackFramehelper, routes the three PR-lookupCache.getcalls through it, and returns toCache.makeWith. The custom cache and its expiry, eviction, and cancellation tests are removed, since Effect'sCacheowns that behavior again.Verification:
Cache(originalmode) retains it on success; the failed-lookup control retained it in one of two runs.detachStackFrame.memory.test.tsandGitManager.test.tspass (116 tests). Scoped lint, formatting, and the server typecheck pass.Claude Opus 5.5 via Claude Code in T3 Code.
🤖 Generated with Claude Code