Build: Retry-mechanism for dts bundle-generation#34212
Conversation
|
View your CI Pipeline Execution ↗ for commit afe1be1
☁️ Nx Cloud last updated this comment at |
| } else if (!process.env.CI) { | ||
| console.log('✅ Generated types for', picocolors.cyan(join(DIR_REL, entryPoint))); | ||
| if (dtsProcess.exitCode !== 0) { | ||
| if (attempt < MAX_DTS_ATTEMPTS) { |
There was a problem hiding this comment.
If you reach max attempts, could it be worth printing an error message that says "This part of the build is known to be flaky, but failed ${MAX_ATTEMPTS} times in a row. Consider re-running the CI job."?
It helps external human contributors if we point out, in CI failures, which ones we suspect to be flaky vs which ones are probably caused by their code.
There was a problem hiding this comment.
The build might also actually be broken @Sidnioulz
yannbf
added
the
patch:yes
📝 WalkthroughWalkthroughAdded bounded retry logic to TypeScript declaration file generation in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches📝 Generate docstrings
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build/utils/generate-types.ts`:
- Line 70: The timeout branch currently calls dtsProcess.kill(408) using an
invalid numeric signal; update the timeout handler in generate-types.ts to call
dtsProcess.kill('SIGTERM') (use the string signal name) so the child is
terminated with a valid Unix signal; optionally follow up by checking the
process state or falling back to dtsProcess.kill('SIGKILL') if the graceful
SIGTERM does not stop the process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7217d3aa-aca8-4202-b4cb-0004b116dec1
📒 Files selected for processing (1)
scripts/build/utils/generate-types.ts
| }, 120000); | ||
| }), | ||
| ]); | ||
| dtsProcess.kill(408); // timed out |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if this pattern (kill(408)) exists elsewhere in the codebase and whether it's intentional
rg -n 'kill\(408\)' --type tsRepository: storybookjs/storybook
Length of output: 156
🏁 Script executed:
cat -n scripts/build/utils/generate-types.ts | sed -n '50,85p'Repository: storybookjs/storybook
Length of output: 1569
🏁 Script executed:
# Check Node.js documentation or comments about kill() and signal values
rg -B3 -A3 'kill\(408\)' scripts/build/utils/generate-types.tsRepository: storybookjs/storybook
Length of output: 314
🏁 Script executed:
# Check if there are any TypeScript type definitions or comments in the file about process signals
rg -i 'signal|timeout' scripts/build/utils/generate-types.ts | head -20Repository: storybookjs/storybook
Length of output: 273
Replace kill(408) with a valid Unix signal.
The value 408 is not a recognized Unix signal and will be silently ignored, causing the timeout mechanism to fail. Standard signals are 1–31 (e.g., SIGTERM = 15, SIGKILL = 9). Use 'SIGTERM' to gracefully terminate the process.
Suggested fix
- dtsProcess.kill(408); // timed out
+ dtsProcess.kill('SIGTERM'); // timed out📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dtsProcess.kill(408); // timed out | |
| dtsProcess.kill('SIGTERM'); // timed out |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build/utils/generate-types.ts` at line 70, The timeout branch
currently calls dtsProcess.kill(408) using an invalid numeric signal; update the
timeout handler in generate-types.ts to call dtsProcess.kill('SIGTERM') (use the
string signal name) so the child is terminated with a valid Unix signal;
optionally follow up by checking the process state or falling back to
dtsProcess.kill('SIGKILL') if the graceful SIGTERM does not stop the process.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 121 | 124 | 🚨 +3 🚨 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 23.54 MB | 23.78 MB | 🚨 +240 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
…eration Build: Retry-mechanism for dts bundle-generation (cherry picked from commit 7801015)
github-actions
Bot
added
the
patch:done
3 participants
What I did
We run dts bundling on 5 entrypoints at a time.
This is usually fine, however occasionally we run into the problem where
entrypoint Ausesentrypoint B.This is fine, of course, but then if
entrypoint Bis at that time getting generated, the file might be partially written.When
entrypoint Athen tries to load theb.d.tsfile it will error and yield a "not a valid module".I've tried a few things to solve this, but ultimately landed on a retry mechanism, because everything else i tried to make the system more predictable had severe performance implications.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
I manually ran the compile step a few times, and I noticed no changes, no regressions.
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Release Notes