Skip to content

namespace replication: wire shadow CHASM transport (PR2) - #12115

Draft
qyc5937 wants to merge 11 commits into
qian/feat-nsrepl-chasm-libfrom
qian/feat-nsrepl-chasm-shadow
Draft

qyc5937 wants to merge 11 commits into
qian/feat-nsrepl-chasm-libfrom
qian/feat-nsrepl-chasm-shadow

Conversation

@qyc5937

@qyc5937 qyc5937 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Stack: PR2 of 7

Depends on #12113.

Summary

  • add global legacy / shadow / chasm transport-mode dynamic config, defaulting to legacy
  • register the namespace-replication CHASM library and history service, and route frontend mutations through the layered history client in shadow mode
  • run the full component and destination-tagged peer transport while hard-skipping source and receiver writes
  • compare deterministic payload fingerprints at both the frontend build boundary and peer receive boundary, with mismatch logging
  • preserve the legacy metadata write and replication queue as the only authoritative path

Tests

  • make proto
  • go test -tags test_dep ./common/namespace/nsreplication/... ./chasm/lib/namespacereplication/... ./service/frontend/...
  • go test -tags test_dep ./service/history ./service/worker
  • make fmt-imports
  • make lint-code-fast

Operational impact

The default remains legacy. shadow exercises CHASM compare-only and does not fail the user mutation if shadow transport fails. Authoritative chasm writes are intentionally rejected until PR4. This is the first stack point suitable for a simple multi-cell shadow transport E2E.

… qian/pr12115-holistic

# Conflicts:
#	common/namespace/nsreplication/conversion_test.go
… qian/pr12115-holistic

# Conflicts:
#	chasm/lib/namespacereplication/gen/namespacereplicationpb/v1/message.pb.go
… qian/pr12115-holistic

# Conflicts:
#	chasm/lib/namespacereplication/tasks.go
#	chasm/lib/namespacereplication/tasks_test.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Seven unresolved findings remain, including one critical issue and multiple moderate rollout and correctness issues.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Wires namespace replication through CHASM in shadow mode while retaining legacy replication as authoritative.

Changes:

  • Adds transport-mode configuration and CHASM history/worker registration.
  • Routes frontend mutations through compare-only shadow transport.
  • Adds deterministic fingerprints, peer validation, generated protobufs, and tests.
File summaries
File Reviewed changes and final findings
tests/xdc/namespace_replication_chasm_test.go Adds shadow transport E2E coverage.
tests/testcore/functional_test_base.go Supports additional test server options.
service/worker/fx.go Registers the namespace replication library.
service/history/fx.go Registers the history-side CHASM library.
service/frontend/workflow_handler.go Injects the layered CHASM client.
service/frontend/workflow_handler_test.go Updates handler fixtures.
service/frontend/service.go Adds transport-mode configuration.
service/frontend/namespace_handler.go Builds and triggers shadow mutations. Moderate (1 vote): decouple shadow triggering from the user-mutation critical path. Moderate (2 votes): reject unsupported chasm mode instead of silently falling back to legacy.
service/frontend/namespace_handler_test.go Updates constructor fixtures.
service/frontend/namespace_handler_chasm_test.go Tests shadow mutation routing.
service/frontend/fx.go Provides the layered CHASM client and library.
service/frontend/admin_handler.go Adds compare-only receiver handling. Moderate (1 vote): distinguish shadow validation from a real applied write.
service/frontend/admin_handler_test.go Tests shadow and rejection outcomes.
proto/internal/temporal/server/api/adminservice/v1/request_response.proto Adds shadow and fingerprint fields.
common/namespace/nsreplication/transmission_task_handler.go Adds conversion and fingerprint helpers.
common/namespace/nsreplication/conversion_test.go Tests deterministic fingerprints and diffs.
common/dynamicconfig/constants.go Defines transport modes and defaults.
chasm/lib/namespacereplication/tasks.go Skips source writes in shadow mode.
chasm/lib/namespacereplication/tasks_test.go Covers shadow task behavior.
chasm/lib/namespacereplication/proto/v1/message.proto Adds the shadow mutation flag.
chasm/lib/namespacereplication/peer_applier.go Sends shadow payloads and fingerprints.
chasm/lib/namespacereplication/library.go Registers the CHASM service. Nit (1 vote): register the library with tdbg.
chasm/lib/namespacereplication/handler.go Critical (2 votes): reject non-shadow mutations. Moderate (1 vote): make triggers idempotent across retries. Moderate (1 vote): validate nested mutation fields.
chasm/lib/namespacereplication/handler_test.go Tests execution routing.
chasm/lib/namespacereplication/gen/namespacereplicationpb/v1/message.pb.go Regenerated CHASM protobuf bindings.
chasm/lib/namespacereplication/fx.go Provides the CHASM handler.
api/adminservice/v1/request_response.pb.go Regenerated admin protobuf bindings.
Review details

Files not reviewed (2)

  • api/adminservice/v1/request_response.pb.go: Generated file
  • chasm/lib/namespacereplication/gen/namespacereplicationpb/v1/message.pb.go: Generated file

Suppressed comments (5)

chasm/lib/namespacereplication/handler.go:55

  • Details
med — Make the trigger idempotent across layered-client retries.

The frontend calls this RPC through a retrying layered client, but StartExecution uses the default BusinessIDConflictPolicyFail and BusinessIDReusePolicyAllowDuplicate. If the first RPC persists the component and loses its response, a retry while it is running returns ExecutionAlreadyStarted; if it has completed, the reuse policy can start a second execution for the same mutation. The frontend ignores the error, so this appears as a false shadow failure or duplicates the entire shadow fan-out and its persistence/visibility work.

Suggestion: Use an idempotent start/retry flow: on an existing execution, resolve the same component and poll it rather than returning an error or creating a new run.

	if _, err := chasm.StartExecution[*NamespaceMutationComponent, *namespacereplicationpb.NamespaceMutation](
		ctx,
		key,
		func(mctx chasm.MutableContext, mutation *namespacereplicationpb.NamespaceMutation) (*NamespaceMutationComponent, error) {
			component := NewNamespaceMutationComponent(mctx, mutation)
			if err := TransitionScheduleLocal.Apply(component, mctx, EventScheduleLocal{}); err != nil {
				return nil, err
			}
			return component, nil
		},
		req.GetMutation(),

chasm/lib/namespacereplication/handler.go:41

  • Details
med — Malformed mutation details can panic during peer transport.

Checking only that namespace_detail is present allows info, config, or replication_config to be nil. The shadow local task skips the metadata write, then NamespaceDetailToTaskAttributes dereferences those nested fields directly when a peer task runs, turning an invalid RPC into a task panic/failure instead of a bounded InvalidArgument response.

Suggestion: Validate all nested fields required by NamespaceDetailToTaskAttributes at this RPC boundary.

	if req.GetMutation().GetNamespaceDetail() == nil {
		return nil, serviceerror.NewInvalidArgument("mutation.namespace_detail is required")

chasm/lib/namespacereplication/library.go:40

  • Details
med — Register the active library with tdbg.

This change now persists namespace-replication CHASM components and adds a registration-only NewNilLibrary, but tools/tdbg/chasm_registry.go still omits this library while registering the other CHASM libraries. As a result, tdbg cannot resolve the new component/task types when inspecting these executions, which removes the debugging path for the feature. Add namespacereplication.NewNilLibrary() to that registry alongside the other registration-only libraries.

Suggestion: Update tools/tdbg/chasm_registry.go to register the namespace-replication nil library.

func (l *Library) RegisterServices(server *grpc.Server) {
	if l.handler != nil {
		server.RegisterService(&namespacereplicationpb.NamespaceReplicationService_ServiceDesc, l.handler)

service/frontend/admin_handler.go:1063

  • Details
med — Shadow peer validation is reported as a real write.

OUTCOME_APPLIED is documented as an existing namespace being updated, but this branch intentionally performs no receiver write. The peer applier maps that response to PeerApplyResultApplied, so the source component records PEER_APPLY_OUTCOME_APPLIED and can expose a successful write even though shadow mode only validated the payload.

Suggestion: Add a distinct validation-only outcome (and map it separately) or avoid recording a write outcome for shadow requests.

	return &adminservice.ApplyNamespaceMutationResponse{
		Outcome: adminservice.ApplyNamespaceMutationResponse_OUTCOME_APPLIED,
	}, nil

service/frontend/namespace_handler.go:1479

  • Details
med — Keep the shadow trigger off the user-mutation critical path.

This RPC uses the caller's context and is awaited synchronously. If the history service is unavailable, the layered client retries and waits until that request context expires before this function logs and ignores the error, so a compare-only failure can still consume the Register/Update deadline or cause the user RPC to be cancelled. The shadow path should be launched with an independently bounded context (or another non-blocking delivery mechanism) so its failure cannot affect user-mutation latency.

Suggestion: Decouple this trigger from the request context and bound its background lifetime; retain the warning log when the detached attempt fails.

	_, err = d.chasmNsReplClient.TriggerNamespaceMutation(ctx, &namespacereplicationpb.TriggerNamespaceMutationRequest{
		NamespaceId:       namespaceID,
		SystemNamespaceId: primitives.SystemNamespaceID,
		BusinessId:        namespaceID + ":" + uuid.NewString(),
  • Files reviewed: 25/27 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread chasm/lib/namespacereplication/handler.go Outdated
Comment thread service/frontend/namespace_handler.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Cross-service asynchronous dispatch has unresolved resource-lifecycle and routing-invariant concerns.

Review details

Files not reviewed (4)

  • api/adminservice/v1/request_response.pb.go: Generated file
  • api/adminservice/v1/service_grpc.pb.go: Generated file
  • chasm/lib/namespacereplication/gen/namespacereplicationpb/v1/message.go-helpers.pb.go: Generated file
  • chasm/lib/namespacereplication/gen/namespacereplicationpb/v1/message.pb.go: Generated file

Suppressed comments (2)

proto/internal/temporal/server/api/adminservice/v1/service.proto:77

  • Details
nit — Document the RPC's current shadow-only semantics.

The next line still promises apply-if-higher writes, but this PR rejects non-shadow requests and shadow requests never mutate state. This public API comment therefore describes behavior that no supported request can reach.

Suggestion: Describe shadow validation and the temporary rejection of authoritative requests.

  // calls this RPC against each peer cell after its local phase resolves.
  // Apply semantics are apply-if-higher (config_version / failover_version).

chasm/lib/namespacereplication/gen/namespacereplicationpb/v1/message.pb.go:310

  • Details
nit — Regenerate the corrupted protobuf outputs.

The changed generated files contain displaced comments: this method's deprecation marker is inserted inside strconv.Itoa, and api/adminservice/v1/request_response.pb.go:115-121 attaches deprecation and scheduler comments to unrelated enum branches. The code still parses, but generated API documentation and deprecation metadata are malformed.

Suggestion: Regenerate both protobuf outputs from their source files and verify that declaration comments remain attached to EnumDescriptor and SchedulerTarget.

  • Files reviewed: 32/36 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread chasm/lib/namespacereplication/handler.go
Comment thread service/frontend/namespace_handler.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The distributed state-machine and asynchronous cross-cluster transport changes require final human validation.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Files not reviewed (4)
  • api/adminservice/v1/request_response.pb.go: Generated file
  • api/adminservice/v1/service_grpc.pb.go: Generated file
  • chasm/lib/namespacereplication/gen/namespacereplicationpb/v1/message.go-helpers.pb.go: Generated file
  • chasm/lib/namespacereplication/gen/namespacereplicationpb/v1/message.pb.go: Generated file
Previously missed (1)

In code that hasn't changed since last review

Medium severity Blocking RPC test double ignores context cancellation

service/​frontend/​namespace_handler_chasm_test.go:48

med — The blocking RPC test double can leak forever on a failed test.

The test double ignores the RPC context while waiting on release. If the test fails before closing that channel, the production timeout cannot stop this goroutine, so it remains blocked for the rest of the test process.

Suggestion: Wait for either release or context cancellation and return ctx.Err() when canceled.

… qian/pr12115-holistic

# Conflicts:
#	common/namespace/nsreplication/conversion_test.go
#	common/namespace/nsreplication/transmission_task_handler.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants