Test teardown: `TestUtils.DeleteDirectory` retries forever on a permanent error, and the 60s cap abandons the spinning thread
Nobody has claimed this yet.
- Dominant language
- C#
- Stars
- 12k
- Forks
- 703
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 36
Description
### Describe the bug
`TestUtils.DeleteDirectory` (`test/standalone/Garnet.test/TestUtils.cs:1128`) retries indefinitely on any `IOException`, with no attempt cap, no backoff, and no distinction between a *transient* error (a file handle briefly held after `Dispose()`, which the retry is designed for) and a *permanent* one (`PathTooLongException`, which derives from `IOException` and will never succeed on retry).
Two loops, both unbounded:
```csharp
// Loop 1 — bare catch, no cap, and does not honor `wait` at all
while (true)
{
try
{
if (!Directory.Exists(path)) return;
foreach (string directory in Directory.GetDirectories(path))
DeleteDirectory(directory, wait);
break;
}
catch { }
}
// Loop 2 — retries forever when wait: true
catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)
{
if (!wait) { /* one more try, then give up */ return; }
retry = true;
_ = Thread.Yield();
}
```
`Thread.Yield()` is not a sleep, so this is a **hot spin at 100% CPU**, not a wait.
This is measured, not inferred. Sampling the stalled run: two `testhost` processes each accumulated **4.94 s of CPU across a 5 s wall-clock window** — a full core pegged apiece, roughly 177 s of CPU in three minutes of "hanging". A deadlock or a wait would consume none. Only one test directory was touched during the entire stall window, matching one of the spinning processes, so the spin is localized to a single `DeleteDirectory` call.
The silence is explained by NUnit reporting a test only after its teardown completes: the test body has already passed, so a teardown that never returns produces no result line, no failure, and no output at all.
That the trigger is path length is directly demonstrable. The same 154 leftover directories that the harness spins on forever were each deleted **instantly** via `[IO.Directory]::Delete("\\?\" + path, true)`. The only difference is the extended-length prefix.
**The amplifier.** `ClusterTestContext.TearDown` bounds the *wait* but not the *work* (`ClusterTestContext.cs:210`):
```csharp
if (!Task.Run(() => TestUtils.DeleteDirectory(TestFolder, true)).Wait(TimeSpan.FromSeconds(timeoutSeconds)))
failureReason ??= "Timed out DeleteDirectory";
```
`Task.Run(...).Wait(timeout)` does not cancel the task. When it times out, the test reports `Timed out DeleteDirectory (primary failure — test itself passed)` and moves on — while the abandoned thread keeps spinning at full CPU **for the remainder of the test process**.
This makes the failure self-amplifying: each timed-out teardown permanently leaks a CPU-burning thread, which slows everything after it, which causes more timeouts. In a run with 37 such teardown timeouts, the run ends with up to 37 threads spinning. This is easily mistaken for ambient "machine load" — we initially diagnosed it that way — when it is actually generated by the test run itself.
The standalone path is worse in one respect: `TestUtils.OnTearDown` → `DeleteDirectory(MethodTestDir, wait: waitForDelete)` (`TestUtils.cs:1356`) has **no timeout wrapper at all**, so a permanent error there spins forever with no output and no failure. **This is confirmed by a controlled comparison.** `dotnet test test/standalone/Garnet.test.scripting --filter MultiDatabase` hangs indefinitely after printing `A total of 1 test files matched the specified pattern.` with no further output, in the 64-character worktree. The identical command, same machine, same filter, run in the 25-character worktree:
```text
A total of 1 test files matched the specified pattern.
Passed! - Failed: 0, Passed: 31, Skipped: 4, Total: 35, Duration: 10 s
```
Ten seconds versus an indefinite hang, with worktree path length as the only difference. It hangs rather than failing precisely because this path has no timeout wrapper — the cluster path would have reported `Timed out DeleteDirectory` after 60 s and continued.
**Triage note: the two symptoms look like unrelated problems and are one defect.** A run of noisy 60 s teardown failures whose test bodies all passed, and a single silent hang producing no output at all, present nothing in common. We investigated them separately and reached a wrong conclusion about each — the cluster noise was written off as ambient machine load, the standalone hang as an unrelated pre-existing issue — before finding they share this root cause. Anyone triaging either symptom in isolation is likely to repeat that.
### Steps to reproduce the bug
Any test whose `MethodTestDir` descendants exceed `MAX_PATH` (260) reproduces it. The practical trigger is checkout path depth, which varies per working copy:
1. Clone or create a worktree whose directory name is long, e.g. `garnet/tedhar-issue-2150-fail-on-recovery-error-true-still-serve-18dd9c` (64 chars).
2. Run `dotnet test test/cluster/Garnet.test.cluster.replication/Garnet.test.cluster.replication.csproj -f net10.0 -c Debug`.
3. Observe many `Timed out DeleteDirectory (primary failure — test itself passed)` failures, each taking 60 s, with test bodies passing.
4. Observe CPU remaining pegged after those tests have "finished".
When reproducing, ensure no other test process is bound to the same sub-project port (`Garnet.test.scripting` uses 34800), since a competing run — especially one using `--Blame:"CollectHangDump"` — produces its own stalls and confuses attribution.
Measured, same suite and machine, differing only in worktree path length:
| Worktree dir name | Length | `Timed out DeleteDirectory` |
|---|---|---|
| `tedhar-effective-carnival` | 25 | 0 |
| `tedhar-issue-2150-fail-on-recovery-error-true-still-serve-18dd9c` | 64 | 34-37 |
Reverting library changes does not affect the count (34 reverted vs. 37 modified — run-to-run variance), confirming it is environmental rather than a product regression. Individual files were observed at 265 characters against the 260 limit.
### Expected behavior
1. **Do not retry a permanent error.** `PathTooLongException` (and arguably `DirectoryNotFoundException`) should fail fast rather than being swallowed into the generic `IOException` retry, since no number of retries can succeed.
2. **Bound the retry** with an attempt or deadline cap plus real backoff, rather than an unbounded `Thread.Yield()` spin.
3. **Do not leave abandoned work running.** Pass a `CancellationToken` that the retry loop observes, so the 60 s cap actually stops the spin instead of orphaning it.
4. **Remove the bare `catch { }`** in the first loop, which currently ignores `wait` entirely and can spin with no exit for a persistent failure.
5. Report the underlying exception in the failure message. `Timed out DeleteDirectory` gives no indication that the real cause was a path-length error, which is what made this expensive to diagnose.
Item 3 is the most valuable in isolation: it converts a run-poisoning cascade into a single localized failure.
### Release version
Current `main` at [`277ea6c`](https://github.com/microsoft/garnet/commit/277ea6c34def67107bd643dd55e269b4e4c0964b).
### OS version
Windows, .NET 10, `net10.0` Debug. Windows-specific in its usual trigger (`MAX_PATH`), but the unbounded-retry and abandoned-thread defects are platform-independent — any persistent `IOException` or `UnauthorizedAccessException` (a locked file, a permissions problem) produces the same cascade on Linux.
### Additional context
Found while validating PR #2145 and PR #2153, where two sessions on the same machine got 2 and 43 failures from the identical suite and commit. The entire difference was worktree path length; the apparent "flakiness" was neither flaky nor load-related.
PR #2147 enables extended-length test paths, which removes the common *trigger*. It does not address the unbounded retry or the abandoned thread, so any other permanent `IOException` would still poison a run the same way. These are worth fixing independently.
Distinct from #2149 (concurrent checkpoint-metadata reads) and #2150 (fail-open recovery); this is purely test infrastructure and affects no product code.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with test/standalone/Garnet.test/TestUtils.cs:1128 and ClusterTestContext.cs:210, then trace OnTearDown at TestUtils.cs:1356 and the DeleteDirectory callers. Reproduce with the stated dotnet test commands in both worktree path lengths; done means permanent errors are reported, retries are bounded with cancellation and backoff, and timed-out teardown leaves no spinning work.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100