Skip to content

fix(streaming): bound translated SSE inspection - #1114

Open
Ingwannu wants to merge 1 commit into
devfrom
fix/1112-translated-sse-bounds
Open

fix(streaming): bound translated SSE inspection#1114
Ingwannu wants to merge 1 commit into
devfrom
fix/1112-translated-sse-bounds

Conversation

@Ingwannu

@Ingwannu Ingwannu commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Summary

  • Reuse the existing byte-bounded SSE inspector for translated request-log tracking and the heartbeat relay.
  • Discard and resynchronize oversized inspection frames without changing any client-facing bytes.
  • Share one JSON parse across request-log, first-output, and terminal observers on translated streams.
  • Document the shared inspection invariant.

Closes #1112

Verification

  • nice -n 10 taskset -c 0,1 bun test tests/sse-inspector-bounds.test.ts tests/passthrough-abort.test.ts — 37 pass, 0 fail
  • nice -n 10 taskset -c 0,1 bun run typecheck — passed
  • nice -n 10 taskset -c 0,1 bun run privacy:scan — passed
  • nice -n 10 taskset -c 0,1 bun run test — passed on ef131787; the branch was then rebased onto a5b37827, whose only intervening change was an issue-quality workflow permission test, and the focused tests plus typecheck were rerun after the rebase

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes
    • Improved SSE stream handling for oversized frames, including reliable resynchronization.
    • Preserved upstream response bytes while tracking request logs and relaying heartbeats.
    • Ensured stream inspection completes cleanly during normal closure, errors, and cancellation.
    • Maintained terminal fallback reporting across stream outcomes.
  • Documentation
    • Documented consistent SSE handling across request-log tracking, first-output detection, and heartbeat relaying.
  • Tests
    • Added coverage for frame-size limits, byte preservation, and consistent parsing across observers.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Aug 6, 2026
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The translated SSE request-log tracker and heartbeat relay now reuse createSseInspector. Cleanup covers EOF, errors, and cancellation. Tests verify bounded frames, resynchronization, byte preservation, terminal callbacks, and single-pass parsing.

Changes

SSE inspection integration

Layer / File(s) Summary
Shared inspector wiring
src/server/relay.ts
trackSseForRequestLog and relaySseWithHeartbeat feed upstream chunks to createSseInspector, finish inspection at EOF, and dispose inspectors during cleanup. Existing terminal fallback reporting remains.
Wrapper validation and documentation
tests/sse-inspector-bounds.test.ts, structure/04_transports-and-sidecars.md
Wrapper tests cover oversized-frame discard, resynchronization, byte-preserving relay, terminal callbacks, overflow counters, single JSON parsing, and heartbeat bounds. Documentation describes the shared inspector behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: lidge-jun, wibias

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes in src/server/relay.ts and tests/sse-inspector-bounds.test.ts satisfy all coding objectives in issue #1112.
Out of Scope Changes check ✅ Passed The relay changes, regression tests, and documentation directly support the linked issue objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: applying bounded inspection to translated SSE streams.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/1112-translated-sse-bounds

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 73706d3b2c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/server/relay.ts
Comment on lines +381 to 382
inspector.finish();
if (!terminalReported) reportTerminal("incomplete");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve terminals for oversized completed frames

When a translated stream's terminal frame itself exceeds the 4 MiB inspection cap (the bridge allows much larger SSE events, and response.completed carries the full output/compaction item), createSseInspector discards that frame and never calls reportTerminal; on clean EOF this branch then records "incomplete" even though the client already received a successful terminal event. This corrupts request-log/account outcome data for large but valid translated responses, so terminal reporting needs to come from the bridge side or from a bounded prefix detector before the frame is discarded.

Useful? React with 👍 / 👎.

@lidge-jun

Copy link
Copy Markdown
Owner

Thank you for this — it is landing.

I have picked your commit into the stacked campaign as #1133 (targeting dev), with your authorship preserved: git log on that branch shows you as the commit author, not me. Nothing was rewritten; the fix is yours as written.

Reusing the existing byte-bounded createSseInspector rather than adding a third scanner was the right call — it keeps one definition of "how far do we look" and made the byte-preservation assertion straightforward.

Verified on the stack head: tests/sse-inspector-bounds.test.ts 23 pass / 0 fail, bun run typecheck clean, bun run privacy:scan green, and the full suite passed the repository prepush gate.

I am leaving this PR open — it is yours to close or continue as you prefer. Closing it is not something I want to do on your behalf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants