fix(test): await startCrlServer so the EADDRINUSE retry covers the listen#1710
Open
kriszyp wants to merge 2 commits into
Open
fix(test): await startCrlServer so the EADDRINUSE retry covers the listen#1710kriszyp wants to merge 2 commits into
kriszyp wants to merge 2 commits into
Conversation
The unawaited return let server.listen()'s rejection escape the try/catch, so the 5-attempt retry loop only guarded generateCrlCertificates, never the actual bind. Ports are drawn from 50000–59999 which overlaps Linux's ephemeral range (32768–60999), so collisions with kernel-assigned ports are realistic. Adding await matches the existing setupOcspResponderWithCerts pattern (which already awaits startOcspServer and is flake-free). Discovered via flaky crl-verification.test.ts on Node 22 in PR #1709's CI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the setupCrlServerWithCerts function in integrationTests/utils/securityServices.ts to await the promise returned by startCrlServer. This ensures that any EADDRINUSE errors thrown during server startup are properly caught by the local try-catch block, allowing the retry logic to execute as intended. There are no review comments, and I have no additional feedback to provide.
Contributor
|
Reviewed; no blockers found. |
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
1 participant
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.
What
One-line test-harness fix in
integrationTests/utils/securityServices.ts. No production code changes.Why
setupCrlServerWithCertshad a 5-attempt retry loop intended to handleEADDRINUSEfromserver.listen(), but the call tostartCrlServerwasreturned withoutawait. BecausestartCrlServerrejects viaserver.on('error', reject), that rejection escapes thetry/catchentirely — so the retry only ever guardedgenerateCrlCertificates, never the actual bind.Ports are chosen randomly from 50000–59999, which overlaps the Linux ephemeral range (32768–60999). A collision with a kernel-assigned port causes the whole suite to fail with
EADDRINUSEinstead of retrying. This surfaced as acrl-verification.test.tsfailure on the Node 22 shard of an unrelated PR (#1709); Node 24/26 shards happened to be clean (pure timing).Fix
Add
awaitbeforestartCrlServer(...). This makes the bind rejection land in thecatch, so the existing retry loop works as intended. The siblingsetupOcspResponderWithCertsalready doesconst server = await startOcspServer(...)inside itstry— this change bringssetupCrlServerWithCertsto the same pattern.Generated by Claude Sonnet 4.6.