Fix existence cache kind poisoning

[AlesProkop]

Fixes #13566

Context

CachingFileSystemWrapper used a single existence cache for FileExists, DirectoryExists, and FileOrDirectoryExists. That allowed a FileExists(directoryPath) miss to poison a later DirectoryExists(directoryPath) lookup, which could make project-level wildcard expansion return empty results after calls such as GetDirectoryNameOfFileAbove(..., '').

Changes Made

• Split cached file, directory, and file-or-directory existence checks by query kind.
• Added direct regression coverage for file/directory existence cache poisoning.
• Added an evaluation-level regression for GetDirectoryNameOfFileAbove(..., '') followed by project-level wildcard item expansion.

Compatibility

This is a bug fix that restores correct filesystem query semantics. It does not add warnings, errors, public API, or ChangeWave-gated behavior.

Testing

• dotnet test src\Build.UnitTests\Microsoft.Build.Engine.UnitTests.csproj -f net10.0 -- --filter-class "*ProjectEvaluationContext_Tests"
• Reproduced the original issue with installed dotnet, then verified it passes with artifacts\bin\bootstrap\core\dotnet.exe from this branch.
• Reproduced and verified the pure MSBuild .proj repro with installed dotnet msbuild vs this branch's bootstrap.

[Copilot]

Pull request overview

This PR fixes a correctness bug in MSBuild’s evaluation file system caching where a cached FileExists(path) miss could incorrectly affect later DirectoryExists(path) queries for the same path, leading to incorrect empty wildcard expansions in evaluation scenarios like GetDirectoryNameOfFileAbove(..., "") followed by project-level globbing.

Changes:

• Split the existence cache in CachingFileSystemWrapper by query kind (file, directory, file-or-directory) to prevent cross-kind cache poisoning.
• Added a direct regression unit test that validates file/directory existence checks don’t interfere with each other in the cache.
• Added an evaluation-level regression test covering GetDirectoryNameOfFileAbove(..., "") followed by wildcard item expansion across all EvaluationContext sharing policies.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Framework/FileSystem/CachingFileSystemWrapper.cs	Splits existence caching into separate dictionaries per existence query kind to restore correct semantics.
src/Build.UnitTests/Definition/ProjectEvaluationContext_Tests.cs	Adds regression tests for existence-cache poisoning and the GetDirectoryNameOfFileAbove(..., "") + wildcard expansion scenario.

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Code Review — PR #14249 "Fix existence cache kind poisoning"

24-dimension review complete. No blocking or major findings. The fix is correct, minimal, and well-tested.

Root cause & fix assessment

The bug was real and the fix is the right approach: FileExists, DirectoryExists, and FileOrDirectoryExists have distinct semantics and must not share a cache. Three separate ConcurrentDictionary instances cleanly enforce this — each query kind now gets its own private namespace, eliminating cross-kind poisoning with no correctness tradeoff.

Findings summary

Dimension	Result
1. Backwards Compatibility	✅ LGTM — internal-only change, no observable API break
2. ChangeWave Discipline	✅ LGTM — pure bug fix, opt-out not needed
3. Performance & Allocation	✅ LGTM — 2 extra dicts allocated once per context; lambdas unchanged
4. Test Coverage	✅ LGTM — unit regression + integration regression across all 3 sharing policies
5. Error Message Quality	✅ LGTM — N/A
6. Logging & Diagnostics	✅ LGTM — N/A
7. String Comparison	✅ LGTM — key comparison unchanged (pre-existing ordinal)
8. API Surface	✅ LGTM — internal sealed, nothing public changed
9. Target Authoring	✅ LGTM — N/A
10. Design	✅ LGTM — three independent dicts is the idiomatic, obvious fix
11. Cross-Platform	✅ LGTM — no hardcoded separators, path handling unchanged
12. Code Simplification	⚠️ NIT — new ConcurrentDictionary<string, bool>() → new() (see inline)
13. Concurrency & Thread Safety	✅ LGTM — ConcurrentDictionary.GetOrAdd throughout; strictly safer than before
14. Naming Precision	✅ LGTM — _directoryExistenceCache / _fileExistenceCache / _fileOrDirectoryExistenceCache are clear
15. SDK Integration	✅ LGTM — N/A
16. Idiomatic C#	⚠️ NIT — same as dim 12 (see inline comment on line 17)
17. File I/O & Path Handling	✅ LGTM — path semantics unchanged
18. Documentation	✅ LGTM — self-documenting names; no docs needed
19. Build Infrastructure	✅ LGTM — N/A
20. Scope & PR Discipline	✅ LGTM — single concern, properly linked to #13566
21. Evaluation Model Integrity	✅ LGTM — fix makes evaluation more order-independent
22. Correctness & Edge Cases	✅ LGTM — original repro covered; reverse direction covered; FileOrDirectoryExists independence covered
23. Dependency Management	✅ LGTM — no new package references
24. Security	✅ LGTM — no security regression

One inline NIT

On CachingFileSystemWrapper.cs lines 15–17: the three new ConcurrentDictionary fields use verbose initializer syntax. The repo style (C# 14) prefers target-typed new(). This is purely cosmetic — the logic is correct as written.

Generated by Expert Code Review (on open) for #14249 · 1.1K AIC · ⊞ 30.3K