diff --git a/.github/instructions/examples.instructions.md b/.github/instructions/examples.instructions.md index 06b3b45..078cfd8 100644 --- a/.github/instructions/examples.instructions.md +++ b/.github/instructions/examples.instructions.md @@ -14,7 +14,7 @@ against a CHR, showing both the **CLI and the library API**. |---|---|---| | `.ts` | **Primary** — runnable Bun script, library API. `#!/usr/bin/env bun`; `if (import.meta.main) main()`; `try…finally` teardown; `process.exitCode = 1` on failure. | always (except `grounding`) | | `.sh` | CLI version — POSIX `sh`, sources `../common.sh`. | most | -| `.ps1` | PowerShell CLI mirror, dot-sources `../common.ps1`. | **all new** examples; existing where the CLI flow is simple | +| `.ps1` | PowerShell CLI mirror. Sets `$ErrorActionPreference = 'Stop'` **before** dot-sourcing `../common.ps1` (see "Exit 0 is not evidence"). | **all new** examples; existing where the CLI flow is simple | | `.py` | Python CLI driver, run with `uv run` (stdlib only). | where a non-TS audience adds value | | `.test.ts` | `bun:test` — only when assertions ARE the documentation. | `grounding` only | | `README.md` | from `_template/README.md`. | always | @@ -50,6 +50,47 @@ in [`COVERAGE.md`](../../examples/COVERAGE.md) (mark docs/test-only with a reaso run in extended verification, across the supported-OS matrix. `trial-license` is manual-only (rate limits). +## Exit 0 is not evidence ([#102](https://github.com/tikoci/quickchr/issues/102)) + +An example that exits 0 and prints *something* is not a passing example. Two +instances shipped green for months: + +- A `ParserError` in `common.ps1` took out the whole file, so every helper in + `quickstart.ps1` was undefined. No quickchr command ran; the script printed one + line and exited 0. +- `mndp.py`'s "no announcement received" branch used a bare `return`, which leaves + `main()` through the `finally` and never reaches its `sys.exit(rc)` — so the + failure path exited 0. + +Three rules, each holding one end of that: + +- **The smoke harness asserts output markers**, not just `code === 0` and + `out.length > 0`. Each entry in `examples-smoke.test.ts`'s `RUNNABLE` names + substrings only a working run produces — including one from the END of the script + and one the CLI/library emitted rather than the example's own `echo`. +- **`.ps1` sets `$ErrorActionPreference = 'Stop'` before dot-sourcing `common.ps1`**, + duplicating what `common.ps1` sets. Load-bearing, not stylistic: if `common.ps1` + fails to load, its own preference never takes effect and each undefined helper is + a non-terminating `CommandNotFound`. Reproduced 2026-08-03 (pwsh 7.4.6, Intel + macOS) against the real files with #102's defect reinstated: rc=0 without the + guard, rc=1 with it. Enforced by `scripts/validate-examples.ts`. +- **A failure path must exit non-zero.** In Python `raise SystemExit(msg)`, never a + bare `return` past a trailing `sys.exit(rc)`. Same run: rc=0 before, rc=1 after. +- **A `Start-Job` block sets both preferences itself.** `Start-Job` runs in a + separate process and inherits no preference variables, so the caller's and + `common.ps1`'s copies do not reach inside it. Without + `$ErrorActionPreference = 'Stop'` **and** + `$PSNativeCommandUseErrorActionPreference = $true` in the block, a `quickchr` call + that exits non-zero *without writing to stderr* is invisible: `Receive-Job` + returns, the script continues, the example exits 0 having booted nothing. Measured + (pwsh 7.4.6): a silent `exit 3` in a job gives **rc=0** unset, **rc=1** with both + set. A job whose command *does* write to stderr happens to propagate — do not rely + on that, it is the stderr-to-error-record mapping, not the exit code. + +`lint-powershell.yml` parses every `.ps1` with PowerShell's own parser before +PSScriptAnalyzer, because a `Severity = @('Error','Warning')` filter does not +surface `ParserError` — a file that cannot be parsed at all passed that gate. + ## A failing example is a quickchr bug until proven otherwise Examples are **canaries**, not chores. The reason each one boots a real CHR is to catch diff --git a/CHANGELOG.md b/CHANGELOG.md index 39d24c8..9a315b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,45 @@ Even minor versions (0.2.x, 0.4.x) are releases; odd minors (0.3.x, 0.5.x) are p ### Fixed +- **One refused connection to `upgrade.mikrotik.com` failed a whole `quickchr start`** + (#121). `resolveVersion()` had no retry at all: `fetchResilient()` is one attempt per + transport (system resolver, then a public-DNS IPv4 fallback), and if the fallback also + threw, the error propagated. #116/#119 gave the *download* path a retry policy and left + version resolution — which sits on the critical path of `start()`, the wizard and the + CLI — with a single shot. Observed on a `platforms=all` run: `Integration + (windows/x86_64 · testing)` went red on one `ConnectionRefused`, on a leg where nothing + about RouterOS, QEMU or the platform was involved. `resolveVersion()` now retries a + connection-class failure up to 3 times with the same backoff shape as `downloadToFile`, + and reports the attempt count plus the host when it exhausts them. An HTTP status is + still terminal on the first attempt — a 404 for a bad channel stays fast. +- **The version-resolution error named an IP address nobody configured** (#121). + `fetchResilient()` discarded the direct attempt's error whenever the IPv4 fallback + itself threw, so the surfaced message read `path: "https://159.148.147.251/routeros/…"` + with no hostname — leaving "the system resolver was broken", "the host refused us" and + "public DNS handed back a bad address" indistinguishable, which is exactly the question + the log has to answer. That fallback fires routinely on hosted runners (their stub + resolver fails slowly, 2–26 s), so its error was often the only one visible. Both + failures are now raised together as a `ResilientFetchError` naming the URL, each + transport's error, the address the fallback used and which resolvers produced it, with + the direct failure kept as `cause`. +- **Every PowerShell example was a silent false green** (#102). A `ParserError` in + `examples/common.ps1` took out the entire file — PowerShell parses `$LASTEXITCODE:` as + a scope-qualified variable reference — so every helper it defines was undefined for the + caller and `quickstart.ps1` ran no quickchr command at all. It exited 0 having printed + one line, and the smoke harness (`code === 0` and `out.length > 0`) reported `(pass)` + for months. The parse itself was fixed in #101; the false-green *class* is closed here: + the smoke harness now asserts per-example output markers — substrings only a working + run produces, including one from the end of the script and one the CLI itself emitted — + and each `.ps1` sets `$ErrorActionPreference = 'Stop'` **before** dot-sourcing + `common.ps1`, so a `common.ps1` that fails to load can no longer leave the example + running past it. Reproduced locally against the real files (pwsh 7.4.6, Intel macOS): + with #102's defect reinstated, rc=0 unguarded and rc=1 guarded. `validate-examples` + enforces the guard. +- **`examples/mndp/mndp.py` exited 0 when no MNDP announcement arrived.** The failure + branch used a bare `return`, which leaves `main()` through the `finally` and never + reaches the trailing `sys.exit(rc)` — the same false green as #102, in a different + language. It now raises `SystemExit`; cleanup still runs. Measured with the identity + match forced to fail: rc=0 before, rc=1 after. - **A healthy large download was reported as a timeout** (#116). Both download paths bounded a transfer by *total duration*: `images.ts` aborted at a flat 120 s per attempt and retried three times, `packages.ts` had no deadline and no retries at all. diff --git a/examples/common.ps1 b/examples/common.ps1 index 4a50f00..b2c6a8d 100644 --- a/examples/common.ps1 +++ b/examples/common.ps1 @@ -1,10 +1,22 @@ # Shared helpers for quickchr CLI examples (PowerShell - the Windows mirror of common.sh). # # Dot-source at the top of .ps1, then wrap the body in try/finally: +# $ErrorActionPreference = 'Stop' # . "$PSScriptRoot/../common.ps1" # $name = Get-ExampleName 'quickstart'; Register-Cleanup $name # try { Invoke-Qc start $name --channel stable; ... } finally { Invoke-QcCleanup } # +# The caller sets $ErrorActionPreference BEFORE the dot-source even though this +# file sets it too, and that duplication is load-bearing. If THIS file fails to +# load, the preference it sets never takes effect - so unless the CALLER already +# set it, the preference stays at its 'Continue' default, every helper call below +# is an ordinary non-terminating CommandNotFound, and the example exits 0 having +# done nothing: the silent false green in #102. The caller's copy is what makes a +# failed load terminate. Enforced by scripts/validate-examples.ts. +# +# Start-Job runs in a separate process and inherits neither preference, so a job +# script block must set both itself (see version-matrix.ps1). +# # Resolution rule mirrors common.sh: prefer an explicit $env:QUICKCHR override, # else the repo source CLI (so CI/local runs exercise THIS checkout), else a # globally installed `quickchr`. diff --git a/examples/device-mode/device-mode.ps1 b/examples/device-mode/device-mode.ps1 index 0e73f79..e79b512 100644 --- a/examples/device-mode/device-mode.ps1 +++ b/examples/device-mode/device-mode.ps1 @@ -1,4 +1,5 @@ # device-mode (CLI, Windows) - enable a device-mode feature on first boot, read it back. +$ErrorActionPreference = 'Stop' # before the dot-source - see common.ps1 (#102) . "$PSScriptRoot/../common.ps1" $name = Get-ExampleName 'device-mode' diff --git a/examples/dude/dude.ps1 b/examples/dude/dude.ps1 index eea4eb3..96f9888 100644 --- a/examples/dude/dude.ps1 +++ b/examples/dude/dude.ps1 @@ -1,5 +1,6 @@ # dude (CLI, Windows) - install the dude package on first boot, enable it, read it back. # PowerShell mirror of dude.sh. +$ErrorActionPreference = 'Stop' # before the dot-source - see common.ps1 (#102) . "$PSScriptRoot/../common.ps1" $name = Get-ExampleName 'dude' diff --git a/examples/grounding/grounding.ps1 b/examples/grounding/grounding.ps1 index 66492cd..7f9f9e5 100644 --- a/examples/grounding/grounding.ps1 +++ b/examples/grounding/grounding.ps1 @@ -1,5 +1,6 @@ # grounding (CLI, Windows) - apply RouterOS config, read it back, prove it took. # PowerShell mirror of grounding.sh. +$ErrorActionPreference = 'Stop' # before the dot-source - see common.ps1 (#102) . "$PSScriptRoot/../common.ps1" $name = Get-ExampleName 'grounding' diff --git a/examples/harness/harness.ps1 b/examples/harness/harness.ps1 index 9547f99..0988347 100644 --- a/examples/harness/harness.ps1 +++ b/examples/harness/harness.ps1 @@ -1,6 +1,7 @@ # harness (CLI, Windows) - hand a CHR's connection env to an external tool. # PowerShell can't eval the shell-quoted `env` output, so use --json and set # $env:* explicitly. This is the natural Windows showcase for env-passing. +$ErrorActionPreference = 'Stop' # before the dot-source - see common.ps1 (#102) . "$PSScriptRoot/../common.ps1" $name = Get-ExampleName 'harness' diff --git a/examples/mndp/mndp.py b/examples/mndp/mndp.py index 2fcb3a1..1e27313 100755 --- a/examples/mndp/mndp.py +++ b/examples/mndp/mndp.py @@ -42,7 +42,21 @@ def run_quickchr(*args: str, check: bool = True) -> subprocess.CompletedProcess: - return subprocess.run([*QUICKCHR, *args], capture_output=True, text=True, check=check) + # check=False: this wrapper does its own returncode handling below, because + # subprocess.run's own check= raises without the output (see next comment). + proc = subprocess.run([*QUICKCHR, *args], capture_output=True, text=True, check=False) + if check and proc.returncode != 0: + # subprocess.run(check=True) raises a CalledProcessError naming the command + # and the exit code and DISCARDING both streams -- and those streams are the + # only place quickchr says what actually went wrong. Run 30852139131 + # (linux/aarch64) failed here on `/system/identity/set` and reported nothing + # but "returned non-zero exit status 1", which is not enough to diagnose it. + raise SystemExit( + f"FAIL: quickchr {' '.join(args)} exited {proc.returncode}\n" + f"--- stdout ---\n{proc.stdout}" + f"--- stderr ---\n{proc.stderr}" + ) + return proc # ── frame parsing ──────────────────────────────────────────────────────────── @@ -234,8 +248,10 @@ def main() -> None: time.sleep(1) if not got: - print(f"FAIL: no MNDP announcement with identity '{IDENTITY}' within {args.timeout}s", file=sys.stderr) - return + # `raise`, not `return`: a bare return leaves main() through the finally + # and never reaches `sys.exit(rc)` below, so the failure would exit 0 — + # the same silent false green as #102's PowerShell example. + raise SystemExit(f"FAIL: no MNDP announcement with identity '{IDENTITY}' within {args.timeout}s") print("\nMNDP received over L2 (socket-connect):") for k in ("identity", "version", "platform", "board", "ifname", "ipv4", "uptime", "mac", "softwareId"): if k in got: diff --git a/examples/quickstart/quickstart.ps1 b/examples/quickstart/quickstart.ps1 index 75276f7..8d04244 100644 --- a/examples/quickstart/quickstart.ps1 +++ b/examples/quickstart/quickstart.ps1 @@ -1,5 +1,6 @@ # quickstart (CLI, Windows) - boot a CHR, read resource + descriptor, tear down. # PowerShell mirror of quickstart.sh. try/finally guarantees teardown. +$ErrorActionPreference = 'Stop' # before the dot-source - see common.ps1 (#102) . "$PSScriptRoot/../common.ps1" $name = Get-ExampleName 'quickstart' diff --git a/examples/rollback/rollback.ps1 b/examples/rollback/rollback.ps1 index 5c77fec..aba30ac 100644 --- a/examples/rollback/rollback.ps1 +++ b/examples/rollback/rollback.ps1 @@ -1,4 +1,5 @@ # rollback (CLI, Windows) - snapshot a CHR, change it, restore the snapshot. +$ErrorActionPreference = 'Stop' # before the dot-source - see common.ps1 (#102) . "$PSScriptRoot/../common.ps1" $name = Get-ExampleName 'rollback' diff --git a/examples/service-forward/service-forward.ps1 b/examples/service-forward/service-forward.ps1 index 0b3dfa3..f1e4ac5 100644 --- a/examples/service-forward/service-forward.ps1 +++ b/examples/service-forward/service-forward.ps1 @@ -1,4 +1,5 @@ # service-forward (CLI, Windows) - pin a guest service to a chosen host port. +$ErrorActionPreference = 'Stop' # before the dot-source - see common.ps1 (#102) . "$PSScriptRoot/../common.ps1" $name = Get-ExampleName 'service-forward' diff --git a/examples/trial-license/trial-license.ps1 b/examples/trial-license/trial-license.ps1 index bb82ffd..3a979f2 100644 --- a/examples/trial-license/trial-license.ps1 +++ b/examples/trial-license/trial-license.ps1 @@ -1,5 +1,6 @@ # trial-license (CLI, Windows) - apply a CHR trial license, read it back. MANUAL-ONLY. # MikroTik rate-limits trial requests, so this is excluded from CI. +$ErrorActionPreference = 'Stop' # before the dot-source - see common.ps1 (#102) . "$PSScriptRoot/../common.ps1" $name = Get-ExampleName 'trial-license' diff --git a/examples/version-matrix/version-matrix.ps1 b/examples/version-matrix/version-matrix.ps1 index d612bcd..c3f2400 100644 --- a/examples/version-matrix/version-matrix.ps1 +++ b/examples/version-matrix/version-matrix.ps1 @@ -2,6 +2,7 @@ # PowerShell mirror of version-matrix.sh (parallel start via background jobs). param([switch]$Lite) +$ErrorActionPreference = 'Stop' # before the dot-source - see common.ps1 (#102) . "$PSScriptRoot/../common.ps1" $channels = if ($Lite) { @('long-term', 'stable') } else { @('long-term', 'stable', 'testing', 'development') } @@ -19,6 +20,14 @@ foreach ($ch in $channels) { # PSUseUsingScopeModifierInNewRunspaces wants -- it doesn't recognize the older # param()+-ArgumentList pattern and flags those as missing the Using: scope. $jobs += Start-Job -ScriptBlock { + # Set INSIDE the block: Start-Job runs in a separate process that inherits + # no preference variables, so the caller's and common.ps1's copies do not + # reach here. Without these, a `quickchr start` that exits non-zero without + # writing to stderr is invisible - Receive-Job returns, the script continues, + # and the example exits 0 having booted nothing. Measured (pwsh 7.4.6): a + # silent `exit 3` in a job gives rc=0 unset, rc=1 with these two set. + $ErrorActionPreference = 'Stop' + $PSNativeCommandUseErrorActionPreference = $true $parts = $using:qc -split '\s+' & $parts[0] @($parts[1..($parts.Length - 1)]) start $using:name --channel $using:ch --no-secure-login --port-base $using:base --add-package container --mem 256 } diff --git a/scripts/validate-examples.ts b/scripts/validate-examples.ts index 612741f..e84f88e 100644 --- a/scripts/validate-examples.ts +++ b/scripts/validate-examples.ts @@ -7,16 +7,20 @@ * - no Makefile / stray files (only .ts/.test.ts/.sh/.ps1/.py/.md + known subdirs) * - the primary script is named after its directory * - every relative link and run-command file reference in the README resolves + * - each .ps1 sets $ErrorActionPreference before dot-sourcing common.ps1 * * Wired into `bun run check`. Exits non-zero on any violation. */ -import { readdirSync, statSync } from "node:fs"; +import { readFileSync, readdirSync, statSync } from "node:fs"; import { existsSync } from "node:fs"; import { join, resolve } from "node:path"; const EXAMPLES = resolve(import.meta.dir, "..", "examples"); const ALLOWED_EXT = new Set([".ts", ".sh", ".ps1", ".py", ".md"]); -const SKIP_DIRS = new Set(["_template", "config", "tool", "node_modules"]); +// `__pycache__` is gitignored, so it never reaches CI and can never be a real +// violation — but any local `python3 -m py_compile` / import of a .py example +// creates one, and flagging it turns `bun run check` red for a build artifact. +const SKIP_DIRS = new Set(["_template", "config", "tool", "node_modules", "__pycache__"]); // (Top-level files like README.md / lib.ts / common.sh are skipped implicitly: // the dir scan below only descends into directories, never stat's loose files.) @@ -80,6 +84,24 @@ for (const name of dirs) { } } + // PowerShell examples must arm $ErrorActionPreference themselves, BEFORE + // dot-sourcing common.ps1. A common.ps1 that fails to load leaves the + // preference at 'Continue', so every undefined helper is a non-terminating + // error and the example exits 0 having run no quickchr command at all — the + // false green in #102, reproduced locally 2026-08-03 (pwsh 7.4.6: rc=0 without + // the guard, rc=1 with it). + for (const f of files.filter((x) => x.endsWith(".ps1"))) { + const lines = readFileSync(join(dir, f), "utf8").split(/\r?\n/); + const dotSource = lines.findIndex((l) => /^\s*\.\s+["']\$PSScriptRoot/.test(l)); + if (dotSource < 0) continue; // self-contained script, nothing to guard against + const armed = lines + .slice(0, dotSource) + .some((l) => /^\s*\$ErrorActionPreference\s*=\s*'Stop'/.test(l)); + if (!armed) { + err(where, `"${f}" must set $ErrorActionPreference = 'Stop' BEFORE dot-sourcing common.ps1`); + } + } + // README references resolve. const readmePath = join(dir, "README.md"); if (existsSync(readmePath)) { diff --git a/src/lib/net.ts b/src/lib/net.ts index 9a569c6..f4670ac 100644 --- a/src/lib/net.ts +++ b/src/lib/net.ts @@ -19,7 +19,9 @@ * error), we retry by resolving the A record against a public DNS server * directly (~10 ms), bypassing the host's resolv.conf, and connecting to the * IPv4 literal with the `Host` header and TLS SNI preserved so certificate - * validation still passes. + * validation still passes. If that attempt fails too, both failures are reported + * together as a `ResilientFetchError` — the fallback's own error names only an IP + * literal, which on its own reads as a host nobody configured. * * Only connection-class failures trigger the fallback; HTTP responses (incl. * 5xx) and aborts (`AbortError`, e.g. from `AbortSignal.timeout`) pass through @@ -32,6 +34,49 @@ import { promises as dns } from "node:dns"; const PUBLIC_DNS_SERVERS = ["1.1.1.1", "8.8.8.8", "1.0.0.1"]; const DNS_TIMEOUT_MS = 3000; +/** `CODE: message` when the error carries a code, else just the message. */ +function describeError(err: unknown): string { + if (!err || typeof err !== "object") return String(err); + const e = err as { name?: string; message?: string; code?: string; cause?: { code?: string } }; + const code = e.code ?? e.cause?.code; + const label = code ?? e.name; + const message = e.message ?? String(err); + return label ? `${label}: ${message}` : message; +} + +/** + * Both transports failed: the direct fetch with a connection-class error, and + * then the public-DNS/IPv4 fallback too. + * + * It exists because the fallback's own error is unreadable on its own — it names + * an IP literal nobody configured and no hostname, so a log cannot distinguish + * "the system resolver was broken" from "the host refused us" from "public DNS + * handed back a bad address" (#121). The message carries the URL, both failures, + * and which address the fallback tried; the direct failure is also the `cause`. + */ +export class ResilientFetchError extends Error { + /** The hostname both transports were aiming at. */ + readonly host: string; + /** The IPv4 literal the fallback connected to, from public DNS. */ + readonly address: string; + /** The fallback transport's failure. The direct attempt's is `cause`. */ + readonly fallbackError: unknown; + + constructor(url: string, address: string, directError: unknown, fallbackError: unknown) { + const host = new URL(url).hostname; + super( + `Fetch failed for ${url} on both transports: ` + + `system resolver — ${describeError(directError)}; ` + + `public DNS (${PUBLIC_DNS_SERVERS.join(", ")}) → ${address} — ${describeError(fallbackError)}`, + { cause: directError }, + ); + this.name = "ResilientFetchError"; + this.host = host; + this.address = address; + this.fallbackError = fallbackError; + } +} + /** * True for the connection-class failures raised when a socket cannot be opened * (e.g. IPv4 blocked on an IPv6-only network, or Bun's `errno: 0` @@ -39,9 +84,14 @@ const DNS_TIMEOUT_MS = 3000; * the standard connect errors. Excludes `AbortError` (aborts/`AbortSignal` * timeouts) and HTTP-level outcomes (those carry a Response and never throw * here). Used to decide whether to fall back from the normal fetch to the - * public-DNS IPv4 attempt. + * public-DNS IPv4 attempt, and by callers deciding whether a failure is worth + * another attempt. */ export function isConnectionFailure(err: unknown): boolean { + // Connection-class by construction: fetchResilient raises this only after a + // direct failure that already passed this test. Callers retrying on a + // connection failure must keep retrying once the fallback also fails. + if (err instanceof ResilientFetchError) return true; if (!err || typeof err !== "object") return false; const e = err as { name?: string; code?: string; errno?: number; cause?: { code?: string } }; if (e.name === "AbortError") return false; @@ -142,6 +192,12 @@ export async function fetchResilient(url: string, init?: BunFetchRequestInit): P const address = await resolveIpv4(new URL(url).hostname); // Public DNS can't help (unreachable / no answer) — surface the original failure. if (address === undefined) throw err; - return fetchOverIpv4(url, address, init); + try { + return await fetchOverIpv4(url, address, init); + } catch (fallbackErr) { + // Never let the fallback's error stand alone: it names only an IP literal, + // which is the one thing the reader cannot map back to what was attempted. + throw new ResilientFetchError(url, address, err, fallbackErr); + } } } diff --git a/src/lib/versions.ts b/src/lib/versions.ts index a0dc027..f5139be 100644 --- a/src/lib/versions.ts +++ b/src/lib/versions.ts @@ -4,7 +4,7 @@ import type { Arch, Channel } from "./types.ts"; import { CHANNELS, QuickCHRError } from "./types.ts"; -import { fetchResilient } from "./net.ts"; +import { fetchResilient, isConnectionFailure } from "./net.ts"; const UPGRADE_BASE = "https://upgrade.mikrotik.com/routeros/NEWESTa7"; const DOWNLOAD_BASE = "https://download.mikrotik.com/routeros"; @@ -65,35 +65,112 @@ export function provisioningSupportHint(minimumVersion = MIN_PROVISION_VERSION): return `Use --channel long-term or --version ${minimumVersion}+ for ${PROVISIONING_FEATURE_SUMMARY}, or keep the older version without provisioning options.`; } -/** Fetch the latest version for a given channel from MikroTik's upgrade server. */ -export async function resolveVersion(channel: Channel): Promise { - const url = `${UPGRADE_BASE}.${channel}`; +/** Attempts for a connection-class failure while resolving a channel version. */ +export const VERSION_RESOLVE_MAX_ATTEMPTS = 3; + +/** Backoff base: attempt N waits N × this before N+1. Mirrors `downloadToFile`. */ +const VERSION_RESOLVE_RETRY_MS = 2000; + +export interface ResolveVersionOptions { + /** Attempts for connection-class failures. Defaults to {@link VERSION_RESOLVE_MAX_ATTEMPTS}. */ + maxAttempts?: number; + /** + * Override the backoff base. A test lever, like `downloadToFile`'s `stallMs`: + * the real backoff spends 6 s of wall clock exhausting the default attempts. + * Production callers should not set it. + */ + retryDelayMs?: number; +} - const response = await fetchResilient(url); - if (!response.ok) { - throw new QuickCHRError( - "DOWNLOAD_FAILED", - `Failed to fetch version for channel "${channel}": HTTP ${response.status}`, - ); - } +/** A whole count of at least 1, or `fallback` for anything that isn't one. */ +function positiveInt(value: number | undefined, fallback: number): number { + if (value === undefined || !Number.isFinite(value)) return fallback; + return Math.max(1, Math.floor(value)); +} + +/** A finite, non-negative delay in ms, or `fallback` for anything that isn't one. */ +function nonNegativeMs(value: number | undefined, fallback: number): number { + if (value === undefined || !Number.isFinite(value)) return fallback; + return Math.max(0, value); +} - const text = await response.text(); - // Response format: "7.22.1 1774276515" (version + unix timestamp) - const version = text.trim().split(/\s+/)[0]?.trim(); - if (!version || !isValidVersion(version)) { - throw new QuickCHRError( - "INVALID_VERSION", - `Unexpected version format for channel "${channel}": "${text.trim()}"`, - ); +/** + * Fetch the latest version for a given channel from MikroTik's upgrade server. + * + * Retried on a connection-class failure only — a transient refusal at + * `upgrade.mikrotik.com` used to fail a whole `quickchr start`, since this sits + * on the critical path of `start()`, the wizard and the CLI while only the + * download path had a retry policy (#121). An HTTP answer is the server's verdict + * and stays terminal on the first attempt: a 404 for a bad channel must fail fast. + */ +export async function resolveVersion( + channel: Channel, + opts: ResolveVersionOptions = {}, +): Promise { + const url = `${UPGRADE_BASE}.${channel}`; + // Sanitized, not just clamped: these are public options, and `Math.max(1, NaN)` + // is NaN — the loop would never run, and the failure would read "all NaN + // attempts: undefined" instead of doing anything. + const maxAttempts = positiveInt(opts.maxAttempts, VERSION_RESOLVE_MAX_ATTEMPTS); + const retryDelayMs = nonNegativeMs(opts.retryDelayMs, VERSION_RESOLVE_RETRY_MS); + let lastError: unknown; + + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + let text: string; + try { + const response = await fetchResilient(url); + if (!response.ok) { + // An HTTP answer is the server's verdict. Thrown as a QuickCHRError so + // the catch below can tell it from a transport failure and let it out. + throw new QuickCHRError( + "DOWNLOAD_FAILED", + `Failed to fetch version for channel "${channel}": HTTP ${response.status}`, + ); + } + // Inside the try: the body can fail *after* the headers arrive (a reset + // mid-response), which is a transport failure like any other and must be + // retried rather than escaping the loop unwrapped. + text = await response.text(); + } catch (err) { + // Only "could not reach the host at all" is retried. An HTTP verdict, an + // abort (the caller gave up) or a real bug surfaces immediately. + if (err instanceof QuickCHRError) throw err; + if (!isConnectionFailure(err)) throw err; + lastError = err; + if (attempt < maxAttempts) await Bun.sleep(attempt * retryDelayMs); + continue; + } + + // Parsing is terminal: the server answered, and answering with something + // unparseable is not a condition another attempt can improve. + // Response format: "7.22.1 1774276515" (version + unix timestamp) + const version = text.trim().split(/\s+/)[0]?.trim(); + if (!version || !isValidVersion(version)) { + throw new QuickCHRError( + "INVALID_VERSION", + `Unexpected version format for channel "${channel}": "${text.trim()}"`, + ); + } + + return version; } - return version; + // Says "1 attempt" rather than "all 1 attempts" — these land in CI logs. + const tries = maxAttempts === 1 ? "1 attempt" : `all ${maxAttempts} attempts`; + throw new QuickCHRError( + "DOWNLOAD_FAILED", + `Failed to fetch version for channel "${channel}" on ${tries}: ` + + `${lastError instanceof Error ? lastError.message : String(lastError)}`, + `Check network access to ${new URL(url).hostname}, or pass an explicit --version to skip channel resolution.`, + ); } /** Fetch latest versions for all channels in parallel. */ -export async function resolveAllVersions(): Promise> { +export async function resolveAllVersions( + opts: ResolveVersionOptions = {}, +): Promise> { const results = await Promise.all( - CHANNELS.map(async (ch) => [ch, await resolveVersion(ch)] as const), + CHANNELS.map(async (ch) => [ch, await resolveVersion(ch, opts)] as const), ); return Object.fromEntries(results) as Record; } diff --git a/test/integration/examples-smoke.test.ts b/test/integration/examples-smoke.test.ts index b570379..ecfe680 100644 --- a/test/integration/examples-smoke.test.ts +++ b/test/integration/examples-smoke.test.ts @@ -47,8 +47,26 @@ interface Runnable { env?: Record; // Restrict to these platforms (process.platform). Omitted = all. os?: NodeJS.Platform[]; + /** + * Substrings that must appear in stdout for the run to count as a pass. + * + * Exit code plus non-empty output is not evidence an example worked: in #102 + * a ParserError left every helper in `common.ps1` undefined, so + * `quickstart.ps1` ran no quickchr command at all — and still printed one line + * and exited 0, which the smoke test reported green for months. + * + * So each entry names output only a working run produces, and always includes + * something from the END of the script (a mid-run silent failure must not + * pass) and something the CLI/library itself emitted (not just the example's + * own `echo`). The interpolated `examples--` machine name is deliberate: + * the tell in #102 was a double space where that name should have been. + */ + expect: string[]; } +/** Emitted by `quickchr inspect` — proof the CLI ran, not just the example's echo. */ +const INSPECT_MARKER = '"descriptorVersion": 1'; + // One representative per language, selected per OS so the CLI mirror that actually // ships for the current platform is the one exercised: // - .ts everywhere (cross-platform library scripts); @@ -56,14 +74,25 @@ interface Runnable { // - .ps1 on Windows (where the .sh/.py mirrors aren't the documented path). // Kept small — each entry boots a real CHR. const RUNNABLE: Runnable[] = [ - { name: "quickstart", lang: "ts", cmd: ["bun", "run", "examples/quickstart/quickstart.ts"] }, - { name: "rollback", lang: "ts", cmd: ["bun", "run", "examples/rollback/rollback.ts"] }, + { + name: "quickstart", + lang: "ts", + cmd: ["bun", "run", "examples/quickstart/quickstart.ts"], + expect: ["RouterOS ", 'ethernet interface(s); identity "'], + }, + { + name: "rollback", + lang: "ts", + cmd: ["bun", "run", "examples/rollback/rollback.ts"], + expect: ['saved snapshot "baseline"', 'rolled back; identity="before-snapshot"'], + }, { name: "quickstart-sh", lang: "sh", cmd: ["sh", "examples/quickstart/quickstart.sh"], env: QUICKCHR_ENV, os: ["linux", "darwin"], + expect: ["starting examples-quickstart-", "connection descriptor", INSPECT_MARKER], }, { name: "mndp-py", @@ -71,6 +100,9 @@ const RUNNABLE: Runnable[] = [ cmd: ["uv", "run", "examples/mndp/mndp.py", "--timeout", "45"], env: QUICKCHR_ENV, os: ["linux", "darwin"], + // "mndp-example" is the identity the script set on the router and then read + // back out of a captured L2 frame — router-sourced, not the script's own echo. + expect: ["MNDP received over L2", "mndp-example", "removed 'examples-mndp-"], }, { name: "quickstart-ps1", @@ -78,6 +110,7 @@ const RUNNABLE: Runnable[] = [ cmd: ["pwsh", "examples/quickstart/quickstart.ps1"], env: QUICKCHR_ENV, os: ["win32"], + expect: ["starting examples-quickstart-", "connection descriptor", INSPECT_MARKER], }, ]; @@ -176,7 +209,11 @@ describe.skipIf(SKIP)("examples smoke", () => { console.error(`[${r.name}] exit ${code} ${detail}`); } expect(code).toBe(0); - expect(out.length).toBeGreaterThan(0); + // Reported as the list of what was missing rather than one toContain per + // marker: the failure then names every marker the run failed to produce, + // which is what distinguishes "wrong result" from "did nothing at all". + // Subsumes the old `out.length > 0` check, which #102 sailed through. + expect(r.expect.filter((marker) => !out.includes(marker))).toEqual([]); }, PER_TEST_TIMEOUT, ); diff --git a/test/unit/net.test.ts b/test/unit/net.test.ts index f460da5..43bbeff 100644 --- a/test/unit/net.test.ts +++ b/test/unit/net.test.ts @@ -1,6 +1,11 @@ import { describe, test, expect, spyOn, mock, afterEach } from "bun:test"; import { promises as dns } from "node:dns"; -import { fetchResilient, isConnectionFailure, toIpv4Url } from "../../src/lib/net.ts"; +import { + fetchResilient, + isConnectionFailure, + ResilientFetchError, + toIpv4Url, +} from "../../src/lib/net.ts"; /** Spy the public-DNS A-record lookup that fetchResilient performs. */ function mockResolve4(impl: { resolve?: string[]; reject?: unknown }) { @@ -35,6 +40,18 @@ describe("isConnectionFailure", () => { ); }); + test("true for a ResilientFetchError — both transports failed to connect", () => { + // Constructed only after a direct failure that already passed this test, so a + // caller retrying connection failures must keep retrying this one (#121). + const both = new ResilientFetchError( + "https://h.example/x", + "9.9.9.9", + Object.assign(new Error("Unable to connect"), { code: "ConnectionRefused" }), + Object.assign(new Error("Unable to connect"), { code: "ConnectionRefused" }), + ); + expect(isConnectionFailure(both)).toBe(true); + }); + test("false for aborts, plain TypeErrors, and non-error values", () => { const abort = new Error("The operation timed out."); abort.name = "AbortError"; @@ -116,6 +133,38 @@ describe("fetchResilient", () => { expect(fetchSpy).toHaveBeenCalledTimes(1); // normal attempt only; IPv4 path never reached }); + test("reports host, both transports' errors and the fallback address when both fail", async () => { + mockResolve4({ resolve: ["159.148.147.251"] }); + let call = 0; + spyOn(globalThis, "fetch").mockImplementation((async () => { + call++; + throw call === 1 + ? Object.assign(new Error("Unable to connect via system resolver"), { + code: "ConnectionRefused", + }) + : Object.assign(new Error("Unable to connect. Is the computer able to access the url?"), { + code: "ConnectionRefused", + }); + }) as unknown as typeof fetch); + + // Anchor on the message: the whole point of #121's gap 2 is that a log which + // names only the IP literal cannot say which transport failed or where the + // address came from. + const err = (await fetchResilient( + "https://upgrade.mikrotik.com/routeros/NEWESTa7.testing", + ).catch((e) => e)) as ResilientFetchError; + + expect(err).toBeInstanceOf(ResilientFetchError); + expect(err.host).toBe("upgrade.mikrotik.com"); + expect(err.address).toBe("159.148.147.251"); + expect(err.message).toContain("https://upgrade.mikrotik.com/routeros/NEWESTa7.testing"); + expect(err.message).toContain("system resolver — ConnectionRefused: Unable to connect via system resolver"); + expect(err.message).toContain("public DNS (1.1.1.1, 8.8.8.8, 1.0.0.1) → 159.148.147.251"); + expect(err.message).toContain("Is the computer able to access the url?"); + // The direct failure stays reachable programmatically, not just in the text. + expect((err.cause as { code?: string }).code).toBe("ConnectionRefused"); + }); + test("rethrows non-connection errors (e.g. timeouts) without a fallback", async () => { const resolveSpy = mockResolve4({ reject: new Error("resolve4 must not be called") }); const fetchSpy = spyOn(globalThis, "fetch").mockImplementation((async () => { diff --git a/test/unit/versions.test.ts b/test/unit/versions.test.ts index 25a374f..50f643f 100644 --- a/test/unit/versions.test.ts +++ b/test/unit/versions.test.ts @@ -17,6 +17,7 @@ import { selectActiveChannels, resolveActiveChannels, resolveChannelStatuses, + VERSION_RESOLVE_MAX_ATTEMPTS, } from "../../src/lib/versions.ts"; import { CHANNELS } from "../../src/lib/types.ts"; import type { Channel } from "../../src/lib/types.ts"; @@ -261,6 +262,130 @@ describe("resolveVersion", () => { const version = await resolveVersion("stable"); expect(version).toBe("7.22.1"); }); + + // --- Retry policy (#121) --- + // + // The stub is the mocked `fetch` above, not a live server: a test must never + // depend on MikroTik's host actually being flaky to exercise the retry. + + test("retries a connection-class failure and succeeds on a later attempt", async () => { + let calls = 0; + globalThis.fetch = makeMockFetch(() => { + calls++; + if (calls < 3) { + return Promise.reject( + Object.assign(new Error("Unable to connect"), { code: "ConnectionRefused" }), + ); + } + return Promise.resolve(new Response("7.22.1 1774276515")); + }); + // retryDelayMs keeps the backoff out of the test's wall clock. + expect(await resolveVersion("stable", { retryDelayMs: 1 })).toBe("7.22.1"); + expect(calls).toBe(3); + }); + + test("gives up after maxAttempts, naming the attempts and the last failure", async () => { + let calls = 0; + globalThis.fetch = makeMockFetch(() => { + calls++; + return Promise.reject( + Object.assign(new Error("Unable to connect"), { code: "ConnectionRefused" }), + ); + }); + await expect( + resolveVersion("testing", { maxAttempts: 3, retryDelayMs: 1 }), + ).rejects.toMatchObject({ + code: "DOWNLOAD_FAILED", + message: expect.stringContaining("on all 3 attempts"), + }); + expect(calls).toBe(3); + // The hint has to name the host — the message may only carry the fallback's + // IP literal when fetchResilient's public-DNS path is what failed last. + await expect( + resolveVersion("testing", { maxAttempts: 1, retryDelayMs: 1 }), + ).rejects.toMatchObject({ + message: expect.stringContaining("on 1 attempt"), + installHint: expect.stringContaining("upgrade.mikrotik.com"), + }); + }); + + test("does not retry an HTTP error status — a bad channel stays fast and terminal", async () => { + let calls = 0; + globalThis.fetch = makeMockFetch(() => { + calls++; + return Promise.resolve(new Response("Not Found", { status: 404 })); + }); + await expect(resolveVersion("stable", { retryDelayMs: 1 })).rejects.toMatchObject({ + code: "DOWNLOAD_FAILED", + }); + expect(calls).toBe(1); + }); + + test("retries a body that fails after the headers arrived", async () => { + // The response can die mid-body (a reset after 200 OK). That is a transport + // failure like any other and must be retried, not escape the loop unwrapped. + let calls = 0; + globalThis.fetch = makeMockFetch(() => { + calls++; + if (calls < 2) { + return Promise.resolve( + new Response( + new ReadableStream({ + pull(controller) { + controller.error( + Object.assign(new Error("The socket connection was closed unexpectedly"), { + code: "ConnectionClosed", + }), + ); + }, + }), + ), + ); + } + return Promise.resolve(new Response("7.22.1 1774276515")); + }); + expect(await resolveVersion("stable", { retryDelayMs: 1 })).toBe("7.22.1"); + expect(calls).toBe(2); + }); + + test("falls back to the defaults for NaN / nonsense options", async () => { + let calls = 0; + globalThis.fetch = makeMockFetch(() => { + calls++; + return Promise.reject( + Object.assign(new Error("Unable to connect"), { code: "ConnectionRefused" }), + ); + }); + // `Math.max(1, NaN)` is NaN, which would skip the loop entirely and report + // "all NaN attempts: undefined" — a failure message describing nothing. + await expect( + resolveVersion("stable", { maxAttempts: Number.NaN, retryDelayMs: 0 }), + ).rejects.toMatchObject({ + code: "DOWNLOAD_FAILED", + message: expect.stringContaining(`all ${VERSION_RESOLVE_MAX_ATTEMPTS} attempts`), + }); + expect(calls).toBe(VERSION_RESOLVE_MAX_ATTEMPTS); + // A fractional count is floored to a whole number of attempts, never used raw. + calls = 0; + await expect(resolveVersion("stable", { maxAttempts: 2.7, retryDelayMs: 0 })).rejects.toThrow( + "all 2 attempts", + ); + expect(calls).toBe(2); + }); + + test("does not retry an abort — the caller gave up", async () => { + let calls = 0; + globalThis.fetch = makeMockFetch(() => { + calls++; + const abort = new Error("The operation timed out."); + abort.name = "AbortError"; + return Promise.reject(abort); + }); + await expect(resolveVersion("stable", { retryDelayMs: 1 })).rejects.toThrow( + "The operation timed out.", + ); + expect(calls).toBe(1); + }); }); describe("resolveAllVersions", () => {