Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions web/app/api/vm/[id]/attach-endpoint/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export async function POST(
throw err;
}
},
{ includeTeams: "selected" },
);
}

Expand Down
1 change: 1 addition & 0 deletions web/app/api/vm/[id]/exec/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,6 @@ export async function POST(
throw err;
}
},
{ includeTeams: "selected" },
);
}
1 change: 1 addition & 0 deletions web/app/api/vm/[id]/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,6 @@ export async function DELETE(
}
return jsonResponse({ ok: true });
},
{ includeTeams: "selected" },
);
}
1 change: 1 addition & 0 deletions web/app/api/vm/[id]/ssh-endpoint/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,6 @@ export async function POST(
throw err;
}
},
{ includeTeams: "selected" },
);
}
27 changes: 27 additions & 0 deletions web/app/api/vm/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ export async function GET(request: Request): Promise<Response> {
}));
return jsonResponse({ vms });
},
() => ({
requestedBillingTeamId: requestedVmTeamIdFromRequest(request),
}),
);
}

Expand Down Expand Up @@ -252,5 +255,29 @@ export async function POST(request: Request): Promise<Response> {
createdAt: created.createdAt,
});
},
vmCreateAuthOptions,
);
}

async function vmCreateAuthOptions(request: Request) {
return {
requestedBillingTeamId: requestedVmTeamIdFromRequest(request) ??
await requestedVmTeamIdFromBody(request),
};
}
Comment thread
cursor[bot] marked this conversation as resolved.
Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Team ID priority in vmCreateAuthOptions is inverted relative to the handler — edge-case resolution failure for multi-team users

vmCreateAuthOptions resolves the billing team ID as header/query → body (line 264), but the handler at line 189 uses body → header/query:

// auth options (line 264): header wins
requestedBillingTeamId: requestedVmTeamIdFromRequest(request) ??
    await requestedVmTeamIdFromBody(request)

// handler (line 189): body wins
const requestedBillingTeamId = body.billingTeamId || requestedVmTeamIdFromRequest(request);

Concrete failure: a multi-team user whose selectedTeam is teamA sends x-cmux-team-id: teamA (header) and {"billingTeamId": "teamB"} (body).

  1. vmCreateAuthOptions picks teamA (header wins). Since teamA === selectedTeam, shouldListTeams = false, so user.teamIds = ["teamA"] only.
  2. The handler picks teamB (body wins).
  3. resolveVmEntitlements checks user.teamIds.includes("teamB") → false → VmBillingTeamResolutionError despite the user legitimately being in teamB.

Fix: align vmCreateAuthOptions to body-first, matching the handler's semantics for a POST payload. request.clone() already ensures the original body is still readable by the handler.

🐛 Proposed fix
 async function vmCreateAuthOptions(request: Request) {
   return {
-    requestedBillingTeamId: requestedVmTeamIdFromRequest(request) ??
-      await requestedVmTeamIdFromBody(request),
+    requestedBillingTeamId: await requestedVmTeamIdFromBody(request) ??
+      requestedVmTeamIdFromRequest(request),
   };
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/app/api/vm/route.ts` around lines 262 - 267, vmCreateAuthOptions
currently prefers header/query over body which conflicts with the handler's
body-first logic; change the resolution order in vmCreateAuthOptions so it
returns requestedBillingTeamId: await requestedVmTeamIdFromBody(request) ??
requestedVmTeamIdFromRequest(request) (use the existing
requestedVmTeamIdFromBody and requestedVmTeamIdFromRequest helpers), ensuring
the POST body is chosen first to match the handler's requestedBillingTeamId
semantics and avoid shouldListTeams/user.teamIds mismatches that lead to
VmBillingTeamResolutionError.

Comment on lines +752 to +796

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.

P1 Auth and handler disagree on team ID priority

vmCreateAuthOptions resolves requestedBillingTeamId as header/URL first, body second (requestedVmTeamIdFromRequest(request) ?? requestedVmTeamIdFromBody(request)). The handler resolves it as body first, header/URL second (body.billingTeamId || requestedVmTeamIdFromRequest(request)).

When a client sends both x-cmux-team-id: team-A (their selected team) and body billingTeamId: team-B, auth uses team-A and skips team listing (since it matches selectedTeam.id), so user.teamIds = [team-A]. The handler then tries to resolve entitlements for team-B, which isn't in user.teamIds, producing a false 403 vm_billing_team_not_found. The mac app always sends the selected-team header; any call that additionally passes a body billingTeamId for a different team will hit this.

Suggested change
return {
requestedBillingTeamId: requestedVmTeamIdFromRequest(request) ??
await requestedVmTeamIdFromBody(request),
};
}
async function vmCreateAuthOptions(request: Request) {
return {
requestedBillingTeamId: await requestedVmTeamIdFromBody(request) ??
requestedVmTeamIdFromRequest(request),
};
}


async function requestedVmTeamIdFromBody(request: Request): Promise<string | null> {
try {
const rawText = await request.clone().text();
if (rawText.length === 0) return null;
const raw = JSON.parse(rawText);
if (raw === null || typeof raw !== "object" || Array.isArray(raw)) return null;
const candidate = raw as Record<string, unknown>;
const bodyBillingTeamId = candidate.billingTeamId ?? candidate.teamId;
return typeof bodyBillingTeamId === "string" && bodyBillingTeamId.trim()
? bodyBillingTeamId.trim()
: null;
} catch {
return null;
}
}
31 changes: 26 additions & 5 deletions web/services/vms/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,22 @@ export type AuthedTeam = {
billingPlanId: string | null;
};

export type VerifyRequestOptions = {
readonly requestedBillingTeamId?: string | null;
readonly includeTeams?: "auto" | "all" | "selected";
};

/**
* Verify the caller's Stack Auth session. Accepts either a cookie (browser path) or a
* `Authorization: Bearer <access>` + `X-Stack-Refresh-Token: <refresh>` pair from the
* native macOS client.
*
* Returns the resolved user or null if unauthenticated.
*/
export async function verifyRequest(request: Request): Promise<AuthedUser | null> {
export async function verifyRequest(
request: Request,
options: VerifyRequestOptions = {},
): Promise<AuthedUser | null> {
if (!isStackConfigured()) {
return null;
}
Expand All @@ -42,22 +50,30 @@ export async function verifyRequest(request: Request): Promise<AuthedUser | null
tokenStore: { accessToken, refreshToken },
});
if (user) {
return await authedUserFromStackUser(user);
return await authedUserFromStackUser(user, options);
}
}
}

// Fall back to the Next.js cookie flow (when browser hits the route).
const user = await stackServerApp.getUser({ tokenStore: request as unknown as { headers: { get(name: string): string | null } } });
if (user) {
return await authedUserFromStackUser(user);
return await authedUserFromStackUser(user, options);
}
return null;
}

async function authedUserFromStackUser(user: StackUserLike): Promise<AuthedUser> {
async function authedUserFromStackUser(
user: StackUserLike,
options: VerifyRequestOptions,
): Promise<AuthedUser> {
const selectedTeam = teamLike(user.selectedTeam);
const listedTeams = typeof user.listTeams === "function"
const requestedTeamId = normalizedOptionalString(options.requestedBillingTeamId);
const includeTeams = options.includeTeams ?? "auto";
const shouldListTeams = typeof user.listTeams === "function" &&
(includeTeams === "all" ||
(includeTeams === "auto" && (!selectedTeam || (requestedTeamId && selectedTeam.id !== requestedTeamId))));
const listedTeams = shouldListTeams && typeof user.listTeams === "function"
? (await user.listTeams()).map(teamLike).filter((team): team is TeamLike => !!team)
: [];
const teamIds = uniqueStrings([
Expand Down Expand Up @@ -117,6 +133,11 @@ function planIdFromMetadata(metadata: unknown): string | null {
return typeof value === "string" && value.trim() ? value.trim() : null;
}

function normalizedOptionalString(value: string | null | undefined): string | null {
const normalized = value?.trim();
return normalized ? normalized : null;
}

function uniqueStrings(values: readonly (string | undefined)[]): readonly string[] {
return [...new Set(values.filter((value): value is string => typeof value === "string" && value.length > 0))];
}
Expand Down
29 changes: 29 additions & 0 deletions web/services/vms/drivers/freestyle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const CREATE_TIMEOUT_MS = 15 * 60 * 1000;
const SNAPSHOT_TIMEOUT_MS = 15 * 60 * 1000;
const EXEC_OVERHEAD_TIMEOUT_MS = 15_000;
const MAX_EXEC_TIMEOUT_MS = 15 * 60 * 1000;
const FREESTYLE_ACTIVE_STATES = new Set(["starting", "running", "suspending"]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suspended VMs excluded from active limit check

Medium Severity

FREESTYLE_ACTIVE_STATES only includes "starting", "running", and "suspending", but omits "suspended" and "resuming". When the live reconciliation path runs, VMs in Freestyle's "suspended" state (which map to "paused" in the DB) are excluded from the live active set. In isActiveForLimit, these VMs then fail all checks (not in live set, not "provisioning") and are not counted toward the limit. This means paused VMs no longer count against the active VM cap when reconciliation is active, diverging from the old behavior where "paused" VMs always counted.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 77025a9. Configure here.


function client(timeoutMs = DEFAULT_TIMEOUT_MS): Freestyle {
const longFetch: typeof fetch = (input, init) =>
Expand Down Expand Up @@ -79,6 +80,34 @@ function mapStatus(state: string | null | undefined): VMStatus {
export class FreestyleProvider implements VMProvider {
readonly id = "freestyle" as const;

async listActiveVmIds(): Promise<readonly string[]> {
return withVmSpan(
"cmux.vm.provider.list_active",
{
"cmux.vm.provider": "freestyle",
"cmux.vm.operation": "list_active",
"cmux.timeout_ms": DEFAULT_TIMEOUT_MS,
},
async (span) => {
try {
const result = await client().vms.list();
const active = result.vms
.filter((vm) => !vm.deleted && FREESTYLE_ACTIVE_STATES.has(vm.state))
.map((vm) => vm.id);
setSpanAttributes(span, {
"cmux.vm.provider.active_count": active.length,
"cmux.vm.provider.total_count": result.totalCount,
"cmux.vm.provider.running_count": result.runningCount,
"cmux.vm.provider.starting_count": result.startingCount,
});
return active;
} catch (err) {
throw new ProviderError("freestyle", "listActiveVmIds", err);
}
},
);
}

async create(options: CreateOptions): Promise<VMHandle> {
const image = options.image.trim();
if (!image) {
Expand Down
2 changes: 2 additions & 0 deletions web/services/vms/drivers/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ export type SnapshotRef = {
export interface VMProvider {
readonly id: ProviderId;

listActiveVmIds?(): Promise<readonly string[]>;

create(options: CreateOptions): Promise<VMHandle>;
destroy(vmId: string): Promise<void>;

Expand Down
8 changes: 8 additions & 0 deletions web/services/vms/providerGateway.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import {
import { VmProviderOperationError } from "./errors";

export type VmProviderGatewayShape = {
readonly listActiveVmIds?: (
provider: ProviderId,
) => Effect.Effect<readonly string[] | null, VmProviderOperationError>;
readonly create: (provider: ProviderId, options: CreateOptions) => Effect.Effect<VMHandle, VmProviderOperationError>;
readonly destroy: (provider: ProviderId, vmId: string) => Effect.Effect<void, VmProviderOperationError>;
readonly exec: (
Expand Down Expand Up @@ -51,6 +54,11 @@ function providerEffect<A>(
}

export const VmProviderGatewayLive = Layer.succeed(VmProviderGateway, {
listActiveVmIds: (provider) =>
providerEffect(provider, "listActiveVmIds", () => {
const vmProvider = getProvider(provider);
return vmProvider.listActiveVmIds ? vmProvider.listActiveVmIds() : Promise.resolve(null);
}),
create: (provider, options) =>
providerEffect(provider, "create", () => getProvider(provider).create(options)),
destroy: (provider, vmId) =>
Expand Down
54 changes: 44 additions & 10 deletions web/services/vms/repository.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { and, count, desc, eq, inArray, isNotNull, isNull, ne, or, sql } from "drizzle-orm";
import { and, desc, eq, isNotNull, isNull, ne, or, sql } from "drizzle-orm";
import * as Context from "effect/Context";
import * as Effect from "effect/Effect";
import * as Layer from "effect/Layer";
Expand Down Expand Up @@ -39,6 +39,7 @@ export type VmRepositoryShape = {
readonly image: string;
readonly imageVersion?: string | null;
readonly maxActiveVms: number;
readonly liveActiveProviderVmIds?: readonly string[] | null;
readonly idempotencyKey?: string;
}) => Effect.Effect<BeginCreateResult, VmDatabaseError | VmLimitExceededError>;
readonly markCreateRunning: (input: {
Expand Down Expand Up @@ -217,19 +218,31 @@ export const VmRepositoryLive = Layer.succeed(VmRepository, {
if (existing) return { inserted: false as const, vm: existing };
}

const [active] = await tx
.select({ total: count() })
const activeCandidates = await tx
.select({
provider: cloudVms.provider,
providerVmId: cloudVms.providerVmId,
status: cloudVms.status,
})
.from(cloudVms)
.where(
and(
inArray(cloudVms.status, ["provisioning", "running", "paused"]),
or(
eq(cloudVms.billingTeamId, input.billingTeamId),
and(isNull(cloudVms.billingTeamId), eq(cloudVms.userId, input.userId)),
),
or(
eq(cloudVms.billingTeamId, input.billingTeamId),
and(isNull(cloudVms.billingTeamId), eq(cloudVms.userId, input.userId)),
Comment thread
cursor[bot] marked this conversation as resolved.
Resolved
),
);

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.

P1 The activeCandidates query now fetches every VM row for a billing team with no status pre-filter, including all historical failed and destroyed rows. The old query used inArray(cloudVms.status, ["provisioning", "running", "paused"]) to keep the result set small. This new query runs inside an advisory-locked transaction, so a long-lived team with hundreds of historical VM rows will hold that lock while transferring and filtering all those rows in application code — adding latency that directly blocks concurrent creates for the same team.

Suggested change
const activeCandidates = await tx
.select({
provider: cloudVms.provider,
providerVmId: cloudVms.providerVmId,
status: cloudVms.status,
})
.from(cloudVms)
.where(
and(
inArray(cloudVms.status, ["provisioning", "running", "paused"]),
or(
eq(cloudVms.billingTeamId, input.billingTeamId),
and(isNull(cloudVms.billingTeamId), eq(cloudVms.userId, input.userId)),
),
or(
eq(cloudVms.billingTeamId, input.billingTeamId),
and(isNull(cloudVms.billingTeamId), eq(cloudVms.userId, input.userId)),
),
);
const activeCandidates = await tx
.select({
provider: cloudVms.provider,
providerVmId: cloudVms.providerVmId,
status: cloudVms.status,
})
.from(cloudVms)
.where(
and(
ne(cloudVms.status, "failed"),
ne(cloudVms.status, "destroyed"),
or(
eq(cloudVms.billingTeamId, input.billingTeamId),
and(isNull(cloudVms.billingTeamId), eq(cloudVms.userId, input.userId)),
),
),
);

const activeCount = Number(active?.total ?? 0);
const liveActiveProviderVmIds = input.liveActiveProviderVmIds
? new Set(input.liveActiveProviderVmIds)
: null;
const activeCount = activeCandidates.filter((vm) =>
isActiveForLimit({
provider: vm.provider,
providerVmId: vm.providerVmId,
status: vm.status,
reconciledProvider: liveActiveProviderVmIds ? input.provider : null,
liveActiveProviderVmIds,
})
).length;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
if (activeCount >= input.maxActiveVms) {
throw new VmLimitExceededError({
kind: "active_vms",
Expand Down Expand Up @@ -420,3 +433,24 @@ export const VmRepositoryLive = Layer.succeed(VmRepository, {
});
}),
});

function isActiveForLimit(input: {
readonly provider: ProviderId;
readonly providerVmId: string | null;
readonly status: CloudVmRow["status"];
readonly reconciledProvider: ProviderId | null;
readonly liveActiveProviderVmIds: ReadonlySet<string> | null;
}): boolean {
if (
input.provider === input.reconciledProvider &&
input.providerVmId &&
input.liveActiveProviderVmIds?.has(input.providerVmId)
) {
return true;
}
if (input.status === "failed" || input.status === "destroyed") return false;
if (input.provider !== input.reconciledProvider) {
return input.status === "provisioning" || input.status === "running" || input.status === "paused";
}
return input.status === "provisioning";
}
12 changes: 10 additions & 2 deletions web/services/vms/routeHelpers.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Span } from "@opentelemetry/api";
import { recordSpanError, withApiRouteSpan, type MaybeAttributes } from "../telemetry";
import { unauthorized, verifyRequest, type AuthedUser } from "./auth";
import { unauthorized, verifyRequest, type AuthedUser, type VerifyRequestOptions } from "./auth";

/** Bearer + refresh token pair the mac app stashes in keychain. */
export type StackBearer = { accessToken: string; refreshToken: string };
Expand All @@ -20,12 +20,17 @@ export type AuthedVmRouteContext = {
span: Span;
};

export type VmRouteAuthOptionsInput =
| VerifyRequestOptions
| ((request: Request) => VerifyRequestOptions | Promise<VerifyRequestOptions>);

export async function withAuthedVmApiRoute(
request: Request,
route: string,
attributes: MaybeAttributes,
failureLog: string,
handler: (context: AuthedVmRouteContext) => Promise<Response>,
authOptions: VmRouteAuthOptionsInput = {},
): Promise<Response> {
return withApiRouteSpan(
request,
Expand All @@ -34,7 +39,10 @@ export async function withAuthedVmApiRoute(
async (span) => {
try {
const bearer = parseBearer(request);
const user = await verifyRequest(request);
const resolvedAuthOptions = typeof authOptions === "function"
? await authOptions(request)
: authOptions;
const user = await verifyRequest(request, resolvedAuthOptions);
if (!user) return unauthorized();
if (requiresBrowserMutationProtection(request.method, bearer) && !browserMutationOriginAllowed(request)) {
return jsonResponse({ error: "forbidden" }, 403);
Expand Down
40 changes: 22 additions & 18 deletions web/services/vms/workflows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,10 @@ export function createVm(input: {
const repo = yield* VmRepository;
const providers = yield* VmProviderGateway;
const billing = yield* VmBillingGateway;
const create = yield* repo.beginCreate(input);
const liveActiveProviderVmIds = input.provider === "freestyle" && providers.listActiveVmIds
? yield* providers.listActiveVmIds(input.provider)
: null;
const create = yield* repo.beginCreate({ ...input, liveActiveProviderVmIds });

if (!create.inserted) {
const existing = create.vm;
Expand Down Expand Up @@ -154,24 +157,24 @@ export function createVm(input: {
),
);

yield* recordCreditEvent(repo, create.vm, "vm.create.credit.reserved", creditReservation)
.pipe(Effect.catchAll(() => Effect.void));

yield* repo.recordUsageEvent({
userId: input.userId,
billingTeamId: input.billingTeamId,
billingPlanId: input.billingPlanId,
vmId: create.vm.id,
eventType: "vm.create.requested",
provider: input.provider,
imageId: input.image,
metadata: {
idempotencyKeySet: !!input.idempotencyKey,
imageVersion: input.imageVersion ?? null,
},
}).pipe(Effect.catchAll(() => Effect.void));
const preProviderAudit = Effect.all([
recordCreditEvent(repo, create.vm, "vm.create.credit.reserved", creditReservation),
repo.recordUsageEvent({
userId: input.userId,
billingTeamId: input.billingTeamId,
billingPlanId: input.billingPlanId,
vmId: create.vm.id,
eventType: "vm.create.requested",
provider: input.provider,
imageId: input.image,
metadata: {
idempotencyKeySet: !!input.idempotencyKey,
imageVersion: input.imageVersion ?? null,
},
}),
], { discard: true, concurrency: 2 }).pipe(Effect.catchAll(() => Effect.void));

const handle = yield* providers.create(input.provider, { image: input.image }).pipe(
const providerCreate = providers.create(input.provider, { image: input.image }).pipe(
Effect.tapError((err) =>
Effect.all([
refundCredit(billing, repo, create.vm, creditReservation),
Expand All @@ -193,6 +196,7 @@ export function createVm(input: {
], { discard: true }).pipe(Effect.catchAll(() => Effect.void))
),
);
const [handle] = yield* Effect.all([providerCreate, preProviderAudit], { concurrency: 2 });

const running = yield* repo
.markCreateRunning({
Expand Down
Loading
Loading