Skip to content

Conversation

@dkorittki
Copy link
Contributor

@dkorittki dkorittki commented Nov 28, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling and propagation in GraphQL subscriptions over Server-Sent Events (SSE)
    • Enhanced response flushing for HTTP response writers to ensure proper stream completion
  • Tests

    • Added test coverage for error scenarios in GraphQL subscriptions

✏️ Tip: You can customize this high-level summary in your review settings.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
  • I have read the Contributors Guide.

When we write errors to a connection backed by HttpFlushWriter (used on subscriptions via SSE/Multipart) we do not flush the connection as oposed to a websocket connection, where this actually happens. I fixed it by simply checking if we are dealing with a flush writer and flush if so. Added a test to back this up.

@coderabbitai
Copy link

coderabbitai bot commented Nov 28, 2025

Walkthrough

This pull request adds error handling improvements to SSE subscription tests and enhances response flushing in the GraphQL handler. Changes include repositioning response body cleanup in existing tests, adding a new test case for SSE error propagation with subgraph 403 Forbidden responses, and introducing HTTP flush logic in the error writing function.

Changes

Cohort / File(s) Summary
SSE Test Updates
router-tests/http_subscriptions_test.go
Refactored response body close operation placement in SSE test. Added new subtest verifying error propagation when subgraph returns 403 Forbidden, asserting correct SSE event sequence including error data with status code.
GraphQL Handler Error Flushing
router/core/graphql_handler.go
Added conditional HTTP flush operation for HttpFlushWriter type in WriteError method, ensuring HTTP responses are flushed after websocket response writer flush.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • New SSE error test requires verification that the event sequence assertions (event type, error message content, status code inclusion) correctly reflect expected behavior
  • HTTP flush conditional logic should be reviewed to confirm proper timing and interaction with websocket flushing path
  • Test assertions depend on correct understanding of SSE protocol and error response formatting

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(router): flush errors on sse streams' directly describes the main change: adding flush logic for errors on SSE (Server-Sent Events) streams, which aligns with the PR's core objective of fixing error flushing behavior.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings

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 and usage tips.

@github-actions
Copy link

github-actions bot commented Nov 28, 2025

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-8a6e7008b1fd0ff3e5511797ff2b230e0c805a20 
@codecov
Copy link

codecov bot commented Nov 28, 2025

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@91471d3). Learn more about missing BASE report.

Files with missing lines Patch % Lines
router/core/graphql_handler.go 0.00% 2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@ Coverage Diff @@ ## main #2377 +/- ## ======================================= Coverage ? 37.58% ======================================= Files ? 209 Lines ? 22591 Branches ? 0 ======================================= Hits ? 8491 Misses ? 13076 Partials ? 1024 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 participants