Skip to content

Fix flaky SessionFs workspace metadata E2E test#1599

Merged
stephentoub merged 1 commit into
mainfrom
stephentoub/special-barnacle
Jun 8, 2026
Merged

Fix flaky SessionFs workspace metadata E2E test#1599
stephentoub merged 1 commit into
mainfrom
stephentoub/special-barnacle

Conversation

@stephentoub

Copy link
Copy Markdown
Collaborator

Why

The Should_Write_Workspace_Metadata_Via_SessionFs C# E2E test fails intermittently in CI (e.g. this run) with:

Assert.Contains() Failure: Sub-string not found
String:    ""
Not found: "<session-id>"

Root cause

The test waited only for workspace.yaml to exist before reading it. The runtime writes that file via the SessionFs provider using a truncate-then-write (File.WriteAllTextAsync), so File.Exists returns true the moment the file is created/truncated to 0 bytes, before the content is flushed. The test could read the empty file in that window and fail the assertion. The same race existed in the sibling plan.md test.

Fix

Wait for the expected content (not just existence) before asserting. Since the content-aware wait already throws on timeout, the trailing Assert.Contains calls became redundant and were removed, leaving the wait as the assertion (matching the existing index.md wait-only style in this file).

I checked the equivalent test in every other language suite:

  • Python: had the same disk-read race; switched both the workspace.yaml and plan.md tests to the already-present (but unused) wait_for_content helper.
  • Go: already robust (waitForFileContent), no change.
  • Node: not susceptible; it uses an in-memory provider with atomic writes and reads back through the provider rather than raw disk. No change.
  • Rust / Java: no equivalent SessionFs workspace-metadata test exists. No change.

Validation

Both .NET target frameworks (net8.0 + net472) build clean, and ruff check passes on the Python file.

The Should_Write_Workspace_Metadata_Via_SessionFs test waited only for
workspace.yaml to exist before reading it, but the runtime creates the
file (truncating to 0 bytes) before the content write completes. This
let the test read an empty file and fail the session-id assertion.

Wait for the expected content (not just existence) before asserting in
both the .NET workspace.yaml and plan.md tests, and drop the now-redundant
trailing asserts. Apply the same content-aware wait to the Python suite
via its existing wait_for_content helper. Go already waits for content and
Node uses an in-memory provider with atomic writes, so neither was affected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 8, 2026 15:31
@stephentoub stephentoub requested a review from a team as a code owner June 8, 2026 15:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces intermittent CI failures in the SessionFs workspace-metadata E2E tests by switching from “wait for file existence” to “wait until file contains expected content”, avoiding a race where the runtime truncates/creates the file before content is fully written.

Changes:

  • Python: replace wait_for_path + read_text + assert with a single wait_for_content(...) for workspace.yaml and plan.md.
  • .NET: update WaitForConditionAsync predicates to require both file existence and expected content before proceeding (removing the follow-up Assert.Contains that could still race on empty reads).
Show a summary per file
File Description
python/e2e/test_session_fs_e2e.py Uses wait_for_content for workspace.yaml and plan.md to avoid empty/partial reads immediately after file creation/truncation.
dotnet/test/E2E/SessionFsE2ETests.cs Waits for workspace.yaml / plan.md to contain expected substrings (session id / plan text) rather than only waiting for the files to exist.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 0

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

I reviewed this PR against all six SDK implementations. The changes are consistent and the PR description's analysis is accurate.

SDK Status Notes
.NET ✅ Fixed in this PR Both workspace.yaml and plan.md tests updated to wait for content, not just existence
Python ✅ Fixed in this PR Both tests switched to wait_for_content helper
Go ✅ Already robust Uses waitForFileContent which polls for both file existence and content
Node.js ✅ Not susceptible Uses MemoryProvider (in-memory); reads go through the provider API, which has atomic semantics — no disk write race
Rust ✅ N/A No equivalent SessionFs workspace-metadata E2E test exists
Java ✅ N/A No equivalent SessionFs workspace-metadata E2E test exists

No further changes needed for cross-SDK consistency.

Generated by SDK Consistency Review Agent for issue #1599 · sonnet46 841.6K ·

@stephentoub stephentoub merged commit 7d6bc92 into main Jun 8, 2026
22 checks passed
@stephentoub stephentoub deleted the stephentoub/special-barnacle branch June 8, 2026 15:54
edburns pushed a commit that referenced this pull request Jun 8, 2026
The Should_Write_Workspace_Metadata_Via_SessionFs test waited only for
workspace.yaml to exist before reading it, but the runtime creates the
file (truncating to 0 bytes) before the content write completes. This
let the test read an empty file and fail the session-id assertion.

Wait for the expected content (not just existence) before asserting in
both the .NET workspace.yaml and plan.md tests, and drop the now-redundant
trailing asserts. Apply the same content-aware wait to the Python suite
via its existing wait_for_content helper. Go already waits for content and
Node uses an in-memory provider with atomic writes, so neither was affected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

2 participants