Conversation
…#33278) process_results() has always had an "interleave ipv4 and ipv6" pass, but it was a faithful port of a broken Zig loop (`for ... else` fired unconditionally), so any resolver result starting with the preferred family — e.g. 4 AAAA records before any A — came out untouched. All 4 concurrent connect attempts then went to the same dead family, and a cold fetch on a network with advertised-but-unroutable IPv6 stalled for the full ~70 s OS connect timeout. Replace it with a stable interleave applied while packing the entries: alternate between the first entry's family and the rest, preserving resolver order within each family. The old post-hoc swap block is deleted; ordering now happens in the copy loop via a closed-form slot mapping (a bijection, so the MaybeUninit init invariant is unchanged). Expose the packing through bun:internal-for-testing to make the ordering testable; three of the new cases fail against the old algorithm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds RFC 8305 address-family interleaving to DNS ChangesRFC 8305 getaddrinfo interleaving
Sequence Diagram(s)sequenceDiagram
participant Test as dns-prefetch.test.ts
participant JS as dnsInternals.getaddrinfoInterleave
participant Bridge as testing_apis.getaddrinfo_interleave
participant Core as internal::process_results
Test->>JS: getaddrinfoInterleave(families)
JS->>Bridge: invoke Rust function
Bridge->>Bridge: build synthetic AddrInfo chain
Bridge->>Core: process_results(chain)
Core-->>Bridge: interleaved ResultEntry list
Bridge-->>JS: encoded order array
JS-->>Test: interleaved family order
Related issues: Suggested labels: dns, rust, testing Suggested reviewers: (none identified from provided context) 🐇 A hare hops through addresses four and six, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
### Problem - GitHub closes only the first reference after a keyword, so "Fixes #1, #2" leaves #2 open. "Supersedes #3" links nothing, and no reference closes a pull request. - The last 1000 merged PRs name 274 such references. PR #32292 is open although merged #36135 says "Supersedes #32292". ### Fix - `.github/workflows/close-linked-issues.yml` runs on `pull_request_target` `closed` (a merge into the default branch of `oven-sh/bun`) and on `workflow_dispatch` with a PR number and `dry_run`. Everything is inline in one `actions/github-script` step, with no checkout. - Each open target is closed as `completed` with the comment "Closed as completed by #N." or "Superseded by #N.". Closed or missing targets, the PR itself and other repositories are skipped. - The parser has no regex. A closing keyword (close, fix, resolve, supersede, replace, any tense) must lead the reference, alone or in a list. A negated, hedged or noun keyword, or one whose subject is another reference, does not count ("may fix", "the rm fix #1", "#100 supersedes #1"). - Verified: `test/internal/close-linked-issues.test.ts` (333 cases) runs the YAML's script against fake `github`, `context` and `core`. Also the 1000-PR parse (Notes). ### Background - GitHub's own keywords are close, fix and resolve (-s, -ed). Each links one reference, and only a merge into the default branch closes it. - `pull_request_target` runs in the base repository with a write token, also for fork PRs. That is safe only when no PR-controlled code runs. Here the description is the only PR input, parsed as text. <details><summary>Notes</summary> A close through the API does not create the "closed this in #N" timeline link that GitHub makes for its own closes. The comment carries the PR number instead. How the parser was calibrated. I pulled the descriptions of the last 1000 merged PRs and listed every line with a keyword next to a reference. The keyword families, list shapes and reference forms in the script are the ones that appear there. A reference is `#1`, `owner/repo#1`, an issue or pull URL (bare or in `<>`), or a markdown link. Four lines would have been wrong with a plain keyword-then-reference rule, and each led to a rule: - "the open `rm` fix #37521" (#38379): "fix" as a noun. Base forms (fix, close, resolve, supersede, replace) count only at the start of a sentence or line, or after will, should, does, and, and a few similar words. "to" is not one of them ("unable to fix #1", "how to fix #1"). - "May also fix #12318 / #10046, untested" (#38242): hedged. may, might, could, would, partially and the negations disqualify the keyword, looking past adverbs such as "also". - "Supersedes the closed #26040" (#36289) and "a comment on closed #35351" (#35365): "closed" as an adjective. A determiner or preposition before the keyword disqualifies it. - "supersedes #33130's optimisation" (#35843): a number that continues into a word is not a reference. Review added: a reference before the keyword is the subject ("#100 supersedes #1"), also through "which" or "that" ("reverts #100, which fixed #1") and across a removed span ("#100 ~~also~~ fixes #1"). A hedge two words before the keyword disqualifies it ("hopefully this fixes #1", "could this fix #1?"). A clause that starts with if, when, once, until or unless is not a statement. The tokenizer keeps a line break as a token so that "Fixes #1" on one line and "Fixes #2" on the next stay two statements. Code spans, fences, indented code, blockquotes, HTML comments and strikethrough are skipped. The block stripping follows CommonMark for fences (also inside a blockquote), indented code, blockquotes with lazy continuation, setext underlines and HTML comments, and GFM for `~~` flanking. Result over the 1000 descriptions: 274 distinct references in 135 PRs. I checked the current state of all of them through GraphQL. All but one are closed (202 issues completed, 5 duplicates, 66 pull requests). The one open target is PR #32292, superseded by merged #36135. No open target is a false positive. Every review change kept this result. Patterns that are deliberately not handled: a bulleted list under "Closes:" on its own line (not seen in the sample), references separated by whitespace only ("#1 #2"), "fix for #1", and GH-1 style references. A `?` after the list is not treated as a question. The block parser tracks no list containers, so a second paragraph of a list item indented by four spaces is read as an indented code block and skipped. A removed span or inline comment reads as one word, so "Fixes <!-- n --> #1" finds nothing. The test suite covers: the phrases above, stopping at the right place in real sentences, CRLF descriptions, URLs with fragments or a `/files` suffix, case-insensitive `Owner/Repo#1`, the fake API where a lookup, an update or a comment fails, the `dry_run` input, an invalid `pr_number` input, an unmerged PR, a PR merged into a non-default branch, the merge event body against a later edit, and a description with no closing statement. The first revision of this PR checked out the repository and ran `scripts/close-linked-issues.ts`. Jarred asked for no checkout and no script file, so the script moved inline into the workflow and the test now reads it out of the YAML. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 9 · 2 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/internal/close-linked-issues.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/internal/close-linked-issues.test.ts bun test v1.4.1 (4448a2e) test/internal/close-linked-issues.test.ts: (pass) finds "Fixes #39852" [176.21ms] (pass) finds "Closes #31772. Fixes #31771." [22.28ms] (pass) finds "- Fixes #39930" [12.28ms] (pass) finds "Fixes: #30429" [10.46ms] (pass) finds "FIXES #1" [7.86ms] (pass) finds "(Fixes #1)" [8.97ms] (pass) finds "**Fixes #1**" [10.20ms] (pass) finds "__Fixes #1__" [9.83ms] (pass) finds "_Fixes #1_" [11.25ms] (pass) finds "Fixes **#1**" [9.72ms] (pass) finds "**Fixes** #1" [7.13ms] (pass) finds "**Fixes:** #1" [8.11ms] (pass) finds "Fixes #1 and **#2**" [11.47ms] (pass) finds "Fixes **#1**, **#2**" [9.13ms] (pass) finds "## Why (fixes #13771, closes #30543)" [16.08ms] (pass) finds "Closes #11418" [19.46ms] (pass) finds "Resolves #1. Resolved #2. Resolve #3." [12.09ms] (pass) finds "Fixes #34055, #30327, #24394, #20816, #32403, #11898, #10056." [17.11ms] (pass) finds "Fixes #18192 and #31675 as a consequence" [10.45ms] (pass) finds "Fixes #1, #2, and #3" [10.96ms] (pass) finds "Fixes #1 & #2" [7.63ms] (pass) finds "Closes #33280, Closes #32864 and Closes #29696 (the timer in #32949 is orthogonal)" [20.29ms] (pass) finds "Closes #33182 and #32947 on top of current main (which already has #36304 for catalogs)." [16.12ms] (pass) finds "Fixes #1,\n#2" [7.76ms] (pass) finds "Fixes #1, #2,\nand #3" [9.27ms] (pass) finds "Fixes #1\nand #2" [8.57ms] (pass) finds "Fixes #1\n& #2" [6.80ms] (pass) finds "Fixes #1 and\n#2" [7.31ms] (pass) finds "Supersedes #39908 (same change, moved from a fork branch)" [13.21ms] (pass) finds "Supersedes #38778 and #38391. Carries the entry point arm of #35053." [14.43ms] (pass) finds "Supersedes #39193 and keeps its three tests." [11.48ms] (pass) finds "This supersedes #33306 and #32803. Their tests are kept here." [13.73ms] (pass) finds "- This replaces #33793. Its ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` .github/workflows/close-linked-issues.yml | 950 ++++++++++++++++++++++++++++++ test/internal/close-linked-issues.test.ts | 598 +++++++++++++++++++ 2 files changed, 1548 insertions(+) ``` </details> **gate history** · 29 passed · 0 rejected · iteration 9 <details><summary>evidence per changed file</summary> ``` file reads edits tests .github/workflows/close-linked-issues.yml 6 12 0 test/internal/close-linked-issues.test.ts 3 11 0 ``` </details> <!-- robobun:evidence:end -->
(Disclosure: Code and writeup by Claude Fable 5)
Fixes #33278.
A cold
fetch()(or any usockets connect) on a network with advertised-but-unroutable IPv6 stalls for the full OS connect timeout (~70 s) when the host resolves to several AAAA records before any A record: allCONCURRENT_CONNECTIONS = 4parallel attempts go to the first four addresses in resolver order — all dead IPv6 — and blackholed SYNs never fail, so the IPv4 addresses are never tried.Root cause
process_results()insrc/runtime/dns_jsc/dns.rs— the function that packs getaddrinfo results for the usockets/QUIC connect cache — has always had a// sort (interleave ipv4 and ipv6)pass, but it never worked:for … else { break }fired unconditionally (the port note said so explicitly), so the outer loop dies after a single index.wantstarts atAF_INET6and only toggles when a swap happens, so any list starting with IPv6 — the exact failing shape — justcontinues and returns completely untouched.Fix
Ordering now happens while the entries are copied into the packed allocation: alternate between the first entry's family and the rest, preserving resolver order within each family (RFC 8305 §4). Starting from the resolver's first family (rather than hardcoding IPv6) respects the OS's RFC 6724 policy, so v4-first results stay v4-first. The slot mapping is a closed-form bijection over
0..count, so theMaybeUninit/assume_initinvariant is unchanged; the old post-hoc swap block is deleted.With interleaving in place, the first batch of 4 attempts covers both families, so a run of dead same-family addresses can no longer monopolize it. This is deliberately scoped to the existing (broken) mechanism; making
dns.setDefaultResultOrdervisible to the native path is a separate concern tracked in #28817.Test
The packing is exposed through
bun:internal-for-testing(dnsInternals.getaddrinfoInterleave), following thesocket_body.rs/linear_fifo_testing.rspattern: the probe builds a synthetic getaddrinfo chain from a list of families, runs the realprocess_results, and walks the rebuiltai_nextchain (original index stashed in the port so stability is observable). 8 cases intest/js/bun/dns/dns-prefetch.test.tscover the issue's exact shape, v4-first, uneven counts, single-family, and empty inputs.Verified the test fails for the right reason: with the copy loop reverted to sequential order, the three mixed-family cases fail (the others are fixed points of interleaving by design). The slot formula was additionally checked exhaustively for every family sequence up to length 8 (bijection, alternating prefix, per-family stability).
Honest caveat on the end-to-end repro: the network I filed the issue from has since changed — IPv6 now fails in ~2 ms (unreachable) instead of blackholing, and fast-failing attempts always recovered via the refill-on-error path — so I could not re-measure the 70 s → sub-second improvement live. Deleting the fix and re-running the new tests is the deterministic evidence.