fix(server): pass maxDurationMs timeout signal to subscription handlers #7037
Conversation
When using httpSubscriptionLink with maxDurationMs, the timeout was not being propagated to the subscription handler's opts.signal. This caused subscriptions that wait for the signal to abort (without yielding values) to hang indefinitely. The fix creates a combined AbortController that aborts when either: 1. The original request signal aborts (client disconnect) 2. The maxDurationMs timeout fires This ensures subscription handlers can properly respond to the timeout by checking opts.signal.aborted or listening for the 'abort' event. Fixes trpc#6991
|
@tt-a1i is attempting to deploy a commit to the trpc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughIntroduce a combined AbortSignal in resolveResponse that merges the request signal with an optional maxDurationMs timeout, propagate that combined signal to procedure invocations (including subscriptions), add timeout-driven abort and cleanup, remove the withMaxDuration async-iterable wrapper, and add regression tests for non-yielding subscriptions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant HTTP as HTTP Handler
participant Resolver as resolveResponse
participant Ponyfill as abortSignalsAnyPonyfill
participant Timer as MaxDuration Timer
participant Proc as Procedure (subscription)
Client->>HTTP: open SSE request (with req.signal)
HTTP->>Resolver: resolveResponse(req, proc, config)
Resolver->>Ponyfill: create combined controller from req.signal
alt config.sse?.maxDurationMs set
Resolver->>Timer: start timeout (maxDurationMs)
end
Resolver->>Proc: invoke procedure with combinedAbort.signal
Note over Proc: handler may run/loop without yielding
alt Timer fires
Timer-->>Resolver: timeout -> trigger cleanup
Resolver->>Resolver: remove listeners, clear timer, abort combined controller
Resolver-->>Proc: combinedAbort.signal abort dispatched
Proc-->>Resolver: onFinally/cleanup executes
Resolver-->>HTTP: close SSE / finish response
HTTP-->>Client: connection closed
else Normal completion/unsubscribe
Proc-->>Resolver: completes / unsubscribes
Resolver->>Resolver: cleanup (clear timer, remove listeners)
Resolver-->>HTTP: finish response
HTTP-->>Client: connection closed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
@trpc/client
@trpc/next
@trpc/react-query
@trpc/server
@trpc/tanstack-react-query
@trpc/upgrade
commit: |
![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)
packages/tests/server/httpSubscriptionLink.test.ts (1)
1363-1424: Regression test exercises non‑yielding subscription path correctly; consider tightening timing and assertionsThis test cleanly reproduces the “never-yielding subscription +
maxDurationMs” case and verifies both the abort signal (onSignalAborted) and the handler’sfinally(onFinally), which aligns well with the intent of #6991. Usingrun(() => testServerAndClientResource(...))withawait usingis also in line with the test resource pattern.Two optional robustness tweaks you might consider:
- The test currently depends on real timers with
MAX_DURATION_MS = 100and a small wait budget (MAX_DURATION_MS + 500). On a heavily loaded CI, this could become a bit timing-sensitive. You could instead switch to vitest fake timers (e.g. via a smallfakeTimersResourcewrapper or explicitvi.useFakeTimers()/vi.advanceTimersByTimeAsync(MAX_DURATION_MS + delta)) to make the test fully deterministic.- You already capture
onError; addingexpect(onError).not.toHaveBeenCalled()after the abort/finally assertions would lock in that this path is treated as a clean abort rather than surfacing an error to the client.As per coding guidelines, this test correctly uses
testServerAndClientResourcewithawait usingfor server/client setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts(1 hunks)packages/tests/server/httpSubscriptionLink.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Always useimport typefor type-only imports
Separate type imports from value imports
Avoid overzealous object destructuring; prefer direct property access
Object destructuring is acceptable when a variable is used 3+ times
Prefer array destructuring
Avoid sparse array destructuring
Never destructure in function parameter declarations
Avoid destructuring potentially nullish nested properties
Maximum 3 parameters per function; use options objects when more
Type parameter names must match /^(T|$)(A-Z)?[0-9]*$/
Prefix unused variables, parameters, and caught errors with_
Prefer namespace imports for validation libraries and large modules (e.g.,import * as z from 'zod',import * as React from 'react')
Follow import order: test helpers, tRPC test imports, third-party, then relative
Never import from@trpc/*/src; import from the package root instead
Do not useSymbol.disposeorSymbol.asyncDispose; usemakeResource()/makeAsyncResource()
Always useawait usingfor resource cleanup
PrefermakeResource()/makeAsyncResource()over manual disposal logic
Avoid non-null assertions (!)
Use proper type guards and optional chaining instead of non-null assertions
Switch statements must be exhaustive for union types
Rely on TypeScript inference; avoid unnecessary explicit return/output types
Use explicit types at public API boundaries, for complex generic constraints, or when inference is insufficient/ambiguous
Use thesatisfiesoperator to retain inference while enforcing shapes
Useas constfor literal type inference when appropriate
Prefer explicit typing overany
Use type assertions sparingly
Leverage TypeScript strict mode features
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/tests/server/httpSubscriptionLink.test.ts
**/*.{ts,tsx,md,mdx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Use camelCase for file names (with exceptions like TRPC/RPC/HTTP/JSON acronyms, .config.js, .d.ts, and tests)
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/tests/server/httpSubscriptionLink.test.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
No
console.login packages; use proper logging instead
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/tests/server/httpSubscriptionLink.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test-patterns.mdc)
**/*.test.ts: ALWAYS useawait using ctx = testServerAndClientResource(...)in tests that need both server and client setup
NEVER use the deprecatedrouterToServerAndClientNew()in tests
ImporttestServerAndClientResourcefrom@trpc/client/__tests__/testClientResource
Create fresh mock instances per test using a factory (e.g.,getMockFetch())
Do not use global mocks that persist across tests
Configure mocks via theclientcallback intestServerAndClientResourceoptions
Usectx.clientfrom the test resource for making tRPC calls
Access server URLs viactx.httpUrlandctx.wssUrl
Configure server options via theserverproperty intestServerAndClientResourceoptions
Configure client options via theclientcallback intestServerAndClientResourceoptions
Use descriptive test names that explain the behavior under test
Focus test names on what the test validates, not just what it does
Use proper TypeScript typing for mocks
Clear mocks between tests when needed
Use factory functions for mock creation to ensure isolation
Files:
packages/tests/server/httpSubscriptionLink.test.ts
{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}}: Do not use Testing LibrarywaitFor; usevi.waitForinstead
Tests may use non-null assertions
Tests may have unused variables
Tests may allow floating promises
Files:
packages/tests/server/httpSubscriptionLink.test.ts
🧠 Learnings (3)
📚 Learning: 2025-09-05T15:17:32.520Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/upgrade-tests.mdc:0-0
Timestamp: 2025-09-05T15:17:32.520Z
Learning: Applies to packages/upgrade/**/*.{test,spec,trpc,snap}.tsx : For migration testing, exercise both legacy and modern patterns via ctx.rq and ctx.trq within the same test when relevant
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:31.379Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/tanstack-react-query-tests.mdc:0-0
Timestamp: 2025-09-05T15:16:31.379Z
Learning: Applies to packages/tanstack-react-query/**/*.test.tsx : Use `ctx.useTRPCClient()` for direct (vanilla) tRPC client access in tests
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:48.745Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/test-patterns.mdc:0-0
Timestamp: 2025-09-05T15:16:48.745Z
Learning: Applies to **/*.test.ts : ALWAYS use `await using ctx = testServerAndClientResource(...)` in tests that need both server and client setup
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: typecheck
- GitHub Check: build
- GitHub Check: Lint and auto-fix
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
Outdated
Show resolved
Hide resolved
Address CodeRabbit review feedback: - Remove stale comment referencing non-existent timeoutId variable - Clear timeout when original request signal aborts for resource cleanliness
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Always useimport typefor type-only imports
Separate type imports from value imports
Avoid overzealous object destructuring; prefer direct property access
Object destructuring is acceptable when a variable is used 3+ times
Prefer array destructuring
Avoid sparse array destructuring
Never destructure in function parameter declarations
Avoid destructuring potentially nullish nested properties
Maximum 3 parameters per function; use options objects when more
Type parameter names must match /^(T|$)(A-Z)?[0-9]*$/
Prefix unused variables, parameters, and caught errors with_
Prefer namespace imports for validation libraries and large modules (e.g.,import * as z from 'zod',import * as React from 'react')
Follow import order: test helpers, tRPC test imports, third-party, then relative
Never import from@trpc/*/src; import from the package root instead
Do not useSymbol.disposeorSymbol.asyncDispose; usemakeResource()/makeAsyncResource()
Always useawait usingfor resource cleanup
PrefermakeResource()/makeAsyncResource()over manual disposal logic
Avoid non-null assertions (!)
Use proper type guards and optional chaining instead of non-null assertions
Switch statements must be exhaustive for union types
Rely on TypeScript inference; avoid unnecessary explicit return/output types
Use explicit types at public API boundaries, for complex generic constraints, or when inference is insufficient/ambiguous
Use thesatisfiesoperator to retain inference while enforcing shapes
Useas constfor literal type inference when appropriate
Prefer explicit typing overany
Use type assertions sparingly
Leverage TypeScript strict mode features
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
**/*.{ts,tsx,md,mdx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Use camelCase for file names (with exceptions like TRPC/RPC/HTTP/JSON acronyms, .config.js, .d.ts, and tests)
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
No
console.login packages; use proper logging instead
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build
- GitHub Check: Lint and auto-fix
- GitHub Check: test
- GitHub Check: typecheck
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
Outdated
Show resolved
Hide resolved
Address CodeRabbit review feedback:
- Check if request signal is already aborted before adding listener
- If already aborted, immediately propagate and clear timeout
- Use { once: true } for one-shot event listener
This prevents the subscription from running until maxDurationMs
when the request was already aborted before reaching this code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (1)
348-389: LGTM! Combined signal implementation is solid.The implementation correctly handles all abort scenarios:
- Timeout fires after
maxDurationMs→ subscription aborted withAbortError- Client disconnects → timeout cleared, subscription aborted with original reason
- Client already disconnected → immediate abort, no timer left running
The past review concerns have been properly addressed:
- Already-aborted
req.signalis detected and handled immediately (lines 380-382)- Event listener uses
{ once: true }to prevent multiple invocations (line 385)timeoutIdis correctly cleared when the request signal aborts (line 376)As an optional refinement: the
DOMExceptionmessage at line 369 could be more descriptive (e.g.,"Subscription exceeded maxDurationMs timeout"instead of"AbortError"), which would improve debugging visibility when handlers inspectsignal.reason.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Always useimport typefor type-only imports
Separate type imports from value imports
Avoid overzealous object destructuring; prefer direct property access
Object destructuring is acceptable when a variable is used 3+ times
Prefer array destructuring
Avoid sparse array destructuring
Never destructure in function parameter declarations
Avoid destructuring potentially nullish nested properties
Maximum 3 parameters per function; use options objects when more
Type parameter names must match /^(T|$)(A-Z)?[0-9]*$/
Prefix unused variables, parameters, and caught errors with_
Prefer namespace imports for validation libraries and large modules (e.g.,import * as z from 'zod',import * as React from 'react')
Follow import order: test helpers, tRPC test imports, third-party, then relative
Never import from@trpc/*/src; import from the package root instead
Do not useSymbol.disposeorSymbol.asyncDispose; usemakeResource()/makeAsyncResource()
Always useawait usingfor resource cleanup
PrefermakeResource()/makeAsyncResource()over manual disposal logic
Avoid non-null assertions (!)
Use proper type guards and optional chaining instead of non-null assertions
Switch statements must be exhaustive for union types
Rely on TypeScript inference; avoid unnecessary explicit return/output types
Use explicit types at public API boundaries, for complex generic constraints, or when inference is insufficient/ambiguous
Use thesatisfiesoperator to retain inference while enforcing shapes
Useas constfor literal type inference when appropriate
Prefer explicit typing overany
Use type assertions sparingly
Leverage TypeScript strict mode features
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
**/*.{ts,tsx,md,mdx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Use camelCase for file names (with exceptions like TRPC/RPC/HTTP/JSON acronyms, .config.js, .d.ts, and tests)
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
No
console.login packages; use proper logging instead
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: typecheck
- GitHub Check: Lint and auto-fix
🔇 Additional comments (1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (1)
396-396: LGTM! Signal correctly passed to procedure.The procedure now receives the combined signal (when applicable) or the original
req.signal, enabling subscription handlers to detectmaxDurationMstimeouts viaopts.signal. This achieves the PR objective of fixing issue #6991.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/unstable-core-do-not-import/stream/sse.ts (1)
45-68: ThemaxDurationMsoption is no longer functional but remains in the interface.The
maxDurationMsproperty (line 55) is still part ofSSEStreamProducerOptions, but the implementation no longer uses it. Per the PR changes, timeout handling has moved to the request layer (resolveResponse.ts), where the timeout signal is combined with the request signal before invoking the procedure.Leaving this option in the interface is misleading—callers might set it expecting timeout behavior that won't occur here. Consider one of:
- Remove the option entirely and update the JSDoc comments to clarify that duration limits are now enforced via the abort signal passed from the calling layer.
- Deprecate it with a
@deprecatedtag explaining that timeout enforcement has moved upstream and this parameter is ignored.Apply this diff to remove the unused option:
export interface SSEStreamProducerOptions<TValue = unknown> { serialize?: Serialize; data: AsyncIterable<TValue>; maxDepth?: number; ping?: SSEPingOptions; - /** - * Maximum duration in milliseconds for the request before ending the stream - * @default undefined - */ - maxDurationMs?: number; /** * End the request immediately after data is sent * Only useful for serverless runtimes that do not support streaming responses * @default false */ emitAndEndImmediately?: boolean;Alternatively, if you prefer to keep it for backward compatibility, deprecate it:
/** * Maximum duration in milliseconds for the request before ending the stream + * @deprecated No longer enforced at the stream producer level. Duration limits are now applied via the abort signal passed from the HTTP handler. * @default undefined */ maxDurationMs?: number;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts(2 hunks)packages/server/src/unstable-core-do-not-import/stream/sse.ts(1 hunks)packages/server/src/unstable-core-do-not-import/stream/utils/asyncIterable.ts(0 hunks)packages/tests/server/httpSubscriptionLink.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/server/src/unstable-core-do-not-import/stream/utils/asyncIterable.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Always useimport typefor type-only imports
Separate type imports from value imports
Avoid overzealous object destructuring; prefer direct property access
Object destructuring is acceptable when a variable is used 3+ times
Prefer array destructuring
Avoid sparse array destructuring
Never destructure in function parameter declarations
Avoid destructuring potentially nullish nested properties
Maximum 3 parameters per function; use options objects when more
Type parameter names must match /^(T|$)(A-Z)?[0-9]*$/
Prefix unused variables, parameters, and caught errors with_
Prefer namespace imports for validation libraries and large modules (e.g.,import * as z from 'zod',import * as React from 'react')
Follow import order: test helpers, tRPC test imports, third-party, then relative
Never import from@trpc/*/src; import from the package root instead
Do not useSymbol.disposeorSymbol.asyncDispose; usemakeResource()/makeAsyncResource()
Always useawait usingfor resource cleanup
PrefermakeResource()/makeAsyncResource()over manual disposal logic
Avoid non-null assertions (!)
Use proper type guards and optional chaining instead of non-null assertions
Switch statements must be exhaustive for union types
Rely on TypeScript inference; avoid unnecessary explicit return/output types
Use explicit types at public API boundaries, for complex generic constraints, or when inference is insufficient/ambiguous
Use thesatisfiesoperator to retain inference while enforcing shapes
Useas constfor literal type inference when appropriate
Prefer explicit typing overany
Use type assertions sparingly
Leverage TypeScript strict mode features
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/server/src/unstable-core-do-not-import/stream/sse.tspackages/tests/server/httpSubscriptionLink.test.ts
**/*.{ts,tsx,md,mdx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Use camelCase for file names (with exceptions like TRPC/RPC/HTTP/JSON acronyms, .config.js, .d.ts, and tests)
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/server/src/unstable-core-do-not-import/stream/sse.tspackages/tests/server/httpSubscriptionLink.test.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
No
console.login packages; use proper logging instead
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/server/src/unstable-core-do-not-import/stream/sse.tspackages/tests/server/httpSubscriptionLink.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test-patterns.mdc)
**/*.test.ts: ALWAYS useawait using ctx = testServerAndClientResource(...)in tests that need both server and client setup
NEVER use the deprecatedrouterToServerAndClientNew()in tests
ImporttestServerAndClientResourcefrom@trpc/client/__tests__/testClientResource
Create fresh mock instances per test using a factory (e.g.,getMockFetch())
Do not use global mocks that persist across tests
Configure mocks via theclientcallback intestServerAndClientResourceoptions
Usectx.clientfrom the test resource for making tRPC calls
Access server URLs viactx.httpUrlandctx.wssUrl
Configure server options via theserverproperty intestServerAndClientResourceoptions
Configure client options via theclientcallback intestServerAndClientResourceoptions
Use descriptive test names that explain the behavior under test
Focus test names on what the test validates, not just what it does
Use proper TypeScript typing for mocks
Clear mocks between tests when needed
Use factory functions for mock creation to ensure isolation
Files:
packages/tests/server/httpSubscriptionLink.test.ts
{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}}: Do not use Testing LibrarywaitFor; usevi.waitForinstead
Tests may use non-null assertions
Tests may have unused variables
Tests may allow floating promises
Files:
packages/tests/server/httpSubscriptionLink.test.ts
🧠 Learnings (3)
📚 Learning: 2025-09-05T15:16:01.878Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/react-query-tests.mdc:0-0
Timestamp: 2025-09-05T15:16:01.878Z
Learning: Applies to packages/react-query/**/*.test.tsx : Import createTRPCReact from 'trpc/react-query' (legacy API)
Applied to files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
📚 Learning: 2025-09-05T16:48:06.982Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-09-05T16:48:06.982Z
Learning: Applies to **/*.{ts,tsx} : Do not use `Symbol.dispose` or `Symbol.asyncDispose`; use `makeResource()`/`makeAsyncResource()`
Applied to files:
packages/server/src/unstable-core-do-not-import/stream/sse.ts
📚 Learning: 2025-09-05T16:48:06.982Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-09-05T16:48:06.982Z
Learning: Applies to **/*.{ts,tsx} : Prefer `makeResource()`/`makeAsyncResource()` over manual disposal logic
Applied to files:
packages/server/src/unstable-core-do-not-import/stream/sse.ts
🧬 Code graph analysis (2)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (1)
packages/server/src/unstable-core-do-not-import/utils.ts (1)
abortSignalsAnyPonyfill(96-119)
packages/tests/server/httpSubscriptionLink.test.ts (4)
packages/server/src/__tests__/fakeTimersResource.ts (1)
fakeTimersResource(3-16)packages/server/src/unstable-core-do-not-import/utils.ts (2)
run(64-64)sleep(88-90)packages/server/src/unstable-core-do-not-import/initTRPC.ts (1)
initTRPC(221-221)packages/client/src/__tests__/testClientResource.ts (1)
testServerAndClientResource(59-181)
🪛 Biome (2.1.2)
packages/tests/server/httpSubscriptionLink.test.ts
[error] 1379-1393: This generator function doesn't contain yield.
(lint/correctness/useYield)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint and auto-fix
- GitHub Check: typecheck
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (3)
packages/tests/server/httpSubscriptionLink.test.ts (1)
1363-1424: LGTM! Solid regression test for non-yielding subscription abort behavior.This test correctly validates the fix for issue #6991 by:
- Configuring
maxDurationMsat the tRPC config level (lines 1370-1372)- Creating a subscription that intentionally never yields, only waiting in a loop (lines 1379-1393)
- Advancing fake timers to trigger the timeout (line 1414)
- Asserting both the abort signal fires (
onSignalAborted) and cleanup occurs (onFinally)The Biome lint error flagging the missing
yieldis a false positive—the generator must not yield to properly test thatmaxDurationMsaborts subscriptions even when they produce no data. This is the exact scenario described in the linked issue.Note: You can suppress the Biome warning with a comment if it becomes noisy in CI:
// biome-ignore lint/correctness/useYield: intentionally non-yielding to test abort behaviorpackages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (2)
354-354: No action required. AbortSignal.timeout() has been supported since Node.js 17.3.0 (backported to 16.14.0), and the project's minimum supported Node version is 22.18.0, which well exceeds this requirement. No compatibility concerns apply.Likely an incorrect or invalid review comment.
352-363: No defensive handling needed foropts.req.signal.The code uses the Web Request API, where
signalis a read-only property that is always defined per the specification. The signal property of the Request interface returns the AbortSignal associated with the request. Defensive checks would be unnecessary and would contradict the Web API contract. The implementation at line 352 is correct as-is.Likely an incorrect or invalid review comment.
KATT
left a comment
KATT
left a comment
There was a problem hiding this comment.
I tried poking a bit because there was a memory leak in yours (timer never cleared). I simplified it hoping that AbortSignal.timeout would be GC'd automatically but that doesn't seem to be the case....
|
Good catch, thanks for digging into this. I agree I pushed an update that:
Commit: cbaa939 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (1)
393-395: Consider using a more descriptive error message for the DOMException.The
DOMExceptionconstructor signature is(message, name). Currently both arguments are'AbortError', which works but results in a less informative message.🔎 Suggested improvement:
timeoutId = setTimeout(() => { - combinedAc.abort(new DOMException('AbortError', 'AbortError')); + combinedAc.abort(new DOMException('Subscription timeout reached', 'AbortError')); }, maxDurationMs);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts(3 hunks)packages/tests/server/httpSubscriptionLink.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test-patterns.mdc)
**/*.test.ts: ALWAYS useawait using ctx = testServerAndClientResource(...)in tests that need both server and client setup
NEVER use the deprecatedrouterToServerAndClientNew()in tests
ImporttestServerAndClientResourcefrom@trpc/client/__tests__/testClientResource
Create fresh mock instances per test using a factory (e.g.,getMockFetch())
Do not use global mocks that persist across tests
Configure mocks via theclientcallback intestServerAndClientResourceoptions
Usectx.clientfrom the test resource for making tRPC calls
Access server URLs viactx.httpUrlandctx.wssUrl
Configure server options via theserverproperty intestServerAndClientResourceoptions
Configure client options via theclientcallback intestServerAndClientResourceoptions
Use descriptive test names that explain the behavior under test
Focus test names on what the test validates, not just what it does
Use proper TypeScript typing for mocks
Clear mocks between tests when needed
Use factory functions for mock creation to ensure isolation
Files:
packages/tests/server/httpSubscriptionLink.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Always useimport typefor type-only imports
Separate type imports from value imports
Avoid overzealous object destructuring; prefer direct property access
Object destructuring is acceptable when a variable is used 3+ times
Prefer array destructuring
Avoid sparse array destructuring
Never destructure in function parameter declarations
Avoid destructuring potentially nullish nested properties
Maximum 3 parameters per function; use options objects when more
Type parameter names must match /^(T|$)(A-Z)?[0-9]*$/
Prefix unused variables, parameters, and caught errors with_
Prefer namespace imports for validation libraries and large modules (e.g.,import * as z from 'zod',import * as React from 'react')
Follow import order: test helpers, tRPC test imports, third-party, then relative
Never import from@trpc/*/src; import from the package root instead
Do not useSymbol.disposeorSymbol.asyncDispose; usemakeResource()/makeAsyncResource()
Always useawait usingfor resource cleanup
PrefermakeResource()/makeAsyncResource()over manual disposal logic
Avoid non-null assertions (!)
Use proper type guards and optional chaining instead of non-null assertions
Switch statements must be exhaustive for union types
Rely on TypeScript inference; avoid unnecessary explicit return/output types
Use explicit types at public API boundaries, for complex generic constraints, or when inference is insufficient/ambiguous
Use thesatisfiesoperator to retain inference while enforcing shapes
Useas constfor literal type inference when appropriate
Prefer explicit typing overany
Use type assertions sparingly
Leverage TypeScript strict mode features
Files:
packages/tests/server/httpSubscriptionLink.test.tspackages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
**/*.{ts,tsx,md,mdx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Use camelCase for file names (with exceptions like TRPC/RPC/HTTP/JSON acronyms, .config.js, .d.ts, and tests)
Files:
packages/tests/server/httpSubscriptionLink.test.tspackages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}}: Do not use Testing LibrarywaitFor; usevi.waitForinstead
Tests may use non-null assertions
Tests may have unused variables
Tests may allow floating promises
Files:
packages/tests/server/httpSubscriptionLink.test.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
No
console.login packages; use proper logging instead
Files:
packages/tests/server/httpSubscriptionLink.test.tspackages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
🧠 Learnings (1)
📚 Learning: 2025-09-05T15:16:01.878Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/react-query-tests.mdc:0-0
Timestamp: 2025-09-05T15:16:01.878Z
Learning: Applies to packages/react-query/**/*.test.tsx : Import createTRPCReact from 'trpc/react-query' (legacy API)
Applied to files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
🧬 Code graph analysis (2)
packages/tests/server/httpSubscriptionLink.test.ts (3)
packages/server/src/__tests__/fakeTimersResource.ts (1)
fakeTimersResource(3-16)packages/server/src/unstable-core-do-not-import/utils.ts (2)
run(64-64)sleep(88-90)examples/next-sse-chat/src/app/channels/[channelId]/utils.ts (1)
run(1-1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (2)
packages/server/src/observable/observable.ts (2)
isObservable(16-18)observable(21-84)packages/server/src/unstable-core-do-not-import/utils.ts (2)
isAsyncIterable(53-59)run(64-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint and auto-fix
- GitHub Check: typecheck
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (4)
packages/tests/server/httpSubscriptionLink.test.ts (1)
1362-1426: Well-structured regression test for issue #6991.The test correctly:
- Uses
fakeTimersResource()withusingfor synchronous disposal- Uses
await usingwithrun()for the async test context resource- Follows the existing test patterns in this file (e.g.,
onSignalAborted,onFinallyspies)- Includes the biome-ignore comment explaining the intentional non-yielding behavior
One minor observation: the test doesn't verify reconnection behavior after the timeout, which aligns with the PR scope (core signal propagation fix). The issue mentioned reconnection latency as a potential future enhancement.
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (3)
1-23: LGTM - Import additions are appropriate.The
observableimport is correctly added to support wrapping observables with cleanup logic in the new combined signal implementation.
352-456: Solid implementation of combined abort signal for subscription timeouts.The implementation correctly:
- Creates a combined
AbortControllerthat aborts on either client disconnect ormaxDurationMstimeout- Handles the edge case where
opts.req.signalis already aborted (lines 397-401)- Properly cleans up timeout and event listeners via the
cleanupfunction- Wraps observables and async iterables to ensure cleanup runs on completion/error/unsubscribe
- Returns early for the subscription path with maxDurationMs, falling through to the default path otherwise
The cleanup logic with the
cleanedUpguard ensures idempotency across multiple abort/completion paths.
458-464: LGTM - Signal variable correctly used for non-subscription paths.The
signalvariable defaults toopts.req.signal(line 356) and is only modified for subscriptions with validmaxDurationMs. This ensures backward compatibility for all other procedure types and subscription configurations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (1)
348-458: Excellent implementation of the combined abort signal with robust cleanup!The implementation correctly:
- Creates a combined
AbortControllerthat respects both the request signal andmaxDurationMstimeout- Prevents resource leaks through careful cleanup of timeouts and event listeners
- Handles already-aborted signals immediately (lines 394-398) per the addressed review feedback
- Uses
{ once: true }to auto-remove the listener after firing- Wraps observables and async iterables to ensure cleanup runs on completion, error, or unsubscribe
- Explicitly clears value references during iteration to prevent memory retention during back-pressure (lines 441-448)
- Returns early (line 456) to prevent double-invocation of the procedure
The cleanup mechanism with the
cleanedUpflag correctly handles race conditions between timeout and request abort.Optional: Consider a more descriptive timeout error message
Line 391 uses
'AbortError'as both the message and name for the timeoutDOMException. While functional, a more descriptive message would improve debugging clarity:- combinedAc.abort(new DOMException('AbortError', 'AbortError')); + combinedAc.abort(new DOMException('Maximum duration exceeded', 'AbortError'));This makes timeout aborts easier to distinguish from other abort reasons in logs and error handlers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Always useimport typefor type-only imports
Separate type imports from value imports
Avoid overzealous object destructuring; prefer direct property access
Object destructuring is acceptable when a variable is used 3+ times
Prefer array destructuring
Avoid sparse array destructuring
Never destructure in function parameter declarations
Avoid destructuring potentially nullish nested properties
Maximum 3 parameters per function; use options objects when more
Type parameter names must match /^(T|$)(A-Z)?[0-9]*$/
Prefix unused variables, parameters, and caught errors with_
Prefer namespace imports for validation libraries and large modules (e.g.,import * as z from 'zod',import * as React from 'react')
Follow import order: test helpers, tRPC test imports, third-party, then relative
Never import from@trpc/*/src; import from the package root instead
Do not useSymbol.disposeorSymbol.asyncDispose; usemakeResource()/makeAsyncResource()
Always useawait usingfor resource cleanup
PrefermakeResource()/makeAsyncResource()over manual disposal logic
Avoid non-null assertions (!)
Use proper type guards and optional chaining instead of non-null assertions
Switch statements must be exhaustive for union types
Rely on TypeScript inference; avoid unnecessary explicit return/output types
Use explicit types at public API boundaries, for complex generic constraints, or when inference is insufficient/ambiguous
Use thesatisfiesoperator to retain inference while enforcing shapes
Useas constfor literal type inference when appropriate
Prefer explicit typing overany
Use type assertions sparingly
Leverage TypeScript strict mode features
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
**/*.{ts,tsx,md,mdx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Use camelCase for file names (with exceptions like TRPC/RPC/HTTP/JSON acronyms, .config.js, .d.ts, and tests)
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
No
console.login packages; use proper logging instead
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint and auto-fix
- GitHub Check: test
- GitHub Check: build
- GitHub Check: typecheck
🔇 Additional comments (2)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (2)
4-4: LGTM!The
observableimport is correctly added as a value import and is properly used to wrap subscription results later in the implementation.
460-467: LGTM!The
signalvariable is correctly passed to procedure invocations. The flow ensures:
- Subscriptions with
maxDurationMsreceive the combined signal (line 407) and return early (line 456)- Other procedures receive
opts.req.signal(the default value ofsignal)This maintains backward compatibility while enabling the new timeout functionality.
|
Quick update: the remaining I pushed a tiny follow-up that switches that temp variable to plain Commit: dad98d0 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (2)
369-383: Cleanup logic is solid.The guard pattern with
cleanedUpflag prevents double cleanup, and resources (timeout, event listener) are properly released.Minor naming note:
cleanupOnceat line 383 is just an alias forcleanup(). The "Once" suffix might suggest additional once-semantics, but that's already handled by thecleanedUpflag. Consider removing the alias or renaming to make the intent clearer.
385-398: LGTM! Abort handling is comprehensive.The code correctly:
- Propagates the original request signal's abort reason
- Fires a timeout after maxDurationMs
- Handles the edge case where the request is already aborted (addressing past review feedback)
- Uses
{ once: true }for automatic listener cleanupOptional refinement: At line 391, the DOMException message could be more descriptive. Consider:
combinedAc.abort(new DOMException('Subscription exceeded maxDurationMs', 'AbortError'));However, the current implementation with
'AbortError'as both message and name is functionally correct and aligns with standard abort error patterns.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Always useimport typefor type-only imports
Separate type imports from value imports
Avoid overzealous object destructuring; prefer direct property access
Object destructuring is acceptable when a variable is used 3+ times
Prefer array destructuring
Avoid sparse array destructuring
Never destructure in function parameter declarations
Avoid destructuring potentially nullish nested properties
Maximum 3 parameters per function; use options objects when more
Type parameter names must match /^(T|$)(A-Z)?[0-9]*$/
Prefix unused variables, parameters, and caught errors with_
Prefer namespace imports for validation libraries and large modules (e.g.,import * as z from 'zod',import * as React from 'react')
Follow import order: test helpers, tRPC test imports, third-party, then relative
Never import from@trpc/*/src; import from the package root instead
Do not useSymbol.disposeorSymbol.asyncDispose; usemakeResource()/makeAsyncResource()
Always useawait usingfor resource cleanup
PrefermakeResource()/makeAsyncResource()over manual disposal logic
Avoid non-null assertions (!)
Use proper type guards and optional chaining instead of non-null assertions
Switch statements must be exhaustive for union types
Rely on TypeScript inference; avoid unnecessary explicit return/output types
Use explicit types at public API boundaries, for complex generic constraints, or when inference is insufficient/ambiguous
Use thesatisfiesoperator to retain inference while enforcing shapes
Useas constfor literal type inference when appropriate
Prefer explicit typing overany
Use type assertions sparingly
Leverage TypeScript strict mode features
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
**/*.{ts,tsx,md,mdx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Use camelCase for file names (with exceptions like TRPC/RPC/HTTP/JSON acronyms, .config.js, .d.ts, and tests)
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
No
console.login packages; use proper logging instead
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: typecheck
- GitHub Check: Lint and auto-fix
- GitHub Check: build
🔇 Additional comments (4)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (4)
4-4: LGTM!The import of
observableis appropriate for wrapping subscription results to ensure cleanup logic runs on completion, error, or unsubscribe.
349-367: LGTM!The combined signal setup correctly:
- Scopes the change to subscription procedures only
- Validates maxDurationMs (positive, finite)
- Creates a new AbortController for signal composition
- Declares cleanup state variables appropriately
400-456: LGTM! Comprehensive cleanup integration.The implementation correctly:
- Invokes the subscription procedure with the combined signal (lines 402-408)
- Ensures cleanup on error via try-catch (lines 409-412)
- Wraps observables to call cleanup on error, completion, or unsubscribe (lines 415-435)
- Wraps async iterables with cleanup in a finally block, including explicit value clearing for memory management (lines 436-451)
- Handles non-stream results by cleaning up immediately (lines 452-454)
- Returns early to prevent the fallback code path from executing (line 456)
The wrapping logic ensures that cleanup (clearing the timeout and removing the abort listener) runs in all scenarios: normal completion, error, client disconnect, or timeout.
460-466: LGTM!The fallback path correctly uses the
signalvariable, which will be:
- The combined signal (with timeout) for subscription procedures with valid maxDurationMs
- The original
opts.req.signalfor all other casesThis ensures backward compatibility while enabling the new timeout behavior for subscriptions.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/tests/server/httpSubscriptionLink.test.ts (1)
36-36: ImportmakeResourcefrom the root@trpc/server/unstable-core-do-not-importpath for consistency.Both
makeResourceandmakeAsyncResourceare re-exported from@trpc/server/unstable-core-do-not-import, but the test imports them from different paths. Consider consolidating the imports to use the root path for both functions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts(4 hunks)packages/tests/server/httpSubscriptionLink.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Always useimport typefor type-only imports
Separate type imports from value imports
Avoid overzealous object destructuring; prefer direct property access
Object destructuring is acceptable when a variable is used 3+ times
Prefer array destructuring
Avoid sparse array destructuring
Never destructure in function parameter declarations
Avoid destructuring potentially nullish nested properties
Maximum 3 parameters per function; use options objects when more
Type parameter names must match /^(T|$)(A-Z)?[0-9]*$/
Prefix unused variables, parameters, and caught errors with_
Prefer namespace imports for validation libraries and large modules (e.g.,import * as z from 'zod',import * as React from 'react')
Follow import order: test helpers, tRPC test imports, third-party, then relative
Never import from@trpc/*/src; import from the package root instead
Do not useSymbol.disposeorSymbol.asyncDispose; usemakeResource()/makeAsyncResource()
Always useawait usingfor resource cleanup
PrefermakeResource()/makeAsyncResource()over manual disposal logic
Avoid non-null assertions (!)
Use proper type guards and optional chaining instead of non-null assertions
Switch statements must be exhaustive for union types
Rely on TypeScript inference; avoid unnecessary explicit return/output types
Use explicit types at public API boundaries, for complex generic constraints, or when inference is insufficient/ambiguous
Use thesatisfiesoperator to retain inference while enforcing shapes
Useas constfor literal type inference when appropriate
Prefer explicit typing overany
Use type assertions sparingly
Leverage TypeScript strict mode features
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/tests/server/httpSubscriptionLink.test.ts
**/*.{ts,tsx,md,mdx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
Use camelCase for file names (with exceptions like TRPC/RPC/HTTP/JSON acronyms, .config.js, .d.ts, and tests)
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/tests/server/httpSubscriptionLink.test.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
No
console.login packages; use proper logging instead
Files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/tests/server/httpSubscriptionLink.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test-patterns.mdc)
**/*.test.ts: ALWAYS useawait using ctx = testServerAndClientResource(...)in tests that need both server and client setup
NEVER use the deprecatedrouterToServerAndClientNew()in tests
ImporttestServerAndClientResourcefrom@trpc/client/__tests__/testClientResource
Create fresh mock instances per test using a factory (e.g.,getMockFetch())
Do not use global mocks that persist across tests
Configure mocks via theclientcallback intestServerAndClientResourceoptions
Usectx.clientfrom the test resource for making tRPC calls
Access server URLs viactx.httpUrlandctx.wssUrl
Configure server options via theserverproperty intestServerAndClientResourceoptions
Configure client options via theclientcallback intestServerAndClientResourceoptions
Use descriptive test names that explain the behavior under test
Focus test names on what the test validates, not just what it does
Use proper TypeScript typing for mocks
Clear mocks between tests when needed
Use factory functions for mock creation to ensure isolation
Files:
packages/tests/server/httpSubscriptionLink.test.ts
{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}}: Do not use Testing LibrarywaitFor; usevi.waitForinstead
Tests may use non-null assertions
Tests may have unused variables
Tests may allow floating promises
Files:
packages/tests/server/httpSubscriptionLink.test.ts
🧠 Learnings (14)
📚 Learning: 2025-09-05T15:16:01.878Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/react-query-tests.mdc:0-0
Timestamp: 2025-09-05T15:16:01.878Z
Learning: Applies to packages/react-query/**/*.test.tsx : Import createTRPCReact from 'trpc/react-query' (legacy API)
Applied to files:
packages/server/src/unstable-core-do-not-import/http/resolveResponse.tspackages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:48.745Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/test-patterns.mdc:0-0
Timestamp: 2025-09-05T15:16:48.745Z
Learning: Applies to **/*.test.ts : Import `testServerAndClientResource` from `trpc/client/__tests__/testClientResource`
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:31.379Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/tanstack-react-query-tests.mdc:0-0
Timestamp: 2025-09-05T15:16:31.379Z
Learning: Applies to packages/tanstack-react-query/**/*.test.tsx : Do NOT import `testReactResource` from `./test/__helpers` (wrong path)
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:48.745Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/test-patterns.mdc:0-0
Timestamp: 2025-09-05T15:16:48.745Z
Learning: Applies to **/*.test.ts : Use `ctx.client` from the test resource for making tRPC calls
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:01.878Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/react-query-tests.mdc:0-0
Timestamp: 2025-09-05T15:16:01.878Z
Learning: Applies to packages/react-query/**/*.test.tsx : Do NOT import testReactResource from './__helpers' or './test/__helpers' in legacy React Query tests
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:48.745Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/test-patterns.mdc:0-0
Timestamp: 2025-09-05T15:16:48.745Z
Learning: Applies to **/*.test.ts : Configure client options via the `client` callback in `testServerAndClientResource` options
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:17:32.520Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/upgrade-tests.mdc:0-0
Timestamp: 2025-09-05T15:17:32.520Z
Learning: Applies to packages/upgrade/**/*.{test,spec,trpc,snap}.tsx : Import both 'trpc/react-query' as rq (legacy) and 'trpc/tanstack-react-query' as trq (modern) when writing migration tests
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:48.745Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/test-patterns.mdc:0-0
Timestamp: 2025-09-05T15:16:48.745Z
Learning: Applies to **/*.test.ts : Access server URLs via `ctx.httpUrl` and `ctx.wssUrl`
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:48.745Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/test-patterns.mdc:0-0
Timestamp: 2025-09-05T15:16:48.745Z
Learning: Applies to **/*.test.ts : Configure mocks via the `client` callback in `testServerAndClientResource` options
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:16:31.379Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/tanstack-react-query-tests.mdc:0-0
Timestamp: 2025-09-05T15:16:31.379Z
Learning: Applies to packages/tanstack-react-query/**/*.test.tsx : Import `testReactResource` from the local path `./__helpers`
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T15:17:32.520Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/upgrade-tests.mdc:0-0
Timestamp: 2025-09-05T15:17:32.520Z
Learning: Applies to packages/upgrade/**/*.{test,spec,trpc,snap}.tsx : For migration testing, exercise both legacy and modern patterns via ctx.rq and ctx.trq within the same test when relevant
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T16:48:06.982Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-09-05T16:48:06.982Z
Learning: Applies to **/*.{ts,tsx} : Prefer `makeResource()`/`makeAsyncResource()` over manual disposal logic
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T16:48:06.982Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-09-05T16:48:06.982Z
Learning: Applies to **/*.{ts,tsx} : Do not use `Symbol.dispose` or `Symbol.asyncDispose`; use `makeResource()`/`makeAsyncResource()`
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
📚 Learning: 2025-09-05T16:48:06.982Z
Learnt from: CR
Repo: trpc/trpc PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-09-05T16:48:06.982Z
Learning: Share common functionality through `trpc/server` and keep client/server concerns separate
Applied to files:
packages/tests/server/httpSubscriptionLink.test.ts
🧬 Code graph analysis (1)
packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (1)
packages/server/src/unstable-core-do-not-import/utils.ts (1)
abortSignalsAnyPonyfill(96-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: typecheck
- GitHub Check: build
- GitHub Check: test
- GitHub Check: Lint and auto-fix
🔇 Additional comments (6)
packages/tests/server/httpSubscriptionLink.test.ts (2)
1364-1427: Well-structured regression test for issue #6991.The test correctly validates that
maxDurationMsaborts non-yielding subscriptions by:
- Using fake timers for deterministic time control
- Setting up a subscription that intentionally blocks without yielding
- Verifying that both the abort signal handler and finally block execute after the timeout
1429-1541: Comprehensive timer cleanup test.The test effectively validates that the
maxDurationMstimer is properly cleared when a subscription ends before the timeout. The approach of spying onsetTimeout/clearTimeoutto track the specific timer by its duration value is a clever way to verify cleanup without relying on fake timers.packages/server/src/unstable-core-do-not-import/http/resolveResponse.ts (4)
46-53: Clean abstraction for combined abort signals.This helper cleanly encapsulates the pattern of creating a controllable abort mechanism that also respects an external signal. The use of
abortSignalsAnyPonyfillensures compatibility across environments while providing the semantics ofAbortSignal.any().
362-371: Correct timeout handling with proper cleanup.The implementation correctly handles both scenarios:
- Timer fires first: Clears the (already-fired) timer, removes the listener, then aborts the controller to signal the subscription.
- Request aborts first: The listener fires, clears the pending timer, then aborts (redundant but harmless).
The listener removal before
abort()prevents recursive cleanup calls.
472-475: Note:maxDurationMsbehavior differs between async generators and observables.The observable-to-iterable conversion uses
opts.req.signalrather than the combined signal, meaningmaxDurationMswon't abort an observable-based subscription's stream.This appears intentional since:
- Async generators can block before yielding → timeout aborts the blocking procedure
- Observables return immediately → timeout on procedure invocation doesn't apply
If future requirements need
maxDurationMsto limit observable streams, consider passing a combined signal here. For now, this aligns with the PR's stated scope of fixing non-yielding subscription handlers.
333-333: Unified signal propagation for all procedure types.Creating a combined abort controller for every RPC call provides consistent behavior: all procedures receive a signal that responds to request abort, while subscriptions additionally respond to
maxDurationMstimeouts. This is a clean approach.Also applies to: 378-378
|
|
||
| test('timer does not leak after subscription ends', async () => { | ||
| const nonce = Math.floor(Math.random() * 1000); | ||
| const MAX_DURATION_MS = 60_0000 * 60 * 24 + nonce; // 1 day + nonce |
There was a problem hiding this comment.
Numeric literal typo: 60_0000 should be 60_000.
The comment says "1 day + nonce" but 60_0000 (600,000) × 60 × 24 = 864,000,000 ms = 10 days, not 1 day.
For 1 day: 60_000 (60,000) × 60 × 24 = 86,400,000 ms.
🔎 Proposed fix
- const MAX_DURATION_MS = 60_0000 * 60 * 24 + nonce; // 1 day + nonce
+ const MAX_DURATION_MS = 60_000 * 60 * 24 + nonce; // 1 day + nonce📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const MAX_DURATION_MS = 60_0000 * 60 * 24 + nonce; // 1 day + nonce | |
| const MAX_DURATION_MS = 60_000 * 60 * 24 + nonce; // 1 day + nonce |
🤖 Prompt for AI Agents
In packages/tests/server/httpSubscriptionLink.test.ts around line 1431, the
numeric literal uses 60_0000 which makes the duration 10 days instead of the
intended 1 day; change the literal to 60_000 so the expression reads 60_000 * 60
* 24 + nonce (1 day + nonce) and run tests to confirm expected timing behavior.
|
This pull request has been locked because we are very unlikely to see comments on closed issues. If you think, this PR is still necessary, create a new one with the same branch. Thank you. |
2 participants
Summary
When using
httpSubscriptionLinkwithmaxDurationMs, the timeout was not being propagated to the subscription handler'sopts.signal. This caused subscriptions that wait for the signal to abort (without yielding values) to hang indefinitely, as described in #6991.The fix:
AbortControllerthat aborts when either:maxDurationMstimeout firesopts.signalAbortController.abort()withAbortError(notAbortSignal.timeout()which throwsTimeoutError)Now subscription handlers can properly respond to the
maxDurationMstimeout by:opts.signal.aborted'abort'event onopts.signalTest plan
maxDurationMsaborts subscriptions that don't yieldhttpSubscriptionLink.test.tstests pass (16 tests)Fixes #6991
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.