Repair the decode-drop-recovery stress test so the job can pass (backport #745 to v5.2)

[kriszyp]

Backport of Repair the decode-drop-recovery stress test so the job can pass (#745, merged to main as 4bd15ec) onto v5.2, so the decode-drop-recovery stress job can go green on the release line.

Clean cherry-pick — git diff against the original merge commit is byte-identical, so the rationale and the four open decisions raised for the reviewer in #745 carry over unchanged. Test-only: one .mjs file, no product code.

v5.2 is affected

Both defects are present on v5.2 (both date to #691, which is on this branch):

• integrationTests/cluster/decodeDropRecovery.test.mjs:151 authorizes add_node with ctx.nodeA.HDB_ADMIN_USERNAME / HDB_ADMIN_PASSWORD — property names the harness does not expose, so the join goes out unauthenticated and A closes it 1008 Unauthorized.
• after() passes bare node objects to teardownHarper, which early-returns on a falsy ctx.harper, so both Harper children leak past teardown.

Verified against the installed harness on this branch (@harperfast/integration-testing 0.7.1): the node object exposes admin: { username, password } (dist/harperLifecycle.js:452) and teardownHarper(ctx) early-returns on !ctx.harper (dist/harperLifecycle.js:608). And decode-drop-recovery is in this branch's stress matrix (.github/workflows/stress-tests.yaml:135), so the job runs here and is red for exactly these two reasons.

Verification

Ran the stress test itself on this branch, at all three states, reproducing #745's table on v5.2:

HARPER_RUN_STRESS_TESTS=1 node integrationTests/run.mjs integrationTests/cluster/decodeDropRecovery.test.mjs

state	test assertion	run exit
neither fix (origin/v5.2 file)	fails at add_node, Connection closed Unauthorized 1008 and connection was required to sign certificate, HTTP 500	1
auth fix only (teardown hunk reverted)	passes	1 — leaked-child backstop fired
both fixes	passes	0

So both hunks are independently load-bearing on this branch, not just on main.

The pass is non-vacuous. With logs pinned to a known directory, node B logged Error decoding replication message exactly 3 times (= POISON_COUNT) and node A 0 times, with 0 Error handling incoming replication message close lines on either node — assertion (3) ruled on real decode drops, not on an injection that never fired. This is structural as well as observed: assertion (3) is ok(/Error decoding replication message/.test(logB)), so the test cannot pass without a real drop.

Not run: the full integration gate. This is a single stress-gated file that no other suite imports, so the blast radius is that file.

Complexity: easy

[gemini-code-assist]

Code Review

This pull request refactors the test cleanup in decodeDropRecovery.test.mjs to tear down Harper nodes concurrently using Promise.all and simplifies the authorization configuration. The review feedback recommends logging any teardown errors with identifying information instead of silently swallowing them in the .catch block.

[gemini-code-assist]

In test cleanup hooks, any failures during process termination or cleanup should be logged with identifying information (such as hostnames) for easier diagnosis, rather than being silently swallowed with an empty .catch(() => {}) block.

Suggested change

	await Promise.all(
	[ctx.nodeA, ctx.nodeB].filter(Boolean).map((node) => teardownHarper({ harper: node }).catch(() => {}))
	);
	await Promise.all(
	[ctx.nodeA, ctx.nodeB].filter(Boolean).map((node) =>
	teardownHarper({ harper: node }).catch((err) => {
	console.error('Failed to teardown Harper node ' + (node.hostname || 'unknown') + ':', err);
	})
	)
	);

References

1. In test cleanup hooks, wrap individual process termination or cleanup steps in separate try-catch blocks to ensure they are attempted independently, and log any failures with identifying information (such as hostnames) for easier diagnosis.

[claude]

Reviewed; no blockers found.

[cb1kenobi]

Reviewed e086be21 — no issues found. This PR looks good, nice job!

Verified independently on v5.2, not just re-read against #745:

• Cherry-pick is exact. git range-diff bc55bf2..e086be2 4bd15eca^..4bd15eca differs only by the (cherry picked from ...) trailer; the patch bytes are identical, and the pre-fix file is identical on v5.2 and main.
• Both defects confirmed present on v5.2. Against the installed harness (@harperfast/integration-testing 0.7.1): HDB_ADMIN_USERNAME appears only as an install CLI flag (dist/harperLifecycle.js:421), never as a node property, so the old call sent {username: undefined, password: undefined}; teardownHarper reads only ctx.harper (plus killHarper's ctx.harper?.process), so { harper: node } is a complete context and matches the convention 8 sibling cluster suites already use.
• Reproduced your table on v5.2. Base file → dies at add_node in 131ms with Connection closed Unauthorized 1008 and connection was required to sign certificate (500 vs 200), and the leaked-child backstop fires. Auth fix only → assertion passes, run still exits 1 on the backstop. Both fixes → exit 0. Both hunks are independently load-bearing here.
• No bar-lowering. The diff touches only after() and the authorization field. TOTAL_ROWS (200), POISON_COUNT (3), CONVERGE_TIMEOUT_MS (90s), the 300s suite timeout, and assertions (1)–(4) are all byte-identical to the base. No new skip or gate: skip: !STRESS is pre-existing and the stress job sets HARPER_RUN_STRESS_TESTS: '1'.
• 10/10 green at head, deterministic: exit 0, pass 1 / fail 0, no leaked-child message, and exactly 3 Error decoding replication message lines on B with 0 on A and 0 close lines in every run — the same non-vacuity signature you reported.
• Detection proven by mutation, both rebuilt to dist/ with the marker confirmed as exactly one isolated match:
  • Rethrow the decode error in replicationConnection.ts (regress fix(replication): classify undecodable records + hold on unknown table id (#537) #545 back to fix(replication): close on per-record value-decode failure instead of skipping past it (#440) #521 close-on-decode) → test fails: Timed out after 90000ms waiting for DecodeDropTest count >= 200 (last 0), with 25 close lines on B. The wedge is reproduced and caught.
  • Neuter maybeInjectDecodeFailureForTest so no drops are induced → test fails at assertion (2), poison-1 must be absent on B (skipped). A dead injection cannot pass silently.

So the repair restores detection rather than lowering the bar: the base test detected nothing at all (it died at its first operation, never reaching decode-drop behavior), and the repaired test catches both a recovery regression and a dead injection.

The four decisions you carried over from #745 are still open and still pre-existing (log-string coupling in assertions (3)/(4), convergence by record_count rather than row identity, the swallowing .catch(() => {}), and patching call sites instead of hardening teardownHarper) — none of them are introduced or worsened here, so nothing to hold this backport on.

—
Generated by Barber AI