web browser (Servo)
git clone https://git.lucas.co/cce-browser.git
fix: the favorites tests stop sharing one scratch directory
`store()` handed both `strip_order_is_insertion_order_and_shifts_move_one_place`
and `internal_pages_are_refused` the same `cce-browser-favs-<pid>/favorites.tsv`,
and wiped the directory on each call. The tests run in parallel threads of one
process, so the second call's `remove_dir_all` could take the first's file out
from under it mid-run — and with one file name between them, the round-trip
read at the end of `strip_order_...` could have been reading the other test's
writes.
It does not surface on its own: 40 runs at 8 threads, zero failures. It is not
theoretical either. Steering the second test's wipe into the first's
write-then-read window with a 50 ms delay failed it 10 times out of 10, the
round-trip read coming back empty because the file had been deleted under it —
and the same steering against per-test directories passes 10 out of 10. Narrow
is the argument for fixing it rather than against: a race this rare surfaces as
one unreproducible CI failure, in a test that failed for a reason nowhere in
its own body.
`store` takes the caller's name now, as `session.rs`'s `temp_session` already
does, so each test owns a directory and the wipe is its own. Neither test left
its directory behind before; `internal_pages_are_refused` now clears its own,
which it could not safely do while the directory was shared.
Co-Authored-By: Claude Opus 5 <[email protected]>
src/pages.rs | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/src/pages.rs b/src/pages.rs
index 48d3eca..2171076 100644
--- a/src/pages.rs
+++ b/src/pages.rs
@@ -579,8 +579,26 @@ impl CceProtocol {
mod tests {
use super::*;
- fn store() -> Favorites {
- let dir = std::env::temp_dir().join(format!("cce-browser-favs-{}", std::process::id()));
+ /// A favorites store in a scratch directory of this TEST's own.
+ ///
+ /// Named per test rather than shared. The tests run in parallel threads
+ /// of one process, so with a single directory between them each call's
+ /// `remove_dir_all` could take the other's file out from under it
+ /// mid-run — and both wrote the same `favorites.tsv` besides, so the
+ /// round-trip read at the end of `strip_order_...` could have been
+ /// reading the other test's writes.
+ ///
+ /// It does not surface on its own — 40 runs at 8 threads, zero failures,
+ /// so the window is narrow. It is not theoretical either: steering the
+ /// second test's wipe into the first's write-then-read window with a
+ /// 50 ms delay failed it 10 times out of 10, the round-trip read coming
+ /// back empty because the file had been deleted under it. Narrow is the
+ /// argument for fixing it rather than against — a race this rare surfaces
+ /// as one unreproducible CI failure, in a test that failed for a reason
+ /// nowhere in its own body.
+ fn store(name: &str) -> Favorites {
+ let dir = std::env::temp_dir()
+ .join(format!("cce-browser-favs-{}-{}", std::process::id(), name));
let _ = fs::remove_dir_all(&dir);
Favorites { entries: Mutex::new(Vec::new()), path: dir.join("favorites.tsv") }
}
@@ -595,7 +613,7 @@ mod tests {
#[test]
fn strip_order_is_insertion_order_and_shifts_move_one_place() {
- let f = store();
+ let f = store("strip-order");
f.add("https://a.example/", "A");
f.add("https://b.example/", "B");
f.add("https://c.example/", "C");
@@ -634,10 +652,11 @@ mod tests {
#[test]
fn internal_pages_are_refused() {
- let f = store();
+ let f = store("internal-pages");
f.add("cce://history", "History");
f.add("about:blank", "");
assert!(f.snapshot().is_empty());
+ let _ = fs::remove_dir_all(f.path.parent().unwrap());
}
}