diff --git a/src/api/routes.ts b/src/api/routes.ts index b3d1da6c12..1e3a4c6615 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -113,6 +113,7 @@ import { upsertContributorEvidence, upsertContributorScoringProfile, upsertRepositorySettings, + clearPullRequestsRegatedAtForOpenPrs, getRepositoryAiKeyStatus, upsertRepositoryAiKey, deleteRepositoryAiKey, @@ -2658,6 +2659,13 @@ export function createApp() { repoFullNames.map(async (repoFullName) => { const current = await getRepositorySettings(c.env, repoFullName); await upsertRepositorySettings(c.env, { ...current, ...changes, repoFullName }); + // #9018: mirrors the single-repo pause/resume tool's own catch-up (mcp/server.ts setAgentPaused) -- + // a paused->live transition performs no re-evaluation by default, so a PR that went green during the + // pause window can be permanently stranded once #never-endless-reregate excludes it from future sweep + // candidacy. Restores one-shot candidacy for every open PR in this repo. + if (current.agentPaused === true && changes.agentPaused === false) { + await clearPullRequestsRegatedAtForOpenPrs(c.env, repoFullName); + } }), ); await recordAuditEvent(c.env, { diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 069b98a09b..3eed1e71d0 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4326,6 +4326,22 @@ export async function markPullRequestsRegated(env: Env, fullName: string, number .where(and(eq(pullRequests.repoFullName, fullName), inArray(pullRequests.number, numbers))); } +/** #9018: on a repo's paused -> live transition, restore sweep candidacy for every currently-OPEN PR by + * clearing its `lastRegatedAt` marker. Without this, a PR that went green DURING the pause window (CI-completion + * passes plan-and-suppress the whole time) is stranded: the sweep's #never-endless-reregate rule + * (agent-sweep.ts's hasBeenRegated) permanently excludes any PR already regated once, so if it had ALSO been + * regated before the pause it never becomes a sweep candidate again on its own -- resume performs no catch-up + * by itself. Clearing the marker restores it to "never regated" exactly once, the same one-shot candidacy a + * genuinely new PR gets. A plain D1 write, never the #1258 GitHub chokepoint -- unconditional, so it is safe to + * call even when nothing was actually stale. */ +export async function clearPullRequestsRegatedAtForOpenPrs(env: Env, fullName: string): Promise { + const db = getDb(env.DB); + await db + .update(pullRequests) + .set({ lastRegatedAt: null, updatedAt: nowIso() }) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.state, "open"))); +} + /** Batch variant of {@link markPullRequestsRegated} for backlog-convergence-sweep (#4502): stamps the SEPARATE * last_backlog_convergence_regated_at marker at sweep DISPATCH time, mirroring the same "stamp immediately, not * in the downstream per-PR job" shape so getLatestBacklogConvergenceRegatedAt reflects this sweep before its diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 079078f94e..41895c2e20 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -63,6 +63,7 @@ import { listNotificationDeliveriesForRecipient, upsertIssueWatchSubscription, upsertRepositorySettings, + clearPullRequestsRegatedAtForOpenPrs, listOpenPullRequests, listPullRequests, listPullRequestFiles, @@ -4872,6 +4873,15 @@ export class LoopoverMcp { await this.requireRepoManageAccess(fullName); const current = await getRepositorySettings(this.env, fullName); const updated = await upsertRepositorySettings(this.env, { ...current, agentPaused: input.paused }); + // #9018: a paused->live transition performs no catch-up by default. A PR that went GREEN during the pause + // window (CI-completion passes plan-and-suppress the whole time) can be permanently stranded: if it was + // ALSO already regated once before the pause, agent-sweep.ts's #never-endless-reregate rule excludes it + // from sweep candidacy forever, and the only other wake (a sibling merge) may never fire in a quiet repo. + // Clearing lastRegatedAt for every open PR restores one-shot candidacy so the sweep re-evaluates and + // dispositions it without a human or a new commit. + if (current.agentPaused === true && input.paused === false) { + await clearPullRequestsRegatedAtForOpenPrs(this.env, fullName); + } return { summary: `Agent actions ${input.paused ? "paused" : "resumed"} for ${fullName}.`, data: { repoFullName: fullName, agentPaused: updated.agentPaused }, diff --git a/src/queue/transient-locks.ts b/src/queue/transient-locks.ts index 9f5aca53ae..06ac669900 100644 --- a/src/queue/transient-locks.ts +++ b/src/queue/transient-locks.ts @@ -182,7 +182,15 @@ export class PrActuationLockContendedError extends RetryableJobError { // view of "how many of this author's PRs are currently open" at the same time. Same short-TTL, best-effort, // per-holder-token shape as claimPrActuationLock above; see this module's own header comment for why a // missing claim()/releaseIfValue() primitive fails OPEN rather than fake exclusivity. -const CONTRIBUTOR_CAP_LOCK_TTL_SECONDS = 30; +// #9024: was 30s, sized only for the executor's brief contributorCapMergeRecheck() call. But +// maybeCloseForContributorCapOnOpen (processors.ts) holds this SAME lock across a much longer body -- a token +// mint, live GitHub calls, ensurePullRequestLabel, the nested pr-actuation-lock, and a full +// executeAgentMaintenanceActions pass (live-CI recheck + GitHub close) -- which can exceed 30s under GitHub +// rate-limit backoff. Once Redis expires the lock while that holder is still inside the executor, a +// concurrent sibling PR's cap check (or the executor's own pre-merge recheck) acquires it and evaluates the +// author's open-PR count before the in-flight close lands -- the exact TOCTOU #7284 this lock exists to close. +// Matches claimPrActuationLock's own 600s TTL, the other lock guarding a comparably long mutating body. +const CONTRIBUTOR_CAP_LOCK_TTL_SECONDS = 600; function contributorCapLockKey(repoFullName: string, authorLogin: string): string { return `contributor-cap-lock:${repoFullName.toLowerCase()}:${authorLogin.toLowerCase()}`; } diff --git a/src/selfhost/redis-cache.ts b/src/selfhost/redis-cache.ts index 2404503eb6..1b68a46c9a 100644 --- a/src/selfhost/redis-cache.ts +++ b/src/selfhost/redis-cache.ts @@ -98,3 +98,40 @@ export function assertSelfhostTransientCacheOwnershipRelease( } export type RedisCache = ReturnType; + +// #9021: every lock built on claimTransientLock (transient-locks.ts) survives a container restart with its +// TTL intact -- there is no boot-time flush anywhere, so on a SINGLE-INSTANCE deployment any lock present at +// boot is provably orphaned (the process that claimed it is gone; nothing else could be holding it). Left +// alone, each class strands real work for its own TTL: pr-actuation-lock (600s) defers every close/merge/label +// pass for that PR, ai-review-lock (1800s) starves that PR's review (#8998), contributor-cap-wake (1800s) +// leaves an over-cap sibling un-reevaluated. Deliberately does NOT touch: `delivery:*` (webhook dedup -- +// flushing would re-process recent deliveries), `pr-panel-retrigger-pending:*` (must survive restarts by +// design, #7626), `ci-pending-first-seen:*` (resets the stuck-CI clock), `fresh-rebase-forced:*` (re-arms a +// safety cap) -- none of those are exclusivity locks, and each has its own restart-survival contract. +export const ORPHANED_LOCK_KEY_PATTERNS: readonly string[] = [ + "pr-actuation-lock:*", + "ai-review-lock:*", + "contributor-cap-wake:*", + "contributor-cap-lock:*", +]; + +/** Best-effort SCAN-delete of every key matching {@link ORPHANED_LOCK_KEY_PATTERNS}, meant to run once at + * self-host boot before the queue starts processing. Never throws: a flush failure just means a lock (or + * several) rides out its own TTL as before -- exactly the pre-#9021 behavior -- rather than blocking startup. + * Returns the number of keys deleted, for a single boot-time log line. */ +export async function flushOrphanedLocksAtBoot(redis: Redis): Promise { + let deleted = 0; + for (const pattern of ORPHANED_LOCK_KEY_PATTERNS) { + try { + const stream = redis.scanStream({ match: pattern, count: 100 }); + for await (const keys of stream) { + const batch = keys as string[]; + if (batch.length === 0) continue; + deleted += await redis.del(...batch); + } + } catch { + // best-effort — a scan/delete failure for one pattern must never block the others or startup itself. + } + } + return deleted; +} diff --git a/src/server.ts b/src/server.ts index 7cf2b697f6..4fc4227141 100644 --- a/src/server.ts +++ b/src/server.ts @@ -687,10 +687,21 @@ async function main(): Promise { const { Redis } = await import("ioredis"); const redisClient = new Redis(redisUrl); const { createRedisRateLimiter } = await import("./selfhost/redis-ratelimit"); - const { createRedisCache, assertSelfhostTransientCacheOwnershipRelease, isWebhookDeliveryDuplicate, rememberWebhookDelivery } = await import("./selfhost/redis-cache"); + const { createRedisCache, assertSelfhostTransientCacheOwnershipRelease, flushOrphanedLocksAtBoot, isWebhookDeliveryDuplicate, rememberWebhookDelivery } = await import("./selfhost/redis-cache"); const rateLimiter = createRedisRateLimiter(redisClient); const webhookCache = createRedisCache(redisClient); assertSelfhostTransientCacheOwnershipRelease(webhookCache); + // #9021: every Redis-backed lock (pr-actuation-lock, ai-review-lock, contributor-cap-wake/-lock) survives a + // container restart with its TTL intact. On this single-instance deployment any lock present at boot is + // provably orphaned -- the process that claimed it is gone -- so flush them before the queue starts + // processing rather than let each strand real work for up to its own TTL (30 min for ai-review-lock, #8998). + // Best-effort: a flush failure just falls back to the pre-#9021 behavior (each lock rides out its TTL). + const flushedOrphanedLocks = await flushOrphanedLocksAtBoot(redisClient); + if (flushedOrphanedLocks > 0) { + console.log( + JSON.stringify({ event: "selfhost_orphaned_locks_flushed", count: flushedOrphanedLocks }), + ); + } // Persist the installation-token cache in Redis so warm GitHub App tokens survive restarts/deploys and are // shared across replicas (the in-isolate Map otherwise re-mints — an Orb round-trip — per replica/cold start). const { createRedisTokenCache } = await import("./selfhost/redis-token-cache"); diff --git a/test/unit/mcp-automation-state.test.ts b/test/unit/mcp-automation-state.test.ts index 22c2dff1cc..35144f500e 100644 --- a/test/unit/mcp-automation-state.test.ts +++ b/test/unit/mcp-automation-state.test.ts @@ -4,7 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { LoopoverMcp } from "../../src/mcp/server"; import { getRepositoryCollaboratorPermission } from "../../src/github/app"; import { mergePullRequest } from "../../src/github/pr-actions"; -import { createPendingAgentActionIfAbsent, getPendingAgentAction, getRepositorySettings, listPendingAgentActions, recordAuditEvent, upsertInstallation, upsertOfficialMinerDetection, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; +import { createPendingAgentActionIfAbsent, getPendingAgentAction, getRepositorySettings, listPendingAgentActions, listPullRequests, markPullRequestRegated, recordAuditEvent, upsertInstallation, upsertOfficialMinerDetection, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; import type { AuthIdentity } from "../../src/auth/security"; import { createTestEnv } from "../helpers/d1"; @@ -193,6 +193,40 @@ describe("MCP loopover_set_agent_paused (#6087)", () => { expect(result.isError).toBe(true); expect(JSON.stringify(result)).toMatch(/MCP_ACTUATION_REPO_ALLOWLIST/); }); + + // #9018: a PR that went green DURING the pause window can be stranded forever if it was ALSO regated once + // before the pause -- agent-sweep.ts's #never-endless-reregate rule then permanently excludes it from sweep + // candidacy, and resume performed no catch-up. Clearing lastRegatedAt on the paused->live transition restores + // one-shot candidacy. + it("#9018: a paused->live transition clears lastRegatedAt for the repo's OPEN PRs (restoring sweep candidacy), never for closed ones or other transitions", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }, 5); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto" } }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 1, title: "Went green during pause", state: "open", user: { login: "contributor" }, head: { sha: "a1" }, labels: [], body: "" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 2, title: "Already closed", state: "closed", user: { login: "contributor" }, head: { sha: "a2" }, labels: [], body: "" }); + await markPullRequestRegated(env, "owner/repo", 1); + await markPullRequestRegated(env, "owner/repo", 2); + const beforePause = await listPullRequests(env, "owner/repo"); + expect(beforePause.find((pr) => pr.number === 1)?.lastRegatedAt).toBeTruthy(); + expect(beforePause.find((pr) => pr.number === 2)?.lastRegatedAt).toBeTruthy(); + + const client = await connect(env); + // Pausing itself must never clear anything -- only the paused->live direction does. + await client.callTool({ name: "loopover_set_agent_paused", arguments: { owner: "owner", repo: "repo", paused: true } }); + const afterPause = await listPullRequests(env, "owner/repo"); + expect(afterPause.find((pr) => pr.number === 1)?.lastRegatedAt).toBeTruthy(); + + await client.callTool({ name: "loopover_set_agent_paused", arguments: { owner: "owner", repo: "repo", paused: false } }); + const afterResume = await listPullRequests(env, "owner/repo"); + expect(afterResume.find((pr) => pr.number === 1)?.lastRegatedAt).toBeNull(); // OPEN -- restored to sweep candidacy + expect(afterResume.find((pr) => pr.number === 2)?.lastRegatedAt).toBeTruthy(); // CLOSED -- untouched + + // A resume-while-already-live call (paused stays false -> false) is a no-op for this marker: re-mark PR 1 + // regated and confirm a repeat "resume" call doesn't clear it again. + await markPullRequestRegated(env, "owner/repo", 1); + await client.callTool({ name: "loopover_set_agent_paused", arguments: { owner: "owner", repo: "repo", paused: false } }); + expect((await listPullRequests(env, "owner/repo")).find((pr) => pr.number === 1)?.lastRegatedAt).toBeTruthy(); + }); }); describe("MCP loopover_set_action_autonomy (#6087)", () => { diff --git a/test/unit/routes-installation-bulk-agent-settings.test.ts b/test/unit/routes-installation-bulk-agent-settings.test.ts new file mode 100644 index 0000000000..f9778c6307 --- /dev/null +++ b/test/unit/routes-installation-bulk-agent-settings.test.ts @@ -0,0 +1,89 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createApp } from "../../src/api/routes"; +import { createSessionForGitHubUser } from "../../src/auth/security"; +import { getRepositorySettings, listPullRequests, markPullRequestRegated, upsertInstallation, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +function stubMinerDetection(): void { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString().includes("gittensor.io")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); +} + +async function seedOwnedInstallation(env: Env, owner: string, installationId: number, repoNames: string[]): Promise { + await upsertInstallation(env, { + installation: { + id: installationId, + account: { login: owner, id: installationId, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read" }, + events: ["repository"], + }, + repositories: repoNames.map((name) => ({ name, full_name: `${owner}/${name}`, private: false, owner: { login: owner } })), + }); + for (const name of repoNames) { + await upsertRepositoryFromGitHub(env, { name, full_name: `${owner}/${name}`, private: false, owner: { login: owner } }, installationId); + } +} + +// #9018 (bulk path): mirrors the single-repo pause/resume tool's own catch-up (mcp/server.ts setAgentPaused) -- +// PUT /v1/app/installations/:id/agent/bulk-settings applies agentPaused across every repo in an installation +// in one call, so it needs the SAME paused->live catch-up per repo, not just the per-repo MCP tool. +describe("PUT /v1/app/installations/:id/agent/bulk-settings (#7676, #9018)", () => { + afterEach(() => vi.unstubAllGlobals()); + + it("#9018: clears lastRegatedAt for every open PR across all repos on a paused->live bulk transition", async () => { + const app = createApp(); + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "" }); + await seedOwnedInstallation(env, "owner", 202, ["repo-a", "repo-b"]); + stubMinerDetection(); + await upsertPullRequestFromGitHub(env, "owner/repo-a", { number: 1, title: "Went green during pause", state: "open", user: { login: "contributor" }, head: { sha: "a1" }, labels: [], body: "" }); + await upsertPullRequestFromGitHub(env, "owner/repo-b", { number: 5, title: "Also went green", state: "open", user: { login: "contributor" }, head: { sha: "b5" }, labels: [], body: "" }); + await markPullRequestRegated(env, "owner/repo-a", 1); + await markPullRequestRegated(env, "owner/repo-b", 5); + const { token } = await createSessionForGitHubUser(env, { login: "owner", id: 202 }); + const headers = { cookie: `loopover_session=${token}`, "content-type": "application/json" }; + + const pauseRes = await app.request( + "/v1/app/installations/202/agent/bulk-settings", + { method: "PUT", headers, body: JSON.stringify({ agentPaused: true }) }, + env, + ); + expect(pauseRes.status).toBe(200); + expect((await getRepositorySettings(env, "owner/repo-a")).agentPaused).toBe(true); + // Pausing must never clear the marker. + expect((await listPullRequests(env, "owner/repo-a")).find((pr) => pr.number === 1)?.lastRegatedAt).toBeTruthy(); + + const resumeRes = await app.request( + "/v1/app/installations/202/agent/bulk-settings", + { method: "PUT", headers, body: JSON.stringify({ agentPaused: false }) }, + env, + ); + expect(resumeRes.status).toBe(200); + expect((await getRepositorySettings(env, "owner/repo-a")).agentPaused).toBe(false); + expect((await listPullRequests(env, "owner/repo-a")).find((pr) => pr.number === 1)?.lastRegatedAt).toBeNull(); + expect((await listPullRequests(env, "owner/repo-b")).find((pr) => pr.number === 5)?.lastRegatedAt).toBeNull(); + }); + + it("does not clear lastRegatedAt when the bulk change does not touch agentPaused at all (e.g. only agentDryRun)", async () => { + const app = createApp(); + const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "" }); + await seedOwnedInstallation(env, "owner", 203, ["repo-c"]); + stubMinerDetection(); + await upsertRepositorySettings(env, { repoFullName: "owner/repo-c", agentPaused: true }); + await upsertPullRequestFromGitHub(env, "owner/repo-c", { number: 9, title: "Paused repo, unrelated bulk change", state: "open", user: { login: "contributor" }, head: { sha: "c9" }, labels: [], body: "" }); + await markPullRequestRegated(env, "owner/repo-c", 9); + const { token } = await createSessionForGitHubUser(env, { login: "owner", id: 203 }); + + const res = await app.request( + "/v1/app/installations/203/agent/bulk-settings", + { method: "PUT", headers: { cookie: `loopover_session=${token}`, "content-type": "application/json" }, body: JSON.stringify({ agentDryRun: true }) }, + env, + ); + + expect(res.status).toBe(200); + expect((await getRepositorySettings(env, "owner/repo-c")).agentPaused).toBe(true); // untouched + expect((await listPullRequests(env, "owner/repo-c")).find((pr) => pr.number === 9)?.lastRegatedAt).toBeTruthy(); + }); +}); diff --git a/test/unit/selfhost-redis-cache.test.ts b/test/unit/selfhost-redis-cache.test.ts index 345b35ef06..c2f9954c0e 100644 --- a/test/unit/selfhost-redis-cache.test.ts +++ b/test/unit/selfhost-redis-cache.test.ts @@ -1,9 +1,12 @@ import type { Redis } from "ioredis"; +import { Readable } from "node:stream"; import { afterEach, describe, expect, it } from "vitest"; import { assertSelfhostTransientCacheOwnershipRelease, createRedisCache, + flushOrphanedLocksAtBoot, isWebhookDeliveryDuplicate, + ORPHANED_LOCK_KEY_PATTERNS, rememberWebhookDelivery, webhookDeliveryCacheKey, } from "../../src/selfhost/redis-cache"; @@ -145,3 +148,75 @@ describe("isWebhookDeliveryDuplicate (#2075)", () => { expect(await cache.get(webhookDeliveryCacheKey("delivery-4"))).toBe("1"); }); }); + +/** Minimal glob match supporting the one wildcard shape ORPHANED_LOCK_KEY_PATTERNS uses ("prefix:*"). */ +function globMatch(pattern: string, key: string): boolean { + if (!pattern.includes("*")) return pattern === key; + const prefix = pattern.slice(0, pattern.indexOf("*")); + return key.startsWith(prefix); +} + +/** Fake ioredis client for flushOrphanedLocksAtBoot: `scanStream` emits the matching keys from `_store` as a + * single batch (real SCAN would paginate; one batch is sufficient to exercise the consumer's own for-await + * loop and del() call), and `del` removes them. */ +function fakeScanRedis(initialKeys: string[]): Redis & { _store: Set; scanCalls: string[] } { + const _store = new Set(initialKeys); + const scanCalls: string[] = []; + return { + _store, + scanCalls, + scanStream({ match }: { match: string; count?: number }) { + scanCalls.push(match); + const matched = [..._store].filter((key) => globMatch(match, key)); + return Readable.from(matched.length > 0 ? [matched] : []); + }, + async del(...keys: string[]) { + let removed = 0; + for (const key of keys) if (_store.delete(key)) removed += 1; + return removed; + }, + } as unknown as Redis & { _store: Set; scanCalls: string[] }; +} + +describe("flushOrphanedLocksAtBoot (#9021)", () => { + it("deletes every key matching the configured orphaned-lock patterns and leaves everything else alone", async () => { + const redis = fakeScanRedis([ + "pr-actuation-lock:owner/repo#1", + "ai-review-lock:owner/repo#1@sha1:block", + "contributor-cap-wake:owner/repo#1#sha1", + "contributor-cap-lock:owner/repo:alice", + // Must survive: not exclusivity locks, each has its own restart-survival contract. + "delivery:abc123", + "pr-panel-retrigger-pending:owner/repo#1", + "ci-pending-first-seen:owner/repo#1", + "fresh-rebase-forced:owner/repo#1", + ]); + + const deleted = await flushOrphanedLocksAtBoot(redis); + + expect(deleted).toBe(4); + expect([...redis._store].sort()).toEqual( + ["delivery:abc123", "pr-panel-retrigger-pending:owner/repo#1", "ci-pending-first-seen:owner/repo#1", "fresh-rebase-forced:owner/repo#1"].sort(), + ); + // Scanned every configured pattern, not just a subset. + expect(redis.scanCalls.sort()).toEqual([...ORPHANED_LOCK_KEY_PATTERNS].sort()); + }); + + it("returns 0 and never throws when nothing matches", async () => { + const redis = fakeScanRedis(["delivery:only-this"]); + await expect(flushOrphanedLocksAtBoot(redis)).resolves.toBe(0); + }); + + it("is best-effort: a scan failure for one pattern never blocks the others or startup itself", async () => { + const redis = fakeScanRedis(["ai-review-lock:owner/repo#1@sha1:block", "contributor-cap-lock:owner/repo:alice"]); + const original = redis.scanStream.bind(redis); + redis.scanStream = ((opts: { match: string; count?: number }) => { + if (opts.match === "pr-actuation-lock:*") throw new Error("redis connection dropped"); + return original(opts); + }) as typeof redis.scanStream; + + await expect(flushOrphanedLocksAtBoot(redis)).resolves.toBe(2); // ai-review-lock + contributor-cap-lock still flushed + expect(redis._store.has("ai-review-lock:owner/repo#1@sha1:block")).toBe(false); + expect(redis._store.has("contributor-cap-lock:owner/repo:alice")).toBe(false); + }); +}); diff --git a/test/unit/transient-locks.test.ts b/test/unit/transient-locks.test.ts index 22c33adcbf..f60e0ea744 100644 --- a/test/unit/transient-locks.test.ts +++ b/test/unit/transient-locks.test.ts @@ -479,6 +479,38 @@ describe("domain wrappers + PrActuationLockContendedError (#8896)", () => { await releaseContributorCapLock(env, "Acme/Widgets", "Alice", cap.ownerToken); }); + // #9024: claimContributorCapLock's TTL was 30s -- sized only for the executor's brief + // contributorCapMergeRecheck() call -- but maybeCloseForContributorCapOnOpen holds this SAME lock across a + // much longer body (token mint, live GitHub calls, ensurePullRequestLabel, the nested pr-actuation-lock, a + // full executeAgentMaintenanceActions pass), which can exceed 30s under GitHub rate-limit backoff. Once Redis + // expired the lock mid-hold, a concurrent sibling's cap check could acquire it before the in-flight close + // landed -- the exact #7284 TOCTOU this lock exists to close. Now matches claimPrActuationLock's own 600s TTL. + it("#9024: claimContributorCapLock's TTL matches claimPrActuationLock's (both guard comparably long mutating bodies)", async () => { + const ttlCalls: number[] = []; + const env = createTestEnv({ + SELFHOST_TRANSIENT_CACHE: { + get: async () => null, + set: async () => undefined, + claim: async (_key, _value, ttlSeconds) => { + ttlCalls.push(ttlSeconds); + return true; + }, + releaseIfValue: async () => true, + }, + }); + delete env.SUBMISSION_LOCK; + + await claimPrActuationLock(env, "acme/widgets", 7); + const [actuationTtl] = ttlCalls; + ttlCalls.length = 0; + + await claimContributorCapLock(env, "acme/widgets", "alice"); + const [capTtl] = ttlCalls; + + expect(capTtl).toBe(actuationTtl); + expect(capTtl).toBe(600); + }); + it("builds a fast-retry contended error with a distinct retryKind", () => { const error = new PrActuationLockContendedError("acme/widgets", 3, "maintenance"); expect(error).toBeInstanceOf(PrActuationLockContendedError); diff --git a/turbo.json b/turbo.json index daf5dbc12b..b7dfeaf22a 100644 --- a/turbo.json +++ b/turbo.json @@ -106,8 +106,16 @@ // packages/loopover-miner/lib/** directory (not the 5 specific files) to match the same "don't // hand-track an ever-growing cross-package surface" reasoning already applied to //#typecheck's fix -- // this UI already imports 5 files from that one directory, more is a matter of when, not if. + // @loopover/engine#build is an explicit dependsOn edge for the same reason @loopover/ui#typecheck + // declares it above: those reached-into packages/loopover-miner/lib/** files import @loopover/engine, + // whose "types" resolve to packages/loopover-engine/dist/index.d.ts -- and miner-ui has no package.json + // dependency on engine to create the edge via ^build. Without it, turbo runs engine build CONCURRENTLY + // with this typecheck; on an engine-build cache miss the typecheck can race ahead of the dist emit and + // fail with phantom "Cannot find module '@loopover/engine'" errors (observed intermittently in CI's + // validate-code job -- a scheduling race, so it only bit when the engine cache missed AND the + // scheduler interleaved the two tasks the wrong way). "@loopover/ui-miner#typecheck": { - "dependsOn": ["^build"], + "dependsOn": ["^build", "@loopover/engine#build"], "inputs": ["$TURBO_DEFAULT$", "$TURBO_ROOT$/packages/loopover-miner/lib/**"], "outputs": [] },