install: restore the slow-lifecycle-script warning from #7719 - #36587
Open
robobun wants to merge 5 commits into
Open
install: restore the slow-lifecycle-script warning from #7719#36587robobun wants to merge 5 commits into
robobun wants to merge 5 commits into
Conversation
The "<pkg>'s <script> script took <duration>" warning has been dead code since #8456 dropped the timer start and #8943 dropped the print call. The Rust port then copied the already-broken scaffolding verbatim (timer never started, empty log entry, print path missing). Start the per-script timer in spawn_next_script_inner, record {package_name, script_id, duration} when a script exceeds 500ms, and print the slowest entry in the install summary (same placement as #7719). stderrForInstall() now also strips this warning, matching its existing handling of the slow-filesystem warning, since debug/ASAN builds push otherwise-fast scripts over the threshold.
Collaborator
Author
|
Status: diff is green; ready for review. CI on 5e907cc has one hard-red test ( Reproduced with |
Contributor
WalkthroughLifecycle scripts now record package, script, and duration data. Installation summaries print the slowest-script warning and clear the log. Tests cover delayed scripts under both waiter-thread configurations. ChangesLifecycle script timing
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/lifecycle_script_runner.rs`:
- Around line 915-919: Move the LifecycleScriptTimeLogEntry recording and append
logic before the exit.code > 0 early-return branch in the lifecycle script
runner, ensuring optional failed scripts exceeding 500ms are included in the
slow-script summary while preserving the existing install continuation behavior.
In `@test/cli/install/bun-install-lifecycle-scripts.test.ts`:
- Around line 2175-2179: Strengthen the warning assertion in the
lifecycle-script test by capturing the reported duration and unit from the
slow-pkg postinstall message, then validate that the parsed duration exceeds
500ms with the appropriate unit conversion. Preserve the existing checks for
warning text, installation output, and successful exit status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: efa1d42e-766b-4e4b-b704-30a8f4147585
📒 Files selected for processing (5)
src/install/PackageManager/PackageManagerLifecycle.rssrc/install/PackageManager/install_with_manager.rssrc/install/lifecycle_script_runner.rstest/cli/install/bun-install-lifecycle-scripts.test.tstest/harness.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — the release-build breakage from the first commit is fixed in 7798b94 (#[cfg(debug_assertions)] now gates the try_lock block). The remaining inline note is cosmetic.
Extended reasoning...
Overview
Restores the warn: <pkg>'s <script> script took <duration> line from #7719, which has been dead since early 2024. Five files touched: LifecycleScriptTimeLogEntry gains real fields, spawn_next_script_inner starts the timer, handle_exit populates the entry, a new print() method emits the warning, print_install_summary calls it, and stderrForInstall in harness.ts strips it (mirroring the existing slow-filesystem handling). New test covers both waiter-thread modes.
Security risks
None. Output-only change to a stderr warning; no new input parsing, no auth/crypto/permissions.
Level of scrutiny
Low-medium. This is reviving previously-working scaffolding that was already present but dead (empty struct, unset timer). The threshold gate and mutex-protected append already existed; the diff just wires them up and adds the print path. The author's rebuttals to CodeRabbit (only-successful-scripts-recorded matches original #7719; regex assertion is sufficient given the 500ms gate) are correct.
Other factors
- My prior blocking finding (release builds failing on cfg-gated
try_lock) was fixed in 7798b94; the diff now uses#[cfg(debug_assertions)] { ... }matching every other in-tree call site. - The remaining nit (trailing
\ninwarn!is a no-op due to_needs_nl) is cosmetic — the siblingreport_slow_lifecycle_scriptsin the same file has the identical pattern, so output is consistent with existing behavior. Not merge-blocking. - Test verified failing on
USE_SYSTEM_BUN=1and passing on the debug build; drains all three pipes concurrently; placed in the existing lifecycle-scripts test file inside theforceWaiterThreadloop. - Checked
script_idindexing intoScripts::NAMES— bounded by the0..NAMES.len()iteration inhandle_exit/spawn_next_script_inner, so no panic risk. - The
stderrForInstallregex is line-bounded (.doesn't match newlines) and specific to the's <word> script tookshape, so it won't over-strip.
Contributor
There was a problem hiding this comment.
LGTM — both prior findings (release-build try_lock gate, no-op trailing \n) are addressed in 7798b94 and c2cad89.
What was reviewed:
print()port:max_by_keyover entries,lockfile::Scripts::NAMES[script_id]indexing matches the existingscript_name()pattern,Box<[u8]>entries drop onclear().stderrForInstallregex is scoped to the exactwarn: X's Y script tookshape and mirrors the existing slow-filesystem strip.- Test sleeps 750ms (fixed, not compute-bound) and runs under both waiter-thread modes via the enclosing loop; verified to fail on system bun per the description.
Extended reasoning...
Overview
Restores the warn: <pkg>'s <script> script took <duration> line from #7719, which has been silently dead since #8456/#8943 dropped the timer start and print call. Five files touched: LifecycleScriptTimeLogEntry gains three fields, LifecycleScriptTimeLog::print() is added (port of the Zig printAndDeinit), spawn_next_script_inner starts timer, handle_exit populates the entry, print_install_summary calls print(), a new test asserts the warning appears, and stderrForInstall in the harness strips the warning so debug/ASAN-slowed lifecycle scripts don't break unrelated tests' not.toContain("warn:") assertions.
Security risks
None. This is diagnostic output only — no untrusted input parsing, no path/credential handling, no allocation sized from external data. package_name is a Box<[u8]> clone of an already-owned field; script_id indexes into the fixed-length Scripts::NAMES array via the same pattern script_name() already uses.
Level of scrutiny
Low-to-medium. This is a narrow restoration of dead scaffolding that a maintainer explicitly flagged on #36576. No new control flow on the install path beyond one print() call in the cold print_install_summary. The mutex-guarded append_concurrent was already in place; print() runs single-threaded after all scripts finish (debug-asserted via try_lock, now correctly #[cfg(debug_assertions)]-gated so release builds compile).
Other factors
- Two prior review rounds: my release-build
try_lockfinding was fixed in 7798b94 (author confirmedcargo check --release -p bun_installclean), and the misleading trailing-\ncomment was dropped in c2cad89. All inline threads are resolved. - CodeRabbit's two suggestions (record failed-optional scripts; parse the duration in the test) were reasonably declined with reference to the original #7719 semantics and the 500ms append-gate invariant, and CodeRabbit withdrew both.
- Test verified to fail with
USE_SYSTEM_BUN=1and pass on the debug build. The 750ms subprocess sleep is fixed-duration (not compute-scaled), so it stays ~750ms under debug/ASAN. - The harness regex is specific enough not to over-match (requires
's <word> script tookshape) and follows the existing slow-filesystem-warning precedent in the same helper. - The bug-hunting system found nothing on this revision.
…t_and_clear Root package scripts run in foreground mode with $ echoed and stdio inherited, so the user already watched them; the summary warning is for background dependency scripts whose duration was invisible. The foreground mode postdates when the original feature was live, so this gate was never a shipped behavior either way. Rename print -> print_and_clear to match the Zig printAndDeinit and the repo's *_and_clear/*_and_reset convention.
Contributor
There was a problem hiding this comment.
LGTM — both earlier findings are addressed (the try_lock block is now #[cfg(debug_assertions)]-gated so release builds compile, and the no-op trailing \n + stale comment are gone).
Extended reasoning...
Overview
Restores the warn: <pkg>'s <script> script took <duration> line from #7719, which has been dead since #8456/#8943 dropped the timer start and print calls. Touches PackageManagerLifecycle.rs (fills in the empty LifecycleScriptTimeLogEntry struct, adds print_and_clear), lifecycle_script_runner.rs (starts the timer, appends entries on exit for background scripts >500ms), install_with_manager.rs (one-line call site in print_install_summary), plus a test and a harness stderr filter.
Security risks
None. This is stderr-only diagnostic output during bun install; no user-controlled input reaches new parsing/allocation, no auth/crypto/permissions.
Level of scrutiny
Low-medium. It fills in scaffolding that was already present but inert (timer: None, empty struct, unused append_concurrent). The only behavioral change visible to users is one extra warning line in the install summary. The two issues I flagged on earlier commits (release-build E0599 from cfg-gated try_lock, and the misleading blank-line comment) were both fixed in 7798b94 and c2cad89/5e907cc; I re-verified Mutex::try_lock is still #[cfg(debug_assertions)]-gated at src/threading/Mutex.rs:53 and the new call site now matches.
Other factors
- Test fails on system bun and passes on the debug build per the PR description; runs under both waiter-thread modes via the existing
forceWaiterThreadloop; drains stdout/stderr/exited concurrently and asserts stdout before exitCode. stderrForInstallregex uses optional\n?\n?so it tolerates the single-newline output.package_name.clone()onBox<[u8]>is a deep copy, so the entry outlives the subprocess;Vec::clearinprint_and_cleardrops eachBox<[u8]>correctly.- All CodeRabbit and comment-cop threads are resolved; the two CodeRabbit suggestions were reasonably declined (matching original #7719 semantics; avoiding coupling to
fmt_duration_one_decimalunit choice).
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 6, 2026
…indings, and JS internals (#36970) Removes code verified to have zero references across `src/`, `scripts/`, `test/`, and freshly regenerated `build/debug/codegen/` output. Every candidate was grepped for bare-name, quoted-string, and `$`-prefixed references before deletion; items referenced from generated bindings, `.classes.ts` files, attribute-macro exports (`uws_callback(export = ...)`), or `extern "C"` surfaces were left alone. The vendored WebKit tree is part of the reference scan as well: `Bun__errorInstance__finalize` was initially removed here, then restored once the darwin LTO link surfaced its `__attribute__((weak))` reference from JSC's `ErrorInstance.cpp` (weak references satisfy non-LTO links silently). ### Rust - `bun_install::Error`: variants `FileTooBig`, `ProcessFdQuotaExceeded`, `ReadOnlyFileSystem`, `FileSystem`, `FileBusy` were never constructed (the same-named live variants belong to `bun_runtime`'s separate error enum; `node_fs.rs` maps onto that one) - `hosted_git_info::Representation::Ssh`: every ssh-flavored protocol maps to `Sshurl` - `FromTextLockfileError::InvalidSemver` plus its only mention, an unreachable match arm in `bun.lock.rs` (`ParseError::InvalidSemver` stays and is still produced) - `MigratePnpmLockfileError::{PnpmLockfileInvalidOverride, PnpmLockfileInvalidPatchedDependency}`: never produced by the pnpm migration - windows-shim `FailReason::InvalidShimDataSize`: no size check produces it - `bun_event_loop::EventLoopTimer::TimerCallback` struct, its `Tag` variant, and the dispatch arm in `runtime/dispatch.rs`: nothing ever constructed one, so the tag could never be dispatched - `bun_dns::Family::Unix`: neither the string map nor the JS numeric mapping yields it (`AF_UNIX` on the result path is a different, live match) - MySQL wire structs: write-only fields `OKPacket::{warnings, info, session_state_changes}`, `EOFPacket::warnings`, `StmtPrepareOKPacket::warning_count`, `LocalInfileRequest::filename`. The wire reads stay so packet parsing consumes the same bytes; only the dead stores and their zero-initializers are gone. ### C++ bindings - `NodeValidator.cpp`: host functions `jsFunction_validateString` / `jsFunction_validateFunction` / `jsFunction_validateBoolean` and their declarations. Their `$newCppFunction` bindings were removed in an earlier sweep (#36937 removed the sibling trio); the `V::validate*` overloads they forwarded to are live and stay. - `ImportMetaObject.cpp`: `jsFunctionRequireResolve` and its only callee `functionRequireResolve` (static, 76 lines; the live `require.resolve` is built elsewhere) - `BunString.cpp`: `BunString__toWTFString` (no Rust-side caller; the regenerated `cpp.rs` drops the import) - `sliceAnsi.cpp`: never-instantiated `struct HyperlinkInfo` (`wrapAnsi.cpp`'s `HyperlinkState` is the live one) - `NodeFSStatFSBinding.cpp`: `getStatFSPrototype<bool>`, a template with zero instantiations - Declarations with no definition: `functionBunPeek` / `functionBunPeekStatus` (BunObject.h), `callBakeResponse` / `constructBakeResponse` (JSBakeResponse.cpp), `jsSqlStatementGetHasMultipleStatements` (JSSQLStatement.cpp), `bn_set_words` (dh-primes.h) - Commented-out blocks from 2023-2024: the `ErrorCaptureStackTrace` experiment in BunProcess.cpp, the `deleteProperty` block in JSAbortSignal.cpp, the `setOnEachMicrotaskTick` block in BakeGlobalObject.cpp ### Built-in JS internals Export-default entries no requirer ever destructures (verified against every `require()` site, C++ `getDirect` lookups, and `test/` imports of internal modules); backing functions that are still used in-file stay: - `internal/repl/node-shims.js`: `isWritable`, `runScriptInThisContext`, `kEmptyObject`, `addAbortListener`, `promisify` (none of repl.js / internal/repl/* touch them) - `internal/streams/iter/from.ts`: `normalizeAsyncSource`, `normalizeSyncSource`, `normalizeSyncValue`, `primitiveToUint8Array` - `internal/sql/sqlite.ts`: `SQLCommand`, `commandToString`, `parseSQLQuery`, `SQLiteQueryHandle` (sole requirer pulls only `SQLiteAdapter`) - `internal/sql/shared.ts`: `parseDefinitelySqliteUrl`, `buildDefinedColumnsAndQuery`, `normalizeSSLMode` - `internal/http1_server_fallback.ts`: `createHttp1FallbackResponseHandle`, `kHttp1ActiveRequests` - one-line entries: `setTid` (trace_events), `FixedCircularBuffer` (fixed_queue), `EXECUTION_CONTEXT_ID` (inspector/cdp), `defineCustomPromisify` (promisify), `SQLQueryStatus` (sql/query), `allUint8Array` (streams/iter/utils) ### Build config and orphaned files - `scripts/build/flags.ts`: defines `IS_BUILD`, `WITH_BORINGSSL=1`, `STATICALLY_LINKED_WITH_BMALLOC=1`, `BUN_SINGLE_THREADED_PER_VM_ENTRY_SCOPE=1` have zero readers in `src/`, `packages/`, or the pinned WebKit checkout (WebKit reads the lowercase `STATICALLY_LINKED_WITH_bmalloc`, which is not what we were defining) - `src/runtime/ffi/libtcc1.a.macos-aarch64`: prebuilt 30KB archive from 2022 with zero references (`libtcc1.c` is embedded via `include_bytes!` and compiled at runtime) ### Verification - `cargo check --workspace` and `bun run rust:check-all` (10 ok, 0 failed) so platform-gated uses would have surfaced - full `bun bd` debug build, which regenerates codegen and relinks the C++ side - smoke tests: `test/js/bun/repl/repl.test.ts` (148 pass), `test/cli/install/migration/migrate.test.ts` (20 pass), `test/js/sql/wire-frames.test.ts`, `test/js/sql/sql-mysql-clean-reentry.test.ts` against MariaDB, `test/js/sql/adapter-override.test.ts`, fixed-queue node tests, plus module-load smokes for repl/stream/trace_events/http2/util - `test/internal/source-lints/` suite passes, including the new `dead-symbols-install-sql-bindings.test.ts` that pins these removals - checked against all open robobun PRs at deletion granularity: a planned removal of the install lifecycle-script time log was dropped from this PR because #36587 restores that feature, and the mysql `CharacterSet` collation table plus the `Bun__CryptoHasherExtern__*` helpers were left alone after verification showed they are referenced (`label()` at MySQLConnection.rs:661, C++ wrappers in CryptoUtil.cpp) ### Verified-dead but deliberately not removed (for a future pass, pending maintainer judgment) - windows-shim `read_without_launch` / `FromBunShellContext` (~120 lines): zero callers, but the crate docs describe it as the staged in-process path for the shell - `src/runtime/bake/incremental_visualizer.html` + `memory_visualizer.html` (~808 lines): the `/_bun/incremental_visualizer` route that served them was never ported from the Zig dev server, while the websocket topic plumbing they rely on is live and tested - `src/jsc/bindings/webcrypto/*.idl` (29 files, ~1055 lines): nothing in the build reads `.idl`, but they are maintained alongside the handwritten bindings as spec reference (SubtleCrypto.idl was edited in July) <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 2 · 43 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/dead-symbols-install-sql-bindings.test.ts bun test v1.4.0 (fc376a3) test/internal/source-lints/dead-symbols-install-sql-bindings.test.ts: 79 | expect(reprEnd).toBeGreaterThan(reprStart); 80 | const reprBody = hosted.slice(reprStart, reprEnd); 81 | if (/^\s*Ssh,$/m.test(reprBody)) { 82 | resurrected.push("src/install/hosted_git_info.rs: Representation::Ssh"); 83 | } 84 | expect(resurrected).toEqual([]); ^ error: expect(received).toEqual(expected) - [] + [ + "src/install/error.rs: ^\s*FileTooBig,$", + "src/install/error.rs: ^\s*ProcessFdQuotaExceeded,$", + "src/install/error.rs: ^\s*ReadOnlyFileSystem,$", + "src/install/error.rs: ^\s*FileSystem,$", + "src/install/error.rs: ^\s*FileBusy,$", + "src/install/resolution.rs: \bInvalidSemver\b", + "src/install/pnpm.rs: PnpmLockfileInvalidOverride|PnpmLockfileInvalidPatchedDependency", + "src/install/windows-shim/bun_shim_impl.rs: \bInvalidShimDataSize\b", + "src/event_loop/EventLoopTimer.rs: \bTimerCallbac ... (truncated) release without fix: 1 FAILED bun test v1.4.0-canary.1 (5fd12c2) test/internal/source-lints/dead-symbols-install-sql-bindings.test.ts: 79 | expect(reprEnd).toBeGreaterThan(reprStart); 80 | const reprBody = hosted.slice(reprStart, reprEnd); 81 | if (/^\s*Ssh,$/m.test(reprBody)) { 82 | resurrected.push("src/install/hosted_git_info.rs: Representation::Ssh"); 83 | } 84 | expect(resurrected).toEqual([]); ^ error: expect(received).toEqual(expected) - [] + [ + "src/install/error.rs: ^\s*FileTooBig,$", + "src/install/error.rs: ^\s*ProcessFdQuotaExceeded,$", + "src/install/error.rs: ^\s*ReadOnlyFileSystem,$", + "src/install/error.rs: ^\s*FileSystem,$", + "src/install/error.rs: ^\s*FileBusy,$", + "src/install/resolution.rs: \bInvalidSemver\b", + "src/install/pnpm.rs: PnpmLockfileInvalidOverride|PnpmLockfileInvalidPatchedDependency", + "src/install/windows-shim/bun_shim_impl.rs: \bInvalidShimDataSize\b", + "src/event_loop/EventLoopTimer.rs: \bTimerCallback\b", + "src/runtime/dispatch.rs: \bTimerCallback\b", + "src/dns/lib.rs: ^\s*Unix,$", + "src/sql/mysql/protocol/OKPacket.rs: session_state_changes|pub info:|pub warnings:", + "src/sql/m ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/dead-symbols-install-sql-bindings.test.ts bun test v1.4.0 (fc376a3) test/internal/source-lints/dead-symbols-install-sql-bindings.test.ts: (pass) dead Rust symbols (install, event_loop, dns, mysql protocol) do not reappear [30.77ms] (pass) dead C++ bindings do not reappear [66.65ms] (pass) the WebKit weak error finalizer stays defined [5.74ms] (pass) dead built-in JS exports and build defines do not reappear [32.43ms] 4 pass 0 fail 6 expect() calls Ran 4 tests across 1 file. [2.15s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 666ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/144] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 239 extern-C blocks audited [2/144] gen cpp.rs (cppbind) [3/144] gen BunProcess.lut.h Generating /workspace/bun/build/release/codegen/BunProcess.lut.h from /workspace/bun/src/jsc/bindings/BunProcess.cpp [4/144] gen JS modules (bundle-modules) Preprocess modules (8773ms) Bundle modules (47ms) Postprocesss modules (48ms) Bundle Functions (653ms) Generate Code (28ms) [9.56s] Bundled "src/js" for production 2571 kb 193 internal modules 13 native modules 84 internal functions across 17 files [4/143] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compi ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` scripts/build/flags.ts | 4 - src/dns/lib.rs | 2 - src/event_loop/EventLoopTimer.rs | 9 -- src/install/error.rs | 15 --- src/install/hosted_git_info.rs | 2 - src/install/lockfile/bun.lock.rs | 8 -- src/install/migration.rs | 2 - src/install/pnpm.rs | 4 - src/install/resolution.rs | 2 - src/install/windows-shim/bun_shim_impl.rs | 2 - src/js/internal/fixed_queue.ts | 1 - src/js/internal/http1_server_fallback.ts | 2 - src/js/internal/inspector/cdp.ts | 1 - src/js/internal/promisify.ts | 1 - src/js/internal/repl/node-shims.js | 23 ---- src/js/internal/sql/query.ts | 1 - src/js/internal/sql/shared.ts | 3 - src/js/internal/sql/sqlite.ts | 4 - src/js/internal/streams/iter/from.ts | 4 - src/js/internal/streams/iter/utils.ts | 1 - src/js/internal/trace_events.ts | 1 - src/jsc/bindings/BunObject.h | 2 - src/jsc/bindings/BunProcess.cpp | 18 --- src/jsc/bindings/BunString.cpp | 21 --- src/jsc/bindings/ErrorStackTrace.cpp | 1 + src/jsc/bindings/ImportMetaObject.cpp | 89 ------------- src/jsc/bindings/JSBakeResponse.cpp | 3 - src/jsc/bindings/NodeFSStatFSBinding.cpp | 10 -- src/jsc/bindings/NodeValidator.cpp | 38 ------ src/jsc/bindings/NodeValidator.h | 3 - src/jsc/bindings/dh-primes.h | 2 - src/jsc/bindings/headers-handwritten.h | 1 - src/jsc/bindings/sliceAnsi.cpp ... (truncated) ``` </details> **gate history** · 4 passed · 1 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests scripts/build/flags.ts 1 1 0 src/dns/lib.rs 1 2 0 src/event_loop/EventLoopTimer.rs 1 4 0 src/install/error.rs 2 3 0 src/install/hosted_git_info.rs 1 1 0 src/install/lockfile/bun.lock.rs 1 1 0 src/install/migration.rs 1 1 0 src/install/pnpm.rs 1 1 0 src/install/resolution.rs 1 1 0 src/install/windows-shim/bun_shim_impl.rs 1 2 0 src/js/internal/fixed_queue.ts 1 1 0 src/js/internal/http1_server_fallback.ts 1 2 0 src/js/internal/inspector/cdp.ts 1 1 0 src/js/internal/promisify.ts 1 1 0 src/js/internal/repl/node-shims.js 3 6 0 src/js/internal/sql/query.ts 1 1 0 (+ 27 more files) ``` </details> <!-- robobun:evidence:end -->
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
2 participants
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.
What
The
warn: <pkg>'s <script> script took <duration>line from #7719 has been silently dead since early 2024: #8456 dropped theTimer.start()call inspawnNextScript, and #8943 dropped theprintAndDeinitcall from the install summary. The Rust port then copied the already-broken scaffolding verbatim (LifecycleScriptSubprocess.timerinitialised toNoneand never set,LifecycleScriptTimeLogEntryan empty struct, no print path), so the warning has not fired since.Flagged by @Jarred-Sumner on #36576.
Fix
spawn_next_script_inner: starttimeralongsidestarted_at.handle_exit: record{package_name, script_id, duration}when a background script exceeds 500ms. Foreground (root-package) scripts are skipped since they're already echoed live to the terminal; that mode was added after this feature had already died, so the combination never shipped either way.LifecycleScriptTimeLog::print_and_clear(): find the longest entry andwarn!it (port of the ZigprintAndDeinit).print_install_summary: call it between the tree and the "N packages installed" line, matching the original feat: print the longest postinstall if it took more than 500ms #7719 placement.stderrForInstall()(harness): also strip this warning, same as the existing slow-filesystem warning handling, since debug/ASAN builds can push any lifecycle-script subprocess over 500ms.Verification
Full
bun-install-lifecycle-scripts.test.ts: 119 pass, 3 fail (the 3 are pre-existingnode: command not foundenv failures, reproduced on main).bun run rust:check-allpasses on all 10 targets.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-lifecycle-scripts.test.ts