diff --git a/CHANGELOG.md b/CHANGELOG.md index 26a75ea6..64384efe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Scale-out, phase A: the foundation for API pods that share one database and runs that outlive them** (`docs/plans/scale-out.md`). Every API process has an id (`OPTIO_INSTANCE_ID`, the pod's name from the chart, plus a suffix new at every boot; `GET /api/health` says which instance answered, to a signed-in request only). One migration adds every table and column the plan's phases build on: the attachment columns on runs (`exec_state`, `exec_pid`, `consumed_bytes`, `attached_by`, `attach_lease_until`, on tasks, Job runs, PR-review runs and agent turns), `leases`, `ws_upgrade_tokens`, `inbound_webhook_deliveries`, `glance_state`, `installed_skill_files`, `ticket_sync_claims`, and one active PR review per PR URL. `services/lease-service.ts` is the one way an instance claims something for a while. The **run protocol** (`packages/container-runtime/src/run-protocol.ts`) is on every container runtime — `startRun`, `attachRun`, `deliverStdin`, `killRun`: a start script launches the agent under a supervisor in its own session in the run's home, with stdin fed from a file through a FIFO until the `__OPTIO_STDIN_EOF__` line, its output appended to `output.ndjson` and its exit code to `exit`; the supervisor writes its own pid file (` `) once its TERM trap is in place, and attach, kill and the pooled start guard trust a pid only when the live process's start time matches it; stdin bytes travel over the exec's stdin (`head -c `), never in a script or its environment; an attach is `tail --pid` from a byte offset; the exec that started a run can go away and any instance can attach later. `tini` is the pods' pid 1 (`images/base.Dockerfile`, every init script), so finished runs are reaped — **the agent images must be rebuilt for the protocol**. The repo-cleanup worker removes the run homes of finished runs from every pod (`run-home-sweep-service.ts`), the backstop for the attached worker that removes them once a run is terminal and consumed (phase B). `buildTaskStartScript` / `buildPooledStartScript` build the start scripts beside the exec scripts the workers still use (they switch in phase B). The fake runtime keeps its pods on disk (`OPTIO_FAKE_RUNTIME_DIR`) and plays a protocol run as a detached process (`fake-agent.mjs`, every event with a `seq`), and `test-utils/e2e/api-cluster.ts` boots N real API servers on one database for the e2e tier: two boot together, one is SIGKILLed, a third joins. +- **Scale-out, phase C1: coordination that holds across API instances** (`docs/plans/scale-out.md` §3). A run's concurrency claim — fewer than the limit running, so take this one — runs under a Postgres advisory lock on the global limit (`claim:tasks`, `claim:workflows`; a repo's or a Job's own limit is counted under it) in one READ COMMITTED transaction (`services/claim-lock.ts`), in place of a mutex one process kept to itself; the count and the compare-and-swap claim run on the lock's transaction (`claimTransitionIn`, `claimWorkflowRunIn`), and what follows the claim (events, webhooks, the reconciler) is registered with `afterCommit` and runs once it commits. A claimer waits for the lock at most 10 s (`lock_timeout`) and otherwise re-queues as when a limit is full. Every periodic sweep (ticket sync, the schedule checker, external PR review, pod cleanup, the PR watcher, the reconcile resync, skill sync, token validation, the config directory) runs on one instance at a time under a lease (`services/poller-lease.ts`), and each is safe to overlap anyway: ticket sync claims each (source, ticket, repo) in `ticket_sync_claims` before creating a task (a claim whose task was deleted, or whose sweep died before making one, is taken over); the schedule checker advances a trigger's `next_fire_at` by compare-and-swap before firing it, so a tick fires once; an external PR review sweep launches only while the review is still the one it saw (none, for a new PR) and treats a lost race or the one-active-review-per-PR conflict as someone else's launch. Repeat jobs are BullMQ job schedulers with stable ids (`services/repeat-jobs.ts`), so instances booting in any order leave exactly one schedule per tick; boot no longer wipes every repeat job, only the hash-keyed ones an older build registered. Inbound webhook deliveries (GitHub, GitLab, Slack, Linear, Jira, PagerDuty, Sentry) are claimed in `inbound_webhook_deliveries` (`services/inbound-delivery-service.ts`) instead of a per-process set and a Redis key; WebSocket upgrade tokens live in `ws_upgrade_tokens`, so a token minted on one instance is accepted by whichever one the browser's upgrade lands on, once; the Optio assistant's one-conversation-per-user rule (30 s lease renewed every 10 s), a user's GitHub token refresh and the config directory's apply are leases (`withLease` takes a `wait` option for the latter two). A GitHub user token is only ever refreshed under its lease — GitHub rotates the refresh token, so two instances refreshing at once would strand one: a waiter re-reads the token the holder stored, the call to GitHub times out after 15 s, a wait that times out uses the stored token while it works and otherwise the PAT, and a refused refresh deletes the stored tokens only while the refresh token is still the one it sent. A skill sync that finds the skill's lease held retries with backoff, and records its result only while the skill's ref and subpath are still the ones it resolved (a compare-and-swap in the same transaction): a sync of the old ref that finishes after a PATCH moved the ref writes nothing, and the skill stays due for the next pass. "Sync now" answers 409 while another instance is applying the directory. A webhook delivery's claim gets one second (`statement_timeout`) before the delivery is accepted unclaimed; an upgrade token's expiry is the database's clock. A housekeeping tick (`workers/sweep-worker.ts`, every 30 s under a lease, `registerSweep` for more) removes deliveries older than a day, expired upgrade tokens and long-expired leases. `docs/production-eks.md` says what stays per pod. Coverage: integration tests drive every claim with two racing callers; `apps/api/e2e/scale-out-coordination.e2e.test.ts` runs two real API servers with auth on — an upgrade token minted on A accepted by B once, one signed webhook delivery posted to both firing once, a due schedule fired once by two sweepers, and one scheduler per repeat job after both boot. - **Android: every trigger type in the automations and agent trigger sheets.** A Local automation's Add trigger sheet and a persistent agent's New trigger sheet take all fourteen trigger types — GitLab, Jira, Pylon, PagerDuty, Sentry, Alertmanager and Datadog events included, with their event kinds, identity and filters — through the same rows as the New work form (one trigger editor in `core:ui`, `triggers/`). A Pylon / Alertmanager / Datadog trigger's own URL and shared secret are shown once, with copy buttons, right after it is created, in the sheets and in the New work form. - **Every agent runtime gets the work's MCP servers.** Connections' tools and MCP servers reached only Claude Code (`.mcp.json`) and Codex (`config.toml`); a Job on Gemini, a persistent agent on Copilot, or a Task on OpenCode or Cursor had the connections' credentials and notes but no tools. Each runtime now gets the same servers in the file it reads, established from the versions the agent image installs: Gemini CLI in the user `settings.json` of a `GEMINI_CLI_HOME` of the run's own (the adapter's settings merged in, each server `trust: true`), OpenCode in an `OPENCODE_CONFIG` file merged before the project's own, GitHub Copilot CLI through `--additional-mcp-config @` (`type: "local"`, every tool enabled), and Cursor in the project's `.cursor/mcp.json` with `--approve-mcps`. See `docs/connections.md` → "Which runtime reads which file". - The per-run files live in one **run home**, `/home/agent/optio/runs/` (`OPTIO_RUN_HOME`), named after the task, Job run, or agent turn — so a retry lands in the same place — and removed when the run's script exits and again when the task's worktree is cleaned up. Codex's per-run `CODEX_HOME` moves there too; it used to be a random directory that was never removed from the repo pod's home volume. diff --git a/CLAUDE.md b/CLAUDE.md index 06f73844..703681f6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -281,7 +281,7 @@ After changing backend code, ALWAYS rebuild + redeploy the local cluster and ver - **Drizzle ORM**: schema in `apps/api/src/db/schema.ts`, run `drizzle-kit generate` after changes. **New migrations use unix-timestamp prefixes** (`migrations.prefix: "unix"` in `drizzle.config.ts`). Existing `00xx_*` files are frozen — never rename them. Migrations are hand-written SQL (the drizzle-kit snapshots stopped at `0012`). **Run columns**: add them to `tasks` as always (`ALTER TABLE "tasks" ...`) and to `runColumns()` in `schema.ts` (the table `workRuns` and the view `tasks` = `repo_tasks` both use it); `migrate-safe.ts` drops the run views before every migration and remakes them after (`db/run-views.ts`), so `repo_tasks` picks the column up by itself. A column Job runs need goes in `workflow_runs`'s list in `run-views.ts` too, and in `workflowRuns` in the schema - **Zustand**: use `useStore.getState()` in callbacks/effects, not hook selectors (avoids infinite re-renders) - **Next.js webpack**: `extensionAlias` in `next.config.ts` resolves `.js` → `.ts` for workspace packages -- **State transitions**: always go through `taskService.transitionTask()` — validates, updates DB, records event, publishes WebSocket +- **State transitions**: always go through `taskService.transitionTask()` — validates, updates DB, records event, publishes WebSocket. Its in-transaction form is `claimTransitionIn(claim, …)` inside `withClaimLock` (`services/claim-lock.ts`): the write runs on the claim's transaction and the announcement is registered with `claim.afterCommit`, so it runs only once the transaction commits - **Secrets**: never log or return secret values. Encrypted at rest with AES-256-GCM - **Cost tracking**: stored as string (`costUsd`) to avoid float precision issues - **K8s RBAC**: namespace-scoped Role (pods, exec, secrets, PVCs) + ClusterRole (nodes, namespaces, metrics) diff --git a/apps/api/e2e/scale-out-coordination.e2e.test.ts b/apps/api/e2e/scale-out-coordination.e2e.test.ts new file mode 100644 index 00000000..ef1dc18a --- /dev/null +++ b/apps/api/e2e/scale-out-coordination.e2e.test.ts @@ -0,0 +1,255 @@ +/** + * E2E: phase C1 of docs/plans/scale-out.md through two REAL API servers on + * one database (the api-cluster harness), with auth ENABLED so the + * WebSocket upgrade path is the real one: + * + * 6. a WebSocket upgrade token minted on A is accepted by B, once; + * 7. the same signed webhook delivery posted to A and to B fires once; + * 8. a schedule trigger due now, with both servers sweeping, fires once; + * + two servers booting leave exactly one scheduler per repeat job. + */ +import { createHash, randomBytes, randomUUID } from "node:crypto"; +import { Queue } from "bullmq"; +import postgres from "postgres"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { startApiCluster, type ApiCluster } from "../src/test-utils/e2e/api-cluster.js"; +import { waitFor } from "../src/test-utils/e2e/api-server.js"; +import { REPEAT_SCHEDULERS } from "../src/services/repeat-jobs.js"; + +const GITLAB_WEBHOOK_SECRET = "e2e-scale-out-gitlab-token"; + +let cluster: ApiCluster; +let adminToken = ""; + +/** An admin with a session, seeded straight into the DB (like webhook-ingress-auth.e2e). */ +async function seedAdmin(): Promise { + const sql = postgres(process.env.DATABASE_URL!, { max: 1 }); + try { + const wsId = randomUUID(); + const userId = randomUUID(); + adminToken = `e2e-admin-${randomBytes(16).toString("hex")}`; + const tokenHash = createHash("sha256").update(adminToken).digest("hex"); + await sql`INSERT INTO workspaces (id, name, slug) VALUES (${wsId}, 'Scale-out e2e', ${`scale-out-e2e-${wsId.slice(0, 8)}`})`; + await sql` + INSERT INTO users (id, provider, external_id, email, display_name, default_workspace_id) + VALUES (${userId}, 'github', 'scale-out-e2e-admin', 'admin@scale-out.e2e', 'Scale-out admin', ${wsId})`; + await sql`INSERT INTO workspace_members (workspace_id, user_id, role) VALUES (${wsId}, ${userId}, 'admin')`; + await sql` + INSERT INTO sessions (user_id, token_hash, expires_at) + VALUES (${userId}, ${tokenHash}, NOW() + INTERVAL '1 day')`; + } finally { + await sql.end(); + } +} + +beforeAll(async () => { + await seedAdmin(); + cluster = await startApiCluster({ + size: 2, + env: { + OPTIO_AUTH_DISABLED: "false", + GITLAB_WEBHOOK_SECRET, + OPTIO_WORKFLOW_TRIGGER_INTERVAL: "1000", + }, + }); +}, 240_000); + +afterAll(async () => { + await cluster?.stop(); +}); + +function server(i: number): string { + return cluster.servers[i].handle.baseUrl; +} + +async function api( + i: number, + method: string, + path: string, + body?: unknown, +): Promise<{ status: number; body: T }> { + const res = await fetch(`${server(i)}${path}`, { + method, + headers: { + authorization: `Bearer ${adminToken}`, + ...(body !== undefined ? { "content-type": "application/json" } : {}), + }, + ...(body !== undefined ? { body: JSON.stringify(body) } : {}), + }); + return { status: res.status, body: (await res.json().catch(() => null)) as T }; +} + +/** + * Opens a WebSocket with the upgrade token in the subprotocol. The upgrade + * itself always completes; the server authenticates after it and closes + * with 4401 when the token is no good. So: accepted when the socket is + * still open a moment later (then we close it), else the server's code. + */ +function upgrade(i: number, token: string): Promise<{ accepted: boolean; closeCode?: number }> { + return new Promise((resolve) => { + const url = `${server(i).replace(/^http/, "ws")}/ws/events`; + const ws = new WebSocket(url, ["optio-ws-v1", `optio-auth-${token}`]); + let settled = false; + let timer: NodeJS.Timeout | undefined; + ws.addEventListener("open", () => { + timer = setTimeout(() => { + settled = true; + ws.close(1000); + resolve({ accepted: true }); + }, 1500); + }); + ws.addEventListener("close", (ev) => { + clearTimeout(timer); + if (!settled) resolve({ accepted: false, closeCode: ev.code }); + }); + ws.addEventListener("error", () => { + /* close follows */ + }); + }); +} + +describe("scale-out coordination across two API servers", () => { + it("a WebSocket upgrade token minted on A is accepted by B, and only once", async () => { + const minted = await api<{ token: string }>(0, "GET", "/api/auth/ws-token"); + expect(minted.status).toBe(200); + expect(minted.body.token).not.toBe("auth-disabled"); + + const onB = await upgrade(1, minted.body.token); + expect(onB).toEqual({ accepted: true }); + + // Consumed on B: A rejects the same token. + const again = await upgrade(0, minted.body.token); + expect(again).toEqual({ accepted: false, closeCode: 4401 }); + }, 30_000); + + it("the same signed webhook delivery posted to A and to B fires once", async () => { + const project = `acme/scale-out-${Date.now()}`; + const created = await api<{ workflow: { id: string } }>(0, "POST", "/api/jobs", { + name: `push summary ${Date.now()}`, + promptTemplate: "Summarize {{commits}} on {{sourceBranch}}", + agentRuntime: "claude-code", + }); + expect(created.status, JSON.stringify(created.body)).toBe(201); + const jobId = created.body.workflow.id; + const trigger = await api(0, "POST", `/api/jobs/${jobId}/triggers`, { + type: "gitlab", + config: { events: ["push"], projects: [project], branches: ["main"] }, + }); + expect(trigger.status, JSON.stringify(trigger.body)).toBe(201); + + const raw = JSON.stringify({ + object_kind: "push", + ref: "refs/heads/main", + before: "aaaa1111", + after: "bbbb2222", + user_username: "alice", + project: { path_with_namespace: project, web_url: `https://gitlab.com/${project}` }, + commits: [{ id: "bbbb2222", title: "fix: thing", message: "fix: thing" }], + }); + const deliveryId = randomUUID(); + const deliver = async (i: number) => { + const res = await fetch(`${server(i)}/api/webhooks/gitlab`, { + method: "POST", + headers: { + "content-type": "application/json", + "x-gitlab-event": "Push Hook", + "x-gitlab-event-uuid": deliveryId, + "x-gitlab-token": GITLAB_WEBHOOK_SECRET, + }, + body: raw, + }); + return { status: res.status, body: await res.json() }; + }; + + // The provider's retry lands on the other instance. + const [onA, onB] = await Promise.all([deliver(0), deliver(1)]); + expect(onA.status).toBe(200); + expect(onB.status).toBe(200); + expect([onA.body, onB.body]).toEqual( + expect.arrayContaining([{ ok: true }, { ok: true, duplicate: true }]), + ); + const runs = async () => + (await api<{ runs: unknown[] }>(1, "GET", `/api/jobs/${jobId}/runs`)).body.runs; + await waitFor(async () => ((await runs()).length >= 1 ? true : null), { + timeoutMs: 30_000, + label: "a run of the GitLab-triggered Job", + }); + await new Promise((r) => setTimeout(r, 1500)); + expect(await runs()).toHaveLength(1); + }, 60_000); + + it("a schedule trigger due now fires once with both servers sweeping", async () => { + const created = await api<{ workflow: { id: string } }>(1, "POST", "/api/jobs", { + name: `nightly ${Date.now()}`, + promptTemplate: "Nightly sweep", + agentRuntime: "claude-code", + }); + expect(created.status).toBe(201); + const jobId = created.body.workflow.id; + // A yearly cron: once nextFireAt is pulled into the past it is due + // exactly once, and the advance puts it a year away. + const trigger = await api<{ trigger: { id: string } }>( + 1, + "POST", + `/api/jobs/${jobId}/triggers`, + { + type: "schedule", + config: { cronExpression: "0 0 1 1 *" }, + }, + ); + expect(trigger.status, JSON.stringify(trigger.body)).toBe(201); + const sql = postgres(process.env.DATABASE_URL!, { max: 1 }); + try { + await sql`UPDATE workflow_triggers SET next_fire_at = NOW() - INTERVAL '1 minute' WHERE id = ${trigger.body.trigger.id}`; + } finally { + await sql.end(); + } + + const runs = async () => + (await api<{ runs: unknown[] }>(0, "GET", `/api/jobs/${jobId}/runs`)).body.runs; + await waitFor(async () => ((await runs()).length >= 1 ? true : null), { + timeoutMs: 30_000, + label: "the schedule to fire", + }); + // Both servers sweep every second; give them several more ticks. + await new Promise((r) => setTimeout(r, 4000)); + expect(await runs()).toHaveLength(1); + const listed = await api<{ triggers: Array<{ id: string; nextFireAt: string | null }> }>( + 0, + "GET", + `/api/jobs/${jobId}/triggers`, + ); + const after = listed.body.triggers.find((t) => t.id === trigger.body.trigger.id); + expect(new Date(after!.nextFireAt!).getTime()).toBeGreaterThan(Date.now()); + }, 60_000); + + it("two servers booting leave exactly one scheduler per repeat job", async () => { + const { getBullMQOptions } = await import("../src/services/redis-config.js"); + const opts = getBullMQOptions(); + const expectations: Array<[string, number]> = [ + ["pr-watcher", 1], + ["external-pr-review", 1], + ["repo-cleanup", 2], + ["ticket-sync", 1], + ["workflow-trigger-checker", 1], + ["token-validation", 1], + ["reconcile-resync", 1], + ["skill-sync", 1], + ["housekeeping", 1], + ]; + for (const [name, expected] of expectations) { + const queue = new Queue(name, { ...opts }); + try { + const schedulers = await queue.getJobSchedulers(); + expect(schedulers.map((s) => s.key).sort(), `${name} schedulers`).toEqual( + [...REPEAT_SCHEDULERS[name as keyof typeof REPEAT_SCHEDULERS]].sort(), + ); + expect(schedulers).toHaveLength(expected); + // And nothing hash-keyed beside them. + expect(await queue.getRepeatableJobs()).toHaveLength(expected); + } finally { + await queue.close(); + } + } + }, 30_000); +}); diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index c4dc5fc1..6a1b508c 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -71,6 +71,8 @@ async function main() { await import("./workers/reconcile-worker.js"); const { startSkillSyncWorker } = await import("./workers/skill-sync-worker.js"); const { startConfigSyncWorker } = await import("./workers/config-sync-worker.js"); + const { startSweepWorker } = await import("./workers/sweep-worker.js"); + const { REPEAT_QUEUE_NAMES, removeLegacyRepeatables } = await import("./services/repeat-jobs.js"); const { getBullMQOptions, describeRedisConfig, inspectRedisEviction, closeSharedRedisClients } = await import("./services/redis-config.js"); const { logTlsStackInfo, initTlsObservability } = await import("./services/tls-observability.js"); @@ -78,22 +80,15 @@ async function main() { const bullmqOpts = getBullMQOptions(); /** - * Remove all stale repeatable jobs from a queue before re-registering. - * Prevents duplicate/orphaned repeat jobs after server restarts. + * Every periodic worker registers its tick as a job scheduler with a + * stable id (services/repeat-jobs.ts), so instances never wipe each + * other's schedules. What a build before that registered — hash-keyed + * repeatables — would tick beside them; remove those once per boot. */ - async function cleanRepeatJobs(queueName: string) { + async function cleanLegacyRepeatJobs(queueName: (typeof REPEAT_QUEUE_NAMES)[number]) { const queue = new Queue(queueName, { ...bullmqOpts }); try { - const repeatableJobs = await queue.getRepeatableJobs(); - for (const job of repeatableJobs) { - await queue.removeRepeatableByKey(job.key); - } - if (repeatableJobs.length > 0) { - logger.info( - { queue: queueName, removed: repeatableJobs.length }, - "Cleaned stale repeat jobs", - ); - } + await removeLegacyRepeatables(queue, queueName); } finally { await queue.close(); } @@ -264,24 +259,13 @@ async function main() { // --- Background initialization (after listen) --- - // Clean stale repeat jobs from previous server sessions - await Promise.all([ - cleanRepeatJobs("pr-watcher"), - cleanRepeatJobs("external-pr-review"), - cleanRepeatJobs("repo-cleanup"), - cleanRepeatJobs("ticket-sync"), - cleanRepeatJobs("workflow-runs"), - cleanRepeatJobs("workflow-trigger-checker"), - cleanRepeatJobs("token-validation"), - cleanRepeatJobs("reconcile"), - cleanRepeatJobs("reconcile-resync"), - cleanRepeatJobs("skill-sync"), - ]); + // Repeat jobs an older build registered (see cleanLegacyRepeatJobs). + await Promise.all(REPEAT_QUEUE_NAMES.map((q) => cleanLegacyRepeatJobs(q))); // Recover before starting workers so a newly claimed run is never mistaken for an orphan. await reconcileOrphanedTasks(); - // Start BullMQ workers (each re-registers its repeat job) + // Start BullMQ workers (each upserts its repeat job scheduler) const worker = startTaskWorker(); logger.info("Task worker started"); @@ -327,6 +311,9 @@ async function main() { const localSweepWorker = startLocalSweepWorker(); logger.info("Local sweep worker started"); + const sweepWorker = startSweepWorker(); + logger.info("Housekeeping worker started"); + const { startGlanceSweepWorker } = await import("./workers/glance-sweep-worker.js"); const glanceSweepWorker = startGlanceSweepWorker(); logger.info("Glance sweep worker started"); @@ -355,6 +342,7 @@ async function main() { await reconcileResyncWorker.close(); await skillSyncWorker.close(); await localSweepWorker.close(); + await sweepWorker.close(); await glanceSweepWorker.close(); await configSyncWorker?.close(); await app.close(); diff --git a/apps/api/src/routes/config.test.ts b/apps/api/src/routes/config.test.ts new file mode 100644 index 00000000..05e2e192 --- /dev/null +++ b/apps/api/src/routes/config.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import type { FastifyInstance } from "fastify"; +import { buildRouteTestApp } from "../test-utils/build-route-test-app.js"; + +const mockSyncEnvSource = vi.fn(); + +vi.mock("../services/config/source.js", () => { + class ConfigApplyBusyError extends Error {} + return { + ConfigApplyBusyError, + configStatus: vi.fn(), + syncEnvSource: (...args: unknown[]) => mockSyncEnvSource(...args), + }; +}); +vi.mock("../services/config/apply.js", () => ({ applyManifests: vi.fn(), detachObject: vi.fn() })); +vi.mock("../services/config/export.js", () => ({ + exportManifests: vi.fn(), + manifestsToYaml: vi.fn(), +})); +vi.mock("../services/optio-action-service.js", () => ({ logAction: vi.fn() })); + +import { configRoutes } from "./config.js"; +import { ConfigApplyBusyError } from "../services/config/source.js"; + +describe("POST /api/config/source/sync", () => { + let app: FastifyInstance; + + beforeEach(async () => { + vi.clearAllMocks(); + app = await buildRouteTestApp(configRoutes); + }); + + it("answers 409 with a readable message while another instance is applying", async () => { + mockSyncEnvSource.mockRejectedValue(new ConfigApplyBusyError()); + const res = await app.inject({ method: "POST", url: "/api/config/source/sync" }); + expect(res.statusCode).toBe(409); + expect(res.json().error).toMatch(/still running; try again/); + }); + + it("answers 404 when config as code is off", async () => { + mockSyncEnvSource.mockResolvedValue(null); + const res = await app.inject({ method: "POST", url: "/api/config/source/sync" }); + expect(res.statusCode).toBe(404); + }); +}); diff --git a/apps/api/src/routes/config.ts b/apps/api/src/routes/config.ts index 0ab362f7..f96bcafd 100644 --- a/apps/api/src/routes/config.ts +++ b/apps/api/src/routes/config.ts @@ -25,7 +25,7 @@ import { } from "../schemas/config.js"; import { applyManifests, detachObject } from "../services/config/apply.js"; import { exportManifests, manifestsToYaml } from "../services/config/export.js"; -import { configStatus, syncEnvSource } from "../services/config/source.js"; +import { ConfigApplyBusyError, configStatus, syncEnvSource } from "../services/config/source.js"; import { logAction } from "../services/optio-action-service.js"; const SCHEMA_PATH = "/api/config/schema.json"; @@ -108,11 +108,28 @@ export async function configRoutes(rawApp: FastifyInstance) { "`?dryRun=true` only plans it. Requires `admin` role.", tags: ["Config"], querystring: SyncQuerySchema, - response: { 200: ConfigApplyResultSchema, 404: ErrorResponseSchema }, + response: { + 200: ConfigApplyResultSchema, + 404: ErrorResponseSchema, + 409: ErrorResponseSchema, + }, }, }, async (req, reply) => { - const result = await syncEnvSource({ dryRun: req.query.dryRun === "true" }); + // Waits for an apply running on another instance (the default), up to + // a minute; past that, a 409 the person can retry. + let result: Awaited>; + try { + result = await syncEnvSource({ dryRun: req.query.dryRun === "true" }); + } catch (err) { + if (err instanceof ConfigApplyBusyError) { + return reply.status(409).send({ + error: + "Another apply of the configuration directory is still running; try again in a minute.", + }); + } + throw err; + } if (!result) { return reply .status(404) diff --git a/apps/api/src/routes/event-ingress-sources.test.ts b/apps/api/src/routes/event-ingress-sources.test.ts index b8a316d4..38fe7df3 100644 --- a/apps/api/src/routes/event-ingress-sources.test.ts +++ b/apps/api/src/routes/event-ingress-sources.test.ts @@ -16,12 +16,20 @@ vi.mock("../services/event-trigger-service.js", async () => { }); vi.mock("../db/client.js", () => ({ db: {} })); vi.mock("../services/trigger-dispatch.js", () => ({ fireTrigger: vi.fn() })); -const mockRedisSet = vi.fn(); -vi.mock("../services/event-bus.js", () => ({ - getRedisClient: () => ({ set: (...args: unknown[]) => mockRedisSet(...args) }), +// Every receiver claims the delivery id in the database first +// (services/inbound-delivery-service.ts); the claim is a set here. +const claimed = new Set(); +const mockClaim = vi.fn(async (source: string, id: string) => { + const key = `${source}:${id}`; + if (claimed.has(key)) return false; + claimed.add(key); + return true; +}); +vi.mock("../services/inbound-delivery-service.js", () => ({ + claimInboundDelivery: (...args: unknown[]) => mockClaim(...(args as [string, string])), })); -import { eventIngressRoutes, resetSlackEventDedupe } from "./event-ingress.js"; +import { eventIngressRoutes } from "./event-ingress.js"; const GITLAB_SECRET = "gl-token"; const JIRA_SECRET = "jira-secret"; @@ -37,8 +45,8 @@ beforeEach(async () => { process.env.JIRA_WEBHOOK_SECRET = JIRA_SECRET; process.env.SENTRY_WEBHOOK_SECRET = SENTRY_SECRET; mockFire.mockReset().mockResolvedValue([]); - mockRedisSet.mockReset().mockResolvedValue("OK"); - resetSlackEventDedupe(); + mockClaim.mockClear(); + claimed.clear(); app = await buildRouteTestApp(eventIngressRoutes, { user: null }); }); @@ -78,13 +86,7 @@ describe("POST /api/webhooks/gitlab", () => { "gitlab", expect.objectContaining({ kinds: ["push"], project: "acme/optio", sourceBranch: "main" }), ); - expect(mockRedisSet).toHaveBeenCalledWith( - "optio:webhook-delivery:gitlab:u-1", - "1", - "EX", - expect.any(Number), - "NX", - ); + expect(mockClaim).toHaveBeenCalledWith("gitlab", "u-1"); const again = await post({ "x-gitlab-token": GITLAB_SECRET, "x-gitlab-event-uuid": "u-1" }); expect(again.json()).toEqual({ ok: true, duplicate: true }); await flush(); @@ -151,13 +153,7 @@ describe("POST /api/webhooks/jira", () => { "jira", expect.objectContaining({ key: "ENG-42", kinds: ["created"] }), ); - expect(mockRedisSet).toHaveBeenCalledWith( - "optio:webhook-delivery:jira:jira:issue_created:10001:1700000000000", - "1", - "EX", - expect.any(Number), - "NX", - ); + expect(mockClaim).toHaveBeenCalledWith("jira", "jira:issue_created:10001:1700000000000"); }); it("fires a comment once when it arrives both as comment_created and as an issue update", async () => { @@ -238,13 +234,7 @@ describe("POST /api/webhooks/sentry", () => { "sentry", expect.objectContaining({ kind: "issue_created", shortId: "API-1A", project: "api" }), ); - expect(mockRedisSet).toHaveBeenCalledWith( - "optio:webhook-delivery:sentry:issue:created:123:1700000000", - "1", - "EX", - expect.any(Number), - "NX", - ); + expect(mockClaim).toHaveBeenCalledWith("sentry", "issue:created:123:1700000000"); // The same delivery again (same timestamp) is a redelivery. const again = await post(issueCreated, signed(issueCreated, "issue")); expect(again.json()).toEqual({ ok: true, duplicate: true }); diff --git a/apps/api/src/routes/event-ingress.test.ts b/apps/api/src/routes/event-ingress.test.ts index f10dd2e8..e2ec984f 100644 --- a/apps/api/src/routes/event-ingress.test.ts +++ b/apps/api/src/routes/event-ingress.test.ts @@ -12,15 +12,21 @@ vi.mock("../services/event-trigger-service.js", async () => { }); vi.mock("../db/client.js", () => ({ db: {} })); vi.mock("../services/trigger-dispatch.js", () => ({ fireTrigger: vi.fn() })); -// `claimDelivery` also claims the id in Redis (SET NX); every claim is first here. -const mockRedisSet = vi.fn(); -vi.mock("../services/event-bus.js", () => ({ - getRedisClient: () => ({ set: (...args: unknown[]) => mockRedisSet(...args) }), +// Every receiver claims the delivery id in the database first +// (services/inbound-delivery-service.ts); the claim is a set here. +const claimed = new Set(); +const mockClaim = vi.fn(async (source: string, id: string) => { + const key = `${source}:${id}`; + if (claimed.has(key)) return false; + claimed.add(key); + return true; +}); +vi.mock("../services/inbound-delivery-service.js", () => ({ + claimInboundDelivery: (...args: unknown[]) => mockClaim(...(args as [string, string])), })); import { eventIngressRoutes, - resetSlackEventDedupe, verifyLinearSignature, verifySlackSignature, } from "./event-ingress.js"; @@ -79,8 +85,8 @@ beforeEach(async () => { process.env.LINEAR_WEBHOOK_SECRET = LINEAR_SECRET; process.env.PAGERDUTY_WEBHOOK_SECRET = PAGERDUTY_SECRET; mockFire.mockReset().mockResolvedValue([]); - mockRedisSet.mockReset().mockResolvedValue("OK"); - resetSlackEventDedupe(); + mockClaim.mockClear(); + claimed.clear(); app = await buildRouteTestApp(eventIngressRoutes, { user: null }); }); @@ -253,13 +259,7 @@ describe("POST /api/webhooks/pagerduty", () => { eventId: "ev-PINC1", }), ); - expect(mockRedisSet).toHaveBeenCalledWith( - "optio:webhook-delivery:pagerduty:wh-1", - "1", - "EX", - expect.any(Number), - "NX", - ); + expect(mockClaim).toHaveBeenCalledWith("pagerduty", "wh-1"); }); it("accepts a header carrying several signatures when one matches", async () => { diff --git a/apps/api/src/routes/event-ingress.ts b/apps/api/src/routes/event-ingress.ts index 72e32cd0..c422181b 100644 --- a/apps/api/src/routes/event-ingress.ts +++ b/apps/api/src/routes/event-ingress.ts @@ -16,7 +16,7 @@ import type { FastifyInstance } from "fastify"; import type { ZodTypeProvider } from "fastify-type-provider-zod"; import { z } from "zod"; import { logger } from "../logger.js"; -import { getRedisClient } from "../services/event-bus.js"; +import { claimInboundDelivery, type DeliverySource } from "../services/inbound-delivery-service.js"; import { ErrorResponseSchema } from "../schemas/common.js"; import { fireEventTriggers, @@ -48,82 +48,17 @@ export { }; /** Providers whose delivery ids the receivers remember. */ -export type DeliveryProvider = - | "slack" - | "linear" - | "github" - | "pagerduty" - | "gitlab" - | "jira" - | "sentry"; +export type DeliveryProvider = DeliverySource; const OkResponse = z.object({ ok: z.boolean(), duplicate: z.boolean().optional() }); const ChallengeResponse = z.object({ challenge: z.string() }); /** - * Remember recent delivery ids so provider retries and replays inside the - * signature window don't fire an automation twice. In-process, like the - * local relay itself (single API replica): a restart forgets, which at - * worst re-fires a delivery retried across the restart. + * Provider retries and replays inside the signature window must not fire an + * automation twice, on any API instance: every receiver claims the + * delivery's id in the database first (services/inbound-delivery-service.ts). */ -const RECENT_EVENT_IDS_MAX = 4000; -const recentDeliveryIds = new Set(); -export function rememberDelivery(provider: DeliveryProvider, id: string): boolean { - const key = `${provider}:${id}`; - if (recentDeliveryIds.has(key)) return false; - recentDeliveryIds.add(key); - if (recentDeliveryIds.size > RECENT_EVENT_IDS_MAX) { - const first = recentDeliveryIds.values().next().value; - if (first) recentDeliveryIds.delete(first); - } - return true; -} - -/** How long a claimed delivery id stays claimed in Redis. */ -const DELIVERY_DEDUPE_TTL_SECS = 24 * 60 * 60; -/** Don't hold a webhook ack hostage to a slow / unreachable Redis. */ -const DELIVERY_DEDUPE_REDIS_TIMEOUT_MS = 1000; - -/** - * Durable variant of {@link rememberDelivery}: also claims the id in Redis - * (SET NX, 24 h TTL) so a replay is dropped across API restarts and replicas. - * Returns true when this caller is the first to see the delivery. Fails open - * (true) when Redis errors or is slow — the signature check still gates it. - */ -export async function claimDelivery(provider: DeliveryProvider, id: string): Promise { - if (!rememberDelivery(provider, id)) return false; - let timer: NodeJS.Timeout | undefined; - try { - const result = await Promise.race([ - getRedisClient().set( - `optio:webhook-delivery:${provider}:${id}`, - "1", - "EX", - DELIVERY_DEDUPE_TTL_SECS, - "NX", - ), - new Promise<"timeout">((resolve) => { - timer = setTimeout(() => resolve("timeout"), DELIVERY_DEDUPE_REDIS_TIMEOUT_MS); - timer.unref?.(); - }), - ]); - if (result === "timeout") { - logger.warn({ provider }, "Webhook delivery dedupe timed out in Redis; accepting"); - return true; - } - return result === "OK"; - } catch (err) { - logger.warn({ err, provider }, "Webhook delivery dedupe failed in Redis; accepting"); - return true; - } finally { - clearTimeout(timer); - } -} - -/** Exported for tests. */ -export function resetSlackEventDedupe(): void { - recentDeliveryIds.clear(); -} +export const claimDelivery = claimInboundDelivery; export async function eventIngressRoutes(rawApp: FastifyInstance) { const app = rawApp.withTypeProvider(); @@ -171,8 +106,8 @@ export async function eventIngressRoutes(rawApp: FastifyInstance) { const event = normalizeSlackEvent(body); if (!event) return reply.status(200).send({ ok: true }); - if (event.eventId && !rememberDelivery("slack", event.eventId)) { - return reply.status(200).send({ ok: true }); + if (event.eventId && !(await claimDelivery("slack", event.eventId))) { + return reply.status(200).send({ ok: true, duplicate: true }); } await reply.status(200).send({ ok: true }); @@ -217,8 +152,8 @@ export async function eventIngressRoutes(rawApp: FastifyInstance) { // identifies a delivery well enough to drop retries of it. const data = (body.data ?? {}) as Record; const deliveryKey = `${body.type}:${body.action}:${data.id ?? ""}:${body.webhookTimestamp ?? ""}`; - if (!rememberDelivery("linear", deliveryKey)) { - return reply.status(200).send({ ok: true }); + if (!(await claimDelivery("linear", deliveryKey))) { + return reply.status(200).send({ ok: true, duplicate: true }); } await reply.status(200).send({ ok: true }); diff --git a/apps/api/src/routes/tickets.test.ts b/apps/api/src/routes/tickets.test.ts index 0676e930..ba6d1dde 100644 --- a/apps/api/src/routes/tickets.test.ts +++ b/apps/api/src/routes/tickets.test.ts @@ -1,20 +1,24 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { createHmac } from "node:crypto"; -// Shared Redis stand-in for the durable delivery-id dedupe (SET NX EX). -const redisKeys = new Set(); -const redisSet = vi.fn(async (key: string) => { - if (redisKeys.has(key)) return null; - redisKeys.add(key); - return "OK"; -}); vi.mock("../services/event-bus.js", () => ({ - getRedisClient: () => ({ set: redisSet }), + getRedisClient: () => ({}), publishEvent: vi.fn(), })); +// Every receiver claims the delivery id in the database first +// (services/inbound-delivery-service.ts); the claim is a set here. +const claimed = new Set(); +const mockClaim = vi.fn(async (source: string, id: string) => { + const key = `${source}:${id}`; + if (claimed.has(key)) return false; + claimed.add(key); + return true; +}); +vi.mock("../services/inbound-delivery-service.js", () => ({ + claimInboundDelivery: (...args: unknown[]) => mockClaim(...(args as [string, string])), +})); import { verifyGitHubSignature } from "./tickets.js"; -import { resetSlackEventDedupe } from "./event-ingress.js"; describe("verifyGitHubSignature", () => { const secret = "test-webhook-secret"; @@ -60,9 +64,8 @@ describe("POST /api/webhooks/github (signature enforcement)", () => { let app: import("fastify").FastifyInstance; beforeEach(async () => { - redisKeys.clear(); - redisSet.mockClear(); - resetSlackEventDedupe(); + claimed.clear(); + mockClaim.mockClear(); process.env.GITHUB_WEBHOOK_SECRET = secret; const { buildRouteTestApp } = await import("../test-utils/build-route-test-app.js"); const { ticketRoutes } = await import("./tickets.js"); @@ -109,30 +112,18 @@ describe("POST /api/webhooks/github (signature enforcement)", () => { it("drops a replayed delivery id without re-processing it", async () => { const headers = { "x-hub-signature-256": sign(raw), "x-github-delivery": "guid-1" }; expect((await deliver(headers)).statusCode).toBe(200); - expect(redisSet).toHaveBeenCalledTimes(1); - expect(redisSet.mock.calls[0]).toEqual([ - "optio:webhook-delivery:github:guid-1", - "1", - "EX", - 86400, - "NX", - ]); - // Replay: answered 200 but short-circuited by the in-process set. + expect(mockClaim).toHaveBeenCalledTimes(1); + expect(mockClaim.mock.calls[0]).toEqual(["github", "guid-1"]); + // Replay: answered 200 but the claim is a conflict. expect((await deliver(headers)).json()).toEqual({ ok: true }); - expect(redisSet).toHaveBeenCalledTimes(1); + expect(mockClaim).toHaveBeenCalledTimes(2); }); it("drops a delivery another replica (or a previous process) already claimed", async () => { - redisKeys.add("optio:webhook-delivery:github:guid-2"); + claimed.add("github:guid-2"); const res = await deliver({ "x-hub-signature-256": sign(raw), "x-github-delivery": "guid-2" }); expect(res.statusCode).toBe(200); - expect(redisSet).toHaveBeenCalledTimes(1); - }); - - it("accepts the delivery when Redis is unavailable (fails open)", async () => { - redisSet.mockRejectedValueOnce(new Error("ECONNREFUSED")); - const res = await deliver({ "x-hub-signature-256": sign(raw), "x-github-delivery": "guid-3" }); - expect(res.statusCode).toBe(200); + expect(mockClaim).toHaveBeenCalledTimes(1); }); it("rejects every delivery when GITHUB_WEBHOOK_SECRET is unset", async () => { diff --git a/apps/api/src/routes/tickets.ts b/apps/api/src/routes/tickets.ts index c4cf93bd..7a85894d 100644 --- a/apps/api/src/routes/tickets.ts +++ b/apps/api/src/routes/tickets.ts @@ -340,7 +340,8 @@ export async function ticketRoutes(rawApp: FastifyInstance) { // GitHub signs no timestamp, so a captured delivery stays validly // signed forever. Drop replays (and GitHub's own redeliveries) by the - // unique X-GitHub-Delivery id, claimed durably in Redis. + // unique X-GitHub-Delivery id, claimed in the database so every API + // instance sees the claim. const delivery = req.headers["x-github-delivery"]; if (typeof delivery === "string" && !(await claimDelivery("github", delivery))) { logger.info({ delivery }, "Ignoring already-processed GitHub delivery"); diff --git a/apps/api/src/services/claim-lock.ts b/apps/api/src/services/claim-lock.ts new file mode 100644 index 00000000..9124fe7b --- /dev/null +++ b/apps/api/src/services/claim-lock.ts @@ -0,0 +1,112 @@ +/** + * Claim locks (docs/plans/scale-out.md §3): the count-then-claim that admits + * a run under a concurrency limit — "fewer than N running, so take this + * one" — is only correct when nobody else counts at the same time. One + * process used to serialize its own claimers with a promise chain; across + * API instances that is a Postgres transaction-level advisory lock, held for + * exactly the count and the claim. + * + * `withClaimLock(keys, fn)` opens a transaction, takes + * `pg_advisory_xact_lock(hashtext(key))` for every key in sorted order (two + * claimers that need the same set of locks take them in the same order, so + * they can't deadlock), runs `fn`, and releases the locks when the + * transaction ends. Only the global keys are used — `claim:tasks` for Repo + * Task runs, `claim:workflows` for Job runs: every claim of a kind takes + * it, so a repo's (or a Job's) own limit is counted under the global lock + * too, and a per-repo key would add nothing. + * + * `fn` counts on `claim.tx` and makes its claim — a compare-and-swap state + * transition — on `claim.tx` too (`claimTransitionIn`, + * `claimWorkflowRunIn`), so the claim commits with the transaction, before + * the lock is released; the next claimer's count starts after that and, + * under READ COMMITTED (each statement sees what committed before it + * started — set explicitly, since a stricter default would make the next + * claimer count from a snapshot older than the claim it waited for), sees + * it. What follows a claim (events, webhooks, the reconciler) is + * registered with `claim.afterCommit` and runs once the transaction has + * committed; a rollback drops it. Nothing inside `fn` may use the shared + * `db` client: a claimer holds one pooled connection while it waits for + * the lock, and a holder that needed a second one could wait forever on a + * pool that the waiters have exhausted. + * + * A claimer waits for the lock at most `lock_timeout` (10 s): past that, + * `withClaimLock` resolves `null` — "couldn't claim now" — and the worker + * takes its usual re-queue-with-delay path. + */ +import { sql } from "drizzle-orm"; +import { db, type Database } from "../db/client.js"; +import { logger } from "../logger.js"; +import { isLockTimeout } from "../utils/db-errors.js"; + +/** A transaction handle `withClaimLock` hands to its callback. */ +export type ClaimTx = Parameters[0]>[0]; + +/** What `withClaimLock` hands its callback. */ +export interface ClaimContext { + /** The claim's transaction: count and claim on it, never on `db`. */ + tx: ClaimTx; + /** Runs `step` once the transaction has committed; dropped on rollback. */ + afterCommit(step: () => unknown): void; +} + +/** The global Repo Task limit (`OPTIO_MAX_CONCURRENT`); per-repo limits are counted under it. */ +export const TASK_CLAIM_KEY = "claim:tasks"; +/** The global Job run limit (`OPTIO_MAX_WORKFLOW_CONCURRENT`); per-Job limits are counted under it. */ +export const WORKFLOW_CLAIM_KEY = "claim:workflows"; + +/** How long a claimer waits for the lock before it gives up for now. */ +const CLAIM_LOCK_TIMEOUT = "10s"; + +/** + * The lock key for a limit. `hashtext` takes text of any length and hashes + * it to 32 bits; two keys that collide only serialize with each other, + * which is harmless. + */ +export function claimKey(...parts: string[]): string { + const key = parts.join(":"); + if (!key) throw new Error("Invalid claim key: empty"); + return key; +} + +/** + * Runs `fn` while holding the advisory locks for `keys`, inside one + * transaction, then the steps it registered with `afterCommit`. Returns + * what `fn` returns, or `null` when the locks weren't free within the lock + * timeout. A throw rolls the transaction back (the locks and the + * registered steps go with it). + */ +export async function withClaimLock( + keys: readonly string[], + fn: (claim: ClaimContext) => Promise, +): Promise { + const sorted = [...new Set(keys.map((k) => claimKey(k)))].sort(); + if (sorted.length === 0) throw new Error("withClaimLock needs at least one key"); + const afterCommit: Array<() => unknown> = []; + let result: T; + try { + result = await db.transaction( + async (tx) => { + await tx.execute(sql.raw(`SET LOCAL lock_timeout = '${CLAIM_LOCK_TIMEOUT}'`)); + for (const key of sorted) { + await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtext(${key}))`); + } + return fn({ tx, afterCommit: (step) => void afterCommit.push(step) }); + }, + { isolationLevel: "read committed" }, + ); + } catch (err) { + if (isLockTimeout(err)) { + logger.info({ keys: sorted }, "Claim lock busy; not claiming now"); + return null; + } + throw err; + } + for (const step of afterCommit) { + try { + await step(); + } catch (err) { + logger.warn({ err, keys: sorted }, "A claim's after-commit step failed"); + } + } + return result; +} diff --git a/apps/api/src/services/config/source.ts b/apps/api/src/services/config/source.ts index f6ab35ee..9b891c27 100644 --- a/apps/api/src/services/config/source.ts +++ b/apps/api/src/services/config/source.ts @@ -13,6 +13,7 @@ import { applyManifests, type ConfigSourceRow } from "./apply.js"; import { readManifestDirectory } from "./files.js"; import { envConfigSource, type EnvConfigSource } from "./env.js"; import { isAuthDisabled } from "../oauth/index.js"; +import { withLease } from "../lease-service.js"; export { envConfigSource, type EnvConfigSource } from "./env.js"; @@ -107,8 +108,14 @@ async function ensureSourceRow( return created ?? (await sourceRow(workspaceId)); } -// One apply at a time in this process: a tick and a "Sync now" that overlap -// would race each other's creates. +// One apply at a time across every API instance: a tick and a "Sync now" +// that overlap would race each other's creates. The apply holds the +// `config-sync:apply` lease (services/lease-service.ts); within one process +// callers are also chained, so a tick never contends with its own instance. +const APPLY_LEASE_KEY = "config-sync:apply"; +const APPLY_LEASE_TTL_MS = 5 * 60_000; +const APPLY_WAIT_MS = 60_000; + let running: Promise = Promise.resolve(); function serialized(run: () => Promise): Promise { @@ -117,66 +124,96 @@ function serialized(run: () => Promise): Promise { return next; } -/** Read the directory and apply it (or plan it). Null when config as code is off. */ +export class ConfigApplyBusyError extends Error { + constructor() { + super("Another API instance is applying the configuration directory; try again shortly"); + this.name = "ConfigApplyBusyError"; + } +} + +/** + * Runs `run` under the apply lease. A periodic tick (`wait: false`) skips + * when another instance holds it — the next tick applies the same files. A + * person's "Sync now" (`wait: true`) waits up to a minute for the lease + * and fails with `ConfigApplyBusyError` otherwise. + */ +async function underApplyLease(wait: boolean, run: () => Promise): Promise { + const result = await withLease(APPLY_LEASE_KEY, run, { + ttlMs: APPLY_LEASE_TTL_MS, + ...(wait ? { wait: { timeoutMs: APPLY_WAIT_MS, pollMs: 500 } } : {}), + }); + if (result !== null) return result; + if (wait) throw new ConfigApplyBusyError(); + logger.debug("config directory: another instance is applying; skipping this tick"); + return null; +} + +/** + * Read the directory and apply it (or plan it). Null when config as code is + * off, or when `wait` is false and another instance is applying right now. + */ export async function syncEnvSource( - opts: { dryRun?: boolean } = {}, + opts: { dryRun?: boolean; wait?: boolean } = {}, ): Promise { const source = await ensureEnvSource(); if (!source) return null; - return serialized(async () => { - const dryRun = opts.dryRun ?? false; - let result: ConfigApplyResult; - let hash: string | null = null; - let error: string | null = null; - try { - const read = await readManifestDirectory(source.path); - hash = read.hash; - result = await applyManifests({ - workspaceId: source.workspaceId, - manifests: read.manifests, - dryRun, - source, - prune: source.prune, - priorErrors: read.errors, - }); - } catch (err) { - error = err instanceof Error ? err.message : String(err); - logger.error({ err, dir: source.path }, "config directory: sync failed"); - result = { - dryRun, - source: { id: source.id, name: source.name }, - items: [], - summary: { - created: 0, - updated: 0, - reverted: 0, - unchanged: 0, - adopted: 0, - replaced: 0, - pruned: 0, - errors: 1, - }, - at: new Date().toISOString(), - }; - } - if (!dryRun) { - await db - .update(configSources) - .set({ - lastSyncAt: new Date(), - lastSyncHash: hash, - lastSyncError: error, - lastSyncResult: result, - updatedAt: new Date(), - }) - .where(eq(configSources.id, source.id)); - const s = result.summary; - if (s.created || s.updated || s.replaced || s.pruned || s.errors || error) { - logger.info({ dir: source.path, ...s, error }, "config directory applied"); + const wait = opts.wait ?? true; + return serialized(() => + underApplyLease(wait, async () => { + const dryRun = opts.dryRun ?? false; + let result: ConfigApplyResult; + let hash: string | null = null; + let error: string | null = null; + try { + const read = await readManifestDirectory(source.path); + hash = read.hash; + result = await applyManifests({ + workspaceId: source.workspaceId, + manifests: read.manifests, + dryRun, + source, + prune: source.prune, + priorErrors: read.errors, + }); + } catch (err) { + error = err instanceof Error ? err.message : String(err); + logger.error({ err, dir: source.path }, "config directory: sync failed"); + result = { + dryRun, + source: { id: source.id, name: source.name }, + items: [], + summary: { + created: 0, + updated: 0, + reverted: 0, + unchanged: 0, + adopted: 0, + replaced: 0, + pruned: 0, + errors: 1, + }, + at: new Date().toISOString(), + }; } - } - return result; - }); + if (!dryRun) { + await db + .update(configSources) + .set({ + lastSyncAt: new Date(), + lastSyncHash: hash, + lastSyncError: error, + lastSyncResult: result, + updatedAt: new Date(), + }) + .where(eq(configSources.id, source.id)); + const s = result.summary; + if (s.created || s.updated || s.replaced || s.pruned || s.errors || error) { + logger.info({ dir: source.path, ...s, error }, "config directory applied"); + } + } + return result; + }), + ); } export function toSourceView(row: ConfigSourceRow, intervalMs: number): ConfigSourceView { diff --git a/apps/api/src/services/github-app-service.ts b/apps/api/src/services/github-app-service.ts index 228aa989..476d920a 100644 --- a/apps/api/src/services/github-app-service.ts +++ b/apps/api/src/services/github-app-service.ts @@ -2,6 +2,10 @@ import type { Signer } from "./crypto/signer.js"; import { Rs256Signer, MlDsa65Signer } from "./crypto/signer.js"; import { logger } from "../logger.js"; +// A per-instance cache of the installation token (an hour long; GitHub +// mints as many as are asked for and rotates nothing), so every API +// instance fetching its own is correct — the lock only folds concurrent +// callers within this process onto one fetch. Not shared state. let cachedToken: { token: string; expiresAt: number } | null = null; let installationTokenLock: Promise | null = null; diff --git a/apps/api/src/services/github-token-service.test.ts b/apps/api/src/services/github-token-service.test.ts index 1794c75a..aaca960c 100644 --- a/apps/api/src/services/github-token-service.test.ts +++ b/apps/api/src/services/github-token-service.test.ts @@ -197,6 +197,12 @@ describe("github-app-service", () => { describe("github-token-service", () => { const mockRetrieveSecret = vi.fn(); + const mockRetrieveSecretVersioned = vi.fn(); + const mockDeleteSecretsIfUnchanged = vi.fn().mockResolvedValue(true); + const mockWithLease = vi.fn( + (_key: string, fn: (signal: AbortSignal) => Promise, _opts?: unknown) => + fn(new AbortController().signal), + ); const mockRetrieveSecretWithFallback = vi.fn(); const mockStoreSecret = vi.fn().mockResolvedValue(undefined); const mockDeleteSecret = vi.fn().mockResolvedValue(undefined); @@ -232,6 +238,8 @@ describe("github-token-service", () => { vi.doMock("./secret-service.js", () => ({ retrieveSecret: (...args: unknown[]) => mockRetrieveSecret(...args), + retrieveSecretVersioned: (...args: unknown[]) => mockRetrieveSecretVersioned(...args), + deleteSecretsIfUnchanged: (...args: unknown[]) => mockDeleteSecretsIfUnchanged(...args), retrieveSecretWithFallback: (...args: unknown[]) => mockRetrieveSecretWithFallback(...args), storeSecret: (...args: unknown[]) => mockStoreSecret(...args), deleteSecret: (...args: unknown[]) => mockDeleteSecret(...args), @@ -242,6 +250,14 @@ describe("github-token-service", () => { getInstallationToken: () => mockGetInstToken(), })); + // The refresh runs under a cross-instance lease; here it is always free. + mockWithLease.mockImplementation((_key, fn) => fn(new AbortController().signal)); + vi.doMock("./lease-service.js", () => ({ + withLease: (key: string, fn: (signal: AbortSignal) => Promise, opts?: unknown) => + mockWithLease(key, fn, opts), + LeaseLostError: class LeaseLostError extends Error {}, + })); + const mod = await import("./github-token-service.js"); getGitHubToken = mod.getGitHubToken; storeUserGitHubTokens = mod.storeUserGitHubTokens; @@ -253,6 +269,7 @@ describe("github-token-service", () => { vi.doUnmock("../db/schema.js"); vi.doUnmock("./secret-service.js"); vi.doUnmock("./github-app-service.js"); + vi.doUnmock("./lease-service.js"); process.env = { ...originalEnv }; globalThis.fetch = originalFetch; }); @@ -270,10 +287,10 @@ describe("github-token-service", () => { it("refreshes expired user token", async () => { const pastDate = new Date(Date.now() - 60 * 1000).toISOString(); - mockRetrieveSecret - .mockResolvedValueOnce("ghu_expired_token") - .mockResolvedValueOnce(pastDate) - .mockResolvedValueOnce("ghr_refresh_token"); + mockRetrieveSecret.mockImplementation(async (name: string) => + name === "GITHUB_USER_ACCESS_TOKEN" ? "ghu_expired_token" : pastDate, + ); + mockRetrieveSecretVersioned.mockResolvedValue({ value: "ghr_refresh_token", version: "v1" }); process.env.GITHUB_APP_CLIENT_ID = "client-id"; process.env.GITHUB_APP_CLIENT_SECRET = "client-secret"; @@ -293,9 +310,99 @@ describe("github-token-service", () => { expect(token).toBe("ghu_new_token"); expect(mockFetch).toHaveBeenCalledWith( "https://github.com/login/oauth/access_token", - expect.objectContaining({ method: "POST" }), + expect.objectContaining({ method: "POST", signal: expect.any(AbortSignal) }), ); expect(mockStoreSecret).toHaveBeenCalledTimes(3); + // Under the lease, waiting for another instance's refresh. + expect(mockWithLease).toHaveBeenCalledWith( + "github-refresh:user-2", + expect.any(Function), + expect.objectContaining({ wait: expect.objectContaining({ timeoutMs: expect.any(Number) }) }), + ); + }); + + it("uses the token another instance stored while it waited for the lease", async () => { + const pastDate = new Date(Date.now() - 60 * 1000).toISOString(); + const futureDate = new Date(Date.now() + 60 * 60 * 1000).toISOString(); + let refreshedElsewhere = false; + mockRetrieveSecret.mockImplementation(async (name: string) => { + if (name === "GITHUB_USER_ACCESS_TOKEN") { + return refreshedElsewhere ? "ghu_from_other_instance" : "ghu_expired"; + } + return refreshedElsewhere ? futureDate : pastDate; + }); + mockWithLease.mockImplementation(async (_key, fn) => { + refreshedElsewhere = true; // the holder refreshed before letting go + return fn(new AbortController().signal); + }); + process.env.GITHUB_APP_CLIENT_ID = "client-id"; + process.env.GITHUB_APP_CLIENT_SECRET = "client-secret"; + const mockFetch = vi.fn(); + globalThis.fetch = mockFetch; + + expect(await getGitHubToken({ userId: "user-5" })).toBe("ghu_from_other_instance"); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("never refreshes without the lease: a timed-out wait uses a still-valid stored token", async () => { + const soon = new Date(Date.now() + 2 * 60 * 1000).toISOString(); // inside the refresh buffer + mockRetrieveSecret.mockImplementation(async (name: string) => + name === "GITHUB_USER_ACCESS_TOKEN" ? "ghu_still_valid" : soon, + ); + mockWithLease.mockResolvedValue(null); // held elsewhere the whole wait + const mockFetch = vi.fn(); + globalThis.fetch = mockFetch; + + expect(await getGitHubToken({ userId: "user-6" })).toBe("ghu_still_valid"); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("falls back to PAT when the wait times out and the stored token has expired", async () => { + const pastDate = new Date(Date.now() - 60 * 1000).toISOString(); + mockRetrieveSecret.mockImplementation(async (name: string) => + name === "GITHUB_USER_ACCESS_TOKEN" ? "ghu_expired" : pastDate, + ); + mockWithLease.mockResolvedValue(null); + mockRetrieveSecretWithFallback.mockResolvedValue("ghp_pat"); + const mockFetch = vi.fn(); + globalThis.fetch = mockFetch; + + expect(await getGitHubToken({ userId: "user-7" })).toBe("ghp_pat"); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("falls back to PAT when the refresh rejects", async () => { + const pastDate = new Date(Date.now() - 60 * 1000).toISOString(); + mockRetrieveSecret.mockImplementation(async (name: string) => + name === "GITHUB_USER_ACCESS_TOKEN" ? "ghu_expired" : pastDate, + ); + mockWithLease.mockRejectedValue(new Error("database unavailable")); + mockRetrieveSecretWithFallback.mockResolvedValue("ghp_pat_after_reject"); + + expect(await getGitHubToken({ userId: "user-8" })).toBe("ghp_pat_after_reject"); + }); + + it("on bad_refresh_token deletes the tokens only if the refresh token is unchanged", async () => { + const pastDate = new Date(Date.now() - 60 * 1000).toISOString(); + mockRetrieveSecret.mockImplementation(async (name: string) => + name === "GITHUB_USER_ACCESS_TOKEN" ? "ghu_expired" : pastDate, + ); + mockRetrieveSecretVersioned.mockResolvedValue({ value: "ghr_used", version: "iv-used" }); + process.env.GITHUB_APP_CLIENT_ID = "client-id"; + process.env.GITHUB_APP_CLIENT_SECRET = "client-secret"; + globalThis.fetch = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ error: "bad_refresh_token" }), + }); + mockRetrieveSecretWithFallback.mockResolvedValue("ghp_pat"); + + expect(await getGitHubToken({ userId: "user-9" })).toBe("ghp_pat"); + expect(mockDeleteSecretsIfUnchanged).toHaveBeenCalledWith( + ["GITHUB_USER_ACCESS_TOKEN", "GITHUB_USER_REFRESH_TOKEN", "GITHUB_USER_TOKEN_EXPIRES_AT"], + "user:user-9", + { name: "GITHUB_USER_REFRESH_TOKEN", version: "iv-used" }, + ); + expect(mockDeleteSecret).not.toHaveBeenCalled(); }); it("falls back to PAT when no user tokens exist", async () => { @@ -325,10 +432,10 @@ describe("github-token-service", () => { it("falls back to PAT when refresh fails", async () => { const pastDate = new Date(Date.now() - 60 * 1000).toISOString(); - mockRetrieveSecret - .mockResolvedValueOnce("ghu_expired") - .mockResolvedValueOnce(pastDate) - .mockResolvedValueOnce("ghr_refresh"); + mockRetrieveSecret.mockImplementation(async (name: string) => + name === "GITHUB_USER_ACCESS_TOKEN" ? "ghu_expired" : pastDate, + ); + mockRetrieveSecretVersioned.mockResolvedValue({ value: "ghr_refresh", version: "v1" }); process.env.GITHUB_APP_CLIENT_ID = "client-id"; process.env.GITHUB_APP_CLIENT_SECRET = "client-secret"; @@ -347,6 +454,7 @@ describe("github-token-service", () => { // Transient failures (HTTP 401) should NOT delete tokens — only definitive // revocation errors (bad_refresh_token) trigger deletion expect(mockDeleteSecret).not.toHaveBeenCalled(); + expect(mockDeleteSecretsIfUnchanged).not.toHaveBeenCalled(); }); it("resolves task creator's token", async () => { diff --git a/apps/api/src/services/github-token-service.ts b/apps/api/src/services/github-token-service.ts index 189afe66..619f34a1 100644 --- a/apps/api/src/services/github-token-service.ts +++ b/apps/api/src/services/github-token-service.ts @@ -3,14 +3,17 @@ import { db } from "../db/client.js"; import { tasks } from "../db/schema.js"; import { retrieveSecret, + retrieveSecretVersioned, retrieveSecretWithFallback, storeSecret, deleteSecret, + deleteSecretsIfUnchanged, } from "./secret-service.js"; import { isGitHubAppConfigured, getInstallationToken } from "./github-app-service.js"; +import { LeaseLostError, withLease } from "./lease-service.js"; import { logger } from "../logger.js"; -const refreshLocks = new Map>(); +const refreshLocks = new Map>(); const TOKEN_REFRESH_BUFFER_MS = 10 * 60 * 1000; export type GitHubTokenContext = @@ -49,27 +52,37 @@ async function getTokenForTask(taskId: string): Promise { return getTokenForUser(task.createdBy, task.workspaceId); } +const ACCESS_TOKEN = "GITHUB_USER_ACCESS_TOKEN"; +const REFRESH_TOKEN = "GITHUB_USER_REFRESH_TOKEN"; +const EXPIRES_AT = "GITHUB_USER_TOKEN_EXPIRES_AT"; + async function getTokenForUser(userId: string, workspaceId?: string | null): Promise { + let refreshed: string | null = null; try { - const accessToken = await retrieveSecret("GITHUB_USER_ACCESS_TOKEN", `user:${userId}`); - const expiresAt = await retrieveSecret("GITHUB_USER_TOKEN_EXPIRES_AT", `user:${userId}`); + const accessToken = await retrieveSecret(ACCESS_TOKEN, `user:${userId}`); + const expiresAt = await retrieveSecret(EXPIRES_AT, `user:${userId}`); const expiryTime = new Date(expiresAt).getTime(); if (Date.now() < expiryTime - TOKEN_REFRESH_BUFFER_MS) { return accessToken; } - return refreshUserToken(userId, workspaceId); + // Awaited here, so a refresh that rejects lands in the catch below. + refreshed = await refreshUserToken(userId, workspaceId); } catch (err) { - logger.warn({ userId, err }, "No stored user token, falling back to PAT"); - return getPatFallback(workspaceId, userId); + logger.warn({ userId, err }, "No usable user token, falling back to PAT"); } + return refreshed ?? getPatFallback(workspaceId, userId); } -async function refreshUserToken(userId: string, workspaceId?: string | null): Promise { +/** Within one instance, concurrent refreshes of a user share one promise. */ +async function refreshUserToken( + userId: string, + workspaceId?: string | null, +): Promise { const existing = refreshLocks.get(userId); if (existing) return existing; - const refreshPromise = doRefreshUserToken(userId, workspaceId); + const refreshPromise = refreshUserTokenLeased(userId, workspaceId); refreshLocks.set(userId, refreshPromise); try { return await refreshPromise; @@ -78,17 +91,79 @@ async function refreshUserToken(userId: string, workspaceId?: string | null): Pr } } -async function doRefreshUserToken(userId: string, workspaceId?: string | null): Promise { +const REFRESH_LEASE_TTL_MS = 30_000; +const REFRESH_WAIT_MS = 30_000; +const REFRESH_FETCH_TIMEOUT_MS = 15_000; + +/** + * Refreshes a user's token under the lease `github-refresh:`, the + * only way it is ever refreshed. GitHub rotates the refresh token on use, + * so two instances refreshing one user at once would strand one of them + * with a dead refresh token. An instance that finds the lease held waits + * for it; inside, it first re-reads the stored token and returns it when it + * is fresh (another instance just refreshed). When the wait times out, or + * the lease is lost under the refresh, it returns the stored token while it + * still works, else null — the caller falls back to the PAT; it never + * refreshes without the lease. Null also when the refresh itself failed. + * + * `holder` lets a test play two instances in one process. + */ +export async function refreshUserTokenLeased( + userId: string, + workspaceId?: string | null, + holder?: string, +): Promise { + try { + const result = await withLease( + `github-refresh:${userId}`, + async (signal) => { + const fresh = await storedToken(userId, TOKEN_REFRESH_BUFFER_MS); + if (fresh) return { token: fresh }; + return { token: await doRefreshUserToken(userId, signal) }; + }, + { + ttlMs: REFRESH_LEASE_TTL_MS, + wait: { timeoutMs: REFRESH_WAIT_MS, pollMs: 250 }, + ...(holder ? { holder } : {}), + }, + ); + if (result) return result.token; + logger.warn({ userId }, "Timed out waiting for another instance's GitHub token refresh"); + } catch (err) { + if (!(err instanceof LeaseLostError)) throw err; + logger.warn({ userId }, "GitHub token refresh outlived its lease"); + } + return storedToken(userId, 0); +} + +/** The user's stored access token while it has more than `marginMs` to live, else null. */ +async function storedToken(userId: string, marginMs: number): Promise { + try { + const accessToken = await retrieveSecret(ACCESS_TOKEN, `user:${userId}`); + const expiresAt = await retrieveSecret(EXPIRES_AT, `user:${userId}`); + return Date.now() < new Date(expiresAt).getTime() - marginMs ? accessToken : null; + } catch { + return null; + } +} + +/** + * One refresh against GitHub (called under the lease). Null when it + * failed; the stored tokens are deleted only on a definitive revocation, + * and only while the refresh token is still the one this refresh used. + */ +async function doRefreshUserToken(userId: string, signal: AbortSignal): Promise { const clientId = process.env.GITHUB_APP_CLIENT_ID; const clientSecret = process.env.GITHUB_APP_CLIENT_SECRET; + const scope = `user:${userId}`; if (!clientId || !clientSecret) { await deleteUserGitHubTokens(userId); - return getPatFallback(workspaceId, userId); + return null; } try { - const refreshToken = await retrieveSecret("GITHUB_USER_REFRESH_TOKEN", `user:${userId}`); + const refresh = await retrieveSecretVersioned(REFRESH_TOKEN, scope); const res = await fetch("https://github.com/login/oauth/access_token", { method: "POST", @@ -97,8 +172,10 @@ async function doRefreshUserToken(userId: string, workspaceId?: string | null): client_id: clientId, client_secret: clientSecret, grant_type: "refresh_token", - refresh_token: refreshToken, + refresh_token: refresh.value, }), + // A hung call must not hold the lease. + signal: AbortSignal.any([signal, AbortSignal.timeout(REFRESH_FETCH_TIMEOUT_MS)]), }); if (!res.ok) throw new Error(`GitHub token refresh failed: ${res.status}`); @@ -106,10 +183,19 @@ async function doRefreshUserToken(userId: string, workspaceId?: string | null): const data = (await res.json()) as Record; if (data.error) { const errorCode = String(data.error); - // Only delete tokens on definitive revocation — not transient failures + // Only delete tokens on definitive revocation — not transient failures — + // and only if nobody stored a newer refresh token meanwhile. if (errorCode === "bad_refresh_token" || errorCode === "incorrect_client_credentials") { - logger.error({ userId, errorCode }, "GitHub token revoked, deleting stored tokens"); - await deleteUserGitHubTokens(userId); + const deleted = await deleteSecretsIfUnchanged( + [ACCESS_TOKEN, REFRESH_TOKEN, EXPIRES_AT], + scope, + { name: REFRESH_TOKEN, version: refresh.version }, + ); + if (deleted) { + logger.error({ userId, errorCode }, "GitHub token revoked, deleted stored tokens"); + } else { + logger.warn({ userId, errorCode }, "GitHub refused a refresh token already replaced"); + } } throw new Error(`GitHub token refresh error: ${errorCode}`); } @@ -126,10 +212,10 @@ async function doRefreshUserToken(userId: string, workspaceId?: string | null): return newAccessToken; } catch (err) { - // Don't delete tokens on transient errors (network, 5xx) — only the - // definitive revocation cases above delete them before re-throwing. + // Don't delete tokens on transient errors (network, 5xx, timeout) — only + // the definitive revocation cases above delete them before re-throwing. logger.warn({ userId, err }, "Token refresh failed, falling back to PAT"); - return getPatFallback(workspaceId, userId); + return null; } } diff --git a/apps/api/src/services/inbound-delivery-service.test.ts b/apps/api/src/services/inbound-delivery-service.test.ts new file mode 100644 index 00000000..475bce3e --- /dev/null +++ b/apps/api/src/services/inbound-delivery-service.test.ts @@ -0,0 +1,68 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const returning = vi.fn(); +const executed: string[] = []; +vi.mock("../db/client.js", () => { + const tx = { + execute: async (q: { queryChunks?: Array<{ value?: string[] }> }) => { + executed.push(JSON.stringify(q)); + }, + insert: () => ({ + values: () => ({ onConflictDoNothing: () => ({ returning: () => returning() }) }), + }), + }; + return { db: { transaction: (fn: (t: typeof tx) => Promise) => fn(tx) } }; +}); +vi.mock("../db/schema.js", () => ({ + inboundWebhookDeliveries: { source: "source", deliveryId: "delivery_id" }, +})); +const warn = vi.fn(); +vi.mock("../logger.js", () => ({ + logger: { info: vi.fn(), warn: (...a: unknown[]) => warn(...a), error: vi.fn(), debug: vi.fn() }, +})); + +import { claimInboundDelivery } from "./inbound-delivery-service.js"; + +describe("claimInboundDelivery", () => { + beforeEach(() => { + returning.mockReset(); + warn.mockReset(); + executed.length = 0; + }); + + it("is the first to see a delivery when the insert returns a row", async () => { + returning.mockResolvedValue([{ source: "github" }]); + expect(await claimInboundDelivery("github", "guid-1")).toBe(true); + // The insert runs under a one-second statement timeout. + expect(executed.join()).toContain("statement_timeout = '1s'"); + }); + + it("accepts the delivery when the insert times out, and says so", async () => { + returning.mockRejectedValue( + Object.assign(new Error("Failed query"), { + cause: Object.assign(new Error("canceling statement due to statement timeout"), { + code: "57014", + }), + }), + ); + expect(await claimInboundDelivery("github", "guid-2")).toBe(true); + expect(warn).toHaveBeenCalledWith( + expect.objectContaining({ source: "github" }), + expect.stringContaining("timed out"), + ); + }); + + it("is a replay when the insert conflicts (no row back)", async () => { + returning.mockResolvedValue([]); + expect(await claimInboundDelivery("github", "guid-1")).toBe(false); + }); + + it("fails open when the database is away, and says so", async () => { + returning.mockRejectedValue(new Error("ECONNREFUSED")); + expect(await claimInboundDelivery("slack", "Ev1")).toBe(true); + expect(warn).toHaveBeenCalledWith( + expect.objectContaining({ source: "slack" }), + expect.stringContaining("accepting"), + ); + }); +}); diff --git a/apps/api/src/services/inbound-delivery-service.ts b/apps/api/src/services/inbound-delivery-service.ts new file mode 100644 index 00000000..f706f719 --- /dev/null +++ b/apps/api/src/services/inbound-delivery-service.ts @@ -0,0 +1,54 @@ +/** + * Inbound webhook dedupe (docs/plans/scale-out.md §3): every provider + * retries and replays, and a delivery must fire an automation once however + * many API instances receive copies. The claim is one row in + * `inbound_webhook_deliveries` keyed by (source, the provider's delivery + * id): the insert that returns a row is the first copy; a conflict is a + * replay. Rows older than a day are swept (workers/sweep-worker.ts). + * + * Fails open: when the database is away or slow (the insert gets one + * second — `statement_timeout` — so a webhook ack is never held hostage), + * the delivery is accepted — the provider's signature still gates it, and + * losing deliveries is worse than a rare double fire. + */ +import { sql } from "drizzle-orm"; +import { inboundWebhookDeliveries } from "../db/schema.js"; +import { db } from "../db/client.js"; +import { logger } from "../logger.js"; +import { isStatementTimeout } from "../utils/db-errors.js"; + +export type DeliverySource = + | "github" + | "slack" + | "linear" + | "pagerduty" + | "gitlab" + | "jira" + | "sentry"; + +const MAX_DELIVERY_ID = 512; +/** How long the claim's insert may take before the delivery is accepted unclaimed. */ +const CLAIM_TIMEOUT = "1s"; + +/** True when this caller is the first to see the delivery. */ +export async function claimInboundDelivery(source: DeliverySource, id: string): Promise { + const deliveryId = id.length > MAX_DELIVERY_ID ? id.slice(0, MAX_DELIVERY_ID) : id; + try { + const rows = await db.transaction(async (tx) => { + await tx.execute(sql.raw(`SET LOCAL statement_timeout = '${CLAIM_TIMEOUT}'`)); + return tx + .insert(inboundWebhookDeliveries) + .values({ source, deliveryId }) + .onConflictDoNothing() + .returning({ source: inboundWebhookDeliveries.source }); + }); + return rows.length > 0; + } catch (err) { + if (isStatementTimeout(err)) { + logger.warn({ source }, "Webhook delivery dedupe timed out in the database; accepting"); + } else { + logger.warn({ err, source }, "Webhook delivery dedupe failed in the database; accepting"); + } + return true; + } +} diff --git a/apps/api/src/services/installed-skill-service.test.ts b/apps/api/src/services/installed-skill-service.test.ts index 05034d71..f96af735 100644 --- a/apps/api/src/services/installed-skill-service.test.ts +++ b/apps/api/src/services/installed-skill-service.test.ts @@ -168,6 +168,14 @@ describe("installed-skill-service", () => { let deletedFrom: unknown; let inserted: any[] = []; const tx = { + // The CAS: the row still has the ref and subpath the sync resolved. + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + for: vi.fn().mockResolvedValue([{ id: "skill-1" }]), + }), + }), + }), delete: vi.fn().mockReturnValue({ where: vi.fn().mockImplementation(async (w: unknown) => { deletedFrom = w; @@ -188,14 +196,19 @@ describe("installed-skill-service", () => { }; (db as any).transaction = vi.fn().mockImplementation(async (fn: any) => fn(tx)); - await recordSyncResult("skill-1", { - ok: true, - resolvedSha: "abc", - manifest: { files: [{ relativePath: "SKILL.md", sizeBytes: 10, executable: false }] }, - hasExecutableFiles: false, - totalSizeBytes: 10, - files: [{ relativePath: "SKILL.md", content: Buffer.from("# hi"), executable: false }], - }); + const recorded = await recordSyncResult( + "skill-1", + { + ok: true, + resolvedSha: "abc", + manifest: { files: [{ relativePath: "SKILL.md", sizeBytes: 10, executable: false }] }, + hasExecutableFiles: false, + totalSizeBytes: 10, + files: [{ relativePath: "SKILL.md", content: Buffer.from("# hi"), executable: false }], + }, + { ref: "main", subpath: "." }, + ); + expect(recorded).toBe(true); expect(deletedFrom).toBeDefined(); expect(inserted).toEqual([ { @@ -221,17 +234,54 @@ describe("installed-skill-service", () => { it("writes error message and leaves resolvedSha alone on failure", async () => { let captured: any; - (db.update as any) = vi.fn().mockReturnValue({ - set: vi.fn().mockImplementation((vals: any) => { - captured = vals; - return { where: vi.fn().mockResolvedValue(undefined) }; + const tx = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + for: vi.fn().mockResolvedValue([{ id: "skill-1" }]), + }), + }), }), - }); + update: vi.fn().mockReturnValue({ + set: vi.fn().mockImplementation((vals: any) => { + captured = vals; + return { where: vi.fn().mockResolvedValue(undefined) }; + }), + }), + }; + (db as any).transaction = vi.fn().mockImplementation(async (fn: any) => fn(tx)); - await recordSyncResult("skill-1", { ok: false, error: "clone failed" }); + await recordSyncResult( + "skill-1", + { ok: false, error: "clone failed" }, + { ref: "main", subpath: "." }, + ); expect(captured.lastSyncError).toBe("clone failed"); expect(captured.resolvedSha).toBeUndefined(); }); + + it("writes nothing when the skill's ref moved during the sync", async () => { + const tx = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ for: vi.fn().mockResolvedValue([]) }), + }), + }), + delete: vi.fn(), + insert: vi.fn(), + update: vi.fn(), + }; + (db as any).transaction = vi.fn().mockImplementation(async (fn: any) => fn(tx)); + + const recorded = await recordSyncResult( + "skill-1", + { ok: false, error: "clone failed" }, + { ref: "main", subpath: "." }, + ); + expect(recorded).toBe(false); + expect(tx.update).not.toHaveBeenCalled(); + expect(tx.delete).not.toHaveBeenCalled(); + }); }); describe("getInstalledSkillsForTask", () => { diff --git a/apps/api/src/services/installed-skill-service.ts b/apps/api/src/services/installed-skill-service.ts index 99bf2999..d7595353 100644 --- a/apps/api/src/services/installed-skill-service.ts +++ b/apps/api/src/services/installed-skill-service.ts @@ -85,8 +85,10 @@ export async function updateInstalledSkill( if (input.agentTypes !== undefined) { updates.agentTypes = input.agentTypes && input.agentTypes.length > 0 ? input.agentTypes : null; } - // Changing ref or subpath invalidates the resolved SHA — sync worker will - // refresh on the next pass. + // Changing ref or subpath invalidates the resolved SHA, in the same + // statement as the change: the skill is due (`dueSkillIds`) the moment the + // new ref is visible, and a sync of the old ref still in flight finds the + // ref moved and records nothing (`recordSyncResult`). if (input.ref !== undefined || input.subpath !== undefined) { updates.resolvedSha = null; updates.lastSyncError = null; @@ -121,7 +123,9 @@ export interface SyncedSkillFile { export const SKILL_SYNC_SENTINEL = ".optio-synced"; /** - * Records a sync: on success the row's manifest, **the files themselves** + * Records a sync — only while the skill's ref and subpath are still the + * ones the sync resolved (false, and nothing written, otherwise): on + * success the row's manifest, **the files themselves** * (`installed_skill_files`, replaced whole) and the sentinel in one * transaction, so a run spawned on any instance reads a complete set at the * recorded commit. The inserts upsert on (skill_id, path): two syncs of one @@ -140,8 +144,40 @@ export async function recordSyncResult( files: readonly SyncedSkillFile[]; } | { ok: false; error: string }, -): Promise { - if (result.ok) { + /** The ref and subpath the sync read from the row (and resolved). */ + synced: { ref: string; subpath: string }, +): Promise { + return db.transaction(async (tx) => { + // A sync is only good for the ref and subpath it resolved. If a PATCH + // moved the skill meanwhile, write nothing: the PATCH left it due, and + // the next sync-due takes the new ref (a sync-one the PATCH queued may + // have been absorbed by this very job). FOR UPDATE: a PATCH that lands + // now waits for this commit, then makes the skill due again. + const [current] = await tx + .select({ id: installedSkills.id }) + .from(installedSkills) + .where( + and( + eq(installedSkills.id, id), + eq(installedSkills.ref, synced.ref), + eq(installedSkills.subpath, synced.subpath), + ), + ) + .for("update"); + if (!current) return false; + + if (!result.ok) { + await tx + .update(installedSkills) + .set({ + lastSyncError: result.error, + lastSyncedAt: new Date(), + updatedAt: new Date(), + }) + .where(eq(installedSkills.id, id)); + return true; + } + const rows = [ ...result.files .filter((f) => f.relativePath !== SKILL_SYNC_SENTINEL) @@ -160,42 +196,32 @@ export async function recordSyncResult( resolvedSha: result.resolvedSha, }, ]; - await db.transaction(async (tx) => { - await tx.delete(installedSkillFiles).where(eq(installedSkillFiles.skillId, id)); - await tx - .insert(installedSkillFiles) - .values(rows) - .onConflictDoUpdate({ - target: [installedSkillFiles.skillId, installedSkillFiles.path], - set: { - content: sql`excluded.content`, - executable: sql`excluded.executable`, - resolvedSha: sql`excluded.resolved_sha`, - }, - }); - await tx - .update(installedSkills) - .set({ - resolvedSha: result.resolvedSha, - cachedManifest: result.manifest, - hasExecutableFiles: result.hasExecutableFiles, - totalSizeBytes: result.totalSizeBytes, - lastSyncedAt: new Date(), - lastSyncError: null, - updatedAt: new Date(), - }) - .where(eq(installedSkills.id, id)); - }); - } else { - await db + await tx.delete(installedSkillFiles).where(eq(installedSkillFiles.skillId, id)); + await tx + .insert(installedSkillFiles) + .values(rows) + .onConflictDoUpdate({ + target: [installedSkillFiles.skillId, installedSkillFiles.path], + set: { + content: sql`excluded.content`, + executable: sql`excluded.executable`, + resolvedSha: sql`excluded.resolved_sha`, + }, + }); + await tx .update(installedSkills) .set({ - lastSyncError: result.error, + resolvedSha: result.resolvedSha, + cachedManifest: result.manifest, + hasExecutableFiles: result.hasExecutableFiles, + totalSizeBytes: result.totalSizeBytes, lastSyncedAt: new Date(), + lastSyncError: null, updatedAt: new Date(), }) .where(eq(installedSkills.id, id)); - } + return true; + }); } /** diff --git a/apps/api/src/services/lease-service.int.test.ts b/apps/api/src/services/lease-service.int.test.ts index a066d65b..98f61321 100644 --- a/apps/api/src/services/lease-service.int.test.ts +++ b/apps/api/src/services/lease-service.int.test.ts @@ -119,6 +119,32 @@ describe("withLease", () => { expect(ran).toBe(false); }); + it("with wait, runs fn once the holder lets go, or resolves null when it never does", async () => { + const key = "poller:wait"; + expect(await acquireLease(key, 5_000, B)).toBe(true); + setTimeout(() => void releaseLease(key, B), 300); + const started = Date.now(); + const result = await withLease(key, async () => "ran", { + holder: A, + wait: { timeoutMs: 3_000, pollMs: 50 }, + }); + expect(result).toBe("ran"); + expect(Date.now() - started).toBeGreaterThanOrEqual(250); + + expect(await acquireLease(key, 5_000, B)).toBe(true); + let ran = false; + const timedOut = await withLease( + key, + async () => { + ran = true; + return "ran"; + }, + { holder: A, wait: { timeoutMs: 300, pollMs: 50 } }, + ); + expect(timedOut).toBeNull(); + expect(ran).toBe(false); + }); + it("aborts fn and rejects with LeaseLostError when the lease is taken away", async () => { const key = "poller:lost"; let aborted = false; diff --git a/apps/api/src/services/lease-service.ts b/apps/api/src/services/lease-service.ts index 1b1bf440..654e1dc6 100644 --- a/apps/api/src/services/lease-service.ts +++ b/apps/api/src/services/lease-service.ts @@ -105,11 +105,36 @@ export interface WithLeaseOptions { /** How often to renew while `fn` runs (default a third of the TTL). */ renewEveryMs?: number; holder?: string; + /** + * Wait for a lease held elsewhere instead of giving up at once: try again + * every `pollMs` (default 250) until `timeoutMs` has passed, then resolve + * `null` as if it had been held all along. + */ + wait?: { timeoutMs: number; pollMs?: number }; +} + +/** Takes the lease now, or — with `wait` — as soon as it is free within the timeout. */ +async function acquireWithin( + key: string, + ttlMs: number, + holder: string, + wait: WithLeaseOptions["wait"], +): Promise { + if (await acquireLease(key, ttlMs, holder)) return true; + if (!wait) return false; + const deadline = Date.now() + wait.timeoutMs; + const pollMs = Math.max(10, wait.pollMs ?? 250); + while (Date.now() < deadline) { + await new Promise((r) => setTimeout(r, Math.min(pollMs, Math.max(0, deadline - Date.now())))); + if (await acquireLease(key, ttlMs, holder)) return true; + } + return false; } /** * Runs `fn` while holding the lease, renewing it on an interval. Resolves - * `null` without running `fn` when the lease is held elsewhere. If a renew + * `null` without running `fn` when the lease is held elsewhere (after + * waiting for it, with `wait`). If a renew * fails while `fn` runs, `fn`'s `signal` aborts and, once `fn` settles, the * call rejects with `LeaseLostError`; `fn` should stop what it is doing when * the signal fires. The lease is released when `fn` finishes. @@ -122,7 +147,7 @@ export async function withLease( const ttlMs = opts.ttlMs ?? 60_000; const renewEveryMs = opts.renewEveryMs ?? Math.max(250, Math.floor(ttlMs / 3)); const holder = opts.holder ?? INSTANCE_ID; - if (!(await acquireLease(key, ttlMs, holder))) return null; + if (!(await acquireWithin(key, ttlMs, holder, opts.wait))) return null; const controller = new AbortController(); let lost = false; diff --git a/apps/api/src/services/poller-lease.ts b/apps/api/src/services/poller-lease.ts new file mode 100644 index 00000000..82b7a510 --- /dev/null +++ b/apps/api/src/services/poller-lease.ts @@ -0,0 +1,43 @@ +/** + * Pollers under a lease (docs/plans/scale-out.md §3): a periodic sweep — + * ticket sync, the schedule checker, external PR review, pod cleanup, skill + * sync, token validation, config sync — runs on one API instance at a time. + * BullMQ hands each tick to one worker, but a slow sweep on one instance + * and the next tick on another would otherwise overlap. Each sweep is also + * written to be safe when it does overlap (claims, CAS, unique keys); the + * lease keeps that the exception. + */ +import { LeaseLostError, withLease } from "./lease-service.js"; +import { logger } from "../logger.js"; + +/** How long a sweep may run before its lease lapses without a renew. */ +const POLLER_LEASE_TTL_MS = 120_000; + +/** + * Runs `fn` under `poller:`. Resolves `null` without running when + * another instance holds the lease (the tick is skipped; the next one + * tries again). A lease lost mid-sweep (the database away for longer than + * the TTL) is logged, not thrown: the sweep's own safeguards cover the + * overlap, and the tick is over anyway. + */ +export async function underPollerLease( + name: string, + fn: (signal: AbortSignal) => Promise, + /** The lease holder (this instance by default; tests play two instances). */ + holder?: string, +): Promise { + try { + const result = await withLease(`poller:${name}`, fn, { + ttlMs: POLLER_LEASE_TTL_MS, + ...(holder ? { holder } : {}), + }); + if (result === null) logger.debug({ poller: name }, "Sweep skipped: another instance holds it"); + return result; + } catch (err) { + if (err instanceof LeaseLostError) { + logger.warn({ poller: name }, "Sweep lost its lease while running"); + return null; + } + throw err; + } +} diff --git a/apps/api/src/services/pr-review-service.ts b/apps/api/src/services/pr-review-service.ts index 3c11ed13..291741a5 100644 --- a/apps/api/src/services/pr-review-service.ts +++ b/apps/api/src/services/pr-review-service.ts @@ -38,6 +38,8 @@ import { getGitPlatformForRepo } from "./git-token-service.js"; import { enqueueReconcile } from "./reconcile-queue.js"; import * as runLogService from "./run-log-service.js"; import { publishEvent } from "./event-bus.js"; +import { isUniqueViolation } from "../utils/db-errors.js"; +import { updatedAtMatches } from "../utils/pg-timestamp.js"; import { logger } from "../logger.js"; export type PrReview = typeof prReviews.$inferSelect; @@ -305,6 +307,26 @@ interface LaunchPrReviewInput { origin?: PrReviewOrigin; /** Start in waiting_ci if the repo wants CI to clear first. */ startInWaitingCi?: boolean; + /** + * The review the caller decided on: its `updatedAt`, or null when it saw + * none. The launch lands only while that is still so, so two sweeps + * (two API instances) that decide on the same view launch once; the + * loser gets `raced: true` and the review as it is. Omitted (a person's + * launch), the current review is relaunched whatever it was. + */ + seenVersion?: Date | null; +} + +/** Of two reviews of one PR, the one `findReviewByUrl` returns: active first, then newest. */ +export function preferredReview>( + a: T | undefined, + b: T, +): T { + if (!a) return b; + const activeA = ACTIVE_PR_REVIEW_STATES.includes(a.state as PrReviewState); + const activeB = ACTIVE_PR_REVIEW_STATES.includes(b.state as PrReviewState); + if (activeA !== activeB) return activeA ? a : b; + return b.updatedAt.getTime() > a.updatedAt.getTime() ? b : a; } /** @@ -365,12 +387,20 @@ export async function launchPrReview(input: LaunchPrReviewInput) { // Upsert pr_reviews row. const existing = await findReviewByUrl(input.prUrl); + if (existing && input.seenVersion === null) { + // The caller saw no review; someone launched one since. + logger.info({ prUrl: input.prUrl, reviewId: existing.id }, "PR review launched elsewhere"); + return { review: existing, raced: true as const }; + } let review: PrReview; if (existing) { // Reset to queued/reviewing with fresh head_sha. Keep origin sticky — // a manual launch on top of an auto review flips to manual so the - // auto-rereview flow no longer applies. - [review] = await db + // auto-rereview flow no longer applies. A sweep's launch is a + // compare-and-swap on the version it saw: two sweeps (two API + // instances) relaunching the same review start one run, not two. A + // person's launch (no `seenVersion`) relaunches the review as it is now. + const [reset] = await db .update(prReviews) .set({ headSha: prContext.headSha, @@ -387,8 +417,19 @@ export async function launchPrReview(input: LaunchPrReviewInput) { reconcileAttempts: 0, updatedAt: new Date(), }) - .where(eq(prReviews.id, existing.id)) + .where( + and( + eq(prReviews.id, existing.id), + input.seenVersion ? updatedAtMatches(prReviews.updatedAt, input.seenVersion) : undefined, + ), + ) .returning(); + if (!reset) { + const theirs = (await findReviewByUrl(input.prUrl)) ?? existing; + logger.info({ prUrl: input.prUrl, reviewId: theirs.id }, "PR review relaunched elsewhere"); + return { review: theirs, raced: true as const }; + } + review = reset; await db.insert(prReviewEvents).values({ prReviewId: review.id, fromState: existing.state as PrReviewState, @@ -396,21 +437,32 @@ export async function launchPrReview(input: LaunchPrReviewInput) { trigger: origin === "auto" ? "auto_relaunch" : "user_relaunch", }); } else { - [review] = await db - .insert(prReviews) - .values({ - workspaceId: input.workspaceId ?? null, - prUrl: input.prUrl, - prNumber, - repoOwner: owner, - repoName, - repoUrl, - headSha: prContext.headSha, - state: input.startInWaitingCi ? PrReviewState.WAITING_CI : PrReviewState.QUEUED, - origin, - createdBy: input.createdBy ?? null, - }) - .returning(); + try { + [review] = await db + .insert(prReviews) + .values({ + workspaceId: input.workspaceId ?? null, + prUrl: input.prUrl, + prNumber, + repoOwner: owner, + repoName, + repoUrl, + headSha: prContext.headSha, + state: input.startInWaitingCi ? PrReviewState.WAITING_CI : PrReviewState.QUEUED, + origin, + createdBy: input.createdBy ?? null, + }) + .returning(); + } catch (err) { + // Another sweep (on another API instance) launched this PR's review + // between the lookup and the insert: `pr_reviews_active_pr_url_key` + // admits one active review per PR. Theirs is the review. + if (!isUniqueViolation(err)) throw err; + const theirs = await findReviewByUrl(input.prUrl); + if (!theirs) throw err; + logger.info({ prUrl: input.prUrl, reviewId: theirs.id }, "PR review launched elsewhere"); + return { review: theirs, raced: true as const }; + } await db.insert(prReviewEvents).values({ prReviewId: review.id, toState: review.state as PrReviewState, diff --git a/apps/api/src/services/repeat-jobs.test.ts b/apps/api/src/services/repeat-jobs.test.ts new file mode 100644 index 00000000..40d8e4be --- /dev/null +++ b/apps/api/src/services/repeat-jobs.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it, vi } from "vitest"; + +vi.mock("../logger.js", () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); + +import { + REPEAT_QUEUE_NAMES, + REPEAT_SCHEDULERS, + removeLegacyRepeatables, + repeatJobName, + scheduleRepeat, +} from "./repeat-jobs.js"; + +function fakeQueue(entries: Array<{ key: string }>) { + const removed: string[] = []; + const upserts: unknown[] = []; + return { + removed, + upserts, + queue: { + getRepeatableJobs: async () => entries, + removeRepeatableByKey: async (key: string) => { + removed.push(key); + return true; + }, + upsertJobScheduler: async (...args: unknown[]) => { + upserts.push(args); + return {}; + }, + } as never, + }; +} + +describe("repeat-jobs", () => { + it("every scheduler id is `.` and names its job after the tick", () => { + for (const queue of REPEAT_QUEUE_NAMES) { + for (const id of REPEAT_SCHEDULERS[queue]) { + expect(id.startsWith(`${queue}.`)).toBe(true); + expect(repeatJobName(id)).toBe(id.slice(queue.length + 1)); + expect(repeatJobName(id)).not.toContain("."); + } + } + }); + + it("scheduleRepeat upserts a scheduler with the stable id and the tick as job name", async () => { + const { queue, upserts } = fakeQueue([]); + const before = Date.now(); + await scheduleRepeat(queue, "pr-watcher.check-prs", 30_000, { + removeOnComplete: { count: 5 }, + }); + // A new scheduler's first tick is one interval out, not at once. + const startDate = ((upserts[0] as unknown[])[1] as { startDate: number }).startDate; + expect(startDate).toBeGreaterThanOrEqual(before + 30_000); + expect(upserts).toEqual([ + [ + "pr-watcher.check-prs", + { every: 30_000, startDate: expect.any(Number) }, + { name: "check-prs", data: {}, opts: { removeOnComplete: { count: 5 } } }, + ], + ]); + }); + + it("scheduleRepeat refuses a nonsense interval", async () => { + const { queue } = fakeQueue([]); + await expect(scheduleRepeat(queue, "pr-watcher.check-prs", 0)).rejects.toThrow(/interval/); + await expect(scheduleRepeat(queue, "pr-watcher.check-prs", Number.NaN)).rejects.toThrow(); + }); + + it("removeLegacyRepeatables drops hash-keyed entries and keeps the known schedulers", async () => { + const { queue, removed } = fakeQueue([ + { key: "repo-cleanup.health-check" }, + { key: "repo-cleanup.stall-check" }, + { key: "3f1a9c0e7b2d4c5a6f8e9d0c1b2a3f4e5d6c7b8a" }, // an older build's repeatable + { key: "health-check::::60000" }, // an even older build's + ]); + const n = await removeLegacyRepeatables(queue, "repo-cleanup"); + expect(n).toBe(2); + expect(removed).toEqual(["3f1a9c0e7b2d4c5a6f8e9d0c1b2a3f4e5d6c7b8a", "health-check::::60000"]); + }); + + it("removeLegacyRepeatables never removes another queue's scheduler ids by accident", async () => { + const { queue, removed } = fakeQueue([{ key: "ticket-sync.sync" }]); + // A queue only ever holds its own entries; a foreign id would be a bug + // elsewhere, and it is still not a legacy key to this queue. + await removeLegacyRepeatables(queue, "repo-cleanup"); + expect(removed).toEqual(["ticket-sync.sync"]); + }); +}); diff --git a/apps/api/src/services/repeat-jobs.ts b/apps/api/src/services/repeat-jobs.ts new file mode 100644 index 00000000..f4d1ed4b --- /dev/null +++ b/apps/api/src/services/repeat-jobs.ts @@ -0,0 +1,103 @@ +/** + * Repeat jobs (docs/plans/scale-out.md §2): every periodic worker registers + * its tick as a BullMQ **job scheduler** with a stable id, so any number of + * API instances booting in any order leave exactly one schedule per tick. + * `upsertJobScheduler` is idempotent on the id and updates the interval + * when the deployment's env changed it. + * + * Before this, every boot removed every repeatable job of every queue and + * let the workers add theirs back — which, with more than one instance, + * wiped the others' schedules. The one piece of that left is + * `removeLegacyRepeatables`: a deployment that ran the old code has the old + * `queue.add(…, { repeat })` entries (keyed by a hash) in the same sorted + * set as the schedulers, and they would tick beside the new ones forever; + * boot removes any entry that isn't a scheduler id we know. + */ +import type { JobsOptions, Queue } from "bullmq"; +import { logger } from "../logger.js"; + +/** + * Every scheduler id, by queue. The id is `.`; a queue's + * boot-time cleanup keeps exactly these. + */ +export const REPEAT_SCHEDULERS = { + "pr-watcher": ["pr-watcher.check-prs"], + "external-pr-review": ["external-pr-review.poll-external-prs"], + "repo-cleanup": ["repo-cleanup.health-check", "repo-cleanup.stall-check"], + "ticket-sync": ["ticket-sync.sync"], + "workflow-trigger-checker": ["workflow-trigger-checker.check-workflow-triggers"], + "token-validation": ["token-validation.validate-token"], + "reconcile-resync": ["reconcile-resync.resync"], + "skill-sync": ["skill-sync.sync-due"], + "config-sync": ["config-sync.sync"], + housekeeping: ["housekeeping.sweep"], +} as const satisfies Record; + +export type RepeatQueueName = keyof typeof REPEAT_SCHEDULERS; +export type RepeatSchedulerId = (typeof REPEAT_SCHEDULERS)[RepeatQueueName][number]; + +/** The queues that have (or had, before this module) repeat jobs. */ +export const REPEAT_QUEUE_NAMES: readonly RepeatQueueName[] = Object.keys( + REPEAT_SCHEDULERS, +) as RepeatQueueName[]; + +/** The job name a scheduler's ticks carry (`.` → ``). */ +export function repeatJobName(id: RepeatSchedulerId): string { + return id.slice(id.indexOf(".") + 1); +} + +/** + * Registers (or updates) the scheduler for a tick: one job named after the + * tick every `everyMs`. Safe to call from every instance at every boot: an + * existing scheduler with the same interval keeps its next tick. A new one + * (or a changed interval) first ticks one interval from now, as the + * repeatables before it did — a scheduler with `every` would otherwise + * tick at once, beside the first passes some workers run at boot. + */ +export async function scheduleRepeat( + queue: Queue, + id: RepeatSchedulerId, + everyMs: number, + opts: Pick = {}, +): Promise { + if (!Number.isFinite(everyMs) || everyMs <= 0) { + throw new Error(`Invalid repeat interval for ${id}: ${everyMs}`); + } + try { + await queue.upsertJobScheduler( + id, + { every: everyMs, startDate: Date.now() + everyMs }, + { name: repeatJobName(id), data: {}, opts }, + ); + } catch (err) { + logger.error({ err, scheduler: id, everyMs }, "Could not register repeat job scheduler"); + throw err; + } +} + +/** + * Removes a queue's repeat entries that are not schedulers this build + * knows: the hash-keyed repeatables an older build registered. Idempotent, + * and harmless to run from every instance. + */ +export async function removeLegacyRepeatables( + queue: Queue, + queueName: RepeatQueueName, +): Promise { + const known = new Set(REPEAT_SCHEDULERS[queueName]); + const entries = await queue.getRepeatableJobs(); + let removed = 0; + for (const entry of entries) { + if (known.has(entry.key)) continue; + try { + await queue.removeRepeatableByKey(entry.key); + removed++; + } catch (err) { + logger.warn({ err, queue: queueName, key: entry.key }, "Could not remove legacy repeat job"); + } + } + if (removed > 0) { + logger.info({ queue: queueName, removed }, "Removed legacy repeat jobs"); + } + return removed; +} diff --git a/apps/api/src/services/scale-out-coordination.int.test.ts b/apps/api/src/services/scale-out-coordination.int.test.ts new file mode 100644 index 00000000..0252a4f3 --- /dev/null +++ b/apps/api/src/services/scale-out-coordination.int.test.ts @@ -0,0 +1,626 @@ +/** + * Phase C1 of docs/plans/scale-out.md against the real database and Redis: + * every place two API instances could step on each other is driven by two + * concurrent callers here, and each must settle exactly once. + * + * - the concurrency claim under the advisory lock never admits more than + * the limit, however many claimers race; + * - a poller's turn, a ticket's sync claim, an inbound webhook delivery, + * a WebSocket upgrade token and a schedule trigger's tick each go to one + * of two racing callers; + * - two overlapping schedule sweeps fire a due trigger once; two + * overlapping ticket syncs create one task per ticket; two overlapping + * external-PR sweeps launch (and relaunch) a review once; + * - a claim that can't get the lock in time gives up (null) after the + * lock timeout; after-commit steps run only after a commit; + * - a person's PR review launch relaunches the review as it is, a sweep's + * only the version it saw; + * - two instances refreshing one user's GitHub token call GitHub once, + * and a refused refresh deletes only the tokens it used; + * - repeat jobs are schedulers with stable ids: two instances registering + * leave one, and an older build's hash-keyed repeatable is removed while + * the scheduler stays. + */ +import { randomUUID } from "node:crypto"; +import { Queue } from "bullmq"; +import { afterAll, afterEach, describe, expect, it, vi } from "vitest"; +import { and, eq, sql } from "drizzle-orm"; +import { TaskState } from "@optio/shared"; +import { db } from "../db/client.js"; +import { + prReviewRuns, + prReviews, + tasks, + ticketProviders, + users, + workflowRuns, +} from "../db/schema.js"; +import { TASK_CLAIM_KEY, claimKey, withClaimLock } from "./claim-lock.js"; +import { claimInboundDelivery } from "./inbound-delivery-service.js"; +import { underPollerLease } from "./poller-lease.js"; +import { REPEAT_SCHEDULERS, removeLegacyRepeatables, scheduleRepeat } from "./repeat-jobs.js"; +import { createWsToken, validateWsToken } from "./session-service.js"; +import * as taskService from "./task-service.js"; +import { + claimTicketForSync, + recordTicketClaimTask, + releaseTicketClaim, +} from "./ticket-sync-service.js"; +import { advanceScheduleCas, getTrigger } from "./trigger-service.js"; +import { getBullMQOptions } from "./redis-config.js"; +import { getRedisClient } from "./event-bus.js"; +import { checkDueScheduleTriggers } from "../workers/workflow-trigger-worker.js"; +import { sweepExternalPrs } from "../workers/external-pr-review-worker.js"; +import { prReviewRunQueue } from "../workers/pr-review-worker.js"; +import { syncAllTickets } from "./ticket-sync-service.js"; +import { launchPrReview } from "./pr-review-service.js"; +import { refreshUserTokenLeased, storeUserGitHubTokens } from "./github-token-service.js"; +import { retrieveSecret } from "./secret-service.js"; +import { taskQueue } from "../workers/task-worker.js"; +import { workflowRunQueue } from "../workers/workflow-worker.js"; +import { reconcileQueue } from "./reconcile-queue.js"; +import { + insertRepo, + insertTask, + insertWorkflow, + insertWorkflowTrigger, +} from "../test-utils/integration/fixtures.js"; + +// The outside world the sweeps poll: one ticket provider and one git +// platform, each answering from what the test put here (and slow enough +// that two sweeps overlap for real). +const outside = vi.hoisted(() => ({ + tickets: [] as Array>, + prs: [] as Array>, +})); + +vi.mock("@optio/ticket-providers", async (importOriginal) => ({ + ...(await importOriginal()), + getTicketProvider: () => ({ + fetchActionableTickets: async () => { + await new Promise((r) => setTimeout(r, 20)); + return outside.tickets; + }, + fetchTicketComments: async () => [], + addComment: async () => {}, + }), +})); + +vi.mock("./git-token-service.js", async (importOriginal) => ({ + ...(await importOriginal()), + getGitPlatformForRepo: async () => ({ + ri: { platform: "github", host: "github.com", owner: "it-org", repo: "it-repo" }, + platform: { + listOpenPullRequests: async () => { + await new Promise((r) => setTimeout(r, 20)); + return outside.prs; + }, + getPullRequest: async (_ri: unknown, n: number) => { + const pr = outside.prs.find((p) => p.number === n)!; + return { title: pr.title, body: "", headSha: pr.headSha }; + }, + getReviews: async () => [], + getIssueComments: async () => [], + getInlineComments: async () => [], + getCIChecks: async () => [], + }, + }), +})); + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +afterEach(async () => { + await db.execute(sql`DELETE FROM "leases"`); +}); + +afterAll(async () => { + await Promise.allSettled([ + taskQueue.close(), + workflowRunQueue.close(), + prReviewRunQueue.close(), + reconcileQueue.close(), + getRedisClient().quit(), + ]); +}); + +describe("claims under the advisory lock", () => { + it("two racing claimers never admit more than the limit over 50 iterations", async () => { + // A 400-character repo URL: the repo's limit is counted under the global key. + const repoUrl = `https://github.com/it-org/claims-${randomUUID().slice(0, 8)}-${"x".repeat(360)}`; + expect(repoUrl.length).toBeGreaterThanOrEqual(400); + const limit = 3; + const iterations = 50; + const queued = await Promise.all( + Array.from({ length: iterations * 2 }, () => + insertTask({ repoUrl, state: TaskState.QUEUED }), + ), + ); + const pending = queued.map((t) => t.id); + const active = sql`${tasks.repoUrl} = ${repoUrl} AND ${tasks.state} IN ('provisioning', 'running')`; + const activeCount = async () => { + const [{ count }] = await db + .select({ count: sql`count(*)::int` }) + .from(tasks) + .where(active); + return Number(count); + }; + + // Watch the count from outside the lock the whole time. + let watching = true; + let maxSeen = 0; + const watcher = (async () => { + while (watching) { + maxSeen = Math.max(maxSeen, await activeCount()); + await sleep(1); + } + })(); + + let claimed = 0; + const claimer = async () => { + const mine: string[] = []; + for (let i = 0; i < iterations; i++) { + const id = pending.pop()!; + const claim = await withClaimLock([TASK_CLAIM_KEY], async (c) => { + const [{ count }] = await c.tx + .select({ count: sql`count(*)::int` }) + .from(tasks) + .where(active); + if (Number(count) >= limit) return null; + return taskService.claimTransitionIn(c, id, TaskState.PROVISIONING, "it_claim"); + }); + if (claim) { + claimed++; + mine.push(id); + } + maxSeen = Math.max(maxSeen, await activeCount()); + // Runs finish at random, so the count goes up and down. + if (mine.length > 0 && Math.random() < 0.5) { + await sleep(Math.floor(Math.random() * 5)); + await db + .update(tasks) + .set({ state: TaskState.FAILED }) + .where(eq(tasks.id, mine.shift()!)); + } + } + }; + await Promise.all([claimer(), claimer()]); + watching = false; + await watcher; + + expect(maxSeen).toBeLessThanOrEqual(limit); + expect(maxSeen).toBe(limit); // the limit was reached, and held + expect(claimed).toBeGreaterThan(limit); // runs came and went + expect(await activeCount()).toBeLessThanOrEqual(limit); + }, 60_000); + + it("runs after-commit steps once the claim commits, and drops them on rollback", async () => { + const task = await insertTask({ state: TaskState.QUEUED }); + const seen: string[] = []; + const claimed = await withClaimLock([TASK_CLAIM_KEY], async (c) => { + c.afterCommit(async () => { + // Committed by now: another connection sees the claim. + const [row] = await db.select().from(tasks).where(eq(tasks.id, task.id)); + seen.push(row.state); + }); + return taskService.claimTransitionIn(c, task.id, TaskState.PROVISIONING, "it_claim"); + }); + expect(claimed?.state).toBe(TaskState.PROVISIONING); + expect(seen).toEqual([TaskState.PROVISIONING]); + + const other = await insertTask({ state: TaskState.QUEUED }); + let ran = false; + await expect( + withClaimLock([TASK_CLAIM_KEY], async (c) => { + await taskService.claimTransitionIn(c, other.id, TaskState.PROVISIONING, "it_claim"); + c.afterCommit(() => { + ran = true; + }); + throw new Error("rolled back"); + }), + ).rejects.toThrow("rolled back"); + expect(ran).toBe(false); + const [still] = await db.select().from(tasks).where(eq(tasks.id, other.id)); + expect(still.state).toBe(TaskState.QUEUED); + }); + + it("takes a key of any length (hashtext)", async () => { + const key = claimKey("claim:repo", `https://github.com/acme/${"r".repeat(400)}`); + expect(await withClaimLock([key], async () => "ok")).toBe("ok"); + }); + + it("gives up after the lock timeout while a holder never commits", async () => { + let release!: () => void; + const held = new Promise((r) => (release = r)); + let taken!: () => void; + const lockTaken = new Promise((r) => (taken = r)); + const holder = db.transaction(async (tx) => { + await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtext(${TASK_CLAIM_KEY}))`); + taken(); + await held; + }); + await lockTaken; + const started = Date.now(); + let ran = false; + try { + // What a worker does: a null claim takes the re-queue-with-delay path. + const claimed = await withClaimLock([TASK_CLAIM_KEY], async () => { + ran = true; + return "claimed"; + }); + const waited = Date.now() - started; + expect(claimed).toBeNull(); + expect(ran).toBe(false); + expect(waited).toBeGreaterThanOrEqual(9_500); + expect(waited).toBeLessThan(15_000); + } finally { + release(); + await holder; + } + // The lock is free again: the next claimer gets it at once. + expect(await withClaimLock([TASK_CLAIM_KEY], async () => "claimed")).toBe("claimed"); + }, 30_000); + + it("two claimers that need the same two keys take them in one order (no deadlock)", async () => { + const a = withClaimLock(["claim:x", "claim:y"], async () => { + await sleep(50); + return "a"; + }); + const b = withClaimLock(["claim:y", "claim:x"], async () => { + await sleep(50); + return "b"; + }); + expect(await Promise.all([a, b])).toEqual(["a", "b"]); + }); +}); + +describe("one of two callers", () => { + it("a poller sweep runs on one instance while the other skips", async () => { + let runs = 0; + const sweep = async () => { + runs++; + await sleep(200); + return "ran"; + }; + // Two instances (one process here, so two explicit holders: the same + // holder re-acquiring is an extension, and BullMQ's concurrency of 1 + // keeps one instance from overlapping itself). + const [a, b] = await Promise.all([ + underPollerLease("it-poller", sweep, "api-a:1"), + underPollerLease("it-poller", sweep, "api-b:2"), + ]); + expect([a, b].filter((r) => r === "ran")).toHaveLength(1); + expect([a, b].filter((r) => r === null)).toHaveLength(1); + expect(runs).toBe(1); + // Released: the next tick runs, on either instance. + expect(await underPollerLease("it-poller", sweep, "api-b:2")).toBe("ran"); + }); + + it("a ticket's sync claim lands once; a task-less claim can be released, a recorded one cannot until its task is gone", async () => { + const id = `ISSUE-${randomUUID().slice(0, 8)}`; + const repo = "https://github.com/it-org/tickets"; + const [a, b] = await Promise.all([ + claimTicketForSync("github", id, repo), + claimTicketForSync("github", id, repo), + ]); + expect([a, b].filter(Boolean)).toHaveLength(1); + expect(await claimTicketForSync("github", id, repo)).toBe(false); + + await releaseTicketClaim("github", id, repo); + expect(await claimTicketForSync("github", id, repo)).toBe(true); + const task = await insertTask({ repoUrl: repo }); + await recordTicketClaimTask("github", id, repo, task.id); + await releaseTicketClaim("github", id, repo); // has a task: stays + expect(await claimTicketForSync("github", id, repo)).toBe(false); + + // The task was deleted: the ticket may get a new one, once. + await db.delete(tasks).where(eq(tasks.id, task.id)); + const [c, d] = await Promise.all([ + claimTicketForSync("github", id, repo), + claimTicketForSync("github", id, repo), + ]); + expect([c, d].filter(Boolean)).toHaveLength(1); + + // A sweep that died between its claim and its task: taken over after ten minutes. + await db.execute(sql` + UPDATE "ticket_sync_claims" SET "created_at" = now() - interval '11 minutes' + WHERE "ticket_external_id" = ${id}`); + expect(await claimTicketForSync("github", id, repo)).toBe(true); + }); + + it("an inbound webhook delivery is claimed by one of two receivers", async () => { + const delivery = randomUUID(); + const [a, b] = await Promise.all([ + claimInboundDelivery("github", delivery), + claimInboundDelivery("github", delivery), + ]); + expect([a, b].filter(Boolean)).toHaveLength(1); + expect(await claimInboundDelivery("gitlab", delivery)).toBe(true); // another source + }); + + it("a WebSocket upgrade token minted anywhere is consumed by exactly one of two upgrades", async () => { + const [user] = await db + .insert(users) + .values({ + provider: "github", + externalId: `ws-${randomUUID()}`, + email: `ws-${randomUUID().slice(0, 8)}@it.test`, + displayName: "WS user", + }) + .returning(); + const token = await createWsToken(user.id); + const [a, b] = await Promise.all([validateWsToken(token), validateWsToken(token)]); + const winners = [a, b].filter((u) => u !== null); + expect(winners).toHaveLength(1); + expect(winners[0]!.id).toBe(user.id); + expect(await validateWsToken(token)).toBeNull(); + + // Expiry is the database's clock: an expired row is refused (and spent). + const late = await createWsToken(user.id); + await db.execute( + sql`UPDATE "ws_upgrade_tokens" SET "expires_at" = now() - interval '1 second'`, + ); + expect(await validateWsToken(late)).toBeNull(); + const [{ left }] = await db.execute<{ left: number }>( + sql`SELECT count(*)::int AS "left" FROM "ws_upgrade_tokens"`, + ); + expect(Number(left)).toBe(0); + }); + + it("a due schedule trigger is advanced by one of two sweeps, and fires once", async () => { + const workflow = await insertWorkflow(); + const seen = new Date(Date.now() - 60_000); + const trigger = await insertWorkflowTrigger(workflow.id, { + workflowId: workflow.id, + targetType: "job", + type: "schedule", + config: { cronExpression: "0 0 1 1 *" }, + enabled: true, + nextFireAt: seen, + }); + const [a, b] = await Promise.all([ + advanceScheduleCas(trigger.id, "0 0 1 1 *", seen), + advanceScheduleCas(trigger.id, "0 0 1 1 *", seen), + ]); + expect([a, b].filter(Boolean)).toHaveLength(1); + const after = await getTrigger(trigger.id); + expect(after!.nextFireAt!.getTime()).toBeGreaterThan(Date.now()); + expect(after!.lastFiredAt).not.toBeNull(); + + // Two whole sweeps overlapping on another due trigger: one run. + const second = await insertWorkflowTrigger(workflow.id, { + workflowId: workflow.id, + targetType: "job", + type: "schedule", + config: { cronExpression: "0 0 1 1 *" }, + enabled: true, + nextFireAt: seen, + }); + const fired = await Promise.all([checkDueScheduleTriggers(), checkDueScheduleTriggers()]); + expect(fired[0] + fired[1]).toBe(1); + const runs = await db.select().from(workflowRuns).where(eq(workflowRuns.triggerId, second.id)); + expect(runs).toHaveLength(1); + }); +}); + +describe("two overlapping sweeps", () => { + it("ticket sync creates one task per ticket", async () => { + const repo = await insertRepo(); + await db.insert(ticketProviders).values({ + source: "linear", + config: { repoUrl: repo.repoUrl }, + enabled: true, + initialSyncAt: new Date(Date.now() - 60_000), + }); + const externalId = `LIN-${randomUUID().slice(0, 6)}`; + outside.tickets = [ + { + externalId, + source: "linear", + title: "Fix the flaky test", + body: "It flakes.", + url: `https://linear.app/it/issue/${externalId}`, + labels: ["optio"], + metadata: {}, + }, + ]; + try { + await Promise.all([syncAllTickets(), syncAllTickets()]); + // And a third sweep later sees the task (or the claim) and skips. + await syncAllTickets(); + } finally { + outside.tickets = []; + } + const created = await db + .select({ id: tasks.id }) + .from(tasks) + .where(and(eq(tasks.ticketSource, "linear"), eq(tasks.ticketExternalId, externalId))); + expect(created).toHaveLength(1); + }); + + it("external PR review launches a new PR's review once, and relaunches it once", async () => { + const repo = await insertRepo({ externalReviewMode: "on_pr_post", reviewEnabled: true }); + const pr = { + url: `${repo.repoUrl}/pull/7`, + number: 7, + title: "Add the thing", + headSha: "aaaa1111", + draft: false, + author: "alice", + labels: [], + }; + outside.prs = [pr]; + const reviewsOfPr = () => db.select().from(prReviews).where(eq(prReviews.prUrl, pr.url)); + const runsOf = (id: string) => + db.select().from(prReviewRuns).where(eq(prReviewRuns.prReviewId, id)); + try { + await Promise.all([sweepExternalPrs(), sweepExternalPrs()]); + const [review, ...more] = await reviewsOfPr(); + expect(more).toHaveLength(0); + expect(await runsOf(review.id)).toHaveLength(1); + + // The review finished; new commits land; two sweeps see them. + await db + .update(prReviews) + .set({ state: "ready", updatedAt: new Date() }) + .where(eq(prReviews.id, review.id)); + pr.headSha = "bbbb2222"; + await Promise.all([sweepExternalPrs(), sweepExternalPrs()]); + expect(await reviewsOfPr()).toHaveLength(1); + expect(await runsOf(review.id)).toHaveLength(2); + } finally { + outside.prs = []; + } + }); +}); + +describe("PR review launches", () => { + it("a person's launch relaunches the current review; a sweep's stale view does not", async () => { + const repo = await insertRepo({ externalReviewMode: "on_request", reviewEnabled: true }); + const pr = { + url: `${repo.repoUrl}/pull/8`, + number: 8, + title: "Another thing", + headSha: "cccc3333", + draft: false, + author: "bob", + labels: [], + }; + outside.prs = [pr]; + const runsOf = (id: string) => + db.select().from(prReviewRuns).where(eq(prReviewRuns.prReviewId, id)); + try { + const first = await launchPrReview({ prUrl: pr.url }); + const id = first.review.id; + expect(await runsOf(id)).toHaveLength(1); + + // A sweep that decided on an older version of the review loses. + const stale = await launchPrReview({ + prUrl: pr.url, + origin: "auto", + seenVersion: new Date(Date.now() - 60 * 60_000), + }); + expect(stale).toMatchObject({ raced: true }); + expect(await runsOf(id)).toHaveLength(1); + + // A person's launch takes the review as it is now, whatever changed. + await db + .update(prReviews) + .set({ state: "ready", updatedAt: new Date() }) + .where(eq(prReviews.id, id)); + const manual = await launchPrReview({ prUrl: pr.url }); + expect(manual).not.toHaveProperty("raced"); + expect(manual.review.id).toBe(id); + expect(await runsOf(id)).toHaveLength(2); + } finally { + outside.prs = []; + } + }); +}); + +describe("GitHub user token refresh across instances", () => { + const ENV = ["GITHUB_APP_CLIENT_ID", "GITHUB_APP_CLIENT_SECRET"] as const; + const saved = Object.fromEntries(ENV.map((k) => [k, process.env[k]])); + afterEach(() => { + vi.unstubAllGlobals(); + for (const k of ENV) { + if (saved[k] === undefined) delete process.env[k]; + else process.env[k] = saved[k]; + } + }); + + const tokenResponse = (body: Record) => + new Response(JSON.stringify(body), { + status: 200, + headers: { "content-type": "application/json" }, + }); + + async function userWithTokenDueForRefresh(refreshToken: string): Promise { + const userId = randomUUID(); + // Expires in a minute: inside the refresh buffer, so it is refreshed. + await storeUserGitHubTokens(userId, { + accessToken: "ghu_old", + refreshToken, + expiresIn: 60, + }); + process.env.GITHUB_APP_CLIENT_ID = "it-client"; + process.env.GITHUB_APP_CLIENT_SECRET = "it-secret"; + return userId; + } + + it("two instances refreshing one user call GitHub once and keep the new tokens", async () => { + const userId = await userWithTokenDueForRefresh("ghr_old"); + const sent: string[] = []; + vi.stubGlobal("fetch", async (_url: string, init: RequestInit) => { + sent.push(JSON.parse(String(init.body)).refresh_token); + await sleep(300); + return tokenResponse({ + access_token: "ghu_new", + refresh_token: "ghr_new", + expires_in: 28800, + }); + }); + + const [a, b] = await Promise.all([ + refreshUserTokenLeased(userId, null, "api-a:1"), + refreshUserTokenLeased(userId, null, "api-b:2"), + ]); + expect(sent).toEqual(["ghr_old"]); + expect([a, b]).toEqual(["ghu_new", "ghu_new"]); + expect(await retrieveSecret("GITHUB_USER_REFRESH_TOKEN", `user:${userId}`)).toBe("ghr_new"); + expect(await retrieveSecret("GITHUB_USER_ACCESS_TOKEN", `user:${userId}`)).toBe("ghu_new"); + }); + + it("bad_refresh_token deletes the tokens only while the refresh token is the one used", async () => { + // Refused, and nobody stored a newer one: the tokens go. + const gone = await userWithTokenDueForRefresh("ghr_revoked"); + vi.stubGlobal("fetch", async () => tokenResponse({ error: "bad_refresh_token" })); + expect(await refreshUserTokenLeased(gone, null, "api-a:1")).toBeNull(); + await expect(retrieveSecret("GITHUB_USER_REFRESH_TOKEN", `user:${gone}`)).rejects.toThrow( + /not found/, + ); + + // Refused, but a newer token landed meanwhile (a refresh that outlived + // its lease, then another instance's): the newer tokens stay. + const kept = await userWithTokenDueForRefresh("ghr_stale"); + vi.stubGlobal("fetch", async () => { + await storeUserGitHubTokens(kept, { + accessToken: "ghu_rotated", + refreshToken: "ghr_rotated", + expiresIn: 28800, + }); + return tokenResponse({ error: "bad_refresh_token" }); + }); + expect(await refreshUserTokenLeased(kept, null, "api-a:1")).toBeNull(); + expect(await retrieveSecret("GITHUB_USER_REFRESH_TOKEN", `user:${kept}`)).toBe("ghr_rotated"); + }); +}); + +describe("repeat jobs are schedulers", () => { + it("two instances registering leave one scheduler; an older build's repeatable is removed", async () => { + const name = `it-repeat-${randomUUID().slice(0, 8)}`; + const queue = new Queue(name, { ...getBullMQOptions() }); + try { + // The queue name is not in the registry; the ids are, and the registry + // is only consulted by name on cleanup — use repo-cleanup's ids here. + const [idA, idB] = REPEAT_SCHEDULERS["repo-cleanup"]; + await Promise.all([ + scheduleRepeat(queue, idA, 60_000), + scheduleRepeat(queue, idA, 60_000), + scheduleRepeat(queue, idB, 30_000), + ]); + await scheduleRepeat(queue, idA, 60_000); // a third boot + expect((await queue.getJobSchedulers()).map((s) => s.key).sort()).toEqual([idA, idB].sort()); + + // What the previous build registered. + await queue.add("health-check", {}, { repeat: { every: 60_000 } }); + expect(await queue.getRepeatableJobs()).toHaveLength(3); + + const removed = await removeLegacyRepeatables(queue, "repo-cleanup"); + expect(removed).toBe(1); + expect((await queue.getRepeatableJobs()).map((s) => s.key).sort()).toEqual([idA, idB].sort()); + } finally { + await queue.obliterate({ force: true }).catch(() => {}); + await queue.close(); + } + }); +}); diff --git a/apps/api/src/services/secret-service.ts b/apps/api/src/services/secret-service.ts index fa08970f..24a91d2f 100644 --- a/apps/api/src/services/secret-service.ts +++ b/apps/api/src/services/secret-service.ts @@ -363,6 +363,66 @@ export async function listVisibleSecrets(actor: Actor, scope?: string): Promise< }); } +/** + * A deployment-level secret (no workspace, no user — e.g. the `user:` + * scoped GitHub tokens) with its version: the row's IV, new at every store. + * For a caller that must later act only while the value is still the one it + * read (`deleteSecretsIfUnchanged`). + */ +export async function retrieveSecretVersioned( + name: string, + scope: string, +): Promise<{ value: string; version: string }> { + const [secret] = await db + .select() + .from(secrets) + .where( + and( + eq(secrets.name, name), + eq(secrets.scope, scope), + isNull(secrets.workspaceId), + isNull(secrets.userId), + ), + ); + if (!secret) throw new Error(`Secret not found: ${name} (scope: ${scope})`); + const value = decrypt( + { + alg: secret.alg ?? ALG_AES_256_GCM_V1, + iv: secret.iv, + ciphertext: secret.encryptedValue, + authTag: secret.authTag, + }, + buildSecretAAD(name, scope, null), + name, + ); + return { value, version: Buffer.from(secret.iv).toString("hex") }; +} + +/** + * Deletes the deployment-level secrets `names` in `scope` — in one + * statement, and only while `guard.name` still holds the version read with + * `retrieveSecretVersioned`. True iff they were deleted. + */ +export async function deleteSecretsIfUnchanged( + names: string[], + scope: string, + guard: { name: string; version: string }, +): Promise { + const rows = await db.execute<{ id: string }>(sql` + DELETE FROM "secrets" + WHERE "scope" = ${scope} AND "workspace_id" IS NULL AND "user_id" IS NULL + AND "name" IN (${sql.join( + names.map((n) => sql`${n}`), + sql`, `, + )}) + AND EXISTS ( + SELECT 1 FROM "secrets" g + WHERE g."scope" = ${scope} AND g."workspace_id" IS NULL AND g."user_id" IS NULL + AND g."name" = ${guard.name} AND encode(g."iv", 'hex') = ${guard.version}) + RETURNING "id"`); + return rows.length > 0; +} + export async function deleteSecret( name: string, scope = "global", diff --git a/apps/api/src/services/session-service.test.ts b/apps/api/src/services/session-service.test.ts index 9b91d0e0..00345f14 100644 --- a/apps/api/src/services/session-service.test.ts +++ b/apps/api/src/services/session-service.test.ts @@ -11,6 +11,12 @@ vi.mock("../db/client.js", () => ({ })); vi.mock("../db/schema.js", () => ({ + wsUpgradeTokens: { + tokenHash: "ws_upgrade_tokens.token_hash", + userId: "ws_upgrade_tokens.user_id", + workspaceId: "ws_upgrade_tokens.workspace_id", + expiresAt: "ws_upgrade_tokens.expires_at", + }, users: { id: "users.id", provider: "users.provider", @@ -43,13 +49,41 @@ import { createWsToken, validateWsToken, cleanupExpiredSessions, - _wsTokenStoreForTesting, } from "./session-service.js"; +/** + * The WS upgrade token table, as the mocked db sees it: `insert().values()` + * stores a row; `delete().where().returning()` consumes it (the hash is the + * only thing the where clause carries, so the stand-in keys rows by the + * hash it finds in the drizzle `eq`). + */ +// Expiry is computed and compared on the database's clock (`now()`), so the +// stand-in keeps only whether the row is still live. +const wsTokenRows = new Map(); +function mockWsTokenTable() { + (db.insert as any) = vi.fn().mockReturnValue({ + values: vi.fn().mockImplementation(async (row: any) => { + wsTokenRows.set(row.tokenHash, { userId: row.userId, live: true }); + }), + }); + (db.delete as any) = vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation((cond: any) => ({ + returning: vi.fn().mockImplementation(async () => { + // drizzle's eq() keeps the right-hand value in its query chunks. + const hash = JSON.stringify(cond).match(/[0-9a-f]{64}/)?.[0]; + const row = hash ? wsTokenRows.get(hash) : undefined; + if (!row) return []; + wsTokenRows.delete(hash!); + return [row]; + }), + })), + }); +} + describe("session-service", () => { beforeEach(() => { vi.clearAllMocks(); - _wsTokenStoreForTesting.clear(); + wsTokenRows.clear(); }); describe("createSession", () => { @@ -280,18 +314,21 @@ describe("session-service", () => { }); describe("createWsToken", () => { - it("creates a short-lived WebSocket upgrade token in memory", async () => { + it("stores a short-lived WebSocket upgrade token, hashed, in the database", async () => { + mockWsTokenTable(); const token = await createWsToken("user-1"); expect(token).toBeDefined(); expect(token.length).toBe(64); // 32 bytes hex - // Token should be stored in the in-memory map, not in the DB - expect(_wsTokenStoreForTesting.size).toBe(1); - expect(db.insert).not.toHaveBeenCalled(); + // Stored by hash (any instance can consume it); the raw token never lands. + expect(wsTokenRows.size).toBe(1); + expect(wsTokenRows.has(testHashToken(token))).toBe(true); + expect(wsTokenRows.has(token)).toBe(false); }); }); describe("validateWsToken", () => { it("validates and consumes a valid upgrade token", async () => { + mockWsTokenTable(); // Mock the user lookup that happens after token validation (db.select as any) = vi.fn().mockReturnValue({ from: vi.fn().mockReturnValue({ @@ -311,18 +348,19 @@ describe("session-service", () => { }); const token = await createWsToken("user-1"); - expect(_wsTokenStoreForTesting.size).toBe(1); + expect(wsTokenRows.size).toBe(1); const user = await validateWsToken(token); expect(user).not.toBeNull(); expect(user!.id).toBe("user-1"); expect(user!.email).toBe("test@test.com"); - // Token should be consumed (deleted from map) - expect(_wsTokenStoreForTesting.size).toBe(0); + // Token should be consumed (deleted from the table) + expect(wsTokenRows.size).toBe(0); }); it("rejects a token that has already been consumed (single-use)", async () => { + mockWsTokenTable(); (db.select as any) = vi.fn().mockReturnValue({ from: vi.fn().mockReturnValue({ where: vi.fn().mockReturnValue({ @@ -352,22 +390,24 @@ describe("session-service", () => { }); it("rejects an unknown token", async () => { + mockWsTokenTable(); const user = await validateWsToken("nonexistent-token"); expect(user).toBeNull(); }); it("rejects an expired token", async () => { + mockWsTokenTable(); const token = await createWsToken("user-1"); - // Manually expire the token in the store - for (const [, entry] of _wsTokenStoreForTesting) { - entry.expiresAt = Date.now() - 1000; + // Manually expire the token in the table + for (const [, entry] of wsTokenRows) { + entry.live = false; } const user = await validateWsToken(token); expect(user).toBeNull(); - // Token should still be removed from the map even though expired - expect(_wsTokenStoreForTesting.size).toBe(0); + // Token should still be removed from the table even though expired + expect(wsTokenRows.size).toBe(0); }); }); diff --git a/apps/api/src/services/session-service.ts b/apps/api/src/services/session-service.ts index 9707ade5..6ad1e31a 100644 --- a/apps/api/src/services/session-service.ts +++ b/apps/api/src/services/session-service.ts @@ -1,7 +1,7 @@ import { randomBytes, createHash, timingSafeEqual } from "node:crypto"; import { db } from "../db/client.js"; -import { users, sessions } from "../db/schema.js"; -import { eq, and, lt } from "drizzle-orm"; +import { users, sessions, wsUpgradeTokens } from "../db/schema.js"; +import { eq, and, lt, sql } from "drizzle-orm"; import type { OAuthUser } from "./oauth/provider.js"; const SESSION_MAX_TTL_MS = 30 * 24 * 60 * 60 * 1000; // 30 days absolute max @@ -160,77 +160,52 @@ export async function revokeAllUserSessions(userId: string): Promise { await db.delete(sessions).where(eq(sessions.userId, userId)); } -// ── In-memory store for single-use WebSocket upgrade tokens ────────────── +// ── Single-use WebSocket upgrade tokens (ws_upgrade_tokens) ─────────────── -const WS_TOKEN_TTL_MS = 30_000; // 30 seconds — just enough for the WS upgrade -const WS_TOKEN_CLEANUP_INTERVAL_MS = 60_000; +const WS_TOKEN_TTL_SECONDS = 30; // just enough for the WS upgrade -interface WsUpgradeEntry { - userId: string; - expiresAt: number; // epoch ms -} - -/** Map from token-hash → { userId, expiresAt }. Tokens are deleted on first use. */ -const wsUpgradeTokens = new Map(); - -/** Periodic cleanup of expired entries (prevents slow leak if tokens are never used). */ -const _wsCleanupTimer = setInterval(() => { - const now = Date.now(); - for (const [hash, entry] of wsUpgradeTokens) { - if (entry.expiresAt <= now) wsUpgradeTokens.delete(hash); - } -}, WS_TOKEN_CLEANUP_INTERVAL_MS); -// Allow the process to exit even if the timer is still running. -if (_wsCleanupTimer.unref) _wsCleanupTimer.unref(); - -/** Create a short-lived, single-use token for WebSocket authentication. */ +/** + * Create a short-lived, single-use token for WebSocket authentication. + * + * The browser fetches it from whichever API instance answers + * `/api/auth/ws-token` and upgrades `/ws/*` through the ingress to any + * instance, so the token lives in `ws_upgrade_tokens` (hashed; the raw + * token is never stored), not in process memory. Expired rows are swept by + * the housekeeping worker. + */ export async function createWsToken(userId: string): Promise { const token = randomBytes(32).toString("hex"); - const tokenHash = hashToken(token); - - wsUpgradeTokens.set(tokenHash, { + // Expiry on the database's clock, as validation compares it: API pods' + // clocks may disagree by more than the token lives. + await db.insert(wsUpgradeTokens).values({ + tokenHash: hashToken(token), userId, - expiresAt: Date.now() + WS_TOKEN_TTL_MS, + expiresAt: sql`now() + make_interval(secs => ${WS_TOKEN_TTL_SECONDS})`, }); - return token; } -/** - * Find a WS upgrade token entry using timing-safe comparison. - * - * Iterates all entries and compares hashes with crypto.timingSafeEqual - * to prevent timing side-channel attacks. Returns the matching key and - * entry, or null if not found. - */ -function findWsTokenEntry(tokenHash: string): { key: string; entry: WsUpgradeEntry } | null { - const targetBuf = Buffer.from(tokenHash, "hex"); - for (const [key, entry] of wsUpgradeTokens) { - const candidateBuf = Buffer.from(key, "hex"); - if (candidateBuf.length === targetBuf.length && timingSafeEqual(candidateBuf, targetBuf)) { - return { key, entry }; - } - } - return null; -} - /** * Validate and consume a single-use WebSocket upgrade token. * Returns the SessionUser on success (token is deleted), or null if * the token is invalid, expired, or already consumed. * - * Uses timing-safe comparison and atomic delete-then-validate semantics. + * Consuming is one `DELETE … RETURNING` on the hash: exactly one of two + * concurrent upgrades with the same token (on any instances) gets the row. + * The lookup is an equality on a SHA-256 of 32 random bytes, so a timing + * side channel on the comparison reveals nothing an attacker could use. */ export async function validateWsToken(token: string): Promise { - const tokenHash = hashToken(token); - const match = findWsTokenEntry(tokenHash); - - if (!match) return null; + const [entry] = await db + .delete(wsUpgradeTokens) + .where(eq(wsUpgradeTokens.tokenHash, hashToken(token))) + .returning({ + userId: wsUpgradeTokens.userId, + live: sql`${wsUpgradeTokens.expiresAt} > now()`, + }); - // Always delete — single use regardless of expiry check (atomic consume) - wsUpgradeTokens.delete(match.key); - - if (match.entry.expiresAt <= Date.now()) return null; + // Consumed either way: an expired token is spent, not left to retry. + if (!entry || !entry.live) return null; // Look up the user by ID const rows = await db @@ -244,7 +219,7 @@ export async function validateWsToken(token: string): Promise { const result = await db.delete(sessions).where(lt(sessions.expiresAt, new Date())).returning(); diff --git a/apps/api/src/services/task-service.ts b/apps/api/src/services/task-service.ts index 669d8006..ccec941c 100644 --- a/apps/api/src/services/task-service.ts +++ b/apps/api/src/services/task-service.ts @@ -366,6 +366,12 @@ export async function searchTasks(opts: SearchTasksOpts) { return { tasks: hydrated, nextCursor, hasMore }; } +/** + * Where a transition's reads and writes run: the shared client, or a + * transaction (a claim under the advisory lock, services/claim-lock.ts). + */ +export type TransitionExecutor = Pick; + export async function transitionTask( id: string, toState: TaskState, @@ -373,7 +379,46 @@ export async function transitionTask( message?: string, userId?: string, ) { - const task = await getTask(id); + const written = await writeTransition(db, id, toState, trigger, message, userId); + await announceTransition(written); + return written.updatedTask; +} + +/** + * `transitionTask` inside a claim (`withClaimLock`): the CAS and its event + * row on the claim's transaction, so they commit with it, and the + * announcement — events, webhooks, the reconciler — registered to run once + * it has committed. The updated task, or null when another worker won the + * race (the shapes `tryTransitionTask` treats as lost). + */ +export async function claimTransitionIn( + claim: { tx: TransitionExecutor; afterCommit(step: () => unknown): void }, + id: string, + toState: TaskState, + trigger: string, +) { + try { + const written = await writeTransition(claim.tx, id, toState, trigger); + claim.afterCommit(() => announceTransition(written)); + return written.updatedTask; + } catch (err) { + if (err instanceof StateRaceError || err instanceof InvalidTransitionError) return null; + throw err; + } +} + +type WrittenTransition = Awaited>; + +/** The read, validation, CAS update and event row of a transition. */ +async function writeTransition( + exec: TransitionExecutor, + id: string, + toState: TaskState, + trigger: string, + message?: string, + userId?: string, +) { + const [task] = await exec.select().from(tasks).where(eq(tasks.id, id)); if (!task) throw new Error(`Task not found: ${id}`); const currentState = task.state as TaskState; @@ -424,7 +469,7 @@ export async function transitionTask( } // Atomic conditional update — only succeeds if state hasn't changed since we read it - const updated = await db + const updated = await exec .update(tasks) .set(updateFields) .where(and(eq(tasks.id, id), eq(tasks.state, currentState as any))) @@ -432,11 +477,11 @@ export async function transitionTask( if (updated.length === 0) { // Another worker changed the state between our read and write - const fresh = await getTask(id); + const [fresh] = await exec.select({ state: tasks.state }).from(tasks).where(eq(tasks.id, id)); throw new StateRaceError(currentState, toState, fresh?.state as TaskState); } - await db.insert(taskEvents).values({ + await exec.insert(taskEvents).values({ taskId: id, fromState: currentState, toState, @@ -445,7 +490,16 @@ export async function transitionTask( userId, }); - const updatedTask = updated[0]; + return { id, task, currentState, toState, trigger, updatedTask: updated[0] }; +} + +/** + * What follows a committed transition: metrics, the state event, linked + * issue, webhooks, notifications, dependents, kills, and the reconciler. + */ +async function announceTransition(written: WrittenTransition) { + const { id, task, currentState, toState, trigger, updatedTask } = written; + const updated = [updatedTask]; // Emit OTel state transition metric and log recordStateTransition(currentState, toState, trigger); @@ -558,8 +612,6 @@ export async function transitionTask( enqueueReconcile({ kind: "repo", id }, { reason: `transition:${currentState}->${toState}` }), ) .catch((err) => logger.warn({ err, taskId: id }, "Failed to enqueue reconcile")); - - return updated[0]; } async function closeIssue(repoUrl: string, issueNumber: string, prUrl?: string | null) { diff --git a/apps/api/src/services/ticket-sync-service.test.ts b/apps/api/src/services/ticket-sync-service.test.ts index 2574096c..e7aa62ac 100644 --- a/apps/api/src/services/ticket-sync-service.test.ts +++ b/apps/api/src/services/ticket-sync-service.test.ts @@ -1,14 +1,24 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; +// The sync claims each ticket in `ticket_sync_claims` before creating its +// task (an insert that returns a row) and records the task id after. vi.mock("../db/client.js", () => ({ db: { select: vi.fn(), insert: vi.fn(), - update: vi.fn(), + execute: vi.fn(async () => [{ ticket_external_id: "claimed" }]), + update: vi.fn(() => ({ set: () => ({ where: async () => undefined }) })), + delete: vi.fn(() => ({ where: async () => undefined })), }, })); vi.mock("../db/schema.js", () => ({ + ticketSyncClaims: { + ticketSource: "ticket_sync_claims.ticket_source", + ticketExternalId: "ticket_sync_claims.ticket_external_id", + repoUrl: "ticket_sync_claims.repo_url", + taskId: "ticket_sync_claims.task_id", + }, ticketProviders: { enabled: "ticket_providers.enabled", id: "ticket_providers.id", @@ -451,6 +461,43 @@ describe("ticket-sync-service", () => { expect(count).toBe(1); // Task still synced despite comment failure }); + it("queues the task, and keeps sweeping, when recording it on its claim fails", async () => { + mockDbSelect([ + { + id: "p1", + source: "github", + config: { repoUrl: "https://github.com/o/r" }, + enabled: true, + initialSyncAt: new Date("2026-01-01"), + }, + ]); + (db.update as any) = vi.fn(() => ({ + set: () => ({ where: () => Promise.reject(new Error("connection reset")) }), + })); + const ticket = (externalId: string) => ({ + title: `Task ${externalId}`, + body: "", + source: "github", + externalId, + url: "", + labels: [], + repo: null, + }); + vi.mocked(getTicketProvider).mockReturnValue({ + fetchActionableTickets: vi.fn().mockResolvedValue([ticket("201"), ticket("202")]), + fetchTicketComments: vi.fn().mockResolvedValue([]), + addComment: vi.fn().mockResolvedValue(undefined), + } as any); + vi.mocked(taskService.listTasks).mockResolvedValue([] as any); + vi.mocked(taskService.createTask) + .mockResolvedValueOnce({ id: "t-201", maxRetries: 3 } as any) + .mockResolvedValueOnce({ id: "t-202", maxRetries: 3 } as any); + + expect(await syncAllTickets()).toBe(2); + expect(taskService.transitionTask).toHaveBeenCalledWith("t-201", "queued", "ticket_sync"); + expect(taskService.transitionTask).toHaveBeenCalledWith("t-202", "queued", "ticket_sync"); + }); + it("queries configuredRepos only once even with multiple providers", async () => { mockDbSelect( [ diff --git a/apps/api/src/services/ticket-sync-service.ts b/apps/api/src/services/ticket-sync-service.ts index 3e510d8e..77d3f716 100644 --- a/apps/api/src/services/ticket-sync-service.ts +++ b/apps/api/src/services/ticket-sync-service.ts @@ -1,6 +1,6 @@ -import { eq } from "drizzle-orm"; +import { and, eq, isNull, sql } from "drizzle-orm"; import { db } from "../db/client.js"; -import { ticketProviders, repos } from "../db/schema.js"; +import { ticketProviders, repos, ticketSyncClaims } from "../db/schema.js"; import { getTicketProvider } from "@optio/ticket-providers"; import type { TicketSource } from "@optio/shared"; import { TaskState, normalizeRepoUrl } from "@optio/shared"; @@ -15,6 +15,76 @@ import { recordAuthEvent } from "./auth-failure-detector.js"; /** Auto-disable a provider after this many consecutive failures. */ const MAX_CONSECUTIVE_FAILURES = 5; +/** A task-less claim older than this was left by a sweep that died mid-way. */ +const ABANDONED_CLAIM = "10 minutes"; + +/** + * Claims (source, ticket, repo) for this sweep: the insert lands once, + * however many sweeps overlap (docs/plans/scale-out.md §3). True iff this + * caller holds the claim and may create the task. A claim whose task never + * got created (`task_id` null) is released by `releaseTicketClaim`, so it + * does not block the ticket forever; one whose sweep died before that is + * taken over after ten minutes, and one whose task was deleted is taken + * over at once (a deleted task is re-created, as before claims existed). + * The takeover is the same one statement, so two racing sweeps still get + * one claim. + */ +export async function claimTicketForSync( + source: string, + externalId: string, + repoUrl: string, +): Promise { + const rows = await db.execute<{ ticket_external_id: string }>(sql` + INSERT INTO "ticket_sync_claims" ("ticket_source", "ticket_external_id", "repo_url") + VALUES (${source}, ${externalId}, ${repoUrl}) + ON CONFLICT ("ticket_source", "ticket_external_id", "repo_url") DO UPDATE + SET "task_id" = NULL, "created_at" = now() + WHERE ("ticket_sync_claims"."task_id" IS NULL + AND "ticket_sync_claims"."created_at" < now() - ${ABANDONED_CLAIM}::interval) + OR ("ticket_sync_claims"."task_id" IS NOT NULL + AND NOT EXISTS (SELECT 1 FROM "tasks" t WHERE t."id" = "ticket_sync_claims"."task_id")) + RETURNING "ticket_external_id"`); + return rows.length > 0; +} + +/** Records the task a claim produced. */ +export async function recordTicketClaimTask( + source: string, + externalId: string, + repoUrl: string, + taskId: string, +): Promise { + await db + .update(ticketSyncClaims) + .set({ taskId }) + .where( + and( + eq(ticketSyncClaims.ticketSource, source), + eq(ticketSyncClaims.ticketExternalId, externalId), + eq(ticketSyncClaims.repoUrl, repoUrl), + ), + ); +} + +/** Gives a claim back when creating its task failed (only a task-less claim is released). */ +export async function releaseTicketClaim( + source: string, + externalId: string, + repoUrl: string, +): Promise { + await db + .delete(ticketSyncClaims) + .where( + and( + eq(ticketSyncClaims.ticketSource, source), + eq(ticketSyncClaims.ticketExternalId, externalId), + eq(ticketSyncClaims.repoUrl, repoUrl), + isNull(ticketSyncClaims.taskId), + ), + ) + .catch((err) => logger.warn({ err, source, externalId }, "Could not release ticket claim")); +} + export async function syncAllTickets(): Promise { const providers = await db .select() @@ -124,6 +194,18 @@ export async function syncAllTickets(): Promise { if (alreadyExists) continue; + // Claim the ticket for this repo before creating anything: two + // sweeps that overlap (two API instances, or a slow sweep and the + // next tick) both get past the scan above, and only the one whose + // claim row lands creates the task. + if (!(await claimTicketForSync(ticket.source, ticket.externalId, normalizedRepoUrl))) { + logger.debug( + { ticketId: ticket.externalId, repoUrl: normalizedRepoUrl }, + "[ticket-sync] Ticket already claimed by another sweep", + ); + continue; + } + // Fetch comments for context let commentsSection = ""; try { @@ -153,18 +235,40 @@ export async function syncAllTickets(): Promise { : null; const agentType = labelAgent ?? repoConfig?.defaultAgentType ?? "claude-code"; - const task = await taskService.createTask({ - title: ticket.title, - prompt: `${ticket.title}\n\n${ticket.body}${commentsSection}${attachmentsSection}`, - repoUrl, - agentType, - ticketSource: ticket.source, - ticketExternalId: ticket.externalId, - metadata: { ticketUrl: ticket.url }, - // Inherit the repo's workspace so webhook/poll-created tasks are - // visible in the (workspace-scoped) UI — see issue #544. - workspaceId: repoConfig?.workspaceId ?? null, - }); + let task: Awaited>; + try { + task = await taskService.createTask({ + title: ticket.title, + prompt: `${ticket.title}\n\n${ticket.body}${commentsSection}${attachmentsSection}`, + repoUrl, + agentType, + ticketSource: ticket.source, + ticketExternalId: ticket.externalId, + metadata: { ticketUrl: ticket.url }, + // Inherit the repo's workspace so webhook/poll-created tasks are + // visible in the (workspace-scoped) UI — see issue #544. + workspaceId: repoConfig?.workspaceId ?? null, + }); + } catch (err) { + // The claim is only good with a task behind it: give it back so + // the next sweep tries again. + await releaseTicketClaim(ticket.source, ticket.externalId, normalizedRepoUrl); + throw err; + } + // The task exists: a failure to note it on the claim must not strand + // it (or stop this provider's sweep). The claim stays task-less and + // is taken over after a while; the scan above still finds the task. + await recordTicketClaimTask( + ticket.source, + ticket.externalId, + normalizedRepoUrl, + task.id, + ).catch((err) => + logger.warn( + { err, taskId: task.id, ticketId: ticket.externalId }, + "[ticket-sync] Could not record the task on its ticket claim", + ), + ); if (isFirstSync) { // Backfill: leave the task pending for the user to start, don't diff --git a/apps/api/src/services/trigger-service.ts b/apps/api/src/services/trigger-service.ts index 86dd5a59..179c2de4 100644 --- a/apps/api/src/services/trigger-service.ts +++ b/apps/api/src/services/trigger-service.ts @@ -35,6 +35,7 @@ import { import { db } from "../db/client.js"; import { workflowTriggers } from "../db/schema.js"; import { computeNextFire } from "../utils/cron.js"; +import { updatedAtMatches } from "../utils/pg-timestamp.js"; // The pool, or a transaction — so a trigger can be written with the row it starts. import type { Db } from "./work-definition-service.js"; @@ -475,6 +476,33 @@ export async function advanceSchedule(id: string, cronExpression: string): Promi .where(eq(workflowTriggers.id, id)); } +/** + * Moves a schedule past the tick a sweep saw — only if nobody moved it + * first. The sweep fires the trigger iff this returns true, so overlapping + * sweeps (two API instances) fire each tick once. A trigger whose + * `next_fire_at` is null is never due, so `seen` is always a date here. + */ +export async function advanceScheduleCas( + id: string, + cronExpression: string, + seen: Date | null, +): Promise { + if (!seen) return false; + const now = new Date(); + const rows = await db + .update(workflowTriggers) + .set({ lastFiredAt: now, nextFireAt: computeNextFire(cronExpression), updatedAt: now }) + .where( + and( + eq(workflowTriggers.id, id), + eq(workflowTriggers.enabled, true), + updatedAtMatches(workflowTriggers.nextFireAt, seen), + ), + ) + .returning({ id: workflowTriggers.id }); + return rows.length > 0; +} + export async function markTriggerFired(id: string): Promise { await db .update(workflowTriggers) diff --git a/apps/api/src/services/workflow-service.ts b/apps/api/src/services/workflow-service.ts index 05588084..8442e5e2 100644 --- a/apps/api/src/services/workflow-service.ts +++ b/apps/api/src/services/workflow-service.ts @@ -410,8 +410,13 @@ export async function getWorkflowRun(id: string) { * OPTIO_MAX_WORKFLOW_CONCURRENT, and this Job's running runs under its * `maxConcurrent`. A run starts only while both are below their max. */ -export async function jobRunCapacity(workflowId: string, maxConcurrent: number) { - const [row] = await db +export async function jobRunCapacity( + workflowId: string, + maxConcurrent: number, + /** The claim lock's transaction (claim-lock.ts), when counting under it. */ + tx: Pick = db, +) { + const [row] = await tx .select({ global: sql`count(*) FILTER (WHERE ${workDefinitions.runTarget} <> 'local')::int`, job: sql`count(*) FILTER (WHERE ${workflowRuns.workflowId} = ${workflowId})::int`, @@ -568,12 +573,45 @@ export async function transitionWorkflowRunCas( startedAt?: Date; wakeReconciler?: boolean; } = {}, +) { + const row = await writeWorkflowRunCas(db, runId, from, to, fields, opts); + if (!row) return null; + await announceWorkflowRunTransition(row, from, to, opts); + return row; +} + +/** + * `transitionWorkflowRunCas` inside a claim (`withClaimLock`): the CAS on + * the claim's transaction, so it commits with it, and the announcement — + * events, webhooks, the reconciler — registered to run once it has + * committed. The updated run, or null when the CAS lost. + */ +export async function claimWorkflowRunIn( + claim: { tx: Pick; afterCommit(step: () => unknown): void }, + runId: string, + from: WorkflowRunState, + to: WorkflowRunState, + fields: PgUpdateSetSource = {}, +) { + const row = await writeWorkflowRunCas(claim.tx, runId, from, to, fields, {}); + if (!row) return null; + claim.afterCommit(() => announceWorkflowRunTransition(row, from, to, {})); + return row; +} + +async function writeWorkflowRunCas( + exec: Pick, + runId: string, + from: WorkflowRunState, + to: WorkflowRunState, + fields: PgUpdateSetSource, + opts: { version?: Date; startedAt?: Date }, ) { if (!canTransitionWorkflowRun(from, to)) { logger.warn({ runId, from, to }, "Invalid workflow run state transition"); return null; } - const [row] = await db + const [row] = await exec .update(workflowRuns) .set({ ...fields, state: to, updatedAt: new Date() }) .where( @@ -585,8 +623,17 @@ export async function transitionWorkflowRunCas( ), ) .returning(); - if (!row) return null; + return row ?? null; +} +/** What follows a committed run transition: the event, webhooks, kills, the reconciler. */ +async function announceWorkflowRunTransition( + row: typeof workflowRuns.$inferSelect, + from: WorkflowRunState, + to: WorkflowRunState, + opts: { wakeReconciler?: boolean }, +) { + const runId = row.id; await publishWorkflowRunEvent({ type: "workflow_run:state_changed", workflowRunId: runId, @@ -661,8 +708,6 @@ export async function transitionWorkflowRunCas( ) .catch((err) => logger.warn({ err, runId }, "Failed to enqueue reconcile")); } - - return row; } // ── Workflow Run Logs ──────────────────────────────────────────────────────── diff --git a/apps/api/src/utils/db-errors.ts b/apps/api/src/utils/db-errors.ts index d80041fd..38d0c38e 100644 --- a/apps/api/src/utils/db-errors.ts +++ b/apps/api/src/utils/db-errors.ts @@ -1,7 +1,22 @@ -/** A Postgres unique violation — drizzle wraps it ("Failed query: …"), so the code is on a `cause`. */ -export function isUniqueViolation(err: unknown): boolean { +/** The Postgres SQLSTATE on an error — drizzle wraps it ("Failed query: …"), so it may be on a `cause`. */ +function hasPgCode(err: unknown, code: string): boolean { for (let e = err; e; e = (e as { cause?: unknown }).cause) { - if ((e as { code?: string }).code === "23505") return true; + if ((e as { code?: string }).code === code) return true; } return false; } + +/** A Postgres unique violation. */ +export function isUniqueViolation(err: unknown): boolean { + return hasPgCode(err, "23505"); +} + +/** `lock_timeout` ran out waiting for a lock (`lock_not_available`). */ +export function isLockTimeout(err: unknown): boolean { + return hasPgCode(err, "55P03"); +} + +/** `statement_timeout` (or a cancel) stopped the statement (`query_canceled`). */ +export function isStatementTimeout(err: unknown): boolean { + return hasPgCode(err, "57014"); +} diff --git a/apps/api/src/workers/config-sync-worker.ts b/apps/api/src/workers/config-sync-worker.ts index 50875711..8bfee730 100644 --- a/apps/api/src/workers/config-sync-worker.ts +++ b/apps/api/src/workers/config-sync-worker.ts @@ -10,6 +10,7 @@ import { Queue, Worker } from "bullmq"; import { logger } from "../logger.js"; import { getBullMQOptions } from "../services/redis-config.js"; import { envConfigSource, syncEnvSource } from "../services/config/source.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; const QUEUE = "config-sync"; @@ -18,29 +19,26 @@ export function startConfigSyncWorker(): Worker | null { if (!env) return null; const bullmqOpts = getBullMQOptions(); const queue = new Queue(QUEUE, { ...bullmqOpts }); - queue - .add( - "sync", - {}, - { - repeat: { every: env.intervalMs }, - removeOnComplete: { count: 20 }, - removeOnFail: { count: 20 }, - }, - ) - .catch((err) => logger.error({ err }, "config sync: could not schedule")); + void scheduleRepeat(queue, "config-sync.sync", env.intervalMs, { + removeOnComplete: { count: 20 }, + removeOnFail: { count: 20 }, + }).catch((err) => logger.error({ err }, "config sync: could not schedule")); + // A tick skips when another instance is applying (source.ts takes the + // apply lease without waiting); "Sync now" from the UI waits for it. const worker = new Worker( QUEUE, async () => { - await syncEnvSource(); + await syncEnvSource({ wait: false }); }, { ...bullmqOpts, concurrency: 1 }, ); worker.on("failed", (_job, err) => logger.error({ err }, "config sync failed")); // Don't wait a whole interval for the first pass. - syncEnvSource().catch((err) => logger.error({ err }, "config sync: first pass failed")); + syncEnvSource({ wait: false }).catch((err) => + logger.error({ err }, "config sync: first pass failed"), + ); logger.info( { dir: env.dir, everyMs: env.intervalMs, prune: env.prune }, "Config sync worker started", diff --git a/apps/api/src/workers/external-pr-review-worker.ts b/apps/api/src/workers/external-pr-review-worker.ts index 7a78cdd2..0008064f 100644 --- a/apps/api/src/workers/external-pr-review-worker.ts +++ b/apps/api/src/workers/external-pr-review-worker.ts @@ -21,9 +21,15 @@ import { db } from "../db/client.js"; import { repos, prReviews } from "../db/schema.js"; import { parseIntEnv, parseRepoUrl, PrReviewState } from "@optio/shared"; import { getGitPlatformForRepo } from "../services/git-token-service.js"; -import { launchPrReview, isOptioAuthoredPr } from "../services/pr-review-service.js"; +import { + launchPrReview, + isOptioAuthoredPr, + preferredReview, +} from "../services/pr-review-service.js"; import { logger } from "../logger.js"; import { getBullMQOptions } from "../services/redis-config.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; +import { underPollerLease } from "../services/poller-lease.js"; import { instrumentWorkerProcessor } from "../telemetry/instrument-worker.js"; import { determineCheckStatus } from "./pr-watcher-worker.js"; @@ -57,147 +63,161 @@ function passesFilters( return true; } +/** + * One sweep at a time across instances (a poller lease); a sweep that does + * overlap another can't launch a second review of a PR anyway: each launch + * names the review the sweep saw (`seenVersion`, none for a new PR) and + * lands only while that is still the review, and + * `pr_reviews_active_pr_url_key` admits one active review per PR URL — + * `launchPrReview` treats either race as "someone else launched it". + */ export function startExternalPrReviewWorker() { - externalPrReviewQueue.add( - "poll-external-prs", - {}, - { - repeat: { - every: parseIntEnv("OPTIO_EXTERNAL_PR_POLL_INTERVAL_MS", 120_000), - }, - }, - ); + void scheduleRepeat( + externalPrReviewQueue, + "external-pr-review.poll-external-prs", + parseIntEnv("OPTIO_EXTERNAL_PR_POLL_INTERVAL_MS", 120_000), + ).catch(() => {}); const worker = new Worker( "external-pr-review", instrumentWorkerProcessor("external-pr-review", async () => { - const activeRepos = await db - .select() - .from(repos) - .where( - sql`${repos.externalReviewMode} IN ('on_pr_hold', 'on_pr_post') AND ${repos.reviewEnabled} = true`, - ); - - for (const repo of activeRepos) { - try { - const ri = parseRepoUrl(repo.repoUrl); - if (!ri) continue; - - const { platform } = await getGitPlatformForRepo(repo.repoUrl, { server: true }).catch( - () => ({ - platform: null as unknown as Awaited< - ReturnType - >["platform"], - }), - ); - if (!platform) continue; - - const prs = await platform.listOpenPullRequests(ri, { perPage: 50 }).catch(() => []); - if (prs.length === 0) continue; - - const prUrls = prs.map((p) => p.url); - const reviews = prUrls.length - ? await db.select().from(prReviews).where(inArray(prReviews.prUrl, prUrls)) - : []; - const byUrl = new Map(reviews.map((r) => [r.prUrl, r])); - - const filters = repo.externalReviewFilters ?? null; - - for (const pr of prs) { - try { - const existing = byUrl.get(pr.url); - - if (existing) { - // Skip in-flight drafting states — reconciler owns those. - if ( - existing.state === PrReviewState.QUEUED || - existing.state === PrReviewState.REVIEWING - ) { - continue; - } + await underPollerLease("external-pr-review", () => sweepExternalPrs()); + }), + { ...bullmqOpts, concurrency: 1 }, + ); - // User has engaged — don't auto-mutate their review. - if (existing.userEngaged) { - // But still advance waiting_ci if CI has cleared. - if (existing.state === PrReviewState.WAITING_CI) { - const checks = await platform.getCIChecks(ri, pr.headSha).catch(() => []); - const status = determineCheckStatus(checks); - if (status !== "pending") { - await launchPrReview({ - prUrl: pr.url, - workspaceId: repo.workspaceId ?? undefined, - origin: existing.origin as "auto" | "manual", - }); - } - } - continue; - } + worker.on("failed", (_job, err) => { + logger.error({ err }, "external-pr-review worker failed"); + }); - // New commits — spawn a rereview. - if (pr.headSha && pr.headSha !== existing.headSha) { + return worker; +} + +/** One sweep over every repo that reviews external PRs. */ +export async function sweepExternalPrs(): Promise { + const activeRepos = await db + .select() + .from(repos) + .where( + sql`${repos.externalReviewMode} IN ('on_pr_hold', 'on_pr_post') AND ${repos.reviewEnabled} = true`, + ); + + for (const repo of activeRepos) { + try { + const ri = parseRepoUrl(repo.repoUrl); + if (!ri) continue; + + const { platform } = await getGitPlatformForRepo(repo.repoUrl, { server: true }).catch( + () => ({ + platform: null as unknown as Awaited< + ReturnType + >["platform"], + }), + ); + if (!platform) continue; + + const prs = await platform.listOpenPullRequests(ri, { perPage: 50 }).catch(() => []); + if (prs.length === 0) continue; + + const prUrls = prs.map((p) => p.url); + const reviews = prUrls.length + ? await db.select().from(prReviews).where(inArray(prReviews.prUrl, prUrls)) + : []; + const byUrl = new Map(); + for (const r of reviews) byUrl.set(r.prUrl, preferredReview(byUrl.get(r.prUrl), r)); + + const filters = repo.externalReviewFilters ?? null; + + for (const pr of prs) { + try { + const existing = byUrl.get(pr.url); + + if (existing) { + // Skip in-flight drafting states — reconciler owns those. + if ( + existing.state === PrReviewState.QUEUED || + existing.state === PrReviewState.REVIEWING + ) { + continue; + } + + // User has engaged — don't auto-mutate their review. + if (existing.userEngaged) { + // But still advance waiting_ci if CI has cleared. + if (existing.state === PrReviewState.WAITING_CI) { + const checks = await platform.getCIChecks(ri, pr.headSha).catch(() => []); + const status = determineCheckStatus(checks); + if (status !== "pending") { await launchPrReview({ prUrl: pr.url, workspaceId: repo.workspaceId ?? undefined, - origin: "auto", + origin: existing.origin as "auto" | "manual", + seenVersion: existing.updatedAt, }); - continue; - } - - // Waiting on CI — promote when CI clears. - if (existing.state === PrReviewState.WAITING_CI) { - const checks = await platform.getCIChecks(ri, pr.headSha).catch(() => []); - const status = determineCheckStatus(checks); - if (status !== "pending") { - await launchPrReview({ - prUrl: pr.url, - workspaceId: repo.workspaceId ?? undefined, - origin: "auto", - }); - } - continue; } - - continue; - } - - // No existing review — apply filters, then decide whether to - // park in waiting_ci or launch immediately. - if (!passesFilters(pr, filters)) continue; - if (filters?.skipOptioAuthored && (await isOptioAuthoredPr(pr.url))) continue; - - let startInWaitingCi = false; - if (repo.externalReviewWaitForCi) { - const checks = await platform.getCIChecks(ri, pr.headSha).catch(() => []); - const status = determineCheckStatus(checks); - if (status === "pending") startInWaitingCi = true; } + continue; + } + // New commits — spawn a rereview. + if (pr.headSha && pr.headSha !== existing.headSha) { await launchPrReview({ prUrl: pr.url, workspaceId: repo.workspaceId ?? undefined, origin: "auto", - startInWaitingCi, + seenVersion: existing.updatedAt, }); - } catch (err) { - logger.warn( - { err, prUrl: pr.url, repoId: repo.id }, - "external PR review: failed to process PR", - ); + continue; + } + + // Waiting on CI — promote when CI clears. + if (existing.state === PrReviewState.WAITING_CI) { + const checks = await platform.getCIChecks(ri, pr.headSha).catch(() => []); + const status = determineCheckStatus(checks); + if (status !== "pending") { + await launchPrReview({ + prUrl: pr.url, + workspaceId: repo.workspaceId ?? undefined, + origin: "auto", + seenVersion: existing.updatedAt, + }); + } + continue; } + + continue; + } + + // No existing review — apply filters, then decide whether to + // park in waiting_ci or launch immediately. + if (!passesFilters(pr, filters)) continue; + if (filters?.skipOptioAuthored && (await isOptioAuthoredPr(pr.url))) continue; + + let startInWaitingCi = false; + if (repo.externalReviewWaitForCi) { + const checks = await platform.getCIChecks(ri, pr.headSha).catch(() => []); + const status = determineCheckStatus(checks); + if (status === "pending") startInWaitingCi = true; } + + await launchPrReview({ + prUrl: pr.url, + workspaceId: repo.workspaceId ?? undefined, + origin: "auto", + startInWaitingCi, + seenVersion: null, + }); } catch (err) { - logger.warn({ err, repoId: repo.id }, "external PR review: failed to process repo"); + logger.warn( + { err, prUrl: pr.url, repoId: repo.id }, + "external PR review: failed to process PR", + ); } } - }), - { ...bullmqOpts, concurrency: 1 }, - ); - - worker.on("failed", (_job, err) => { - logger.error({ err }, "external-pr-review worker failed"); - }); - - return worker; + } catch (err) { + logger.warn({ err, repoId: repo.id }, "external PR review: failed to process repo"); + } + } } // Keep `and`, `eq` available for future filters so lint doesn't strip them. diff --git a/apps/api/src/workers/pr-watcher-worker.ts b/apps/api/src/workers/pr-watcher-worker.ts index 3282f2e6..c6a61d20 100644 --- a/apps/api/src/workers/pr-watcher-worker.ts +++ b/apps/api/src/workers/pr-watcher-worker.ts @@ -14,6 +14,8 @@ import { recordPrWatchCycleDuration } from "../telemetry/metrics.js"; import { instrumentWorkerProcessor } from "../telemetry/instrument-worker.js"; import { getBullMQOptions } from "../services/redis-config.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; +import { underPollerLease } from "../services/poller-lease.js"; const bullmqOpts = getBullMQOptions(); @@ -50,232 +52,238 @@ export function determineReviewStatus(reviews: { state: string; body?: string }[ export const prWatcherQueue = new Queue("pr-watcher", { ...bullmqOpts }); +/** + * One poll cycle at a time across instances (a poller lease): the cycle is + * idempotent — it refreshes PR fields and wakes the reconciler — but every + * instance polling would multiply the GitHub API calls. + */ export function startPrWatcherWorker() { - prWatcherQueue.add( - "check-prs", - {}, - { - repeat: { - every: parseIntEnv("OPTIO_PR_WATCH_INTERVAL", 30000), - }, - }, - ); + void scheduleRepeat( + prWatcherQueue, + "pr-watcher.check-prs", + parseIntEnv("OPTIO_PR_WATCH_INTERVAL", 30000), + ).catch(() => {}); const worker = new Worker( "pr-watcher", instrumentWorkerProcessor("pr-watcher", async () => { - const cycleStart = Date.now(); - // Per-cycle cache to avoid redundant token lookups / secret decryption - const platformCache = new Map(); - async function getCachedPlatform( - repoUrl: string, - context: GitTokenContext, - ): Promise<{ platform: GitPlatform; ri: RepoIdentifier } | null> { - const key = `${repoUrl}::${context.userId ?? "server"}`; - const cached = platformCache.get(key); - if (cached) return cached; + await underPollerLease("pr-watcher", () => pollPrs()); + }), + { ...bullmqOpts, concurrency: 1 }, + ); + + worker.on("failed", (_job, err) => { + logger.error({ err }, "PR watcher failed"); + }); + + return worker; +} + +/** One poll cycle over every watched PR (the tick's body). */ +export async function pollPrs(): Promise { + const cycleStart = Date.now(); + // Per-cycle cache to avoid redundant token lookups / secret decryption + const platformCache = new Map(); + async function getCachedPlatform( + repoUrl: string, + context: GitTokenContext, + ): Promise<{ platform: GitPlatform; ri: RepoIdentifier } | null> { + const key = `${repoUrl}::${context.userId ?? "server"}`; + const cached = platformCache.get(key); + if (cached) return cached; + try { + const result = await getGitPlatformForRepo(repoUrl, context); + platformCache.set(key, result); + return result; + } catch { + return null; + } + } + + // --- Task PR watching --- + // Find all tasks with open PRs. Watch pr_opened tasks + failed tasks + // that have a PR (CI may recover, auto-merge may become possible). + // Only watch coding tasks, NOT review subtasks (avoid recursive reviews). + // + // The watcher's only job is to refresh the PR fields on the row and + // wake the reconciler — every transition / side-effect (auto-merge, + // review launch, resume, completion) is decided in reconcile-repo.ts + // and applied by reconcile-executor.ts. + const openPrTasks = await db + .select() + .from(tasks) + .where( + sql`${tasks.state} IN ('pr_opened', 'failed') AND ${tasks.prUrl} IS NOT NULL AND (${tasks.taskType} = 'coding' OR ${tasks.taskType} IS NULL)`, + ); + + for (const task of openPrTasks) { + if (!task.prUrl) continue; + + try { + const parsed = parsePrUrl(task.prUrl); + if (!parsed) continue; + const { prNumber } = parsed; + + const platformResult = await getCachedPlatform(task.repoUrl, { + userId: task.createdBy ?? undefined, + }); + if (!platformResult) continue; + const { platform, ri } = platformResult; + + const prData = await platform.getPullRequest(ri, prNumber).catch(() => null); + if (!prData) continue; + + const checkRuns = await platform.getCIChecks(ri, prData.headSha).catch(() => []); + const reviewsData = await platform.getReviews(ri, prNumber).catch(() => []); + const checksStatus = determineCheckStatus(checkRuns); + const reviewResult = determineReviewStatus(reviewsData); + const reviewStatus = reviewResult.status; + let reviewComments = reviewResult.comments; + + // If changes requested, also fetch inline comments for context. + if (reviewStatus === "changes_requested") { try { - const result = await getGitPlatformForRepo(repoUrl, context); - platformCache.set(key, result); - return result; - } catch { - return null; - } + const inlineComments = await platform.getInlineComments(ri, prNumber); + const recent = inlineComments.slice(-5); + if (recent.length > 0) { + reviewComments += + "\n\nInline comments:\n" + + recent.map((c) => `${c.path}:${c.line ?? ""} — ${c.body}`).join("\n"); + } + } catch {} } - // --- Task PR watching --- - // Find all tasks with open PRs. Watch pr_opened tasks + failed tasks - // that have a PR (CI may recover, auto-merge may become possible). - // Only watch coding tasks, NOT review subtasks (avoid recursive reviews). - // - // The watcher's only job is to refresh the PR fields on the row and - // wake the reconciler — every transition / side-effect (auto-merge, - // review launch, resume, completion) is decided in reconcile-repo.ts - // and applied by reconcile-executor.ts. - const openPrTasks = await db + // Conflicts override the raw checks status — once we've recorded + // conflicts, keep that label until mergeable flips back. + const effectiveChecksStatus = + task.prChecksStatus === "conflicts" && prData.mergeable === false + ? "conflicts" + : checksStatus; + + // Write all PR fields in one update. The reconciler reads these + // from the snapshot to decide the next action. + const updates: Record = { + prNumber, + prState: prData.merged ? "merged" : prData.state, + prChecksStatus: effectiveChecksStatus, + prReviewStatus: reviewStatus, + updatedAt: new Date(), + }; + if (reviewComments) { + updates.prReviewComments = reviewComments; + } + await db.update(tasks).set(updates).where(eq(tasks.id, task.id)); + + await enqueueReconcile( + { kind: "repo", id: task.id }, + { reason: `pr_watch:${prData.merged ? "merged" : prData.state}` }, + ); + } catch (err: any) { + logger.warn({ err, taskId: task.id }, "Failed to check PR status"); + if (err?.status === 401 || err?.message?.includes("Bad credentials")) { + recordAuthEvent("github", err.message ?? "GitHub 401", "pr-watcher").catch(() => {}); + } + } + } + + // --- Session PR watching --- + // Poll PRs tracked in active sessions to keep CI/review/merge status up to date + try { + const activeSessions = await db + .select({ id: interactiveSessions.id }) + .from(interactiveSessions) + .where(eq(interactiveSessions.state, "active")); + + if (activeSessions.length > 0) { + const sessionIds = activeSessions.map((s) => s.id); + const openSessionPrs = await db .select() - .from(tasks) + .from(sessionPrs) .where( - sql`${tasks.state} IN ('pr_opened', 'failed') AND ${tasks.prUrl} IS NOT NULL AND (${tasks.taskType} = 'coding' OR ${tasks.taskType} IS NULL)`, + sql`${sessionPrs.sessionId} IN ${sessionIds} AND (${sessionPrs.prState} IS NULL OR ${sessionPrs.prState} = 'open')`, ); - for (const task of openPrTasks) { - if (!task.prUrl) continue; - + for (const spr of openSessionPrs) { try { - const parsed = parsePrUrl(task.prUrl); - if (!parsed) continue; - const { prNumber } = parsed; - - const platformResult = await getCachedPlatform(task.repoUrl, { - userId: task.createdBy ?? undefined, + const sprParsed = parsePrUrl(spr.prUrl); + if (!sprParsed) continue; + + // Infer repo URL from PR URL for platform resolution + const sprRepoUrl = `https://${sprParsed.host}/${sprParsed.owner}/${sprParsed.repo}`; + const sprResult = await getCachedPlatform(sprRepoUrl, { server: true }); + if (!sprResult) continue; + const { platform: sprPlatform, ri: sprRi } = sprResult; + + const sprData = await sprPlatform + .getPullRequest(sprRi, sprParsed.prNumber) + .catch(() => null); + if (!sprData) continue; + + const sprCheckRuns = await sprPlatform + .getCIChecks(sprRi, sprData.headSha) + .catch(() => []); + const sprChecksStatus = determineCheckStatus(sprCheckRuns); + + const sprReviewsData = await sprPlatform + .getReviews(sprRi, sprParsed.prNumber) + .catch(() => []); + const sprReviewResult = determineReviewStatus(sprReviewsData); + + await updateSessionPr(spr.id, { + prState: sprData.merged ? "merged" : sprData.state, + prChecksStatus: sprChecksStatus, + prReviewStatus: sprReviewResult.status, }); - if (!platformResult) continue; - const { platform, ri } = platformResult; - - const prData = await platform.getPullRequest(ri, prNumber).catch(() => null); - if (!prData) continue; - - const checkRuns = await platform.getCIChecks(ri, prData.headSha).catch(() => []); - const reviewsData = await platform.getReviews(ri, prNumber).catch(() => []); - const checksStatus = determineCheckStatus(checkRuns); - const reviewResult = determineReviewStatus(reviewsData); - const reviewStatus = reviewResult.status; - let reviewComments = reviewResult.comments; - - // If changes requested, also fetch inline comments for context. - if (reviewStatus === "changes_requested") { - try { - const inlineComments = await platform.getInlineComments(ri, prNumber); - const recent = inlineComments.slice(-5); - if (recent.length > 0) { - reviewComments += - "\n\nInline comments:\n" + - recent.map((c) => `${c.path}:${c.line ?? ""} — ${c.body}`).join("\n"); - } - } catch {} - } - - // Conflicts override the raw checks status — once we've recorded - // conflicts, keep that label until mergeable flips back. - const effectiveChecksStatus = - task.prChecksStatus === "conflicts" && prData.mergeable === false - ? "conflicts" - : checksStatus; - - // Write all PR fields in one update. The reconciler reads these - // from the snapshot to decide the next action. - const updates: Record = { - prNumber, - prState: prData.merged ? "merged" : prData.state, - prChecksStatus: effectiveChecksStatus, - prReviewStatus: reviewStatus, - updatedAt: new Date(), - }; - if (reviewComments) { - updates.prReviewComments = reviewComments; - } - await db.update(tasks).set(updates).where(eq(tasks.id, task.id)); - - await enqueueReconcile( - { kind: "repo", id: task.id }, - { reason: `pr_watch:${prData.merged ? "merged" : prData.state}` }, - ); - } catch (err: any) { - logger.warn({ err, taskId: task.id }, "Failed to check PR status"); - if (err?.status === 401 || err?.message?.includes("Bad credentials")) { - recordAuthEvent("github", err.message ?? "GitHub 401", "pr-watcher").catch(() => {}); - } + } catch (err) { + logger.warn({ err, sessionPrId: spr.id }, "Failed to check session PR status"); } } + } + } catch (err) { + logger.warn({ err }, "Failed to run session PR watcher"); + } - // --- Session PR watching --- - // Poll PRs tracked in active sessions to keep CI/review/merge status up to date - try { - const activeSessions = await db - .select({ id: interactiveSessions.id }) - .from(interactiveSessions) - .where(eq(interactiveSessions.state, "active")); - - if (activeSessions.length > 0) { - const sessionIds = activeSessions.map((s) => s.id); - const openSessionPrs = await db - .select() - .from(sessionPrs) - .where( - sql`${sessionPrs.sessionId} IN ${sessionIds} AND (${sessionPrs.prState} IS NULL OR ${sessionPrs.prState} = 'open')`, - ); - - for (const spr of openSessionPrs) { - try { - const sprParsed = parsePrUrl(spr.prUrl); - if (!sprParsed) continue; - - // Infer repo URL from PR URL for platform resolution - const sprRepoUrl = `https://${sprParsed.host}/${sprParsed.owner}/${sprParsed.repo}`; - const sprResult = await getCachedPlatform(sprRepoUrl, { server: true }); - if (!sprResult) continue; - const { platform: sprPlatform, ri: sprRi } = sprResult; - - const sprData = await sprPlatform - .getPullRequest(sprRi, sprParsed.prNumber) - .catch(() => null); - if (!sprData) continue; - - const sprCheckRuns = await sprPlatform - .getCIChecks(sprRi, sprData.headSha) - .catch(() => []); - const sprChecksStatus = determineCheckStatus(sprCheckRuns); - - const sprReviewsData = await sprPlatform - .getReviews(sprRi, sprParsed.prNumber) - .catch(() => []); - const sprReviewResult = determineReviewStatus(sprReviewsData); - - await updateSessionPr(spr.id, { - prState: sprData.merged ? "merged" : sprData.state, - prChecksStatus: sprChecksStatus, - prReviewStatus: sprReviewResult.status, - }); - } catch (err) { - logger.warn({ err, sessionPrId: spr.id }, "Failed to check session PR status"); - } - } - } - } catch (err) { - logger.warn({ err }, "Failed to run session PR watcher"); - } + // --- PR Review staleness detection --- + // When a ready review's PR gets new commits, mark stale so the UI + // can prompt a rereview. The reconciler handles auto-rereview for + // origin='auto' reviews; this watcher just surfaces the signal. + try { + const readyReviews = await db.select().from(prReviews).where(eq(prReviews.state, "ready")); - // --- PR Review staleness detection --- - // When a ready review's PR gets new commits, mark stale so the UI - // can prompt a rereview. The reconciler handles auto-rereview for - // origin='auto' reviews; this watcher just surfaces the signal. + for (const review of readyReviews) { try { - const readyReviews = await db.select().from(prReviews).where(eq(prReviews.state, "ready")); - - for (const review of readyReviews) { - try { - const draftParsed = parsePrUrl(review.prUrl); - if (!draftParsed) continue; - - const draftRepoUrl = `https://${draftParsed.host}/${draftParsed.owner}/${draftParsed.repo}`; - const draftResult = await getCachedPlatform(draftRepoUrl, { server: true }); - if (!draftResult) continue; - const { platform: draftPlatform, ri: draftRi } = draftResult; - - const prData = await draftPlatform - .getPullRequest(draftRi, review.prNumber) - .catch(() => null); - if (!prData) continue; - - if (prData.headSha && prData.headSha !== review.headSha) { - const { markStale } = await import("../services/pr-review-service.js"); - await markStale(review.id); - logger.info( - { - prReviewId: review.id, - oldSha: review.headSha, - newSha: prData.headSha, - }, - "PR review marked stale — PR has new commits", - ); - } - } catch (err) { - logger.warn({ err, prReviewId: review.id }, "Failed to check review staleness"); - } + const draftParsed = parsePrUrl(review.prUrl); + if (!draftParsed) continue; + + const draftRepoUrl = `https://${draftParsed.host}/${draftParsed.owner}/${draftParsed.repo}`; + const draftResult = await getCachedPlatform(draftRepoUrl, { server: true }); + if (!draftResult) continue; + const { platform: draftPlatform, ri: draftRi } = draftResult; + + const prData = await draftPlatform + .getPullRequest(draftRi, review.prNumber) + .catch(() => null); + if (!prData) continue; + + if (prData.headSha && prData.headSha !== review.headSha) { + const { markStale } = await import("../services/pr-review-service.js"); + await markStale(review.id); + logger.info( + { + prReviewId: review.id, + oldSha: review.headSha, + newSha: prData.headSha, + }, + "PR review marked stale — PR has new commits", + ); } } catch (err) { - logger.warn({ err }, "Failed to run PR review staleness check"); + logger.warn({ err, prReviewId: review.id }, "Failed to check review staleness"); } + } + } catch (err) { + logger.warn({ err }, "Failed to run PR review staleness check"); + } - recordPrWatchCycleDuration((Date.now() - cycleStart) / 1000); - }), - { ...bullmqOpts, concurrency: 1 }, - ); - - worker.on("failed", (_job, err) => { - logger.error({ err }, "PR watcher failed"); - }); - - return worker; + recordPrWatchCycleDuration((Date.now() - cycleStart) / 1000); } diff --git a/apps/api/src/workers/reconcile-worker.ts b/apps/api/src/workers/reconcile-worker.ts index 9da04adf..5bc7b5ec 100644 --- a/apps/api/src/workers/reconcile-worker.ts +++ b/apps/api/src/workers/reconcile-worker.ts @@ -8,6 +8,8 @@ import { } from "@optio/shared"; import type { RunRef, Action } from "@optio/shared"; import { getBullMQOptions } from "../services/redis-config.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; +import { underPollerLease } from "../services/poller-lease.js"; import { buildWorldSnapshot } from "../services/reconcile-snapshot.js"; import { executeAction, type ExecuteOutcome } from "../services/reconcile-executor.js"; import { reconcileQueue, enqueueReconcile } from "../services/reconcile-queue.js"; @@ -115,56 +117,14 @@ export const resyncQueue = new Queue("reconcile-resync", { ...bullmqOpts }); export function startReconcileResyncWorker() { const intervalMs = parseIntEnv("OPTIO_RECONCILE_RESYNC_INTERVAL", 5 * 60 * 1000); - resyncQueue.add( - "resync", - {}, - { - repeat: { every: intervalMs }, - }, - ); + void scheduleRepeat(resyncQueue, "reconcile-resync.resync", intervalMs).catch(() => {}); const worker = new Worker( "reconcile-resync", instrumentWorkerProcessor("reconcile-resync", async () => { - const { db } = await import("../db/client.js"); - const { workRuns, prReviews, persistentAgents } = await import("../db/schema.js"); - const { sql } = await import("drizzle-orm"); - - // Repo tasks and Job runs: `kind` is the run's RunKind. - const nonTerminalRuns = await db - .select({ id: workRuns.id, kind: workRuns.kind }) - .from(workRuns) - .where(sql`${workRuns.state} NOT IN ('completed')`); - - const nonTerminalReviews = await db - .select({ id: prReviews.id }) - .from(prReviews) - .where(sql`${prReviews.state} NOT IN ('cancelled')`); - - const livePersistentAgents = await db - .select({ id: persistentAgents.id }) - .from(persistentAgents) - .where(sql`${persistentAgents.state} NOT IN ('archived')`); - - logger.info( - { - tasks: nonTerminalRuns.filter((r) => r.kind === "repo").length, - runs: nonTerminalRuns.filter((r) => r.kind === "standalone").length, - reviews: nonTerminalReviews.length, - persistentAgents: livePersistentAgents.length, - }, - "reconcile.resync.sweep", - ); - - for (const r of nonTerminalRuns) { - await enqueueReconcile({ kind: r.kind, id: r.id }, { reason: "resync" }); - } - for (const r of nonTerminalReviews) { - await enqueueReconcile({ kind: "pr-review", id: r.id }, { reason: "resync" }); - } - for (const r of livePersistentAgents) { - await enqueueReconcile({ kind: "persistent-agent", id: r.id }, { reason: "resync" }); - } + // Idempotent (it only enqueues reconcile keys), but one sweep at a + // time keeps the reconcile queue from taking every key N times. + await underPollerLease("reconcile-resync", () => resyncAll()); }), { ...bullmqOpts, @@ -179,4 +139,47 @@ export function startReconcileResyncWorker() { return worker; } +/** One resync: enqueue a reconcile for every non-terminal run, review and agent. */ +export async function resyncAll(): Promise { + const { db } = await import("../db/client.js"); + const { workRuns, prReviews, persistentAgents } = await import("../db/schema.js"); + const { sql } = await import("drizzle-orm"); + + // Repo tasks and Job runs: `kind` is the run's RunKind. + const nonTerminalRuns = await db + .select({ id: workRuns.id, kind: workRuns.kind }) + .from(workRuns) + .where(sql`${workRuns.state} NOT IN ('completed')`); + + const nonTerminalReviews = await db + .select({ id: prReviews.id }) + .from(prReviews) + .where(sql`${prReviews.state} NOT IN ('cancelled')`); + + const livePersistentAgents = await db + .select({ id: persistentAgents.id }) + .from(persistentAgents) + .where(sql`${persistentAgents.state} NOT IN ('archived')`); + + logger.info( + { + tasks: nonTerminalRuns.filter((r) => r.kind === "repo").length, + runs: nonTerminalRuns.filter((r) => r.kind === "standalone").length, + reviews: nonTerminalReviews.length, + persistentAgents: livePersistentAgents.length, + }, + "reconcile.resync.sweep", + ); + + for (const r of nonTerminalRuns) { + await enqueueReconcile({ kind: r.kind, id: r.id }, { reason: "resync" }); + } + for (const r of nonTerminalReviews) { + await enqueueReconcile({ kind: "pr-review", id: r.id }, { reason: "resync" }); + } + for (const r of livePersistentAgents) { + await enqueueReconcile({ kind: "persistent-agent", id: r.id }, { reason: "resync" }); + } +} + export { reconcileQueue }; diff --git a/apps/api/src/workers/repo-cleanup-worker.test.ts b/apps/api/src/workers/repo-cleanup-worker.test.ts index 6c77f59f..615f0211 100644 --- a/apps/api/src/workers/repo-cleanup-worker.test.ts +++ b/apps/api/src/workers/repo-cleanup-worker.test.ts @@ -12,6 +12,12 @@ vi.mock("bullmq", () => ({ return { on: vi.fn() }; }), })); +vi.mock("../services/poller-lease.js", () => ({ + underPollerLease: (_name: string, fn: () => Promise) => fn(), +})); +vi.mock("../services/repeat-jobs.js", () => ({ + scheduleRepeat: vi.fn().mockResolvedValue(undefined), +})); // ── DB mock ──────────────────────────────────────────────────────────────── diff --git a/apps/api/src/workers/repo-cleanup-worker.ts b/apps/api/src/workers/repo-cleanup-worker.ts index 432f9430..75d24d0b 100644 --- a/apps/api/src/workers/repo-cleanup-worker.ts +++ b/apps/api/src/workers/repo-cleanup-worker.ts @@ -33,6 +33,8 @@ import { emitPodHealthEventLog } from "../telemetry/logs.js"; import { instrumentWorkerProcessor } from "../telemetry/instrument-worker.js"; import { getBullMQOptions } from "../services/redis-config.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; +import { underPollerLease } from "../services/poller-lease.js"; import { removeRunHome } from "../utils/harness-config.js"; import { sweepFinishedRunHomes } from "../services/run-home-sweep-service.js"; @@ -61,486 +63,476 @@ async function recordHealthEvent( emitPodHealthEventLog(eventType, podName ?? "unknown", repoUrl, message); } +/** + * Both ticks (the health check and the faster stall check) run the same + * sweep, one instance at a time (a poller lease); every write in it is a + * conditional update or a CAS transition, so an overlap is harmless. + */ export function startRepoCleanupWorker() { - repoCleanupQueue.add( - "health-check", - {}, - { - repeat: { - every: parseIntEnv("OPTIO_HEALTH_CHECK_INTERVAL", 60000), - }, - }, - ); + void scheduleRepeat( + repoCleanupQueue, + "repo-cleanup.health-check", + parseIntEnv("OPTIO_HEALTH_CHECK_INTERVAL", 60000), + ).catch(() => {}); // Dedicated stall-check cadence (30s) — more responsive than the 60s health-check - repoCleanupQueue.add( - "stall-check", - {}, - { - repeat: { - every: parseIntEnv("OPTIO_STALL_CHECK_INTERVAL", 30000), - }, - }, - ); + void scheduleRepeat( + repoCleanupQueue, + "repo-cleanup.stall-check", + parseIntEnv("OPTIO_STALL_CHECK_INTERVAL", 30000), + ).catch(() => {}); const worker = new Worker( "repo-cleanup", instrumentWorkerProcessor("repo-cleanup", async () => { - const rt = getRuntime(); - const pods = (await listRepoPods()).map((p) => ({ ...p, repoUrl: p.poolKey })); - - for (const pod of pods) { - // Skip pods without a K8s name. For bare pods, also skip "provisioning" - // (still being created). For StatefulSet pods, "provisioning" means - // recovering from a crash — we need to check if they've come back. - if (!pod.podName) continue; - if (pod.state === "provisioning" && pod.managedBy !== "statefulset") continue; - - try { - const status = await rt.status({ - id: pod.podId ?? pod.podName, - name: pod.podName, - }); - - if (status.state === "failed" || status.state === "unknown") { - const isOom = status.reason?.includes("OOMKilled") ?? false; - const eventType = isOom ? "oom_killed" : "crashed"; - const message = isOom - ? `Pod OOM killed: ${status.reason}` - : `Pod ${status.state}: ${status.reason ?? "unknown reason"}`; - - await recordHealthEvent(pod.id, pod.repoUrl, eventType, pod.podName, message); - - // Mark worktrees dirty for tasks on this pod and wake the - // reconciler — it observes pod.phase=error from the snapshot and - // fires the FAILED transition via decideRunning / decideProvisioning. - const activeTasks = await db - .select({ id: tasks.id, state: tasks.state }) - .from(tasks) - .where( - sql`${tasks.repoUrl} = ${pod.repoUrl} AND ${tasks.state} IN ('running', 'provisioning')`, - ); - - for (const task of activeTasks) { - try { - await updateWorktreeState(task.id, "dirty"); - await taskService.updateTaskResult(task.id, undefined, message); - await enqueueReconcile( - { kind: "repo", id: task.id }, - { reason: `pod_${eventType}` }, - ); - } catch {} - } + await underPollerLease("repo-cleanup", () => sweepPods()); + }), + { + ...bullmqOpts, + concurrency: 1, + }, + ); - if (pod.managedBy === "statefulset") { - // StatefulSet pods auto-restart (restartPolicy: Always). - // Mark as provisioning and wait for recovery rather than deleting. - await db - .update(agentPods) - .set({ - state: "provisioning", - activeCount: 0, - errorMessage: message, - updatedAt: new Date(), - }) - .where(eq(agentPods.id, pod.id)); - - logger.warn( - { repoUrl: pod.repoUrl, podName: pod.podName, eventType }, - "StatefulSet pod crashed — waiting for auto-restart", - ); - } else { - // Bare pod: delete and clear record for auto-recreation - await db - .update(agentPods) - .set({ - state: "error", - errorMessage: message, - updatedAt: new Date(), - }) - .where(eq(agentPods.id, pod.id)); - - try { - await deleteNetworkPolicy(pod.podName).catch(() => {}); - await rt.destroy({ id: pod.podId ?? pod.podName, name: pod.podName }); - } catch {} - await deletePod(pod.id); - await recordHealthEvent( - pod.id, - pod.repoUrl, - "restarted", - pod.podName, - "Pod record cleared for auto-recreation", - ); - - logger.warn( - { repoUrl: pod.repoUrl, podName: pod.podName, eventType }, - "Unhealthy pod cleaned up", - ); - } - } else if ( - status.state === "running" && - (pod.state === "error" || pod.state === "provisioning") - ) { - // Pod recovered (StatefulSet auto-restart, or unexpected recovery) - await db - .update(agentPods) - .set({ state: "ready", errorMessage: null, updatedAt: new Date() }) - .where(eq(agentPods.id, pod.id)); - await recordHealthEvent(pod.id, pod.repoUrl, "healthy", pod.podName, "Pod recovered"); - } - } catch (err) { - if (!isPodNotFound(err)) { - logger.warn({ err, podId: pod.id }, "Pod status unavailable; retaining workload"); - continue; - } - if (pod.managedBy === "statefulset") { - // StatefulSet pod not found — it may be restarting. Mark as provisioning. - await db - .update(agentPods) - .set({ - state: "provisioning", - activeCount: 0, - errorMessage: `Pod not found, may be restarting: ${describeError(err)}`, - updatedAt: new Date(), - }) - .where(eq(agentPods.id, pod.id)); - } else { - // Bare pod not found — clean up the record - if (pod.podName) { - await deleteNetworkPolicy(pod.podName).catch(() => {}); - } - await deletePod(pod.id); - await recordHealthEvent( - pod.id, - pod.repoUrl, - "crashed", - pod.podName, - `Pod not found in cluster: ${describeError(err)}`, - ); - } - } - } + worker.on("failed", (_job, err) => { + logger.error({ err }, "Health check failed"); + }); - // Clean up orphaned worktrees inside running pods - for (const pod of pods) { - if (!pod.podName || pod.state !== "ready") continue; + return worker; +} - try { - // List worktrees in the pod - const session = await rt.exec( - { id: pod.podId ?? pod.podName, name: pod.podName }, - ["bash", "-c", "ls /workspace/tasks/ 2>/dev/null || echo ''"], - { tty: false }, +/** One sweep: pod health, stalls, idle cleanup, expired sessions, finished run homes. */ +export async function sweepPods(): Promise { + const rt = getRuntime(); + const pods = (await listRepoPods()).map((p) => ({ ...p, repoUrl: p.poolKey })); + + for (const pod of pods) { + // Skip pods without a K8s name. For bare pods, also skip "provisioning" + // (still being created). For StatefulSet pods, "provisioning" means + // recovering from a crash — we need to check if they've come back. + if (!pod.podName) continue; + if (pod.state === "provisioning" && pod.managedBy !== "statefulset") continue; + + try { + const status = await rt.status({ + id: pod.podId ?? pod.podName, + name: pod.podName, + }); + + if (status.state === "failed" || status.state === "unknown") { + const isOom = status.reason?.includes("OOMKilled") ?? false; + const eventType = isOom ? "oom_killed" : "crashed"; + const message = isOom + ? `Pod OOM killed: ${status.reason}` + : `Pod ${status.state}: ${status.reason ?? "unknown reason"}`; + + await recordHealthEvent(pod.id, pod.repoUrl, eventType, pod.podName, message); + + // Mark worktrees dirty for tasks on this pod and wake the + // reconciler — it observes pod.phase=error from the snapshot and + // fires the FAILED transition via decideRunning / decideProvisioning. + const activeTasks = await db + .select({ id: tasks.id, state: tasks.state }) + .from(tasks) + .where( + sql`${tasks.repoUrl} = ${pod.repoUrl} AND ${tasks.state} IN ('running', 'provisioning')`, ); - let output = ""; - for await (const chunk of session.stdout as AsyncIterable) { - output += chunk.toString(); - } - session.close(); - - const worktreeIds = output.trim().split("\n").filter(Boolean); - if (worktreeIds.length === 0) continue; - - // State-aware worktree cleanup: - // - "active" / "preserved" worktrees: leave alone - // - "dirty" worktrees for failed tasks WITH retries remaining: leave for same-pod retry - // - "dirty" worktrees for failed tasks WITHOUT retries: remove after grace period - // - completed/cancelled tasks: remove after grace period - // - No task found: orphan, remove immediately - const WORKTREE_GRACE_MS = 120_000; // 2 minutes after terminal state before cleanup - for (const taskId of worktreeIds) { - const [task] = await db - .select({ - state: tasks.state, - updatedAt: tasks.updatedAt, - worktreeState: tasks.worktreeState, - retryCount: tasks.retryCount, - maxRetries: tasks.maxRetries, - }) - .from(tasks) - .where(eq(tasks.id, taskId)); - - if (!task) { - // No task found — orphan worktree, clean it up - try { - const cleanSession = await rt.exec( - { id: pod.podId ?? pod.podName, name: pod.podName }, - [ - "bash", - "-c", - `cd /workspace/repo && git worktree remove --force /workspace/tasks/${taskId} 2>/dev/null; rm -rf /workspace/tasks/${taskId}; ${removeRunHome(taskId)}`, - ], - { tty: false }, - ); - for await (const _ of cleanSession.stdout as AsyncIterable) { - } - cleanSession.close(); - await recordHealthEvent( - pod.id, - pod.repoUrl, - "orphan_cleaned", - pod.podName, - `Cleaned orphan worktree for task ${taskId}`, - ); - } catch {} - continue; - } - - // Preserve worktrees for active tasks and tasks awaiting resume - if (task.worktreeState === "active" || task.worktreeState === "preserved") continue; - if (["running", "provisioning", "pr_opened", "needs_attention"].includes(task.state)) { - continue; - } + for (const task of activeTasks) { + try { + await updateWorktreeState(task.id, "dirty"); + await taskService.updateTaskResult(task.id, undefined, message); + await enqueueReconcile({ kind: "repo", id: task.id }, { reason: `pod_${eventType}` }); + } catch {} + } - // For failed tasks with retries remaining, keep worktree for same-pod retry - if ( - task.state === "failed" && - task.worktreeState === "dirty" && - task.retryCount < task.maxRetries - ) { - continue; - } + if (pod.managedBy === "statefulset") { + // StatefulSet pods auto-restart (restartPolicy: Always). + // Mark as provisioning and wait for recovery rather than deleting. + await db + .update(agentPods) + .set({ + state: "provisioning", + activeCount: 0, + errorMessage: message, + updatedAt: new Date(), + }) + .where(eq(agentPods.id, pod.id)); + + logger.warn( + { repoUrl: pod.repoUrl, podName: pod.podName, eventType }, + "StatefulSet pod crashed — waiting for auto-restart", + ); + } else { + // Bare pod: delete and clear record for auto-recreation + await db + .update(agentPods) + .set({ + state: "error", + errorMessage: message, + updatedAt: new Date(), + }) + .where(eq(agentPods.id, pod.id)); + + try { + await deleteNetworkPolicy(pod.podName).catch(() => {}); + await rt.destroy({ id: pod.podId ?? pod.podName, name: pod.podName }); + } catch {} + await deletePod(pod.id); + await recordHealthEvent( + pod.id, + pod.repoUrl, + "restarted", + pod.podName, + "Pod record cleared for auto-recreation", + ); - // Terminal state or failed with no retries — clean up after grace period - const age = task.updatedAt ? Date.now() - new Date(task.updatedAt).getTime() : 0; - if (age > WORKTREE_GRACE_MS) { - try { - const cleanSession = await rt.exec( - { id: pod.podId ?? pod.podName, name: pod.podName }, - [ - "bash", - "-c", - `cd /workspace/repo && git worktree remove --force /workspace/tasks/${taskId} 2>/dev/null; rm -rf /workspace/tasks/${taskId}; ${removeRunHome(taskId)}`, - ], - { tty: false }, - ); - for await (const _ of cleanSession.stdout as AsyncIterable) { - } - cleanSession.close(); - - await updateWorktreeState(taskId, "removed"); - await recordHealthEvent( - pod.id, - pod.repoUrl, - "orphan_cleaned", - pod.podName, - `Cleaned worktree for task ${taskId} (state: ${task.state})`, - ); - } catch {} - } - } - } catch { - // Pod may not be accessible — skip + logger.warn( + { repoUrl: pod.repoUrl, podName: pod.podName, eventType }, + "Unhealthy pod cleaned up", + ); } + } else if ( + status.state === "running" && + (pod.state === "error" || pod.state === "provisioning") + ) { + // Pod recovered (StatefulSet auto-restart, or unexpected recovery) + await db + .update(agentPods) + .set({ state: "ready", errorMessage: null, updatedAt: new Date() }) + .where(eq(agentPods.id, pod.id)); + await recordHealthEvent(pod.id, pod.repoUrl, "healthy", pod.podName, "Pod recovered"); } - - // ── Soft stall detection ────────────────────────────────────────────── - // Flag running tasks that have been silent beyond their threshold. - // This does NOT fail/retry the task — it's an observable warning only. - try { - const globalThreshold = parseInt( - process.env.OPTIO_STALL_THRESHOLD_MS ?? String(DEFAULT_STALL_THRESHOLD_MS), - 10, + } catch (err) { + if (!isPodNotFound(err)) { + logger.warn({ err, podId: pod.id }, "Pod status unavailable; retaining workload"); + continue; + } + if (pod.managedBy === "statefulset") { + // StatefulSet pod not found — it may be restarting. Mark as provisioning. + await db + .update(agentPods) + .set({ + state: "provisioning", + activeCount: 0, + errorMessage: `Pod not found, may be restarting: ${describeError(err)}`, + updatedAt: new Date(), + }) + .where(eq(agentPods.id, pod.id)); + } else { + // Bare pod not found — clean up the record + if (pod.podName) { + await deleteNetworkPolicy(pod.podName).catch(() => {}); + } + await deletePod(pod.id); + await recordHealthEvent( + pod.id, + pod.repoUrl, + "crashed", + pod.podName, + `Pod not found in cluster: ${describeError(err)}`, ); - - // Fetch all running tasks with lastActivityAt set - const runningTasks = await db + } + } + } + + // Clean up orphaned worktrees inside running pods + for (const pod of pods) { + if (!pod.podName || pod.state !== "ready") continue; + + try { + // List worktrees in the pod + const session = await rt.exec( + { id: pod.podId ?? pod.podName, name: pod.podName }, + ["bash", "-c", "ls /workspace/tasks/ 2>/dev/null || echo ''"], + { tty: false }, + ); + + let output = ""; + for await (const chunk of session.stdout as AsyncIterable) { + output += chunk.toString(); + } + session.close(); + + const worktreeIds = output.trim().split("\n").filter(Boolean); + if (worktreeIds.length === 0) continue; + + // State-aware worktree cleanup: + // - "active" / "preserved" worktrees: leave alone + // - "dirty" worktrees for failed tasks WITH retries remaining: leave for same-pod retry + // - "dirty" worktrees for failed tasks WITHOUT retries: remove after grace period + // - completed/cancelled tasks: remove after grace period + // - No task found: orphan, remove immediately + const WORKTREE_GRACE_MS = 120_000; // 2 minutes after terminal state before cleanup + for (const taskId of worktreeIds) { + const [task] = await db .select({ - id: tasks.id, - repoUrl: tasks.repoUrl, - workspaceId: tasks.workspaceId, - lastActivityAt: tasks.lastActivityAt, - activitySubstate: tasks.activitySubstate, + state: tasks.state, + updatedAt: tasks.updatedAt, + worktreeState: tasks.worktreeState, + retryCount: tasks.retryCount, + maxRetries: tasks.maxRetries, }) .from(tasks) - .where(sql`${tasks.state} = 'running' AND ${tasks.lastActivityAt} IS NOT NULL`); - - // Cache repo configs to avoid repeated queries - const repoConfigCache = new Map(); - - for (const task of runningTasks) { - const now = Date.now(); - const lastActivity = new Date(task.lastActivityAt!).getTime(); - const silentForMs = now - lastActivity; - - // Get per-repo threshold - let repoConfig = repoConfigCache.get(task.repoUrl); - if (repoConfig === undefined) { - const [rc] = await db.select().from(repos).where(eq(repos.repoUrl, task.repoUrl)); - repoConfig = rc ?? null; - repoConfigCache.set(task.repoUrl, repoConfig); - } - const threshold = taskService.getStallThresholdForRepo(repoConfig); - - if (silentForMs >= threshold && task.activitySubstate !== "stalled") { - // Mark as stalled - await db - .update(tasks) - .set({ activitySubstate: "stalled" }) - .where(eq(tasks.id, task.id)); - - // Get last log summary for the stall event - const lastLogSummary = await taskService.getLastLogSummary(task.id); - - await publishEvent({ - type: "task:stalled", - taskId: task.id, - lastActivityAt: task.lastActivityAt!.toISOString(), - silentForMs, - lastLogSummary, - timestamp: new Date().toISOString(), - }); - - logger.info( - { taskId: task.id, silentForMs, threshold }, - "Task flagged as stalled (soft)", + .where(eq(tasks.id, taskId)); + + if (!task) { + // No task found — orphan worktree, clean it up + try { + const cleanSession = await rt.exec( + { id: pod.podId ?? pod.podName, name: pod.podName }, + [ + "bash", + "-c", + `cd /workspace/repo && git worktree remove --force /workspace/tasks/${taskId} 2>/dev/null; rm -rf /workspace/tasks/${taskId}; ${removeRunHome(taskId)}`, + ], + { tty: false }, ); - } else if (silentForMs < threshold && task.activitySubstate === "stalled") { - // Recovered — activity flush in task-worker already handles this, - // but catch edge cases here too - await db - .update(tasks) - .set({ activitySubstate: "recovered" }) - .where(eq(tasks.id, task.id)); - - await publishEvent({ - type: "task:recovered", - taskId: task.id, - silentWasMs: silentForMs, - timestamp: new Date().toISOString(), - }); - - logger.info({ taskId: task.id }, "Task recovered from stall"); - } - } - } catch (err) { - logger.warn({ err }, "Soft stall detection pass failed"); - } - - // Detect stale running/provisioning tasks (agent exec died without updating state). - // - // TODO(reconciler): move this block into the reconciler. It needs: - // 1. Stale-retry counter exposed in the snapshot (count of - // auto_retry_stale events) so the decision function can cap retries. - // 2. A new side-effect action in the executor that calls - // killOrphanedAgentInPod + updateWorktreeState before the - // FAILED→QUEUED transition. - // 3. An "escalate to NEEDS_ATTENTION on cleanup failure" branch in - // decideRunning, which today only emits a plain FAILED transition. - // Until then, this loop owns stall recovery for tasks whose updatedAt - // hasn't moved in OPTIO_STALE_TASK_MS — the reconciler's heartbeat-based - // decideRunning stall handler is a safety net, not a replacement. - const STALE_TASK_MS = parseIntEnv("OPTIO_STALE_TASK_MS", 600000); // 10 min - const staleTasks = await db - .select() - .from(tasks) - .where( - sql`${tasks.state} IN ('running', 'provisioning') - AND ${tasks.updatedAt} < NOW() - INTERVAL '1 millisecond' * ${STALE_TASK_MS}`, - ); - - for (const task of staleTasks) { - try { - // Silence cannot prove that side effects did not happen. Preserve - // the checkout and require explicit recovery instead of replaying. - await updateWorktreeState(task.id, "preserved"); - await taskService.transitionTask( - task.id, - task.state === "running" ? TaskState.NEEDS_ATTENTION : TaskState.FAILED, - "stale_recovery_required", - "Execution stopped reporting progress. Workspace preserved; inspect the process and its last results before resuming.", - ); - } catch (err) { - logger.warn({ err, taskId: task.id }, "Failed to mark stale task for recovery"); + for await (const _ of cleanSession.stdout as AsyncIterable) { + } + cleanSession.close(); + await recordHealthEvent( + pod.id, + pod.repoUrl, + "orphan_cleaned", + pod.podName, + `Cleaned orphan worktree for task ${taskId}`, + ); + } catch {} + continue; } - } - - // Reconcile each repo pod's active count to catch any drift - const reconciled = await reconcileActiveTaskCounts(); - if (reconciled > 0) { - logger.info({ reconciled }, "Reconciled repo pod activeTaskCounts"); - } - // Clean up idle pods (existing behavior) - const cleaned = await cleanupIdleRepoPods(); - if (cleaned > 0) { - logger.info({ cleaned }, "Cleaned up idle repo pods"); - } - - // Clean up idle workflow pods, after repairing their counts the same way - try { - const runCountsFixed = await reconcileActiveRunCounts(); - if (runCountsFixed > 0) { - logger.info({ runCountsFixed }, "Reconciled workflow pod active counts"); - } - const workflowCleaned = await cleanupIdleWorkflowPods(); - if (workflowCleaned > 0) { - logger.info({ workflowCleaned }, "Cleaned up idle workflow pods"); + // Preserve worktrees for active tasks and tasks awaiting resume + if (task.worktreeState === "active" || task.worktreeState === "preserved") continue; + if (["running", "provisioning", "pr_opened", "needs_attention"].includes(task.state)) { + continue; } - } catch (err) { - logger.warn({ err }, "Failed to clean up idle workflow pods"); - } - // Clean up persistent agent pods past their warm window. - try { - const paCleaned = await cleanupIdlePersistentAgentPods(); - if (paCleaned > 0) { - logger.info({ paCleaned }, "Cleaned up idle persistent agent pods"); + // For failed tasks with retries remaining, keep worktree for same-pod retry + if ( + task.state === "failed" && + task.worktreeState === "dirty" && + task.retryCount < task.maxRetries + ) { + continue; } - } catch (err) { - logger.warn({ err }, "Failed to clean up idle persistent agent pods"); - } - // Run homes of finished runs (the run protocol's directories in the - // pods): the attached worker removes them; this is the backstop. - try { - const homes = await sweepFinishedRunHomes(); - if (homes > 0) logger.info({ homes }, "Removed finished run homes"); - } catch (err) { - logger.warn({ err }, "Failed to sweep run homes"); - } + // Terminal state or failed with no retries — clean up after grace period + const age = task.updatedAt ? Date.now() - new Date(task.updatedAt).getTime() : 0; + if (age > WORKTREE_GRACE_MS) { + try { + const cleanSession = await rt.exec( + { id: pod.podId ?? pod.podName, name: pod.podName }, + [ + "bash", + "-c", + `cd /workspace/repo && git worktree remove --force /workspace/tasks/${taskId} 2>/dev/null; rm -rf /workspace/tasks/${taskId}; ${removeRunHome(taskId)}`, + ], + { tty: false }, + ); + for await (const _ of cleanSession.stdout as AsyncIterable) { + } + cleanSession.close(); - // Detect zombie workflow_runs (running but pod terminated/gone) - try { - const zombieRuns = await cleanupZombieWorkflowRuns(); - if (zombieRuns > 0) { - logger.info({ zombieRuns }, "Cleaned up zombie workflow runs"); + await updateWorktreeState(taskId, "removed"); + await recordHealthEvent( + pod.id, + pod.repoUrl, + "orphan_cleaned", + pod.podName, + `Cleaned worktree for task ${taskId} (state: ${task.state})`, + ); + } catch {} } - } catch (err) { - logger.warn({ err }, "Failed to clean up zombie workflow runs"); } - - // Detect orphaned repo tasks (running but pod record gone) - try { - const orphanedTasks = await cleanupOrphanedRepoTasks(); - if (orphanedTasks > 0) { - logger.info({ orphanedTasks }, "Cleaned up orphaned repo tasks"); - } - } catch (err) { - logger.warn({ err }, "Failed to clean up orphaned repo tasks"); + } catch { + // Pod may not be accessible — skip + } + } + + // ── Soft stall detection ────────────────────────────────────────────── + // Flag running tasks that have been silent beyond their threshold. + // This does NOT fail/retry the task — it's an observable warning only. + try { + const globalThreshold = parseInt( + process.env.OPTIO_STALL_THRESHOLD_MS ?? String(DEFAULT_STALL_THRESHOLD_MS), + 10, + ); + + // Fetch all running tasks with lastActivityAt set + const runningTasks = await db + .select({ + id: tasks.id, + repoUrl: tasks.repoUrl, + workspaceId: tasks.workspaceId, + lastActivityAt: tasks.lastActivityAt, + activitySubstate: tasks.activitySubstate, + }) + .from(tasks) + .where(sql`${tasks.state} = 'running' AND ${tasks.lastActivityAt} IS NOT NULL`); + + // Cache repo configs to avoid repeated queries + const repoConfigCache = new Map(); + + for (const task of runningTasks) { + const now = Date.now(); + const lastActivity = new Date(task.lastActivityAt!).getTime(); + const silentForMs = now - lastActivity; + + // Get per-repo threshold + let repoConfig = repoConfigCache.get(task.repoUrl); + if (repoConfig === undefined) { + const [rc] = await db.select().from(repos).where(eq(repos.repoUrl, task.repoUrl)); + repoConfig = rc ?? null; + repoConfigCache.set(task.repoUrl, repoConfig); } - - // Clean up expired auth sessions - try { - const expiredSessions = await cleanupExpiredSessions(); - if (expiredSessions > 0) { - logger.info({ expiredSessions }, "Cleaned up expired sessions"); - } - } catch (err) { - logger.warn({ err }, "Failed to clean up expired sessions"); + const threshold = taskService.getStallThresholdForRepo(repoConfig); + + if (silentForMs >= threshold && task.activitySubstate !== "stalled") { + // Mark as stalled + await db.update(tasks).set({ activitySubstate: "stalled" }).where(eq(tasks.id, task.id)); + + // Get last log summary for the stall event + const lastLogSummary = await taskService.getLastLogSummary(task.id); + + await publishEvent({ + type: "task:stalled", + taskId: task.id, + lastActivityAt: task.lastActivityAt!.toISOString(), + silentForMs, + lastLogSummary, + timestamp: new Date().toISOString(), + }); + + logger.info({ taskId: task.id, silentForMs, threshold }, "Task flagged as stalled (soft)"); + } else if (silentForMs < threshold && task.activitySubstate === "stalled") { + // Recovered — activity flush in task-worker already handles this, + // but catch edge cases here too + await db.update(tasks).set({ activitySubstate: "recovered" }).where(eq(tasks.id, task.id)); + + await publishEvent({ + type: "task:recovered", + taskId: task.id, + silentWasMs: silentForMs, + timestamp: new Date().toISOString(), + }); + + logger.info({ taskId: task.id }, "Task recovered from stall"); } - }), - { - ...bullmqOpts, - concurrency: 1, - }, - ); - - worker.on("failed", (_job, err) => { - logger.error({ err }, "Health check failed"); - }); - - return worker; + } + } catch (err) { + logger.warn({ err }, "Soft stall detection pass failed"); + } + + // Detect stale running/provisioning tasks (agent exec died without updating state). + // + // TODO(reconciler): move this block into the reconciler. It needs: + // 1. Stale-retry counter exposed in the snapshot (count of + // auto_retry_stale events) so the decision function can cap retries. + // 2. A new side-effect action in the executor that calls + // killOrphanedAgentInPod + updateWorktreeState before the + // FAILED→QUEUED transition. + // 3. An "escalate to NEEDS_ATTENTION on cleanup failure" branch in + // decideRunning, which today only emits a plain FAILED transition. + // Until then, this loop owns stall recovery for tasks whose updatedAt + // hasn't moved in OPTIO_STALE_TASK_MS — the reconciler's heartbeat-based + // decideRunning stall handler is a safety net, not a replacement. + const STALE_TASK_MS = parseIntEnv("OPTIO_STALE_TASK_MS", 600000); // 10 min + const staleTasks = await db + .select() + .from(tasks) + .where( + sql`${tasks.state} IN ('running', 'provisioning') + AND ${tasks.updatedAt} < NOW() - INTERVAL '1 millisecond' * ${STALE_TASK_MS}`, + ); + + for (const task of staleTasks) { + try { + // Silence cannot prove that side effects did not happen. Preserve + // the checkout and require explicit recovery instead of replaying. + await updateWorktreeState(task.id, "preserved"); + await taskService.transitionTask( + task.id, + task.state === "running" ? TaskState.NEEDS_ATTENTION : TaskState.FAILED, + "stale_recovery_required", + "Execution stopped reporting progress. Workspace preserved; inspect the process and its last results before resuming.", + ); + } catch (err) { + logger.warn({ err, taskId: task.id }, "Failed to mark stale task for recovery"); + } + } + + // Reconcile each repo pod's active count to catch any drift + const reconciled = await reconcileActiveTaskCounts(); + if (reconciled > 0) { + logger.info({ reconciled }, "Reconciled repo pod activeTaskCounts"); + } + + // Clean up idle pods (existing behavior) + const cleaned = await cleanupIdleRepoPods(); + if (cleaned > 0) { + logger.info({ cleaned }, "Cleaned up idle repo pods"); + } + + // Clean up idle workflow pods, after repairing their counts the same way + try { + const runCountsFixed = await reconcileActiveRunCounts(); + if (runCountsFixed > 0) { + logger.info({ runCountsFixed }, "Reconciled workflow pod active counts"); + } + const workflowCleaned = await cleanupIdleWorkflowPods(); + if (workflowCleaned > 0) { + logger.info({ workflowCleaned }, "Cleaned up idle workflow pods"); + } + } catch (err) { + logger.warn({ err }, "Failed to clean up idle workflow pods"); + } + + // Clean up persistent agent pods past their warm window. + try { + const paCleaned = await cleanupIdlePersistentAgentPods(); + if (paCleaned > 0) { + logger.info({ paCleaned }, "Cleaned up idle persistent agent pods"); + } + } catch (err) { + logger.warn({ err }, "Failed to clean up idle persistent agent pods"); + } + + // Run homes of finished runs (the run protocol's directories in the + // pods): the attached worker removes them; this is the backstop. + try { + const homes = await sweepFinishedRunHomes(); + if (homes > 0) logger.info({ homes }, "Removed finished run homes"); + } catch (err) { + logger.warn({ err }, "Failed to sweep run homes"); + } + + // Detect zombie workflow_runs (running but pod terminated/gone) + try { + const zombieRuns = await cleanupZombieWorkflowRuns(); + if (zombieRuns > 0) { + logger.info({ zombieRuns }, "Cleaned up zombie workflow runs"); + } + } catch (err) { + logger.warn({ err }, "Failed to clean up zombie workflow runs"); + } + + // Detect orphaned repo tasks (running but pod record gone) + try { + const orphanedTasks = await cleanupOrphanedRepoTasks(); + if (orphanedTasks > 0) { + logger.info({ orphanedTasks }, "Cleaned up orphaned repo tasks"); + } + } catch (err) { + logger.warn({ err }, "Failed to clean up orphaned repo tasks"); + } + + // Clean up expired auth sessions + try { + const expiredSessions = await cleanupExpiredSessions(); + if (expiredSessions > 0) { + logger.info({ expiredSessions }, "Cleaned up expired sessions"); + } + } catch (err) { + logger.warn({ err }, "Failed to clean up expired sessions"); + } } diff --git a/apps/api/src/workers/skill-sync-worker.int.test.ts b/apps/api/src/workers/skill-sync-worker.int.test.ts index 36240178..fe76a1f0 100644 --- a/apps/api/src/workers/skill-sync-worker.int.test.ts +++ b/apps/api/src/workers/skill-sync-worker.int.test.ts @@ -30,6 +30,22 @@ vi.mock("bullmq", () => ({ }, })); +// A hook that runs just before a sync records its result: the moment a +// PATCH can land while the sync of the old ref is in flight. +const beforeRecord = vi.hoisted(() => ({ hook: null as null | (() => Promise) })); +vi.mock("../services/installed-skill-service.js", async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + recordSyncResult: async (...args: Parameters) => { + const hook = beforeRecord.hook; + beforeRecord.hook = null; + if (hook) await hook(); + return real.recordSyncResult(...args); + }, + }; +}); + const run = promisify(execFile); let root: string; let source: string; @@ -182,6 +198,46 @@ describe("skill sync into the database", () => { expect(second.lastSyncedAt?.getTime()).toBe(first.lastSyncedAt?.getTime()); }); + it("discards a sync of the old ref when the ref changes meanwhile; the next sync-due takes the new one", async () => { + const { syncOne, dueSkillIds, readInstalledSkillFiles } = + await import("./skill-sync-worker.js"); + const { updateInstalledSkill } = await import("../services/installed-skill-service.js"); + // A branch whose skill differs from main's. + await git(["checkout", "-q", "-b", "next"]); + await fs.writeFile(path.join(source, "skills", "greet", "next.md"), "only on next\n"); + await git(["add", "-A"]); + await git(["commit", "-q", "-m", "next"]); + await git(["checkout", "-q", "main"]); + + const skill = await insertSkill(); + // The sync of main is cloned and read; before it records, the ref moves. + beforeRecord.hook = async () => { + await updateInstalledSkill(skill.id, { ref: "next" }); + }; + await syncOne(skill.id); + + const [after] = await db.select().from(installedSkills).where(eq(installedSkills.id, skill.id)); + expect(after.ref).toBe("next"); + expect(after.resolvedSha).toBeNull(); // nothing of main's sync landed + const files = await db + .select() + .from(installedSkillFiles) + .where(eq(installedSkillFiles.skillId, skill.id)); + expect(files).toEqual([]); + expect(await dueSkillIds()).toContain(skill.id); + + // The periodic pass syncs the new ref. + await syncOne(skill.id); + const [synced] = await db + .select() + .from(installedSkills) + .where(eq(installedSkills.id, skill.id)); + expect(synced.resolvedSha).toMatch(/^[0-9a-f]{40}$/); + const read = await readInstalledSkillFiles(skill.id, synced.resolvedSha); + expect(read.map((f) => f.relativePath)).toContain("next.md"); + expect(await dueSkillIds()).not.toContain(skill.id); + }); + it("two syncs of one skill at once both succeed (the writes upsert)", async () => { const { syncOne, readInstalledSkillFiles } = await import("./skill-sync-worker.js"); const skill = await insertSkill(); diff --git a/apps/api/src/workers/skill-sync-worker.test.ts b/apps/api/src/workers/skill-sync-worker.test.ts index fc4e8945..0235c4f5 100644 --- a/apps/api/src/workers/skill-sync-worker.test.ts +++ b/apps/api/src/workers/skill-sync-worker.test.ts @@ -26,7 +26,37 @@ vi.mock("../services/redis-config.js", () => ({ })); vi.mock("../db/client.js", () => ({ db: {} })); -import { parseFrontmatter, readSkillDir } from "./skill-sync-worker.js"; +const leaseFree = { value: true }; +vi.mock("../services/poller-lease.js", () => ({ + underPollerLease: async (_name: string, fn: () => Promise) => + leaseFree.value ? fn() : null, +})); + +import { + SKILL_SYNC_JOB_OPTIONS, + SkillSyncBusyError, + parseFrontmatter, + processSkillSyncJob, + readSkillDir, +} from "./skill-sync-worker.js"; + +describe("sync-one under another instance's lease", () => { + it("throws a retryable busy error instead of dropping the sync", async () => { + leaseFree.value = false; + try { + await expect( + processSkillSyncJob({ name: "sync-one", data: { id: "skill-1" } }), + ).rejects.toBeInstanceOf(SkillSyncBusyError); + } finally { + leaseFree.value = true; + } + }); + + it("every job on the queue retries with exponential backoff", () => { + expect(SKILL_SYNC_JOB_OPTIONS.attempts).toBeGreaterThan(1); + expect(SKILL_SYNC_JOB_OPTIONS.backoff.type).toBe("exponential"); + }); +}); describe("a cloned skill directory", () => { const baseDir = path.join(tmpdir(), `optio-skill-test-${Date.now()}`); diff --git a/apps/api/src/workers/skill-sync-worker.ts b/apps/api/src/workers/skill-sync-worker.ts index 999fe767..ff14011e 100644 --- a/apps/api/src/workers/skill-sync-worker.ts +++ b/apps/api/src/workers/skill-sync-worker.ts @@ -32,6 +32,8 @@ import { parseIntEnv } from "@optio/shared"; import type { InstalledSkillManifest } from "@optio/shared"; import { logger } from "../logger.js"; import { getBullMQOptions } from "../services/redis-config.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; +import { underPollerLease } from "../services/poller-lease.js"; import { db } from "../db/client.js"; import { installedSkillFiles, installedSkills } from "../db/schema.js"; import { and, eq, isNotNull, isNull, notExists, or, sql } from "drizzle-orm"; @@ -44,7 +46,43 @@ import { const execFileP = promisify(execFile); const bullmqOpts = getBullMQOptions(); -export const skillSyncQueue = new Queue("skill-sync", { ...bullmqOpts }); +/** + * Every job on the queue retries with backoff: a sync-one whose skill + * another instance is syncing right now throws `SkillSyncBusyError` and + * comes back later instead of being dropped (re-adding it from inside the + * job would be ignored while the job's own id is still taken). + */ +export const SKILL_SYNC_JOB_OPTIONS = { + attempts: 8, + backoff: { type: "exponential", delay: 5_000 }, +} as const; + +export const skillSyncQueue = new Queue("skill-sync", { + ...bullmqOpts, + defaultJobOptions: SKILL_SYNC_JOB_OPTIONS, +}); + +/** A sync-one found its skill's lease held: retried by BullMQ with backoff. */ +export class SkillSyncBusyError extends Error { + constructor(public readonly skillId: string) { + super(`skill ${skillId} is being synced by another instance; retrying`); + this.name = "SkillSyncBusyError"; + } +} + +/** One job's work (the worker's processor; exported for tests). */ +export async function processSkillSyncJob(job: { name: string; data: unknown }): Promise { + if (job.name === "sync-due") { + await underPollerLease("skill-sync", () => syncDue()); + } else if (job.name === "sync-one") { + const { id } = job.data as { id: string }; + const ran = await underPollerLease(`skill-sync:${id}`, async () => { + await syncOne(id); + return true; + }); + if (ran === null) throw new SkillSyncBusyError(id); + } +} function scratchDir(): string { return process.env.OPTIO_SKILLS_SCRATCH_DIR ?? path.join(tmpdir(), "optio-skills"); @@ -55,16 +93,13 @@ const SYNC_INTERVAL_MS = parseIntEnv("OPTIO_SKILL_SYNC_INTERVAL", 300_000); // 5 const MAX_SKILL_BYTES = parseIntEnv("OPTIO_SKILL_MAX_BYTES", 1_048_576); // 1 MiB export function startSkillSyncWorker() { - // Periodic sweep for rows that need a sync. - skillSyncQueue.add( - "sync-due", - {}, - { - repeat: { every: SYNC_INTERVAL_MS }, - removeOnComplete: { count: 50 }, - removeOnFail: { count: 50 }, - }, - ); + // Periodic sweep for rows that need a sync — one instance at a time; a + // one-skill sync holds its own lease so two instances never clone the + // same skill at once (the one that finds it held retries later). + void scheduleRepeat(skillSyncQueue, "skill-sync.sync-due", SYNC_INTERVAL_MS, { + removeOnComplete: { count: 50 }, + removeOnFail: { count: 50 }, + }).catch(() => {}); // And one pass now: a skill synced to the former cache volume before the // upgrade (a commit, no files in the database) is back within seconds. @@ -72,20 +107,16 @@ export function startSkillSyncWorker() { .add("sync-due", {}, { removeOnComplete: true, removeOnFail: { count: 50 } }) .catch((err) => logger.warn({ err }, "skill sync: could not enqueue the boot pass")); - const worker = new Worker( - "skill-sync", - async (job) => { - if (job.name === "sync-due") { - await syncDue(); - } else if (job.name === "sync-one") { - const { id } = job.data as { id: string }; - await syncOne(id); - } - }, - { ...bullmqOpts, concurrency: 1 }, - ); + const worker = new Worker("skill-sync", (job) => processSkillSyncJob(job), { + ...bullmqOpts, + concurrency: 1, + }); worker.on("failed", (job, err) => { + if (err instanceof SkillSyncBusyError) { + logger.debug({ skillId: err.skillId, attempt: job?.attemptsMade }, err.message); + return; + } logger.error({ err, jobName: job?.name, jobId: job?.id }, "skill-sync job failed"); }); @@ -173,6 +204,8 @@ export async function syncOne(id: string): Promise { return; } + // What this sync resolves; its result lands only if the row still says so. + const synced = { ref: row.ref, subpath: row.subpath }; let clone: string | null = null; try { const sha = await resolveRefToSha(row.sourceUrl, row.ref); @@ -191,14 +224,22 @@ export async function syncOne(id: string): Promise { const inspected = await readSkillDir(subpathDir, MAX_SKILL_BYTES); const { files } = inspected; - await recordSyncResult(id, { - ok: true, - resolvedSha: sha, - manifest: inspected.manifest, - hasExecutableFiles: inspected.hasExecutableFiles, - totalSizeBytes: inspected.totalSizeBytes, - files, - }); + const recorded = await recordSyncResult( + id, + { + ok: true, + resolvedSha: sha, + manifest: inspected.manifest, + hasExecutableFiles: inspected.hasExecutableFiles, + totalSizeBytes: inspected.totalSizeBytes, + files, + }, + synced, + ); + if (!recorded) { + log.info({ sha, ref: row.ref }, "skill's ref changed during the sync; result discarded"); + return; + } log.info( { sha, fileCount: files.length, bytes: inspected.totalSizeBytes }, "installed skill synced", @@ -206,7 +247,7 @@ export async function syncOne(id: string): Promise { } catch (err) { const msg = err instanceof Error ? err.message : String(err); log.error({ err }, "installed skill sync failed"); - await recordSyncResult(id, { ok: false, error: msg }); + await recordSyncResult(id, { ok: false, error: msg }, synced); } finally { if (clone) await fs.rm(path.dirname(clone), { recursive: true, force: true }).catch(() => {}); } diff --git a/apps/api/src/workers/sweep-worker.test.ts b/apps/api/src/workers/sweep-worker.test.ts new file mode 100644 index 00000000..4402c642 --- /dev/null +++ b/apps/api/src/workers/sweep-worker.test.ts @@ -0,0 +1,83 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("bullmq", () => ({ + Queue: class { + upsertJobScheduler = vi.fn().mockResolvedValue({}); + }, + Worker: class { + on = vi.fn(); + close = vi.fn().mockResolvedValue(undefined); + }, +})); +vi.mock("../services/redis-config.js", () => ({ + getBullMQOptions: () => ({ connection: {}, prefix: "bull" }), +})); +const mockExecute = vi.fn(); +vi.mock("../db/client.js", () => ({ db: { execute: (...a: unknown[]) => mockExecute(...a) } })); +const warn = vi.fn(); +const info = vi.fn(); +vi.mock("../logger.js", () => ({ + logger: { + info: (...a: unknown[]) => info(...a), + warn: (...a: unknown[]) => warn(...a), + error: vi.fn(), + debug: vi.fn(), + }, +})); + +import { registerSweep, registeredSweeps, runSweeps } from "./sweep-worker.js"; + +describe("sweep-worker", () => { + beforeEach(() => { + mockExecute.mockReset().mockResolvedValue([]); + warn.mockReset(); + info.mockReset(); + }); + + it("registers the three coordination sweeps at import", () => { + expect(registeredSweeps()).toEqual( + expect.arrayContaining(["inbound-webhook-deliveries", "ws-upgrade-tokens", "leases"]), + ); + }); + + it("runs every sweep, reports counts, and a failing sweep does not stop the others", async () => { + const order: string[] = []; + registerSweep("a", async () => { + order.push("a"); + return 2; + }); + registerSweep("b", async () => { + order.push("b"); + throw new Error("boom"); + }); + registerSweep("c", async () => { + order.push("c"); + return 0; + }); + const removed = await runSweeps(); + expect(order).toEqual(expect.arrayContaining(["a", "b", "c"])); + expect(removed.a).toBe(2); + expect(removed.c).toBe(0); + expect(removed).not.toHaveProperty("b"); + expect(warn).toHaveBeenCalledWith( + expect.objectContaining({ sweep: "b" }), + "Housekeeping sweep failed", + ); + expect(info).toHaveBeenCalledWith( + expect.objectContaining({ sweep: "a", removed: 2 }), + "Housekeeping sweep", + ); + }); + + it("the built-in sweeps delete through the database", async () => { + mockExecute.mockResolvedValue([{ x: 1 }, { x: 2 }]); + const removed = await runSweeps(); + expect(removed["inbound-webhook-deliveries"]).toBe(2); + expect(removed["ws-upgrade-tokens"]).toBe(2); + expect(removed.leases).toBe(2); + const statements = mockExecute.mock.calls.map((c) => JSON.stringify(c[0])); + expect(statements.some((s) => s.includes("inbound_webhook_deliveries"))).toBe(true); + expect(statements.some((s) => s.includes("ws_upgrade_tokens"))).toBe(true); + expect(statements.some((s) => s.includes('DELETE FROM \\"leases\\"'))).toBe(true); + }); +}); diff --git a/apps/api/src/workers/sweep-worker.ts b/apps/api/src/workers/sweep-worker.ts new file mode 100644 index 00000000..585c0e37 --- /dev/null +++ b/apps/api/src/workers/sweep-worker.ts @@ -0,0 +1,92 @@ +/** + * Housekeeping (docs/plans/scale-out.md §3): one 30-second tick, on one + * instance at a time, that runs every registered sweep — the rows that + * coordination leaves behind and that nothing else deletes: inbound + * webhook deliveries older than a day, expired WebSocket upgrade tokens, + * leases that expired a day ago, and whatever another module registers + * (`registerSweep`). + */ +import { Queue, Worker } from "bullmq"; +import { sql } from "drizzle-orm"; +import { parseIntEnv } from "@optio/shared"; +import { db } from "../db/client.js"; +import { logger } from "../logger.js"; +import { sweepLeases } from "../services/lease-service.js"; +import { underPollerLease } from "../services/poller-lease.js"; +import { getBullMQOptions } from "../services/redis-config.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; + +const bullmqOpts = getBullMQOptions(); + +export const housekeepingQueue = new Queue("housekeeping", { ...bullmqOpts }); + +export type Sweep = () => Promise; + +const sweeps = new Map(); + +/** + * Adds a sweep to the housekeeping tick. A sweep returns how many rows it + * removed (logged when non-zero); a sweep that throws is logged and the + * others still run. Registering a name twice replaces the sweep. + */ +export function registerSweep(name: string, sweep: Sweep): void { + sweeps.set(name, sweep); +} + +/** The registered sweeps, in registration order (for tests). */ +export function registeredSweeps(): string[] { + return [...sweeps.keys()]; +} + +/** Runs every registered sweep once (the tick's body; exported for tests). */ +export async function runSweeps(): Promise> { + const removed: Record = {}; + for (const [name, sweep] of sweeps) { + try { + const n = await sweep(); + if (typeof n === "number") { + removed[name] = n; + if (n > 0) logger.info({ sweep: name, removed: n }, "Housekeeping sweep"); + } + } catch (err) { + logger.warn({ err, sweep: name }, "Housekeeping sweep failed"); + } + } + return removed; +} + +const DELIVERIES_RETENTION = "24 hours"; + +registerSweep("inbound-webhook-deliveries", async () => { + const rows = await db.execute<{ source: string }>(sql` + DELETE FROM "inbound_webhook_deliveries" + WHERE "received_at" < now() - ${DELIVERIES_RETENTION}::interval + RETURNING "source"`); + return rows.length; +}); + +registerSweep("ws-upgrade-tokens", async () => { + const rows = await db.execute<{ token_hash: string }>(sql` + DELETE FROM "ws_upgrade_tokens" WHERE "expires_at" < now() RETURNING "token_hash"`); + return rows.length; +}); + +registerSweep("leases", () => sweepLeases()); + +export function startSweepWorker(): Worker { + const intervalMs = parseIntEnv("OPTIO_HOUSEKEEPING_INTERVAL", 30_000); + void scheduleRepeat(housekeepingQueue, "housekeeping.sweep", intervalMs, { + removeOnComplete: { count: 20 }, + removeOnFail: { count: 20 }, + }).catch(() => {}); + + const worker = new Worker( + "housekeeping", + async () => { + await underPollerLease("housekeeping", () => runSweeps()); + }, + { ...bullmqOpts, concurrency: 1 }, + ); + worker.on("failed", (_job, err) => logger.error({ err }, "Housekeeping tick failed")); + return worker; +} diff --git a/apps/api/src/workers/task-worker.ts b/apps/api/src/workers/task-worker.ts index 5d4a2851..697f1011 100644 --- a/apps/api/src/workers/task-worker.ts +++ b/apps/api/src/workers/task-worker.ts @@ -72,31 +72,12 @@ import { } from "../utils/harness-config.js"; import { applyGitAccess } from "../services/git-access-env.js"; import { activityFlusher } from "../services/activity-flush.js"; +import { TASK_CLAIM_KEY, withClaimLock } from "../services/claim-lock.js"; const bullmqOpts = getBullMQOptions(); export const taskQueue = new Queue("tasks", { ...bullmqOpts }); -/** - * Serialized claim lock. - * Prevents concurrent BullMQ workers from all passing the concurrency - * pre-check simultaneously (seeing 0 running), all claiming their tasks, - * and then all failing the post-check — which creates a storm of - * provisioning→queued state events that repeats every 10s. - * - * With the lock, only one worker at a time checks counts + claims, - * so the counts are always accurate. - */ -let claimLockChain: Promise = Promise.resolve(); - -function withClaimLock(fn: () => Promise): Promise { - let releaseLock!: () => void; - const nextLink = new Promise((r) => (releaseLock = r)); - const prev = claimLockChain; - claimLockChain = nextLink; - return prev.then(fn).finally(releaseLock); -} - export function startTaskWorker() { const worker = new Worker( "tasks", @@ -208,23 +189,26 @@ export function startTaskWorker() { } // ── Serialized concurrency check + claim ───────────────────── - // The claim lock ensures only one worker at a time checks - // counts and claims a task. Without this, N workers all see - // 0 running (pre-check race), all claim (provisioning), then - // all fail the post-check and re-queue — creating 2N state - // events per cycle that repeat every 10s ("event storm") and - // preventing ANY task from ever running. + // The claim lock (a Postgres advisory lock on the global limit, + // services/claim-lock.ts) ensures only one worker — on any API + // instance — at a time checks counts and claims a task; the repo's + // own limit is counted under the same lock. Without it, N workers all see 0 running (pre-check race), + // all claim (provisioning), then all fail the post-check and + // re-queue — creating 2N state events per cycle that repeat every + // 10s ("event storm") and preventing ANY task from ever running. // Compute effective concurrency: maxAgentsPerPod * maxPodInstances const maxAgentsPerPod = repoConfig?.maxAgentsPerPod ?? 2; const maxPodInstances = repoConfig?.maxPodInstances ?? 1; const effectiveRepoConcurrency = maxAgentsPerPod * maxPodInstances; - const claimed = await withClaimLock(async () => { + // Null when another worker held the lock past its timeout: re-queue. + const claimed = await withClaimLock([TASK_CLAIM_KEY], async (claim) => { + const { tx } = claim; const globalMax = parseIntEnv("OPTIO_MAX_CONCURRENT", 5); // Global concurrency check - const [{ count: activeCount }] = await db + const [{ count: activeCount }] = await tx .select({ count: sql`count(*)` }) .from(tasks) .where(sql`${tasks.state} IN ('provisioning', 'running')`); @@ -239,7 +223,7 @@ export function startTaskWorker() { const repoMax = repoConfig?.maxConcurrentTasks ? Math.min(repoConfig.maxConcurrentTasks, effectiveRepoConcurrency) : effectiveRepoConcurrency; - const [{ count: repoCount }] = await db + const [{ count: repoCount }] = await tx .select({ count: sql`count(*)` }) .from(tasks) .where( @@ -253,8 +237,15 @@ export function startTaskWorker() { return null; } - // Claim — atomic conditional update (queued → provisioning) - return taskService.tryTransitionTask(taskId, TaskState.PROVISIONING, "worker_pickup"); + // Claim — atomic conditional update (queued → provisioning) on + // the lock's transaction, so it commits before the lock goes; + // its announcement runs after the commit. + return taskService.claimTransitionIn( + claim, + taskId, + TaskState.PROVISIONING, + "worker_pickup", + ); }); if (!claimed) { diff --git a/apps/api/src/workers/ticket-sync-worker.ts b/apps/api/src/workers/ticket-sync-worker.ts index 2ec84900..3826b4e1 100644 --- a/apps/api/src/workers/ticket-sync-worker.ts +++ b/apps/api/src/workers/ticket-sync-worker.ts @@ -3,28 +3,33 @@ import { parseIntEnv } from "@optio/shared"; import { logger } from "../logger.js"; import { getBullMQOptions } from "../services/redis-config.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; +import { underPollerLease } from "../services/poller-lease.js"; const bullmqOpts = getBullMQOptions(); export const ticketSyncQueue = new Queue("ticket-sync", { ...bullmqOpts }); +/** + * Periodic ticket sync: one sweep at a time across instances (a poller + * lease); the sync itself claims each (source, ticket, repo) in + * `ticket_sync_claims` before creating a task, so two sweeps that do + * overlap still create one task per ticket. + */ export function startTicketSyncWorker(syncFn: () => Promise) { - // Add repeatable job for periodic sync - ticketSyncQueue.add( - "sync", - {}, - { - repeat: { - every: parseIntEnv("OPTIO_TICKET_SYNC_INTERVAL", 60000), // default: 60s - }, - }, - ); + void scheduleRepeat( + ticketSyncQueue, + "ticket-sync.sync", + parseIntEnv("OPTIO_TICKET_SYNC_INTERVAL", 60000), // default: 60s + ).catch(() => {}); const worker = new Worker( "ticket-sync", async () => { - logger.info("Running periodic ticket sync"); - await syncFn(); + await underPollerLease("ticket-sync", async () => { + logger.info("Running periodic ticket sync"); + await syncFn(); + }); }, { ...bullmqOpts, diff --git a/apps/api/src/workers/token-validation-worker.ts b/apps/api/src/workers/token-validation-worker.ts index 6446c067..770749e7 100644 --- a/apps/api/src/workers/token-validation-worker.ts +++ b/apps/api/src/workers/token-validation-worker.ts @@ -1,6 +1,8 @@ import { Queue, Worker } from "bullmq"; import { logger } from "../logger.js"; import { getBullMQOptions } from "../services/redis-config.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; +import { underPollerLease } from "../services/poller-lease.js"; import { getRedisClient } from "../services/event-bus.js"; const bullmqOpts = getBullMQOptions(); @@ -108,107 +110,14 @@ export async function validateClaudeToken( export function startTokenValidationWorker() { const intervalMs = parseInt(process.env.OPTIO_TOKEN_VALIDATION_INTERVAL ?? "300000", 10); // 5 min - tokenValidationQueue.add( - "validate-token", - {}, - { - repeat: { - every: intervalMs, - }, - }, + void scheduleRepeat(tokenValidationQueue, "token-validation.validate-token", intervalMs).catch( + () => {}, ); const worker = new Worker( "token-validation", async () => { - const redis = getRedisClient(); - - // Try to retrieve the stored OAuth token from the secrets store - let token: string | null = null; - try { - const { retrieveSecret } = await import("../services/secret-service.js"); - token = await retrieveSecret("CLAUDE_CODE_OAUTH_TOKEN").catch(() => null); - } catch { - // No secret-service available or encryption key not set — skip - } - - // Also check host credentials (max-subscription mode) - if (!token) { - try { - const { getClaudeAuthToken } = await import("../services/auth-service.js"); - const authResult = getClaudeAuthToken(); - if (authResult.available && authResult.token) { - token = authResult.token; - } - } catch { - // auth-service not available - } - } - - const now = new Date().toISOString(); - - if (!token) { - // No token configured — nothing to validate. Cache that fact. - const result: TokenValidationResult = { - valid: true, - lastValidated: now, - tokenExists: false, - }; - await redis.setex(TOKEN_VALIDATION_CACHE_KEY, CACHE_TTL_SECS, JSON.stringify(result)); - return; - } - - // Validate the token - const validation = await validateClaudeToken(token); - - const result: TokenValidationResult = { - valid: validation.valid, - lastValidated: now, - tokenExists: true, - ...(validation.error ? { error: validation.error } : {}), - }; - - await redis.setex(TOKEN_VALIDATION_CACHE_KEY, CACHE_TTL_SECS, JSON.stringify(result)); - - if (!validation.valid) { - logger.warn("Claude OAuth token validation failed — token is expired or invalid"); - - // A paired machine (Optio Local daemon) may hold a fresh login: take - // it from there before bothering anyone with the paste banner. - try { - const { autoRefreshClaudeToken } = - await import("../services/local-auth-refresh-service.js"); - if (await autoRefreshClaudeToken("validation-failed")) { - logger.info("Claude OAuth token refreshed automatically from a paired machine"); - return; - } - } catch (err) { - logger.warn({ err }, "automatic Claude token refresh failed"); - } - - // Invalidate the usage cache so the dashboard shows fresh data - try { - const { invalidateUsageCache } = await import("../services/auth-service.js"); - invalidateUsageCache(); - } catch { - // non-fatal - } - - // Publish auth:failed event so the UI shows a banner immediately - try { - const { publishEvent } = await import("../services/event-bus.js"); - await publishEvent({ - type: "auth:failed", - message: - "Claude Code OAuth token has expired. Go to Secrets to paste a new token, or re-run 'claude setup-token'.", - timestamp: now, - }); - } catch { - // non-fatal — UI will pick it up on next poll - } - } else { - logger.debug("Claude OAuth token validation passed"); - } + await underPollerLease("token-validation", () => validateStoredToken()); }, { ...bullmqOpts, @@ -222,3 +131,94 @@ export function startTokenValidationWorker() { return worker; } + +/** One validation pass (the tick's body). */ +export async function validateStoredToken(): Promise { + const redis = getRedisClient(); + + // Try to retrieve the stored OAuth token from the secrets store + let token: string | null = null; + try { + const { retrieveSecret } = await import("../services/secret-service.js"); + token = await retrieveSecret("CLAUDE_CODE_OAUTH_TOKEN").catch(() => null); + } catch { + // No secret-service available or encryption key not set — skip + } + + // Also check host credentials (max-subscription mode) + if (!token) { + try { + const { getClaudeAuthToken } = await import("../services/auth-service.js"); + const authResult = getClaudeAuthToken(); + if (authResult.available && authResult.token) { + token = authResult.token; + } + } catch { + // auth-service not available + } + } + + const now = new Date().toISOString(); + + if (!token) { + // No token configured — nothing to validate. Cache that fact. + const result: TokenValidationResult = { + valid: true, + lastValidated: now, + tokenExists: false, + }; + await redis.setex(TOKEN_VALIDATION_CACHE_KEY, CACHE_TTL_SECS, JSON.stringify(result)); + return; + } + + // Validate the token + const validation = await validateClaudeToken(token); + + const result: TokenValidationResult = { + valid: validation.valid, + lastValidated: now, + tokenExists: true, + ...(validation.error ? { error: validation.error } : {}), + }; + + await redis.setex(TOKEN_VALIDATION_CACHE_KEY, CACHE_TTL_SECS, JSON.stringify(result)); + + if (!validation.valid) { + logger.warn("Claude OAuth token validation failed — token is expired or invalid"); + + // A paired machine (Optio Local daemon) may hold a fresh login: take + // it from there before bothering anyone with the paste banner. + try { + const { autoRefreshClaudeToken } = await import("../services/local-auth-refresh-service.js"); + if (await autoRefreshClaudeToken("validation-failed")) { + logger.info("Claude OAuth token refreshed automatically from a paired machine"); + return; + } + } catch (err) { + logger.warn({ err }, "automatic Claude token refresh failed"); + } + + // Invalidate the usage cache so the dashboard shows fresh data + try { + const { invalidateUsageCache } = await import("../services/auth-service.js"); + invalidateUsageCache(); + } catch { + // non-fatal + } + + // Publish auth:failed event so the UI shows a banner immediately + try { + const { publishEvent } = await import("../services/event-bus.js"); + await publishEvent({ + type: "auth:failed", + message: + "Claude Code OAuth token has expired. Go to Secrets to paste a new token, or re-run 'claude setup-token'.", + timestamp: now, + }); + } catch { + // non-fatal — UI will pick it up on next poll + } + } else { + logger.debug("Claude OAuth token validation passed"); + } +} diff --git a/apps/api/src/workers/workflow-trigger-worker.test.ts b/apps/api/src/workers/workflow-trigger-worker.test.ts index 7eb20938..5632dce3 100644 --- a/apps/api/src/workers/workflow-trigger-worker.test.ts +++ b/apps/api/src/workers/workflow-trigger-worker.test.ts @@ -42,11 +42,22 @@ function seedDefinition(row: { id: string; kind: string; enabled: boolean; name? // The worker finds due schedules through the trigger service and fires them // through the dispatcher, which looks the target up as a work definition and // starts it through the per-kind services mocked here. +// The sweep advances each trigger by CAS before firing it; here every +// advance lands (the two-sweep race is an integration test). vi.mock("../services/trigger-service.js", () => ({ listDueScheduleTriggers: (...args: unknown[]) => mockGetDueScheduleTriggersAll(...args), - advanceSchedule: (...args: unknown[]) => mockMarkTriggerFired(...args), + advanceScheduleCas: async (id: string, cron: string) => { + mockMarkTriggerFired(id, cron); + return true; + }, markTriggerFired: vi.fn().mockResolvedValue(undefined), })); +vi.mock("../services/poller-lease.js", () => ({ + underPollerLease: (_name: string, fn: () => Promise) => fn(), +})); +vi.mock("../services/repeat-jobs.js", () => ({ + scheduleRepeat: vi.fn().mockResolvedValue(undefined), +})); vi.mock("../services/work-definition-service.js", async (importOriginal) => { const actual = await importOriginal(); @@ -79,6 +90,7 @@ function jobTrigger(overrides: Partial> = {}) { type: "schedule", config: { cronExpression: "0 0 * * *" }, paramMapping: null, + nextFireAt: new Date(Date.now() - 1000), ...overrides, }; } diff --git a/apps/api/src/workers/workflow-trigger-worker.ts b/apps/api/src/workers/workflow-trigger-worker.ts index b10e3b72..da99a3e1 100644 --- a/apps/api/src/workers/workflow-trigger-worker.ts +++ b/apps/api/src/workers/workflow-trigger-worker.ts @@ -2,8 +2,10 @@ import { Queue, Worker } from "bullmq"; import { parseIntEnv } from "@optio/shared"; import { logger } from "../logger.js"; import { getBullMQOptions } from "../services/redis-config.js"; -import { advanceSchedule, listDueScheduleTriggers } from "../services/trigger-service.js"; +import { advanceScheduleCas, listDueScheduleTriggers } from "../services/trigger-service.js"; import { fireTrigger } from "../services/trigger-dispatch.js"; +import { scheduleRepeat } from "../services/repeat-jobs.js"; +import { underPollerLease } from "../services/poller-lease.js"; const bullmqOpts = getBullMQOptions(); @@ -14,63 +16,24 @@ export const workflowTriggerQueue = new Queue("workflow-trigger-checker", { /** * Polls for due schedule triggers and hands each to the trigger dispatcher, * which starts whatever the trigger targets (a Job run, a Task, a Local - * terminal, an agent turn, a re-review). The schedule is advanced whether - * or not the dispatch succeeded, so one broken target can't re-fire every - * tick. + * terminal, an agent turn, a re-review). A sweep runs on one instance at a + * time (a poller lease), and each trigger is advanced **before** it fires, + * by compare-and-swap on the `next_fire_at` the sweep saw: two sweeps that + * overlap anyway both see the trigger due, and only the one whose update + * lands fires it. The schedule is advanced whether or not the dispatch + * succeeds, so one broken target can't re-fire every tick. */ export function startWorkflowTriggerWorker() { - workflowTriggerQueue.add( - "check-workflow-triggers", - {}, - { - repeat: { - every: parseIntEnv("OPTIO_WORKFLOW_TRIGGER_INTERVAL", 60000), - }, - }, - ); + void scheduleRepeat( + workflowTriggerQueue, + "workflow-trigger-checker.check-workflow-triggers", + parseIntEnv("OPTIO_WORKFLOW_TRIGGER_INTERVAL", 60000), + ).catch(() => {}); const worker = new Worker( "workflow-trigger-checker", async () => { - const triggers = await listDueScheduleTriggers(); - if (triggers.length === 0) return; - - logger.info({ count: triggers.length }, "Processing due schedule triggers"); - - for (const trigger of triggers) { - const config = trigger.config as Record | null; - const cronExpression = config?.cronExpression as string | undefined; - - if (!cronExpression) { - logger.warn( - { triggerId: trigger.id, targetType: trigger.targetType, targetId: trigger.targetId }, - "Schedule trigger missing cronExpression in config, skipping", - ); - continue; - } - - try { - await fireTrigger(trigger, { - source: "schedule", - params: trigger.paramMapping ?? undefined, - }); - } catch (err) { - logger.error( - { - err, - triggerId: trigger.id, - targetType: trigger.targetType, - targetId: trigger.targetId, - }, - "Failed to fire schedule trigger", - ); - } - try { - await advanceSchedule(trigger.id, cronExpression); - } catch { - // best-effort - } - } + await underPollerLease("workflow-trigger-checker", () => checkDueScheduleTriggers()); }, { ...bullmqOpts, concurrency: 1 }, ); @@ -81,3 +44,58 @@ export function startWorkflowTriggerWorker() { return worker; } + +/** One sweep: fires every due schedule trigger it manages to advance first. Returns how many fired. */ +export async function checkDueScheduleTriggers(): Promise { + const triggers = await listDueScheduleTriggers(); + if (triggers.length === 0) return 0; + + logger.info({ count: triggers.length }, "Processing due schedule triggers"); + + let fired = 0; + for (const trigger of triggers) { + const config = trigger.config as Record | null; + const cronExpression = config?.cronExpression as string | undefined; + + if (!cronExpression) { + logger.warn( + { triggerId: trigger.id, targetType: trigger.targetType, targetId: trigger.targetId }, + "Schedule trigger missing cronExpression in config, skipping", + ); + continue; + } + + // Advance first: the firing belongs to whoever moved the schedule off + // the tick the sweep saw. A sweep that lost the race skips the trigger. + let advanced = false; + try { + advanced = await advanceScheduleCas(trigger.id, cronExpression, trigger.nextFireAt); + } catch (err) { + logger.error({ err, triggerId: trigger.id }, "Failed to advance schedule trigger"); + continue; + } + if (!advanced) { + logger.debug({ triggerId: trigger.id }, "Schedule trigger already fired by another sweep"); + continue; + } + + try { + await fireTrigger(trigger, { + source: "schedule", + params: trigger.paramMapping ?? undefined, + }); + fired++; + } catch (err) { + logger.error( + { + err, + triggerId: trigger.id, + targetType: trigger.targetType, + targetId: trigger.targetId, + }, + "Failed to fire schedule trigger", + ); + } + } + return fired; +} diff --git a/apps/api/src/workers/workflow-worker.ts b/apps/api/src/workers/workflow-worker.ts index 8b88754b..20bdd2b4 100644 --- a/apps/api/src/workers/workflow-worker.ts +++ b/apps/api/src/workers/workflow-worker.ts @@ -14,12 +14,17 @@ import { workflowRuns } from "../db/schema.js"; import { and, eq } from "drizzle-orm"; import { updatedAtMatches } from "../utils/pg-timestamp.js"; import * as workflowService from "../services/workflow-service.js"; -import { transitionWorkflowRunCas, type Workflow } from "../services/workflow-service.js"; +import { + claimWorkflowRunIn, + transitionWorkflowRunCas, + type Workflow, +} from "../services/workflow-service.js"; import { COMMAND_SCRIPT, commandResult, parseCommandLine } from "../services/command-run.js"; import { renderCommandTemplate } from "../services/prompt-template-service.js"; import * as workflowPool from "../services/workflow-pool-service.js"; import { addUsage } from "../services/run-usage.js"; import { activityFlusher } from "../services/activity-flush.js"; +import { WORKFLOW_CLAIM_KEY, withClaimLock } from "../services/claim-lock.js"; import { resolvePodSecrets } from "../services/secret-service.js"; import { detectAuthFailureInLogs, recordAuthEvent } from "../services/auth-failure-detector.js"; import { @@ -90,18 +95,6 @@ export function buildWorkflowAgentCommand( }); } -// ── Concurrency lock ─────────────────────────────────────────────────────────── - -let claimLockChain: Promise = Promise.resolve(); - -function withClaimLock(fn: () => Promise): Promise { - let releaseLock!: () => void; - const nextLink = new Promise((r) => (releaseLock = r)); - const prev = claimLockChain; - claimLockChain = nextLink; - return prev.then(fn).finally(releaseLock); -} - // ── Worker ───────────────────────────────────────────────────────────────────── export function startWorkflowWorker() { @@ -162,34 +155,42 @@ export function startWorkflowWorker() { } // ── Concurrency check ───────────────────────────────────────── - const claimed = await withClaimLock(async () => { + // Under the advisory lock for the global limit, so claimers on every + // API instance count one at a time (claim-lock.ts); this Job's own + // limit is counted under the same lock. Null when another worker + // held the lock past its timeout: re-queue. + const claimed = await withClaimLock([WORKFLOW_CLAIM_KEY], async (claim) => { // Global workflow concurrency (cluster runs only; local runs don't // occupy pods), then this workflow's own. const capacity = await workflowService.jobRunCapacity( workflow.id, workflow.maxConcurrent, + claim.tx, ); if (capacity.global.running >= capacity.global.max) { log.info( { activeCount: capacity.global.running, globalMax: capacity.global.max }, "Global workflow concurrency saturated", ); - return false; + return null; } if (capacity.job.running >= capacity.job.max) { log.info( { activeCount: capacity.job.running, max: capacity.job.max }, "Per-workflow concurrency saturated", ); - return false; + return null; } // Claim: transition to running. CAS — a second worker holding the // same run (a reconcile re-enqueue) loses here instead of running it twice. // The claim is the attempt's first sign of life: a retried run must // not be judged stalled by the previous attempt's last activity. + // On the lock's transaction, so it commits before the lock goes; + // its announcement runs after the commit. const now = new Date(); - return transitionWorkflowRunCas( + return claimWorkflowRunIn( + claim, workflowRunId, WorkflowRunState.QUEUED, WorkflowRunState.RUNNING, diff --git a/apps/api/src/ws/optio-chat.test.ts b/apps/api/src/ws/optio-chat.test.ts index 6feef365..9e0b997c 100644 --- a/apps/api/src/ws/optio-chat.test.ts +++ b/apps/api/src/ws/optio-chat.test.ts @@ -7,15 +7,10 @@ import { parseActionResult, toAnthropicTools, streamAnthropicResponse, - _resetActiveConnections, } from "./optio-chat.js"; import { OPTIO_TOOL_SCHEMAS } from "@optio/shared"; describe("optio-chat", () => { - beforeEach(() => { - _resetActiveConnections(); - }); - // ─── toolRequiresConfirmation ─── describe("toolRequiresConfirmation", () => { diff --git a/apps/api/src/ws/optio-chat.ts b/apps/api/src/ws/optio-chat.ts index f51beba8..4617278f 100644 --- a/apps/api/src/ws/optio-chat.ts +++ b/apps/api/src/ws/optio-chat.ts @@ -15,6 +15,9 @@ import { executeToolCall, truncateToolResult } from "../services/optio-tool-exec import { resolveLiveModelId } from "../services/agent-options-service.js"; import { acceptWs } from "./ws-connection.js"; import { isMessageWithinSizeLimit, WS_CLOSE_MESSAGE_TOO_LARGE } from "./ws-limits.js"; +import { randomUUID } from "node:crypto"; +import { INSTANCE_ID } from "../services/instance.js"; +import { acquireLease, releaseLease, renewLease } from "../services/lease-service.js"; // ─── Constants ─────────────────────────────────────────────────────────────── @@ -29,12 +32,44 @@ const DEFAULT_MAX_TURNS = 10; // ─── Per-user concurrency tracking ────────────────────────────────────────── -/** Map of userId → active WebSocket (only one active conversation per user). */ -const activeConnections = new Map(); +/** + * One active conversation per user, across every API instance: the socket + * holds the lease `chat:` (services/lease-service.ts) while open, + * renewed every 10 s, released on close. A second socket for the same + * user — on this instance or another — finds the lease held. A socket whose + * instance died stops renewing, and its lease lapses within 30 s. + */ +const CHAT_LEASE_TTL_MS = 30_000; +const CHAT_LEASE_RENEW_MS = 10_000; +const CHAT_LEASE_RELEASE_WAIT_MS = 2_000; -/** @internal Reset active connections — only for tests. */ -export function _resetActiveConnections(): void { - activeConnections.clear(); +export function chatLeaseKey(userId: string): string { + return `chat:${userId}`; +} + +/** + * Gives a conversation's lease back, waiting at most two seconds. One that + * doesn't land (the database slow or away) lapses with its TTL. + */ +async function releaseChatLease( + key: string, + holder: string, + log: { warn: (obj: unknown, msg?: string) => void }, +): Promise { + let timer: NodeJS.Timeout | undefined; + try { + const outcome = await Promise.race([ + releaseLease(key, holder).then(() => "released" as const), + new Promise<"timeout">((resolve) => { + timer = setTimeout(() => resolve("timeout"), CHAT_LEASE_RELEASE_WAIT_MS); + }), + ]); + if (outcome === "timeout") log.warn({}, "Optio chat lease release timed out; it will lapse"); + } catch (err) { + log.warn({ err }, "Optio chat lease release failed; it will lapse"); + } finally { + clearTimeout(timer); + } } // ─── Anthropic API types ──────────────────────────────────────────────────── @@ -416,8 +451,26 @@ export async function optioChatWs(app: FastifyInstance) { } if (conn.closed) return; - // Enforce one active conversation per user - if (activeConnections.has(userId)) { + // Enforce one active conversation per user (on any instance). Each + // socket is its own lease holder, so a second socket of the same user + // on this instance can't extend the first one's lease. + const leaseKey = chatLeaseKey(userId); + const leaseHolder = `${INSTANCE_ID}:${randomUUID()}`; + let acquired: boolean; + try { + acquired = await acquireLease(leaseKey, CHAT_LEASE_TTL_MS, leaseHolder); + } catch (err) { + log.error({ err }, "Could not take the Optio chat lease"); + socket.send( + JSON.stringify({ + type: "error", + message: "Optio couldn't start the conversation right now. Try again in a moment.", + }), + ); + socket.close(1011, "Could not start the conversation"); + return conn.discard(); + } + if (!acquired) { socket.send( JSON.stringify({ type: "error", @@ -427,12 +480,25 @@ export async function optioChatWs(app: FastifyInstance) { socket.close(4409, "Concurrent conversation"); return conn.discard(); } + if (conn.closed) { + await releaseChatLease(leaseKey, leaseHolder, log); + return; + } - activeConnections.set(userId, socket as unknown as WebSocket); + const renew = setInterval(() => { + renewLease(leaseKey, CHAT_LEASE_TTL_MS, leaseHolder) + .then((ok) => { + if (!ok) { + log.warn("Optio chat lease lost; closing"); + socket.close(4409, "Concurrent conversation"); + } + }) + .catch(() => {}); + }, CHAT_LEASE_RENEW_MS); + renew.unref?.(); conn.onClose(() => { - if (activeConnections.get(userId) === (socket as unknown as WebSocket)) { - activeConnections.delete(userId); - } + clearInterval(renew); + void releaseChatLease(leaseKey, leaseHolder, log); }); log.info("Optio chat connected"); diff --git a/apps/api/src/ws/ws-auth.ts b/apps/api/src/ws/ws-auth.ts index d7019f80..bc1951e9 100644 --- a/apps/api/src/ws/ws-auth.ts +++ b/apps/api/src/ws/ws-auth.ts @@ -55,7 +55,8 @@ function extractUpgradeTokenFromProtocol(req: FastifyRequest): string | undefine * 1. Session cookie (`optio_session`) — validated against the sessions table. * Browsers send cookies on WebSocket upgrade requests automatically. * 2. Single-use upgrade token via `Sec-WebSocket-Protocol` header — validated - * and consumed from the in-memory WS token store (short-lived, ~30 s, one-time use). + * and consumed from `ws_upgrade_tokens` (short-lived, ~30 s, one-time use; any + * API instance may have minted it). * Used for cross-origin setups where cookies are not available. * * Tokens are NEVER read from URL query params to prevent leaking into logs. diff --git a/docs/production-eks.md b/docs/production-eks.md index 0deb64c9..67943213 100644 --- a/docs/production-eks.md +++ b/docs/production-eks.md @@ -7,6 +7,18 @@ for upgrades. This includes a brief control-plane outage; it is not an HA API. Queue locks alone do not make the process-local relays safe across replicas. Agent pods scale independently within each workspace, owner and access profile. +The pieces of coordination that several API pods will share are already in the +database ([docs/plans/scale-out.md](plans/scale-out.md)): a run's concurrency +claim is a Postgres advisory lock, every periodic sweep runs under a lease in +`leases` and is safe to overlap anyway (ticket claims, a compare-and-swap on a +schedule's next tick, one active review per PR), inbound webhook deliveries and +WebSocket upgrade tokens are rows, and repeat jobs are BullMQ job schedulers +with stable ids, so pods booting in any order leave one schedule per tick. What +stays **per pod** is the WebSocket connection cap (50 sockets per client +address, counted by the pod that holds them, so with N pods an address may hold +up to N times that) and the installation-token cache (each pod mints its own +hour-long GitHub App token). + ## Managed services and existing Secrets Start with `helm/optio/values.production.yaml`. Set your public hostname, ingress