From d029dba663c02d35afa326a7a009696d73e18842 Mon Sep 17 00:00:00 2001 From: Albert <22015497+TheNaubit@users.noreply.github.com> Date: Wed, 9 Sep 2026 23:05:41 +0200 Subject: [PATCH] fix(workspaces): enforce readiness command deadlines --- apps/docs/docs/reference/project-manifest.md | 4 +- .../api/src/workspaces/project-environment.ts | 42 +++++- .../project-environment.integration.test.ts | 33 +++++ services/api/test/project-readiness.test.ts | 129 ++++++++++++++++++ 4 files changed, 202 insertions(+), 6 deletions(-) create mode 100644 services/api/test/project-readiness.test.ts diff --git a/apps/docs/docs/reference/project-manifest.md b/apps/docs/docs/reference/project-manifest.md index 0f6279c9..a2919fe8 100644 --- a/apps/docs/docs/reference/project-manifest.md +++ b/apps/docs/docs/reference/project-manifest.md @@ -89,7 +89,9 @@ setup so a normal wake does not repeatedly seed the environment. Make `start` safe to repeat. It should return after bringing services up; use a detached Compose command, process manager, or equivalent. Make `ready` fail quickly while the service is unavailable -and succeed only when a user can exercise it. +and succeed only when a user can exercise it. Readiness checks share a two-minute deadline, +including time spent inside each check. Reopening a prepared workspace also bounds its initial +readiness probe; a timed-out probe does not restart or reseed the application. ## Secrets and variables diff --git a/services/api/src/workspaces/project-environment.ts b/services/api/src/workspaces/project-environment.ts index 3799a93a..14d70dc1 100644 --- a/services/api/src/workspaces/project-environment.ts +++ b/services/api/src/workspaces/project-environment.ts @@ -19,6 +19,7 @@ import type { WorkspaceLocator, WorkspaceRuntime, } from "./runtime.js"; +import { WorkspaceRuntimeError } from "./runtime.js"; const RepositoryName = z .string() @@ -288,7 +289,7 @@ export class ProjectEnvironmentService { const preparedInput = await this.withDeclaredEnvironment(input); const ready = preparedInput.manifest.environment.ready; const alreadyReady = ready - ? (await this.command(preparedInput, ready, primaryPath(preparedInput.credentials))) + ? (await this.checkReady(preparedInput, Date.now() + (input.readinessTimeoutMs ?? 120_000))) .exitCode === 0 : false; return this.startServices(preparedInput, input.setupChecksum, alreadyReady); @@ -529,9 +530,11 @@ export class ProjectEnvironmentService { const deadline = Date.now() + (input.readinessTimeoutMs ?? 120_000); let last: WorkspaceCommandResult | undefined; while (Date.now() < deadline) { - last = await this.command(input, ready, primaryPath(input.credentials)); + last = await this.checkReady(input, deadline); if (last.exitCode === 0) return; - await new Promise((resolve) => setTimeout(resolve, 500)); + await new Promise((resolve) => + setTimeout(resolve, Math.min(500, Math.max(0, deadline - Date.now()))), + ); } throw new ProjectEnvironmentError("environment_not_ready", "environment readiness timed out", { command: input.manifest.environment.ready, @@ -546,6 +549,30 @@ export class ProjectEnvironmentService { }); } + private async checkReady(input: EnvironmentInput, deadline: number) { + const timeout = () => + new ProjectEnvironmentError("environment_not_ready", "environment readiness timed out", { + command: input.manifest.environment.ready, + }); + const remaining = deadline - Date.now(); + if (remaining <= 0) throw timeout(); + try { + const result = await this.command( + input, + input.manifest.environment.ready ?? "", + primaryPath(input.credentials), + remaining, + ); + if (Date.now() >= deadline) throw timeout(); + return result; + } catch (error) { + if (error instanceof WorkspaceRuntimeError && error.code === "workspace_command_timeout") { + throw timeout(); + } + throw error; + } + } + private async run(input: EnvironmentInput, script: string, cwd: string, phase: string) { const result = await this.command(input, script, cwd); const safeResult = redactResult( @@ -564,13 +591,18 @@ export class ProjectEnvironmentService { return result; } - private command(input: EnvironmentInput, script: string, cwd: string) { + private command( + input: EnvironmentInput, + script: string, + cwd: string, + timeoutMs = 30 * 60 * 1_000, + ) { return this.runtime.exec(input.workspace, { command: "sh", args: ["-lc", script], cwd, env: input.credentials.environment, - timeoutMs: 30 * 60 * 1_000, + timeoutMs, }); } diff --git a/services/api/test/project-environment.integration.test.ts b/services/api/test/project-environment.integration.test.ts index 7027b6bf..446f4dde 100644 --- a/services/api/test/project-environment.integration.test.ts +++ b/services/api/test/project-environment.integration.test.ts @@ -242,6 +242,39 @@ environment: ).toContain("shared"); }); + it("bounds a hanging readiness probe without resetting prepared workspace data", async () => { + const environment = new ProjectEnvironmentService(db, runtime, `file://${remotes}`); + const prepared = await environment.prepare({ + orgId, + projectId, + workspace, + manifest, + credentials, + branch: "facility/story-environment", + }); + const setupCount = await readFile( + join(workspace.volumeRef, "repos/acme/app/.setup-count"), + "utf8", + ); + await expect( + environment.startPrepared({ + orgId, + projectId, + workspace, + credentials, + manifest: { ...manifest, environment: { ...manifest.environment, ready: "exec sleep 2" } }, + setupChecksum: prepared.setupChecksum, + readinessTimeoutMs: 50, + }), + ).rejects.toMatchObject({ code: "environment_not_ready" }); + expect(await readFile(join(workspace.volumeRef, "repos/acme/app/.setup-count"), "utf8")).toBe( + setupCount, + ); + expect( + await readFile(join(workspace.volumeRef, "repos/acme/app/.facility-test/seed"), "utf8"), + ).toBe("seeded"); + }); + it("opens previews on the agent's changed workspace without checkout, setup, or reseeding", async () => { const environment = new ProjectEnvironmentService(db, runtime, `file://${remotes}`); const prepared = await environment.prepare({ diff --git a/services/api/test/project-readiness.test.ts b/services/api/test/project-readiness.test.ts new file mode 100644 index 00000000..1590aa9e --- /dev/null +++ b/services/api/test/project-readiness.test.ts @@ -0,0 +1,129 @@ +import type { FacilityDb } from "@facility/db"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + ProjectEnvironmentService, + parseProjectManifest, +} from "../src/workspaces/project-environment.js"; +import { + type WorkspaceCommand, + type WorkspaceRuntime, + WorkspaceRuntimeError, +} from "../src/workspaces/runtime.js"; + +vi.mock("../src/workspaces/events.js", () => ({ appendWorkspaceEvent: vi.fn() })); + +const success = { exitCode: 0, stdout: "", stderr: "", durationMs: 0 }; +const input = { + orgId: "org_test", + projectId: "proj_test", + workspace: { id: "ws_test", image: "test", externalRef: "test", volumeRef: "test" }, + setupChecksum: "retained", + manifest: parseProjectManifest( + "repositories:\n primary: github.com/acme/app\nenvironment:\n start: start-app\n ready: check-app\n", + ), + credentials: { + gitIdentity: { name: "Test", email: "test@example.invalid" }, + repositories: [{ owner: "acme", name: "app", role: "primary" as const, defaultBranch: "main" }], + environment: {}, + expiresAt: new Date("2030-01-01"), + }, +}; + +function fixture() { + const exec = vi + .fn<(workspace: unknown, command: WorkspaceCommand) => Promise>() + .mockResolvedValue(success); + const expose = vi.fn().mockResolvedValue([]); + const db = { + update: () => ({ set: () => ({ where: async () => undefined }) }), + } as unknown as FacilityDb; + const service = new ProjectEnvironmentService(db, { + exec, + expose, + } as unknown as WorkspaceRuntime); + return { exec, expose, service }; +} + +beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(0); +}); +afterEach(() => { + vi.useRealTimers(); + vi.clearAllMocks(); +}); + +describe("project readiness deadline", () => { + it("bounds the initial prepared-workspace probe and reports runtime timeouts as readiness failures", async () => { + const { exec, expose, service } = fixture(); + exec.mockRejectedValueOnce(new WorkspaceRuntimeError("workspace_command_timeout", "timed out")); + await expect(service.startPrepared(input)).rejects.toMatchObject({ + code: "environment_not_ready", + }); + expect(exec).toHaveBeenCalledWith( + input.workspace, + expect.objectContaining({ args: ["-lc", "check-app"], timeoutMs: 120_000 }), + ); + expect(exec).toHaveBeenCalledTimes(1); + expect(expose).not.toHaveBeenCalled(); + }); + + it("rejects a successful probe that completes after its deadline", async () => { + const { exec, expose, service } = fixture(); + exec.mockImplementationOnce(async () => { + vi.setSystemTime(120_001); + return success; + }); + await expect(service.startPrepared(input)).rejects.toMatchObject({ + code: "environment_not_ready", + }); + expect(expose).not.toHaveBeenCalled(); + }); + + it("uses the remaining budget for retries without reducing the start command timeout", async () => { + const { exec, service } = fixture(); + let probes = 0; + exec.mockImplementation(async (_workspace, command) => { + if (command.args?.[1] !== "check-app") return success; + probes += 1; + if (probes === 2) vi.setSystemTime(1_000); + return { ...success, exitCode: probes < 3 ? 1 : 0 }; + }); + const pending = service.startPrepared({ ...input, readinessTimeoutMs: 2_500 }); + await vi.runAllTimersAsync(); + await expect(pending).resolves.toMatchObject({ setupChecksum: "retained" }); + const commands = exec.mock.calls.map(([, command]) => command); + expect( + commands + .filter((command) => command.args?.[1] === "check-app") + .map((command) => command.timeoutMs), + ).toEqual([2_500, 2_500, 1_000]); + expect(commands.find((command) => command.args?.[1] === "start-app")?.timeoutMs).toBe( + 30 * 60 * 1_000, + ); + }); + + it("reuses a healthy prepared workspace without restarting its application", async () => { + const { exec, expose, service } = fixture(); + await expect(service.startPrepared(input)).resolves.toMatchObject({ + setupChecksum: "retained", + }); + expect(exec).toHaveBeenCalledTimes(1); + expect(expose).toHaveBeenCalledOnce(); + }); + + it("does not replace unrelated runtime failures with timeout errors", async () => { + const { exec, service } = fixture(); + const error = new WorkspaceRuntimeError("workspace_not_found", "missing workspace"); + exec.mockRejectedValueOnce(error); + await expect(service.startPrepared(input)).rejects.toBe(error); + }); + + it("does not start a probe when the configured budget is exhausted", async () => { + const { exec, service } = fixture(); + await expect(service.startPrepared({ ...input, readinessTimeoutMs: 0 })).rejects.toMatchObject({ + code: "environment_not_ready", + }); + expect(exec).not.toHaveBeenCalled(); + }); +});