From 603c0dc6d342f6d7e36df55ad537f6cd6bc25208 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 23 Oct 2026 10:15:25 -0600 Subject: [PATCH 1/5] Fail a rolling restart job that did not restart every peer A replicated restart_service (the job behind deploy_component restart: 'rolling') recorded a peer it could not restart and still completed, so a caller polling get_job saw COMPLETE while that peer ran old code. It now ends ERROR naming each peer that failed or was skipped, with every peer's outcome kept in result.replicated. - restartEachPeer: retries a peer that refused for write back-pressure after the other peers (three attempts), polls get_job sequentially and keeps polling through a connection drop instead of an unhandled rejection, bounds every request to a peer, and halts the roll at a peer whose restart failed or timed out. - addJob rethrows the search/insert error it used to swallow, and executeJob refuses a missing createdJob, so a failed job insert surfaces its own error instead of "Empty job passed to parseMessage". - jobProcess keeps a failed job's err.result. Refs #3468 Dispatch-Task: harper-3468-rolling-restart-outcome Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 1 + bin/restart.ts | 139 ++++++++----- components/DESIGN.md | 12 ++ server/jobs/jobProcess.ts | 2 + server/jobs/jobs.ts | 26 +-- server/serverHelpers/serverUtilities.ts | 25 +-- .../bin/fixtures/replicatedRestartHarness.cjs | 30 +++ .../bin/replicatedRestartService.test.js | 53 +++++ unitTests/bin/restartEachPeer.test.js | 194 ++++++++++++++++++ unitTests/server/jobs/executeJob.test.js | 34 +++ unitTests/server/jobs/jobs.test.js | 39 +--- 11 files changed, 440 insertions(+), 115 deletions(-) create mode 100644 unitTests/bin/fixtures/replicatedRestartHarness.cjs create mode 100644 unitTests/bin/replicatedRestartService.test.js create mode 100644 unitTests/bin/restartEachPeer.test.js create mode 100644 unitTests/server/jobs/executeJob.test.js diff --git a/DESIGN.md b/DESIGN.md index f531770aea..ba8e9eb56f 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -187,6 +187,7 @@ Index of the design notes for the harper core: one line per note (closely relate - [Component preparation is serialized across worker threads](components/DESIGN.md#component-preparation-is-serialized-across-worker-threads) — One component transaction at a time across workers; the lifecycle broadcast sits outside the lock; the load lock is keyed by application and plugin. - [Peer-side deploy_component payload read: retryable blob stalls and `Readable.from()` cancellation](components/DESIGN.md#peer-side-deploy_component-payload-read-retryable-blob-stalls-and-readablefrom-cancellation) — Retry a `BLOB_UNAVAILABLE` read only before any byte reached the consumer; use `Readable.from()` for cancellation, not a hand-rolled `ReadableStream`. - [An origin waits for a peer's deploy answer only as long as the peer may take](components/DESIGN.md#an-origin-waits-for-a-peers-deploy-answer-only-as-long-as-the-peer-may-take) — The replicated deploy's per-peer deadline sums the peer's own allowances, so it never undercuts a healthy peer; a missed deadline is a failure with an unknown outcome, not cancellation. +- [A rolling restart job completes only if every peer restarted (`bin/restart.ts` `restartEachPeer`)](components/DESIGN.md#a-rolling-restart-job-completes-only-if-every-peer-restarted-binrestartts-restarteachpeer) — `ERROR` names each peer that failed or was skipped, with every peer's outcome in `result.replicated`; a failed restart halts the roll; only write back-pressure is retried. - [A replicated deploy reports each node's install fingerprint, and never acts on it](components/DESIGN.md#a-replicated-deploy-reports-each-nodes-install-fingerprint-and-never-acts-on-it) — Each node records the source identity its resolver already produced and the sha256 of its root lockfiles; the origin compares peers once on the aggregate and reports differences in a warning, the message and `peer_results`, without ever failing the deploy. - [A dangling symlink silently truncates the deploy tarball (`components/packageComponent.ts`)](components/DESIGN.md#a-dangling-symlink-silently-truncates-the-deploy-tarball-componentspackagecomponentts) — tar-fs treats a dangling symlink's ENOENT as end-of-stream, so `scanPackageDirectory()` pre-walks the tree · [The packer never ships the loader's links to the install (`components/packageComponent.ts`)](components/DESIGN.md#the-packer-never-ships-the-loaders-links-to-the-install-componentspackagecomponentts) — The application's `node_modules/harper` is skipped by name; a nested one only when it resolves to the running install. - [Deploy watcher generations preserve logical entry events](components/DESIGN.md#deploy-watcher-generations-preserve-logical-entry-events) — `EntryHandler` snapshots matching paths across a deploy so consumers see logical change and delete events, not raw rescan adds. diff --git a/bin/restart.ts b/bin/restart.ts index 1c24790664..d52aaabe40 100644 --- a/bin/restart.ts +++ b/bin/restart.ts @@ -19,6 +19,7 @@ const { HTTP_STATUS_CODES } = hdbErrors; import * as envMgr from '../utility/environment/environmentManager.ts'; import * as path from 'node:path'; import { rmSync } from 'node:fs'; +import { setTimeout as delay } from 'node:timers/promises'; import { getThisNodeName } from '../server/nodeName.ts'; import { armRestartExitWatchdog } from './restartExitWatchdog.ts'; import { PRODUCT_NAME } from '../utility/packageUtils.js'; @@ -30,7 +31,7 @@ const ISOLATED_TOPOLOGY_REQUEST_TIMEOUT_MS = 5000; let calledFromCli; -export { restart, restartService, activateDeploymentOnPeers }; +export { restart, restartService, activateDeploymentOnPeers, restartEachPeer }; // Add ITC event listener to main thread which will be called from child that receives restart request. if (isMainThread) { @@ -205,6 +206,93 @@ async function activateEachPeer(peers: any[], { project, deploymentId, deploymen } } +const PEER_RESTART_TIMING = { + attempts: 3, + retryDelayMs: 1000, + dispatchTimeoutMs: 60 * 1000, + pollIntervalMs: 250, + pollTimeoutMs: 30 * 1000, + jobDeadlineMs: 10 * 60 * 1000, +}; +// A peer that refused the operation for write back-pressure never started it. Errors cross replication as +// text, and 5.3 peers word it "too long of queue". +const WRITE_QUEUE_BACK_PRESSURE = /Outstanding write transactions have too long (?:a|of) queue/; + +/** + * A rolling restart: restarts each peer in turn, waiting for its restart job before starting the next. The + * job completes only if every peer's restart completed; otherwise it fails naming each peer that did not + * restart, with every peer's outcome in `result.replicated`. A peer whose restart ran and failed or timed out + * halts the roll (the code being rolled out may be what failed), so the peers after it are skipped. + */ +async function restartEachPeer(peers: any[], request: any, timing = PEER_RESTART_TIMING) { + const replication = (global as any).server.replication; + const replicated = []; + const queue = peers.map((node) => ({ node, attempt: 1, retryAt: 0 })); + let haltedBy: string | undefined; + while (queue.length > 0) { + const { node, attempt, retryAt } = queue.shift(); + if (haltedBy) { + replicated.push({ node: node.name, error: `skipped: the restart of ${haltedBy} did not complete` }); + continue; + } + if (retryAt > Date.now()) await delay(retryAt - Date.now()); + let jobId; + try { + ({ job_id: jobId } = await replication.sendOperationToNode(node, request, { + timeoutMs: timing.dispatchTimeoutMs, + })); + } catch (error) { + const message = error?.message ?? String(error); + if (attempt < timing.attempts && WRITE_QUEUE_BACK_PRESSURE.test(message)) { + hdbLogger.warn(`Restart of ${node.name} was refused (${message}); retrying after the other nodes`); + queue.push({ node, attempt: attempt + 1, retryAt: Date.now() + timing.retryDelayMs * 2 ** (attempt - 1) }); + } else replicated.push({ node: node.name, error: message }); + continue; + } + const outcome = jobId + ? await awaitPeerJob(replication, node, jobId, timing) + : { node: node.name, error: 'it answered without a restart job id' }; + replicated.push(outcome); + if (outcome.error !== undefined) haltedBy = node.name; + } + const failed = replicated.filter((outcome) => outcome.error !== undefined); + if (failed.length > 0) { + const error: any = new Error( + `Restart of ${request.service} did not complete on ${failed.length} of ${replicated.length} peer node(s): ` + + failed + .map(({ node, error }) => `${node} (${typeof error === 'string' ? error : JSON.stringify(error)})`) + .join('; ') + ); + error.result = { replicated }; + throw error; + } + return { replicated }; +} + +async function awaitPeerJob(replication: any, node: any, jobId: string, timing: typeof PEER_RESTART_TIMING) { + const deadline = Date.now() + timing.jobDeadlineMs; + while (Date.now() < deadline) { + await delay(timing.pollIntervalMs); + let response; + try { + response = await replication.sendOperationToNode( + node, + { operation: 'get_job', id: jobId }, + { timeoutMs: Math.max(1, Math.min(timing.pollTimeoutMs, deadline - Date.now())) } + ); + } catch (error) { + // The peer drops this connection while it restarts its own workers. + hdbLogger.debug?.(`Polling restart job ${jobId} on ${node.name} failed, still waiting:`, error?.message); + continue; + } + const job = response?.results?.[0]; + if (!job) return { node: node.name, error: `its restart job ${jobId} was not found` }; + if (job.status === hdbTerms.JOB_STATUS_ENUM.COMPLETE) return { node: node.name, message: job.message }; + if (job.status === hdbTerms.JOB_STATUS_ENUM.ERROR) return { node: node.name, error: job.message }; + } + return { node: node.name, error: `Timed out waiting for restart job ${jobId} to complete` }; +} + /** * Used to restart a particular service, services includes - httpWorkers * @param req @@ -291,53 +379,10 @@ async function restartService(req: any) { } }); }); - let replicatedResponses; if (req.replicated) { - req.replicated = false; // don't send a replicated flag to the nodes we are sending to - replicatedResponses = []; - for (let node of (global as any).server.nodes) { - if (node.name === getThisNodeName()) continue; - // for now, only one at a time - let job_id; - try { - ({ job_id } = await (global as any).server.replication.sendOperationToNode(node, req)); - } catch (err) { - // If request to node fails, add the error to the response and continue to the next node - replicatedResponses.push({ node: node.name, message: err.message }); - continue; - } - // wait for the job to finish by polling for the completion of the job - replicatedResponses.push( - await new Promise((resolve, reject) => { - const RETRY_INTERVAL = 250; - let retriesLeft = 2400; // 10 minutes - let interval = setInterval(async () => { - if (retriesLeft-- <= 0) { - clearInterval(interval); - let error: any = new Error('Timed out waiting for restart job to complete'); - error.replicated = replicatedResponses; // report the finished restarts - reject(error); - } - let response = await (global as any).server.replication.sendOperationToNode(node, { - operation: 'get_job', - id: job_id, - }); - const jobResult = response.results[0]; - if (jobResult.status === 'COMPLETE') { - clearInterval(interval); - resolve({ node: node.name, message: jobResult.message }); - } - if (jobResult.status === 'ERROR') { - clearInterval(interval); - let error: any = new Error(jobResult.message); - error.replicated = replicatedResponses; // report the finished restarts - reject(error); - } - }, RETRY_INTERVAL); - }) - ); - } - return { replicated: replicatedResponses }; + const thisNode = getThisNodeName(); + const peers = ((global as any).server.nodes ?? []).filter((node) => node.name !== thisNode); + return restartEachPeer(peers, { ...req, replicated: false }); } return; } diff --git a/components/DESIGN.md b/components/DESIGN.md index e3cdd74c03..2a4bb956f3 100644 --- a/components/DESIGN.md +++ b/components/DESIGN.md @@ -770,6 +770,18 @@ may still have them open, and a later drop of the now-absent application discard restart replaces is answered just before it, with any retention already decided and `null` for what is not done yet; a later failure to remove branches is only logged, and a second drop retries it (harper#3091). +## A rolling restart job completes only if every peer restarted (`bin/restart.ts` `restartEachPeer`) + +The job behind `restart: 'rolling'` (a replicated `restart_service`) restarts one peer at a time and polls +each peer's own restart job. It ends `COMPLETE` only when every peer's job did; otherwise `ERROR`, naming +each peer that failed or was skipped, with one `result.replicated` entry per peer in both outcomes +(`{ node, message }` restarted, `{ node, error }` not). A peer that could not be asked is recorded and the +roll moves on; a peer whose restart ran and failed or timed out halts it, since the release being rolled +out may be what failed. Only the write-queue back-pressure refusal is retried (after the other peers, three +attempts): it is the one refusal known to precede execution, and errors cross replication only as text. +`COMPLETE` means each peer's restart job completed, not that every worker there took new code. Enforced by +`unitTests/bin/restartEachPeer.test.js` and `replicatedRestartService.test.js` (harper#3468). + ## A replicated deploy reports each node's install fingerprint, and never acts on it Every node of a replicated `deploy_component` resolves and installs the release itself, so two nodes can end diff --git a/server/jobs/jobProcess.ts b/server/jobs/jobProcess.ts index 4d1f0a0950..6a31cb7d74 100644 --- a/server/jobs/jobProcess.ts +++ b/server/jobs/jobProcess.ts @@ -83,6 +83,8 @@ const JOB_ID = JOB_NAME.substring(4); // get_job answers a refused bulk load with its structured permission report as the message. const report = err?.http_resp_msg; jobObj.message = report !== null && typeof report === 'object' ? report : err?.message ? err.message : err; + // A job that fails part way (a rolling restart that did not reach every node) keeps what it did finish. + if (err?.result !== undefined) jobObj.result = err.result; jobObj.end_datetime = Date.now(); } finally { // A rejected updateJob must not skip handle cleanup and exit scheduling below (that would diff --git a/server/jobs/jobs.ts b/server/jobs/jobs.ts index 402141b5e5..805f58a4cc 100644 --- a/server/jobs/jobs.ts +++ b/server/jobs/jobs.ts @@ -172,14 +172,7 @@ export async function addJob(jsonBody: any) { ['id'] ); - let foundJob; - try { - foundJob = Array.from(await pSearchByValue(searchObj)); - } catch (e) { - let message = `There was an error inserting a new job: ${e}`; - log.error(message); - return result; - } + let foundJob: any = Array.from(await pSearchByValue(searchObj)); //TODO: Once https://harperdb.atlassian.net/browse/HDB-501 is resolved, this check is no longer needed. let foundValues = Array.isArray(foundJob) ? foundJob : Object.keys(foundJob); @@ -188,13 +181,7 @@ export async function addJob(jsonBody: any) { // that we will just throw an error assuming there is bad input causing the issue. if (foundValues && foundValues.length > 0) { newJob.id = uuidV4(); - try { - foundJob = await pSearchByValue(searchObj); - } catch (e) { - let message = `There was an error inserting a new job: ${e}`; - log.error(message); - return result; - } + foundJob = await pSearchByValue(searchObj); //TODO: Once https://harperdb.atlassian.net/browse/HDB-501 is resolved, this check is no longer needed. foundValues = Array.isArray(foundJob) ? foundJob : Object.keys(foundJob); if (foundValues && foundValues.length > 0) { @@ -216,14 +203,7 @@ export async function addJob(jsonBody: any) { 'id', [newJob] ); - let insertResult; - try { - insertResult = await pInsert(insertObject); - } catch (e) { - log.error(`There was an error inserting a job for job type: ${jsonBody.operation} -- ${e}`); - result.success = false; - return result; - } + const insertResult = await pInsert(insertObject); if (insertResult.inserted_hashes.length === 0) { result.message = `Had a problem creating a job with type ${newJob.operation} and id ${newJob.id}`; diff --git a/server/serverHelpers/serverUtilities.ts b/server/serverHelpers/serverUtilities.ts index a9851d4a92..3c3fe7e526 100644 --- a/server/serverHelpers/serverUtilities.ts +++ b/server/serverHelpers/serverUtilities.ts @@ -596,21 +596,18 @@ interface JobResult { export async function executeJob(json: OperationRequestBody): Promise { transformReq(json); - let newJobObject; - let result; try { - result = await jobs.addJob(json); - if (result) { - newJobObject = result.createdJob; - operationLog.info('addJob result', result); - const jobRunnerMessage = new jobRunner.RunnerMessage(newJobObject, json); - const returnMessage = await jobRunner.parseMessage(jobRunnerMessage); - - return { - message: returnMessage ?? `Starting job with id ${newJobObject.id}`, - job_id: newJobObject.id, - }; - } + const result = await jobs.addJob(json); + const newJobObject = result.createdJob; + if (!newJobObject) throw new Error(result.error || result.message || `Unable to create a ${json.operation} job`); + operationLog.info('addJob result', result); + const jobRunnerMessage = new jobRunner.RunnerMessage(newJobObject, json); + const returnMessage = await jobRunner.parseMessage(jobRunnerMessage); + + return { + message: returnMessage ?? `Starting job with id ${newJobObject.id}`, + job_id: newJobObject.id, + }; } catch (err) { // errors that already carry a statusCode (e.g. ClientError from job validation) are // client-facing as-is; wrapping them here would turn a 400/404/409 into a 500 diff --git a/unitTests/bin/fixtures/replicatedRestartHarness.cjs b/unitTests/bin/fixtures/replicatedRestartHarness.cjs new file mode 100644 index 0000000000..7418f0f8db --- /dev/null +++ b/unitTests/bin/fixtures/replicatedRestartHarness.cjs @@ -0,0 +1,30 @@ +'use strict'; + +// Runs a replicated restart_service the way its job worker does — off the main thread, waiting for the +// main thread's restart-complete — against peers whose answers come from workerData. +const { workerData, parentPort } = require('node:worker_threads'); +require('../../testUtils.js').preTestPrep(); +const { getThisNodeName } = require('#src/server/nodeName'); + +const unhandled = []; +process.on('unhandledRejection', (reason) => unhandled.push(String(reason?.message ?? reason))); +const { dispatchFailures = {}, pollFailures = {} } = workerData; +global.server = { + nodes: [{ name: getThisNodeName() }, { name: 'peer-a' }, { name: 'peer-b' }], + replication: { + monitorNodeCAs() {}, + async sendOperationToNode(node, operation) { + const failures = operation.operation === 'get_job' ? pollFailures : dispatchFailures; + if (failures[node.name]?.length) throw new Error(failures[node.name].shift()); + if (operation.operation === 'get_job') + return { results: [{ status: 'COMPLETE', message: `restarted ${node.name}` }] }; + return { job_id: `job-${node.name}` }; + }, + }, +}; +const { restartService } = require('#src/bin/restart'); +const settle = (outcome) => setImmediate(() => parentPort.postMessage({ ...outcome, unhandled })); +restartService({ operation: 'restart_service', service: 'http', replicated: true }).then( + (value) => settle({ completed: value }), + (error) => settle({ failed: { message: error.message, result: error.result } }) +); diff --git a/unitTests/bin/replicatedRestartService.test.js b/unitTests/bin/replicatedRestartService.test.js new file mode 100644 index 0000000000..d502c2e7a7 --- /dev/null +++ b/unitTests/bin/replicatedRestartService.test.js @@ -0,0 +1,53 @@ +'use strict'; + +const assert = require('node:assert'); +const { join } = require('node:path'); +const { Worker } = require('node:worker_threads'); + +function runReplicatedRestart(workerData) { + return new Promise((resolve, reject) => { + const worker = new Worker(join(__dirname, 'fixtures', 'replicatedRestartHarness.cjs'), { workerData }); + worker.on('message', (message) => { + if (message.type === 'restart') worker.postMessage({ type: 'restart-complete' }); + else { + resolve(message); + worker.terminate(); + } + }); + worker.on('error', reject); + }); +} + +const BACK_PRESSURE = 'ServerError: Outstanding write transactions have too long a queue, please try again later'; + +describe('a replicated restart_service job', function () { + this.timeout(30000); + + it('fails naming the peer it could not restart, keeping every peer outcome', async () => { + const outcome = await runReplicatedRestart({ dispatchFailures: { 'peer-b': ['Error: Connection closed 1006'] } }); + assert.strictEqual(outcome.completed, undefined, 'a restart that missed a peer must not complete'); + assert.match(outcome.failed.message, /peer-b \(Error: Connection closed 1006\)/); + assert.deepStrictEqual(outcome.failed.result.replicated, [ + { node: 'peer-a', message: 'restarted peer-a' }, + { node: 'peer-b', error: 'Error: Connection closed 1006' }, + ]); + }); + + it('completes once a peer that refused for back-pressure restarts on retry', async () => { + const outcome = await runReplicatedRestart({ dispatchFailures: { 'peer-a': [BACK_PRESSURE] } }); + assert.deepStrictEqual(outcome.completed, { + replicated: [ + { node: 'peer-b', message: 'restarted peer-b' }, + { node: 'peer-a', message: 'restarted peer-a' }, + ], + }); + }); + + it('keeps polling, with no unhandled rejection, when a restarting peer drops the connection', async () => { + const outcome = await runReplicatedRestart({ + pollFailures: { 'peer-a': ['Error: Connection closed 1006'] }, + }); + assert.deepStrictEqual(outcome.unhandled, []); + assert.strictEqual(outcome.completed.replicated.length, 2); + }); +}); diff --git a/unitTests/bin/restartEachPeer.test.js b/unitTests/bin/restartEachPeer.test.js new file mode 100644 index 0000000000..7b54537213 --- /dev/null +++ b/unitTests/bin/restartEachPeer.test.js @@ -0,0 +1,194 @@ +'use strict'; + +const assert = require('node:assert'); +const { restartEachPeer } = require('#src/bin/restart'); + +const TIMING = { + attempts: 3, + retryDelayMs: 5, + dispatchTimeoutMs: 1000, + pollIntervalMs: 1, + pollTimeoutMs: 1000, + jobDeadlineMs: 500, +}; +const BACK_PRESSURE = 'ServerError: Outstanding write transactions have too long a queue, please try again later'; + +describe('a rolling restart of each peer', function () { + this.timeout(10000); + let previousServer; + let sent; + // per node: a list of answers to its restart_service requests, then to its get_job polls + let dispatchAnswers; + let pollAnswers; + + const peers = ['peer-a', 'peer-b', 'peer-c'].map((name) => ({ name })); + const request = { operation: 'restart_service', service: 'http', replicated: false }; + + beforeEach(() => { + previousServer = global.server; + sent = []; + dispatchAnswers = {}; + pollAnswers = {}; + global.server = { + replication: { + async sendOperationToNode(node, operation, options) { + sent.push({ node: node.name, operation: operation.operation, options }); + const answers = operation.operation === 'get_job' ? pollAnswers : dispatchAnswers; + const answer = answers[node.name]?.length + ? answers[node.name].shift() + : operation.operation === 'get_job' + ? { results: [{ status: 'COMPLETE', message: `restarted ${node.name}` }] } + : { job_id: `job-${node.name}` }; + if (answer instanceof Error) throw answer; + return answer; + }, + }, + }; + }); + afterEach(() => { + global.server = previousServer; + }); + + const restarted = (name) => ({ node: name, message: `restarted ${name}` }); + + it('completes when every peer restarted, one at a time', async () => { + assert.deepStrictEqual(await restartEachPeer(peers, request, TIMING), { + replicated: peers.map(({ name }) => restarted(name)), + }); + assert.ok( + sent.every(({ options }) => options.timeoutMs > 0), + 'every request to a peer is bounded' + ); + }); + + it('fails naming a peer that could not be asked to restart, after restarting the others', async () => { + dispatchAnswers['peer-b'] = [new Error('Error: Connection closed 1006')]; + await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { + assert.match( + error.message, + /did not complete on 1 of 3 peer node\(s\): peer-b \(Error: Connection closed 1006\)/ + ); + assert.deepStrictEqual(error.result, { + replicated: [ + restarted('peer-a'), + { node: 'peer-b', error: 'Error: Connection closed 1006' }, + restarted('peer-c'), + ], + }); + return true; + }); + }); + + it('retries a peer that refused for write back-pressure after the other peers', async () => { + dispatchAnswers['peer-a'] = [new Error(BACK_PRESSURE)]; + assert.deepStrictEqual(await restartEachPeer(peers, request, TIMING), { + replicated: [restarted('peer-b'), restarted('peer-c'), restarted('peer-a')], + }); + }); + + it('also retries the 5.3 wording of the back-pressure refusal', async () => { + dispatchAnswers['peer-a'] = [ + new Error('HdbError: Outstanding write transactions have too long of queue, please try again later'), + ]; + assert.deepStrictEqual((await restartEachPeer(peers, request, TIMING)).replicated.at(-1), restarted('peer-a')); + }); + + it('gives up on a peer that keeps refusing for back-pressure', async () => { + dispatchAnswers['peer-a'] = [new Error(BACK_PRESSURE), new Error(BACK_PRESSURE), new Error(BACK_PRESSURE)]; + await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { + assert.match(error.message, /1 of 3 peer node\(s\): peer-a \(ServerError: Outstanding write/); + assert.deepStrictEqual(error.result.replicated.at(-1), { node: 'peer-a', error: BACK_PRESSURE }); + return true; + }); + assert.strictEqual(sent.filter((s) => s.node === 'peer-a' && s.operation === 'restart_service').length, 3); + }); + + it('does not retry any other refusal', async () => { + dispatchAnswers['peer-a'] = [new Error('Error: Unauthorized')]; + await assert.rejects(restartEachPeer(peers, request, TIMING), /peer-a \(Error: Unauthorized\)/); + assert.strictEqual(sent.filter((s) => s.node === 'peer-a').length, 1); + }); + + it('keeps polling through a dropped connection while the peer restarts', async () => { + const unhandled = []; + const onUnhandled = (reason) => unhandled.push(reason); + process.on('unhandledRejection', onUnhandled); + try { + pollAnswers['peer-a'] = [ + new Error('Error: Connection closed 1006'), + new Error('Client network socket disconnected before secure TLS connection was established'), + { results: [{ status: 'IN_PROGRESS' }] }, + ]; + assert.deepStrictEqual(await restartEachPeer(peers, request, TIMING), { + replicated: peers.map(({ name }) => restarted(name)), + }); + await new Promise((resolve) => setImmediate(resolve)); + } finally { + process.off('unhandledRejection', onUnhandled); + } + assert.deepStrictEqual(unhandled, []); + }); + + it('polls one request at a time', async () => { + let inFlight = 0; + const send = global.server.replication.sendOperationToNode; + global.server.replication.sendOperationToNode = async (...args) => { + assert.strictEqual(++inFlight, 1); + await new Promise((resolve) => setTimeout(resolve, 5)); + try { + return await send(...args); + } finally { + inFlight--; + } + }; + pollAnswers['peer-a'] = [{ results: [{ status: 'IN_PROGRESS' }] }, { results: [{ status: 'IN_PROGRESS' }] }]; + await restartEachPeer(peers, request, TIMING); + }); + + it('halts at a peer whose restart failed and names the peers it skipped', async () => { + pollAnswers['peer-a'] = [{ results: [{ status: 'ERROR', message: 'workers did not start' }] }]; + await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { + assert.match(error.message, /3 of 3 peer node\(s\): peer-a \(workers did not start\); peer-b \(skipped/); + assert.deepStrictEqual(error.result.replicated, [ + { node: 'peer-a', error: 'workers did not start' }, + { node: 'peer-b', error: 'skipped: the restart of peer-a did not complete' }, + { node: 'peer-c', error: 'skipped: the restart of peer-a did not complete' }, + ]); + return true; + }); + assert.deepStrictEqual( + sent.filter((s) => s.operation === 'restart_service').map((s) => s.node), + ['peer-a'] + ); + }); + + it('skips a peer waiting for its back-pressure retry once the roll halts', async () => { + dispatchAnswers['peer-a'] = [new Error(BACK_PRESSURE)]; + pollAnswers['peer-c'] = [{ results: [{ status: 'ERROR', message: 'workers did not start' }] }]; + await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { + assert.deepStrictEqual(error.result.replicated, [ + restarted('peer-b'), + { node: 'peer-c', error: 'workers did not start' }, + { node: 'peer-a', error: 'skipped: the restart of peer-c did not complete' }, + ]); + return true; + }); + }); + + it('halts at a peer whose restart job never finishes', async () => { + pollAnswers['peer-a'] = Array.from({ length: 100000 }, () => ({ results: [{ status: 'IN_PROGRESS' }] })); + await assert.rejects(restartEachPeer(peers, request, { ...TIMING, jobDeadlineMs: 50 }), (error) => { + assert.match(error.result.replicated[0].error, /Timed out waiting for restart job job-peer-a/); + assert.strictEqual(error.result.replicated.length, 3); + return true; + }); + }); + + it('halts at a peer that no longer has its restart job', async () => { + pollAnswers['peer-a'] = [{ results: [] }]; + await assert.rejects( + restartEachPeer(peers, request, TIMING), + /peer-a \(its restart job job-peer-a was not found\)/ + ); + }); +}); diff --git a/unitTests/server/jobs/executeJob.test.js b/unitTests/server/jobs/executeJob.test.js new file mode 100644 index 0000000000..5b5ba9473b --- /dev/null +++ b/unitTests/server/jobs/executeJob.test.js @@ -0,0 +1,34 @@ +'use strict'; + +const testUtils = require('../../testUtils.js'); +testUtils.preTestPrep(); + +const assert = require('node:assert'); +const sinon = require('sinon'); +const harperBridge = require('#src/dataLayer/harperBridge/harperBridge').default; +const { ServerError } = require('#src/utility/errors/hdbError'); +const { executeJob } = require('#src/server/serverHelpers/serverUtilities'); + +describe('executeJob', function () { + this.timeout(10000); + + before(async function () { + await testUtils.ensureSystemTables(); + }); + + afterEach(function () { + sinon.restore(); + }); + + it('surfaces the error that kept its job row from being written', async function () { + const backPressure = new ServerError( + 'Outstanding write transactions have too long a queue, please try again later', + 503 + ); + sinon.stub(harperBridge, 'createRecords').rejects(backPressure); + await assert.rejects(executeJob({ operation: 'restart_service', service: 'http' }), (error) => { + assert.strictEqual(error, backPressure); + return true; + }); + }); +}); diff --git a/unitTests/server/jobs/jobs.test.js b/unitTests/server/jobs/jobs.test.js index bd37223cfe..3be3021008 100644 --- a/unitTests/server/jobs/jobs.test.js +++ b/unitTests/server/jobs/jobs.test.js @@ -295,58 +295,35 @@ describe('Test jobs.js', () => { await testUtils.assertErrorAsync(addJob, [test_job], expected_err); }); - it('test error result is returned', async () => { + it('rejects with the error from searching for an id collision', async () => { const test_job = { operation: hdb_term.JOB_TYPE_ENUM.import_from_s3, hdb_user: 'test user', }; - const expected_result = { - message: '', - error: '', - success: false, - createdJob: undefined, - }; p_search_by_value_stub.throws(new Error('error with search by value')); - const result = await addJob(test_job); - - assert.deepEqual(result, expected_result); + await assert.rejects(addJob(test_job), /error with search by value/); }); - it('test error result is returned when error with duplicate id search', async () => { + it('rejects with the error from the duplicate id search', async () => { const test_job = { operation: hdb_term.JOB_TYPE_ENUM.import_from_s3, hdb_user: 'test user', }; - const expected_result = { - message: '', - error: '', - success: false, - createdJob: undefined, - }; p_search_by_value_stub.resetHistory(); p_search_by_value_stub.resolves(['123abc']); p_search_by_value_stub.onCall(1).throws(new Error('error with search by value')); - const result = await addJob(test_job); - - assert.deepEqual(result, expected_result); + await assert.rejects(addJob(test_job), /error with search by value/); }); - it('test error result is returned when error with insert', async () => { + it('rejects with the error from inserting the job row', async () => { const test_job = { operation: hdb_term.JOB_TYPE_ENUM.csv_file_load, hdb_user: 'test user', }; - const expected_result = { - message: '', - error: '', - success: false, - createdJob: undefined, - }; p_search_by_value_stub.resolves([]); - p_insert_stub.throws(new Error('error inserting')); - const result = await addJob(test_job); - - assert.deepEqual(result, expected_result); + const insert_error = new Error('error inserting'); + p_insert_stub.throws(insert_error); + await assert.rejects(addJob(test_job), (error) => error === insert_error); }); it('test error result is returned when inserted hashes length is zero', async () => { From 7560f66d20e8b047508245517032d953d82b45e0 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 23 Oct 2026 11:32:39 -0600 Subject: [PATCH 2/5] Halt a rolling restart at a peer that may be restarting A dispatch the replicator reports as having an unknown outcome (a timeout, or the connection closed before the answer) may have started that peer's restart, so moving on could restart two peers at once. Such a peer now halts the roll, as does an answer with no job id. A peer job in ERROR without a message, or with a status the poller does not know, also counts as failed, and a poll that times out names its last failure. The executeJob test fails a real job-row encode instead of a sinon stub. Dispatch-Task: harper-3468-rolling-restart-outcome Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- bin/restart.ts | 35 +++++++---- components/DESIGN.md | 22 ++++--- server/jobs/jobProcess.ts | 1 - .../bin/replicatedRestartService.test.js | 12 ++-- unitTests/bin/restartEachPeer.test.js | 63 +++++++++++++++++-- unitTests/server/jobs/executeJob.test.js | 22 +++---- 7 files changed, 113 insertions(+), 44 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index ba8e9eb56f..f20e3b21c9 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -187,7 +187,7 @@ Index of the design notes for the harper core: one line per note (closely relate - [Component preparation is serialized across worker threads](components/DESIGN.md#component-preparation-is-serialized-across-worker-threads) — One component transaction at a time across workers; the lifecycle broadcast sits outside the lock; the load lock is keyed by application and plugin. - [Peer-side deploy_component payload read: retryable blob stalls and `Readable.from()` cancellation](components/DESIGN.md#peer-side-deploy_component-payload-read-retryable-blob-stalls-and-readablefrom-cancellation) — Retry a `BLOB_UNAVAILABLE` read only before any byte reached the consumer; use `Readable.from()` for cancellation, not a hand-rolled `ReadableStream`. - [An origin waits for a peer's deploy answer only as long as the peer may take](components/DESIGN.md#an-origin-waits-for-a-peers-deploy-answer-only-as-long-as-the-peer-may-take) — The replicated deploy's per-peer deadline sums the peer's own allowances, so it never undercuts a healthy peer; a missed deadline is a failure with an unknown outcome, not cancellation. -- [A rolling restart job completes only if every peer restarted (`bin/restart.ts` `restartEachPeer`)](components/DESIGN.md#a-rolling-restart-job-completes-only-if-every-peer-restarted-binrestartts-restarteachpeer) — `ERROR` names each peer that failed or was skipped, with every peer's outcome in `result.replicated`; a failed restart halts the roll; only write back-pressure is retried. +- [A rolling restart job completes only if every peer restarted (`bin/restart.ts` `restartEachPeer`)](components/DESIGN.md#a-rolling-restart-job-completes-only-if-every-peer-restarted-binrestartts-restarteachpeer) — `ERROR` names each peer that failed or was skipped, with every peer's outcome in `result.replicated`; a failed or possibly-running peer restart halts the roll; only write back-pressure is retried. - [A replicated deploy reports each node's install fingerprint, and never acts on it](components/DESIGN.md#a-replicated-deploy-reports-each-nodes-install-fingerprint-and-never-acts-on-it) — Each node records the source identity its resolver already produced and the sha256 of its root lockfiles; the origin compares peers once on the aggregate and reports differences in a warning, the message and `peer_results`, without ever failing the deploy. - [A dangling symlink silently truncates the deploy tarball (`components/packageComponent.ts`)](components/DESIGN.md#a-dangling-symlink-silently-truncates-the-deploy-tarball-componentspackagecomponentts) — tar-fs treats a dangling symlink's ENOENT as end-of-stream, so `scanPackageDirectory()` pre-walks the tree · [The packer never ships the loader's links to the install (`components/packageComponent.ts`)](components/DESIGN.md#the-packer-never-ships-the-loaders-links-to-the-install-componentspackagecomponentts) — The application's `node_modules/harper` is skipped by name; a nested one only when it resolves to the running install. - [Deploy watcher generations preserve logical entry events](components/DESIGN.md#deploy-watcher-generations-preserve-logical-entry-events) — `EntryHandler` snapshots matching paths across a deploy so consumers see logical change and delete events, not raw rescan adds. diff --git a/bin/restart.ts b/bin/restart.ts index d52aaabe40..901f9aab74 100644 --- a/bin/restart.ts +++ b/bin/restart.ts @@ -217,12 +217,13 @@ const PEER_RESTART_TIMING = { // A peer that refused the operation for write back-pressure never started it. Errors cross replication as // text, and 5.3 peers word it "too long of queue". const WRITE_QUEUE_BACK_PRESSURE = /Outstanding write transactions have too long (?:a|of) queue/; +// How the replicator words a request that may have run there: it timed out, or its connection closed first. +const OUTCOME_UNKNOWN = /outcome there is unknown/; +const PEER_JOB_PENDING: string[] = [hdbTerms.JOB_STATUS_ENUM.CREATED, hdbTerms.JOB_STATUS_ENUM.IN_PROGRESS]; /** - * A rolling restart: restarts each peer in turn, waiting for its restart job before starting the next. The - * job completes only if every peer's restart completed; otherwise it fails naming each peer that did not - * restart, with every peer's outcome in `result.replicated`. A peer whose restart ran and failed or timed out - * halts the roll (the code being rolled out may be what failed), so the peers after it are skipped. + * Restarts each peer in turn, one at a time (components/DESIGN.md, "A rolling restart job completes only if + * every peer restarted"). A peer that may be restarting, or whose restart failed, halts the roll. */ async function restartEachPeer(peers: any[], request: any, timing = PEER_RESTART_TIMING) { const replication = (global as any).server.replication; @@ -238,15 +239,17 @@ async function restartEachPeer(peers: any[], request: any, timing = PEER_RESTART if (retryAt > Date.now()) await delay(retryAt - Date.now()); let jobId; try { - ({ job_id: jobId } = await replication.sendOperationToNode(node, request, { - timeoutMs: timing.dispatchTimeoutMs, - })); + const answer = await replication.sendOperationToNode(node, request, { timeoutMs: timing.dispatchTimeoutMs }); + jobId = answer?.job_id; } catch (error) { const message = error?.message ?? String(error); if (attempt < timing.attempts && WRITE_QUEUE_BACK_PRESSURE.test(message)) { hdbLogger.warn(`Restart of ${node.name} was refused (${message}); retrying after the other nodes`); queue.push({ node, attempt: attempt + 1, retryAt: Date.now() + timing.retryDelayMs * 2 ** (attempt - 1) }); - } else replicated.push({ node: node.name, error: message }); + } else { + replicated.push({ node: node.name, error: message }); + if (OUTCOME_UNKNOWN.test(message)) haltedBy = node.name; + } continue; } const outcome = jobId @@ -271,6 +274,7 @@ async function restartEachPeer(peers: any[], request: any, timing = PEER_RESTART async function awaitPeerJob(replication: any, node: any, jobId: string, timing: typeof PEER_RESTART_TIMING) { const deadline = Date.now() + timing.jobDeadlineMs; + let lastPollError; while (Date.now() < deadline) { await delay(timing.pollIntervalMs); let response; @@ -282,15 +286,24 @@ async function awaitPeerJob(replication: any, node: any, jobId: string, timing: ); } catch (error) { // The peer drops this connection while it restarts its own workers. - hdbLogger.debug?.(`Polling restart job ${jobId} on ${node.name} failed, still waiting:`, error?.message); + lastPollError = error?.message ?? String(error); + hdbLogger.debug?.(`Polling restart job ${jobId} on ${node.name} failed, still waiting:`, lastPollError); continue; } const job = response?.results?.[0]; if (!job) return { node: node.name, error: `its restart job ${jobId} was not found` }; if (job.status === hdbTerms.JOB_STATUS_ENUM.COMPLETE) return { node: node.name, message: job.message }; - if (job.status === hdbTerms.JOB_STATUS_ENUM.ERROR) return { node: node.name, error: job.message }; + if (job.status === hdbTerms.JOB_STATUS_ENUM.ERROR) + return { node: node.name, error: job.message ?? `its restart job ${jobId} failed` }; + if (!PEER_JOB_PENDING.includes(job.status)) + return { node: node.name, error: `its restart job ${jobId} reported status ${job.status}` }; } - return { node: node.name, error: `Timed out waiting for restart job ${jobId} to complete` }; + return { + node: node.name, + error: + `Timed out waiting for restart job ${jobId} to complete` + + (lastPollError ? `; the last poll failed: ${lastPollError}` : ''), + }; } /** diff --git a/components/DESIGN.md b/components/DESIGN.md index 2a4bb956f3..c684b9bcef 100644 --- a/components/DESIGN.md +++ b/components/DESIGN.md @@ -772,13 +772,21 @@ a later failure to remove branches is only logged, and a second drop retries it ## A rolling restart job completes only if every peer restarted (`bin/restart.ts` `restartEachPeer`) -The job behind `restart: 'rolling'` (a replicated `restart_service`) restarts one peer at a time and polls -each peer's own restart job. It ends `COMPLETE` only when every peer's job did; otherwise `ERROR`, naming -each peer that failed or was skipped, with one `result.replicated` entry per peer in both outcomes -(`{ node, message }` restarted, `{ node, error }` not). A peer that could not be asked is recorded and the -roll moves on; a peer whose restart ran and failed or timed out halts it, since the release being rolled -out may be what failed. Only the write-queue back-pressure refusal is retried (after the other peers, three -attempts): it is the one refusal known to precede execution, and errors cross replication only as text. +The job behind an uncertified `restart: 'rolling'` deploy (a replicated `restart_service`) restarts one +peer at a time and polls each peer's own restart job. It ends `COMPLETE` only when every peer's job did; +otherwise `ERROR`, naming each peer that failed or was skipped, with one `result.replicated` entry per peer +in both outcomes (`{ node, message }` restarted, `{ node, error }` not). A certified rolling deploy runs +`activateDeploymentOnPeers` instead, which reports through `http_resp_msg`. + +- A peer that refused before running anything is recorded and the roll moves on. +- A peer that may be restarting halts the roll, so two peers never restart at once: the replicator's + "outcome there is unknown" (a timeout, or the connection closed before the answer), or an answer with no + job id. So does a peer whose restart failed or timed out, since the release being rolled out may be what + failed. Every peer after it is recorded as skipped. +- Only the write-queue back-pressure refusal is retried (after the other peers, three attempts): it is the + one refusal known to precede execution. Errors cross replication only as text, so both classifications + match wording. + `COMPLETE` means each peer's restart job completed, not that every worker there took new code. Enforced by `unitTests/bin/restartEachPeer.test.js` and `replicatedRestartService.test.js` (harper#3468). diff --git a/server/jobs/jobProcess.ts b/server/jobs/jobProcess.ts index 6a31cb7d74..d6883b026d 100644 --- a/server/jobs/jobProcess.ts +++ b/server/jobs/jobProcess.ts @@ -83,7 +83,6 @@ const JOB_ID = JOB_NAME.substring(4); // get_job answers a refused bulk load with its structured permission report as the message. const report = err?.http_resp_msg; jobObj.message = report !== null && typeof report === 'object' ? report : err?.message ? err.message : err; - // A job that fails part way (a rolling restart that did not reach every node) keeps what it did finish. if (err?.result !== undefined) jobObj.result = err.result; jobObj.end_datetime = Date.now(); } finally { diff --git a/unitTests/bin/replicatedRestartService.test.js b/unitTests/bin/replicatedRestartService.test.js index d502c2e7a7..8f23269e22 100644 --- a/unitTests/bin/replicatedRestartService.test.js +++ b/unitTests/bin/replicatedRestartService.test.js @@ -18,18 +18,22 @@ function runReplicatedRestart(workerData) { }); } +const POLL_CLOSED = + 'The connection to wss://peer-a:9933 closed (1006) before get_job was answered; its outcome there is unknown'; const BACK_PRESSURE = 'ServerError: Outstanding write transactions have too long a queue, please try again later'; describe('a replicated restart_service job', function () { this.timeout(30000); it('fails naming the peer it could not restart, keeping every peer outcome', async () => { - const outcome = await runReplicatedRestart({ dispatchFailures: { 'peer-b': ['Error: Connection closed 1006'] } }); + const outcome = await runReplicatedRestart({ + dispatchFailures: { 'peer-b': ['Error: connect ECONNREFUSED 10.0.0.2:9933'] }, + }); assert.strictEqual(outcome.completed, undefined, 'a restart that missed a peer must not complete'); - assert.match(outcome.failed.message, /peer-b \(Error: Connection closed 1006\)/); + assert.match(outcome.failed.message, /peer-b \(Error: connect ECONNREFUSED 10.0.0.2:9933\)/); assert.deepStrictEqual(outcome.failed.result.replicated, [ { node: 'peer-a', message: 'restarted peer-a' }, - { node: 'peer-b', error: 'Error: Connection closed 1006' }, + { node: 'peer-b', error: 'Error: connect ECONNREFUSED 10.0.0.2:9933' }, ]); }); @@ -45,7 +49,7 @@ describe('a replicated restart_service job', function () { it('keeps polling, with no unhandled rejection, when a restarting peer drops the connection', async () => { const outcome = await runReplicatedRestart({ - pollFailures: { 'peer-a': ['Error: Connection closed 1006'] }, + pollFailures: { 'peer-a': [POLL_CLOSED] }, }); assert.deepStrictEqual(outcome.unhandled, []); assert.strictEqual(outcome.completed.replicated.length, 2); diff --git a/unitTests/bin/restartEachPeer.test.js b/unitTests/bin/restartEachPeer.test.js index 7b54537213..848c09c604 100644 --- a/unitTests/bin/restartEachPeer.test.js +++ b/unitTests/bin/restartEachPeer.test.js @@ -11,13 +11,14 @@ const TIMING = { pollTimeoutMs: 1000, jobDeadlineMs: 500, }; +const CLOSED_FIRST = (operation) => + `The connection to wss://peer:9933 closed (1006) before ${operation} was answered; its outcome there is unknown`; const BACK_PRESSURE = 'ServerError: Outstanding write transactions have too long a queue, please try again later'; describe('a rolling restart of each peer', function () { this.timeout(10000); let previousServer; let sent; - // per node: a list of answers to its restart_service requests, then to its get_job polls let dispatchAnswers; let pollAnswers; @@ -62,16 +63,16 @@ describe('a rolling restart of each peer', function () { }); it('fails naming a peer that could not be asked to restart, after restarting the others', async () => { - dispatchAnswers['peer-b'] = [new Error('Error: Connection closed 1006')]; + dispatchAnswers['peer-b'] = [new Error('Error: connect ECONNREFUSED 10.0.0.2:9933')]; await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { assert.match( error.message, - /did not complete on 1 of 3 peer node\(s\): peer-b \(Error: Connection closed 1006\)/ + /did not complete on 1 of 3 peer node\(s\): peer-b \(Error: connect ECONNREFUSED 10.0.0.2:9933\)/ ); assert.deepStrictEqual(error.result, { replicated: [ restarted('peer-a'), - { node: 'peer-b', error: 'Error: Connection closed 1006' }, + { node: 'peer-b', error: 'Error: connect ECONNREFUSED 10.0.0.2:9933' }, restarted('peer-c'), ], }); @@ -115,7 +116,7 @@ describe('a rolling restart of each peer', function () { process.on('unhandledRejection', onUnhandled); try { pollAnswers['peer-a'] = [ - new Error('Error: Connection closed 1006'), + new Error(CLOSED_FIRST('get_job')), new Error('Client network socket disconnected before secure TLS connection was established'), { results: [{ status: 'IN_PROGRESS' }] }, ]; @@ -191,4 +192,56 @@ describe('a rolling restart of each peer', function () { /peer-a \(its restart job job-peer-a was not found\)/ ); }); + + it('halts at a peer whose restart may have started, since its outcome is unknown', async () => { + dispatchAnswers['peer-a'] = [new Error(CLOSED_FIRST('restart_service'))]; + await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { + assert.deepStrictEqual( + error.result.replicated.map(({ node, error }) => [node, error.startsWith('skipped') ? 'skipped' : 'failed']), + [ + ['peer-a', 'failed'], + ['peer-b', 'skipped'], + ['peer-c', 'skipped'], + ] + ); + return true; + }); + assert.deepStrictEqual( + sent.map((s) => s.node), + ['peer-a'] + ); + }); + + it('halts at a peer that answered without a job id', async () => { + dispatchAnswers['peer-a'] = [undefined]; + await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { + assert.deepStrictEqual(error.result.replicated[0], { + node: 'peer-a', + error: 'it answered without a restart job id', + }); + assert.match(error.result.replicated[1].error, /^skipped/); + return true; + }); + }); + + it('halts at a peer whose restart job failed without a message', async () => { + pollAnswers['peer-a'] = [{ results: [{ status: 'ERROR' }] }]; + await assert.rejects(restartEachPeer(peers, request, TIMING), /peer-a \(its restart job job-peer-a failed\)/); + }); + + it('halts at a peer whose restart job reports a status it does not know', async () => { + pollAnswers['peer-a'] = [{ results: [{}] }]; + await assert.rejects( + restartEachPeer(peers, request, TIMING), + /peer-a \(its restart job job-peer-a reported status undefined\)/ + ); + }); + + it('reports the last poll failure when a peer never answers in time', async () => { + pollAnswers['peer-a'] = Array.from({ length: 100000 }, () => new Error(CLOSED_FIRST('get_job'))); + await assert.rejects(restartEachPeer(peers, request, { ...TIMING, jobDeadlineMs: 50 }), (error) => { + assert.match(error.result.replicated[0].error, /Timed out .*; the last poll failed: The connection to/); + return true; + }); + }); }); diff --git a/unitTests/server/jobs/executeJob.test.js b/unitTests/server/jobs/executeJob.test.js index 5b5ba9473b..19f9451753 100644 --- a/unitTests/server/jobs/executeJob.test.js +++ b/unitTests/server/jobs/executeJob.test.js @@ -4,9 +4,6 @@ const testUtils = require('../../testUtils.js'); testUtils.preTestPrep(); const assert = require('node:assert'); -const sinon = require('sinon'); -const harperBridge = require('#src/dataLayer/harperBridge/harperBridge').default; -const { ServerError } = require('#src/utility/errors/hdbError'); const { executeJob } = require('#src/server/serverHelpers/serverUtilities'); describe('executeJob', function () { @@ -16,19 +13,14 @@ describe('executeJob', function () { await testUtils.ensureSystemTables(); }); - afterEach(function () { - sinon.restore(); - }); - it('surfaces the error that kept its job row from being written', async function () { - const backPressure = new ServerError( - 'Outstanding write transactions have too long a queue, please try again later', - 503 + // The job row stores the request, and a symbol cannot be encoded into it. + await assert.rejects( + executeJob({ operation: 'restart_service', service: 'http', unencodable: Symbol() }), + (error) => { + assert.match(error.message, /Unknown type: symbol/); + return true; + } ); - sinon.stub(harperBridge, 'createRecords').rejects(backPressure); - await assert.rejects(executeJob({ operation: 'restart_service', service: 'http' }), (error) => { - assert.strictEqual(error, backPressure); - return true; - }); }); }); From 61194f355f35afe5302c643e260d405e9659dae1 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 23 Oct 2026 11:55:16 -0600 Subject: [PATCH 3/5] Halt a rolling restart on any dispatch failure that may have reached the peer Only a peer's own error answer or a connection that never opened (ECONNREFUSED and similar) lets the roll move on; any other socket error halts it. A poll that succeeds clears the last poll failure, so a timeout on a reachable peer does not blame the network. Dispatch-Task: harper-3468-rolling-restart-outcome Co-Authored-By: Claude Opus 5.5 --- bin/restart.ts | 14 +++++++++++-- components/DESIGN.md | 15 +++++++------- unitTests/bin/restartEachPeer.test.js | 29 +++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/bin/restart.ts b/bin/restart.ts index 901f9aab74..aceec5349c 100644 --- a/bin/restart.ts +++ b/bin/restart.ts @@ -217,7 +217,9 @@ const PEER_RESTART_TIMING = { // A peer that refused the operation for write back-pressure never started it. Errors cross replication as // text, and 5.3 peers word it "too long of queue". const WRITE_QUEUE_BACK_PRESSURE = /Outstanding write transactions have too long (?:a|of) queue/; -// How the replicator words a request that may have run there: it timed out, or its connection closed first. +// Rejections known to precede the request reaching the peer. Anything else with a socket error code, or the +// replicator's "outcome there is unknown" (a timeout, or the connection closing first), may have started it. +const NEVER_CONNECTED = new Set(['ECONNREFUSED', 'ENOTFOUND', 'EAI_AGAIN', 'EHOSTUNREACH', 'ENETUNREACH']); const OUTCOME_UNKNOWN = /outcome there is unknown/; const PEER_JOB_PENDING: string[] = [hdbTerms.JOB_STATUS_ENUM.CREATED, hdbTerms.JOB_STATUS_ENUM.IN_PROGRESS]; @@ -248,7 +250,7 @@ async function restartEachPeer(peers: any[], request: any, timing = PEER_RESTART queue.push({ node, attempt: attempt + 1, retryAt: Date.now() + timing.retryDelayMs * 2 ** (attempt - 1) }); } else { replicated.push({ node: node.name, error: message }); - if (OUTCOME_UNKNOWN.test(message)) haltedBy = node.name; + if (mayHaveStarted(error, message)) haltedBy = node.name; } continue; } @@ -272,6 +274,13 @@ async function restartEachPeer(peers: any[], request: any, timing = PEER_RESTART return { replicated }; } +/** A peer's own error answer carries no code; a socket error does. */ +function mayHaveStarted(error: any, message: string) { + if (OUTCOME_UNKNOWN.test(message)) return true; + const code = error?.code; + return code !== undefined && !NEVER_CONNECTED.has(code); +} + async function awaitPeerJob(replication: any, node: any, jobId: string, timing: typeof PEER_RESTART_TIMING) { const deadline = Date.now() + timing.jobDeadlineMs; let lastPollError; @@ -290,6 +299,7 @@ async function awaitPeerJob(replication: any, node: any, jobId: string, timing: hdbLogger.debug?.(`Polling restart job ${jobId} on ${node.name} failed, still waiting:`, lastPollError); continue; } + lastPollError = undefined; const job = response?.results?.[0]; if (!job) return { node: node.name, error: `its restart job ${jobId} was not found` }; if (job.status === hdbTerms.JOB_STATUS_ENUM.COMPLETE) return { node: node.name, message: job.message }; diff --git a/components/DESIGN.md b/components/DESIGN.md index c684b9bcef..d97c98e0bb 100644 --- a/components/DESIGN.md +++ b/components/DESIGN.md @@ -778,14 +778,15 @@ otherwise `ERROR`, naming each peer that failed or was skipped, with one `result in both outcomes (`{ node, message }` restarted, `{ node, error }` not). A certified rolling deploy runs `activateDeploymentOnPeers` instead, which reports through `http_resp_msg`. -- A peer that refused before running anything is recorded and the roll moves on. -- A peer that may be restarting halts the roll, so two peers never restart at once: the replicator's - "outcome there is unknown" (a timeout, or the connection closed before the answer), or an answer with no - job id. So does a peer whose restart failed or timed out, since the release being rolled out may be what - failed. Every peer after it is recorded as skipped. +- A peer that answered with its own error, or whose connection never opened (`ECONNREFUSED` and similar), + never started a restart: it is recorded and the roll moves on. +- Any other dispatch failure halts the roll, so two peers never restart at once: the replicator's "outcome + there is unknown" (a timeout, or the connection closed before the answer), any other socket error, + or an answer with no job id. So does a peer whose restart failed or timed out, since the release being + rolled out may be what failed. Every peer after it is recorded as skipped. - Only the write-queue back-pressure refusal is retried (after the other peers, three attempts): it is the - one refusal known to precede execution. Errors cross replication only as text, so both classifications - match wording. + one refusal known to be transient. Errors cross replication as text plus, for a socket error, its `code`, so + these classifications match wording and codes. `COMPLETE` means each peer's restart job completed, not that every worker there took new code. Enforced by `unitTests/bin/restartEachPeer.test.js` and `replicatedRestartService.test.js` (harper#3468). diff --git a/unitTests/bin/restartEachPeer.test.js b/unitTests/bin/restartEachPeer.test.js index 848c09c604..bfe9195bfa 100644 --- a/unitTests/bin/restartEachPeer.test.js +++ b/unitTests/bin/restartEachPeer.test.js @@ -244,4 +244,33 @@ describe('a rolling restart of each peer', function () { return true; }); }); + + it('halts at a peer whose connection failed after the request may have been sent', async () => { + dispatchAnswers['peer-a'] = [Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' })]; + await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { + assert.match(error.result.replicated[1].error, /^skipped/); + return true; + }); + }); + + it('moves on from a peer it could not connect to', async () => { + dispatchAnswers['peer-a'] = [ + Object.assign(new Error('connect ECONNREFUSED 10.0.0.2:9933'), { code: 'ECONNREFUSED' }), + ]; + await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { + assert.deepStrictEqual(error.result.replicated.slice(1), [restarted('peer-b'), restarted('peer-c')]); + return true; + }); + }); + + it('does not blame the network for a timeout once the peer answered again', async () => { + pollAnswers['peer-a'] = [ + new Error(CLOSED_FIRST('get_job')), + ...Array.from({ length: 100000 }, () => ({ results: [{ status: 'IN_PROGRESS' }] })), + ]; + await assert.rejects(restartEachPeer(peers, request, { ...TIMING, jobDeadlineMs: 50 }), (error) => { + assert.strictEqual(error.result.replicated[0].error, 'Timed out waiting for restart job job-peer-a to complete'); + return true; + }); + }); }); From 4a0db21ea4f77229292a8526cdcdbc863aa25cc5 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 23 Oct 2026 20:46:29 -0600 Subject: [PATCH 4/5] Halt a rolling restart when the peer session closes with the restart pending The session rejects requests pending at close with a code-less 'Connection closed' error, which read as a peer's own answer, so the roll moved on while the peer could be restarting. Co-Authored-By: Claude Sonnet 5.5 Dispatch-Task: pr-maint-0346cd78c3159a8a3a9f365e1b7c7aec --- bin/restart.ts | 5 +++-- unitTests/bin/restartEachPeer.test.js | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/bin/restart.ts b/bin/restart.ts index aceec5349c..be37a624dd 100644 --- a/bin/restart.ts +++ b/bin/restart.ts @@ -218,9 +218,10 @@ const PEER_RESTART_TIMING = { // text, and 5.3 peers word it "too long of queue". const WRITE_QUEUE_BACK_PRESSURE = /Outstanding write transactions have too long (?:a|of) queue/; // Rejections known to precede the request reaching the peer. Anything else with a socket error code, or the -// replicator's "outcome there is unknown" (a timeout, or the connection closing first), may have started it. +// replicator's "outcome there is unknown" (a timeout, or the connection closing first) or the session's code-less +// "Connection closed" rejection of requests pending at close, may have started it. const NEVER_CONNECTED = new Set(['ECONNREFUSED', 'ENOTFOUND', 'EAI_AGAIN', 'EHOSTUNREACH', 'ENETUNREACH']); -const OUTCOME_UNKNOWN = /outcome there is unknown/; +const OUTCOME_UNKNOWN = /outcome there is unknown|^Connection closed\b/; const PEER_JOB_PENDING: string[] = [hdbTerms.JOB_STATUS_ENUM.CREATED, hdbTerms.JOB_STATUS_ENUM.IN_PROGRESS]; /** diff --git a/unitTests/bin/restartEachPeer.test.js b/unitTests/bin/restartEachPeer.test.js index bfe9195bfa..0cba89dd1c 100644 --- a/unitTests/bin/restartEachPeer.test.js +++ b/unitTests/bin/restartEachPeer.test.js @@ -193,6 +193,21 @@ describe('a rolling restart of each peer', function () { ); }); + it('halts at a peer whose session closed with its request pending', async () => { + dispatchAnswers['peer-a'] = [new Error('Connection closed 1006')]; + await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { + assert.deepStrictEqual( + error.result.replicated.map(({ node }) => node), + ['peer-a', 'peer-b', 'peer-c'] + ); + return true; + }); + assert.deepStrictEqual( + sent.map((s) => s.node), + ['peer-a'] + ); + }); + it('halts at a peer whose restart may have started, since its outcome is unknown', async () => { dispatchAnswers['peer-a'] = [new Error(CLOSED_FIRST('restart_service'))]; await assert.rejects(restartEachPeer(peers, request, TIMING), (error) => { From b3239222234f420bf86655e510ac3f7797c1cb20 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 23 Oct 2026 20:49:47 -0600 Subject: [PATCH 5/5] Trim restart comments Co-Authored-By: Claude Sonnet 5.5 Dispatch-Task: pr-maint-0346cd78c3159a8a3a9f365e1b7c7aec --- bin/restart.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/bin/restart.ts b/bin/restart.ts index be37a624dd..33361e3b4f 100644 --- a/bin/restart.ts +++ b/bin/restart.ts @@ -224,10 +224,7 @@ const NEVER_CONNECTED = new Set(['ECONNREFUSED', 'ENOTFOUND', 'EAI_AGAIN', 'EHOS const OUTCOME_UNKNOWN = /outcome there is unknown|^Connection closed\b/; const PEER_JOB_PENDING: string[] = [hdbTerms.JOB_STATUS_ENUM.CREATED, hdbTerms.JOB_STATUS_ENUM.IN_PROGRESS]; -/** - * Restarts each peer in turn, one at a time (components/DESIGN.md, "A rolling restart job completes only if - * every peer restarted"). A peer that may be restarting, or whose restart failed, halts the roll. - */ +// See components/DESIGN.md, "A rolling restart job completes only if every peer restarted". async function restartEachPeer(peers: any[], request: any, timing = PEER_RESTART_TIMING) { const replication = (global as any).server.replication; const replicated = []; @@ -275,7 +272,7 @@ async function restartEachPeer(peers: any[], request: any, timing = PEER_RESTART return { replicated }; } -/** A peer's own error answer carries no code; a socket error does. */ +// A peer's own error answer carries no code; a socket error usually does, a session close does not. function mayHaveStarted(error: any, message: string) { if (OUTCOME_UNKNOWN.test(message)) return true; const code = error?.code;