diff --git a/server/src/__tests__/heartbeat-external-lifecycle-concurrency-flag.test.ts b/server/src/__tests__/heartbeat-external-lifecycle-concurrency-flag.test.ts index a79e3ed47513..33ad4ac1d57b 100644 --- a/server/src/__tests__/heartbeat-external-lifecycle-concurrency-flag.test.ts +++ b/server/src/__tests__/heartbeat-external-lifecycle-concurrency-flag.test.ts @@ -334,12 +334,33 @@ describe("resolveK8sRunIsolationIdentity: writer key follows the tree, not the r ).toEqual({ isolationMode: "run", isolationKey: "run:run-A", reservationKey: "run:run-A" }); }); - // AC4 lower bound. `agent-shared:` is already STRICTER than per-tree - // -- one writer per agent -- so substituting a tree key there would LOOSEN it - // and let a concurrency-1 agent hold two reservations for different issues, - // inverting BLO-16842's containment. Exclusivity at concurrency 1 comes from - // the shared key, not from this fix. - it("leaves the shared concurrency-1 key alone", () => { + // BLO-19422 REVERSES BLO-31443's AC4 lower bound. Do not "restore" this. + // + // This test used to assert the opposite -- that the concurrency-1 shared key + // is left alone -- on the reasoning that `agent-shared:` is "already + // STRICTER than per-tree, one writer per agent", and that widening it would + // invert BLO-16842's containment. Both halves were wrong: + // + // - `agent-shared` is stricter along the AGENT axis and carries NO tree scope, + // so it cannot exclude ACROSS agents. Agent A and agent B both running + // `project_primary` on one project workspace held `agent-shared:A` and + // `agent-shared:B`, both satisfied the writer index, and both wrote one + // directory -- BLO-19422's measured defect. Because `concurrencyEnabled` + // defaults false, this was the DEFAULT path, not an edge case. + // - Containment is mostly not this index's job. The per-agent ceiling is + // enforced at dispatch by `availableSlots = effectiveMaxConcurrentRuns - + // runningCount` in `startNextQueuedRunForAgent` -- except where BLO-12990 + // excludes a silent run from `countRunsOccupyingSlots`, which lets a second + // run in at effective concurrency 1. There, and only there, this key was + // doing real containment work and widening it gives that up. See the KNOWN + // GAP on `resolveWorkspaceWriterTreeKey`: that case needs a silent run AND + // an un-backfilled issue, where the cross-agent defect above needs neither. + // + // Nor does widening loosen the case the old rationale named: two runs of one + // agent on DIFFERENT issues of one project checkout both key + // `project-primary:` and still collide. Keys only diverge where the + // directories genuinely do. + it("tree-scopes the shared concurrency-1 reservation, keeping isolationKey agent-scoped", () => { expect( resolveK8sRunIsolationIdentity({ ...base, @@ -349,6 +370,30 @@ describe("resolveK8sRunIsolationIdentity: writer key follows the tree, not the r perIssueWorkspaceTreeKey: treeKey, effectiveMaxConcurrentRuns: 1, }), + ).toEqual({ + isolationMode: "shared", + // Unchanged, and load-bearing: `isolationKey` derives the home/session + // roots and gates saved-session resume. At concurrency 1 the agent keeps + // its warm shared roots; only `reservationKey` widens to the tree. + isolationKey: "agent-shared:agent-abc", + reservationKey: `workspace-tree:${treeKey}`, + }); + }); + + // The lower bound that DOES still hold: with no tree key there is nothing to + // scope to, so the run keeps the agent-scoped key rather than being handed a + // more permissive one. This is the un-backfilled-issue gap documented on + // `resolveWorkspaceWriterTreeKey` -- accepted, not overlooked. + it("keeps the agent-scoped key when there is no tree to scope to", () => { + expect( + resolveK8sRunIsolationIdentity({ + ...base, + runId: "run-A", + isWorkspaceIsolated: false, + persistedExecutionWorkspaceId: null, + perIssueWorkspaceTreeKey: null, + effectiveMaxConcurrentRuns: 1, + }), ).toEqual({ isolationMode: "shared", isolationKey: "agent-shared:agent-abc", reservationKey: "agent-shared:agent-abc" }); }); diff --git a/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts b/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts new file mode 100644 index 000000000000..3977315ee73e --- /dev/null +++ b/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts @@ -0,0 +1,288 @@ +import { describe, expect, it } from "vitest"; + +import { + resolveK8sRunIsolationIdentity, +} from "../services/heartbeat.js"; +import { resolveWorkspaceWriterTreeKey } from "../services/workspace-writer-key.js"; + +/** + * BLO-19422: two concurrent runs that resolve to the SAME on-disk checkout must + * not both hold a writer reservation. + * + * The single-writer guarantee is an index on `external_runtime_reservations` + * (`..._active_isolation_writer_idx`) keyed by the reservation key, so the whole + * property reduces to: do two runs sharing a tree produce the SAME key? These + * tests assert on the key rather than on the index, because the key derivation + * is the half that was wrong -- the index has worked correctly throughout. + * + * The defect these lock down: every arm of the original predicate required + * isolation or a git worktree, so a `project_primary` (shared project checkout) + * run produced a null key and fell through to `run:` -- unique per run. + * Two such runs held distinct keys and both wrote one directory. + */ + +const PW = "pw-1"; +const OTHER_PW = "pw-2"; + +describe("resolveWorkspaceWriterTreeKey", () => { + describe("shared project checkout (project_primary) — the BLO-19422 defect", () => { + it("gives two DIFFERENT issues sharing one project checkout the SAME key", () => { + // This is the exact measured shape: `shared_workspace` mode, no worktree, + // two issues, one directory. Before the fix both sides were null. + const a = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: { id: "issue-a", projectWorkspaceId: PW }, + }); + const b = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: { id: "issue-b", projectWorkspaceId: PW }, + }); + + expect(a).not.toBeNull(); + expect(a).toBe(b); + }); + + it("does NOT key on the issue, so the key cannot vary per issue", () => { + // Guards the specific wrong fix: reusing the own-tree `pw:issue` form here + // would look plausible, pass a naive "key is non-null" test, and still let + // two issues write one tree. + const key = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: { id: "issue-a", projectWorkspaceId: PW }, + }); + expect(key).not.toContain("issue-a"); + expect(key).toBe(`project-primary:${PW}`); + }); + + it("keeps DIFFERENT project workspaces independent", () => { + const a = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: { id: "issue-a", projectWorkspaceId: PW }, + }); + const b = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: { id: "issue-a", projectWorkspaceId: OTHER_PW }, + }); + expect(a).not.toBe(b); + }); + + it("still collides when a per_run runScope sits on a NON-worktree strategy", () => { + // `per_run` appends a run token to the BRANCH, so it only makes a run + // tree-unique when a worktree is actually cut. Under project_primary no + // branch is derived, the runs share the base checkout anyway, and + // excluding them here would reopen the defect for exactly that config. + const a = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: true, + issue: { id: "issue-a", projectWorkspaceId: PW }, + }); + const b = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: true, + issue: { id: "issue-b", projectWorkspaceId: PW }, + }); + expect(a).toBe(`project-primary:${PW}`); + expect(a).toBe(b); + }); + }); + + describe("own tree (worktree / isolated / reused) — unchanged by BLO-19422", () => { + it("keys on the issue so two issues do NOT serialize", () => { + const a = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: true, + usesPerRunScope: false, + issue: { id: "issue-a", projectWorkspaceId: PW }, + }); + const b = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: true, + usesPerRunScope: false, + issue: { id: "issue-b", projectWorkspaceId: PW }, + }); + expect(a).toBe(`${PW}:issue-a`); + expect(a).not.toBe(b); + }); + + it("collides for two runs of ONE issue (the BLO-31443 guarantee)", () => { + const of = (id: string) => resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: true, + usesPerRunScope: false, + issue: { id, projectWorkspaceId: PW }, + }); + expect(of("issue-a")).toBe(of("issue-a")); + }); + + it("does not key a per_run run, which is tree-unique by construction", () => { + expect(resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: true, + usesPerRunScope: true, + issue: { id: "issue-a", projectWorkspaceId: PW }, + })).toBeNull(); + }); + }); + + it("never keys a stateless PR review, on either branch", () => { + for (const runResolvesToOwnTree of [true, false]) { + expect(resolveWorkspaceWriterTreeKey({ + statelessPrReview: true, + runResolvesToOwnTree, + usesPerRunScope: false, + issue: { id: "issue-a", projectWorkspaceId: PW }, + })).toBeNull(); + } + }); + + it("returns null when there is nothing identifying the shared tree", () => { + // NOT "there is no shared checkout to exclude on" -- there usually is one, + // and this is a known gap rather than a safe case. `projectWorkspaceId` is + // only backfilled onto the issue AFTER the first run realizes a workspace + // (`issueRef?.projectWorkspaceId ?? resolvedWorkspace.workspaceId`), so the + // first run of a fresh issue into a shared checkout keys null and is not + // excluded. Accepted deliberately: the reservation must bind before the + // workspace is realized, so no sound key exists at this point. See the + // KNOWN GAP note on `resolveWorkspaceWriterTreeKey`. + expect(resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: null, + })).toBeNull(); + expect(resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: { id: "issue-a", projectWorkspaceId: null }, + })).toBeNull(); + }); +}); + +describe("the key reaches the reservation (end-to-end through the resolver)", () => { + // The key above is only worth anything if `resolveK8sRunIsolationIdentity` + // actually puts it on `reservationKey` -- that is the field bound to the + // single-writer index. Asserting the key alone would pass even if the + // resolver dropped it, which is how the shared-checkout case stayed broken. + // + // Parameterized over `effectiveMaxConcurrentRuns` deliberately. An earlier + // revision hardcoded 3, which exercised only the `> 1` exit and hid a live + // defect: the resolver DID drop the key at 1, which is the DEFAULT for every + // external-lifecycle agent (`concurrencyEnabled` is false unless an operator + // sets it, and `resolveExternalLifecycleConcurrency` then returns a hard 1). + // The untested branch was the only branch that ships. Any new exit reachable + // with `isWorkspaceIsolated: false` and no persisted workspace must be + // exercised here. The other two exits are NOT reachable from this helper -- + // stateless PR review and the persisted-`workspace` exit both need inputs + // this helper hardcodes; the latter is covered in + // `heartbeat-external-lifecycle-concurrency-flag.test.ts`. + const identityFor = ( + runId: string, + treeKey: string | null, + opts: { agentId?: string; effectiveMaxConcurrentRuns: number }, + ) => + resolveK8sRunIsolationIdentity({ + adapterType: "claude_k8s", + runId, + agentId: opts.agentId ?? "agent-1", + statelessPrReview: false, + isWorkspaceIsolated: false, + persistedExecutionWorkspaceId: null, + effectiveMaxConcurrentRuns: opts.effectiveMaxConcurrentRuns, + perIssueWorkspaceTreeKey: treeKey, + }); + + const sharedCheckoutKey = (issueId: string) => resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: { id: issueId, projectWorkspaceId: PW }, + }); + + // 1 is the default posture; 3 is an operator who opted into concurrency. + for (const effectiveMaxConcurrentRuns of [1, 3]) { + describe(`effectiveMaxConcurrentRuns: ${effectiveMaxConcurrentRuns}`, () => { + it("two shared-checkout runs land on ONE reservationKey", () => { + const treeKey = sharedCheckoutKey("issue-a"); + const first = identityFor("run-1", treeKey, { effectiveMaxConcurrentRuns }); + const second = identityFor("run-2", treeKey, { effectiveMaxConcurrentRuns }); + + expect(first?.reservationKey).toBe(`workspace-tree:project-primary:${PW}`); + expect(first?.reservationKey).toBe(second?.reservationKey); + // isolationKey stays private: it gates saved-session resume and names + // the run's own roots, so widening it would let a run resume a session + // that is not under its own sessionRoot. At concurrency 1 it stays + // agent-scoped, which is what keeps the warm shared home/session roots. + expect(first?.isolationKey).toBe( + effectiveMaxConcurrentRuns > 1 ? "run:run-1" : "agent-shared:agent-1", + ); + }); + + it("excludes TWO DIFFERENT AGENTS sharing one project checkout", () => { + // BLO-19422's headline case, and the one no earlier test covered. The + // pre-fix keys were `agent-shared:A` / `agent-shared:B` -- distinct, + // both admitted by the writer index, both writing one directory. + // Different issues too, because that is the measured shape: the + // project checkout is shared across issues AND across agents. + const first = identityFor("run-1", sharedCheckoutKey("issue-a"), { + agentId: "agent-1", + effectiveMaxConcurrentRuns, + }); + const second = identityFor("run-2", sharedCheckoutKey("issue-b"), { + agentId: "agent-2", + effectiveMaxConcurrentRuns, + }); + + expect(first?.reservationKey).toBe(second?.reservationKey); + }); + + it("keeps different project workspaces independent across agents", () => { + // The other half of the contract: serializing runs that do NOT share a + // directory would be a throughput regression, not a fix. + const first = identityFor("run-1", `project-primary:${PW}`, { + agentId: "agent-1", + effectiveMaxConcurrentRuns, + }); + const second = identityFor("run-2", `project-primary:${OTHER_PW}`, { + agentId: "agent-2", + effectiveMaxConcurrentRuns, + }); + + expect(first?.reservationKey).not.toBe(second?.reservationKey); + }); + + it("regression: a null key leaves both runs writing one tree unexcluded", () => { + // Pins the pre-fix behaviour as the thing being prevented. If a future + // change makes resolveWorkspaceWriterTreeKey return null for the shared + // checkout again, the tests above fail and this one explains why. + // + // At concurrency 1 both runs fall back to `agent-shared:`, so + // two runs of ONE agent still collide -- assert across agents, which is + // the pairing that genuinely goes unexcluded on a null key. That is the + // known un-backfilled-issue gap documented on `resolveWorkspaceWriter + // TreeKey`, not an oversight. + const first = identityFor("run-1", null, { + agentId: "agent-1", + effectiveMaxConcurrentRuns, + }); + const second = identityFor("run-2", null, { + agentId: "agent-2", + effectiveMaxConcurrentRuns, + }); + expect(first?.reservationKey).not.toBe(second?.reservationKey); + }); + }); + } +}); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 9cd25901f547..ee210788dac0 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -364,6 +364,7 @@ import { } from "./issue-tree-control.js"; import { RUN_STALE_SILENCE_MS } from "./issue-run-holding.js"; import { describeSharedCheckoutOccupancy } from "./shared-checkout-occupancy.js"; +import { resolveWorkspaceWriterTreeKey } from "./workspace-writer-key.js"; import { countRunsOccupyingSlots, resolveAgentConcurrencyPolicy, @@ -6879,7 +6880,18 @@ export function resolveK8sRunIsolationIdentity(input: { input.perIssueWorkspaceTreeKey, ); } - return runUniqueIdentity({ isolationMode: "shared", isolationKey: `agent-shared:${input.agentId}` }); + // BLO-19422: this is the DEFAULT exit -- `concurrencyEnabled` is false unless + // an operator sets it, so `effectiveMaxConcurrentRuns` is a hard 1 for every + // external-lifecycle agent and every such run lands here. `agent-shared: + // ` carries no tree scope, so two AGENTS on one shared project + // checkout held distinct keys and both wrote it. Tree-scope the reservation + // here too; `isolationKey` stays agent-scoped so warm session/home roots are + // untouched. See `withTreeScopedReservationKey` for why the old "shared is + // already stricter" reasoning was wrong. + return withTreeScopedReservationKey( + { isolationMode: "shared", isolationKey: `agent-shared:${input.agentId}` }, + input.perIssueWorkspaceTreeKey, + ); } /** @@ -6929,14 +6941,11 @@ export function resolveK8sRunIsolationIdentity(input: { * this key gates. * * THE INVARIANT for the reservation key: only ever replace a key that is - * RUN-UNIQUE. A key that already names a shared tree is at least as strict as - * the per-issue key, so substituting it would loosen exclusivity instead of - * tightening it. Three keys are therefore left alone, each for its own reason: + * RUN-UNIQUE, or one whose scope is NARROWER THAN THE TREE. A key that already + * names a shared tree is at least as strict as the per-issue key, so + * substituting it would loosen exclusivity instead of tightening it. Two keys + * are therefore left alone: * - * - `shared` (`agent-shared:`) is already STRICTER than per-tree — one - * writer per agent. Substituting a per-tree key there would *loosen* it and - * let an effective-concurrency-1 agent hold two reservations for different - * issues, inverting BLO-16842's containment. * - `workspace:` for an EXPLICITLY reused persisted workspace already names * the tree, and several issues may share one such workspace, so a per-issue * key would let those issues write it concurrently. Gated at the call site in @@ -6947,6 +6956,39 @@ export function resolveK8sRunIsolationIdentity(input: { * - stateless PR review never reaches this helper; it returns run-scoped * isolation ahead of every other branch and must stay fully ephemeral. * + * BLO-19422: `shared` (`agent-shared:`) USED TO BE left alone here, on + * the reasoning that one-writer-per-agent is already stricter than per-tree. + * That was wrong, and it is the whole defect. `agent-shared` is stricter along + * the AGENT axis and carries no tree scope at all, so it cannot exclude ACROSS + * agents: agent A and agent B both running `project_primary` against project + * workspace pw-1 hold `agent-shared:A` and `agent-shared:B`, both satisfy the + * writer index, and both write one directory. That is BLO-19422 verbatim, and + * because `concurrencyEnabled` defaults false (`resolveExternalLifecycle + * Concurrency` returns a hard 1), this exit is the DEFAULT posture rather than + * an edge case. + * + * The "inverting BLO-16842's containment" half of that rationale is weaker than + * it looks, but it is NOT free: the per-agent ceiling is enforced at dispatch by + * `availableSlots = effectiveMaxConcurrentRuns - runningCount` in + * `startNextQueuedRunForAgent`, not by this index -- EXCEPT where BLO-12990 + * excludes a silent run from `countRunsOccupyingSlots`. One silent running row + * leaves `runningRunRows.length === 1` (so the zero-rows guard does not fire) + * while `runningCount` collapses to 0, so `availableSlots = 1 - 0 = 1` and a + * second run IS admitted at effective concurrency 1. In exactly that case + * `agent-shared` was not a belt over braces -- it was the sole restraint, and + * widening the key gives it up. That narrow loss is stated as a KNOWN GAP on + * `resolveWorkspaceWriterTreeKey`; it needs a silent run AND an un-backfilled + * issue, where the cross-agent case this buys needs no loophole at all and is + * the measured default defect. The trade is deliberate, not an oversight. + * + * The cost is real and deliberate: this serializes ALL issues of one project + * workspace across ALL agents, because they are one mutable directory. That is + * the correct outcome for a shared checkout, and it is the same trade the + * module doc makes -- serializing runs that could have been parallel costs + * latency, letting two runs share one tree corrupts a checkout. It does not + * touch runs that get their own worktree: those key on the ISSUE and stay + * independent across issues. + * * `isolationMode` is untouched, so every filesystem root keeps deriving from * `runId`/`persistedExecutionWorkspaceId` exactly as before. */ @@ -6955,7 +6997,7 @@ function withTreeScopedReservationKey( perIssueWorkspaceTreeKey: string | null | undefined, ): K8sRunIsolationIdentity { const treeKey = readNonEmptyString(perIssueWorkspaceTreeKey ?? null); - if (!treeKey || identity.isolationMode === "shared") return runUniqueIdentity(identity); + if (!treeKey) return runUniqueIdentity(identity); return { ...identity, reservationKey: `workspace-tree:${treeKey}` }; } @@ -29027,47 +29069,32 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) ? randomUUID() : null ); - // BLO-31443: the writer reservation must exclude on the TREE this run will - // work in, not on the run. The literal `cwd` is unavailable here -- it needs - // `repoRoot` from a `git rev-parse` against `resolvedWorkspace.cwd`, which is - // not resolved until ~700 lines below, and the reservation has to be bound - // before the workspace is realized. Under `per_issue` runScope the resolved - // path is a pure function of the issue (identifier + title -> branch name -> - // directory, with no run input), so the issue IS the equivalence class of - // that path: keying on it collides exactly when two runs would share a tree. - // - // Scoped by `projectWorkspaceId` because one issue can hold trees in several - // repos of a multi-repo project, and those are genuinely independent. - // - // Two deliberate exclusions: - // - `per_run` runScope appends a run token to the branch, hence to the - // directory, so those runs are already tree-unique and must NOT collide. - // - a stateless PR review is run-unique by construction and is filtered in - // the resolver ahead of every other branch. - // - // Conservative in the one case where issue and path disagree: an issue - // retitled between runs resolves to a NEW directory while keeping its id, so - // this over-serializes rather than under-serializes. Serializing two runs - // that could have been parallel costs latency; letting two runs share one - // tree corrupts a checkout. - const perIssueWorkspaceTreeKey = - issueRef?.id && - paperclipPrReview === null && - !executionWorkspaceUsesPerRunScopeForIssue && - ( - workspaceIsolationRequested || - workspaceReuseRequest.existingExecutionWorkspaceAvailable || - executionWorkspaceUsesGitWorktree({ - agentConfig: config, - projectPolicy: projectExecutionWorkspacePolicy, - issueSettings: issueExecutionWorkspaceSettings, - mode: requestedExecutionWorkspaceMode, - legacyUseProjectWorkspace: issueAssigneeOverrides?.useProjectWorkspace ?? null, - issueAdapterConfig: issueAssigneeOverrides?.adapterConfig ?? null, - }) - ) - ? `${issueRef.projectWorkspaceId ?? "no-project-workspace"}:${issueRef.id}` - : null; + // BLO-31443 / BLO-19422: the writer reservation must exclude on the TREE this + // run will work in, not on the run. The literal `cwd` is unavailable here -- + // it needs `repoRoot` from a `git rev-parse` against `resolvedWorkspace.cwd`, + // which is not resolved until ~700 lines below, and the reservation has to be + // bound before the workspace is realized. So the key is derived from the + // equivalence class of that path instead; see `resolveWorkspaceWriterTreeKey` + // for which class applies to which shape and why. + const runResolvesToOwnTree = + workspaceIsolationRequested || + workspaceReuseRequest.existingExecutionWorkspaceAvailable || + executionWorkspaceUsesGitWorktree({ + agentConfig: config, + projectPolicy: projectExecutionWorkspacePolicy, + issueSettings: issueExecutionWorkspaceSettings, + mode: requestedExecutionWorkspaceMode, + legacyUseProjectWorkspace: issueAssigneeOverrides?.useProjectWorkspace ?? null, + issueAdapterConfig: issueAssigneeOverrides?.adapterConfig ?? null, + }); + const perIssueWorkspaceTreeKey = resolveWorkspaceWriterTreeKey({ + statelessPrReview: paperclipPrReview !== null, + runResolvesToOwnTree, + usesPerRunScope: executionWorkspaceUsesPerRunScopeForIssue, + issue: issueRef + ? { id: issueRef.id ?? null, projectWorkspaceId: issueRef.projectWorkspaceId ?? null } + : null, + }); const k8sIsolationIdentity = resolveK8sRunIsolationIdentity({ adapterType: agent.adapterType, runId: run.id, diff --git a/server/src/services/workspace-writer-key.ts b/server/src/services/workspace-writer-key.ts new file mode 100644 index 000000000000..987b4cc4e687 --- /dev/null +++ b/server/src/services/workspace-writer-key.ts @@ -0,0 +1,119 @@ +/** + * BLO-31443 / BLO-19422: the equivalence class of the working tree a run will + * write, used as the single-writer reservation key + * (`external_runtime_reservations_active_isolation_writer_idx`). + * + * Deliberately dependency-free and in its own module: it is pure policy, it is + * the half of the shared-checkout guarantee that has been wrong twice, and + * keeping it importable without the heartbeat dependency graph is what lets it + * be tested directly. + * + * The contract is one line: two runs that would share a directory must produce + * the SAME string; two runs that would not must produce different ones. `null` + * means "nothing shared to exclude on" and leaves the run's own run-unique key + * in place. + * + * That contract depends on the index being keyed on `isolation_key` ALONE + * (migration 0130: `ON (isolation_key) WHERE released_at IS NULL AND + * isolation_key IS NOT NULL`). Adding `isolation_mode` to it would silently + * un-exclude every mixed-mode pair this relies on -- agent A at concurrency 1 + * (mode `shared`) and agent B at concurrency 3 (mode `run`) on one project + * workspace produce the same key and MUST collide. Every test here would still + * pass while that case stayed broken. + * + * The class depends on which shape the run resolves to, and the two shapes key + * on different things: + * + * - `runResolvesToOwnTree` (git worktree / isolated / explicitly reused + * workspace) -> key on the ISSUE. Under `per_issue` runScope the path is a + * pure function of the issue (identifier + title -> branch name -> directory, + * no run input), so the issue is exactly that class. Scoped by + * `projectWorkspaceId` because one issue can hold trees in several repos of a + * multi-repo project and those are genuinely independent. + * + * - otherwise (`project_primary`, the SHARED project checkout) -> key on the + * PROJECT WORKSPACE. `realizeExecutionWorkspace` returns `input.base.baseCwd` + * for every non-`git_worktree` strategy, so every issue of that project + * workspace lands in ONE directory. Keying on the issue here would reproduce + * the defect: two issues, two keys, one tree. + * + * One caveat on that "returns baseCwd" claim, because it is load-bearing for + * anyone deciding what this key means: `rebindProjectPrimaryToManagedCheckout` + * can substitute a managed checkout resolved from `(companyId, projectId, + * repoName)` -- keyed by PROJECT + REPO, not by project workspace. Two project + * workspaces of one project pointing at one repo URL would therefore rebind to + * a single directory while holding two distinct keys here. Not reachable in + * any config today, and deliberately not defended against: keying on the + * project instead would over-serialize unrelated workspaces in every config + * that IS reachable. If that config ever becomes reachable, this key is the + * thing that has to change. + * + * BLO-19422 is that second branch. Every arm of the original predicate required + * isolation or a worktree, so a shared-checkout run produced a null key and fell + * through to `run:` in the resolver -- unique per run. Two such runs held + * distinct writer keys and both wrote the same tree, which is the measured + * defect (a torn read of an in-flight external edit failing a `go build`). + * + * Colliding DEFERS the second run -- `deferRunForK8sIsolationConflict` re-queues + * it with backoff carrying `conflictingRunId` -- it does not fail it. That is + * the deliberately cheap arm of the fix. Handing each run its own tree instead + * costs a worktree plus a full dependency install per concurrent run (measured + * on this repo: ~105 MB of tree and ~2.2 GB of node_modules), which is not + * affordable on a volume already at 88%. Serializing runs that genuinely share + * one mutable directory is the correct outcome, not a degradation. + * + * Two exclusions, and note the asymmetry between them: + * - a stateless PR review is run-unique by construction (and is filtered in + * `resolveK8sRunIsolationIdentity` ahead of every other branch), so it never + * keys. + * - `per_run` runScope excludes ONLY on the own-tree branch, where it appends a + * run token to the branch and hence to the directory. Under `project_primary` + * no branch or directory is derived at all, so `runScope: "per_run"` sitting + * on a non-worktree strategy does NOT make the run tree-unique -- those runs + * still share the base checkout and must still collide. + * + * Conservative where issue and path disagree: an issue retitled between runs + * resolves to a NEW directory while keeping its id, so this over-serializes + * rather than under-serializes. Serializing two runs that could have been + * parallel costs latency; letting two runs share one tree corrupts a checkout. + * + * KNOWN GAP -- the first run of an un-backfilled issue is UNPROTECTED, and this + * is accepted rather than fixed. The only source for `projectWorkspaceId` at + * reservation-bind time is `issueRef`, but the run's actual workspace is not + * resolved until ~600 lines later (`issueRef?.projectWorkspaceId ?? + * resolvedWorkspace.workspaceId`) and is written back onto the issue after + * that. The id is a RESULT of the first run, not a precondition of it, so run 1 + * of a fresh issue keys null and only run 2 onward is excluded. + * + * Two consequences worth stating, because the second is a real (narrow) loss: + * + * - There is no sound fix available at bind time. Every candidate is a proxy + * with its own gap, and the reservation MUST bind before the workspace is + * realized -- binding after it would mean the loser has already mutated the + * tree it was supposed to be excluded from. Closing this properly means + * hoisting the workspace-base resolution above the bind, which is a dispatch- + * path change and not this row's scope. + * - Now that the `agent-shared` exit is tree-scoped too (BLO-19422), a null key + * falls back to `agent-shared:` and therefore no longer collides + * with the SAME agent's tree-keyed runs on that tree. Pre-BLO-19422 it did. + * That window needs two concurrent runs of one agent at effective + * concurrency 1, which requires the BLO-12990 silent-run exclusion from + * `countRunsOccupyingSlots`, and one of the two to be an un-backfilled first + * run. It is strictly narrower than the cross-agent case it buys: that one + * needs no loophole at all and is the measured defect. + */ +export function resolveWorkspaceWriterTreeKey(input: { + statelessPrReview: boolean; + runResolvesToOwnTree: boolean; + usesPerRunScope: boolean; + issue: { id: string | null; projectWorkspaceId: string | null } | null; +}): string | null { + if (input.statelessPrReview) return null; + if (input.runResolvesToOwnTree) { + if (input.usesPerRunScope) return null; + if (!input.issue?.id) return null; + return `${input.issue.projectWorkspaceId ?? "no-project-workspace"}:${input.issue.id}`; + } + if (!input.issue?.projectWorkspaceId) return null; + return `project-primary:${input.issue.projectWorkspaceId}`; +}