feat(deploy): cluster-wide stage/activate barrier and trusted peer phase operation - #2301
Conversation
…r phase operation Restores the peer coordination protocol that #1849 carried before it was split, so it can be reviewed on its own and land with real multi-node verification. This is the half unit tests cannot validate: the harness mocks `replicateOperation` and enters the authorization bypass directly, so it cannot observe a peer at all. On top of #1849's per-node staged deploy, this adds: - `component_deploy_phase`, a trusted peer-only operation carrying the phase and deployment id. Authorization travels in AsyncLocalStorage rather than on the request, so it is unreachable over HTTP with ordinary credentials, and an older peer rejects an unknown operation instead of misreading a phase marker as a one-shot deploy. - The cluster-wide barrier: the origin stages everywhere and waits for every node to report a good stage before any node activates. A node that cannot fetch or install fails during staging while the live component is untouched everywhere. - The separated public phases — `activate: false` returns a staged deployment_id, `deployment_id` activates it later, `two_phase: false` forces single-phase — plus the `harper stage` / `harper activate` CLI verbs and the capability probe that stops a staged request reaching a server that would silently deploy it live. - The deployment row as the peer channel: the immutable activation specification, the payload blob peers read their bytes from, and staged-row retention. - Origin-owned row semantics: peers claim with `persist: false` so the replicated row has exactly one writer, with local activation artifacts as their crash evidence. Known open work, carried from the review rounds on #1849 rather than hidden: - No in-repo end-to-end coverage of the trusted dispatch path. This is the gap that motivated the split; the three-node harper-pro suite has to run against this revision before it merges. - #2294 — nothing orders two concurrently-originated deploys, so both can report success with different versions live. The barrier orders stage-before-activate, not deploy-against-deploy. - #2295 — a `package:` deploy resolves per node, so the barrier guarantees "everyone staged something", not "everyone staged the same bytes". Stacked on #1849; the diff here is exactly the coordination protocol.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a two-phase component deployment process (stage then activate) to ensure cluster-wide deployments are all-or-nothing. It introduces the component_deploy_phase operation for peer replication, updates the CLI to support stage and activate verbs, adds capability checks to prevent sending staged deploys to older servers, and includes comprehensive unit and integration tests. Feedback on the tests suggests using optional chaining (?.) when asserting on nested properties of parsed JSON manifests or lockfiles to prevent unhandled TypeErrors during future structure changes.
| const applicationLock = JSON.parse(await fs.readFile(path.join(configRoot, 'harper-application-lock.json'))); | ||
| assert.strictEqual(applicationLock.applications[project], undefined); |
There was a problem hiding this comment.
When asserting on nested properties of parsed JSON manifests or lockfiles (such as harper-application-lock.json), use optional chaining (?.) to ensure that future structure changes lead to a focused assertion failure rather than an unhandled TypeError.
| const applicationLock = JSON.parse(await fs.readFile(path.join(configRoot, 'harper-application-lock.json'))); | |
| assert.strictEqual(applicationLock.applications[project], undefined); | |
| const applicationLock = JSON.parse(await fs.readFile(path.join(configRoot, 'harper-application-lock.json'))); | |
| assert.strictEqual(applicationLock?.applications?.[project], undefined); |
References
- When asserting on nested properties of parsed JSON manifests (such as package.json or lockfiles) in tests, use optional chaining (?.) to ensure that future structure changes lead to a focused assertion failure rather than an unhandled TypeError.
|
|
||
| await operations.deployComponent({ project, deployment_id: staged.deployment_id }); | ||
|
|
||
| assert.strictEqual(readConfigFile()[project].package, packageIdentifier); |
There was a problem hiding this comment.
When asserting on nested properties of parsed JSON manifests (such as harper-config.yaml read via readConfigFile()), use optional chaining (?.) to prevent unhandled TypeErrors if the project entry is missing.
| assert.strictEqual(readConfigFile()[project].package, packageIdentifier); | |
| assert.strictEqual(readConfigFile()?.[project]?.package, packageIdentifier); |
References
- When asserting on nested properties of parsed JSON manifests (such as package.json or lockfiles) in tests, use optional chaining (?.) to ensure that future structure changes lead to a focused assertion failure rather than an unhandled TypeError.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const requestsStagedDeploy = | ||
| req._cliVerb !== undefined || req.activate === false || req.deployment_id !== undefined || req.two_phase === true; | ||
| if (target && requestsStagedDeploy && !(await targetSupportsStagedDeploy(options))) { |
There was a problem hiding this comment.
Staged-deploy capability probe also gates harper revert
What: requestsStagedDeploy is true whenever req._cliVerb !== undefined (line 961-962). _cliVerb is set for all three deploy-family CLI verbs, including revert (OP_VERB_PROPS.revert = { operation: 'revert_component', _cliVerb: 'revert' }, line ~44). So harper revert against a remote target now requires the target to advertise componentDeployTwoPhase capability via registration_info, even though revert_component is a pre-existing, unrelated operation that doesn't depend on two-phase deploy support at all.
Why it matters: Against any remote target that supports revert_component but hasn't yet advertised the new componentDeployTwoPhase capability (i.e. every currently-deployed Harper server, or any node mid-rolling-upgrade), harper revert will now fail with "Target Harper does not advertise staged-deploy support" — a false rejection of an operation the probe was never meant to cover. This contradicts the PR's own comment on the revert entry in OP_VERB_PROPS ("_cliVerb here only drives the missing-target guard below") and the asymmetric, correctly-scoped precedent two lines below at line 974 (req.operation === 'deploy_component' && ... for the streaming-deploy probe).
Suggested fix: Scope requestsStagedDeploy to the staged-deploy verbs only, e.g. drop the bare req._cliVerb !== undefined and instead check req._cliVerb === 'stage' || req._cliVerb === 'activate' (or gate on req.operation === 'deploy_component' alongside the existing activate/deployment_id/two_phase checks), so revert_component requests never trigger the probe.
|
Found one blocker (inline): the staged-deploy capability probe in |
Review of the shrunk PR found the leftovers, which is what a review of a large deletion is for. The handler still branched on `activate`, `two_phase` and `deployment_id` after the validator dropped them — and the schema allows unknown keys, so a caller still sending the never-released staged contract would have had `activate: false` silently ignored and received a full deploy instead. Those branches are gone and the fields are now `forbidden()`, naming #2301, so they fail fast rather than doing the opposite of what was asked. The first test to hit that was one of ours that had been passing `activate: false` as a no-op. `applicationConfigFromActivationSpec` dereferenced `spec.package` without a null guard, and startup reconciliation feeds it `row.activation_spec` from rows it did not write — a row from before the spec was recorded, or a hand-edited one. That threw during boot and left the component failed closed on every restart with no recovery but repairing the row by hand. A missing spec now degrades to a config no-op. `drop_component` settled only `staged`/`activating` rows, but a deploy interrupted mid-stage rests at `staging`, and nothing else settles it: payload retention only reclaims terminal rows, so the tarball stayed pinned and `get_deployment` never converged for an already-dropped component. Docs and comments: I had deleted the caveat that the pre-swap load check is a no-op on the main thread — which is where the operations API runs deploys — so DESIGN.md was claiming a guarantee the code does not provide on the origin. It is restored and scoped explicitly. Also removed the activate-by-id consequence paragraph, and corrected the comments still describing two-phase orchestration, the `component_deploy_phase` fan-out, and `harper activate`. `claimStagedDeployment` and `settleStagedRows` have no caller in this tree now. They are marked RESERVED FOR #2301 rather than deleted: that PR is stacked directly on this one and is their only consumer, so deleting here would just mean re-adding there. The comments say plainly that no resting `staged` row producer exists in-tree until it lands. Reported by cursor-composer pre-push.
…the split took `claimStagedDeployment`, `settleStagedRows`, and `expireOldStagedDeployments` have no caller in this tree — they exist only for the stacked coordination PR, and were carried here behind RESERVED comments to save re-adding them there. That is exactly backwards for a PR whose point was to shrink to what this tree can verify: dead code reviewed here is dead code nobody can exercise here. They land with their caller, along with the ten unit tests that were their only consumer. Removing the staged-deploy capability probe also deleted the whole `deploy_component cross-version compatibility` block, which covered more than the probe: the pre-5.1 package-JSON downgrade, the pre-5.1 directory/CBOR downgrade, the 5.1+ streaming path, and a failed version probe assuming modern. Those branches are still live in `cliOperations`, so a request-format change could have silently broken deploying to an older Harper with nothing to catch it. Restored, and checked against a mutation that disables the downgrade. DESIGN.md claimed two guarantees this branch does not provide: that every staged deploy validates the candidate loads (it is a no-op on the main thread, which is where the operations API deploys — the caveat was already documented 300 lines later, contradicting it), and that automatic payload pruning appends a `payload_dropped` event to the rows it prunes (it deliberately does not — `event_log` is append-only and a read-copy-write would lose a concurrent writer's entry). Both now describe what actually happens. Also drops comments that narrated code or restated a contract validation now forbids. Reported by codex pre-push.
Reference only — this PR will not merge, and will not be folded into #1849We're re-planning this work as a sequence of small, independently mergeable PRs. #2315 is the coordination issue. The original plan was to merge this branch back into #1849 so there'd be one reference. That isn't possible any more, and it shouldn't be forced:
Merging would mean hand-reconciling a design we've decided not to ship. So both branches stay as separate references instead, each internally coherent:
Where this design wentStep 5 in #2315 supersedes the barrier. What the barrier uniquely solved — ordering two concurrently-originated deploys — remains open as #2294, and is the one piece that genuinely needs multi-node test infrastructure we don't have. That's why it's tracked as an issue rather than queued as a PR. The code here stays available to lift from; it went through several rounds of cross-model review. Reviewers: no action needed. Follow #2315. |
1 participant
Why this is a separate PR
#1849 originally carried both halves: the per-node staged deploy, and the cluster-wide protocol that coordinates it. Across four review rounds every escaped defect traced to the second half, and twice a unit test passed against genuinely broken behavior — once because
replicateOperationwas stubbed, once because a payload was present but not replayable. That is not a coincidence: the unit harness mocks replication and enters the authorization bypass directly, so it cannot observe a peer at all.So the halves were split. #1849 keeps everything a single node can prove. This PR carries the part that needs a cluster to prove, and should not merge on unit evidence alone.
What this adds on top of #1849
component_deploy_phase— a trusted peer-only operation carrying the phase and deployment id. Authorization travels in AsyncLocalStorage rather than on the request, so it is unreachable over HTTP with ordinary credentials, and an older peer rejects an unknown operation instead of misreading a phase marker as a one-shot deploy.npm installfails during staging while the live component is untouched on every node.activate: falsereturns a stageddeployment_id;deployment_idactivates it later;two_phase: falseforces single-phase. Plus theharper stage/harper activateCLI verbs and the capability probe that stops a staged request reaching a server that would silently deploy it live.persist: falseso the replicated row has exactly one writer; their crash evidence is local activation artifacts, not the row. Without this, a peer's lateactivatingcould land after the origin wrotesuccessand park a converged deploy in a non-terminal status forever.What must happen before this leaves draft
persist: falsecrash recovery are all unverified here. This is the whole reason for the split.package:deploy resolves per node, so the barrier guarantees "everyone staged something", not "everyone staged the same bytes". A movinglatest, a semver range, or a git branch can resolve differently per node while every stage reports success.Review history
The code here has been through several cross-model rounds while it lived on #1849, and the fixes are in its commit history: revert compensation keyed on commit attempt rather than success, activation evidence classified under the component lock, the origin-owns-the-row change, retention protecting anything newer than the returning request, and the one-shot payload actually being replayable. Those rounds are also what surfaced the coverage gap above — they kept finding real defects that the tests could not.