Conversation
`bounds public-IP lookup and omits the field on timeout` asserted
`Date.now() - started >= 2000` against a 2000ms `AbortSignal.timeout`. The
timer and `Date.now()` are not the same clock source, so the observed
elapsed time can land just under the configured timeout. The Bun
compatibility job has already failed on it:
AssertionError: expected 1999 to be greater than or equal to 2000
Assert the mechanism instead: capture the abort reason and require it to be
a `TimeoutError`, which only the lookup's own `AbortSignal.timeout` produces.
The upper bound stays, so an unbounded hang is still caught.
Contributor
🟢 No documentation gap found — high confidenceThe pull request only changes tests, lockfiles, generated metadata, or dependency metadata. This review is advisory and does not block merging. |
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.
Problem
src/site-memory/environment.test.ts→bounds public-IP lookup and omits the field on timeoutasserts a wall-clock floor against the lookup's 2000 msAbortSignal.timeout:The timer that fires the signal and
Date.now()are not the same clock source, sothe observed elapsed time can land a millisecond under the configured timeout. This
is flaky by construction, and it has already failed in CI on the Bun compatibility
job:
(Seen on the Bun job for #511, a docs/CLI-only PR that cannot affect this code path.)
Fix
Assert the mechanism rather than the clock. The fetch mock already receives the
signal, so capture
signal.reasonon abort and require it to be aTimeoutError—which only
AbortSignal.timeoutproduces. The< 4000upper bound stays, so anunbounded hang is still caught.
This keeps exactly what the test was written to prove — the lookup is bounded by its
own timeout and drops the field rather than propagating — without racing two clocks.
Verification
npx vitest run --project unit src/site-memory/environment.test.ts→ 6 passed.bun vitest run --project unit src/site-memory/environment.test.ts→ 6 passed.AbortSignal.timeout(...).reason.name === 'TimeoutError'on both Node24 and Bun 1.3.14, so the assertion is safe on the Bun job.
timeout signal with a manual
AbortControlleraborted after 10 ms, and the testfailed with
expected 'Error' to be 'TimeoutError'. Source restored afterwards.npx tsc --noEmit→ clean.npx vitest run --project unit src/site-memory/→ the only remaining failures arefour pre-existing
EPERM: symlinkcases inlocal-store.test.ts, which are alocal Windows privilege limitation on my machine and pass on CI. Untouched here.