Skip to content

attempt to fix bun install hanging in codex - #19721

Closed
Jarred-Sumner wants to merge 17 commits into
mainfrom
jarred/more-flags-1
Closed

Jarred-Sumner wants to merge 17 commits into
mainfrom
jarred/more-flags-1

Conversation

@Jarred-Sumner

@Jarred-Sumner Jarred-Sumner commented May 17, 2025 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

  • Add some locks to logger.Log which is supposed to be threadlocal but ExtractTarball is using the package manager's logger from inside the threadpool, which is not threadsafe.
  • Add BUN_DISABLE_ATOMIC_RENAME be able to rule out renameat2 as the cause
  • Add BUN_FEATURE_FLAG_DISABLE_INSTALL_INDEX to be able to rule out manifest cache installs as the cause
  • Fix saveAsync always being called when manifest cache is diasbled

How did you verify your code works?

@robobun

robobun commented May 17, 2025 •

Copy link
Copy Markdown
Collaborator
Updated 1:41 AM PT - May 19th, 2025

❌ @Jarred-Sumner, your commit 87a3132 has 1 failures in Build #16780:


🧪   To try this PR locally:

bunx bun-pr 19721

That installs a local version of the PR into your bun-19721 executable, so you can run:

bun-19721 --bun

Comment thread package.json Outdated
Comment thread src/logger.zig
Comment thread src/logger.zig
Comment on lines +894 to +903
if (Environment.isDebug or Environment.enable_asan) {
if (!log.lock.tryLock()) {
@panic("Cannot deinit the log while a thread is active");
}
}
defer {
if (Environment.isDebug or Environment.enable_asan) {
log.lock.unlock();
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a potential issue in the lock handling during deinit(). The code panics if tryLock() fails, but the defer block unconditionally calls unlock() regardless of whether the lock was successfully acquired. This could lead to undefined behavior if the panic is caught or in future refactoring.

Consider modifying the code to only unlock if the lock was successfully acquired:

if (Environment.isDebug or Environment.enable_asan) {
    const lock_acquired = log.lock.tryLock();
    if (!lock_acquired) {
        @panic("Cannot deinit the log while a thread is active");
    }
    defer if (lock_acquired) log.lock.unlock();
}

This ensures the lock is only released if it was successfully acquired.

Suggested change
if (Environment.isDebug or Environment.enable_asan) {
if (!log.lock.tryLock()) {
@panic("Cannot deinit the log while a thread is active");
}
}
defer {
if (Environment.isDebug or Environment.enable_asan) {
log.lock.unlock();
}
}
if (Environment.isDebug or Environment.enable_asan) {
const lock_acquired = log.lock.tryLock();
if (!lock_acquired) {
@panic("Cannot deinit the log while a thread is active");
}
defer if (lock_acquired) log.lock.unlock();
}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

@Jarred-Sumner

Jarred-Sumner commented May 19, 2025 •

Copy link
Copy Markdown
Collaborator Author

I split this PR into 5 separate PRs, but none of them actually fix the issue impacting Codex. My current hunch there is a bug in our HTTP client, but I'm ont sure yet. Please follow along in #19755.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants