refactor(server): consolidate the SSE keepalive interval and its guard - #1133
Merged
Conversation
The 15-second SSE keepalive interval was declared three times, once per surface: `SSE_KEEPALIVE_INTERVAL_SECS` in `streaming.rs` and a file-private `KEEPALIVE_INTERVAL_SECS` in each of `streaming_responses.rs` and `streaming_anthropic.rs`. The compile-time assertion that gives the number its meaning, that the interval stays under the 60s reverse-proxy idle default, named only the first of the three, so `/v1/responses` and the Anthropic-compatible surface could be raised past a proxy timeout without the build noticing. The module docs at `streaming.rs` state the invariant as a property of SSE generally rather than of one route family, so the narrow coverage was drift, not intentional scoping. `streaming_responses.rs` and `streaming_anthropic.rs` now import the constant and their local copies are gone, leaving exactly one `15` literal for this interval in `src/server/`. The `const _: () = assert!(...)` moves from `streaming_tests.rs` to sit directly under the definition, so it covers every consumer by construction and needs no per-surface duplication. Verified by setting the constant to 61 and building: `error[E0080]: evaluation panicked: SSE keepalive interval must be less than the 60s default used by most reverse proxies`, pointing at `src/server/streaming.rs`, then reverted. The three newtypes stay separate on purpose. They are distinct types so a route cannot attach another surface's keepalive; only the constant and the assertion were duplicated. `router_front.rs` built its two SSE responses with `KeepAlive::default()`, which is behaviourally identical today because `KeepAlive::new()` sets a 15s interval under axum 0.7.9, but it defeats the design the newtype exists for: if the shared constant is ever lowered for proxy compatibility, those two streams would silently keep 15s. Both now go through `SseKeepAlive::default_for_long_prefill()`, and no `KeepAlive::default()` call site remains in `src/server/`. Behaviour is unchanged. Every value was already 15 and still is. Validated on GB10: `cargo test --profile test-fast --features cuda --lib server::streaming` is 25 passed and 0 failed, `cargo clippy --profile test-fast --features cuda --lib --tests -- -D warnings` is clean, and `cargo fmt --all -- --check` is clean. The acceptance criterion names `--features metal,accelerate`, which this Linux CUDA box cannot build; the feature set does not reach this code. Closes #1105
8 tasks
Review caught that this PR made the `SseKeepAlive` rustdoc wrong in the act of fixing the constants. It said the type is "constructed by `sse_channel`" and listed three consumers, but `router_front.rs` now constructs it directly at two sites and is a fourth consumer, since its streams do not come from `sse_channel` at all. The `Used by:` list is a convention this repo maintains across 20-odd rustdoc sites and deliberately tracks for staleness (see the report for PR #1120), so leaving it short would have been the exact defect that report was written about. The sibling list on `sse_channel` is untouched and still correct: `router_front` does not call it. Two pre-existing inaccuracies in the same file, folded in while the lines were open. The `sse_channel` doc told handlers to attach via `keep_alive(keepalive.0)`, but the field is private outside the module and all seven attach sites use `into_inner()`, which the struct doc five lines above already said correctly. The module-doc lead-in presented the keepalive as something only `sse_channel` hands out, which stopped being true for two of the seven sites. The report gains a paragraph on a second way the guard's reach widened, which was not obvious going in: `streaming_tests.rs` is included under `#[cfg(test)]`, so the assertion was const-evaluated only in test builds. As a module-level `const _` in `streaming.rs` it is evaluated in every build, which is why the 61 experiment reproduces under a plain `cargo check --lib`. Three surfaces instead of one, and all profiles instead of test-only. No code changed. Validated on GB10: `cargo test --profile test-fast --features cuda --lib server::streaming` is 25 passed and 0 failed, clippy with `-D warnings` is clean, and `cargo fmt --all -- --check` is clean. Refs #1105
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The 15-second SSE keepalive interval was declared three times, once per surface, and the compile-time assertion that gives the number its meaning guarded only one of them.
/v1/responsesand the Anthropic-compatible surface could therefore be raised past a reverse-proxy idle timeout without the build failing. This consolidates the constant and moves the guard next to it, so it covers every consumer by construction.What changed
src/server/streaming.rs:SSE_KEEPALIVE_INTERVAL_SECSis now documented as the single definition for every SSE surface, and theconst _: () = assert!(SSE_KEEPALIVE_INTERVAL_SECS < 60, ...)moved here fromstreaming_tests.rsto sit directly under it. The module docs no longer restate15; they point at the constant and record that the invariant now covers all surfaces.src/server/streaming_responses.rs,src/server/streaming_anthropic.rs: localconst KEEPALIVE_INTERVAL_SECS: u64 = 15;deleted; both import the shared constant. The two newtypes are untouched and stay distinct, which is the property refactor(server): consolidate the three duplicated SSE keepalive constants under one proxy-timeout assertion #1105 asks to keep: a route cannot attach another surface's keepalive, because the type it receives comes from the channel constructor it called. Only the constant and the assertion were duplicated.src/server/streaming_tests.rs: the assertion and its now-unused import are removed, replaced by a comment recording where the invariant lives and why it moved.src/server/router_front.rs: bothKeepAlive::default()sites now build throughSseKeepAlive::default_for_long_prefill(). Behaviourally identical today (KeepAlive::new()is 15s under axum 0.7.9), but it restores the design the newtype exists for: if the shared constant is ever lowered for proxy compatibility, these two streams would otherwise silently keep 15s.Behaviour is unchanged. Every value was already 15 and still is.
Acceptance criteria
15literal for the SSE keepalive interval insrc/server/grep -rn "KEEPALIVE_INTERVAL_SECS.*=" src/server/returns one line,streaming.rs61fails the build, attributable to the shared assertionKeepAlive::default()call sites remain insrc/server/grep -rn "KeepAlive::default()" src/returns four lines, all comment text in the two explanatory blocks left at the former call sites, and zero codeserver::streamingtests greenThe
61check, run againstcargo check --profile test-fast --features cuda --liband then reverted:Before this change the same edit compiled cleanly for the Responses and Anthropic surfaces, because their copies were never named by any assertion.
Test plan
cargo test --profile test-fast --features cuda --lib server::streaming: 25 passed, 0 failedcargo clippy --profile test-fast --features cuda --lib --tests -- -D warningscargo fmt --all -- --check61build-failure check above, reverted afterwardsThe acceptance criterion names
--features metal,accelerate. This is a Linux CUDA box and cannot build that feature set; the feature selection does not reach any of this code, which is backend-agnostic SSE plumbing. NoCHANGELOG.mdentry: nothing observable changes for a user.Closes #1105
Review round
A review pass confirmed the refactor is behaviour-neutral against the vendored axum 0.7.9 source:
KeepAlive::default()isKeepAlive::new(), which setsmax_interval: Duration::from_secs(15)and the:\n\ncomment event, soSseKeepAlive::default_for_long_prefill()overwritesmax_intervalwith the same value and leaves the event alone. The tworouter_frontsites emit the same frame on the same schedule.It also found the guard's reach widened on a second axis that was not obvious going in.
streaming_tests.rsis included under#[cfg(test)], so the assertion was const-evaluated only in test builds. As a module-levelconst _instreaming.rsit is evaluated in every build, which is why the61check above reproduces under a plaincargo check --lib. Three surfaces instead of one, and all profiles instead of test-only.One finding was that this PR made the
SseKeepAliverustdoc stale in the act of fixing the constants: it said the type is "constructed bysse_channel" and listed three consumers, whilerouter_front.rsnow constructs it directly and is a fourth. Fixed, along with two pre-existing inaccuracies in the same file (thesse_channeldoc pointed atkeepalive.0, which is private outside the module, and the module-doc lead-in presented the keepalive as something onlysse_channelhands out).