feat(deploy): two-phase stage/activate for deploy_component (draft/RFC)#1849
feat(deploy): two-phase stage/activate for deploy_component (draft/RFC)#1849dawsontoth wants to merge 26 commits into
Conversation
Split deploy_component into two replicated phases so a cluster deploy is all-or-nothing at go-live, and expose each phase as a first-class operation. - stage_component (phase 1): build the incoming version — download/npm pack (incl. git clone), extract, npm install — into a hidden `.deploy-staging` dir on every node. Never touches the live component dir, writes no config, never restarts, so it's safe to run cluster-wide and gate on. - activate_component (phase 2): atomically rename the staged copy into the live path and restart; persist root config for a `package` deploy at go-live. - deploy_component orchestrates stage -> (barrier: every node staged OK) -> activate. Request/response contract unchanged; SSE now emits stage/activate phases. `two_phase: false` forces the legacy one-shot path, preserved verbatim for opt-out, for peers replaying a one-shot deploy, and when `system` isn't replicated. Application.ts gains stageApplication/activateApplication/discardStagedApplication; extract/install now build into `buildDirPath` (defaults to the live dir, so the one-shot path, boot install, and direct extractApplication callers are unchanged). Staging lives under the components root — same filesystem — so go-live is an atomic rename (an os.tmpdir() location risks EXDEV and a slow copy at the worst moment). Adds unit tests for the stage/activate/discard primitives and the new validators; DESIGN.md documents the model and the atomic-rename/filesystem tradeoff. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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 introduces a two-phase deployment process (stage then activate) for components to ensure all-or-nothing cluster-wide deploys. Staging builds the incoming version into a hidden staging directory, while activation atomically swaps it into the live path. The review feedback highlights two critical issues in components/Application.ts: first, recursively deleting the parent staging directory during cleanup can accidentally destroy parallel staged builds for the same component; second, using access to check directory existence follows symlinks, which will fail on dangling symlinks and cause subsequent operations to throw EEXIST. Both comments provide actionable code suggestions to resolve these issues.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed; no blockers found. |
…ling-safe cleanup Three fixes to the two-phase staging path: - Create the per-deploy staging parent before extraction. For a `package` deploy the first filesystem touch is the `npm pack`/git-clone spawn, whose cwd is dirname(stagingDirPath); it didn't exist yet, so the spawn failed with `ENOENT posix_spawn` (surfaced by the deploy-from-github integration test; the payload-only unit tests never hit the spawn). stageApplication now mkdirs it. - moveDirAside uses lstat, not access(F_OK): access follows symlinks, so a DANGLING symlink at the target reported ENOENT and was skipped, then mkdir failed EEXIST. lstat sees the link itself. (Gemini review.) - Reorder staging to .deploy-staging/<deploymentId>/<name> (was <name>/<id>). The leaf basename is now the component name, which the pre-go-live validation load needs (componentLoader keys ApplicationScope/status off basename); and each deploy gets its own parent, so cleanup can't sweep a parallel/queued deploy's staged build. activate cleanup is a non-recursive rmdir of that parent (empty-only). (Gemini review.) Adds regression tests: a `file:`-tarball package-path stage, sibling-build survival across activate, and dangling-symlink aside on activate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nt, one-shot Adds orchestration tests that call operations.stageComponent / activateComponent / deployComponent(two_phase:false) directly, stubbing the build/swap primitives, blob ingest, and credential resolution so they assert control flow (which primitive runs, replication + restart firing, response shape) without npm/network or the component loader. Covers the two new first-class operations and the legacy one-shot path (previously only exercised indirectly). Addresses the coverage gap flagged in review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The two-phase deploy lifecycle emits stage/activate phases instead of the one-shot prepare/replicate. Update the deployment-tracking integration tests to match: a failed install is now recorded against phase 'stage' (not 'prepare'), and a successful deploy's event_log spine is stage → activate (not prepare → replicate). No behavior change — the recorded status='failed', error.message, install_output, deployment_id, error/payload_dropped events are all still asserted and preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ewire) AGENTS.md forbids new sinon/rewire in unit tests; the prior version of this file stubbed the build primitives via rewire/sinon. Rewritten in the deployStaging.test.js style: plain node:assert against the real operation handlers, driving real tarball payloads through a real temp components root. Now exercises stage_component, activate_component, deploy_component (two-phase default), and deploy_component(two_phase:false) end-to-end — asserting the staged/live directories on disk, the deployment_id, the no-restart message, and the deployment_id requirement — which is stronger coverage than the stubs gave. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve the DESIGN.md conflict by keeping both appended sections (two-phase deploy + universalHeaders). main did not touch any of the deploy-family source files, so the code merged cleanly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve the DESIGN.md conflict by keeping both appended sections (two-phase deploy + scheduler). main did not touch any deploy-family source file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ency call Unrelated to the two-phase deploy feature — fixes a pre-existing type error on main (introduced by #1688's commit-latency analytics) that main's other build steps swallow via `|| true`/`continue-on-error`, but the newly-added Next.js adapter integration workflow (#1385) runs the build without that tolerance and so fails on it (tsc exit 2) for every PR built on current main. `commitResolution` is declared with the wider `Promise<number | void> | void` (the abort() branch reassigns it), but at this call site it is the `commit()` promise already cast to `Promise<void>` on the line above. recordCommitLatency only awaits it for timing and never reads the resolved value, so the cast is type-only with no runtime effect — matching the author's existing cast and safety comment two lines up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses the draft PR's open questions:
- Reversibility (Q3): activate now RETAINS the outgoing live version as
.deploy-previous/<name> (one per component, older evicted) instead of
discarding it, and a new revert_component operation swaps live <-> previous
cluster-wide (replicated like activate). The swap is bidirectional, so
reverting a revert rolls forward again. This unlocks customer-driven rollback
(deploy -> run your own health checks -> revert if unhappy) and deploy_component
gains an opt-in revert_on_failure that rolls the whole cluster back when the
activate phase leaves it split across versions. New revertApplication primitive,
operation handler, validator, enum, authorization, SSE, and 'reverting' status.
- CLI (Q4): `harper stage` (packages + uploads like `harper deploy`, no go-live),
`harper activate`, and `harper revert` — aliases + SSE progress wired in
bin/cliOperations.ts; stage shares deploy's cwd-packaging prep.
- Mixed-version clusters (Q1): clusters stay in lockstep on their version, so the
rolling-upgrade caveat and any capability-negotiation framing are dropped from
DESIGN.md / validator comments.
- Replicator contract (Q2): documented in DESIGN.md from harper-pro's
replicator.ts — replicateOperation fans to server.nodes, sets replicated=false
as the peer re-fan guard, surfaces per-peer {status:'failed',reason,node},
authenticates by node cert, and runs replicated ops with authorize=false for
trusted nodes (skipping the permission gate). Confirms the sub-op design is
structurally identical to the proven deploy_component fan-out.
Adds 12 tests: retention + bidirectional revert primitives, revert operation
end-to-end, and the revert/revert_on_failure validators. 39 deploy unit tests pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
replicateOperation fans to every node with no subset targeting, so the prior revert_on_failure reverted failed-activate peers too. But a peer that failed to activate never ran retainAsPrevious — its live dir is still the correct pre-deploy version and its .deploy-previous holds a copy from two deploys ago — so reverting it rolled it back an EXTRA version, splitting the cluster across three versions instead of reconverging on one. Scope the swap-back to the origin plus the peers that actually activated, sent point-to-point via sendOperationToNode (skipping recorder.getFailedPeers()), and leave the failed peers on their already-correct version. (Review catch.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fan-out The origin is reverted directly, then activatedPeers (server.nodes minus failed peers) was sent a point-to-point revert too. server.nodes normally excludes self (knownNodes populates it with a `!== getThisNodeName()` guard), but a not-yet-named node can slip in, and the established convention (bin/restart.ts) guards self on every point-to-point fan-out — without it, a self-directed revert would run the handler again and, because the swap is bidirectional, flip the origin back to the just-activated (broken) version. Filter out getThisNodeName() alongside the failed peers. (Review catch.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The revert_on_failure fan-out needs a live multi-node cluster to run end-to-end, so its node-targeting (skip failed peers, skip self) had no unit coverage — which is why both bugs there were caught by review rather than a test. Extract that pure set-difference into an exported selectRevertTargets(nodes, failedPeers, thisNode) and cover it directly with plain assert: excludes self (the bidirectional double-revert guard), excludes every failed peer, and is safe with empty/undefined inputs and null-node failed entries. deployComponentTwoPhase now calls the helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @kriszyp — draft or not, the review landed the key question squarely. A few calls I deliberately left for a maintainer rather than deciding unilaterally — your read would be welcome:
Still a draft on purpose: the cross-node fan-out is validated against 🤖 Automated update via Claude Code on behalf of @dawsontoth |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
I think there is an associated PR comment associated with this review (in draft state that needs a submission).
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Really like the direction here — building the incoming version fully off to the side and gating the whole cluster on a successful stage before anyone flips is a genuine correctness win over the in-place, no-barrier one-shot. The build-target seam (buildDirPath defaulting to the live dir) is a nice, low-blast-radius way to add staging without disturbing the one-shot / boot-install / direct-extractApplication callers, and the two criticals from the first review round (recursive cleanup destroying parallel staged builds; the access→lstat dangling-symlink case) are both cleanly resolved in the current tree. CI is green across the full matrix. Comments below are all RFC-level — one architectural question that I think is bigger than the naming you flagged, plus a few smaller items.
The big one: do stage_component / activate_component need to be public operations?
As implemented these are fully public, super_user-gated operations — registered in OPERATIONS_ENUM, the op function map, operation_authorization, and the SSE set, so they're reachable over the operations API exactly like deploy_component (they're not yet wired into the CLI). Meanwhile deploy_component on the origin doesn't actually call these handlers — it calls the stageApplication/activateApplication primitives directly and only uses the two named ops as the replication wire format to fan out to peers. So the two ops are pulling double duty: internal peer fan-out and a new operator-facing "stage now / activate later" capability.
Those two needs are separable, and only one of them arguably needs public surface:
- Peer fan-out needs a wire representation, but not two named public ops.
deploy_componentalready tags replicated bodies with_deploymentId(the_-prefixed internal convention peers already branch on); the peer stage/activate work could ride ondeploy_componentwith an internal_phase: 'stage' | 'activate'marker — same mechanism, zero new public surface. - Operator stage-now/activate-later is only worth permanent API surface if it's a committed product feature (pre-stage the cluster, flip later). Right now it reads as emergent from the implementation rather than a requirement.
If we want to keep the surface at one op, I think the cleanest shape is a convergence model: deploy_component default = full stage+activate (contract unchanged); deploy_component({ activate: false }) = stop after the cluster-wide staged barrier and return the deployment_id in a staged state; deploy_component({ deployment_id }) on an already-staged deployment = the staged dir already exists (content-addressed by deployment id), so skip fetch/install and just activate. That reads as "deploy_component does whatever remaining work to get the component deployed," and it reuses the deployment_id that activate_component already consumes — mostly a routing change rather than new machinery.
The one case for keeping them split is RBAC: if we want a CI role that can stage but a separate approver role that must activate, distinct operations express that governance story naturally (as would distinct CLI verbs). If that's something the product wants, the two ops earn their keep; if not, I'd fold them back into deploy_component. Either way I think this surface-shape decision is worth settling before de-drafting — happy to help sketch whichever direction we pick.
Smaller findings
1. Peers skip the load-validation during stage — a behavior change from one-shot worth a decision. In the one-shot path every execution including replicated peers ran the component load to surface load-time errors early. In two-phase, peers run stageComponent, which never calls loadValidateComponent — only the origin validates (in deployComponentTwoPhase). So a component that installs cleanly but fails to load passes the stage barrier on peers and only surfaces at activate/restart — which is the half-baked-peer outcome the barrier is meant to prevent, just for load-time rather than install-time faults. Standalone stage_component never load-validates at all. Worth either running the validation load in the stage handler, or explicitly scoping the all-or-nothing guarantee to fetch/install (not load) in the docs.
2. activate_component's deployment_id is unvalidated and flows into a filesystem path. activateComponentValidator has deployment_id: Joi.string().optional() with no pattern, unlike project (/^[a-zA-Z0-9-_]+$/). In activateComponent it becomes stagingId, and stagingDirPath = join(dirname(dirPath), DEPLOY_STAGING_DIR, stagingId, name); a value containing ../ resolves the staging source outside .deploy-staging, and activateApplication then renames whatever it finds there into the live components dir. It's super_user-only (who can already deploy arbitrary code), so this is defense-in-depth rather than privilege escalation — but constraining deployment_id to the same safe charset as project is a cheap guard, and auth/encoding-boundary regressions are exactly the class most likely to slip a static review.
3. Activate-phase partial failure (already noted as deferred #3, just confirming the shape). The origin swaps + restarts before the peer activate gate runs, and a failed origin swap leaves the live dir moved-aside with root config already written and no automatic restore. It's low-probability (same-fs rename) and you've explicitly deferred rollback-on-partial-activate — no action needed here, just flagging that the moved-aside/no-restore window is understood.
Confirmations (verified, all good)
- Watcher claim holds.
EntryHandlerwatchescomponent.commonPatternBase, derived per live component dir —.deploy-stagingunder the components root is not a watched base, so building there fires no restart-on-change storm, and it's correctly added to thegetComponentsskip list. Theos.tmpdir()-vs-EXDEVrationale for keeping staging on-volume is sound. - Legacy one-shot preserved verbatim behind
two_phase: false/ non-replicated-system/ peer-replay, and the big refactor into shared helpers (sourceExtractionPayload,resolveNodeCredentials,buildDeployApplication,loadValidateComponent,finalizeDeployFailure,maybeReclaimPayload) is a faithful move — I diffed the extracted bodies against the originals. harper-proreplicateOperation(your open question #2) I can't assess from this repo — the real cross-node fan-out for the two new ops lives there and needs validating before they're trusted in a real cluster.
On the two things you asked about
- Naming (
stage/activate): I'd keep it. The verb pair reads cleanly in the SSE stream and dodges thepromote/commitoverloading. (Moot if we fold back intodeploy_component.) - Mixed-version: the
two_phase: false/ignore_replication_errorsescape hatches are fine for now, and capability negotiation is the right deferred fix. Given main is 5.2, let's make sure a capability-probe follow-up is tracked before two-phase ships as the default.
Nice work — the correctness core is solid and this is close. The surface-shape question is really the only thing I'd want resolved before it leaves draft.
— KrAIs (Kris's Claude agent) 🤖
…ring stage Two review findings from kriszyp's review: - deployment_id becomes a staging-dir path segment (.deploy-staging/<deployment_id>/<name>), but activate/revert validators accepted any string — a `../` value could resolve the staging source outside .deploy-staging. Constrain it to the same safe charset as `project` (defense-in-depth; the ops are super_user-only). +tests. - stageComponent never ran the pre-go-live component load check, so a standalone stage_component didn't validate at all and the stage barrier didn't cover load-time faults. Run loadValidateComponent on the staged build in the stage handler (matches deployComponentTwoPhase's origin check). It is a no-op on the main thread where replicated peer executions run (app code must not load there), so DESIGN.md now scopes the cluster-wide barrier guarantee to fetch + install and documents load-validation as origin/worker-side. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @kriszyp — genuinely useful review. Pushed fixes for the two concrete findings; the big surface-shape question I'm taking back to the team rather than deciding unilaterally. Smaller findings
Naming / mixed-version. Keeping The big one — public ops vs. fold into 🤖 Automated update via Claude Code on behalf of @dawsontoth |
|
Another reason that supports splitting them is the deployment strategies it unlocks, and customer facing features that we can enable. Especially on the rollback side of the equation. My theory? Most customers don't need to be able to rollback to something a week or a month ago. They only need to be able to rollback the 1 bad rollout that just happened, and they want it to happen really fast. By splitting the ops, we can unlock this across the cluster without having to do any package or secret resolution or artifact download or installation or anything else. |
Resolve the DatabaseTransaction.ts conflict by combining both changes: keep my `as Promise<void>` cast on the recordCommitLatency call (the Next.js-workflow build fix) AND main's new write-queue-depth accounting (enterWriteQueue / leaveWriteQueue) that landed on the same line. main did not touch any deploy-family source file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… _phase)
Per review (harper#1849): stage_component / activate_component are no longer
public operations. Their two-phase cluster fan-out now rides deploy_component
itself, tagged with an internal `_phase: 'stage' | 'activate'` marker, and the
operator-facing capability is exposed as deploy_component properties:
- deploy_component (default): full stage + activate (contract unchanged).
- deploy_component({ activate: false }): stage cluster-wide and stop, returning
the deployment_id in a `staged` state.
- deploy_component({ deployment_id }): activate a previously-staged deployment.
deployComponent now dispatches replicated _phase executions to internal
deployPhaseStage / deployPhaseActivate handlers, and public calls to the
orchestrator / activate-existing path. Removed the two ops from OPERATIONS_ENUM,
the op function map, operation_authorization, the SSE set, and their validators;
added activate + deployment_id (safe charset) to deployComponentValidator. Added
markDeploymentTerminal to flip a stage-and-stop row to success on later activate.
revert_component stays a distinct public op (a rollback, not a deploy phase).
CLI: `harper stage` -> deploy_component activate=false (still packages the cwd);
`harper activate deployment_id=<id>` -> deploy_component deployment_id=<id> (no
upload). Tests + DESIGN.md updated to the single-public-op shape; 34 deploy unit
tests pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Surface-shape decision landed (thanks @kriszyp): folded
DESIGN.md updated to the single-public-op model; 34 deploy unit tests pass. 🤖 Automated update via Claude Code on behalf of @dawsontoth |
…age)
With deploy_component({ activate: false }) leaving staged builds around for a
later deploy_component({ deployment_id }), those not-yet-activated builds could
accumulate without limit (a full deploy consumes its build on activate, so only
stage-and-stops pile up). stageApplication now evicts the oldest such builds for
a component beyond deployment_stagingRetention_maxCount (default 5) after each
successful stage — always keeping the just-staged one plus the newest N-1 by
mtime. Eviction is best-effort but awaited so the count is settled when the
stage returns.
Per the harper#1849 decision: retention is count-only and fully automatic (no
new op); hdb_deployment rows stay as the audit trail (payload blobs already
self-reclaim by size). Consequence: activating a deployment_id that has aged
out of the window fails "no staged build found", as expected. DESIGN.md +
a retention test added.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t full deploy) After folding activate into deploy_component, `harper activate` mapped to deploy_component with no marker. deployComponent only routes to activate-existing when deployment_id is truthy — otherwise it falls through to a full two-phase deploy, and packageCwdForUpload (skips only for package || deployment_id) would tar + upload the CWD. So `harper activate project=x` with a missing/mistyped deployment_id silently ran a brand-new deploy from local files. The `activate` verb now carries a CLI-internal `_verb` marker, and verbRequirementError (pure, exported) rejects it when deployment_id is absent — checked BEFORE packaging, with a clear message, so it fails fast instead of deploying. The marker is stripped before the request body. Adds CLI verb tests covering stage/activate mapping and the missing-deployment_id guard. (Review catch.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Docs companion: HarperFast/documentation#599 (draft) — documents the two-phase 🤖 Automated update via Claude Code on behalf of @dawsontoth |
…de/deploy-component-two-phase-94969a # Conflicts: # components/Application.ts
deployPhaseOperations deploys genuinely-new components, which post-merge trips deployComponent's requestRestart() (harper#674 new-component scoping) and sets the process-wide restart-needed shared buffer. That leaked into requestRestart.test.js's "pristine buffer" assertion, failing it on suite ordering alone. Restore the buffer in the existing after() hook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The merge with main brought harper#674/#1806: a deploy_component with restart:false must set get_status restartRequired for a genuinely-new (never-loaded) component, scoped so a redeploy of an already-active component stays quiet. Main implemented this only in the one-shot deploy path (requestRestart() gated on Application.isNewComponent). Two-phase deploy — the default whenever the system db is replicated — never carried that behavior, so inactive-component-404.test.ts failed: a fresh deploy left restartRequired false and the actionable 404 never surfaced. Two-phase also can't set isNewComponent the way the one-shot path does: stageApplication extracts into a fresh staging dir, so extractApplication never sees the live directory and isNewComponent stayed default-true for everything (which would wrongly mark a restart on a redeploy). Fix both: - activateApplication now sets isNewComponent from whether the live dir existed BEFORE the swap (the two-phase equivalent of extract's in-place check) — true for a first deploy, false for a redeploy. - Extract markRestartRequiredForNewComponent() and call it on every no-restart path: one-shot (unchanged behavior), two-phase origin, activate-existing, and the per-node peer activate — so a new component deployed cluster-wide with restart:false reports restartRequired on every node, matching the one-shot peer behavior. Unit coverage: deployStaging asserts activate sets isNewComponent true for a first-ever activate and false when replacing an existing live version. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The activate-existing test only checked res.message, which never mentions "restart" on the no-restart path — so it passed whether or not the restart-required marking ran. Assert restartNeeded() directly instead, and cover the legs that had no fast test at all: - activate-existing (deployComponentActivateExisting): now asserts a never-live component marks a restart. - origin two-phase (deployComponentTwoPhase): asserts a fresh deploy marks a restart. - peer _phase:activate (deployPhaseActivate): new test driving the peer activate leg via the public op with internal markers, over a directly staged build — the per-node marking had zero coverage. - redeploy negative: a redeploy of an already-live component must NOT self-request a restart (harper#1806), guarding activateApplication's isNewComponent:false path at the operation level. beforeEach resets the process-wide restart buffer so each assertion reflects only its own deploy. Regressions on any leg now fail fast here instead of only in the inactive-component-404 integration test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Conflict in components/operations.js: main (#1809/#1821) wrapped the throwaway deploy-validation load in runWithDeployValidationGuard so process-wide server.* registrations (registerOperation, setMcpQuotaHandler) are no-op'd during validation. This branch had already refactored that inline load into the loadValidateComponent() helper. Resolved by keeping the helper and porting main's guard into it — which also extends the guard to the two-phase staging validation load (loadValidateComponent serves both the one-shot and two-phase paths), not just the one-shot path main touched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Conflicts in components/operations.js (main's deploy-blob-read-retry work, #1838 / e02efb4 / 11cff13 / f296376): - Import union: keep markDeploymentTerminal (ours) alongside main's new readPayloadBlobWithRetry + coerceTimeoutMs from deploymentRecorder. - deployComponentOneShot payload/credential sourcing: main hardened the inline block with readPayloadBlobWithRetry (retry peer blob read on a transient 503 stall) and coerceTimeoutMs (dedup'd timeout coercion for both the payload-row wait and the credential grace period). This branch had already refactored that block into the sourceExtractionPayload / resolveNodeCredentials helpers, from a pre-fix version. Resolved by keeping the helpers and porting both fixes into them — which also extends the blob-read retry to the two-phase origin AND peer-stage paths (both route through sourceExtractionPayload), not just the one-shot path. deploymentRecorder.test.js (main's retry tests) + full deploy suite: 114 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d row
markDeploymentTerminal read the deployment row via table.get() and assigned
onto it (row.status = …). table.get() returns a read-only record, so that
throws "Cannot assign to read only property 'status'". The one caller
(deploy_component({ deployment_id }) taking a stage-and-stop build live)
treats the failure as best-effort, so it only surfaced as a caught warning —
but the observability write never landed: get_deployment kept reporting
'staged' after the component had gone live.
Use table.patch(id, { status, completed_at }) instead — the idiomatic
partial update (see resources/dataLoader.ts). It avoids the read-only
mutation and, unlike a spread-and-put, can't truncate the rest of the row.
Regression test: deploymentRecorder mock now models the real table
(read-only get() via an opt-in freezeGet, plus patch()), and a new
markDeploymentTerminal suite asserts the row flips to a terminal status and
is a no-op for an absent id. A revert to the row.status = … form throws
against the frozen get(), so the regression can't return silently.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I'm adding get_deployment_payload and delete_deployment_payload operations (linked above) and wanted to make sure I am still aligned with you. Generated summary:
But the defaults should stay conservative. Free/low-tier instances have 5GB total; N retained copies of a large app payload quietly competing with the customer's actual database for quota is a nasty failure mode.
OK? |
|
Yup, ok! |
|
Big +1 on the stage-barrier framing being the right foundation here. On the specifics:
Proposing we lock 1–3 at the team call so this PR and your #1893 land aligned; keeping this PR's surface as-is until then. 🤖 Drafted via Claude Code |
Conflict in resources/DatabaseTransaction.ts (comment-only): main's TS2345 commit-path fix (#18367779a / #1da1c65a7) landed the same `commitResolution as Promise<void>` cast this branch carried from an earlier merge — only the explanatory comment differed. Took main's comment; the code is identical on both sides. No deploy files touched by this merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Why
Today the whole deploy runs as one replicated operation. On every node, extract +
npm install(the slow, failure-prone work — git clone, registry install) happen in place in the live component directory, right before restart. The only atomic bit that exists (extractApplication) just renames the old dir aside; the new version is still built in place. There is no cluster-wide barrier — a peer can failnpm installand be left half-baked while other nodes have already restarted onto the new code. Nodes flip independently.What this does
Two phases, each a distinct replicated operation that
deploy_componentorchestrates:stage_componentnpm pack(incl. git clone), extract,npm installinto a hidden.deploy-staging/<name>/<deploymentId>dir on every nodeactivate_componentrename(staging → live)+ restart; persist root config for apackagedeploy at go-livedeploy_componentnow: stage on origin → replicatestage_componentto peers → wait for every node to report a good stage (the barrier) → replicateactivate_component. If a node can't fetch the package or failsnpm install, it fails during staging and the live component is untouched on every node. Go-live shrinks to a fast atomic swap.deployment_idin the response, same SSE stream (phase names change:stage/activateinstead ofprepare/replicate, and the generic CLI renderer already handles any phase name).two_phase: falseforces the legacy one-shot path, which is preserved verbatim (deployComponentOneShot).npm installruns off to the side, so the live component keeps serving during a long install and go-live is just the swap.On the names (please weigh in)
Went with
stage_component/activate_component— a crisp verb pair that reads well in the stream (staging…,staged on 3/3,activating…). Consideredpromote(release/blue-green flavor) andcommit(2PC flavor, but overloaded). Easy to change now, painful after release.Why staging lives under the components root (not
os.tmpdir())Go-live is
rename(stagingDir, liveDir), atomic only when both share a filesystem.os.tmpdir()is frequently a different mount →EXDEV→ slow recursive copy at the exact moment you want an instant swap. So staging is a hidden.deploy-stagingdir under the components root: same volume (atomic swap), dot-prefixed so the loader ignores it, and not the watched base of any component's file watcher — so building there fires no restart-on-change events and needs nodeploy:startsuppression (that's now scoped to the activate swap only).Key implementation notes
extractApplication/installApplicationbuild intoapplication.buildDirPath, which defaults to the live dir — so the one-shot path, boot-timeinstallApplications, and directextractApplicationcallers are unchanged.stageApplicationrepoints it at staging for the duration of a stage;activateApplicationswaps and resets it.activate_component(a separate op, and on peers a separate invocation fromstage_component) can find what the stage built.hdb_deploymentstatusesstaging/staged/activating; a standalonestage_componentleaves the row instaged(build exists cluster-wide, nothing live yet).Tests
unitTests/components/deployStaging.test.js— stage/activate/discard primitives end-to-end on the real filesystem (staging doesn't touch live; atomic swap; old version moved aside; failed stage leaves live untouched; non-colliding staging dirs). 6/6 green locally.unitTests/components/deployPhaseValidators.test.js— the two new validators + thetwo_phaseflag. 13/13 green locally.deploy_componentunit tests to assert the two-phase path (stage + activate) instead of the old in-placeprepareApplication.gitSemverCommittish8/8,gitCredentialClone6/6,packageComponent6/6).Update — the four open questions are resolved
two_phase: false+ thesystem-not-replicated fallback remain, but only for those specific cases, not version skew.)harper-pro. Readharper-pro/replication/replicator.ts:replicateOperationfans the op toserver.nodes, setsreplicated = falseas the peer re-fan guard, surfaces per-peer{status:'failed', reason, node}(never throws), authenticates node-to-node by TLS cert, and runs the received op viaserver.operation(data, {user}, !isAuthorizedNode)—authorize = falsefor a trusted node, so a replicated super-user op skips the permission gate. Mystage/activate/revertops (samepermission(true,[]), dispatched by name) are structurally identical to the long-provendeploy_componentfan-out. Documented in DESIGN.md.activatenow retains the outgoing version as.deploy-previous/<name>instead of discarding it, and a newrevert_componentswaps live ↔ previous cluster-wide (bidirectional — reverting a revert rolls forward). This unlocks customer-driven rollback (deploy → run your own health checks →revertif unhappy, even on a healthy-looking cluster) and an opt-inrevert_on_failureondeploy_componentthat rolls the whole cluster back when the activate phase leaves it split across versions.harper stage(packages + uploads likeharper deploy, no go-live),harper activate,harper revert— with SSE progress.What changed in this round
revertApplicationprimitive +revert_componentoperation (validator, enum, authorization, SSE,revertingstatus).activateApplicationretains one previous per component (.deploy-previous/<name>, older evicted);deploy_componentgainsrevert_on_failure.stage/activate/revertaliases inbin/cliOperations.ts.revert_componentend-to-end, revert/revert_on_failurevalidators) — 39 deploy unit tests pass.Still deferred
revert_on_failure), best-effort, and not the default — a policy call for review.deployment_idbeyond the auditrollback_of.🤖 Generated with Claude Code