Conversation
onFinish declared `state` in the factory closure, so every interceptor it created kept the last result or error reachable for as long as the interceptor itself lived. `state` is now scoped to each call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NZzg6qmouJqzvpNAwfnYxU
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/bun
@orpc/client
@orpc/cloudflare
@orpc/contract
@orpc/experimental-effect
@orpc/evlog
@orpc/hibernation
@orpc/json-schema
@orpc/experimental-lock
@orpc/experimental-msw
@orpc/nest
@orpc/next
@orpc/node
@orpc/openapi
@orpc/opentelemetry
@orpc/pinia-colada
@orpc/pino
@orpc/publisher
@orpc/ratelimit
@orpc/server
@orpc/shared
@orpc/swr
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/zod
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Move
stateinto the per-call closure inonFinish—let state: anyis now declared inside the returned async function inpackages/shared/src/interceptor.tsinstead of the factory closure, so the last call's result/error tuple is no longer pinned for the lifetime of the interceptor.
The fix is correct and minimally scoped. There is no await between assigning state and passing it into callback in the finally block, so concurrent calls never raced on the shared binding before; each invocation now gets its own tuple and the previous result/error becomes collectable once the call returns. The callback contract (tuple shape, options, rest args) and observable behavior are unchanged, so existing tests in packages/shared/src/interceptor.test.ts remain valid.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
orpc | 8cba8db | Commit Preview URL Branch Preview URL |
Sep 30 2026, 02:07 PM |
Two tests force full garbage collections (exposing gc through v8.setFlagsFromString) and check through a WeakRef that neither the last result nor the last error stays reachable from the interceptor once the call ends. Both fail before the fix. A third test pins that overlapping calls each get their own state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NZzg6qmouJqzvpNAwfnYxU
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Added GC-based leak tests —
packages/shared/src/interceptor.test.tsnow asserts, via aWeakRefand forced full GCs (isGarbageCollected), that the last call's result and thrown error become collectable after the call ends while the interceptor itself stays alive. - Added an overlapping-calls test — a failing call and a slow successful call interleave, pinning that each invocation's callback receives its own
[error, undefined, false]/[null, result, true]tuple.
The new tests are genuine regression coverage: I reverted state to the factory closure and both result and error cases fail at interceptor.test.ts:434, then pass with the fix restored. The overlapping test passes on main as expected — it pins the per-call contract rather than the leak. The fix itself was reviewed and approved in the prior run; this delta is test-only.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Expose gc() through `--expose-gc` in the vitest node project instead of enabling it from inside the test with node:v8 and node:vm, collect once after a macrotask instead of retrying, and drop the overlapping calls test, which passed before the fix too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NZzg6qmouJqzvpNAwfnYxU
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run covers only the delta since the prior pullfrog review (b6c4eb7b): commit cf4cd70, a test-simplification pass. The underlying fix was reviewed and approved earlier and is unchanged.
- Simplified the GC leak test — removed the
isGarbageCollectedhelper (setFlagsFromString+runInNewContext) and the retry loop; bothresult/errorcases now do onesetTimeoutmacrotask followed by a singleglobalThis.gc!()before asserting theWeakRefis cleared. - Dropped the overlapping-calls test — the per-call tuple contract it pinned is still covered by
interceptor.test.ts:292-373(viaintercept) and multiple cases initerator.test.ts, so no coverage gap is left. - Enabled
--expose-gcfor the root node test project —vitest.config.tsaddsexecArgv: ['--expose-gc'], scoped to the node project (jsdom/bun/cloudflare unaffected).
I re-verified the simplified test is genuine regression coverage: reverting state to the factory closure makes both cases fail at interceptor.test.ts:403, and 15/15 local repetitions pass with the fix in place. The single-gc() approach is not obviously flakier than the previous loop — a full GC collects the now-unreachable value deterministically once the interceptor no longer pins it.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Drop the garbage collection tests and the `--expose-gc` vitest flag they needed. Instead, check that overlapping calls each pass their own state to the callback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NZzg6qmouJqzvpNAwfnYxU
There was a problem hiding this comment.
ℹ️ No critical issues — one test-coverage observation inline.
Reviewed changes
This run covers only the delta since the prior pullfrog review (cf4cd70): commit 8cba8db, a test-only pass that drops the GC leak tests and the --expose-gc flag they needed.
- Replaced the GC leak tests — removed the
WeakRef/globalThis.gc!()it.eachcases and theexecArgv: ['--expose-gc']entry invitest.config.ts. - Added an overlapping-calls test — a slow successful call and an immediate failing call interleave, asserting each invocation's callback receives its own
[error, undefined, false]/[null, result, true]tuple.
The source fix itself is unchanged and remains correct. The only point worth a decision is that the new test cannot observe the leak.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| it('gives each call its own state when calls overlap', async () => { | ||
| const callback = vi.fn() | ||
| const interceptor = onFinish(callback) | ||
| const error = new Error('__error__') | ||
| const { promise, resolve } = promiseWithResolvers<string>() | ||
|
|
||
| const first = interceptor({ next: () => promise }) | ||
| await expect(interceptor({ next: () => Promise.reject(error) })).rejects.toBe(error) | ||
| resolve('__first__') | ||
| await expect(first).resolves.toBe('__first__') | ||
|
|
||
| expect(callback).toHaveBeenNthCalledWith(1, [error, undefined, false], expect.anything()) | ||
| expect(callback).toHaveBeenNthCalledWith(2, [null, '__first__', true], expect.anything()) |
There was a problem hiding this comment.
This test passes even when state is reverted to the factory closure (I verified locally), so it does not guard against the leak this PR fixes — it passes on main too. The GC-based test it replaced did fail when the fix was reverted, which leaves the fix with no regression coverage. If dropping GC-based testing is intentional (avoiding --expose-gc flakiness), a note that the leak is uncovered would help; otherwise keep one WeakRef/gc() case for it.
Technical details
# `onFinish` leak test no longer fails when the leak returns
## Affected sites
- `packages/shared/src/interceptor.test.ts:387-400` — new overlap test only asserts callback invocation order; the shared-`state` bug produces the same order and passes.
- `packages/shared/src/interceptor.ts:89` — the fix under test (per-call `let state`).
## Required outcome
- Either restore regression coverage that fails when `state` lives in the factory closure (needs `globalThis.gc!()` + `--expose-gc`, as the removed `it.each` did), or explicitly document that the fix is intentionally covered only by the contract/ordering test.
## Open questions for the human
- Was the GC test dropped for flakiness, or because `--expose-gc` was unwanted on the root node project? The two have different remedies (tighten the GC test vs. accept no leak coverage).This branch has not been deployed
2 participants
onFinishdeclaredlet statein the factory closure, not inside the function it returns. Every interceptor or middleware built withonFinish(...)therefore kept its most recent call's result or error reachable for as long as the interceptor lived. Interceptors are usually module-level and live for the whole process, so this could pin a large response object or an error with its cause chain.stateis now declared per call.Notes
statetuple as before. Concurrent calls were never mixed up, because nothing awaits between settingstateand passing it to the callback. The only change is that the result or error can be collected after the call.Testing
main, because the shared variable never mixed calls up; the retention itself would only show under forced garbage collection, which isn't tested.pnpm vitest run packages/shared,pnpm type:checkandpnpm lintpass.🤖 Generated with Claude Code
https://claude.ai/code/session_01NZzg6qmouJqzvpNAwfnYxU