Skip to content

A deadline for the coroutine receive, contract-wide (#130) - #198

Merged
aaylward merged 11 commits into
mainfrom
claude/smithy-cpp-dedup-issues-hy5key
Aug 26, 2026
Merged

A deadline for the coroutine receive, contract-wide (#130)#198
aaylward merged 11 commits into
mainfrom
claude/smithy-cpp-dedup-issues-hy5key

Conversation

@aaylward

Copy link
Copy Markdown
Collaborator

What

Closes #130: co_await stream.Receive(timeout) — the completion-driven twin of #128's blocking deadline, with the same contract. A Detached serve loop can now bound every await instead of parking its frame forever (heap-invisible, holding its session) on a peer that never sends.

  • WebSocket::ReceiveAsync(timeout, callback) joins the contract: same four outcomes as the blocking overload — message, clean close, terminal error, Error::Timeout (code TimeoutError) — and the same rule: a timeout is not terminal, the receive slot is released exactly as a completed receive releases it, a message the wire delivers after the deadline waits for the next receive, and a non-positive timeout polls. A refusing default keeps every implementor compiling (the async family is opt-in, unlike the pure-virtual blocking overload); the shared contract suite holds every SupportsAsync() implementation to it.
  • The design question the issue flags (stash vs. cancel) dissolves at the socket layer: both transports already land inbound messages in per-session state and hand them to whoever receives next, so timing out a parked callback releases the slot without touching the wire, the read pump, or any in-flight message — no per-stream stash above, no cancellation primitive below.
  • The exactly-once race (timer vs. delivery vs. terminal transition) settles under the session lock via a park generation: every park bumps it, a deadline fires only for the generation it was armed with, so a stale deadline can never time out a later receive (timed or not). Beast arms a steady_timer on the connection's executor, captured weakly so a deadline never extends a session's life; the executor-less in-memory pair arms a short-lived watchdog thread on its shared state; JsonRpcStreamSocket passes the deadline through its classifier, where a timeout rides untouched (no violation, no close — the blocking overload's exact posture).
  • The coroutine layer: AsyncEventStream::Receive(timeout) (the async twin of EventStream's timed Receive — the matrix is now complete) and a ReceiveMessage(socket, timeout) overload for the raw pre-stream await (the jsonRpc2 opening-envelope read). Only decoder rejections close the session; the timeout passes through the awaitable untouched.
  • No ADR: like Add a deadline to WebSocket::Receive() #128's blocking half, this extends the ADR-0019 contract rather than changing architecture.

Folded in, as asked: out-of-tree, real-socket e2e for the #109 numeric bounds. The consumer's todo model gains an intEnum priority and float effortHours (optional — handlers and the model-evolution script untouched), so the narrowing/membership checks now run through the exact pipeline consumers ship: generator at build time → regenerated server on the production transport → hostile wire values (2^32+2, an unknown member, 1e300) rejected before the handler with suite-exact identities → the generated client serializing valid ones.

Also: TSan on the new deadline tests surfaced a latent race in the test suite's own Mailbox (notify outside the lock — the rule ContractMailbox already documents); fixed to match.

Testing

  • Contract suite (websocket_contract_test.h): four new type-parameterized deadline cases — quiet-wire timeout with slot release across two rounds, the non-positive poll, a terminal transition beating a parked deadline (with the one-shot mailbox proving the stale timer never double-completes), and one-outstanding across the overloads. They run over every instantiation: the pair, Beast, JsonRpcStreamSocket, and the consumer's own out-of-tree socket (which now implements the timed op, watchdog-shaped and generation-guarded).
  • Pair + coroutine layers (async_event_stream_test.cc): delivery-beats-deadline, timeout-then-late-message (nothing lost), the stale-deadline no-op, the zero-timeout poll, a serve loop that ticks through timeouts and then echoes, and the raw awaitable's deadline.
  • Real wire: beast_websocket_test.cc runs the timer against Beast's executor (timeout, then delivery under a fresh deadline with the stale timer live); jsonrpc_stream_socket_test.cc pins the wrapper's timed pass-through next to its blocking twin; the consumer's async_acceptance_test.cc runs the stated audience end to end — a hand-written Detached loop over a real Beast socket bounding its awaits out of tree.
  • Consumer numeric bounds: todo_beast_acceptance_test.cc posts the hostile bodies over the real socket and asserts the suite-exact rejections, then round-trips valid values through the generated client.
  • Locally: bazel test //... in-tree 126/126 and in the consumer module 13/13, both --config=werror; TSan over the four touched concurrency suites (which is what caught the Mailbox race) and gcc UBSan over the same, all green; clang-format/buildifier/changelog lint clean. The consumer's model-evolution script anchors are untouched by the model change.

Checklist

  • Tests added/updated for the change
  • bazel test //... and (cd codegen && gradle build spotlessCheck) pass locally (no codegen/Java changes in this PR)
  • Formatting clean (clang-format, buildifier, spotless)
  • Architectural decisions recorded as an ADR (not applicable — extends the ADR-0019 contract the way Add a deadline to WebSocket::Receive() #128 extended the blocking half; no new architecture)

🤖 Generated with Claude Code

https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU


Generated by Claude Code

claude and others added 10 commits August 25, 2026 21:37
ReceiveAsync(timeout, callback): the blocking Receive(timeout)'s
completion-driven twin — the same four outcomes, Error::Timeout when the
budget runs out, the slot released and the session untouched, non-positive
polls. A refusing default keeps every implementor compiling (the async
family is opt-in, unlike the pure-virtual blocking overload); the shared
contract suite grows four deadline cases that hold every SupportsAsync()
implementation to it.

The timeout/delivery race settles exactly once under the session lock via
a park generation: every park bumps it and a deadline fires only for the
generation it was armed with, so a stale deadline can never time out a
later receive. Beast arms a steady_timer on the connection's executor
(weakly captured — a deadline never extends a session); the executor-less
in-memory pair arms a short-lived watchdog thread on its shared state; the
JsonRpcStreamSocket wrapper passes the deadline through its classifier,
where a timeout rides untouched — no violation, no close.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
The async twin of EventStream's timed Receive, for the Detached loops that
are its audience: a bounded await resolves with Error::Timeout
("TimeoutError") on a session that keeps serving — the frame no longer
parks forever holding its session on a peer that never sends, and an event
delivered after the deadline waits for the next await. ReceiveMessage
grows the same overload for the raw pre-stream await (the jsonRpc2
opening-envelope read). Only decoder rejections close the session; the
timeout passes through the awaitable untouched.

Pinned over the pair at both layers: delivery-beats-deadline, timeout-
then-late-message, the stale-deadline no-op, the zero-timeout poll, a
serve loop that ticks through timeouts and then echoes, and the raw
awaitable's deadline.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
The consumer's own WebSocket implements the timed ReceiveAsync (the
watchdog shape, generation-guarded, holding itself alive via
shared_from_this for at most its own deadline) and runs the contract
suite's new deadline cases across the module boundary. The acceptance
suite adds the stated audience end to end: a hand-written Detached loop
over a real Beast socket bounds its await, treats the timeout as a tick,
and serves the message that arrives after one.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
The consumer's todo model gains an intEnum priority and a float
effortHours, so the narrowing and membership checks run through the exact
pipeline consumers ship: generator at build time, regenerated server on
the production transport, hostile wire values (2^32+2, an unknown member,
1e300) rejected before the handler with the suite-exact identities, and
the generated client serializing valid ones. Optional members — the
existing handlers and the model-evolution script are untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
TSan on the new deadline tests surfaced a latent race in the suite's own
Mailbox: notify_all ran after the lock was released, so a poster on a
foreign thread — the pair's deadline watchdog, here — raced the waiter's
return, and ~Mailbox could destroy the condition variable mid-notify.
ContractMailbox already documents and follows the notify-under-the-lock
rule; the test Mailbox now matches it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
…view panel)

Survivors of the panel on #198, all verified: (1) a std::thread spawn
that throws must not escape ReceiveAsync beside a still-armed park — from
a coroutine that is a use-after-free when the completion later fires into
the freed frame; the pair and the consumer reference implementation now
take the park back and refuse (exactly-once, nothing thrown), under
__cpp_exceptions in the pair since //runtime:http is in the -fno-exceptions
gate. (2) Watchdogs spawn outside the session lock — pthread_create is too
heavy to hold both ends' traffic behind. (3) now + timeout saturates at a
year, so milliseconds::max() as "practically forever" waits instead of
overflowing into an instant spurious timeout. (4) The poll doc no longer
promises inline completion for a ready message (Beast posts ready results
by design), and the stale-deadline test gets a full second of delivery
margin against sanitizer-loaded runners. The Beast timer's saturation
lands in the companion commit.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
… panel)

The companion test half of the hardening commit: the pair test's
first-round deadline grows to a second with a 1300ms probe, so a
sanitizer-loaded runner cannot let the deadline win the delivery race the
test is not about; and the test Mailbox posts its notify under the lock
(ContractMailbox's rule) so a watchdog-thread poster cannot race ~Mailbox.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
…ew panel)

The Beast half of the hardening: ArmReceiveDeadlineLocked clamps the
timer's expiry at a year, so milliseconds::max() as "practically forever"
waits instead of overflowing now + duration into the past and firing a
spurious instant timeout — the same saturation the pair and the consumer
reference implementation apply.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
The hardening's parked_generation sentinel left clang-tidy unable to
correlate "we parked" with "the callback was moved into the session", so
bugprone-use-after-move and clang-analyzer-cplusplus.Move both flagged the
inline completion on a path that cannot happen — CI lint's exit 123.

The pairing is real even if the path is not, so settle it structurally
rather than silence it: the completion the call still owes lives in a
local, and parking hands it over with the std::exchange the terminal
paths already use, which empties the local. Whether this call parked is
then read off the slot itself (`if (!deliver)`), so the two outcomes are
one variable rather than a flag a reader has to keep in sync with a move.
The pair and the consumer reference implementation both take the shape;
behavior is unchanged, and the watchdog still spawns outside the lock.

Verified: clang-tidy clean on websocket_pair.cc and across the CI sweep,
clang-format clean, and websocket_pair / beast_websocket / async_event_stream /
jsonrpc_stream_socket plus the out-of-tree consumer contract suite all pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
@cursor

cursor Bot commented Aug 26, 2026

Copy link
Copy Markdown

Review

LGTM — closes the #130 gap cleanly as the completion-driven twin of #128, and the prior panel survivors (spawn-failure refuse, lock scope around pthread_create, far-future saturation, park-off-the-slot) look correctly landed. CI is green across the full matrix.

What looks right

  • Park generation settles the timer/delivery/terminal race exactly once under the session lock on every path that matters: Beast (ArmReceiveDeadlineLocked / OnReceiveDeadline), the in-memory pair watchdog, and the out-of-tree ConsumerSocket. A stale deadline cannot complete a later park; a terminal transition that beats the deadline leaves the one-shot mailbox alone.
  • Timeout is not terminal is held end to end: slot released, session usable, late wire message waits for the next receive. ClassifyInbound passes !ok() through untouched, so JsonRpcStreamSocket neither violates nor closes on TimeoutError — matching the blocking overload.
  • Coroutine layer is the thin pass-through it should be (Receive / ReceiveMessage timeout overloads); only decoder rejection closes. The Detached serve-loop test and the consumer Beast acceptance test are the right audience proofs.
  • Contract suite gains the four deadline cases every SupportsAsync() driver must pass, including the consumer module — so the refusing default cannot silently strand a third-party socket that claims async.
  • Mailbox notify-under-lock fix matches ContractMailbox's rule; TSan catching it on the new deadline tests is the right failure mode.
  • Tracking: C++ Core Guidelines conformance review #109 fold (optional priority / effortHours over the real socket) is a clean consumer-boundary pin and leaves handlers / model-evolution anchors alone.

Nits (non-blocking)

  1. Beast spent timersDeliver / TakeAsyncWaitersLocked clear pending_receive_ but leave receive_deadline_ armed until it fires or the next timed park emplaces over it. Correct via generation + weak capture; receive_deadline_.reset() on slot release would just spare a useless executor wakeup.
  2. ConsumerSocket overload shape — untimed ReceiveAsync still early-returns under the lock; timed uses the deliver-slot / std::exchange shape the pair settled on for clang-tidy. Behavior matches; unifying the untimed path would remove the last parallel park style in that file.

No correctness issues found; happy to see this land.

Comment thread examples/bazel-consumer/websocket_contract_consumer_test.cc Outdated
Comment thread runtime/src/http/beast_transport.cc
Both non-blocking nits from the review on #198, verified against the code:

The Beast timer outlived the park it bounded. Deliver and
TakeAsyncWaitersLocked emptied pending_receive_ but left receive_deadline_
armed until it expired or the next timed park emplaced over it, so a
session that got its message immediately still woke the executor once at
the old deadline. The generation guard already made that wakeup a no-op —
this just spares it. Resetting is safe from any thread for the same reason
the arming emplace is: every touch of the timer is under mutex_, and the
armed handler captures the session weakly rather than the timer, so
destroying it only cancels a pending wait whose handler then returns on
operation_aborted.

The consumer reference implementation had two park styles side by side —
the untimed receive early-returning under the lock, the timed one using
the deliver-slot shape the pair settled on. Since this file is what a
third party copies, the untimed path now takes the same shape; behavior is
identical.

Verified: clang-format clean; beast_websocket / async_event_stream /
websocket_pair / jsonrpc_stream_socket / session_registry pass plain, and
the first four again under ASan and under TSan (the lifetime and race
questions the timer reset actually raises); all 13 consumer module tests
pass out of tree.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
@aaylward
aaylward enabled auto-merge August 26, 2026 10:31
@aaylward
aaylward merged commit fc209de into main Aug 26, 2026
16 checks passed
@aaylward
aaylward deleted the claude/smithy-cpp-dedup-issues-hy5key branch August 26, 2026 10:39
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.

A deadline for the coroutine receive (AsyncEventStream / ReceiveMessage)

2 participants