fix: resolve cumulative update links from new Microsoft download pages by cheenamalhotra · Pull Request #295 · tediousjs/setup-sqlserver

cheenamalhotra · 2026-10-06T05:32:11Z

Fixes the cumulative update downloader for tediousjs/tedious#1807.

Microsoft download pages now put the installer URL in embedded JSON, which the old href regex didn't handle reliably.

Changes

Parse window.__DLCDetails__ JSON with JSON.parse (page scripts are never run), and fall back to legacy <a href> links.
Accept only HTTPS download.microsoft.com .exe URLs. Report an error if none or more than one is found.
Retry page fetches up to 3 times for network errors, timeouts, 408, 429 and 5xx.
If the update still can't be downloaded, log a warning with the reason (e.g. HTTP 403, no installer link) and install SQL Server without updates, as before.
Direct .exe update URLs (2016) are unchanged.

Testing

New tests cover the real JSON shape, legacy links, the combined layout, malformed/missing/multiple files, retries, fatal errors and the warn-and-continue path.
Live check: the 2017, 2019 and 2022 pages each resolve to one x64 installer.

Note: the linked tedious CI runs got HTTP 404/403 from the 2022 page. That page returns 200 now. If those errors happen again, the warning will now show the actual reason.

@arthurschreiber @saurabh500 @David-Engel

Microsoft download pages now put the installer URL in embedded JSON metadata. Parse that metadata (without running page scripts), fall back to legacy anchor links, and accept only HTTPS download.microsoft.com .exe URLs. Retry transient page-fetch failures. Fail the action when a requested update can't be resolved or downloaded instead of silently installing without updates. Refs tediousjs/tedious#1807 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Keep the previous non-fatal behaviour: log a warning with the failure reason and continue SQL Server setup without cumulative updates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟢 Approval recommended

The implementation matches the stated behavior and is supported by focused, comprehensive tests.

Review effort: Balanced
Findings: None

What changed in this PR

Updates cumulative-update resolution for Microsoft’s JSON-based download pages while preserving legacy links and warn-and-continue behavior.

Changes:

Securely parses and validates update installer URLs.
Retries transient page-fetch failures with backoff.
Adds comprehensive tests and documentation.

File	Description
`.github/copilot-instructions.md`	Documents the updated installation flow.
`README.md`	Explains retry and fallback behavior.
`lib/main/index.js`	Updates the generated action bundle.
`lib/main/index.js.map`	Updates the generated source map.
`src/install.ts`	Warns and continues when updates fail.
`src/utils.ts`	Adds URL extraction, validation, and retries.
`test/install.ts`	Tests warn-and-continue behavior.
`test/utils.ts`	Tests parsing, validation, errors, and retries.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

dhensby

Thanks for picking this up, @cheenamalhotra!

Parsing the __DLCDetails__ metadata (without executing anything) and limiting accepted URLs to HTTPS download.microsoft.com .exe links is good future-proofing. Surfacing the actual failure reason in the warning will also make these failures much easier to diagnose next time. I'm happy with warning and installing without updates rather than failing the action.

I've left a few comments inline. The main ones are falling back to the href scan when the metadata can't be used, and retrying 403/404 so we match what the README now says. The rest are nits.

Commits: the two commits currently contradict each other. 42f3e01 says the action will now fail when a requested update can't be resolved or downloaded, and 5a5a08f changes that back to warn-and-continue. We merge with merge commits, so both would land on master and both would show up in the changelog as bug fixes with opposite descriptions. Could you squash them into a single fix: commit whose message describes the final behaviour?

SQL Server 2025 updates: Microsoft now publishes cumulative updates for 2025 at https://www.microsoft.com/en-us/download/details.aspx?id=108788 (currently KB5122048), and the new metadata parsing resolves that page to a single x64 installer. That means the comment in src/versions.ts saying the updateUrl can be added once Microsoft publishes them is out of date. This PR doesn't need to cover it, but if you want to add the URL, a separate commit would be great. Otherwise we can pick it up in a follow-up.

Heads-up: this conflicts with #296 (README, the node:fs/promises import in src/utils.ts, the module mocks in test/utils.ts, and lib/). Whichever PR lands second will need a rebase and a fresh npm run build.

dhensby · 2026-10-06T10:16:45Z

+    const links = new Set<string>();
+    for (const [, script] of body.matchAll(/<script\b[^>]*>([\s\S]*?)<\/script\s*>/gi)) {
+        const assignment = script.match(/^\s*window\.__DLCDetails__\s*=\s*(\{[\s\S]*\})\s*;?\s*$/);
+        if (!assignment) continue;


nit: the rest of the codebase always uses braces for if bodies (also lines 257 and 262), so it'd be good to match that here. Similarly, const [link] = links; return link; on line 270 would avoid the non-null assertion.

dhensby · 2026-10-06T10:16:45Z

+        try {
+            details = JSON.parse(assignment[1]);
+        } catch (error) {
+            throw new Error('Invalid cumulative update metadata in Microsoft download page', { cause: error });
+        }
+        if (!isRecord(details) || !isRecord(details.dlcDetailsView)) {
+            throw new Error('Invalid cumulative update metadata in Microsoft download page');
+        }
+        const files = details.dlcDetailsView.downloadFile;
+        if (!Array.isArray(files)) {
+            throw new Error('Invalid cumulative update file list in Microsoft download page');
+        }


Could problems with the metadata fall back to the <a href> scan below instead of throwing? Today all three CU pages carry both the __DLCDetails__ JSON and a legacy download link. If Microsoft renames dlcDetailsView/downloadFile, or the JSON stops parsing, this would fail in cases where the old href regex would still have worked. We want the extra parser to make us more resilient, not less.

I'd suggest treating any parse or shape problem as "no usable metadata": log the reason with core.debug and carry on to the link scan. Only throw if neither path finds an installer, and include the metadata problem in that final error so it isn't lost. The rejects malformed metadata tests would then become "falls back to legacy links when metadata is malformed" cases, plus one where there's no link either.

dhensby · 2026-10-06T10:16:45Z

+    if (links.size !== 1) {
+        throw new Error(links.size
+            ? 'Multiple cumulative update installers found in Microsoft download page'
+            : 'No HTTPS download.microsoft.com .exe cumulative update installer found in Microsoft download page');
+    }


nit: the previous implementation called core.debug(body) when it couldn't find a link. That's handy the next time Microsoft changes the page, because you can re-run with debug logging and see exactly what we received. Could we keep that before throwing here?

Also, the metadata has a dlcDetailsView.error field (empty today). If Microsoft ever fills it in, including it in this error would tell us more than "no installer found".

dhensby · 2026-10-06T10:16:45Z

+        try {
+            const res = await fetch(url, { signal: AbortSignal.timeout(30_000) });
+            if (!res.ok) {
+                retryable = res.status === 408 || res.status === 429 || res.status >= 500;


The README now says transient download-page failures are retried, but the failures we've actually seen from these pages are 403 and 404 (the tedious runs mentioned in the description). Those turned out to be transient, since the page was back to 200 shortly afterwards.

updateUrl is a fixed, known-good details page rather than user input, so I think it's reasonable to retry any non-2xx here, or at least 403 and 404. If a page is ever genuinely retired, the worst case is a few seconds' delay before the warning. A slightly longer backoff than 1s/2s might also improve the odds of recovering. The fails explicitly without retrying HTTP ${status} tests would flip to asserting the retries.

dhensby · 2026-10-06T10:16:45Z

+            return await res.text();
+        } catch (error) {
+            retryable ||= error instanceof TypeError || (error instanceof Error && error.name === 'TimeoutError');
+            const reason = error instanceof Error ? error.message : String(error);


nit: for network failures, undici's message is just fetch failed. The useful detail (e.g. ECONNRESET, ENOTFOUND, UND_ERR_SOCKET) is on error.cause, and it gets dropped from the message when we wrap it here. Since the aim is to show the reason in the warning, it'd be worth appending the cause's message or code when there is one.

dhensby · 2026-10-06T10:16:45Z

+            if (!retryable || attempt === attempts) {
+                throw new Error(`Unable to fetch cumulative update page ${url} after ${attempt} attempt(s): ${reason}`, { cause: error });
+            }
+            core.warning(`Cumulative update page fetch failed (${reason}); retrying (${attempt + 1}/${attempts})`);


nit: core.warning adds an annotation to the run summary, so a fetch that succeeds on its second attempt still leaves a warning on an otherwise green job. Could we use core.info for the intermediate retries? The warning in install.ts still flags the case where we give up. (@actions/tool-cache logs its own download retries at info level too.)

dhensby · 2026-10-06T10:16:45Z

+                try {
+                    return await findOrDownloadUpdates(config);
+                } catch (error) {
+                    core.warning(`Unable to download cumulative updates; installing without updates. ${error}`);


nit: ${error} becomes Error: Unable to …, so the warning reads …installing without updates. Error: Unable to fetch…. error instanceof Error ? error.message : error would read a bit more cleanly. The test regex would need updating to match.

dhensby · 2026-10-06T10:16:45Z

+When `install-updates: true` is set for a version with a configured update URL,
+the action downloads the update before starting SQL Server setup. Transient
+download-page failures are tried up to three times. If the update still can't be
+downloaded, the action logs a warning with the reason and installs SQL Server
+without updates. Versions without a configured update URL skip updates.


nit: this paragraph sits between the generated usage block and ### Basic usage without a heading. A ### Cumulative updates heading would make it easier to find and link to. (On "tried up to three times", see my comment on the retry status codes in src/utils.ts.)

cheenamalhotra and others added 2 commits October 5, 2026 22:31

fix: warn and install without updates when update download fails

5a5a08f

Keep the previous non-fatal behaviour: log a warning with the failure reason and continue SQL Server setup without cumulative updates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

cheenamalhotra mentioned this pull request Oct 6, 2026

Random RPC stream error with Strict encryption / TDS 8.0 tediousjs/tedious#1807

Open

dhensby requested a balanced review from Copilot October 6, 2026 09:43

Copilot started reviewing on behalf of dhensby October 6, 2026 09:44 View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

dhensby requested changes Oct 6, 2026

View reviewed changes

dhensby mentioned this pull request Oct 6, 2026

fix: harden SQL Server 2025 installer downloads #296

Open

Navigation Menu

Repository navigation

fix: resolve cumulative update links from new Microsoft download pages - #295

fix: resolve cumulative update links from new Microsoft download pages#295
cheenamalhotra wants to merge 2 commits into
tediousjs:masterfrom
cheenamalhotra:fix/cumulative-update-download

cheenamalhotra commented Oct 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

dhensby left a comment

Uh oh!

dhensby Oct 6, 2026

Uh oh!

dhensby Oct 6, 2026

Uh oh!

dhensby Oct 6, 2026

Uh oh!

dhensby Oct 6, 2026

Uh oh!

dhensby Oct 6, 2026

Uh oh!

dhensby Oct 6, 2026

Uh oh!

dhensby Oct 6, 2026

Uh oh!

dhensby Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cheenamalhotra commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

dhensby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cheenamalhotra commented Oct 6, 2026 •

edited

Loading