Share one localhost WS listener across native product executions - #600
Share one localhost WS listener across native product executions#600decrypto21 wants to merge 3 commits into
Conversation
23e18a3 to
5a0934c
Compare
github-actions
Bot
added
the
host-work
6e9e02c to
822ee5e
Compare
|
CI Status: not green, 1 item needs attention.
All job results
Commit |
|
@lore-bot-app review |
|
Reading the diff and checking what the record says. Back in a few minutes. |
|
@lore-bot-app review |
|
Reading the diff and checking what the record says. Back in a few minutes. |
![lore-bot-app[bot]](https://avatars.githubusercontent.com/u/14176906?s=60&v=4){kind=small}
There was a problem hiding this comment.
TL;DR
Consolidates the per-execution localhost WS listeners into one shared, token-multiplexed listener owned by the host runtime, plus a lock-ordering fix in ProductRuntime::dispose. 2 blocking, 4 minor. The blockers are a test build break against main's new frame envelope and a connection-cap budget that undoes the per-execution isolation issue #355 asked for.
Summary
NativeTrUApiHostRuntime now owns one SharedWsBridge. Each NativeProductExecution::start_ws_bridge registers a fresh 256-bit token against it rather than binding its own listener, and stop_ws_bridge revokes that token and tears down only that execution's connections. Handshakes moved out of the accept loop into one task per accepted connection, bounded by HANDSHAKE_TIMEOUT and a separate MAX_PENDING_HANDSHAKES backlog that evicts its oldest entry when full. Connection caps split into per-execution and listener-wide counters reserved by CAS inside the handshake callback. Separately, host_core.rs moves dispose's disposed swap inside in_flight's lock and adds a matching re-check in receive_frame, closing the window where a dispatch could insert after the drain.
What the record says
- Issue #355 is the driving request, and it sets the acceptance bar explicitly: a single shared listener that "maintains strict isolation of queues, subscriptions, and backpressure per execution while reducing system resource usage" (issues/355). Concern 2 below is where the implementation trades that isolation away for the connection-count dimension.
- PR #600 is this change; Lore's index already records it with the same design intent (per-connection tasks, constant-time token matching, bounded caps) and lists
822ee5e8— the teardown/backlog follow-up commit — as part of it (pull/600). - Issue #263 (request cancellation) is the prior decision that
ProductRuntime's in-flight registry plus RAII teardown is the cancellation mechanism, and that "RAII cleanup propagates through subxt" (issues/263). The newDisposeGuardis consistent with that: it makes disposal happen on the abort path, where an explicit trailingdispose()call would have been skipped. - Issue #576 is the closest precedent for the
dispose/receive_framechange: a dual-mutex subscription-routing race where "a recent commit fixed an immediate race but left the dual-lock architecture difficult to maintain," and the team's answer was to consolidate onto one lock rather than add a second check (issues/576). This diff takes the second-check route inhost_core.rs. It is correct as written —disposedhas exactly one writer (host_core.rs:1393) and both critical sections now serialize onin_flight— but it is the pattern #576 flagged as a maintenance cost, so it's worth a sentence in the PR body saying why a single lock wasn't the answer here. - I found no prior discussion of the connection-cap sizing, the handshake-backlog eviction policy, or an incident behind them. Those appear to be new to this PR.
- Ownership for this area:
replghost,pgherveou(who filed #355),decrypto21(who_knows).
Concerns
1. Two new tests use the pre-#357 Payload shape and will not compile. rust/crates/truapi-server/src/ws_bridge.rs:1752 and rust/crates/truapi-server/src/native.rs:3555 both construct Payload { id: ids.request_id, value }. After e8ee375e (merged into this branch at 0c4621fe), Payload is { trait_id, method_id, message_type, value } (frame.rs:220) and MethodIds is { trait_id, method_id } — there is no id field and no request_id. The merge updated the pre-existing test at ws_bridge.rs:1262 but not the two this PR adds, so cargo test --workspace fails to build. Also needs message_type: crate::frame::MESSAGE_TYPE_REQUEST.
2. MAX_TOTAL_WS_CONNECTIONS = 64 against MAX_WS_CONNECTIONS_PER_EXECUTION = 32 lets one execution starve the others. ws_bridge.rs:80-86, enforced at ws_bridge.rs:890-910. Two tokens' worth of misbehaving peers exhaust the shared budget and every subsequent execution — App, Widget, Chat — gets a 503 at handshake with zero connections of its own. Before this change each execution had a private listener and a private 32-connection cap, so one product could not affect another's ability to connect at all; the shared total is a new cross-execution coupling, which is exactly the isolation #355 asked to preserve. The per-execution doc comment says "Each execution uses exactly one connection," so 32 looks like it was carried over from the old whole-listener cap rather than re-derived. Either drop the per-execution cap to a small single-digit number or raise the total well above per_execution × plausible_executions.
3. find_matching holds the registry mutex across a scan whose cost the peer controls. ws_bridge.rs:298-310 locks entries and calls path_token_matches once per registered token, and each of those re-parses and re-walks the whole query string. That runs inside the synchronous tungstenite handshake callback, and entries is the same lock every register and revoke must take. A peer padding ?t= pairs into the request URI multiplies the work under the lock by the number of pairs; I did not confirm what upper bound tungstenite puts on the request line, so I can't size the amplification. The fix is cheap regardless: collect the query's t values once before taking the lock, then compare each token against that slice.
4. a_panicking_execution_does_not_affect_a_sibling asserts a property the shipped binary does not have. ws_bridge.rs:1624, and its own doc comment at :1617 says so: the isolation it observes comes from tokio's per-task panic containment, and the root Cargo.toml:20 sets panic = "abort" for release. A test whose doc explains that it only holds in test builds is not evidence about production. Either delete it or change WsProductRuntimeFactory::product_runtime to return a Result so factory failure is a real, non-panicking path worth testing.
5. Several new comments narrate the change rather than the code, against CLAUDE.md. The repo rule is "Do not add code comments or doc comments that narrate migrations, compatibility shims, or historical changes. Comments should describe only the current code." Instances: ws_bridge.rs:822 ("Connection setup now runs concurrently"), ws_bridge.rs:448 ("logger is still used for"), ws_bridge.rs:1674 ("before that, everything shared the accept loop's own inline handshake"), native.rs:3504 ("The PR's headline behavior... rather than only ws_bridge.rs's own lower-level unit tests"). The same rule argues for shrinking the ~50-line module header (ws_bridge.rs:1-53) and the 10-line explanatory blocks at ws_bridge.rs:232-238, :741-747, :584-591 — most of those restate the code immediately below them. That history belongs in the PR body.
6. WsBridgeStartError::AlreadyRunning's doc is now wrong. ws_bridge.rs:132-134 says "A bridge is already running for this host." The only site that returns it is native.rs:1174, where it means this execution already holds a token; the listener running for siblings is the normal case. The Kotlin docstring got updated (TrUAPIHost.kt:849) but the Rust error variant and the Swift wrapper at ios/truapi-host/Sources/TrUAPIHost/TrUAPIHost.swift:858 did not.
Questions for the author
Drop for NativeProductExecution(native.rs:1207) →shutdown→stop_bridge→SharedWsBridge::revoke→block_until_finished, whichws_bridge.rs:618documents as having no deadline. On iOS/Android that destructor can run on the thread that releases the UniFFI object. The oldWsBridge::stopblocked the same way, so this isn't a regression in shape — but it now fires on every execution release rather than once per host shutdown. Was a deadline considered and rejected, or just not reached yet?- The backlog eviction at
ws_bridge.rs:692aborts the oldest pending handshake. That is right for a stalled peer, but a legitimate handshake that happens to be oldest when 64 connections land at once gets aborted with no retry and no distinguishable error at the client. Is the assumption that loopback handshakes are fast enough (sub-ms, per:688) that this can't be hit in practice, or is a client-side retry expected? bind_portis now advisory for every caller after the first (ws_bridge.rs:362-367). Does any host actually pass a non-zero port? If not, it would be cleaner to drop the parameter than to keep abind_port_ignoredlog for a case nobody exercises.- Per #576, the team's stated preference for this class of dual-lock race was consolidating onto one lock. Why is the
disposedatomic +in_flightmutex pairing (host_core.rs:1278-1287,:1389-1399) the right shape here instead of foldingdisposedinto thein_flightmutex's own state?
🤖 Reviewed by Lore (Parity knowledge base) · 40 agent turns · 404.3s · knowledge as of 2026-09-11
| .drain() | ||
| { | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()); | ||
| if self.disposed.swap(true, Ordering::AcqRel) { |
There was a problem hiding this comment.
Issue #576 is the closest precedent for the dispose/receive_frame change: a dual-mutex subscription-routing race where "a recent commit fixed an immediate race but left the dual-lock architecture difficult to maintain," and the team's answer was to consolidate onto one lock rather than add a second check (issues/576). This diff takes the second-check route in host_core.rs. It is correct as written — disposed has exactly one writer (host_core.rs:1393) and both critical sections now serialize on in_flight — but it is the pattern #576 flagged as a maintenance cost, so it's worth a sentence in the PR body saying why a single lock wasn't the answer here.
| let request_frame = ProtocolMessage { | ||
| request_id: "p:1".into(), | ||
| payload: Payload { | ||
| id: ids.request_id, |
There was a problem hiding this comment.
Ownership for this area: replghost, pgherveou (who filed #355), decrypto21 (who_knows).
Concerns
1. Two new tests use the pre-#357 Payload shape and will not compile. rust/crates/truapi-server/src/ws_bridge.rs:1752 and rust/crates/truapi-server/src/native.rs:3555 both construct Payload { id: ids.request_id, value }. After e8ee375e (merged into this branch at 0c4621fe), Payload is { trait_id, method_id, message_type, value } (frame.rs:220) and MethodIds is { trait_id, method_id } — there is no id field and no request_id. The merge updated the pre-existing test at ws_bridge.rs:1262 but not the two this PR adds, so cargo test --workspace fails to build. Also needs message_type: crate::frame::MESSAGE_TYPE_REQUEST.
2. MAX_TOTAL_WS_CONNECTIONS = 64 against MAX_WS_CONNECTIONS_PER_EXECUTION = 32 lets one execution starve the others. ws_bridge.rs:80-86, enforced at ws_bridge.rs:890-910. Two tokens' worth of misbehaving peers exhaust the shared budget and every subsequent execution — App, Widget, Chat — gets a 503 at handshake with zero connections of its own. Before this change each execution had a private listener and a private 32-connection cap, so one product could not affect another's ability to connect at all; the shared total is a new cross-execution coupling, which is exactly the isolation #355 asked to preserve. The per-execution doc comment says "Each execution uses exactly one connection," so 32 looks like it was carried over from the old whole-listener cap rather than re-derived. Either drop the per-execution cap to a small single-digit number or raise the total well above per_execution × plausible_executions.
3. find_matching holds the registry mutex across a scan whose cost the peer controls. ws_bridge.rs:298-310 locks entries and calls path_token_matches once per registered token, a
| /// and blocks on a synchronous channel rather than awaiting directly, | ||
| /// since this is called from ordinary host threads with no executor of | ||
| /// their own. There is no deadline: a connection wedged in non-yielding | ||
| /// work holds the caller for as long as it stays wedged. |
There was a problem hiding this comment.
Drop for NativeProductExecution (native.rs:1207) → shutdown → stop_bridge → SharedWsBridge::revoke → block_until_finished, which ws_bridge.rs:618 documents as having no deadline. On iOS/Android that destructor can run on the thread that releases the UniFFI object. The old WsBridge::stop blocked the same way, so this isn't a regression in shape — but it now fires on every execution release rather than once per host shutdown. Was a deadline considered and rejected, or just not reached yet?
| // full backlog is one that has stalled, while refusing the | ||
| // newcomer would let a peer holding every slot lock the whole | ||
| // shared listener out for every execution. | ||
| if setup_tasks.len() >= MAX_PENDING_HANDSHAKES |
There was a problem hiding this comment.
The backlog eviction at ws_bridge.rs:692 aborts the oldest pending handshake. That is right for a stalled peer, but a legitimate handshake that happens to be oldest when 64 connections land at once gets aborted with no retry and no distinguishable error at the client. Is the assumption that loopback handshakes are fast enough (sub-ms, per :688) that this can't be hit in practice, or is a client-side retry expected?
| /// Ensure the shared listener is running and register a fresh | ||
| /// per-execution token against it. | ||
| /// | ||
| /// `bind_port` only takes effect for the first execution to register; |
There was a problem hiding this comment.
bind_port is now advisory for every caller after the first (ws_bridge.rs:362-367). Does any host actually pass a non-zero port? If not, it would be cleaner to drop the parameter than to keep a bind_port_ignored log for a case nobody exercises.
| // for its own swap-and-drain, closes that window - whichever runs first is | ||
| // what the other observes, so a dispatch that loses the race is turned away | ||
| // instead of running past a disposal that already happened. | ||
| { |
There was a problem hiding this comment.
Per #576, the team's stated preference for this class of dual-lock race was consolidating onto one lock. Why is the disposed atomic + in_flight mutex pairing (host_core.rs:1278-1287, :1389-1399) the right shape here instead of folding disposed into the in_flight mutex's own state?
3 participants
Closes #355.
What
Every product execution (App, Widget, Chat/Worker) connects through one shared localhost WebSocket listener instead of its own, each with an independent token.
NativeTrUApiHostRuntime, started lazily on the first execution'sstart_ws_bridgecall and kept alive for the runtime's lifetime.t=query pairs, so timing can't reveal which token, if any, matched.ProductRuntime, cancelling its host-core subscriptions and detaching its chat state. Called off the shared executor, revocation waits for the aborted connection tasks to be joined before it returns. Joining a task is not a barrier on the destructors inside it, so a caller cannot treat the return as proof that every resource the connection held is already released, and the wait carries no deadline: a connection wedged in non-yielding work holds the caller until it unwedges. Both properties match whatstopalready does for the whole listener.start_ws_bridge/stop_ws_bridgekeep their existing signatures, so iOS/Android need no source changes. Confirmed by regenerating the UniFFI Swift bindings, where only doc-comment text and the API checksum moved.NativeTrUApiCore, the combined single-host/single-execution wrapper the issue's acceptance criteria call the legacy API, was already removed from this repo in refactor(native): remove legacy single-execution core #508, before this PR started. OnlyNativeTrUApiHostRuntime/NativeProductExecutionremain, and opening exactly one execution from one host runtime through them keeps working end-to-end unchanged:native.rs's existingpending_permission_decision_does_not_stall_bridgeandstart_ws_bridge_twice_returns_already_runningtests cover a real connect, a real request/response round-trip, and a clean stop through exactly that path.Why
Every execution binding its own listener means its own port, accept loop, and connection cap: unnecessary overhead that matters more as a host runs multiple product executions concurrently (App, Widget, Chat side by side, not just one at a time). One shared listener removes the duplication while keeping every execution's traffic, queues, and backpressure as isolated as they were before.
How
A connection's token match happens inside the WebSocket handshake, before its task is spawned or registered against its execution, so in principle a token could be revoked in the gap between a sibling's handshake resolving and its registration landing. Both sides serialize on the same per-execution lock:
revokemarks the execution revoked and aborts whatever is already registered under one lock acquisition, and the connection is spawned and registered under that same lock only after reading the flag. Whichever happens first is what the other observes, so a revoked execution never gets a task reading from the socket, and a connection that registers first is never left running with no owner.Handshakes run in independent tasks rather than serialized through the accept loop, so the connection-count caps are reserved with a compare-and-swap loop instead of a plain read-then-increment. Only the first execution to register against an idle bridge can choose
bind_port; a later caller's request is logged (truapi.ws_bridge.bind_port_ignored) rather than silently dropped if it differs from the port already running.Downstream compatibility
Checked against a real consumer,
polkadot-ios-community, which already opens two executions (SPA/App and Chat) from oneTrUAPIHostRuntime, the exact multi-execution shape this change targets. ItsstartWsBridge/stopWsBridgecall sites are untouched, so it keeps working unchanged once it picks up a release that includes this PR:Verification
cargo build --workspacecargo +nightly fmt --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspacecargo build -p truapi-server --target wasm32-unknown-unknown --no-default-featuresmake uniffi && ./ios/truapi-host/scripts/sync-bindings.sh --checkLive verification
Running
polkadot-ios-communityagainst this branch in the iOS Simulator, the TrUAPI Playground opens throughNativeProductExecution.startWsBridge()and renders inside the app.