Skip to content

fix(sse,h2): SSE API hardening (#266-#270) and h2 teardown close-out (#230-#232) - #350

Merged
cryo2010 merged 7 commits into
mainfrom
fix/sse-hardening-and-h2-closeout
Sep 29, 2026
Merged

fix(sse,h2): SSE API hardening (#266-#270) and h2 teardown close-out (#230-#232)#350
cryo2010 merged 7 commits into
mainfrom
fix/sse-hardening-and-h2-closeout

Conversation

@cryo2010

@cryo2010 cryo2010 commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Fixes the five SSE issues from the API review, one commit per issue, and closes out the three HTTP/2 teardown issues that PR #242 fixed but never closed (its body did not use per-issue closing keywords), with the regression coverage that was missing.

Fixes #266
Fixes #267
Fixes #268
Fixes #269
Fixes #270
Fixes #230
Fixes #231
Fixes #232

SSE (src/vortex/request.nim)

  • SSE: send("") (empty-data event) dispatches nothing on the client #266 s.send("") now dispatches on the client. An empty data is emitted as two empty data: fields: the WHATWG EventSource algorithm appends an LF per field and strips one trailing LF before its "empty buffer, do not dispatch" check, so one field never fires a listener and two leave "\n". Documented on send, in the README and the changelog.
  • SSE: literal CR / CRLF inside data is silently lost #267 send splits on the three terminators the SSE grammar defines (CRLF, LF, CR) explicitly instead of splitLines; behaviour is byte-identical and the inherent CR lossiness is now documented with the base64/JSON recommendation. Wire bytes for CRLF, bare CR, LF and a trailing break are pinned by tests.
  • SSE: alive/bufferedAmount read loop state with no thread guard (race vs mutators) #268 req.isAlive, res.bufferedAmount and the SseStream alive/bufferedAmount that delegate to them take the same loop-thread guard every mutating call takes (false / 0 off-thread). A blocking: worker still sees isAlive == true through the pin.
  • SSE: withSse cannot pass retry or headers (silently drops them) #269 res.withSse forwards headers and retry to res.sse: res.withSse(s, retry = 3000): .... It is now a macro over varargs[untyped] because Nim binds a trailing block to the last parameter positionally, so a template with defaulted params handed the block to headers. The bare res.withSse(s): ... form is unchanged; positional or unknown args are compile errors naming the accepted ones.
  • SSE: off-thread send/comment return false indistinguishably from backpressure #270 s.send / s.comment assert on off-thread use (the repo's existing res.headers convention, elided under --assertions:off / -d:danger). res.write, res.onDrain and the SSE wrappers document that false also means an unsupported off-thread call, not only backpressure. Response.write itself keeps its silent no-op: turning a hot core primitive into a Defect is a bigger blast radius than the issue needs.

HTTP/2 teardown (#230, #231, #232)

All three were fixed in #242 (commit 24d7fd7). This PR re-audited every path each issue lists against current main (all streams.del sites, all 28 streamError callers, the connDeferred increment/decrement pairing, every h2Stream(...) / template st holder across table-mutating calls) and found no residual gap, so there is no production change. It adds tests/test_http2_teardown.nim:

Follow-ups found during the audit (not changed here)

  1. onBody(last=true) is delivered twice on a normal streaming-upload completion when the handler responds from inside the callback: h2DeliverBody never clears st.rs.onBodyCb, so h2Respond -> teardownStream fires it again. Harmless for the shipped adapters (markEof is idempotent), visible to a user callback that counts completions. One-line fix: nil the callback before invoking it with last.
  2. h2WsTeardownAll iterates h2.streams.mpairs across the user onClose, which can tear down another stream and invalidate the iterator (same class as fix(h2): use-after-move via ptr H2Stream held across streams-table mutation in finish() #230). Fix mirrors h2NotifyClosed: snapshot, nil, then fire.
  3. h2WsFinalize is the only stream removal that bypasses teardownStream. Benign today, a standing drift risk.
  4. tests/h2client.nim sendRaw uses std/net.send, which re-sends from offset 0 after a partial write and spins at 100% CPU once the peer is gone; the new suite carries a local sendAll on posix.send that should move into the helper.

Validation

Stress smoke

Two complete passes of VORTEX_PROTO=all VORTEX_SERVER=all VORTEX_SECONDS=10 VORTEX_REPORT_SECONDS=2 (5 workloads x h1/h2/h3 x sync/async/async-await/chronos/chronos-await, 75 cells, canary plus chaos sidecar, 64 MiB streams), both 75/75 with every checksum verified and no failure signature in any server log:

  1. Sequential, loaded host: nimble stress, run while eight unrelated yes > /dev/null containers held the host at load average ~23. 65 minutes.
  2. Parallel, quiet host: the five workloads launched concurrently with distinct VORTEX_RUN_IDs after the burners were removed (load average ~3 before start). 14 minutes. Five cells share the host at any moment, so this run has its own contention.

Throughput per workload and protocol, mean over the five server runtimes:

workload proto loaded host (mean of 5 servers) parallel quiet (mean) delta
requests h1 2902 req/s 2791 req/s -4%
requests h2 2218 req/s 2002 req/s -10%
requests h3 982 req/s 913 req/s -7%
ws h1 51221 msg/s 52115 msg/s +2%
ws h2 48473 msg/s 50800 msg/s +5%
ws h3 8817 msg/s 8252 msg/s -6%
sse h1 45414 ev/s 39734 ev/s -13%
sse h2 33705 ev/s 30244 ev/s -10%
sse h3 37884 ev/s 30156 ev/s -20%
streamupload h1 1095 MB/s 1081 MB/s -1%
streamupload h2 419 MB/s 395 MB/s -6%
streamupload h3 173 MB/s 140 MB/s -19%
streamdownload h1 968 MB/s 889 MB/s -8%
streamdownload h2 248 MB/s 243 MB/s -2%
streamdownload h3 41 MB/s 32 MB/s -22%

A cleaner reference from the eight cells of a quiet sequential run that was stopped to make room for the parallel one: requests h1 averaged 3409 req/s (vs 2902 loaded, 2791 parallel) and requests h2 averaged 2564 req/s (vs 2218 loaded, 2002 parallel). So the parallel run's contention costs roughly what the burners did, about 10 to 20 percent on the CPU-bound cells and most on h3, where QUIC crypto and the aioquic client compete hardest. None of the deltas is a regression signal: both runs are the same commit, and the ordering across protocols and runtimes is stable. Peak server RSS was 325 MB (loaded) and 315 MB (parallel), both on streamupload h1 sync.

Per-cell results (75 rows): ok count, throughput and RSS for both runs
workload proto server ok (loaded / parallel) loaded host parallel quiet delta RSS loaded / parallel
requests h1 sync 30504 / 25596 2852 req/s 2539 req/s -11% 258 / 257 MB
requests h1 async 28149 / 24798 2571 req/s 2838 req/s +10% 148 / 146 MB
requests h1 async-await 28461 / 27957 3030 req/s 2854 req/s -6% 146 / 150 MB
requests h1 chronos 30483 / 27933 3009 req/s 2889 req/s -4% 148 / 144 MB
requests h1 chronos-await 28122 / 27078 3047 req/s 2836 req/s -7% 148 / 147 MB
requests h2 sync 22176 / 18432 2172 req/s 1902 req/s -12% 229 / 237 MB
requests h2 async 22482 / 18069 2202 req/s 1967 req/s -11% 93 / 98 MB
requests h2 async-await 22620 / 19569 2172 req/s 2118 req/s -2% 92 / 95 MB
requests h2 chronos 22920 / 20598 2225 req/s 1852 req/s -17% 91 / 90 MB
requests h2 chronos-await 22761 / 20238 2317 req/s 2171 req/s -6% 93 / 93 MB
requests h3 sync 22089 / 17589 988 req/s 692 req/s -30% 218 / 208 MB
requests h3 async 22155 / 20112 897 req/s 1468 req/s +64% 114 / 112 MB
requests h3 async-await 22200 / 20436 965 req/s 1211 req/s +25% 112 / 107 MB
requests h3 chronos 22185 / 20436 1028 req/s 1196 req/s +16% 112 / 110 MB
requests h3 chronos-await 22149 / 20433 1030 req/s 0 req/s -100% 108 / 109 MB
ws h1 sync 543274 / 478644 52301 msg/s 45525 msg/s -13% 178 / 160 MB
ws h1 async 553900 / 492372 50294 msg/s 54173 msg/s +8% 45 / 43 MB
ws h1 async-await 532333 / 557286 49811 msg/s 58783 msg/s +18% 48 / 45 MB
ws h1 chronos 562093 / 554278 50800 msg/s 55230 msg/s +9% 40 / 47 MB
ws h1 chronos-await 559533 / 524042 52897 msg/s 46866 msg/s -11% 41 / 45 MB
ws h2 sync 507490 / 414800 49156 msg/s 41999 msg/s -15% 220 / 159 MB
ws h2 async 494181 / 446517 44964 msg/s 50550 msg/s +12% 79 / 77 MB
ws h2 async-await 493524 / 488535 45229 msg/s 52651 msg/s +16% 79 / 75 MB
ws h2 chronos 540075 / 521257 51037 msg/s 57507 msg/s +13% 69 / 74 MB
ws h2 chronos-await 519430 / 488781 51977 msg/s 51295 msg/s -1% 77 / 75 MB
ws h3 sync 106505 / 100447 8415 msg/s 9166 msg/s +9% 163 / 163 MB
ws h3 async 107607 / 85266 8808 msg/s 7343 msg/s -17% 61 / 62 MB
ws h3 async-await 114796 / 90455 9098 msg/s 7614 msg/s -16% 62 / 61 MB
ws h3 chronos 106056 / 100549 8929 msg/s 8042 msg/s -10% 60 / 60 MB
ws h3 chronos-await 104968 / 108354 8833 msg/s 9097 msg/s +3% 60 / 62 MB
sse h1 sync 451200 / 384000 45414 ev/s 38476 ev/s -15% 201 / 197 MB
sse h1 async 451200 / 365700 45009 ev/s 39773 ev/s -12% 63 / 63 MB
sse h1 async-await 451200 / 412800 46256 ev/s 42611 ev/s -8% 65 / 63 MB
sse h1 chronos 451200 / 412800 45388 ev/s 40770 ev/s -10% 63 / 63 MB
sse h1 chronos-await 451200 / 403200 45005 ev/s 37038 ev/s -18% 63 / 63 MB
sse h2 sync 345600 / 297600 34126 ev/s 29630 ev/s -13% 220 / 244 MB
sse h2 async 326400 / 259200 31962 ev/s 27894 ev/s -13% 79 / 75 MB
sse h2 async-await 336100 / 301200 33827 ev/s 31515 ev/s -7% 75 / 75 MB
sse h2 chronos 326400 / 316800 33179 ev/s 31474 ev/s -5% 73 / 76 MB
sse h2 chronos-await 345600 / 297600 35433 ev/s 30708 ev/s -13% 70 / 74 MB
sse h3 sync 698200 / 551900 37842 ev/s 21420 ev/s -43% 154 / 158 MB
sse h3 async 695300 / 599800 39762 ev/s 33261 ev/s -16% 60 / 60 MB
sse h3 async-await 720600 / 634100 43160 ev/s 31361 ev/s -27% 60 / 61 MB
sse h3 chronos 697100 / 642300 35787 ev/s 30869 ev/s -14% 60 / 60 MB
sse h3 chronos-await 686200 / 633900 32867 ev/s 33869 ev/s +3% 60 / 60 MB
streamupload h1 sync 291 / 331 1862 MB/s 1901 MB/s +2% 325 / 307 MB
streamupload h1 async 260 / 287 631 MB/s 882 MB/s +40% 269 / 300 MB
streamupload h1 async-await 258 / 299 621 MB/s 846 MB/s +36% 284 / 290 MB
streamupload h1 chronos 267 / 312 724 MB/s 787 MB/s +9% 285 / 312 MB
streamupload h1 chronos-await 253 / 310 1636 MB/s 990 MB/s -39% 294 / 315 MB
streamupload h2 sync 69 / 63 424 MB/s 422 MB/s -0% 233 / 211 MB
streamupload h2 async 69 / 63 402 MB/s 398 MB/s -1% 81 / 79 MB
streamupload h2 async-await 69 / 57 426 MB/s 384 MB/s -10% 72 / 73 MB
streamupload h2 chronos 71 / 58 450 MB/s 460 MB/s +2% 75 / 81 MB
streamupload h2 chronos-await 69 / 51 394 MB/s 313 MB/s -21% 75 / 76 MB
streamupload h3 sync 11 / 12 293 MB/s 128 MB/s -56% 161 / 161 MB
streamupload h3 async 12 / 10 152 MB/s 122 MB/s -20% 60 / 61 MB
streamupload h3 async-await 11 / 9 170 MB/s 165 MB/s -3% 61 / 61 MB
streamupload h3 chronos 10 / 11 90 MB/s 146 MB/s +62% 60 / 61 MB
streamupload h3 chronos-await 11 / 10 158 MB/s 139 MB/s -12% 60 / 60 MB
streamdownload h1 sync 162 / 156 1125 MB/s 924 MB/s -18% 204 / 203 MB
streamdownload h1 async 153 / 132 971 MB/s 867 MB/s -11% 46 / 45 MB
streamdownload h1 async-await 148 / 147 971 MB/s 662 MB/s -32% 46 / 45 MB
streamdownload h1 chronos 147 / 153 841 MB/s 1046 MB/s +24% 46 / 50 MB
streamdownload h1 chronos-await 147 / 147 930 MB/s 945 MB/s +2% 48 / 44 MB
streamdownload h2 sync 42 / 39 260 MB/s 259 MB/s -0% 229 / 244 MB
streamdownload h2 async 42 / 42 252 MB/s 262 MB/s +4% 77 / 78 MB
streamdownload h2 async-await 42 / 39 255 MB/s 230 MB/s -10% 77 / 79 MB
streamdownload h2 chronos 42 / 39 233 MB/s 254 MB/s +9% 70 / 77 MB
streamdownload h2 chronos-await 39 / 33 242 MB/s 210 MB/s -13% 74 / 76 MB
streamdownload h3 sync 9 / 9 40 MB/s 34 MB/s -15% 153 / 163 MB
streamdownload h3 async 9 / 9 41 MB/s 40 MB/s -2% 60 / 60 MB
streamdownload h3 async-await 9 / 6 40 MB/s 11 MB/s -72% 60 / 61 MB
streamdownload h3 chronos 9 / 6 41 MB/s 38 MB/s -7% 62 / 60 MB
streamdownload h3 chronos-await 9 / 9 41 MB/s 35 MB/s -15% 60 / 62 MB

@cryo2010
fix(sse): dispatch a payload-free event by emitting two data: fields
8b291d4 
`s.send("")` wrote a single `data:` field. A client appends an LF to its
data buffer per `data:` field and strips one trailing LF before dispatch,
so the buffer was empty at the dispatch step, where the WHATWG EventSource
algorithm resets the buffers and returns without firing any listener. A
payload-free notification (`s.send("", event = "ping")`, a reload poke, a
typed signal whose meaning is the event name) therefore did nothing on the
client, with nothing on the wire or in the server to show why.

Emit two empty `data:` fields for an empty `data`, the workaround the wire
format leaves: the client reconstructs "\n", which is non-empty after the
trailing-LF strip, so the event dispatches and the listener runs with
`event.data == "\n"`. Documented on `send`, in the README and the changelog,
since the payload is "\n" rather than "".

Fixes #266
@cryo2010
fix(sse): split data on the wire format's own line terminators, and s…
af2481f 
…ay so

`send` split `data` with `splitLines`, so which byte sequences end a
`data:` field was a property of a text helper rather than of the protocol.
It happens to match the SSE grammar (CRLF, LF, CR), but nothing recorded
that, and the consequence was undocumented: the format has no escape for a
literal CR, so a CR inside `data` is a field boundary and the client
rebuilds it as an LF. `send("a\r\nb")` arrives as "a\nb", which silently
corrupts a payload a caller expected to be byte exact.

Spell the split out inline with a comment naming the three terminators, so
the wire contract is readable at the point it is produced, and document the
lossiness on `send` and in the README with the fix for it: encode the
payload (base64, or JSON, which escapes a CR). No behaviour change; the new
test pins the wire bytes for CRLF, bare CR, LF and a trailing break.

Fixes #267
@cryo2010
fix(sse,streaming): guard the loop-owned reads behind the thread check
912a72c 
`req.isAlive` and `res.bufferedAmount`, and the `SseStream.alive` /
`SseStream.bufferedAmount` that delegate to them, read state the loop
thread owns: the connection table, the h2/h3 stream maps and the response
write buffers. Every mutating call on those handles (write, sendHead,
finish, onDrain) already early-returns when it is not on the loop thread,
but these two read it unsynchronised, so a producer polling `s.alive` from
a worker raced the loop over a table the loop can resize and a generation
counter it can bump, and could act on a torn answer.

Add the same guard, with the same conservative answers the WebSocket
handles already give: off-thread `isAlive` is false and `bufferedAmount` is
0. A `blocking:` worker is unaffected, it returns true from the `snap`
check above the guard, since its connection is pinned for the body. The
private `currentThreadId` helper moves up in the file so `isAlive` can see
it.

Fixes #268
@cryo2010
feat(sse): forward res.sse's arguments through withSse
a841d2c 
`withSse` hardcoded `res.sse()`, so the block form could not set the
reconnect delay or add response headers: wanting either meant dropping to
the handle constructor and hand-writing the close/abort pairing the block
form exists to provide, and the two `res.sse` arguments were invisible from
the call site.

`res.withSse(s, retry = 3000): ...` and
`res.withSse(s, headers = [...], retry = 3000): ...` now work, and the
existing `res.withSse(s): ...` is unchanged. It becomes a macro over
varargs, because Nim binds a trailing block to the last parameter
positionally: with `headers`/`retry` as defaulted template parameters the
bare form hands the block to `headers` and fails to compile. The macro
resolves the named arguments and expands to a private template, so `sse`,
`close` and `abort` still bind at definition scope, and an unknown or
positional argument is a compile error that names the accepted ones.

Fixes #269
@cryo2010
fix(sse): assert on an off-thread send, and document what false means
628b5f8 
`s.send` and `s.comment` returned false from off the loop thread, the same
value they return for a full write backlog. A producer's contract is "false
means pause and wait for onDrain", so a worker pushing events saw a silent
no-op it read as backpressure, then waited for a drain callback that an
off-thread `onDrain` never registered either: events vanished with nothing
logged and no way to tell the case apart from a slow client.

Assert the loop thread in both, following `res.headers` / `res.trailers`
(plain assert, so it is compiled out under `--assertions:off` / `-d:danger`
and the call degrades to the no-op it was). The buffered `res.send` still
marshals from a `blocking:` worker, and `req.isAlive` still answers there
through the pin, so nothing legitimate is caught; the streaming primitives
have always been loop-thread only. `res.write`, `res.onDrain` and the SSE
wrappers keep their silent returns, since a hot core primitive should not
become a Defect, but now document that false covers a dead connection and
an off-thread call as well as backpressure.

Fixes #270
@cryo2010
test(h2): cover deferred connection credit reclaimed on stream teardown
917580e 
Regression coverage for #231, fixed in #242.

A streaming-upload route debits the connection receive window on receipt and
defers the WINDOW_UPDATE to consumption, so a manualAck sink that never acks
leaves every byte owed. The new suite uploads 48 KiB on two streams, cancels
both with RST_STREAM, and asserts the server credits the connection window
back and still accepts a further upload that only fits because of that credit.
Without the reclaim the connection window drains and the next upload is a
FLOW_CONTROL_ERROR, which is the deadlock the issue describes.
@cryo2010
test(h2): cover parked handler callbacks fired on stream teardown
564cac0 
Regression coverage for #232, fixed in #242.

Three cases the pre-#242 teardown paths stranded: a peer RST_STREAM must
deliver onBody(last=true) to a streaming request sink, a peer RST_STREAM must
fire a response producer parked on its onDrain, and a client disconnect must
do both for every stream still open. Each one leaked a suspended handler
coroutine (and whatever it pinned) on an ordinary client cancel.
@cryo2010
cryo2010 merged commit 0f048ab into main Sep 29, 2026
35 checks passed
@cryo2010
cryo2010 deleted the fix/sse-hardening-and-h2-closeout branch September 29, 2026 04:55
This was referenced Sep 30, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment