Fix/bidi streaming cancellation - #2
Merged
Merged
Conversation
The HTTP/2 bidirectional streaming transport could not cancel a parked invocation: a CANCEL signal woke the handler but surfaced the wrong outcome, and a combinator reached while cancelled re-suspended forever. Two transport-shared bugs: - Combinators (select/awaitAll/awaitAny/awaitAllSucceeded) suspended with a cancel-inclusive predicate but, when woken only by a cancel, threw a LogicException / misleading TerminalException instead of CancelledException (HTTP 409). They now short-circuit cancellation both before suspending (so a request/response re-invocation carrying the CANCEL signal in its journal terminates instead of looping) and after the streaming park resumes, mirroring StateMachine::awaitCompletion. - The streaming driver resumed the handler fiber at most once per inbound chunk, then blocked on the next read(); a chunk that already carried the notification resolving the next await (batched completions, or a cancel reached straight after) caused a spurious SuspensionMessage. driveStreaming now drains every resolvable park after start() and after each chunk. Also lands the AmpStreamingServer transport fixes the bidi path needs at all (h2c prior-knowledge via allowHttp2Upgrade, per-stream/connection idle timeouts raised above the runtime's own windows) and the bidi conformance image (Dockerfile.amp + main-amp.php). Adds streaming-driver coverage for the cancel/drain paths (the cross-invocation cancel path previously had none): combinator-while-cancelled -> 409, cancel mid-stream then combinator, batched completions, awakeable signal on the open stream, and the awakeable suspension await-tree.
Two service-protocol-V7 corrections that let a parked bidi-streaming
invocation receive external signals (the CANCEL signal, and awakeables
resolved by another invocation), which Restate 1.7.0 delivers only when
service protocol V7 is enabled (RESTATE_EXPERIMENTAL_ENABLE_PROTOCOL_V7):
- Awakeable id prefix was `prom_1` (the completion/promise-backed form);
V7 awakeables are signal-backed and use `sign_1`. With `prom_1` the
runtime decodes a resolution as a completion index with no backing
command and kills the partition ("storage corruption"). Matches
sdk-shared-core v7.0.0 AWAKEABLE_PREFIX.
- The streaming suspender parked silently, so the runtime never learned
what a parked invocation was waiting on and never pushed the awaited
signal/completion onto the open stream. It now emits an AwaitingOnMessage
(0x0006) with the await tree on each park, per the V7 protocol.
Adds the AwaitingOnMessage frame and StateMachine::writeAwaitingOn, and an
env-gated (RESTATE_STREAM_DEBUG) wire trace in the streaming driver. Streaming
unit tests updated to expect the AwaitingOn frame at each park (486 green).
With these, a cancelled invocation now observes the cancel and terminates
with 409 at the SDK level; finalizing that terminal back to the runtime over
bidi is tracked separately.
Brings Cancellation conformance over the V7 bidi transport from 0/6 to 5/6 (cancelFromContext fully green). Three corrections, all matched to sdk-shared-core's cancellation model: - Cancel-guard await tree: a single-leaf await now flattens its ids next to the built-in CANCEL signal under a FirstCompleted node (matching the canonical encoding the runtime keys its cancel wake-up off) instead of nesting the await below a top-level signal. Without the flat shape the runtime never re-invokes a suspended invocation on cancel. - Implicit cancellation propagation: a handler now tracks the invocation ids of the calls it issues and, when cancelled at an await, sends the built-in CANCEL signal to each known child before failing with 409. A cancelled parent therefore tears down the calls it was blocked on, so the child releases its virtual-object lock (the conformance verifies this). - Cancellation is raised through StateMachine::raiseCancellation() from every await/combinator site so the propagation runs uniformly. Remaining: cancelFromAdminAPI on an awakeable await still fails with an h2 "unexpected frame type" on the runtime side; tracked separately.
The Restate runtime is a single trusted peer that opens one long-lived bidi
connection per in-flight invocation, all from the same IP. amphp's default
ceilings (1000 total, 10 per IP, 1000 concurrent) are far too low: at ~10 the
runtime is denied new connections ("too many existing connections from <ip>"),
which the runtime sees as broken-pipe / unexpected-frame errors and dropped
invocations under load. Raised the total, per-IP, and concurrency limits so the
runtime governs how many invocations run at once.
With this, Cancellation conformance over the V7 bidi transport is 6/6.
Cancellation (6/6) and KillInvocation (1/1) now pass over the bidirectional (amp) transport against a V7-enabled runtime, with no regression (State 3/3, ServiceToServiceCommunication 5/5 — V7 also recovered oneWayCallWithDelay). - Remove the Cancellation.*/KillInvocation.kill entries from every profile in conformance/exclusions.yaml and explain the V7 requirement. - Add conformance/Dockerfile.restate-v7 (restate:latest + the experimental-enable-protocol-v7 flag) for running the bidi suite. - Add docs/adr/0001 recording the cancellation-over-bidi decision and the service-protocol-V7 requirement.
The env-gated per-invocation streaming trace served its purpose diagnosing the bidi cancellation work; drop it (and its README note) to keep the hot path and the production code clean.
…binators
Closes three pre-existing bidi conformance gaps (RunRetry 3/3, UserErrors 10/10,
Sleep 2/2), each verified against the V7 runtime.
- ctx->run over bidi hung forever: the runtime confirms a proposed run with a
ProposeRunCompletionAck control frame (processing phase), which the streaming
ingest dropped, so the parked run never resolved. The SDK now stashes the
proposed result and promotes it into the completion table on the ack. (r/r was
unaffected — it suspends and replays the durable RunCompletion.)
- Failure.metadata (V7 field 3): round-trip the user error-metadata map through
Failure encode/decode, TerminalException, InvocationProcessor, DurableFuture
(callee propagation) and ctx->run's terminal-failure path.
- Conformance services: Failing.php handlers accept the {errorMessage, metadata}
object the contract sends (was typed `string` → TypeError → retry-forever
timeout); VirtualObjectCommandInterpreter gains awaitAllCompleted (+ the sibling
awaitFirst*/awaitAllSucceeded command types) so Sleep.manySleeps runs.
The streaming run unit test now feeds only the ack (matching the real V7 runtime)
instead of a fabricated RunCompletion.
Closes the bidi conformance gaps for Signals (2/2) and the signal/mixed Combinators (9/9; the two awakeable-combinator cases stay excluded, as in the Rust SDK). SDK: - Named-signal receive: StateMachine tracks a name-keyed signal table, routes name-only SignalNotifications into it, and exposes awaitNamedSignal/isNamedSignalReady/peekNamedSignal (cancel-guarded like awaitSignal). Context::createSignal(name) returns a name-addressed DurableFuture; Future::forNamedSignal + the widened suspend*/partitionFutures carry waiting_named_signals through the combinators. - Named-signal send: SendSignalCommand::resolveNamed/rejectNamed (name + value / failure), StateMachine::sysSendSignal, Context::resolveSignal/rejectSignal. - Context::runAsync(name, action): a non-blocking ctx.run future (the blocking run() keeps its retry-policy path). Conformance: TestUtilsService.resolveSignal/rejectSignal; the command interpreter gains createSignal (await + future), and runReturns/runThrowTerminalException as concurrent run futures for the mixed combinators. 503 unit tests + lint green; new coverage for named-signal routing/await/suspend, the send-command encoding, Future::forNamedSignal, and runAsync.
With named signals, run-completion-ack, Failure.metadata and the conformance service completions, the default suite over the bidirectional (amp) transport on a V7 runtime now passes 48/48 runnable tests (Cancellation 6/6, KillInvocation 1/1, Signals 2/2, Combinators 9/9, RunRetry 3/3, UserErrors 10/10, S2S 5/5, ...). Exclude ServiceToServiceScopeConcurrency.scopeAndLimitKeyArePropagatedOnService ToServiceCalls across all profiles — V7 scoped concurrency (call scope + limit key) needs both SDK support and the runtime's experimental virtual-queues subsystem, a distinct unimplemented feature. Update the conformance README result.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
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
2 participants
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.
No description provided.