Skip to content

fix(responses): harden sparse SSE tool streams#175

Merged
AnthonyRonning merged 2 commits intomasterfrom
fix/responses-sse-keepalive
Apr 22, 2026
Merged

fix(responses): harden sparse SSE tool streams#175
AnthonyRonning merged 2 commits intomasterfrom
fix/responses-sse-keepalive

Conversation

@AnthonyRonning
Copy link
Copy Markdown
Contributor

@AnthonyRonning AnthonyRonning commented Apr 22, 2026

Summary

  • add a Responses SSE wrapper that sets anti-buffering headers and emits 1s keepalive heartbeats
  • keep pending tool-call events flowing during sparse idle gaps instead of batching them until tool completion
  • refresh dev and prod PCR measurements for the resulting enclave build

Validation

  • cargo fmt --check
  • cargo test
  • just update-pcr-all
  • just verify-pcr-history dev
  • just verify-pcr-history prod

Open in Devin Review

Summary by CodeRabbit

  • New Features

    • Server-sent events streams now include automatic keep-alive signals sent every second to maintain active connections.
  • Improvements

    • Enhanced caching and buffering control headers for streaming responses.
  • Chores

    • Updated PCR development and production history records with new entries containing updated hash values and timestamps.

AnthonyRonning and others added 2 commits April 22, 2026 12:57
Match the agent SSE buffering protections so pending tool calls flush through idle gaps instead of arriving only after tool completion.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Refresh the signed enclave measurements for the master-based Responses SSE keepalive build before opening the PR.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

Walkthrough

The PR appends new PCR history records to both development and production JSON history files, and refactors the SSE response handler to return a typed Response with keepalive heartbeat configuration and HTTP cache/buffering headers.

Changes

Cohort / File(s) Summary
PCR History Data
pcrDevHistory.json, pcrProdHistory.json
Appended new record entries with updated PCR hash values, new timestamps (1776880840 for dev, 1776880894 for prod), and corresponding signatures; no existing records modified.
SSE Response Handler
src/web/responses/handlers.rs
Refactored create_response_stream() to return Response instead of Sse<...>, wrapping the SSE stream with a new helper that configures keepalive heartbeat (1-second interval with "keep-alive" text) and sets HTTP headers (Cache-Control: no-cache, x-accel-buffering: no); updated imports to support KeepAlive, IntoResponse, and header types.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Possibly related PRs

Poem

🐰 The keepalive drum beats strong and true,
A heartbeat every second, fresh and new,
Headers whisper "no cache" with gentle care,
While PCR records bloom in data's air,
The stream flows on, alive and fair! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly reflects the main change: adding SSE keepalive and anti-buffering headers to harden sparse SSE tool streams in the responses module.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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/responses-sse-keepalive

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

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/web/responses/handlers.rs (1)

2943-2948: Optional: strengthen cache directive for intermediaries.

Consider including no-transform in Cache-Control to further reduce proxy-side response transformations on streaming traffic.

Suggested tweak
-            (header::CACHE_CONTROL, HeaderValue::from_static("no-cache")),
+            (
+                header::CACHE_CONTROL,
+                HeaderValue::from_static("no-cache, no-transform"),
+            ),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/web/responses/handlers.rs` around lines 2943 - 2948, The Cache-Control
header currently uses HeaderValue::from_static("no-cache"); update the header
value to include no-transform (e.g. "no-cache, no-transform") so intermediaries
won't alter streaming responses; change the value where header::CACHE_CONTROL is
set (the HeaderValue::from_static("no-cache") call) and keep the existing
x-accel-buffering header unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/web/responses/handlers.rs`:
- Around line 2943-2948: The Cache-Control header currently uses
HeaderValue::from_static("no-cache"); update the header value to include
no-transform (e.g. "no-cache, no-transform") so intermediaries won't alter
streaming responses; change the value where header::CACHE_CONTROL is set (the
HeaderValue::from_static("no-cache") call) and keep the existing
x-accel-buffering header unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: bcc363a1-d9be-44a3-a815-b2c0ae61a37a

📥 Commits

Reviewing files that changed from the base of the PR and between 7121385 and 4e5c5b5.

⛔ Files ignored due to path filters (2)
  • pcrDev.json is excluded by !pcrDev.json
  • pcrProd.json is excluded by !pcrProd.json
📒 Files selected for processing (3)
  • pcrDevHistory.json
  • pcrProdHistory.json
  • src/web/responses/handlers.rs

Comment on lines +2932 to 2953
fn responses_sse_response<S>(event_stream: S) -> Response
where
S: Stream<Item = Result<Event, Infallible>> + Send + 'static,
{
let sse = Sse::new(event_stream).keep_alive(
KeepAlive::new()
.interval(Duration::from_secs(RESPONSES_SSE_KEEPALIVE_INTERVAL_SECS))
.text("keep-alive"),
);

(
[
(header::CACHE_CONTROL, HeaderValue::from_static("no-cache")),
(
HeaderName::from_static("x-accel-buffering"),
HeaderValue::from_static("no"),
),
],
sse,
)
.into_response()
}
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.

🚩 Other SSE endpoint lacks keepalive and proxy headers

The chat completions SSE endpoint at src/web/openai.rs:380 uses Sse::new(stream).into_response() without any keepalive or x-accel-buffering: no header. If the motivation for this PR is that sparse SSE streams get dropped by proxies/load balancers, the same issue could affect the other SSE endpoint. Consider whether the responses_sse_response helper should be reused there as well.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@AnthonyRonning AnthonyRonning merged commit 7bb9121 into master Apr 22, 2026
11 checks passed
@AnthonyRonning AnthonyRonning deleted the fix/responses-sse-keepalive branch April 22, 2026 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant