[ci-fix-net11] De-flake Essentials.AI file-based tests (FileShare.ReadWrite) - #36604
[ci-fix-net11] De-flake Essentials.AI file-based tests (FileShare.ReadWrite)#36604github-actions[bot] wants to merge 1 commit into
Conversation
…estData with FileShare.ReadWrite Parallel xUnit test classes read the same TestData files via File.ReadAllLines, which defaults to FileShare.Read and races on Windows, intermittently throwing IOException 'The process cannot access the file because it is being used by another process'. Route all shared-file reads through a DataStreamsHelper helper that opens with FileShare.ReadWrite. No assertions changed. Refs: #36452 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run maui-pr-uitests |
|
/azp run maui-pr-devicetests |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Target test StreamingJsonDeserializerTests + FileBasedTests validated green on head 0f84184 — Windows Helix Unit Tests Debug + Release both succeeded (build 1511617). Windows-only tests, so no cross-platform gap. Remaining CI red is unrelated flake/baseline (AOT macOS #36081; iOS Blazor canceled/infra). De-flake introduces no muting or test weakening. Ready for maintainer review.
|
|
♻️ Attempt 1/10 — red is unrelated flake/baseline, not this fix. On
The target legs for this de-flake — Windows Helix Unit Tests (Debug) and (Release) — both
|
|
🎯 Target test validated green on The change is a genuine de-flake (replacing Transitioning from draft → ready for review and adding
|
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
kubaflo
left a comment
There was a problem hiding this comment.
🔍 AI-generated review (multi-model pipeline: Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro), on behalf of @kubaflo.
Multi-model consensus: the fix masks the symptom, not the root cause
All three models independently converged on the same mechanism, and I verified it against the source at this PR's head. The de-flake is a net improvement for the reported IOException, but it targets the wrong half of the race and can trade a loud failure for a silent torn read.
The real root cause (verified)
The PR premise — "multiple parallel xUnit classes using File.ReadAllLines conflict" — is not what causes the sharing violation. File.ReadAllLines opens with FileAccess.Read / FileShare.Read; concurrent pure readers can never conflict with each other.
The actual writer is here:
// src/AI/tests/Essentials.AI.UnitTests/Tests/JsonStreamChunkerTests/Integration.cs:31-33
// For debugging - write chunks to .txt file
var txtPath = DataStreamsHelper.GetTxtItinerary(fileName);
File.WriteAllLines(txtPath, chunks);- Writer (
JsonStreamChunkerTests.IntegrationTests) doesFile.WriteAllLines→FileMode.Create(truncate-then-write) onTestData/DataStreams/Itinerary/<name>.txt. - Reader (
StreamingJsonDeserializerTests.FileBasedTests, viaDataStreamsHelper.TxtItinerariesMemberData) reads those exact same*.txtfiles — a different class ⇒ parallel under xUnit.
The original IOException was reader (FileShare.Read) vs. concurrent writer — the writer's granted Write access is not permitted by the reader's FileShare.Read. The IOException is itself the proof the two collide.
Why FileShare.ReadWrite is a regression in disguise
Relaxing the reader to FileShare.ReadWrite lets the open succeed while the writer holds the file open. But because File.WriteAllLines truncates to 0 bytes first, a reader that opens inside the write window can now observe an empty or partially-written file → chunks short/empty → e.g. lastItinerary stays null (Assert.NotNull fails) or partial JSON fails an equivalence assert. The comment claiming this "makes the shared reads deterministic" is inaccurate — it makes them non-throwing, not deterministic; content-wise it becomes less deterministic. Net effect: a loud, obviously-infra IOException becomes a rare, hard-to-diagnose content flake that looks like a real test failure (or silently false-passes).
Model panel
| Model | Verdict | Note |
|---|---|---|
| Gemini 3.1 Pro | NEEDS_CHANGES | Root-cause misdiagnosis + silent-corruption race; revert and delete the debug-write |
| GPT-5.5 | NEEDS_DISCUSSION | Torn/partial-read risk from the concurrent truncating writer |
| Opus 4.8 | LGTM ( |
Confirms the identical writer/reader race; flags it as a follow-up |
Unanimous on the mechanism; the split is only on whether it blocks. Because a trivial, strictly-better fix exists that removes the root cause, we're landing on NEEDS_CHANGES.
Suggested fix (trivial, eliminates the root cause)
Prefer any of these over relaxing the share mode:
- Delete the debug-write at
Integration.cs:31-33— it's labeled "For debugging" and mutates checked-in fixtures that another parallel class reads. Removing it makes every access toItinerary/*.txta pure read, so no share-mode change is needed at all. (Recommended — smallest, and it's what makes the reads genuinely deterministic.) - Redirect the debug-write to a unique temp path outside the
Itinerary/*.txtread set (e.g.Path.GetTempFileName()), so no test writes a file another test reads. - Put the writer and reader classes in the same xUnit
[Collection]to serialize them.
What's genuinely good here
ReadAllLinesSharedis rigorously equivalent toFile.ReadAllLines(UTF-8 + BOM detection, all line terminators, single trailing newline, empty-file → empty array) and is stateless/thread-safe — all three models verified this.- All racing read sites are covered (direct + via
GetFileLines). - CI red is not PR-caused — the failing legs are UI/device-test builds and AOT-Blazor integration; none touch
Essentials.AI.UnitTests.
Bottom line: keep the equivalence work if you like, but fix the writer — dropping the Integration.cs:31-33 debug-write is the clean, root-cause fix. As written, the PR reduces the IOException frequency at the cost of a subtler torn-read possibility on the same shared files.
ℹ Automated multi-model review. This is advisory — a human maintainer makes the final merge decision.
|
/azp run maui-pr |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Summary
Fixes an intermittent
IOException("The process cannot access the file because it is being used by another process") in theEssentials.AI.UnitTestsfile-based tests on Windows Helix.Root cause: Multiple parallel xUnit test classes (
StreamingJsonDeserializerTests.FileBasedTests,JsonStreamingRoundtripTests.FileBasedTests,JsonStreamChunkerTests.IntegrationTests) read the same sharedTestData/DataStreamsfiles viaFile.ReadAllLines(...).File.ReadAllLinesopens withFileShare.Read, so a concurrent opener that holds a write/exclusive handle races and throws on Windows.Fix: Added
DataStreamsHelper.ReadAllLinesShared(path)which opens the file withFileShare.ReadWriteand routes all shared-file reads through it. This is a pure test-synchronization de-flake — no assertions were changed or weakened, and no test was muted/ignored/retried.Changed call sites (10, test project only)
TestHelpers/DataStreamsHelper.cs(new helper +GetFileLines)Tests/StreamingJsonDeserializerTests/FileBasedTests.cs(6)Tests/JsonStreamingRoundtripTests/FileBasedTests.cs(2)Tests/JsonStreamChunkerTests/Integration.cs(1)Validation
Not run locally — the failure is a Windows-only, intermittent parallel-file-access race (2/10 builds) that is not deterministically reproducible on this Linux runner. The change is confined to the test project and preserves all assertions; correctness is validated by CI.
Target branch: net11.0
Refs: #36452
Attempt: 1/10