Skip to content

feat(std.http): SSE upgrade-in-place, and the retry: field - #1899

Merged
paul-hammant merged 1 commit into
mainfrom
feat/sse-upgrade-retry
Sep 4, 2026
Merged

feat(std.http): SSE upgrade-in-place, and the retry: field#1899
paul-hammant merged 1 commit into
mainfrom
feat/sse-upgrade-retry

Conversation

@paul-hammant

Copy link
Copy Markdown
Collaborator

Two additions to std.http, both requested by the Datastar SDK port and both confirmed against their reply before building rather than inferred from reading their code.

1. response_upgrade_sse(res)

server_sse registers a whole route as SSE, so the decision is made before the request is parsed. A handler that must answer 400 on a malformed body and only then stream cannot use it:

stream, derr = events_frames(doc)
if derr != "" { respond_error(res, 400, derr); return }   // still an ordinary response
sse, err = http.response_upgrade_sse(res)                  // NOW become a stream

The Datastar port worked around this by seizing the raw socket with response_accept_tunnel.

That workaround is silently plaintext-only

http_response_accept_tunnel returns NULL when conn->ssl || conn->pure_tls — a raw std.tcp socket cannot carry TLS framing. So every SSE endpoint built that way fails the moment it is served over HTTPS, with a runtime error string rather than anything a build or test would catch. They had not spotted it; their whole conformance matrix is plain HTTP.

This upgrade writes through conn_send, the connection's own send path, so it works over TLS. Verified end-to-end against a real HTTPS server with a self-signed cert, not asserted from reading the code.

The contract question they asked

They asked what happens when the handler has already touched res, since the 400-first flow means it may have. Answered explicitly in the doc comment and enforced:

  • headers set earlier are discarded — the SSE head is fixed, and a stale Content-Type/Content-Length would corrupt the stream;
  • a response that already has a body is refused — that body is data the caller believes it sent, and silently dropping it would lose it.

2. sse_send_full(sse, event, data, id, retry_ms)

retry: was emitted nowhere. It is SSE's own reconnection-backoff field, so a surface writing id:/event:/data: but not retry: is incomplete rather than merely minimal — that is the reason to add it. That 5 of the Datastar port's 20 cross-SDK conformance goldens require it is corroboration, not the argument.

sse_send and sse_send_id are unchanged, passing 0 to omit the field. Order on the wire is id, event, retry, dataretry: must precede the data lines, because the blank line dispatches the event and a field after the data would land in the next one.

What they asked me not to build

  • No compression coupling. They argued the negotiation strategy is SDK-specific and that a compressing upgrade call "would make the simple case easy and the Datastar case unreachable". Agreed — zlib.stream_* composes over the returned handle instead.
  • No client-side SSE reader. They said their conformance suite drives the real upstream Go runner and their one streaming test reads raw bytes off a socket, so nothing wants it.

Testing

The suite asserts the four behaviours that matter: a handler answers 400 without upgrading, the upgraded response carries the fixed SSE head, retry: appears before the data lines, and upgrading over a committed body is refused.

Verified on three platforms rather than CI alone:

build http_sse_upgrade http_server_sse (existing) unit
Linux PASS PASS 409/409
macOS 15.7.7 PASS PASS 409/409
Windows MSYS2 SKIP-WIN (by design) SKIP-WIN 397/397

The existing http_server_sse passing everywhere is the guard that the route-based path is unregressed.

Two pre-existing failures seen on those boxes, both confirmed on clean main and unrelated: http_server_h2 (HTTP/2 framing) and the Windows -lfyaml link error (#1896).

🤖 Generated with Claude Code

Both asked for by the Datastar SDK port, and both confirmed against their
reply before building rather than inferred from reading their code.

1. `response_upgrade_sse(res)` — turn an ORDINARY in-flight response into an
   SSE stream.

   `server_sse` registers a whole ROUTE as SSE, so the decision is made before
   the request is parsed. A handler that must answer 400 on a malformed body
   and only THEN stream cannot use it. The Datastar port worked around this by
   seizing the raw socket with `response_accept_tunnel` -- which returns NULL
   when `conn->ssl || conn->pure_tls`, so every SSE endpoint built that way is
   silently plaintext-only. They had not spotted it; their whole test matrix
   is HTTP.

   This upgrade writes through `conn_send`, the connection's own send path, so
   it works over TLS. Verified end-to-end against a real HTTPS server with a
   self-signed cert, not asserted from the code.

   Their contract question -- what happens when the handler has already
   touched `res`, since the 400-first flow means it may have -- is answered
   explicitly in the doc comment and enforced: headers set earlier are
   DISCARDED (the SSE head is fixed, and a stale Content-Length would corrupt
   the stream), and a response that already has a BODY is REFUSED, because
   that body is data the caller believes it sent.

2. `sse_send_full(sse, event, data, id, retry_ms)` — emit `retry:`.

   The field was emitted nowhere. It is SSE's own reconnection-backoff
   mechanism, not a Datastar invention, so a surface that writes id/event/data
   but not retry is incomplete rather than merely minimal -- which is the
   reason to add it; that 5 of their 20 cross-SDK conformance goldens require
   it is corroboration. `sse_send` and `sse_send_id` are unchanged, passing 0
   to omit it.

   Field order is id, event, retry, data. retry goes BEFORE the data lines
   because the blank line dispatches the event, so a field after the data
   would land in the next one.

They also asked that compression NOT be coupled to the upgrade call, arguing
the negotiation strategy is SDK-specific and a compressing upgrade "would make
the simple case easy and the Datastar case unreachable". Agreed, and none is
wired in -- `zlib.stream_*` composes over the returned handle instead. They
declined a client-side SSE reader, so there is none.

The test asserts the four behaviours that matter: a handler answers 400
WITHOUT upgrading, the upgraded response carries the fixed SSE head, `retry:`
appears before the data lines, and upgrading over a committed body is refused.

Verified on three platforms rather than CI alone: Linux and macOS 15.7.7 both
pass the new suite and the existing http_server_sse (so the route-based path
is unregressed); Windows builds clean and skips the suite by design, as
http_server_sse already does, with unit tests 397/397.

Pre-existing failures on these boxes, confirmed on clean main and unrelated:
http_server_h2 (HTTP/2 framing) and the Windows -lfyaml link error (#1896).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@paul-hammant
paul-hammant merged commit 82baae6 into main Sep 4, 2026
27 checks passed
@paul-hammant
paul-hammant deleted the feat/sse-upgrade-retry branch September 4, 2026 17:42
paul-hammant added a commit that referenced this pull request Sep 4, 2026
The three Linux legs failed with

  integration_tinyweb_tls_half   (TIMED OUT — hung; killed by per-test timeout)
  integration_tinyweb_tls_server (TIMED OUT — hung; killed by per-test timeout)

test-ae runs every .ae under tests/ STANDALONE, so both fixtures were executed
directly rather than only by their shell script -- and tw_start blocks forever,
so each hung until the sweep killed it. The sibling http_server_sse/ is pruned
for exactly this reason; tinyweb_tls/ needed the same entry and did not get it.

Worth noting the difference from the SSE fixture added in #1899, which is NOT
pruned and is nonetheless fine: it ends with sleep(60000); exit(0), so it
self-terminates. Verified that directly rather than assuming it shared this
bug -- it exits 0 when run standalone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
paul-hammant added a commit to paul-hammant/aether-fork that referenced this pull request Sep 5, 2026
tinyweb wraps std.http, whose server has had `server_set_tls` all along --
tinyweb simply never surfaced it, so every tinyweb app was plaintext-only for
no deeper reason than a missing setter.

    server = tinyweb.web_server_host("0.0.0.0", 443) { ... }
    _ = tinyweb.with_tls(server, "/etc/ssl/cert.pem", "/etc/ssl/key.pem")
    err = tinyweb.tw_start(server)

`with_tls` records the PEM pair on the existing config map; tw_start applies it
after bind and BEFORE registering routes, so a bad pair fails start-up rather
than after the server looks ready.

The design decision worth stating: a bad or HALF-configured pair is a start-up
ERROR, not a silent skip. A server that quietly downgrades to plaintext on a
port the caller believes is encrypted is worse than one that refuses to start,
so `with_tls(srv, cert, "")` returns "with_tls needs BOTH a cert and a key"
rather than serving HTTP. That is the same failure shape as the
`response_accept_tunnel` TLS gap found in aether-lang-dev#1899, where a silent plaintext
fallback had shipped unnoticed downstream.

New suite tests three things: a real TLS handshake serving the route body,
plain http to the TLS port NOT returning the body, and the half-configured
pair refusing to start. It is also the first tests/integration/tinyweb_* suite,
so contrib/tinyweb gains CI coverage it did not have.

Verified on Linux and macOS 15.7.7 (suite passes on both); Windows skips it by
design, as the other HTTP-server suites do, and still compiles the module.
The three existing in-tree tinyweb tests are unaffected. make test 409/409;
fmt gate and check-docs green.

NB the diff is deliberately additive-only (35 insertions, 0 deletions in
module.ae). `ae fmt contrib` reformats 23 unrelated contrib modules and most of
tinyweb's own examples; that noise is reverted here so the change is
reviewable, and the tree stays fmt-canonical either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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