Skip to content

cloud: share concurrent VM stats reads - #13327

Merged
teamleaderleo merged 3 commits into
manaflow-ai:mainfrom
teamleaderleo:leo/cloud-stats-single-flight
Sep 24, 2026
Merged

teamleaderleo merged 3 commits into
manaflow-ai:mainfrom
teamleaderleo:leo/cloud-stats-single-flight

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The Machines panel, surface registry, and CLI can ask for the same VM's resource stats at the same time. VMResourceStatsStore already fences ordering and resize/reset generations, but every caller still starts its own /api/vm/:id/stats request.

Change

Share only the active stats request for a given machine. This is single-flight, not a TTL cache: once the request settles, the next read fetches fresh data. Resize, reset, and retention invalidation fence the old task so a late completion cannot clear or overwrite a newer read. One consumer's cancellation does not cancel the shared request for other consumers.

Validation

  • Added behavioral coverage for sharing, independent machines, failures/retry, retention/reset/resize invalidation, and stale completions.
  • swiftc -parse passes for every changed Swift file.
  • Full Xcode tests were not runnable in the fresh worktree because the vendor/bonsplit submodule is uninitialized; run the focused VMResourceStatsStoreTests target in CI or an initialized checkout.

Follow-up measurement

Use the Cloud startup/fleet harness with 1/10/50 machines and multiple panels to compare stats request count and foreground latency before and after. This PR removes duplicate in-flight work but does not claim an end-to-end latency number.


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

Shares concurrent reads of the same destination VM's resource stats so the Machines panel, surface registry, and CLI no longer each start their own /api/vm/:id/stats request.

This is single-flight, not a cache: once the active request settles, the next read fetches fresh data. Resize, reset, and retention invalidation discard the in-flight task so a late completion can't clear or overwrite a newer read, and one consumer's cancellation doesn't cancel the shared request for other consumers.

Testing

  • Adds coverage for request sharing, independent machines, failures and retries, invalidation, and stale completions.
  • swiftc -parse passes for changed files; the full Xcode suite needs the vendor/bonsplit submodule, so run the VMResourceStatsStoreTests target in CI.

Written for commit 2a32159. Summary will update on new commits.

Review in cubic

@cursor

cursor Bot commented Sep 21, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 12 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: feeb2c03-aea0-43c6-be55-b69211947679

📥 Commits

Reviewing files that changed from the base of the PR and between 02972b7 and 2a32159.

📒 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.

@greptile-apps

greptile-apps Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The PR appears safe to merge, with a non-blocking cancellation-lifecycle issue that can retain cancelled stats consumers until the shared request settles.

Findings

  1. P2 Cancellation Cannot Stop Waiting ▶

Summary

This PR adds per-machine single-flight sharing for active VM resource-stat requests while retaining fresh reads after each request settles.

  • Stores the active request in the main-actor resource-state entry.
  • Uses revision changes and entry removal to fence resize, reset, retention, and eviction invalidations.
  • Adds behavioral coverage for request sharing, retries, independent machines, and stale completions.
  • Cancellation no longer propagates to the shared fetch, but individual cancelled waiters also remain suspended until that fetch settles.

Diagram

sequenceDiagram
    participant C1 as Consumer 1
    participant C2 as Consumer 2
    participant Store as VMResourceStatsStore
    participant API as VM stats API
    C1->>Store: read(machineID)
    Store->>API: start shared request
    C2->>Store: read(machineID)
    Store-->>C2: existing Task handle
    C1-xC1: consumer cancelled
    Note over C1,API: Cancelled waiter remains suspended
    API-->>Store: response or timeout
    Store-->>C1: task settles, then cancellation is observed
    Store-->>C2: shared result
    Store->>Store: clear active task
Loading

Reviews (1) · Last reviewed commit: "cloud: share concurrent VM stats reads"

Comment thread Sources/Cloud/VMClient+ResourceStats.swift
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Addressed the cancellation finding in stacked follow-up teamleaderleo#95. Each consumer now stops awaiting the shared stats fetch when canceled, while the underlying single-flight request remains available to other consumers; a regression test covers this separation.

@teamleaderleo
teamleaderleo enabled auto-merge (squash) September 21, 2026 08:31
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Parked — Thornquay 💠 (triage, 2026-09-23). Merges cleanly into main, 4 files, and nothing has superseded the single-flight. The 8 failing checks are all macos / app-host unit tests (N/6) from 2026-09-21 — that is the pre-#13759 breakage on main, not this PR. To confirm that, it needs a rebase and a full Mac run, which is why it is parked rather than landed: the macOS pool is saturated and this is a contention optimization, not a bug fix. Good candidate once runners free up — the rebase should be a no-op.

@cursor

cursor Bot commented Sep 24, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@teamleaderleo
teamleaderleo merged commit c72f659 into manaflow-ai:main Sep 24, 2026
52 of 53 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 24, 2026
rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 24, 2026
9567d6e refactor: give About and Licenses windows explicit ownership (manaflow-ai#13148)
fae46b6 ci: stop retrying a missing cmux-tui manifest (manaflow-ai#14168)
8421357 Point PR checklist and welcome note at the hidden Review Trigger block (manaflow-ai#14167)
b9415db ci: run app-host product consumers on compile admission's pool and Xcode (manaflow-ai#14163)
ac0ceae fix(sidebar): order panels without reading split-container geometry (manaflow-ai#13931)
37edc16 ci: judge Web complexity's trusted files in the pull request's merge (manaflow-ai#14018)
679f4e2 ci: leave three-day-old queued ghosts to GitHub instead of retrying them (manaflow-ai#14166)
07a2e22 fix(web): enumerate complexity-gate sources with git ls-files -z (manaflow-ai#13682)
c72f659 cloud: share concurrent VM stats reads (manaflow-ai#13327)
aa51f16 ci: trim package setup before the macOS compile admission build (manaflow-ai#14160)
82ea1ed ci: land the fleet review fixes manaflow-ai#14159 merged without (manaflow-ai#14165)
adddb59 docs: propose routing CI by capability instead of by vendor (manaflow-ai#14010)
f862390 ci: fix three fleet command gaps from the manaflow-ai#14159 review (manaflow-ai#14164)
77d56b3 agent-chat: make installed harnesses first-class (manaflow-ai#13347)
7dc57f6 Clarify writing guidance for issue and PR descriptions (manaflow-ai#13275)
ccf4963 ci: name the hung test when a Swift package test step stalls (manaflow-ai#14055)
9fca985 ci: guard the fleet routing switch, Xcode pin and quarantine (manaflow-ai#14159)
02972b7 fix: thin around and Developer ID sign the bundled cmux-tui SSH payloads (manaflow-ai#14154)

# Conflicts:
#	.github/workflows/app-host-test-rerun.yml
#	.github/workflows/ci-guards.yml
#	.github/workflows/ci-macos.yml
#	.github/workflows/test-ios.yml
#	.github/workflows/web-complexity-trusted.yml
teamleaderleo added a commit to teamleaderleo/cmux that referenced this pull request Sep 24, 2026
…le run

It fails on main since manaflow-ai#13327: after the first refresh succeeds, the
second model.refresh() sends neither the list nor the stats request
within 10 s (CI run 36004127379 logs one GET of each). The per-selector
runner stops at the first red suite, so this also hid every XCTest suite
selected after it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo added a commit that referenced this pull request Sep 24, 2026
#14228)

#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>
teamleaderleo added a commit that referenced this pull request Sep 24, 2026
…y fixtures after #14216 (#14222)

* test(cloud): count stats waiters after VMResourceStatsStore shares reads

#13327 moved per-machine stats sharing into VMResourceStatsStore, so
concurrent callers for one machine now reach CloudReadRequestCoordinator
as a single waiter, and a cancelled consumer no longer cancels the shared
read. Two older coordinator tests still counted a waiter per caller and
expected a dropped machine's read to disappear, so they have failed on
every main run since. Expect one waiter per machine, and keep the removed
machine's read alive until it answers.

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

* test(ssh): point legacy Workspace SSH fixtures at a VM-baked daemon

Since 5f0d222 (#13866) Workspace.configureRemoteConnection hands every
SSH config whose terminal uses SSH and whose daemon is bootstrapped to
configureSSHTuiConnection, so app-side tests built on that shape never
tracked a remote terminal surface and their assertions failed. The legacy
Workspace path is still live for VM-baked daemons, so these fixtures now
pass skipDaemonBootstrap: true. Assertions are unchanged.

Tests that depend on daemon bootstrap, the reverse relay, relay cleanup,
persistent SSH resume bindings or persistent SSH PTY restore are left
alone: that code is unreachable for SSH after the migration, and the
VM-baked path turns it off.

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

* test: wait for held stats batches only after release; fix the sibling fork fixture

Both stats batches wait on shared reads that stay alive after
cancellation, so awaiting either before releaseResponses() would hang
until the coordinator's 30 s budget. Await them after release and check
that late completions do not revive an owner.

testForkAgentConversationInRemoteWorkspaceUsesFallbackDirectoryInForkCommand
builds the same daemon-bootstrapping config as its fixed siblings; move it
to the VM-baked legacy path too.

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

* test: skip legacy SSH tests unreachable since cmux-tui routing

Since 5f0d222 (#13866), SSH configs that bootstrap the daemon route
to cmux-tui and preserved SSH snapshots restore through
tuiSSHConfiguration, so the legacy bootstrap, relay/slot cleanup,
persistent PTY reattach, and remote resume binding paths these tests
drive are no longer reachable for SSH. Skip them with a uniform reason
instead of deleting them so they can be rewritten against cmux-tui.

Swift Testing tests use the .disabled trait; XCTest methods use the
existing `try XCTSkipIf(true, ...)` idiom, which adds no
unreachable-code warning.

testPersistentSSHPTYRestorePreservesLocalTerminalWorkingDirectory is
left enabled on purpose.

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

* test: route the remaining legacy SSH fixtures off cmux-tui

Seven tests exercise legacy Workspace behaviour that is not gated on
daemon bootstrap (ControlMaster cleanup on close and session end,
duplicate relay callbacks, proxy-only error handling, persistent PTY
session ID seeding and reset, session-index splits). Their fixtures now
pass skipDaemonBootstrap: true so configureRemoteConnection keeps them on
the legacy path; assertions are unchanged.

Three tests check behaviour that only exists for daemon-bootstrapping SSH
configs, which route to cmux-tui since 5f0d222: the daemon upload
(RemoteSessionCoordinator skips bootstrap for VM-baked daemons), fresh
relay namespace minting on fork, and the persistent PTY restore fallback
without a socket path (both require skipDaemonBootstrap != true in
SessionRemoteWorkspaceSnapshot.workspaceConfiguration, and preserved SSH
snapshots divert to tuiSSHConfiguration first). They are skipped with the
same reason as the rest of this branch.

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

* test(cloud): disable the failed-stats refresh test pending a debuggable run

It fails on main since #13327: after the first refresh succeeds, the
second model.refresh() sends neither the list nor the stats request
within 10 s (CI run 36004127379 logs one GET of each). The per-selector
runner stops at the first red suite, so this also hid every XCTest suite
selected after it.

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

* test: narrow legacy SSH fixture changes to the post-#14216 routing

#14216 sends a config to cmux-tui only when it is relay-less
(routesThroughSSHTui), so relay-bearing fixtures reach the legacy
Workspace path again without skipDaemonBootstrap. Revert those flips and
the skips on tests that configure a relay-bearing connection directly.

Keep the flip only where the config is relay-less (RemotePTYReconnect)
or where a snapshot restore would produce a relay-less or preserved SSH
config, with comments that name the actual reason. Give the two daemon
bootstrap tests the no-TTY CLI relay shape so they exercise the live
legacy bootstrap path instead of being skipped. Tests that restore a
preserved SSH snapshot stay skipped, since tuiSSHConfiguration owns that
restore.

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

* test: park the local-shell-in-SSH-TUI restore test behind an explicit regression note

It fails because Workspace.createPanel restores every terminal in a
preserved SSH TUI workspace through restoreDeviceDisplayPanel, local
shells included. That may be a real regression, but the fix is a product
decision for the migration owner. Skipping it with that reason lets the
changed-suites runner reach the suites queued after this one.

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

* test(cloud): drop the VMClientReadCoalescingTests changes from this PR

The two adjusted cases passed in one run and failed in the next with no
source change, so they are timing-sensitive in ways this Mac cannot
debug. Leave the suite exactly as on main and track it separately.

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.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.

1 participant