Skip to content

test: give each drained write its own deadline in the short-chunks reader test - #13999

Merged
teamleaderleo merged 1 commit into
mainfrom
fix/async-reader-per-write-deadline
Sep 23, 2026
Merged

teamleaderleo merged 1 commit into
mainfrom
fix/async-reader-per-write-deadline

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

asyncReaderSurvivesManyShortChunksAheadOfTheConsumer fails on a loaded host with nothing wrong in the reader. It writes 200 one-byte chunks and, after each, polls in 1 ms sleeps until the reader has drained it, but all 200 polls share one 5 s deadline. When each 1 ms sleep wakes tens of milliseconds late, the shared budget runs out partway through and the test fails at try #require(drainedEveryWrite).

Each write now gets its own 5 s deadline. A reader that stops draining still fails, on the write where it stalls; a slow host no longer adds up 200 slow wakeups against one budget. Test-only change.

Testing

Full CmuxControlSocket package suite (swift test), five runs each on the same shared Mac at load average 215–275 from concurrent peer builds:

runs failed at drainedEveryWrite
upstream/main 5 2 (suite took 12.2 s and 5.1 s)
this change 5 0 (slowest suite 6.5 s)

Five runs each is a small sample. It shows the failure reproduces on main under load and did not recur with the change; it does not bound the remaining flake rate. The test passed 3 of 3 when run alone at load ~100, which is why it shows up mainly under full-suite parallelism.

Demo Video

Not applicable: test-only change, no UI or runtime behavior.

Checklist

  • I tested the change locally
  • I added or updated tests for behavior changes
  • For iOS connectivity, auth, lifecycle, workspace or terminal changes, I updated the deterministic soak coverage — not applicable
  • I updated docs/changelog if needed — not needed
  • I requested bot reviews after my latest commit
  • All code review bot comments are resolved
  • All human review comments are resolved

— Ibex g1 🌿
Run: run_cmux_section_a_landing_review_repairs_and_test_flake_repairs_2026_09_23_20260923_fca3ce2b

🤖 Generated with Claude Code


Summary by cubic

Fixes asyncReaderSurvivesManyShortChunksAheadOfTheConsumer failing on loaded hosts. The test polled all 200 one-byte writes against one shared 5 s deadline, and late 1 ms sleep wakeups under load exhausted that budget partway through, failing the test with nothing wrong in the reader. Each write now gets its own 5 s deadline, so a reader that stops draining still fails on the write where it stalls. Test-only change.

Written for commit 34a08b4. Summary will update on new commits.

Review in cubic

…ader test

`asyncReaderSurvivesManyShortChunksAheadOfTheConsumer` polled for 200
serial drains against one 5 s deadline. Each poll sleeps 1 ms, and on a
loaded host each sleep wakes much later, so the shared budget ran out
partway through and the test failed at `drainedEveryWrite` with nothing
wrong in the reader. Each write now gets its own 5 s, so a reader that
stops draining still fails on the write it stalls on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 10 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: bb572f04-e652-4158-bdeb-0ad5ad25fc32

📥 Commits

Reviewing files that changed from the base of the PR and between db5d212 and 34a08b4.

📒 Files selected for processing (1)
  • Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlClientAsyncTransportTests.swift

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Self-review before auto-merge.

  • Scope: one test, test-only. The deadline moves inside the loop; the poll condition, the queued == index + 1 check, and the #require are unchanged, so a reader that stops draining still fails, on the write where it stalls.
  • Worst case: a drain that stalls just under 5 s on every write would now take up to 200 × 5 s to fail. That needs a reader that almost works on every chunk; the old shared budget would have failed it at the first write past 5 s total, so this is not a new blind spot.
  • Evidence: 2/5 failures on upstream/main under load, 0/5 with the change, same host and hour. The flake also reproduced twice more on air-blue today while validating Name the workspace that workspace.reorder could not resolve #13961 (load 115 and 191), both on this assertion.
  • CI: 22 pass, 20 skipped, no failures; no bot findings.

Enabling squash auto-merge with the user's go-ahead.

— Ibex g1 🌿

@teamleaderleo
teamleaderleo merged commit 5d1ecb8 into main Sep 23, 2026
46 of 48 checks passed
rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 23, 2026
06c2101 ci: route streamed validation by capability instead of by lane name (manaflow-ai#14002)
1773c54 ci(e2e): start builds from main's DerivedData so test-only changes skip the app compile (manaflow-ai#14016)
c890374 ci: pin the nightly runner guards to the whole expression (manaflow-ai#13997)
8abd2e9 ci: flag condition polls bounded by a Task.yield() count (manaflow-ai#14019)
e4ca672 ci(ios): record the cmux.app upload once Apple accepts it (manaflow-ai#14014)
260b648 ci: check what the runner variables hold, not just what the workflows say (manaflow-ai#13992)
25ad5af feat(terminal): opt-in macOS text-editing gestures at the shell prompt (manaflow-ai#13921)
daf9649 test: drop six focus-history cases superseded by FocusHistoryScopeTests (manaflow-ai#13975)
11202e3 Name the workspace that workspace.reorder could not resolve (manaflow-ai#13961)
2a4f3f6 fix(fork): make the fallback refresh await its own queued validation (manaflow-ai#13960)
4b82298 ci: let test-depot run one app-host test by selector (manaflow-ai#14001)
5d1ecb8 test: give each drained write its own deadline in the short-chunks reader test (manaflow-ai#13999)

# Conflicts:
#	.github/workflows/ci-guards.yml
#	.github/workflows/ci-health-report.yml
#	.github/workflows/ios-appstore-upload.yml
#	.github/workflows/ios-streamed-validate.yml
#	.github/workflows/iroh-release-gate.yml
#	.github/workflows/nightly.yml
#	.github/workflows/test-depot.yml
#	.github/workflows/test-e2e.yml
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