fix(release): state the propagation budget as a duration, not an attempt count (META-158) - #45
Open
qmarcelle wants to merge 2 commits into
Open
fix(release): state the propagation budget as a duration, not an attempt count (META-158)#45qmarcelle wants to merge 2 commits into
qmarcelle wants to merge 2 commits into
Conversation
…mpt count (META-158)
The 0.5.0 release published both packages successfully at 00:05:31 and then
failed its own receipt at 00:06:48. Re-running the identical script against the
identical registry minutes later passed. The release was real; the receipt said
otherwise, which is precisely the false-red META-158 describes.
The cause is a comment and a constant disagreeing. The comment says propagation
lags "seconds to low minutes". The configuration was six attempts with a linear
five-second step — seventy-five seconds. Nobody multiplies that out while
reading, so the claim and the behavior drifted apart unnoticed until a release
landed in the gap.
The budget is now a duration: ten minutes, with capped backoff so the tail of a
long wait stays responsive. A duration sits next to the sentence that describes
it and cannot silently contradict it.
On exhaustion the script no longer just exits. It says which of the two possible
events this is, because they need opposite responses:
version present on the registry -> the package shipped, only the receipt is
missing; re-run this script
version absent -> the publish did not land; re-cut
A failed lookup is not proof of a failed publish, and the old message let a
reader infer that it was.
Scripts are not in the packages' "files" lists and do not ship, so this carries
no changeset and does not move the version.
Reviewer's GuideSwitches registry propagation handling in the release verification script from a fixed retry count to a time-budget-driven loop, enhances logging to expose elapsed time and guidance when propagation appears exhausted, and caps backoff delays to keep the tail of long waits responsive. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
qmarcelle has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider using a monotonic clock (e.g.,
process.hrtime.bigint()orperformance.now()) instead ofDate.now()for elapsed time calculations so the propagation budget isn’t affected by system clock adjustments. - The retry/backoff constants and logic are now more complex; consider extracting them into a small helper (e.g.,
withRegistryPropagationBudget(...)) so the core verification flow reads more linearly and the policy is easier to reuse and adjust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using a monotonic clock (e.g., `process.hrtime.bigint()` or `performance.now()`) instead of `Date.now()` for elapsed time calculations so the propagation budget isn’t affected by system clock adjustments.
- The retry/backoff constants and logic are now more complex; consider extracting them into a small helper (e.g., `withRegistryPropagationBudget(...)`) so the core verification flow reads more linearly and the policy is easier to reuse and adjust.
## Individual Comments
### Comment 1
<location path="scripts/verify-published.mjs" line_range="136-141" />
<code_context>
- const delayMs = REGISTRY_PROPAGATION_BASE_DELAY_MS * attempt;
- console.log(`${pkg.name}@${version} not yet visible on the registry (attempt ${attempt}/${REGISTRY_PROPAGATION_RETRIES}) — retrying in ${delayMs}ms`);
+
+ const delayMs = Math.min(REGISTRY_PROPAGATION_BASE_DELAY_MS * attempt, REGISTRY_PROPAGATION_MAX_DELAY_MS);
+ console.log(
+ `${pkg.name}@${version} not yet visible on the registry ` +
+ `(attempt ${attempt}, ${seconds(elapsedMs)} of ${seconds(REGISTRY_PROPAGATION_BUDGET_MS)} elapsed) — retrying in ${delayMs}ms`,
+ );
await new Promise((resolve) => setTimeout(resolve, delayMs));
}
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Retry delay selection can overshoot the intended propagation budget by up to the max delay.
Since `exhausted` is checked before the sleep and `delayMs` is capped independently, the loop can run past `REGISTRY_PROPAGATION_BUDGET_MS` by up to `REGISTRY_PROPAGATION_MAX_DELAY_MS`. If the budget is meant to be a hard limit, clamp `delayMs` to `REGISTRY_PROPAGATION_BUDGET_MS - elapsedMs`, and skip the retry when that computed delay is non‑positive so the final sleep cannot exceed the declared budget.
```suggestion
const remainingMs = REGISTRY_PROPAGATION_BUDGET_MS - elapsedMs;
const delayMs = Math.min(
REGISTRY_PROPAGATION_BASE_DELAY_MS * attempt,
REGISTRY_PROPAGATION_MAX_DELAY_MS,
remainingMs,
);
if (delayMs <= 0) {
console.log(
`${pkg.name}@${version} not yet visible on the registry ` +
`(attempt ${attempt}, ${seconds(elapsedMs)} of ${seconds(REGISTRY_PROPAGATION_BUDGET_MS)} elapsed) — propagation budget exhausted, giving up.`,
);
break;
}
console.log(
`${pkg.name}@${version} not yet visible on the registry ` +
`(attempt ${attempt}, ${seconds(elapsedMs)} of ${seconds(REGISTRY_PROPAGATION_BUDGET_MS)} elapsed) — retrying in ${delayMs}ms`,
);
await new Promise((resolve) => setTimeout(resolve, delayMs));
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+136
to
141
| const delayMs = Math.min(REGISTRY_PROPAGATION_BASE_DELAY_MS * attempt, REGISTRY_PROPAGATION_MAX_DELAY_MS); | ||
| console.log( | ||
| `${pkg.name}@${version} not yet visible on the registry ` + | ||
| `(attempt ${attempt}, ${seconds(elapsedMs)} of ${seconds(REGISTRY_PROPAGATION_BUDGET_MS)} elapsed) — retrying in ${delayMs}ms`, | ||
| ); | ||
| await new Promise((resolve) => setTimeout(resolve, delayMs)); |
There was a problem hiding this comment.
suggestion (bug_risk): Retry delay selection can overshoot the intended propagation budget by up to the max delay.
Since exhausted is checked before the sleep and delayMs is capped independently, the loop can run past REGISTRY_PROPAGATION_BUDGET_MS by up to REGISTRY_PROPAGATION_MAX_DELAY_MS. If the budget is meant to be a hard limit, clamp delayMs to REGISTRY_PROPAGATION_BUDGET_MS - elapsedMs, and skip the retry when that computed delay is non‑positive so the final sleep cannot exceed the declared budget.
Suggested change
| const delayMs = Math.min(REGISTRY_PROPAGATION_BASE_DELAY_MS * attempt, REGISTRY_PROPAGATION_MAX_DELAY_MS); | |
| console.log( | |
| `${pkg.name}@${version} not yet visible on the registry ` + | |
| `(attempt ${attempt}, ${seconds(elapsedMs)} of ${seconds(REGISTRY_PROPAGATION_BUDGET_MS)} elapsed) — retrying in ${delayMs}ms`, | |
| ); | |
| await new Promise((resolve) => setTimeout(resolve, delayMs)); | |
| const remainingMs = REGISTRY_PROPAGATION_BUDGET_MS - elapsedMs; | |
| const delayMs = Math.min( | |
| REGISTRY_PROPAGATION_BASE_DELAY_MS * attempt, | |
| REGISTRY_PROPAGATION_MAX_DELAY_MS, | |
| remainingMs, | |
| ); | |
| if (delayMs <= 0) { | |
| console.log( | |
| `${pkg.name}@${version} not yet visible on the registry ` + | |
| `(attempt ${attempt}, ${seconds(elapsedMs)} of ${seconds(REGISTRY_PROPAGATION_BUDGET_MS)} elapsed) — propagation budget exhausted, giving up.`, | |
| ); | |
| break; | |
| } | |
| console.log( | |
| `${pkg.name}@${version} not yet visible on the registry ` + | |
| `(attempt ${attempt}, ${seconds(elapsedMs)} of ${seconds(REGISTRY_PROPAGATION_BUDGET_MS)} elapsed) — retrying in ${delayMs}ms`, | |
| ); | |
| await new Promise((resolve) => setTimeout(resolve, delayMs)); |
Evidence packet for the M2A causal-review proof experiment: - 3 scenarios (billfold, integrations, syncpack) x 3 arms x 3 runs = 27 accepted runs - 4 degraded-evidence conditions x 3 runs = 12 accepted runs - 27 preflight/rejected runs (evidence injection bug, labeled, not counted) - Protocol scripts, registered diffs, and evidence payloads - MANIFEST.json with SHA-256 for every file - REDACTIONS.md documenting all redactions (local paths only, no credentials) - RECEIPT.md with corrected bounded result Result: 1/3 scenarios PASS, 2/3 FAIL, 4/4 degraded controls PASS. Disposition: NARROW (META-372).
|
Too many files changed for review (283 files, 100 file limit). Bypass the limit by tagging |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
META-158 with a reproduction and numbers. The 0.5.0 release is the case it describes.
What happened
Both packages were on the registry. The same script, unchanged, against the same registry, passed minutes later. The release was real; the receipt said otherwise.
The cause is a comment and a constant disagreeing
The comment above the constants says propagation lags "seconds to low minutes." The configuration was:
Which is
5 + 10 + 15 + 20 + 25= 75 seconds. Nobody multiplies that out while reading, so the claim and the behavior drifted apart unnoticed until a release landed in the gap.The fix
The budget is now a duration, not an attempt count:
A duration sits next to the sentence describing it and cannot silently contradict it. An attempt count has to be mentally expanded before it can be compared to the claim, which is how this drifted.
Progress lines now carry elapsed time against the budget, so a future failure arrives with its own evidence:
Exhaustion now says which event this is
The old message exited on an
ETARGETand left the reader to infer a failed publish from a failed lookup. Those are different events needing opposite responses:The script prints exactly that, with the
npm viewcommand to settle it.Verified
99.99.99, a version that will never exist, and killed at 25s. Backoff, elapsed accounting and budget reporting all behave (output above is from that run).check:docsandcheck:architecturepass.Scripts are not in either package's
fileslist and do not ship, so this carries no changeset and does not move the version.Not addressed here
The budget is generous rather than tuned — 10 minutes was chosen because a published version with no receipt is the one state this pipeline cannot recover from, so waiting too long is the cheaper error. The actual propagation time for the 0.5.0 publish is unknown: it was over 75 seconds and under "several minutes later when I re-ran it by hand." Tightening it needs a measurement this release did not produce.
Summary by Sourcery
Improve release verification reliability and preserve the associated causal-review experiment evidence.
Bug Fixes:
Enhancements:
Documentation: