fix(release): bound the crates.io publish phase by the token's real lifetime, not just the budget - #3383
Conversation
…emaining lifetime, not just the release-wide budget release-crates.mjs's PUBLISH_PHASE_BUDGET_MS clock started only inside publishAllCrates, so the second `pnpm build`, `test:esm`, and the entire npm publish that release.yml runs between minting CARGO_REGISTRY_TOKEN and reaching the crates loop were never charged against it. A large budget could therefore sail through even though the 30-minute token was already close to (or past) its claimed lifetime by the time the loop started. release.yml now stamps the mint time right after the OIDC exchange and passes it down as CRATES_TOKEN_MINTED_AT_MS. publishAllCrates takes the deadline as the earlier of its own budget and (mint time + 30 minutes minus a margin), so unmeasured work before the loop is charged against the token instead of escaping the bound. The env value is validated at the boundary in main() and refused outright rather than propagating a NaN deadline that would silently defeat every comparison downstream. Omitting the timestamp (a manual/local run) reproduces the previous budget-only behavior exactly. Refs #3258
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Heads-up on this PR's CI, because it will read green without having been tested. It targets It currently carries 13 checks, and none of them build or test the code: Missing: This is the same shape you documented on #3185 — the required aggregate is not red, it is absent, and So the CI signal here is worth nothing on its own. What the change actually has behind it is a local run, reported in full rather than summarised: RED was confirmed by reverting Treat that local run as the evidence, not the green ticks above. If you would rather this ran the full matrix, the options are to land |
Summary
#3258:
PUBLISH_PHASE_BUDGET_MSinscripts/release-crates.mjs(onrelfix/crates-poll-budget, #3188's line of work) starts its clock only insidepublishAllCrates. Everything between mintingCARGO_REGISTRY_TOKENand reaching that loop — a secondpnpm build,test:esm, and the entire npm publish with per-tarball OIDC provenance — was never charged against it, so a large release-wide budget could pass even when the 30-minute token was already close to (or past) its claimed lifetime by the time the crates loop started.The ordering, quoted
Mint (
.github/workflows/release.yml:213-215, unchanged):Unrelated work runs next, inside the changesets step (
release.yml:217-244,publish-script: pnpm run release), whichpackage.jsondefines as:and
scripts/release-all.mjsruns npm first, crates second.The clock previously started only here,
scripts/release-crates.mjs:— i.e. after the second build,
test:esm, and the whole npm publish had already run and consumed an unmeasured, unbounded amount of the 30-minute token.Worst-case gap
Unbounded before this change: a slow second build +
test:esm+ npm publish (many packages, provenance attached to each) could consume most or all of the token beforepublishAllCratesever took its first timestamp, and a budget sized only against the crates work would not notice.Fix
release.ymlnow stamps the mint time immediately after the OIDC exchange (Stamp crates.io token mint time, a singleDate.now()— no secret touched or logged) and passes it to the changesets step asCRATES_TOKEN_MINTED_AT_MS.publishAllCratestakes the deadline as whichever is earlier: its owntotalBudgetMs, ortokenMintedAtMs + 30min − a 60s margin. Time spent before the loop is now charged against the token's real remaining lifetime instead of escaping the bound.main()validates the env value at the boundary (parseTokenMintedAtMs) and throws on a non-numeric value rather than letting aNaNdeadline silently defeat every comparison downstream — the same failure shape flagged in the issue for an unvalidated numeric env input. Omitting the env var (a manual/local run) reproduces the exact previous budget-only behavior — pinned by a regression test.Could this be unit-tested? Yes.
The workflow ordering itself (mint → unrelated work → clock) is YAML and isn't unit-testable, so I read it directly and quoted it above. The arithmetic bug — the clock not accounting for the token's real remaining lifetime — lives entirely in
scripts/release-crates.mjs, which already hasscripts/release-crates.test.mjswith a fake-clock harness. Added there:parseTokenMintedAtMsboundary validation: unset →undefined, valid numeric string → parsed, non-numeric → throws (guards the NaN-hangs-forever shape)Confirmed RED against the pre-fix code: reverting
scripts/release-crates.mjsalone while keeping the new tests fails immediately (parseTokenMintedAtMsisn't exported yet), and restoring the fix turns the suite green again.Same-shape grep elsewhere
Searched
.github/workflows/andscripts/for another mint-then-unrelated-work-then-clock sequence. The only other OIDC/trusted-publish flow is PyPI (python-wheels.yml,pypa/gh-action-pypi-publish), which performs the OIDC exchange and the publish inside one self-contained action step — no separate script-side budget/poll loop of ours sits between a mint and a clock there, so this issue's family doesn't recur. No other occurrence found; nothing else changed.Base branch
This PR targets
relfix/crates-poll-budget(notmain) because the budget code #3258 describes was introduced there and hasn't merged yet —PUBLISH_PHASE_BUDGET_MSdoesn't exist onmain.Refs #3258