fix(request-context): add deterministic failure-boundary coverage for runWithContext

[Sagethepeak]

📝 Description

runWithContext decides whether a request is traceable and whether an attacker-controlled string can forge a log line. It is the single point at which an inbound correlation id is admitted to, or refused entry to, the AsyncLocalStorage context that every downstream audit write, outbound RPC call and log line reads — and it had no coverage of its own.

Its combined failure boundary in particular went entirely untested: when the inbound id is unusable and the id source is also unavailable, nothing asserted that the run still succeeds, that the id it degrades to is log-safe, or that a caller is never left without a context. That is the boundary #2697 (getCorrelationId), #2699 (getOrGenerateCorrelationId) and #2702 (the middleware) each deliberately do not reach.

Root cause: the test seam had gone dead

The coverage could not have been written as things stood. Commit 4e9bcaf0 (#2705) changed generateCorrelationId to call ulid() directly, which left _setUlidGeneratorForTesting write-only: it still assigned ulidGenerator, but nothing read it. Every test that set the hook silently exercised the real generator and asserted nothing about the failure it meant to drive — a green test proving nothing.

🎯 Type of Change

• Bug fix
• Security enhancement
• New feature
• Breaking change
• Documentation update
• Refactoring
• Performance improvement
• Other

🔧 Changes Made

Files Modified

• backend/src/lib/requestContext.ts — restore the seam; document the invariants

New Files Added

• backend/src/tests/run-with-context.test.ts — 81 deterministic tests

Key Changes

The seam (requestContext.ts, 3 functional lines). ulidGenerator became a nullable ulidOverride that generateCorrelationId resolves per call via ulidOverride ?? ulid. This matters in three ways:

• ulid stays dereferenced lazily, so module-level mocking still reaches the healthy path. The original ulidGenerator was captured at module load, which broke that too.
• The override cannot weaken validation. A value it returns is still checked against ULID_PATTERN and still falls through to the degraded path, so a test double can drive the failure boundary but can never launder a malformed or tainted id into the context.
• null is the default, so production behaviour and the healthy path are untouched.

Because the package is ESM ("type": "module") and no test runner can spy on a module namespace object, monkey-patching the ulid export is not available — this exported hook is the only portable way to reach the boundary.

The invariants (runWithContext docblock). The function's body is unchanged; what was missing was a statement of what it guarantees. Now documented: never stores an unusable id; never throws on the id path; is total under id-source failure; propagates fn's outcome with original identity and tears the context down on both paths; never pollutes the caller's scope, while work scheduled inside fn keeps the id.

The tests. Success, rejection, length and degraded-source handling; throw and rejection propagation with teardown; work that outlives the run; concurrency, nesting and retry; the middleware and request-logger paths that feed it; plus a bounded deterministic property pass over raw and sanitised input.

🧪 Testing

Test Coverage

• New suite: 81/81 pass, identical across 3 consecutive runs.
• Measured against the unfixed source: 10 of 81 fail, every one on the combined id-source boundary — which is what makes the number meaningful rather than incidental.
• Target file: 96.03% statements, 84.44% branches, 100% functions. The remainder needs node:async_hooks and node:crypto module mocking, which this suite avoids by design; it is not reachable without that machinery.
• Blast radius isolated. Running the full backend suite twice — identical tree, only requestContext.ts swapped — the only file whose outcome differs is run-with-context.test.ts (10F/71P → 0F/81P). Suite totals go 219 failed / 621 passed → 209 failed / 631 passed.
• No pre-existing failure introduced or masked: 209 failures before and after, against the same captured baseline.
• Typecheck: 136 errors before and after, none in either file touched here.

Determinism

No clock, network, database, or Express app is involved, and randomness is never asserted against a fixed value. Uniqueness is asserted as "all distinct". ULID ordering is never asserted, because plain ulid() is not monotonic within a millisecond. The one place time is involved mocks Date.now explicitly and restores it in a finally.

Edge cases tested

Non-string, empty, blank, and whitespace-only ids (including NBSP); over-length ids at MAX + 1; ids made only of separator characters; ids whose length is exactly at the limit; CR, LF, CRLF, NUL, tab, C0 controls, DEL and CSI/ANSI escape sequences; a planted sentinel that must never survive into the context; ids whose only defect is case. Also asserted in the other direction — a legitimate id is preserved verbatim, so refusing unsafe ids has not become a blanket "rewrite anything unfamiliar" policy that would break a caller's own correlation.

📋 Contract-Specific Checks

Not applicable — backend-only change; no Soroban contract code touched.

📋 Review Checklist

• Code follows project style guidelines
• Documentation updated if needed
• No sensitive data exposed
• Error handling implemented
• Edge cases considered
• Code is self-documenting
• No hardcoded values
• No unused imports or variables
• Functions are properly documented
• Complex logic is commented
• No breaking changes introduced

🔍 Code Quality

• No unused imports or variables
• Functions are properly documented
• Complex logic is commented

🚀 Performance & Security

• No potential security vulnerabilities introduced
• Input validation implemented
• No sensitive information in logs

One production-code behaviour change, and it only widens what is reachable in tests. Adding the assertion that a valid id is stored verbatim caught a real assumption gap in the first draft of this suite.

📚 Documentation

• Code comments added for complex logic
• README updated if needed
• API documentation updated if needed
• Changelog updated

🔗 Related Issues

Closes #2696

Related to #2697, #2699 and #2702 — each covers one caller of the boundary this PR covers jointly. Note #2705: the commit that introduced this regression.

📋 Additional Notes

Three pre-existing problems on main are out of scope here and deliberately untouched, per triage. Flagging rather than fixing, because each is larger than this issue and several block CI on every branch:

1. npm ci fails — backend/package.json and backend/package-lock.json are out of sync. The lockfile is missing ajv, ajv-formats, cors, dotenv, helmet, ulid and zod, all of which the manifest requires.
2. CI calls a script that does not exist — .github/workflows/backend-ci.yml invokes npm run typecheck, but no typecheck script is defined. It also invokes Prettier, which is not a devDependency.
3. npm test / npm run coverage / jest are dead — Download export #2844 migrated the backend to Vitest but left these scripts in place, and backend/jest.config.js is a leftover.

Why validation ran in a scratch environment

Because of (1), dependencies cannot be installed from the lockfile at all. I validated in a copy at /tmp/opencode/be2696 with Vitest 2.1.9 + @vitest/coverage-v8 2.1.9 and a repaired node_modules. CI will not go green on this branch for reasons unrelated to it, so the per-run evidence above is provided instead — please read it as the validation record rather than as a claim that npm ci && npm test passes.

Sibling suites still unloadable

Following #2844, request-context-middleware.test.ts (needs fast-check) and request-context.test.ts (needs supertest) both collect 0 tests because those packages are no longer dependencies. src/lib/__tests__/requestContext.test.ts loads and runs but has 2 assertions that went stale. Left as-is by decision; this PR does not depend on them.

Source corruption on main (worth a separate issue)

While baselining I found files that do not parse, all pre-existing on main:

• src/tests/shutdown-ordering.test.ts — base64-encoded, never decoded
• src/lib/migrations/runner.ts — 1365 lines collapsed onto one (newlines stripped)
• src/tests/api-key-rotation-integration.test.ts, api-keys-audit-logs.test.ts, conditional-write.test.ts, migration-failure-boundary.test.ts, migration-runner-mocked.test.ts, shutdown-ordering.test.ts — syntax errors from the same corruption

migration-failure-boundary.test.ts is the same pattern of work as this PR, so this one may be recoverable by checking git log for a recoverable revision. Suggest a dedicated issue for the corruption sweep.

🧪 How to Test

cd backend
npx vitest run src/tests/run-with-context.test.ts
npx vitest run src/tests/run-with-context.test.ts --coverage --coverage.include='src/lib/requestContext.ts'

To see the coverage earn its keep, revert the source and watch the suite fail:

git show upstream/main:backend/src/lib/requestContext.ts > src/lib/requestContext.ts
npx vitest run src/tests/run-with-context.test.ts   # 10 failed | 68 passed

⚠️ Requires a working node_modules; see the note above.

⚠️ Breaking Changes

None.

🔄 Migration Steps

None.

[Sagethepeak]

Correction: my "may be recoverable by checking git log" note was wrong

I checked properly before filing the follow-up, and the recovery story is different from what I wrote. Correcting it:

• shutdown-ordering.test.ts — recoverable. It was base64-encoded by fd6ea43b (fix: add deterministic failure-boundary coverage for isShuttingDown #2833), but its parent 04b047b8 holds a clean 638-line version. My earlier check only looked at the introducing commit and wrongly concluded no clean revision existed.
• runner.ts — not a "collapsed single line" file. I said that and it was wrong. It is 1365 normal-length lines; the damage is ~88 dropped or substituted bytes inside them. Long predates HEAD — already affected at 773d8f2c.
• migration-failure-boundary.test.ts — not recoverable by revert. a66970f6 (fix: add deterministic failure-boundary coverage for migrations CLI main #2832) both corrupted it and added ~100 lines of real new tests, so reverting would lose the work.

Root cause, now identified

Individual bytes are being dropped or substituted inside committed source. Two variants observed:

• Dropped — runner.ts: instance of Error (the s gone from instanceof), and 18 template literals missing their ``` escapes, so a bare backtick terminates the literal early and the parser desyncs for everything after.
• Substituted — migration-failure-boundary.test.ts: runner.loa\MigrationsFromFS()whereloadshould be (a backtick replaced thed), and QFC_MIGRATION_902_ALLOWEB\x03whereALLOWED should be — 4 occurrences, with a raw ETX byte (0x03) sitting in identifier position. The fixture it tests,v902_gated.ts`, spells it correctly.

The parser desync explains the misleading error counts: 77 of runner.ts's 88 TS1005 errors are cascade from one unescaped backtick, not 77 separate bugs.

Proof the diagnosis is right

migration-failure-boundary.test.ts goes from 9 typecheck errors to 0 with exactly 5 character edits:

Line	Corrupt	Intended
298	runner.loa`MigrationsFromFS()	runner.loadMigrationsFromFS()
257, 370, +2	QFC_MIGRATION_902_ALLOWEB\x03	QFC_MIGRATION_902_ALLOWED

So this is mechanically fixable, not a rewrite — worth knowing before scoping the sweep.

Separately, one number from this PR is now better explained: all 136 typecheck errors on main come from these 10 files, and not one from healthy code. But it is still not part of #2696 — runWithContext imports only node:async_hooks, node:crypto and ulid, and none of its 7 production consumers touch any corrupted file. This PR remains scoped to the issue.

[Sagethepeak]

@Baskarayelu — this is the #2696 deliverable (assigned to me), following the merged #2866 on the same module.

With ~8 boundary-coverage PRs open and CI red repo-wide, the evidence up front:

• 81/81 pass, identical across 3 consecutive runs.
• Reverting only requestContext.ts to main fails 10 of the 81 — measured, not asserted, and all 10 on the combined id-source boundary.
• Full-suite delta with only that file swapped: 219F/621P → 209F/631P, and run-with-context.test.ts is the only file whose outcome changes. No pre-existing failure introduced or masked.

The one thing worth knowing: _setUlidGeneratorForTesting was write-only after #2705, so this boundary was previously untestable — a suite written against the old hook would have passed while asserting nothing. Repairing that dead seam is the only production change (3 lines), and it can't weaken validation, since an override value is still checked against ULID_PATTERN.

CI on this branch is red for reasons unrelated to it: broken npm ci (manifest/lockfile desync), a typecheck script CI invokes that doesn't exist, and 136 pre-existing typecheck errors from byte-level corruption in committed source — the latter filed as #2870 with a verified 5-character fix for one file. Happy to split those out as follow-ups if you'd rather have CI green before merging this.

One caveat on my own evidence: requestContext.ts sits at 96.03% statements / 84.44% branches from this suite alone. The remaining branches need node:async_hooks and node:crypto module mocking, which I avoided deliberately because the package is ESM and this suite is meant to be self-contained — but that is a deliberate limit, not full coverage.