Repository navigation
refactor(cardinal): move client and inter-shard communication into pkg/transport - #1059
Open
winton-library wants to merge 7 commits into
Open
winton-library wants to merge 7 commits into
winton-library wants to merge 7 commits into
Conversation
winton-library
added this pull request to stack #1060
October 2, 2026 22:55
Code review in progress
|
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 19 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 7 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
winton-library
force-pushed
the
winton/communication-layer-separation
branch
from
October 4, 2026 14:01
2928f4b to
c994b14
Compare
There was a problem hiding this comment.
0 issues found across 11 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 6 unresolved issues from previous reviews.
Re-trigger cubic
Start now flushes the NATS connection after registering endpoints, so a command sent right after Start returns is not answered with no responders and dropped; this was the cause of the intermittent CI failures in the cardinal inter-shard tests. SendCommandWithReply registers its reply waiter before dispatching, so a handler that publishes its reply before returning, as a non-ECS service may, is delivered. A second Start call is rejected instead of leaking the first connection, and Start is split into startNATS and startHTTP to stay within the linter's length limit. On the cardinal side, NewWorld and the end-to-end harness now build the transport through one newTransport helper instead of two copies, the debug service test goes through startTransport so it fails if startup stops finalizing the catalog, and the end-to-end harness sends the X-Email header that Dev auth has required since passthrough auth was removed.
…ork on Stop The transport no longer opens its own NATS connection or HTTP server. The app passes its micro.Client to Start and serves the handler returned by Handler on its own server, so one connection can be shared and the app decides when connections open and close. Cardinal's World now opens one NATS client, shares it with the JetStream snapshot store instead of letting the store open a second connection, serves the transport and debug service on its own mux, and closes the client last on shutdown. Enqueue and Flush are now safe to call from any goroutine, so a service that handles commands concurrently can send to other shards from inside its handlers. Stop now rejects new commands as unavailable and unsubscribes the NATS endpoints, waits for running handlers, sends what they flushed, and then ends open event streams and reply waits, so a reply committed by a running handler is no longer lost and an open stream no longer holds the HTTP server's shutdown until the deadline. Commands flushed after Stop are dropped and logged.
…returns Publish picks the subscribed streams under the lock, releases it, and then sends to each one. If a stream's handler returned in between, because the client disconnected or Stop ended the stream, connect-go finished the stream while the send was still writing to it, which the race detector reports. The tick made this rare in Cardinal, but Stop ending every stream at once and services that publish from concurrent handlers make it likely. Each stream subscriber now has a closed flag guarded by the lock that send already holds around every write. The stream handler sets it before returning, which waits for a send in progress, and later sends skip the stream. A race test publishes in a loop while streams open and close.
…the SDK generator A service on pkg/transport could only register commands by name with Handle, so every handler received the raw command and decoded it itself, and the SDK generator, which finds wire types through Cardinal's RegisterCommand and RegisterEvent, found nothing to generate for a service without Cardinal. RegisterCommand[T] now registers a handler for T.Name() that receives the payload already decoded into T together with the sender's persona; a payload that does not decode is rejected back to its sender. RegisterEvent[T] declares an event or result the service publishes. It is only a declaration, since a result named after a request is only named at runtime. Handle stays for callers that want the raw command. Cardinal's RegisterCommand and RegisterEvent now go through these: the command handler pushes the decoded value onto the tick queue, so commands are still decoded once, and systems read them through Commands[T] as before. The SDK generator recognises the transport's RegisterCommand and RegisterEvent by import path, so a service without Cardinal gets wire code for its commands, events and correlated results, and discovery on Cardinal backends is unchanged.
Check that each command in the concurrent enqueue/flush test reaches the target it was sent to, and fail the stream-close test if the first message never arrives. Drop JetStreamStorageOptions.Logger: the caller now passes in the NATS client, so nothing reads it.
winton-library
force-pushed
the
winton/communication-layer-separation
branch
from
October 6, 2026 18:11
ac03056 to
97438df
Compare
…d lint rules Close the NATS connection when the JetStream snapshot store fails to set up: no World is returned, so shutdown never runs to close it. Give the hung-target test half the send timeout instead of 500ms, so a slow CI machine does not fail it while a Flush that waits on the send still does. Replace the log import in the test setup and wrap long lines, as main's new lint rules require.
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval skipped because cubic reviewed only this push, not the earlier force-push. Comment @cubic review to review the whole pull request.
Turn on auto-fix | Re-trigger cubic
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
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.
Cardinal's ConnectRPC service, auth, event streams and inter-shard send/receive move into a new pkg/transport that knows nothing about ECS, so a non-ECS service (for example lobby or meta) can use the same transport. Wire format and span names are unchanged.
The app owns its connections. The transport no longer opens NATS or an HTTP server. The app passes its
micro.ClienttoStartand mountsHandler()on its own server. Cardinal creates one NATS client, shares it with the JetStream snapshot store, and closes it last.Typed registration.
RegisterCommand[T](handler)decodes each command once and hands the handler the typed payload and persona.RegisterEvent[T]()declares an event. The SDK generator finds both by the transport's import path, so a service without Cardinal gets generated code for its commands, events and correlated results. Cardinal'sRegisterCommandandRegisterEventpass through to these, and Cardinal pushes the decoded command into its queue.Handlers can run at once. Non-ECS services handle each request on its own goroutine, so
EnqueueandFlushare safe to call from any goroutine and keep each goroutine's order per target.Stop order.
The app then shuts down its HTTP server and closes the NATS client.
Event stream fix. Publishing to a stream whose handler has returned no longer writes to a finished stream.
Stack created with GitHub Stacks CLI • Give Feedback 💬
Summary by cubic
Moves Cardinal's client-facing ConnectRPC service, auth, event streams, and inter-shard send/receive over NATS into a new
pkg/transportthat is independent of the ECS, so non-ECS services can reuse the same transport.Refactors
micro.ClienttoStartand serves the handler fromHandleron its own server; Cardinal shares one NATS client with the JetStream snapshot store and closes it last on shutdown, including whenNewWorldfails before a World exists.Startflushes the NATS connection after registering endpoints so a command sent right after start is not dropped; a secondStartis rejected.SendCommandWithReplyregisters its reply waiter before dispatching, so a handler that publishes its reply before returning receives it;EnqueueandFlushare safe to call from any goroutine.Stopno longer races with the stream handler's return.Stoprejects new commands as unavailable, unsubscribes the NATS endpoints, waits for running handlers, sends what they flushed, then ends open event streams and reply waits; commands flushed after stop are dropped and logged.New Features
RegisterCommand[T](handler)andRegisterEvent[T]()give typed registration the SDK generator discovers by import path, so a service without Cardinal gets wire code for its commands, events, and correlated results; Cardinal's own registrations route through them, decoding each command once.Written for commit 6857c8f. Summary will update on new commits.