Skip to content

fix: remove channel filter in e-rcv - #911

Merged
MarshallOfSound merged 1 commit into
mainfrom
fix/remove-channel-filter-e-rcv
Oct 10, 2026
Merged

MarshallOfSound merged 1 commit into
mainfrom
fix/remove-channel-filter-e-rcv

Conversation

@dsanders11

Copy link
Copy Markdown
Member

Doesn't add value (there's always way under 1,000 release versions returned). and can prevent using e rcv with certain Chromium values

@dsanders11
dsanders11 requested review from a team and ckerr as code owners October 8, 2026 22:34

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked whether dropping the Canary-only filter (while keeping num=1000) could cause silent truncation now that all channels are merged into the response — a single Chromium milestone has nowhere near 1000 release entries across Stable/Beta/Dev/Extended/Canary combined, so the existing cap isn't a practical risk here.

Extended reasoning...

Single-line change in src/e-rcv.ts removing the channel=Canary query parameter from a chromiumdash.appspot.com fetch_releases call, so the request now returns releases across all channels for the milestone instead of just Canary; no auth/crypto/permissions surface is touched. A CONFIRMED pre-existing finding (missing resp.ok check before resp.json(), unlike the sibling fetchChromiumDashCommit function) is already posted inline, which is reason enough to want human eyes rather than auto-approve, and I additionally checked the num=1000 cap for truncation risk and ruled it out given realistic per-milestone release counts.

Comment thread src/e-rcv.ts
Comment on lines 172 to 174
const releases = (await fetch(
`https://chromiumdash.appspot.com/fetch_releases?channel=Canary&platform=Linux,Mac,Win32,Windows&milestone=${milestone}&num=1000`,
`https://chromiumdash.appspot.com/fetch_releases?platform=Linux,Mac,Win32,Windows&milestone=${milestone}&num=1000`,
).then((resp) => resp.json())) as Array<{ version: string }>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: Pre-existing, but now exercised with a much bigger, less-targeted query: the fetch_releases call at src/e-rcv.ts:172-174 never checks resp.ok before resp.json(), unlike fetchChromiumDashCommit at line 110 which does and calls fatal() on failure. Removing channel=Canary makes this call pull every channel for the milestone instead of one, increasing payload size/latency against the same unguarded call. If chromiumdash returns a non-200 or non-JSON body, resp.json() throws and the CLI crashes with an unhandled rejection instead of the clean fatal() message every other chromiumdash call produces. Fix: check resp.ok (and handle JSON parse failure) before resp.json() here, consistent with fetchChromiumDashCommit.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Trigger: fetch_releases at src/e-rcv.ts:172-174 is called once per milestone in e rcv; after this diff it omits channel=Canary, so it now requests Linux/Mac/Win32/Windows releases across all channels (Stable/Beta/Dev/Extended/Canary) instead of just Canary, enlarging the response per call. The code does .then((resp) => resp.json()) with no resp.ok check, unlike fetchChromiumDashCommit (line 110) which checks resp.ok and calls fatal(). If chromiumdash errors (5xx, rate-limit, timeout) on this heavier query, resp.json() throws on a non-JSON body, producing an unhandled promise rejection that crashes the process instead of the fatal() UX used elsewhere in the same file for the same endpoint family. Nothing in the diff adds this check even though it touches exactly this call.

Verification: pre-existing. The fetch at src/e-rcv.ts:172-174 does .then((resp) => resp.json()) with no resp.ok check, unlike fetchChromiumDashCommit which guards at lines 110-112. On a chromiumdash error, resp.json() rejects on non-JSON, or returns a non-array so releases.map at line 175 throws. The missing-guard route is byte-identical on base and HEAD; removing channel=Canary only widens a response still capped by num=1000, so merging makes nothing worse here.

@MarshallOfSound
MarshallOfSound enabled auto-merge (squash) October 10, 2026 10:58
@MarshallOfSound
MarshallOfSound merged commit 63230ab into main Oct 10, 2026
14 checks passed
@MarshallOfSound
MarshallOfSound deleted the fix/remove-channel-filter-e-rcv branch October 10, 2026 10:59
claude Bot pushed a commit to electron/electron that referenced this pull request Oct 15, 2026
Bump the pinned @electron/build-tools SHA from 32e2253f to 3bf7c9f5 to
pick up electron/build-tools#912, which raises siso's remote timeout for
Windows clang-cl compiles to 10m (matching macOS) so heavy compiles
finish remotely and get cached instead of falling back to local on every
warm build. Also picks up electron/build-tools#911 (e-rcv only).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GkpQ9nmAGxxVpwT8wPzfbC
jkleinsc pushed a commit to electron/electron that referenced this pull request Oct 15, 2026
…4958)

Bump the pinned @electron/build-tools SHA from 32e2253f to 3bf7c9f5 to
pick up electron/build-tools#912, which raises siso's remote timeout for
Windows clang-cl compiles to 10m (matching macOS) so heavy compiles
finish remotely and get cached instead of falling back to local on every
warm build. Also picks up electron/build-tools#911 (e-rcv only).


Claude-Session: https://claude.ai/code/session_01GkpQ9nmAGxxVpwT8wPzfbC

Co-authored-by: Claude <noreply@anthropic.com>
MarshallOfSound pushed a commit to electron/electron that referenced this pull request Oct 15, 2026
…5026)

Bump the pinned @electron/build-tools SHA from 5a3a3bf9 to 3bf7c9f5 to
pick up electron/build-tools#912, which raises siso's remote timeout for
Windows clang-cl compiles to 10m (matching macOS) so heavy compiles
finish remotely and get cached instead of falling back to local on every
warm build. Also picks up electron/build-tools#911 (e-rcv only).

Manual backport of #54958.


Claude-Session: https://claude.ai/code/session_01GkpQ9nmAGxxVpwT8wPzfbC

Co-authored-by: Claude <noreply@anthropic.com>
MarshallOfSound pushed a commit to electron/electron that referenced this pull request Oct 15, 2026
…5025)

Bump the pinned @electron/build-tools SHA from 5a3a3bf9 to 3bf7c9f5 to
pick up electron/build-tools#912, which raises siso's remote timeout for
Windows clang-cl compiles to 10m (matching macOS) so heavy compiles
finish remotely and get cached instead of falling back to local on every
warm build. Also picks up electron/build-tools#911 (e-rcv only).

Manual backport of #54958.


Claude-Session: https://claude.ai/code/session_01GkpQ9nmAGxxVpwT8wPzfbC

Co-authored-by: Claude <noreply@anthropic.com>
MarshallOfSound pushed a commit to electron/electron that referenced this pull request Oct 15, 2026
…5027)

Bump the pinned @electron/build-tools SHA from 5a3a3bf9 to 3bf7c9f5 to
pick up electron/build-tools#912, which raises siso's remote timeout for
Windows clang-cl compiles to 10m (matching macOS) so heavy compiles
finish remotely and get cached instead of falling back to local on every
warm build. Also picks up electron/build-tools#911 (e-rcv only).

Manual backport of #54958.


Claude-Session: https://claude.ai/code/session_01GkpQ9nmAGxxVpwT8wPzfbC

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants