Skip to content

fix(test): eliminate race in Txn Expiration flaky test - #1459

Closed
kriszyp wants to merge 2 commits into
mainfrom
kris/fix-txn-expiration-flake
Closed

fix(test): eliminate race in Txn Expiration flaky test#1459
kriszyp wants to merge 2 commits into
mainfrom
kris/fix-txn-expiration-flake

Conversation

@kriszyp

@kriszyp kriszyp commented Jun 23, 2026

Copy link
Copy Markdown
Member

Root cause

The Slow txn will expire test asserts trackedTxns.size === existingTxns at the end, where existingTxns was sampled before the SlowResource.get call. The problem: trackedTxns is a module-level singleton Set shared across the whole test suite. The 5 «existing» transactions sampled as the baseline are leftover open txns from earlier tests — and they can expire (and be removed from the Set by the 20ms timer) during the 50ms Promise.race window that follows, causing the count to fall below existingTxns. This produces the 0 == 5 AssertionError seen on Node v26.

The polling loop added to stabilize the baseline only checks for a 5ms quiet window; a txn whose countdown hits zero just after that window still fires in the next tick.

Fix

Replace the aggregate-count assertion with a direct membership check on lastTxn — the specific DatabaseTransaction object added by SlowResource.get(3). This is immune to other tests' txns expiring concurrently.

// before
assert.equal(trackedTxns.size, existingTxns);

// after
assert.ok(!trackedTxns.has(lastTxn), 'expected the slow transaction to have been expired and removed from trackedTxns');

Test plan

  • CI passes on all three Node versions (v22, v24, v26) — the assertion is now independent of background txn churn
  • The lastTxn reference was already captured unconditionally on line 52; only the final assertion changes

🤖 Generated with Claude Code

The `assert.equal(trackedTxns.size, existingTxns)` assertion was flaky
because other tests' open transactions can expire and be removed from
the module-level set during the 50ms Promise.race window, causing the
count to drop below the sampled baseline.

Fix by checking whether the specific SlowResource transaction (already
captured as `lastTxn`) was removed, rather than relying on the aggregate
count staying stable.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@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 updates a transaction expiration unit test in txn-tracking.test.js to assert that a specific transaction was expired and removed, rather than asserting on the total size of tracked transactions, which was unreliable due to concurrent expirations. There are no review comments, so there is no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Fixes the Format Check failure on this branch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kriszyp pushed a commit that referenced this pull request Jun 23, 2026
The Slow txn will expire test asserted trackedTxns.size === existingTxns,
where existingTxns was sampled before SlowResource.get. trackedTxns is a
module-level singleton shared across the suite; leftover open txns from
earlier tests can expire (removed by the 20ms timer) during the following
50ms Promise.race window, dropping the count below existingTxns (the
0 == 5 / 1 == 6 AssertionError seen in CI). This branch's added blob tests
shift suite timing enough to make the latent flake deterministic.

Assert membership of the specific txn from SlowResource.get(3) instead,
which is immune to background txn churn. Identical to #1459.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp kriszyp closed this Jun 23, 2026
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