Is overwriting_files_and_lone_directories_works flaky?

[EliahKagan]

I noticed in d7fa797, which was the initial empty version of the first commit in #1785 as work began on it, that test-fast failed on Windows:

        FAIL [   0.018s] gix-worktree-state-tests::worktree state::checkout::overwriting_files_and_lone_directories_works
──── STDOUT:             gix-worktree-state-tests::worktree state::checkout::overwriting_files_and_lone_directories_works

running 1 test
test state::checkout::overwriting_files_and_lone_directories_works ... FAILED

failures:

failures:
    state::checkout::overwriting_files_and_lone_directories_works

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 14 filtered out; finished in 0.00s

──── STDERR:             gix-worktree-state-tests::worktree state::checkout::overwriting_files_and_lone_directories_works
thread 'state::checkout::overwriting_files_and_lone_directories_works' panicked at gix-worktree-state\tests\state\checkout.rs:182:9:
The probe must detect to be able to generate symlinks
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This looks familiar, but I'm not sure I've seen it before. I was unable to reproduce it, and it wasn't due to changes in that commit, since there were no changes in that commit.

Although I don't think this is what I was reminded of, it somewhat resembles #1373, but I believe that is fixed.

Is this a bug on the symlink detection on Windows--or perhaps in general?--where it nondeterministically fails on occasion? The GitHub-hosted Windows runners do allow symlinks to be created (there are various ways this can be configured, but the reason it works on the GHA Windows runners is that the runner software is executing as an administrator with full trust).

(In the same workflow run, the test-fixtures-windows job passed, which also includes that test, and I don't know of a way running fixture scripts would prevent that from happening. So this is another reason to think the failure is nondeterministic rather than being triggered by something about that situation, such as the commit making no changes.)

[Byron]

I believe to remember that the non-determinism comes from the probe itself. Initially it was racy, and I think I did combat this by putting it behind a lock and shared memory within a single test binary. This may mean that when multiple test binaries are run in parallel, they'd still step on each other's feet.

But… a quick check reveals that this should now be permanently fixed, assuming the random number generator is automatically seeded differently among different binaries. A trait I'd expect but didn't verify.

gitoxide/gix-fs/src/capabilities.rs

Lines 106 to 113 in bec648d

	let rand = fastrand::usize(..);
	let link_path = root.join(format!("__file_link{rand}"));
	if crate::symlink::create("dangling".as_ref(), &link_path).is_err() {
	return Ok(false);
	}
	let res = std::fs::symlink_metadata(&link_path).map(|m| m.file_type().is_symlink());
	crate::symlink::remove(&link_path).or_else(|_| std::fs::remove_file(&link_path))?;

[Byron]

tempfile can certainly be told to create a file in a particular directory, even though I keep thinking that it's better 'learn' how to create uniquely named files or symlinks instead. If I remember correctly, tempfile also does this with a closure that attempts to create a particular file with a generated name, and that retries automatically.

With that said, right now the probe is quite similar to the related Git code, so I'd hope it doesn't have to become more complex than that. On the other hand, the Git code isn't thread-safe either, but nobody seemed to have noticed yet (or it wasn't ever a problem). Thus one might have to go with more complex logic, which might be generalizable to "create a filesystem entry without collision (file or link)".

[EliahKagan]

tempfile can certainly be told to create a file in a particular directory, even though I keep thinking that it's better 'learn' how to create uniquely named files or symlinks instead.

Do you mean that we should have code that uses the same general technique, or that tempfile should be enhanced to be able to create things other than regular files and directories?

[Byron]

That's a valid question to ask! I thought about implementing it here as it's hard to argue why tempfile would expose its implementation detail as public API.

[EliahKagan]

This point you raised initially in #1789 (comment) turns out to be the key:

But… a quick check reveals that this should now be permanently fixed, assuming the random number generator is automatically seeded differently among different binaries. A trait I'd expect but didn't verify.

We are using fastrand per-thread Rng instances, which are automatically seeded based on the current monotonic clock time (which typically ticks very quickly, but even if so, can occasionally be the same in separate parallel checks), and the thread ID (which is usually a small number, with 1 being common). No attempt is made to distinguish separate processes.

We are able to observe this occasionally when running the test suite on CI because nextest uses separate processes, rather than separate threads, to run parallel tests. While I was thinking about Windows when I opened this question, I don't believe the problem is at all specific to Windows.

Whether the approach used in the symlink probe is changed or not, I believe that cannot solve the problem here, which is broader, because:

• It also affects the use of pseudorandom numbers in backoff, where randomized jitter is used to avoid some cases of the thundering herd problem.
• The backoff facilities are public, so other crates may be using them, too.

I've opened #1816 to present the issue in greater detail.

[EliahKagan]

By the way, I've updated the title here to avoid creating the impression that the problem is specific to Windows (for people who may find it while searching for something different but related, or otherwise who are looking at it cursorily).