fix(test): make the decode-drop and poisoned-cursor cluster suites actually run, pass, and clean up - #718
fix(test): make the decode-drop and poisoned-cursor cluster suites actually run, pass, and clean up#718heskew wants to merge 2 commits into
Conversation
…tually run, pass, and clean up (#716) - stress matrix: add poisonedCopyCursorDataLoss.test.mjs — stress-gated but listed in no matrix, so the 279-line suite and its injection hook ran in no CI path at all - both suites: deploy without the implicit restart, then restartNode() + pollHealth() — deploy_component restart:true answers before the old process exits, so the fixed 10s sleep raced the outgoing process - both suites: stopNodeProcess() before teardownHarper() — a restarted node is detached from the harness child handle, so teardown alone leaks the live process - decodeDropRecovery: teardownHarper({ harper: node }) — the bare-node call has no .harper and silently no-ops; and add_node authorization used nonexistent handle properties (HDB_ADMIN_USERNAME), failing the join with a certificate-signing 1008 - hoist pollHealth from blockCacheEviction into clusterShared Both suites now pass locally under HARPER_RUN_STRESS_TESTS=1 — their first full executions (17.2s and 21.8s). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4VjVDt6egBUxmGiASuahs
There was a problem hiding this comment.
Code Review
This pull request refactors integration tests to share the pollHealth helper function from clusterShared.mjs and improves the reliability of node restarts and teardowns. It replaces implicit restarts and hardcoded delays during component deployment with explicit restarts and health polling, and ensures that live processes are stopped before teardown to prevent node leaks. The review feedback suggests improving the pollHealth function by capturing and reporting the last encountered error when the retry loop times out, which would greatly aid in debugging persistent failures.
|
Reviewed; no blockers found. |
Gemini review: a persistent fault (auth/config) was indistinguishable from a slow start. Verified against a live decode-drop suite run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4VjVDt6egBUxmGiASuahs
|
I checked the claims against the harness source and the workflow: teardownHarper in the harness returns immediately when ctx.harper is absent, so the old bare-node calls really were silent no-ops; ctx.nodeA.admin matches the idiom working suites use for add_node; and the new matrix entry arms because the workflow sets HARPER_RUN_STRESS_TESTS for every entry. On the three decision points: (1) accept; hoisting the helpers into clusterShared already covers future suite authors, and the product-side restart:true race deserves its own tracked issue. (2) Accept; a pid-file-aware teardownHarper in integration-testing is the durable fix, but the compensation is correct today. (3) Accept as designed; the injection knob survives on current main, and the #699/#701 cursor banking gates when the receiver persists a cursor, not whether the sender validates a resumed one, so the loss oracle should still hold. Cross-link: the decode-drop suite here is the natural home for the #715 hold-path e2e discussed on #717. Lavinia, via Claude (I reviewed this before posting) |
Test-only: makes the decode-drop and poisoned-copy-cursor cluster suites actually run, pass, and clean up after themselves, and enrolls
poisonedCopyCursorDataLoss.test.mjsin the stress matrix (it was stress-gated but listed in no matrix — the 279-line suite and its injection hook ran in no CI path). Substantive fixes: explicit pid-change-verified restarts + health polls replacedeploy_component restart:true+ a blind 10s sleep (the implicit restart answers before the old process exits); teardown kills the current process by pid file beforeteardownHarper({ harper: node })(a restarted node is detached from the harness child handle, and the old bare-node call silently no-ops);add_nodeauthorization uses the harness'snode.admin(the old properties don't exist — undefined credentials are part of why the suite never ran);pollHealthhoisted verbatim intoclusterShared. Fixes #716. Surfaced by @kriszyp's post-approval review on #691.For the human reviewer
restartNode+pollHealtharounddeploy_component's answer-before-exit race, rather than fixingrestart: true(or shipping a deploy-and-restart helper). Every future suite author will re-discover this race; fully reversible by later extracting the helper or fixing the operation.stopNodeProcessbeforeteardownHarper, rather than making teardown pid-file-aware so restarted nodes are cleaned up everywhere at once. Any not-yet-updated restarting suite still leaks; reversible, but the knowledge lives in comments instead of the harness.Follow-up hardening note from adjudication (pre-existing, not this diff): the poisoned-cursor suite's "B considers itself current" oracle checks connected sockets + row count but never asserts
COPY_COMPLETEwas received.Verification
HARPER_RUN_STRESS_TESTS=1— their first full executions anywhere: decode-drop 1/1 in 17.2s, poisoned-cursor 1/1 in 21.8s.restartre-spawns from the running process which carries the env var, and the suite asserts the hook fired via a log marker — a silent injection loss fails loudly).test:integration:all): 131 tests — 118 pass, 12 skipped (stress-gated), 1 failure (can track child process CPU time) reproduced identically on an untouched origin/main worktree — pre-existing platform failure.pollHealthimport and stop-before-teardown pattern present in all three restart-using suites (3/3); no other suite usesrestartNode.Review coverage
Authored by Claude (Fable 5). Cross-model review at f227213: codex ✓ (graded), gemini ✓ (contributed findings), Harper-domain adjudication ✓ — adjudicated severity: nit; all three codex minors dropped with traced reasons (verbatim-moved code bounded by suite timeout; repo-wide teardown idiom that strictly narrows the old unconditional leak; pre-existing oracle predating this diff). cursor-composer ✗ (preflight failed), cursor-grok pruned. Step-6 planning-mode CLI did not run before coding; the implementation-mode full review above is the outside coverage.
Human-Review-Need: 3 (decisions: compensate-in-tests-vs-fix-restart-semantics, per-suite-stop-vs-harness-aware-teardown, characterization-test-in-scheduled-ci) @ f227213