Fix flaky socket-readable test on loopback - #570
Queued
npapagna wants to merge 3 commits into
Queued
Conversation
The "reports a socket readable when data is actually waiting on it" test asserted isReadable() immediately after the server-side write resolved, assuming the bytes had already landed in the client's kernel receive buffer at that point. That's not guaranteed: the write callback only confirms the server flushed the send, not that loopback delivery to the client's socket completed, and under CI load the two can be a beat apart. Poll for readiness with vi.waitFor instead of asserting once. This matches the "positive assertion, eventually true" pattern used elsewhere in the client test suite, and keeps isReadable() itself unchanged: it's meant to be a true non-blocking, single-shot check (poll with a 0 timeout), so retry logic belongs in the test, not in the thing under test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
npapagna
force-pushed
the
npapagna/fix-flaky-socket-readable-test
branch
from
September 7, 2026 20:13
76c6e6a to
c283352
Compare
MatiasFernandez
approved these changes
Sep 7, 2026
Address review feedback on #570: the "resolving doesn't mean the client side has seen it" caveat only covered writeFromServer, but resetFromServer has the identical gap (already noted, but buried inside the POSIX-gated reset test's own comment as the reason that assertion is Windows-only). Folding it into this doc comment makes it the one place both caveats live, instead of leaving the second one stranded in an unrelated test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Windows-only "reports the socket as unusable when it has been reset by its peer" test carried a comment claiming the same assertion (isReadable throwing) would also hold on POSIX, just too flaky to test given RST delivery timing. Verified live via poll() on macOS: that's not accurate. POSIX poll() leaves POLLIN set alongside POLLHUP/POLLERR for a reset connection, so isReadable() reports it readable there instead of throwing, unlike WSAPoll, which clears POLLRDNORM. That's a platform semantic difference, not a timing race, so no POSIX equivalent of that test existed at all before now. Corrected the comment and added the POSIX case, polling with vi.waitFor since the RST still needs a moment to reach and be processed by the client fd's kernel state, the same timing gap this PR is already about. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
npapagna
added this pull request to the merge queue
Sep 7, 2026
Any commits made after this event will not be merged.
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
2 participants
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.
Summary
nativeSocketLibrary.test.tsasserted readiness immediately after the server-side write resolved, assuming loopback delivery to the client's kernel receive buffer was instantaneous. Under CI load that's not guaranteed, causing an intermittent failure (seen in this run).vi.waitFor, matching the existing "positive assertion, eventually true" pattern used elsewhere in the client suite, instead of adding retry logic toisReadable()itself, which needs to stay a true non-blocking, single-shot poll for its future callers.loopbackConnection.tsdoc comment that had claimed the client side always "already seen it" by the timewriteFromServerresolves.Test plan
npx vitest run src/sockets/__tests__/nativeSocketLibrary.test.ts(client workspace)npm run test:client(full client suite, 459 test files passed)npm run lint && npm run format:check && npm run compile🤖 Generated with Claude Code