fix: give ensureNode() its own isolated write context#550
Merged
Conversation
ensureNode() (called from setNode()/addNodeBack() to persist a peer's or this node's own hdb_nodes row during cluster mesh formation) wrote through the static Resource API with no explicit context, relying on ambient fallback (contextStorage.getStore()). Two problems follow from that: 1. Attribution: ensureNode() runs both for genuine admin-initiated add_node calls and for internal inter-node bookkeeping (addNodeBack, whose "hdb_user" is just the resolved auth principal of the replication socket connection) — neither should be stamped onto a system table row as if a user directly wrote it. 2. Correctness (now fixed generally at the core level by HarperFast/harper#1720, but worth hardening here too): core's processLocalTransaction (#1591/#1592) wraps an entire operation handler invocation in one contextStorage.run(...) call, installing a single, long-lived, mutable ambient context for the whole handler. setNode()/addNodeBack() each call ensureNode() twice in sequence (once for this node's own record, once for the peer's) — before harper#1720, the second call could see the first call's already-completed transaction still attached to the shared context and wrongly join it, silently dropping one of the two writes. Confirmed via an instrumented 4-node cluster-formation run (integrationTests/cluster/ replicationTopology.test.mjs) against core commit af646222d: a peer's `ca` field failed to persist into hdb_nodes, producing SELF_SIGNED_CERT_IN_CHAIN on subsequent mesh TLS connections. Passing a fresh `{}` context per write restores the pre-#1591 behavior (every internal static Resource API call here always got its own throwaway context) and fixes both the attribution issue and the correctness issue as defense in depth, independent of the core-level fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces an explicit, isolated write context (writeContext) in ensureNode within replication/subscriptionManager.ts. This prevents internal node-registry bookkeeping writes from inheriting ambient user contexts or incorrectly joining already-completed transactions from shared contexts, addressing potential correctness and attribution issues. There are no review comments, and I have no additional feedback to provide.
Contributor
|
Reviewed; no blockers found. |
cb1kenobi
approved these changes
Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Companion to HarperFast/harper#1720, which is the primary/sufficient fix for a cluster-formation regression: bumping the
coresubmodule pointer pastaf646222d("establish ambient user context for operation handlers so audit records carry attribution", #1591/#1592) causedintegrationTests/cluster/replicationTopology.test.mjsto fail withTimed out waiting for cluster to connect/SELF_SIGNED_CERT_IN_CHAIN— see harper-pro PR #512's CI.Root cause (full detail in the harper#1720 description):
processLocalTransactionnow wraps an entire operation handler invocation in onecontextStorage.run({ user: hdbUser }, handler)call, installing a single, long-lived, mutable ambient context for the whole handler.resources/Resource.ts's dispatcher decided whether to join an existing transaction on that ambient context via blunt truthiness, with no check that the transaction was still meaningfully open — so two sequential, logically-independent static Resource API writes in one handler could get coalesced into one transaction, silently dropping one of them.ensureNode()(called twice peradd_node/add_node_backinvocation — once for this node's ownhdb_nodesrecord, once for the peer's) hit this: a peer'scafield could silently fail to persist, so the peer's cert was never trusted, producingSELF_SIGNED_CERT_IN_CHAIN.This change
ensureNode()'s writes now use an explicit, isolated{}context instead of relying on ambientcontextStoragefallback. This is defense in depth on top of the core fix, and also fixes a smaller, independent issue:ensureNode()runs for both genuine admin-initiatedadd_nodecalls and internal inter-node bookkeeping (addNodeBack, whose ambient "hdb_user" is just the resolved auth principal of the replication socket connection) — neither should be attributed to a system table row as if a user directly wrote it (core's ownregisterLiveSubscriptionForContextdocuments the same invariant: "Internal watchers, replication and local-bypass have no user principal").This fix is not required for the connectivity regression once harper#1720 lands (that core fix alone resolved the full cluster-formation test, 3/3 clean runs at pre-regression speed, verified with this harper-pro change reverted). It's included as a belt-and-suspenders correctness improvement that's safe to take independently.
Testing
Verified via an instrumented 4-node cluster-formation run (
integrationTests/cluster/replicationTopology.test.mjs, "connect nodes") withcorepinned ataf646222d:hdb_nodesCA-persistence symptom is resolved (confirmed via directsearch_by_valuedumps —ca_infocorrectly populated for every peer on every node), but the cluster-connect test can still fail because the same ambient-context hazard also affects other call sites in the same operation handlers (e.g.security/certificate.ts'ssignCertificate(), which does afor awaitstatic-API search before later static-API writes) — this is exactly why the general core-level fix in harper#1720 is the one that should ship.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com