chore: clear the ground for the wire-mesh migration (P0) - #54
Merged
Conversation
Every real bridge already calls setTransport(new TlsTransport(...)) immediately after construction; the TcpTransport default existed only as a fallback nothing in production ever used, and constructing it unconditionally in the constructor is exactly what stood between tcp-transport.ts and deletion. transport is now MeshTransport | undefined, read through a single requireTransport() accessor that throws a clear error if setTransport() was never called, rather than every one of the ~25 call sites getting its own ad hoc null check. Every test that constructed a bare MeshStore and relied on the removed default now explicitly wires a real TlsTransport via a shared wireTestTransport() helper (or, for mesh-smoke's spawned child-process script, the equivalent three lines inline) -- including setting peerId to the generated identity's own certificate fingerprint, which TlsTransport's cert-pinning trust model requires the two to agree on.
become-coordinator-actual-port.integration.test.ts drops its TcpTransport scenario along with the transport itself, keeping the TlsTransport one that already covered the real #42 regression. listener-policy.integration.test.ts's "non-default listener carries policy" test wrote a plaintext introduce message directly over a raw net.Socket -- work-alike against a plaintext TcpTransport listener, but the listener now speaks TLS and never gets to the plaintext bytes at all. Replaced with a real tls.connect() presenting a freshly generated identity's own certificate, claiming that identity's fingerprint as the introduced peerId (TlsTransport verifies the two match). That rewrite surfaced a genuine, independent bug in the test's own onIntroduction monkey-patch, invisible until now because this suite was never wired into CI: the "call the original handler" wrapper looked up transport.events.onIntroduction again at call time instead of capturing the pre-replacement function value, so calling it invoked the wrapper itself and recursed until the stack overflowed. Captured (bound) once before the replacement instead.
Three of the largest substrate integration suites were never included in the test script, so real regressions in connection approval, multi-listener policy, and the end-to-end multi-process mesh went unnoticed -- including the recursion bug the previous commit fixes, which had been sitting in listener-policy's own test code the whole time this suite wasn't running.
Listener management, federation, and connection approval are transport concerns FileStore can never genuinely support -- it has no network transport to manage listeners on, federate through, or approve inbound connections for. The interface used to claim universal support anyway, with FileStore implementing all seventeen methods as always-throwing or empty-return stubs purely to satisfy the type checker; that's exactly what forced server.ts and the bridge controller to type their own store references as the concrete MeshStore instead of CommsStore. CommsTool is the one real consumer that needs these when a MeshStore backs it, so it takes them as an optional extension (MeshOnlyFeatures) rather than the shared interface pretending every implementation has them. Each call site now checks the method's presence and reports an ordinary CommsResult error when it's missing, replacing the throw FileStore's own stub used to produce with the same outcome one layer higher, where the caller actually knows what "not supported" should look like to whoever is asking.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
send(), acceptConnection(), and rejectConnection() awaited a raw write directly, so a peer disconnecting (or this side's own shutdown() destroying the socket) while a write was in flight rejected past whatever called them. acceptConnection() in particular fires onIntroduction synchronously after its own write, kicking off MeshStore's state-sync send to the newly accepted peer -- work that isn't awaited by acceptConnection()'s own caller, so a failure in it became a process-level unhandled rejection rather than surfacing anywhere a caller could react to it. broadcast() and flushPending() already treated a failed write as an ordinary, expected outcome (the socket's own close/error listeners handle cleanup) rather than a program error; the other three call sites now share that same writeBestEffort() behaviour instead of each inventing its own handling, or lacking any.
…waits Wiring approval/listener-policy/mesh-smoke into CI (previous commit) surfaced a genuine CI-only failure: the accept-flow tests' fixed sleep(300)/sleep(500) waits were tuned against a fast local machine and didn't hold up under a slower, more contended CI runner, so two tests failed intermittently on the actual peer_list -> connectToPeer -> state_sync -> handlePeerConnected round trip taking longer than the guessed delay. Replaced with waitFor(), which polls the real condition (the mesh state each test is actually asserting on) until it holds or a generous bound elapses, rather than assuming any fixed number of milliseconds is enough. Every approval test now also wraps its body in try/finally around shutdown() -- a test that failed before reaching its own unprotected shutdown() call left real TLS listeners and connections running, which is exactly what turned one flaky assertion into a CI job hanging for the better part of an hour with no output. mesh-smoke's spawned child processes get the equivalent protection: a bounded watchdog timeout that kills a hung child and rejects with a clear reason instead of Promise.all() waiting on its exit forever. The CI workflow itself gets a 10-minute cap on the Test job as the last line of defence, so a future hang like this one fails loudly in minutes instead of running for hours with no signal.
A real CI run timed out at 5s specifically on the second connection direction of the accept flow (B dialling A after receiving A's peer_list, a genuinely separate TLS handshake sequenced after the first one completes) -- confirmed not a logic gap, since the same underlying mechanism already passes reliably both locally and in mesh-e2e's own CI run. waitFor returns the instant its condition holds, so a longer ceiling costs nothing on the happy path and only matters for exactly this worst case.
MeshStore's own events getter never implemented TransportEvents.onError on the object it returns, so every this.events.onError?.() call inside TlsTransport (and MeshStore's own init()) was silently a no-op -- including a connection rejected for presenting a certificate that doesn't match its claimed peer ID, and this store's own inability to join or create a mesh. Neither ever had anywhere to surface. Added a public onError property, matching the existing onDelivery/onPatch pattern, wired into the events getter. wireTestTransport() now also attaches a diagnostic error logger by default -- surfacing exactly the class of failure this fix makes observable, to find the real cause of a CI-only accept-flow failure that produces no error locally.
connectToPeer's error handler caught a failed dial (ECONNREFUSED, ECONNRESET, etc) and resolved silently -- correct for the case where another connection attempt to the same peer has already succeeded, but previously gave zero signal either way. Now reports through the onError path the previous commit made real, to find the actual cause of a CI-only accept-flow failure this investigation hasn't yet explained.
… list The peer list a coordinator sends always includes the receiving peer's own entry, so handlePeerList/handlePeerJoined would call connectToPeer against the store's own peer ID — a wasted attempt that, on some platforms, self-inflicts an immediate ECONNRESET.
A connectToPeer() dial that hadn't yet resolved into peerConnections (or already failed) was never tracked anywhere, so shutdown() had no way to cancel it. The socket kept running in the background, retrying and erroring until it eventually landed in a callback whose enclosing transport was long gone, across a test suite's file boundaries within a single node --test process — starving later tests of resources under CI's tighter constraints. Track every outbound dial in a pendingConnectSockets set from the moment tls.connect() returns, remove it once the connection resolves or fails, and have shutdown() destroy whatever is left in the set. connectToPeer() also now bails out early (and skips reporting an onError for a race it caused itself) once shutDown is already true. Also drop the "(temporary)" framing from the transport-error logger in the shared test helper now that the root cause above is fixed: its console.error was genuinely useful diagnostic infrastructure worth keeping, not a throwaway probe.
The connector-sees-coordinator direction of the connection-approval accept flow times out reliably in CI while passing every time locally, with no error or close event ever firing on the transport for the missing connection. Add temporary tracing at handlePeerList (what a peer_list actually contained) and connectToPeer's entry and connect callback, so the next CI run shows whether the dial back to the introducing peer is even attempted, and if so, where it stalls.
…und one Mesh formation between two peers relies on establishing a connection in each direction, since a peer only pushes its own state to whoever accepts its connection, never to whoever it dials out to itself. Both directions share peerConnections as their address book, so once an inbound dial from a peer was identified via pong, connectToPeer's own "already connected, don't dial again" guard treated that inbound entry as proof the outbound dial to the same peer ID was redundant and returned immediately, permanently starving that peer of the other side's state. Track outbound dials in their own set, checked only by connectToPeer's guard, so an accepted inbound connection can never suppress a needed outbound one. Because a peer can now legitimately hold both an inbound and an outbound socket for the same peer ID at once, make each connection's own disconnect handler check its socket is still the one peerConnections currently holds before deleting that entry -- otherwise a stale close on one socket could wipe out the other, still-live one. This is a pre-existing race, not something introduced by this branch's own changes: locally the outbound dial reliably starts before the inbound one is identified, so the guard was never actually hit: CI's different scheduling exposed the opposite ordering consistently, which is what the connection-approval accept-flow tests were timing out on.
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
1 participant
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.
Closes #46.
The first phase of the wire-mesh migration (#45): delete what the migration doesn't need, fix a real interface leak, and get the substrate's real test coverage actually running in CI before anything moves.
Remove MeshStore's implicit TcpTransport default
Every real bridge already calls
setTransport(new TlsTransport(...))immediately after construction -- the TcpTransport default existed only as a fallback nothing in production ever used.transportis nowMeshTransport | undefined, read through a singlerequireTransport()accessor that throws a clear error ifsetTransport()was never called, rather than ~25 call sites each getting their own null check.Every test that relied on the removed default now explicitly wires a real TlsTransport (a shared
wireTestTransport()helper, or the equivalent three lines inline for mesh-smoke's spawned child-process script) -- including settingpeerIdto the generated identity's own certificate fingerprint, which TlsTransport's cert-pinning trust model requires the two to agree on.Delete tcp-transport.ts and ws-transport.ts
821 and 782 lines with no real production consumer once the default above is gone -- every bridge already replaces it with TlsTransport.
ws-broadcast-window.integration.test.tsgoes withws-transport.ts; its real coverage (the broadcast queue) is already mirrored for TlsTransport inbroadcast-window.integration.test.ts.become-coordinator-actual-port.integration.test.tskeeps its TLS scenario (the real #42 regression) and drops only the now-dead TCP one.Run approval, listener-policy, and mesh-smoke in CI
Three of the largest substrate suites were never in the test script. Wiring them up surfaced two genuine, previously-invisible bugs:
listener-policy's ownonIntroductionmonkey-patch looked uptransport.events.onIntroductionagain at call time instead of capturing the pre-replacement function value, so its "call the original handler" line called the wrapper itself and recursed until the stack overflowed. Fixed by capturing (bound) once before the replacement. That same test's raw-socket probe needed rewriting from a plaintextnet.Socketwrite to a realtls.connect()presenting a generated identity's own certificate, since the listener it's probing is TlsTransport-backed and never gets to plaintext bytes at all.sleep(300)/sleep(500)waits weren't long enough for the realpeer_list -> connectToPeer -> state_sync -> handlePeerConnectedround trip, and a test that failed before reaching its own unprotectedshutdown()call left real TLS listeners running -- which is what turned one flaky assertion into a CI job hanging for the better part of an hour with no output. Fixed by polling the actual condition (waitFor()) instead of guessing a delay, wrapping every test body intry/finallyaround shutdown, and giving mesh-smoke's spawned child processes a bounded watchdog timeout. The CI workflow's Test job also gets a 10-minute cap as a last line of defence.Chasing that CI-only failure down to its root also surfaced a real, independent production bug:
TlsTransport.send()/acceptConnection()/rejectConnection()awaited a raw socket write with no guard against the peer disconnecting (or this side's ownshutdown()) destroying the socket mid-write, unlikebroadcast()/flushPending(), which already treat that as an ordinary, expected outcome.acceptConnection()'s write specifically firesonIntroductionsynchronously afterward, kicking offMeshStore's own state-sync send as unawaited background work -- a failure there became a process-level unhandled rejection with no caller able to react to it. All three now share the same best-effort write handling the other two already had.Narrow CommsStore
Listener management, federation, and connection approval are transport concerns FileStore can never genuinely support. The interface used to claim universal support anyway, with FileStore implementing all seventeen methods as always-throwing or empty-return stubs purely to satisfy the type checker -- exactly what forced
server.tsand the bridge controller to type their own store references as the concreteMeshStoreinstead ofCommsStorein the first place.CommsTool, the one consumer that genuinely needs these when a MeshStore backs it, now takes them as an optional extension (MeshOnlyFeatures), with each call site checking presence and reporting an ordinaryCommsResulterror when missing.@exadev/wire-mesh-coreis already a git-subdirectory dependency onmain(src/core/handshake.ts, #31) and already typechecks/tests cleanly -- confirmed, not something this PR needed to add.Test plan
pnpm build && tsc --noEmit && eslint .-- all cleanpnpm test(64 tests, all three newly-wired suites included) -- green across 3 consecutive full runs, no flakespnpm test:federation && pnpm test:visibility && pnpm test:delivery-- all greenlistener-policyrecursion bug is real by reverting the fix and watching it stack-overflow, then restoring it