Repository navigation
fix(messagebox): keep live sockets and room subscriptions across reconnects, push unpaid HTTP sends - #739
Open
kjartan221 wants to merge 28 commits into
Open
fix(messagebox): keep live sockets and room subscriptions across reconnects, push unpaid HTTP sends#739kjartan221 wants to merge 28 commits into
kjartan221 wants to merge 28 commits into
Conversation
…today Characterisation tests, not fixes. Each asserts what the client does now and is expected to fail when the defect it names is fixed; that failure is the signal. A room is never rejoined after disconnectWebSocket, because the set that records membership is not cleared and the joinRoom guard then matches on the next listen. The socket is live, the client believes it is subscribed, and the server never put it in the room. No relay restart or network is involved. A dropped socket is orphaned rather than disposed: the reference is cleared and disconnect() is never called on it, so Socket.IO keeps reconnecting an object nothing is watching. The mock collects every handler per event rather than the last, because the client registers more than one disconnect listener and firing only the latest exercises the wrong one.
off() now actually detaches, and every handler for an event is fired rather than the last registered. Storing only the latest meant the test called an auth-phase disconnect handler that flips a flag and never touches the socket, so it simulated no disconnect at all and reported the defect absent. While fixing it: three disconnect listeners are attached after authentication and none are detached. Worth knowing for the fix work, since the socket these accumulate on is the one that gets replaced.
…embership A disconnect cleared this.socket without stopping the socket. Socket.IO reconnects by default, so that object kept reconnecting unwatched, and its handlers still read this.socket: once a replacement existed, the orphan authenticated on it and its next disconnect cleared it. joinedRooms outlived the socket that joined, so the guard in joinRoom matched and a new socket never emitted a join. Connected, believed subscribed, and never put in the room by the server. disconnectWebSocket left the same stale claim, which reproduces it without a relay restart: listen, disconnect, listen. leaveRoom dropped the room after its no-socket guard, so leaving while disconnected left the claim in place. It now drops first. Rejoining rooms on a reconnect is deliberately not here: eight call sites treat a null socket as no connection, so keeping the socket across a transient drop is its own change.
…cription An unrequested drop keeps the socket object; the connect handler re-authenticates and rejoins the rooms the consumer asked for, tracked in requestedRooms separately from the per-socket joinedRooms. A reconnect registers in connectionInitPromise so concurrent callers await it. disconnect() is back to disconnectWebSocket() only. The auth wait is settled by the socket's long-lived handlers, so no listeners accumulate.
Live-message handlers are held per room and re-attached when a socket is rebuilt after a terminal disconnect, so a restored room is not silent. A wait that times out with the socket still down marks it stale and the next caller rebuilds, covering host configs that disable or exhaust Socket.IO reconnection. Tests now hand out a distinct production-shaped mock (no off) per construction.
…, honour reconnection:false Socket.IO never retries io server disconnect and a listener makes no calls to trigger the lazy rebuild, so the client rebuilds once on its own, only for a socket that had authenticated. With managerOptions.reconnection false a drop disposes the socket and the next call rebuilds; nothing rebuilds proactively. listenForLiveMessages no longer attaches a callback a rebuild already restored.
The WebSocket sendMessage handler announced a stored message to the recipient's joined sockets; the HTTP route never did, so an HTTP send reached recipients only through polling. Move the announcement into deliverLiveMessage and call it from both paths. HTTP reaches the socket registry through a LiveDelivery handle on the context, null until sockets are attached or after they close.
Require ctx.liveDelivery when mounting the send route instead of silently skipping delivery, refuse a second attach on one context, and only clear the handle when it still points at the closing registry. Tests now wait out a quiet period after the first push so a double emit is counted, for both the HTTP and WebSocket send paths, and the disabled case runs against a live joined recipient.
A recipient fee is stored on the message row as { message, payment }, but
the live push carries the request body, which has no payment in it. The
live handler only decrypts; internalization happens in listMessages. So a
consumer that acknowledges on the push deletes the row before the
recipient wallet ever sees the output, and the fee is lost.
Paid sends now stay poll-only, which is what they were before this branch
started pushing HTTP sends at all. The socket send path never forwards a
payment, so it cannot reach a paying recipient either way.
Reverting each fix one at a time left four passing: the handler identity guards, the connected check before reusing a socket, leaveRoom's delete ahead of its socket guard, and the two liveDelivery status guards. The first of those is the orphaned socket, which is the defect this branch exists for, so it was shipping on faith. The mock now fires `disconnect` from `disconnect()` as Socket.IO does, which is what the orphan test needs to replace a socket and then have the old one report on its own handlers. Still unkillable, and left alone: disconnectWebSocket detaching before it disconnects, which under faithful semantics reaches the same state either way, and liveDelivery's status/success checks, which the results-array check already covers.
The branch changes @bsv/message-box-client's published bytes, so the governance entry and the package changelog have to say what changed and what it means for a consumer. The migration decision is: nothing to do, with two behaviour notes for anyone who worked around the old defects — getJoinedRooms() is no longer a lever, and a dropped socket is kept. The entry also claimed the pending release had no source change, which stopped being true here. The ledger is regenerated from those notes. Note that regenerating it on Windows needs a separate one-line fix to the generator, which is not this branch's business and travels on its own.
Nothing runs the branch client against a real server: infra pins the published @bsv/message-box-client, as every infra package pins published first-party versions, and switching that one to workspace:* for test convenience would break a convention the whole directory keeps. The opt-in live suite is the seam the repo already provides for this, so the rejoin-after-disconnect case goes there. Unrun here: it needs MESSAGE_BOX_RUN_INTEGRATION=true, a host and a json-api wallet. The deterministic twin is in the defect suite and is mutation-verified; this one is the acceptance check against a real relay.
The server has always answered joinRoom with joinedRoom or joinFailed and the client has never listened, so a refused join left it certain it was subscribed to a room it had never been given. joinFailed now also names the room: a client can have several joins in flight, and without the room the event cannot be attributed to any of them. The record stays optimistic on emit, so a server that answers nothing behaves exactly as before. An answer only corrects it: a refusal drops the claim, and one that re-emitting can never clear — a room owned by another identity, an invalid room — also stops being asked for on every reconnect. The room's message handler stays attached either way, since nothing is pushed to a room we are not in and dropping it would let a later listen attach a second one. The HTTP announce fanned out with Promise.all over the recipient list. One send reaches up to MAX_RECIPIENTS recipients on up to WEBSOCKET_MAX_RECIPIENT_CONNECTIONS sockets each, so a standard profile could issue 2500 emits off one request while the notification fan-out over the same list, in the same function, was bounded to four. It now uses that same bound.
Two client defects left from the review. A room's listener could never be taken off the socket, because the wrapper has no `off`. leaveRoom dropped its record and a later listen attached a second listener, so every message after that arrived twice for the life of the socket. There is now one listener per room per socket that reads its subscribers when a message arrives, which makes subscribing and unsubscribing list operations. It also reads the body once rather than once per subscriber, which matters because reading it rewrites `message.body` in place. That does change one thing for a caller: delivery is now always a tick after arrival. It already was for an encrypted message, since decrypting awaited; the unencrypted path happened to run straight through. The existing listen test asserted synchronously and now waits a tick — same assertion, one await. The rebuild after `io server disconnect` had no delay, so a relay that authenticated a socket and dropped it got one rebuild per authentication for as long as it kept doing that. The first rebuild is still immediate and the rest double to a thirty-second cap, resetting once a socket has held its authentication that long. A deliberate close cancels one in flight, which it previously could not.
…the notes Subscribers share one room listener since the last commit, so a throwing one took the rest of the room's message with it and left an unhandled rejection. 2.5.4 isolated them by giving each its own listener, so this was a regression; each is now called in its own try/catch. The acknowledgement path had the leak the room path just lost. Every sendLiveMessage attached a `sendMessageAck-<room>` listener and removed it through `typeof socketAny?.off === 'function'`, which is always false because AuthSocketClient has no `off`: five sends left five listeners, and every acknowledgement walked all of them. One listener per room now, with sends queued behind it. A refusal carries no messageId, so the oldest send in flight still takes the acknowledgement, which is what the per-send listeners did — the settled ones returned at once and the first unsettled one took it. The hardening test waited for a new listener per send, which was the leak restated as an expectation. It now waits for the send to be queued; both assertions are unchanged. The live WebSocket test never awaited its listen, so it read the joined rooms before the join had happened. It also claimed to be a reconnect when a deliberate close is terminal by design, so it is named for what it does prove: a socket built after a close joins its room for itself. Release text: the two rebuild descriptions are now one and state the reset rule as the code implements it, including that a relay dropping sockets just outside the window still gets an immediate rebuild. Reading the join answer only corrects anything against a server that names the room, which is said where it was previously implied by omission, and "nothing else changes for a caller" is gone.
The AsyncAPI spec described joinFailed as reason-only and said the room broadcast followed a socket send. Both are now wrong: the refusal names the room it refused and carries a code, and the broadcast also follows an HTTP store. Where it does not follow one is the part worth writing down — only this process's sockets, never a paid send, never a refused send — so that goes on the channel and on the HTTP route that now triggers it. Both specs take a minor bump; every change is additive. The client README said reconnection settings "remain supported" without saying what honouring reconnection: false costs a pure subscriber, which is that nothing brings it back. It now says so, next to a section on what the socket does on its own and the two things that follow for a caller: keep polling, and getJoinedRooms is reporting rather than a lever. The server README describes the join answer and the announcement on the same terms, and names the concurrency limit the fan-out borrows. JSDoc on joinRoom, listenForLiveMessages and leaveRoom: resolving a join means the request went out, not that it was granted; subscribers to a room share one listener, so the body is read once and delivery is a tick late; leaving releases the room whether or not the socket is up.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Sonar put initializeConnection at cognitive complexity 17 against a limit of 15, and oxlint had it at cyclomatic 23 against 20. It had grown a socket construction, seven inline handlers and their shared closure state inside one `if (this.socket == null)` block. Construction moves to openSocket, wiring to attachSocketHandlers, and each handler to a method of its own. The guard that each repeats — return if this is no longer the client's socket — reads as the rule it is rather than as seven copies of a line inside one function. The `wasAuthenticated` closure that authenticationSuccess shared with disconnect becomes an authenticatedSockets WeakSet, since the two handlers no longer share a scope. Same rule: only a socket that authenticated is rebuilt. No behaviour change. All twelve client mutations still turn a test red, including the two this touches most — the handler identity guards and rebuilding only an authenticated socket.
Codecov puts the patch at 86.74% against a 90% target. These are the reachable misses in the client: a join the server confirms, a body whose ciphertext is not a string, and a socket-level error event. Each is a behaviour worth pinning rather than a line worth touching — a confirmation restores a room a refusal had dropped, an unreadable body reaches the subscriber as an error instead of vanishing, and an error event does not take the socket with it. This file's patch coverage goes from 97.50% to 97.93%. It will not move the Codecov verdict on its own: the aggregate lane collects `packages/*/coverage/lcov.info` only, so every line added under `infra/` is counted as a miss no matter how well the server is tested.
onLiveStatus(listener) reports connecting, live with the rooms joined on the socket, reconnecting when something will restore it, or closed when nothing will. It calls the listener once with the current status, returns a function that unsubscribes, and `liveStatus` reads the same value without subscribing. Listeners are isolated from each other, same as the message subscribers. Until now a consumer could only learn the socket's state by sending and watching what happened, which is why group-messaging kept a self-addressed ping: not for the ping, but to manufacture the traffic the answer was hidden in. This is the cheap half of that — no wire change, no server work. It does not replace the echo idea, which is parked: status says what this client knows, not that the room still routes. A new public method makes this a minor, so 2.5.5 becomes 2.6.0 and the governance entry moves from patch to minor. Also covers nine of the eighteen partial branches Codecov counted against the patch — two rooms on one socket, an orphan's room traffic, a missing identity key at connect and at initialize, and a conflicting identity pin.
The bump moved package.json and the release ledger and left three behind: the repository-health baselines, the docs page frontmatter, and the generated stack facts. `calendar expiry is advisory in source CI and enforced by the maintenance audit` is the test that catches it, and the whole repository-health suite passes now. stack-facts.md is regenerated, not edited. I had run prettier across it with the rest of the changed files, which rewrote 75 lines the generator owns; upstream's own copy is not prettier-clean either, so that was my mistake rather than a latent one. Both generated artifacts verify with --check. The infra package-lock files still name the published 2.5.4/2.5.5, which is correct: they consume the registry, not this working tree.
Picks up the mutation-scope classifier fix (0b7dff8). No overlap with this branch: it touches scripts/ci-affected-scope.mjs, its test, and docs/reference/ci-performance.md.
Resolve the release-metadata conflicts with bsv-blockchain#629 (published as 2.6.0) and bsv-blockchain#1243 (unpublished 2.6.1, SDK4 peer range): release the live-socket work as 2.7.0, a minor from published 2.6.0, keep both CHANGELOG sections and both release-note passages, and regenerate the package API migrations and stack facts.
…ce pin conflicts Cap room joins awaiting the server's answer at 16 per socket and queue the rest, freed by joinedRoom, joinFailed or a 5 s timeout. The server's authsocket transport drops a socket with more than 32 signed messages in process, so a client on more than 32 boxes re-emitted every join after each authentication and was dropped in a loop without ever holding a room. Report a new live status when a join is refused, so liveStatus.rooms no longer lists a room that getJoinedRooms() has dropped. sendLiveMessage falls back to HTTP when the socket cannot be established, authenticated or joined, but rethrows a conflicting WebSocket identity pin, which is a misconfiguration the fallback would hide. Also tidy docs, types and tests after review: restore PeerMessage's JSDoc, define "joined" once, align the AsyncAPI and server README with the code, and replace real sleeps in the live-socket tests with fake timers.
…rst was in flight
kjartan221
marked this pull request as ready for review
October 21, 2026 15:40
kjartan221
requested review from
BraydenLangley,
tonesnotes and
ty-everett
as code owners
October 21, 2026 15:40
Resolve docs/reference/package-api-migrations.md by regenerating it with scripts/package-documentation.mjs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve docs/reference/package-api-migrations.md by regenerating it with scripts/package-documentation.mjs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This branch has not been deployed
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
2 participants
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.
MessageBoxClient discarded a socket at the moment Socket.IO began reconnecting
it. The orphan kept reconnecting, re-sent
authenticateddown whichever sockethad replaced it, and cleared that healthy socket on its own next drop. Room
membership was recorded as intent and never reconciled, so a replacement socket
inherited a claim it had never made and was pushed nothing. Separately, the
server pushed a stored message to a recipient's room only when the sender had
used a socket, so a message sent over HTTP reached nobody live.
Program and scope
@bsv/group-messaging, whichcarried ~300 lines of transport machinery working around this behaviour.
authenticated and certain it was subscribed while receiving nothing, with no
supported way to detect or recover from it.
infra/message-box-serverat the workspace client instead of the publishedone; the POSIX-separator fix to
scripts/package-documentation.mjs, heldback for maintainers since it is repo tooling rather than package code.
240a614649a0e4bc91caa0f7b5c73fffc7c5699fClient
cannot act on its successor.
current socket, and re-emitted after each authentication. Membership is
cleared on a drop because it belongs to the server-side socket.
joinedRoomandjoinFailedare read. The record stays optimistic on emit,so a server that answers nothing behaves as before; an answer corrects it.
in place of listeners accumulated per subscription and per send that the
socket wrapper has no
offto remove.io server disconnecttriggers a rebuild: immediate, then doubling from 1sto a 30s cap.
reconnection: falseis honoured by disposing.leaveRoomreleases its room whether or not the socket is up.onLiveStatus(listener)/liveStatusreport what the socket is doing:connecting,livewith the rooms joined on it,reconnecting, orclosed. A refused join reports a new status, so its room leavesrooms.by
joinedRoom,joinFailedor a 5 s timeout, the rest queued. The server'sauthsocket transport drops a socket with more than 32 signed messages in
process, so a client on more than 32 boxes used to be dropped after every
authentication. Leaving a room frees its slot, so joining it again asks again.
sendLiveMessagefalls back to HTTP when the socket cannot be established,authenticated or joined (for example right after a server restart) instead
of throwing. A conflicting WebSocket identity pin still throws.
Server
sendMessage-<roomId>from the HTTP sendroute, as the socket route already did.
the request body, which holds no payment, while the stored row holds
{ message, payment }; a recipient acknowledging on the announcement woulddelete the row before its wallet internalized the output, and only
listMessagesinternalizes.joinFailednames the refused room and carries a code.applies to notifications over the same recipient list.
Impact
New public API:
onLiveStatus(), theliveStatusgetter and theLiveStatus/LiveConnectionStatetypes, all additive. The third box isalso ticked for observable behaviour: a live message is delivered a tick after arrival rather than
synchronously (already so whenever it was encrypted);
getJoinedRooms()reports rather than acts, so mutating the returned set no longer cancels a
rejoin; a dropped socket is kept while Socket.IO reconnects. All three are in
the package CHANGELOG and the release note.
The fifth is ticked for two boundaries: recipient-fee payment handling on the
new push path, and the room-ownership refusal the client now acts on.
Affected packages/services and intended patch versions:
@bsv/message-box-client2.6.0 → 2.7.0 (minor: newonLiveStatus()/liveStatus, additive behaviour fixes).mainhas since published 2.6.0(fix(message-box-client): keep list-API payments the wallet did not store #629) and carries an unpublished 2.6.1 (feat: @bsv/sdk 4.0 module balances and BRC-207 color permission module #1243, SDK4 peer range); this PR
supersedes both as 2.7.0. Updated:
package.json, the CHANGELOG (new 2.7.0section above 2.6.0),
governance/package-release-notes.json,governance/repository-health/baselines.json, the package docs page,docs/reference/package-api-migrations.mdanddocs/reference/stack-facts.md.message-box-server(infra/, private, not published) — source only.Verification
pnpm --filter @bsv/message-box-client run test— 434 passed, 16 suites (after mergingmain, including fix(message-box-client): keep list-API payments the wallet did not store #629's tests)pnpm --filter @bsv/message-box-client run test:coverage— see belowinfra/message-box-server:npm test— 237 passed, 29 suites;npm run typecheckandnpm run lint— cleanpnpm exec oxlinton the changed files with--deny-warnings— cleanpnpm exec prettier --checkon the changed files — cleanpnpm install --frozen-lockfile --ignore-scriptsafter rebasing — clean12 of 13 client mutations and 4 of 7 server mutations turn a test red. The
three surviving server mutations are the redundant
status === 200andstatus === 'success'guards, which the results-array check alreadycovers, and passing the configured socket cap, which is covered where it
lives in
webSocketConnections.test.ts. The surviving client mutation isdisconnectWebSocketdetaching before it disconnects, which reaches thesame state either way under faithful wrapper semantics. The fixes added
after the live stress test (join pacing, refused-join status, HTTP
fallback, pin rethrow, leave-then-rejoin) were each written test-first and
mutation-checked the same way.
branch's client and a server image built from this branch's
infra/message-box-server, then the published 2.6.0 client and 1.1.53 imagefor comparison, on a clean database:
docker restartwithio server disconnect, leave/rejoin, HTTP announce single and batch, paidsend not announced, refused join and the 128-room limit, a 10-subscriber
soak through kill/restart/kill, 40 and 100 boxes, send right after a
restart): 10/10 on this branch, failing on the published builds.
again, all back in 7.4 s, 452 sent and 452 stored, 0 lost. Published
2.6.0: 0/50 old clients ever received again.
pacing fix: dropped in a loop, 0 rooms held).
The run surfaced three client defects, all fixed in this PR before merge:
joins outrunning the server's in-flight limit, a refused room left in
liveStatus, andsendLiveMessagethrowing after a restart.3f3bef8a0; pending on240a61464.(
docs/specs/authsocket.md).specs/messaging/authsocket-asyncapi.yamlandspecs/messaging/message-box-http.yamlare updated and parse; both take aminor bump, and every change is additive.
@bsv/message-box-clientall files 93.14% statements /89.89% branches / 94.64% lines;
MessageBoxClient.ts92.06% / 87.72% /93.86%, up from 90.72% / 87.15% / 92.30% before this work.
unchanged from base.
pack:checkfails onthe authoring host (Windows) at
spawn pnpm ENOENTinsidescripts/check-package-artifact.mjsbefore it reaches the artifact; thebuild itself succeeds.
test:browserwas not run. Both need hosted CI.new import;
MessageBoxClient.tsandtypes.tstogether are +675 / −212.compatibility, public API, artifacts, dependencies, docs, and operations
scope-based skip is expected and validated by the merge gate
Security and dependencies
audit results were reviewed
(including accepted or false-positive issue states) and zero unreviewed hotspots;
Sonar's aggregate
Quality Gate passedverdict alone is not merge evidenceand removal condition
No manifest, lockfile or workflow is touched by this branch, so the second box
is ticked as reviewed and not applicable. Negative tests cover both changed
boundaries: a paid send is not announced, a refused send is not announced, a
room owned by another identity is refused and named, and a permanent refusal
stops being requested. No temporary exception is added. CodeQL and Sonar need
hosted CI.
Dependency evidence
Not applicable: no manifest, lockfile, container base, action or generator
dependency changed.
Release and operations
controlling program
Release order: the client depends on
@bsv/authsocket-clientthroughworkspace:^, which publishes as^2.1.9frommain(#1243). npm has2.1.8, so
@bsv/authsocket-client2.1.9 must be published before or with@bsv/message-box-client2.7.0.Deployment: the client and the server can ship in either order. Reading the
join answer only corrects the client's record against a server that names the
refused room, which is in this PR; against any other server the client records
joins exactly as it always did. The HTTP announcement starts only once this
server image is deployed, and it is best effort — it reaches only sockets held
by the process that served the request, so
listMessagespolling remainsrequired for correctness. Rollback is a redeploy of the previous image with no
data or wire migration; the recipient falls back to polling, which is current
behaviour.
Documentation updated in this PR:
specs/messaging/authsocket-asyncapi.yaml,specs/messaging/message-box-http.yaml,packages/messaging/message-box-client/README.mdand its CHANGELOG,infra/message-box-server/README.md,docs/packages/messaging/message-box-client.md,governance/package-release-notes.json, the regenerateddocs/reference/package-api-migrations.md, and JSDoc onjoinRoom,listenForLiveMessages,leaveRoomandgetJoinedRooms.Completion evidence
passing checks, resolved alerts, measurements, or an approved exception
guidance are current or concretely not applicable
being handed to another contributor as "complete"
restriction is assumed
Known gaps, stated rather than left to be found
infra/message-box-serverdevDepends on the published@bsv/message-box-client@^2.6.0, as everyinfra/package pins publishedfirst-party versions, so no test in this repository exercises this client
against this server. The live stress test above did, outside the repository.
The opt-in live suite (
MESSAGE_BOX_RUN_INTEGRATION=true) has not been run.length of the next delay, so a relay dropping sockets just outside that
window still gets an immediate rebuild each time. The backoff bites when
drops come faster than the current delay.
twice.