test(core): wait for the ack instead of assuming it arrives within 20 ms - #156
Merged
Conversation
`inbound.ack-ordering.mjs` failed in a clean container build of a commit
CI had just passed:
✖ the ack waits for a promise-returning handler (the whole R1.6 premise)
actual: [ 'handler-start', 'persisted' ]
expected: [ 'handler-start', 'persisted', 'ack' ]
Nothing had regressed. `vti-didcomm-js` 0.7.0's `_dispatchFrame` still
awaits `_deliver` before `_ackReceived`, and the same file passes alone
every time. All three tests did `await settle()` — a fixed 20 ms — and
then asserted the ack was on the socket. The ack is an authcrypt `pack`,
and `node --test tests/*.mjs` runs core's 57 files in parallel with
concurrency `cores - 1`. Measured in the container: idle, the ack lands
3-4 ms after the handler resolves; with the suite cold-starting beside
it, 14 ms — most of the budget — and on a 24-core machine racing 23
other crypto-heavy files it once took more. GitHub's 4-core runners run
three files at a time, which is the only reason CI stays green; every
crypto-touching test file added to core moves that runner along the same
curve.
This is the wrong test to tolerate a flake in. It is the pin on
persist-before-ack: the one that goes red if the library ever acks
first again, and a pin that goes red for unrelated reasons trains
everyone to hit retry past it.
The positive assertions now poll for the condition with a 5 s deadline
(`until`), so they cost time only when about to fail. The one negative
assertion — no ack while the persist is in flight — keeps its fixed
wait, because "nothing happened" can only be established by giving it
time to happen; under load that window can only false-pass, never
false-fail, so it did not share the flake. The KNOWN GAP test's
guidance about the library beginning to honour rejections moves into
the timeout message, where it now fires.
Not reproducible on demand: seen once for certain in four stock
container builds, then 0 of 16 further full-suite runs. The fix is
argued from the mechanism and the measurements, not from a
reproduction. Old and new versions both pass 8/8 in the full parallel
suite; the new one also 4/4 under 24 busy loops.
Signed-off-by: Geoff Turk <geoff@ic3.dev>
geoffturk
force-pushed
the
fix/ack-ordering-test-load
branch
from
September 1, 2026 09:00
72f7b56 to
0c1eee6
Compare
|
🛡️ AI Agentic Security Code Review — all clear. We checked this change and found nothing to report. Keep shipping secure code! Note: for major, breaking, or feature-introducing changes, you can always request an in-depth review from the security team. |
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.
inbound.ack-ordering.mjs— the pin on persist-before-ack (R1.6) — failed in a clean container build of a commit CI had just passed:Nothing regressed
vti-didcomm-js0.7.0's_dispatchFramestill awaits_deliverbefore_ackReceived, and the file passes alone every time (5/5, ~85 ms). All three tests didawait settle()— a fixed 20 ms — then asserted the ack was on the fake socket. The ack is an authcryptpack, andnode --test tests/*.mjsruns core's 57 files in parallel at concurrencycores - 1.Measured in the build container (node 24):
That is most of a 20 ms budget before the worst case: 23 files all cold-starting at once on a 24-core machine. GitHub's 4-core runners run three files at a time, which is the only reason CI stays green — and every crypto-touching test file added to core moves that runner along the same curve.
Why this test in particular should not flake
It is the one that goes red if the library ever acks before the handler settles — the defect that lost consent requests on MV3 teardown. A pin that goes red for unrelated reasons trains everyone to retry past it.
The change
until(). They cost time only when about to fail.onInboundMessage's comment and this test together") moves into the timeout message, which is now where it would fire.Honesty about reproduction
Seen once for certain in four stock container builds, then 0 of 16 further full-suite runs and 0 of 4 under 24 artificial busy loops. This fix is argued from the mechanism and the measurements above, not from an on-demand reproduction. Both old and new pass 8/8 in the full parallel suite; the new one also passes 4/4 under load.
Test-only change; nothing in
dist/moves.