Skip to content

refactor(network): generalize paired streams into service sessions - #956

Merged
czarcas7ic merged 2 commits into
adam/getblocks-paired-transportfrom
evan/service-session-streams
Sep 10, 2026
Merged

refactor(network): generalize paired streams into service sessions#956
czarcas7ic merged 2 commits into
adam/getblocks-paired-transportfrom
evan/service-session-streams

Conversation

@evan-forbes

@evan-forbes evan-forbes commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Motivation

A service with several required persistent streams should start only after every stream is ready. #943 implements that guarantee for a data/request pair, but hardcodes the role count, request queue, and write policies in the transport.

This draft targets #943 (adam/getblocks-paired-transport).

Solution

  • Derive complete service-session layouts from the existing stream declarations. Persistent streams sharing a capability start and retire together. Request/response streams keep their independent lifecycle.
  • Replace the pair API with Session* policies and resources, and rename the persistent mode from Ordered to Persistent. Services declare opening policy once and supply each stream's queue limits, message bounds, and write policy.
  • Handle one or more persistent streams through the same setup and worker path. Preserve bounded incomplete setup, shared admission resources, cancellation, and reopening. Report exit after every worker and reader finishes.
  • Select whole session versions before opening streams. The lowest stream kind supplies the layout version, so crossed member versions cannot produce a mixed layout.
  • Preserve existing single-stream preludes and the two-stream identifier encoding. Production block sync remains on feat(network)!: add bounded service sessions and request resources #943's existing single-stream protocol.

Single-stream cancellation now resets an unfinished frame. A persistent write timeout retires only its service session, preserving unrelated services on the connection. These changes also apply to existing single-stream services.

See the service-session guide for declarations, negotiation, and migration details. The later #945 activation must remove its pair hook and declare its request queue and 32-second data write deadline through the service hooks.

Testing

  • cargo +1.97.0 test -p zakura-network --lib zakura:: --locked: 894 passed, 3 ignored.
  • Real QUIC regressions cover three-stream setup in a different arrival order, admission at the exact stream limit, reopening, expiry, duplicate members, mismatched/zero identifiers, per-stream queue limits, ephemeral completion, and cancellation with a live sibling service.
  • Registry tests cover complete version selection with crossed member versions and incompatible declarations.
  • cargo +1.97.0 clippy -p zakura-network --all-targets --locked -- -D warnings.
  • Formatting, Markdown lint, spelling, and diff checks.

Changelog

Added docs/changelog/unreleased/956.md for service-local write timeout handling. ./scripts/changelog.py check passes.

@czarcas7ic
czarcas7ic force-pushed the adam/getblocks-paired-transport branch from d908fe7 to a5b40f2 Compare September 10, 2026 05:09
@evan-forbes
evan-forbes force-pushed the evan/service-session-streams branch from 0c11b10 to 680a8d2 Compare September 10, 2026 05:26
@evan-forbes
evan-forbes force-pushed the evan/service-session-streams branch from 680a8d2 to 09bfd72 Compare September 10, 2026 15:57
@czarcas7ic
czarcas7ic marked this pull request as ready for review September 10, 2026 17:20
@czarcas7ic
czarcas7ic merged commit f327e9b into adam/getblocks-paired-transport Sep 10, 2026
9 checks passed
@czarcas7ic
czarcas7ic deleted the evan/service-session-streams branch September 10, 2026 17:20
@v12-auditor

v12-auditor Bot commented Sep 10, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 found 17 issues worth reviewing.

Open the full results here.

FindingSeverityDetails
F-272903 🔵 Low
Valid boundary completion kills connection

Pending-session expiry is enforced twice with different consequences. The connection loop normally expires an incomplete setup locally, but PendingSessions::insert also checks Instant::now() >= pending.deadline and returns InvalidServiceSession even when the final member has the correct identifier and role. A completed setup can win the tokio::select! poll just before the deadline and reach this synchronous check just after the deadline due to ordinary elapsed wall time or OS preemption. finish_bi_stream_setup treats that timing condition like a malformed session and cancels the shared connection token, tearing down unrelated service workers instead of only expiring the incomplete session.

F-272904 🟡 Medium
Stale partial reopen kills connection

open_service_session sends a fresh multi-stream wire identifier member-by-member. If an early member reaches the peer but a later prepare_ordered_stream fails, dropping the already-prepared member resets its QUIC stream, yet the receiver's incomplete PendingSession has no worker that observes this reset and remains stored until its deadline. The opener treats the failure as retryable and quickly generates a different identifier for the next attempt. Because the first reopen backoffs are shorter than the setup timeout, the replacement commonly reaches the stale pending entry, whose identifier mismatch is classified as InvalidServiceSession and escalated to connection-wide cancellation.

F-272905 🟠 High
Stalled receivers replay block serving

Block sync inherits the generalized Service API's ten-second persistent-write deadline and enables automatic session reopening. An authenticated peer can send a valid status, issue valid GetBlocks requests for locally servable ranges, and then stop consuming the block-sync stream, causing state range reads and full block serialization before the transport write stalls. The timeout-specific worker path exits only the block-sync session and intentionally leaves the authenticated connection alive. Teardown removes the active peer without recording the block-sync no-progress park, so demand returns OpenNow and the handler re-admits the same peer after bounded backoff. The peer can repeat the cycle indefinitely, and duplicate requested ranges are not suppressed by the serving admission counter.

F-272906 🟡 Medium
Header timeout cascades to connection teardown

Header sync inherits the default ten-second persistent-write deadline but represents connection ownership only through its currently active peer record and reactor admission. When a peer stops consuming a valid header response, the timeout path retires only the header session; supervised teardown then removes that active peer record before any replacement is admitted. HeaderSyncService::owns_connection_for_peer immediately returns false during this gap, even if the reactor has not yet processed the disconnect event. Discovery periodically samples other service owners and interprets a false result as a discovery-only connection, retires itself, and cancels the shared connection. On inbound connections, header sync's initiator-only policy means the local node is not entitled to reopen the session at all; on locally initiated connections there is still a backoff gap.

F-272907 🟡 Medium
Valid alternative layouts fail registration

The new session declaration contract permits alternative persistent layouts under distinct capabilities and requires only the stable primary stream's version to advance when a layout changes. ServiceRegistry::new nevertheless enforces per-service uniqueness using only (kind, version) before it groups declarations by capability. Two alternatives such as {primary 6/v1, member 7/v1} and {primary 6/v2, member 7/v1} therefore reject the unchanged secondary member as a duplicate, despite following the documented layout-version rule. Registry construction fails before the node can start the service. The internal (kind, version) layout map has the same inability to represent one unchanged member in multiple alternatives.

F-272908 🟡 Medium
Dropped handles strand session resources

spawn_service_session does not attach the shared session cancellation token to the application-facing FramedRecv or FramedSend handles. When the last sender is dropped, the worker receives None, disables the outbound branch, and continues running; when the receiver is dropped while the peer is quiet, the reader remains blocked on QUIC input because it does not select on channel closure. No worker exits, so the coordinator never cancels the remaining roles or emits SessionExit. An UntilCancelled write already blocked by flow control is likewise unaffected by application handle closure. Sibling traffic can keep the connection fresh indefinitely while the abandoned session remains alive.

F-272909 🟡 Medium
Tiny frame cap kills sibling services

Persistent-stream setup accepts the peer-provided StreamPrelude.max_frame_bytes without checking that it can contain the fixed frame header. The value is used directly when deriving outbound_frame_cap, so a peer can advertise a cap below FRAME_HEADER_BYTES. Any service frame then fails locally in Frame::encode before a QUIC write begins. The worker distinguishes only timeout and peer Stopped errors; this deterministic encode failure takes the generic branch and cancels the connection-wide token.

F-272910 🔵 Low
Truncated headers evade protocol teardown

After the first frame-header byte is received, read_frame maps every failure while reading the remaining header bytes to ZakuraHandlerError::Closed. A peer can therefore send one to seven header bytes and gracefully finish its send half, and the truncated in-progress frame is treated as clean remote closure. The persistent worker suppresses connection teardown for Closed, publishes remote-close attribution, and retires only the service session. This differs from payload handling, which treats a normal truncated FIN as a protocol error and accepts only an explicit reset as clean closure.

F-272911 🟡 Medium
Request streams bypass version negotiation

The registry computes the highest negotiated request-response version, but request opening and admission do not enforce that result. Outbound dispatch uses stream_for_kind, which returns the first declaration for the kind regardless of the connection's negotiated mask, so a peer that negotiated only a later capability can receive an unnegotiated older prelude. Inbound admission checks exact declaration and capability membership, but the selected-version rejection is restricted to persistent streams, allowing an older request-response version whenever both alternative bits were negotiated. The request callback receives only the stream kind and cannot recover the admitted version.

F-272912 🔵 Low
Admitted sessions get wrong capability mask

add_escalated_peer removes admitted stream handles by kind while iterating every declared version, and records the capability of whichever declaration encounters that kind first. For a service whose older and newer layouts reuse the same primary kind under different capability bits, the older declaration can claim a newer admitted handle because the retained ServiceStream.version is ignored. The returned admitted-capability mask then identifies an unselected or unnegotiated alternative. Connection cleanup later expands that incorrect mask back into service removal callbacks.

F-272913 🟡 Medium
Sibling sessions suppress request-service admission

The production connection path calls full add_peer fanout only when no persistent stream is negotiated anywhere on the connection. If any unrelated persistent service exists, it calls add_escalated_peer with only opened persistent handles, and that method skips every negotiated service whose extracted handle map is empty. A valid service declaring only request-response streams therefore receives lifecycle admission when used alone but receives no add_peer callback when a sibling persistent capability is also negotiated. Its request streams remain independently dispatchable by kind.

F-272914 🔵 Low
Request streams bypass negotiated limit

The connection-wide semaphore is initialized from negotiated max_open_streams and is used by persistent-session opening and inbound stream setup, but outbound request-response streams bypass it. The outbound request branch invokes write_outbound_request_frame without a permit, and the helper directly calls connection.open_bi(). A request can therefore create an additional live stream while all negotiated permits are occupied. The connection loop awaits the complete request inline, so a peer that withholds the response can also prevent that loop from processing session exits, reopens, or new incoming streams until the request timeout.

F-272915 🟡 Medium
Shared capabilities corrupt removal fanout

The registry permits multiple services to register the same capability bit, while lazy admission returns only a capability bitmask as the record of services actually reached. add_escalated_peer can skip one same-bit service because it has no opened handles and admit another, but the returned mask still contains their common bit. Connection cleanup passes that bit to remove_peer, which expands it back to every service registered under the capability, including the skipped service that never received a matching add_peer. A capability bitmask cannot encode partial admission among same-bit services.

F-272916 🔵 Low
Initial admission hides negotiated capabilities

During initial persistent-session opening, the handler builds opened_capabilities only from sessions successfully opened locally and passes that partial value as the service-facing Peer.negotiated field. This conflicts with the field's contract that it represents all capabilities accepted by both peers and differs from later reopen and remote-admission paths, which pass the full accepted_capabilities. Capabilities for remote-opened, deferred, or request-response-only sibling services are hidden from initially admitted services. The same service can therefore observe different negotiated context across generations of one connection.

F-272917 🔵 Low
Unserviceable streams are advertised

ServiceRegistry::new accepts request-response stream declarations even when the owning service leaves as_request_response() at its default None. The capability is still advertised and negotiated, and inbound setup accepts the declared stream. When an honest peer sends the first valid request, dispatch fails solely because the local owner has no request handler. The request worker classifies that local configuration failure as a protocol rejection and cancels the peer connection.

F-272918 🔵 Low
One capability ambiguously selects request versions

The registry allows multiple request-response versions of one kind to share the same capability bit. Generic validation accepts them because their (kind, version) pairs differ, and persistent layout validation does not inspect request-response declarations. The request selector then silently chooses the higher local version, even though the shared capability mask carries no information about which version the peer supports. Inbound admission also accepts either exact version under the same negotiated bit.

F-272919 🔵 Low
Public peers lose session generation

The exported Peer::new and Peer::new_with_direction constructors synthesize conn_id = 0, session_id = 0, and version = 0 for every supplied stream. They also assign a distinct child cancellation token to each stream, after which the peer-level service token is selected from an arbitrary HashMap value. Successive peers built through this API are therefore generation-indistinguishable to the session-aware accessors, and a multi-stream peer does not have one cancellation scope shared across all members. The native handler avoids these constructors, but public embedders and test/service integrations can rely on them.

Analyzed 12 files, diff 7b00a40...09bfd72.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-10T17:26:17.081650Z 09bfd72 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants