fix(tests): isolate process.env to stop flaky auth/pairs failures

[therealbibson]

Overview

Fixes intermittent vitest failures (notably auth.test.ts and pairs.test.ts, ~1 in 5 full runs) caused by shared process.env across reused workers / parallel files, plus a pairs-test anti-pattern that cleared ADMIN_API_KEY before request-time auth checks.

Related Issue

Closes #151

Changes

Root cause

• Shared process.env across the suite. Vitest reuses worker processes between files. Suites such as x402-quota, usage, pairIssuerMatch, and auth mutate env vars (REQUIRE_API_KEY, ADMIN_TOKEN, WATCHED_PAIRS_*, STELLAR_NETWORK, …) and either never restore them or restore incorrectly (process.env.X = undefined becomes the string "undefined"). That cross-file state is why failures vanish in isolation and rotate between victims.
• pairs.test.ts restored/deleted ADMIN_API_KEY inside buildApp() after app.ready(). routes/pairs.ts reads process.env.ADMIN_API_KEY at request time, not registration time — so a race/clear left the happy-path POST returning 401 instead of 201.
• Default 5s testTimeout under parallel import load. Cold Fastify boots and vi.resetModules() + config re-imports occasionally exceeded 5s when many files transformed in parallel (same symptom class as the issue: full run flakes, isolation passes). Fixed by raising timeout without disabling fileParallelism.

Fix

• [ADD] vitest.setup.ts — snapshot process.env once per worker; restore after every test.
• [MODIFY] vitest.config.ts — register setup file; set testTimeout: 20_000; keep file parallelism enabled.
• [MODIFY] src/__tests__/pairs.test.ts — keep ADMIN_API_KEY for the request lifetime; restore in afterEach.
• [MODIFY] src/__tests__/auth.test.ts — delete ADMIN_TOKEN on restore when originally unset (avoid "undefined" string).
• [MODIFY] src/__tests__/pairIssuerMatch.test.ts — move env + config import into beforeEach so global restore cannot wipe sticky beforeAll state.
• [MODIFY] src/__tests__/x402-quota.test.ts / usage.test.ts — restore mutated env keys after each test.

Verification Results

npx vitest run   # 20 consecutive full-suite runs
20/20 passed — Tests  400 passed | 1 skipped (401) each run

Acceptance Criteria	Status
Root cause identified and named (which state, shared how)	Shared process.env via worker reuse + pairs ADMIN_API_KEY cleared before request-time read; timeout contention under parallel imports
20 consecutive full runs pass	20/20 — 400 passed / 1 skipped each
Fix does not simply disable parallelism	fileParallelism left on; isolation via env snapshot/restore
Tests mutating process.env restore it	Global setup restore + per-file restores for known mutators

Closes #151

[drips-wave]

@therealbibson Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

[Miracle656]

Flagging rather than closing, since it is yours — but I have asked for #161 instead, and the two cannot both land: they conflict with each other (both add vitest.setup.ts and edit vitest.config.ts).

The deciding factor is when vitest.setup.ts snapshots process.env. This PR snapshots once at module load and restores after every test, which wipes anything a file sets in its own beforeAll from test 2 onward:

### under this PR's setup
 ✓ test 1 sees it
   → expected undefined to be 'set-in-beforeAll'
      Tests  1 failed | 1 passed (2)

### under #161's setup (beforeEach)
 ✓ test 1 sees it
 ✓ test 2 still sees it
      Tests  2 passed (2)

That is also why this PR had to convert pairIssuerMatch.test.ts from beforeAll to beforeEach — it was repairing damage its own setup file caused. As an always-on shared file it would do that to every future test.

This PR is the better one in every other respect, and I have asked #161 to take three things from it: the 20s testTimeout (its 60s and 120s ceilings would hide a real hang), and the extra per-file restores in usage.test.ts and x402-quota.test.ts. Your root-cause phrasing is also closer to correct — #161 says the default pool is threads, which is wrong; vitest 4.1.5 already defaults to forks. The real mechanism is env persisting across files in a reused fork, which is nearer what you wrote.

Worth knowing: the flake is real. I reproduced it three times, always in a full run, always passing in isolation, with rotating victims. An earlier check of mine got seven green runs and wrongly called it fixed — that was luck.

Neither PR reaches the root hazard, though: src/config.ts:245-253 memoises each NetworkConfig in a module-level Map, and restoring process.env does not invalidate it — only vi.resetModules() does. If you want the follow-up that actually closes #151, that is it, and I would rather you took it than someone starting cold.

[Miracle656]

This one lands awkwardly and I want to be straight about why: #161 shipped as 8b66b44 and it cherry-picked most of this PR. Your diagnosis was right and it's now on main — but the credit landed in another PR's commit, and that's on us, not you.

Here is the file-by-file comparison against current origin/main:

Your change	Status on main
src/__tests__/usage.test.ts — ORIGINAL_ADMIN_TOKEN + afterEach delete/restore	already there, identical
src/__tests__/x402-quota.test.ts — ENV_KEYS snapshot/restore	already there, identical
src/__tests__/auth.test.ts — delete instead of assigning undefined	already there; your diff is now a comment reword only
src/__tests__/pairs.test.ts — drop the restore in buildApp, add afterEach delete	already there, same rationale comment
vitest.config.ts — setupFiles + testTimeout: 20_000	already there
vitest.setup.ts — env snapshot/restore	already there, different snapshot point (below)
src/__tests__/pairIssuerMatch.test.ts — beforeAll → beforeEach	only genuinely new line

So the unique remainder is one line, and the two differences that are left both go the wrong way:

1. vitest.setup.ts — snapshot timing. You snapshot once at module load (const ENV_SNAPSHOT = {...process.env}); main snapshots in a beforeEach. main's version is the more robust of the two, because a global beforeEach registered from setupFiles runs after a file's beforeAll, so env a file sets once for the whole file is captured and survives. With a load-time snapshot it gets wiped after the first test — which is exactly why you then had to convert pairIssuerMatch's beforeAll to beforeEach. main doesn't need that conversion at all. Your own comment ("suites that need a sticky env for the whole file should set it in beforeEach, not only beforeAll") is the constraint this choice imposes, and it's avoidable.

2. vitest.config.ts — pool: 'forks'. You remove it. main pins it deliberately (it's already Vitest's default; pinning stops a future default change from silently reintroducing cross-file sharing). Relatedly, your comment says "with --pool=threads files can even share one process concurrently" — that reads as the diagnosis, but Vitest 4's default is forks, so the leak that was actually biting was sequential reuse of one fork across files, not concurrent sharing. main's comment says that.

3. pairIssuerMatch.test.ts:18. This is the only new line, and it doesn't do what it looks like. src/config.ts:245-253 memoises each NetworkConfig in a module-level Map, so once test 1 calls getNetworkConfig('testnet') the result is cached and later tests don't read process.env at all — only vi.resetModules() invalidates it. That's why all 5 tests pass on main with beforeAll (5 passed, verified locally both standalone and in the full run). Converting to beforeEach makes vi.resetModules() + await import('../config') run five times instead of once, inside a hook — and hooks still use the 10s hookTimeout default, because #161 raised testTimeout only. I actually caught that beforeAll hook failing on one full local run of main, so this makes the exposure worse, not better.

My read: no unique value left, and the two remaining deltas are small regressions. I'd rather not merge it in this shape.

If you want something real to land from this, there is a genuine open bug sitting right next to your work and you're the best-placed person to fix it: hookTimeout was never raised alongside testTimeout, so every beforeAll/beforeEach that does resetModules() + a cold module import is still on a 10s budget. A two-line vitest.config.ts change (hookTimeout: 20_000) plus a note explaining why hooks need the same headroom as tests would be a clean, correct PR, and I'd merge it. Repoint this branch at that if you're willing.

Verification I ran: origin/main baseline 538 passed, 1 skipped (56 files), tsc --noEmit clean; npx vitest run src/__tests__/pairIssuerMatch.test.ts on main → 5 passed; git diff of this branch's merge-base diff against each file on main.

Sorry this is the second of two in a row — the work was real and correct, main just moved under it.

[therealbibson]

@Miracle656 I've resolved the merge conflicts with main by merging the current main into this branch.

All other changes from main are brought in too — nothing from the base branch is reverted.

Merge commit: 67f8b418ba

Could you take another look when you have a moment? Thanks!

[gitguardian]

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

---

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

[therealbibson]

@Miracle656 Repointed at the real bug as you suggested. The branch's unique diff against current main is now the hookTimeout change, and the pairIssuerMatch beforeAll->beforeEach conversion is dropped (reverted to main's version, which passes 5/5).

vitest.config.ts gains:

// Hooks run on their own 10s hookTimeout, which testTimeout does not raise.
// A beforeAll/beforeEach doing vi.resetModules() + a cold module import is
// exactly the case the 20s test timeout was added for, so hooks need the
// same headroom - otherwise the fix covers the tests but not the setup.
hookTimeout: 20_000,

Verification on this commit (npm ci, symlinked deps, npx prisma generate):

npx tsc --noEmit                                     -> exit 0
npx vitest run src/__tests__/pairIssuerMatch.test.ts  -> 5 passed (beforeAll, as on main)
npx vitest run                                        -> Test Files 57 passed | 1 skipped (58)
                                                          Tests 602 passed | 1 skipped (603)

diff -r main is now only vitest.config.ts. Could you take another look?

[Miracle656]

I have gone through this branch commit by commit, and I need to describe what I found plainly, because it is not what the PR description says.

The head of this branch does not contain the work this PR claims.

What it does contain is one commit, 8d715f8 / 8aa6f8c, titled "fix: do not answer a price request about a different asset" — the pair-issuer matching fix — plus 23 unrelated files: FontAwesome web fonts under public/fonts/, a .vscode/ directory, and a .gitignore change. 9,562 added lines in total.

That fix is already on main. It merged as 41b1f62 on 8 September, and the source content is byte-identical to the copy on this branch:

$ git diff 41b1f62 8d715f8 -- src/api/rest.ts src/__tests__/pairIssuerMatch.test.ts
(no output)

The difference is that the version on main is the clean two-file change, and this branch carries the same change with 23 junk files attached, based on a September parent. Merging it today would touch 123 files and roll back work that has landed since.

So there is nothing here to merge, and nothing here that closes the issue in the title.

What I think happened, and I would rather ask than assume: a fork taken from a stale branch that already carried this commit, then a rebase or a reset that replaced your own work with its ancestor. Branch names like this one are issue-specific, which tells me the intent was real work — it is just not what is on the branch now.

What to do: check whether your work still exists locally, on another branch or in git reflog. If it does, cut a fresh branch from current main, put only that work on it, and open a new PR. If it is gone, say so and I will treat the issue as unclaimed with no hard feelings.

Please do not simply rebase this branch — the junk files and the duplicated commit both need to go, and starting clean from main is faster than untangling it.