fix(asset/centrifuge): speak bidirectional Centrifuge protocol (#938)#944
Conversation
…n/websocket (bsv-blockchain#938) The WS transport reported `Unidirectional()=true`, which made the centrifuge core emit the connect reply as a Push envelope (no id, Connect at the wrong nesting). Any bidirectional client (centrifuge-go, centrifuge-js) saw `Reply.Id == 0` and `Reply.Push == nil`, treated it as a server ping, and timed out after ~5s. Fix the transport and the handler: - `Unidirectional()` returns `false`. - Replace the legacy manual `conn.ReadMessage` + custom JSON ConnectRequest + `c.Connect()` + drain loop with the upstream pattern: a `centrifuge.HandleReadFrame(c, reader)` loop over `conn.NextReader()`. - Drop the dead custom `ConnectRequest` / `SubscribeRequest` types and the no-op subs-translation block that ranged over a never-populated map. - Detach cancellation from `r.Context()` via `context.WithoutCancel`: `r.Context()` is cancelled the instant ServeHTTP returns (right after the goroutine launches), and centrifuge's `HandleCommand` checks `<-c.ctx.Done()` on every frame. The old `c.Connect()` path skipped that check, masking the bug. Keep request-scoped values (credentials set by `authMiddleware`) but strip the cancellation. Wire-format consumers updated: - Dashboard `p2pStore.ts`: send `{id:1,connect:{}}` on open instead of `{}`; parse `push.pub.data` (push envelope) instead of unwrapped `pub.data`; ignore empty centrifuge ping replies; drop the dead unwrapped-`json.type` fallback path. - Docs: add bidirectional command-reply and push wire-format examples. Tests: - New `TestWebsocketHandler_BidirectionalConnect` exercises the full handshake against a real `centrifuge.Node`: sends `{id:1,connect:{}}`, asserts the reply carries `id=1`, a non-empty `connect.client`, and the `OnConnecting` subscription. Would have failed on every prior variant (original push-envelope bug, the issue's one-liner with nil-deref panic, and the bidirectional-without-WithoutCancel variant with silent abort). - Flipped the `assert.True(t, transport.Unidirectional())` assertion. The existing integration test at services/p2p/asset_websocket_integration_test.go was already written for the bidirectional wire format and was silently failing against the buggy server; it should turn green against the new one. Refs: bsv-blockchain#938
|
🤖 Claude Code Review Status: Complete Review Summary: No critical issues found. The PR correctly fixes the Centrifuge WebSocket transport to operate bidirectionally and includes comprehensive test coverage. Strengths:
Verified:
Notes:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes the Asset Server’s /connection/websocket Centrifuge transport to correctly operate as bidirectional (protocol v2), restoring compatibility with standard Centrifuge clients (Go/JS) and unblocking affected acceptance tests. It updates the server-side WebSocket read handling to follow Centrifuge’s expected framing/command processing flow, and aligns the dashboard client + documentation with the proper envelope formats.
Changes:
- Updated the Asset Server WebSocket transport to be bidirectional and to process incoming frames via
centrifuge.HandleReadFrame(instead of the legacy unidirectionalc.Connect()path). - Added an end-to-end unit test that validates a real bidirectional connect handshake (reply includes
id,connect.client, andconnect.subs). - Updated the dashboard WebSocket client parsing/sending to use Centrifuge command/reply + push envelope shapes, and documented the wire format.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| services/asset/centrifuge_impl/websocket.go | Switch transport to bidirectional and use Centrifuge frame handling loop; detach request context cancellation for connection lifetime correctness. |
| services/asset/centrifuge_impl/centrifuge_test.go | Update transport assertion and add an end-to-end bidirectional connect handshake test. |
| ui/dashboard/src/internal/stores/p2pStore.ts | Send a proper connect command on open; parse publications from push.pub.data; ignore empty ping replies. |
| docs/topics/services/assetServer.md | Document bidirectional connect/reply and push publication envelope formats for clients. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-27 10:53 UTC |
ordishs
left a comment
There was a problem hiding this comment.
Approving. Root-cause analysis is correct and the fix adopts upstream's pattern rather than reinventing it.
Verified during review:
Unidirectional()=falsematches the centrifuge package's own WebSocket transport default — the fork was a divergence, not intentional.- The "just flip the boolean" rebuttal holds: the old
c.Connect()path routes throughunidirectionalConnect(req, nil)and the bidirectionalconnectCmdbranch dereferencescmd.Idwith no nil check, so a naive flip would have panicked.HandleReadFrame(c, reader)is the right call. context.WithoutCancel(r.Context())is the correct fix for the silent-abort:net/httpcancelsr.Context()the momentServeHTTPreturns (right after the goroutine launches), and centrifuge'sHandleCommandchecksctx.Done()per frame.WithoutCancelkeeps theauthMiddlewareCredentials while detaching cancellation.- Removed
ConnectRequest/SubscribeRequesttypes confirmed unused outside the file; thesubstranslation block was dead (built, never read). go test -race ./services/asset/centrifuge_impl/...passes locally.
Please address before/after merge:
-
(near-blocking)
services/asset/centrifuge_impl/client/index.html:28-30still sendsJSON.stringify({})on open. It's served live via the/client/file server and your own test plan references it as the smoke target. The server now treats{}as an empty command — update it to{ id: 1, connect: {} }to match the dashboard, otherwise the canonical in-tree sample is broken. -
(follow-up)
ui/dashboard/src/routes/wstest/+page.svelte:52still inspects the unwrappeddata.type/pub.datashapes the server no longer emits. Harmless dead code, worth a cleanup. -
(follow-up) The new test stubs Credentials via
OnConnecting; production sets them throughauthMiddleware— the exact path theWithoutCancelfix protects. A test driving the handler throughauthMiddlewareand asserting the reply still arrives afterServeHTTPreturns would guard against regressing the context handling. -
The three unchecked manual items in the test plan (live integration test,
make devdashboard,centrifuge-gosample) are the scenarios that matter most for an external-protocol fix — worth ticking at least the dashboard run before merge.
Not introduced here, but flagging for a separate ticket: sameHostOriginCheck returns nil unconditionally (websocket.go:486-505).
- services/asset/centrifuge_impl/client/index.html: the in-tree smoke
client was still sending `JSON.stringify({})` on open and parsing the
legacy unwrapped centrifuge fields (`data.pub`, `data.disconnect`,
`data.subscribe`, `data.unsubscribe`). Update the send to a real
centrifuge connect command `{id:1,connect:{}}` and parse push frames
out of `data.push.*` so the canonical smoke target matches the new
protocol.
- ui/dashboard/src/routes/wstest/+page.svelte: clean up the debug page
to recognise both shapes — raw payloads (when pointed at /p2p-ws) and
centrifuge push envelopes at `data.push.pub.data` (when pointed at the
asset endpoint). Same connect command on open as the production client.
- services/asset/centrifuge_impl/centrifuge_test.go: add
TestWebsocketHandler_BidirectionalConnectThroughAuthMiddleware. The
existing test stubbed Credentials via OnConnecting; production wires
them through authMiddleware, and authMiddleware is the exact path that
depends on `context.WithoutCancel`. The new test drives the handler
through authMiddleware, completes the handshake, and asserts the
reply still reaches the client after ServeHTTP returns — verified to
fail when the WithoutCancel detach is removed.
Refs: review feedback on bsv-blockchain#944
|
Addressed the followups in 4a31c5c.
Separate ticket for the |
|
oskarszoon
left a comment
There was a problem hiding this comment.
Approve. Core fix correct, regression test pins it (TestWebsocketHandler_BidirectionalConnectThroughAuthMiddleware fails without WithoutCancel). HandleReadFrame loop terminates cleanly on conn close + deadline + centrifuge !proceed; defer closeFn() handles client cleanup; authMiddleware credentials propagate through context.WithoutCancel correctly; no goroutine leaks; both new tests use require. Frontend has one real bug worth fixing in the same PR.
Worth fixing in this PR
ui/dashboard/src/internal/stores/p2pStore.ts:89-91, 329-344 — reconnect exponential backoff is broken. connectToP2PServer() resets connectionAttempts = 0 at entry, so every reconnect iteration restarts from attempt 1. Backoff never accumulates — it's always a flat 2-second delay. Production outages hammer the server. Fix: move the reset out of connectToP2PServer() to the explicit "user opened connection" path; let reconnect callers increment the existing counter.
Plus two smaller TS items in p2pStore.ts:
:134-168centrifuge message shapes (connect reply / push frame / ping) all typedany;tsconfig.json:8setsnoImplicitAny: falsewhich overridesstrict: trueand hides the gap. Either add typed interfaces or drop the override.:138-140ping detection should match the raw string beforeJSON.parse(matchesclient/index.html:43), not after parse to empty object.
Nits (non-blocking)
services/asset/centrifuge_impl/websocket.go:183-184—ctxChis created and deferred-closed inside the goroutine but never read anywhere. Leftover from the refactor, delete both lines.ui/dashboard/src/routes/wstest/+page.svelteroute is not inprotectedRoutes— accessible unauthenticated in production. Add a dev-only guard or comment that it's intentionally public.wstest/+page.svelte:185messages keyed bymsg.timeISO string — sub-millisecond collisions possible, use a counter/UUID.client/index.html:43-46reconnect uses linear delay (numFailures * 1000ms), dashboard uses (currently broken) exponential. Inconsistent.
Summary
/connection/websocketadvertisedUnidirectional()=trueon a WebSocket transport, so centrifuge emitted the connect reply as a Push envelope without the command id.centrifuge-go/centrifuge-jsclients couldn't complete the handshake and timed out after ~5s. This has blocked the node-validation acceptance suite (CLIENT-1, CLIENT-3) since v0.15.0-beta-2.c.Connect()always routes throughunidirectionalConnect(req, nil), and the bidirectional branch inconnectCmddereferencescmd.Idwith no nil check. The handler had to be rewritten to the upstreamHandleReadFrame(c, reader)pattern.r.Context()is cancelled the instantServeHTTPreturns (immediately after the goroutine launches). Centrifuge'sHandleCommandchecks<-c.ctx.Done()on every frame and aborted silently. The legacyc.Connect()path didn't go throughHandleCommand, which masked it. Switched tocontext.WithoutCancel(r.Context())so the goroutine keeps request-scoped credentials (set byauthMiddleware) but isn't killed by handler return.Changes
services/asset/centrifuge_impl/websocket.go:Unidirectional()→false; replaced manual JSON-decode +c.Connect()+ drain withcentrifuge.HandleReadFrame(c, reader)loop overconn.NextReader(); deleted dead customConnectRequest/SubscribeRequesttypes and the no-op subs translation block; detached request-context cancellation.services/asset/centrifuge_impl/centrifuge_test.go: flipped theUnidirectional()assertion; addedTestWebsocketHandler_BidirectionalConnectthat exercises the real handshake end-to-end against acentrifuge.Node(asserts the reply carriesid=1, a non-emptyconnect.client, and theOnConnectingsubscription).ui/dashboard/src/internal/stores/p2pStore.ts: send{id:1,connect:{}}on open instead of{}; parsepush.pub.datainstead of unwrappedpub.data; ignore empty centrifuge ping replies; drop dead unwrapped-json.typefallback.docs/topics/services/assetServer.md: documented the bidirectional command-reply and push wire-format envelopes.Test plan
go test -race ./services/asset/centrifuge_impl/...→ PASS, new test included.go vet+golangci-lint run ./services/asset/centrifuge_impl/...→ 0 issues.go build ./...→ clean.npm run build --prefix ./ui/dashboard→ clean build.go test -tags integration -run TestAssetServiceWebSocketIntegration ./services/p2p/against a live node (the existing integration test was already written for the bidirectional shape and was silently failing against the buggy server).make dev: confirmnode_statuspopulates the network view and live block notifications flow through.centrifuge-go: the in-tree sample atservices/asset/centrifuge_impl/clientconnects in <1s with a non-empty client id.Notes for reviewers
centrifuge.WebsocketHandleralready does. Long-term we could replace it with the upstream handler outright; this PR keeps the scaffolding to minimise diff.npm run checkonmainwas already red with 30 pre-existing TS errors across 20 files (2 of them in this samep2pStore.ts, onMap.keys().next().valuetyping); none introduced by this PR. Fixing those is out of scope.