fix(core): stop dropping broadcasts sent while peer connections are still dialling - #25
Merged
Merged
Conversation
Clearing the sign bit on the random serial number can leave a leading zero byte, which is not a minimal DER INTEGER encoding; OpenSSL rejects such certificates as illegal padding when they are loaded, so tls.createServer failed for roughly one generated identity in 128 and the bridge intermittently could not start. Pin a masked-to-zero first byte to one, and load a batch of generated certificates in the test suite to catch a reintroduction.
…till dialling State patches broadcast in the window between store.init() returning and the TLS data connections registering had nowhere to go and were silently dropped, so a bridge that registers immediately after init() (as every production bridge does through ensureRegistered) could stay invisible in established peers' list_agents, and a restarted bridge with a persisted identity could stay offline on peers that missed its re-activation upsert even though delivery routing to it worked. Two windows caused the loss. connectToCoordinator() resolved once the introduction was sent, before the coordinator answered with the peer list, so the post-join dials had not even started when the first broadcast fired; and broadcasts to a dialling peer were discarded because the dial is fire-and-forget. The handshake now resolves on the peer list (with the existing connect timeout as a degraded fallback) and each transport queues broadcasts for dialling peers, flushing them in order when the connection registers from either side, bounded so a dial that never completes cannot grow the queue unbounded. The restart-continuity test no longer needs its settle-delay workaround, and a regression test pins immediate-registration visibility. Found while testing the persistent-identity work; also exposed an intermittent invalid-certificate bug fixed separately.
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. |
|
🎉 This PR is included in version 1.25.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
Fixes #23.
Two windows caused the loss described in #23, and both needed closing:
The join handshake resolved too early.
connectToCoordinator()resolved once the introduction was sent, before the coordinator answered with the peer list — soinit()returned before the post-join dials had even started, and the firstbroadcastPatch()fired with no connections and no dials under way. The handshake now resolves on the peer list (with the existing connect timeout as a degraded fallback for an unresponsive coordinator), so the dial loops — and their broadcast queues — exist before the first registration.Broadcasts to dialling peers were discarded. The peer dials are fire-and-forget, so messages sent between
connectToPeer()and the connection registering went nowhere. Each transport (TCP, TLS, WS) now queues broadcasts for dialling peers and flushes them in order when the connection registers, from either side of the dial (the dialler on connect, the acceptor on pong), bounded per peer so a dial that never completes cannot grow the queue unbounded.Evidence: instrumented traces showed the coordinator never receiving the joiner's
agent_upsertat all; after both fixes, the same trace shows the upsert arriving within milliseconds, and the restart case converges toactiveunder the same agent ID.Tests:
broadcast-windowintegration test pins the exact State patches broadcast before TLS data connections are established are silently lost #23 repro: a peer registering immediately afterinit()(the production pattern) must be visible to the coordinator with no settle delays.Also fixed here (separate commit): chasing the test flakes this work exposed found that
generateIdentity()intermittently produced certificates OpenSSL rejects (illegal padding) — clearing the serial's sign bit can leave a leading zero byte, a non-minimal DER INTEGER, hitting ~1 in 128 generated identities and makingtls.createServerfail despite its retries. The serial is now pinned to a minimal encoding, with a batch certificate-loading regression test wired intopnpm test(adds <1s).Known boundary, deliberately out of scope:
state_sync's receive path merges add-only, so it cannot repair an entity that changed while a peer was fully disconnected — converging that needs entity versioning and is a separate piece of work. The dial-window loss this PR fixes was the cause of every symptom traced so far.