Skip to content

fix: provisioner falls back to cache on timeouts, self-heals its cache dir, orders versions by SemVer 2 - #28

Merged
Arthurvdv merged 6 commits into
mainfrom
fix/provisioner-robustness
Sep 20, 2026
Merged

Arthurvdv merged 6 commits into
mainfrom
fix/provisioner-robustness

Conversation

@Arthurvdv

@Arthurvdv Arthurvdv commented Sep 20, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Internal NuGet timeouts (resolve: 10s, download: 60s) now fall back to the newest valid cached analyzer version instead of leaving Ready = null for the entire session.
  • A half-extracted cache directory (from a crash during a previous provisioning) is now detected and replaced on the next startup, instead of being returned as-is and causing repeated download failures.
  • All three version-comparison call sites (NuGet version resolution, cache fallback, DevTools tool-store probe) now use a shared SemanticVersion type with full SemVer 2 prerelease ordering, fixing 1.3.0-preview.10 < 1.3.0-preview.9 and base-only prerelease tie-breaks.
  • Orphaned .tmp-* extraction directories (from sessions where the cache was undeletable) are swept on startup; fresh ones (< 1 hour) are kept to avoid interfering with concurrent processes.
  • The undeletable-cache-directory test now runs on every OS, not just Windows.
  • A lock file (.in-use with FileShare.None) guards temp extraction directories from the sweep while another process is still extracting or using them as a session fallback.
  • The sweep now enumerates every TFM subdirectory of the cache root, not just the detected TFM.

What changed

  • AlcopsAnalyzerProvisioner: ResolveVersionAsync and DownloadAndExtractAsync translate their internal OperationCanceledException into TimeoutException so the existing when (ex is not OperationCanceledException) filters catch it and route to FallbackToCacheOrWarn. ExtractPackage deletes an incomplete targetDir before the move and re-validates after a concurrent-process race. FindNewestCachedVersion uses SemanticVersion instead of Version with the dash-strip trick. SweepStaleTempDirectories enumerates every immediate subdirectory of _cacheRoot (each a TFM folder), sweeps *.tmp-* directories older than one hour, and checks an .in-use lock file before deleting; held directories are skipped. ExtractPackage acquires the lock during extraction, releases it before Directory.Move, and re-acquires it in the return tempDir fallback for the lifetime of the process. The class now implements IDisposable (disposes _inUseLock and HttpClient). A guard before the fallback return tempDir throws IOException if the directory was swept, so the caller falls back to cache.
  • SemanticVersion (new): IComparable<SemanticVersion> with TryParse, IsStable, Raw, and a null-safe Comparer. Numeric identifiers compare numerically (arbitrary length); alphanumeric compare OrdinalIgnoreCase; stable > prerelease at equal base; build metadata ignored. The NullSafeComparer nested class is replaced by Comparer<SemanticVersion?>.Create(...).
  • NuGetVersions.Parse: replaced ParseVersion/CompareVersions with a single SemanticVersion.TryParse loop. The accumulator variable is renamed from prerelease to newest with a clarifying comment; the tuple element name Prerelease is unchanged.
  • BcToolsLocator.OrderByDescendingVersion: now internal static, uses SemanticVersion.Comparer.

Behaviour changes

  • A slow NuGet index or package download no longer leaves the session without analyzers when a cached version exists.
  • --alcops-analyzers prerelease now correctly resolves 1.3.0-preview.10 over 1.3.0-preview.9.
  • The DevTools tool-store probe orders prereleases correctly (numeric, not lexicographic).
  • ExtractPackage is now internal (was private) for testability.
  • Orphaned .tmp-* extraction directories are cleaned up on each startup.
  • Two concurrent alcops-mcp instances sharing the default cache root no longer risk one instance's sweep deleting a temp directory the other is actively using.
  • The sweep covers all TFM folders in the cache root, not just the detected TFM.
  • Environment.IsPrivilegedProcess replaces Environment.UserName == "root" in the undeletable-cache test.

Test plan

  • dotnet build --configuration Release — 0 warnings, 0 errors
  • dotnet test --configuration Release — 148 passed, 0 failed, 0 skipped
  • All 6 existing NuGetVersionsTests pass unchanged
  • All 12 existing BcToolsLocatorTests pass unchanged
  • All 6 existing AlcopsAnalyzerProvisionerTests pass unchanged (including Provision_SweepsStaleTempDirectories_KeepsFreshOnes)
  • All existing [AlMcpFact] integration tests pass unchanged
  • New SemanticVersionTests: 18 test cases (12 comparison theory, 1 parse-odd, 5 reject)
  • New NuGetVersionsTests: 5 (two-digit revision, list-order-independence, build-metadata raw return, stable outranks its own RC, newer prerelease outranks older stable)
  • New BcToolsLocatorTests: 1 (stable-then-prerelease-numerically, junk last)
  • New AlcopsAnalyzerProvisionerTests: 11 (index timeout, download timeout, caller-cancelled, half-extracted replace, target-already-valid, prerelease-numeric fallback, skip-invalid-and-tmp, undeletable cache cross-platform with lock assertions, stale temp directory sweep, sweep skips held directory, sweep covers every TFM folder)

🤖 Generated with Claude Code

Arthurvdv and others added 6 commits September 20, 2026 15:44
ResolveVersionAsync and DownloadAndExtractAsync use internal
CancellationTokenSources with CancelAfter for their 10s / 60s
deadlines. When the timer fires HttpClient throws
TaskCanceledException (an OperationCanceledException), which
the call-site filters in ProvisionCoreAsync exclude because they
only catch non-OCE exceptions. ProvisionAsync's OCE catch is also
false because the caller's token was not cancelled. Result: a slow
NuGet response sets Ready = null for the whole session even though
a valid cached version exists on disk.

Fix: translate the internal timeout into a TimeoutException inside
each method so the existing catch filters treat it as any other
transient network failure and reach FallbackToCacheOrWarn. Add
ct.ThrowIfCancellationRequested() before ExtractPackage so a
shutdown that arrives after the body download does not spend time
extracting. The ResolveTimeout and DownloadTimeout properties are
init-settable so tests can exercise the paths without real delays.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…urning it

ExtractPackage wraps Directory.Move in a catch (IOException) that
fires when targetDir already exists. It deletes the temp directory
and returns targetDir — but the caller only reached this path
because IsCacheValid(targetDir) was false (a previous run died
mid-extraction), so an invalid directory is returned and every
subsequent startup re-downloads and hits the same move failure.

Fix: before the move, if targetDir exists and is not valid, delete
it and let the move proceed. The IOException catch now re-checks
IsCacheValid: if a concurrent process placed a valid directory
there, log and use it; otherwise throw so FallbackToCacheOrWarn
can find the newest valid cached version instead.

ExtractPackage is now internal so the concurrent-process race path
can be unit-tested.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Three call sites compared version strings with ordinal or
base-version-only logic:

- NuGetVersions.Parse compared prerelease tie-breaks with
  StringComparer.OrdinalIgnoreCase, so 1.3.0-preview.10 sorted
  before 1.3.0-preview.9.
- FindNewestCachedVersion stripped everything after the first dash,
  making two prereleases with the same base version
  indistinguishable.
- BcToolsLocator.OrderByDescendingVersion had the same base-only
  ordering.

Fix: introduce SemanticVersion with full SemVer 2 ordering
(numeric identifiers compared numerically, stable > prerelease of
same base, NuGet-style case-insensitive alphanumeric comparison)
and a null-safe Comparer. All three call sites now use it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t be replaced

- ExtractPackage: when Directory.Move fails and targetDir is invalid,
  return the temp dir instead of deleting it and throwing (#1)
- Catch UnauthorizedAccessException alongside IOException on pre-move
  delete (#2)
- Log the pre-move delete failure at Warning, not Debug (#3)
- Pass the inner exception to TimeoutException in ResolveVersionAsync
  and DownloadAndExtractAsync (#4)
- Extract WriteManifest helper shared by SeedCache and
  SeedInvalidCache in tests (#6)
- Drop fully qualified System.Text.Json.JsonSerializer call in
  Fallback_SkipsInvalidAndTmpDirs test (#8)
- Add ExtractPackage_TargetInvalidAndUndeletable_ReturnsTempDir test

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eletable-cache path on every OS

- Add SweepStaleTempDirectories to clean up .tmp-* dirs older than 1 hour on each startup
- Rewrite ExtractPackage_TargetInvalidAndUndeletable test with cross-platform DirectoryLock helper (FileShare.None on Windows, remove write permission on Unix)
- Simplify SemanticVersion.Comparer from nested class to Comparer<T>.Create one-liner
- Rename NuGetVersions.Parse accumulator from prerelease to newest with clarifying comment
- Add tests: Provision_SweepsStaleTempDirectories_KeepsFreshOnes, Parse_Prerelease_StableOutranksItsOwnReleaseCandidate, Parse_Prerelease_NewerPrereleaseOutranksOlderStable

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cover every cache folder

- Lock file (.in-use) with FileShare.None protects temp directories from
  the sweep while another process is still extracting or using them as a
  session fallback.
- SweepStaleTempDirectories now enumerates every TFM subdirectory of the
  cache root instead of only the detected TFM.
- ExtractPackage acquires the lock during extraction, releases it before
  Directory.Move, and re-acquires it for the lifetime of the process in
  the return-tempDir fallback path.
- AlcopsAnalyzerProvisioner implements IDisposable (releases _inUseLock
  and HttpClient); DI disposes singletons at host shutdown.
- Guard before return tempDir: throws IOException if the temp directory
  was swept, so the caller falls back to cache.
- Replace Environment.UserName == "root" with Environment.IsPrivilegedProcess.
- New tests: Sweep_SkipsTempDirectoryHeldByAnotherProcess,
  Sweep_CoversEveryTfmFolder, and lock-release assertions in the
  undeletable-cache test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Arthurvdv
Arthurvdv merged commit dfcbd07 into main Sep 20, 2026
5 checks passed
@Arthurvdv
Arthurvdv deleted the fix/provisioner-robustness branch September 20, 2026 16:03
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.

1 participant