Skip to content

fix(cloud): let stats reads cancel through the shared read coordinator - #14228

Merged
teamleaderleo merged 1 commit into
mainfrom
fix/vm-stats-single-flight-cancellation
Sep 24, 2026
Merged

teamleaderleo merged 1 commit into
mainfrom
fix/vm-stats-single-flight-cancellation

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Why

Three VMClientReadCoalescingTests cases fail on every main full-suite run since #13327 landed (06:35Z on 09-24):

  • "Overlapping machine stats callers share one HTTP request"
  • "A failed stats sample clears the last live reading"
  • "An authoritative fleet replaces the old stats batch and preserves other readers"

The failures point at a real behavior regression, not just stale tests.

#13151 (merged ~03:00Z) added CloudReadRequestCoordinator. It already shares every concurrent /stats GET per account, team, and auth generation, and it cancels the HTTP read once its last caller leaves. #13327 was written before that landed. It added a second single-flight layer inside VMResourceStatsStore that runs the fetch in an unstructured Task. That layer has two effects:

  • The coordinator now sees one waiter per machine instead of one per caller, so the tests' waiter accounting times out after 10 s.
  • The store-level task never cancels. A removed machine or a hidden panel keeps its stats request alive until the 30 s timeout. The fleet test catches exactly that: /api/vm/removed/stats stays in flight after the fleet drops the machine.

What changed

This reverts #13327:

  • VMClient.stats fences each caller with beginRead/finishRead. That keeps the revision and sequence ordering the store owns, so resize, reset, and removal still reject late completions.
  • The network read is shared through the coordinator.
  • VMResourceStatsStore.read(machineID:), its readTask field, and the tests that only covered that method are removed.

VMClientReadCoalescingTests covers the same ground at the client, with cancellation included: shared reads, hidden-panel cancellation, and resize invalidation.

Verification

Focused run of VMClientReadCoalescingTests and VMResourceStatsStoreTests on blacksmith-6vcpu-macos-15: 36 tests in 2 suites pass. run

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes a stats read regression so machine stats requests cancel when their last caller leaves.

  • Reverts the single-flight layer added in the previous stats store, which ran unstructured tasks that kept requests alive until the 30 s timeout after a machine was removed or a panel hidden.
  • Stats reads now share the network request through the coordinator while fencing each caller with beginRead/finishRead to preserve revision and sequence ordering for resize, reset, and removal.
  • Removes VMResourceStatsStore.read(machineID:), its readTask field, and the tests that only covered the store-level method; VMClientReadCoalescingTests covers sharing, cancellation, and resize invalidation at the client.

Written for commit 839a0c4. Summary will update on new commits.

Review in cubic

#13327 added single-flight stats reads inside VMResourceStatsStore. It was
written before #13151 landed CloudReadRequestCoordinator, which already
shares every concurrent /stats GET per account, team, and auth generation
and cancels the HTTP read once its last caller leaves. The store-level task
duplicated that sharing but ran unstructured, so a removed machine or a
hidden panel kept its stats request alive until the 30 s timeout. The
coordinator's waiter accounting also stopped seeing callers, and three
VMClientReadCoalescingTests cases timed out on every main run.

Stats reads now fence per caller with beginRead/finishRead, which keeps the
revision and sequence ordering from #13327's store, and share the network
request through the coordinator. The store's read(machineID:) and its tests
go away with it; VMClientReadCoalescingTests covers sharing, cancellation,
and resize invalidation at the client.

This reverts commit c72f659df746cfcabf1e5ab1b92b99ec8b09b38c.

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

coderabbitai Bot commented Sep 24, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 1 minute.

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: c9cdff2a-4aa4-483d-977d-bad752fc8c4c

📥 Commits

Reviewing files that changed from the base of the PR and between ea10f16 and 839a0c4.

📒 Files selected for processing (4)
  • Sources/Cloud/VMClient+ResourceStats.swift
  • Sources/Cloud/VMResourceStatsStore+Entry.swift
  • Sources/Cloud/VMResourceStatsStore.swift
  • cmuxTests/VMResourceStatsStoreTests.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
teamleaderleo enabled auto-merge (squash) September 24, 2026 13:51
@teamleaderleo
teamleaderleo merged commit 7434ad0 into main Sep 24, 2026
56 of 57 checks passed
rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 24, 2026
7434ad0 fix(cloud): let stats reads cancel through the shared read coordinator (manaflow-ai#14228)
7c0cd0b test(ssh): fix SSHStartupManualReconnectTests after the cmux-tui SSH migration (manaflow-ai#14209)
b3b50e8 ci: let persistent-compile up onboard a mini that has no gh (manaflow-ai#14227)
ea10f16 ci: let a second nightly mini take requests (manaflow-ai#14223)
d5d578a ci: rerun app-host tests against the merge a pull_request run built (manaflow-ai#14221)

# Conflicts:
#	.github/workflows/app-host-test-rerun.yml
#	.github/workflows/nightly-mini-build.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