Skip to content

Repair the decode-drop-recovery stress test so the job can pass - #745

Open
kriszyp wants to merge 1 commit into
mainfrom
kris/fix-decode-drop-auth
Open

Repair the decode-drop-recovery stress test so the job can pass#745
kriszyp wants to merge 1 commit into
mainfrom
kris/fix-decode-drop-auth

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 20, 2026

Copy link
Copy Markdown
Member

The decode-drop-recovery stress job could never pass: add_node was authorized with two property names the integration-testing harness does not expose, so the join was unauthenticated and node A closed it 1008 Unauthorized in ~230ms — and even with that fixed, after() passed bare nodes to teardownHarper, which early-returns on a falsy ctx.harper, leaking both Harper children into run.mjs's leaked-child backstop. Both defects date to #691, and neither surfaced earlier because the job's three runs before 2026-08-20 were all cancelled; the 2026-08-20 run was its first completed run.

Found while clearing CI reds during the v5.2.4 release cut. The job is a stress-matrix test only — no product code changes.

For the human reviewer

  1. Patched the call sites, not the harness. teardownHarper(node) silently no-ops instead of throwing — that is the actual invariant hole, and the next suite author will hit it exactly the same way. I fixed the two call sites here to match the { harper: node } convention every sibling suite uses. Making teardownHarper throw on a malformed argument lives in @harperfast/integration-testing and needs its own PR; it would also surface latent misuse elsewhere, which is worth doing but is not this change. Say the word and I'll open it.

  2. This guard's strength is coupled to core log strings, not to the decode-drop counter fix(replication): classify undecodable records + hold on unknown table id (#537) #545 added for it. Assertion (3) counts Error handling incoming replication message lines and assertion (4) probes Error decoding replication message as a substring. Reword either string in core/ — a plausible, review-approved change — and a full re-regression of the decode→close→resume loop lands green. Related: convergence gates on describe_table.record_count >= 200 rather than row identity, so "197 clean rows plus 3 applied poison rows" satisfies (1) and only assertion (2) catches it, by failing with the misleading "poison-1 present" instead of "3 clean rows missing". Both are pre-existing and out of scope here, but this is the change that first makes those assertions reachable, so it is the moment to decide whether to harden them.

  3. No CI baseline — discharged. The stress workflow does not run on PRs (workflow_dispatch/schedule only), so I dispatched it against this branch: decode-drop-recovery passed, non-vacuously. Evidence below. Nothing left to hold on here.

  4. Teardown still swallows every error (.catch(() => {})), and I left it that way. The review argued the other side well: this suite's subject is a wedged replication leg, so a node that won't exit holds port 9933 and its loopback address, and the next stress suite then fails with an unrelated bind error. Most sibling suites don't swallow. I kept the pre-existing behavior because flipping it changes this suite's failure semantics and that is a separate call from making the job runnable — but it is one line if you want it now.

Verification

Route: ran the affected stress test itself, on this branch, and confirmed both fixes are independently load-bearing.

HARPER_RUN_STRESS_TESTS=1 node integrationTests/run.mjs integrationTests/cluster/decodeDropRecovery.test.mjs
state test assertion run exit
neither fix (CI, run 32415687129) fails at add_node, ~232ms, 1008 Unauthorized 1
auth fix only passes 1 — child leaked, backstop fired
both fixes passes 0

Also confirmed in CI: the Stress Tests workflow was dispatched against this branch (duration knobs trimmed to 1 minute) and Stress decode-drop-recovery (Node.js v24) passedrun 32429142918, pass 1 / fail 0, no leaked-child message. This is the job's first ever green run; its three runs before 2026-08-20 were cancelled and the 2026-08-20 run failed at add_node. The uploaded server logs show 3 decode drops on B and 0 on A, matching the local runs, so the CI pass is non-vacuous for the same reason.

Fails-on-base is satisfied by CI itself: the job failed on d6ff755 (the pre-fix base) with the exact Connection closed Unauthorized 1008 and connection was required to sign certificate signature, root-caused from the uploaded server logs — node A logging SELF_SIGNED_CERT_IN_CHAIN, authorized: false, then No authorization provided.

The pass is non-vacuous: node B logged Error decoding replication message exactly 3 times (= POISON_COUNT) with 0 close lines, so assertion (3) ruled on real decode drops rather than on an injection that never fired. Confirmed across all three local runs.

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

Dismissed from bot review: a suggestion to wrap each teardown in Promise.resolve().then(...) so a synchronous throw can't strand the other node's cleanup — teardownHarper is declared export async function (harperLifecycle.js:607), and an async function returns a rejected promise rather than throwing synchronously, so there is nothing for the wrapper to catch.

Dismissed from review: a leak concern about passing a synthetic { harper: node } rather than the real test context — teardownHarper and killHarper reference only ctx.harper (verified in the installed harness), and { harper: node } is the shape sibling suites already pass.

Complexity: easy

Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=3 @ 5d70487

Human-Review-Need: 3 (decisions: regression-signal-log-grep-vs-counter, land-before-first-green-run, convergence-by-count-vs-row-identity, teardown-swallows-all-errors) @ 5d70487

…rdown

The `decode-drop-recovery` stress job could never pass. Two defects in the test,
both dating to when it was added in #691:

`add_node` was authorized with `ctx.nodeA.HDB_ADMIN_USERNAME` /
`HDB_ADMIN_PASSWORD`. Those properties do not exist on the integration-testing
harness's node object, which exposes `admin: { username, password }` — the name
appears in exactly one file in `integrationTests/`, this one. Both values were
`undefined`, so the operation went out as `authorization: {}` and `add_node_back`
followed with `authorization: null`. Node B then presented its still-self-signed
certificate to A with no credentials behind it, and A — installed with the prod
config profile, so certificate validation is enforced — closed the connection
1008 Unauthorized. The test died in ~230ms at its first operation, never reaching
any decode-drop behavior.

`after()` called `teardownHarper(ctx.nodeA)`, passing the node where the harness
expects a context. `teardownHarper` opens with `if (!ctx.harper) return`, so both
calls were silent no-ops and both Harper children survived the suite. The
leaked-child backstop in `integrationTests/run.mjs` then forced a non-zero exit,
which would have kept the job red even once the join was fixed. Teardown now also
runs both nodes concurrently, matching the sibling suites, so a hang on A cannot
strand B.

Neither surfaced earlier because the job's three runs before 2026-08-20 were all
cancelled; the 2026-08-20 run was its first completed run.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kriszyp
kriszyp requested a review from heskew August 20, 2026 21:44

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the after cleanup hook in decodeDropRecovery.test.mjs to tear down nodes concurrently using Promise.all and simplifies the authorization configuration. The review feedback suggests improving the cleanup robustness by wrapping the teardown calls in Promise.resolve().then(...) to catch synchronous exceptions and avoiding swallowing errors so that teardown failures can surface.

Comment thread integrationTests/cluster/decodeDropRecovery.test.mjs
@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kriszyp
kriszyp marked this pull request as ready for review August 20, 2026 23:37
@kriszyp
kriszyp requested a review from a team as a code owner August 20, 2026 23:37
@kriszyp
kriszyp removed the request for review from a team August 20, 2026 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant