test(cluster): pin three node-lifecycle invariants — hdb_nodes reload auth, clone readiness, remove_node blast radius - #615
Open
kriszyp wants to merge 11 commits into
Open
Conversation
…ive peers Integration-level regression anchor for #602, which shipped with unit tests only (resolveNodeForSendAuth / shouldCloseSendAuthWatch). Nothing in integrationTests/cluster/ drives an actual hdb_nodes reload marker, so the end-to-end property #602 restored — a base copy of the system database must not tear down unrelated live replication connections — was unpinned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new integration test file, hdbNodesReloadSendAuth.test.mjs, to serve as a regression anchor ensuring that hdb_nodes reload markers do not incorrectly de-authorize live peers. The review feedback highlights two key improvements: replacing the CommonJS-specific module.path fallback with an ESM-safe alternative to avoid runtime errors, and assigning started test instances within their individual promise chains inside Promise.all to prevent resource leaks if one of the startup promises fails.
Contributor
|
Reviewed; no blockers found. |
…le key set Integration-level regression anchor for the readiness gate reported in #611 ("cloneNode's checkSyncStatus passes mid-copy, so a clone declares itself complete while still holding partial data"). addNodeFullCopy.test.mjs asserts POST-convergence state; nothing in integrationTests/cluster/ asserted the readiness PREDICATE itself. This drives the real cloneNode CLI bootstrap (HDB_LEADER_TOKEN/HDB_LEADER_URL), polls production's own get_status(availability), and at the FIRST Available tick takes a bidirectional leader/clone key-set diff -- never a count, never cluster_status alone. One assertion: Available implies 0 missing keys. Forced onto storage.engine 'lmdb' deliberately. Under the default RocksDB engine, RocksTransactionLogStore.getKeys() is an unimplemented stub, so schemaDescribe's fallback assigns a compound index key to last_updated_record, findMostRecentTimestamp compares an array to a number, and the watermark stays 0 -- which checkSyncStatus treats as "no target, skip". The predicate is therefore vacuously true from tick one on RocksDB, so a test there would exercise nothing. On LMDB the watermark is genuinely computed (observed non-zero on both runs), so the comparison logic actually runs. That vacuity is tracked separately; this anchor covers the path where the gate is real. Runs in ~25s (2 nodes, 250k records). requires-isolation: drives a real clone bootstrap and mutates topology, so it must not share an instance. Oracle is armed: a self-test feeds the comparator a truncated set with an injected extra key and asserts it reports exactly that gap, so the 0/0 result is a proven-working comparator rather than a broken one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kriszyp
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
kriszyp
changed the title
…-join recovery `remove_node` on A deletes A's own hdb_nodes row for B, and best-effort sends `remove_node_back` to B. Nothing in integrationTests/cluster/ asserted what the remover looks like afterwards, that its answer survives a restart, or that a node can be re-joined once it has been fully removed. Asserted (4 tests, 2 nodes, ~13s) — all fix-agnostic invariants: - A drops B promptly: A reports B disconnected and A's persisted hdb_nodes no longer carries B. - That removal survives a restart of A with its ORIGINAL config. Re-passing the config is load-bearing: omitting it wipes replication.databases, which would make the assertion vacuous. - Recovery: from the fully-removed end state, an authenticated `add_node` re-join restores a live connection and a fresh write on A replicates to B. Deliberately NOT asserted, only logged: - Whether B stops receiving writes after being removed. Today it does not (the reciprocal removal does not reach B), so asserting the observed behavior would pin a bug and turn this anchor red when that is fixed; asserting the corrected behavior would make it red today. Both legs print what the build did so a CI reader can still see it. - Any claim about whether SUBSCRIPTION_REQUEST performs a synchronous membership check. An authenticated add_node is a deliberate operator re-join, so its acceptance cannot distinguish "no check exists" from "the check is satisfied". From QA-758. requires-isolation: kills and restarts nodes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/qa-promote-hdbnodes-sendauth
branch
from
July 26, 2026 09:23
e4e5c1e to
4b186a3
Compare
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Keep explicitly unsubscribed entries for iterator and URL cleanup while allowing restored membership to schedule a fresh subscription. Exercise the lifecycle without restarting the removed node and pin the three-node blast radius. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Resolve reviewed edge cases in deleted-node iterator cleanup, reset rejoin timing state, and constrain remove_node_back to authenticated peer targets. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Pin fail-closed behavior for unrelated targets and missing authenticated peer identity. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
kriszyp
marked this pull request as ready for review
August 3, 2026 17:21
cb1kenobi
reviewed
Aug 3, 2026
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
DavidCockerill
approved these changes
Aug 3, 2026
DavidCockerill
left a comment
DavidCockerill
left a comment
Member
There was a problem hiding this comment.
name: 'remove_node_back;' good find! 🥇
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
3 participants
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.
What
Pins three node-lifecycle invariants and fixes the production full-replication remove/rejoin path they exposed:
remove_node_backis registered under its real operation name (the previous trailing semicolon made it unroutable).nodeMap, so cleanup still works if worker reassignment emptied the per-database map.Regression coverage
hdbNodesReloadSendAuth.test.mjs: requires a B-local post-join reload marker and continuously requires the expected A↔B sockets to remain present and connected; status errors and missing sockets fail.cloneReadinessKeySet.test.mjs: uses a deterministic receive throttle and ~8 MB dataset, requires a readable partial key set while production reportsUnavailable, then requires the first confirmedAvailablekey-set snapshot to be complete.removeNodeBlastRadius.test.mjs: exercises A/B/C full replication. Removing B deletes B's self row and disconnects B from both A and C; authenticatedadd_noderestores both peer subscriptions without restarting B; a second removal remains durable across B restart.removeNodeBackAuthorization.test.mjs: rejects unrelated targets and requests without authenticated peer identity.The three-node test documents the production semantics: full-replication reciprocal removal deletes the removed node's self row, so it ejects that node from every peer, not only the remover.
Validation
git diff --check: clean.npm run build: no errors in changed files; the repository currently has unrelated baseline TypeScript errors inanalytics/profile.tsandreplication/replicationConnection.ts.Unavailableprecondition, and that throttled assertion passed repeatedly.Squash on merge is recommended because the branch preserves the review/audit sequence.