Skip to content

feat(truapi-provider): tell a listener which way its connection closed - #461

Merged
TarikGul merged 4 commits into
mainfrom
fix/chain-close-reason
Aug 20, 2026
Merged

feat(truapi-provider): tell a listener which way its connection closed#461
TarikGul merged 4 commits into
mainfrom
fix/chain-close-reason

Conversation

@decrypto21

@decrypto21 decrypto21 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add ChainCloseReason, with StreamEnded and ListenerFailed { reason }
  • ChainMessageListener::on_closed takes it, so a host is told which way its connection ended rather than inferring it
  • carry the listener's own error text into ListenerFailed, bounded to 256 Unicode scalar values on the path that reaches a host
  • refuse connect from a listener callback with a typed error, instead of panicking inside the nested executor
  • hold that refusal across the listener's own drop, so a foreign destructor cannot slip past it
  • start the response pump on a named, fallibly spawned thread, so a spawn failure is the Result connect already has
  • render a close reason straight into a capped buffer, rather than materializing the whole foreign string first
  • pin the light-backed catalog that the StreamEnded contract depends on
  • regenerate the committed TrUAPIProvider Swift bindings and update both host guides

Why a bare on_closed is not enough

pump_responses calls on_closed on two paths that mean different things:

  • the response stream ended, which on the light-client path means the connection was closed, usually by this host
  • the host's own on_message returned an error, so the connection was closed under it and pumping stopped

A 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 a tracing::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, and connect blocks again. futures refuses to nest executors and panics, and panic = "abort" on the shipping profile turns that into a dead app. Since deciding whether to reconnect is exactly what this enum is for, connect now checks and returns ChainProviderError::Connect there.

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.

disconnect is safe from either callback. send is safe from on_message, where the connection is still open, but not from on_closed: by then the connection is closed on both paths, and send on 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 last Arc reference to it was released with the flag already false. That release runs the host's own destructor, a Swift deinit or a Kotlin cleaner, on the pump thread while block_on is 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 PumpGuard restores the previous value rather than clearing it, so the guard held inside pump_responses cannot 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 main today, independently of this change: on_closed() already exists, the pump is already thread::spawn(move || block_on(..)), connect already calls block_on again, and From<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 editing ffi.rs and 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_display renders into a buffer that stops accepting characters at the bound instead. Same output, no peak.

bounded_reason stays on the From<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 default and else guidance is source compatibility only, because adding a variant later does not move the on_closed checksum, 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 .so have to be regenerated together. And provider-swift-check now diffs the generated modulemap, which it previously skipped, alongside the .swift and the header.

Breaking

ChainMessageListener::on_closed gains a parameter. Every conformer must take reason: 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.0 is published with an xcframework asset, and Package.swift pins that URL and checksum as the default binary target, so a consumer who does not set TRUAPI_PROVIDER_USE_LOCAL_BINARY=1 resolves a binary whose on_closed has the old arity. iOS catches that: the checksum guard runs before the callback vtable is installed, so it is a fatalError at first use rather than a wrong-slot read. Merging this wants an ios-provider republish, which is a manual step because release.yml's allowlist covers only truapi, truapi-host and ios-host.

Android is not exposed the same way, because the AAR bundles bindings and .so together, and the Maven publication is local-only. The npm @parity/truapi-provider builds are wasm, where mod ffi is compiled out entirely.

Two notes for whoever adopts it. Adding a ChainCloseReason variant later will not move the on_closed checksum, 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 as on_closed never firing. The @unknown default and else branches 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 .so paired with fresh bindings is not detected there; iOS checks it before installing the callback vtable.

Validation

  • cargo test --workspace --all-features: 986 passed
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo +nightly clippy for truapi-provider on wasm32-unknown-unknown and on the native uniffi feature, both with -D warnings
  • cargo +nightly fmt --check
  • ./scripts/codegen.sh with no tracked-file drift
  • cargo doc with -D rustdoc::broken_intra_doc_links
  • make provider-swift-check: the committed bindings match a fresh generation, now including the generated modulemap
  • scripts/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 prerequisite

Nine tests carry the behaviour, each mutation-checked against the defect it describes:

  • a listener failing on the second of three messages closes with ListenerFailed { reason: "cannot decode" }, and the connection is closed
  • a stream ending on its own closes with StreamEnded, and the pump leaves the handle alone, since dropping it belongs to the caller
  • a connection whose owner has already dropped it still reports the listener that failed, rather than falling back to the variant a host reads as ordinary
  • on_closed fires exactly once, and after any close the pump itself performed
  • an over-long reason arrives at exactly 256 scalar values. The fixture is a marked prefix followed by astral characters, so a bound written in bytes splits one and panics, a bound that kept the tail loses the prefix, and a bound counted in UTF-16 code units lands on half as many scalars
  • an undeclared foreign error converts to a rejection rather than panicking, and is bounded on that path too. Replacing that impl with a panic! used to pass the whole suite
  • connect from inside on_closed returns an error; remove the guard and the test panics inside futures-executor instead
  • a guard nested inside another does not clear the outer one's flag; restore the unconditional clear and the test fails
  • every bundled chain resolves to a light-client source, which is what makes StreamEnded mean a close rather than a dropped socket; point one catalog entry at a ws node and the test names it

Notes

The generated Swift surface is a plain enum, so a host switches on it directly:

public enum ChainCloseReason: Equatable, Hashable {
    case streamEnded
    case listenerFailed(reason: String)
}

open func onClosed(reason: ChainCloseReason) throws

ListenerFailed carries 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.

StreamEnded does 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 by close(), so the merged stream cannot end before a close. In practice StreamEnded is the host's own disconnect() or dropped handle arriving back. A listener that rejects an in-flight frame while shutting down reports ListenerFailed even though the host asked for the teardown, because on_message blocks in foreign code and a disconnect() can land while a frame is already in flight.

Refs #463

@decrypto21
decrypto21 requested a review from a team August 20, 2026 08:54
@TarikGul
TarikGul added this pull request to the merge queue Aug 20, 2026
Merged via the queue into main with commit 676461e Aug 20, 2026
18 checks passed
@TarikGul
TarikGul deleted the fix/chain-close-reason branch August 20, 2026 13:57
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.

2 participants