Skip to content

Commit 2e4de2b

Browse files
authored
Usage of ActiveInCluster should be justified (#10485)
## What changed? 1. Add linter rule to guard`ActiveInCluster` usage. 2. Fixed the callsites for `ActiveInCluster` where should use `ActiveClusterName` instead. 3. Add explicit `//nolint:forbidigo justifications` for genuine namespace-level checks. ## Why? ActiveInCluster is namespace-level and not businessID-aware. For workflow-scoped decisions, the active cluster must be resolved with `ActiveClusterName(routingKey)` so future businessID-aware routing does not incorrectly treat an entire namespace as active or standby. ## How did you test it? - [x] built - [ ] run locally and tested manually - [x] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s)
1 parent 598e145 commit 2e4de2b

18 files changed

Lines changed: 25 additions & 7 deletions

File tree

.github/.golangci.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ linters:
5252
msg: "Use await.Require / s.Await for assertion conditions, or await.RequireTrue / s.AwaitTrue for bool predicates, instead of testify Eventually helpers"
5353
- pattern: 'assert\.\w+'
5454
msg: "Use require.X / protorequire.X instead of assert.X / protoassert.X — assert doesn't stop the test on failure."
55+
- pattern: '\bActiveInCluster\b'
56+
msg: "ActiveInCluster is not businessID-aware. For any decision tied to a workflow / businessID, use ns.ActiveClusterName(routingKey) instead. For genuine namespace-level checks (no workflow context), suppress with //nolint:forbidigo // <justification>."
5557
depguard:
5658
rules:
5759
main:
@@ -200,6 +202,10 @@ linters:
200202
text: "FunctionalTestBase"
201203
linters:
202204
- forbidigo
205+
- path: common/namespace/(namespace|replication_resolver)(_test)?\.go$ # ActiveInCluster definition and direct unit tests
206+
text: "ActiveInCluster"
207+
linters:
208+
- forbidigo
203209
- path-except: tests/.+_test\.go # only enforce in test files
204210
text: "context.Background"
205211
linters:

common/namespace/nsregistry/registry.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@ func DefaultNamespaceStateChanged(currentClusterName string, oldNS *namespace.Na
185185
oldNS.State() != newNS.State() ||
186186
oldNS.Name() != newNS.Name() ||
187187
oldNS.IsGlobalNamespace() != newNS.IsGlobalNamespace() ||
188+
//nolint:forbidigo // ns-wide state diff for cache invalidation.
188189
oldNS.ActiveInCluster(currentClusterName) != newNS.ActiveInCluster(currentClusterName) ||
189190
oldNS.ReplicationState("") != newNS.ReplicationState("")
190191
}

service/frontend/nexus_handler.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ func (c *operationContext) interceptRequest(
187187
return commonnexus.ConvertGRPCError(err, false)
188188
}
189189

190+
//nolint:forbidigo // Nexus requests are not tied to a business ID by design (see line 184)
190191
if !c.namespace.ActiveInCluster(c.clusterMetadata.GetCurrentClusterName()) {
191192
if c.shouldForwardRequest(ctx, header) {
192193
// Handler methods should have special logic to forward requests if this method returns

service/frontend/workflow_handler.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,7 @@ func (wh *WorkflowHandler) Start() {
414414

415415
if ns.IsGlobalNamespace() &&
416416
ns.ReplicationPolicy() == namespace.ReplicationPolicyMultiCluster &&
417+
//nolint:forbidigo // namespace state-change callback; cancels all pollers on ns deactivation
417418
!ns.ActiveInCluster(wh.clusterMetadata.GetCurrentClusterName()) {
418419
pollers, ok := wh.outstandingPollers.Get(ns.ID().String())
419420
if ok {

service/history/api/deleteworkflow/api.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func Invoke(
5555
if err != nil {
5656
return nil, err
5757
}
58-
if ns.ActiveInCluster(shardContext.GetClusterMetadata().GetCurrentClusterName()) {
58+
if ns.ActiveClusterName(namespace.RoutingKey{ID: request.WorkflowExecution.GetWorkflowId()}) == shardContext.GetClusterMetadata().GetCurrentClusterName() {
5959
// If workflow execution is running and in active cluster.
6060
if err := api.UpdateWorkflowWithNew(
6161
shardContext,

service/history/api/queryworkflow/api.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ func Invoke(
156156
// 1. the namespace is not active, in this case history is immutable so a query dispatched at any time is consistent
157157
// 2. the workflow is not running, whenever a workflow is not running dispatching query directly is consistent
158158
// 3. if there is no pending or started workflow tasks it means no events came before query arrived, so its safe to dispatch directly
159-
safeToDispatchDirectly := !nsEntry.ActiveInCluster(shardContext.GetClusterMetadata().GetCurrentClusterName()) ||
159+
safeToDispatchDirectly := nsEntry.ActiveClusterName(namespace.RoutingKey{ID: workflowKey.WorkflowID}) != shardContext.GetClusterMetadata().GetCurrentClusterName() ||
160160
!mutableState.IsWorkflowExecutionRunning() ||
161161
(!mutableState.HasPendingWorkflowTask() && !mutableState.HasStartedWorkflowTask())
162162
if safeToDispatchDirectly {

service/history/chasm_engine.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -585,7 +585,7 @@ func (e *ChasmEngine) deleteExecution(
585585
if err != nil {
586586
return err
587587
}
588-
if ns.ActiveInCluster(shardContext.GetClusterMetadata().GetCurrentClusterName()) {
588+
if ns.ActiveClusterName(namespace.RoutingKey{ID: ref.BusinessID}) == shardContext.GetClusterMetadata().GetCurrentClusterName() {
589589
chasmTree, ok := mutableState.ChasmTree().(*chasm.Node)
590590
if !ok {
591591
return serviceerror.NewInternalf(

service/history/history_engine.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -393,6 +393,7 @@ func (e *historyEngineImpl) registerNamespaceStateChangeCallback() {
393393

394394
if ns.IsGlobalNamespace() &&
395395
ns.ReplicationPolicy() == namespace.ReplicationPolicyMultiCluster &&
396+
//nolint:forbidigo // namespace state-change callback; FailoverNamespace operates per-namespace, no workflow context
396397
ns.ActiveInCluster(e.currentClusterName) {
397398

398399
for _, queueProcessor := range e.queueProcessors {

service/history/ndc/workflow_state_replicator.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1218,7 +1218,7 @@ func (r *WorkflowStateReplicatorImpl) bringLocalEventsUpToSourceCurrentBranch(
12181218
}
12191219
// In standby cluster, use a background low priority which is higher than the standby task processing
12201220
callerType := headers.CallerTypeBackgroundLow
1221-
if ns.ActiveInCluster(r.clusterMetadata.GetCurrentClusterName()) {
1221+
if ns.ActiveClusterName(namespace.RoutingKey{ID: workflowID}) == r.clusterMetadata.GetCurrentClusterName() {
12221222
// In active cluster, use lowest priority to minimize the impact to live traffic
12231223
callerType = headers.CallerTypePreemptable
12241224
}
@@ -1727,7 +1727,7 @@ func (r *WorkflowStateReplicatorImpl) backfillHistory(
17271727
}
17281728
// In standby cluster, use a background low priority which is higher than the standby task processing
17291729
callerType := headers.CallerTypeBackgroundLow
1730-
if ns.ActiveInCluster(r.clusterMetadata.GetCurrentClusterName()) {
1730+
if ns.ActiveClusterName(namespace.RoutingKey{ID: workflowID}) == r.clusterMetadata.GetCurrentClusterName() {
17311731
// In active cluster, use lowest priority to minimize the impact to live traffic
17321732
callerType = headers.CallerTypePreemptable
17331733
}

service/history/queues/executable.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -861,7 +861,7 @@ func taskBaseMetricTagsWithoutArchetype(
861861
ns, err := namespaceRegistry.GetNamespaceByID(namespace.ID(task.GetNamespaceID()))
862862
if err == nil {
863863
namespaceTag = metrics.NamespaceTag(ns.Name().String())
864-
isActive = ns.ActiveInCluster(currentClusterName)
864+
isActive = ns.ActiveClusterName(namespace.RoutingKey{ID: task.GetWorkflowID()}) == currentClusterName
865865
}
866866

867867
taskType := taskTypeTagProvider(task, isActive, chasmRegistry)

0 commit comments

Comments
 (0)