-
Notifications
You must be signed in to change notification settings - Fork 91
fix(install): resilient latest-version fetch (both installers) #946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/windows-powershell-installer
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -112,16 +112,22 @@ function Test-Avx2 { | |||||||||||||
| # --------------------------------------------------------------------------- | ||||||||||||||
| if ([string]::IsNullOrWhiteSpace($Version)) { | ||||||||||||||
| $useLatest = $true | ||||||||||||||
| try { | ||||||||||||||
| $rel = Invoke-RestMethod -Uri "https://api.github.com/repos/AltimateAI/altimate-code/releases/latest" -Headers @{ "User-Agent" = "altimate-install" } | ||||||||||||||
| $specificVersion = ($rel.tag_name -replace '^v', '') | ||||||||||||||
| } catch { | ||||||||||||||
| Write-Err "Failed to fetch version information" | ||||||||||||||
| exit 1 | ||||||||||||||
| # The download below resolves "latest" server-side (releases/latest/download), | ||||||||||||||
| # so this API call only feeds the version-string display and the | ||||||||||||||
| # already-installed short-circuit. A transient api.github.com blip or the | ||||||||||||||
| # unauthenticated rate limit (60/hr/IP) must NOT abort the install — retry a | ||||||||||||||
| # few times, then proceed without the version string. | ||||||||||||||
| $specificVersion = "" | ||||||||||||||
| for ($attempt = 1; $attempt -le 3; $attempt++) { | ||||||||||||||
| try { | ||||||||||||||
| $rel = Invoke-RestMethod -Uri "https://api.github.com/repos/AltimateAI/altimate-code/releases/latest" -Headers @{ "User-Agent" = "altimate-install" } | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth considering — neither retry sets an explicit HTTP timeout.
A short timeout here keeps the worst case bounded:
Suggested change
Same gap on the bash side at install:216 if you want to keep parity — |
||||||||||||||
| $specificVersion = ($rel.tag_name -replace '^v', '') | ||||||||||||||
| if (-not [string]::IsNullOrWhiteSpace($specificVersion)) { break } | ||||||||||||||
| } catch {} | ||||||||||||||
| if ($attempt -lt 3) { Start-Sleep -Seconds $attempt } | ||||||||||||||
| } | ||||||||||||||
| if ([string]::IsNullOrWhiteSpace($specificVersion)) { | ||||||||||||||
| Write-Err "Failed to fetch version information" | ||||||||||||||
| exit 1 | ||||||||||||||
| Write-Muted "Could not resolve the latest version from GitHub (API unavailable) — installing the latest release anyway." | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [🟠 MEDIUM] By changing the API failure behavior from However, this introduces a subtle bug further down in the script when it checks if the app is already installed: try { $installedVersion = (& $probe --version 2>$null | Select-Object -First 1).ToString().Trim() } catch {}
if ($installedVersion -eq $specificVersion) {
Write-Muted "Version $specificVersion already installed"
exit 0
}If the GitHub API fails ( To avoid this, you can explicitly reset Suggested change:
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A subtle interaction worth guarding against on the PS side: if this branch fires (API blip → For reference, the equivalent bash check at install:270 has the matching
Suggested change
|
||||||||||||||
| } | ||||||||||||||
| } else { | ||||||||||||||
| $useLatest = $false | ||||||||||||||
|
|
@@ -177,7 +183,7 @@ function Install-Target { | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| Write-Host "" | ||||||||||||||
| Write-Host "Installing $App version: $specificVersion" | ||||||||||||||
| Write-Host "Installing $App version: $(if ($specificVersion) { $specificVersion } else { 'latest' })" | ||||||||||||||
|
|
||||||||||||||
| $tmpDir = Join-Path ([System.IO.Path]::GetTempPath()) "altimate_install_$PID" | ||||||||||||||
| New-Item -ItemType Directory -Force -Path $tmpDir | Out-Null | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /** | ||
| * Latest-version resolution must be resilient, in BOTH installers. | ||
| * | ||
| * The `latest` install path hits api.github.com/.../releases/latest only for the | ||
| * version-string display + the already-installed short-circuit — the download | ||
| * itself uses releases/latest/download/<file> (server-side latest). A transient | ||
| * 504 or the 60/hr/IP unauthenticated rate limit must NOT abort the install: | ||
| * retry a few times, then degrade gracefully and install latest anyway. | ||
| */ | ||
| import { describe, test, expect } from "bun:test" | ||
| import { readFileSync } from "node:fs" | ||
| import { join } from "node:path" | ||
|
|
||
| const REPO_ROOT = join(import.meta.dir, "../../../..") | ||
| const BASH = readFileSync(join(REPO_ROOT, "install"), "utf-8") | ||
| const PS1 = readFileSync(join(REPO_ROOT, "install.ps1"), "utf-8") | ||
|
|
||
| describe("bash installer — latest-version fetch is non-fatal", () => { | ||
| test("retries the releases/latest API call", () => { | ||
| expect(BASH).toContain("for attempt in 1 2 3") | ||
| // --fail so a 504 errors out (and retries) instead of parsing an error body. | ||
| expect(BASH).toContain("curl -fsSL https://api.github.com") | ||
| }) | ||
|
|
||
| test("degrades gracefully instead of exiting on API failure", () => { | ||
| expect(BASH).toContain("installing the latest release anyway") | ||
| // The old fatal hard-fail must be gone from the latest path. | ||
| expect(BASH).not.toContain("Failed to fetch version information") | ||
| }) | ||
|
|
||
| test("only short-circuits as already-installed on a real version match", () => { | ||
| expect(BASH).toContain('[ -n "$specific_version" ] && [[ "$installed_version" == "$specific_version" ]]') | ||
| }) | ||
| }) | ||
|
|
||
| describe("PowerShell installer — latest-version fetch is non-fatal", () => { | ||
| test("retries the releases/latest API call", () => { | ||
| expect(PS1).toContain("for ($attempt = 1; $attempt -le 3; $attempt++)") | ||
| }) | ||
|
|
||
| test("degrades gracefully instead of exiting on API failure", () => { | ||
| expect(PS1).toContain("installing the latest release anyway") | ||
| // The old fatal hard-fail must be gone. | ||
| expect(PS1).not.toContain("Failed to fetch version information") | ||
| }) | ||
| }) |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One subtle interaction to flag here — under the script's
set -euo pipefail(line 2), the retry loop may not iterate the way it reads.When
curl --failis paired withpipefail+set -e, a non-zero curl exit (e.g. a real 504 or rate-limit 403) propagates through the pipeline → the command substitution → the assignment, andset -eexits the script before[ -n "$specific_version" ] && breakis evaluated. The loop iterates at most once and the muted "installing latest anyway" notice doesn't get a chance to print.I ran a quick repro against a local 504-serving stub using these exact lines and the script aborted at attempt 1 with curl's exit 22, never reaching the degrade path.
A one-line fix that lets the inner pipeline failure be absorbed cleanly:
With
|| trueappended, all three attempts iterate and the degrade banner prints on persistent failure.One related note for a possible follow-up:
packages/opencode/test/install/version-fetch-resilience.test.tssubstring-matches the script source, soexpect(BASH).toContain("for attempt in 1 2 3")passes regardless of whether the loop body actually runs. A small behavioural test that runsinstallagainst a stubbed 504 server and asserts the degrade banner appears would catch this kind of regression in CI.