[NA] [SDK] fix: respect search wait timeouts - #7960
Conversation
| const traces = await parseNdjsonStreamToArray<OpikApi.TracePublic>( | ||
| streamResponse, | ||
| serialization.TracePublic, | ||
| maxResults | ||
| maxResults, | ||
| ); |
There was a problem hiding this comment.
Truncation and stream resource leak
parseNdjsonStreamToArray stops after maxResults, so client.searchTraces() truncates backend-overdelivered batches allowed by the integration contract — should we parse the complete response instead? When stopping early, it only calls reader.releaseLock() rather than cancelling the ReadableStream, so remaining trace/thread/span bodies can continue consuming connection resources — should we call await reader.cancel() with lock release in a finally block?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/typescript/src/opik/utils/searchHelpers.ts` around lines 26-30, update
`searchTracesWithFilters` and the analogous thread/span search helpers so they do not
pass `maxResults` as a client-side cap to `parseNdjsonStreamToArray`; the backend may
legitimately return more results than requested. If an intentional parser limit must
remain, modify the early-return path to `await reader.cancel()` and release the lock in
a `finally` block so remaining HTTP stream resources are not consumed.
| oqlFactory: (filterString: string) => OpikQueryLanguage, | ||
| ): TFilter[] | null { |
There was a problem hiding this comment.
Misleading parser documentation omits spans
The generic parser comment still says it supports “both traces and threads,” even though oqlFactory is used by forTraces, forThreads, and forSpans to select the resource-specific OQL schema, so it can mislead contributors — should we update it to mention traces, threads, spans, and oqlFactory’s role?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/typescript/src/opik/utils/searchHelpers.ts` around lines 102-103, update the
`parseFilterStringGeneric` documentation to state that it supports traces, threads, and
spans. Document that `oqlFactory` selects the resource-specific OQL schema/parser used
to interpret the filter string, so contributors understand how to extend or reuse this
helper.
| maxResults: number, | ||
| truncate: boolean, | ||
| exclude?: OpikApi.SpanSearchStreamRequestPublicExcludeItem[] | ||
| exclude?: OpikApi.SpanSearchStreamRequestPublicExcludeItem[], |
There was a problem hiding this comment.
Generated enum blocks forward compatibility
The public searchSpansWithFilters helper types exclude as OpikApi.SpanSearchStreamRequestPublicExcludeItem[], so callers cannot pass forward-compatible backend values — should we use exclude?: string[] for both searchSpansWithFilters and searchTracesWithFilters?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/typescript/src/opik/utils/searchHelpers.ts around lines 176 and 16, update the
`exclude` parameters of `searchSpansWithFilters` and `searchTracesWithFilters` from the
generated exclusion-item array types to `string[]`. Preserve forwarding these values to
the API request while decoupling the public helpers from the current generated enum so
future backend exclusion values are accepted.
|
Thanks for the review. I checked each Baz finding against the diff and am keeping this PR scoped to the reported timeout-boundary bug:
I am leaving these as separate follow-up work rather than bundling unrelated behavior changes into this focused PR. |
Details
searchAndWaitForDonecurrently sleeps for the full polling interval even when less time remains in the caller-provided timeout. This can make a short timeout wait for several seconds longer than requested. The polling sleep is now capped to the remaining timeout budget, with a fake-timer regression covering a poll interval longer than the timeout.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
./node_modules/.bin/vitest run tests/unit/utils/searchHelpers.test.ts— 28 passed./node_modules/.bin/eslint src/opik/utils/searchHelpers.ts tests/unit/utils/searchHelpers.test.ts— passed./node_modules/.bin/prettier --check src/opik/utils/searchHelpers.ts tests/unit/utils/searchHelpers.test.ts— passedgit diff --check origin/main...HEAD— passedDocumentation
No documentation changes are needed for this internal timeout-boundary fix.