Skip to content

fix(control-plane): close tenant-isolation, webhook-secret, and overlap bugs before first deploy #9170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 27, 2026
Merged

fix(control-plane): close tenant-isolation, webhook-secret, and overlap bugs before first deploy #9170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 45 additions & 8 deletions control-plane/src/ams-wake.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ export type AmsWakeConfig = {
/** Overridable for tests only -- production always uses real wall-clock time and real delays. */
pollIntervalMs?: number;
pollTimeoutMs?: number;
/** Caps how many due tenants ONE tick will wake (#9143, defect 7) -- production always uses
* {@link DEFAULT_MAX_TENANTS_PER_TICK}; overridable for tests only. */
maxTenantsPerTick?: number;
now?: () => Date;
};

Expand All @@ -45,10 +48,23 @@ export type AmsWakeResult = {

const HOSTED_ENTRY_BIN = "loopover-miner-hosted";
const DEFAULT_POLL_INTERVAL_MS = 1_000;
// A generous ceiling for a real discover/manage-poll/attempt cycle -- long enough that a real, working run
// almost never hits it, short enough that a genuinely hung container doesn't block this tick's remaining
// tenants indefinitely (this loop processes due tenants one at a time, not in parallel; see wakeDueAmsTenants).
const DEFAULT_POLL_TIMEOUT_MS = 10 * 60 * 1000;
// #9143 (defect 7): wrangler.jsonc's own Cron Trigger fires this module's `scheduled()` caller every 5
// minutes (`*/5 * * * *`). The OLD 10-minute default here GUARANTEED an overlapping tick: nextDueAt used to
// advance only AFTER a woken tenant's poll resolved (see wakeDueAmsTenants's own header comment on the fix),
// so one hung tenant's poll alone could still be running when the NEXT tick fired 5 minutes later -- which
// would then see that SAME tenant's nextDueAt untouched (still overdue) and wake it a SECOND time
// concurrently, with no lock/in-flight marker anywhere to stop it, racing both ticks' upserts against the
// same record. Capped well under the cron interval, AND `wakeDueAmsTenants` now claims each tenant's next
// nextDueAt BEFORE starting its poll (not after) so a genuinely-overlapping tick observes the claim and skips
// it regardless of this timeout's exact value -- but a short timeout is still what keeps a normal, non-
// overlapping tick's wall-clock budget sane across every due tenant it processes sequentially, not just one.
const DEFAULT_POLL_TIMEOUT_MS = 2 * 60 * 1000;
// #9143 (defect 7): bounds how many due tenants ONE tick will wake. Sequential-by-design (see
// wakeDueAmsTenants's own comment on why not Promise.all) means a fleet with more due tenants than this in a
// single 5-minute window would otherwise risk the WHOLE tick running long even when every individual tenant
// behaves within DEFAULT_POLL_TIMEOUT_MS -- deferring the excess to the next tick (their nextDueAt is already
// in the past, so nothing is lost, only delayed) keeps one tick's total wall-clock bounded too.
const DEFAULT_MAX_TENANTS_PER_TICK = 20;

/** Same `${product}:${name}` composite container-driver.ts's own `instanceNameFor` derives -- duplicated
* (not imported) because container-driver.ts's version takes a `TenantProvisioningRequest`, a shape this
Expand Down Expand Up @@ -82,21 +98,42 @@ async function pollForExitCode(stub: WakeStubLike, pollIntervalMs: number, timeo
/** Wakes every currently-due AMS tenant, one at a time (deliberately sequential, not `Promise.all` -- a
* single Cloudflare Cron Trigger invocation has a bounded wall-clock budget shared across every tenant this
* tick processes; running them concurrently would trade a slow tick for cross-tenant resource contention on
* shared infra this module has no visibility into). Advances each woken tenant's `nextDueAt` from the tick's
* OWN start time (not the run's completion time) so schedule drift doesn't accumulate when a cycle runs long.
*/
* shared infra this module has no visibility into), up to {@link DEFAULT_MAX_TENANTS_PER_TICK} (#9143, defect
* 7) per invocation.
*
* #9143: `nextDueAt` is now advanced BEFORE the poll starts, not after it resolves -- the pre-#9143 order
* (advance only once `pollForExitCode` returned) meant a hung/slow tenant's `nextDueAt` stayed in the past
* for the ENTIRE poll, so a genuinely-overlapping tick (the 5-minute cron firing again while a >5-minute poll
* from the prior tick was still running, or any other concurrent invocation) would see the same tenant still
* due and wake it a SECOND time, racing both ticks' upserts against the same record with no lock or in-flight
* marker anywhere. Claiming the slot up front means an overlapping tick observes `nextDueAt` already moved
* and skips it -- from the tick's OWN start time (not the run's completion time), so schedule drift doesn't
* accumulate when a cycle runs long, exactly as before. */
export async function wakeDueAmsTenants(config: AmsWakeConfig): Promise<AmsWakeResult[]> {
const now = config.now ?? (() => new Date());
const pollIntervalMs = config.pollIntervalMs ?? DEFAULT_POLL_INTERVAL_MS;
const pollTimeoutMs = config.pollTimeoutMs ?? DEFAULT_POLL_TIMEOUT_MS;
const maxTenantsPerTick = config.maxTenantsPerTick ?? DEFAULT_MAX_TENANTS_PER_TICK;
const tickStartedAt = now();

const records = await config.registry.list();
const results: AmsWakeResult[] = [];
let woken = 0;

for (const record of records) {
if (woken >= maxTenantsPerTick) break;
if (!isDue(record, tickStartedAt)) continue;
const schedule = record.amsSchedule!;
woken += 1;

// Claim this tenant's next slot BEFORE starting its poll (#9143) -- see this function's own header
// comment on why.
const claimedNextDueAt = new Date(tickStartedAt.getTime() + schedule.intervalMs).toISOString();
await config.registry.upsert({
...record,
amsSchedule: { ...schedule, nextDueAt: claimedNextDueAt },
updatedAt: now().toISOString(),
});

const stub = config.binding.getByName(instanceNameFor(record.tenant.name, record.product));
await stub.start({ entrypoint: [HOSTED_ENTRY_BIN, schedule.command, ...schedule.args] });
Expand All @@ -109,7 +146,7 @@ export async function wakeDueAmsTenants(config: AmsWakeConfig): Promise<AmsWakeR
...schedule,
lastRunAt: ranAt,
lastExitCode: exitCode,
nextDueAt: new Date(tickStartedAt.getTime() + schedule.intervalMs).toISOString(),
nextDueAt: claimedNextDueAt,
},
updatedAt: ranAt,
});
Expand Down
15 changes: 13 additions & 2 deletions control-plane/src/container-driver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,15 @@ export type ContainerStubLike = {
start(options?: { envVars?: Record<string, string>; entrypoint?: string[]; enableInternet?: boolean }): Promise<void>;
stop(): Promise<void>;
isProvisioned(): Promise<boolean>;
markProvisioned(): Promise<void>;
/** `envVars` (#9143, defect 4): the SAME map just passed to `start()` above, so the real DO-backed
* implementation (worker.ts's `ProvisionedContainer`) can persist it in its own durable storage and reload
* it into the base `Container` class's `this.envVars` on every future construction -- the vendored
* `@cloudflare/containers` SDK only ever applies `envVars` from the CALLER's own `start()` options or from
* `this.envVars`, never durably remembering a past `start({envVars})` call itself, so a container's
* auto-wake-on-request path after it sleeps (`containerFetch`'s own restart, which calls `start()` with NO
* options at all) would otherwise silently boot with an empty environment on every restart after the
* first. Optional so a fresh (no envVars at all) create still matches the exact pre-#9143 call shape. */
markProvisioned(envVars?: Record<string, string>): Promise<void>;
markDeprovisioned(): Promise<void>;
};

Expand Down Expand Up @@ -104,10 +112,13 @@ export async function createTenantContainer(config: ContainerDriverConfig, reque
if (config.centralPosthogKey) envVars[CENTRAL_POSTHOG_KEY_ENV_VAR] = config.centralPosthogKey;
if (Object.keys(envVars).length > 0) {
await stub.start({ envVars });
// #9143 (defect 4): hand the SAME envVars to markProvisioned so the real DO-backed implementation can
// persist them durably and survive a later implicit restart -- see ContainerStubLike's own doc comment.
await stub.markProvisioned(envVars);
} else {
await stub.start();
await stub.markProvisioned();
}
await stub.markProvisioned();
}

/** Idempotent: a tenant that was never provisioned (or already torn down) is a safe no-op, matching every
Expand Down
123 changes: 69 additions & 54 deletions control-plane/src/http-app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import {
provisionTenant,
type ProvisioningPagerDutyOptions,
} from "./provisioning.js";
import type { Product, TenantLifecycleState, TenantProvisioningDriver } from "./tenant-provisioning-driver.js";
import type { TenantLifecycleState, TenantProvisioningDriver } from "./tenant-provisioning-driver.js";
import type { AmsCycleSchedule, TenantRegistry, TenantRegistryRecord } from "./tenant-registry.js";

/** States that do NOT block re-creating a tenant of the same name+product (or re-claiming its installation
Expand Down Expand Up @@ -111,29 +111,6 @@ function parseOrbInstallationId(value: unknown): number | string | undefined {
return value;
}

/** Validated body of `POST /v1/tenants/rollout` (#4898): an explicit tenant-name list (no percentage/canary
* selector — no such primitive exists elsewhere in this codebase to build on) plus the version to pin.
* `pinnedVersion: null` is an explicit unpin (revert to the release channel's default). Scoped to a single
* `product` for the whole batch (#8024: a name is no longer globally unique across products, and a version
* rollout is naturally a per-product-fleet operation anyway -- ORB and AMS ship independently versioned
* images, so mixing them in one rollout call was never a meaningful use case even before #8024). */
type RolloutRequest = { names: string[]; product: Product; pinnedVersion: string | null };

function parseRolloutRequest(body: unknown): RolloutRequest | string {
if (body === null || typeof body !== "object" || Array.isArray(body)) return "body must be a JSON object";
const { names, product, pinnedVersion } = body as Record<string, unknown>;
if (!Array.isArray(names) || names.length === 0) return "names must be a non-empty array of tenant names";
if (!names.every((name): name is string => typeof name === "string" && name.trim() !== "")) {
return "names must be a non-empty array of tenant names";
}
if (new Set(names).size !== names.length) return "names must not repeat a tenant";
if (typeof product !== "string" || !product.trim()) return "product is required";
if (pinnedVersion !== null && (typeof pinnedVersion !== "string" || !pinnedVersion.trim())) {
return "pinnedVersion must be a non-blank string, or null to unpin";
}
return { names, product: product.trim(), pinnedVersion: pinnedVersion === null ? null : pinnedVersion.trim() };
}

export function createTenantHttpApp(deps: TenantHttpAppDeps): Hono {
const app = new Hono();

Expand Down Expand Up @@ -225,40 +202,67 @@ export function createTenantHttpApp(deps: TenantHttpAppDeps): Hono {
return c.json({ tenants: records.map((record) => ({ ...safeRecord(record), createdAt: record.createdAt, updatedAt: record.updatedAt })) });
});

// #4898: rollout/rollback = updating one or more tenants' pinnedVersion via an explicit list. Validates the
// WHOLE list before touching any record (all-or-nothing) so a typo'd name can never leave a fleet half
// rolled out; each updated tenant's container picks its new version up at its next (re)start
// (container-driver.ts's PINNED_VERSION_ENV_VAR). Every unlisted tenant is untouched by construction —
// the per-tenant-independence guarantee this endpoint exists to keep.
app.post("/v1/tenants/rollout", async (c) => {
const body: unknown = await c.req.json().catch(() => null);
if (body === null) return c.json({ error: "invalid_json" }, 400);
const parsed = parseRolloutRequest(body);
if (typeof parsed === "string") return c.json({ error: "invalid_request", message: parsed }, 400);
// #9143 (defect 3): DISABLED, not implemented. #4898's rollout/rollback route used to silently no-op on
// every one of its four promised effects: (1) it only ever wrote the registry's own `pinnedVersion` field
// and restarted nothing; (2) `createTenantContainer` early-returns once `isProvisioned()`, so a "rollout"
// against an already-running tenant never even reaches the one call site that injects
// `PINNED_VERSION_ENV_VAR`; (3) `provisionTenant({name}, ...)` constructs a FRESH `Tenant` carrying only
// `name`, dropping any previously-stored `pinnedVersion` the moment a tenant is next (re)provisioned anyway;
// and (4) nothing anywhere in this repo ever reads `LOOPOVER_PINNED_VERSION` back out of a container. "Rollback
// is the control you least want to discover is fake" — until the image side of a real rollout mechanism
// exists (a restart-and-repin path through createContainer, or a pin baked into the image at build time),
// this returns an explicit 501 rather than accepting a request that changes nothing any caller can observe.
// `Tenant.pinnedVersion` and `PINNED_VERSION_ENV_VAR` (tenant-provisioning-driver.ts/container-driver.ts) are
// left in place, unused by any route now — exactly what a real rollout mechanism would build on later.
app.post("/v1/tenants/rollout", (c) => c.json({ error: "not_implemented", message: "tenant rollout is not implemented yet (#9143)" }, 501));

const existing = new Map<string, TenantRegistryRecord>();
for (const name of parsed.names) {
const record = await deps.registry.get(name, parsed.product);
if (!record) return c.json({ error: "tenant_not_found", message: `unknown tenant "${name}"` }, 404);
// A torn-down tenant has no container to ever read the pin — surfacing the mistake beats silently
// stamping a version onto a terminated record (same conflict posture as the create route's 409).
if (record.state === "torn down") return c.json({ error: "tenant_torn_down", message: `tenant "${name}" is torn down` }, 409);
existing.set(name, record);
// #9143 (defect 2 manual recovery): re-links an EXISTING tenant's `orbInstallationId`. tenant-registry.ts's
// `upsert` used to unconditionally delete the OLD installation-ID index entry whenever a tenant's own
// `orbInstallationId` changed (or was cleared) between upserts — without checking whether that index entry
// still actually pointed at THIS tenant. Since `isRecreatableState` deliberately lets a "failed"/"torn down"
// tenant's installation claim be taken over by a brand-new tenant of a different name, a stale tenant being
// re-created or torn down LONG after its installation ID was reclaimed by someone else could silently delete
// the CURRENT claimant's live routing pointer — leaving that tenant ACTIVE but permanently unreachable by
// webhook (`orbInstallationId` is otherwise settable only at create; a create against an existing tenant
// 409s). `upsert` itself is now fixed to never do this going forward (see its own header comment), but a
// tenant whose pointer was ALREADY wiped by the pre-fix bug has no other way back — this route is that path.
app.patch("/v1/tenants/:name/orb-installation", async (c) => {
const name = c.req.param("name");
// Product is required so the registry can resolve the same `${product}:${name}` key used at create (#8024).
const product = c.req.query("product");
if (typeof product !== "string" || !product.trim()) {
return c.json({ error: "invalid_request", message: "product query parameter is required" }, 400);
}
if (product !== "orb") {
return c.json({ error: "invalid_request", message: 'orbInstallationId is only valid for product "orb"' }, 400);
}

const now = new Date().toISOString();
const updated: TenantRegistryRecord[] = [];
for (const name of parsed.names) {
const record = existing.get(name)!;
const next: TenantRegistryRecord = {
...record,
tenant: { ...record.tenant, pinnedVersion: parsed.pinnedVersion },
updatedAt: now,
};
await deps.registry.upsert(next);
updated.push(next);
const existing = await deps.registry.get(name, product);
if (!existing) return c.json({ error: "tenant_not_found" }, 404);

const body: unknown = await c.req.json().catch(() => null);
if (body === null || typeof body !== "object") return c.json({ error: "invalid_json" }, 400);
const { orbInstallationId: orbInstallationIdInput } = body as Record<string, unknown>;
if (orbInstallationIdInput === undefined) {
return c.json({ error: "invalid_request", message: "orbInstallationId is required" }, 400);
}
const orbInstallationId = parseOrbInstallationId(orbInstallationIdInput);
if (typeof orbInstallationId === "string") return c.json({ error: "invalid_request", message: orbInstallationId }, 400);

// Refuse to steal an installation ID a DIFFERENT, currently-claiming tenant still legitimately holds —
// same conflict posture as the create route's own check, just excluding this tenant's own current record
// (re-linking to the ID it already has, or recovering one it lost, must not 409 against itself).
const conflicting = await deps.registry.getByOrbInstallationId(orbInstallationId!);
if (conflicting && conflicting.tenant.name !== existing.tenant.name && !isRecreatableState(conflicting.state)) {
return c.json(
{ error: "installation_already_claimed", message: `installation ${orbInstallationId} is already claimed by tenant "${conflicting.tenant.name}"` },
409,
);
}
return c.json({ tenants: updated.map((record) => ({ ...safeRecord(record), createdAt: record.createdAt, updatedAt: record.updatedAt })) });

const record: TenantRegistryRecord = { ...existing, orbInstallationId, updatedAt: new Date().toISOString() };
await deps.registry.upsert(record);
return c.json(safeRecord(record));
});

app.delete("/v1/tenants/:name", async (c) => {
Expand All @@ -271,6 +275,17 @@ export function createTenantHttpApp(deps: TenantHttpAppDeps): Hono {

const existing = await deps.registry.get(name, product);
if (!existing) return c.json({ error: "tenant_not_found" }, 404);
// #9143 (defect 8 minimum): a tenant still standing up has no container/database/secrets settled yet --
// tearing it down mid-provision races the in-flight provisionTenant call (which is still writing its OWN
// "active"/"failed" transition to this same record, #7677) and there is nothing durable yet to actually
// revoke/drop/destroy. KV's eventual consistency means even THIS read-then-act check is advisory, not a
// real compare-and-swap (see tenant-registry.ts's own header comment on why a real serialized claim needs
// a Durable Object or D1) -- but refusing the request while the record is observably "provisioning" is
// strictly better than racing it silently, and costs nothing: the caller can simply retry once the
// in-flight create settles to "active" or "failed".
if (existing.state === "provisioning") {
return c.json({ error: "tenant_provisioning", message: `tenant "${name}" is still provisioning; retry once it settles` }, 409);
}

const result = await deprovisionTenant(existing.tenant, existing.product, deps.driver, deps.pagerDuty ?? {}, existing.secretRef);
await deps.registry.upsert({ tenant: result.tenant, product: result.product, state: result.state, createdAt: existing.createdAt, updatedAt: new Date().toISOString() });
Expand Down
Loading