Conversation
…PR1) Groundwork for a CHASM-based namespace-replication transport, landed as an inert foundation: it compiles and unit-tests but is wired into nothing. No service fx graph imports it, no gRPC receiver handler is registered, and the frontend still routes all namespace replication through the legacy queue path. Adds: - chasm/lib/namespacereplication: the NamespaceMutationComponent (per-mutation ephemeral CHASM component), its state machine, the two side-effect task handlers (local CAS apply + per-peer fan-out via the ApplyNamespaceMutation admin RPC), the pluggable PeerApplier transport seam with an admin-client default, the CHASM Library (component + task registration) and the fx Module. - chasm/namespacereplication.go: library/component name constants + component ID, mirroring chasm/callback.go. - adminservice ApplyNamespaceMutation RPC + request/response (Outcome enum); regenerated adminservice pb, gomock, admin client wrappers and logtags. - CHASM protos (message/service/tasks) + generated namespacereplicationpb. Deferred to later PRs in the stack: the history-side NamespaceReplicationService gRPC handler (handler.go) and the Library's RegisterServices override that binds it; shadow/authoritative transport modes; and all frontend/history/worker fx wiring. Until then the Library inherits the no-op RegisterServices from chasm.UnimplementedLibrary. go build ./... clean; go test ./chasm/lib/namespacereplication/... green; golangci-lint v2.9.0 (--new-from-rev=main) 0 issues. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
qyc5937
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
A CHASM state-save failure can leave a committed source mutation marked failed without peer fan-out.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Introduces an inert CHASM state machine for ordered namespace mutation and peer replication.
Changes:
- Adds local CAS application and peer fan-out handlers.
- Implements retry backoff, lifecycle transitions, and error classification.
- Adds pluggable AdminService transport and tests.
File summaries
| File | Description |
|---|---|
tasks.go |
Implements local and peer task handlers. |
tasks_test.go |
Tests handlers, transport, and classification. |
statemachine.go |
Defines lifecycle and retry transitions. |
statemachine_test.go |
Tests transition scheduling and backoff. |
peer_applier.go |
Adds the pluggable peer transport. |
library.go |
Registers components and tasks. |
fx.go |
Provides the library’s FX module. |
component.go |
Defines namespace mutation state and lifecycle. |
component_test.go |
Tests construction, termination, and completion. |
Review details
Suppressed comments (1)
chasm/lib/namespacereplication/tasks.go:392
-
Details
small — The peer-failure log also mislabels the composite business ID.
ref.BusinessID contains namespace_id:mutation_uuid, not only a namespace ID. Queries and alerts using this structured tag will therefore see a different value for every mutation rather than the namespace identifier promised by the key.
Suggestion: Use an accurate business-ID tag here as well.
tag.NewStringTag("namespace_id", ref.BusinessID),
- Files reviewed: 9/9 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Validate gating (pure — Validate ignores the chasm.Context). | ||
| // ----------------------------------------------------------------------------- | ||
|
|
||
| func TestApplyLocalTaskHandler_Validate(t *testing.T) { |
…' into qian/pr12113-fixes
…' into qian/pr12113-fixes
There was a problem hiding this comment.
🟡 Changes recommended
Ambiguous or superseded committed writes can be recorded as terminal failures, suppressing peer fan-out.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
chasm/lib/namespacereplication/tasks.go:243
-
Details
high — Persistence read normalization can turn a committed write into a terminal failure.
GetNamespace canonicalizes namespace details before returning them (common/persistence/metadata_manager.go:156-165), including materializing an absent BadBinaries message and default replication fields. Those values are optional in the mutation contract, so a write can commit and then fail this raw proto.Equal check solely because readback normalized the payload. For CREATE, the subsequent NamespaceAlreadyExists branch declares the write definitely not applied; for UPDATE, an advanced metadata version does the same. The component then records FAILED and skips peer fan-out even though the source write committed.
Suggestion: Canonicalize a clone of the expected detail with the same persistence defaults before comparing (or use a shared semantic comparison), and test ambiguous CREATE/UPDATE recovery with a real normalized readback shape.
if response.IsGlobalNamespace != isGlobal || !proto.Equal(response.Namespace, detail) {
return false, nil
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
| if _, ok := errors.AsType[*serviceerror.NamespaceAlreadyExists](applyErr); ok && | ||
| operation == namespacereplicationpb.NAMESPACE_OPERATION_CREATE { | ||
| // Exact-state reconciliation already ruled out a retry of our own create. | ||
| return true, nil | ||
| } | ||
| if !persistence.OperationPossiblySucceeded(applyErr) { | ||
| return true, nil |
| if outcome == namespacereplicationpb.PEER_APPLY_OUTCOME_FAILED_RETRIABLE { | ||
| peer := c.GetPeerApply()[task.GetTargetCell()] | ||
| firstAt := now | ||
| if peer.GetFirstAttemptAt() != nil { | ||
| firstAt = peer.GetFirstAttemptAt().AsTime() | ||
| } | ||
| if now.Sub(firstAt) < peerRetryBudget { |
…' into qian/pr12113-fixes
Stack: PR1b
Depends on #12135 (contracts/codegen), which depends on #12112.
Summary
PeerAppliertransport seam and AdminService client implementationqueueserrors.DestinationDownErrorwhile avoiding double-counting service throttlingThe protobuf and generated API surface are reviewed separately in #12135.
Tests
go test -tags test_dep ./chasm/lib/namespacereplication/... ./common/namespace/nsreplicationmake lint-code-fastOperational impact
The library remains inert: it is not imported into a service FX graph, no receiver handler is registered, and the frontend continues using the legacy namespace-replication path.