Skip to content

test(pm): reproduce downloader cache race on linux - #3255

Closed
elrrrrrrr wants to merge 2 commits into
feat/utoo-runtimefrom
codex/downloader-race-repro
Closed

test(pm): reproduce downloader cache race on linux#3255
elrrrrrrr wants to merge 2 commits into
feat/utoo-runtimefrom
codex/downloader-race-repro

Conversation

@elrrrrrrr

Copy link
Copy Markdown
Contributor

Temporary PR to run a Linux GitHub Actions reproduction for downloader cache races.\n\nThis adds an ignored downloader stress repro and a PR-triggered workflow that retries it on ubuntu-latest. The target signal is a resolved cache slot whose package.json becomes zero-length after a racing downloader process is killed post-_resolved.

@elrrrrrrr
elrrrrrrr force-pushed the codex/downloader-race-repro branch from df03cc6 to c8b2ac9 Compare July 21, 2026 14:18

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request adds new integration tests to crates/pm/src/util/downloader.rs to verify downloading to a shared destination across multiple processes and to reproduce a race condition that can leave a zero-length file. The review feedback suggests improving the test helpers by using the standard library's Child::kill method instead of spawning an external kill process, and replacing synchronous, blocking calls (path.exists() and std::fs::read_dir) with their asynchronous counterparts (crate::fs::try_exists and crate::fs::read_dir) to avoid blocking the Tokio executor.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +583 to +590
for child in &children {
if child.id() != 0 {
let _ = Command::new("kill")
.arg("-KILL")
.arg(child.id().to_string())
.status();
}
}

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.

medium

Instead of spawning an external kill process, which is non-portable and less efficient, use the standard library's Child::kill method. Since child.kill() is already used elsewhere in this test file, doing so here also improves consistency.

        let mut children = children;
        for child in &mut children {
            let _ = child.kill();
        }

Comment on lines +606 to +615
async fn wait_for_path(path: &Path, timeout: Duration) {
let deadline = Instant::now() + timeout;
while Instant::now() < deadline {
if path.exists() {
return;
}
sleep(Duration::from_millis(10)).await;
}
panic!("timed out waiting for {}", path.display());
}

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.

medium

The path.exists() call is synchronous and blocks the Tokio executor thread. Use the asynchronous crate::fs::try_exists re-export instead. To ensure permission or other critical errors fail early and clearly, propagate I/O errors using ? rather than silently ignoring them or using .unwrap().

Suggested change
async fn wait_for_path(path: &Path, timeout: Duration) {
let deadline = Instant::now() + timeout;
while Instant::now() < deadline {
if path.exists() {
return;
}
sleep(Duration::from_millis(10)).await;
}
panic!("timed out waiting for {}", path.display());
}
async fn wait_for_path(path: &Path, timeout: Duration) -> std::io::Result<()> {
let deadline = Instant::now() + timeout;
while Instant::now() < deadline {
if crate::fs::try_exists(path).await? {
return Ok(());
}
sleep(Duration::from_millis(10)).await;
}
panic!("timed out waiting for {}", path.display());
}
References
  1. Propagate I/O errors (e.g., from try_exists) using ? rather than silently ignoring them with unwrap_or(false), to ensure permission or other critical errors fail early and clearly.

Comment on lines +617 to +632
async fn wait_for_ready_files(path: &Path, count: usize, timeout: Duration) {
let deadline = Instant::now() + timeout;
while Instant::now() < deadline {
let ready_count = std::fs::read_dir(path)
.map(|entries| entries.count())
.unwrap_or(0);
if ready_count >= count {
return;
}
sleep(Duration::from_millis(10)).await;
}
panic!(
"timed out waiting for {count} ready files in {}",
path.display()
);
}

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.

medium

The std::fs::read_dir call is synchronous and blocks the Tokio executor thread. Use the asynchronous crate::fs::read_dir re-export instead to keep the helper fully non-blocking.

    async fn wait_for_ready_files(path: &Path, count: usize, timeout: Duration) {
        let deadline = Instant::now() + timeout;
        while Instant::now() < deadline {
            let mut ready_count = 0;
            if let Ok(mut entries) = crate::fs::read_dir(path).await {
                while let Ok(Some(_)) = entries.next_entry().await {
                    ready_count += 1;
                }
            }
            if ready_count >= count {
                return;
            }
            sleep(Duration::from_millis(10)).await;
        }
        panic!(
            "timed out waiting for {count} ready files in {}",
            path.display()
        );
    }

@elrrrrrrr
elrrrrrrr force-pushed the codex/downloader-race-repro branch 2 times, most recently from 13a79d0 to 0726c18 Compare July 21, 2026 14:31
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.

1 participant