Repository navigation
chore: enable shard failover and coordinator - #23987
Merged
Merged
Conversation
The coordinator log helpers were hard-wired to the default topology's "workflow" nodeset. A sharded topology runs the coordinator on several shard nodesets, so the helpers now take the nodeset name. Existing callers pass coordinatedEngineDON; no behaviour change.
Runs the manual shard assignment flow with CoordinatedEngineEnabled on. Workflows must still execute only on their owner shard, the owner shard must route them to the coordinated engine and register their triggers through the coordinator, and no other shard may register them. This fails until sharded nodes may use the coordinated engine: today useCoordinatedEngine keeps every sharded node on the legacy path. It is not in the CI matrix yet.
The trigger coordinator needs to recognise admission outcomes returned by an EventSink, but triggers cannot import v2. Define ErrAdmissionDenied and ErrAdmissionCached in triggers; the v2 shard denial errors wrap ErrAdmissionDenied and ErrAdmissionCache aliases ErrAdmissionCached, so existing errors.Is checks are unchanged. Name the "admission_denied" drop reason as a monitoring constant so the coordinator can record the same label as the legacy engine.
Contributor
|
I see you updated files related to
|
Contributor
|
✅ No conflicts with other open PRs targeting |
A sharded node will register the manager with the trigger coordinator, which subscribes and delivers events through it. The coordinated engine never calls OnTriggerAdmission, so the manager must run the shard ownership check in ExecuteTrigger and return the admission error for the coordinator to act on. Cover Subscribe delegation, admission in ExecuteTrigger (owner, secondary caching, denials, engine errors) and what happens to an event cached there when the primary reports an outcome: dropped on success or user error, replayed to the engine on a system error. These fail until the manager implements the methods; today both return ErrCoordinatedShardingUnsupported.
On a sharded node the coordinator delivers through the shard failover manager, which returns the admission error instead of acting on it. The coordinator holds the trigger handles, so it must do what the legacy engine's put does after OnTriggerAdmission: ACK a denied event and count it as an admission_denied drop, and leave a cached event un-ACKed so the secondary can replay it. Cover denied, cached, other errors and success. The denied case fails until deliver handles ErrAdmissionDenied; the others pin the current behaviour.
A sharded node with the CoordinatedEngine flag open should build the coordinated engine inside its ShardFailoverManager and register the manager with the trigger coordinator, so every delivered event passes the shard admission check. With the flag closed it stays on the legacy engine. The engine-create fixture now wraps whichever engine it builds in a manager on a sharded node, as the real factory does. The flag-open cases fail until useCoordinatedEngine stops keeping sharded nodes on the legacy path.
Move the body of ExecuteFailoverManualSwapTest into runFailoverManualSwap, which takes a workflow name prefix and returns the pre-swap shard roles and each phase's workflow IDs, so a coordinated variant can assert per-workflow logs on each shard. The swap phase previously overwrote the phase 1 IDs in place; it now collects them separately. Workflow names and assertions are unchanged.
Runs the manual failover swap on a new topology whose shard nodes boot with CoordinatedEngineEnabled. Both shards must route every workflow to the coordinated engine and register its triggers through the coordinator, execution must still follow the primary, and once the swap makes the original primary the secondary it must cache every workflow's events for failover, proving the shard failover manager's admission check runs on the coordinated path. The flag is set in the topology because ApplyCRESettings cannot deliver settings on the failover topology: the settings job is built from the boot settings, and its validator rejects ShardingFailoverEnabled, which is not part of the cresettings schema. Generalise the coordinator log helpers to match any message for a workflow. This fails until sharded nodes may use the coordinated engine. It is not in the CI matrix yet.
The trigger coordinator subscribes and delivers through the registered engine, which on a sharded node is the ShardFailoverManager. Subscribe now delegates to the wrapped engine, and ExecuteTrigger runs the shard admission check before delegating, since the coordinated engine never calls OnTriggerAdmission. Admission errors are returned for the coordinator to act on. Failover replay still calls the wrapped engine directly: the secondary is not the owner, so admission would only cache the event again. Remove ErrCoordinatedShardingUnsupported.
On a sharded node the coordinator delivers through the shard failover manager, which returns the admission outcome instead of acting on it. The coordinator holds the trigger handles, so it now does what the legacy engine's put does after OnTriggerAdmission: on ErrAdmissionDenied it ACKs the event and records an admission_denied drop. A cached event is left un-ACKed so the secondary can replay it. The ACK runs without the reader's cancellation, like the execution, so unregistering mid-delivery cannot leave a denied event un-ACKed. Document both admission errors in the EventSink contract.
useCoordinatedEngine no longer keeps sharded nodes on the legacy engine. With the CoordinatedEngine flag open, a sharded node builds the coordinated engine inside its ShardFailoverManager and registers the manager with the trigger coordinator, which delivers through it, so every event still passes the shard admission check and denied events are ACKed by the coordinator.
The CRE smoke matrix discovers every test and runs any test without a per-test override on the default capabilities topology, which has no shard DONs. Route Test_CRE_V2_ShardManualAssignmentCoordinated to the sharded-manual topology and Test_CRE_V2_FailoverManualSwapCoordinated to the new failover-coordinated topology, in both the workflow matrix and the matrix generator.
Contributor
CORA - Pending ReviewersAll codeowners have approved! ✅ Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
Develop added the sharded confidential-workflows tests to the AdditionalOverrides fixture while this branch added the coordinated ones. Both sides bumped the expected count from 4 to 6, so the merge kept 6 though the fixture now yields 8 entries.
MStreet3
marked this pull request as ready for review
October 14, 2026 18:21
product-security-plaid-production
Bot
requested review from
cedric-cordenier,
chainchad,
prashantkumar1982 and
skudasov
October 14, 2026 18:21
MStreet3
enabled auto-merge
October 14, 2026 18:21
bolekk
reviewed
Oct 15, 2026
bolekk
left a comment
bolekk
left a comment
Contributor
There was a problem hiding this comment.
A simple review with Sonnet 5 flags multiple problems and at least a couple of them look legit to me. Please check:
- Errors logged for normal shard decisions (core/services/workflows/v2/triggers/coordinator.go:338): deliver logs "Failed to execute trigger event" at ERROR before it checks for ErrAdmissionCached or ErrAdmissionDenied. On a node that doesn't own the workflow, every event logs an ERROR, and again on each retransmit. The legacy path logs these at Info or not at all, so this could trip log-based alerts.
- Ownership lookup can hang forever (coordinator.go:336): context.WithoutCancel is applied before ExecuteTrigger, so the remote shard-ownership lookup in admissionCheck has no deadline or cancel. If the orchestrator hangs, reader goroutines hang too, UnregisterTriggers can't stop them, and handles get released while the reader is still running.
- Brief orchestrator errors drop events permanently (core/services/workflows/v2/base_engine.go:53): ErrShardDeniedOrchestrator now wraps ErrAdmissionDenied, which means "ACK and drop". A short ResolveShard failure on the real owner shard ACKs the event and it never runs anywhere. Before this change, retransmit would have retried it.
- Cached events keep coming back (core/services/workflows/syncer/v2/shard_failover_manager.go:218): when the primary reports SUCCESS or USER_ERROR, the secondary removes the event from its cache but never ACKs it. Retransmit sends it again, it gets cached again, and this repeats until the capability gives up.
- Cache lost when the engine is replaced (shard_failover_manager.go:429): admissionCheck can cache an event before the drain or closed check. The cache belongs to one manager instance, so if a workflow is updated while events are cached, they are discarded. A later SYSTEM_ERROR from the primary then has nothing to replay.
- Registration ID may not match (coordinator.go:354): dropDeniedEvent rebuilds the ID from cw.wid.Hex(), but Register keys handles by the tenant's raw workflow ID string. If the formats differ (for example uppercase hex), the ACK fails and the denied event keeps being re-sent.
bolekk
previously requested changes
Oct 15, 2026
jmank88
reviewed
Oct 15, 2026
|
Contributor
Author
legacy also logs error level for shard decisions. See here. But there are some minor log differences that I plan to address in a separate ticket.
there is a default timeout for ownership lookups that go through the shard orchestrator client here, which covers the ring resolver and the committed-owner check. but there is an ordering difference here that I've split to this ticket, along with confirming the manual resolver path is bounded too.
valid concern but this is identical to existing behavior. See here so moved to separate bug ticket.
seems possible from the code, but path also exists in legacy mode. moved to this ticket.
yes, legacy has same issue. moved to this ticket.
Pushed a PR to ensure only canonical workflow IDs are parsed. |
agparadiso
approved these changes
Oct 23, 2026
jmank88
approved these changes
Oct 23, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 23, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 23, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 23, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 23, 2026
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
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.
CRE-6747
Lets sharded nodes run the coordinated engine. The shard admission check still runs on every event.
ShardFailoverManagernow implements the coordinator methods.Subscribepasses through to the wrapped engine, andExecuteTriggerruns the shard admission check before passing the event on. Failover replay skips this check.ErrCoordinatedShardingUnsupportedis removed.deliver. OnErrAdmissionDeniedit ACKs the event and records anadmission_denieddrop. Cached events stay un-ACKed so the secondary can replay them.useCoordinatedEngineno longer keeps sharded nodes on the legacy engine when theCoordinatedEngineflag is on.Test_CRE_V2_ShardManualAssignmentCoordinatedandTest_CRE_V2_FailoverManualSwapCoordinated, plus a new failover-coordinated topology. CI sends each test to its matching topology.Requires
Supports
Deployment Validation Steps
Behind the
CoordinatedEngineflag. With the flag off, sharded nodes stay on the legacy engine.