mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-22 18:13:27 +00:00
🛡️ test: address comprehensive review NITs (initial-emit guard + isStreamWritable coverage)
NIT — chat-completions initial emit skips writableEnded check The Open Responses initial emit was switched to use the new `isStreamWritable` predicate in the round-3 commit, but the chat-completions initial emit kept the older narrower check (`streamId || res.headersSent`). On a client disconnect mid-stream (`writableEnded === true`) it would still hit `res.write` and raise `ERR_STREAM_WRITE_AFTER_END` — caught by the outer IIFE catch but logged as noise. Switch this site to `isStreamWritable` too so both initial-emit paths share the same gate as the deferred update emits. NIT — `isStreamWritable` not directly unit-tested The predicate was only covered indirectly via the deferred-preview SSE tests (writableEnded skip, headersSent check). Export from `callbacks.js` and add 5 parametric tests pinning down each branch (streamId truthy, res null, !headersSent, writableEnded, happy path) so a future condition addition can't silently regress.
This commit is contained in:
parent
5d6b0a5c9b
commit
8b990d654a
2 changed files with 47 additions and 2 deletions
|
|
@ -575,3 +575,39 @@ describe('createToolEndCallback', () => {
|
|||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('isStreamWritable', () => {
|
||||
/* Direct parametric coverage of the predicate that gates SSE writes
|
||||
* in both the chat-completions and Open Responses callbacks. The
|
||||
* existing deferred-preview tests cover this indirectly via the
|
||||
* `writeAttachmentUpdate` writableEnded path; these tests pin down
|
||||
* each individual branch so a future modification (e.g. adding a
|
||||
* new condition) can't silently regress.
|
||||
* (Comprehensive review NIT on PR #12957.) */
|
||||
const { isStreamWritable } = require('../callbacks');
|
||||
|
||||
it('returns true when streamId is truthy regardless of res state', () => {
|
||||
/* Resumable mode writes go to the job emitter; res state is
|
||||
* irrelevant. Even a closed res with no headers should not block. */
|
||||
expect(isStreamWritable(null, 'stream-1')).toBe(true);
|
||||
expect(isStreamWritable({ headersSent: false, writableEnded: true }, 'stream-1')).toBe(true);
|
||||
expect(isStreamWritable(undefined, 'stream-1')).toBe(true);
|
||||
});
|
||||
|
||||
it('returns false when streamId is falsy and res is null/undefined', () => {
|
||||
expect(isStreamWritable(null, null)).toBe(false);
|
||||
expect(isStreamWritable(undefined, null)).toBe(false);
|
||||
});
|
||||
|
||||
it('returns false when headers have not been sent yet', () => {
|
||||
expect(isStreamWritable({ headersSent: false, writableEnded: false }, null)).toBe(false);
|
||||
});
|
||||
|
||||
it('returns false when the stream has already ended', () => {
|
||||
expect(isStreamWritable({ headersSent: true, writableEnded: true }, null)).toBe(false);
|
||||
});
|
||||
|
||||
it('returns true on the happy path: headers sent, not ended, no streamId', () => {
|
||||
expect(isStreamWritable({ headersSent: true, writableEnded: false }, null)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -641,8 +641,16 @@ function createToolEndCallback({ req, res, artifactPromises, streamId = null })
|
|||
/* Initial emit: ship the attachment to the client immediately
|
||||
* (carries `status: 'pending'` for office buckets so the UI
|
||||
* shows "preparing preview…"). The agent's response stops
|
||||
* blocking on extraction here. */
|
||||
if (streamId || res.headersSent) {
|
||||
* blocking on extraction here.
|
||||
*
|
||||
* Use the shared `isStreamWritable` predicate rather than the
|
||||
* narrower `streamId || res.headersSent` check that lived
|
||||
* here before — a client disconnect mid-stream
|
||||
* (`res.writableEnded`) would otherwise hit `res.write` and
|
||||
* raise `ERR_STREAM_WRITE_AFTER_END` (caught by the outer
|
||||
* IIFE catch but logged as noise). Same gate the Responses
|
||||
* path uses below. */
|
||||
if (isStreamWritable(res, streamId)) {
|
||||
writeAttachment(res, streamId, fileMetadata);
|
||||
}
|
||||
/* Deferred preview rendering: extraction continues running
|
||||
|
|
@ -1017,6 +1025,7 @@ module.exports = {
|
|||
agentLogHandlerObj,
|
||||
getDefaultHandlers,
|
||||
createToolEndCallback,
|
||||
isStreamWritable,
|
||||
markSummarizationUsage,
|
||||
buildSummarizationHandlers,
|
||||
createResponsesToolEndCallback,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue