Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
183 changes: 135 additions & 48 deletions DESIGN.md

Large diffs are not rendered by default.

48 changes: 46 additions & 2 deletions bin/cliOperations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,27 @@ const OP_ALIASES = {
package: 'package_component',
};

// `stage` and `activate` are sugar over `deploy_component` — there are no separate stage/activate
// operations to find.
const OP_VERB_PROPS: Record<string, Record<string, unknown>> = {
// `harper revert` uploads nothing: the version it activates is already on disk. `_cliVerb` is a
// CLI-internal marker (stripped before the request is sent) that drives the missing-target guard below.
stage: { operation: 'deploy_component', activate: false, _cliVerb: 'stage' },
// `_cliVerb` is a CLI-internal marker (stripped before the request is sent) so verbRequirementError
// can enforce that `harper activate` carries a deployment_id — without it, deploy_component's generic
// "no deployment_id → full deploy" fallback would silently build a brand-new deploy from the CWD.
// It also tells the staged-deploy capability probe that this invocation needs two-phase support.
activate: { operation: 'deploy_component', _cliVerb: 'activate' },
// `harper revert` uploads nothing: the version it activates is already on every node. `_cliVerb`
// here only drives the missing-target guard below.
revert: { operation: 'revert_component', _cliVerb: 'revert' },
};

// Guard CLI-verb requirements that the operation itself can't enforce (the op has no notion of which
// verb invoked it). Returns an error message, or null when the request is fine. Pure + exported so it
// is unit-testable without the network/process-exit machinery in cliOperations.
function verbRequirementError(req: any): string | null {
if (req._cliVerb === 'activate' && !req.deployment_id) {
return '`harper activate` requires a deployment_id from a prior `harper stage` — usage: harper activate project=<name> deployment_id=<id>';
}
// revert_component requires its target so a retry can't toggle the rejected release back in. Caught
// here too, so the CLI names the flag instead of surfacing a raw validation error.
if (req._cliVerb === 'revert' && !req.to_deployment_id) {
Expand Down Expand Up @@ -186,6 +197,22 @@ async function targetSupportsStreamingDeploy(options: any): Promise<boolean> {
}
}

async function targetSupportsStagedDeploy(options: any): Promise<boolean> {
try {
const probeOptions = {
...options,
headers: { ...options.headers, Accept: 'application/json' },
timeout: CLI_OPERATION_TIMEOUT_MS,
};
delete probeOptions.streamResponse;
const response = await httpRequest(probeOptions, { operation: 'registration_info' });
if (response.statusCode !== 200 || !response.body) return false;
return JSON.parse(response.body)?.capabilities?.componentDeployTwoPhase === 1;
} catch {
return false;
}
}

// Wraps the local packaging stream so an fs error while tar'ing up the payload (e.g. a file
// vanishing after the pre-deploy scan, or a permissions failure reading the project tree)
// surfaces as a descriptive packaging error instead of a raw fs error code. Without this, an
Expand Down Expand Up @@ -604,6 +631,12 @@ const prepareRevert = async (req) => {
const PREPARE_OPERATION: any = {
revert_component: prepareRevert,
deploy_component: async (req) => {
// `harper activate deployment_id=<id>` takes an already-staged build live, so there is nothing to
// package — but it still needs the CWD project default every deploy-family verb gets.
if (req.deployment_id) {
req.project ||= directoryProjectName(process.cwd());
return;
}
if (req.package) {
return;
}
Expand Down Expand Up @@ -921,6 +954,17 @@ async function cliOperations(req: any, skipResponseLog = false) {
let options: any, target: any;
try {
({ options, target } = await resolveRequestOptions(req));
// Staged (two-phase) deploy controls must never reach a server that doesn't understand them: an
// older target ignores `activate: false`/`deployment_id` and deploys LIVE cluster-wide instead —
// the opposite of the operator's intent, silently. Probe before packaging so the refusal costs
// nothing. Local (domain-socket) calls hit this same build, so no probe is needed there.
const requestsStagedDeploy =
req._cliVerb !== undefined || req.activate === false || req.deployment_id !== undefined || req.two_phase === true;
if (target && requestsStagedDeploy && !(await targetSupportsStagedDeploy(options))) {
Comment on lines +961 to +963

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.

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.

throw new Error(
`Target Harper does not advertise staged-deploy support; refusing the request because an older server could deploy it live`
);
}
delete req._cliVerb;
await PREPARE_OPERATION[req.operation]?.(req);
// Streaming deploy (multipart upload + SSE progress) only works against >= 5.1 servers.
Expand Down
Loading
Loading