fix: exclude cluster/replication topology operations from ambient user context

[kriszyp]

Summary

Excludes cluster/replication topology operations from the #1591/#1592 ambient-user context wrap in processLocalTransaction, and strips any inherited ambient user for those operations.

Root cause

af646222d (#1591, part of #1592) made processLocalTransaction run every operation handler carrying hdb_user inside contextStorage.run({ ...currentStore, user: hdbUser }, handler), so static Resource API writes inside component-registered handlers inherit user attribution for audit records.

Cluster/replication topology operations (add_node, add_node_back, set_node, update_node, set_node_replication, remove_node, …) start long-lived, non-awaited replication subscription work (a peer connection's connect/handshake/reconnect lifecycle). The handler returns quickly, but that background async chain keeps running — and because it was constructed while the handler's AsyncLocalStorage scope was active, it keeps observing the ambient { user } for its entire life, not just the request. That corrupts the subscription's identity and produces a WebSocket 1006 reconnect storm that never meshes.

This is distinct from #1720 (a stale ambient transaction leaking into a later static write within one handler invocation, fixed in resources/Resource.ts). Both stem from #1591's design change — long-lived ambient context per handler invocation — but at different points. This fix is orthogonal to #1720 (different file, different mechanism; verified #1720's leak test passes with this change).

Fix

processLocalTransaction now skips the ambient-user wrap for a documented set of topology-management operations, and — for the nested case where an ambient context already carries a user (e.g. a server.operation() call from within an authenticated handler) — strips that inherited user too, preserving other ambient state and never mutating the outer context object. Their writes are internal node-registry bookkeeping (hdb_nodes, certificates), not user-authored data, so removing user attribution is also correct, not just safe.

Why this direction

The architecturally cleanest fix is option (b) from the investigation — detach the long-lived work from the ambient context where it is spawned. That spawn point lives in harper-pro's replication component (subscriptionManager/replicationConnection), not core, so it can't be done in a core-only change, and the deliverable requires a self-sufficient core fix (the harper-pro integration test must pass on the core change alone). The core-only variant of (b) — clearing the ambient user after the handler settles — is unsafe: operation handlers can return Readable streams that are consumed after processLocalTransaction returns (serverHandlers.js), so stripping the user mid-stream would corrupt late/streaming authorization.

This change is the safe, self-sufficient core fix (option a): it is proven-equivalent to the investigation's validated "revert the wrap" for exactly the affected operations, while preserving #1591 attribution for all data operations.

Where to look / open items

• OPERATIONS_EXCLUDED_FROM_AMBIENT_USER (server/serverHelpers/serverUtilities.ts): the set spans core's OPERATIONS_ENUM entries and harper-pro-registered names (add_node_back, set_node, …). This cross-layer name list is the fragile part — it must be extended if new topology-management operations that spawn long-lived background work are added. The durable fix is to detach that background work from the ambient context at the spawn point in harper-pro's replication component; recommend a coordinated follow-up. A prior harper-pro branch (fix/ensurenode-ambient-context) already isolates ensureNode's write context in that layer.

Verification

• harper-pro integrationTests/cluster/replicationReconnect.test.mjs against this core branch: 2/3 → 3/3, reliably (previously-failing tests dropped from ~15–17s timeouts to ~1–2s), confirmed over two runs.
• New unit coverage in unitTests/server/serverHelpers/serverUtilities.test.js: each topology op runs with no ambient user; background work spawned by a topology handler does not observe the request user after the handler returns; inherited ambient user is stripped for the nested case; and a non-topology op still gets Audit records lack user attribution for writes from registered operations (no ambient operation context) #1591 attribution (no over-exclusion).
• Existing Audit records lack user attribution for writes from registered operations (no ambient operation context) #1591 attribution tests (operationContextUser.test.js, serverUtilities.test.js) pass; fix: only join an ambient transaction if it is genuinely still OPEN #1720's leak test passes with this change overlaid.
• Full local suites green: unitTests/resources/** (1027), unitTests/components/mcp/** (479), unitTests/server/serverHelpers/** (211).

Generated by Claude (Opus 4.8), reviewed cross-model by Codex (one P1 addressed: strip inherited ambient user for the nested case).

[gemini-code-assist]

Code Review

This pull request prevents cluster and replication topology operations from running under the request's ambient user context, resolving issues where long-lived background tasks incorrectly inherit the user principal. The changes introduce a set of excluded operations and strip the user context when processing them, supported by comprehensive unit tests. The review feedback suggests optimizing this context-stripping logic to avoid unnecessary contextStorage.run calls when the user is already undefined.

[gemini-code-assist]

We can optimize this check and avoid an unnecessary contextStorage.run call when the current store already has no user (i.e., currentStore.user is undefined or not set). Since contextStorage.run introduces a new async context scope and adds overhead, we should only invoke it if there is an active user in the store that actually needs to be stripped. Additionally, when overriding properties in an ambient context like contextStorage, ensure we merge the existing context to preserve other critical properties.

Suggested change

	data = currentStore
	? await contextStorage.run({ ...currentStore, user: undefined }, callOperationFunction)
	: await callOperationFunction();
	data = currentStore?.user !== undefined
	? await contextStorage.run({ ...currentStore, user: undefined }, callOperationFunction)
	: await callOperationFunction();

References

1. When updating or overriding properties in an ambient context (such as AsyncLocalStorage), merge the existing context instead of replacing it entirely. This ensures that other critical properties of the context, such as active transactions, signals, or caches, are preserved and transactional safety is maintained.

[claude]

Reviewed; no blockers found.

[cb1kenobi]

Nit: remove_node_back won't match the op as registered in harper-pro

harper-pro registers this operation with a trailing-semicolon typo in the name — name: 'remove_node_back;' (replication/setNode.ts:331). So this exclusion entry (remove_node_back, no semicolon) never matches req.body.operation for that handler, and the entry is currently a no-op.

Low impact in practice: removeNodeBack only deletes an hdb_nodes record and spawns no long-lived subscription work, so no 1006 storm results — the exclusion here is attribution-only. But it's a live instance of exactly the cross-layer name-drift this PR's own comment warns about. The real fix is dropping the stray ; in harper-pro's registration; keep this list entry as-is (it's the correct intended name).

—
Generated by Barber AI

[ldt1996]

Reviewed the mechanism and the tests hold it firmly: the ALS registration-time binding for detached background work is pinned directly (the setTimeout-after-return test), the nested strip preserves the rest of the ambient state and never mutates the outer object, and the over-exclusion guard keeps #1591 attribution for data operations. The user: undefined spread is the right call over deleting the key, and running topology writes unattributed is correct on its own merits, matching the #550 companion's rationale.

Two field notes from this week that support and contextualize this: our stage cluster's rolling-restart churn showed transient 1008 "Unauthorized database subscription" flaps and identity oddities consistent with a subscription chain observing the wrong ambient identity, so the long-lived-inheritance mechanism is not hypothetical. And as an empirical cross-check of the "distinct from #1720" claim: harper-pro #555 (core pinned to #1720 alone, without this PR) just ran the previously-red integration suite 12/12 green, so #1720 was sufficient for the current main breakage and this correctly targets the residual storm class rather than duplicating that fix.

One non-blocking thought for a follow-up: the op set is name-synced by hand across repos; a self-declared flag at server.registerOperation time would let harper-pro's topology ops opt out without core knowing their names. Fits the "durable fix lives at the spawn point" direction the description already lays out.

Lavinia, via Claude

[kriszyp]

Closing in favor of harper#1720, which is already merged and is the actual root-cause fix.

Root-caused this further: the WS 1006 reconnect storm isn't about ambient user context — it's stale-transaction reuse. add_node calls ensureNode() twice (self-row, then peer-row), both via table.patch() with no explicit context, sharing the single long-lived ambient context that #1591/#1592 installs. Resource.ts's transactional() joined any context.transaction without checking it was still OPEN, so the second patch() silently reused the first's already-committed transaction and dropped the first write (the self-row). That produced the "identity mismatch" / missing peer CA / SELF_SIGNED_CERT_IN_CHAIN chain that manifests as the reconnect storm.

#1720's OPEN-guard in transactional() fixes this generally, for any handler doing sequential independent writes under one ambient context — not just the topology-op list this PR would have hardcoded. Verified with a single-variable test: buggy core + only the #1720 guard = replicationReconnect.test.mjs 3/3 pass with zero identity/cert errors; same core without it = 2/3 fail.

On harper-pro's side, harper-pro#555 bumps the core submodule pointer past #1720 and is what actually delivers the fix there (green, auto-merge enabled, just needs review). harper-pro#550 adds an isolated write context for ensureNode as defense-in-depth on top of that.

Thanks for the review here — the nested server.operation() catch was good, but with #1720 that surface no longer needs a fix of its own.

— KrAIs