Skip to content

test: add deterministic crash checkpoints for installs#4963

Open
cachebag wants to merge 1 commit into
rust-lang:mainfrom
cachebag:test/crash-checkpoint-harness
Open

test: add deterministic crash checkpoints for installs#4963
cachebag wants to merge 1 commit into
rust-lang:mainfrom
cachebag:test/crash-checkpoint-harness

Conversation

@cachebag

Copy link
Copy Markdown
Contributor

Under the guidance of @rami3l, this is the first unit of work toward #4400, with #4724 as a related failure mode.

Ref: toolchain pool proposal

The plan is to establish deterministic infrastructure for testing hard interruptions without relying on debugger automation. This PR adds a test-only, environment-selected checkpoint mechanism. A child rustup process signals that it has reached the selected checkpoint and blocks; the test driver then kills and reaps it before retrying the operation in a fresh process.

This was my solution as opposed to something like a a debugger automation..but if it proves to be weak we can go that route instead. Would love feedback on this part.

The initial test interrupts installation after component payloads are installed but before toolchain metadata is written, then verifies that retrying converges to a working toolchain. This PR intentionally adds test infrastructure rather than changing recovery behavior.

(Additional named interruption points can be added as needed without changing the parent-process test harness, I figured a smaller scoped PR was appropriate first)

Follow-up work, generally speaking, will be as follows:

@cachebag
cachebag force-pushed the test/crash-checkpoint-harness branch from 3531118 to c6d83d7 Compare July 16, 2026 21:11
@rustbot

This comment has been minimized.

Comment thread src/test/checkpoint.rs Outdated

@djc djc 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.

This looks okay to me.

View changes since this review

Comment thread src/dist/manifestation.rs Outdated
}

#[cfg(feature = "test")]
crate::test::checkpoint::wait(download_cfg.process, "manifestation-update-before-metadata");

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.

Nit: suggest importing checkpoint at the top of the module.

Comment thread src/test/checkpoint.rs Outdated
@@ -0,0 +1,24 @@
use std::{fs, path::PathBuf, thread};

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.

IMO adding such small modules makes the source tree harder to navigate.

Comment thread src/test/clitools.rs Outdated
let _ = child.wait();
panic!("timed out waiting for checkpoint {checkpoint:?}");
}
std::thread::sleep(StdDuration::from_millis(10));

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.

Nit: import thread?

@cachebag
cachebag force-pushed the test/crash-checkpoint-harness branch from c6d83d7 to c069a84 Compare July 17, 2026 13:32
@rustbot

This comment has been minimized.

@cachebag
cachebag requested review from djc and rami3l July 17, 2026 13:32
@cachebag
cachebag force-pushed the test/crash-checkpoint-harness branch from c069a84 to 17511f4 Compare July 17, 2026 13:34
@cachebag
cachebag force-pushed the test/crash-checkpoint-harness branch from 17511f4 to c7f9e73 Compare July 17, 2026 13:34
@rustbot

rustbot commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rami3l rami3l left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Make sure that you have addressed all remaining concerns and I think we can merge this.

View changes since this review

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.

4 participants