Skip to content

Fix LMDB partial-record history resolution broken by #2497 (harper#2275), hold the QA-701 409 probe's window open causally, and wait for MQTT deliveries instead of sampling 200ms - #2526

Merged
kriszyp merged 5 commits into
mainfrom
fix/nightly-test-stability
Sep 8, 2026

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 8, 2026

Copy link
Copy Markdown
Member

Two nightly-only failures on main, both wall-clock assertions inside tests, plus the deterministic LMDB failure of subscribe to retained record with patch operations that has been red on every Linux unit run since #2497 (item 3, the one product change).

Fixes #2275

Item 1 — QA-701 delete_deployment_payload 409 probe

The evidence supports (a), a test-side race — not (b), a product guard gap.

handleDeleteDeploymentPayload re-reads the row itself and throws 409 for any status outside TERMINAL_STATUSES, so its 200 branch is unreachable from a non-terminal read: the 200 the nightly saw can only mean the handler's own read found a terminal row. The failing response corroborates that independently — freed_bytes: 262530 requires a populated payload_size, which the recorder writes only after the payload stream, blob save, and ingest write have all completed. The deployment finished between the test's list_deployments poll and its delete call. The guard is correct; the window was not held.

The old window was elapsed time: the payload was uploaded in 20 chunks 100 ms apart, on the assumption that deploy_component is dispatched before the request body finishes arriving. That assumption is runtime-dependent — under Bun the operation is not dispatched until the body is released — and on a loaded runner the deploy simply won the race.

The window is now causal. deploy_component awaits prepareApplication, which awaits the component's install_command, so the fixture ships a package.json and an install gate that parks until the test releases it — the same lever integrationTests/deploy/stage-swap-availability.test.ts already uses to sample a mid-deploy state.

The probe also has to prove the window held, or a future red run repeats this ambiguity. It checks two signals at the instant the delete response lands: the gate's exited marker (written before the install command returns) and the still-unsettled in-process deploy — the second covers what the first cannot, a child killed without running its finally. If either says the window collapsed, the test fails with that message instead of blaming the guard. The 409 assertion itself stays unconditional and nothing skips it.

Item 2 — MQTT can repeatedly publish

Five publishers on a 1 ms interval, a fixed 200 ms sleep, then assert(received.length > 10) — a throughput measurement that starves on a contended runner, which is what happened on the Node 24 and Node 26 legs of the 2026-09-04 unit run. The publishers now stay running until the eleventh delivery arrives, under a budget taken from what remains of the test’s own mocha timeout (captured at test entry, before the five serial connects, with 2 s reserved for teardown — so a stalled run cannot leave publishers running into the next test). Both original assertions — the count and received[0].name — are unchanged.

Item 3 — issue #2275, subscribe to retained record with patch operations

This is the failure blocking Unit Test on every PR, and it is a product bug, not a test race. It fails only in the HARPER_STORAGE_ENGINE=lmdb pass of test:unit:all, which is why the Windows job stays green.

#2497 changed getRecordAtTime to seed its reverse audit walk from the record's version. On LMDB the audit-store key is the entry's localTime, a different clock, so the first auditStore.get missed, the walk broke, and every historical partial-record read returned the live record. A durable MQTT subscriber resuming across two patches was handed the newest state twice and never saw the older one: observed as update 3, update 3, update 4. RocksDB was unaffected because the two clocks coincide there for a local write.

The walk now seeds from localTime unless the entry carries additionalAuditRefs, in which case #2497's ref-based resolution from the version is kept. The new unit test models the LMDB entry shape, version distinct from log key with no refs, and fails on base with the live record returned for both timestamps.

For the human reviewer

  • Item 3 picks the seed clock by whether additionalAuditRefs is present rather than by store.isRocksDB. Only RocksDB records carry refs, so the two are equivalent today; the refs check keeps crdt.ts free of a store-shape dependency. If an LMDB record ever carried refs the ref path would resolve them from the record version, which is wrong on LMDB. Reversible in one line.
  • Declined from item 3's review: the object-literal allocation on the no-refs path. The previous code allocated the same { txnLogKey, nodeId } inside resolveAuditPosition, and this function already spreads the record and allocates a Set on entry; it runs only for partial-record history reads.
  • The MQTT test no longer has a throughput floor. It now proves repeated publishes are delivered, not that eleven of them land inside 200 ms. If that floor was the point, it needs a separate explicit bound on delivery latency — a fixed sample cannot express it without reintroducing the flake.
  • Probe 3's fixture now carries a package.json and a custom install command, so it deploys through the custom-install branch while every other probe in the file deploys a manifest-less component. The 409 guard is indifferent to that, but the "blob untouched after a refused delete" assertion is now measured on a component with an opaque install.
  • Declined from the planning review: lifting the blocking-install fixture builder into a helper shared with stage-swap-availability.test.ts. The acceptance criteria for this task forbid modifying unrelated tests, and a shared module with one real consumer is not an improvement; the QA-701 comment names the sibling instead.
  • The AGENTS.md sentence records the deploy-window lever so the next flaky deploy probe does not re-derive it. Its Bun claim (the operation is not dispatched before the body completes) is my observation from this investigation, reproduced under Bun 1.4.0 locally, not something provable from the checkout.
  • Two MQTT robustness fixes came in from review and are strictly wider than the flake: the try/finally now covers the connect loop (a connect that threw part-way used to leave the clients it had already created publishing into every later test), and the subscription promise is marked handled at creation (a rejecting subscribeAsync used to be an unhandled rejection that took the runner down rather than failing this test). Both shapes pre-date this change; both are the same "one failure takes the whole run with it" class the task is about.
  • Declined from review, with reasons. The --no-timeouts finding (raised four times): the describe pins this.timeout(10000), which overrides it. An ENOENT-in-finally crash: this file runs under node:test, cleanup is after() rather than afterEach, and the runner sets no --test-timeout, so a test is never abandoned mid-flight. A waitFor-called-with-an-options-object finding: the helper takes (condition, timeoutOrOptions, interval) and branches on the type. And the 1 ms publish interval itself, which is the workload the test exists to exercise.

Verification

Built first — integration tests load dist/.

check result
QA-701 file, 20x under Node 20/20 pass
QA-701 file, 20x under Bun 1.4.0 (the runtime that failed) 20/20 pass
409 guard neutered, QA-701 re-run fails with should 409, got 200 and install command still blocked throughout — the probe genuinely reaches the guard, and names the right cause
guard restored, QA-701 re-run pass
test:unit:apitests (the MQTT suite) 201 passing, 0 failing
test:integration over integrationTests/deploy/ 76 pass, 0 fail, 1 skipped
test:unit:resources pass
test:unit:main 5417 passing, 1 failing — configValidator › does not warn when a relative rootPath resolves within the limit, the known worktree-path-length artefact (it resolves a relative rootPath against cwd, and any .claude/worktrees/<name> cwd blows the UDS path limit); fails from every agent worktree, unrelated to this diff
item 3 unit test, fix reverted fails: getRecordAtTime returns the live record for both historical timestamps
crdt + dualClockAuditRecord suites 43 passing
mqtt-test.mjs under LMDB, 3 runs (failed 3/3 before the fix) 29 passing each
mqtt-test.mjs under RocksDB, 2 runs 30 passing each
test:unit:resources under LMDB 1711 passing
format:check, lint:required clean

The Node/Bun matrix was run twice in full — once on the first commit, once after the review fixes — 20/20 on each runtime both times. The three later review commits were re-verified with test:unit:apitests plus a Node and a Bun run of the QA-701 file, all green.

Six pre-push review rounds (codex + gemini, with Harper domain adjudication on rounds 1 and 6). Rounds 5 and 6 covered item 3; round 5's only actionable finding was a comment claim, corrected in round 6. Rounds 2 and 3 each produced a real fix, listed above. Round 4 produced nothing new — every item was already adjudicated, pre-existing and unreachable, or factually incorrect.

The usual fails-on-base check does not apply literally — the "bug" here is the test's own flakiness, and the old test passes on base most of the time, which is the problem. The guard-mutation row above is the substantive equivalent: it proves the new probe reaches the assertion it claims to.

Items 1 and 2 were surfaced by the scheduled nightly runs with no tracked issue; item 3 is #2275, bisected to #2497 in that issue's comments. Item 1: https://github.com/HarperFast/harper/actions/runs/33951439751 (Integration Tests 5/6, Bun), on the anchor promoted days earlier by #2431. Item 2: https://github.com/HarperFast/harper/actions/runs/33888858312 (Unit Test Node 24 and 26).

Complexity: medium

Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=6 @ 0432f35

Human-Review-Need: 3 (decisions: engine-branch-by-refs-presence, install-gate-as-window-lever, newest-row-over-unique-row, gate-self-release-120s, forced-mqtt-end) @ 0432f35

kriszyp and others added 4 commits September 8, 2026 06:51
…eliveries instead of sampling 200ms

Two nightly-only failures on main, both wall-clock assertions in tests. No product code changes.

QA-701's non-terminal probe derived its window from a paced payload upload, which only holds if
deploy_component is dispatched before the request body finishes arriving -- a runtime-dependent
assumption that does not hold under Bun, and one a loaded runner can lose anyway. The deploy is
now parked inside its own install_command (the lever stage-swap-availability.test.ts already
uses), and the gate script writes an `install-exited` marker on its way out so the probe can prove
the window was still open when the delete returned. That makes "the window closed early" a
different, separately worded failure from "the guard did not fire"; the 409 assertion stays
unconditional and nothing skips it.

'can repeatedly publish' sampled a fixed 200ms window and asserted more than ten deliveries, which
starves on a contended runner. The publishers now stay running until the eleventh delivery
arrives, bounded by a backstop derived from the test's own mocha timeout -- the idiom the
retained-message test in the same file already uses, so the inner wait cannot race the outer one.
Both original assertions are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WB3hjUgCynwqBCKKpTckkx
…e, prove the deploy gate two ways

Codex and the Harper domain lens both found the MQTT delivery budget was measured from AFTER the
five serial connects, so a stalled run could keep publishers alive past mocha's deadline and spill
into the next test. Capture the deadline at test entry and spend only what is left, reserving 2s
for teardown.

The domain lens also found the hole in the gate's proof: a child killed without running its
`finally` leaves no `exited` marker either, so an absent marker alone would report "guard did not
fire" for what was a window collapse. Require the in-process deploy to also still be unsettled at
that instant, and name both in the failure message.

Gemini's `--no-timeouts` variant of the budget finding was dropped on adjudication -- the describe
pins this.timeout(10000), which overrides it -- so the budget reads this.timeout() directly. Both
lenses' comment-narration nit is applied: the gate rationale is stated once, and the nightly-run
anecdote is gone (AGENTS.md carries the durable version).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WB3hjUgCynwqBCKKpTckkx
…get to the deadline

Gemini found that the try/finally covered only the delivery wait, so a connect that threw part-way
left the clients it had already created publishing every 1 ms into every following test -- the
pre-existing shape, and exactly the cross-test contamination this task is about. The setup loop is
now inside the try.

Both lenses found the 1000 ms floor on the wait budget could outlast what was left of the mocha
deadline, reintroducing the overrun the budget exists to prevent. Floor is 0: with no budget left
the test fails immediately with the count it reached, which is what mocha was about to do anyway
with a less useful message.

Declined, with reasons: Gemini's ENOENT-in-finally crash (this file runs under node:test, not
mocha; cleanup is `after()`, once, not `afterEach`; the runner sets no --test-timeout so the
default is Infinity and a test is never abandoned mid-flight -- and the cleanup already swallows
errors), and its re-raised `--no-timeouts` variant (round 1 adjudicated it: the describe pins
this.timeout(10000), which overrides it).

Comment narration trimmed again on the lines both lenses named.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WB3hjUgCynwqBCKKpTckkx
…oyment row

Gemini found the subscription IIFE had no synchronous handler, so a subscribeAsync that rejected
while the loop was still connecting the next client was an unhandled rejection -- taking the runner
down rather than failing this one test. Marked handled at creation; the Promise.all await is still
the real handler. Pre-existing, but it is the same "one failure takes the whole run with it" class
this task is fixing.

Also replaced the exactly-one-row assertion I introduced with newest-row selection. A fresh
per-suite instance cannot produce a second row for this project today, so the rigidity bought
nothing and would misfire the day something deploys that project twice.

Declined again, unchanged reason: the `--no-timeouts` variant (third raising) -- the describe pins
this.timeout(10000), which overrides it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WB3hjUgCynwqBCKKpTckkx
@kriszyp
kriszyp requested review from dawsontoth and heskew September 8, 2026 13:26

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

Copy link
Copy Markdown
Contributor

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 replaces flaky time-based pacing and fixed delays in integration and API tests with deterministic synchronization mechanisms. In qa701-deployment-payload-ops.test.ts, paced uploads are replaced with an install gate script that holds the deployment in a non-terminal state until released. In mqtt-test.mjs, fixed timeouts are replaced with waitFor polling, and a try-finally block is introduced to ensure cleanup of intervals and MQTT clients. Feedback is provided on mqtt-test.mjs to use optional chaining (error?.message) when handling caught errors to prevent a secondary TypeError if the thrown error is null, undefined, or a primitive.

Comment thread unitTests/apiTests/mqtt-test.mjs
…e record version, so LMDB partial-record history resolves (harper#2275)

On LMDB the audit-store key (localTime) and the record version are separate
clocks, and a record carries no additionalAuditRefs. #2497 seeded the walk
from the version, so the first audit lookup missed and every historical read
returned the live record: a durable MQTT subscriber resuming across two
patches received the newest state twice and lost the older one.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@kriszyp kriszyp changed the title Hold the QA-701 409 probe's window open causally, and wait for MQTT deliveries instead of sampling 200ms Fix LMDB partial-record history resolution broken by #2497 (harper#2275), hold the QA-701 409 probe's window open causally, and wait for MQTT deliveries instead of sampling 200ms Sep 8, 2026
@kriszyp kriszyp added this to the v5.3 milestone Sep 8, 2026
@kriszyp
kriszyp marked this pull request as ready for review September 8, 2026 15:43
@kriszyp
kriszyp merged commit 724c317 into main Sep 8, 2026
60 of 62 checks passed
@kriszyp
kriszyp deleted the fix/nightly-test-stability branch September 8, 2026 15:44
@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant