fix(ci): retry staging-tmp dist-tag removal after npm publish#28534
fix(ci): retry staging-tmp dist-tag removal after npm publish#28534IshankDev wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses intermittent CI failures during the nightly release process caused by npm registry propagation delays. By implementing a retry mechanism for the removal of 'staging-tmp' dist-tags, the release pipeline is now more resilient to transient errors that occur when attempting to clean up tags immediately after a package publish. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/L
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new script remove-npm-dist-tag.js (along with unit tests) to remove npm dist-tags with a retry mechanism, resolving issues where npm/Wombat acknowledges a publish before dist-tags are queryable. The release action is updated to use this script. The review feedback correctly points out that using execFileSync('sleep', ...) is platform-dependent and will fail on Windows. The reviewer suggests using Atomics.wait for a cross-platform synchronous sleep, and provides detailed suggestions to update the unit tests to mock and assert on Atomics.wait instead of the external sleep command.
| function sleep(ms) { | ||
| // Sync sleep so this helper stays callable from CI shell steps without top-level await. | ||
| execFileSync('sleep', [String(ms / 1000)], { stdio: 'ignore' }); | ||
| } |
There was a problem hiding this comment.
Using execFileSync('sleep', ...) is platform-dependent and will fail on Windows environments (such as local developer machines or Windows-based CI runners) because the sleep command is not natively available. Spawning a child process just to pause execution also introduces unnecessary overhead.
Instead, use Atomics.wait to perform a synchronous, cross-platform sleep without spawning any external processes.
| function sleep(ms) { | |
| // Sync sleep so this helper stays callable from CI shell steps without top-level await. | |
| execFileSync('sleep', [String(ms / 1000)], { stdio: 'ignore' }); | |
| } | |
| function sleep(ms) { | |
| // Sync sleep so this helper stays callable from CI shell steps without top-level await. | |
| Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); | |
| } |
| describe('removeNpmDistTag', () => { | ||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
To support the cross-platform Atomics.wait sleep implementation, mock Atomics.wait in a beforeEach block so that tests run instantly without actually sleeping.
describe('removeNpmDistTag', () => {
beforeEach(() => {
vi.spyOn(Atomics, 'wait').mockImplementation(() => 'ok');
});
afterEach(() => {
vi.clearAllMocks();
vi.restoreAllMocks();
});| execFileSync | ||
| .mockImplementationOnce(() => { | ||
| throw notReady; | ||
| }) | ||
| .mockImplementationOnce(() => '') // sleep | ||
| .mockImplementationOnce(() => ''); // successful rm | ||
|
|
||
| removeNpmDistTag({ | ||
| packageName: '@google/gemini-cli-core', | ||
| tag: 'staging-tmp', | ||
| maxAttempts: 3, | ||
| retryDelayMs: 1000, | ||
| }); | ||
|
|
||
| expect(execFileSync).toHaveBeenNthCalledWith( | ||
| 1, | ||
| 'npm', | ||
| ['dist-tag', 'rm', '@google/gemini-cli-core', 'staging-tmp'], | ||
| expect.any(Object), | ||
| ); | ||
| expect(execFileSync).toHaveBeenNthCalledWith( | ||
| 2, | ||
| 'sleep', | ||
| ['1'], | ||
| expect.any(Object), | ||
| ); | ||
| expect(execFileSync).toHaveBeenNthCalledWith( | ||
| 3, | ||
| 'npm', | ||
| ['dist-tag', 'rm', '@google/gemini-cli-core', 'staging-tmp'], | ||
| expect.any(Object), | ||
| ); |
There was a problem hiding this comment.
Update the test assertions to reflect that execFileSync is no longer called for sleeping, and assert that Atomics.wait was called instead.
execFileSync
.mockImplementationOnce(() => {
throw notReady;
})
.mockImplementationOnce(() => ''); // successful rm
removeNpmDistTag({
packageName: '@google/gemini-cli-core',
tag: 'staging-tmp',
maxAttempts: 3,
retryDelayMs: 1000,
});
expect(execFileSync).toHaveBeenNthCalledWith(
1,
'npm',
['dist-tag', 'rm', '@google/gemini-cli-core', 'staging-tmp'],
expect.any(Object),
);
expect(Atomics.wait).toHaveBeenCalledWith(
expect.any(Int32Array),
0,
0,
1000,
);
expect(execFileSync).toHaveBeenNthCalledWith(
2,
'npm',
['dist-tag', 'rm', '@google/gemini-cli-core', 'staging-tmp'],
expect.any(Object),
);| execFileSync.mockImplementation((command) => { | ||
| if (command === 'sleep') { | ||
| return ''; | ||
| } | ||
| throw notReady; | ||
| }); | ||
|
|
||
| expect(() => | ||
| removeNpmDistTag({ | ||
| packageName: '@google/gemini-cli-core', | ||
| tag: 'staging-tmp', | ||
| maxAttempts: 2, | ||
| retryDelayMs: 1000, | ||
| }), | ||
| ).toThrow(/Failed to remove dist-tag "staging-tmp"/); | ||
|
|
||
| // 2 rm attempts + 1 sleep between them | ||
| expect(execFileSync).toHaveBeenCalledTimes(3); |
There was a problem hiding this comment.
Update the exhausted retries test to assert on Atomics.wait instead of mocking the sleep command in execFileSync.
execFileSync.mockImplementation(() => {
throw notReady;
});
expect(() =>
removeNpmDistTag({
packageName: '@google/gemini-cli-core',
tag: 'staging-tmp',
maxAttempts: 2,
retryDelayMs: 1000,
}),
).toThrow(/Failed to remove dist-tag "staging-tmp"/);
expect(execFileSync).toHaveBeenCalledTimes(2);
expect(Atomics.wait).toHaveBeenCalledTimes(1);
|
cla: yes |
Large packages can be acknowledged by Wombat before dist-tags are queryable, causing immediate `npm dist-tag rm staging-tmp` to fail the nightly release. Retry until the tag propagates, using a cross-platform Atomics.wait delay. Fixes google-gemini#28533
21072f0 to
19a3d6c
Compare
gemini-cli
Bot
added
priority/p1
1 participant
Summary
#28533failed because Wombat/npm acknowledged publishing the large@google/gemini-cli-corepackage before thestaging-tmpdist-tag was queryable, so immediatenpm dist-tag rm staging-tmpexited withstaging-tmp is not a dist-tag.scripts/remove-npm-dist-tag.jsto retry removal until the tag propagates (failing fast on unrelated errors), and use it for core/CLI/a2a publish steps.Fixes #28533
Test plan
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/remove-npm-dist-tag.test.jsstaging-tmpremovalstaging-tmpfrom the failed run is cleaned up once a successful publish completes (or cleaned manually if needed)Made with Cursor