From 38b620de52a6c8dd13df2747f8617bab84f555f4 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Thu, 15 Oct 2026 12:56:12 +0800 Subject: [PATCH 1/2] fix(npm): wait for npm to apply each publish, and drop the one-time @gitlawb/zero steps npm applies a publish asynchronously: `npm publish` returns once the upload is accepted, and the version appears later. In the 0.10.0 release the @twigpine/zero wrapper appeared 9 seconds after publish-packages.sh stopped checking for it, and the last platform payload about ten minutes after its upload. Two things went wrong: - The wrapper became visible before four of its five platform payloads, so installs in that window resolved aliases that 404 and fell back to the GitHub-release download. Ordering the uploads does not order what npm shows. - The final `latest` check gave up after 25 seconds and failed the job even though every publish landed, which also skipped the @gitlawb/zero steps until a re-run. publish-packages.sh now waits until npm shows every platform version before it publishes the wrapper, then waits until `latest` is the wrapper version. One deadline bounds the waits (ZERO_NPM_PUBLISH_WAIT_TIMEOUT, default 30 minutes). On timeout it fails before the wrapper goes out and says what npm never showed. These lookups treat an error as "not yet", unlike the skip decision, which still stops on anything but E404. The test fake can now apply a publish only after a number of later lookups, and records which aliased versions were missing when the wrapper was published. Against the script as released in v0.10.0, the two new tests fail the way the release did: the job exits 1 although everything landed, and the wrapper goes out while its platform payload is not visible. The two release steps that published 0.10.0 once more as @gitlawb/zero have done their job and are removed; docs/NPM_PACKAGING.md and the comments that pointed at them now describe what happened. The Go and wrapper handling of the old name stays, since only a build installed under it can trigger it. --- .github/workflows/release-artifacts.yml | 53 ++---------------- docs/NPM_PACKAGING.md | 71 +++++++++---------------- internal/npmwrapper/publish_test.go | 67 +++++++++++++++++++++-- internal/update/installmethod.go | 10 ++-- scripts/npm/build-platform-packages.mjs | 9 ++-- scripts/npm/publish-packages.sh | 65 ++++++++++++++++------ 6 files changed, 153 insertions(+), 122 deletions(-) diff --git a/.github/workflows/release-artifacts.yml b/.github/workflows/release-artifacts.yml index c1ed0cc88..c5938dc3f 100644 --- a/.github/workflows/release-artifacts.yml +++ b/.github/workflows/release-artifacts.yml @@ -206,55 +206,10 @@ jobs: # Platform payloads first under the `platform` dist-tag, then the wrapper, # with the `latest` dist-tag checked before and after (platform versions # are semver PRERELEASES of the wrapper version and must never become - # `latest`). Versions already on the registry are skipped, so "Re-run - # failed jobs" is safe after a partial publish. + # `latest`). npm applies publishes asynchronously, so the script waits + # until npm shows every platform version before the wrapper goes out, + # and until `latest` moves after it. Versions already on the registry + # are skipped, so "Re-run failed jobs" is safe after a partial publish. - name: Publish npm packages shell: bash run: scripts/npm/publish-packages.sh dist/npm - - # One final release under the pre-rename name @gitlawb/zero. Zero 0.9.0's - # `zero upgrade` runs `npm install -g @gitlawb/zero@latest`, and people - # reinstall the old name from shell history and old tutorials, so this - # release is published once more under it: the same verified artifacts, - # assembled as @gitlawb/zero, which the binary and wrapper recognise and - # answer with the switch steps (docs/NPM_PACKAGING.md). Runs after the - # @twigpine/zero publish, so the package it points at already exists. - # - # The gate keeps this to a single release: it is owed only while the old - # name's latest is still the last pre-rename version, or is this version - # (a re-run after it went out). After this release, `npm deprecate` the - # old package and delete these two steps. - - name: Check whether the final @gitlawb/zero release is owed - id: legacy-name - shell: bash - env: - LAST_PRE_RENAME_VERSION: 0.9.0 - run: | - version="$(node -p "require('./package.json').version")" - latest="" - for _ in 1 2 3; do - latest="$(npm view @gitlawb/zero dist-tags.latest 2>/dev/null || true)" - if [ -n "$latest" ]; then - break - fi - sleep 10 - done - if [ -z "$latest" ]; then - echo "::error::could not read the latest dist-tag of @gitlawb/zero from the registry" >&2 - exit 1 - fi - if [ "$latest" = "$LAST_PRE_RENAME_VERSION" ] || [ "$latest" = "$version" ]; then - owed=true - else - owed=false - fi - echo "@gitlawb/zero latest is ${latest}; final release under the old name owed: ${owed}" - echo "owed=${owed}" >> "$GITHUB_OUTPUT" - - - name: Publish the final @gitlawb/zero release - if: ${{ steps.legacy-name.outputs.owed == 'true' }} - shell: bash - run: | - node scripts/npm/build-platform-packages.mjs \ - --artifacts-dir dist/artifacts --out-dir dist/npm-legacy --package-name @gitlawb/zero - scripts/npm/publish-packages.sh dist/npm-legacy diff --git a/docs/NPM_PACKAGING.md b/docs/NPM_PACKAGING.md index 50ddffa2d..199eaa18b 100644 --- a/docs/NPM_PACKAGING.md +++ b/docs/NPM_PACKAGING.md @@ -130,9 +130,14 @@ registry. bootstrap publish) — a missing value must not abort the job between the platform and wrapper publishes, which would strand a half-published release. -2. **Platform versions publish before the wrapper.** The wrapper's - `optionalDependencies` pin exact suffixed versions; publishing the wrapper - first would create a window where installs resolve aliases that 404. +2. **Platform versions are on the registry before the wrapper.** The + wrapper's `optionalDependencies` pin exact suffixed versions; publishing + the wrapper first would create a window where installs resolve aliases + that 404. Ordering the uploads is not enough: npm applies a publish + asynchronously, and in 0.10.0 a platform version appeared ten minutes + after its upload returned, after the wrapper. The publish script waits + until npm shows every platform version before it publishes the wrapper, + and waits the same way for `latest` afterwards (up to 30 minutes in all). 3. **Exact-version pinning.** The wrapper at `X.Y.Z` references platform versions `X.Y.Z--` exactly — never ranges — so a wrapper and its binaries can never skew. The publish script refuses to start when @@ -161,54 +166,30 @@ bin, so npm refuses to install one over the other (`EEXIST`), and forcing it lets a later uninstall of the old package delete the shared bin. The user has to uninstall the old package and install the new one, in that order. -Deprecating the old package alone does not reach existing users. 0.9.0's -`zero upgrade` runs `npm install -g @gitlawb/zero@latest`; with nothing newer -under that name, npm reinstalls nothing, prints no deprecation warning (it -only warns when it installs something), and Zero reports the update as done. -So 0.10.0 is published once more under the old name, as a bridge: - -- The publish job assembles the same verified artifacts a second time with - `build-platform-packages.mjs --package-name @gitlawb/zero` and publishes - them with the same script, after the `@twigpine/zero` publish. A gate runs - it only while the old name's `latest` is still 0.9.0 (or is this version, - on a re-run), so it cannot happen twice. +Deprecating the old package alone would not have reached existing users. +0.9.0's `zero upgrade` runs `npm install -g @gitlawb/zero@latest`; with +nothing newer under that name, npm reinstalls nothing, prints no deprecation +warning (it only warns when it installs something), and Zero reports the +update as done. So 0.10.0 was published once more under the old name, as a +bridge: + +- The release job assembled the same verified artifacts a second time with + `build-platform-packages.mjs --package-name @gitlawb/zero` and published + them with the same script, after the `@twigpine/zero` publish. The steps + that did this ran once and have been removed from `release-artifacts.yml`. - The bridge build knows its name from the `package.json` beside it. The Go binary reports it as `InstallMethodNpmLegacy`: `zero upgrade` refuses with the switch steps instead of reinstalling it, and `zero update --check` adds them. `bin/zero.js` prints the same steps after `zero` exits, on a - terminal only. -- Updating from 0.9.0 now installs a real version, so npm also shows the + terminal only. This handling stays in the code, since only a build + installed under the old name ever triggers it. +- Updating from 0.9.0 installs that bridge build, so npm also shows the deprecation message. -### Runbook - -Before the first release under the new name: - -1. Create the package, since npm only attaches a trusted publisher to a - package that exists: publish an empty `@twigpine/zero@0.0.0` by hand with - `npm publish --access public --tag bootstrap`, then - `npm deprecate @twigpine/zero@0.0.0 "Placeholder; install @twigpine/zero@latest"`. -2. Trust this workflow for it: - `npm trust github @twigpine/zero --repo Twigpine/zero --file release-artifacts.yml --env npm-publish --allow-publish`. -3. Point the old package's trusted publisher at this repository too, for the - bridge publish: `npm trust list @gitlawb/zero`, revoke the existing entry - with `npm trust revoke @gitlawb/zero --id `, then run the same - `npm trust github` command for `@gitlawb/zero`. -4. Confirm Settings → Environments → `npm-publish` has a required reviewer. - The environment exists either way; without a reviewer the job does not - wait for approval. - -After the release, once both packages show 0.10.0 as `latest`: - -1. `npm deprecate @gitlawb/zero "Zero moved to @twigpine/zero. Switch with: npm uninstall -g @gitlawb/zero && npm install -g @twigpine/zero"` - (no version range, so every version, including the platform payloads, - carries it). Never unpublish: existing installs resolve their platform - payload through these versions. -2. Revoke the old package's trusted publisher (`npm trust revoke`), so - nothing can publish under that name again. -3. Delete the two `@gitlawb/zero` steps from `release-artifacts.yml`. The Go - and wrapper handling of the old name can stay; it only activates in a - build installed under that name. +`@gitlawb/zero` is deprecated on every version and has no trusted publisher, +so nothing can publish under it again. Its versions stay on the registry: +existing installs resolve their platform payload through them, so they must +never be unpublished. ## Runbook: bumping the vendored helpers diff --git a/internal/npmwrapper/publish_test.go b/internal/npmwrapper/publish_test.go index af0e2fc39..55725edb9 100644 --- a/internal/npmwrapper/publish_test.go +++ b/internal/npmwrapper/publish_test.go @@ -23,6 +23,10 @@ import ( // FAKE_NPM_CLOBBER_LATEST=1 makes every publish take `latest`, the failure the // script must stop on. FAKE_NPM_VERSION_LOOKUP_ERRORS= fails the first n // version lookups with a network error instead of answering them. +// FAKE_NPM_APPLY_AFTER= applies each publish only after n further lookups, +// the way npm accepts an upload and shows the version minutes later; until +// then the version and its dist-tag are invisible. When a wrapper is published, +// the aliased versions not yet visible are recorded in missingAtWrapperPublish. const fakeNpm = `#!/usr/bin/env node const fs = require('fs'); const stateFile = process.env.FAKE_NPM_STATE; @@ -31,7 +35,17 @@ const args = process.argv.slice(2); fs.appendFileSync(stateFile + '.log', JSON.stringify({ args, cwd: process.cwd() }) + '\n'); const save = () => fs.writeFileSync(stateFile, JSON.stringify(state)); const notFound = (detail) => { process.stderr.write('npm error code E404\nnpm error 404 ' + detail + '\n'); process.exit(1); }; +state.pending = state.pending || []; +const apply = (publish) => { + state.versions.push(publish.version); + state.distTags[publish.tag] = publish.version; + if (process.env.FAKE_NPM_CLOBBER_LATEST === '1') state.distTags.latest = publish.version; +}; if (args[0] === 'view') { + for (const publish of state.pending) publish.wait -= 1; + state.pending.filter((publish) => publish.wait <= 0).forEach(apply); + state.pending = state.pending.filter((publish) => publish.wait > 0); + save(); const [spec, field] = args.slice(1); const at = spec.lastIndexOf('@'); const name = at > 0 ? spec.slice(0, at) : spec; @@ -62,14 +76,19 @@ if (args[0] === 'publish') { process.stderr.write('fake npm: refusing to publish ' + pkg.name + '\n'); process.exit(1); } - if (state.versions.includes(pkg.version)) { + if (state.versions.includes(pkg.version) || state.pending.some((publish) => publish.version === pkg.version)) { process.stderr.write('npm error code EPUBLISHCONFLICT\n'); process.exit(1); } + if (pkg.optionalDependencies) { + state.missingAtWrapperPublish = Object.values(pkg.optionalDependencies) + .map((spec) => spec.slice(spec.lastIndexOf('@') + 1)) + .filter((version) => !state.versions.includes(version)); + } const tagIndex = args.indexOf('--tag'); - state.versions.push(pkg.version); - state.distTags[tagIndex === -1 ? 'latest' : args[tagIndex + 1]] = pkg.version; - if (process.env.FAKE_NPM_CLOBBER_LATEST === '1') state.distTags.latest = pkg.version; + const publish = { version: pkg.version, tag: tagIndex === -1 ? 'latest' : args[tagIndex + 1], wait: Number(process.env.FAKE_NPM_APPLY_AFTER || 0) }; + if (publish.wait > 0) state.pending.push(publish); + else apply(publish); save(); process.exit(0); } @@ -88,6 +107,9 @@ type fakeRegistryState struct { DistTags map[string]string `json:"distTags"` // VersionLookups counts `npm view @ version` calls. VersionLookups int `json:"versionLookups,omitempty"` + // MissingAtWrapperPublish lists the platform versions the wrapper aliased + // that npm did not show yet when the wrapper was published. + MissingAtWrapperPublish []string `json:"missingAtWrapperPublish,omitempty"` } type npmCall struct { @@ -325,3 +347,40 @@ func TestPublishPackagesFailsWhenTheRegistryCannotBeReached(t *testing.T) { t.Fatalf("stderr does not explain the failure or show npm's error: %s", run.stderr) } } + +// npm accepts an upload and shows the version minutes later (ten minutes for a +// 0.10.0 platform payload). Publishing the wrapper as soon as the platform +// uploads return would leave installs resolving aliases that 404, and checking +// `latest` right after the wrapper upload reports a failure that is not one. +func TestPublishPackagesWaitsForNpmToApplyEachPublish(t *testing.T) { + platforms := []string{"darwin-arm64", "linux-x64"} + npmDir := writePublishFixture(t, platforms, platforms) + run := runPublishPackages(t, npmDir, fakeRegistryState{Name: publishFixtureName}, "FAKE_NPM_APPLY_AFTER=8") + if run.exitCode != 0 { + t.Fatalf("exit = %d while npm applied the publishes late; stdout=%s stderr=%s", run.exitCode, run.stdout, run.stderr) + } + if len(run.state.MissingAtWrapperPublish) != 0 { + t.Fatalf("the wrapper went out while npm did not show %v yet; installs would resolve aliases that 404", run.state.MissingAtWrapperPublish) + } + if latest := run.state.DistTags["latest"]; latest != publishFixtureVersion { + t.Fatalf("latest = %q, want %q", latest, publishFixtureVersion) + } +} + +// A publish npm never applies must fail the job at the deadline, before the +// wrapper goes out, instead of waiting forever. +func TestPublishPackagesGivesUpWhenNpmNeverAppliesAPublish(t *testing.T) { + platforms := []string{"linux-x64"} + npmDir := writePublishFixture(t, platforms, platforms) + run := runPublishPackages(t, npmDir, fakeRegistryState{Name: publishFixtureName}, + "FAKE_NPM_APPLY_AFTER=1000000", "ZERO_NPM_PUBLISH_WAIT_TIMEOUT=1") + if run.exitCode == 0 { + t.Fatalf("exit = 0 although npm never showed the platform version; stdout=%s", run.stdout) + } + if got := strings.Join(run.publishedDirs(), ","); got != "zero-linux-x64" { + t.Fatalf("published %s, want only the platform payload (the wrapper must wait for it)", got) + } + if !strings.Contains(run.stderr, "still does not show "+publishFixtureName+"@"+publishFixtureVersion+"-linux-x64") { + t.Fatalf("stderr does not say what npm never showed: %s", run.stderr) + } +} diff --git a/internal/update/installmethod.go b/internal/update/installmethod.go index d1e738c6b..1c6c0575c 100644 --- a/internal/update/installmethod.go +++ b/internal/update/installmethod.go @@ -17,11 +17,11 @@ import ( const npmPackageName = "@twigpine/zero" // legacyNpmPackageName is the name zero was published under on npm up to -// 0.9.0. The 0.10.0 release is published once more under it (the legacy step -// in .github/workflows/release-artifacts.yml), so that 0.9.0's `zero upgrade` -// and anyone still installing the old name land on a build that says where -// the package went. Nothing is published under it after that, which is why an -// install of it is told to switch instead of offered an update. +// 0.9.0. The 0.10.0 release was published once more under it, so that 0.9.0's +// `zero upgrade` and anyone still installing the old name land on a build that +// says where the package went (docs/NPM_PACKAGING.md). Nothing is published +// under it after that, which is why an install of it is told to switch instead +// of offered an update. const legacyNpmPackageName = "@gitlawb/zero" // InstallMethod identifies how the running zero binary was installed. diff --git a/scripts/npm/build-platform-packages.mjs b/scripts/npm/build-platform-packages.mjs index d2cb34068..60d7e93be 100644 --- a/scripts/npm/build-platform-packages.mjs +++ b/scripts/npm/build-platform-packages.mjs @@ -25,10 +25,11 @@ // CI must always assemble the full matrix, and the wrapper's // optionalDependencies always reference the full matrix regardless). // -// --package-name assembles the same artifacts under another name. It exists -// for the one final release under the pre-rename name @gitlawb/zero (see -// docs/NPM_PACKAGING.md); everything in the payloads that names the package -// follows it, so the binary and wrapper know which name they were installed as. +// --package-name assembles the same artifacts under another name. It was +// added for the one release (0.10.0) also published under the pre-rename name +// @gitlawb/zero (see docs/NPM_PACKAGING.md); everything in the payloads that +// names the package follows it, so the binary and wrapper know which name they +// were installed as. // // Usage: // node scripts/npm/build-platform-packages.mjs \ diff --git a/scripts/npm/publish-packages.sh b/scripts/npm/publish-packages.sh index 84c88e4db..04fa41f46 100755 --- a/scripts/npm/publish-packages.sh +++ b/scripts/npm/publish-packages.sh @@ -4,8 +4,15 @@ # with the dist-tags docs/NPM_PACKAGING.md requires: # # 1. every platform payload, under the `platform` dist-tag, never `latest`; -# 2. check that `latest` was not moved onto a platform version; -# 3. the wrapper, which takes `latest`, then confirm `latest` is its version. +# 2. wait until npm shows every platform version, then check that `latest` +# was not moved onto one; +# 3. the wrapper, which takes `latest`, then wait until `latest` is its +# version. +# +# The waits exist because npm applies a publish asynchronously: `npm publish` +# returns once the upload is accepted, and the version can appear minutes +# later (ten minutes for a platform payload of 0.10.0). Ordering the uploads +# alone would still let the wrapper appear first, with aliases that 404. # # The package name comes from the assembled wrapper's package.json, so the # same script publishes whatever name the payloads were built under. @@ -19,7 +26,9 @@ # Usage: scripts/npm/publish-packages.sh # # Auth comes from the environment (npm OIDC trusted publishing in CI). -# ZERO_NPM_PUBLISH_RETRY_DELAY overrides the seconds between dist-tag lookups. +# ZERO_NPM_PUBLISH_RETRY_DELAY overrides the seconds between registry lookups, +# and ZERO_NPM_PUBLISH_WAIT_TIMEOUT the seconds to wait for npm to apply the +# publishes (default 1800). set -euo pipefail fail() { @@ -40,6 +49,7 @@ name="$(manifest_field "$wrapper_dir" name)" version="$(manifest_field "$wrapper_dir" version)" [ -n "$name" ] && [ -n "$version" ] || fail "${wrapper_dir}/package.json has no name or version" delay="${ZERO_NPM_PUBLISH_RETRY_DELAY:-5}" +wait_timeout="${ZERO_NPM_PUBLISH_WAIT_TIMEOUT:-1800}" # The wrapper pins every platform version exactly, so a wrapper whose aliases # do not match the payloads beside it would install aliases that 404. Check @@ -105,16 +115,49 @@ publish() { (cd "$dir" && npm publish --access public --ignore-scripts "$@") } +# Unlike already_published, these lookups may fail without stopping the job: +# while npm has not applied a publish, an error or an E404 both mean "not +# yet", and the deadline bounds the wait. +version_visible() { + [ "$(npm view "${name}@$1" version 2>/dev/null || true)" = "$1" ] +} + +latest_is() { + [ "$(npm view "$name" dist-tags.latest 2>/dev/null || true)" = "$1" ] +} + +# One deadline covers every wait in this run, so the job cannot wait +# wait_timeout once per version. +deadline=$((SECONDS + wait_timeout)) + +# wait_until +wait_until() { + local what="$1" + shift + until "$@"; do + if [ "$SECONDS" -ge "$deadline" ]; then + fail "npm accepted the upload but still does not show ${what} after ${wait_timeout}s. Re-run once it does: versions already on the registry are skipped." + fi + sleep "$delay" + done +} + for dir in "${platform_dirs[@]}"; do dir="${dir%/}" publish "$dir" "$(manifest_field "$dir" version)" --tag platform done +for dir in "${platform_dirs[@]}"; do + platform_version="$(manifest_field "${dir%/}" version)" + wait_until "${name}@${platform_version}" version_visible "$platform_version" + echo "${name}@${platform_version} is on the registry." +done + # Platform versions are semver prereleases of the wrapper version, so a # platform publish that took `latest` would make a plain install deliver a # payload with no bin. Stop before the wrapper goes out. A failed lookup is -# tolerated (registry lag, or no `latest` yet on a package's first release): -# failing here would strand the platform payloads without their wrapper. +# tolerated (no `latest` yet on a package's first release): failing here would +# strand the platform payloads without their wrapper. latest="$(npm view "$name" dist-tags.latest || true)" case "$latest" in "${version}-"*) @@ -125,13 +168,5 @@ echo "dist-tag latest is ${latest:-unset} (unchanged by the platform publishes). publish "$wrapper_dir" "$version" -# Registry metadata can lag a publish by a few seconds. -for _ in 1 2 3 4 5; do - latest="$(npm view "$name" dist-tags.latest || true)" - if [ "$latest" = "$version" ]; then - echo "dist-tag latest is ${version}." - exit 0 - fi - sleep "$delay" -done -fail "dist-tag latest of ${name} is ${latest:-unset}, expected ${version} after the wrapper publish." +wait_until "${name}@${version} as the latest dist-tag" latest_is "$version" +echo "dist-tag latest is ${version}." From 26b779b9d715074690d87c8cbd0e331320c05a5f Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Thu, 15 Oct 2026 13:19:32 +0800 Subject: [PATCH 2/2] fix(npm): validate the publish wait settings, and correct the legacy publishing claim ZERO_NPM_PUBLISH_WAIT_TIMEOUT went into Bash arithmetic as typed. A leading zero reads as octal (0600 waited 384 seconds), and 08 is an arithmetic error that, in a script, does not stop it: Bash prints the error, leaves the deadline unset and carries on, so the run only died at its first wait, after the platform payloads were published. A negative timeout ended the wait before it started, and a bad ZERO_NPM_PUBLISH_RETRY_DELAY only failed at the first sleep, also after publishing began. Both settings are now checked before anything is published: whole seconds only, converted with 10# so a leading zero is decimal, bounded before the conversion, and capped (timeout 86400, delay 600). A bad value fails with the variable's name. TestPublishPackagesReadsTheWaitTimeoutAsDecimal makes the script wait, which is where the old code broke: against the reviewed script it publishes the platform payload and then exits 1 on "08: value too great for base". TestPublishPackagesRejectsAnInvalidWaitSetting covers -5, abc, 1e3, an over-range and an overflowing timeout, and a non-numeric delay; the old script published with five of them and crashed on the sixth with "abc: unbound variable". docs/NPM_PACKAGING.md said nothing could publish @gitlawb/zero again. Removing the trusted publisher stops this workflow; a maintainer could still publish by hand, and the doc now says that. --- docs/NPM_PACKAGING.md | 9 +++--- internal/npmwrapper/publish_test.go | 47 +++++++++++++++++++++++++++++ scripts/npm/publish-packages.sh | 20 ++++++++++-- 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/docs/NPM_PACKAGING.md b/docs/NPM_PACKAGING.md index 199eaa18b..bb5846b87 100644 --- a/docs/NPM_PACKAGING.md +++ b/docs/NPM_PACKAGING.md @@ -186,10 +186,11 @@ bridge: - Updating from 0.9.0 installs that bridge build, so npm also shows the deprecation message. -`@gitlawb/zero` is deprecated on every version and has no trusted publisher, -so nothing can publish under it again. Its versions stay on the registry: -existing installs resolve their platform payload through them, so they must -never be unpublished. +`@gitlawb/zero` is deprecated on every version and no longer has a trusted +publisher, so this release workflow cannot publish it again; another version +would take a maintainer publishing by hand. Its versions stay on the +registry: existing installs resolve their platform payload through them, so +they must never be unpublished. ## Runbook: bumping the vendored helpers diff --git a/internal/npmwrapper/publish_test.go b/internal/npmwrapper/publish_test.go index 55725edb9..ced669fb5 100644 --- a/internal/npmwrapper/publish_test.go +++ b/internal/npmwrapper/publish_test.go @@ -384,3 +384,50 @@ func TestPublishPackagesGivesUpWhenNpmNeverAppliesAPublish(t *testing.T) { t.Fatalf("stderr does not say what npm never showed: %s", run.stderr) } } + +// Bash arithmetic reads a leading zero as octal and rejects 08 outright, so +// the timeout has to be converted explicitly rather than used as typed. The +// rejection does not stop a script: it prints an error, leaves the deadline +// unset and carries on, so the failure only shows once the script has to wait, +// after the platform payloads are out. npm applying late makes it wait here. +func TestPublishPackagesReadsTheWaitTimeoutAsDecimal(t *testing.T) { + platforms := []string{"linux-x64"} + npmDir := writePublishFixture(t, platforms, platforms) + run := runPublishPackages(t, npmDir, fakeRegistryState{Name: publishFixtureName}, + "ZERO_NPM_PUBLISH_WAIT_TIMEOUT=08", "FAKE_NPM_APPLY_AFTER=3") + if run.exitCode != 0 { + t.Fatalf("exit = %d with ZERO_NPM_PUBLISH_WAIT_TIMEOUT=08; stdout=%s stderr=%s", run.exitCode, run.stdout, run.stderr) + } + if got := strings.Join(run.publishedDirs(), ","); got != "zero-linux-x64,wrapper" { + t.Fatalf("published %s, want zero-linux-x64,wrapper", got) + } +} + +// A setting the script cannot use must stop it before anything is published: +// a negative timeout would otherwise end the wait before it starts, and a bad +// delay would only fail at the first sleep, after the platform payloads. +func TestPublishPackagesRejectsAnInvalidWaitSetting(t *testing.T) { + for _, tc := range []struct{ variable, value string }{ + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "-5"}, + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "abc"}, + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "1e3"}, + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "86401"}, + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "9999999999999999999999"}, + {"ZERO_NPM_PUBLISH_RETRY_DELAY", "soon"}, + } { + t.Run(tc.variable+"="+tc.value, func(t *testing.T) { + platforms := []string{"linux-x64"} + npmDir := writePublishFixture(t, platforms, platforms) + run := runPublishPackages(t, npmDir, fakeRegistryState{Name: publishFixtureName}, tc.variable+"="+tc.value) + if run.exitCode == 0 { + t.Fatalf("exit = 0 with %s=%s; stdout=%s", tc.variable, tc.value, run.stdout) + } + if len(run.publishes) != 0 { + t.Fatalf("published %v before rejecting %s=%s, want nothing", run.publishedDirs(), tc.variable, tc.value) + } + if !strings.Contains(run.stderr, tc.variable) { + t.Fatalf("stderr does not name %s: %s", tc.variable, run.stderr) + } + }) + } +} diff --git a/scripts/npm/publish-packages.sh b/scripts/npm/publish-packages.sh index 04fa41f46..338b153b5 100755 --- a/scripts/npm/publish-packages.sh +++ b/scripts/npm/publish-packages.sh @@ -48,8 +48,24 @@ manifest_field() { name="$(manifest_field "$wrapper_dir" name)" version="$(manifest_field "$wrapper_dir" version)" [ -n "$name" ] && [ -n "$version" ] || fail "${wrapper_dir}/package.json has no name or version" -delay="${ZERO_NPM_PUBLISH_RETRY_DELAY:-5}" -wait_timeout="${ZERO_NPM_PUBLISH_WAIT_TIMEOUT:-1800}" +# whole_seconds : the value as a decimal number of +# seconds. Checked here, before anything is published: Bash arithmetic reads a +# leading zero as octal (0600 would wait 384 seconds, and 08 is an error), a +# negative timeout would end the wait before it starts, and a bad delay would +# only fail at the first sleep, possibly after the platform payloads went out. +whole_seconds() { + case "$2" in + '' | *[!0-9]*) fail "$1 must be a whole number of seconds, got '$2'" ;; + esac + # Bounded before the conversion so it cannot overflow. + [ "${#2}" -le 18 ] || fail "$1 must be at most $3 seconds, got '$2'" + local seconds=$((10#$2)) + [ "$seconds" -le "$3" ] || fail "$1 must be at most $3 seconds, got '$2'" + echo "$seconds" +} + +delay="$(whole_seconds ZERO_NPM_PUBLISH_RETRY_DELAY "${ZERO_NPM_PUBLISH_RETRY_DELAY:-5}" 600)" +wait_timeout="$(whole_seconds ZERO_NPM_PUBLISH_WAIT_TIMEOUT "${ZERO_NPM_PUBLISH_WAIT_TIMEOUT:-1800}" 86400)" # The wrapper pins every platform version exactly, so a wrapper whose aliases # do not match the payloads beside it would install aliases that 404. Check