Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions apps/gateway/src/chat/chat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ import type { ServerTypes } from "@/vars.js";
// Pre-compiled regex pattern to avoid recompilation per request
const SSE_FIELD_PATTERN = /^[a-zA-Z_-]+:\s*/;

// Reusable TextDecoder to avoid per-chunk allocation in the streaming hot path
const sharedTextDecoder = new TextDecoder();
Comment on lines +110 to +111

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

TextDecoder stream:true internal state buffer concurrent calls

💡 Result:

  • TextDecoder has mutable internal state: a decoder instance plus an internal I/O queue (byte buffer) and flags. On each decode(input, { stream }) call, the input bytes are pushed into that internal queue and the decoder consumes from it. (encoding.spec.whatwg.org)
  • { stream: true } means “do not flush”: when the internal queue reaches end-of-queue, the algorithm returns without resetting the decoder, so any incomplete multi‑byte sequence (or any other state for stateful encodings) can be completed by later calls. (encoding.spec.whatwg.org)
  • You’re expected to call decode(..., {stream:true}) zero or more times, then call decode() once with stream omitted/false to flush remaining buffered state (often producing "" if nothing was buffered). (encoding.spec.whatwg.org)
  • It is not an “accumulate everything until the end” switch: each streaming call still returns whatever can be decoded now; only the undecodable tail is buffered. This is a common point of confusion. (github.com)

Concurrent / interleaved calls: decode() itself is synchronous, so you don’t get true parallel execution in one JS thread—but you can accidentally interleave use of the same TextDecoder across multiple byte streams (e.g., two async sources). Because the decoder and internal queue are shared mutable state, interleaving will corrupt decoding. Use one TextDecoder per byte stream, or use TextDecoderStream per stream pipeline. (encoding.spec.whatwg.org)

Citations:


🏁 Script executed:

cd /tmp && find . -name "chat.ts" -path "*/gateway/src/*" 2>/dev/null | head -5

Repository: theopenco/llmgateway

Length of output: 46


🏁 Script executed:

find . -name "chat.ts" -path "*/gateway/src/*"

Repository: theopenco/llmgateway

Length of output: 95


🏁 Script executed:

wc -l ./apps/gateway/src/chat/chat.ts

Repository: theopenco/llmgateway

Length of output: 100


🏁 Script executed:

sed -n '100,120p' ./apps/gateway/src/chat/chat.ts

Repository: theopenco/llmgateway

Length of output: 929


🏁 Script executed:

sed -n '3120,3200p' ./apps/gateway/src/chat/chat.ts

Repository: theopenco/llmgateway

Length of output: 3029


🏁 Script executed:

sed -n '3420,3440p' ./apps/gateway/src/chat/chat.ts

Repository: theopenco/llmgateway

Length of output: 769


🏁 Script executed:

sed -n '3100,3130p' ./apps/gateway/src/chat/chat.ts

Repository: theopenco/llmgateway

Length of output: 1266


🏁 Script executed:

grep -n "sharedTextDecoder" ./apps/gateway/src/chat/chat.ts

Repository: theopenco/llmgateway

Length of output: 183


Critical: shared TextDecoder with { stream: true } corrupts data across concurrent requests.

The module-level sharedTextDecoder is used inside an async streaming loop that awaits reader.read(). While one request is blocked on the await, another concurrent request can execute and call sharedTextDecoder.decode(). Since TextDecoder with { stream: true } maintains mutable internal state (buffered incomplete multi-byte sequences), interleaved calls corrupt both streams—the decoder will prepend bytes buffered from request A to request B's input.

Each streaming request needs its own TextDecoder instance. Move the declaration from the module level into the streaming handler (around line 3122, after let buffer = "").

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/chat/chat.ts` around lines 110 - 111, The shared
module-level TextDecoder (sharedTextDecoder) is unsafe for concurrent streaming
because TextDecoder with {stream:true} keeps internal mutable state; create a
new TextDecoder per streaming request instead of using the module-level
instance. Inside the streaming handler (where you have let buffer = "" and you
call reader.read()), move the TextDecoder instantiation into that scope (create
a local const textDecoder = new TextDecoder()) and replace uses of
sharedTextDecoder.decode(...) with textDecoder.decode(...) so each request has
its own decoder state.

Comment on lines +110 to +111

Copilot AI Feb 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared TextDecoder instance with stream: true option creates a concurrency bug. When stream: true is used, TextDecoder maintains internal state for multi-byte character sequences across chunk boundaries. With concurrent requests, one request's decode() call can corrupt another request's multi-byte character state, leading to incorrect character decoding. The TextDecoder should be created per-request (inside the streaming loop setup) rather than shared globally.

Suggested change
// Reusable TextDecoder to avoid per-chunk allocation in the streaming hot path
const sharedTextDecoder = new TextDecoder();
// TextDecoder wrapper: creates a fresh TextDecoder per decode call to avoid
// sharing internal streaming state across concurrent requests.
const sharedTextDecoder = {
decode(
input?: BufferSource,
options?: TextDecodeOptions,
): string {
const decoder = new TextDecoder();
return decoder.decode(input, options);
},
};

Copilot uses AI. Check for mistakes.

export const chat = new OpenAPIHono<ServerTypes>();

const completions = createRoute({
Expand Down Expand Up @@ -3173,7 +3176,7 @@ chat.openapi(completions, async (c) => {
}
} else {
// Convert the Uint8Array to a string for SSE
chunk = new TextDecoder().decode(value);
chunk = sharedTextDecoder.decode(value, { stream: true });

Copilot AI Feb 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the shared TextDecoder with stream: true creates a state corruption issue when multiple requests are processed concurrently. Create a new TextDecoder instance at the start of the streaming section instead, e.g., before line 3151: const decoder = new TextDecoder(); and use it with decoder.decode(value, { stream: true }).

Copilot uses AI. Check for mistakes.
}

// Log error on large chunks (1MB+) - should almost never happen
Expand Down Expand Up @@ -3422,7 +3425,13 @@ chat.openapi(completions, async (c) => {
.trim();

// Debug logging for troublesome events
if (eventData.includes("event:") || eventData.includes("id:")) {
// Only scan for SSE field contamination on small events to avoid
// O(n) scans on multi-MB payloads (e.g. base64 image data).
// Large events (>64KB) are almost always valid image/binary data.
if (
eventData.length < 65536 &&
(eventData.includes("event:") || eventData.includes("id:"))
) {
logger.warn("Event data contains SSE field", {
eventData:
eventData.substring(0, 200) +
Expand Down
6 changes: 5 additions & 1 deletion apps/gateway/src/chat/tools/extract-images.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import type { ImageObject } from "./types.js";
import type { Provider } from "@llmgateway/models";

/**
* Extracts images from streaming data based on provider format
* Extracts images from streaming data based on provider format.
*
* For large base64 image data, we reference the original inlineData fields
* directly rather than creating new concatenated strings, to avoid unnecessary
* multi-MB string copies.
Comment on lines +7 to +9

Copilot AI Feb 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation comment states "we reference the original inlineData fields directly rather than creating new concatenated strings, to avoid unnecessary multi-MB string copies," but the code at line 22 uses template literals to create new concatenated strings. If avoiding string copies is the goal, the code should be refactored to return references to the original data or use a different approach. Otherwise, remove or update the comment to accurately reflect what the code does.

Suggested change
* For large base64 image data, we reference the original inlineData fields
* directly rather than creating new concatenated strings, to avoid unnecessary
* multi-MB string copies.
* For providers that return inlineData (for example, Google formats), this
* function locates image parts and constructs data URLs from their mimeType
* and base64-encoded data.

Copilot uses AI. Check for mistakes.
*/
export function extractImages(data: any, provider: Provider): ImageObject[] {
switch (provider) {
Expand Down
58 changes: 58 additions & 0 deletions apps/gateway/src/chat/tools/might-be-complete-json.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,62 @@ describe("mightBeCompleteJson", () => {
it("returns false for actually unbalanced brackets", () => {
expect(mightBeCompleteJson("[1,2]]")).toBe(false);
});

// Tests for large payload optimization (>100KB threshold)
describe("large payloads (>100KB)", () => {
const LARGE_SIZE = 120 * 1024; // 120KB to exceed the 100KB threshold

it("returns true for large valid object with big base64 string value", () => {
// Simulate a Google streaming chunk with inline base64 image data
const base64Data = "A".repeat(LARGE_SIZE);
const json = `{"candidates":[{"content":{"parts":[{"inlineData":{"mimeType":"image/png","data":"${base64Data}"}}]}}]}`;
expect(mightBeCompleteJson(json)).toBe(true);
});

it("returns false for incomplete large object", () => {
const base64Data = "A".repeat(LARGE_SIZE);
// Missing closing brackets
const json = `{"candidates":[{"content":{"parts":[{"inlineData":{"data":"${base64Data}"`;
expect(mightBeCompleteJson(json)).toBe(false);
});

it("returns true for large object with escaped quotes in string value", () => {
const base64Data = "A".repeat(LARGE_SIZE);
const json = `{"key":"value with \\"escaped\\" quotes","data":"${base64Data}"}`;
expect(mightBeCompleteJson(json)).toBe(true);
});

it("returns false for large object missing closing brace", () => {
const base64Data = "A".repeat(LARGE_SIZE);
const json = `{"data":"${base64Data}"`;
expect(mightBeCompleteJson(json)).toBe(false);
});

it("returns true for large nested object", () => {
const base64Data = "A".repeat(LARGE_SIZE);
const json = `{"outer":{"inner":{"data":"${base64Data}"}}}`;
expect(mightBeCompleteJson(json)).toBe(true);
});

it("returns false for large nested object with unbalanced braces", () => {
const base64Data = "A".repeat(LARGE_SIZE);
// Extra opening brace
const json = `{"outer":{{"inner":{"data":"${base64Data}"}}}`;
expect(mightBeCompleteJson(json)).toBe(false);
});

it("handles large payload performance efficiently", () => {
// 5MB base64 data simulating a real image
const base64Data = "A".repeat(5 * 1024 * 1024);
const json = `{"candidates":[{"content":{"parts":[{"inlineData":{"mimeType":"image/png","data":"${base64Data}"}}]}}]}`;

const start = performance.now();
const result = mightBeCompleteJson(json);
const elapsed = performance.now() - start;

expect(result).toBe(true);
// Should complete in under 10ms for 5MB (vs 100ms+ without optimization)

Copilot AI Feb 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states "Should complete in under 10ms" but the assertion checks for less than 50ms. Update the comment to match the actual assertion, or consider tightening the assertion if 10ms is the expected performance target.

Suggested change
// Should complete in under 10ms for 5MB (vs 100ms+ without optimization)
// Should complete in under 50ms for 5MB (vs 100ms+ without optimization)

Copilot uses AI. Check for mistakes.
expect(elapsed).toBeLessThan(50);
Comment on lines +132 to +133

Copilot AI Feb 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance tests that measure elapsed time can be flaky on slower CI systems or under load. Consider making this test more resilient by either: (1) increasing the threshold further (e.g., to 100ms or 200ms), (2) running multiple iterations and checking the average/median, or (3) making this test optional/skippable in CI environments where timing is unreliable.

Suggested change
// Should complete in under 10ms for 5MB (vs 100ms+ without optimization)
expect(elapsed).toBeLessThan(50);
// Performance check: in non-CI environments, this should typically
// complete well under 200ms for a 5MB payload.
if (!process.env.CI) {
expect(elapsed).toBeLessThan(200);
}

Copilot uses AI. Check for mistakes.
});
Comment on lines +122 to +134

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Performance test: comment says "under 10ms" but assertion uses 50ms.

Line 132 comment says "Should complete in under 10ms" but line 133 asserts < 50. Minor inconsistency — either tighten the comment or the assertion. Also, be aware that time-based assertions can be flaky in resource-constrained CI environments.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/chat/tools/might-be-complete-json.spec.ts` around lines 122
- 134, Test comment and assertion mismatch: the comment in the
might-be-complete-json.spec.ts test for mightBeCompleteJson says "under 10ms"
but the assertion checks elapsed < 50ms; either update the comment to match the
50ms assertion or tighten the assertion to < 10ms. Preferably remove or relax
the strict timing assertion to a safer threshold (or mock performance.now() /
avoid wall-clock timing) to prevent flaky CI; update the inline comment to
reflect the chosen threshold and ensure the test still validates correctness of
mightBeCompleteJson rather than brittle timing.

});
});
134 changes: 134 additions & 0 deletions apps/gateway/src/chat/tools/might-be-complete-json.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
* Returns false if brackets are definitely unbalanced (avoiding expensive JSON.parse).
* Returns true if it might be valid (still needs JSON.parse to confirm).
* This is a performance optimization for SSE parsing where we do many validity checks.
*
* For large strings (e.g. base64 image data), we use an optimized approach:
* scan from both ends inward to find the structural boundaries, avoiding
* a full O(n) scan of multi-MB payloads.
*/
export function mightBeCompleteJson(str: string): boolean {
const trimmed = str.trim();
Expand All @@ -27,6 +31,20 @@ export function mightBeCompleteJson(str: string): boolean {
return false;
}

// For large payloads (e.g. containing base64 image data), scanning the entire
// string character-by-character is extremely expensive (O(n) on multi-MB data).
// Instead, we scan from the start and end inward, only examining the structural
// JSON boundaries. Large base64 strings in the middle are inside a JSON string
// value, so we only need to verify the outer structure is balanced.
//
// The threshold is set at 100KB - below this, the full scan is fast enough.
// Above this, payloads almost always contain large opaque string values
// (base64 images, long text) where scanning every character is wasteful.
const LARGE_PAYLOAD_THRESHOLD = 100 * 1024;
if (trimmed.length > LARGE_PAYLOAD_THRESHOLD) {
return mightBeCompleteJsonLarge(trimmed);
}

// Count brackets/braces, skipping content inside strings
let braces = 0;
let brackets = 0;
Expand Down Expand Up @@ -67,3 +85,119 @@ export function mightBeCompleteJson(str: string): boolean {

return braces === 0 && brackets === 0;
}

/**
* Optimized heuristic for large JSON payloads (100KB+).
*
* Instead of scanning the entire string, we scan from the start until we
* enter a string value that extends beyond our scan window, then scan backward
* from the end until we enter the same string. The structural depth counted
* from each end should match for the JSON to be balanced.
*
* This turns an O(n) scan into O(k) where k is the size of the structural
* JSON skeleton (typically a few hundred bytes even for multi-MB payloads).
*/
function mightBeCompleteJsonLarge(trimmed: string): boolean {
const SCAN_LIMIT = 8192; // scan at most 8KB from each end

// Forward scan: count structural depth until we enter a long string
let fBraces = 0;
let fBrackets = 0;
let inString = false;
let i = 0;
const forwardEnd = Math.min(trimmed.length, SCAN_LIMIT);

while (i < forwardEnd) {
const c = trimmed[i];
if (inString) {
if (c === "\\") {
i += 2;
continue;
} else if (c === '"') {
inString = false;
}
} else {
if (c === '"') {
inString = true;
} else if (c === "{") {
fBraces++;
} else if (c === "}") {
fBraces--;
} else if (c === "[") {
fBrackets++;
} else if (c === "]") {
fBrackets--;
}
}
i++;
}

// If we finished scanning the entire string (unlikely given >100KB), use result directly
if (i >= trimmed.length) {
return !inString && fBraces === 0 && fBrackets === 0;
}

// If we're NOT in a string at the forward scan limit, the structural content
// extends beyond our window in a non-string context. This is unusual for large
// payloads. Use a conservative approach: the first/last char check already passed.
if (!inString) {
return true;
}

// We entered a large string value in the forward scan.
// Now scan backward from the end. The reverse scan counts structural depth
// from the end until it enters a string going backward (which should be the
// same string the forward scan entered).
//
// For the JSON to be balanced:
// - Forward opened fBraces braces before the string
// - Reverse should see the same number of closing braces after the string
// - Same for brackets
let rBraces = 0;
let rBrackets = 0;
let rInString = false;
let j = trimmed.length - 1;
const reverseEnd = Math.max(0, trimmed.length - SCAN_LIMIT);

while (j >= reverseEnd) {
const c = trimmed[j];
if (rInString) {
// Inside a string scanning backward - check for unescaped quote
if (c === '"') {
let backslashes = 0;
let k = j - 1;
while (k >= 0 && trimmed[k] === "\\") {
backslashes++;
k--;
}
if (backslashes % 2 === 0) {
rInString = false;
}
}
// Once we enter the large string going backward, we've accounted for
// all the closing structure. Stop scanning.
if (rInString && j < trimmed.length - SCAN_LIMIT + 1) {
// We're deep in the string and past our scan limit - stop
break;
}
} else {
if (c === '"') {
rInString = true;
} else if (c === "}") {
rBraces++;
} else if (c === "{") {
rBraces--;
} else if (c === "]") {
rBrackets++;
} else if (c === "[") {
rBrackets--;
}
}
j--;
}

// Forward scan opened fBraces/fBrackets before entering the string.
// Reverse scan should have closed the same number after the string.
// rBraces counts '}' as +1 and '{' as -1, so for balance: fBraces === rBraces
return fBraces === rBraces && fBrackets === rBrackets;
}
23 changes: 0 additions & 23 deletions apps/gateway/src/chat/tools/transform-streaming-to-openai.ts
Original file line number Diff line number Diff line change
Expand Up @@ -637,17 +637,6 @@ export function transformStreamingToOpenai(
case "azure":
case "openai": {
if (data.type) {
// Log full OpenAI event data for debugging
logger.info("[OpenAI Streaming Debug]", {
eventType: data.type,
hasAnnotations: !!(data.annotations || data.part?.annotations),
annotationsCount: (data.annotations || data.part?.annotations || [])
.length,
hasDelta: !!data.delta,
deltaKeys: data.delta ? Object.keys(data.delta) : [],
fullData: JSON.stringify(data),
});

switch (data.type) {
case "response.created":
case "response.in_progress":
Expand Down Expand Up @@ -966,18 +955,6 @@ export function transformStreamingToOpenai(
break;
}
} else {
// Log standard OpenAI streaming format for debugging
logger.info("[OpenAI Standard Streaming Debug]", {
hasChoices: !!data.choices,
choicesLength: data.choices?.length || 0,
firstChoiceDeltaKeys: data.choices?.[0]?.delta
? Object.keys(data.choices[0].delta)
: [],
hasAnnotations: !!data.choices?.[0]?.delta?.annotations,
annotationsCount: data.choices?.[0]?.delta?.annotations?.length || 0,
fullData: JSON.stringify(data),
});

transformedData = transformOpenaiStreaming(data, usedModel);
}
break;
Expand Down
Loading