diff --git a/docs/guides/commands.md b/docs/guides/commands.md index db1aa7f..f60b171 100644 --- a/docs/guides/commands.md +++ b/docs/guides/commands.md @@ -108,6 +108,7 @@ Notes: - The replacement needs write access to the installed binary. If it lives in a system directory such as `/usr/local/bin`, run `sudo ignatius update --yes`, or reinstall with the [install script](getting-started.md#install-script-recommended). - Outside a terminal (CI) it will not prompt: it reports the available version and exits without installing unless you pass `--yes`. - Windows binaries cannot replace themselves while running; on Windows the command points you at the release download instead. +- While the binary downloads, a single line updates in place: `Downloading 41.9 / 73.2 MB (57%)`, ending with `Downloaded 73.2 MB (100%)` before the swap. Off a terminal (CI, piped output) this line is suppressed and one static line prints instead. If the server does not report a size, it shows the megabytes downloaded so far with no percentage. ## Keyboard shortcuts diff --git a/docs/spec/update-download-progress.md b/docs/spec/update-download-progress.md new file mode 100644 index 0000000..46ab6d7 --- /dev/null +++ b/docs/spec/update-download-progress.md @@ -0,0 +1,80 @@ +# Update download progress + + +## Goal + + +Show live download progress while `ignatius update` replaces the running binary. Today `downloadAndReplace` buffers the whole asset with `arrayBuffer()` and prints one static `Downloading ignatius …` line, so a ~50MB download looks identical to a hang. Stream the asset instead, count bytes as they arrive, and rewrite a single status line in place until 100%. + +The mechanism is the one `atomic` already ships (`internal/selfupdate/selfupdate.go:519`, `cmd/atomic/cmd_update.go:360`): count bytes off the response body and fire a callback every 512KB. No timer, no `stat()` polling. + + +## Non-goals + + +- A stall/hang watchdog on the download (a real gap, tracked as a follow-up, not this change). +- Resumable downloads via HTTP range requests. +- Progress for the checksums.txt fetch (a few hundred bytes; noise). +- Changing the update decision, asset naming, checksum, or swap semantics. +- Windows self-replace (still refused with a manual-download message). + + +## Success criteria + + +- [ ] `downloadProgressRenderer(write, isTTY)` is exported from `src/cli/update.ts`, pure apart from its injected `write`, and unit-tested. +- [ ] Off-TTY it returns `null`. Without `\r` rewriting, every tick would print its own line into redirected output. +- [ ] Mid-stream with a known total it writes `\rDownloading / MB (%)` and does **not** end the line. +- [ ] At `received >= total` it writes a `100%` line terminated with `\n`, then goes quiet. Later calls write nothing. +- [ ] With an unknown total (no `Content-Length`) it writes bare MB and no percent. +- [ ] `downloadAndReplace` streams the response body to the staging file rather than buffering it; the whole asset is never held in memory. +- [ ] sha256 is computed incrementally over the streamed chunks, so the file is never read a second time and no copy is held in memory. +- [ ] Checksum verification still happens **after** download and **before** the rename, and an unreachable checksums.txt is still non-fatal while a genuine mismatch still aborts. +- [ ] A failed or aborted download leaves no staging file behind. +- [ ] `docs/guides/commands.md` describes the progress output; the `docs/wiki/feature-map.md` row for self-update gains its spec surface. +- [ ] `bun run test` passes (`bun run build:cli` first, since the suite asserts on `dist/`), and `bunx tsc --noEmit` reports no *new* errors. This file starts with two pre-existing `TS2339` errors and ends with one: `Bun.CryptoHasher` and `Bun.write` do not resolve off the global `Bun` despite `bun-types` declaring them, and the `Bun.write` call disappears with the buffering path. The runtime is unaffected. Tracked as a follow-up, not fixed here. + + +## Approaches + + +| # | Approach | Sketch | Cost | Risk | +|---|----------|--------|------|------| +| A | Stream + byte-threshold callback + `\r` renderer (chosen) | Read `response.body` chunks, write each to the staging file, hash it, emit every 512KB | low | none material; mirrors a mechanism already in production in `atomic` and `noorm` | +| B | Poll the staging file's size on a 100ms timer | `setInterval` + `stat()` until the fetch resolves | low | a second source of truth for "how far along"; timer outlives the download on error; needs its own teardown; still has to buffer or stream underneath | +| C | Keep `arrayBuffer()`, show an indeterminate spinner | Spinner while awaiting | trivial | tells the user nothing they didn't know; still holds ~50MB in memory | + + +## Recommendation + + +**A.** The byte count is already flowing through the process. The only reason it isn't visible is that `arrayBuffer()` collapses the whole stream into one await. Reading the body chunk-wise surfaces it for free and drops peak memory from the asset's full size to one chunk. **B** is what the request described, but polling the file re-derives a number the loop already holds, and a timer that must be cleared on every exit path is more moving parts than the counter it replaces. Emitting on a byte threshold rather than a time interval also makes the renderer deterministic to test, with no fake clock. + + +## Checkpoints + + +| # | Checkpoint | Files/areas | Agent | Est. files | Verifies | +|---|------------|-------------|-------|------------|----------| +| 1 | Pure `downloadProgressRenderer(write, isTTY)` + unit tests covering off-TTY null, mid-stream no-newline, final 100% + newline, quiet-after-done, unknown-total | `src/cli/update.ts`, new `test/checks/test-update-progress.ts` | atomic-implementer (surgical) | ~2 | `bun test/checks/test-update-progress.ts` green | +| 2 | Stream `downloadAndReplace`: chunk-wise body read to the staging file, incremental sha256, 512KB progress emit, staging cleanup on failure; wire the renderer in `runUpdateCommand` | `src/cli/update.ts` | atomic-implementer (surgical) | 1 | `bun run typecheck`; `bun run test`; asset never buffered whole; verify-before-rename preserved | +| 3 | Document the progress output and close the surface map | `docs/guides/commands.md`, `docs/wiki/feature-map.md` | atomic-implementer (surgical) | ~2 | feature-map row lists the spec; guide describes what the user sees | + + +## Change log + + +### 2026-09-08 — typecheck criterion restated + +**What changed:** The success criterion read "`bun run typecheck` passes". It now reads "reports no *new* errors", and names the two pre-existing `TS2339` errors this file carries. + +**Why:** Correction. The criterion was written before the baseline was measured and was never true. The repo carries 652 pre-existing typecheck errors, two of them in this file, so a green typecheck was never available as a gate. + +**Superseded:** the prior contract required a fully green `bun run typecheck`. + + +### 2026-09-08 — prose and accuracy pass + +**What changed:** Em dashes removed from prose throughout. The typecheck criterion no longer cites baseline line numbers, and now states that the file ends the change with one error rather than two. + +**Why:** Audit findings. Em dashes in prose break the atomic-writing voice rule, and the `Bun.write` error disappears with the buffering path this change deletes, so the criterion had gone stale against its own implementation. diff --git a/docs/wiki/feature-map.md b/docs/wiki/feature-map.md index 2108caa..10fe4da 100644 --- a/docs/wiki/feature-map.md +++ b/docs/wiki/feature-map.md @@ -21,7 +21,7 @@ Paths are relative to `docs/design/`, `docs/spec/`, `docs/guides/`, and `skills/ | Schema lint + error UX (findings) | schema-lint-and-error-ux | schema-lint-and-error-ux | validation | verification (rule table + loop) | | Alternate keys (AK): cardinality derivation, dict/graph key-cell marker, `ak_unknown_column` validation | derive-classification (cardinality) | derive-classification, schema-lint-and-error-ux | derivation | entity-flow E4 | | CLI subcommands (`serve` SPA + `export` unified static + `validate`) — `dict`/`graph`/`flow` removed | cli-and-outputs, unified-app | cli-and-outputs, unified-app | commands, building-from-source, getting-started | verification (runs `ignatius validate`) | -| CLI version + self-update (`version`/`--version`, `update`) | — | — | commands, getting-started | — | +| CLI version + self-update (`version`/`--version`, `update`) | — | update-download-progress | commands, getting-started | — | | Project config + model discovery (`ignatius.yml`) | ignatius-project-config | ignatius-project-config | getting-started, folder-format | entity-flow E0, model-flow M1–M8, templates | | Themes | cli-and-outputs | cli-and-outputs | themes-and-branding | model-flow M4 | | Branding | branding | branding | themes-and-branding | model-flow M5 | diff --git a/src/cli/update.ts b/src/cli/update.ts index e35dd59..e3fb788 100644 --- a/src/cli/update.ts +++ b/src/cli/update.ts @@ -64,6 +64,44 @@ export function parseChecksums(text: string): Record { return out; } +const MIB = 1024 * 1024; + +// Wide enough to fully overwrite the longest status line this renderer ever +// prints, so each `\r` rewrite erases the previous line by construction +// instead of by a hand-counted trailing-space total. +const STATUS_LINE_WIDTH = 40; + +/** Receives byte counts as a download streams; `total` is 0 when unknown. */ +export type ProgressCallback = (received: number, total: number) => void; + +/** + * Builds a `\r`-rewriting status line for a download in progress, or `null` + * off-TTY — without `\r` rewriting, every tick would print its own line into + * redirected output. + */ +export function downloadProgressRenderer( + write: (s: string) => void, + isTTY: boolean, +): ProgressCallback | null { + if (!isTTY) return null; + let done = false; + return (received: number, total: number) => { + if (done) return; + if (total > 0 && received >= total) { + done = true; + const line = `Downloaded ${(total / MIB).toFixed(1)} MB (100%)`; + write(`\r${line.padEnd(STATUS_LINE_WIDTH)}\n`); + } else if (total > 0) { + const pct = Math.floor((received * 100) / total); + const line = `Downloading ${(received / MIB).toFixed(1)} / ${(total / MIB).toFixed(1)} MB (${pct}%)`; + write(`\r${line.padEnd(STATUS_LINE_WIDTH)}`); + } else { + const line = `Downloading ${(received / MIB).toFixed(1)} MB`; + write(`\r${line.padEnd(STATUS_LINE_WIDTH)}`); + } + }; +} + // ── Network + filesystem ────────────────────────────────────────────────────── function errMessage(err: unknown): string { @@ -97,21 +135,57 @@ function runningBinaryPath(): string | null { return exe; } -async function sha256(bytes: Uint8Array): Promise { - const hasher = new Bun.CryptoHasher('sha256'); - hasher.update(bytes); - return hasher.digest('hex'); -} +const PROGRESS_EMIT_BYTES = 512 * 1024; /** Download the asset for this platform, verify its checksum, replace `target`. */ -async function downloadAndReplace(tag: string, target: string): Promise { +async function downloadAndReplace( + tag: string, + target: string, + onProgress: ProgressCallback | null, +): Promise { const asset = assetForPlatform(process.platform, process.arch); if (!asset) throw new Error(`no prebuilt binary for ${process.platform}/${process.arch}`); const base = `https://github.com/${REPO}/releases/download/${tag}`; const binRes = await fetch(`${base}/${asset}`); if (!binRes.ok) throw new Error(`download failed (HTTP ${binRes.status}) for ${asset}`); - const bytes = new Uint8Array(await binRes.arrayBuffer()); + const total = Number(binRes.headers.get('content-length')) || 0; + if (!binRes.body) throw new Error('empty response body'); + + // Stage next to the target (same filesystem) then atomically rename over it. + // Overwriting a running executable is safe on Unix: the live process keeps the + // old inode until it exits. + const tmp = join(dirname(target), `.${basename(target)}.update-${process.pid}`); + + const hasher = new Bun.CryptoHasher('sha256'); + let received = 0; + let actual: string; + + const sink = Bun.file(tmp).writer(); + try { + let sinceLastEmit = 0; + // Stream chunk-wise instead of arrayBuffer(): the asset is tens of MB and + // hashing off the same chunks avoids a second read of the file. + for await (const chunk of binRes.body as AsyncIterable) { + sink.write(chunk); + hasher.update(chunk); + received += chunk.byteLength; + sinceLastEmit += chunk.byteLength; + if (sinceLastEmit >= PROGRESS_EMIT_BYTES) { + sinceLastEmit = 0; + onProgress?.(received, total); + } + } + await sink.end(); + // `|| received` covers unknown Content-Length: forces the renderer's + // done-branch so it terminates its line before the next console.log. + onProgress?.(received, total || received); + actual = hasher.digest('hex'); + } catch (err) { + try { await sink.end(); } catch { /* best effort */ } + try { unlinkSync(tmp); } catch { /* best effort */ } + throw err; + } // Verify the checksum when checksums.txt is reachable. A network failure // fetching the sums is non-fatal; a genuine mismatch aborts the update. @@ -119,23 +193,19 @@ async function downloadAndReplace(tag: string, target: string): Promise { const sumRes = await fetch(`${base}/checksums.txt`); if (sumRes.ok) { const expected = parseChecksums(await sumRes.text())[asset]; - if (expected) { - const actual = await sha256(bytes); - if (actual !== expected) { - throw new Error(`checksum mismatch for ${asset} (expected ${expected}, got ${actual})`); - } + if (expected && actual !== expected) { + throw new Error(`checksum mismatch for ${asset} (expected ${expected}, got ${actual})`); } } } catch (err) { - if (errMessage(err).includes('checksum mismatch')) throw err; - // otherwise: couldn't fetch sums — proceed without verification + // A mismatch aborts and cleans up the staging file; any other failure here + // (sums unreachable) is non-fatal — fall through and proceed unverified. + if (errMessage(err).includes('checksum mismatch')) { + try { unlinkSync(tmp); } catch { /* best effort */ } + throw err; + } } - // Stage next to the target (same filesystem) then atomically rename over it. - // Overwriting a running executable is safe on Unix: the live process keeps the - // old inode until it exits. - const tmp = join(dirname(target), `.${basename(target)}.update-${process.pid}`); - await Bun.write(tmp, bytes); chmodSync(tmp, 0o755); try { renameSync(tmp, target); @@ -199,10 +269,17 @@ export async function runUpdateCommand(opts: UpdateOptions): Promise { } } + let lineOpen = false; + const renderer = downloadProgressRenderer( + (s) => { lineOpen = !s.endsWith('\n'); process.stdout.write(s); }, + Boolean(process.stdout.isTTY), + ); + if (!renderer) console.log(`Downloading ignatius ${info.latest}…`); + try { - console.log(`Downloading ignatius ${info.latest}…`); - await downloadAndReplace(info.tag, target); + await downloadAndReplace(info.tag, target, renderer); } catch (err) { + if (lineOpen) process.stdout.write('\n'); const message = errMessage(err); if (/EACCES|EPERM|EROFS|permission|denied/i.test(message)) { process.stderr.write( diff --git a/test/checks/test-update-progress.ts b/test/checks/test-update-progress.ts new file mode 100644 index 0000000..83ad791 --- /dev/null +++ b/test/checks/test-update-progress.ts @@ -0,0 +1,71 @@ +/** + * test-update-progress.ts — unit tests for `downloadProgressRenderer`, the + * pure `\r`-rewriting status line behind `ignatius update`'s download step. + * + * No network, no TTY: `write` is captured into an array and `isTTY` is passed + * explicitly, so every branch (off-TTY, mid-stream, 100% latch, unknown total) + * is deterministic. + */ + +import { downloadProgressRenderer } from '../../src/cli/update'; + +function assert(cond: boolean, msg: string): asserts cond { + if (!cond) { + console.error('FAIL:', msg); + process.exit(1); + } +} + +// ── off-TTY ────────────────────────────────────────────────────────────────── + +{ + const lines: string[] = []; + const renderer = downloadProgressRenderer((s) => lines.push(s), false); + assert(renderer === null, 'off-TTY returns null'); + console.log('PASS: off-TTY returns null'); +} + +// ── mid-stream, known total ────────────────────────────────────────────────── + +{ + const lines: string[] = []; + const renderer = downloadProgressRenderer((s) => lines.push(s), true); + assert(renderer !== null, 'TTY returns a renderer'); + renderer!(5 * 1024 * 1024, 10 * 1024 * 1024); + assert(lines.length === 1, 'mid-stream writes exactly once'); + assert(lines[0]!.startsWith('\rDownloading 5.0 / 10.0 MB (50%)'), `mid-stream line: ${lines[0]}`); + assert(!lines[0]!.endsWith('\n'), 'mid-stream line has no trailing newline'); + console.log('PASS: mid-stream known total'); +} + +// ── final tick latches quiet ───────────────────────────────────────────────── + +{ + const lines: string[] = []; + const renderer = downloadProgressRenderer((s) => lines.push(s), true)!; + const total = 10 * 1024 * 1024; + renderer(total, total); + assert(lines.length === 1, 'final tick writes exactly once'); + assert(lines[0]!.includes('100%'), `final line has 100%: ${lines[0]}`); + assert(lines[0]!.endsWith('\n'), 'final line ends with newline'); + + renderer(total, total); + renderer(total + 1024, total); + assert(lines.length === 1, 'calls after 100% write nothing'); + console.log('PASS: final tick latches quiet'); +} + +// ── unknown total ───────────────────────────────────────────────────────────── + +{ + const lines: string[] = []; + const renderer = downloadProgressRenderer((s) => lines.push(s), true)!; + renderer(3 * 1024 * 1024, 0); + assert(lines.length === 1, 'unknown total writes exactly once'); + assert(lines[0]!.startsWith('\rDownloading 3.0 MB'), `unknown-total line: ${lines[0]}`); + assert(!lines[0]!.includes('%'), 'unknown-total line has no percent'); + assert(!lines[0]!.endsWith('\n'), 'unknown-total line has no trailing newline'); + console.log('PASS: unknown total'); +} + +console.log('\nAll update-progress assertions passed.');