feat(tern): one deployment correlates to exactly one remote apply

[aparajon]

Why this matters

Deployment-keyed dispatch attaches every sibling operation into the deployment's one data-plane apply, so all of a deployment's operation rows must record the same remote apply id — that shared id is what an operator greps in the data plane's storage and logs during an incident. Nothing enforced this: a dispatch result carrying a different id (an in-flight apply spanning a dispatch-key rollout, or a data plane that lost its keyed apply and minted a fresh one) would be persisted silently, splitting one deployment across two remote applies and breaking every correlation built on top.

What it does

• persistRemoteApplyID fails closed when storing a dispatch's remote apply id would correlate the operation's deployment to a second remote apply — either because the deployment's siblings already recorded a different id, or because the siblings themselves already disagree. Sibling deployments of the same apply are exempt: they own their own remote applies.
• The invariant is enforced atomically at write time: SaveExternalID locks the apply's operation rows and re-verifies the deployment's id inside the writing transaction, so sibling dispatches persisting concurrently across the driver pool cannot each pass an unlocked check and commit divergent ids. The unlocked guard read stays for precise triage logging.
• storage.DeploymentRemoteApplyID resolves the single remote apply id shared by a deployment's operations (with the legacy engine-resume-context carrier honored), erroring on disagreement instead of picking one. The write-path guard consumes it now; the status/progress read model consumes it next.
• ApplyOperation.RemoteApplyID names the external-id-else-legacy-carrier lookup both sites used. The drive scope and the data-plane log fan-out now resolve ids through it too, so an id recorded only in the legacy carrier still contributes its logs.
• New counter schemabot.remote_apply_deployment_id_conflict_total (database, environment, deployment) counts the refusals, documented in the metrics README with the operator action.

How it moves us toward the northstar

One data-plane apply per deployment; operations dispatch into it.

0  per-shard reconcile                          ✅ shipped
pre vschema-only deployment-scoped shape        ✅ shipped
pre op-lease drives settle projection-safely    ✅ shipped
1  dispatch granularity
   ├─ data-plane sibling attach                 ✅ merged
   └─ deployment-keyed idempotency + echo       ✅ merged
2  correlation persistence                      ⬅ this PR
   one shared external_id per deployment, fail-closed on divergence
3  op-scoped data-plane drive (shared apply)    up next
4  read model exposes the deployment apply id   up next
5  CLI renders it as APPLY ID in deployment mode
6  test matrix

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

This PR strengthens SchemaBot’s remote (gRPC) apply correlation model by enforcing the invariant that a single deployment must map to exactly one remote data-plane apply ID, failing closed when a dispatch response would split a deployment across multiple remote applies. This improves operator triage by ensuring there’s a single remote apply identifier to correlate across control-plane and data-plane logs/storage.

Changes:

• Add a write-path guard (guardDeploymentRemoteApplyID) that rejects persisting a remote apply ID when sibling operations for the same deployment already recorded a different ID (or already disagree).
• Introduce shared storage helpers (ApplyOperation.RemoteApplyID() + storage.DeploymentRemoteApplyID) to consistently resolve the remote apply ID (including legacy engine_resume_context fallback) and fail closed on disagreement.
• Add a new metric (schemabot.remote_apply_deployment_id_conflict_total) and document the operator action in the metrics README.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/tern/grpc_client.go	Enforces deployment-level single-remote-apply-ID persistence with a fail-closed guard and emits a new conflict metric.
pkg/tern/grpc_client_deployment_id_test.go	Adds focused tests for the new deployment-shared remote apply ID invariant on the gRPC client write path.
pkg/storage/deployment_remote_apply.go	Adds helpers to resolve per-operation and per-deployment remote apply IDs (with legacy fallback) and fail closed on disagreement.
pkg/storage/deployment_remote_apply_test.go	Adds unit tests covering RemoteApplyID() and DeploymentRemoteApplyID() behavior, including disagreement cases.
pkg/metrics/README.md	Documents the new conflict counter and the expected operator response.
pkg/metrics/metrics.go	Adds RecordRemoteApplyDeploymentIDConflict for emitting the new counter with deployment dimension.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1060, 34b5418.

Verdict: 4 findings — 1 blocking (TOCTOU race in the new deployment guard), 2 non-blocking (test-coverage gap, legacy-carrier log fetch gap), 1 general suggestion (duplicate id-precedence logic).

Blocking

• pkg/tern/grpc_client.go:1655 — guardDeploymentRemoteApplyID's read (ListByApply, an unlocked SELECT) and the caller's later write (SaveExternalID) are two independent, unserialized DB calls with no transaction or per-deployment lock spanning them. Sibling operations of one deployment are, by this repo's own design (apply_operations.go:701-705: "same-deployment shard work siblings of one apply drive concurrently across the driver pool"), claimed and dispatched by different driver goroutines. Two siblings whose dispatch responses are processed concurrently can each run the guard's ListByApply before either has committed its SaveExternalID, both see sharedID == "", both pass, and both persist divergent remote apply ids for the same deployment — exactly the state this guard exists to prevent. See the dedicated section below for the full proof chain.

Non-blocking

• pkg/tern/grpc_client_deployment_id_test.go:90 — No test exercises "current op's own row already equals the dispatched remoteID (idempotent replay, own-row check at line 1608 passes trivially via equality) while a sibling of the same deployment already holds a different id." Hand-tracing confirms the code handles it correctly today (the guard's peer scan still catches the sibling divergence), but it's the one case that most directly proves the deployment guard isn't redundant with the pre-existing own-row check, and a future refactor that short-circuits on "current already matches" would silently reintroduce the bug with nothing in the suite to catch it.
• pkg/api/log_handlers.go:169 — Data-plane log fan-out reads op.ExternalID directly and never falls back to EngineResumeContext the way this PR's new RemoteApplyID() helper does. An operation whose remote apply id lives only in the legacy carrier (a state this PR's own HonorsLegacyResumeContextCarrier test proves is real) will hit the externalID == "" branch and be silently skipped from log fetching in a multi-operation deployment, even though it ran remotely. Pre-existing gap, not introduced by this diff, but newly visible now that RemoteApplyID() is the documented canonical accessor.

General suggestions

• pkg/tern/grpc_client.go:1429 — applyTaskScope.remoteApplyID hand-rolls the identical ExternalID-else-EngineResumeContext precedence that this PR just extracted into ApplyOperation.RemoteApplyID(), instead of calling the new helper. A future change to the precedence rule applied to RemoteApplyID() and its call sites but not to this untouched duplicate would let remoteApplyID() silently diverge from persistRemoteApplyID()/guardDeploymentRemoteApplyID() for the same operation. Worth a follow-up to call s.operation.RemoteApplyID() here.

The one thing that could have broken, verified

The concurrency question: does guardDeploymentRemoteApplyID's read-then-write actually race in practice, or is there a serialization guarantee elsewhere that makes it moot? Confirmed: the race is real, not hypothetical. pkg/api/operator.go runs DefaultDrivers=4 concurrent driver goroutines, each independently claiming rows via FindNextApplyOperation's FOR UPDATE SKIP LOCKED; that function's own doc comment states same-deployment shard-work siblings of one apply "drive concurrently across the driver pool," and under cutover_policy=parallel no gate blocks a sibling's dispatch. Tracing the dispatch path (driveClaimedMultiOperation → resumeClaimedApplyWithOptions → RoutingClient.ResumeApplyOperation) shows it never acquires the delegateApplyLock mutex that RoutingClient.Apply uses elsewhere in the same file — so nothing serializes two siblings' calls into persistRemoteApplyID. guardDeploymentRemoteApplyID itself (grpc_client.go:1656) then does a plain unlocked ListByApply SELECT, and the caller's SaveExternalID write is guarded only by that operation's own lease token, not by any check against sibling state. Two siblings can therefore each read "no sibling id yet," each pass the guard, and each write a different id — leaving one deployment correlated to two remote applies, the exact invariant this PR's guard was built to enforce and fails to under this timing window.

Verified correct

• Hand-traced all four persistence scenarios (empty current/no siblings; empty current/diverging sibling; siblings already disagreeing; idempotent replay) against persistRemoteApplyID/guardDeploymentRemoteApplyID and confirmed each matches intended fail-closed semantics and the 5 new grpc_client_deployment_id_test.go tests plus the 6 storage-level DeploymentRemoteApplyID unit tests.
• guardDeploymentRemoteApplyID correctly excludes current from the peer scan before computing the shared sibling id, so current's own pending value can't self-confirm against the check meant to validate it.
• No double-metric-recording: the own-row disagreement branch returns before guardDeploymentRemoteApplyID runs, so RecordRemoteApplyDeploymentIDConflict is never double-counted for a single refusal.
• The single-operation (non-deployment-keyed) dispatch path at the top of persistRemoteApplyID is untouched by this diff and intentionally has no guard — with no siblings, the correlation problem this PR solves doesn't apply there.
• pkg/metrics/README.md's documentation of schemabot.remote_apply_deployment_id_conflict_total (name, labels, description) matches exactly what RecordRemoteApplyDeploymentIDConflict emits.
• DeploymentRemoteApplyID's nil-safety and deployment-filtering logic were hand-traced against the loop implementation and match all 6 unit test expectations (no ops, nothing recorded, all agree, legacy carrier counts, other deployments kept separate, disagreement fails closed).

This review was generated by Claude Code (claude-sonnet-5).

[aparajon]

🤖 Addressed in two commits:

• Blocking (TOCTOU in guardDeploymentRemoteApplyID) — fixed in 0dd9620. Enforcement moved into the write: SaveExternalID now takes the apply id, locks the apply's operation rows (SELECT … FOR UPDATE) and re-verifies the deployment invariant inside the writing transaction, refusing divergence with storage.ErrRemoteApplyDeploymentIDConflict. Racing siblings serialize at the row locks; the loser sees the winner's committed id and is refused. The unlocked guard read stays for per-shape triage logging, documented as advisory. New sqlstore tests cover MySQL and PostgreSQL, including a legacy-carrier sibling.
• Test gap (idempotent replay vs diverged sibling) — added in 0dd9620 at both layers: TestPersistRemoteApplyIDRefusesIdempotentReplayWhenSiblingDiverged (client) and TestApplyOperationStore_SaveExternalIDRefusesReplayWhenSiblingsDiverged (store).
• Log fan-out legacy-carrier gap — fixed in a0d25a2. handleDeploymentLogs resolves ids via ApplyOperation.RemoteApplyID(), gated by moving the existing IsRemote() check ahead of the fetch loop so a local drive's engine resume state is never mistaken for a remote apply id. Test: TestHandleDeploymentLogsHonorsLegacyResumeContextCarrier.
• applyTaskScope.remoteApplyID duplication — also 0dd9620; it now delegates to s.operation.RemoteApplyID().

This reply was written by Claude Code (Fable 5).