From 86fba9cc6a6abe8f303d8eae80602c655bb9a512 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 9 May 2025 21:02:33 -0400 Subject: [PATCH 1/4] Test `overwrite_existing` apparent symlink dereferencing To investigate #2006, this explodes the occasionally failing test `writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed` into 1000 identical tests, using `test_case::test_matrix`. When run in series, they all pass, but when run in parallel with `cargo nextest`, some failures always occur, at least when this is done on macOS. This is as predicted in: https://github.com/GitoxideLabs/gitoxide/issues/2006#issuecomment-2866226601 A more specific result in local testing on macOS 15 is that, while the number of failures is 0 with `--test-threads=1`, with 2 or more threads, the number of failures tends to *decrease* with the number of threads. The most failures occur with `--test-threads=2`, about 30 on the system tested, while `--test-threads=16` has about 10 failures. The tests were run with this command, with any `--test-threads=` argument added: cargo nextest run -p gix-worktree-state-tests writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed --no-fail-fast --- Cargo.lock | 34 ++++++++++++++++++++++ gix-worktree-state/tests/Cargo.toml | 1 + gix-worktree-state/tests/state/checkout.rs | 5 ++-- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a154c740c80..3e6feba3de8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2600,6 +2600,7 @@ dependencies = [ "gix-worktree-state", "once_cell", "symlink", + "test-case", "walkdir", ] @@ -4757,6 +4758,39 @@ version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8f50febec83f5ee1df3015341d8bd429f2d1cc62bcba7ea2076759d315084683" +[[package]] +name = "test-case" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eb2550dd13afcd286853192af8601920d959b14c401fcece38071d53bf0768a8" +dependencies = [ + "test-case-macros", +] + +[[package]] +name = "test-case-core" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "adcb7fd841cd518e279be3d5a3eb0636409487998a4aff22f3de87b81e88384f" +dependencies = [ + "cfg-if", + "proc-macro2", + "quote", + "syn 2.0.101", +] + +[[package]] +name = "test-case-macros" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c89e72a01ed4c579669add59014b9a524d609c0c88c6a585ce37485879f6ffb" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.101", + "test-case-core", +] + [[package]] name = "thiserror" version = "1.0.69" diff --git a/gix-worktree-state/tests/Cargo.toml b/gix-worktree-state/tests/Cargo.toml index 7f41c26467b..e4e5e0fba17 100644 --- a/gix-worktree-state/tests/Cargo.toml +++ b/gix-worktree-state/tests/Cargo.toml @@ -33,3 +33,4 @@ symlink = "0.1.0" once_cell = "1.21.3" walkdir = "2.3.2" +test-case = "3.3.1" diff --git a/gix-worktree-state/tests/state/checkout.rs b/gix-worktree-state/tests/state/checkout.rs index 764b52020d3..50ad0cd2d43 100644 --- a/gix-worktree-state/tests/state/checkout.rs +++ b/gix-worktree-state/tests/state/checkout.rs @@ -12,6 +12,7 @@ use gix_object::{bstr::ByteSlice, Data}; use gix_testtools::tempfile::TempDir; use gix_worktree_state::checkout::Collision; use once_cell::sync::Lazy; +use test_case::test_matrix; use crate::fixture_path; @@ -103,8 +104,8 @@ fn accidental_writes_through_symlinks_are_prevented_if_overwriting_is_forbidden( } } -#[test] -fn writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed() { +#[test_matrix(0..=999)] +fn writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed(_i: i32) { let mut opts = opts_from_probe(); // with overwrite mode opts.overwrite_existing = true; From 70f3b1c82140b08a16140dffa29124d0abd57b64 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 10 May 2025 07:41:51 -0400 Subject: [PATCH 2/4] Make `test-fast` CI matrix run all jobs even when one fails To see if this can sometimes reproduce #2006 even outside macOS. The `fail-fast: false` added here can be removed after testing. --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8728d06747f..31927f159f6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -164,6 +164,7 @@ jobs: - macos-latest - ubuntu-latest - ubuntu-24.04-arm + fail-fast: false runs-on: ${{ matrix.os }} From 1a8a85e94f2eb54881780c89d0fb1fd1ed8c108b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 18 May 2025 03:58:40 -0400 Subject: [PATCH 3/4] Produce clash on CI in GNU/Linux with case-folding ext4 This (from EliahKagan#36) adds a `test-ext4-casefold` matrix job definition to `ci.yml`, which is intended probably to be temporary, to demonstrate for #2008 that #2006 affects GNU/Linux (in addition to macOS and Windows), when the GNU/Linux system is using an ext4 volume with a case-insensitive directory tree, i.e., where the ext4 filesystem is created with `-O casefold` and operations are performed in a directory within that filesystem where `chattr +F` has been used to enable case-folding path equivalence. The bug is nondeterministic and challenging to produce on GNU/Linux, even when the case-folding precondition is satisfied (as all the `test-ext4-casefold` jobs achieve). It is much harder to produce on GNU/Linux than on macOS or Windows. Accordingly, the number of duplicate test cases is increased 25-fold from 1000 to 25000 in those tests, and multiple jobs are created, some of them equivalent. As expected, the failures do not occur when the runner does not run tests in parallel, and they do occur most of the time when the runner runs tests in parallel with a maximum number of parallel tests set to 2 or to 4. However, unexpectedly, the failures almost never occur when the test are run in parallel with a maximum number of parallel tests set to 3. It is unclear to what extent, if any, this generalizes, but I have not observed these patterns when testing locally, including when modifying the local procedure to be more similar to the behavior here (such as by suppressing reporting of non-failing tests and suppressing the progress bar). In local testing, I am unable to produce failures on some GNU/Linux systems, but on those on which I am able to produce them, they are easier to produce with all values of the `--test-threads` operand, including 3. Not all operating systems, versions, and kernel builds and options support case-folding on ext4. But support is fairly common on Linux-based systems, and the CI runners have no problem with that. In contrast, although case-folding tmpfs also exists on Linux, it is newer and not (yet) supported as widely, and the GitHub-hosted runners do not support case-folding tmpfs. It is possible to mount a tmpfs filesystem and create an ext4 image in it so that, in principle, fewer operations need to access the disk. That is one of the approaches that was tried, but it does not appear to make it any easier to surface these failures. This squashes multiple commits. (Mistakes, and less illuminating experiments, have already been omitted; the commits being squashes here are those that are expected to be of possible interest, but whose details are almost but not quite important enough to justify the cumbersome effect of having them individually in the history.) The squashed commits' individual messages are shown below. For full changes and CI results from each commit, see: https://github.com/EliahKagan/gitoxide/pull/36 --- * Add a case-folding GNU/Linux CI test job This can likely be removed later. But it might be kept if it finds anything interesting, especially if it helps test a bugfix. In that case, it may make sense to have it run only tests that are of special interest in connection with case-sensitivity. * Case-fold `$TMPDIR` as well in CI casefold test * Run only 2 parallel test processes in test-ext4-casefold This seemed to make the bug *easier* to reproduce (compared to larger values) locally, so maybe it will surface it on CI even in GNU/Linux. * Increase symlink experiment reps 10x in test-ext4-casefold From 1000 (0..=999) to 10000 (0..=9999). * Try even harder to reproduce the failure on GNU/Linux By repeating `cargo nextext` on the affected test group, up to 20 times. * Add `ubuntu-24.04-arm` to `test-ext4-casefold` job In case the failure might somehow be easier to reproduce there. * Use tmpfs casefold instead of ext4 casefold * Use tmpfs-backed ext4 as a workaround For when tmpfs does not suppport `-o casefold` in mounting. This does require that ext4 support `-O casefold`, but that was already working before. Even though that worked before, this could fail when using an ext4 image on tmpfs, because the ext4 image is smaller than it was when tmpfs was not used, in order to allow it to be created in memory. While this allows it to be created, the tests may fail if build artifacts and other files need more space than is available. * Run only the test(s) of interest In `test-linux-casefold`, this runs only the tests that are duplicates of each other, where the failure has been observed, rather than first running the whole test suite. The reason is to try to build fewer crates and test executables, since this is currenntly failing because it runs out of space on the ext4 image whose size is itself constrained by the amount of available memory for the tmpfs filesystem on which the image is created. * Don't attempt to use case-folding tmpfs But check if it is reported as available and, if so, issue a GHA warning that it should be used instead of the current way. * Try to fail at least as often, with less overhead This combines a few changes: - Only report the status of tests that actually fail. - Run the `cargo nextest` run command once, not 20 times. - Multiply the number of test cases by another factor of 10. - Stop after the first failing case. * Move the ext4 image off tmpfs This tries going back to not using tmpfs for I/O speedup, with the idea that the increased complexity and decreased flexibility might be possible to avoid now that other adjustments are made to surface the failure more reliably. * Try to fail more often, with less delay The `test-ext4-casefold` jobs were still not surfacing the error quite reliably enough. The wait was also longer than ideal. This commit makes some changes that hope to improve on both. - Decrease the number of duplicate test cases by a factor of 4. This has the effect that they are increased by 25x compared to the multiplier in the commited code, instead of 100x. - Create 10 times as many CI jobs in the matrix, by adding a `num` variable whose values are those in the range 0..=9. This variable is not actually used, it just serves to produce more CI jobs. - Don't use `Swatinem/rust-cache` in these jobs anymore. It is not obvious that it will make things work better overall, in terms of speedup, cache usage, and cache access, now that we are increasing the number of matrix jobs by a factor of 10. Moving some of the repetition into separate jobs, which may run in parallel, is not only, nor even primarily, to leverage runner parallelism to do more test runs faster. Instead, it is because it looks like some chaotic initial state may contribute to how likely the failure is to occur. No definitive experiment on CI shows this to be the case -- but in local testing, on some systems, I often have runs that fail early over and over again, and then wait a while, come back, and have many runs that don't surface the bug, then come back later and it is easy to produce again, and so on. * Further diversify runners * Undiversify runners but vary test parallelism Since varying `runs-on` didn't surface more errors, nor help smooth out inconsistencies across runs. The `*-arm` runners seem to have fewer errors with the particular way the tests are running now, so this just uses `ubuntu-latest`. This also adjusts `rep`, having it take on more values, so as to more than make up for what would otherwise be a smaller number of jobs. * Test more values for test parallelism This creates additional CI `test-ext4-casefold` jobs, for more values of the `--test-threads` operand passed to `cargo nextest`. The motivation is that the value of 3 curiously seems never to surface the failure on CI. Unlike 1, 3 should is expected to surface the failure comparably to 2 and 4, both of which do fail more often than not. Yet the jobs with `--test-threads=3` keep passing. (This is unlikely to be due to case-sensitivity misdetection, since the assertions for when the filesystem is case-sensitive would also fail, if they fire in a job where the filesystem is case-folding.) If these jobs are kept, then `rep` should probably be adjusted to have fewer, perhaps 7, values. However, this adjusts `rep` to have 16 values (i.e. twice as many as before), since the higher operands to `--test-threads` are expected to produce the failure less often. * Test fewer test parallelism values and reps - Test `--test-threads` operands of 1 to 4, rather than 1 to 7. - Do 7 reps instead of 16. In addition to the 1000x repetition in `checkout.rs` of the writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed test case -- which almost always surfaces #2006 on CI on macOS and Windows without further modifications -- the `test-ext4-casefold` jobs as they currenly stand seem like they would ber sufficient to test whether a possible future solution would fix the bug on GNU/Linux. This looks like about the minmum amount we should reasonably verify still fails (in some jobs) immediately before a #2006 fix, while no longer failing *any* jobs afterwards. But the `test-ext4-casefold` jobs are still much more than would be wanted on the main branch. Assuming the cause of the failures is the same on all systems, and the solution neither explicitly nor implicitly operates differently across systems (when comparing how it works when the filesystem is case-insensitive, that is), it may be enough on CI to regression test only macOS and Windows for #2006. Thus, this also adds a TODO comment atop the `test-ext4-casefold` definition reminding that it should not be kept, or not in full. --- .github/workflows/ci.yml | 69 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 31927f159f6..8470ff51722 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -185,6 +185,74 @@ jobs: - name: Check that tracked archives are up to date run: git diff --exit-code # If this fails, the fix is usually to commit a regenerated archive. + # TODO(integration): Remove all or at least most test-ext4-casefold jobs prior to merging #2008. + test-ext4-casefold: + strategy: + matrix: + # `--test-threads` operand + parallel-tests: + - 1 # Never fails. + - 2 # Usually fails. + - 3 # Almost never fails, somehow! + - 4 # Usually fails. + # Duplicate jobs + rep: [A, B, C, D, E, F, G] + fail-fast: false + + runs-on: ubuntu-latest + + steps: + - name: Warn if we could use tmpfs mounted with `-o casefold` instead + run: | + case "$(cat > "$GITHUB_ENV" + - name: Verify case folding in workspace and TMPDIR + run: | + set -ux + shopt -s nullglob + verify() { + touch a A + files=(?) + test "${#files[@]}" -eq 1 + rm a + } + verify + (cd -- "$TMPDIR"; verify) + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - uses: taiki-e/install-action@v2 + with: + tool: nextest + - name: More writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed reps + run: | + # Prepend leading digits "24" to the upper bound of the label range. + sed -Ei 's/^(#\[test_matrix\(0\.\.=)([[:digit:]]+\)])$/\124\2/' \ + gix-worktree-state/tests/state/checkout.rs + - name: Test writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed (nextest) + run: | + cargo nextest run -p gix-worktree-state-tests \ + writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed \ + --status-level=fail --test-threads=${{ matrix.parallel-tests }} + test-fixtures-windows: runs-on: windows-latest @@ -497,6 +565,7 @@ jobs: - test - test-journey - test-fast + - test-ext4-casefold - test-fixtures-windows - test-32bit - test-32bit-windows-size-doc From c5c08d2463b1c52188f2fde79455484d0099aeda Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 18 May 2025 07:18:17 -0400 Subject: [PATCH 4/4] Use single-threaded checkout in tests This change (from EliahKagan#37), which is intended to be temporary, sets `thread_limit: Some(1)` in the `opts_from_probe()` test helper function in the `checkout.rs` test module, so that tests including `writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed` will use single-threaded checkout. This applies no matter how the tests are run. In addition, for the `test-ext4-casefold` CI jobs (but not others), this adds the `--no-default-features` option to the `cargo nextest` command, so that the `gix-features-parallel` feature defined in `gix-worktree-state/Cargo.toml`, which when enabled causes `gix-features/parallel` to be enabled for the tests, is not enabled. The purpose of these changes it to examine if, when checkout operations are themselves performed without multithreading, there is any change to #2006. This does not make the failures go away. They still occur on all systems on which they had occurred before. It is unclear if it makes any difference to the failures. It seems like they may be slightly less likely to occur on GNU/Linux, but experiments so far are insufficient to confirm this. Furthermore, even if there is an effect, it may be that the effect is more subtle (such as possibly shifting the number of parallel tests where failures peak, from 2 to some higher number). The changes here should not be confused with the duplication of the test case, nor with the argument to `--test-threads`, which actually specifies the number of parallel test *processes*, since `cargo nextest` is being used. This squashes a few commits trying some minor variations. The original messages are shown below. For full changes and CI results from each commit, see: https://github.com/EliahKagan/gitoxide/pull/37 --- * Try to avoid parallel checkout in `test-ext4-casefold` This attempts to turn off parallelism within individual runs of writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed by passing `--no-default-features` to turn off the `gix-features-parallel` feature in `gix-worktree-state/Cargo.toml` (which is how `gix-features/parallel` is enabled in those tests). This is to check if the multithreading parallelism within the checkout operations participates in #2006. See: https://github.com/GitoxideLabs/gitoxide/issues/2006#issuecomment-2878911616 * Patch the `thread_limit` argument to `Some(1)` too In the `text-ext4-casefold` jobs. * Modify the test code to use single-threaded checkout This moves setting `thread_limit: Some(1)` out of being a step done with `sed` in the `test-ext4-casefold` jobs, and into being an actual change to the source code of the `checkout.rs` test module. This change is intended to be temporary. The goal is the same as before, but to observe the effect outside `text-ext4-casefold`. That is, the goal here is to temporarily see if there is a change in results in the other jobs where failure almost always occurs due to #2006, i.e., the `test-fast` jobs on `macos-latest` and `windows-latest`, and the `test-fixtures-windows` job. Note that, as of this change, the `gix-features/parallel` feature is only turned off in the `test-ext4-casefold` jobs. In others, that feature is still turned on, just a parallelism of 1, i.e., a single thread, is used for the checkout. But a parallelism of 1 seems usually to be special-cased to not use facilities related to multithreading. (This `thread_limit` argument change, as well as the immediately preceding change of disabling the `parallel` feature flag in the `test-ext4-casefold` test jobs, are entirely separate from how many writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed duplicated test cases there are, and also entirely separate from the operand to `--test-threads`, which actually controls the number of test *processes* `cargo nextest` creates to run the tests.) --- .github/workflows/ci.yml | 6 ++++-- gix-worktree-state/tests/state/checkout.rs | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8470ff51722..782f28ec034 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -242,14 +242,16 @@ jobs: - uses: taiki-e/install-action@v2 with: tool: nextest - - name: More writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed reps + - name: Increase writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed reps run: | # Prepend leading digits "24" to the upper bound of the label range. sed -Ei 's/^(#\[test_matrix\(0\.\.=)([[:digit:]]+\)])$/\124\2/' \ gix-worktree-state/tests/state/checkout.rs + - name: Display the changes made for this test + run: git diff - name: Test writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed (nextest) run: | - cargo nextest run -p gix-worktree-state-tests \ + cargo nextest run -p gix-worktree-state-tests --no-default-features \ writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed \ --status-level=fail --test-threads=${{ matrix.parallel-tests }} diff --git a/gix-worktree-state/tests/state/checkout.rs b/gix-worktree-state/tests/state/checkout.rs index 50ad0cd2d43..506b6471e06 100644 --- a/gix-worktree-state/tests/state/checkout.rs +++ b/gix-worktree-state/tests/state/checkout.rs @@ -689,10 +689,11 @@ fn probe_gitoxide_dir() -> crate::Result { fn opts_from_probe() -> gix_worktree_state::checkout::Options { static CAPABILITIES: Lazy = Lazy::new(|| probe_gitoxide_dir().unwrap()); + // FIXME(integration): Restore multithreaded `thread_limit` prior to merging #2008. gix_worktree_state::checkout::Options { fs: *CAPABILITIES, destination_is_initially_empty: true, - thread_limit: gix_features::parallel::num_threads(None).into(), + thread_limit: Some(1), // gix_features::parallel::num_threads(None).into(), ..Default::default() } }