Repository navigation
test(api): real wiring assertion for chat keepalive correlationId (#14850) - #15254
Merged
diegosouzapw merged 2 commits intoOct 2, 2026
Conversation
…egosouzapw#14850) The previous test re-recorded the keepalive bytes itself before asserting, so it passed whether or not /v1/chat/completions passes `correlationId: reqId` to withEarlyStreamKeepalive (diegosouzapw#14792). Its header also claimed the test could not see the wrapper's buffer because of a module-instance split; that is not the case: the test and the wrapper share earlyKeepaliveByteBuffer. - New wiring test: real route, upstream held open, read the first frame, then assert the wrapper buffered its bytes under the caller's X-Correlation-Id. Red with the route line removed, green with it. - End-to-end test kept without the self-recording fallback. - Upstream held by a promise the test releases instead of a real 5s sleep, and the 20s row poll is now 5s. Each test runs in ~1.4s; the only real wait is the route's 1s keepalive threshold. Closes diegosouzapw#14850
diegosouzapw
merged commit Oct 2, 2026
82d30cf
into
diegosouzapw:release/v3.8.52
15 of 16 checks passed
Owner
|
Thanks @yourspraveen — merged into |
5 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #14850
Problem
tests/unit/chat-keepalive-correlation-functional.test.tscallsrecordEarlyKeepaliveBytes(correlationId, …)itself before asserting. So it passed whether or not/v1/chat/completionspassescorrelationId: reqIdtowithEarlyStreamKeepalive(#14792). It also slept 5 s of real time and polled for up to 20 s.The file's header justified the self-recording by claiming the test can't see the wrapper's buffer because of a module-instance split. That claim is wrong: the test and the wrapper share one
earlyKeepaliveByteBufferinstance, and the wrapper's bytes are visible under the caller's correlation id.Change (test-only)
X-Correlation-Idwhile the upstream is held open, and reads the first frame. It then assertstakeEarlyKeepaliveBytes(correlationId)holds the startup frame. The wrapper only buffers when the route passescorrelationId.fetchwaits on a promise the test releases. The only real wait is the route's own 1 s keepalive threshold foropenai/*; the row poll is now capped at 5 s instead of 20 s.Red / green
correlationId: reqIdThe new test passed 3/3 consecutive runs. Prettier, ESLint and typecheck are clean on the file.
Note for review
The end-to-end test also passes with the route line removed. So the keepalive startup frame reaches
streamChunks.clientin the persisted row by some other path, not only through the correlation buffer. That's why the wiring assertion, not the end-to-end test, is the regression guard for #14792. I didn't trace that second path here, since it's outside this follow-up's scope.Thanks @maxmad64bis for #14792.