feat(truapi-provider): tell a listener which way its connection closed - #461
Merged
Conversation
TarikGul
approved these changes
Aug 20, 2026
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
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.
Summary
ChainCloseReason, withStreamEndedandListenerFailed { reason }ChainMessageListener::on_closedtakes it, so a host is told which way its connection ended rather than inferring itListenerFailed, bounded to 256 Unicode scalar values on the path that reaches a hostconnectfrom a listener callback with a typed error, instead of panicking inside the nested executorResultconnectalready hasStreamEndedcontract depends onWhy a bare
on_closedis not enoughpump_responsescallson_closedon two paths that mean different things:on_messagereturned an error, so the connection was closed under it and pumping stoppedA host that reconnects on the second walks straight back into the failure that closed the connection, because it was closed precisely because that listener could not handle what arrived. Until now both paths arrived as a bare
on_closed(), and the distinction existed only in atracing::warn!line no host can read.Reconnecting is a different thread, and a serial one
A listener callback runs on the pump thread, which is already inside
block_on, andconnectblocks again.futuresrefuses to nest executors and panics, andpanic = "abort"on the shipping profile turns that into a dead app. Since deciding whether to reconnect is exactly what this enum is for,connectnow checks and returnsChainProviderError::Connectthere.The docs say to reconnect from a serial queue off that thread. Serial matters: the refusal returns an error rather than blocking, so hopping to a concurrent queue lets reconnects run in parallel, and each one costs an OS thread plus a chain-spec parse.
disconnectis safe from either callback.sendis safe fromon_message, where the connection is still open, but not fromon_closed: by then the connection is closed on both paths, andsendon a closed connection returns early with no error and no frame for the request's id, so a consumer correlating by id would wait forever.The guard has to outlive the listener
The refusal is a thread-local set while the pump runs. Set inside
pump_responses, it was dropped one step too early: Rust drops a function's body locals before its parameters, and the listener is a parameter, so the lastArcreference to it was released with the flag already false. That release runs the host's own destructor, a Swiftdeinitor a Kotlin cleaner, on the pump thread whileblock_onis still entered. A destructor that reconnects would have gone straight past the refusal into the panic the guard exists to prevent.The guard is now entered at the spawn site so it outlives everything the future owns, and
PumpGuardrestores the previous value rather than clearing it, so the guard held insidepump_responsescannot clear the outer one when it drops.Three fixes that are not this enum
The reentrancy guard, the fallible spawn and the bounded reason all fix behaviour reachable on
maintoday, independently of this change:on_closed()already exists, the pump is alreadythread::spawn(move || block_on(..)),connectalready callsblock_onagain, andFrom<UnexpectedUniFFICallbackError>already carries a listener's reason with no bound. None of them is a regression introduced here.They are in this PR rather than their own because this is the change that makes a host act inside
on_closed, which is what turns the first one from theoretical into likely. No in-tree caller implements the listener, so landing them separately would relieve no in-repo exposure, and splitting would mean editingffi.rsand both READMEs twice. If the aborts matter to a downstream consumer sooner than this API change does, that is the argument for splitting and I would take it.Bounding without the copy
bounded_reason(error.to_string())rendered the listener's error in full before discarding most of it, so a 16 MB reason peaked around 30.5 MB on the pump thread to produce 1 KB.bounded_displayrenders into a buffer that stops accepting characters at the bound instead. Same output, no peak.bounded_reasonstays on theFrom<UnexpectedUniFFICallbackError>path, where the string is already owned by the time it is bounded and there is no copy to avoid.Three smaller things ride along, all in the same area. Both guides now say the
@unknown defaultandelseguidance is source compatibility only, because adding a variant later does not move theon_closedchecksum, so older bindings pass the integrity check and then fail to decode the reason. The Android guide also says the generated checksum guard is never invoked there, so bindings and.sohave to be regenerated together. Andprovider-swift-checknow diffs the generated modulemap, which it previously skipped, alongside the.swiftand the header.Breaking
ChainMessageListener::on_closedgains a parameter. Every conformer must takereason: ChainCloseReason, and Swift callers switching over it exhaustively will need both cases.Nothing in this repo implements the trait, on either platform, so no in-tree caller changes.
The surface has shipped, though.
@parity/ios-provider@0.7.0is published with an xcframework asset, andPackage.swiftpins that URL and checksum as the default binary target, so a consumer who does not setTRUAPI_PROVIDER_USE_LOCAL_BINARY=1resolves a binary whoseon_closedhas the old arity. iOS catches that: the checksum guard runs before the callback vtable is installed, so it is afatalErrorat first use rather than a wrong-slot read. Merging this wants anios-providerrepublish, which is a manual step becauserelease.yml's allowlist covers onlytruapi,truapi-hostandios-host.Android is not exposed the same way, because the AAR bundles bindings and
.sotogether, and the Maven publication is local-only. The npm@parity/truapi-providerbuilds are wasm, wheremod ffiis compiled out entirely.Two notes for whoever adopts it. Adding a
ChainCloseReasonvariant later will not move theon_closedchecksum, because a uniffi enum's type id is built from its name rather than its variants, so old bindings pass the integrity check and then fail inside the converter, which surfaces ason_closednever firing. The@unknown defaultandelsebranches the guides ask for are source-level forward compatibility, not a runtime guarantee. And on Android the generated checksum guard is never invoked, so a stale.sopaired with fresh bindings is not detected there; iOS checks it before installing the callback vtable.Validation
cargo test --workspace --all-features: 986 passedcargo clippy --workspace --all-targets --all-features -- -D warningscargo +nightly clippyfortruapi-provideronwasm32-unknown-unknownand on the nativeuniffifeature, both with-D warningscargo +nightly fmt --check./scripts/codegen.shwith no tracked-file driftcargo docwith-D rustdoc::broken_intra_doc_linksmake provider-swift-check: the committed bindings match a fresh generation, now including the generated modulemapscripts/battery.sh --chat-host: 9 of 9 cases pass. The signing phase reports 46 of 66 with the same 20 failures as an unrelated branch on the same machine, 13 of them the committed unsupported baseline and 7 the documented personhood-ring prerequisiteNine tests carry the behaviour, each mutation-checked against the defect it describes:
ListenerFailed { reason: "cannot decode" }, and the connection is closedStreamEnded, and the pump leaves the handle alone, since dropping it belongs to the calleron_closedfires exactly once, and after any close the pump itself performedpanic!used to pass the whole suiteconnectfrom insideon_closedreturns an error; remove the guard and the test panics insidefutures-executorinsteadStreamEndedmean a close rather than a dropped socket; point one catalog entry at awsnode and the test names itNotes
The generated Swift surface is a plain enum, so a host switches on it directly:
ListenerFailedcarries the reason for logs and diagnostics, not for control flow: a host that needs to distinguish its own failure modes should track them where it raised them. The bound is 256 Unicode scalar values, which is up to 512 UTF-16 code units and up to 1024 bytes, so a host sizing a buffer or asserting on a length should use the unit its own platform counts in.StreamEndeddoes not mean the peer went away. Every connection this crate hands a host runs on the embedded light client, whose response stream is a merge of smoldot's stream and an error channel that is closed only byclose(), so the merged stream cannot end before a close. In practiceStreamEndedis the host's owndisconnect()or dropped handle arriving back. A listener that rejects an in-flight frame while shutting down reportsListenerFailedeven though the host asked for the teardown, becauseon_messageblocks in foreign code and adisconnect()can land while a frame is already in flight.Refs #463