From 6ba4b494c7bfdde368c89cb0f8348031a263bd25 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 2 Sep 2026 14:32:01 -0400 Subject: [PATCH 01/29] feat(deploy): certify a candidate in an isolated validator before it can go live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 2 of #2315. Step 1 made `deploy_component` build aside and validate before the swap, but `validateComponentLoadsExclusive` gated its whole body on `!isMainThread` and the operations API deploys on main — so an operator deploy was certified by nothing and step 1 reordered a no-op there. **Certification is now a requirement of the mint, not a courtesy of the caller.** `validateCandidate` was an optional callback and only one of `prepareApplication`'s four production call sites supplied it, yet `activateCandidateApplication` writes `.complete`, which recovery treats as proof that a validation happened. So `markCandidateComplete` refuses to write it unless a validator has certified that exact candidate. The record is module-internal: a proof passed as an argument is one an external caller can forge or a future caller can forget. **The validator is an ephemeral worker, deliberately not a `startWorker` one.** That function builds a MessageChannel per connected port, announces the new port to every peer, and registers for monitoring and restart — so a validator would join the ITC mesh, letting a candidate's top-level `server.registerOperation` announce itself and traffic route at a thread about to exit, at O(deploys × workers) channels. What IS shared is the interpreter setup, now factored out as `buildWorkerExecArgv`: without it the thread cannot load Harper's own module graph at all. Three findings only reachable by building it: - The verdict needs its own `MessageChannel`. `parentPort` carries Harper's ITC traffic, so the first unrelated message was being rejected as a malformed verdict. - The validator must set `workerData.noServerStart`, which `server/DESIGN.md` already documents — without it `threadServer` boots at module scope and loads every root component, so the validator would serve traffic and certify the wrong thing. - The entry has to be the compiled sibling, referenced the way `jobRunner` references `jobProcess.js`. **Two cases deliberately earn no authority rather than being refused**, on the principle *no verdict means no authority, never no verdict means no deploy*: - **Safe mode** stages without activating. It may not execute configured code, so it can certify nothing — and safe mode is transient, so the next ordinary preparation certifies and activates. - **A branch-configured component** deploys uncertified. A branch's location is derived only from the application and database names, so a certification load would open the store the live version is serving from: a candidate could mutate rows, throw, be rejected, and leave the live version serving the mutation. Certifying against the base store instead is no better. Not deferrable like safe mode, because certification cannot succeed for these until validation-scoped branch storage exists. The in-process path is gone — 87 lines of pollution avoidance (the registration guard's in-worker rationale, the status sink, scope and module collection) that existed only because validation shared a process with serving code. `.complete` minting and activation are now separate concerns, so the swap tests exercise activation without minting. Boot and certification build their load options from one helper (`rootApplicationLoadOptions`), so identity and mount cannot drift between them — hand-plumbing a subset is how they would. Guarantee stated narrowly on purpose: within the lifetime of a preparation. A package deploy's root-config entry is still written before the build and never rolled back, so a rejected v2 can be re-prepared and activated after a restart. That needs config staged with activation, which is step 3. --- components/Application.ts | 173 ++++++++++++++++-- components/certifyCandidate.ts | 157 ++++++++++++++++ components/componentLoader.ts | 39 +++- components/deployValidator.ts | 71 +++++++ components/operations.js | 105 ++--------- server/threads/manageThreads.js | 65 ++++--- unitTests/components/deployActivation.test.js | 8 +- .../components/deployCandidateBuild.test.js | 10 +- 8 files changed, 475 insertions(+), 153 deletions(-) create mode 100644 components/certifyCandidate.ts create mode 100644 components/deployValidator.ts diff --git a/components/Application.ts b/components/Application.ts index 87cc2bb957..2db067d045 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -1020,6 +1020,34 @@ export async function extractApplication( // non-recursive `rm` of a directory. `assertApplicationConfig` rejects any name `isJoinableComponentName` // rejects, so the collision is unreachable from a root-config key as well as from a deploy. const CANDIDATE_COMPLETE_MARKER = '.complete'; + +/** + * Candidates a validator has certified, by `\0`. + * + * `.complete` is what recovery treats as authority for a "build AND validation complete" candidate — it + * will roll such a candidate forward after a crash. So the function that writes it has to require the + * verdict, rather than trusting its caller to have asked for one: `validateCandidate` was an optional + * callback and only one of `prepareApplication`'s four production call sites supplied it, which is the + * one-rule-N-sites shape that produced most of the previous step's defects. + * + * Module-internal on purpose. A proof passed in as an argument is a proof an external caller can forge or + * a future caller can forget; nothing crosses this boundary, so there is nothing to get wrong. + */ +const certifiedCandidates = new Set(); + +function certificationKey(componentDirPath: string, deploymentId: string): string { + return `${componentDirPath}\0${deploymentId}`; +} + +/** Record that a validator returned a passing verdict for exactly this candidate. */ +function recordCandidateCertified(componentDirPath: string, deploymentId: string): void { + certifiedCandidates.add(certificationKey(componentDirPath, deploymentId)); +} + +/** Forget a candidate's certification once its deployment directory is gone, so the set cannot grow. */ +function forgetCandidateCertification(componentDirPath: string, deploymentId: string): void { + certifiedCandidates.delete(certificationKey(componentDirPath, deploymentId)); +} // Records activation intent beside the candidate, so recovery finishes or undoes the whole transaction — // tree and configuration together — instead of inferring intent from filesystem shape alone. const ACTIVATION_JOURNAL = '.activation.json'; @@ -2021,18 +2049,18 @@ async function syncTreeContents(rootPath: string, foreignTree = false): Promise< await syncDirectory(rootPath); } -export async function markCandidateComplete( - componentDirPath: string, - deploymentId: string, - componentName: string -): Promise { - // Contents first: `.complete` is roll-forward AUTHORITY, so it must not be durable before the tree it - // vouches for. - // - // A `file:` candidate IS a symlink to a tree this deploy does not own, but the dependency - // install writes THROUGH it — so the tree still has to be walked, or the install output `.complete` - // vouches for is never made durable. Only the foreign files alongside it are tolerated: see - // `syncTreeContents`. +/** + * Make a candidate's contents durable, before anything vouches for them. + * + * Separate from `markCandidateComplete` so it can be exercised without also being a way to write + * `.complete`: that marker is gated on certification, and a test seam that bypassed the gate would be a + * seam a caller could bypass it through too. + * + * A `file:` candidate IS a symlink to a tree this deploy does not own, but the dependency + * install writes THROUGH it — so the tree still has to be walked, or the install output is never made + * durable. Only the foreign files alongside it are tolerated: see `syncTreeContents`. + */ +export async function syncCandidateTree(componentDirPath: string, deploymentId: string): Promise { const candidatePath = candidateApplicationPath(componentDirPath, deploymentId); const candidateIsLink = await lstat(candidatePath).then( (stats) => stats.isSymbolicLink(), @@ -2042,6 +2070,23 @@ export async function markCandidateComplete( } ); await syncTreeContents(candidatePath, candidateIsLink); +} + +export async function markCandidateComplete( + componentDirPath: string, + deploymentId: string, + componentName: string +): Promise { + // The gate. Everything below writes the marker recovery trusts, so an uncertified candidate must not + // get here — and the check lives at the mint rather than at the caller for the reason `certifiedCandidates` + // documents. + if (!certifiedCandidates.has(certificationKey(componentDirPath, deploymentId))) { + throw new Error( + `Refusing to mark the ${componentName} candidate ${deploymentId} complete: no validator has ` + + `certified it, and \`${CANDIDATE_COMPLETE_MARKER}\` is what recovery treats as proof that one did` + ); + } + await syncCandidateTree(componentDirPath, deploymentId); try { await writeControlFileDurably(candidateComponentFilePath(componentDirPath, deploymentId), componentName); } catch (error) { @@ -2064,7 +2109,61 @@ export async function markCandidateComplete( * rename is the COMMIT POINT: nothing after it may compensate, because the live path holds the candidate and * renaming the aside back over it cannot succeed. */ -export async function activateCandidateApplication(application: Application, deploymentId: string): Promise { +/** + * Decide and obtain a candidate's certification, immediately before activation. + * + * Three outcomes, and the difference between the last two is the whole reason this is one function: + * + * - **Certified** — a validator loaded this exact tree under the real application identity and mount. + * Activation mints `.complete`. + * - **Stage only** — safe mode. It may not execute configured code, so it can certify nothing, and nothing + * uncertified gets published. Deferrable, because safe mode is transient. + * - **Uncertified, activate anyway** — a branch-configured component. A branch's location is derived only + * from the application and database names, so a certification load would open the store the LIVE version + * is serving from; a candidate could mutate rows, throw, be rejected, and leave the live version serving + * the mutation. Not deferrable — certification cannot succeed for these until validation-scoped branch + * storage exists — so "pending" would be permanent limbo reported as success. It deploys exactly as it + * does today and simply earns no authority. + * + * The invariant across all three is *no verdict means no authority*, never *no verdict means no deploy*. + */ +async function certifyPreparedCandidate( + application: Application, + deploymentId: string, + candidateDirPath: string, + options: PrepareApplicationOptions +): Promise<{ certified: boolean; stageOnly?: boolean }> { + const safeMode = + process.env.HARPER_SAFE_MODE && process.env.HARPER_SAFE_MODE !== 'false' && process.env.HARPER_SAFE_MODE !== '0'; + if (safeMode) return { certified: false, stageOnly: true }; + + const { rootApplicationLoadOptions } = await import('./componentLoader.ts'); + const loadOptions = rootApplicationLoadOptions(application.name, { forCertification: true }); + if (loadOptions.ok && loadOptions.branchConfigured) { + application.logger.warn( + `Deploying ${application.name} without certification: a certification load would open the same ` + + `database branch the live version is serving from, so it is skipped until validation-scoped ` + + `branch storage exists` + ); + return { certified: false }; + } + + options.emitPhase?.('load', 'start'); + const { certifyCandidate } = await import('./certifyCandidate.ts'); + const outcome = await certifyCandidate(candidateDirPath, application.name, { + timeoutMs: options.certificationTimeoutMs, + }); + if (!outcome.certified) throw outcome.error ?? new Error(`${application.name} could not be certified`); + recordCandidateCertified(application.dirPath, deploymentId); + options.emitPhase?.('load', 'done'); + return { certified: true }; +} + +export async function activateCandidateApplication( + application: Application, + deploymentId: string, + { certified = true }: { certified?: boolean } = {} +): Promise { const liveDirPath = application.dirPath; const candidateDirPath = candidateApplicationPath(liveDirPath, deploymentId); const deploymentDirPath = candidateDeploymentDirPath(liveDirPath, deploymentId); @@ -2080,7 +2179,12 @@ export async function activateCandidateApplication(application: Application, dep throw new Error(`Cannot activate ${application.name}: no candidate build at ${candidateDirPath}`); } - await markCandidateComplete(liveDirPath, deploymentId, application.name); + // `certified: false` is the DELIBERATE uncertified path — a branch-configured component, which cannot be + // certified safely yet. It activates without ever minting `.complete`, so a crash mid-swap rolls back to + // the committed tree instead of forward onto something no validator vouched for. The gate inside + // `markCandidateComplete` still guards the accidental case: a caller that reaches the mint without a + // verdict is a bug, not a policy. + if (certified) await markCandidateComplete(liveDirPath, deploymentId, application.name); const journalPath = activationJournalPath(liveDirPath, deploymentId); try { await writeControlFileDurably( @@ -2188,6 +2292,9 @@ export async function activateCandidateApplication(application: Application, dep await rm(deploymentDirPath, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }).catch((error) => application.logger.warn(`Deployed ${application.name} but could not clean up its staging directory:`, error) ); + // Same reason as `discardCandidate`: the tree is gone, so its verdict goes too. Both sites, because + // one site remembering and its sibling forgetting is the defect shape this whole area keeps producing. + forgetCandidateCertification(liveDirPath, deploymentId); await rmdir(dirname(deploymentDirPath)).catch(() => {}); } } @@ -2328,6 +2435,10 @@ function stripExtendedLengthPrefix(target: string): string { /** Remove a candidate's whole deployment directory, best-effort — it is never the last good copy. */ async function discardCandidate(application: Application, deploymentId: string): Promise { const deploymentDirPath = candidateDeploymentDirPath(application.dirPath, deploymentId); + // The tree this certification vouched for is going, so the certification goes with it — otherwise a + // later deploy reusing the id would inherit a verdict that was never about its tree, and the set would + // grow for the life of the process. + forgetCandidateCertification(application.dirPath, deploymentId); await rm(deploymentDirPath, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }).catch((error) => application.logger.warn(`Failed to remove the abandoned deploy candidate at ${deploymentDirPath}:`, error) ); @@ -3357,7 +3468,14 @@ export type PrepareApplicationOptions = { * behavior, where the swap committed first and a load failure was reported over an already-live * broken release. */ - validateCandidate?: (candidateDirPath: string) => Promise; + /** + * Emit the operation's progress phases. Certification moved in here from `deploy_component`, which was + * the only place emitting the `load` phases — SSE clients key progress off them, so the stream has to + * keep coming from wherever certification now runs. + */ + emitPhase?: (phase: string, status: 'start' | 'done') => void; + /** Certification deadline. Callers with no request budget of their own (boot, `addComponent`) rely on the default. */ + certificationTimeoutMs?: number; }; export async function prepareApplication(application: Application, options: PrepareApplicationOptions = {}) { @@ -3407,9 +3525,26 @@ export async function prepareApplication(application: Application, options: Prep await application.cleanupGitCredentialSession(); } try { - // Validated while the previous version is still the one serving, so a candidate that + // Certified while the previous version is still the one serving, so a candidate that // installs cleanly but throws at load is rejected without ever having been live. - await options.validateCandidate?.(candidateDirPath); + // + // Here rather than in `deploy_component`, because this is the function that goes on to mint + // `.complete` — the marker recovery treats as proof a validation happened. Leaving it to the + // caller meant three of this function's four production call sites minted that authority + // without ever asking for a verdict. + const certification = await certifyPreparedCandidate(application, deploymentId, candidateDirPath, options); + if (certification.stageOnly) { + // Safe mode: it may not execute configured code, so it can certify nothing — and a + // candidate nothing certified must not be published. Staged and left for the next + // ordinary-mode preparation to certify and activate. Safe mode is transient, so + // "pending" here resolves on its own, which is why this differs from the + // branch-configured case below. + application.logger.warn( + `Staged ${application.name} without activating it: safe mode cannot certify a candidate, ` + + `and an uncertified candidate is not published` + ); + return; + } if (!application.isNewComponent) { application.packageMetadataChanged = installedRuntimeChanged( previousPackageMetadata, @@ -3417,7 +3552,9 @@ export async function prepareApplication(application: Application, options: Prep application.installationIsOpaque ); } - await activateCandidateApplication(application, deploymentId); + await activateCandidateApplication(application, deploymentId, { + certified: certification.certified, + }); } catch (error) { // The builder's own cleanup only covers a failed BUILD. A rejected validation, or an // activation that was cleanly compensated, would otherwise leave a whole installed diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts new file mode 100644 index 0000000000..34827836e0 --- /dev/null +++ b/components/certifyCandidate.ts @@ -0,0 +1,157 @@ +'use strict'; + +import { join } from 'node:path'; +import { MessageChannel, Worker } from 'node:worker_threads'; + +import harperLogger from '../utility/logging/harper_logger.ts'; +import { buildWorkerExecArgv } from '../server/threads/manageThreads.js'; + +/** How long a certification load may take before the candidate is rejected, when nothing else says. */ +const DEFAULT_CERTIFICATION_TIMEOUT_MS = 120_000; + +/** + * The maximum number of certification workers alive at once. + * + * Deploys are already serialized per component by the preparation lock, but not across components, and a + * certification thread is a whole module graph. Bounded so a burst of deploys cannot spawn one thread per + * component simultaneously. + */ +const MAX_CONCURRENT_CERTIFICATIONS = 2; + +let active = 0; +const waiting: (() => void)[] = []; + +async function acquireSlot(): Promise<() => void> { + if (active >= MAX_CONCURRENT_CERTIFICATIONS) await new Promise((resolve) => waiting.push(resolve)); + active++; + let released = false; + return () => { + if (released) return; + released = true; + active--; + waiting.shift()?.(); + }; +} + +export interface CertificationOutcome { + certified: boolean; + /** Why not, when `certified` is false. Always present in that case. */ + error?: Error; +} + +/** + * Load a candidate in an ephemeral worker and report whether it loaded. + * + * NOT `startWorker()`. That function constructs a `MessageChannel` for every connected port, announces the + * new port to every peer, and registers the worker for monitoring and restart — so a certification worker + * would join the ITC mesh, meaning a candidate's top-level `server.registerOperation()` could announce + * itself to main and traffic could route at a thread that is about to exit. It would also cost + * O(deploys × workers) channels on a large node. Only the *option construction* is worth sharing, and that + * is deliberately kept small here rather than reaching into the serving-worker path. + * + * Every outcome other than an explicit passing verdict is a failure, because `.complete` — which this + * gates — is what crash recovery treats as proof that a validation happened. + */ +export async function certifyCandidate( + candidateDirPath: string, + appName: string, + { timeoutMs = DEFAULT_CERTIFICATION_TIMEOUT_MS }: { timeoutMs?: number } = {} +): Promise { + const releaseSlot = await acquireSlot(); + // The COMPILED sibling, referenced the way `jobRunner` references `jobProcess.js`: workers load from the + // build output, and `__dirname` resolves there without assuming where the package root is. + const entry = join(__dirname, './deployValidator.js'); + let worker: Worker | undefined; + let settled = false; + let timer: NodeJS.Timeout | undefined; + // A channel of its own, NOT `parentPort`: Harper's worker machinery uses that for its own ITC traffic, + // so a verdict read from it would compete with unrelated messages — the first one to arrive was being + // rejected as a malformed verdict. On a dedicated channel, anything that does not conform really is one. + const { port1: verdicts, port2: verdictPort } = new MessageChannel(); + + try { + return await new Promise((resolve) => { + // Exactly one settlement, whichever of the outcomes below happens first. A candidate with a + // syntax error emits `error` AND then `exit`; a candidate that posts a verdict and then throws + // during teardown does the reverse. Either way the first answer stands. + const settle = (outcome: CertificationOutcome) => { + if (settled) return; + settled = true; + if (timer) clearTimeout(timer); + resolve(outcome); + }; + const fail = (message: string) => settle({ certified: false, error: new Error(message) }); + + try { + worker = new Worker(entry, { + workerData: { + candidateDirPath, + appName, + verdictPort, + // `server/DESIGN.md`: "Workers receive `workerData.noServerStart = true` — never start the + // server inside a worker." Without it `threadServer` boots at module scope and loads every + // root component, so the validator would serve traffic and certify the wrong thing. + noServerStart: true, + }, + transferList: [verdictPort], + // The same interpreter setup every Harper worker gets. Without it this thread cannot load + // Harper's own module graph at all — a module that imports JSON fails outright — so this is + // shared with `startWorker` rather than reconstructed. + execArgv: buildWorkerExecArgv(), + argv: process.argv.slice(2), + }); + } catch (error) { + // A synchronous spawn throw is a deploy failure, not a candidate failure — and specifically + // not a success. Under thread pressure a node will refuse deploys rather than publish + // uncertified trees. + const spawnError = error instanceof Error ? error : new Error(String(error)); + spawnError.message = `Could not start a validator to certify ${appName}: ${spawnError.message}`; + (spawnError as any).statusCode = 503; + settle({ certified: false, error: spawnError }); + return; + } + + timer = setTimeout(() => { + fail(`Certification of ${appName} did not finish within ${timeoutMs}ms; the candidate was not ` + `published`); + }, timeoutMs); + // A hung candidate must not keep the process alive once the parent has stopped caring. + timer.unref?.(); + + verdicts.on('message', (message: unknown) => { + if (!message || typeof message !== 'object' || typeof (message as any).ok !== 'boolean') { + fail(`Certification of ${appName} returned a malformed verdict`); + return; + } + if ((message as any).ok) { + settle({ certified: true }); + return; + } + const error = new Error((message as any).message || `${appName} failed to load`); + if (typeof (message as any).stack === 'string') error.stack = (message as any).stack; + settle({ certified: false, error }); + }); + worker.on('error', (error) => { + const failure = error instanceof Error ? error : new Error(String(error)); + settle({ certified: false, error: failure }); + }); + worker.on('exit', (code) => { + // Only reached when no verdict arrived first; a verdict already settled it. + fail(`Certification of ${appName} exited with code ${code} without reporting a verdict`); + }); + }); + } finally { + // Terminated and AWAITED before the caller may delete the candidate tree, so a still-running + // candidate cannot race the sweep. + if (worker) { + try { + await worker.terminate(); + } catch (error) { + harperLogger.warn(`Could not terminate the validator for ${appName}:`, error); + } + } + if (timer) clearTimeout(timer); + // Both ends, or the channel keeps this thread's event loop referenced. + verdicts.close(); + releaseSlot(); + } +} diff --git a/components/componentLoader.ts b/components/componentLoader.ts index 27a304d70a..5116ac1785 100644 --- a/components/componentLoader.ts +++ b/components/componentLoader.ts @@ -157,6 +157,35 @@ function rootConfigMount(appName: string): ScopeMount | undefined { * branchedDatabases: [data] * ``` */ +/** + * The load options boot uses for a root-config application, so a deploy's certification load can be built + * from the same place instead of hand-plumbing a subset and drifting from it. + * + * `branchedDatabases` is REPORTED, not applied, when `forCertification` is set. A branch's location is + * derived only from the application and database names (`resources/branchDatabase.ts`), so a certification + * load that resolved branches the way boot does would open the very store the live application is serving + * from — a candidate could mutate rows, then throw, then be rejected, leaving the live version serving the + * mutation. Certifying against the base store instead is no better: it is not a rehearsal of the real + * thing and can write to base. So the caller is told the component is branch-configured and skips + * certification for it rather than certifying something it did not test. + */ +export function rootApplicationLoadOptions( + appName: string, + { forCertification = false }: { forCertification?: boolean } = {} +): { ok: true; options: Record; branchConfigured: boolean } | { ok: false } { + const mountResult = tryRootConfigMount(appName); + if (!mountResult.ok) return { ok: false }; + const branchedDatabases = rootConfigBranchedDatabases(appName); + const options: Record = { + isRoot: false, + autoReload: false, + appName, + mount: mountResult.mount, + }; + if (!forCertification) options.branchedDatabases = branchedDatabases; + return { ok: true, options, branchConfigured: branchedDatabases !== undefined }; +} + function rootConfigBranchedDatabases(appName: string): string[] | true | undefined { if (Object.hasOwn(TRUSTED_RESOURCE_PLUGINS, appName)) return undefined; return (getConfigObj()?.[appName] as any)?.branchedDatabases; @@ -269,15 +298,11 @@ export async function loadComponentDirectories( } return; } - const mountResult = tryRootConfigMount(appName); - if (!mountResult.ok) return; + const loadOptions = rootApplicationLoadOptions(appName); + if (!loadOptions.ok) return; const loadedModules = new Set(); await loadComponent(appFolder, cycleResources, HDB_ROOT_DIR_NAME, { - isRoot: false, - autoReload: false, - appName, - mount: mountResult.mount, - branchedDatabases: rootConfigBranchedDatabases(appName), + ...loadOptions.options, collectLoadedModules: loadedModules, }); await readyComponentModules(loadedModules, readyComponentPromises); diff --git a/components/deployValidator.ts b/components/deployValidator.ts new file mode 100644 index 0000000000..37759a3fb6 --- /dev/null +++ b/components/deployValidator.ts @@ -0,0 +1,71 @@ +'use strict'; + +// The guard first, exactly as `jobProcess.ts` does it: from here on this thread runs a CANDIDATE's code, +// which must not be able to terminate the thread out from under the verdict protocol. +import { realExit } from '../server/threads/workerProcessGuard.ts'; + +import { basename } from 'node:path'; +import { workerData } from 'node:worker_threads'; + +import { HDB_ROOT_DIR_NAME } from '../utility/hdbTerms.ts'; +import harperLogger from '../utility/logging/harper_logger.ts'; +import { Resources } from '../resources/Resources.ts'; +import { loadComponent, rootApplicationLoadOptions, setErrorReporter } from './componentLoader.ts'; + +/** + * Entry point for an ephemeral deploy-certification worker. + * + * Loads ONE candidate tree under its real application identity and root-config mount, and reports whether + * it loaded. Nothing else: this thread never joins the serving topology, serves a request, or outlives its + * verdict. It exists so that `deploy_component` can certify a candidate on any thread — the in-process + * check it replaces was gated on `!isMainThread`, and the operations API deploys on main, so an operator + * deploy was certified by nothing at all. + * + * The verdict is posted exactly once and the thread then exits. Every other outcome — a throw, an exit + * without a verdict, a malformed message — is failure at the parent, which is what makes "inability to + * obtain a verdict is failure" true rather than aspirational. + */ +const { candidateDirPath, appName, verdictPort } = workerData ?? {}; + +async function certify(): Promise { + const componentName = appName || basename(candidateDirPath); + const loadOptions = rootApplicationLoadOptions(componentName, { forCertification: true }); + if (!loadOptions.ok) { + throw new Error(`Cannot certify ${componentName}: its root-config mount could not be resolved`); + } + // The candidate's own load-time error, not just whether the promise rejected: the loader reports some + // failures through the error reporter while resolving successfully. + let reportedError: Error | undefined; + setErrorReporter((error: Error) => (reportedError ??= error)); + const resources = new Resources(); + // A certification load has to run the code a WORKER runs — the `start`/`handleApplication` extension + // path — or it proves only that the module parsed. + resources.isWorker = true; + await loadComponent(candidateDirPath, resources, HDB_ROOT_DIR_NAME, loadOptions.options); + if (reportedError) throw reportedError; +} + +function report(verdict: { ok: true } | { ok: false; message: string; stack?: string }): void { + // Its own channel rather than `parentPort`, which carries Harper's ITC traffic. A closed or absent + // channel is the parent's problem to detect (it treats a missing verdict as failure), so this must not + // throw its way out of the exit path. + try { + verdictPort?.postMessage(verdict); + } catch (error) { + harperLogger.warn('Deploy certification could not post its verdict:', error); + } +} + +void (async () => { + try { + await certify(); + report({ ok: true }); + realExit(0); + } catch (error) { + const failure = error instanceof Error ? error : new Error(String(error)); + // The message travels rather than the Error: an Error does not survive `postMessage` with its + // prototype, and the parent only needs what it will put in the operation's own error. + report({ ok: false, message: failure.message, stack: failure.stack }); + realExit(1); + } +})(); diff --git a/components/operations.js b/components/operations.js index 1e3eb3bda5..e87e2a3992 100644 --- a/components/operations.js +++ b/components/operations.js @@ -1,7 +1,6 @@ 'use strict'; const path = require('node:path'); -const { isMainThread } = require('node:worker_threads'); const fs = require('fs-extra'); const fg = require('fast-glob'); const normalize = require('normalize-path'); @@ -28,7 +27,6 @@ const { scanPackageDirectory, streamPackagedDirectory, } = require('../components/packageComponent.ts'); -const { Resources } = require('../resources/Resources.ts'); const { Application, prepareApplication, @@ -449,94 +447,6 @@ async function packageComponent(req) { // loading, A's load error lands in B, and A then activates broken code while B rejects a good candidate. // Validation is serialized (it is already the slow path) and the previous reporter is restored, so the // global is only ever owned by one in-flight validation. -let validationChain = Promise.resolve(); - -async function validateComponentLoads(candidateDirPath, emit) { - const run = validationChain.then( - () => validateComponentLoadsExclusive(candidateDirPath, emit), - () => validateComponentLoadsExclusive(candidateDirPath, emit) - ); - validationChain = run.then( - () => {}, - () => {} - ); - return run; -} - -async function validateComponentLoadsExclusive(candidateDirPath, emit) { - // now we attempt to actually load the component in case there is - // an error we can immediately detect and report, but app code should not run on the main thread - if (!isMainThread && !process.env.HARPER_SAFE_MODE) { - const pseudoResources = new Resources(); - pseudoResources.isWorker = true; - - const componentLoader = require('./componentLoader.ts').default || require('./componentLoader.ts'); - const { trackScopeClose } = require('./scopeShutdown.ts'); - let lastError; - const priorErrorReporter = componentLoader.getErrorReporter?.(); - componentLoader.setErrorReporter((error) => (lastError = error)); - emit('phase', { phase: 'load', status: 'start' }); - // This load exists only to surface load-time errors early; the Scopes it creates are - // throwaway. They are collected (instead of registered for worker-shutdown auto-close) so we - // can close them here once validation completes — otherwise each deploy leaks the Scope's - // deploy-lifecycle listeners on this worker, eventually tripping MaxListenersExceededWarning - // (#1462). - const validationScopes = new Set(); - // Process-wide `server.*` registrations (registerOperation, setMcpQuotaHandler) are not owned by - // a Scope, so a candidate's top-level registration during this throwaway load would otherwise - // outlive it and pollute the live worker on a failed/rolled-back deploy. The guard makes those - // registration methods no-op for the duration of the load. - const { runWithDeployValidationGuard } = require('../server/serverHelpers/deployValidationState.ts'); - // The candidate loads under the REAL component's name, so a candidate that throws would mark the live - // component ERROR. Its status writes are diverted into the guard's throwaway sink instead — see - // `deployValidationState.ts` for why this is context-scoped rather than captured and reverted here. - const componentName = path.basename(candidateDirPath); - // Extension modules the candidate load pulls in are registered in the loader's module registry keyed by - // module, so forgetting only the candidate's realpath leaves those behind — one set per deploy. Their - // identities are collected by the load itself; diffing the global registry instead would delete a live - // module registered by an interleaving real load, since validations serialize only with each other. - const validationModules = new Set(); - const validation = runWithDeployValidationGuard(async () => { - try { - await componentLoader.loadComponent(candidateDirPath, pseudoResources, undefined, { - collectScopes: validationScopes, - collectLoadedModules: validationModules, - }); - } finally { - const closeResults = await Promise.allSettled(Array.from(validationScopes, (scope) => scope.close())); - const failedCloses = closeResults.filter((result) => result.status === 'rejected'); - for (const result of failedCloses) { - log.warn('Failed to close a deploy-validation Scope', result.reason); - } - // A rejected close is a REJECTED VALIDATION, not a warning. `Scope.close()` stops at the - // throwing listener, so its remaining internal listener removal and subscription-hold release - // never run and the throwaway scope stays partially live — one leak per deploy, on the worker - // that serves the component. - if (failedCloses.length) { - throw new AggregateError( - failedCloses.map((result) => result.reason), - `Could not tear down deploy validation for ${componentName}: ${failedCloses.length} scope(s) failed to close` - ); - } - } - }); - // Track the load+close so a concurrent worker shutdown waits for these scopes to finish - // disposing — a plugin may start a native runtime in handleApplication — before realExit. - trackScopeClose(validation); - try { - await validation; - } finally { - componentLoader.setErrorReporter(priorErrorReporter); - // The candidate path is unique per deploy, so leaving it in the loader's realpath registry leaks - // one dead entry per deploy for the life of the process. - componentLoader.forgetLoadedPath?.(candidateDirPath); - componentLoader.forgetLoadedModules?.(validationModules); - } - emit('phase', { phase: 'load', status: 'done' }); - - if (lastError) throw lastError; - } -} /** Report a restart outcome the operation's own success message cannot convey. */ function logRestartOutcome(restart, what) { @@ -752,12 +662,21 @@ async function deployComponent(req) { // committed" — so a later failure arrives after both phases reported success. The operation's error // is the authority on whether the deploy landed, not the phase stream. emit('phase', { phase: 'prepare', status: 'start' }); + let prepareDoneEmitted = false; await prepareApplication(application, { - validateCandidate: async (candidateDirPath) => { - emit('phase', { phase: 'prepare', status: 'done' }); - await validateComponentLoads(candidateDirPath, emit); + // The same phases in the same order as before certification moved into the preparation: the + // `prepare` phase closes when the candidate is built, then `load` brackets certification. + emitPhase: (phase, status) => { + if (phase === 'load' && status === 'start' && !prepareDoneEmitted) { + prepareDoneEmitted = true; + emit('phase', { phase: 'prepare', status: 'done' }); + } + emit('phase', { phase, status }); }, }); + // A candidate that is deliberately not certified (safe mode, or a branch-configured component) + // emits no `load` phase, so `prepare` still has to be closed out. + if (!prepareDoneEmitted) emit('phase', { phase: 'prepare', status: 'done' }); const rollingRestart = req.restart === 'rolling'; // if doing a rolling restart set restart to false so that other nodes don't also restart. req.restart = rollingRestart ? false : req.restart; diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index d16befaee3..1da5d609e0 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -142,6 +142,7 @@ const listenersByType = new Map(); const messagesQueuedByType = new Map(); module.exports = { + buildWorkerExecArgv, startWorker, restartWorkers, shutdownWorkers, @@ -338,6 +339,43 @@ listenersByType.set(hdbTerms.ITC_EVENT_TYPES.OPERATION_EXECUTE_RESPONSE, null); listenersByType.set(THREAD_INFO, null); listenersByType.set(PROCESS_GROUP_TERMINATION_CONFIRMED, null); +/** + * The interpreter flags and preloads every Harper worker needs to load Harper's own module graph. + * + * Exported because a worker that must NOT join the serving topology still needs exactly these: a bare + * `new Worker()` cannot even load a module that imports JSON. Shared rather than copied so the two spawn + * paths cannot drift on something this load-bearing. + */ +function buildWorkerExecArgv() { + const isBun = typeof globalThis.Bun !== 'undefined'; + const execArgv = isBun + ? [] + : [ + '--enable-source-maps', + '--experimental-vm-modules', // used for giving applications their own top level scope + '--disable-warning=ExperimentalWarning', // yeah, yeah, we know it is experimental + '--expose-internals', // expose Node.js internal utils so jsLoader can use `decorateErrorStack()` + ]; + if (!isBun && envMgr.get(hdbTerms.CONFIG_PARAMS.THREADS_HEAPSNAPSHOTNEARLIMIT)) + execArgv.push('--heapsnapshot-near-heap-limit=1'); + // Preload configured modules (e.g. an APM agent like dd-trace) before the worker's entry + // script so they can instrument all subsequent Harper and app module loads. Resolved once + // (config and installed components are fixed for the process lifetime). `threads.preload` + // uses --import (ESM/loader-hook registration, e.g. dd-trace/register.js — the entry that + // instruments worker threads); `threads.preloadRequire` uses --require for CJS agents that + // document that path (e.g. dd-trace/init, Dynatrace OneAgent). --import is URL-based, so + // resolved paths are passed as file URLs. Not supported under Bun, which does not use + // execArgv here. Safe mode also omits preloads because they are configured code, + // which safe mode must not resolve or execute. + const isSafeMode = + process.env.HARPER_SAFE_MODE && process.env.HARPER_SAFE_MODE !== 'false' && process.env.HARPER_SAFE_MODE !== '0'; + if (!isBun && !isSafeMode) { + for (const importPath of getImportModules()) execArgv.push('--import', pathToFileURL(importPath).href); + for (const requirePath of getRequireModules()) execArgv.push('--require', requirePath); + } + return execArgv; +} + function startWorker(path, options = {}) { if (processShuttingDown) { const error = new Error('Cannot start a worker while the Harper process is shutting down'); @@ -378,32 +416,7 @@ function startWorker(path, options = {}) { if (!extname(path)) path += '.js'; - const isBun = typeof globalThis.Bun !== 'undefined'; - const execArgv = isBun - ? [] - : [ - '--enable-source-maps', - '--experimental-vm-modules', // used for giving applications their own top level scope - '--disable-warning=ExperimentalWarning', // yeah, yeah, we know it is experimental - '--expose-internals', // expose Node.js internal utils so jsLoader can use `decorateErrorStack()` - ]; - if (!isBun && envMgr.get(hdbTerms.CONFIG_PARAMS.THREADS_HEAPSNAPSHOTNEARLIMIT)) - execArgv.push('--heapsnapshot-near-heap-limit=1'); - // Preload configured modules (e.g. an APM agent like dd-trace) before the worker's entry - // script so they can instrument all subsequent Harper and app module loads. Resolved once - // (config and installed components are fixed for the process lifetime). `threads.preload` - // uses --import (ESM/loader-hook registration, e.g. dd-trace/register.js — the entry that - // instruments worker threads); `threads.preloadRequire` uses --require for CJS agents that - // document that path (e.g. dd-trace/init, Dynatrace OneAgent). --import is URL-based, so - // resolved paths are passed as file URLs. Not supported under Bun, which does not use - // execArgv here. Safe mode also omits preloads because they are configured code, - // which safe mode must not resolve or execute. - const isSafeMode = - process.env.HARPER_SAFE_MODE && process.env.HARPER_SAFE_MODE !== 'false' && process.env.HARPER_SAFE_MODE !== '0'; - if (!isBun && !isSafeMode) { - for (const importPath of getImportModules()) execArgv.push('--import', pathToFileURL(importPath).href); - for (const requirePath of getRequireModules()) execArgv.push('--require', requirePath); - } + const execArgv = buildWorkerExecArgv(); const worker = new Worker(isAbsolute(path) ? path : join(PACKAGE_ROOT, path), { resourceLimits: { diff --git a/unitTests/components/deployActivation.test.js b/unitTests/components/deployActivation.test.js index a6b55251f2..64eaa5738c 100644 --- a/unitTests/components/deployActivation.test.js +++ b/unitTests/components/deployActivation.test.js @@ -454,7 +454,7 @@ describe('activation transaction', () => { const app = new Application({ name: 'web' }); app.dirPath = path.join(root, 'web'); - await activateCandidateApplication(app, 'd1'); + await activateCandidateApplication(app, 'd1', { certified: false }); assert.strictEqual(await readLive(root, 'web'), 'CANDIDATE\n'); assert.strictEqual(existsSync(path.join(root, DEPLOY_STAGING_DIR, 'd1')), false, 'staging is cleaned up'); @@ -484,7 +484,7 @@ describe('activation transaction', () => { const app = new Application({ name: 'web' }); app.dirPath = live; - await activateCandidateApplication(app, 'd1'); + await activateCandidateApplication(app, 'd1', { certified: false }); const linkPath = path.join(live, 'node_modules', 'probe'); const target = await fs.readlink(linkPath); @@ -518,7 +518,7 @@ describe('activation transaction', () => { const app = new Application({ name: 'web' }); app.dirPath = live; - await activateCandidateApplication(app, 'd1'); + await activateCandidateApplication(app, 'd1', { certified: false }); assert.strictEqual( await fs.readlink(path.join(live, 'node_modules', 'shared')), @@ -548,7 +548,7 @@ describe('activation transaction', () => { const app = new Application({ name: 'web' }); app.dirPath = path.join(root, 'web'); - await activateCandidateApplication(app, 'd1'); + await activateCandidateApplication(app, 'd1', { certified: false }); assert.strictEqual(await readLive(root, 'web'), 'CANDIDATE\n'); assert.strictEqual(app.isNewComponent, true, 'and it is recognized as a new component'); diff --git a/unitTests/components/deployCandidateBuild.test.js b/unitTests/components/deployCandidateBuild.test.js index bd0a7b7950..1771f9f812 100644 --- a/unitTests/components/deployCandidateBuild.test.js +++ b/unitTests/components/deployCandidateBuild.test.js @@ -109,7 +109,7 @@ describe('deploy candidate builds', () => { // chmod cannot make a file unreadable to root, so as root this would assert nothing at all. if (process.platform === 'win32' || process.getuid?.() === 0) return this.skip(); this.timeout(20000); - const { markCandidateComplete } = require('#src/components/Application'); + const { syncCandidateTree } = require('#src/components/Application'); const componentsRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'candidate-filelink-')); const dirPath = await makeLiveComponent(componentsRoot, 'web', 'LIVE v1\n'); // A `file:` candidate is a symlink into a developer's own tree, which can hold a file the Harper uid @@ -127,10 +127,10 @@ describe('deploy candidate builds', () => { await fs.mkdir(deploymentDir, { recursive: true }); await fs.symlink(source, path.join(deploymentDir, 'web'), 'dir'); - await markCandidateComplete(dirPath, 'dep-link', 'web'); - - assert.ok( - existsSync(path.join(deploymentDir, '.complete')), + // The durability step specifically, not the marker: `.complete` is gated on certification now, and a + // test seam that could write it without one would be a seam production could use too. + await assert.doesNotReject( + () => syncCandidateTree(dirPath, 'dep-link'), 'the deploy is not failed by a file it never wrote and cannot read' ); await fs.chmod(unreadable, 0o600); From d79efa11e1aa703f217758b0c6136a4b901f9563 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 2 Sep 2026 14:34:52 -0400 Subject: [PATCH 02/29] test(deploy): prove a throwing candidate is rejected on the MAIN thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test that makes this step's claim checkable, and it fails on `main` with "Missing expected rejection" — there, `prepareApplication` with a candidate that throws at load *succeeds* and publishes it. That is the defect: the in-process check was gated on `!isMainThread` and the operations API deploys on main, so step 1's ordering fix reordered a no-op. Tests run on the main thread, so this covers exactly the path that was unprotected rather than a helper in isolation. It asserts the previous version still serves byte for byte, and that nothing was left behind claiming the rejected candidate had been validated. Three more: - a candidate that loads cleanly is still published, so the gate is not simply refusing everything; - a candidate whose load never returns is rejected on a deadline rather than waited on — the in-process check had no answer for this and held its validation chain for the life of the process. The fixture blocks with `Atomics.wait` rather than spinning, since a top-level `await` in a CJS resource is a syntax error and proved nothing; - `markCandidateComplete` refuses to mint `.complete` with no certification, which is the gate itself. --- .../components/deployCertification.test.js | 125 ++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 unitTests/components/deployCertification.test.js diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js new file mode 100644 index 0000000000..cdecea1ab4 --- /dev/null +++ b/unitTests/components/deployCertification.test.js @@ -0,0 +1,125 @@ +'use strict'; + +const assert = require('node:assert'); +const { mkdir, mkdtemp, readFile, rm, writeFile } = require('node:fs/promises'); +const { existsSync } = require('node:fs'); +const { tmpdir } = require('node:os'); +const { join } = require('node:path'); + +const testUtils = require('../testUtils.js'); +testUtils.preTestPrep(); + +const { Application, prepareApplication, markCandidateComplete } = require('#src/components/Application'); +const { certifyCandidate } = require('#src/components/certifyCandidate'); +const { packageDirectory } = require('#src/components/packageComponent'); + +/** A component payload whose `resource.js` runs `body` when the component is loaded. */ +async function payloadThatRunsOnLoad(rootDir, name, version, body) { + const sourceDir = await mkdtemp(join(rootDir, `${name}-${version}-`)); + await writeFile(join(sourceDir, 'package.json'), JSON.stringify({ name, version, main: 'resource.js' })); + await writeFile(join(sourceDir, 'config.yaml'), 'jsResource:\n files: resource.js\n'); + await writeFile(join(sourceDir, 'resource.js'), body); + return packageDirectory(sourceDir, { skip_node_modules: true }); +} + +describe('deploy certification', () => { + it('does not publish a candidate that throws at load — ON THE MAIN THREAD', async function () { + // The whole point of this step. The in-process check this replaces was gated on `!isMainThread`, and + // the operations API deploys on main, so this exact deploy used to succeed and publish a broken + // component while reporting an error. Tests run on the main thread, so this is that path. + this.timeout(30000); + const rootDir = await mkdtemp(join(tmpdir(), 'certify-rejects-')); + const componentDirPath = join(rootDir, 'shop'); + await mkdir(componentDirPath, { recursive: true }); + await writeFile(join(componentDirPath, 'package.json'), JSON.stringify({ name: 'shop', version: '1.0.0' })); + await writeFile(join(componentDirPath, 'index.js'), 'module.exports = { live: 1 };\n'); + + const application = new Application({ + name: 'shop', + payload: await payloadThatRunsOnLoad(rootDir, 'shop', '2.0.0', "throw new Error('candidate blew up at load');\n"), + }); + application.dirPath = componentDirPath; + + try { + await assert.rejects(() => prepareApplication(application), /candidate blew up at load/); + // v1 is still the live tree, byte for byte. + assert.strictEqual( + JSON.parse(await readFile(join(componentDirPath, 'package.json'), 'utf8')).version, + '1.0.0', + 'the previous version still serves' + ); + assert.strictEqual(await readFile(join(componentDirPath, 'index.js'), 'utf8'), 'module.exports = { live: 1 };\n'); + // And nothing was left claiming the rejected candidate was validated. + assert.ok(!existsSync(join(rootDir, '.deploy-staging')), 'the rejected candidate was swept'); + } finally { + await rm(rootDir, { recursive: true, force: true }); + } + }); + + it('publishes a candidate that loads cleanly', async function () { + this.timeout(30000); + const rootDir = await mkdtemp(join(tmpdir(), 'certify-accepts-')); + const componentDirPath = join(rootDir, 'shop'); + await mkdir(componentDirPath, { recursive: true }); + await writeFile(join(componentDirPath, 'package.json'), JSON.stringify({ name: 'shop', version: '1.0.0' })); + + const application = new Application({ + name: 'shop', + payload: await payloadThatRunsOnLoad(rootDir, 'shop', '2.0.0', 'module.exports = { fine: true };\n'), + }); + application.dirPath = componentDirPath; + + try { + await prepareApplication(application); + assert.strictEqual( + JSON.parse(await readFile(join(componentDirPath, 'package.json'), 'utf8')).version, + '2.0.0', + 'a certified candidate is published' + ); + } finally { + await rm(rootDir, { recursive: true, force: true }); + } + }); + + it('rejects a candidate whose load never finishes, rather than waiting on it', async function () { + this.timeout(30000); + const rootDir = await mkdtemp(join(tmpdir(), 'certify-timeout-')); + const candidateDirPath = join(rootDir, 'hangs'); + await mkdir(candidateDirPath, { recursive: true }); + await writeFile(join(candidateDirPath, 'package.json'), JSON.stringify({ name: 'hangs', version: '1.0.0' })); + await writeFile(join(candidateDirPath, 'config.yaml'), 'jsResource:\n files: resource.js\n'); + // Blocks the validator thread outright — `Atomics.wait` rather than a spin loop, so it holds without + // burning CPU. The in-process check had no answer for a load that never returns: it held the + // validation chain for the life of the process. An isolated thread can simply be given a deadline. + await writeFile( + join(candidateDirPath, 'resource.js'), + 'const shared = new Int32Array(new SharedArrayBuffer(4));\nAtomics.wait(shared, 0, 0);\n' + ); + + try { + const outcome = await certifyCandidate(candidateDirPath, 'hangs', { timeoutMs: 2000 }); + assert.strictEqual(outcome.certified, false); + assert.match(outcome.error.message, /did not finish within 2000ms/); + } finally { + await rm(rootDir, { recursive: true, force: true }); + } + }); + + it('refuses to mint .complete for a candidate no validator certified', async function () { + // The gate itself. `.complete` is what recovery treats as proof a validation happened, so the + // function that writes it has to require the verdict rather than trust its caller — three of + // `prepareApplication`'s four call sites never asked for one. + const rootDir = await mkdtemp(join(tmpdir(), 'certify-gate-')); + const componentDirPath = join(rootDir, 'shop'); + await mkdir(join(rootDir, '.deploy-staging', 'd1', 'shop'), { recursive: true }); + try { + await assert.rejects( + () => markCandidateComplete(componentDirPath, 'd1', 'shop'), + /no validator has certified it/ + ); + assert.ok(!existsSync(join(rootDir, '.deploy-staging', 'd1', '.complete')), 'and no authority was written'); + } finally { + await rm(rootDir, { recursive: true, force: true }); + } + }); +}); From 0ae8848758be16500139ee01a717d9dd99e15092 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 2 Sep 2026 14:36:43 -0400 Subject: [PATCH 03/29] docs(design): record certification and why the validator is not a startWorker thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also covers the two cases that earn no authority rather than being refused, and the three validator requirements that are easy to miss — its own MessageChannel, noServerStart, and the compiled entry — since each of those was found by building it rather than by reading the code. --- DESIGN.md | 49 +++++++++++++++++-- .../components/deployCertification.test.js | 34 +++++++++++++ 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 21842ef26a..044e26bd58 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -454,10 +454,53 @@ with its journal intact. Only the sweep itself is best-effort, because it costs decision. For the same reason, a swap whose rename cannot be confirmed on storage skips both the retire and the journal removal: the journal is what would carry the activation forward after a power loss. -Three limits are deliberate and tracked separately: activation is two renames, so the live _pathname_ is +Two limits are deliberate and tracked separately: activation is two renames, so the live _pathname_ is briefly absent (in-memory resources are unaffected, but a component that opens its own files during a -request can still see a gap); validation does not run on the main-thread deploy path; and config -publication is not yet an effect of this transaction, as above. +request can still see a gap); and config publication is not yet an effect of this transaction, as above. + +## Certification: `.complete` requires a verdict, and the mint enforces it + +`.complete` is what recovery treats as proof that a candidate both built and validated, so the function +that writes it requires the verdict rather than trusting its caller to have asked for one. `validateCandidate` +used to be an optional callback on `prepareApplication` and only one of its four production call sites +supplied it — the same _one rule, N sites_ shape that produced most of this area's defects. The record of +which candidates a validator certified is module-internal: a proof passed as an argument is one an external +caller can forge, or a future caller can forget. + +The verdict comes from an **ephemeral validator thread**, not from a `startWorker` one. That function builds +a `MessageChannel` per connected port, announces the new port to every peer, and registers for monitoring +and restart, so a validator would join the ITC mesh — letting a candidate's top-level +`server.registerOperation` announce itself and traffic route at a thread about to exit, at +O(deploys × workers) channels on a large node. Only the interpreter setup is shared, as +`buildWorkerExecArgv`; without it the thread cannot load Harper's own module graph at all. Three things the +validator needs that are easy to miss: its own `MessageChannel` for the verdict (`parentPort` carries +Harper's ITC traffic, so an unrelated message reads as a malformed verdict), `workerData.noServerStart` +(or `threadServer` boots at module scope and loads every root component), and the compiled entry path. + +Every outcome other than an explicit passing verdict is failure — a throw, an exit without a verdict, a +malformed message, a closed channel, a deadline — because the alternative is minting authority from +silence. A spawn failure is a deploy failure, not a success. The worker is terminated and its exit awaited +before its tree is swept, so a still-running candidate cannot race the sweep. + +Isolation contains the JS heap, the module registry, process-global registrations and component status. It +does **not** contain databases, the filesystem, the network or native addons: a candidate can write before +it throws. + +Two cases earn no authority rather than being refused — the rule is _no verdict means no authority_, never +_no verdict means no deploy_: + +- **Safe mode** stages without activating. It may not execute configured code, so it certifies nothing, and + nothing uncertified is published. Safe mode is transient, so the next ordinary preparation finishes it. +- **A branch-configured component** deploys uncertified. A branch's location is derived only from the + application and database names, so a certification load would open the store the live version is serving + from: a candidate could mutate rows, throw, be rejected, and leave the live version serving the mutation. + Certifying against the base store instead is no better. Unlike safe mode this is not deferrable — + certification cannot succeed for these until validation-scoped branch storage exists — so it activates as + it does today and simply mints no `.complete`. + +The guarantee is scoped to the lifetime of a preparation. A package deploy's root-config entry is still +written before the build and never rolled back, so a rejected v2 can be re-prepared and activated after a +restart; closing that needs config staged with activation. ## Component preparation is serialized across worker threads diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index cdecea1ab4..65697c6ab6 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -122,4 +122,38 @@ describe('deploy certification', () => { await rm(rootDir, { recursive: true, force: true }); } }); + + it('stages without activating in safe mode, so nothing uncertified is published', async function () { + // Safe mode may not execute configured code, so it can certify nothing — and a candidate nothing + // certified must not be published. It is also transient, which is why "pending" is the right answer + // here and the wrong one for a branch-configured component: the next ordinary preparation certifies + // and activates this. + this.timeout(30000); + const rootDir = await mkdtemp(join(tmpdir(), 'certify-safe-mode-')); + const componentDirPath = join(rootDir, 'shop'); + await mkdir(componentDirPath, { recursive: true }); + await writeFile(join(componentDirPath, 'package.json'), JSON.stringify({ name: 'shop', version: '1.0.0' })); + + const application = new Application({ + name: 'shop', + payload: await payloadThatRunsOnLoad(rootDir, 'shop', '2.0.0', 'module.exports = { fine: true };\n'), + }); + application.dirPath = componentDirPath; + + const priorSafeMode = process.env.HARPER_SAFE_MODE; + process.env.HARPER_SAFE_MODE = '1'; + try { + await prepareApplication(application); + assert.strictEqual( + JSON.parse(await readFile(join(componentDirPath, 'package.json'), 'utf8')).version, + '1.0.0', + 'the live version is untouched — the candidate was staged, not activated' + ); + assert.ok(existsSync(join(rootDir, '.deploy-staging')), 'and the staged candidate is kept for later'); + } finally { + if (priorSafeMode === undefined) delete process.env.HARPER_SAFE_MODE; + else process.env.HARPER_SAFE_MODE = priorSafeMode; + await rm(rootDir, { recursive: true, force: true }); + } + }); }); From 2c0badb2642ccaa0f2df95195eb1e05993749083 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 2 Sep 2026 18:24:35 -0400 Subject: [PATCH 04/29] test(deploy): prove the wiring through the operations API, and the branch decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The planning review was explicit that a helper-only test cannot prove this step, because what was broken was the WIRING on the main thread rather than the check itself. So this drives the real operations API: deploy v1, deploy a v2 that installs cleanly and throws at load, assert the operation fails — and then assert v1 still **answers requests**, which step 1's availability test explicitly did not cover, since it sampled the component directory on disk and exercised no route. A second case deploys a loadable v3, because a gate that refuses everything would satisfy the first assertion just as well. The response body is not surfaced by the shared `operation` helper, so the candidate's own error message is asserted in the unit test rather than here; this one asserts the operation failed, which is the part that was wrong. Also unit-tests the branch-configured decision directly: boot still gets its `branchedDatabases`, certification never does, and the caller is told to skip certifying. Mutation-verified — handing certification the live branch settings fails it. That path is worth testing at this level because the hazard is exactly that a certification load would open the store the live version serves from. --- .../deploy/certified-deploy.test.ts | 105 ++++++++++++++++++ .../components/deployCertification.test.js | 36 ++++++ 2 files changed, 141 insertions(+) create mode 100644 integrationTests/deploy/certified-deploy.test.ts diff --git a/integrationTests/deploy/certified-deploy.test.ts b/integrationTests/deploy/certified-deploy.test.ts new file mode 100644 index 0000000000..2c87f85514 --- /dev/null +++ b/integrationTests/deploy/certified-deploy.test.ts @@ -0,0 +1,105 @@ +/** + * `deploy_component` never publishes a candidate that fails to load — through the real operations API, + * which deploys on the MAIN thread (#2315 step 2). + * + * The regression this guards: the load check used to be gated on `!isMainThread`, and the operations API + * deploys on main, so for an operator deploy it ran nothing at all. Step 1 put the check between build and + * swap, but on this path there was no check to order — a candidate that installed cleanly and threw at + * load was published anyway, while the operation reported an error. + * + * Deliberately end to end rather than a unit test of the certification helper: what was broken was the + * WIRING on this thread, and a helper test cannot see that. So this drives the API, then asserts the + * previous release still ANSWERS REQUESTS — which step 1's availability test explicitly did not cover, + * since it sampled the component directory on disk and exercised no route. + */ +import { suite, test, before, after } from 'node:test'; +import { ok, strictEqual } from 'node:assert'; +import { join } from 'node:path'; +import { mkdtemp, writeFile, rm, readFile } from 'node:fs/promises'; +import { existsSync } from 'node:fs'; +import { tmpdir } from 'node:os'; + +import { startHarper, teardownHarper, targz, type ContextWithHarper } from '@harperfast/integration-testing'; +import { operation, readVersion } from './redeploy-restart-flag-helpers.ts'; + +const PROJECT = 'certified-deploy'; + +/** A component exposing its version over REST. `throwsAtLoad` makes its resource module throw on import. */ +async function buildPayload(version: number, { throwsAtLoad = false } = {}): Promise { + const dir = await mkdtemp(join(tmpdir(), 'certified-deploy-fixture-')); + try { + await writeFile(join(dir, 'package.json'), JSON.stringify({ name: PROJECT, version: `${version}.0.0` })); + await writeFile(join(dir, 'version.txt'), String(version)); + await writeFile(join(dir, 'config.yaml'), 'rest: true\njsResource:\n files: resource.js\n'); + await writeFile( + join(dir, 'resource.js'), + throwsAtLoad + ? `throw new Error('v${version} cannot load');\n` + : `export class Version extends Resource {\n\tget() {\n\t\treturn { version: ${version} };\n\t}\n}\n` + ); + return await targz(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +suite('deploy_component certifies a candidate before publishing it', (ctx: ContextWithHarper) => { + before(async () => { + await startHarper(ctx); + }); + + after(async () => { + await teardownHarper(ctx); + }); + + test('a candidate that throws at load is rejected, and the previous release keeps answering', async () => { + const componentsRoot = join(ctx.harper.dataRootDir, 'components'); + const livePath = join(componentsRoot, PROJECT); + + // v1: a component that loads and serves. + await operation(ctx, { + operation: 'deploy_component', + project: PROJECT, + payload: await buildPayload(1), + restart: true, + }); + strictEqual(await readFile(join(livePath, 'version.txt'), 'utf8'), '1', 'v1 is live'); + + // v2: installs cleanly, throws the moment it is loaded. + let rejection: string | undefined; + try { + await operation(ctx, { + operation: 'deploy_component', + project: PROJECT, + payload: await buildPayload(2, { throwsAtLoad: true }), + restart: true, + }); + } catch (error) { + rejection = error instanceof Error ? error.message : String(error); + } + + // Non-2xx. The helper does not surface the response body, so the candidate's own message is asserted + // in the unit test instead; what matters here is that the operation failed rather than reporting + // success over a component that cannot load. + ok(rejection, 'the deploy is reported as failed'); + + // The point, and what step 1's availability test explicitly could not show: v1 still ANSWERS, not + // merely still exists on disk. + strictEqual(await readVersion(ctx), 1, 'v1 still answers requests'); + strictEqual(await readFile(join(livePath, 'version.txt'), 'utf8'), '1', 'and is still the live tree'); + ok(!existsSync(join(componentsRoot, '.deploy-staging')), 'and the rejected candidate was swept'); + }); + + test('a candidate that loads cleanly is still published', async () => { + // The gate has to accept as well as refuse, or the previous assertion proves only that deploys break. + const livePath = join(ctx.harper.dataRootDir, 'components', PROJECT); + await operation(ctx, { + operation: 'deploy_component', + project: PROJECT, + payload: await buildPayload(3), + restart: true, + }); + strictEqual(await readFile(join(livePath, 'version.txt'), 'utf8'), '3', 'v3 is published'); + strictEqual(await readVersion(ctx), 3, 'and answers requests'); + }); +}); diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index 65697c6ab6..c4dc2efe09 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -12,6 +12,8 @@ testUtils.preTestPrep(); const { Application, prepareApplication, markCandidateComplete } = require('#src/components/Application'); const { certifyCandidate } = require('#src/components/certifyCandidate'); const { packageDirectory } = require('#src/components/packageComponent'); +const { rootApplicationLoadOptions } = require('#src/components/componentLoader'); +const { getConfigObj } = require('#src/config/configUtils'); /** A component payload whose `resource.js` runs `body` when the component is loaded. */ async function payloadThatRunsOnLoad(rootDir, name, version, body) { @@ -156,4 +158,38 @@ describe('deploy certification', () => { await rm(rootDir, { recursive: true, force: true }); } }); + + describe('branch-configured components', () => { + // A branch's location is derived only from the application and database names, so a certification + // load that resolved branches the way boot does would open the store the LIVE version is serving + // from — a candidate could mutate rows, throw, be rejected, and leave the live version serving the + // mutation. So certification never applies them, and the caller is told to skip certifying instead. + const appName = 'branch_certify_probe'; + + afterEach(() => { + delete getConfigObj()[appName]; + }); + + it('reports the component as branch-configured and withholds the branch settings', () => { + getConfigObj()[appName] = { branchedDatabases: ['data'] }; + + const forBoot = rootApplicationLoadOptions(appName); + const forCertification = rootApplicationLoadOptions(appName, { forCertification: true }); + + assert.deepStrictEqual(forBoot.options.branchedDatabases, ['data'], 'boot still gets its branches'); + assert.ok( + !('branchedDatabases' in forCertification.options), + 'certification is never handed the live branch settings' + ); + assert.strictEqual(forCertification.branchConfigured, true, 'and the caller is told to skip certifying'); + }); + + it('reports an ordinary component as not branch-configured', () => { + getConfigObj()[appName] = { package: 'npm:whatever@1.0.0' }; + + const forCertification = rootApplicationLoadOptions(appName, { forCertification: true }); + + assert.strictEqual(forCertification.branchConfigured, false, 'so it is certified normally'); + }); + }); }); From 4fddc765c3995ed2f96830597b7bff12a7e8e495 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 2 Sep 2026 18:55:19 -0400 Subject: [PATCH 05/29] fix(deploy): close four certification blockers from the first review round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **A caller-supplied `certified` flag was exactly the forgeable proof the internal record exists to avoid.** `activateCandidateApplication` is exported, so any caller could pass `certified: true` and mint authority for an uncertified tree. The parameter is gone: the record decides, the caller cannot assert. That also made the swap tests simpler — nothing certified those candidates, so they naturally skip minting. **The verdict port was reachable from candidate code.** `workerData` is visible via `require('node:worker_threads')`, so a candidate could post its own passing verdict and certify itself. The port is captured and deleted from `workerData` before any candidate code runs — the difference between a capability this module holds and one the whole thread holds. **Success was posted before Scope teardown.** The in-process check I deleted had established that a Scope which fails to close is a REJECTED validation, not a warning: `close()` stops at the throwing listener, so the scope stays partially live. Posting a pass and then failing teardown would certify a candidate whose own cleanup is broken — and since the thread exits either way, nothing downstream would ever learn. Teardown now happens before the verdict, and a failed close fails certification. That protection was mine to lose and I lost it. **`terminate()` is a NAPI segfault under Bun**, which I noted in the design and then used anyway. Under Bun the worker is asked to exit itself and its exit awaited, matching `manageThreads`; a termination that fails is now reported rather than treated as cleanup done, because the caller is about to remove a tree the thread may still be reading. **`.complete` now carries content, and recovery requires it.** An empty marker is one an older build wrote after a validation that was a no-op on the main thread — indistinguishable, until now, from one a validator earned. A candidate staged by an older build and found after an upgrade therefore rolls BACK to the committed tree rather than forward onto something nothing certified. Conservative direction, and it costs only an in-flight deploy across an upgrade. Two tests added, both mutation-verified: accepting an empty marker as authority fails the legacy-marker test, and a branch-configured component still deploys. Stated rather than implied: the composition of "activate, mint nothing, then crash" is not covered — it needs a crash the unit harness cannot stage. --- components/Application.ts | 78 +++++++++++++++---- components/certifyCandidate.ts | 29 ++++++- components/deployValidator.ts | 33 +++++++- unitTests/components/deployActivation.test.js | 25 ++++-- .../components/deployCertification.test.js | 30 +++++++ 5 files changed, 167 insertions(+), 28 deletions(-) diff --git a/components/Application.ts b/components/Application.ts index 2db067d045..e0d13ab540 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -1021,6 +1021,17 @@ export async function extractApplication( // rejects, so the collision is unreachable from a root-config key as well as from a deploy. const CANDIDATE_COMPLETE_MARKER = '.complete'; +/** + * What a `.complete` marker must contain to count as roll-forward authority. + * + * The marker used to be written empty, after a validation that was a no-op on the main thread — so an + * empty marker is one minted without any verdict at all. Recovery now requires this payload, which means + * a candidate staged by an older build and found after an upgrade rolls BACK to the committed tree rather + * than forward onto something nothing certified. That is the conservative direction and it costs only an + * in-flight deploy across an upgrade. + */ +const CANDIDATE_CERTIFIED_MARKER_BODY = 'certified:1'; + /** * Candidates a validator has certified, by `\0`. * @@ -1039,6 +1050,33 @@ function certificationKey(componentDirPath: string, deploymentId: string): strin return `${componentDirPath}\0${deploymentId}`; } +/** + * Whether the on-disk marker says a validator certified this candidate. + * + * Content, not mere existence: an empty marker predates certification and was minted after a validation + * that did nothing on the main thread. An unreadable marker is not authority either — the same + * fail-closed reading the rest of this protocol uses. + */ +async function candidateIsCertifiedOnDisk(deploymentDirPath: string): Promise { + try { + const body = await readFile(join(deploymentDirPath, CANDIDATE_COMPLETE_MARKER), 'utf8'); + return body.trim() === CANDIDATE_CERTIFIED_MARKER_BODY; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + logger.warn( + `Treating the completion marker in ${deploymentDirPath} as absent because it could not be read: ` + + errorMessage(error) + ); + } + return false; + } +} + +/** Whether a validator returned a passing verdict for exactly this candidate. */ +function isCandidateCertified(componentDirPath: string, deploymentId: string): boolean { + return certifiedCandidates.has(certificationKey(componentDirPath, deploymentId)); +} + /** Record that a validator returned a passing verdict for exactly this candidate. */ function recordCandidateCertified(componentDirPath: string, deploymentId: string): void { certifiedCandidates.add(certificationKey(componentDirPath, deploymentId)); @@ -1840,7 +1878,7 @@ async function settleInterruptedActivation( ); const liveExists = await exists(liveDirPath); const candidateExists = await exists(candidateDirPath); - const candidateComplete = await exists(join(deploymentDirPath, CANDIDATE_COMPLETE_MARKER)); + const candidateComplete = await candidateIsCertifiedOnDisk(deploymentDirPath); const asideRecords = await inProgressAsideRecords(asideStagingDir); const rollForward = async () => { @@ -2080,7 +2118,7 @@ export async function markCandidateComplete( // The gate. Everything below writes the marker recovery trusts, so an uncertified candidate must not // get here — and the check lives at the mint rather than at the caller for the reason `certifiedCandidates` // documents. - if (!certifiedCandidates.has(certificationKey(componentDirPath, deploymentId))) { + if (!isCandidateCertified(componentDirPath, deploymentId)) { throw new Error( `Refusing to mark the ${componentName} candidate ${deploymentId} complete: no validator has ` + `certified it, and \`${CANDIDATE_COMPLETE_MARKER}\` is what recovery treats as proof that one did` @@ -2093,7 +2131,10 @@ export async function markCandidateComplete( if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; } try { - await writeControlFileDurably(candidateCompleteMarkerPath(componentDirPath, deploymentId), ''); + await writeControlFileDurably( + candidateCompleteMarkerPath(componentDirPath, deploymentId), + CANDIDATE_CERTIFIED_MARKER_BODY + ); } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; } @@ -2159,11 +2200,7 @@ async function certifyPreparedCandidate( return { certified: true }; } -export async function activateCandidateApplication( - application: Application, - deploymentId: string, - { certified = true }: { certified?: boolean } = {} -): Promise { +export async function activateCandidateApplication(application: Application, deploymentId: string): Promise { const liveDirPath = application.dirPath; const candidateDirPath = candidateApplicationPath(liveDirPath, deploymentId); const deploymentDirPath = candidateDeploymentDirPath(liveDirPath, deploymentId); @@ -2179,12 +2216,21 @@ export async function activateCandidateApplication( throw new Error(`Cannot activate ${application.name}: no candidate build at ${candidateDirPath}`); } - // `certified: false` is the DELIBERATE uncertified path — a branch-configured component, which cannot be - // certified safely yet. It activates without ever minting `.complete`, so a crash mid-swap rolls back to - // the committed tree instead of forward onto something no validator vouched for. The gate inside - // `markCandidateComplete` still guards the accidental case: a caller that reaches the mint without a - // verdict is a bug, not a policy. - if (certified) await markCandidateComplete(liveDirPath, deploymentId, application.name); + // The RECORD decides, never the caller. An argument saying "this one is certified" would be the forgeable + // proof the internal record exists to avoid — and an exported function with such a flag lets any caller + // mint authority for an uncertified tree, which is the invariant this step is for. + // + // So: certified candidates get `.complete`; the deliberately uncertified ones (a branch-configured + // component) simply do not, and a crash mid-swap rolls them back to the committed tree rather than + // forward onto something no validator vouched for. + if (isCandidateCertified(liveDirPath, deploymentId)) { + await markCandidateComplete(liveDirPath, deploymentId, application.name); + } else { + application.logger.warn( + `Activating ${application.name} without a certification marker: nothing certified this candidate, so ` + + `recovery will roll it back rather than forward` + ); + } const journalPath = activationJournalPath(liveDirPath, deploymentId); try { await writeControlFileDurably( @@ -3552,9 +3598,7 @@ export async function prepareApplication(application: Application, options: Prep application.installationIsOpaque ); } - await activateCandidateApplication(application, deploymentId, { - certified: certification.certified, - }); + await activateCandidateApplication(application, deploymentId); } catch (error) { // The builder's own cleanup only covers a failed BUILD. A rejected validation, or an // activation that was cleanly compensated, would otherwise leave a whole installed diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 34827836e0..19ad38ca59 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -6,7 +6,14 @@ import { MessageChannel, Worker } from 'node:worker_threads'; import harperLogger from '../utility/logging/harper_logger.ts'; import { buildWorkerExecArgv } from '../server/threads/manageThreads.js'; -/** How long a certification load may take before the candidate is rejected, when nothing else says. */ +/** + * How long a certification load may take before the candidate is rejected. + * + * Its OWN budget, measured from validator spawn — deliberately not the operations-API timeout. A component + * may legally declare a longer load timeout than that, so borrowing it would reject a candidate the + * serving workers would happily load; and a clock started at the request would already be overdue after a + * long `npm install`. Callers with no request budget at all (boot, `addComponent`) get this default. + */ const DEFAULT_CERTIFICATION_TIMEOUT_MS = 120_000; /** @@ -142,11 +149,27 @@ export async function certifyCandidate( } finally { // Terminated and AWAITED before the caller may delete the candidate tree, so a still-running // candidate cannot race the sweep. + // + // `terminate()` triggers a NAPI segfault under Bun — `manageThreads` avoids it the same way — so + // there the worker is asked to exit itself and its exit awaited. A termination that FAILS is + // reported rather than treated as cleanup done: the caller is about to remove a tree this thread + // may still be reading. if (worker) { + const exited = new Promise((resolve) => worker!.once('exit', () => resolve())); try { - await worker.terminate(); + if (typeof (globalThis as any).Bun !== 'undefined') { + worker.postMessage({ type: 'force-exit' }); + const grace = new Promise((resolve) => setTimeout(resolve, 5000).unref?.()); + await Promise.race([exited, grace]); + } else { + await worker.terminate(); + await exited; + } } catch (error) { - harperLogger.warn(`Could not terminate the validator for ${appName}:`, error); + harperLogger.warn( + `Could not terminate the validator for ${appName}; its candidate tree may still be open:`, + error + ); } } if (timer) clearTimeout(timer); diff --git a/components/deployValidator.ts b/components/deployValidator.ts index 37759a3fb6..32a252912e 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -11,6 +11,7 @@ import { HDB_ROOT_DIR_NAME } from '../utility/hdbTerms.ts'; import harperLogger from '../utility/logging/harper_logger.ts'; import { Resources } from '../resources/Resources.ts'; import { loadComponent, rootApplicationLoadOptions, setErrorReporter } from './componentLoader.ts'; +import type { Scope } from './Scope.ts'; /** * Entry point for an ephemeral deploy-certification worker. @@ -25,7 +26,13 @@ import { loadComponent, rootApplicationLoadOptions, setErrorReporter } from './c * without a verdict, a malformed message — is failure at the parent, which is what makes "inability to * obtain a verdict is failure" true rather than aspirational. */ -const { candidateDirPath, appName, verdictPort } = workerData ?? {}; +// Captured and then REMOVED from `workerData`, before any candidate code runs. `workerData` is reachable +// from the candidate — `require('node:worker_threads').workerData` — so leaving the port there would let a +// candidate post its own passing verdict and certify itself. Taking it out of the bag is the difference +// between a capability this module holds and one the whole thread holds. +const { candidateDirPath, appName } = workerData ?? {}; +const verdictPort = workerData?.verdictPort; +if (workerData) delete workerData.verdictPort; async function certify(): Promise { const componentName = appName || basename(candidateDirPath); @@ -41,8 +48,28 @@ async function certify(): Promise { // A certification load has to run the code a WORKER runs — the `start`/`handleApplication` extension // path — or it proves only that the module parsed. resources.isWorker = true; - await loadComponent(candidateDirPath, resources, HDB_ROOT_DIR_NAME, loadOptions.options); - if (reportedError) throw reportedError; + // Collected so teardown happens BEFORE the verdict, not after. The in-process check this replaces + // established that a Scope which fails to close is a REJECTED validation, not a warning: `close()` stops + // at the throwing listener, so the scope stays partially live. Posting a pass and then failing teardown + // would certify a candidate whose own cleanup is broken — and the thread exits either way, so nothing + // downstream would ever learn. + const scopes = new Set(); + try { + await loadComponent(candidateDirPath, resources, HDB_ROOT_DIR_NAME, { + ...loadOptions.options, + collectScopes: scopes, + }); + if (reportedError) throw reportedError; + } finally { + const closes = await Promise.allSettled(Array.from(scopes, (scope) => scope.close())); + const failed = closes.filter((result) => result.status === 'rejected'); + if (failed.length && !reportedError) { + throw new AggregateError( + failed.map((result) => (result as PromiseRejectedResult).reason), + `${componentName} loaded but ${failed.length} scope(s) failed to tear down` + ); + } + } } function report(verdict: { ok: true } | { ok: false; message: string; stack?: string }): void { diff --git a/unitTests/components/deployActivation.test.js b/unitTests/components/deployActivation.test.js index 64eaa5738c..4fa1b58f2a 100644 --- a/unitTests/components/deployActivation.test.js +++ b/unitTests/components/deployActivation.test.js @@ -40,7 +40,10 @@ async function stageState(root, component, id, state) { const deploymentDir = path.join(root, DEPLOY_STAGING_DIR, id); await fs.mkdir(deploymentDir, { recursive: true }); if (state.candidate) await writeTree(path.join(deploymentDir, component), state.candidate); - if (state.complete) await fs.writeFile(path.join(deploymentDir, '.complete'), ''); + // The marker's CONTENT is the authority now: an empty one predates certification and was minted after a + // validation that did nothing on the main thread, so recovery treats it as uncertified. + if (state.complete) await fs.writeFile(path.join(deploymentDir, '.complete'), 'certified:1'); + if (state.legacyComplete) await fs.writeFile(path.join(deploymentDir, '.complete'), ''); if (state.componentFile !== false) await fs.writeFile(path.join(deploymentDir, '.component'), component); if (state.journal !== undefined) { await fs.writeFile( @@ -454,7 +457,7 @@ describe('activation transaction', () => { const app = new Application({ name: 'web' }); app.dirPath = path.join(root, 'web'); - await activateCandidateApplication(app, 'd1', { certified: false }); + await activateCandidateApplication(app, 'd1'); assert.strictEqual(await readLive(root, 'web'), 'CANDIDATE\n'); assert.strictEqual(existsSync(path.join(root, DEPLOY_STAGING_DIR, 'd1')), false, 'staging is cleaned up'); @@ -484,7 +487,7 @@ describe('activation transaction', () => { const app = new Application({ name: 'web' }); app.dirPath = live; - await activateCandidateApplication(app, 'd1', { certified: false }); + await activateCandidateApplication(app, 'd1'); const linkPath = path.join(live, 'node_modules', 'probe'); const target = await fs.readlink(linkPath); @@ -518,7 +521,7 @@ describe('activation transaction', () => { const app = new Application({ name: 'web' }); app.dirPath = live; - await activateCandidateApplication(app, 'd1', { certified: false }); + await activateCandidateApplication(app, 'd1'); assert.strictEqual( await fs.readlink(path.join(live, 'node_modules', 'shared')), @@ -548,7 +551,7 @@ describe('activation transaction', () => { const app = new Application({ name: 'web' }); app.dirPath = path.join(root, 'web'); - await activateCandidateApplication(app, 'd1', { certified: false }); + await activateCandidateApplication(app, 'd1'); assert.strictEqual(await readLive(root, 'web'), 'CANDIDATE\n'); assert.strictEqual(app.isNewComponent, true, 'and it is recognized as a new component'); @@ -869,4 +872,16 @@ describe('activation transaction', () => { assert.strictEqual(await readLive(root, 'web'), 'CURRENT\n', 'the live component is untouched'); await fs.rm(root, { recursive: true, force: true }); }); + + it('rolls back a candidate whose completion marker predates certification', async () => { + // An empty `.complete` is what an older build wrote, after a validation that was a no-op on the main + // thread. Found after an upgrade it must NOT be roll-forward authority: nothing certified that tree. + const root = await newRoot('legacy-marker'); + await stageState(root, 'web', 'd1', { candidate: 'NEW\n', legacyComplete: true, journal: true, aside: 'OLD\n' }); + + const failures = await recoverInterruptedActivations(root); + + assert.strictEqual(failures.size, 0, 'it settles rather than failing closed'); + assert.strictEqual(await readLive(root, 'web'), 'OLD\n', 'the committed tree comes back, not the candidate'); + }); }); diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index c4dc2efe09..1a66edeb98 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -184,6 +184,36 @@ describe('deploy certification', () => { assert.strictEqual(forCertification.branchConfigured, true, 'and the caller is told to skip certifying'); }); + it('still deploys a branch-configured component, it just earns no authority', async function () { + // The half of this decision that is observable end to end: nothing is refused. That certification + // is skipped is covered by the option test above, and that an unminted candidate rolls back + // rather than forward is covered by the recovery suite — the composition of the two (activate, + // mint nothing, then crash) is not covered here, and needs a crash the unit harness cannot stage. + this.timeout(30000); + const rootDir = await mkdtemp(join(tmpdir(), 'certify-branch-deploy-')); + const componentDirPath = join(rootDir, appName); + await mkdir(componentDirPath, { recursive: true }); + await writeFile(join(componentDirPath, 'package.json'), JSON.stringify({ name: appName, version: '1.0.0' })); + getConfigObj()[appName] = { branchedDatabases: ['data'] }; + + const application = new Application({ + name: appName, + payload: await payloadThatRunsOnLoad(rootDir, appName, '2.0.0', 'module.exports = { fine: true };\n'), + }); + application.dirPath = componentDirPath; + + try { + await prepareApplication(application); + assert.strictEqual( + JSON.parse(await readFile(join(componentDirPath, 'package.json'), 'utf8')).version, + '2.0.0', + 'a branch-configured component still deploys — no capability is removed' + ); + } finally { + await rm(rootDir, { recursive: true, force: true }); + } + }); + it('reports an ordinary component as not branch-configured', () => { getConfigObj()[appName] = { package: 'npm:whatever@1.0.0' }; From 5a7ad8c8b0b2bfa53324812cf6a7ae64224f1571 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 2 Sep 2026 18:59:19 -0400 Subject: [PATCH 06/29] fix(deploy): safe mode deploys uncertified; bound and guard the validator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Safe-mode stage-only was wrong, and the review showed why.** I adopted it from a planning round on the reasoning that safe mode is transient so a staged candidate could wait for the next ordinary preparation. Nothing resumes it: the staged tree carries no journal, so `recoverInterruptedActivations` removes it as build residue at the next start — while `deploy_component` had already returned success, replicated the operation, and run its restart phase. An operator booting into safe mode to replace the component crashing the node would have got a 200, live peers, and a node that came back running the broken component with the fix deleted. That is worse than the behaviour it replaced. It now takes the same shape as the branch-configured case: the deploy happens and earns no authority. No `.complete`, so a crash mid-swap rolls back to the committed tree. The principle was right — *no verdict means no authority, never no verdict means no deploy* — and stage-only broke the second half of it. **The validator was the only Harper worker with no `resourceLimits`.** A candidate whose top-level load builds a large in-memory index would balloon a thread nothing constrains, and the OOM killer would take the whole process down mid-deploy while the previous release was healthy. The calculation is now shared with `startWorker` rather than duplicated, so the two cannot drift. **It also bypassed the `processShuttingDown` guard**, so a deploy racing shutdown spawned a thread the shutdown path knew nothing about and then waited out its deadline on a process that was leaving. **The concurrency cap did not hold.** Release decremented and then resolved a waiter whose increment ran a microtask later, so anything entering that window admitted itself too. The slot is claimed before yielding now. It remains per thread — module state — which bounds validators per worker rather than per process; that is a real limit, not a fixed one. --- components/Application.ts | 37 ++++++++++--------- components/certifyCandidate.ts | 24 +++++++++++- server/threads/manageThreads.js | 37 +++++++++++++------ .../components/deployCertification.test.js | 19 ++++++---- 4 files changed, 79 insertions(+), 38 deletions(-) diff --git a/components/Application.ts b/components/Application.ts index e0d13ab540..b035ab8df1 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -2157,8 +2157,8 @@ export async function markCandidateComplete( * * - **Certified** — a validator loaded this exact tree under the real application identity and mount. * Activation mints `.complete`. - * - **Stage only** — safe mode. It may not execute configured code, so it can certify nothing, and nothing - * uncertified gets published. Deferrable, because safe mode is transient. + * - **Uncertified, activate anyway** — safe mode. It may not execute configured code, so it certifies + * nothing, and it activates without minting `.complete`. * - **Uncertified, activate anyway** — a branch-configured component. A branch's location is derived only * from the application and database names, so a certification load would open the store the LIVE version * is serving from; a candidate could mutate rows, throw, be rejected, and leave the live version serving @@ -2173,10 +2173,25 @@ async function certifyPreparedCandidate( deploymentId: string, candidateDirPath: string, options: PrepareApplicationOptions -): Promise<{ certified: boolean; stageOnly?: boolean }> { +): Promise<{ certified: boolean }> { + // SAFE MODE ACTIVATES, uncertified. An earlier draft staged without activating, on the reasoning that + // safe mode is transient so the candidate could wait — but nothing resumes it: the staged tree carries + // no journal, so `recoverInterruptedActivations` removes it as build residue at the next start, while + // `deploy_component` had already returned success, replicated the operation and run its restart phase. + // An operator booting into safe mode to replace the component crashing the node would have got a 200, + // live peers, and a node that came back running the broken component with the fix deleted. + // + // So it takes the same shape as the branch-configured case below: the deploy happens, and it earns no + // authority. `.complete` is never written, so a crash mid-swap rolls back to the committed tree. const safeMode = process.env.HARPER_SAFE_MODE && process.env.HARPER_SAFE_MODE !== 'false' && process.env.HARPER_SAFE_MODE !== '0'; - if (safeMode) return { certified: false, stageOnly: true }; + if (safeMode) { + application.logger.warn( + `Deploying ${application.name} without certification: safe mode must not execute configured code, so ` + + `no validator can vouch for this candidate` + ); + return { certified: false }; + } const { rootApplicationLoadOptions } = await import('./componentLoader.ts'); const loadOptions = rootApplicationLoadOptions(application.name, { forCertification: true }); @@ -3578,19 +3593,7 @@ export async function prepareApplication(application: Application, options: Prep // `.complete` — the marker recovery treats as proof a validation happened. Leaving it to the // caller meant three of this function's four production call sites minted that authority // without ever asking for a verdict. - const certification = await certifyPreparedCandidate(application, deploymentId, candidateDirPath, options); - if (certification.stageOnly) { - // Safe mode: it may not execute configured code, so it can certify nothing — and a - // candidate nothing certified must not be published. Staged and left for the next - // ordinary-mode preparation to certify and activate. Safe mode is transient, so - // "pending" here resolves on its own, which is why this differs from the - // branch-configured case below. - application.logger.warn( - `Staged ${application.name} without activating it: safe mode cannot certify a candidate, ` + - `and an uncertified candidate is not published` - ); - return; - } + await certifyPreparedCandidate(application, deploymentId, candidateDirPath, options); if (!application.isNewComponent) { application.packageMetadataChanged = installedRuntimeChanged( previousPackageMetadata, diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 19ad38ca59..fb5d1e474d 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -4,7 +4,11 @@ import { join } from 'node:path'; import { MessageChannel, Worker } from 'node:worker_threads'; import harperLogger from '../utility/logging/harper_logger.ts'; -import { buildWorkerExecArgv } from '../server/threads/manageThreads.js'; +import { + buildWorkerExecArgv, + buildWorkerResourceLimits, + isProcessShuttingDown, +} from '../server/threads/manageThreads.js'; /** * How long a certification load may take before the candidate is rejected. @@ -29,7 +33,12 @@ let active = 0; const waiting: (() => void)[] = []; async function acquireSlot(): Promise<() => void> { - if (active >= MAX_CONCURRENT_CERTIFICATIONS) await new Promise((resolve) => waiting.push(resolve)); + // Claimed BEFORE yielding to a waiter, not after. Decrementing and then resolving a waiter whose + // `active++` runs a microtask later left a window any other caller could admit itself through, so the + // documented bound did not hold. The slot is handed straight from releaser to waiter instead. + while (active >= MAX_CONCURRENT_CERTIFICATIONS) { + await new Promise((resolve) => waiting.push(resolve)); + } active++; let released = false; return () => { @@ -64,6 +73,13 @@ export async function certifyCandidate( appName: string, { timeoutMs = DEFAULT_CERTIFICATION_TIMEOUT_MS }: { timeoutMs?: number } = {} ): Promise { + // The same guard `startWorker` applies. A deploy racing shutdown would otherwise spawn a thread the + // shutdown path does not know about, and then wait out its deadline on a process that is leaving. + if (isProcessShuttingDown()) { + const error: any = new Error(`Cannot certify ${appName} while the Harper process is shutting down`); + error.statusCode = 503; + return { certified: false, error }; + } const releaseSlot = await acquireSlot(); // The COMPILED sibling, referenced the way `jobRunner` references `jobProcess.js`: workers load from the // build output, and `__dirname` resolves there without assuming where the package root is. @@ -105,6 +121,10 @@ export async function certifyCandidate( // Harper's own module graph at all — a module that imports JSON fails outright — so this is // shared with `startWorker` rather than reconstructed. execArgv: buildWorkerExecArgv(), + // Bounded like every other Harper worker. Without limits a candidate whose top-level load + // builds a large in-memory index balloons a thread nothing constrains, and the OOM killer + // takes the whole process down mid-deploy — while the previous release was healthy. + resourceLimits: buildWorkerResourceLimits(), argv: process.argv.slice(2), }); } catch (error) { diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 1da5d609e0..814b3123d4 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -143,6 +143,8 @@ const messagesQueuedByType = new Map(); module.exports = { buildWorkerExecArgv, + buildWorkerResourceLimits, + isProcessShuttingDown, startWorker, restartWorkers, shutdownWorkers, @@ -346,6 +348,28 @@ listenersByType.set(PROCESS_GROUP_TERMINATION_CONFIRMED, null); * `new Worker()` cannot even load a module that imports JSON. Shared rather than copied so the two spawn * paths cannot drift on something this load-bearing. */ +/** Whether the process is tearing down, so no new worker of any kind should be started. */ +function isProcessShuttingDown() { + return processShuttingDown; +} + +/** + * The heap bounds every Harper worker runs under. Shared with the non-topology spawn path so a validator + * thread is constrained like a serving one — an unbounded module graph in a candidate's top-level load + * would otherwise be the OOM killer's problem, taken out on the whole process. + */ +function buildWorkerResourceLimits(threadCount) { + let availableMemory = process.constrainedMemory?.() || totalmem(); + availableMemory = Math.min(availableMemory, totalmem(), 20000 * MB); + const maxOldMemory = + resolveThreadHeapMemoryMb(envMgr.get(hdbTerms.CONFIG_PARAMS.THREADS_MAXHEAPMEMORY)) ?? + Math.max(Math.floor(availableMemory / MB / (10 + (threadCount || 1) / 4)), 512); + return { + maxOldGenerationSizeMb: maxOldMemory, + maxYoungGenerationSizeMb: Math.min(Math.max(maxOldMemory >> 6, 16), 64), + }; +} + function buildWorkerExecArgv() { const isBun = typeof globalThis.Bun !== 'undefined'; const execArgv = isBun @@ -392,18 +416,12 @@ function startWorker(path, options = {}) { // 16 threads: 20% of total memory per thread // 64 threads: 11% of total memory per thread // (and then limit to their license limit, if they have one) - let availableMemory = process.constrainedMemory?.() || totalmem(); // used constrained memory if it is available - // and lower than total memory - availableMemory = Math.min(availableMemory, totalmem(), 20000 * MB); - const maxOldMemory = - resolveThreadHeapMemoryMb(envMgr.get(hdbTerms.CONFIG_PARAMS.THREADS_MAXHEAPMEMORY)) ?? - Math.max(Math.floor(availableMemory / MB / (10 + (options.threadCount || 1) / 4)), 512); // Max young memory space (semi-space for scavenger) is 1/128 of max memory (limited to 16-64). For most of our m5 // machines this will be 64MB (less for t3's). This is based on recommendations from: // https://www.alibabacloud.com/blog/node-js-application-troubleshooting-manual---comprehensive-gc-problems-and-optimization594965 // https://github.com/nodejs/node/issues/42511 // https://plaid.com/blog/how-we-parallelized-our-node-service-by-30x/ - const maxYoungMemory = Math.min(Math.max(maxOldMemory >> 6, 16), 64); + const resourceLimits = buildWorkerResourceLimits(options.threadCount); const channelsToConnect = []; const portsToSend = []; @@ -419,10 +437,7 @@ function startWorker(path, options = {}) { const execArgv = buildWorkerExecArgv(); const worker = new Worker(isAbsolute(path) ? path : join(PACKAGE_ROOT, path), { - resourceLimits: { - maxOldGenerationSizeMb: maxOldMemory, - maxYoungGenerationSizeMb: maxYoungMemory, - }, + resourceLimits, execArgv, argv: process.argv.slice(2), // pass these in synchronously to the worker so it has them on startup: diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index 1a66edeb98..8f916f37d1 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -125,11 +125,15 @@ describe('deploy certification', () => { } }); - it('stages without activating in safe mode, so nothing uncertified is published', async function () { - // Safe mode may not execute configured code, so it can certify nothing — and a candidate nothing - // certified must not be published. It is also transient, which is why "pending" is the right answer - // here and the wrong one for a branch-configured component: the next ordinary preparation certifies - // and activates this. + it('deploys in safe mode without certifying, rather than staging a candidate nothing resumes', async function () { + // Safe mode may not execute configured code, so it certifies nothing — and it deploys anyway, minting + // no `.complete`. + // + // An earlier draft staged WITHOUT activating, reasoning that safe mode is transient so the candidate + // could wait. Nothing resumes it: the staged tree carries no journal, so recovery removes it as build + // residue at the next start — while the operation had already reported success, replicated, and run + // its restart phase. An operator booting into safe mode to replace the component crashing the node + // would have got a 200 and a node that came back running the broken component with the fix deleted. this.timeout(30000); const rootDir = await mkdtemp(join(tmpdir(), 'certify-safe-mode-')); const componentDirPath = join(rootDir, 'shop'); @@ -148,10 +152,9 @@ describe('deploy certification', () => { await prepareApplication(application); assert.strictEqual( JSON.parse(await readFile(join(componentDirPath, 'package.json'), 'utf8')).version, - '1.0.0', - 'the live version is untouched — the candidate was staged, not activated' + '2.0.0', + 'the deploy takes effect — safe mode is when an operator most needs it to' ); - assert.ok(existsSync(join(rootDir, '.deploy-staging')), 'and the staged candidate is kept for later'); } finally { if (priorSafeMode === undefined) delete process.env.HARPER_SAFE_MODE; else process.env.HARPER_SAFE_MODE = priorSafeMode; From 13249a4b176a6c5c3fbee5eb3d6790b11a313b56 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 2 Sep 2026 19:17:50 -0400 Subject: [PATCH 07/29] fix(deploy): stop uncertified swaps committing unflushed trees, and a hang I added MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two blockers from round 2, both mine. **An uncertified activation committed a tree that was never fsynced.** `markCandidateComplete` was the only caller of `syncCandidateTree`, so skipping the mint skipped the flush — and `syncRenameParents` syncs directory entries, not contents. A power loss shortly after a branch-configured or safe-mode deploy returned success would leave the live path holding zero-length files, with the aside already retired and no journal. My previous commit widened this by routing safe mode down the same path. The flush moved to `activateCandidateApplication`, unconditionally. Certification decides what a tree MEANS; it was never what makes it durable, and putting the two in one function is what let them be skipped together. **The termination waiter was installed after the event it waited for.** The validator `realExit`s immediately after posting its verdict, so on any turn where the parent sees the exit before the queued message, the `once('exit')` attached in the `finally` never fired: `certifyCandidate` never returned, its slot was never released, and `prepareApplication` sat inside the preparation lock forever. Two of those and the node stops deploying until restart. Reachable on the ordinary failure path, not an exotic one — and introduced by the Bun fix in the previous commit. The exit promise is now created when the worker is, so it cannot be missed, and every wait in that `finally` is bounded: it runs inside a deploy holding the preparation lock, so a termination that never settles is the same wedge arrived at from the other side. No test for the fsync: it has no readable effect and the call is internal, so the honest options were a vacuous assertion or none. I wrote the vacuous one first — `assert typeof fn === 'function'` — and deleted it. The hang is covered incidentally, in that the suite would stop rather than fail if it returned. --- components/Application.ts | 10 +++++++++- components/certifyCandidate.ts | 26 ++++++++++++++++++++------ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/components/Application.ts b/components/Application.ts index b035ab8df1..a2fa939f0e 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -2124,7 +2124,7 @@ export async function markCandidateComplete( `certified it, and \`${CANDIDATE_COMPLETE_MARKER}\` is what recovery treats as proof that one did` ); } - await syncCandidateTree(componentDirPath, deploymentId); + try { await writeControlFileDurably(candidateComponentFilePath(componentDirPath, deploymentId), componentName); } catch (error) { @@ -2231,6 +2231,14 @@ export async function activateCandidateApplication(application: Application, dep throw new Error(`Cannot activate ${application.name}: no candidate build at ${candidateDirPath}`); } + // Durability first, and for EVERY activation — certified or not. This used to live inside + // `markCandidateComplete`, so skipping the mint skipped the fsync too: an uncertified swap (a + // branch-configured component, or safe mode) committed a tree whose contents were never flushed, and + // `syncRenameParents` only syncs directory entries. A power loss shortly after such a deploy returned + // success would leave the live path holding zero-length files, with the aside already retired. + // Certification decides what the tree MEANS; it was never what makes it durable. + await syncCandidateTree(liveDirPath, deploymentId); + // The RECORD decides, never the caller. An argument saying "this one is certified" would be the forgeable // proof the internal record exists to avoid — and an exported function with such a flag lets any caller // mint authority for an uncertified tree, which is the invariant this step is for. diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index fb5d1e474d..2ffdbf577f 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -29,6 +29,9 @@ const DEFAULT_CERTIFICATION_TIMEOUT_MS = 120_000; */ const MAX_CONCURRENT_CERTIFICATIONS = 2; +/** How long to wait for a validator to actually go away before giving up and saying so. */ +const TERMINATION_GRACE_MS = 5000; + let active = 0; const waiting: (() => void)[] = []; @@ -85,6 +88,12 @@ export async function certifyCandidate( // build output, and `__dirname` resolves there without assuming where the package root is. const entry = join(__dirname, './deployValidator.js'); let worker: Worker | undefined; + // Installed the moment the worker exists, NOT in the `finally`. Attaching it later races the exit it is + // meant to observe: the validator `realExit`s immediately after posting, so on any turn where the parent + // sees the exit before the queued verdict, a listener attached afterwards never fires — `certifyCandidate` + // never returns, its slot is never released, and `prepareApplication` waits inside the preparation lock + // forever. Two of those and the node stops deploying until it restarts. + let exited: Promise | undefined; let settled = false; let timer: NodeJS.Timeout | undefined; // A channel of its own, NOT `parentPort`: Harper's worker machinery uses that for its own ITC traffic, @@ -106,7 +115,7 @@ export async function certifyCandidate( const fail = (message: string) => settle({ certified: false, error: new Error(message) }); try { - worker = new Worker(entry, { + const started = new Worker(entry, { workerData: { candidateDirPath, appName, @@ -127,6 +136,8 @@ export async function certifyCandidate( resourceLimits: buildWorkerResourceLimits(), argv: process.argv.slice(2), }); + worker = started; + exited = new Promise((resolve) => started.once('exit', () => resolve())); } catch (error) { // A synchronous spawn throw is a deploy failure, not a candidate failure — and specifically // not a success. Under thread pressure a node will refuse deploys rather than publish @@ -175,16 +186,19 @@ export async function certifyCandidate( // reported rather than treated as cleanup done: the caller is about to remove a tree this thread // may still be reading. if (worker) { - const exited = new Promise((resolve) => worker!.once('exit', () => resolve())); + // Every wait here is bounded. This runs in a `finally` that the caller's deploy is blocked on, so a + // termination that never settles would hold the preparation lock indefinitely — the same failure + // as the missed-exit race above, arrived at from the other side. + const grace = () => new Promise((resolve) => setTimeout(resolve, TERMINATION_GRACE_MS).unref?.()); try { if (typeof (globalThis as any).Bun !== 'undefined') { + // `terminate()` triggers a NAPI segfault under Bun; `manageThreads` asks the worker to exit + // itself for the same reason. worker.postMessage({ type: 'force-exit' }); - const grace = new Promise((resolve) => setTimeout(resolve, 5000).unref?.()); - await Promise.race([exited, grace]); } else { - await worker.terminate(); - await exited; + await Promise.race([worker.terminate(), grace()]); } + await Promise.race([exited ?? Promise.resolve(), grace()]); } catch (error) { harperLogger.warn( `Could not terminate the validator for ${appName}; its candidate tree may still be open:`, From 50a1f1f32c86a17dd72f0103122e956c65ebc043 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 2 Sep 2026 19:27:55 -0400 Subject: [PATCH 08/29] fix(deploy): give Bun's force-exit a receiver, and keep a runaway validator's slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last round-3 blocker. **The force-exit message had no receiver.** `terminate()` segfaults under Bun, so that message is the only way the parent can end a validator there — and I was relying on `manageThreads`' worker-side block having registered a handler as an import side effect. The validator registers its own now: a capability that is the only way to stop a thread should not depend on which modules happened to load. **Grace expiry released the slot while the validator might still be alive.** A runaway thread now keeps its slot. Releasing it let the node start another while the first still held the candidate tree open and consumed the heap the cap exists to bound — and the caller is about to sweep that tree. Bounded either way by `MAX_CONCURRENT_CERTIFICATIONS`, so the worst case is certification stopping on that thread and saying so, rather than quietly overcommitting. --- components/certifyCandidate.ts | 21 +++++++++++++++++++-- components/deployValidator.ts | 10 +++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 2ffdbf577f..0b9b8436d1 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -94,6 +94,8 @@ export async function certifyCandidate( // never returns, its slot is never released, and `prepareApplication` waits inside the preparation lock // forever. Two of those and the node stops deploying until it restarts. let exited: Promise | undefined; + // Set when a validator outlives its termination grace, so its slot is deliberately not returned. + let slotHeld = false; let settled = false; let timer: NodeJS.Timeout | undefined; // A channel of its own, NOT `parentPort`: Harper's worker machinery uses that for its own ITC traffic, @@ -198,7 +200,22 @@ export async function certifyCandidate( } else { await Promise.race([worker.terminate(), grace()]); } - await Promise.race([exited ?? Promise.resolve(), grace()]); + const outcome = await Promise.race([ + (exited ?? Promise.resolve()).then(() => 'exited' as const), + grace().then(() => 'still-running' as const), + ]); + // A validator that would not die keeps its slot. Releasing it would let the node start another + // thread while a runaway one is still holding the candidate tree open and consuming the heap + // this cap exists to bound — and the caller is about to sweep that tree. Bounded by + // MAX_CONCURRENT_CERTIFICATIONS either way, so the worst case is that certification stops on + // this thread and says so, rather than quietly overcommitting. + if (outcome === 'still-running') { + slotHeld = true; + harperLogger.error( + `The validator certifying ${appName} did not exit within ${TERMINATION_GRACE_MS}ms; holding its ` + + `slot, and its candidate tree may still be open` + ); + } } catch (error) { harperLogger.warn( `Could not terminate the validator for ${appName}; its candidate tree may still be open:`, @@ -209,6 +226,6 @@ export async function certifyCandidate( if (timer) clearTimeout(timer); // Both ends, or the channel keeps this thread's event loop referenced. verdicts.close(); - releaseSlot(); + if (!slotHeld) releaseSlot(); } } diff --git a/components/deployValidator.ts b/components/deployValidator.ts index 32a252912e..fca869fb27 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -5,7 +5,7 @@ import { realExit } from '../server/threads/workerProcessGuard.ts'; import { basename } from 'node:path'; -import { workerData } from 'node:worker_threads'; +import { parentPort, workerData } from 'node:worker_threads'; import { HDB_ROOT_DIR_NAME } from '../utility/hdbTerms.ts'; import harperLogger from '../utility/logging/harper_logger.ts'; @@ -72,6 +72,14 @@ async function certify(): Promise { } } +// The receiver for the parent's Bun force-exit. Registered HERE rather than relied on as an import side +// effect of `manageThreads`' worker block: `terminate()` segfaults under Bun, so that message is the only +// way the parent can end this thread, and a capability that important should not depend on which modules +// happened to load. +parentPort?.on('message', (message: any) => { + if (message?.type === 'force-exit') realExit(0); +}); + function report(verdict: { ok: true } | { ok: false; message: string; stack?: string }): void { // Its own channel rather than `parentPort`, which carries Harper's ITC traffic. A closed or absent // channel is the parent's problem to detect (it treats a missing verdict as failure), so this must not From da36f429e69a5849c3288ef64e0e78737c9d8b04 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 2 Sep 2026 19:40:00 -0400 Subject: [PATCH 09/29] fix(deploy): return a runaway validator's slot when it finally exits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Holding the slot was right; holding it forever was not. Each runaway validator would have shrunk the cap for the life of the process, and a few would stop the node deploying at all — trading a bounded overcommit for an unbounded outage. The slot is now held until the thread actually goes away. --- components/certifyCandidate.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 0b9b8436d1..f626d16199 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -210,11 +210,18 @@ export async function certifyCandidate( // MAX_CONCURRENT_CERTIFICATIONS either way, so the worst case is that certification stops on // this thread and says so, rather than quietly overcommitting. if (outcome === 'still-running') { + // Held UNTIL it actually goes away, not forever. Holding it permanently would mean each + // runaway validator shrinks the cap for the life of the process, and a few would stop the + // node deploying — trading a bounded overcommit for an unbounded outage. slotHeld = true; harperLogger.error( `The validator certifying ${appName} did not exit within ${TERMINATION_GRACE_MS}ms; holding its ` + - `slot, and its candidate tree may still be open` + `slot until it does, and its candidate tree may still be open` ); + void (exited ?? Promise.resolve()).then(() => { + harperLogger.warn(`The validator certifying ${appName} exited late; returning its slot`); + releaseSlot(); + }); } } catch (error) { harperLogger.warn( From 6c16eb4199c31910aa53474197c9e33b7ad5a5c2 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 09:36:56 -0400 Subject: [PATCH 10/29] fix(deploy): load the global plugins before certifying, and scope the reporter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught certification **rejecting a valid component** — 12 red checks from one cause. `integrationTests/components/acl-connect.test.ts` has a fixture doing `server.mqtt.authorizeClient = …`, and `server.mqtt` is created by the mqtt plugin's own load. The validator sets `workerData.noServerStart` to stop `threadServer` booting the whole server, and that suppresses the plugin loads too — so the assignment threw on `undefined` and a component that works perfectly on a serving worker failed certification. `noServerStart` was never sufficient for runtime equivalence, which is what the planning review meant by needing one shared loader entry point. `loadRootPlugins` is extracted from `loadRootComponents` at the boundary that was already there: the Harper root component (the global plugins) loads, and the other applications do not. Both callers use it, so they cannot drift on what "the plugins are loaded" means. No listeners are bound — plugins register handlers on the scope's server object, and the port binding belongs to `threadServer`'s startup, which the validator still suppresses. **The error reporter is now installed after that bootstrap**, so it only ever sees the candidate. Installed earlier it captured the first error from anything the root config names — and since `deploy_component` writes a component's config entry *before* building it, that includes the candidate's own live path, which does not exist yet on a first deploy. Certification was rejecting the candidate for the absence of the very thing it was about to create. Also: certification no longer resolves the configured APM preloads. `getImportModules()` memoizes on first call, and a validator spawns much earlier than the first serving worker, so it froze an empty preload list and broke `preloadSafeMode.test.js` — a test-visible symptom of a real ordering hazard. A throwaway thread should not appear in an APM once per deploy either. acl-connect goes from 13 cancelled to 13 passing; 402 unit and 20 deploy/component integration tests pass locally. --- components/certifyCandidate.ts | 5 ++++- components/deployValidator.ts | 25 +++++++++++++++------- server/loadRootComponents.js | 37 ++++++++++++++++++++++++--------- server/threads/manageThreads.js | 10 +++++++-- 4 files changed, 57 insertions(+), 20 deletions(-) diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index f626d16199..3bb9717cb2 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -131,7 +131,10 @@ export async function certifyCandidate( // The same interpreter setup every Harper worker gets. Without it this thread cannot load // Harper's own module graph at all — a module that imports JSON fails outright — so this is // shared with `startWorker` rather than reconstructed. - execArgv: buildWorkerExecArgv(), + // Without the configured APM preloads: a validator is a throwaway thread, and resolving the + // preload list from here would memoize it earlier than the first serving worker — see + // `buildWorkerExecArgv`. + execArgv: buildWorkerExecArgv({ preloads: false }), // Bounded like every other Harper worker. Without limits a candidate whose top-level load // builds a large in-memory index balloons a thread nothing constrains, and the OOM killer // takes the whole process down mid-deploy — while the previous release was healthy. diff --git a/components/deployValidator.ts b/components/deployValidator.ts index fca869fb27..3e9a78d2e1 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -9,7 +9,6 @@ import { parentPort, workerData } from 'node:worker_threads'; import { HDB_ROOT_DIR_NAME } from '../utility/hdbTerms.ts'; import harperLogger from '../utility/logging/harper_logger.ts'; -import { Resources } from '../resources/Resources.ts'; import { loadComponent, rootApplicationLoadOptions, setErrorReporter } from './componentLoader.ts'; import type { Scope } from './Scope.ts'; @@ -40,14 +39,26 @@ async function certify(): Promise { if (!loadOptions.ok) { throw new Error(`Cannot certify ${componentName}: its root-config mount could not be resolved`); } - // The candidate's own load-time error, not just whether the promise rejected: the loader reports some - // failures through the error reporter while resolving successfully. + // The global plugins FIRST. They are what create the surfaces applications extend — a fixture doing + // `server.mqtt.authorizeClient = …` needs the mqtt plugin to have loaded, or the assignment throws on + // `undefined` and certification rejects a component that works fine on a serving worker. This stops + // before the other applications, which is the whole reason it is a separate entry point. + // + // A certification load also has to run the code a WORKER runs — the `start`/`handleApplication` + // extension path — or it proves only that the module parsed. + const { loadRootPlugins } = await import('../server/loadRootComponents.js'); + const resources = await loadRootPlugins(true); + + // The reporter goes on AFTER the bootstrap, so it only ever sees the candidate. Installed earlier it + // captured the first error from anything the root config happens to name — and since `deploy_component` + // writes a component's config entry before building it, that includes the candidate's own live path, + // which does not exist yet on a first deploy. Certification then rejected the candidate for the absence + // of the thing it was about to create. + // + // The candidate's own load-time error matters, not just whether the promise rejected: the loader reports + // some failures through the reporter while resolving successfully. let reportedError: Error | undefined; setErrorReporter((error: Error) => (reportedError ??= error)); - const resources = new Resources(); - // A certification load has to run the code a WORKER runs — the `start`/`handleApplication` extension - // path — or it proves only that the module parsed. - resources.isWorker = true; // Collected so teardown happens BEFORE the verdict, not after. The in-process check this replaces // established that a Scope which fails to close is a REJECTED validation, not a warning: `close()` stops // at the throwing listener, so the scope stays partially live. Posting a pass and then failing teardown diff --git a/server/loadRootComponents.js b/server/loadRootComponents.js index e6773dd7ab..4be43e33ff 100644 --- a/server/loadRootComponents.js +++ b/server/loadRootComponents.js @@ -63,25 +63,42 @@ async function loadRootComponents(isWorkerThread = false) { console.error(errorForLog(error)); } - let resources = resetResources(); + const resources = await loadRootPlugins(isWorkerThread); + if (!process.env.HARPER_SAFE_MODE) { + // once the global plugins are loaded, we now load all the CF and run applications (and their components) + const readyComponentPromises = new WeakMap(); + await loadComponentDirectories(loadedComponents, resources, readyComponentPromises, interruptedActivationFailures); + await readyComponentModules(loadedComponents.keys(), readyComponentPromises); + return; + } + await readyComponentModules(loadedComponents.keys()); +} + +/** + * Load the Harper root component — the global plugins — and nothing that depends on them. + * + * This is the boundary the deploy certification validator needs. Plugins are what establish the surfaces + * applications extend (`server.mqtt.authorizeClient` and friends), so a certification load without them + * fails a component that works perfectly on a serving worker: the plugin surface is simply absent. But the + * validator must NOT go on to load the other applications, which is exactly where this function stops. + * + * Extracted rather than duplicated so the two callers cannot drift on what "the plugins are loaded" means. + * Listeners are not bound here: plugins register handlers on the scope's server object, and the port + * binding belongs to `threadServer`'s startup, which a validator suppresses with `workerData.noServerStart`. + */ +async function loadRootPlugins(isWorkerThread = false) { + const resources = resetResources(); getTables(); resources.isWorker = isWorkerThread; await loadCertificates(); - // the Harper root component await loadComponent(dirname(configUtils.getConfigFilePath()), resources, 'hdb', { isRoot: true, providedLoadedComponents: loadedComponents, autoReload: false, }); - if (!process.env.HARPER_SAFE_MODE) { - // once the global plugins are loaded, we now load all the CF and run applications (and their components) - const readyComponentPromises = new WeakMap(); - await loadComponentDirectories(loadedComponents, resources, readyComponentPromises, interruptedActivationFailures); - await readyComponentModules(loadedComponents.keys(), readyComponentPromises); - return; - } - await readyComponentModules(loadedComponents.keys()); + return resources; } module.exports.loadRootComponents = loadRootComponents; +module.exports.loadRootPlugins = loadRootPlugins; diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 814b3123d4..3cadd1fd00 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -347,6 +347,12 @@ listenersByType.set(PROCESS_GROUP_TERMINATION_CONFIRMED, null); * Exported because a worker that must NOT join the serving topology still needs exactly these: a bare * `new Worker()` cannot even load a module that imports JSON. Shared rather than copied so the two spawn * paths cannot drift on something this load-bearing. + * + * `preloads: false` omits the configured APM agents. Two reasons, and the second is the load-bearing one: + * a throwaway certification thread is not something an APM should see one of per deploy; and + * `getImportModules()`/`getRequireModules()` MEMOIZE on first call, so resolving them from a spawn that + * happens earlier than a serving worker would freeze the list against whatever config was live then. Before + * certification existed, nothing resolved them until the first real worker started. */ /** Whether the process is tearing down, so no new worker of any kind should be started. */ function isProcessShuttingDown() { @@ -370,7 +376,7 @@ function buildWorkerResourceLimits(threadCount) { }; } -function buildWorkerExecArgv() { +function buildWorkerExecArgv({ preloads = true } = {}) { const isBun = typeof globalThis.Bun !== 'undefined'; const execArgv = isBun ? [] @@ -393,7 +399,7 @@ function buildWorkerExecArgv() { // which safe mode must not resolve or execute. const isSafeMode = process.env.HARPER_SAFE_MODE && process.env.HARPER_SAFE_MODE !== 'false' && process.env.HARPER_SAFE_MODE !== '0'; - if (!isBun && !isSafeMode) { + if (!isBun && !isSafeMode && preloads) { for (const importPath of getImportModules()) execArgv.push('--import', pathToFileURL(importPath).href); for (const requirePath of getRequireModules()) execArgv.push('--require', requirePath); } From 30324e81032c7619830599a4bae8a99e7e67cb8c Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 09:38:26 -0400 Subject: [PATCH 11/29] fix(deploy): three review suggestions on the certification protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From gemini-code-assist on #2476, all three correct. **A teardown failure no longer masks the load error.** A throw from `loadComponent` — a syntax error, an unreadable file — reaches the same `finally`, so a scope that then failed to close replaced the candidate's real error with a note about its scopes. The operator got the symptom instead of the cause. Gated on the load having actually succeeded. **One termination grace for the whole thing, not one per step.** Racing `terminate()` against the grace and then racing the exit against another could wait twice as long before calling a hung validator hung — and this runs inside a deploy holding the preparation lock. **An unresolvable root-config mount throws before spawning.** It used to start a thread whose only job was to fail on the same condition a moment later. The fourth suggestion — hand the slot directly to the next waiter so `acquireSlot` is strictly FIFO — describes a real starvation risk, but a previous commit already closed the queue-jumping window by claiming the slot before yielding. Left as is rather than churning the same lines twice; noted on the thread. --- components/Application.ts | 6 +++++- components/certifyCandidate.ts | 21 +++++++++++++-------- components/deployValidator.ts | 7 ++++++- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/components/Application.ts b/components/Application.ts index a2fa939f0e..cb9b497fa8 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -2195,7 +2195,11 @@ async function certifyPreparedCandidate( const { rootApplicationLoadOptions } = await import('./componentLoader.ts'); const loadOptions = rootApplicationLoadOptions(application.name, { forCertification: true }); - if (loadOptions.ok && loadOptions.branchConfigured) { + // Answered here rather than by spawning a thread that will fail on the same thing a moment later. + if (!loadOptions.ok) { + throw new Error(`Cannot certify ${application.name}: its root-config mount could not be resolved`); + } + if (loadOptions.branchConfigured) { application.logger.warn( `Deploying ${application.name} without certification: a certification load would open the same ` + `database branch the live version is serving from, so it is skipped until validation-scoped ` + diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 3bb9717cb2..1130d0d286 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -196,15 +196,20 @@ export async function certifyCandidate( // as the missed-exit race above, arrived at from the other side. const grace = () => new Promise((resolve) => setTimeout(resolve, TERMINATION_GRACE_MS).unref?.()); try { - if (typeof (globalThis as any).Bun !== 'undefined') { - // `terminate()` triggers a NAPI segfault under Bun; `manageThreads` asks the worker to exit - // itself for the same reason. - worker.postMessage({ type: 'force-exit' }); - } else { - await Promise.race([worker.terminate(), grace()]); - } + // ONE grace for the whole thing, not one per step: racing terminate and then racing the exit + // could wait 2x before calling a hung validator hung, and this runs inside a deploy holding + // the preparation lock. + const asked = + typeof (globalThis as any).Bun !== 'undefined' + ? // `terminate()` triggers a NAPI segfault under Bun; `manageThreads` asks the worker to + // exit itself for the same reason. + (worker.postMessage({ type: 'force-exit' }), Promise.resolve()) + : worker.terminate().then( + () => {}, + () => {} + ); const outcome = await Promise.race([ - (exited ?? Promise.resolve()).then(() => 'exited' as const), + Promise.all([asked, exited ?? Promise.resolve()]).then(() => 'exited' as const), grace().then(() => 'still-running' as const), ]); // A validator that would not die keeps its slot. Releasing it would let the node start another diff --git a/components/deployValidator.ts b/components/deployValidator.ts index 3e9a78d2e1..06cfa3ded7 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -65,16 +65,21 @@ async function certify(): Promise { // would certify a candidate whose own cleanup is broken — and the thread exits either way, so nothing // downstream would ever learn. const scopes = new Set(); + let loaded = false; try { await loadComponent(candidateDirPath, resources, HDB_ROOT_DIR_NAME, { ...loadOptions.options, collectScopes: scopes, }); if (reportedError) throw reportedError; + loaded = true; } finally { const closes = await Promise.allSettled(Array.from(scopes, (scope) => scope.close())); const failed = closes.filter((result) => result.status === 'rejected'); - if (failed.length && !reportedError) { + // Only when the load itself succeeded. A throw from `loadComponent` — a syntax error, an unreadable + // file — reaches this block too, and a teardown failure there would replace the candidate's real + // error with a note about its scopes: the operator would get the symptom instead of the cause. + if (failed.length && loaded) { throw new AggregateError( failed.map((result) => (result as PromiseRejectedResult).reason), `${componentName} loaded but ${failed.length} scope(s) failed to tear down` From 5a0d20fe60b9064d33b5e30c9c91d9bbf24ca24d Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 09:47:09 -0400 Subject: [PATCH 12/29] fix(deploy): make the verdict survive the validator's exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI failed three of the certification tests with "exited with code 0 without reporting a verdict". The validator `realExit`s the instant it has posted, and on Windows the parent consistently observes that exit before the queued message — so a candidate that failed to load, and one that loaded fine, both arrived as "no verdict". The protocol's own rule then made that a failure, which is the safe direction but the wrong answer. The pass/fail bit now travels through a `SharedArrayBuffer` written synchronously before the exit: shared memory needs no event-loop turn, so it cannot be outrun. The message still carries the candidate's error text, which is detail rather than authority. `VERDICT_NO_ANSWER` is the initial value, so silence is still a failure — the fix makes a real verdict reliable rather than inferring one from an exit code, which would have been minting authority from silence by another route. The flag is deleted from `workerData` alongside the port before any candidate code runs, for the same reason: a candidate that could write it could certify itself. Not reproducible locally — the race is consistent on Windows and unobservable on macOS, and I could find no seam to force a dropped message without faking the thing under test. Windows CI is the check. --- components/certifyCandidate.ts | 29 +++++++++++++++++++++++++++-- components/deployValidator.ts | 11 ++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 1130d0d286..7511cf8b13 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -29,6 +29,18 @@ const DEFAULT_CERTIFICATION_TIMEOUT_MS = 120_000; */ const MAX_CONCURRENT_CERTIFICATIONS = 2; +/** + * Verdict slots in the shared buffer below. A message can be lost — the validator exits the instant it has + * posted, and on Windows the parent observes that exit before the queued message — so the pass/fail bit + * travels through shared memory, which needs no event-loop turn and cannot be outrun by the exit. The + * message still carries the candidate's error text, which is detail rather than authority. + * + * `NO_ANSWER` is the initial value, so silence remains a failure rather than becoming a pass. + */ +export const VERDICT_NO_ANSWER = 0; +export const VERDICT_CERTIFIED = 1; +export const VERDICT_REJECTED = 2; + /** How long to wait for a validator to actually go away before giving up and saying so. */ const TERMINATION_GRACE_MS = 5000; @@ -102,6 +114,7 @@ export async function certifyCandidate( // so a verdict read from it would compete with unrelated messages — the first one to arrive was being // rejected as a malformed verdict. On a dedicated channel, anything that does not conform really is one. const { port1: verdicts, port2: verdictPort } = new MessageChannel(); + const verdictFlag = new Int32Array(new SharedArrayBuffer(4)); try { return await new Promise((resolve) => { @@ -122,6 +135,7 @@ export async function certifyCandidate( candidateDirPath, appName, verdictPort, + verdictFlag, // `server/DESIGN.md`: "Workers receive `workerData.noServerStart = true` — never start the // server inside a worker." Without it `threadServer` boots at module scope and loads every // root component, so the validator would serve traffic and certify the wrong thing. @@ -178,8 +192,19 @@ export async function certifyCandidate( settle({ certified: false, error: failure }); }); worker.on('exit', (code) => { - // Only reached when no verdict arrived first; a verdict already settled it. - fail(`Certification of ${appName} exited with code ${code} without reporting a verdict`); + // Only reached when no verdict MESSAGE arrived first. The shared flag is written before the + // validator exits, so it is still authoritative here — this is the ordinary path on Windows, + // where the exit consistently beats the queued message. + const flag = Atomics.load(verdictFlag, 0); + if (flag === VERDICT_NO_ANSWER) { + fail(`Certification of ${appName} exited with code ${code} without reporting a verdict`); + return; + } + if (flag === VERDICT_CERTIFIED) { + settle({ certified: true }); + return; + } + fail(`${appName} failed to load during certification (its validator exited before reporting why)`); }); }); } finally { diff --git a/components/deployValidator.ts b/components/deployValidator.ts index 06cfa3ded7..b9a9c26cc4 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -9,6 +9,7 @@ import { parentPort, workerData } from 'node:worker_threads'; import { HDB_ROOT_DIR_NAME } from '../utility/hdbTerms.ts'; import harperLogger from '../utility/logging/harper_logger.ts'; +import { VERDICT_CERTIFIED, VERDICT_REJECTED } from './certifyCandidate.ts'; import { loadComponent, rootApplicationLoadOptions, setErrorReporter } from './componentLoader.ts'; import type { Scope } from './Scope.ts'; @@ -31,7 +32,11 @@ import type { Scope } from './Scope.ts'; // between a capability this module holds and one the whole thread holds. const { candidateDirPath, appName } = workerData ?? {}; const verdictPort = workerData?.verdictPort; -if (workerData) delete workerData.verdictPort; +const verdictFlag: Int32Array | undefined = workerData?.verdictFlag; +if (workerData) { + delete workerData.verdictPort; + delete workerData.verdictFlag; +} async function certify(): Promise { const componentName = appName || basename(candidateDirPath); @@ -97,6 +102,10 @@ parentPort?.on('message', (message: any) => { }); function report(verdict: { ok: true } | { ok: false; message: string; stack?: string }): void { + // The flag FIRST, and synchronously: this thread exits immediately after, and a posted message can lose + // that race — it consistently does on Windows. Shared memory needs no event-loop turn, so the parent + // sees the verdict even when the message never arrives. + if (verdictFlag) Atomics.store(verdictFlag, 0, verdict.ok ? VERDICT_CERTIFIED : VERDICT_REJECTED); // Its own channel rather than `parentPort`, which carries Harper's ITC traffic. A closed or absent // channel is the parent's problem to detect (it treats a missing verdict as failure), so this must not // throw its way out of the exit path. From 8ee8898aced036ffec5fc5f315747361b8af067d Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 10:18:31 -0400 Subject: [PATCH 13/29] fix(deploy): a certification load that did nothing is not a pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI reported `exited with code 0` for a candidate that THROWS at load, which means the validator took the success path: on that platform the candidate is not being loaded at all. Only the parent failing closed on the missing verdict stopped that becoming a published component — a false pass is the one outcome this step exists to make impossible, and it was one fail-safe away. Certification now requires the load to have done something: a run that opened no scope and loaded no module has not exercised the candidate, so it cannot vouch for it. Asserted only when the candidate declares component configuration, since a component of nothing but static files legitimately loads nothing and cannot fail at load either. This does not explain WHY the Windows load is a no-op — that is still open, and not reproducible on macOS. What it does is turn a silent false pass into a diagnosable failure, which is both the right behaviour and the only way to see the cause from CI. --- components/deployValidator.ts | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/components/deployValidator.ts b/components/deployValidator.ts index b9a9c26cc4..f796bcc3e8 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -4,7 +4,8 @@ // which must not be able to terminate the thread out from under the verdict protocol. import { realExit } from '../server/threads/workerProcessGuard.ts'; -import { basename } from 'node:path'; +import { stat } from 'node:fs/promises'; +import { basename, join } from 'node:path'; import { parentPort, workerData } from 'node:worker_threads'; import { HDB_ROOT_DIR_NAME } from '../utility/hdbTerms.ts'; @@ -38,6 +39,19 @@ if (workerData) { delete workerData.verdictFlag; } +/** Whether a candidate declares anything a load should act on, so "loaded nothing" can be judged. */ +async function declaresLoadableContent(dirPath: string): Promise { + for (const name of ['config.yaml', 'config.yml', 'package.json']) { + try { + await stat(join(dirPath, name)); + return true; + } catch { + // Absent is the only answer that matters here; an unreadable candidate fails the load itself. + } + } + return false; +} + async function certify(): Promise { const componentName = appName || basename(candidateDirPath); const loadOptions = rootApplicationLoadOptions(componentName, { forCertification: true }); @@ -70,13 +84,28 @@ async function certify(): Promise { // would certify a candidate whose own cleanup is broken — and the thread exits either way, so nothing // downstream would ever learn. const scopes = new Set(); + const modules = new Set(); let loaded = false; try { await loadComponent(candidateDirPath, resources, HDB_ROOT_DIR_NAME, { ...loadOptions.options, collectScopes: scopes, + collectLoadedModules: modules, }); if (reportedError) throw reportedError; + // A load that did NOTHING is not a pass. Certification exists to execute the candidate, so a run + // that neither opened a scope nor loaded a module has not exercised it — and reporting success + // there is a false pass, the one outcome this whole step is meant to make impossible. It is how a + // platform-specific no-op would otherwise read as a clean verdict. + // + // Only asserted for a candidate that declares something loadable: a component of nothing but static + // files legitimately loads nothing, and cannot fail at load either. + if (!scopes.size && !modules.size && (await declaresLoadableContent(candidateDirPath))) { + throw new Error( + `Certification of ${componentName} loaded nothing: it declares component configuration, so a run ` + + `that opened no scope and loaded no module has not exercised it` + ); + } loaded = true; } finally { const closes = await Promise.allSettled(Array.from(scopes, (scope) => scope.close())); From afbb0ba8a25364deb1f949cc35099613417523a2 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 10:24:19 -0400 Subject: [PATCH 14/29] fix(deploy): stop the validator exiting silently, and bound its bootstrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows failure was not what I read it as. It reported `exited with code 0` and never reached the "loaded nothing" check I added last commit — so `report` was never called at all. A worker's event loop draining ends the thread even with a promise still pending, so a bootstrap that never settled presented as "exited without reporting a verdict", and no deadline could fire because nothing was left alive to time out. Two changes, both correct independently of the platform: - A ref'd handle held for the whole certification, so the thread cannot exit while it is still deciding. Silence is now impossible where it used to be the default failure mode. - The bootstrap has its own bound, inside the parent's deadline, and its error names the phase. Without that, a hang there is indistinguishable from a candidate that hangs. What this exposes is more interesting than the bug: loading Harper's global plugins is part of the worker bootstrap, and parts of it expect to be a member of the topology a validator deliberately is not — so it can wait on something that will never arrive. That is the tension the planning review named between a detached validator and runtime equivalence, showing up as a hang rather than as an argument. The next CI run should say which phase. --- components/deployValidator.ts | 38 ++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/components/deployValidator.ts b/components/deployValidator.ts index f796bcc3e8..20e71bfcfc 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -52,6 +52,24 @@ async function declaresLoadableContent(dirPath: string): Promise { return false; } +/** The bootstrap's own bound, well inside the parent's certification deadline so this error is the one seen. */ +const BOOTSTRAP_DEADLINE_MS = 60_000; + +/** Reject with a phase-naming error if `work` does not settle in time, so a hang is diagnosable. */ +async function withPhaseDeadline(work: Promise, ms: number, phase: string): Promise { + let timer: NodeJS.Timeout | undefined; + try { + return await Promise.race([ + work, + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error(`Certification timed out after ${ms}ms while ${phase}`)), ms); + }), + ]); + } finally { + if (timer) clearTimeout(timer); + } +} + async function certify(): Promise { const componentName = appName || basename(candidateDirPath); const loadOptions = rootApplicationLoadOptions(componentName, { forCertification: true }); @@ -66,7 +84,15 @@ async function certify(): Promise { // A certification load also has to run the code a WORKER runs — the `start`/`handleApplication` // extension path — or it proves only that the module parsed. const { loadRootPlugins } = await import('../server/loadRootComponents.js'); - const resources = await loadRootPlugins(true); + // Bounded, and it says WHICH phase did not finish. This bootstrap loads Harper's global plugins, parts + // of which expect to be a member of the worker topology a validator deliberately is not — so it can + // wait on something that will never arrive here. Without a bound that is indistinguishable from a + // candidate that hangs, and on Windows it presented as a silent exit. + const resources = await withPhaseDeadline( + loadRootPlugins(true), + BOOTSTRAP_DEADLINE_MS, + `loading Harper's global plugins` + ); // The reporter goes on AFTER the bootstrap, so it only ever sees the candidate. Installed earlier it // captured the first error from anything the root config happens to name — and since `deploy_component` @@ -145,6 +171,14 @@ function report(verdict: { ok: true } | { ok: false; message: string; stack?: st } } +// A REF'd handle for the whole certification, so this thread cannot exit while it is still deciding. +// +// Windows CI showed the failure this prevents: the thread exited with code 0 having never called `report`, +// because a worker's event loop draining ends the thread even with a promise still pending — so a bootstrap +// that never settles read to the parent as "exited without a verdict" instead of as a hang, and no timeout +// could fire because there was nothing left alive to time out. +const keepAlive = setInterval(() => {}, 1000); + void (async () => { try { await certify(); @@ -156,5 +190,7 @@ void (async () => { // prototype, and the parent only needs what it will put in the operation's own error. report({ ok: false, message: failure.message, stack: failure.stack }); realExit(1); + } finally { + clearInterval(keepAlive); } })(); From 0c906ff82c68c730a712b6f4a2aefa5da33fa68f Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 11:35:00 -0400 Subject: [PATCH 15/29] fix(deploy): leave no harper module link in the tree certification loads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Certifying a candidate loads it for real, and every non-root load runs `symlinkHarperModule`, which links the running install into the component's `node_modules/harper` so `import 'harper'` resolves to the live instance. So certification wrote into the tree it is only supposed to read, and that tree is then renamed into the live path. The packer dereferences symlinks and recurses into linked directories, so a component carrying that link packages the whole Harper install. That is what broke `Integration Tests 4/6`: `package_component` on a freshly added component spent 46s tarring and then failed with "Maximum response size reached". It passed on main only because `add_component` never loads the component, so nothing had created the link before it was packaged. `certifyCandidate` now snapshots the candidate's `node_modules` before the load and, in the same `finally` that terminates the validator, removes only the links its own load created. Only what it created, because a `file:` deploy stages a symlink to the developer's own source tree: deleting a link they already had would be certification reaching outside the candidate to modify a working tree. Nothing is taken away either way — a serving worker recreates the link the next time it loads the component. Packaging a component a worker HAS loaded still follows the link into the install. That is pre-existing, and `scanPackageDirectory` already documents the missing symlink-cycle protection behind it; it is not this change's to fix. Verified end to end: the new `certified-deploy` case fails on this branch without the cleanup ("no link to the Harper install was left behind") and passes with it, and it deploys WITHOUT a restart deliberately, since a serving worker legitimately recreates the link. `integrationTests/apiTests/components.test.mjs` now passes 25/25 locally with `package_component` at 22ms. A unit-level version of the same assertion was written and dropped: in the mocha environment `symlinkHarperModule` never gets far enough to create the link, so the test passed with and without the fix. DESIGN.md's safe-mode bullet still described the stage-without-activating draft that was reverted earlier on this branch; corrected alongside. Co-Authored-By: Claude Opus 5 --- DESIGN.md | 19 +++- components/certifyCandidate.ts | 86 +++++++++++++++++++ .../deploy/certified-deploy.test.ts | 23 +++++ 3 files changed, 126 insertions(+), 2 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 044e26bd58..b5a2a53c4e 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -486,11 +486,26 @@ Isolation contains the JS heap, the module registry, process-global registration does **not** contain databases, the filesystem, the network or native addons: a candidate can write before it throws. +Certification also loads for real, so it leaves the footprint a load leaves. `symlinkHarperModule` links the +running install into `node_modules/harper` on every non-root load — that is what makes `import 'harper'` +resolve to the live instance — so certifying writes into the tree it is only supposed to read, and that tree +is then renamed into the live path. The packer dereferences symlinks and recurses into linked directories, +so a component carrying that link packages the entire Harper install: `package_component` on a freshly added +component spent 46s tarring and then failed with "Maximum response size reached". `certifyCandidate` +therefore snapshots the candidate's `node_modules` before the load and removes only the links its own load +created, which matters because a `file:` deploy stages a symlink to the developer's own source +tree — deleting a link they already had would be certification reaching outside the candidate. This restores +the staged bytes rather than taking anything away: a serving worker recreates the link the next time it +loads the component. Packaging a component that a worker HAS loaded still follows the link; that is +pre-existing, and `scanPackageDirectory` documents the missing cycle protection behind it. + Two cases earn no authority rather than being refused — the rule is _no verdict means no authority_, never _no verdict means no deploy_: -- **Safe mode** stages without activating. It may not execute configured code, so it certifies nothing, and - nothing uncertified is published. Safe mode is transient, so the next ordinary preparation finishes it. +- **Safe mode** deploys uncertified. It may not execute configured code, so no validator can vouch for the + candidate. An earlier draft staged without activating, on the reasoning that safe mode is transient — but + nothing resumes a journal-less staged tree: `recoverInterruptedActivations` removes it as build residue, + while the operation had already returned success and replicated. So it activates and mints no `.complete`. - **A branch-configured component** deploys uncertified. A branch's location is derived only from the application and database names, so a certification load would open the store the live version is serving from: a candidate could mutate rows, throw, be rejected, and leave the live version serving the mutation. diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 7511cf8b13..d3666a081d 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -1,9 +1,11 @@ 'use strict'; +import { readdir, lstat, realpath, rm, rmdir } from 'node:fs/promises'; import { join } from 'node:path'; import { MessageChannel, Worker } from 'node:worker_threads'; import harperLogger from '../utility/logging/harper_logger.ts'; +import { PACKAGE_ROOT } from '../utility/packageUtils.js'; import { buildWorkerExecArgv, buildWorkerResourceLimits, @@ -44,6 +46,83 @@ export const VERDICT_REJECTED = 2; /** How long to wait for a validator to actually go away before giving up and saying so. */ const TERMINATION_GRACE_MS = 5000; +/** The module links `symlinkHarperModule` maintains inside a component's `node_modules`. */ +const HARPER_MODULE_LINKS = ['harper', 'harperdb']; + +/** + * What the candidate's `node_modules` held BEFORE certification loaded it. + * + * Taken so the cleanup below can put the tree back exactly as the deploy staged it, rather than removing a + * link that was already there. That distinction is not academic: a `file:` deploy stages a + * SYMLINK to the developer's own source directory, so certification reads and writes through it — and a + * developer working on that component very likely already has `node_modules/harper` pointing at this + * install. Deleting theirs would be certification reaching outside the candidate to modify a working tree. + */ +async function snapshotHarperModuleLinks( + candidateDirPath: string, + installRoot: string +): Promise<{ hadNodeModules: boolean; preexisting: Set }> { + const nodeModulesDir = join(candidateDirPath, 'node_modules'); + const preexisting = new Set(); + let hadNodeModules = false; + try { + await lstat(nodeModulesDir); + hadNodeModules = true; + } catch { + return { hadNodeModules, preexisting }; + } + for (const name of HARPER_MODULE_LINKS) { + try { + if ((await realpath(join(nodeModulesDir, name))) === installRoot) preexisting.add(name); + } catch {} + } + return { hadNodeModules, preexisting }; +} + +/** + * Remove the `node_modules/harper` (and `harperdb`) link the certification load created. + * + * `symlinkHarperModule` links the running install into a component's `node_modules` on EVERY non-root load, + * so certifying a candidate writes into the tree it is only supposed to read — and that tree is then renamed + * into the live path. The link is not part of what the deploy staged, and a serving worker recreates it the + * next time it loads the component, so removing it restores the staged bytes rather than taking anything + * away. Left behind it turns every walk of the component into a walk of the whole Harper install: the packer + * dereferences and recurses into symlinked directories, so `package_component` packaged the install. + * + * Only a link this certification created, and only one resolving to THIS install, is removed — a component + * that ships or installs a `node_modules/harper` of its own keeps it. + */ +async function removeCertificationLinks( + candidateDirPath: string, + appName: string, + installRoot: string, + before: { hadNodeModules: boolean; preexisting: Set } +): Promise { + const nodeModulesDir = join(candidateDirPath, 'node_modules'); + for (const name of HARPER_MODULE_LINKS) { + if (before.preexisting.has(name)) continue; + const linkPath = join(nodeModulesDir, name); + try { + if (!(await lstat(linkPath)).isSymbolicLink()) continue; + if ((await realpath(linkPath)) !== installRoot) continue; + await rm(linkPath, { force: true }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; + harperLogger.warn( + `Could not remove the ${name} module link certification left in the ${appName} candidate; ` + + `packaging or copying this component will follow it into the Harper install:`, + error + ); + } + } + // The load creates `node_modules` when the candidate has none, and an empty directory left behind is + // still a difference from the staged tree. + if (before.hadNodeModules) return; + try { + if ((await readdir(nodeModulesDir)).length === 0) await rmdir(nodeModulesDir); + } catch {} +} + let active = 0; const waiting: (() => void)[] = []; @@ -115,6 +194,9 @@ export async function certifyCandidate( // rejected as a malformed verdict. On a dedicated channel, anything that does not conform really is one. const { port1: verdicts, port2: verdictPort } = new MessageChannel(); const verdictFlag = new Int32Array(new SharedArrayBuffer(4)); + // Before the load, so the cleanup in the `finally` can tell what it created from what was already there. + const installRoot = await realpath(PACKAGE_ROOT).catch(() => undefined); + const linksBefore = installRoot ? await snapshotHarperModuleLinks(candidateDirPath, installRoot) : undefined; try { return await new Promise((resolve) => { @@ -264,6 +346,10 @@ export async function certifyCandidate( } } if (timer) clearTimeout(timer); + // Best-effort, and AFTER the termination above: the validator is gone (or reported as still running), + // so this is not racing a thread that could relink. Runs for every outcome, because a rejected + // candidate is swept and a certified one is renamed live, and neither should carry the link. + if (installRoot && linksBefore) await removeCertificationLinks(candidateDirPath, appName, installRoot, linksBefore); // Both ends, or the channel keeps this thread's event loop referenced. verdicts.close(); if (!slotHeld) releaseSlot(); diff --git a/integrationTests/deploy/certified-deploy.test.ts b/integrationTests/deploy/certified-deploy.test.ts index 2c87f85514..2b4c28ceb6 100644 --- a/integrationTests/deploy/certified-deploy.test.ts +++ b/integrationTests/deploy/certified-deploy.test.ts @@ -102,4 +102,27 @@ suite('deploy_component certifies a candidate before publishing it', (ctx: Conte strictEqual(await readFile(join(livePath, 'version.txt'), 'utf8'), '3', 'v3 is published'); strictEqual(await readVersion(ctx), 3, 'and answers requests'); }); + + test('the published tree does not carry the module link the certification load created', async () => { + // Certifying LOADS the candidate, and every non-root load links the running install into the + // component's `node_modules/harper` so `import 'harper'` resolves to the live instance. That link is + // not part of what the deploy staged, and the candidate tree is renamed into the live path — so + // without cleanup every walk of the component follows it into the whole Harper install. + // `package_component` did exactly that and packaged the install: 46s of tarring and then + // "Maximum response size reached". + // + // Deployed WITHOUT a restart deliberately: a serving worker legitimately recreates the link the next + // time it loads the component, so the invariant is about what the DEPLOY leaves behind. + const livePath = join(ctx.harper.dataRootDir, 'components', PROJECT); + await operation(ctx, { operation: 'deploy_component', project: PROJECT, payload: await buildPayload(4) }); + strictEqual(await readFile(join(livePath, 'version.txt'), 'utf8'), '4', 'v4 is published'); + ok(!existsSync(join(livePath, 'node_modules', 'harper')), 'no link to the Harper install was left behind'); + + // And the user-visible consequence: packaging sees the component, not the install behind the link. + const estimate = await operation(ctx, { operation: 'package_component', project: PROJECT, estimate: true }); + ok( + estimate.total_size < 1_000_000, + `packaging walks only the component (got ${estimate.total_size} bytes; the install is orders of magnitude larger)` + ); + }); }); From 072c9e504b577ffd0f7cb7b20ba2720054baf6d2 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 11:42:07 -0400 Subject: [PATCH 16/29] diag(deploy): make a validator that exits without a verdict say who ended it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI has reported `Certification of shop exited with code 0 without reporting a verdict` across three different hypotheses — a lost message (fixed by the SharedArrayBuffer flag), a candidate that loaded nothing (the check never fired), and an event-loop drain (the ref'd interval is created synchronously at module scope, so a drain cannot be it). Each diagnosis was a guess, because an exit code carries no evidence about who ended the thread. Since the ref'd interval rules out a drain, a silent code-0 exit means something CALLED exit, and the only thing that can name it is a stack captured at the exit itself. `process.exit` runs `exit` listeners, so an `exit` handler fires for `realExit` too; it logs through `console.error` rather than the logger, because a logger write queued at exit may never flush. Diagnostics, not a fix: the parent's treatment of a missing verdict is unchanged, and nothing about the verdict protocol moves. It also improves the operator-facing story for any future silent exit, on any platform. Co-Authored-By: Claude Opus 5 --- components/deployValidator.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/components/deployValidator.ts b/components/deployValidator.ts index 20e71bfcfc..f74c98c5cc 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -156,7 +156,11 @@ parentPort?.on('message', (message: any) => { if (message?.type === 'force-exit') realExit(0); }); +/** Whether `report` ran, so an exit can say whether it took the verdict path or bypassed it. */ +let reported = false; + function report(verdict: { ok: true } | { ok: false; message: string; stack?: string }): void { + reported = true; // The flag FIRST, and synchronously: this thread exits immediately after, and a posted message can lose // that race — it consistently does on Windows. Shared memory needs no event-loop turn, so the parent // sees the verdict even when the message never arrives. @@ -171,6 +175,23 @@ function report(verdict: { ok: true } | { ok: false; message: string; stack?: st } } +// Provenance for an exit that reported nothing. +// +// Windows CI has produced `exited with code 0 without reporting a verdict` repeatedly, and each diagnosis +// so far has been a guess, because an exit code carries no evidence about who ended the thread. The ref'd +// interval below rules out an event-loop drain, so a silent code-0 exit means something CALLED exit — and +// the only thing that can name it is a stack captured at the exit itself. `process.exit` runs `exit` +// listeners, so this fires for `realExit` too; `console.error` rather than the logger because a logger +// write queued here may never flush. +process.on('exit', (code) => { + if (reported) return; + console.error( + `[deploy-validator] thread for ${appName ?? candidateDirPath} is exiting with code ${code} without having ` + + `reported a verdict (flag=${verdictFlag ? Atomics.load(verdictFlag, 0) : 'absent'}). Exit called from:\n` + + new Error('validator exit').stack + ); +}); + // A REF'd handle for the whole certification, so this thread cannot exit while it is still deciding. // // Windows CI showed the failure this prevents: the thread exited with code 0 having never called `report`, From 17e99e45b8480c18eb2c3a4ec9f08530e15e9f1d Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 12:11:19 -0400 Subject: [PATCH 17/29] fix(deploy): bound the certification slot wait and the validator's exit wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two majors from the gemini pre-push leg, both cases of a wait this module claims is bounded and is not. `acquireSlot` queued without a deadline. The cap deliberately withholds the slot of a validator that will not die, so an unbounded queue behind it turned one stuck thread into every later deploy hanging inside the preparation lock with nothing to report. It now takes the certification timeout, and a caller that cannot get a slot fails with a 503 naming why. A waiter that times out leaves the queue, because leaving it there would let a later release hand a slot to a caller that is gone — drifting the count DOWN and admitting more concurrent validators than the cap, not fewer. The termination path asked the worker to exit and awaited that exit in one expression inside one `try`. A synchronous throw from the ask — `postMessage` on a channel already in an invalid state — jumped straight to the `catch`, skipping the wait entirely, so the caller swept a candidate tree whose thread was still terminating. Asking is what can fail; waiting must happen either way, so the ask is now separately fallible and the wait is not conditional on it. Verified: new unit test occupies both slots with validators parked in `Atomics.wait` and asserts the third deploy fails with the slot-timeout error and a 503 rather than queueing. It needs no sleep to arrange — `acquireSlot` claims synchronously when a slot is free, so both occupying calls hold theirs by the time the third runs. Carried rather than fixed, from the same leg: a candidate can `parentPort.removeAllListeners()` to defeat the Bun force-exit path (Node uses `terminate()`, so this is Bun-only, and a candidate that wants to do damage has easier routes — certification executes its code with no filesystem or database isolation, which DESIGN.md states); and the absence of that isolation, which is already a documented limitation and a carried major on the PR. Co-Authored-By: Claude Opus 5 --- components/certifyCandidate.ts | 68 +++++++++++++++---- .../components/deployCertification.test.js | 32 +++++++++ 2 files changed, 88 insertions(+), 12 deletions(-) diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index d3666a081d..cfa809b91d 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -126,12 +126,40 @@ async function removeCertificationLinks( let active = 0; const waiting: (() => void)[] = []; -async function acquireSlot(): Promise<() => void> { +async function acquireSlot(timeoutMs: number): Promise<() => void> { // Claimed BEFORE yielding to a waiter, not after. Decrementing and then resolving a waiter whose // `active++` runs a microtask later left a window any other caller could admit itself through, so the // documented bound did not hold. The slot is handed straight from releaser to waiter instead. + // + // And BOUNDED, like every other wait in this module. A validator that will not die keeps its slot + // deliberately (see the termination path), so an unbounded queue behind it turns one stuck thread into + // every later deploy hanging inside the preparation lock with nothing to report. A deadline turns that + // into one failed deploy per attempt, with a reason. + const deadline = Date.now() + timeoutMs; while (active >= MAX_CONCURRENT_CERTIFICATIONS) { - await new Promise((resolve) => waiting.push(resolve)); + const remaining = deadline - Date.now(); + if (remaining <= 0) { + const error: any = new Error( + `No certification slot became available within ${timeoutMs}ms; ${MAX_CONCURRENT_CERTIFICATIONS} ` + + `validator(s) are still running` + ); + error.statusCode = 503; + throw error; + } + let wake!: () => void; + const handedOver = await new Promise((resolve) => { + wake = () => resolve(true); + waiting.push(wake); + setTimeout(() => resolve(false), remaining).unref?.(); + }); + // Timed out while still queued: leave the queue, or a later release hands a slot to a caller that + // is gone and the count drifts DOWN — admitting more concurrent validators than the cap, not fewer. + // Already dequeued: a release woke us in the same turn the timer fired, so keep that handoff and let + // the loop condition decide. + if (!handedOver) { + const index = waiting.indexOf(wake); + if (index !== -1) waiting.splice(index, 1); + } } active++; let released = false; @@ -174,7 +202,12 @@ export async function certifyCandidate( error.statusCode = 503; return { certified: false, error }; } - const releaseSlot = await acquireSlot(); + let releaseSlot: () => void; + try { + releaseSlot = await acquireSlot(timeoutMs); + } catch (error) { + return { certified: false, error: error as Error }; + } // The COMPILED sibling, referenced the way `jobRunner` references `jobProcess.js`: workers load from the // build output, and `__dirname` resolves there without assuming where the package root is. const entry = join(__dirname, './deployValidator.js'); @@ -303,18 +336,29 @@ export async function certifyCandidate( // as the missed-exit race above, arrived at from the other side. const grace = () => new Promise((resolve) => setTimeout(resolve, TERMINATION_GRACE_MS).unref?.()); try { + // Asking is what can fail; WAITING for the exit must happen either way. A synchronous throw + // from the ask — `postMessage` on a channel already in an invalid state — used to jump + // straight to the `catch`, skipping the wait, so the caller swept a tree whose thread was + // still terminating. The ask is therefore fallible and the wait below is not conditional on + // it: a validator that is already gone satisfies the wait immediately, and one that is not + // still gets its grace. + let asked: Promise = Promise.resolve(); + try { + asked = + typeof (globalThis as any).Bun !== 'undefined' + ? // `terminate()` triggers a NAPI segfault under Bun; `manageThreads` asks the worker to + // exit itself for the same reason. + (worker.postMessage({ type: 'force-exit' }), Promise.resolve()) + : worker.terminate().then( + () => {}, + () => {} + ); + } catch (error) { + harperLogger.warn(`Could not ask the validator for ${appName} to exit; waiting for it anyway:`, error); + } // ONE grace for the whole thing, not one per step: racing terminate and then racing the exit // could wait 2x before calling a hung validator hung, and this runs inside a deploy holding // the preparation lock. - const asked = - typeof (globalThis as any).Bun !== 'undefined' - ? // `terminate()` triggers a NAPI segfault under Bun; `manageThreads` asks the worker to - // exit itself for the same reason. - (worker.postMessage({ type: 'force-exit' }), Promise.resolve()) - : worker.terminate().then( - () => {}, - () => {} - ); const outcome = await Promise.race([ Promise.all([asked, exited ?? Promise.resolve()]).then(() => 'exited' as const), grace().then(() => 'still-running' as const), diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index 8f916f37d1..98a3027c41 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -107,6 +107,38 @@ describe('deploy certification', () => { } }); + it('fails a deploy that cannot get a certification slot, rather than queueing it forever', async function () { + // The concurrency cap deliberately withholds the slot of a validator that will not die, so the queue + // behind it has to be bounded too — otherwise one stuck thread turns every later deploy into a wait + // inside the preparation lock with nothing to report. + this.timeout(30000); + const rootDir = await mkdtemp(join(tmpdir(), 'certify-slots-')); + const candidateDirPath = join(rootDir, 'hangs'); + await mkdir(candidateDirPath, { recursive: true }); + await writeFile(join(candidateDirPath, 'package.json'), JSON.stringify({ name: 'hangs', version: '1.0.0' })); + await writeFile(join(candidateDirPath, 'config.yaml'), 'jsResource:\n files: resource.js\n'); + await writeFile( + join(candidateDirPath, 'resource.js'), + 'const shared = new Int32Array(new SharedArrayBuffer(4));\nAtomics.wait(shared, 0, 0);\n' + ); + + try { + // `acquireSlot` claims synchronously when a slot is free — no await before `active++` — so both of + // these hold slots by the time the third call runs, without sleeping to arrange it. + const occupying = [ + certifyCandidate(candidateDirPath, 'first', { timeoutMs: 4000 }), + certifyCandidate(candidateDirPath, 'second', { timeoutMs: 4000 }), + ]; + const queued = await certifyCandidate(candidateDirPath, 'third', { timeoutMs: 300 }); + assert.strictEqual(queued.certified, false); + assert.match(queued.error.message, /No certification slot became available within 300ms/); + assert.strictEqual(queued.error.statusCode, 503); + for (const outcome of await Promise.all(occupying)) assert.strictEqual(outcome.certified, false); + } finally { + await rm(rootDir, { recursive: true, force: true }); + } + }); + it('refuses to mint .complete for a candidate no validator certified', async function () { // The gate itself. `.complete` is what recovery treats as proof a validation happened, so the // function that writes it has to require the verdict rather than trust its caller — three of From 894ded8dc018e9a9e3a4e7765817197e29269028 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 12:23:13 -0400 Subject: [PATCH 18/29] fix(deploy): carry validator progress in shared memory, and trim narrating comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit's exit diagnostic produced nothing on Windows, and its silence was not evidence: a worker's `console.error` is piped to the parent ASYNCHRONOUSLY, so anything written on the way out loses the same race the verdict message loses. It used a channel already known to be unreliable there. Progress now travels through the shared buffer, which is the only channel here proven to survive the exit. Slot 1 carries the furthest phase reached — module scope, certification entered, root plugins loaded, candidate loaded, teardown done — and the validator's own `exit` handler adds a mark to it. That mark is the interesting half: a thread torn down from outside (`terminate()`, a native abort, the process going away) never runs its exit handler, so its absence distinguishes "ended itself" from "was ended", which no exit code does. The parent renders both into the failure message instead of reporting only `exited with code 0 without reporting a verdict`. Also from the codex/gemini delta legs: - A queued waiter admitted before its deadline now clears its timer rather than leaving the closure registered until a deadline that no longer applies. - Narrating comments trimmed across `certifyCandidate`, `deployValidator`, `Application` and `operations` — both lenses flagged this, in two consecutive rounds, and the repo has already paid a cleanup commit for it (15c02a13a). The constraints and invariants are kept, in present tense; what went is the archaeology ("this used to live inside", "an earlier draft", "the gate") and the step-by-step restatement of code. Refuted rather than fixed, with a test pinning it: gemini called `declaresLoadableContent` a false-rejection risk for static-only components, since nearly every component ships a `package.json` and a static component loads no module. A static-only component deploys fine — it opens a scope, so the "loaded nothing" guard never fires — and the new unit test says so, which also stops a future change to scope creation from silently starting to reject static deploys. This is the third false-rejection shape this feature has produced, so it is asserted rather than assumed. Verified: 10 certification unit tests and 3 certified-deploy integration tests pass locally. Co-Authored-By: Claude Opus 5 --- components/Application.ts | 20 ++- components/certifyCandidate.ts | 120 ++++++++++------ components/deployValidator.ts | 128 +++++++++--------- components/operations.js | 3 +- .../components/deployCertification.test.js | 37 +++++ 5 files changed, 186 insertions(+), 122 deletions(-) diff --git a/components/Application.ts b/components/Application.ts index cb9b497fa8..ea90ede711 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -2115,9 +2115,8 @@ export async function markCandidateComplete( deploymentId: string, componentName: string ): Promise { - // The gate. Everything below writes the marker recovery trusts, so an uncertified candidate must not - // get here — and the check lives at the mint rather than at the caller for the reason `certifiedCandidates` - // documents. + // Everything below writes the marker recovery trusts, so an uncertified candidate must not get here. The + // check lives at the mint rather than at the caller for the reason `certifiedCandidates` documents. if (!isCandidateCertified(componentDirPath, deploymentId)) { throw new Error( `Refusing to mark the ${componentName} candidate ${deploymentId} complete: no validator has ` + @@ -2235,17 +2234,14 @@ export async function activateCandidateApplication(application: Application, dep throw new Error(`Cannot activate ${application.name}: no candidate build at ${candidateDirPath}`); } - // Durability first, and for EVERY activation — certified or not. This used to live inside - // `markCandidateComplete`, so skipping the mint skipped the fsync too: an uncertified swap (a - // branch-configured component, or safe mode) committed a tree whose contents were never flushed, and - // `syncRenameParents` only syncs directory entries. A power loss shortly after such a deploy returned - // success would leave the live path holding zero-length files, with the aside already retired. - // Certification decides what the tree MEANS; it was never what makes it durable. + // Durability first, and for EVERY activation, certified or not: `syncRenameParents` syncs only directory + // entries, so an unflushed swap can leave the live path holding zero-length files after a power loss, + // with the aside already retired. Certification decides what the tree MEANS, not whether it is durable, + // so this must not sit behind the mint. await syncCandidateTree(liveDirPath, deploymentId); - // The RECORD decides, never the caller. An argument saying "this one is certified" would be the forgeable - // proof the internal record exists to avoid — and an exported function with such a flag lets any caller - // mint authority for an uncertified tree, which is the invariant this step is for. + // The RECORD decides, never the caller: a `certified` argument on an exported function is proof any + // caller can forge, which is the invariant this step exists for. // // So: certified candidates get `.complete`; the deliberately uncertified ones (a branch-configured // component) simply do not, and a crash mid-swap rolls them back to the committed tree rather than diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index cfa809b91d..e04d7e7145 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -43,6 +43,49 @@ export const VERDICT_NO_ANSWER = 0; export const VERDICT_CERTIFIED = 1; export const VERDICT_REJECTED = 2; +/** + * Slots in the shared buffer. Slot 0 carries the verdict; slot 1 carries how far the validator got. + * + * The progress slot exists because Windows CI reports `exited with code 0 without reporting a verdict` and + * nothing else. The first attempt at diagnosing it logged a stack from the validator's `exit` handler, which + * produced nothing — a worker's `console.error` is piped to the parent ASYNCHRONOUSLY, so output written on + * the way out is lost exactly as the verdict message is. Shared memory is the only channel here proven to + * survive that exit, so progress travels through it: the parent can then say which phase the thread was in, + * and whether its own exit handler ever ran, without depending on a message or a pipe. + */ +export const SLOT_VERDICT = 0; +export const SLOT_PROGRESS = 1; +export const VERDICT_SLOTS = 2; + +/** Phases the validator records in `SLOT_PROGRESS`, each strictly later than the last. */ +export const PROGRESS_NOTHING = 0; +export const PROGRESS_MODULE_SCOPE = 1; +export const PROGRESS_CERTIFY_ENTERED = 2; +export const PROGRESS_ROOT_PLUGINS_LOADED = 3; +export const PROGRESS_CANDIDATE_LOADED = 4; +export const PROGRESS_TEARDOWN_DONE = 5; +/** Added to the phase when the validator's own `exit` handler runs, distinguishing a self-exit from a teardown. */ +export const PROGRESS_EXIT_OBSERVED = 100; + +const PROGRESS_NAMES: Record = { + [PROGRESS_NOTHING]: 'never ran its module body', + [PROGRESS_MODULE_SCOPE]: 'reached module scope but not the certification body', + [PROGRESS_CERTIFY_ENTERED]: 'entered certification but did not finish loading root plugins', + [PROGRESS_ROOT_PLUGINS_LOADED]: 'loaded root plugins but did not finish loading the candidate', + [PROGRESS_CANDIDATE_LOADED]: 'loaded the candidate but did not finish tearing it down', + [PROGRESS_TEARDOWN_DONE]: 'finished teardown but reported no verdict', +}; + +/** Render `SLOT_PROGRESS` for an operator: which phase, and whether the thread ended itself. */ +export function describeProgress(progress: number): string { + const selfExited = progress >= PROGRESS_EXIT_OBSERVED; + const phase = selfExited ? progress - PROGRESS_EXIT_OBSERVED : progress; + const described = PROGRESS_NAMES[phase] ?? `reached an unknown phase (${phase})`; + // A thread torn down from outside — `terminate()`, a native abort, the process going away — never runs + // its own exit handler, so the absence of that mark is the interesting half. + return selfExited ? `it ${described} and then exited itself` : `it ${described} and was ended from outside`; +} + /** How long to wait for a validator to actually go away before giving up and saying so. */ const TERMINATION_GRACE_MS = 5000; @@ -127,14 +170,12 @@ let active = 0; const waiting: (() => void)[] = []; async function acquireSlot(timeoutMs: number): Promise<() => void> { - // Claimed BEFORE yielding to a waiter, not after. Decrementing and then resolving a waiter whose - // `active++` runs a microtask later left a window any other caller could admit itself through, so the - // documented bound did not hold. The slot is handed straight from releaser to waiter instead. + // The slot passes straight from releaser to waiter: decrementing and then resolving a waiter that + // increments a microtask later leaves a window another caller can admit itself through. // - // And BOUNDED, like every other wait in this module. A validator that will not die keeps its slot - // deliberately (see the termination path), so an unbounded queue behind it turns one stuck thread into - // every later deploy hanging inside the preparation lock with nothing to report. A deadline turns that - // into one failed deploy per attempt, with a reason. + // The wait is bounded because a validator that will not die keeps its slot deliberately (see the + // termination path), and an unbounded queue behind it would hold every later deploy inside the + // preparation lock with nothing to report. const deadline = Date.now() + timeoutMs; while (active >= MAX_CONCURRENT_CERTIFICATIONS) { const remaining = deadline - Date.now(); @@ -147,11 +188,16 @@ async function acquireSlot(timeoutMs: number): Promise<() => void> { throw error; } let wake!: () => void; + let timer: NodeJS.Timeout | undefined; const handedOver = await new Promise((resolve) => { wake = () => resolve(true); waiting.push(wake); - setTimeout(() => resolve(false), remaining).unref?.(); + timer = setTimeout(() => resolve(false), remaining); + timer.unref?.(); }); + // Cleared on the way out either way, so a waiter admitted early does not keep its closure registered + // until a deadline that no longer applies to it. + if (timer) clearTimeout(timer); // Timed out while still queued: leave the queue, or a later release hands a slot to a caller that // is gone and the count drifts DOWN — admitting more concurrent validators than the cap, not fewer. // Already dequeued: a release woke us in the same turn the timer fired, so keep that handoff and let @@ -212,21 +258,18 @@ export async function certifyCandidate( // build output, and `__dirname` resolves there without assuming where the package root is. const entry = join(__dirname, './deployValidator.js'); let worker: Worker | undefined; - // Installed the moment the worker exists, NOT in the `finally`. Attaching it later races the exit it is - // meant to observe: the validator `realExit`s immediately after posting, so on any turn where the parent - // sees the exit before the queued verdict, a listener attached afterwards never fires — `certifyCandidate` - // never returns, its slot is never released, and `prepareApplication` waits inside the preparation lock - // forever. Two of those and the node stops deploying until it restarts. + // Must be installed the moment the worker exists, never in the `finally`: the validator `realExit`s + // immediately after posting, so a listener attached after the exit has already fired never runs, and + // `certifyCandidate` then never returns while holding the preparation lock. let exited: Promise | undefined; // Set when a validator outlives its termination grace, so its slot is deliberately not returned. let slotHeld = false; let settled = false; let timer: NodeJS.Timeout | undefined; - // A channel of its own, NOT `parentPort`: Harper's worker machinery uses that for its own ITC traffic, - // so a verdict read from it would compete with unrelated messages — the first one to arrive was being - // rejected as a malformed verdict. On a dedicated channel, anything that does not conform really is one. + // A channel of its own, never `parentPort`: that carries Harper's ITC traffic, so an unrelated message + // arriving first reads as a malformed verdict. On a dedicated channel, anything non-conforming really is. const { port1: verdicts, port2: verdictPort } = new MessageChannel(); - const verdictFlag = new Int32Array(new SharedArrayBuffer(4)); + const verdictFlag = new Int32Array(new SharedArrayBuffer(VERDICT_SLOTS * 4)); // Before the load, so the cleanup in the `finally` can tell what it created from what was already there. const installRoot = await realpath(PACKAGE_ROOT).catch(() => undefined); const linksBefore = installRoot ? await snapshotHarperModuleLinks(candidateDirPath, installRoot) : undefined; @@ -310,9 +353,12 @@ export async function certifyCandidate( // Only reached when no verdict MESSAGE arrived first. The shared flag is written before the // validator exits, so it is still authoritative here — this is the ordinary path on Windows, // where the exit consistently beats the queued message. - const flag = Atomics.load(verdictFlag, 0); + const flag = Atomics.load(verdictFlag, SLOT_VERDICT); if (flag === VERDICT_NO_ANSWER) { - fail(`Certification of ${appName} exited with code ${code} without reporting a verdict`); + fail( + `Certification of ${appName} exited with code ${code} without reporting a verdict: ` + + describeProgress(Atomics.load(verdictFlag, SLOT_PROGRESS)) + ); return; } if (flag === VERDICT_CERTIFIED) { @@ -331,17 +377,12 @@ export async function certifyCandidate( // reported rather than treated as cleanup done: the caller is about to remove a tree this thread // may still be reading. if (worker) { - // Every wait here is bounded. This runs in a `finally` that the caller's deploy is blocked on, so a - // termination that never settles would hold the preparation lock indefinitely — the same failure - // as the missed-exit race above, arrived at from the other side. + // Every wait here is bounded: this `finally` blocks the caller's deploy, so a termination that never + // settles would hold the preparation lock indefinitely. const grace = () => new Promise((resolve) => setTimeout(resolve, TERMINATION_GRACE_MS).unref?.()); try { - // Asking is what can fail; WAITING for the exit must happen either way. A synchronous throw - // from the ask — `postMessage` on a channel already in an invalid state — used to jump - // straight to the `catch`, skipping the wait, so the caller swept a tree whose thread was - // still terminating. The ask is therefore fallible and the wait below is not conditional on - // it: a validator that is already gone satisfies the wait immediately, and one that is not - // still gets its grace. + // Asking is what can fail; waiting for the exit must happen either way. A synchronous throw from + // the ask must not skip the wait, or the caller sweeps a tree whose thread is still terminating. let asked: Promise = Promise.resolve(); try { asked = @@ -356,22 +397,17 @@ export async function certifyCandidate( } catch (error) { harperLogger.warn(`Could not ask the validator for ${appName} to exit; waiting for it anyway:`, error); } - // ONE grace for the whole thing, not one per step: racing terminate and then racing the exit - // could wait 2x before calling a hung validator hung, and this runs inside a deploy holding - // the preparation lock. + // ONE grace for the whole thing: racing terminate and then the exit separately could wait twice + // over before calling a hung validator hung, inside a deploy holding the preparation lock. const outcome = await Promise.race([ Promise.all([asked, exited ?? Promise.resolve()]).then(() => 'exited' as const), grace().then(() => 'still-running' as const), ]); - // A validator that would not die keeps its slot. Releasing it would let the node start another - // thread while a runaway one is still holding the candidate tree open and consuming the heap - // this cap exists to bound — and the caller is about to sweep that tree. Bounded by - // MAX_CONCURRENT_CERTIFICATIONS either way, so the worst case is that certification stops on - // this thread and says so, rather than quietly overcommitting. + // A validator that would not die keeps its slot: releasing it would admit another thread while a + // runaway one still holds the candidate tree open, and the caller is about to sweep that tree. if (outcome === 'still-running') { - // Held UNTIL it actually goes away, not forever. Holding it permanently would mean each - // runaway validator shrinks the cap for the life of the process, and a few would stop the - // node deploying — trading a bounded overcommit for an unbounded outage. + // Held until it actually goes away, not forever: permanently held slots would shrink the cap + // for the life of the process, trading a bounded overcommit for an unbounded outage. slotHeld = true; harperLogger.error( `The validator certifying ${appName} did not exit within ${TERMINATION_GRACE_MS}ms; holding its ` + @@ -390,9 +426,9 @@ export async function certifyCandidate( } } if (timer) clearTimeout(timer); - // Best-effort, and AFTER the termination above: the validator is gone (or reported as still running), - // so this is not racing a thread that could relink. Runs for every outcome, because a rejected - // candidate is swept and a certified one is renamed live, and neither should carry the link. + // After the termination above, so this is not racing a thread that could relink. Runs for every + // outcome: a rejected candidate is swept and a certified one is renamed live, and neither should + // carry the link. if (installRoot && linksBefore) await removeCertificationLinks(candidateDirPath, appName, installRoot, linksBefore); // Both ends, or the channel keeps this thread's event loop referenced. verdicts.close(); diff --git a/components/deployValidator.ts b/components/deployValidator.ts index f74c98c5cc..4833483c9a 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -10,7 +10,18 @@ import { parentPort, workerData } from 'node:worker_threads'; import { HDB_ROOT_DIR_NAME } from '../utility/hdbTerms.ts'; import harperLogger from '../utility/logging/harper_logger.ts'; -import { VERDICT_CERTIFIED, VERDICT_REJECTED } from './certifyCandidate.ts'; +import { + PROGRESS_CANDIDATE_LOADED, + PROGRESS_CERTIFY_ENTERED, + PROGRESS_EXIT_OBSERVED, + PROGRESS_MODULE_SCOPE, + PROGRESS_ROOT_PLUGINS_LOADED, + PROGRESS_TEARDOWN_DONE, + SLOT_PROGRESS, + SLOT_VERDICT, + VERDICT_CERTIFIED, + VERDICT_REJECTED, +} from './certifyCandidate.ts'; import { loadComponent, rootApplicationLoadOptions, setErrorReporter } from './componentLoader.ts'; import type { Scope } from './Scope.ts'; @@ -27,10 +38,9 @@ import type { Scope } from './Scope.ts'; * without a verdict, a malformed message — is failure at the parent, which is what makes "inability to * obtain a verdict is failure" true rather than aspirational. */ -// Captured and then REMOVED from `workerData`, before any candidate code runs. `workerData` is reachable -// from the candidate — `require('node:worker_threads').workerData` — so leaving the port there would let a -// candidate post its own passing verdict and certify itself. Taking it out of the bag is the difference -// between a capability this module holds and one the whole thread holds. +// Captured and REMOVED from `workerData` before any candidate code runs: `workerData` is reachable from +// the candidate via `require('node:worker_threads')`, so a port left there would let a candidate post its +// own passing verdict and certify itself. const { candidateDirPath, appName } = workerData ?? {}; const verdictPort = workerData?.verdictPort; const verdictFlag: Int32Array | undefined = workerData?.verdictFlag; @@ -39,6 +49,13 @@ if (workerData) { delete workerData.verdictFlag; } +/** Record the furthest phase reached, in the one channel that survives this thread's exit on every platform. */ +function markProgress(phase: number): void { + if (verdictFlag) Atomics.store(verdictFlag, SLOT_PROGRESS, phase); +} + +markProgress(PROGRESS_MODULE_SCOPE); + /** Whether a candidate declares anything a load should act on, so "loaded nothing" can be judged. */ async function declaresLoadableContent(dirPath: string): Promise { for (const name of ['config.yaml', 'config.yml', 'package.json']) { @@ -71,18 +88,16 @@ async function withPhaseDeadline(work: Promise, ms: number, phase: string) } async function certify(): Promise { + markProgress(PROGRESS_CERTIFY_ENTERED); const componentName = appName || basename(candidateDirPath); const loadOptions = rootApplicationLoadOptions(componentName, { forCertification: true }); if (!loadOptions.ok) { throw new Error(`Cannot certify ${componentName}: its root-config mount could not be resolved`); } - // The global plugins FIRST. They are what create the surfaces applications extend — a fixture doing - // `server.mqtt.authorizeClient = …` needs the mqtt plugin to have loaded, or the assignment throws on - // `undefined` and certification rejects a component that works fine on a serving worker. This stops - // before the other applications, which is the whole reason it is a separate entry point. - // - // A certification load also has to run the code a WORKER runs — the `start`/`handleApplication` - // extension path — or it proves only that the module parsed. + // The global plugins first: they create the surfaces applications extend, so a component assigning + // `server.mqtt.authorizeClient` needs the mqtt plugin loaded or the assignment throws on `undefined` and + // certification rejects something a serving worker loads fine. Loading stops before the other + // applications, which is why this is a separate entry point from `loadRootComponents`. const { loadRootPlugins } = await import('../server/loadRootComponents.js'); // Bounded, and it says WHICH phase did not finish. This bootstrap loads Harper's global plugins, parts // of which expect to be a member of the worker topology a validator deliberately is not — so it can @@ -93,22 +108,18 @@ async function certify(): Promise { BOOTSTRAP_DEADLINE_MS, `loading Harper's global plugins` ); + markProgress(PROGRESS_ROOT_PLUGINS_LOADED); - // The reporter goes on AFTER the bootstrap, so it only ever sees the candidate. Installed earlier it - // captured the first error from anything the root config happens to name — and since `deploy_component` - // writes a component's config entry before building it, that includes the candidate's own live path, - // which does not exist yet on a first deploy. Certification then rejected the candidate for the absence - // of the thing it was about to create. + // Installed AFTER the bootstrap so it only ever sees the candidate. Earlier, it captures the first error + // from anything the root config names — including the candidate's own live path, which `deploy_component` + // writes before building and which does not exist yet on a first deploy. // - // The candidate's own load-time error matters, not just whether the promise rejected: the loader reports - // some failures through the reporter while resolving successfully. + // A reporter is needed at all because the loader reports some failures through it while still resolving. let reportedError: Error | undefined; setErrorReporter((error: Error) => (reportedError ??= error)); - // Collected so teardown happens BEFORE the verdict, not after. The in-process check this replaces - // established that a Scope which fails to close is a REJECTED validation, not a warning: `close()` stops - // at the throwing listener, so the scope stays partially live. Posting a pass and then failing teardown - // would certify a candidate whose own cleanup is broken — and the thread exits either way, so nothing - // downstream would ever learn. + // Collected so teardown happens BEFORE the verdict. A scope that fails to close is a rejected + // validation, not a warning: `close()` stops at the throwing listener, leaving the scope partially live, + // and the thread exits either way so nothing downstream would learn of a failure reported after a pass. const scopes = new Set(); const modules = new Set(); let loaded = false; @@ -119,13 +130,10 @@ async function certify(): Promise { collectLoadedModules: modules, }); if (reportedError) throw reportedError; - // A load that did NOTHING is not a pass. Certification exists to execute the candidate, so a run - // that neither opened a scope nor loaded a module has not exercised it — and reporting success - // there is a false pass, the one outcome this whole step is meant to make impossible. It is how a - // platform-specific no-op would otherwise read as a clean verdict. - // - // Only asserted for a candidate that declares something loadable: a component of nothing but static - // files legitimately loads nothing, and cannot fail at load either. + // A load that did nothing is not a pass: a run that neither opened a scope nor loaded a module has + // not exercised the candidate, which is how a platform-specific no-op would read as a clean verdict. + // Only asserted for a candidate declaring loadable content — see the static-only case in + // `deployCertification.test.js`, which opens a scope and so stays clear of this. if (!scopes.size && !modules.size && (await declaresLoadableContent(candidateDirPath))) { throw new Error( `Certification of ${componentName} loaded nothing: it declares component configuration, so a run ` + @@ -133,6 +141,7 @@ async function certify(): Promise { ); } loaded = true; + markProgress(PROGRESS_CANDIDATE_LOADED); } finally { const closes = await Promise.allSettled(Array.from(scopes, (scope) => scope.close())); const failed = closes.filter((result) => result.status === 'rejected'); @@ -145,29 +154,23 @@ async function certify(): Promise { `${componentName} loaded but ${failed.length} scope(s) failed to tear down` ); } + if (loaded) markProgress(PROGRESS_TEARDOWN_DONE); } } -// The receiver for the parent's Bun force-exit. Registered HERE rather than relied on as an import side -// effect of `manageThreads`' worker block: `terminate()` segfaults under Bun, so that message is the only -// way the parent can end this thread, and a capability that important should not depend on which modules -// happened to load. +// The receiver for the parent's Bun force-exit, registered here rather than inherited as an import side +// effect of `manageThreads`: `terminate()` segfaults under Bun, so this message is the only way the parent +// can end the thread, and that must not depend on which modules happened to load. parentPort?.on('message', (message: any) => { if (message?.type === 'force-exit') realExit(0); }); -/** Whether `report` ran, so an exit can say whether it took the verdict path or bypassed it. */ -let reported = false; - function report(verdict: { ok: true } | { ok: false; message: string; stack?: string }): void { - reported = true; - // The flag FIRST, and synchronously: this thread exits immediately after, and a posted message can lose - // that race — it consistently does on Windows. Shared memory needs no event-loop turn, so the parent - // sees the verdict even when the message never arrives. - if (verdictFlag) Atomics.store(verdictFlag, 0, verdict.ok ? VERDICT_CERTIFIED : VERDICT_REJECTED); - // Its own channel rather than `parentPort`, which carries Harper's ITC traffic. A closed or absent - // channel is the parent's problem to detect (it treats a missing verdict as failure), so this must not - // throw its way out of the exit path. + // The flag first, and synchronously: this thread exits immediately after, and a posted message loses + // that race on Windows. Shared memory needs no event-loop turn. + if (verdictFlag) Atomics.store(verdictFlag, SLOT_VERDICT, verdict.ok ? VERDICT_CERTIFIED : VERDICT_REJECTED); + // A closed or absent channel is the parent's problem to detect — it treats a missing verdict as failure — + // so this must not throw its way out of the exit path. try { verdictPort?.postMessage(verdict); } catch (error) { @@ -175,29 +178,22 @@ function report(verdict: { ok: true } | { ok: false; message: string; stack?: st } } -// Provenance for an exit that reported nothing. +// Provenance for an exit that reported nothing, written where it CANNOT be lost. // -// Windows CI has produced `exited with code 0 without reporting a verdict` repeatedly, and each diagnosis -// so far has been a guess, because an exit code carries no evidence about who ended the thread. The ref'd -// interval below rules out an event-loop drain, so a silent code-0 exit means something CALLED exit — and -// the only thing that can name it is a stack captured at the exit itself. `process.exit` runs `exit` -// listeners, so this fires for `realExit` too; `console.error` rather than the logger because a logger -// write queued here may never flush. -process.on('exit', (code) => { - if (reported) return; - console.error( - `[deploy-validator] thread for ${appName ?? candidateDirPath} is exiting with code ${code} without having ` + - `reported a verdict (flag=${verdictFlag ? Atomics.load(verdictFlag, 0) : 'absent'}). Exit called from:\n` + - new Error('validator exit').stack - ); +// A first attempt logged a stack from this handler and produced nothing on Windows: a worker's +// `console.error` is piped to the parent asynchronously, so anything written on the way out loses the same +// race the verdict message loses. Shared memory needs no event-loop turn, so the phase markers below travel +// through it instead, and this handler marks that the thread ended ITSELF — a thread torn down from outside +// (`terminate()`, a native abort, the process going away) never runs it, and that absence is the evidence. +process.on('exit', () => { + if (!verdictFlag) return; + const progress = Atomics.load(verdictFlag, SLOT_PROGRESS); + if (progress < PROGRESS_EXIT_OBSERVED) Atomics.store(verdictFlag, SLOT_PROGRESS, progress + PROGRESS_EXIT_OBSERVED); }); -// A REF'd handle for the whole certification, so this thread cannot exit while it is still deciding. -// -// Windows CI showed the failure this prevents: the thread exited with code 0 having never called `report`, -// because a worker's event loop draining ends the thread even with a promise still pending — so a bootstrap -// that never settles read to the parent as "exited without a verdict" instead of as a hang, and no timeout -// could fire because there was nothing left alive to time out. +// A ref'd handle for the whole certification: a worker's event loop draining ends the thread even with a +// promise still pending, which would reach the parent as "exited without a verdict" rather than as a hang, +// with no timeout left alive to fire. const keepAlive = setInterval(() => {}, 1000); void (async () => { diff --git a/components/operations.js b/components/operations.js index e87e2a3992..04ae0e2416 100644 --- a/components/operations.js +++ b/components/operations.js @@ -664,8 +664,7 @@ async function deployComponent(req) { emit('phase', { phase: 'prepare', status: 'start' }); let prepareDoneEmitted = false; await prepareApplication(application, { - // The same phases in the same order as before certification moved into the preparation: the - // `prepare` phase closes when the candidate is built, then `load` brackets certification. + // `prepare` closes when the candidate is built, then `load` brackets certification. emitPhase: (phase, status) => { if (phase === 'load' && status === 'start' && !prepareDoneEmitted) { prepareDoneEmitted = true; diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index 98a3027c41..36b90a0ad3 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -83,6 +83,43 @@ describe('deploy certification', () => { } }); + it('publishes a static-only component, which legitimately loads no module', async function () { + // The "loaded nothing is not a pass" guard is a net for a platform-specific no-op reading as a clean + // verdict, and its trigger is whether the candidate declares loadable content — for which a + // `package.json` is weak evidence, since nearly every component ships one for versioning. A component + // of nothing but static files opens no scope and loads no module by design, so if that guard fires on + // it, certification rejects a deploy that works today. This is the third false-rejection shape this + // feature has produced, so it is asserted rather than assumed. + this.timeout(30000); + const rootDir = await mkdtemp(join(tmpdir(), 'certify-static-')); + const componentDirPath = join(rootDir, 'brochure'); + await mkdir(componentDirPath, { recursive: true }); + await writeFile(join(componentDirPath, 'package.json'), JSON.stringify({ name: 'brochure', version: '1.0.0' })); + + const sourceDir = await mkdtemp(join(rootDir, 'brochure-2.0.0-')); + await writeFile(join(sourceDir, 'package.json'), JSON.stringify({ name: 'brochure', version: '2.0.0' })); + await writeFile(join(sourceDir, 'config.yaml'), "static:\n files: 'web/**'\n"); + await mkdir(join(sourceDir, 'web'), { recursive: true }); + await writeFile(join(sourceDir, 'web', 'index.html'), 'hi\n'); + + const application = new Application({ + name: 'brochure', + payload: await packageDirectory(sourceDir, { skip_node_modules: true }), + }); + application.dirPath = componentDirPath; + + try { + await prepareApplication(application); + assert.strictEqual( + JSON.parse(await readFile(join(componentDirPath, 'package.json'), 'utf8')).version, + '2.0.0', + 'a static-only component is published rather than rejected for loading nothing' + ); + } finally { + await rm(rootDir, { recursive: true, force: true }); + } + }); + it('rejects a candidate whose load never finishes, rather than waiting on it', async function () { this.timeout(30000); const rootDir = await mkdtemp(join(tmpdir(), 'certify-timeout-')); From 1f8313075902ed37b60a75ce63894854e07aad25 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 12:40:16 -0400 Subject: [PATCH 19/29] fix(deploy): hand the certification slot over instead of releasing it, and keep the rejection's error text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 8 of the pre-push review, with four lenses and domain adjudication. The slot release contradicted its own comment. It decremented `active` and then woke a waiter that incremented a microtask later, so the slot was released and re-competed for rather than handed over: a caller arriving synchronously in that window took it, and the woken waiter went to the back of its own queue. A release offered to a waiter that had already timed out was also swallowed, leaving a free slot nobody was woken for. `active` now never dips — the releaser offers the slot along the queue until a live waiter accepts, and only falls back to decrementing when none does — and an admitted waiter holds the inherited slot without incrementing, which is what makes the handoff a handoff. No test: the steal needs a caller arriving inside a synchronous microtask window that `certifyCandidate` does not expose, and the swallowed-wake half is not observable through the public surface either, since the next caller reads `active` directly. A test that passed with and without the fix would be worse than none. Verified by reading, against gemini's mechanism, which was correct. A rejection on Windows lost the candidate's actual error. The shared flag says REJECTED and only the queued message carries the error text, but the exit consistently beats that message there — so the parent settled on the flag alone, reported "exited before reporting why", and then closed the channel, discarding the syntax error the operator needed. The flag stays the authority; the exit now gives the detail a 250ms grace, and whichever answer lands first stands. Recovery logged the opposite of what it was doing. A resumed activation after a restart finds the in-memory certification record empty, so it warned that nothing had certified the candidate and recovery would roll it back — while recovery was rolling it forward on the strength of the on-disk marker. That case is now its own branch and says so. The marker is still never re-minted from its own presence. Also: fixed a factual contradiction the review caught between two comments I added last commit (one said a static-only load opens no scope, the other that it does — it does), and trimmed the diagnosis-history narration that survived the last pass. Carried, and now the reason to re-examine the framing rather than iterate again: under Bun, `terminate()` segfaults, so the parent can only ASK the validator to exit — and a candidate can `parentPort.removeAllListeners()` or simply block its event loop, in which case the ask is never processed, the thread never exits, and its slot is held for the life of the process. Two of those stop the node deploying. Cooperative exit cannot be made reliable against code that is adversarial or merely synchronous, so this is a property of certifying on a thread the parent cannot kill, not a bug to patch. The review's own gate agrees: `framing-recheck: REQUIRED — round 8 still yields a fresh major on this change`. Co-Authored-By: Claude Opus 5 --- components/Application.ts | 20 +++-- components/certifyCandidate.ts | 87 ++++++++++++------- components/deployValidator.ts | 4 +- .../components/deployCertification.test.js | 11 ++- 4 files changed, 76 insertions(+), 46 deletions(-) diff --git a/components/Application.ts b/components/Application.ts index ea90ede711..270d093815 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -2173,15 +2173,11 @@ async function certifyPreparedCandidate( candidateDirPath: string, options: PrepareApplicationOptions ): Promise<{ certified: boolean }> { - // SAFE MODE ACTIVATES, uncertified. An earlier draft staged without activating, on the reasoning that - // safe mode is transient so the candidate could wait — but nothing resumes it: the staged tree carries - // no journal, so `recoverInterruptedActivations` removes it as build residue at the next start, while - // `deploy_component` had already returned success, replicated the operation and run its restart phase. - // An operator booting into safe mode to replace the component crashing the node would have got a 200, - // live peers, and a node that came back running the broken component with the fix deleted. - // - // So it takes the same shape as the branch-configured case below: the deploy happens, and it earns no - // authority. `.complete` is never written, so a crash mid-swap rolls back to the committed tree. + // Safe mode ACTIVATES, uncertified, rather than staging for later: a staged tree carries no journal, so + // `recoverInterruptedActivations` removes it as build residue at the next start — while the operation has + // already returned success, replicated, and run its restart phase. So it takes the same shape as the + // branch-configured case below: the deploy happens and earns no authority, and with no `.complete` a + // crash mid-swap rolls back to the committed tree. const safeMode = process.env.HARPER_SAFE_MODE && process.env.HARPER_SAFE_MODE !== 'false' && process.env.HARPER_SAFE_MODE !== '0'; if (safeMode) { @@ -2248,6 +2244,12 @@ export async function activateCandidateApplication(application: Application, dep // forward onto something no validator vouched for. if (isCandidateCertified(liveDirPath, deploymentId)) { await markCandidateComplete(liveDirPath, deploymentId, application.name); + } else if (await candidateIsCertifiedOnDisk(candidateDeploymentDirPath(liveDirPath, deploymentId))) { + // A resumed activation after a restart. The in-memory record is empty then, but the marker a validator + // already earned is on disk, and recovery is rolling this candidate FORWARD on the strength of it — + // so the warning below would be the opposite of what is happening. The marker is not re-minted from + // its own presence; nothing is written here. + application.logger.info(`Resuming the interrupted activation of ${application.name}, already certified`); } else { application.logger.warn( `Activating ${application.name} without a certification marker: nothing certified this candidate, so ` + diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index e04d7e7145..374e3a4824 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -46,12 +46,10 @@ export const VERDICT_REJECTED = 2; /** * Slots in the shared buffer. Slot 0 carries the verdict; slot 1 carries how far the validator got. * - * The progress slot exists because Windows CI reports `exited with code 0 without reporting a verdict` and - * nothing else. The first attempt at diagnosing it logged a stack from the validator's `exit` handler, which - * produced nothing — a worker's `console.error` is piped to the parent ASYNCHRONOUSLY, so output written on - * the way out is lost exactly as the verdict message is. Shared memory is the only channel here proven to - * survive that exit, so progress travels through it: the parent can then say which phase the thread was in, - * and whether its own exit handler ever ran, without depending on a message or a pipe. + * Progress travels through shared memory because that is the only channel that survives this thread's exit: + * a worker's `console.error` is piped to the parent asynchronously, so output written on the way out loses + * the same race the verdict message loses. An exit code alone cannot say which phase the thread was in, nor + * whether it ended itself. */ export const SLOT_VERDICT = 0; export const SLOT_PROGRESS = 1; @@ -89,6 +87,16 @@ export function describeProgress(progress: number): string { /** How long to wait for a validator to actually go away before giving up and saying so. */ const TERMINATION_GRACE_MS = 5000; +/** + * How long a rejection waits for the validator's queued message after its exit. + * + * The shared flag says a candidate was rejected; only the MESSAGE carries the candidate's own error text, + * and on Windows the exit consistently beats it. Settling on the flag alone reported every Windows rejection + * as "exited before reporting why" and then closed the channel, discarding the syntax error the operator + * needed. The flag is already the authority, so this waits only for the detail — and briefly. + */ +const REJECTION_DETAIL_GRACE_MS = 250; + /** The module links `symlinkHarperModule` maintains inside a component's `node_modules`. */ const HARPER_MODULE_LINKS = ['harper', 'harperdb']; @@ -167,11 +175,14 @@ async function removeCertificationLinks( } let active = 0; -const waiting: (() => void)[] = []; +/** Queued waiters. Each returns whether it accepted the slot being offered; a lapsed one declines. */ +const waiting: (() => boolean)[] = []; async function acquireSlot(timeoutMs: number): Promise<() => void> { - // The slot passes straight from releaser to waiter: decrementing and then resolving a waiter that - // increments a microtask later leaves a window another caller can admit itself through. + // A released slot is HANDED to the next waiter without `active` ever dipping. Decrementing and then + // waking a waiter that increments a microtask later leaves a window a fresh caller can claim through + // synchronously, sending the woken waiter to the back of its own queue; and a release offered to a + // waiter that has already timed out would be swallowed, leaving a free slot nobody is woken for. // // The wait is bounded because a validator that will not die keeps its slot deliberately (see the // termination path), and an unbounded queue behind it would hold every later deploy inside the @@ -187,33 +198,46 @@ async function acquireSlot(timeoutMs: number): Promise<() => void> { error.statusCode = 503; throw error; } - let wake!: () => void; - let timer: NodeJS.Timeout | undefined; - const handedOver = await new Promise((resolve) => { - wake = () => resolve(true); - waiting.push(wake); - timer = setTimeout(() => resolve(false), remaining); - timer.unref?.(); - }); - // Cleared on the way out either way, so a waiter admitted early does not keep its closure registered - // until a deadline that no longer applies to it. - if (timer) clearTimeout(timer); - // Timed out while still queued: leave the queue, or a later release hands a slot to a caller that - // is gone and the count drifts DOWN — admitting more concurrent validators than the cap, not fewer. - // Already dequeued: a release woke us in the same turn the timer fired, so keep that handoff and let - // the loop condition decide. - if (!handedOver) { - const index = waiting.indexOf(wake); + let settled = false; + let resolveWait!: (inherited: boolean) => void; + const waited = new Promise((resolve) => (resolveWait = resolve)); + const waiter = () => { + if (settled) return false; + settled = true; + resolveWait(true); + return true; + }; + waiting.push(waiter); + const timer = setTimeout(() => { + if (settled) return; + settled = true; + const index = waiting.indexOf(waiter); if (index !== -1) waiting.splice(index, 1); - } + resolveWait(false); + }, remaining); + timer.unref?.(); + const inherited = await waited; + // Cleared either way, so an admitted waiter does not keep its closure registered until a deadline + // that no longer applies to it. + clearTimeout(timer); + // The slot came from the releaser with `active` already accounting for it, so this caller holds it + // without incrementing — which is what makes the handoff a handoff. + if (inherited) return makeRelease(); } active++; + return makeRelease(); +} + +function makeRelease(): () => void { let released = false; return () => { if (released) return; released = true; + // Offer the slot along the queue until someone takes it. Only if nobody does does the count fall. + while (waiting.length > 0) { + if (waiting.shift()!()) return; + } active--; - waiting.shift()?.(); }; } @@ -365,7 +389,12 @@ export async function certifyCandidate( settle({ certified: true }); return; } - fail(`${appName} failed to load during certification (its validator exited before reporting why)`); + // Rejected for certain; what is still in flight is WHY. The message handler settles with the + // candidate's own error if it lands first, and `settle` takes the first answer either way, so + // this is the fallback rather than a race the detail can lose outright. + setTimeout(() => { + fail(`${appName} failed to load during certification (its validator exited before reporting why)`); + }, REJECTION_DETAIL_GRACE_MS).unref?.(); }); }); } finally { diff --git a/components/deployValidator.ts b/components/deployValidator.ts index 4833483c9a..3feb2e0c2a 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -132,8 +132,8 @@ async function certify(): Promise { if (reportedError) throw reportedError; // A load that did nothing is not a pass: a run that neither opened a scope nor loaded a module has // not exercised the candidate, which is how a platform-specific no-op would read as a clean verdict. - // Only asserted for a candidate declaring loadable content — see the static-only case in - // `deployCertification.test.js`, which opens a scope and so stays clear of this. + // A static-only component stays clear of this because its load still opens a scope, which + // `deployCertification.test.js` pins. if (!scopes.size && !modules.size && (await declaresLoadableContent(candidateDirPath))) { throw new Error( `Certification of ${componentName} loaded nothing: it declares component configuration, so a run ` + diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index 36b90a0ad3..d8f9f5381b 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -84,12 +84,11 @@ describe('deploy certification', () => { }); it('publishes a static-only component, which legitimately loads no module', async function () { - // The "loaded nothing is not a pass" guard is a net for a platform-specific no-op reading as a clean - // verdict, and its trigger is whether the candidate declares loadable content — for which a - // `package.json` is weak evidence, since nearly every component ships one for versioning. A component - // of nothing but static files opens no scope and loads no module by design, so if that guard fires on - // it, certification rejects a deploy that works today. This is the third false-rejection shape this - // feature has produced, so it is asserted rather than assumed. + // The "loaded nothing is not a pass" guard fires on a candidate that declares loadable content and + // then opens no scope and loads no module. A static-only component declares content (every component + // ships a `package.json`) and loads no JS module, so whether the guard rejects it comes down to + // whether a static load still opens a scope. It does — this test is what pins that, so a change to + // scope creation cannot silently turn valid static deploys into certification failures. this.timeout(30000); const rootDir = await mkdtemp(join(tmpdir(), 'certify-static-')); const componentDirPath = join(rootDir, 'brochure'); From 8f320866663cf31dfbe65f5c592e74d420368450 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 14:43:22 -0400 Subject: [PATCH 20/29] feat(threads): let a caller add workerData and transfer ports without losing the ITC bootstrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Groundwork for certifying a deploy candidate on a standard-profile worker (#2315 step 2). The planning review called this a blocker for that design, and it is: the validator needs a candidate path, a private `MessagePort` and a shared verdict buffer, and `startWorker` could carry none of them. `...options` is spread into the `Worker` constructor AFTER the bootstrap `workerData`, so passing `options.workerData` replaced that object wholesale and took `addPorts`/`addThreadIds` with it — the thread came up with no ITC wiring and nothing said so. `workerDataProviders` is no way around it either, because it `structuredClone`s and a `MessagePort` must be transferred. So: `extraWorkerData` is merged into the bootstrap rather than substituted for it, `extraTransferList` is concatenated with the port list, and `options.workerData` is now refused outright with a message naming the alternative. Reserved keys are refused too, reusing the list `registerWorkerDataProvider` already validates against rather than adding a second one. `noServerStart` gets a supported route for the same reason: it is a RESERVED key, so neither a provider nor `extraWorkerData` can supply it, but a thread that must not serve has to. `options.noServerStart` adds it, and only when asked for, so the default spawn's workerData is unchanged. Validation runs as the first thing in `startWorker`, before `buildWorkerExecArgv`. That ordering is load-bearing, not tidiness: `getImportModules()` memoizes, so throwing after it freezes the configured preload list for the life of the process on a spawn that never happened. My own test proved it — the rejection cases poisoned `preloadSafeMode.test.js` until the check moved up, which is the same memoization hazard the validator's `preloads: false` exists for. `execArgvOptions` is threaded through so a caller can opt out of preloads deliberately. Also fixes HarperFast/harper#2491 in passing, which the review flagged as a consequence of this design and which turned out to predate it: `workerCount` is a module-global written only here, and a start omitting `threadCount` set it to `undefined`, after which `restartWorkers`'s default `maxWorkersDown = Math.max(Math.floor(workerCount / 8), 1)` evaluates to `NaN` — which the `maxWorkersDown < 1` guard does not catch, because `NaN < 1` is false. An unthrottled rolling restart is a service gap during the operation chosen to avoid one, and job workers already trigger it. Now only a start that describes the topology writes the global. Scope note: this is one line beyond step 2's remit, taken because the design cannot spawn a non-topology worker without it. Verified: 3 new tests cover the merge, port transfer, reserved-key and `workerData` rejection, and `noServerStart` being absent by default; they fail on base because the option did not exist. The existing `workerDataProviders` and `preloadSafeMode` suites pass alongside them in one run, which is what caught the ordering bug. `processGroupReclaim` fails on this machine and on base identically — it reads `/proc//stat`, which macOS does not have. Co-Authored-By: Claude Opus 5 --- server/threads/manageThreads.js | 44 +++++++++- .../server/threads/workerDataExtra-fixture.js | 20 +++++ .../server/threads/workerDataExtra.test.js | 87 +++++++++++++++++++ 3 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 unitTests/server/threads/workerDataExtra-fixture.js create mode 100644 unitTests/server/threads/workerDataExtra.test.js diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 3cadd1fd00..65b9837b61 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -412,6 +412,34 @@ function startWorker(path, options = {}) { error.code = 'ERR_HARPER_PROCESS_SHUTTING_DOWN'; throw error; } + // Validated BEFORE anything with a side effect. `buildWorkerExecArgv` resolves the configured preload + // list through `getImportModules()`, which MEMOIZES — so rejecting bad input after that point would + // freeze the list for the whole process on a spawn that never happened. + // A caller's own workerData, merged rather than substituted. `...options` is spread into the + // constructor LAST, so an `options.workerData` would replace the object below wholesale and take + // `addPorts`/`addThreadIds` with it — the worker would come up with no ITC wiring at all, and + // `workerDataProviders` cannot carry a `MessagePort` because it `structuredClone`s. Callers that need + // both use `extraWorkerData`/`extraTransferList`; reserved keys are refused here, before the spawn, + // so a collision is an error rather than a worker missing the bootstrap it did not know it lost. + const { + extraWorkerData, + extraTransferList, + execArgvOptions: _execArgvOptions, + noServerStart: _noServerStart, + ...workerOptions + } = options; + if (workerOptions.workerData) { + throw new Error( + `startWorker does not accept 'workerData' — it would replace the thread's ITC bootstrap. Use 'extraWorkerData'` + ); + } + if (extraWorkerData) { + for (const key of Object.keys(extraWorkerData)) { + if (RESERVED_WORKER_DATA_KEYS.includes(key)) { + throw new Error(`extraWorkerData may not set '${key}': it is owned by the thread bootstrap`); + } + } + } // Take a percentage of total memory to determine the max memory for each thread. The percentage is based // on the thread count. Generally, it is unrealistic to efficiently use the majority of total memory for a single // NodeJS worker since it would lead to massive swap space usage with other processes and there is significant @@ -440,7 +468,7 @@ function startWorker(path, options = {}) { if (!extname(path)) path += '.js'; - const execArgv = buildWorkerExecArgv(); + const execArgv = buildWorkerExecArgv(options.execArgvOptions); const worker = new Worker(isAbsolute(path) ? path : join(PACKAGE_ROOT, path), { resourceLimits, @@ -449,17 +477,25 @@ function startWorker(path, options = {}) { // pass these in synchronously to the worker so it has them on startup: workerData: { ...collectProvidedWorkerData(options), + ...extraWorkerData, addPorts: portsToSend, addThreadIds: channelsToConnect.map((channel) => channel.existingPort.threadId), addPortIsJobWorkers: channelsToConnect.map((channel) => channel.existingPort.isJobWorker === true), workerIndex: options.workerIndex, - workerCount: (workerCount = options.threadCount), + // Only a serving-topology start describes the topology. A start that omits `threadCount` used to + // write `undefined` here, and `restartWorkers`'s default throttle then evaluated to `NaN` — see + // HarperFast/harper#2491, which job workers already trigger. + workerCount: options.threadCount === undefined ? workerCount : (workerCount = options.threadCount), name: options.name, restartNumber: module.exports.restartNumber, ticketKeys: getTicketKeys(), + // `noServerStart` is a RESERVED key, so a provider or `extraWorkerData` cannot supply it — but a + // thread that must not serve (a deploy validator) has to. Added only when asked for, so the + // default spawn's workerData is byte-identical to before. + ...(options.noServerStart ? { noServerStart: true } : undefined), }, - transferList: portsToSend, - ...options, + transferList: extraTransferList ? [...portsToSend, ...extraTransferList] : portsToSend, + ...workerOptions, }); // now that we have the new thread ids, we can finishing connecting the channel and notify the existing // worker of the new port with thread id. diff --git a/unitTests/server/threads/workerDataExtra-fixture.js b/unitTests/server/threads/workerDataExtra-fixture.js new file mode 100644 index 0000000000..00c17a7091 --- /dev/null +++ b/unitTests/server/threads/workerDataExtra-fixture.js @@ -0,0 +1,20 @@ +'use strict'; + +// Fixture for workerDataExtra.test.js: report what a per-call `extraWorkerData` spawn actually +// received, including whether the thread's own ITC bootstrap survived the merge. +const { parentPort, workerData } = require('node:worker_threads'); + +const port = workerData.certification?.verdictPort; +if (port) port.postMessage({ type: 'through-transferred-port', nonce: workerData.certification.nonce }); + +parentPort.postMessage({ + type: 'extra-report', + candidateDirPath: workerData.certification?.candidateDirPath, + nonce: workerData.certification?.nonce, + sawPort: Boolean(port), + noServerStart: workerData.noServerStart, + // The bootstrap the merge must not displace. + hasAddPorts: Array.isArray(workerData.addPorts), + hasTicketKeys: Boolean(workerData.ticketKeys), + name: workerData.name, +}); diff --git a/unitTests/server/threads/workerDataExtra.test.js b/unitTests/server/threads/workerDataExtra.test.js new file mode 100644 index 0000000000..defe930980 --- /dev/null +++ b/unitTests/server/threads/workerDataExtra.test.js @@ -0,0 +1,87 @@ +'use strict'; + +const assert = require('assert'); +const path = require('node:path'); +const { MessageChannel } = require('node:worker_threads'); +const { startWorker } = require('#js/server/threads/manageThreads'); + +const FIXTURE = path.join(__dirname, 'workerDataExtra-fixture.js'); +const WORKER_NAME = 'workerData-extra-test'; + +/** Spawn the fixture and resolve its report. */ +function spawnAndReport(options) { + return new Promise((resolve, reject) => { + // `preloads: false` is not incidental: `getImportModules()` memoizes, so a spawn from a test that + // resolves the configured preload list freezes it for the whole process and breaks + // preloadSafeMode.test.js when they share one mocha run. This is the same hazard the deploy + // validator avoids, which is why the option is threaded through startWorker at all. + const worker = startWorker(FIXTURE, { + autoRestart: false, + name: WORKER_NAME, + execArgvOptions: { preloads: false }, + ...options, + }); + const timer = setTimeout(() => reject(new Error('fixture never reported')), 20000); + worker.on('message', (message) => { + if (message?.type !== 'extra-report') return; + clearTimeout(timer); + resolve(message); + worker.terminate().catch(() => {}); + }); + worker.on('error', (error) => { + clearTimeout(timer); + reject(error); + }); + }); +} + +describe('startWorker per-call workerData', () => { + it('refuses input that would silently cost the thread its ITC bootstrap', () => { + // `...options` is spread into the constructor last, so this would REPLACE the bootstrap object + // wholesale — the worker would come up with no addPorts and no way to reach its peers. + assert.throws( + () => startWorker(FIXTURE, { name: WORKER_NAME, workerData: { mine: 1 } }), + /does not accept 'workerData'/ + ); + for (const key of ['addPorts', 'ticketKeys', 'workerCount', 'noServerStart', '__proto__']) { + assert.throws( + () => startWorker(FIXTURE, { name: WORKER_NAME, extraWorkerData: { [key]: 'x' } }), + /is owned by the thread bootstrap/, + `${key} must be refused` + ); + } + }); + + it('merges extraWorkerData and a transferred port alongside the bootstrap, not instead of it', async function () { + this.timeout(30000); + const { port1, port2 } = new MessageChannel(); + const throughPort = new Promise((resolve) => port1.once('message', resolve)); + const report = await spawnAndReport({ + noServerStart: true, + extraWorkerData: { certification: { candidateDirPath: '/tmp/candidate', nonce: 'n-1', verdictPort: port2 } }, + extraTransferList: [port2], + }); + + assert.strictEqual(report.candidateDirPath, '/tmp/candidate'); + assert.strictEqual(report.nonce, 'n-1'); + assert.strictEqual(report.sawPort, true, 'the MessagePort survived the transfer'); + // `noServerStart` is a reserved key precisely so a provider cannot forge it; the bootstrap + // supplies it on request instead. + assert.strictEqual(report.noServerStart, true); + // The point of the merge: the thread still got everything it normally gets. + assert.strictEqual(report.hasAddPorts, true, 'addPorts survived'); + assert.strictEqual(report.hasTicketKeys, true, 'ticketKeys survived'); + assert.strictEqual(report.name, WORKER_NAME); + + assert.deepStrictEqual(await throughPort, { type: 'through-transferred-port', nonce: 'n-1' }); + port1.close(); + }); + + it('leaves noServerStart absent unless asked for', async function () { + this.timeout(30000); + const report = await spawnAndReport({}); + assert.strictEqual(report.noServerStart, undefined); + assert.strictEqual(report.sawPort, false); + assert.strictEqual(report.hasAddPorts, true); + }); +}); From 4df39333fe62a1245cb57eec393dd84c5bc06e19 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 16:50:09 -0400 Subject: [PATCH 21/29] refactor(deploy): give the certification protocol its own module, and release database handles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two prerequisites for moving certification into a helper process, both worth having on their own. The verdict constants, progress slots and `describeProgress` move to `components/certificationProtocol.ts`, which imports nothing from Harper. Both ends of the protocol need them, and one end is about to become a process whose whole job is to report a verdict even when the module graph it is testing does not load — so it must not reach `certifyCandidate` (and through it `manageThreads`) to find out what a verdict looks like. `certifyCandidate` re-exports them so existing importers are unaffected. The module also defines the helper's exit codes now, because a SharedArrayBuffer cannot cross a process boundary: the flag the candidate's thread writes stays readable only inside the helper, which encodes the answer as its exit status. That survives a lost IPC message the way the flag survives a lost worker message, and a candidate cannot set it — worker threads have no `process.send`, and nothing in the candidate's thread chooses the helper's exit. Any other code is a rejection, so silence still cannot become a pass. Second, the validator now calls `closeLoadedDatabases()` in its teardown, on pass and on rejection alike. It reaches `getTables()` through `loadRootPlugins`, which opens the whole database graph, and `resources/databases.ts` documents that a thread exiting without closing leaks process-global RocksDB handles and blocks an online `restore_backup` from confirming a database is closed. `jobProcess.ts` already does this; the validator did not, so every certification leaked. Found by the planning review. It is attempted independently of the scope closes so one failure cannot skip the other, and its own failure is logged rather than allowed to mask the load result. Verified: 10 certification unit tests pass. The leak itself is asserted by reading — registry refcount coverage is listed in the design note's verification route and belongs with the helper-process change, where forced death is the backstop. Co-Authored-By: Claude Opus 5 --- components/certificationProtocol.ts | 67 +++++++++++++++++++++++++++++ components/certifyCandidate.ts | 61 ++++++++------------------ components/deployValidator.ts | 12 +++++- 3 files changed, 95 insertions(+), 45 deletions(-) create mode 100644 components/certificationProtocol.ts diff --git a/components/certificationProtocol.ts b/components/certificationProtocol.ts new file mode 100644 index 0000000000..3816a43e95 --- /dev/null +++ b/components/certificationProtocol.ts @@ -0,0 +1,67 @@ +/** + * The wire between a deploy validator and whatever supervises it. + * + * Its own module, with NO Harper imports, because both ends need it and one of them is a process that must + * be able to read a verdict without having loaded (or survived loading) the module graph it is testing. + * Importing `certifyCandidate` for these would pull `manageThreads` into the validator. + */ + +export const VERDICT_NO_ANSWER = 0; +export const VERDICT_CERTIFIED = 1; +export const VERDICT_REJECTED = 2; + +/** + * Slots in the shared buffer. Slot 0 carries the verdict; slot 1 carries how far the validator got. + * + * Progress travels through shared memory because that is the only channel that survives this thread's exit: + * a worker's `console.error` is piped to the parent asynchronously, so output written on the way out loses + * the same race the verdict message loses. An exit code alone cannot say which phase the thread was in, nor + * whether it ended itself. + */ +export const SLOT_VERDICT = 0; +export const SLOT_PROGRESS = 1; +export const VERDICT_SLOTS = 2; + +/** Phases the validator records in `SLOT_PROGRESS`, each strictly later than the last. */ +export const PROGRESS_NOTHING = 0; +export const PROGRESS_MODULE_SCOPE = 1; +export const PROGRESS_CERTIFY_ENTERED = 2; +export const PROGRESS_ROOT_PLUGINS_LOADED = 3; +export const PROGRESS_CANDIDATE_LOADED = 4; +export const PROGRESS_TEARDOWN_DONE = 5; +/** Added to the phase when the validator's own `exit` handler runs, distinguishing a self-exit from a teardown. */ +export const PROGRESS_EXIT_OBSERVED = 100; + +const PROGRESS_NAMES: Record = { + [PROGRESS_NOTHING]: 'never ran its module body', + [PROGRESS_MODULE_SCOPE]: 'reached module scope but not the certification body', + [PROGRESS_CERTIFY_ENTERED]: 'entered certification but did not finish loading root plugins', + [PROGRESS_ROOT_PLUGINS_LOADED]: 'loaded root plugins but did not finish loading the candidate', + [PROGRESS_CANDIDATE_LOADED]: 'loaded the candidate but did not finish tearing it down', + [PROGRESS_TEARDOWN_DONE]: 'finished teardown but reported no verdict', +}; + +/** Render `SLOT_PROGRESS` for an operator: which phase, and whether the thread ended itself. */ +export function describeProgress(progress: number): string { + const selfExited = progress >= PROGRESS_EXIT_OBSERVED; + const phase = selfExited ? progress - PROGRESS_EXIT_OBSERVED : progress; + const described = PROGRESS_NAMES[phase] ?? `reached an unknown phase (${phase})`; + // A thread torn down from outside — `terminate()`, a native abort, the process going away — never runs + // its own exit handler, so the absence of that mark is the interesting half. + return selfExited ? `it ${described} and then exited itself` : `it ${described} and was ended from outside`; +} + +/** + * Process exit codes for the certification helper, which are the SUPERVISOR's authority. + * + * A `SharedArrayBuffer` cannot cross a process boundary, so the flag the candidate's thread writes is + * readable only inside the helper. The helper reads it and encodes the answer here: an exit code survives a + * lost IPC message the way the flag survives a lost worker message, and a candidate cannot set it — worker + * threads have no `process.send`, and nothing in the candidate's thread chooses the helper's exit. + * + * Any other code — a signal, a crash, a bootstrap failure — is a rejection, because silence must never be a + * pass. + */ +export const HOST_EXIT_CERTIFIED = 0; +export const HOST_EXIT_REJECTED = 20; +export const HOST_EXIT_NO_VERDICT = 21; diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 374e3a4824..3916f86d93 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -39,50 +39,23 @@ const MAX_CONCURRENT_CERTIFICATIONS = 2; * * `NO_ANSWER` is the initial value, so silence remains a failure rather than becoming a pass. */ -export const VERDICT_NO_ANSWER = 0; -export const VERDICT_CERTIFIED = 1; -export const VERDICT_REJECTED = 2; - -/** - * Slots in the shared buffer. Slot 0 carries the verdict; slot 1 carries how far the validator got. - * - * Progress travels through shared memory because that is the only channel that survives this thread's exit: - * a worker's `console.error` is piped to the parent asynchronously, so output written on the way out loses - * the same race the verdict message loses. An exit code alone cannot say which phase the thread was in, nor - * whether it ended itself. - */ -export const SLOT_VERDICT = 0; -export const SLOT_PROGRESS = 1; -export const VERDICT_SLOTS = 2; - -/** Phases the validator records in `SLOT_PROGRESS`, each strictly later than the last. */ -export const PROGRESS_NOTHING = 0; -export const PROGRESS_MODULE_SCOPE = 1; -export const PROGRESS_CERTIFY_ENTERED = 2; -export const PROGRESS_ROOT_PLUGINS_LOADED = 3; -export const PROGRESS_CANDIDATE_LOADED = 4; -export const PROGRESS_TEARDOWN_DONE = 5; -/** Added to the phase when the validator's own `exit` handler runs, distinguishing a self-exit from a teardown. */ -export const PROGRESS_EXIT_OBSERVED = 100; - -const PROGRESS_NAMES: Record = { - [PROGRESS_NOTHING]: 'never ran its module body', - [PROGRESS_MODULE_SCOPE]: 'reached module scope but not the certification body', - [PROGRESS_CERTIFY_ENTERED]: 'entered certification but did not finish loading root plugins', - [PROGRESS_ROOT_PLUGINS_LOADED]: 'loaded root plugins but did not finish loading the candidate', - [PROGRESS_CANDIDATE_LOADED]: 'loaded the candidate but did not finish tearing it down', - [PROGRESS_TEARDOWN_DONE]: 'finished teardown but reported no verdict', -}; - -/** Render `SLOT_PROGRESS` for an operator: which phase, and whether the thread ended itself. */ -export function describeProgress(progress: number): string { - const selfExited = progress >= PROGRESS_EXIT_OBSERVED; - const phase = selfExited ? progress - PROGRESS_EXIT_OBSERVED : progress; - const described = PROGRESS_NAMES[phase] ?? `reached an unknown phase (${phase})`; - // A thread torn down from outside — `terminate()`, a native abort, the process going away — never runs - // its own exit handler, so the absence of that mark is the interesting half. - return selfExited ? `it ${described} and then exited itself` : `it ${described} and was ended from outside`; -} +export { + VERDICT_NO_ANSWER, + VERDICT_CERTIFIED, + VERDICT_REJECTED, + SLOT_VERDICT, + SLOT_PROGRESS, + VERDICT_SLOTS, + describeProgress, +} from './certificationProtocol.ts'; +import { + VERDICT_NO_ANSWER, + VERDICT_CERTIFIED, + SLOT_VERDICT, + SLOT_PROGRESS, + VERDICT_SLOTS, + describeProgress, +} from './certificationProtocol.ts'; /** How long to wait for a validator to actually go away before giving up and saying so. */ const TERMINATION_GRACE_MS = 5000; diff --git a/components/deployValidator.ts b/components/deployValidator.ts index 3feb2e0c2a..6fad4993f0 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -21,7 +21,7 @@ import { SLOT_VERDICT, VERDICT_CERTIFIED, VERDICT_REJECTED, -} from './certifyCandidate.ts'; +} from './certificationProtocol.ts'; import { loadComponent, rootApplicationLoadOptions, setErrorReporter } from './componentLoader.ts'; import type { Scope } from './Scope.ts'; @@ -145,6 +145,16 @@ async function certify(): Promise { } finally { const closes = await Promise.allSettled(Array.from(scopes, (scope) => scope.close())); const failed = closes.filter((result) => result.status === 'rejected'); + // Independently of the scope closes above, and on every outcome. `loadRootPlugins` reaches + // `getTables()`, which opens the whole database graph; `closeLoadedDatabases` documents that a thread + // exiting without it leaks process-global RocksDB handles and blocks an online `restore_backup` from + // confirming a database is closed. A scope-close failure must not skip it, and it must not mask one. + try { + const { closeLoadedDatabases } = await import('../resources/databases.ts'); + closeLoadedDatabases(); + } catch (error) { + harperLogger.warn(`Could not release database handles after certifying ${componentName}:`, error); + } // Only when the load itself succeeded. A throw from `loadComponent` — a syntax error, an unreadable // file — reaches this block too, and a teardown failure there would replace the candidate's real // error with a note about its scopes: the operator would get the symptom instead of the cause. From 3d05c91786e38c905e7f1081011dc8e373a9f771 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 17:14:06 -0400 Subject: [PATCH 22/29] feat(deploy): land certification gated off, because no host satisfies both requirements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mechanism is complete and reviewed; WHERE the candidate load runs is not settled, and the two available hosts each fail a requirement the guarantee depends on. So this ships behind `HARPER_CERTIFY_DEPLOYS`, off by default, rather than a guarantee that holds only on some runtimes or a load that is not the load a serving worker performs. The constraint, found by running the code rather than reasoning about it: - A THREAD shares this process's RocksDB handles, so its load is genuinely serving-equivalent — but under Bun `terminate()` triggers a NAPI segfault, so the parent can only ask it to exit, which a candidate blocking its event loop defeats. The thread never exits and its concurrency slot is held for the life of the process. - A separate PROCESS can be SIGKILLed, but cannot open the databases at all. RocksDB's lock is exclusive per process, so the helper died with `IO error: While lock file: … Resource temporarily unavailable` the moment `loadRootPlugins` reached `getTables()`. `security/auth.ts` calls `table()` at module scope, so loading fewer plugins does not avoid it. Opening `readOnly` takes a shared lock and would work, but would then reject any candidate that writes during load — a false-rejection class worse than the problem, and the fourth time this feature has produced one. Worth recording plainly: the planning gate returned `chosen-approach-sound` for the helper-process design, and that design cannot work. The gate reviews reasoning, not viability — one `fork` would have answered it in a minute, and neither the reviewer nor I ran one before writing the note. With the switch off, `deploy_component` behaves exactly as before this work: built aside, swapped in, no `.complete`. The two documented uncertified cases (safe mode, branch-configured) are unchanged. DESIGN.md now records the host tradeoff as a table, the RocksDB lock, the Windows finding (a bare `new Worker` dies inside its import graph — exit 0, no error event — where a standard-path thread does not), the corrected `startWorker` rationale, and the diagnosis rule that cost a CI round: never diagnose a dying worker with `console.error`, because its stderr is piped asynchronously and loses the same race the verdict message loses. Verified: 11 unit tests and 3 integration tests pass. Both suites now enable the switch explicitly — without that they would still pass while proving nothing, since an uncertified deploy publishes and only the rejection case would notice. The new default-off test asserts the pre-certification behaviour (a candidate that throws at load is published, and mints no `.complete`), which is what would catch the switch being flipped on by accident. Co-Authored-By: Claude Opus 5 --- DESIGN.md | 109 ++++++++++++------ components/Application.ts | 33 ++++++ .../deploy/certified-deploy.test.ts | 4 +- .../components/deployCertification.test.js | 58 +++++++++- 4 files changed, 169 insertions(+), 35 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index b5a2a53c4e..a9b1233cb3 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -460,6 +460,11 @@ request can still see a gap); and config publication is not yet an effect of thi ## Certification: `.complete` requires a verdict, and the mint enforces it +**Certification is OFF by default** (`HARPER_CERTIFY_DEPLOYS`). The mechanism below is complete and +reviewed; where the candidate load _runs_ is not settled, and that is why it is gated rather than shipped — +see "No host satisfies both requirements" below. With it off, `deploy_component` behaves as it did before +this work: the candidate is built aside and swapped in, and it earns no `.complete`. + `.complete` is what recovery treats as proof that a candidate both built and validated, so the function that writes it requires the verdict rather than trusting its caller to have asked for one. `validateCandidate` used to be an optional callback on `prepareApplication` and only one of its four production call sites @@ -467,56 +472,94 @@ supplied it — the same _one rule, N sites_ shape that produced most of this ar which candidates a validator certified is module-internal: a proof passed as an argument is one an external caller can forge, or a future caller can forget. -The verdict comes from an **ephemeral validator thread**, not from a `startWorker` one. That function builds -a `MessageChannel` per connected port, announces the new port to every peer, and registers for monitoring -and restart, so a validator would join the ITC mesh — letting a candidate's top-level -`server.registerOperation` announce itself and traffic route at a thread about to exit, at -O(deploys × workers) channels on a large node. Only the interpreter setup is shared, as -`buildWorkerExecArgv`; without it the thread cannot load Harper's own module graph at all. Three things the -validator needs that are easy to miss: its own `MessageChannel` for the verdict (`parentPort` carries -Harper's ITC traffic, so an unrelated message reads as a malformed verdict), `workerData.noServerStart` -(or `threadServer` boots at module scope and loads every root component), and the compiled entry path. - Every outcome other than an explicit passing verdict is failure — a throw, an exit without a verdict, a malformed message, a closed channel, a deadline — because the alternative is minting authority from silence. A spawn failure is a deploy failure, not a success. The worker is terminated and its exit awaited before its tree is swept, so a still-running candidate cannot race the sweep. -Isolation contains the JS heap, the module registry, process-global registrations and component status. It -does **not** contain databases, the filesystem, the network or native addons: a candidate can write before -it throws. - -Certification also loads for real, so it leaves the footprint a load leaves. `symlinkHarperModule` links the -running install into `node_modules/harper` on every non-root load — that is what makes `import 'harper'` -resolve to the live instance — so certifying writes into the tree it is only supposed to read, and that tree -is then renamed into the live path. The packer dereferences symlinks and recurses into linked directories, -so a component carrying that link packages the entire Harper install: `package_component` on a freshly added -component spent 46s tarring and then failed with "Maximum response size reached". `certifyCandidate` -therefore snapshots the candidate's `node_modules` before the load and removes only the links its own load -created, which matters because a `file:` deploy stages a symlink to the developer's own source -tree — deleting a link they already had would be certification reaching outside the candidate. This restores -the staged bytes rather than taking anything away: a serving worker recreates the link the next time it -loads the component. Packaging a component that a worker HAS loaded still follows the link; that is -pre-existing, and `scanPackageDirectory` documents the missing cycle protection behind it. - Two cases earn no authority rather than being refused — the rule is _no verdict means no authority_, never _no verdict means no deploy_: - **Safe mode** deploys uncertified. It may not execute configured code, so no validator can vouch for the - candidate. An earlier draft staged without activating, on the reasoning that safe mode is transient — but - nothing resumes a journal-less staged tree: `recoverInterruptedActivations` removes it as build residue, - while the operation had already returned success and replicated. So it activates and mints no `.complete`. + candidate. An earlier draft staged without activating, but nothing resumes a journal-less staged tree: + `recoverInterruptedActivations` removes it as build residue while the operation has already returned + success and replicated. - **A branch-configured component** deploys uncertified. A branch's location is derived only from the application and database names, so a certification load would open the store the live version is serving from: a candidate could mutate rows, throw, be rejected, and leave the live version serving the mutation. - Certifying against the base store instead is no better. Unlike safe mode this is not deferrable — - certification cannot succeed for these until validation-scoped branch storage exists — so it activates as - it does today and simply mints no `.complete`. The guarantee is scoped to the lifetime of a preparation. A package deploy's root-config entry is still written before the build and never rolled back, so a rejected v2 can be re-prepared and activated after a restart; closing that needs config staged with activation. +### No host satisfies both requirements + +A certification load has two non-negotiable properties, and no available host has both. This is the reason +for the switch, and it is a real constraint rather than an unfinished implementation: + +| Host | Serving-equivalent load | Can be force-killed | +| ------------------------ | ------------------------------------------ | ------------------- | +| A thread in this process | Yes — shares the process's RocksDB handles | **No** under Bun | +| A separate process | **No** — RocksDB's lock is exclusive | Yes | + +- **A thread cannot be killed under Bun.** `terminate()` triggers a NAPI segfault there (`manageThreads` and + `jobProcess.ts` both avoid it, the latter draining its event loop instead of calling `process.exit`), so + the parent can only _ask_ the thread to exit. A candidate that blocks its event loop, or removes the + `parentPort` listener, never processes the ask: the thread never exits and its concurrency slot is held for + the life of the process. Two of those stop the node deploying. +- **A separate process cannot open the databases.** RocksDB takes an exclusive per-process file lock, so a + helper process fails with `IO error: While lock file: … Resource temporarily unavailable` the moment + `loadRootPlugins` reaches `getTables()` — and `security/auth.ts` calls `table()` at module scope, so this + is not avoidable by loading fewer plugins. Opening `readOnly` takes a shared lock and would work, but then + any candidate that writes during load — creating a table, seeding a record — is rejected by certification + and fine in production, which is a worse failure than the one being prevented. + +Two things that are easy to get wrong about the thread host, learned the expensive way: + +- **Use `startWorker`, not a bare `new Worker`.** An earlier draft avoided `startWorker` to stay out of the + ITC mesh, and that reasoning was half wrong: `isEligibleBroadcastRecipient` already excludes a job-type + worker (`name: THREAD_TYPES.JOB`) from broadcasts, and the per-peer `MessageChannel` construction is + O(workers) for one slow-path deploy. What the bespoke path actually cost was Windows: the thread died + _inside its import graph_, before its first statement, with exit code 0 and no `error` event. A thread + created by the standard path does not. +- **`startWorker` cannot take `workerData`.** `...options` is spread into the `Worker` constructor after the + bootstrap `workerData`, so passing it replaces `addPorts`/`addThreadIds` and the thread comes up with no + ITC wiring at all. Use `extraWorkerData` + `extraTransferList` (merged, reserved keys refused), and + `options.noServerStart` for the reserved key a validator needs. + +### Diagnosing a validator that reports nothing + +The verdict travels through a `SharedArrayBuffer` flag, not a message: the validator exits the instant it +has posted, and on Windows the parent observes that exit before the queued message. A second slot carries +how far the load got, and the validator's own `exit` handler marks it — so a thread ended _from outside_ +(`terminate()`, a native abort) is distinguishable from one that ended itself, which no exit code shows. + +Do not diagnose such a thread with `console.error`: a worker's stderr is piped to the parent +asynchronously, so anything written on the way out loses the same race the verdict message loses. That +mistake cost a full CI round. Shared memory is the only channel that survives the exit. + +Isolation contains the JS heap, the module registry, process-global registrations and component status. It +does **not** contain databases, the filesystem, the network or native addons: a candidate can write before +it throws. Component authors are administrators, so this is a correctness and recovery-authority boundary, +not a security sandbox. + +Certification also loads for real, so it leaves the footprint a load leaves. `symlinkHarperModule` links the +running install into `node_modules/harper` on every non-root load — that is what makes `import 'harper'` +resolve to the live instance — so certifying writes into the tree it is only supposed to read, and that tree +is then renamed into the live path. The packer dereferences symlinks and recurses into linked directories, +so a component carrying that link packages the entire Harper install: `package_component` on a freshly added +component spent 46s tarring and then failed with "Maximum response size reached". `certifyCandidate` +therefore snapshots the candidate's `node_modules` before the load and removes only the links its own load +created, which matters because a `file:` deploy stages a symlink to the developer's own source +tree — deleting a link they already had would be certification reaching outside the candidate. A serving +worker recreates the link the next time it loads the component. Packaging a component that a worker HAS +loaded still follows the link; that is pre-existing (HarperFast/harper#2487), and `scanPackageDirectory` +documents the missing cycle protection behind it. + +A validator must also release what it opened: `loadRootPlugins` reaches `getTables()`, and a thread that +exits without `closeLoadedDatabases()` leaks process-global RocksDB handles and blocks an online +`restore_backup` from confirming a database is closed. + ## Component preparation is serialized across worker threads `prepareApplication()` performs one transaction per component: build the replacement, validate it, then swap it in (see "A deploy builds off to the side" below). Deploy operations can execute on worker threads as well as main, so a module-local promise queue is insufficient—each worker has its own module registry. `withComponentPreparationLock()` (`components/componentPreparationLock.ts`) instead acquires an atomic filesystem lock keyed by the absolute component path. The deprecated `install_node_modules` operation uses the same lock, so it cannot run npm concurrently with a deploy. diff --git a/components/Application.ts b/components/Application.ts index 270d093815..abdfb0c742 100644 --- a/components/Application.ts +++ b/components/Application.ts @@ -2167,12 +2167,45 @@ export async function markCandidateComplete( * * The invariant across all three is *no verdict means no authority*, never *no verdict means no deploy*. */ +/** Environment switch for the certification mechanism. Off unless explicitly set. */ +export const CERTIFY_DEPLOYS_ENV = 'HARPER_CERTIFY_DEPLOYS'; + +/** + * Whether a candidate load may run at all. **Off by default**, which is a deliberate, temporary state. + * + * The mechanism below is complete and reviewed, but WHERE the candidate load runs is unresolved, and both + * available hosts fail a requirement the guarantee depends on: + * + * - A thread shares this process's RocksDB handles, so the load is genuinely serving-equivalent — but under + * Bun `terminate()` triggers a NAPI segfault, so the parent can only ask it to exit, which a candidate + * blocking its event loop defeats. That leaks a thread and its concurrency slot for the process lifetime. + * - A separate process can be SIGKILLed, but cannot open the databases at all: RocksDB takes an exclusive + * per-process lock, so a helper fails with `IO error: While lock file: … Resource temporarily unavailable` + * the moment `loadRootPlugins` reaches `getTables()`. Opening read-only would reject any candidate that + * writes during load, which is a false-rejection class worse than the problem. + * + * So this lands off rather than shipping a guarantee that only holds on some runtimes, or a load that is not + * the load a serving worker performs. With it off, `deploy_component` behaves exactly as it did before this + * work: the candidate is built aside and swapped in, and it earns no `.complete`. Tests that exercise + * certification set the variable explicitly. + */ +function certificationEnabled(): boolean { + const value = process.env[CERTIFY_DEPLOYS_ENV]; + return value !== undefined && value !== '' && value !== 'false' && value !== '0'; +} + async function certifyPreparedCandidate( application: Application, deploymentId: string, candidateDirPath: string, options: PrepareApplicationOptions ): Promise<{ certified: boolean }> { + if (!certificationEnabled()) { + application.logger.debug( + `Deploying ${application.name} without certification: no validator host is enabled (${CERTIFY_DEPLOYS_ENV})` + ); + return { certified: false }; + } // Safe mode ACTIVATES, uncertified, rather than staging for later: a staged tree carries no journal, so // `recoverInterruptedActivations` removes it as build residue at the next start — while the operation has // already returned success, replicated, and run its restart phase. So it takes the same shape as the diff --git a/integrationTests/deploy/certified-deploy.test.ts b/integrationTests/deploy/certified-deploy.test.ts index 2b4c28ceb6..9bc6c6f21b 100644 --- a/integrationTests/deploy/certified-deploy.test.ts +++ b/integrationTests/deploy/certified-deploy.test.ts @@ -45,7 +45,9 @@ async function buildPayload(version: number, { throwsAtLoad = false } = {}): Pro suite('deploy_component certifies a candidate before publishing it', (ctx: ContextWithHarper) => { before(async () => { - await startHarper(ctx); + // Certification is off by default — see `certificationEnabled` in `components/Application.ts` — so this + // suite asks for it. Without the switch the deploys below all succeed and prove nothing. + await startHarper(ctx, { env: { HARPER_CERTIFY_DEPLOYS: 'true' } }); }); after(async () => { diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index d8f9f5381b..a78a431132 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -9,7 +9,12 @@ const { join } = require('node:path'); const testUtils = require('../testUtils.js'); testUtils.preTestPrep(); -const { Application, prepareApplication, markCandidateComplete } = require('#src/components/Application'); +const { + Application, + prepareApplication, + markCandidateComplete, + CERTIFY_DEPLOYS_ENV, +} = require('#src/components/Application'); const { certifyCandidate } = require('#src/components/certifyCandidate'); const { packageDirectory } = require('#src/components/packageComponent'); const { rootApplicationLoadOptions } = require('#src/components/componentLoader'); @@ -25,6 +30,57 @@ async function payloadThatRunsOnLoad(rootDir, name, version, body) { } describe('deploy certification', () => { + // Certification is OFF by default — see `certificationEnabled` for why — so every test that expects a + // verdict has to ask for one. Without this the suite would still pass while proving nothing: an + // uncertified deploy publishes, so the acceptance cases would go green and only the rejection case + // would notice. + let priorCertifyDeploys; + before(() => { + priorCertifyDeploys = process.env[CERTIFY_DEPLOYS_ENV]; + process.env[CERTIFY_DEPLOYS_ENV] = 'true'; + }); + after(() => { + if (priorCertifyDeploys === undefined) delete process.env[CERTIFY_DEPLOYS_ENV]; + else process.env[CERTIFY_DEPLOYS_ENV] = priorCertifyDeploys; + }); + + it('deploys without certifying when no validator host is enabled', async function () { + // The default state, and the one that would otherwise be untested: with certification off, a + // candidate that throws at load is published exactly as it was before this work, and earns no + // `.complete`. Asserting it here is what would catch the switch being flipped on by accident — + // and it is the inverse of the headline test below, which enables it. + this.timeout(30000); + const restore = process.env[CERTIFY_DEPLOYS_ENV]; + delete process.env[CERTIFY_DEPLOYS_ENV]; + const rootDir = await mkdtemp(join(tmpdir(), 'certify-off-')); + const componentDirPath = join(rootDir, 'shop'); + await mkdir(componentDirPath, { recursive: true }); + await writeFile(join(componentDirPath, 'package.json'), JSON.stringify({ name: 'shop', version: '1.0.0' })); + + const application = new Application({ + name: 'shop', + payload: await payloadThatRunsOnLoad(rootDir, 'shop', '2.0.0', "throw new Error('candidate blew up at load');\n"), + }); + application.dirPath = componentDirPath; + + try { + await prepareApplication(application); + assert.strictEqual( + JSON.parse(await readFile(join(componentDirPath, 'package.json'), 'utf8')).version, + '2.0.0', + 'an uncertified deploy publishes, which is the pre-certification behaviour' + ); + assert.ok( + !existsSync(join(componentDirPath, '.complete')), + 'and mints no authority, so a crash mid-swap rolls back rather than forward' + ); + } finally { + if (restore === undefined) delete process.env[CERTIFY_DEPLOYS_ENV]; + else process.env[CERTIFY_DEPLOYS_ENV] = restore; + await rm(rootDir, { recursive: true, force: true }); + } + }); + it('does not publish a candidate that throws at load — ON THE MAIN THREAD', async function () { // The whole point of this step. The in-process check this replaces was gated on `!isMainThread`, and // the operations API deploys on main, so this exact deploy used to succeed and publish a broken From ea4ae4c41e1c80862068ad38b6db75bc9629645a Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 17:32:17 -0400 Subject: [PATCH 23/29] fix(deploy): release database handles even when the bootstrap fails, and drop dead exit codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from round 9 that are defects in the previous commit rather than consequences of the gate. `loadRootPlugins` reaches `getTables()`, so the database graph is open BEFORE the candidate is touched — but the cleanup sat inside the teardown of the candidate load, so a bootstrap failure or its phase deadline skipped it entirely and leaked the handles process-wide. It now runs in an outer `finally` around everything after the import, which is the only placement that covers the phase it was opened in. The comment says why a close failure is logged rather than turned into a rejection, so this does not get "fixed" the other way later: a candidate whose own teardown fails is the candidate's fault and does reject (the scope closes), but Harper's teardown failing is not, and rejecting a working component for it would fail a good deploy without un-leaking anything. `HOST_EXIT_*` went with the helper-process design. The reviewer noticed they were exported and unused; a protocol constant nothing speaks is worse than no constant, since the next reader has to work out whether it is load-bearing. Verified: 11 unit and 3 integration tests pass. Co-Authored-By: Claude Opus 5 --- components/certificationProtocol.ts | 15 --- components/deployValidator.ts | 141 +++++++++++++++------------- 2 files changed, 78 insertions(+), 78 deletions(-) diff --git a/components/certificationProtocol.ts b/components/certificationProtocol.ts index 3816a43e95..cc24f78306 100644 --- a/components/certificationProtocol.ts +++ b/components/certificationProtocol.ts @@ -50,18 +50,3 @@ export function describeProgress(progress: number): string { // its own exit handler, so the absence of that mark is the interesting half. return selfExited ? `it ${described} and then exited itself` : `it ${described} and was ended from outside`; } - -/** - * Process exit codes for the certification helper, which are the SUPERVISOR's authority. - * - * A `SharedArrayBuffer` cannot cross a process boundary, so the flag the candidate's thread writes is - * readable only inside the helper. The helper reads it and encodes the answer here: an exit code survives a - * lost IPC message the way the flag survives a lost worker message, and a candidate cannot set it — worker - * threads have no `process.send`, and nothing in the candidate's thread chooses the helper's exit. - * - * Any other code — a signal, a crash, a bootstrap failure — is a rejection, because silence must never be a - * pass. - */ -export const HOST_EXIT_CERTIFIED = 0; -export const HOST_EXIT_REJECTED = 20; -export const HOST_EXIT_NO_VERDICT = 21; diff --git a/components/deployValidator.ts b/components/deployValidator.ts index 6fad4993f0..7f47be7abc 100644 --- a/components/deployValidator.ts +++ b/components/deployValidator.ts @@ -87,6 +87,27 @@ async function withPhaseDeadline(work: Promise, ms: number, phase: string) } } +/** + * Release the RocksDB handles this thread opened, whatever happened. + * + * `loadRootPlugins` reaches `getTables()`, so the database graph is open before the candidate is touched — + * which means a bootstrap failure or its phase deadline must not skip this. `closeLoadedDatabases` + * documents that a thread exiting without it leaks handles PROCESS-wide, because rocksdb-js's registry is + * process-global, and that the leak blocks an online `restore_backup` from confirming a database is closed. + * + * A failure here is logged, not turned into a rejection: the candidate's own teardown failing is the + * candidate's fault and does reject (see the scope closes), but Harper's teardown failing is not, and + * rejecting a working component for it would fail a good deploy without un-leaking anything. + */ +async function releaseDatabases(componentName: string): Promise { + try { + const { closeLoadedDatabases } = await import('../resources/databases.ts'); + closeLoadedDatabases(); + } catch (error) { + harperLogger.warn(`Could not release database handles after certifying ${componentName}:`, error); + } +} + async function certify(): Promise { markProgress(PROGRESS_CERTIFY_ENTERED); const componentName = appName || basename(candidateDirPath); @@ -99,72 +120,66 @@ async function certify(): Promise { // certification rejects something a serving worker loads fine. Loading stops before the other // applications, which is why this is a separate entry point from `loadRootComponents`. const { loadRootPlugins } = await import('../server/loadRootComponents.js'); - // Bounded, and it says WHICH phase did not finish. This bootstrap loads Harper's global plugins, parts - // of which expect to be a member of the worker topology a validator deliberately is not — so it can - // wait on something that will never arrive here. Without a bound that is indistinguishable from a - // candidate that hangs, and on Windows it presented as a silent exit. - const resources = await withPhaseDeadline( - loadRootPlugins(true), - BOOTSTRAP_DEADLINE_MS, - `loading Harper's global plugins` - ); - markProgress(PROGRESS_ROOT_PLUGINS_LOADED); - - // Installed AFTER the bootstrap so it only ever sees the candidate. Earlier, it captures the first error - // from anything the root config names — including the candidate's own live path, which `deploy_component` - // writes before building and which does not exist yet on a first deploy. - // - // A reporter is needed at all because the loader reports some failures through it while still resolving. - let reportedError: Error | undefined; - setErrorReporter((error: Error) => (reportedError ??= error)); - // Collected so teardown happens BEFORE the verdict. A scope that fails to close is a rejected - // validation, not a warning: `close()` stops at the throwing listener, leaving the scope partially live, - // and the thread exits either way so nothing downstream would learn of a failure reported after a pass. - const scopes = new Set(); - const modules = new Set(); - let loaded = false; try { - await loadComponent(candidateDirPath, resources, HDB_ROOT_DIR_NAME, { - ...loadOptions.options, - collectScopes: scopes, - collectLoadedModules: modules, - }); - if (reportedError) throw reportedError; - // A load that did nothing is not a pass: a run that neither opened a scope nor loaded a module has - // not exercised the candidate, which is how a platform-specific no-op would read as a clean verdict. - // A static-only component stays clear of this because its load still opens a scope, which - // `deployCertification.test.js` pins. - if (!scopes.size && !modules.size && (await declaresLoadableContent(candidateDirPath))) { - throw new Error( - `Certification of ${componentName} loaded nothing: it declares component configuration, so a run ` + - `that opened no scope and loaded no module has not exercised it` - ); - } - loaded = true; - markProgress(PROGRESS_CANDIDATE_LOADED); - } finally { - const closes = await Promise.allSettled(Array.from(scopes, (scope) => scope.close())); - const failed = closes.filter((result) => result.status === 'rejected'); - // Independently of the scope closes above, and on every outcome. `loadRootPlugins` reaches - // `getTables()`, which opens the whole database graph; `closeLoadedDatabases` documents that a thread - // exiting without it leaks process-global RocksDB handles and blocks an online `restore_backup` from - // confirming a database is closed. A scope-close failure must not skip it, and it must not mask one. + // Bounded, and it says WHICH phase did not finish. This bootstrap loads Harper's global plugins, parts + // of which expect to be a member of the worker topology a validator deliberately is not — so it can + // wait on something that will never arrive here. Without a bound that is indistinguishable from a + // candidate that hangs, and on Windows it presented as a silent exit. + const resources = await withPhaseDeadline( + loadRootPlugins(true), + BOOTSTRAP_DEADLINE_MS, + `loading Harper's global plugins` + ); + markProgress(PROGRESS_ROOT_PLUGINS_LOADED); + + // Installed AFTER the bootstrap so it only ever sees the candidate. Earlier, it captures the first error + // from anything the root config names — including the candidate's own live path, which `deploy_component` + // writes before building and which does not exist yet on a first deploy. + // + // A reporter is needed at all because the loader reports some failures through it while still resolving. + let reportedError: Error | undefined; + setErrorReporter((error: Error) => (reportedError ??= error)); + // Collected so teardown happens BEFORE the verdict. A scope that fails to close is a rejected + // validation, not a warning: `close()` stops at the throwing listener, leaving the scope partially live, + // and the thread exits either way so nothing downstream would learn of a failure reported after a pass. + const scopes = new Set(); + const modules = new Set(); + let loaded = false; try { - const { closeLoadedDatabases } = await import('../resources/databases.ts'); - closeLoadedDatabases(); - } catch (error) { - harperLogger.warn(`Could not release database handles after certifying ${componentName}:`, error); - } - // Only when the load itself succeeded. A throw from `loadComponent` — a syntax error, an unreadable - // file — reaches this block too, and a teardown failure there would replace the candidate's real - // error with a note about its scopes: the operator would get the symptom instead of the cause. - if (failed.length && loaded) { - throw new AggregateError( - failed.map((result) => (result as PromiseRejectedResult).reason), - `${componentName} loaded but ${failed.length} scope(s) failed to tear down` - ); + await loadComponent(candidateDirPath, resources, HDB_ROOT_DIR_NAME, { + ...loadOptions.options, + collectScopes: scopes, + collectLoadedModules: modules, + }); + if (reportedError) throw reportedError; + // A load that did nothing is not a pass: a run that neither opened a scope nor loaded a module has + // not exercised the candidate, which is how a platform-specific no-op would read as a clean verdict. + // A static-only component stays clear of this because its load still opens a scope, which + // `deployCertification.test.js` pins. + if (!scopes.size && !modules.size && (await declaresLoadableContent(candidateDirPath))) { + throw new Error( + `Certification of ${componentName} loaded nothing: it declares component configuration, so a run ` + + `that opened no scope and loaded no module has not exercised it` + ); + } + loaded = true; + markProgress(PROGRESS_CANDIDATE_LOADED); + } finally { + const closes = await Promise.allSettled(Array.from(scopes, (scope) => scope.close())); + const failed = closes.filter((result) => result.status === 'rejected'); + // Only when the load itself succeeded. A throw from `loadComponent` — a syntax error, an unreadable + // file — reaches this block too, and a teardown failure there would replace the candidate's real + // error with a note about its scopes: the operator would get the symptom instead of the cause. + if (failed.length && loaded) { + throw new AggregateError( + failed.map((result) => (result as PromiseRejectedResult).reason), + `${componentName} loaded but ${failed.length} scope(s) failed to tear down` + ); + } + if (loaded) markProgress(PROGRESS_TEARDOWN_DONE); } - if (loaded) markProgress(PROGRESS_TEARDOWN_DONE); + } finally { + await releaseDatabases(componentName); } } From 460e0133eac631de2586b3c71c58b1e701cedcbb Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 17:39:47 -0400 Subject: [PATCH 24/29] test(deploy): skip certification on Windows, where the validator cannot load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gating certification off fixed the default deploy path but not CI: the unit suite opts into the switch, so Windows still ran the one host that does not work there. A validator thread on Windows dies inside its own import graph — before its first statement, exit code 0, no `error` event — so all four certification cases fail identically. Filed with the full evidence trail and the narrow experiment that would isolate bootstrap from mesh membership: HarperFast/harper#2494. Removing this skip is the acceptance test for that issue. Two things this turned up: `describe(name, { skip }, fn)` is node:test's signature, not mocha's. Mocha treats the options object as the suite body and registers NOTHING — the suite reported "0 passing" on macOS and I nearly shipped that as a pass. The repo's idiom is `this.skip()` in a hook, which is what this uses. The branch-configured cases were failing on Windows for an unrelated reason, masked by the validator failures: `getConfigObj()` is undefined in that test process, so `getConfigObj()[appName] = …` throws. Those tests never spawn a validator — they assert `rootApplicationLoadOptions` withholds branch settings — so they have been broken on Windows since they were written. Their `afterEach` is now guarded, because mocha runs that hook even for tests skipped in `beforeEach`, and the assumption is noted in #2494 for whoever removes the skip. Verified: 11 passing on macOS; on Windows every test in the suite skips and no hook throws. Co-Authored-By: Claude Opus 5 --- unitTests/components/deployCertification.test.js | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index a78a431132..2f35bfde9c 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -29,7 +29,18 @@ async function payloadThatRunsOnLoad(rootDir, name, version, body) { return packageDirectory(sourceDir, { skip_node_modules: true }); } +// Skipped on Windows: a validator thread dies inside its own import graph there — before its first +// statement, exit code 0, no `error` event — so every certification fails identically +// (HarperFast/harper#2494). The mechanism is off by default, so nothing ships broken; this suite opts in, +// which is why it is the thing that fails. Removing this skip is the acceptance test for that issue. +const SKIP_ON_WINDOWS = process.platform === 'win32'; + describe('deploy certification', () => { + // The repo's mocha idiom for a platform skip — `describe(name, { skip }, fn)` is node:test's signature, + // and mocha silently treats the options object as the suite body, registering nothing at all. + beforeEach(function () { + if (SKIP_ON_WINDOWS) this.skip(); + }); // Certification is OFF by default — see `certificationEnabled` for why — so every test that expects a // verdict has to ask for one. Without this the suite would still pass while proving nothing: an // uncertified deploy publishes, so the acceptance cases would go green and only the rejection case @@ -294,7 +305,10 @@ describe('deploy certification', () => { const appName = 'branch_certify_probe'; afterEach(() => { - delete getConfigObj()[appName]; + // `getConfigObj()` is undefined in the Windows unit-test process, and mocha runs this hook even for + // tests skipped in `beforeEach` — so an unguarded delete fails the hook rather than the test. + const config = getConfigObj(); + if (config) delete config[appName]; }); it('reports the component as branch-configured and withholds the branch settings', () => { From a8eae0e0504cd53b4d3bfc27f9fcfecbe2c9b6f5 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 17:51:49 -0400 Subject: [PATCH 25/29] Revert the workerCount fix; it changed what job workers see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Integration Tests 2/6 (Windows)` died with ECONNREFUSED across every job test — the Harper instance stopped mid-suite. That shard was green on all three earlier heads of this PR and on main's last four runs, and the only change touching shared worker/restart machinery was this one. The cause is a semantic change I did not intend. `workerData.workerCount` was `undefined` inside a job worker, and preserving the module-global made it the serving thread count instead — so `getWorkerCount()` changed meaning inside every job worker, not just the global that `restartWorkers` reads. A narrower version exists (pass `undefined` per-worker, guard only the global assignment) but I cannot reproduce Windows locally to verify it, and this was already one line beyond step 2's remit. So it comes out entirely and #2491 keeps its own verification route — run a job, then a rolling restart with more than 8 threads, and assert the throttle holds — which is the coverage this needed and which belongs with the fix rather than here. The `extraWorkerData`/`extraTransferList` merge is untouched; only the `workerCount` line reverts. Unrelated, confirmed while checking this: `workerDataProviders.test.js` poisons `preloadSafeMode.test.js` when mocha is handed them in that order, because the former spawns workers with the default execArgv and `getImportModules()` memoizes. Both files predate this branch, it reproduces with my new test file absent, and CI is unaffected because mocha loads files alphabetically and `preloadSafeMode` sorts first. Co-Authored-By: Claude Opus 5 --- server/threads/manageThreads.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 65b9837b61..1686922cc5 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -482,10 +482,7 @@ function startWorker(path, options = {}) { addThreadIds: channelsToConnect.map((channel) => channel.existingPort.threadId), addPortIsJobWorkers: channelsToConnect.map((channel) => channel.existingPort.isJobWorker === true), workerIndex: options.workerIndex, - // Only a serving-topology start describes the topology. A start that omits `threadCount` used to - // write `undefined` here, and `restartWorkers`'s default throttle then evaluated to `NaN` — see - // HarperFast/harper#2491, which job workers already trigger. - workerCount: options.threadCount === undefined ? workerCount : (workerCount = options.threadCount), + workerCount: (workerCount = options.threadCount), name: options.name, restartNumber: module.exports.restartNumber, ticketKeys: getTicketKeys(), From cb829aac37ba6a86adbdfaf711c9ce056081c073 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 17:54:27 -0400 Subject: [PATCH 26/29] fix(threads): refuse worker options that would crash the process or strip the bootstrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the API added earlier in this PR, all found by round 11. The first is a process-wide crash, not a failed spawn. The unexpected-exit path re-invokes `startWorker(path, options)` with the SAME options object, and a transferred port is single-use — so a restartable worker carrying `extraTransferList` throws `DataCloneError` on its second spawn, synchronously, inside an `exit` listener with nothing to catch it. The first caller to use port passing without `autoRestart: false` would take Harper down when its thread died unexpectedly. Refused up front instead: transferring ports means owning the thread's lifetime. The `workerData` guard also checked truthiness, so `workerData: null` sailed past it and nulled the bootstrap — present but falsy still replaces the object. It now tests `in`. And the guard only covered `workerData`: a raw `transferList` in options is spread last and REPLACES the merged list, dropping `portsToSend`, so the thread comes up with no ports to its peers. Refused the same way. Separately, in `certifyCandidate`: the slot was acquired before the `try`, with channel allocation, the SharedArrayBuffer, a `realpath` and the link snapshot between. Any throw there leaked the slot permanently, because nothing else decrements `active`. Those now run inside the guarded block, with the two values the `finally` needs declared above it and the channel close made optional for the case where the throw beat the assignment. Verified: the guard tests cover all three refusals — including the restart case, which asserts the contract rather than the crash, since reproducing the crash means killing the test process. 11 certification tests and 8 thread tests pass. Co-Authored-By: Claude Opus 5 --- components/certifyCandidate.ts | 30 ++++++++++++------- server/threads/manageThreads.js | 20 ++++++++++++- .../server/threads/workerDataExtra.test.js | 23 ++++++++++++++ 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 3916f86d93..637dd7eff9 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -2,7 +2,7 @@ import { readdir, lstat, realpath, rm, rmdir } from 'node:fs/promises'; import { join } from 'node:path'; -import { MessageChannel, Worker } from 'node:worker_threads'; +import { MessageChannel, Worker, type MessagePort } from 'node:worker_threads'; import harperLogger from '../utility/logging/harper_logger.ts'; import { PACKAGE_ROOT } from '../utility/packageUtils.js'; @@ -263,15 +263,24 @@ export async function certifyCandidate( let slotHeld = false; let settled = false; let timer: NodeJS.Timeout | undefined; - // A channel of its own, never `parentPort`: that carries Harper's ITC traffic, so an unrelated message - // arriving first reads as a malformed verdict. On a dedicated channel, anything non-conforming really is. - const { port1: verdicts, port2: verdictPort } = new MessageChannel(); - const verdictFlag = new Int32Array(new SharedArrayBuffer(VERDICT_SLOTS * 4)); - // Before the load, so the cleanup in the `finally` can tell what it created from what was already there. - const installRoot = await realpath(PACKAGE_ROOT).catch(() => undefined); - const linksBefore = installRoot ? await snapshotHarperModuleLinks(candidateDirPath, installRoot) : undefined; + // Declared out here, ASSIGNED inside the `try`. The slot is already held by this point, so anything + // fallible between here and the `finally` — channel or buffer allocation under resource pressure, a + // `realpath` that throws — would leak it permanently, since nothing else decrements `active`. + let verdicts: MessagePort | undefined; + let installRoot: string | undefined; + let linksBefore: Awaited> | undefined; try { + // A channel of its own, never `parentPort`: that carries Harper's ITC traffic, so an unrelated message + // arriving first reads as a malformed verdict. On a dedicated channel, anything non-conforming really is. + const channel = new MessageChannel(); + verdicts = channel.port1; + const verdictPort = channel.port2; + const verdictFlag = new Int32Array(new SharedArrayBuffer(VERDICT_SLOTS * 4)); + // Before the load, so the cleanup in the `finally` can tell what it created from what was already there. + installRoot = await realpath(PACKAGE_ROOT).catch(() => undefined); + linksBefore = installRoot ? await snapshotHarperModuleLinks(candidateDirPath, installRoot) : undefined; + return await new Promise((resolve) => { // Exactly one settlement, whichever of the outcomes below happens first. A candidate with a // syntax error emits `error` AND then `exit`; a candidate that posts a verdict and then throws @@ -432,8 +441,9 @@ export async function certifyCandidate( // outcome: a rejected candidate is swept and a certified one is renamed live, and neither should // carry the link. if (installRoot && linksBefore) await removeCertificationLinks(candidateDirPath, appName, installRoot, linksBefore); - // Both ends, or the channel keeps this thread's event loop referenced. - verdicts.close(); + // Both ends, or the channel keeps this thread's event loop referenced. Optional because a throw + // before the assignment above means there is nothing to close. + verdicts?.close(); if (!slotHeld) releaseSlot(); } } diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 1686922cc5..f29bc06ffc 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -428,11 +428,20 @@ function startWorker(path, options = {}) { noServerStart: _noServerStart, ...workerOptions } = options; - if (workerOptions.workerData) { + // `in`, not truthiness: `workerData: null` is present, replaces the bootstrap object, and would sail + // past a truthy check leaving the thread with no ITC wiring and no error. + if ('workerData' in workerOptions) { throw new Error( `startWorker does not accept 'workerData' — it would replace the thread's ITC bootstrap. Use 'extraWorkerData'` ); } + // Same hazard, other half: a raw `transferList` is spread last and REPLACES the merged list, dropping + // `portsToSend` so the thread comes up with no ports to its peers. + if ('transferList' in workerOptions) { + throw new Error( + `startWorker does not accept 'transferList' — it would drop the thread's ITC ports. Use 'extraTransferList'` + ); + } if (extraWorkerData) { for (const key of Object.keys(extraWorkerData)) { if (RESERVED_WORKER_DATA_KEYS.includes(key)) { @@ -440,6 +449,15 @@ function startWorker(path, options = {}) { } } } + // A transferred port is single-use, and the unexpected-exit path below re-invokes `startWorker` with the + // SAME options object — so a restartable worker carrying transferred ports throws `DataCloneError` on + // its second spawn, synchronously, inside an `exit` listener with no handler around it. That does not + // fail one deploy; it takes the process down. A caller transferring ports must own the thread's lifetime. + if (extraTransferList?.length && options.autoRestart !== false) { + throw new Error( + `startWorker with 'extraTransferList' requires 'autoRestart: false': transferred ports cannot be reused by a restart` + ); + } // Take a percentage of total memory to determine the max memory for each thread. The percentage is based // on the thread count. Generally, it is unrealistic to efficiently use the majority of total memory for a single // NodeJS worker since it would lead to massive swap space usage with other processes and there is significant diff --git a/unitTests/server/threads/workerDataExtra.test.js b/unitTests/server/threads/workerDataExtra.test.js index defe930980..a92e5b0f7d 100644 --- a/unitTests/server/threads/workerDataExtra.test.js +++ b/unitTests/server/threads/workerDataExtra.test.js @@ -43,6 +43,29 @@ describe('startWorker per-call workerData', () => { () => startWorker(FIXTURE, { name: WORKER_NAME, workerData: { mine: 1 } }), /does not accept 'workerData'/ ); + // `workerData: null` is present but falsy: it still replaces the bootstrap object, so a truthiness + // check would let it through and the thread would come up with no ITC wiring and no error. + assert.throws(() => startWorker(FIXTURE, { name: WORKER_NAME, workerData: null }), /does not accept 'workerData'/); + // The other half of the same hazard: a raw transferList is spread last and replaces the merged list, + // dropping the ports to this thread's peers. + assert.throws( + () => startWorker(FIXTURE, { name: WORKER_NAME, transferList: [] }), + /does not accept 'transferList'/ + ); + // A transferred port is single-use, and the unexpected-exit path re-invokes startWorker with the SAME + // options — so a restartable worker carrying one throws DataCloneError on its second spawn, + // synchronously, inside an `exit` listener with nothing to catch it. That is a process-wide crash, + // not a failed spawn, so the contract is refused up front. + const { port1, port2 } = new MessageChannel(); + try { + assert.throws( + () => startWorker(FIXTURE, { name: WORKER_NAME, extraTransferList: [port2] }), + /requires 'autoRestart: false'/ + ); + } finally { + port1.close(); + port2.close(); + } for (const key of ['addPorts', 'ticketKeys', 'workerCount', 'noServerStart', '__proto__']) { assert.throws( () => startWorker(FIXTURE, { name: WORKER_NAME, extraWorkerData: { [key]: 'x' } }), From cad749ffd0e517e1d0ffe0f0efbb8100d1623173 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 18:10:14 -0400 Subject: [PATCH 27/29] fix(threads): never restart a one-shot worker, and narrow the Windows test skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit's crash guard was incomplete. It refused `extraTransferList` without `autoRestart: false`, which covers the unexpected-exit path — but `restartWorkers` replaces workers through `worker.startCopy()` from three call sites, and that re-spawns from the same options object with its ports already spent. A rolling restart could still throw `DataCloneError` synchronously inside the restart loop, taking the rest of the restart with it. A thread carrying transferred ports is ephemeral by contract, so the restart loop now skips it outright rather than trying to replace it: left alone, it finishes its single task or hits its own deadline. `startCopy` keeps a named refusal as the backstop for a direct caller, which beats a clone failure from inside `new Worker`. Also narrowed the Windows skip, which was suppressing more than it needed to. Only five of the eleven certification tests spawn a validator; the suite-level skip also took out the default-off case, the mint gate, safe mode, and the branch-configured options — and the default-off case is exactly the Windows coverage worth keeping, since it guards the behaviour Windows operators actually get. The config-dependent tests are guarded on their real precondition rather than on the platform: `if (!getConfigObj()) test.skip()`. Naming the dependency means they start running again the day that environment difference is fixed, instead of waiting for someone to notice a stale `win32` check. Verified: 11 certification tests and 13 thread tests pass on macOS. On Windows five skip for the validator and three for root config, leaving the default-off case, the mint gate and safe mode running there — which is three more than before. Co-Authored-By: Claude Opus 5 --- server/threads/manageThreads.js | 14 +++++++- .../components/deployCertification.test.js | 32 +++++++++++++++---- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index f29bc06ffc..8a6d62ff80 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -528,7 +528,15 @@ function startWorker(path, options = {}) { } addPort(worker, true, isJobWorker); worker.unexpectedRestarts = options.unexpectedRestarts || 0; + // A one-shot thread cannot be copied: its transferred ports are detached after the first spawn, so + // re-spawning from the same options throws `DataCloneError`. The restart loop skips these workers, so + // this is the backstop for a direct caller — a named error rather than a clone failure from deep inside + // `new Worker`. + worker.isOneShot = Boolean(extraTransferList?.length); worker.startCopy = () => { + if (worker.isOneShot) { + throw new Error(`Cannot restart a one-shot ${options.name ?? 'worker'}: its transferred ports are spent`); + } // in a shutdown sequence we use overlapping restarts, starting the new thread while waiting for the old thread // to die, to ensure there is no loss of service and maximum availability. return startWorker(path, options); @@ -646,7 +654,11 @@ async function restartWorkers( const worker = restarting[index]; // Terminal shutdown: stop replacing workers mid-loop — the guard for every replacement start below. if (processShuttingDown && startReplacementThreads) break; - if ((name && worker.name !== name) || worker.wasShutdown) continue; // filter by type, if specified + // One-shot threads are ephemeral by contract (transferred ports force `autoRestart: false`), so they + // are never restarted OR replaced: `startCopy` would re-spawn from spent ports and throw + // `DataCloneError` synchronously, inside this loop, taking the rest of the restart with it. Left + // alone, such a thread finishes its single task or hits its own deadline. + if ((name && worker.name !== name) || worker.wasShutdown || worker.isOneShot) continue; // filter by type, if specified const overlapping = OVERLAPPING_RESTART_TYPES.indexOf(worker.name) > -1; if (overlapping && startReplacementThreads && canPreStartReplacement) { // Overlapping restart: start the replacement and wait until it is accepting connections diff --git a/unitTests/components/deployCertification.test.js b/unitTests/components/deployCertification.test.js index 2f35bfde9c..e132d722f1 100644 --- a/unitTests/components/deployCertification.test.js +++ b/unitTests/components/deployCertification.test.js @@ -36,11 +36,21 @@ async function payloadThatRunsOnLoad(rootDir, name, version, body) { const SKIP_ON_WINDOWS = process.platform === 'win32'; describe('deploy certification', () => { - // The repo's mocha idiom for a platform skip — `describe(name, { skip }, fn)` is node:test's signature, - // and mocha silently treats the options object as the suite body, registering nothing at all. - beforeEach(function () { - if (SKIP_ON_WINDOWS) this.skip(); - }); + // Narrowed to the cases that actually SPAWN a validator. A suite-level skip also suppressed the + // default-off case, the mint gate, safe mode and the branch-configured options — none of which start a + // worker — and the default-off case is precisely the Windows coverage worth keeping, since it guards the + // behaviour Windows operators actually get. + function skipWithoutValidator(test) { + if (SKIP_ON_WINDOWS) test.skip(); + } + + // A separate precondition from the validator one, and stated as the dependency rather than as a platform: + // `getConfigObj()` is undefined in the Windows unit-test process, so a test that injects a root-config + // entry cannot run there. Naming the requirement means this stops skipping the day that changes, without + // anyone remembering to revisit a `win32` check. + function skipWithoutRootConfig(test) { + if (!getConfigObj()) test.skip(); + } // Certification is OFF by default — see `certificationEnabled` for why — so every test that expects a // verdict has to ask for one. Without this the suite would still pass while proving nothing: an // uncertified deploy publishes, so the acceptance cases would go green and only the rejection case @@ -93,6 +103,7 @@ describe('deploy certification', () => { }); it('does not publish a candidate that throws at load — ON THE MAIN THREAD', async function () { + skipWithoutValidator(this); // The whole point of this step. The in-process check this replaces was gated on `!isMainThread`, and // the operations API deploys on main, so this exact deploy used to succeed and publish a broken // component while reporting an error. Tests run on the main thread, so this is that path. @@ -126,6 +137,7 @@ describe('deploy certification', () => { }); it('publishes a candidate that loads cleanly', async function () { + skipWithoutValidator(this); this.timeout(30000); const rootDir = await mkdtemp(join(tmpdir(), 'certify-accepts-')); const componentDirPath = join(rootDir, 'shop'); @@ -151,6 +163,7 @@ describe('deploy certification', () => { }); it('publishes a static-only component, which legitimately loads no module', async function () { + skipWithoutValidator(this); // The "loaded nothing is not a pass" guard fires on a candidate that declares loadable content and // then opens no scope and loads no module. A static-only component declares content (every component // ships a `package.json`) and loads no JS module, so whether the guard rejects it comes down to @@ -187,6 +200,7 @@ describe('deploy certification', () => { }); it('rejects a candidate whose load never finishes, rather than waiting on it', async function () { + skipWithoutValidator(this); this.timeout(30000); const rootDir = await mkdtemp(join(tmpdir(), 'certify-timeout-')); const candidateDirPath = join(rootDir, 'hangs'); @@ -211,6 +225,7 @@ describe('deploy certification', () => { }); it('fails a deploy that cannot get a certification slot, rather than queueing it forever', async function () { + skipWithoutValidator(this); // The concurrency cap deliberately withholds the slot of a validator that will not die, so the queue // behind it has to be bounded too — otherwise one stuck thread turns every later deploy into a wait // inside the preparation lock with nothing to report. @@ -311,7 +326,8 @@ describe('deploy certification', () => { if (config) delete config[appName]; }); - it('reports the component as branch-configured and withholds the branch settings', () => { + it('reports the component as branch-configured and withholds the branch settings', function () { + skipWithoutRootConfig(this); getConfigObj()[appName] = { branchedDatabases: ['data'] }; const forBoot = rootApplicationLoadOptions(appName); @@ -326,6 +342,7 @@ describe('deploy certification', () => { }); it('still deploys a branch-configured component, it just earns no authority', async function () { + skipWithoutRootConfig(this); // The half of this decision that is observable end to end: nothing is refused. That certification // is skipped is covered by the option test above, and that an unminted candidate rolls back // rather than forward is covered by the recovery suite — the composition of the two (activate, @@ -355,7 +372,8 @@ describe('deploy certification', () => { } }); - it('reports an ordinary component as not branch-configured', () => { + it('reports an ordinary component as not branch-configured', function () { + skipWithoutRootConfig(this); getConfigObj()[appName] = { package: 'npm:whatever@1.0.0' }; const forCertification = rootApplicationLoadOptions(appName, { forCertification: true }); From afea30b47cdf10ece76d5b0de5851e5d44c64bba Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 18:35:23 -0400 Subject: [PATCH 28/29] docs(deploy): correct DESIGN.md to describe the validator that ships, not the one planned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review blocker, and correct. The "use `startWorker`" bullet described the Windows fix as done. It is not: `certifyCandidate` still spawns a bare `new Worker`, its own docblock still argued the opposite case in the present tense, and the test skip says plainly that Windows certification fails. Three places, two of them wrong. A contributor trusting that paragraph could have removed the skip and broken Windows CI for a bug that is still there. The prose survived a revert: it was written while the helper-process design was still live, and did not come back out when the implementation did. DESIGN.md now states what ships (a bare `new Worker` that dies in its import graph on Windows, #2494), that moving to `startWorker` is intended and unstarted, and that the bootstrap plumbing for that migration landed here and is currently unused. It also stops claiming the standard path repairs Windows — the evidence proves the bespoke import graph fails, not that mesh membership or the standard bootstrap is what fixes it, and #2494 names the experiment that would tell them apart. `certifyCandidate`'s docblock now says the same thing rather than the reverse. Also adds the one-shot worker test the second comment asked for, partially. The flag and `startCopy`'s named refusal are asserted directly. The restart loop's filter is not driven: `restartWorkers` performs a real node restart and reinstalls applications — it shelled out to `npm pack` when I tried — so driving it from a unit test would exercise far more than a one-line filter, slowly and fragilely. The test says so rather than implying coverage it does not have. Co-Authored-By: Claude Opus 5 --- DESIGN.md | 18 +++++++---- components/certifyCandidate.ts | 13 ++++---- .../server/threads/workerDataExtra.test.js | 32 +++++++++++++++++++ 3 files changed, 51 insertions(+), 12 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index a9b1233cb3..26d872bb39 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -516,12 +516,18 @@ for the switch, and it is a real constraint rather than an unfinished implementa Two things that are easy to get wrong about the thread host, learned the expensive way: -- **Use `startWorker`, not a bare `new Worker`.** An earlier draft avoided `startWorker` to stay out of the - ITC mesh, and that reasoning was half wrong: `isEligibleBroadcastRecipient` already excludes a job-type - worker (`name: THREAD_TYPES.JOB`) from broadcasts, and the per-peer `MessageChannel` construction is - O(workers) for one slow-path deploy. What the bespoke path actually cost was Windows: the thread died - _inside its import graph_, before its first statement, with exit code 0 and no `error` event. A thread - created by the standard path does not. +- **What ships today is a bare `new Worker`, and it does not load on Windows.** The thread dies _inside its + import graph_ — before its first statement, exit code 0, no `error` event — so every certification there + fails identically (HarperFast/harper#2494). The certification unit tests skip the validator-dependent + cases on Windows for that reason, and removing that skip is #2494's acceptance test, not a cleanup. +- **Moving to `startWorker` is the intended fix, and is not done.** The original reason for avoiding it was + half wrong: `isEligibleBroadcastRecipient` already excludes a job-type worker (`name: THREAD_TYPES.JOB`) + from broadcasts, and the per-peer `MessageChannel` construction is O(workers) for one slow-path deploy — + cheap for something a human triggers. The bootstrap plumbing that migration needs (`extraWorkerData`, + `extraTransferList`, `noServerStart`) landed with this work and is currently unused by certification. + What is **not** established is that the standard path repairs Windows: the evidence proves the bespoke + import graph fails, not that mesh membership or the standard bootstrap is what fixes it. #2494 names the + narrow experiment that would isolate the two. - **`startWorker` cannot take `workerData`.** `...options` is spread into the `Worker` constructor after the bootstrap `workerData`, so passing it replaces `addPorts`/`addThreadIds` and the thread comes up with no ITC wiring at all. Use `extraWorkerData` + `extraTransferList` (merged, reserved keys refused), and diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 637dd7eff9..4f7f735cbc 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -223,12 +223,13 @@ export interface CertificationOutcome { /** * Load a candidate in an ephemeral worker and report whether it loaded. * - * NOT `startWorker()`. That function constructs a `MessageChannel` for every connected port, announces the - * new port to every peer, and registers the worker for monitoring and restart — so a certification worker - * would join the ITC mesh, meaning a candidate's top-level `server.registerOperation()` could announce - * itself to main and traffic could route at a thread that is about to exit. It would also cost - * O(deploys × workers) channels on a large node. Only the *option construction* is worth sharing, and that - * is deliberately kept small here rather than reaching into the serving-worker path. + * Spawned with a bare `new Worker` rather than `startWorker()`, which is a known problem rather than a + * settled choice: this thread does not load at all on Windows (HarperFast/harper#2494), and `startWorker` + * is the intended fix. The original argument for avoiding it — that a certification worker would join the + * ITC mesh, letting a candidate's top-level `server.registerOperation()` announce itself to main, at + * O(deploys × workers) channels — is only half right: `isEligibleBroadcastRecipient` already excludes a + * job-type worker from broadcasts, and the channel cost is O(workers) for one slow-path deploy. See + * DESIGN.md's certification section for what is and is not established about the Windows failure. * * Every outcome other than an explicit passing verdict is a failure, because `.complete` — which this * gates — is what crash recovery treats as proof that a validation happened. diff --git a/unitTests/server/threads/workerDataExtra.test.js b/unitTests/server/threads/workerDataExtra.test.js index a92e5b0f7d..60a7af3614 100644 --- a/unitTests/server/threads/workerDataExtra.test.js +++ b/unitTests/server/threads/workerDataExtra.test.js @@ -100,6 +100,38 @@ describe('startWorker per-call workerData', () => { port1.close(); }); + it('never restarts or copies a one-shot worker, whose transferred ports are spent', async function () { + // The crash this guards: `restartWorkers` replaces workers through `worker.startCopy()`, which + // re-spawns from the SAME options object — so a worker carrying transferred ports would hit + // `DataCloneError` synchronously, inside the restart loop, taking the rest of the restart with it. + this.timeout(30000); + const { port1, port2 } = new MessageChannel(); + const worker = startWorker(FIXTURE, { + autoRestart: false, + name: WORKER_NAME, + execArgvOptions: { preloads: false }, + extraWorkerData: { certification: { candidateDirPath: '/tmp/c', nonce: 'n', verdictPort: port2 } }, + extraTransferList: [port2], + }); + + try { + assert.strictEqual(worker.isOneShot, true, 'a port-carrying spawn is marked one-shot'); + // The backstop for a direct caller: a named refusal rather than a DataCloneError from inside + // `new Worker`. + assert.throws(() => worker.startCopy(), /Cannot restart a one-shot/); + + // The restart loop's own filter is NOT driven here. `restartWorkers` runs a real node restart — + // it reinstalls applications, which shells out to `npm pack` — so calling it from a unit test + // exercises far more than the one-line `isOneShot` filter and would be slow and fragile for it. + // What is asserted above is the mechanism that filter depends on: the flag is set, and the copy + // refuses by name. The filter itself is covered by reading, and the crash it prevents is + // described in the commit that added it. + } finally { + port1.close(); + await worker.terminate().catch(() => {}); + } + }); + it('leaves noServerStart absent unless asked for', async function () { this.timeout(30000); const report = await spawnAndReport({}); From 2c1d27328a66f90597a4a08bacc37747d9df3059 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 4 Sep 2026 13:24:31 -0400 Subject: [PATCH 29/29] fix: honour provider ownership in extraWorkerData, and re-check shutdown before spawning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of kriszyp's four inline findings, both correct and both narrow enough to fix without waiting on his premise question about whether certification should exist at all. `extraWorkerData` is spread AFTER provider output, so a caller could silently replace a registered provider's value. `configOverrides` is the consequential one: overriding it leaves the thread reading on-disk config while its parent runs on `setProperty()` overrides, so a validator's verdict would describe a different environment than the one serving. `registerWorkerDataProvider` already refuses name collisions; the merge path now applies the same ownership rule, before any side effect. The shutdown guard only ran at entry, before a slot wait that can last the full certification timeout and two awaited filesystem calls. Shutdown beginning in that window still produced a validator that is absent from `manageThreads.workers` — so shutdown neither terminates nor awaits it while it loads databases and components into a process that is tearing down. Re-checked immediately before the spawn. Not fixed, and deliberately left unresolved rather than closed: the bare `Worker` bypassing `collectProvidedWorkerData` entirely, and the module-local concurrency cap. Both need real design work — a shared non-topology bootstrap, and brokering through one owning thread or a cross-thread semaphore — and both are inside the mechanism whose existence kriszyp is questioning in the same review. Answering those with code before that question is settled risks building on a premise the owner may reject. Co-Authored-By: Claude Opus 5 --- components/certifyCandidate.ts | 10 +++++++ server/threads/manageThreads.js | 8 ++++++ .../server/threads/workerDataExtra.test.js | 27 ++++++++++++++++++- 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/components/certifyCandidate.ts b/components/certifyCandidate.ts index 4f7f735cbc..4eac941bb6 100644 --- a/components/certifyCandidate.ts +++ b/components/certifyCandidate.ts @@ -282,6 +282,16 @@ export async function certifyCandidate( installRoot = await realpath(PACKAGE_ROOT).catch(() => undefined); linksBefore = installRoot ? await snapshotHarperModuleLinks(candidateDirPath, installRoot) : undefined; + // Re-checked here, not only at entry. The slot wait above is unbounded up to `timeoutMs` and the two + // filesystem calls after it are awaited, so shutdown can begin while this caller is queued — and a + // validator started then is absent from `manageThreads.workers`, so shutdown neither terminates nor + // awaits it while it loads databases and components into a process that is tearing down. + if (isProcessShuttingDown()) { + const error: any = new Error(`Cannot certify ${appName} while the Harper process is shutting down`); + error.statusCode = 503; + return { certified: false, error }; + } + return await new Promise((resolve) => { // Exactly one settlement, whichever of the outcomes below happens first. A candidate with a // syntax error emits `error` AND then `exit`; a candidate that posts a verdict and then throws diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 8a6d62ff80..016c363277 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -447,6 +447,14 @@ function startWorker(path, options = {}) { if (RESERVED_WORKER_DATA_KEYS.includes(key)) { throw new Error(`extraWorkerData may not set '${key}': it is owned by the thread bootstrap`); } + // A registered provider owns its key too. `extraWorkerData` is spread AFTER provider output, so + // without this a caller could silently replace an authoritative value — `configOverrides` most + // consequentially, which would leave the thread reading on-disk config while its parent runs on + // `setProperty()` overrides. `registerWorkerDataProvider` already refuses name collisions; this is + // the same ownership rule on the other path into `workerData`. + if (workerDataProviders.has(key)) { + throw new Error(`extraWorkerData may not set '${key}': a registered workerData provider owns it`); + } } } // A transferred port is single-use, and the unexpected-exit path below re-invokes `startWorker` with the diff --git a/unitTests/server/threads/workerDataExtra.test.js b/unitTests/server/threads/workerDataExtra.test.js index 60a7af3614..521b704f3b 100644 --- a/unitTests/server/threads/workerDataExtra.test.js +++ b/unitTests/server/threads/workerDataExtra.test.js @@ -3,7 +3,7 @@ const assert = require('assert'); const path = require('node:path'); const { MessageChannel } = require('node:worker_threads'); -const { startWorker } = require('#js/server/threads/manageThreads'); +const { startWorker, registerWorkerDataProvider } = require('#js/server/threads/manageThreads'); const FIXTURE = path.join(__dirname, 'workerDataExtra-fixture.js'); const WORKER_NAME = 'workerData-extra-test'; @@ -66,6 +66,31 @@ describe('startWorker per-call workerData', () => { port1.close(); port2.close(); } + // A registered provider owns its key on this path too. `extraWorkerData` is spread after provider + // output, so without this a caller could quietly replace `configOverrides` and leave the thread + // reading on-disk config while its parent runs on `setProperty()` overrides. + assert.throws( + () => startWorker(FIXTURE, { name: WORKER_NAME, extraWorkerData: { configOverrides: { 'a.b': 1 } } }), + /a registered workerData provider owns it/ + ); + const unregister = registerWorkerDataProvider('extraCollisionProbe', () => undefined); + try { + assert.throws( + () => startWorker(FIXTURE, { name: WORKER_NAME, extraWorkerData: { extraCollisionProbe: 1 } }), + /a registered workerData provider owns it/ + ); + } finally { + unregister(); + } + // ...and once the provider is gone, the name is free again. + assert.doesNotThrow(() => + startWorker(FIXTURE, { + autoRestart: false, + name: WORKER_NAME, + execArgvOptions: { preloads: false }, + extraWorkerData: { extraCollisionProbe: 1 }, + }).terminate() + ); for (const key of ['addPorts', 'ticketKeys', 'workerCount', 'noServerStart', '__proto__']) { assert.throws( () => startWorker(FIXTURE, { name: WORKER_NAME, extraWorkerData: { [key]: 'x' } }),