From 33be779054013df298a2e57a40afc484bc4921ce Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Tue, 18 Aug 2026 12:54:12 -0400 Subject: [PATCH 01/16] Propagate grantable component operations to the main thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `server.registerOperation({ requiresSuperUser })` marks an operation grantable in a role's `operations` allowlist, but that mark landed only in the worker that registered it (components load per-worker). Meanwhile `validateOperations` is consulted on the main thread — by add_role and alter_role, by impersonation payload validation, and by OIDC trust policies — so naming a component-registered operation in any of those was rejected as "not a valid operation name or group" even though the operation existed and was designed to be grantable. The OPERATION_REGISTERED announcement already crosses that boundary for execution routing, so carry grantability on it too and mirror the mark on main. This only widens what an allowlist may name; enforcement is unchanged, still running on the worker's own `chooseOperation`. Also arm the thread-exit cleanup when the registry gains its first entry rather than on the first forwarded call, so a worker that registers and exits without ever being called no longer leaks its entries, and revoke the mirrored mark when the last registering worker is gone. Co-Authored-By: Claude Opus 5 --- .../registered-operation/resources.js | 15 +++ .../components/registered-operation.test.ts | 121 ++++++++++++++++++ server/DESIGN.md | 10 ++ server/serverHelpers/registeredOperations.ts | 31 ++++- server/serverHelpers/serverUtilities.ts | 5 +- .../serverHelpers/serverUtilities.test.js | 36 ++++++ 6 files changed, 210 insertions(+), 8 deletions(-) diff --git a/integrationTests/components/fixtures/registered-operation/resources.js b/integrationTests/components/fixtures/registered-operation/resources.js index 8933786f96..9b348a4475 100644 --- a/integrationTests/components/fixtures/registered-operation/resources.js +++ b/integrationTests/components/fixtures/registered-operation/resources.js @@ -37,3 +37,18 @@ server.registerOperation({ throw error; }, }); + +// requiresSuperUser makes this one both enforced and grantable, which is what puts its name in +// front of the main thread's validateOperations. +server.registerOperation({ + name: 'component_registered_grantable', + requiresSuperUser: true, + execute: async function componentRegisteredGrantable(op) { + return { + granted: true, + executedOnMainThread: isMainThread, + executedOnThreadId: threadId, + username: op.hdb_user?.username ?? null, + }; + }, +}); diff --git a/integrationTests/components/registered-operation.test.ts b/integrationTests/components/registered-operation.test.ts index 4ac138eeb4..8b136ecbff 100644 --- a/integrationTests/components/registered-operation.test.ts +++ b/integrationTests/components/registered-operation.test.ts @@ -6,6 +6,11 @@ * OPERATION_FUNCTION_MAP instances; the ops-API dispatcher runs on the main thread with its * own instance. The cross-thread bridge (server/serverHelpers/registeredOperations.ts) * forwards an unrecognized operation to one registering worker and relays the result. + * + * The same split governs the role `operations` allowlist: registerOperationPermission marks a + * declared op grantable on the worker, but validateOperations is consulted on the main thread. + * Only an integration test crosses that boundary — unit tests register and validate on one + * thread, so the topology, and therefore the gap, is invisible to them. */ import { suite, test, before, after } from 'node:test'; import { strictEqual, ok } from 'node:assert'; @@ -15,6 +20,14 @@ import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from ' const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/registered-operation'); +// The one op the fixture declares a permission for, and so the only grantable one. +const GRANTABLE_OP = 'component_registered_grantable'; +const GRANTED_ROLE = 'component_op_granted_role'; +const GRANTED_USER = 'component_op_granted_user'; +const UNGRANTED_ROLE = 'component_op_ungranted_role'; +const UNGRANTED_USER = 'component_op_ungranted_user'; +const USER_PASS = 'Abc1234!'; + suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { async function op(body: any): Promise<{ status: number; body: any }> { const { username, password } = ctx.harper.admin; @@ -29,6 +42,18 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { return { status: response.status, body: await response.json() }; } + async function asUser(username: string, body: any): Promise<{ status: number; body: any }> { + const response = await fetch(ctx.harper.operationsAPIURL, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Authorization': `Basic ${Buffer.from(`${username}:${USER_PASS}`).toString('base64')}`, + }, + body: JSON.stringify(body), + }); + return { status: response.status, body: await response.json() }; + } + before(async () => { // Multiple HTTP workers so the forward actually has a choice of registering threads. await setupHarperWithFixture(ctx, FIXTURE_PATH, { @@ -88,4 +113,100 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { strictEqual(status, 400, JSON.stringify(body)); ok(JSON.stringify(body).includes('not found'), `expected operation-not-found error, got: ${JSON.stringify(body)}`); }); + + suite('grantable in a role `operations` allowlist across the worker/main boundary', () => { + before(async () => { + // The announcement is fire-and-forget ITC. A successful forward proves the handler ran, and + // it carries the grantable flag in the same message — so this gates on the exact state + // these tests depend on, rather than on elapsed time. + const deadline = Date.now() + 15_000; + for (;;) { + const { status } = await op({ operation: GRANTABLE_OP }); + if (status === 200) break; + if (Date.now() > deadline) throw new Error(`main thread never registered '${GRANTABLE_OP}'`); + await new Promise((resolve) => setTimeout(resolve, 100)); + } + }); + + test('add_role accepts the worker-registered op name in `operations`', async () => { + const { status, body } = await op({ + operation: 'add_role', + role: GRANTED_ROLE, + permission: { operations: [GRANTABLE_OP] }, + }); + strictEqual(status, 200, JSON.stringify(body)); + }); + + test('add_role still rejects an op name no component registered', async () => { + // Negative control: proves the assertion above is not passing because validateOperations + // was skipped for this payload shape. + const { status, body } = await op({ + operation: 'add_role', + role: 'component_op_bogus_role', + permission: { operations: ['component_registered_never_declared'] }, + }); + strictEqual(status, 400, JSON.stringify(body)); + ok( + JSON.stringify(body).includes('component_registered_never_declared'), + `expected the offending op name in the error, got: ${JSON.stringify(body)}` + ); + }); + + test('alter_role accepts it too', async () => { + const { status, body } = await op({ + operation: 'alter_role', + id: GRANTED_ROLE, + permission: { operations: [GRANTABLE_OP, 'user_info'] }, + }); + strictEqual(status, 200, JSON.stringify(body)); + }); + + test('a non-super_user granted the op can actually call it', async () => { + const added = await op({ + operation: 'add_user', + role: GRANTED_ROLE, + username: GRANTED_USER, + password: USER_PASS, + active: true, + }); + strictEqual(added.status, 200, JSON.stringify(added.body)); + + const { status, body } = await asUser(GRANTED_USER, { operation: GRANTABLE_OP }); + strictEqual(status, 200, JSON.stringify(body)); + strictEqual(body.granted, true); + strictEqual(body.username, GRANTED_USER); + // Still executed on the worker — main only had to accept the name, not enforce it. + strictEqual(body.executedOnMainThread, false); + }); + + test('a non-super_user without the grant is still denied (enforcement unchanged)', async () => { + const role = await op({ + operation: 'add_role', + role: UNGRANTED_ROLE, + permission: { operations: ['user_info'] }, + }); + strictEqual(role.status, 200, JSON.stringify(role.body)); + const added = await op({ + operation: 'add_user', + role: UNGRANTED_ROLE, + username: UNGRANTED_USER, + password: USER_PASS, + active: true, + }); + strictEqual(added.status, 200, JSON.stringify(added.body)); + + const { status, body } = await asUser(UNGRANTED_USER, { operation: GRANTABLE_OP }); + strictEqual(status, 403, JSON.stringify(body)); + }); + + test('impersonation accepts an inline role naming the op', async () => { + // applyImpersonation runs validateOperations on the main thread too. + const { status, body } = await op({ + operation: GRANTABLE_OP, + impersonate: { role: { permission: { operations: [GRANTABLE_OP] } } }, + }); + strictEqual(status, 200, JSON.stringify(body)); + strictEqual(body.granted, true); + }); + }); }); diff --git a/server/DESIGN.md b/server/DESIGN.md index 7c6b5885a5..529b726a5e 100644 --- a/server/DESIGN.md +++ b/server/DESIGN.md @@ -148,6 +148,16 @@ must run on a worker, `registeredOperations.ts` carries that state in the same-p separately from the structured-cloned body. Never attach trusted dispatch state to an operation payload. +`server.registerOperation()` runs per-worker, so anything the **main** thread must later know about a +registered op has to ride the OPERATION_REGISTERED announcement — a module-local registry populated +during registration exists only in the worker that registered. The bridge carries two such facts +today: name→thread routing (for execution forwarding) and `grantable` (so `validateOperations` on +main will accept the name in a role's `operations` allowlist, for add_role/alter_role, impersonation, +and OIDC trust policies). Adding a third main-thread consumer of a worker-registered fact means +extending that message, not reading a registry that main never populated. Grantability is safe to +mirror because it only widens what an allowlist may _name_; enforcement stays on the worker's +`chooseOperation`. + ## Resource ↔ HTTP boundary `REST.ts → http(request, nextHandler)` is the chief integration point: it takes a `Request`, asks the `Resources` registry for a match, builds a `RequestTarget`, and dispatches into the Resource class's static method. Cache headers are translated to `request.expiresAt` / `onlyIfCached` / `noCache` flags within the same function. diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index 5013c3e786..967ba7f323 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -11,7 +11,9 @@ * so a request is sent to exactly ONE registering worker (never broadcast-first-wins). * * - Worker: `registerOperation()` announces the name (OPERATION_REGISTERED) to all threads; - * only the main thread records it, as name -> Set. + * only the main thread records it, as name -> Set. An op that declared a permission + * also announces `grantable`: validateOperations runs on main (add_role/alter_role, + * impersonation, OIDC trust policies) but the mark was made in the worker's own module scope. * - Main: on an OPERATION_FUNCTION_MAP miss, `getRemoteOperationFunction()` supplies a forwarding * function that sends the request body (OPERATION_EXECUTE_REQUEST) to one live registering * worker and awaits the correlated OPERATION_EXECUTE_RESPONSE. @@ -27,6 +29,7 @@ import harperLogger from '../../utility/logging/harper_logger.ts'; import { ServerError } from '../../utility/errors/hdbError.ts'; import { sendItcEvent } from '../threads/itc.js'; import { onMessageByType, onThreadExit } from '../threads/manageThreads.js'; +import { registerGrantableOperation, unregisterGrantableOperation } from '../../utility/operationPermissions.ts'; import { runWithOperationAuthorizationBypass } from './operationAuthorizationState.ts'; const operationLog = harperLogger.loggerWithTag('operation'); @@ -59,6 +62,8 @@ export function setLocalOperationDispatch(dispatch: typeof localDispatch) { /** name -> threadIds of workers that registered it (main thread only) */ const registeredByWorker = new Map>(); +// Marks made on a worker's behalf, so thread-exit cleanup never revokes one this thread owns. +const grantableFromWorkers = new Set(); const pendingExecutions = new Map< number, { targetThreadId: number; resolve: (result: any) => void; reject: (error: Error) => void } @@ -71,24 +76,35 @@ let mainListenersAttached = false; * a lost announcement just means the op stays unreachable (the pre-#1736 status quo), and the * broadcast has its own ack timeout. */ -export function announceRegisteredOperation(name: string) { +export function announceRegisteredOperation(name: string, grantable = false) { if (isMainThread) return; sendItcEvent({ type: terms.ITC_EVENT_TYPES.OPERATION_REGISTERED, - message: { name }, + message: { name, grantable }, }).catch((error) => operationLog.error(`Failed to announce registered operation '${name}'`, error)); } /** * ITC handler (all threads receive the broadcast; only main records it). */ -export function operationRegisteredHandler(event: { message: { name: string; originator: number } }) { +export function operationRegisteredHandler(event: { + message: { name: string; grantable?: boolean; originator: number }; +}) { if (!isMainThread) return; - const { name, originator } = event.message; + const { name, grantable, originator } = event.message; if (typeof name !== 'string' || typeof originator !== 'number') return; + // Arm thread-exit cleanup when the registry gains its first entry, not on the first forward: + // a worker that exits before any call would otherwise leave its entries here forever. + attachMainListeners(); let workerIds = registeredByWorker.get(name); if (!workerIds) registeredByWorker.set(name, (workerIds = new Set())); workerIds.add(originator); + // Mirroring the worker's grantable mark only widens what an allowlist may name; it grants + // nothing. Enforcement stays on the worker's own chooseOperation (see getRemoteOperationFunction). + if (grantable) { + grantableFromWorkers.add(name); + registerGrantableOperation(name); + } operationLog.debug(`Registered operation '${name}' announced by worker thread ${originator}`); } @@ -128,7 +144,10 @@ function attachMainListeners() { onThreadExit((deadThreadId: number) => { for (const [name, workerIds] of registeredByWorker) { workerIds.delete(deadThreadId); - if (workerIds.size === 0) registeredByWorker.delete(name); + if (workerIds.size === 0) { + registeredByWorker.delete(name); + if (grantableFromWorkers.delete(name)) unregisterGrantableOperation(name); + } } for (const [requestId, pending] of pendingExecutions) { if (pending.targetThreadId === deadThreadId) { diff --git a/server/serverHelpers/serverUtilities.ts b/server/serverHelpers/serverUtilities.ts index c98d82b740..28417da53a 100644 --- a/server/serverHelpers/serverUtilities.ts +++ b/server/serverHelpers/serverUtilities.ts @@ -209,8 +209,9 @@ server.registerOperation = (operationDefinition: OperationDefinition) => { OPERATION_FUNCTION_MAP.set(name as any, new OperationFunctionObject(handler)); // Components load per-worker, so a registration made there is invisible to the main-thread // ops-API dispatcher (each thread has its own OPERATION_FUNCTION_MAP instance). Announce it - // so the main thread can forward calls to this worker (#1736). - if (!isMainThread) announceRegisteredOperation(name); + // so the main thread can forward calls to this worker (#1736), and so it can mirror the + // role-allowlist mark that registerOperationPermission above made only in this thread's scope. + if (!isMainThread) announceRegisteredOperation(name, requiresSuperUser !== undefined); }; // Register the durable MCP quota policy as a function (see components/mcp/quota.ts). Worker-local, diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 5a6a4b4007..9d0346275c 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -221,6 +221,42 @@ describe('Test serverUtilities.js module ', () => { }); }); + // Only the receiving half is reachable here — announceRegisteredOperation returns early on the + // main thread, so integrationTests/components/registered-operation.test.ts owns the real hop. + describe('cross-thread grantable operation mirroring', function () { + const { validateOperations, unregisterGrantableOperation } = require('#src/utility/operationPermissions'); + const GRANTABLE = 'test_cross_thread_grantable_op'; + const PLAIN = 'test_cross_thread_plain_op'; + + after(function () { + // Don't leak either name into the process-global grantable set for later suites. + unregisterGrantableOperation(GRANTABLE); + unregisterGrantableOperation(PLAIN); + }); + + it('makes a worker-announced declared op grantable on the main thread', function () { + assert.notEqual(validateOperations([GRANTABLE]), null, 'name should be unknown before the announcement'); + + registeredOperations.operationRegisteredHandler({ + message: { name: GRANTABLE, grantable: true, originator: 31 }, + }); + + assert.equal(validateOperations([GRANTABLE]), null, 'name should be grantable after the announcement'); + }); + + it('leaves an op that declared no permission ungrantable', function () { + // requiresSuperUser omitted means no verifyPerms entry and nothing to grant — the routing + // entry must not smuggle the name into the allowlist. + registeredOperations.operationRegisteredHandler({ + message: { name: PLAIN, grantable: false, originator: 31 }, + }); + + assert.notEqual(validateOperations([PLAIN]), null); + // A forward is still set up for it, so the routing half is unaffected. + assert.equal(typeof registeredOperations.getRemoteOperationFunction(PLAIN), 'function'); + }); + }); + describe('operation authorization state', function () { it('is scoped across awaits and restores nested authorization', async function () { assert.strictEqual(operationAuthorizationState.isOperationAuthorizationBypassed(), false); From 5d12b23164b51d768c7572d10afe2d04b070b6a4 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Tue, 18 Aug 2026 13:06:28 -0400 Subject: [PATCH 02/16] Address cross-model review: scope the grantable mark's ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claim a mirrored name for thread-exit cleanup only when the mirror is what made it admissible. As written, the ownership set was unconditional, so a name that main had already registered itself — or an enum/group name — was revoked when the last worker offering the same name exited, which is the opposite of what the set exists to prevent and of what its comment claimed. Flagged independently by both review lenses. Route both prune paths through one `dropRegistration`: the failed-send path in `executeRemoteOperation` dropped the routing entry without revoking the mark, so routing and grantability could disagree about whether an op was still offered. Also cut the comments the review flagged as narration or as duplicating the note now carried in server/DESIGN.md. Co-Authored-By: Claude Opus 5 --- .../components/registered-operation.test.ts | 5 +-- server/serverHelpers/registeredOperations.ts | 37 ++++++++++++------- server/serverHelpers/serverUtilities.ts | 4 +- 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/integrationTests/components/registered-operation.test.ts b/integrationTests/components/registered-operation.test.ts index 8b136ecbff..5585818a51 100644 --- a/integrationTests/components/registered-operation.test.ts +++ b/integrationTests/components/registered-operation.test.ts @@ -138,8 +138,8 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { }); test('add_role still rejects an op name no component registered', async () => { - // Negative control: proves the assertion above is not passing because validateOperations - // was skipped for this payload shape. + // Guards the test above: without this, a validateOperations that stopped running at all + // would look like a pass. const { status, body } = await op({ operation: 'add_role', role: 'component_op_bogus_role', @@ -200,7 +200,6 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { }); test('impersonation accepts an inline role naming the op', async () => { - // applyImpersonation runs validateOperations on the main thread too. const { status, body } = await op({ operation: GRANTABLE_OP, impersonate: { role: { permission: { operations: [GRANTABLE_OP] } } }, diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index 967ba7f323..cb26570976 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -11,9 +11,8 @@ * so a request is sent to exactly ONE registering worker (never broadcast-first-wins). * * - Worker: `registerOperation()` announces the name (OPERATION_REGISTERED) to all threads; - * only the main thread records it, as name -> Set. An op that declared a permission - * also announces `grantable`: validateOperations runs on main (add_role/alter_role, - * impersonation, OIDC trust policies) but the mark was made in the worker's own module scope. + * only the main thread records it, as name -> Set, plus `grantable` (see the + * per-worker registration note in server/DESIGN.md). * - Main: on an OPERATION_FUNCTION_MAP miss, `getRemoteOperationFunction()` supplies a forwarding * function that sends the request body (OPERATION_EXECUTE_REQUEST) to one live registering * worker and awaits the correlated OPERATION_EXECUTE_RESPONSE. @@ -29,7 +28,11 @@ import harperLogger from '../../utility/logging/harper_logger.ts'; import { ServerError } from '../../utility/errors/hdbError.ts'; import { sendItcEvent } from '../threads/itc.js'; import { onMessageByType, onThreadExit } from '../threads/manageThreads.js'; -import { registerGrantableOperation, unregisterGrantableOperation } from '../../utility/operationPermissions.ts'; +import { + registerGrantableOperation, + unregisterGrantableOperation, + validateOperations, +} from '../../utility/operationPermissions.ts'; import { runWithOperationAuthorizationBypass } from './operationAuthorizationState.ts'; const operationLog = harperLogger.loggerWithTag('operation'); @@ -62,7 +65,8 @@ export function setLocalOperationDispatch(dispatch: typeof localDispatch) { /** name -> threadIds of workers that registered it (main thread only) */ const registeredByWorker = new Map>(); -// Marks made on a worker's behalf, so thread-exit cleanup never revokes one this thread owns. +// Only names this thread newly marked grantable for a worker, so cleanup below cannot revoke a mark +// that was already admissible for another reason (an enum op, a group, a main-thread registration). const grantableFromWorkers = new Set(); const pendingExecutions = new Map< number, @@ -99,15 +103,25 @@ export function operationRegisteredHandler(event: { let workerIds = registeredByWorker.get(name); if (!workerIds) registeredByWorker.set(name, (workerIds = new Set())); workerIds.add(originator); - // Mirroring the worker's grantable mark only widens what an allowlist may name; it grants - // nothing. Enforcement stays on the worker's own chooseOperation (see getRemoteOperationFunction). - if (grantable) { + // Mirroring only widens what an allowlist may name; enforcement stays on the worker's own + // chooseOperation. Claim the name for cleanup only when this mirror is what made it admissible. + if (grantable && validateOperations([name]) !== null) { grantableFromWorkers.add(name); registerGrantableOperation(name); } operationLog.debug(`Registered operation '${name}' announced by worker thread ${originator}`); } +/** + * Forget an operation whose last registering worker is gone. Both prune paths (thread exit, and a + * failed send discovering a dead port) must route through here so routing and grantability can + * never disagree about whether the op is still offered. + */ +function dropRegistration(name: string) { + registeredByWorker.delete(name); + if (grantableFromWorkers.delete(name)) unregisterGrantableOperation(name); +} + let rotation = 0; /** * Main-thread dispatch fallback: if a worker registered `name`, return a forwarding operation @@ -144,10 +158,7 @@ function attachMainListeners() { onThreadExit((deadThreadId: number) => { for (const [name, workerIds] of registeredByWorker) { workerIds.delete(deadThreadId); - if (workerIds.size === 0) { - registeredByWorker.delete(name); - if (grantableFromWorkers.delete(name)) unregisterGrantableOperation(name); - } + if (workerIds.size === 0) dropRegistration(name); } for (const [requestId, pending] of pendingExecutions) { if (pending.targetThreadId === deadThreadId) { @@ -209,7 +220,7 @@ async function executeRemoteOperation(name: string, body: any, bypassAuth: boole }); }); } - if (registeredByWorker.get(name)?.size === 0) registeredByWorker.delete(name); + if (registeredByWorker.get(name)?.size === 0) dropRegistration(name); throw new ServerError( `Operation '${name}' is registered by a component but no worker thread is available to run it`, 503 diff --git a/server/serverHelpers/serverUtilities.ts b/server/serverHelpers/serverUtilities.ts index 28417da53a..d3c3d28db4 100644 --- a/server/serverHelpers/serverUtilities.ts +++ b/server/serverHelpers/serverUtilities.ts @@ -209,8 +209,8 @@ server.registerOperation = (operationDefinition: OperationDefinition) => { OPERATION_FUNCTION_MAP.set(name as any, new OperationFunctionObject(handler)); // Components load per-worker, so a registration made there is invisible to the main-thread // ops-API dispatcher (each thread has its own OPERATION_FUNCTION_MAP instance). Announce it - // so the main thread can forward calls to this worker (#1736), and so it can mirror the - // role-allowlist mark that registerOperationPermission above made only in this thread's scope. + // so the main thread can forward calls to this worker (#1736), and can mirror the role-allowlist + // mark that registerOperationPermission above made only in this thread's scope. if (!isMainThread) announceRegisteredOperation(name, requiresSuperUser !== undefined); }; From 3144e346506ac127be487e654c494274555e383a Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Tue, 18 Aug 2026 13:20:43 -0400 Subject: [PATCH 03/16] Keep worker-mirrored grantability in its own registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ownership probe added in the previous commit only protected marks that predated a worker's announcement, which leaves a reachable hole: on a hot deploy `restartWorkers` awaits `loadRootComponents()` before it begins draining the old workers (`server/threads/manageThreads.js`), so a `startOnMainThread` component can register an operation the retiring worker also offered. The worker's exit then revoked the main thread's own mark and role validation started rejecting an operation that was registered and executable. Track mirrored names in a separate set that `validateOperations` unions instead of sharing one. The two threads can now register the same name independently and neither can revoke the other, which removes the ownership question rather than narrowing it — the probe and its bookkeeping set are gone. Found by the round-2 cross-model review, which also supplied this approach. Co-Authored-By: Claude Opus 5 --- .../registered-operation/resources.js | 2 -- .../components/registered-operation.test.ts | 2 -- server/serverHelpers/registeredOperations.ts | 17 +++------- .../serverHelpers/serverUtilities.test.js | 32 ++++++++++++++++--- utility/operationPermissions.ts | 23 ++++++++++++- 5 files changed, 55 insertions(+), 21 deletions(-) diff --git a/integrationTests/components/fixtures/registered-operation/resources.js b/integrationTests/components/fixtures/registered-operation/resources.js index 9b348a4475..f9e9796873 100644 --- a/integrationTests/components/fixtures/registered-operation/resources.js +++ b/integrationTests/components/fixtures/registered-operation/resources.js @@ -38,8 +38,6 @@ server.registerOperation({ }, }); -// requiresSuperUser makes this one both enforced and grantable, which is what puts its name in -// front of the main thread's validateOperations. server.registerOperation({ name: 'component_registered_grantable', requiresSuperUser: true, diff --git a/integrationTests/components/registered-operation.test.ts b/integrationTests/components/registered-operation.test.ts index 5585818a51..ea64a77feb 100644 --- a/integrationTests/components/registered-operation.test.ts +++ b/integrationTests/components/registered-operation.test.ts @@ -20,7 +20,6 @@ import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from ' const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/registered-operation'); -// The one op the fixture declares a permission for, and so the only grantable one. const GRANTABLE_OP = 'component_registered_grantable'; const GRANTED_ROLE = 'component_op_granted_role'; const GRANTED_USER = 'component_op_granted_user'; @@ -175,7 +174,6 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { strictEqual(status, 200, JSON.stringify(body)); strictEqual(body.granted, true); strictEqual(body.username, GRANTED_USER); - // Still executed on the worker — main only had to accept the name, not enforce it. strictEqual(body.executedOnMainThread, false); }); diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index cb26570976..d61b0d3ba9 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -29,9 +29,8 @@ import { ServerError } from '../../utility/errors/hdbError.ts'; import { sendItcEvent } from '../threads/itc.js'; import { onMessageByType, onThreadExit } from '../threads/manageThreads.js'; import { - registerGrantableOperation, - unregisterGrantableOperation, - validateOperations, + registerWorkerGrantableOperation, + unregisterWorkerGrantableOperation, } from '../../utility/operationPermissions.ts'; import { runWithOperationAuthorizationBypass } from './operationAuthorizationState.ts'; @@ -65,9 +64,6 @@ export function setLocalOperationDispatch(dispatch: typeof localDispatch) { /** name -> threadIds of workers that registered it (main thread only) */ const registeredByWorker = new Map>(); -// Only names this thread newly marked grantable for a worker, so cleanup below cannot revoke a mark -// that was already admissible for another reason (an enum op, a group, a main-thread registration). -const grantableFromWorkers = new Set(); const pendingExecutions = new Map< number, { targetThreadId: number; resolve: (result: any) => void; reject: (error: Error) => void } @@ -104,11 +100,8 @@ export function operationRegisteredHandler(event: { if (!workerIds) registeredByWorker.set(name, (workerIds = new Set())); workerIds.add(originator); // Mirroring only widens what an allowlist may name; enforcement stays on the worker's own - // chooseOperation. Claim the name for cleanup only when this mirror is what made it admissible. - if (grantable && validateOperations([name]) !== null) { - grantableFromWorkers.add(name); - registerGrantableOperation(name); - } + // chooseOperation. + if (grantable) registerWorkerGrantableOperation(name); operationLog.debug(`Registered operation '${name}' announced by worker thread ${originator}`); } @@ -119,7 +112,7 @@ export function operationRegisteredHandler(event: { */ function dropRegistration(name: string) { registeredByWorker.delete(name); - if (grantableFromWorkers.delete(name)) unregisterGrantableOperation(name); + unregisterWorkerGrantableOperation(name); } let rotation = 0; diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 9d0346275c..3bdcc75673 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -224,14 +224,22 @@ describe('Test serverUtilities.js module ', () => { // Only the receiving half is reachable here — announceRegisteredOperation returns early on the // main thread, so integrationTests/components/registered-operation.test.ts owns the real hop. describe('cross-thread grantable operation mirroring', function () { - const { validateOperations, unregisterGrantableOperation } = require('#src/utility/operationPermissions'); + const { + validateOperations, + registerGrantableOperation, + unregisterGrantableOperation, + unregisterWorkerGrantableOperation, + } = require('#src/utility/operationPermissions'); const GRANTABLE = 'test_cross_thread_grantable_op'; const PLAIN = 'test_cross_thread_plain_op'; + const SHARED = 'test_cross_thread_shared_op'; after(function () { - // Don't leak either name into the process-global grantable set for later suites. - unregisterGrantableOperation(GRANTABLE); - unregisterGrantableOperation(PLAIN); + // Don't leak these names into the process-global registries for later suites. + for (const op of [GRANTABLE, PLAIN, SHARED]) { + unregisterWorkerGrantableOperation(op); + unregisterGrantableOperation(op); + } }); it('makes a worker-announced declared op grantable on the main thread', function () { @@ -255,6 +263,22 @@ describe('Test serverUtilities.js module ', () => { // A forward is still set up for it, so the routing half is unaffected. assert.equal(typeof registeredOperations.getRemoteOperationFunction(PLAIN), 'function'); }); + + it('keeps a main-thread registration of the same name independent of the worker mirror', function () { + // A hot deploy can load a startOnMainThread component that registers an op a retiring + // worker also offers (manageThreads restartWorkers loads root components before draining + // the old workers), so the two marks have to survive each other. + registeredOperations.operationRegisteredHandler({ + message: { name: SHARED, grantable: true, originator: 41 }, + }); + registerGrantableOperation(SHARED); + + unregisterWorkerGrantableOperation(SHARED); + assert.equal(validateOperations([SHARED]), null, 'main-thread registration should survive worker revocation'); + + unregisterGrantableOperation(SHARED); + assert.notEqual(validateOperations([SHARED]), null, 'both marks gone should make it ungrantable again'); + }); }); describe('operation authorization state', function () { diff --git a/utility/operationPermissions.ts b/utility/operationPermissions.ts index 3ad4d0fecf..67a87b4ae2 100644 --- a/utility/operationPermissions.ts +++ b/utility/operationPermissions.ts @@ -97,6 +97,11 @@ const validGroups: Set = new Set(Object.keys(OPERATION_PERMISSION_GROUPS // permission) whose names may fall outside OPERATIONS_ENUM. Tracked so they're grantable in a // role's `operations` allowlist (add_role/alter_role/impersonation validate against this set too). const dynamicallyRegisteredOps: Set = new Set(); +// The same, for ops registered on a WORKER and mirrored here by the OPERATION_REGISTERED bridge. +// Deliberately a separate set: the two threads can register the same name independently (a hot +// deploy can add a `startOnMainThread` component while the worker offering that name is retiring), +// so worker-lifecycle cleanup must not be able to revoke a mark this thread made itself. +const workerRegisteredOps: Set = new Set(); /** * Mark a dynamically-registered operation name as a valid target for role `operations` grants. @@ -115,13 +120,29 @@ export function unregisterGrantableOperation(name: string): void { dynamicallyRegisteredOps.delete(name); } +/** + * Mirror a worker's grantable operation so role validation, which runs on the main thread, accepts + * the name. See server/serverHelpers/registeredOperations.ts — the worker still owns enforcement. + */ +export function registerWorkerGrantableOperation(name: string): void { + workerRegisteredOps.add(name); +} + +/** Drop a mirrored name once no worker offers the operation any more. */ +export function unregisterWorkerGrantableOperation(name: string): void { + workerRegisteredOps.delete(name); +} + /** * Validates that every entry in an operations array is a known operation name or group name. * Returns the first invalid entry, or null if all entries are valid. */ export function validateOperations(operations: readonly unknown[]): string | null { for (const op of operations) { - if (typeof op !== 'string' || (!validOps.has(op) && !validGroups.has(op) && !dynamicallyRegisteredOps.has(op))) { + if ( + typeof op !== 'string' || + (!validOps.has(op) && !validGroups.has(op) && !dynamicallyRegisteredOps.has(op) && !workerRegisteredOps.has(op)) + ) { return String(op); } } From c125086e7fd3c9545ed2f9b8a5b350409c2c6e0a Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 21 Aug 2026 11:47:19 -0400 Subject: [PATCH 04/16] Track mirrored grantability per declaring worker The planning review returned better-alternative-exists on the previous approach, and it was right: a name-level mirror set cannot express which worker declared the operation grantable, so it did not enforce the invariant the design claimed ("admissible iff a live worker declares it grantable"). Concretely, a rolling deploy whose new generation keeps an operation but drops `requiresSuperUser` left the name admissible with no live declarer: the routing set stayed non-empty via the new workers, so the mark was never revoked, and `add_role` accepted a grant whose execution then failed closed with operation-not-found. Track claims as name -> Set and re-derive the mirrored mark from live claims, so grantability is retracted when a thread withdraws it or exits even while other workers keep routing the name. Also ignore an announcement from a thread already reported dead: exit notification is deduplicated for the process lifetime, so such an entry could never be cleaned up afterwards. Extract the thread-exit cleanup and export a test seam for it, which is what finally lets the revocation paths be tested at all. Co-Authored-By: Claude Opus 5 --- .../components/registered-operation.test.ts | 3 +- server/serverHelpers/registeredOperations.ts | 85 ++++++++++++++----- .../serverHelpers/serverUtilities.test.js | 62 ++++++++++++-- utility/operationPermissions.ts | 6 +- 4 files changed, 122 insertions(+), 34 deletions(-) diff --git a/integrationTests/components/registered-operation.test.ts b/integrationTests/components/registered-operation.test.ts index ea64a77feb..c90e78920f 100644 --- a/integrationTests/components/registered-operation.test.ts +++ b/integrationTests/components/registered-operation.test.ts @@ -137,8 +137,7 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { }); test('add_role still rejects an op name no component registered', async () => { - // Guards the test above: without this, a validateOperations that stopped running at all - // would look like a pass. + // Guards the test above: a validateOperations that stopped running would look like a pass. const { status, body } = await op({ operation: 'add_role', role: 'component_op_bogus_role', diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index d61b0d3ba9..630fb7576a 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -64,6 +64,17 @@ export function setLocalOperationDispatch(dispatch: typeof localDispatch) { /** name -> threadIds of workers that registered it (main thread only) */ const registeredByWorker = new Map>(); +// name -> threadIds that declared it grantable. Tracked per originator rather than per name so the +// mirrored mark lives exactly as long as a live worker declares it: a rolling deploy whose new +// generation drops `requiresSuperUser` keeps the routing entry (it still executes) while retracting +// grantability, which a name-level flag cannot express. +const grantableByWorker = new Map>(); +// Threads already reported dead. Exit notification is deduplicated for the life of the process +// (manageThreads.notifyThreadExit), so an announcement that lost a race with its own thread's exit +// would otherwise install an entry no later exit event can ever clean up. Never pruned, and never +// wrongly rejects a replacement: worker ids are monotonically increasing and not reused in a +// process, so this grows by one per worker restart (the same reasoning as notifiedDeadThreadIds). +const exitedThreadIds = new Set(); const pendingExecutions = new Map< number, { targetThreadId: number; resolve: (result: any) => void; reject: (error: Error) => void } @@ -96,22 +107,70 @@ export function operationRegisteredHandler(event: { // Arm thread-exit cleanup when the registry gains its first entry, not on the first forward: // a worker that exits before any call would otherwise leave its entries here forever. attachMainListeners(); + if (exitedThreadIds.has(originator)) { + operationLog.debug(`Ignoring operation '${name}' announced by exited worker thread ${originator}`); + return; + } let workerIds = registeredByWorker.get(name); if (!workerIds) registeredByWorker.set(name, (workerIds = new Set())); workerIds.add(originator); // Mirroring only widens what an allowlist may name; enforcement stays on the worker's own - // chooseOperation. - if (grantable) registerWorkerGrantableOperation(name); + // chooseOperation. A re-announcement that no longer declares a permission retracts this + // thread's claim rather than leaving a stale one behind. + setWorkerGrantable(name, originator, grantable === true); operationLog.debug(`Registered operation '${name}' announced by worker thread ${originator}`); } /** - * Forget an operation whose last registering worker is gone. Both prune paths (thread exit, and a - * failed send discovering a dead port) must route through here so routing and grantability can - * never disagree about whether the op is still offered. + * A worker that dies mid-execution can never respond, so fail its in-flight forwards rather than + * waiting out the timeout, and forget its registrations (a replacement re-registers on load). + */ +function handleThreadExit(deadThreadId: number) { + exitedThreadIds.add(deadThreadId); + for (const [name, workerIds] of registeredByWorker) { + workerIds.delete(deadThreadId); + // Retract this thread's grantability claim even when others still route the name — a + // remaining worker that never declared a permission must not keep it admissible. + if (workerIds.size === 0) dropRegistration(name); + else setWorkerGrantable(name, deadThreadId, false); + } + for (const [requestId, pending] of pendingExecutions) { + if (pending.targetThreadId === deadThreadId) { + pendingExecutions.delete(requestId); + pending.reject(new ServerError('The worker thread executing this operation exited', 503)); + } + } +} + +/** Test seam for the cleanup above, which `attachMainListeners` wires to the real thread-exit event. */ +export function notifyThreadExitedForTest(deadThreadId: number) { + handleThreadExit(deadThreadId); +} + +/** Record or retract one thread's grantability claim, then re-derive the mirrored mark from live claims. */ +function setWorkerGrantable(name: string, threadId: number, grantable: boolean) { + let grantableIds = grantableByWorker.get(name); + if (grantable) { + if (!grantableIds) grantableByWorker.set(name, (grantableIds = new Set())); + grantableIds.add(threadId); + } else { + grantableIds?.delete(threadId); + } + if (grantableIds?.size) registerWorkerGrantableOperation(name); + else { + grantableByWorker.delete(name); + unregisterWorkerGrantableOperation(name); + } +} + +/** + * Forget an operation no live worker offers any more. Both prune paths — thread exit, and a failed + * send discovering a dead port — route through here so a name can never keep a route without an + * owner. Grantability is dropped per owner instead, in `setWorkerGrantable`. */ function dropRegistration(name: string) { registeredByWorker.delete(name); + grantableByWorker.delete(name); unregisterWorkerGrantableOperation(name); } @@ -145,21 +204,7 @@ function attachMainListeners() { if (error) pending.reject(new ServerError(error.message, error.statusCode || 500)); else pending.resolve(result); }); - // A worker that dies mid-execution can never respond; fail its in-flight forwards rather - // than waiting out the timeout, and forget its registrations (a replacement worker - // re-registers on component load). - onThreadExit((deadThreadId: number) => { - for (const [name, workerIds] of registeredByWorker) { - workerIds.delete(deadThreadId); - if (workerIds.size === 0) dropRegistration(name); - } - for (const [requestId, pending] of pendingExecutions) { - if (pending.targetThreadId === deadThreadId) { - pendingExecutions.delete(requestId); - pending.reject(new ServerError('The worker thread executing this operation exited', 503)); - } - } - }); + onThreadExit(handleThreadExit); } async function executeRemoteOperation(name: string, body: any, bypassAuth: boolean): Promise { diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 3bdcc75673..2d87dff883 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -233,10 +233,13 @@ describe('Test serverUtilities.js module ', () => { const GRANTABLE = 'test_cross_thread_grantable_op'; const PLAIN = 'test_cross_thread_plain_op'; const SHARED = 'test_cross_thread_shared_op'; + const ROLLED = 'test_cross_thread_rolled_op'; + const RETRACTED = 'test_cross_thread_retracted_op'; + const ZOMBIE = 'test_cross_thread_zombie_op'; after(function () { // Don't leak these names into the process-global registries for later suites. - for (const op of [GRANTABLE, PLAIN, SHARED]) { + for (const op of [GRANTABLE, PLAIN, SHARED, ROLLED, RETRACTED, ZOMBIE]) { unregisterWorkerGrantableOperation(op); unregisterGrantableOperation(op); } @@ -253,21 +256,18 @@ describe('Test serverUtilities.js module ', () => { }); it('leaves an op that declared no permission ungrantable', function () { - // requiresSuperUser omitted means no verifyPerms entry and nothing to grant — the routing - // entry must not smuggle the name into the allowlist. registeredOperations.operationRegisteredHandler({ message: { name: PLAIN, grantable: false, originator: 31 }, }); assert.notEqual(validateOperations([PLAIN]), null); - // A forward is still set up for it, so the routing half is unaffected. + // The routing half is unaffected — only admissibility is withheld. assert.equal(typeof registeredOperations.getRemoteOperationFunction(PLAIN), 'function'); }); it('keeps a main-thread registration of the same name independent of the worker mirror', function () { - // A hot deploy can load a startOnMainThread component that registers an op a retiring - // worker also offers (manageThreads restartWorkers loads root components before draining - // the old workers), so the two marks have to survive each other. + // restartWorkers loads root components before draining old workers, so a startOnMainThread + // component can register an op a retiring worker also offers. registeredOperations.operationRegisteredHandler({ message: { name: SHARED, grantable: true, originator: 41 }, }); @@ -279,6 +279,54 @@ describe('Test serverUtilities.js module ', () => { unregisterGrantableOperation(SHARED); assert.notEqual(validateOperations([SHARED]), null, 'both marks gone should make it ungrantable again'); }); + + it('retracts grantability when a re-announcement drops the declared permission', function () { + registeredOperations.operationRegisteredHandler({ + message: { name: RETRACTED, grantable: true, originator: 51 }, + }); + assert.equal(validateOperations([RETRACTED]), null); + + registeredOperations.operationRegisteredHandler({ + message: { name: RETRACTED, grantable: false, originator: 51 }, + }); + assert.notEqual(validateOperations([RETRACTED]), null, 'the same thread withdrawing must retract its claim'); + }); + + it('stops being grantable once the last declaring worker is gone, even while another still routes it', function () { + // The rolling-deploy case: the new generation keeps the operation but drops + // requiresSuperUser, so the name must stay routable and stop being grantable. + registeredOperations.operationRegisteredHandler({ + message: { name: ROLLED, grantable: true, originator: 61 }, + }); + registeredOperations.operationRegisteredHandler({ + message: { name: ROLLED, grantable: false, originator: 62 }, + }); + assert.equal(validateOperations([ROLLED]), null, 'still declared by thread 61'); + + registeredOperations.notifyThreadExitedForTest(61); + + assert.notEqual(validateOperations([ROLLED]), null, 'no live worker declares it grantable any more'); + assert.equal( + typeof registeredOperations.getRemoteOperationFunction(ROLLED), + 'function', + 'thread 62 still routes it' + ); + }); + + it('ignores an announcement that lost a race with its own thread exit', function () { + registeredOperations.notifyThreadExitedForTest(71); + + registeredOperations.operationRegisteredHandler({ + message: { name: ZOMBIE, grantable: true, originator: 71 }, + }); + + assert.notEqual(validateOperations([ZOMBIE]), null, 'a dead thread must not install a grant'); + assert.equal( + registeredOperations.getRemoteOperationFunction(ZOMBIE), + undefined, + 'nor a route nothing will ever clean up' + ); + }); }); describe('operation authorization state', function () { diff --git a/utility/operationPermissions.ts b/utility/operationPermissions.ts index 67a87b4ae2..45d60e2e27 100644 --- a/utility/operationPermissions.ts +++ b/utility/operationPermissions.ts @@ -120,15 +120,11 @@ export function unregisterGrantableOperation(name: string): void { dynamicallyRegisteredOps.delete(name); } -/** - * Mirror a worker's grantable operation so role validation, which runs on the main thread, accepts - * the name. See server/serverHelpers/registeredOperations.ts — the worker still owns enforcement. - */ +/** Ownership of the mirror lives in registeredOperations.ts; enforcement stays on the worker. */ export function registerWorkerGrantableOperation(name: string): void { workerRegisteredOps.add(name); } -/** Drop a mirrored name once no worker offers the operation any more. */ export function unregisterWorkerGrantableOperation(name: string): void { workerRegisteredOps.delete(name); } From fa56b39701abb9b124e2986d3bc2c47cf0ecde91 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 21 Aug 2026 12:18:08 -0400 Subject: [PATCH 05/16] Close the missed-exit window and retract on failed sends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real gaps found by the gemini and cursor-composer legs, corroborating each other on the second one. Arm the main-thread listeners at module load instead of on first use. `attachMainListeners` ran lazily from the registration handler, but thread-exit notification fires once per thread and is dropped outright when no listener is attached yet — so a worker that died before its first announcement was processed left a registration nothing could ever clean up, and the exited-thread guard never learned about it. serverUtilities imports this module during its own load, before any worker exists, so arming at load is well ordered. Retract grantability when a failed send prunes a dead originator. `executeRemoteOperation` dropped the routing entry but left the claim, so a dead worker could keep a name admissible while a surviving worker that never declared a permission kept routing it — the same false-admissible case this change exists to close. Also guard the ITC payload destructure. A malformed OPERATION_REGISTERED with no `message` would have thrown on the main thread; the envelope is trusted and in-process, but three review rounds have now flagged it and the guard is one expression. Co-Authored-By: Claude Opus 5 --- server/serverHelpers/registeredOperations.ts | 33 ++++++++++--------- server/serverHelpers/serverUtilities.ts | 4 +-- .../serverHelpers/serverUtilities.test.js | 7 ++-- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index 630fb7576a..ffb8176ab6 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -11,8 +11,7 @@ * so a request is sent to exactly ONE registering worker (never broadcast-first-wins). * * - Worker: `registerOperation()` announces the name (OPERATION_REGISTERED) to all threads; - * only the main thread records it, as name -> Set, plus `grantable` (see the - * per-worker registration note in server/DESIGN.md). + * only the main thread records it, as name -> Set, plus `grantable`. * - Main: on an OPERATION_FUNCTION_MAP miss, `getRemoteOperationFunction()` supplies a forwarding * function that sends the request body (OPERATION_EXECUTE_REQUEST) to one live registering * worker and awaits the correlated OPERATION_EXECUTE_RESPONSE. @@ -64,10 +63,8 @@ export function setLocalOperationDispatch(dispatch: typeof localDispatch) { /** name -> threadIds of workers that registered it (main thread only) */ const registeredByWorker = new Map>(); -// name -> threadIds that declared it grantable. Tracked per originator rather than per name so the -// mirrored mark lives exactly as long as a live worker declares it: a rolling deploy whose new -// generation drops `requiresSuperUser` keeps the routing entry (it still executes) while retracting -// grantability, which a name-level flag cannot express. +// Per originator, not per name: a rolling deploy whose new generation drops `requiresSuperUser` +// must keep routing the name while retracting grantability, which a name-level flag cannot express. const grantableByWorker = new Map>(); // Threads already reported dead. Exit notification is deduplicated for the life of the process // (manageThreads.notifyThreadExit), so an announcement that lost a race with its own thread's exit @@ -99,14 +96,11 @@ export function announceRegisteredOperation(name: string, grantable = false) { * ITC handler (all threads receive the broadcast; only main records it). */ export function operationRegisteredHandler(event: { - message: { name: string; grantable?: boolean; originator: number }; + message?: { name?: string; grantable?: boolean; originator?: number }; }) { if (!isMainThread) return; - const { name, grantable, originator } = event.message; + const { name, grantable, originator } = event?.message ?? {}; if (typeof name !== 'string' || typeof originator !== 'number') return; - // Arm thread-exit cleanup when the registry gains its first entry, not on the first forward: - // a worker that exits before any call would otherwise leave its entries here forever. - attachMainListeners(); if (exitedThreadIds.has(originator)) { operationLog.debug(`Ignoring operation '${name}' announced by exited worker thread ${originator}`); return; @@ -114,9 +108,8 @@ export function operationRegisteredHandler(event: { let workerIds = registeredByWorker.get(name); if (!workerIds) registeredByWorker.set(name, (workerIds = new Set())); workerIds.add(originator); - // Mirroring only widens what an allowlist may name; enforcement stays on the worker's own - // chooseOperation. A re-announcement that no longer declares a permission retracts this - // thread's claim rather than leaving a stale one behind. + // Mirroring only widens what an allowlist may name; enforcement stays on the worker's + // chooseOperation. A re-announcement that drops the permission retracts this thread's claim. setWorkerGrantable(name, originator, grantable === true); operationLog.debug(`Registered operation '${name}' announced by worker thread ${originator}`); } @@ -129,8 +122,7 @@ function handleThreadExit(deadThreadId: number) { exitedThreadIds.add(deadThreadId); for (const [name, workerIds] of registeredByWorker) { workerIds.delete(deadThreadId); - // Retract this thread's grantability claim even when others still route the name — a - // remaining worker that never declared a permission must not keep it admissible. + // A surviving worker that never declared a permission must not keep the name admissible. if (workerIds.size === 0) dropRegistration(name); else setWorkerGrantable(name, deadThreadId, false); } @@ -207,6 +199,12 @@ function attachMainListeners() { onThreadExit(handleThreadExit); } +// Armed at load rather than on first use: serverUtilities imports this module during its own load, +// before any worker exists. Thread-exit notification fires once per thread and is dropped outright +// if no listener is attached yet, so a worker dying before its first announcement is processed +// would otherwise leave a registration nothing could ever clean up. +if (isMainThread) attachMainListeners(); + async function executeRemoteOperation(name: string, body: any, bypassAuth: boolean): Promise { attachMainListeners(); const workerIds = registeredByWorker.get(name); @@ -236,7 +234,10 @@ async function executeRemoteOperation(name: string, body: any, bypassAuth: boole ); } if (!sent) { + // The port is gone, so this thread's claims go with it — grantability included, which + // handleThreadExit would otherwise not retract while other workers still route the name. workerIds.delete(targetThreadId); + setWorkerGrantable(name, targetThreadId, false); continue; } return new Promise((promiseResolve, promiseReject) => { diff --git a/server/serverHelpers/serverUtilities.ts b/server/serverHelpers/serverUtilities.ts index d3c3d28db4..841824beeb 100644 --- a/server/serverHelpers/serverUtilities.ts +++ b/server/serverHelpers/serverUtilities.ts @@ -209,8 +209,8 @@ server.registerOperation = (operationDefinition: OperationDefinition) => { OPERATION_FUNCTION_MAP.set(name as any, new OperationFunctionObject(handler)); // Components load per-worker, so a registration made there is invisible to the main-thread // ops-API dispatcher (each thread has its own OPERATION_FUNCTION_MAP instance). Announce it - // so the main thread can forward calls to this worker (#1736), and can mirror the role-allowlist - // mark that registerOperationPermission above made only in this thread's scope. + // so the main thread can forward calls here (#1736), and can mirror the role-allowlist mark that + // registerOperationPermission above made only in this thread's scope. if (!isMainThread) announceRegisteredOperation(name, requiresSuperUser !== undefined); }; diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 2d87dff883..f3a30e5f45 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -246,7 +246,7 @@ describe('Test serverUtilities.js module ', () => { }); it('makes a worker-announced declared op grantable on the main thread', function () { - assert.notEqual(validateOperations([GRANTABLE]), null, 'name should be unknown before the announcement'); + assert.notEqual(validateOperations([GRANTABLE]), null); registeredOperations.operationRegisteredHandler({ message: { name: GRANTABLE, grantable: true, originator: 31 }, @@ -267,7 +267,7 @@ describe('Test serverUtilities.js module ', () => { it('keeps a main-thread registration of the same name independent of the worker mirror', function () { // restartWorkers loads root components before draining old workers, so a startOnMainThread - // component can register an op a retiring worker also offers. + // component can claim a name a retiring worker still offers. registeredOperations.operationRegisteredHandler({ message: { name: SHARED, grantable: true, originator: 41 }, }); @@ -293,8 +293,7 @@ describe('Test serverUtilities.js module ', () => { }); it('stops being grantable once the last declaring worker is gone, even while another still routes it', function () { - // The rolling-deploy case: the new generation keeps the operation but drops - // requiresSuperUser, so the name must stay routable and stop being grantable. + // Rolling deploy: the new generation keeps the operation but drops requiresSuperUser. registeredOperations.operationRegisteredHandler({ message: { name: ROLLED, grantable: true, originator: 61 }, }); From 98e28323c99dc1bb6591e8c587c488927da76b3c Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 21 Aug 2026 12:29:25 -0400 Subject: [PATCH 06/16] Harden the lifecycle tests and trim narration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Synthetic thread ids in the new tests were small positive integers inserted permanently into the module-global tombstone set, which the `after` hook cannot clear — a later suite starting a real worker in the same process could have been assigned one of those ids and had its legitimate announcement ignored. Use ids the runtime will never assign. Cover the failed-send retraction through its production trigger rather than only the exit seam: a forward whose `sendToThread` reports a dead port must retract the claim, not just the route. Also drop comments the review flagged as narrating the line beneath them. Co-Authored-By: Claude Opus 5 --- .../components/registered-operation.test.ts | 1 - server/serverHelpers/registeredOperations.ts | 1 - .../serverHelpers/serverUtilities.test.js | 51 +++++++++++++++---- utility/operationPermissions.ts | 1 - 4 files changed, 41 insertions(+), 13 deletions(-) diff --git a/integrationTests/components/registered-operation.test.ts b/integrationTests/components/registered-operation.test.ts index c90e78920f..aa8026cdd5 100644 --- a/integrationTests/components/registered-operation.test.ts +++ b/integrationTests/components/registered-operation.test.ts @@ -137,7 +137,6 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { }); test('add_role still rejects an op name no component registered', async () => { - // Guards the test above: a validateOperations that stopped running would look like a pass. const { status, body } = await op({ operation: 'add_role', role: 'component_op_bogus_role', diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index ffb8176ab6..800b7856f2 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -139,7 +139,6 @@ export function notifyThreadExitedForTest(deadThreadId: number) { handleThreadExit(deadThreadId); } -/** Record or retract one thread's grantability claim, then re-derive the mirrored mark from live claims. */ function setWorkerGrantable(name: string, threadId: number, grantable: boolean) { let grantableIds = grantableByWorker.get(name); if (grantable) { diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index f3a30e5f45..651d8d764b 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -236,10 +236,16 @@ describe('Test serverUtilities.js module ', () => { const ROLLED = 'test_cross_thread_rolled_op'; const RETRACTED = 'test_cross_thread_retracted_op'; const ZOMBIE = 'test_cross_thread_zombie_op'; + const FAILED_SEND = 'test_cross_thread_failed_send_op'; + // Tombstones in exitedThreadIds are permanent and module-global, so synthetic ids must be + // ones the runtime will never assign to a real worker in this process. + const DECLARING_THREAD = 9_000_061; + const ROUTING_THREAD = 9_000_062; + const DEAD_THREAD = 9_000_071; + const SENDER_THREAD = 9_000_081; after(function () { - // Don't leak these names into the process-global registries for later suites. - for (const op of [GRANTABLE, PLAIN, SHARED, ROLLED, RETRACTED, ZOMBIE]) { + for (const op of [GRANTABLE, PLAIN, SHARED, ROLLED, RETRACTED, ZOMBIE, FAILED_SEND]) { unregisterWorkerGrantableOperation(op); unregisterGrantableOperation(op); } @@ -261,7 +267,6 @@ describe('Test serverUtilities.js module ', () => { }); assert.notEqual(validateOperations([PLAIN]), null); - // The routing half is unaffected — only admissibility is withheld. assert.equal(typeof registeredOperations.getRemoteOperationFunction(PLAIN), 'function'); }); @@ -295,28 +300,54 @@ describe('Test serverUtilities.js module ', () => { it('stops being grantable once the last declaring worker is gone, even while another still routes it', function () { // Rolling deploy: the new generation keeps the operation but drops requiresSuperUser. registeredOperations.operationRegisteredHandler({ - message: { name: ROLLED, grantable: true, originator: 61 }, + message: { name: ROLLED, grantable: true, originator: DECLARING_THREAD }, }); registeredOperations.operationRegisteredHandler({ - message: { name: ROLLED, grantable: false, originator: 62 }, + message: { name: ROLLED, grantable: false, originator: ROUTING_THREAD }, }); - assert.equal(validateOperations([ROLLED]), null, 'still declared by thread 61'); + assert.equal(validateOperations([ROLLED]), null, 'still declared by the first thread'); - registeredOperations.notifyThreadExitedForTest(61); + registeredOperations.notifyThreadExitedForTest(DECLARING_THREAD); assert.notEqual(validateOperations([ROLLED]), null, 'no live worker declares it grantable any more'); assert.equal( typeof registeredOperations.getRemoteOperationFunction(ROLLED), 'function', - 'thread 62 still routes it' + 'the surviving worker still routes it' ); }); + it('retracts grantability when a failed send prunes the declaring worker', async function () { + const originalThreads = global.threads; + global.threads = { + sendToThread() { + return false; + }, + }; + try { + registeredOperations.operationRegisteredHandler({ + message: { name: FAILED_SEND, grantable: true, originator: SENDER_THREAD }, + }); + assert.equal(validateOperations([FAILED_SEND]), null); + + const forward = registeredOperations.getRemoteOperationFunction(FAILED_SEND, true); + await assert.rejects(forward({ operation: FAILED_SEND }), /no worker thread is available/); + + assert.notEqual( + validateOperations([FAILED_SEND]), + null, + 'a dead port must retract the claim, not just the route' + ); + } finally { + global.threads = originalThreads; + } + }); + it('ignores an announcement that lost a race with its own thread exit', function () { - registeredOperations.notifyThreadExitedForTest(71); + registeredOperations.notifyThreadExitedForTest(DEAD_THREAD); registeredOperations.operationRegisteredHandler({ - message: { name: ZOMBIE, grantable: true, originator: 71 }, + message: { name: ZOMBIE, grantable: true, originator: DEAD_THREAD }, }); assert.notEqual(validateOperations([ZOMBIE]), null, 'a dead thread must not install a grant'); diff --git a/utility/operationPermissions.ts b/utility/operationPermissions.ts index 45d60e2e27..cb0ae6785c 100644 --- a/utility/operationPermissions.ts +++ b/utility/operationPermissions.ts @@ -120,7 +120,6 @@ export function unregisterGrantableOperation(name: string): void { dynamicallyRegisteredOps.delete(name); } -/** Ownership of the mirror lives in registeredOperations.ts; enforcement stays on the worker. */ export function registerWorkerGrantableOperation(name: string): void { workerRegisteredOps.add(name); } From 61d1b97ab56da56a4f691e7acef0b53c2f80f306 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 21 Aug 2026 12:44:37 -0400 Subject: [PATCH 07/16] Skip cleanup for operations the exiting thread never registered Both suggestions from the gemini review, and both behaviour-preserving: a grantability claim implies a registration, since claims are only recorded alongside one, so an operation the dead thread was not registered for can have no claim to retract either. `handleThreadExit` now continues when the id was not in the routing set, and `setWorkerGrantable` only re-derives the mirrored mark when a claim was actually removed, instead of unregistering a name it never held. Co-Authored-By: Claude Opus 5 --- server/serverHelpers/registeredOperations.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index 800b7856f2..665c23ec75 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -121,7 +121,7 @@ export function operationRegisteredHandler(event: { function handleThreadExit(deadThreadId: number) { exitedThreadIds.add(deadThreadId); for (const [name, workerIds] of registeredByWorker) { - workerIds.delete(deadThreadId); + if (!workerIds.delete(deadThreadId)) continue; // A surviving worker that never declared a permission must not keep the name admissible. if (workerIds.size === 0) dropRegistration(name); else setWorkerGrantable(name, deadThreadId, false); @@ -144,11 +144,8 @@ function setWorkerGrantable(name: string, threadId: number, grantable: boolean) if (grantable) { if (!grantableIds) grantableByWorker.set(name, (grantableIds = new Set())); grantableIds.add(threadId); - } else { - grantableIds?.delete(threadId); - } - if (grantableIds?.size) registerWorkerGrantableOperation(name); - else { + registerWorkerGrantableOperation(name); + } else if (grantableIds?.delete(threadId) && grantableIds.size === 0) { grantableByWorker.delete(name); unregisterWorkerGrantableOperation(name); } From b1b267ecd1834266a672442cd21b0a7f661106b4 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 21 Aug 2026 16:56:37 -0400 Subject: [PATCH 08/16] Cover the OIDC trust-policy consumer now that #2173 has landed `add_oidc_trust` is the third main-thread caller of `validateOperations`, and until this change its own source carried a `Known limitation` note saying a component-registered operation "is NOT recognized here and a policy naming one is rejected", pointing at this bridge as where the fix belonged. That note is now false, so remove it rather than leave a comment describing behaviour the code no longer has. The operation was absent from the checkout when this branch started, which is why the earlier rounds could only cover add_role/alter_role and impersonation. Add the two cases that were always wanted: a trust policy naming a component-registered operation is accepted, and one naming an unregistered operation is still rejected. Co-Authored-By: Claude Opus 5 --- .../components/registered-operation.test.ts | 39 +++++++++++++++++++ security/authn/oidc/trustPolicyOperations.ts | 9 ----- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/integrationTests/components/registered-operation.test.ts b/integrationTests/components/registered-operation.test.ts index aa8026cdd5..6376a36aaa 100644 --- a/integrationTests/components/registered-operation.test.ts +++ b/integrationTests/components/registered-operation.test.ts @@ -26,6 +26,17 @@ const GRANTED_USER = 'component_op_granted_user'; const UNGRANTED_ROLE = 'component_op_ungranted_role'; const UNGRANTED_USER = 'component_op_ungranted_user'; const USER_PASS = 'Abc1234!'; +// A trust policy the OIDC operation will accept on every axis except the one under test, so a +// rejection can only be about the operation name. Shapes taken from the GitHub Actions profile's +// own requirements: a canonical audience (explicit port, trailing slash) and specific claims. +const TRUST_POLICY = { + issuer: 'https://token.actions.githubusercontent.com', + audience: 'https://my-instance.harperdb.io:9925/', + claims: { + repository_id: '67890', + workflow_ref: 'HarperFast/my-app/.github/workflows/deploy.yml@refs/heads/main', + }, +}; suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { async function op(body: any): Promise<{ status: number; body: any }> { @@ -195,6 +206,34 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { strictEqual(status, 403, JSON.stringify(body)); }); + test('add_oidc_trust accepts the op in a trust policy scope', async () => { + // The third main-thread consumer of validateOperations, and the one whose own source + // documented this gap as a known limitation until this change. + const { status, body } = await op({ + operation: 'add_oidc_trust', + id: 'component-op-policy', + ...TRUST_POLICY, + user: GRANTED_USER, + operations: [GRANTABLE_OP], + }); + strictEqual(status, 200, JSON.stringify(body)); + }); + + test('add_oidc_trust still rejects an op name no component registered', async () => { + const { status, body } = await op({ + operation: 'add_oidc_trust', + id: 'component-op-bogus-policy', + ...TRUST_POLICY, + user: GRANTED_USER, + operations: ['component_registered_never_declared'], + }); + strictEqual(status, 400, JSON.stringify(body)); + ok( + JSON.stringify(body).includes('not a Harper operation'), + `expected the trust-policy rejection, got: ${JSON.stringify(body)}` + ); + }); + test('impersonation accepts an inline role naming the op', async () => { const { status, body } = await op({ operation: GRANTABLE_OP, diff --git a/security/authn/oidc/trustPolicyOperations.ts b/security/authn/oidc/trustPolicyOperations.ts index b4d1a1f4ee..42fc8f39bd 100644 --- a/security/authn/oidc/trustPolicyOperations.ts +++ b/security/authn/oidc/trustPolicyOperations.ts @@ -53,15 +53,6 @@ function validate(validation: any): void { * caught here, where the reader is the administrator who wrote it. Delegates to the same helper * add_role/alter_role use, so group names resolve identically rather than through a second * definition that could drift. - * - * Known limitation, inherited rather than introduced: that helper's registry of runtime-registered - * operations is process-local, and the OPERATION_REGISTERED bridge propagates only name→thread - * routing, never grantability (server/serverHelpers/registeredOperations.ts). A component's - * `server.registerOperation` runs in a worker while this operation runs on the main thread, so an - * operation registered that way is NOT recognized here and a policy naming one is rejected. It - * fails closed — a rejected policy, never a widened one — and `add_role`, `alter_role`, and - * impersonation validation all share the gap, which is why the fix belongs to that bridge rather - * than to a local workaround here. */ function assertOperationsAreKnown(operations: string[]): void { const invalidOperation = validateOperations(operations); From 53a3049e5fa856d8db5c4802c310ae630745514e Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 21 Aug 2026 17:05:18 -0400 Subject: [PATCH 09/16] Correct the OIDC test comment that still described the old gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment on "accepts an operation registered in this process" said a component's operation "is NOT recognized here in production" and named this bridge as where the fix belonged. The test itself is unchanged and still correct — it asserts the delegation to validateOperations — but its explanation described behaviour this branch removes. Surfaced by the review leg grepping for leftovers after the source comment came out, which is the half-true remnant that sweep was looking for. Co-Authored-By: Claude Opus 5 --- .../authn/oidc/trustPolicyOperations.test.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/unitTests/security/authn/oidc/trustPolicyOperations.test.js b/unitTests/security/authn/oidc/trustPolicyOperations.test.js index a1542365b5..ec5e7bddf2 100644 --- a/unitTests/security/authn/oidc/trustPolicyOperations.test.js +++ b/unitTests/security/authn/oidc/trustPolicyOperations.test.js @@ -264,15 +264,10 @@ describe('oidc trustPolicyOperations', () => { assert.strictEqual(installed.mock.rows.size, 0, 'expected nothing stored'); }); - // Delegating to validateOperations rather than checking OPERATIONS_ENUM locally means an - // operation registered in THIS process is accepted. That is the seam, not a promise about - // component-registered operations: the registry is process-local, `add_oidc_trust` runs on the - // main thread, and `server.registerOperation` runs in a worker whose OPERATION_REGISTERED - // announcement carries only name→thread routing, never grantability. So a component's - // operation is NOT recognized here in production, and this same-thread test cannot show that - // — it is asserting the delegation, not the topology. A policy naming such an operation is - // rejected, which fails closed; add_role, alter_role, and impersonation validation share the - // gap, so the fix belongs to that bridge rather than to a workaround here. + // Asserts the delegation to validateOperations, not the worker→main topology: registering here + // puts the mark in this thread's own registry, so a same-thread test cannot distinguish the two. + // The cross-thread path a real component takes is covered in + // integrationTests/components/registered-operation.test.ts. it('accepts an operation registered in this process', async () => { const dynamicOp = 'test_dynamic_scope_op'; opAuth.registerOperationPermission(dynamicOp, { requiresSu: true }); From 5daaa6d96a496a52e134bb7761ad9b7bd99928d0 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 21 Aug 2026 17:12:34 -0400 Subject: [PATCH 10/16] Retract the permission entry when a re-registration drops the flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-model review found a real authorization divergence this branch introduced. Registering an operation with `requiresSuperUser` installs a `requiredPermissions` entry keyed by the operation name; re-registering the same name without the flag left that entry in place. Before this branch that was inert, because the operation could never have been granted in the first place. Now main retracts the mirrored grantable mark on the re-announcement while the worker keeps honouring a role grant persisted earlier, so the declaration and the enforcement disagree — reachable whenever the handler's own `.name` matches the operation name, which is a natural way to write one. Retract the entry alongside the mark, tracking the names this API installed so a component re-declaring a built-in's name cannot strip the built-in's permission. Tests cover both directions: a persisted grant stops being honoured once the declaration is dropped, and an entry registered by anyone else survives. Also finishes the limitation cleanup the review caught mid-flight and trims the comments it flagged as narration. Co-Authored-By: Claude Opus 5 --- .../components/registered-operation.test.ts | 3 +- server/serverHelpers/registeredOperations.ts | 2 +- server/serverHelpers/serverUtilities.ts | 13 ++++- .../serverHelpers/serverUtilities.test.js | 55 ++++++++++++++++++- 4 files changed, 67 insertions(+), 6 deletions(-) diff --git a/integrationTests/components/registered-operation.test.ts b/integrationTests/components/registered-operation.test.ts index 6376a36aaa..86783c12bb 100644 --- a/integrationTests/components/registered-operation.test.ts +++ b/integrationTests/components/registered-operation.test.ts @@ -207,8 +207,7 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { }); test('add_oidc_trust accepts the op in a trust policy scope', async () => { - // The third main-thread consumer of validateOperations, and the one whose own source - // documented this gap as a known limitation until this change. + // The third main-thread consumer of validateOperations. const { status, body } = await op({ operation: 'add_oidc_trust', id: 'component-op-policy', diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index 665c23ec75..3de4ccb860 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -134,7 +134,7 @@ function handleThreadExit(deadThreadId: number) { } } -/** Test seam for the cleanup above, which `attachMainListeners` wires to the real thread-exit event. */ +/** `attachMainListeners` wires the same function to the real thread-exit event. */ export function notifyThreadExitedForTest(deadThreadId: number) { handleThreadExit(deadThreadId); } diff --git a/server/serverHelpers/serverUtilities.ts b/server/serverHelpers/serverUtilities.ts index 841824beeb..c89f494812 100644 --- a/server/serverHelpers/serverUtilities.ts +++ b/server/serverHelpers/serverUtilities.ts @@ -186,6 +186,10 @@ export type OperationDefinition = { requiresSuperUser?: boolean; }; +// Operation names this API installed a permission entry for, so a later registration that drops +// `requiresSuperUser` can retract exactly that entry and nothing else. +const declaredPermissionNames = new Set(); + /** * Register an operation function with the server. * @param operationDefinition @@ -195,7 +199,13 @@ server.registerOperation = (operationDefinition: OperationDefinition) => { if (isDeployValidating()) return; const { name, execute, requiresSuperUser } = operationDefinition; let handler = execute; - if (requiresSuperUser !== undefined) { + if (requiresSuperUser === undefined) { + // A re-registration that drops the flag must also drop the entry the earlier one installed, + // or the declaration and enforcement disagree: main retracts the grantable mark while this + // worker keeps honouring an already-persisted role grant. Only names this API registered are + // cleared, so a component cannot strip a built-in's permission by re-declaring its name. + if (declaredPermissionNames.delete(name)) opAuth.unregisterOperationPermission(name); + } else { // verifyPerms keys requiredPermissions by the handler's function `.name`, but registered ops // are typically anonymous arrows (all named "execute") which collide and can't be keyed. Wrap // in a FRESH function named after the op so the lookup resolves the right entry. Wrap rather @@ -205,6 +215,7 @@ server.registerOperation = (operationDefinition: OperationDefinition) => { handler = (...args: any[]) => (execute as any)(...args); Object.defineProperty(handler, 'name', { value: name, configurable: true }); opAuth.registerOperationPermission(name, { requiresSu: requiresSuperUser }); + declaredPermissionNames.add(name); } OPERATION_FUNCTION_MAP.set(name as any, new OperationFunctionObject(handler)); // Components load per-worker, so a registration made there is invisible to the main-thread diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 651d8d764b..8e18260878 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -298,7 +298,6 @@ describe('Test serverUtilities.js module ', () => { }); it('stops being grantable once the last declaring worker is gone, even while another still routes it', function () { - // Rolling deploy: the new generation keeps the operation but drops requiresSuperUser. registeredOperations.operationRegisteredHandler({ message: { name: ROLLED, grantable: true, originator: DECLARING_THREAD }, }); @@ -938,7 +937,15 @@ describe('Test serverUtilities.js module ', () => { // Keep the process-global registries clean — these test-only ops shouldn't leak into other // suites. registerOperation touches three globals (the op-function map plus verifyPerms' // requiredPermissions and the grantable-ops set), so undo all three, not just the map. - for (const op of [SU_OP, OPEN_OP, 'test_name_pinning_op', 'shared_op_a', 'shared_op_b', 'dyn_grantable_op']) { + for (const op of [ + SU_OP, + OPEN_OP, + 'test_name_pinning_op', + 'shared_op_a', + 'shared_op_b', + 'dyn_grantable_op', + 'test_redeclared_op', + ]) { serverUtilities.OPERATION_FUNCTION_MAP.delete(op); op_auth.unregisterOperationPermission(op); } @@ -973,6 +980,50 @@ describe('Test serverUtilities.js module ', () => { assert.equal(validateOperations(['dyn_grantable_op']), null); // grantable after registration }); + it('retracts the permission entry when a re-registration drops requiresSuperUser', function () { + // Declaration and enforcement must not disagree: main retracts the grantable mark on the + // re-announcement, so a role grant persisted while the op was declared must stop being + // honoured here too. Named after the op so verifyPerms resolves the stale entry if it + // survives — an anonymous handler would mask the bug rather than test it. + const op = 'test_redeclared_op'; + // eslint-disable-next-line func-names + const named = { + [op]: async function () { + return {}; + }, + }[op]; + server.registerOperation({ name: op, execute: named, requiresSuperUser: true }); + assert.equal(validateOperations([op]), null, 'grantable while declared'); + assert.equal(op_auth.verifyPerms(nonSuRequest(op, [op]), op), null, 'granted role may call it'); + + server.registerOperation({ name: op, execute: named }); + + assert.notEqual(validateOperations([op]), null, 'no longer grantable once undeclared'); + let threw; + try { + op_auth.verifyPerms(nonSuRequest(op, [op]), op); + } catch (err) { + threw = err; + } + assert.ok(threw, 'a persisted grant must not survive the declaration being dropped'); + assert.equal(threw.statusCode, 400); + }); + + it('does not strip a permission entry this API never registered', function () { + // Ownership protection, exercised against an independent registrant rather than a real + // built-in so the assertion does not depend on mutating shared dispatch state. + const op = 'test_independent_registrant_op'; + op_auth.registerOperationPermission(op, { requiresSu: true }); + try { + server.registerOperation({ name: op, execute: async () => ({}) }); + assert.equal(validateOperations([op]), null, 'an entry this API did not create must survive'); + assert.ok(op_auth.verifyPerms(nonSuRequest(op), op), 'and must still gate a non-super_user'); + } finally { + op_auth.unregisterOperationPermission(op); + serverUtilities.OPERATION_FUNCTION_MAP.delete(op); + } + }); + it('does not touch handler name or register perms when requiresSuperUser is omitted (opt-in)', function () { const def = { name: OPEN_OP, execute: async () => ({}) }; server.registerOperation(def); From b7d33a07910040adbcf5d1014c3848d719b7ff9a Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 24 Aug 2026 14:14:58 -0400 Subject: [PATCH 11/16] Narrow the ownership claim in the re-registration comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment overstated the guard: a bare flagless registration cannot clear a built-in's entry, but declaring that name first puts it in declaredPermissionNames, so a later flagless registration can. The declaring call already overwrote the built-in entry at that point, so this only follows it — but the comment claimed a guarantee the code does not make. Also drops the two comments the review flagged as restating the line beneath. Co-Authored-By: Claude Opus 5 --- integrationTests/components/registered-operation.test.ts | 1 - server/serverHelpers/registeredOperations.ts | 1 - server/serverHelpers/serverUtilities.ts | 9 +++++---- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/integrationTests/components/registered-operation.test.ts b/integrationTests/components/registered-operation.test.ts index 86783c12bb..34d2e4e486 100644 --- a/integrationTests/components/registered-operation.test.ts +++ b/integrationTests/components/registered-operation.test.ts @@ -207,7 +207,6 @@ suite('Component: registered-operation (#1736)', (ctx: ContextWithHarper) => { }); test('add_oidc_trust accepts the op in a trust policy scope', async () => { - // The third main-thread consumer of validateOperations. const { status, body } = await op({ operation: 'add_oidc_trust', id: 'component-op-policy', diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index 3de4ccb860..e0e94b794b 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -134,7 +134,6 @@ function handleThreadExit(deadThreadId: number) { } } -/** `attachMainListeners` wires the same function to the real thread-exit event. */ export function notifyThreadExitedForTest(deadThreadId: number) { handleThreadExit(deadThreadId); } diff --git a/server/serverHelpers/serverUtilities.ts b/server/serverHelpers/serverUtilities.ts index c89f494812..e7c06cc148 100644 --- a/server/serverHelpers/serverUtilities.ts +++ b/server/serverHelpers/serverUtilities.ts @@ -200,10 +200,11 @@ server.registerOperation = (operationDefinition: OperationDefinition) => { const { name, execute, requiresSuperUser } = operationDefinition; let handler = execute; if (requiresSuperUser === undefined) { - // A re-registration that drops the flag must also drop the entry the earlier one installed, - // or the declaration and enforcement disagree: main retracts the grantable mark while this - // worker keeps honouring an already-persisted role grant. Only names this API registered are - // cleared, so a component cannot strip a built-in's permission by re-declaring its name. + // A re-registration that drops the flag must also drop the entry the earlier one installed, or + // declaration and enforcement disagree: main retracts the grantable mark while this worker keeps + // honouring an already-persisted role grant. Scoped to names declared through this API, so one it + // never declared is untouched — a component that declared a built-in's name already overwrote + // that entry by declaring it, and this only follows. if (declaredPermissionNames.delete(name)) opAuth.unregisterOperationPermission(name); } else { // verifyPerms keys requiredPermissions by the handler's function `.name`, but registered ops From d97a5cc547b4cb6307fad1fab406680eeaba7f40 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 24 Aug 2026 15:56:12 -0400 Subject: [PATCH 12/16] Reuse the shared dead-thread registry instead of a second one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pointed out that jobs launch a fresh `autoRestart: false` worker per job (server/jobs/jobRunner.ts), so a per-thread tombstone here grows with every completed job on a long-lived node — not once per worker restart, which is what the comment claimed. `manageThreads` already records exactly this in `notifiedDeadThreadIds`, and records it before firing exit listeners, so the local set was duplicating state that was already there and already correct. Expose a sync `hasThreadExited` from `manageThreads` and read that instead. `isThreadRunning` cannot serve: it is async because it awaits process-group confirmation, and this runs on a synchronous announcement path. Export `notifyThreadExit` too, which lets the lifecycle tests drive the real exit path and removes `notifyThreadExitedForTest` from production entirely. The tests now exercise the `onThreadExit` wiring they previously bypassed. Co-Authored-By: Claude Opus 5 --- server/serverHelpers/registeredOperations.ts | 19 ++++++------------- server/threads/manageThreads.js | 13 +++++++++++++ .../serverHelpers/serverUtilities.test.js | 5 +++-- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index e0e94b794b..e62ed51124 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -26,7 +26,7 @@ import * as env from '../../utility/environment/environmentManager.ts'; import harperLogger from '../../utility/logging/harper_logger.ts'; import { ServerError } from '../../utility/errors/hdbError.ts'; import { sendItcEvent } from '../threads/itc.js'; -import { onMessageByType, onThreadExit } from '../threads/manageThreads.js'; +import { hasThreadExited, onMessageByType, onThreadExit } from '../threads/manageThreads.js'; import { registerWorkerGrantableOperation, unregisterWorkerGrantableOperation, @@ -66,12 +66,6 @@ const registeredByWorker = new Map>(); // Per originator, not per name: a rolling deploy whose new generation drops `requiresSuperUser` // must keep routing the name while retracting grantability, which a name-level flag cannot express. const grantableByWorker = new Map>(); -// Threads already reported dead. Exit notification is deduplicated for the life of the process -// (manageThreads.notifyThreadExit), so an announcement that lost a race with its own thread's exit -// would otherwise install an entry no later exit event can ever clean up. Never pruned, and never -// wrongly rejects a replacement: worker ids are monotonically increasing and not reused in a -// process, so this grows by one per worker restart (the same reasoning as notifiedDeadThreadIds). -const exitedThreadIds = new Set(); const pendingExecutions = new Map< number, { targetThreadId: number; resolve: (result: any) => void; reject: (error: Error) => void } @@ -101,7 +95,11 @@ export function operationRegisteredHandler(event: { if (!isMainThread) return; const { name, grantable, originator } = event?.message ?? {}; if (typeof name !== 'string' || typeof originator !== 'number') return; - if (exitedThreadIds.has(originator)) { + // An announcement can lose the race with its own thread's exit, and exit notification fires once + // per thread, so without this the entry would never be cleaned up. Reads manageThreads' tombstone + // rather than keeping a second one: job threads are one-shot workers, so a duplicate here would + // grow per completed job, not per worker restart. + if (hasThreadExited(originator)) { operationLog.debug(`Ignoring operation '${name}' announced by exited worker thread ${originator}`); return; } @@ -119,7 +117,6 @@ export function operationRegisteredHandler(event: { * waiting out the timeout, and forget its registrations (a replacement re-registers on load). */ function handleThreadExit(deadThreadId: number) { - exitedThreadIds.add(deadThreadId); for (const [name, workerIds] of registeredByWorker) { if (!workerIds.delete(deadThreadId)) continue; // A surviving worker that never declared a permission must not keep the name admissible. @@ -134,10 +131,6 @@ function handleThreadExit(deadThreadId: number) { } } -export function notifyThreadExitedForTest(deadThreadId: number) { - handleThreadExit(deadThreadId); -} - function setWorkerGrantable(name: string, threadId: number, grantable: boolean) { let grantableIds = grantableByWorker.get(name); if (grantable) { diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 4581f067d1..73695d6eda 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -152,6 +152,10 @@ module.exports = { restoreShutdownDeadline, registerWorkerDataProvider, onThreadExit, + hasThreadExited, + // Exported so tests can drive the real exit path rather than a module-local stand-in; the + // dedupe above makes a spurious call a no-op. + notifyThreadExit, registerProcessGroup, unregisterProcessGroup, isThreadRunning, @@ -1119,6 +1123,15 @@ if (isMainThread) { }); } +/** + * Whether a thread has already been reported dead. Sync, unlike `isThreadRunning`, because it only + * reads the tombstone `notifyThreadExit` records below — callers on the exit path need an answer + * without awaiting process-group confirmation. + */ +function hasThreadExited(threadId) { + return notifiedDeadThreadIds.has(threadId); +} + function notifyThreadExit(deadThreadId) { if (deadThreadId == null || notifiedDeadThreadIds.has(deadThreadId)) return; notifiedDeadThreadIds.add(deadThreadId); diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 8e18260878..f882712024 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -9,6 +9,7 @@ const sandbox = sinon.createSandbox(); const { TEST_JSON_SUPER_USER, TEST_JSON_NON_SU } = require('../../test_data'); const serverUtilities = require('#src/server/serverHelpers/serverUtilities'); const registeredOperations = require('#src/server/serverHelpers/registeredOperations'); +const manageThreads = require('#src/server/threads/manageThreads'); const operationAuthorizationState = require('#src/server/serverHelpers/operationAuthorizationState'); const { runWithDeployValidationGuard } = require('#src/server/serverHelpers/deployValidationState'); const quota = require('#src/components/mcp/quota'); @@ -306,7 +307,7 @@ describe('Test serverUtilities.js module ', () => { }); assert.equal(validateOperations([ROLLED]), null, 'still declared by the first thread'); - registeredOperations.notifyThreadExitedForTest(DECLARING_THREAD); + manageThreads.notifyThreadExit(DECLARING_THREAD); assert.notEqual(validateOperations([ROLLED]), null, 'no live worker declares it grantable any more'); assert.equal( @@ -343,7 +344,7 @@ describe('Test serverUtilities.js module ', () => { }); it('ignores an announcement that lost a race with its own thread exit', function () { - registeredOperations.notifyThreadExitedForTest(DEAD_THREAD); + manageThreads.notifyThreadExit(DEAD_THREAD); registeredOperations.operationRegisteredHandler({ message: { name: ZOMBIE, grantable: true, originator: DEAD_THREAD }, From dca12d545fa0022161e7d8939a0c74cc17f0ee96 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 24 Aug 2026 16:04:13 -0400 Subject: [PATCH 13/16] Drop the inaccurate export-list comment It placed the dedupe "above" the export when notifyThreadExit is defined far below it, and an export list is not where that rationale belongs. Co-Authored-By: Claude Opus 5 --- server/threads/manageThreads.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 73695d6eda..9182242eae 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -153,8 +153,6 @@ module.exports = { registerWorkerDataProvider, onThreadExit, hasThreadExited, - // Exported so tests can drive the real exit path rather than a module-local stand-in; the - // dedupe above makes a spurious call a no-op. notifyThreadExit, registerProcessGroup, unregisterProcessGroup, From 5380480d7ba05a1632ae445318d8fdf0fc739782 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Tue, 25 Aug 2026 19:18:20 -0400 Subject: [PATCH 14/16] Describe the tombstone without naming a private set The test comment still said exitedThreadIds, which no longer exists. Phrased without naming the holding set so it stays true regardless of which module owns it. Co-Authored-By: Claude Opus 5 --- unitTests/server/serverHelpers/serverUtilities.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index f882712024..95bb2dc1ae 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -238,8 +238,8 @@ describe('Test serverUtilities.js module ', () => { const RETRACTED = 'test_cross_thread_retracted_op'; const ZOMBIE = 'test_cross_thread_zombie_op'; const FAILED_SEND = 'test_cross_thread_failed_send_op'; - // Tombstones in exitedThreadIds are permanent and module-global, so synthetic ids must be - // ones the runtime will never assign to a real worker in this process. + // Thread-exit tombstones are permanent and process-global, so synthetic ids must be ones the + // runtime will never assign to a real worker. const DECLARING_THREAD = 9_000_061; const ROUTING_THREAD = 9_000_062; const DEAD_THREAD = 9_000_071; From 9d66638a48820b195dd6cdc97caa3242d86ad70b Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 26 Aug 2026 09:41:52 -0400 Subject: [PATCH 15/16] Keep the dead-thread tombstone local to this module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the shared-registry reuse from d97a5cc54. CI bisects a Windows regression to that commit: `Integration Tests 2/6 (Windows)` fails with it on two independent builds (375s, 389s, then 465s on a fresh build) and passes without it (259s), matching main at 262-280s. `manageThreads.js` is untouched again as a result. I do not have a root cause. Reading `notifiedDeadThreadIds` instead of an equivalent local Set should not cost minutes of HTTP-worker readiness, and it does not reproduce on macOS, so the revert is on evidence rather than understanding. Kris's underlying point stands and is answered in the comment instead: the set holds one integer per dead thread, which is the same growth profile manageThreads already accepts for notifiedDeadThreadIds a few lines from where it records them. Narrowing it to threads that already hold a registration was tried and rejected — it defeats the guard's purpose, because the case it exists for is a thread whose FIRST announcement is in flight when it dies, and such a thread holds no registration at exit time. A unit test covers that. Co-Authored-By: Claude Opus 5 --- server/serverHelpers/registeredOperations.ts | 21 +++++++++++++------ server/threads/manageThreads.js | 11 ---------- .../serverHelpers/serverUtilities.test.js | 5 ++--- 3 files changed, 17 insertions(+), 20 deletions(-) diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index e62ed51124..1d5d4cdac0 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -26,7 +26,7 @@ import * as env from '../../utility/environment/environmentManager.ts'; import harperLogger from '../../utility/logging/harper_logger.ts'; import { ServerError } from '../../utility/errors/hdbError.ts'; import { sendItcEvent } from '../threads/itc.js'; -import { hasThreadExited, onMessageByType, onThreadExit } from '../threads/manageThreads.js'; +import { onMessageByType, onThreadExit } from '../threads/manageThreads.js'; import { registerWorkerGrantableOperation, unregisterWorkerGrantableOperation, @@ -66,6 +66,14 @@ const registeredByWorker = new Map>(); // Per originator, not per name: a rolling deploy whose new generation drops `requiresSuperUser` // must keep routing the name while retracting grantability, which a name-level flag cannot express. const grantableByWorker = new Map>(); +// Threads already reported dead, so an announcement that lost the race with its own thread's exit +// cannot install an entry no later exit event will clean up — including a thread whose FIRST +// announcement is the one in flight, which is why this cannot be narrowed to threads already +// holding a registration. One integer per dead thread, never pruned, matching the growth profile +// manageThreads accepts for notifiedDeadThreadIds; it cannot reject a replacement because worker +// ids are monotonically increasing and not reused within a process. Deliberately a local set: +// reading manageThreads' equivalent regressed Windows worker-restart timing (see PR #2260). +const exitedThreadIds = new Set(); const pendingExecutions = new Map< number, { targetThreadId: number; resolve: (result: any) => void; reject: (error: Error) => void } @@ -95,11 +103,7 @@ export function operationRegisteredHandler(event: { if (!isMainThread) return; const { name, grantable, originator } = event?.message ?? {}; if (typeof name !== 'string' || typeof originator !== 'number') return; - // An announcement can lose the race with its own thread's exit, and exit notification fires once - // per thread, so without this the entry would never be cleaned up. Reads manageThreads' tombstone - // rather than keeping a second one: job threads are one-shot workers, so a duplicate here would - // grow per completed job, not per worker restart. - if (hasThreadExited(originator)) { + if (exitedThreadIds.has(originator)) { operationLog.debug(`Ignoring operation '${name}' announced by exited worker thread ${originator}`); return; } @@ -117,6 +121,7 @@ export function operationRegisteredHandler(event: { * waiting out the timeout, and forget its registrations (a replacement re-registers on load). */ function handleThreadExit(deadThreadId: number) { + exitedThreadIds.add(deadThreadId); for (const [name, workerIds] of registeredByWorker) { if (!workerIds.delete(deadThreadId)) continue; // A surviving worker that never declared a permission must not keep the name admissible. @@ -131,6 +136,10 @@ function handleThreadExit(deadThreadId: number) { } } +export function notifyThreadExitedForTest(deadThreadId: number) { + handleThreadExit(deadThreadId); +} + function setWorkerGrantable(name: string, threadId: number, grantable: boolean) { let grantableIds = grantableByWorker.get(name); if (grantable) { diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 9182242eae..4581f067d1 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -152,8 +152,6 @@ module.exports = { restoreShutdownDeadline, registerWorkerDataProvider, onThreadExit, - hasThreadExited, - notifyThreadExit, registerProcessGroup, unregisterProcessGroup, isThreadRunning, @@ -1121,15 +1119,6 @@ if (isMainThread) { }); } -/** - * Whether a thread has already been reported dead. Sync, unlike `isThreadRunning`, because it only - * reads the tombstone `notifyThreadExit` records below — callers on the exit path need an answer - * without awaiting process-group confirmation. - */ -function hasThreadExited(threadId) { - return notifiedDeadThreadIds.has(threadId); -} - function notifyThreadExit(deadThreadId) { if (deadThreadId == null || notifiedDeadThreadIds.has(deadThreadId)) return; notifiedDeadThreadIds.add(deadThreadId); diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 95bb2dc1ae..826f503cb0 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -9,7 +9,6 @@ const sandbox = sinon.createSandbox(); const { TEST_JSON_SUPER_USER, TEST_JSON_NON_SU } = require('../../test_data'); const serverUtilities = require('#src/server/serverHelpers/serverUtilities'); const registeredOperations = require('#src/server/serverHelpers/registeredOperations'); -const manageThreads = require('#src/server/threads/manageThreads'); const operationAuthorizationState = require('#src/server/serverHelpers/operationAuthorizationState'); const { runWithDeployValidationGuard } = require('#src/server/serverHelpers/deployValidationState'); const quota = require('#src/components/mcp/quota'); @@ -307,7 +306,7 @@ describe('Test serverUtilities.js module ', () => { }); assert.equal(validateOperations([ROLLED]), null, 'still declared by the first thread'); - manageThreads.notifyThreadExit(DECLARING_THREAD); + registeredOperations.notifyThreadExitedForTest(DECLARING_THREAD); assert.notEqual(validateOperations([ROLLED]), null, 'no live worker declares it grantable any more'); assert.equal( @@ -344,7 +343,7 @@ describe('Test serverUtilities.js module ', () => { }); it('ignores an announcement that lost a race with its own thread exit', function () { - manageThreads.notifyThreadExit(DEAD_THREAD); + registeredOperations.notifyThreadExitedForTest(DEAD_THREAD); registeredOperations.operationRegisteredHandler({ message: { name: ZOMBIE, grantable: true, originator: DEAD_THREAD }, From 9ac125c459f2d79d18a2965fd68f6f1e66d7e026 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 26 Aug 2026 10:02:12 -0400 Subject: [PATCH 16/16] Reinstate the shared dead-thread registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts 9d66638a4, which was made on a conclusion I have since retracted. I had bisected a Windows failure to the shared-registry change and reverted it on that basis. The bisect was noise: the same test fails at 350s with the change reverted, and ranges 259-465s across builds with identical code, so ~200s of variance was being read as a 110s signal. Windows is red here for #2273 (risk-query and describe_all: npm work, then restart_service, then route readiness — open and reproducing on main) and #2313 (set_configuration), neither reachable from this diff. So this restores the better shape, which is also what review asked for: no second dead-thread registry beside the one manageThreads already maintains, and notifyThreadExitedForTest is out of production surface again, with the lifecycle tests driving the real onThreadExit event instead of a module-local stand-in. Co-Authored-By: Claude Opus 5 --- server/serverHelpers/registeredOperations.ts | 21 ++++++------------- server/threads/manageThreads.js | 11 ++++++++++ .../serverHelpers/serverUtilities.test.js | 5 +++-- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/server/serverHelpers/registeredOperations.ts b/server/serverHelpers/registeredOperations.ts index 1d5d4cdac0..e62ed51124 100644 --- a/server/serverHelpers/registeredOperations.ts +++ b/server/serverHelpers/registeredOperations.ts @@ -26,7 +26,7 @@ import * as env from '../../utility/environment/environmentManager.ts'; import harperLogger from '../../utility/logging/harper_logger.ts'; import { ServerError } from '../../utility/errors/hdbError.ts'; import { sendItcEvent } from '../threads/itc.js'; -import { onMessageByType, onThreadExit } from '../threads/manageThreads.js'; +import { hasThreadExited, onMessageByType, onThreadExit } from '../threads/manageThreads.js'; import { registerWorkerGrantableOperation, unregisterWorkerGrantableOperation, @@ -66,14 +66,6 @@ const registeredByWorker = new Map>(); // Per originator, not per name: a rolling deploy whose new generation drops `requiresSuperUser` // must keep routing the name while retracting grantability, which a name-level flag cannot express. const grantableByWorker = new Map>(); -// Threads already reported dead, so an announcement that lost the race with its own thread's exit -// cannot install an entry no later exit event will clean up — including a thread whose FIRST -// announcement is the one in flight, which is why this cannot be narrowed to threads already -// holding a registration. One integer per dead thread, never pruned, matching the growth profile -// manageThreads accepts for notifiedDeadThreadIds; it cannot reject a replacement because worker -// ids are monotonically increasing and not reused within a process. Deliberately a local set: -// reading manageThreads' equivalent regressed Windows worker-restart timing (see PR #2260). -const exitedThreadIds = new Set(); const pendingExecutions = new Map< number, { targetThreadId: number; resolve: (result: any) => void; reject: (error: Error) => void } @@ -103,7 +95,11 @@ export function operationRegisteredHandler(event: { if (!isMainThread) return; const { name, grantable, originator } = event?.message ?? {}; if (typeof name !== 'string' || typeof originator !== 'number') return; - if (exitedThreadIds.has(originator)) { + // An announcement can lose the race with its own thread's exit, and exit notification fires once + // per thread, so without this the entry would never be cleaned up. Reads manageThreads' tombstone + // rather than keeping a second one: job threads are one-shot workers, so a duplicate here would + // grow per completed job, not per worker restart. + if (hasThreadExited(originator)) { operationLog.debug(`Ignoring operation '${name}' announced by exited worker thread ${originator}`); return; } @@ -121,7 +117,6 @@ export function operationRegisteredHandler(event: { * waiting out the timeout, and forget its registrations (a replacement re-registers on load). */ function handleThreadExit(deadThreadId: number) { - exitedThreadIds.add(deadThreadId); for (const [name, workerIds] of registeredByWorker) { if (!workerIds.delete(deadThreadId)) continue; // A surviving worker that never declared a permission must not keep the name admissible. @@ -136,10 +131,6 @@ function handleThreadExit(deadThreadId: number) { } } -export function notifyThreadExitedForTest(deadThreadId: number) { - handleThreadExit(deadThreadId); -} - function setWorkerGrantable(name: string, threadId: number, grantable: boolean) { let grantableIds = grantableByWorker.get(name); if (grantable) { diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 4581f067d1..9182242eae 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -152,6 +152,8 @@ module.exports = { restoreShutdownDeadline, registerWorkerDataProvider, onThreadExit, + hasThreadExited, + notifyThreadExit, registerProcessGroup, unregisterProcessGroup, isThreadRunning, @@ -1119,6 +1121,15 @@ if (isMainThread) { }); } +/** + * Whether a thread has already been reported dead. Sync, unlike `isThreadRunning`, because it only + * reads the tombstone `notifyThreadExit` records below — callers on the exit path need an answer + * without awaiting process-group confirmation. + */ +function hasThreadExited(threadId) { + return notifiedDeadThreadIds.has(threadId); +} + function notifyThreadExit(deadThreadId) { if (deadThreadId == null || notifiedDeadThreadIds.has(deadThreadId)) return; notifiedDeadThreadIds.add(deadThreadId); diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 826f503cb0..95bb2dc1ae 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -9,6 +9,7 @@ const sandbox = sinon.createSandbox(); const { TEST_JSON_SUPER_USER, TEST_JSON_NON_SU } = require('../../test_data'); const serverUtilities = require('#src/server/serverHelpers/serverUtilities'); const registeredOperations = require('#src/server/serverHelpers/registeredOperations'); +const manageThreads = require('#src/server/threads/manageThreads'); const operationAuthorizationState = require('#src/server/serverHelpers/operationAuthorizationState'); const { runWithDeployValidationGuard } = require('#src/server/serverHelpers/deployValidationState'); const quota = require('#src/components/mcp/quota'); @@ -306,7 +307,7 @@ describe('Test serverUtilities.js module ', () => { }); assert.equal(validateOperations([ROLLED]), null, 'still declared by the first thread'); - registeredOperations.notifyThreadExitedForTest(DECLARING_THREAD); + manageThreads.notifyThreadExit(DECLARING_THREAD); assert.notEqual(validateOperations([ROLLED]), null, 'no live worker declares it grantable any more'); assert.equal( @@ -343,7 +344,7 @@ describe('Test serverUtilities.js module ', () => { }); it('ignores an announcement that lost a race with its own thread exit', function () { - registeredOperations.notifyThreadExitedForTest(DEAD_THREAD); + manageThreads.notifyThreadExit(DEAD_THREAD); registeredOperations.operationRegisteredHandler({ message: { name: ZOMBIE, grantable: true, originator: DEAD_THREAD },