Repair the decode-drop-recovery stress test so the job can pass - #745
Open
kriszyp wants to merge 1 commit into
Open
Repair the decode-drop-recovery stress test so the job can pass#745kriszyp wants to merge 1 commit into
kriszyp wants to merge 1 commit into
Conversation
…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>
There was a problem hiding this comment.
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.
Contributor
|
Reviewed; no blockers found. |
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.
The
decode-drop-recoverystress job could never pass:add_nodewas authorized with two property names the integration-testing harness does not expose, so the join was unauthenticated and node A closed it1008 Unauthorizedin ~230ms — and even with that fixed,after()passed bare nodes toteardownHarper, which early-returns on a falsyctx.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
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. MakingteardownHarperthrow on a malformed argument lives in@harperfast/integration-testingand 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.This guard's strength is coupled to core log strings, not to the
decode-dropcounter fix(replication): classify undecodable records + hold on unknown table id (#537) #545 added for it. Assertion (3) countsError handling incoming replication messagelines and assertion (4) probesError decoding replication messageas a substring. Reword either string incore/— a plausible, review-approved change — and a full re-regression of the decode→close→resume loop lands green. Related: convergence gates ondescribe_table.record_count >= 200rather 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.No CI baseline— discharged. The stress workflow does not run on PRs (workflow_dispatch/scheduleonly), so I dispatched it against this branch:decode-drop-recoverypassed, non-vacuously. Evidence below. Nothing left to hold on here.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.
add_node, ~232ms,1008 UnauthorizedAlso 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)passed — run 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 atadd_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 exactConnection closed Unauthorized 1008 and connection was required to sign certificatesignature, root-caused from the uploaded server logs — node A loggingSELF_SIGNED_CERT_IN_CHAIN,authorized: false, thenNo authorization provided.The pass is non-vacuous: node B logged
Error decoding replication messageexactly 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:allgate. 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 —teardownHarperis declaredexport 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 —teardownHarperandkillHarperreference onlyctx.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