From 35f272e7a2985d47ce906f58056534c17e74c30b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 12:10:35 -0700 Subject: [PATCH 1/2] fix(orb): flush orphaned locks at boot, restore sweep candidacy on resume, and widen the contributor-cap-lock TTL Three related lock/liveness fixes: #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. Left alone, each class strands real work for its own TTL (30 min for ai-review-lock, #8998). Adds flushOrphanedLocksAtBoot (selfhost/redis-cache.ts), a best-effort SCAN-delete over the four exclusivity-lock prefixes, wired in at server boot before the queue starts. Deliberately leaves delivery:*, pr-panel-retrigger-pending:*, ci-pending-first-seen:*, and fresh-rebase-forced:* untouched -- none are exclusivity locks, and each has its own restart-survival contract. #9018 -- a paused repo's PRs can go green DURING the pause window (CI- completion passes plan-and-suppress the whole time), and resuming performed no catch-up: if a PR was ALSO regated once before the pause, agent-sweep.ts's #never-endless-reregate rule permanently excludes it from future sweep candidacy, stranding it silently. Adds clearPullRequestsRegatedAtForOpenPrs and calls it on the paused->live transition, both from the single-repo MCP pause/resume tool and the installation-wide bulk-settings route -- restoring one-shot sweep candidacy for the repo's open PRs exactly once. #9024 -- claimContributorCapLock's 30s TTL was sized only for the executor's brief pre-merge recheck, but maybeCloseForContributorCapOnOpen holds the SAME lock across a much longer body (token mint, live GitHub calls, label writes, the nested pr-actuation-lock, a full executeAgentMaintenanceActions pass) that can exceed 30s under GitHub rate-limit backoff -- reopening the exact #7284 TOCTOU this lock exists to close. Widened to 600s, matching claimPrActuationLock's own TTL for a comparably long mutating body. Closes #9018, #9021, #9024 Tests: 3 new flushOrphanedLocksAtBoot tests (deletes matching keys, returns 0 with nothing to do, fails open per-pattern on a scan error), a paused->live MCP-tool test (clears open PRs' markers, never closed ones, never on pause or a repeat resume), two bulk-settings-route tests (clears across all repos in an installation; a non-agentPaused bulk change never touches the marker), and a TTL-parity regression pinning claimContributorCapLock's TTL to claimPrActuationLock's. 100% line+branch coverage on every changed line (src/server.ts is Codecov's own documented ignore-listed entrypoint file). --- src/api/routes.ts | 8 ++ src/db/repositories.ts | 16 ++++ src/mcp/server.ts | 10 +++ src/queue/transient-locks.ts | 10 ++- src/selfhost/redis-cache.ts | 37 ++++++++ src/server.ts | 13 ++- test/unit/mcp-automation-state.test.ts | 36 +++++++- ...s-installation-bulk-agent-settings.test.ts | 89 +++++++++++++++++++ test/unit/selfhost-redis-cache.test.ts | 75 ++++++++++++++++ test/unit/transient-locks.test.ts | 32 +++++++ 10 files changed, 323 insertions(+), 3 deletions(-) create mode 100644 test/unit/routes-installation-bulk-agent-settings.test.ts 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); From 6c77c73d45997de3e2440dc92a59f826cff68e26 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 12:14:12 -0700 Subject: [PATCH 2/2] fix(ci): add the missing engine-build edge to @loopover/ui-miner#typecheck The miner-UI typecheck reaches into packages/loopover-miner/lib/** (declared as an input), and those files import @loopover/engine -- whose types resolve to packages/loopover-engine/dist/index.d.ts. But unlike @loopover/ui#typecheck (which declares the explicit @loopover/engine#build dependsOn edge for exactly this reason), the miner-UI task only had ^build, and miner-ui has no package.json dependency on engine to create that edge implicitly. Turbo therefore ran engine build CONCURRENTLY with this typecheck; whenever the engine cache missed AND the scheduler interleaved the two the wrong way, the typecheck raced ahead of the dist emit and failed with dozens of phantom "Cannot find module '@loopover/engine'" errors -- an intermittent, whole-job validate-code failure with no real defect behind it (observed live on #9107's first run and on main run 30214733191, while sibling runs of the identical commit passed). --- turbo.json | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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": [] },