From be4331217438842333b78fb2ccc5faf7f8da8fd0 Mon Sep 17 00:00:00 2001 From: CTO Date: Sun, 20 Sep 2026 04:09:42 +0000 Subject: [PATCH 1/3] fix(heartbeat): exclude concurrent runs sharing one project checkout (BLO-19422) Every arm of the writer-reservation predicate required isolation or a git worktree, so a `project_primary` (shared project checkout) run produced a null tree key and fell through to `run:` -- unique per run. Two such runs held distinct writer keys and both wrote the same directory, which is the measured defect (a torn read of an in-flight external edit failing a `go build` on trafficcontrol, 2026-09-17). `realizeExecutionWorkspace` returns `input.base.baseCwd` verbatim for the `project_primary` strategy, so every issue of one project workspace lands in ONE directory. Key that branch on the PROJECT WORKSPACE, not the issue: keying on the issue would reproduce the defect (two issues, two keys, one tree). Colliding DEFERS the second run with backoff carrying `conflictingRunId`; it does not fail it. Serializing runs that genuinely share one mutable directory is the correct outcome, not a degradation -- and handing each its own tree costs ~2.2 GB of node_modules per run on a volume already at 89%. Extracted to its own dependency-free module so the policy is testable without the heartbeat dependency graph. Pure refactor otherwise; the own-tree branch (BLO-31443) is unchanged. Co-Authored-By: Claude --- ...shared-checkout-writer-exclusivity.test.ts | 211 ++++++++++++++++++ server/src/services/heartbeat.ts | 68 +++--- server/src/services/workspace-writer-key.ts | 75 +++++++ 3 files changed, 313 insertions(+), 41 deletions(-) create mode 100644 server/src/__tests__/shared-checkout-writer-exclusivity.test.ts create mode 100644 server/src/services/workspace-writer-key.ts 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..cd01a81ac923 --- /dev/null +++ b/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts @@ -0,0 +1,211 @@ +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", () => { + // An unscoped run has no project workspace, so there is no shared project + // checkout to exclude on. Keying it would serialize unrelated runs. + 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. + const identityFor = (runId: string, treeKey: string | null) => + resolveK8sRunIsolationIdentity({ + adapterType: "claude_k8s", + runId, + agentId: "agent-1", + statelessPrReview: false, + isWorkspaceIsolated: false, + persistedExecutionWorkspaceId: null, + effectiveMaxConcurrentRuns: 3, + perIssueWorkspaceTreeKey: treeKey, + }); + + it("two shared-checkout runs land on ONE reservationKey", () => { + const treeKey = resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: { id: "issue-a", projectWorkspaceId: PW }, + }); + const first = identityFor("run-1", treeKey); + const second = identityFor("run-2", treeKey); + + expect(first?.reservationKey).toBe(`workspace-tree:project-primary:${PW}`); + expect(first?.reservationKey).toBe(second?.reservationKey); + // isolationKey stays run-private: it gates saved-session resume and names + // the run's own ephemeral roots, so widening it would let a run resume a + // session that is not under its own sessionRoot. + expect(first?.isolationKey).not.toBe(second?.isolationKey); + }); + + 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 test above fails and this one explains why. + const first = identityFor("run-1", null); + const second = identityFor("run-2", null); + expect(first?.reservationKey).not.toBe(second?.reservationKey); + }); +}); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 9cd25901f547..b1b29434747b 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, @@ -29027,47 +29028,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..e05d92320834 --- /dev/null +++ b/server/src/services/workspace-writer-key.ts @@ -0,0 +1,75 @@ +/** + * 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. + * + * 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` + * verbatim 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. + * + * 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. + */ +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}`; +} From 37e866f364869d545a9b7d597b51eeb5227e4593 Mon Sep 17 00:00:00 2001 From: CTO Date: Mon, 21 Sep 2026 12:17:28 +0000 Subject: [PATCH 2/3] fix(heartbeat): tree-scope the default shared reservation key (BLO-19422) The extracted tree key never reached the reservation on the DEFAULT path, so the ticket's headline case -- two different agents writing one shared project checkout -- was still unexcluded. `resolveK8sRunIsolationIdentity` applied the key on two of its four exits; the `agent-shared:` exit dropped it, and that exit is reached whenever `effectiveMaxConcurrentRuns <= 1`, which is every external-lifecycle agent unless an operator sets `concurrencyEnabled` (default false -> hard 1). Agent A and agent B on project workspace pw-1 therefore held `agent-shared:A` / `agent-shared:B`, both satisfied the writer index, and both wrote one directory. This REVERSES BLO-31443's AC4 lower bound, deliberately. Its rationale was that `agent-shared` is "already stricter than per-tree" and that widening it would invert BLO-16842's containment. Both halves are wrong: `agent-shared` is stricter along the AGENT axis and carries no tree scope at all, so it cannot exclude across agents; and the per-agent ceiling is enforced at dispatch by `availableSlots = effectiveMaxConcurrentRuns - runningCount`, not by this index, so widening the key cannot let an agent exceed its ceiling. Nor does it loosen the case that rationale named -- two runs of one agent on different issues of one project checkout both key `project-primary:` and still collide. `isolationKey` is untouched, so home/session roots and saved-session resume are unaffected; only `reservationKey` widens. The cost is explicit: this serializes all issues of a project workspace across all agents, which is correct for one mutable directory and does not touch runs that get their own worktree. Tests: the e2e helper hardcoded `effectiveMaxConcurrentRuns: 3`, so it exercised only the `> 1` exit -- the untested branch was the only branch that ships, which is how this survived. Parameterized over [1, 3] and added the cross-agent case the ticket is actually about. Verified by mutation: restoring the old guard fails both, and only at concurrency 1. Also records two gaps rather than asserting them away: `projectWorkspaceId` is backfilled by the first run, so run 1 of a fresh issue keys null and is unexcluded (no sound key exists before the workspace is realized, and the reservation must bind first); and `rebindProjectPrimaryToManagedCheckout` keys a managed checkout by project+repo rather than project workspace. Co-Authored-By: Claude --- ...xternal-lifecycle-concurrency-flag.test.ts | 53 ++++++- ...shared-checkout-writer-exclusivity.test.ts | 131 ++++++++++++++---- server/src/services/heartbeat.ts | 52 +++++-- server/src/services/workspace-writer-key.ts | 42 +++++- 4 files changed, 231 insertions(+), 47 deletions(-) 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..8acfef40944f 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,29 @@ 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 not this index's job. The per-agent ceiling is enforced at + // dispatch by `availableSlots = effectiveMaxConcurrentRuns - runningCount` + // in `startNextQueuedRunForAgent`, so widening the key cannot let an agent + // exceed its ceiling -- the slot counter never admits the second run. + // + // 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 +366,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 index cd01a81ac923..508715c1375c 100644 --- a/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts +++ b/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts @@ -148,8 +148,14 @@ describe("resolveWorkspaceWriterTreeKey", () => { }); it("returns null when there is nothing identifying the shared tree", () => { - // An unscoped run has no project workspace, so there is no shared project - // checkout to exclude on. Keying it would serialize unrelated runs. + // 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, @@ -170,42 +176,109 @@ describe("the key reaches the reservation (end-to-end through the resolver)", () // 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. - const identityFor = (runId: string, treeKey: string | null) => + // + // 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 added to + // that resolver must be reachable from this helper. + const identityFor = ( + runId: string, + treeKey: string | null, + opts: { agentId?: string; effectiveMaxConcurrentRuns: number }, + ) => resolveK8sRunIsolationIdentity({ adapterType: "claude_k8s", runId, - agentId: "agent-1", + agentId: opts.agentId ?? "agent-1", statelessPrReview: false, isWorkspaceIsolated: false, persistedExecutionWorkspaceId: null, - effectiveMaxConcurrentRuns: 3, + effectiveMaxConcurrentRuns: opts.effectiveMaxConcurrentRuns, perIssueWorkspaceTreeKey: treeKey, }); - it("two shared-checkout runs land on ONE reservationKey", () => { - const treeKey = resolveWorkspaceWriterTreeKey({ - statelessPrReview: false, - runResolvesToOwnTree: false, - usesPerRunScope: false, - issue: { id: "issue-a", projectWorkspaceId: PW }, - }); - const first = identityFor("run-1", treeKey); - const second = identityFor("run-2", treeKey); - - expect(first?.reservationKey).toBe(`workspace-tree:project-primary:${PW}`); - expect(first?.reservationKey).toBe(second?.reservationKey); - // isolationKey stays run-private: it gates saved-session resume and names - // the run's own ephemeral roots, so widening it would let a run resume a - // session that is not under its own sessionRoot. - expect(first?.isolationKey).not.toBe(second?.isolationKey); + const sharedCheckoutKey = (issueId: string) => resolveWorkspaceWriterTreeKey({ + statelessPrReview: false, + runResolvesToOwnTree: false, + usesPerRunScope: false, + issue: { id: issueId, projectWorkspaceId: PW }, }); - 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 test above fails and this one explains why. - const first = identityFor("run-1", null); - const second = identityFor("run-2", null); - expect(first?.reservationKey).not.toBe(second?.reservationKey); - }); + // 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 b1b29434747b..de98360bb234 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -6880,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, + ); } /** @@ -6930,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 @@ -6948,6 +6956,32 @@ 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 does not hold + * either: the per-agent concurrency ceiling is enforced at dispatch by + * `availableSlots = effectiveMaxConcurrentRuns - runningCount` in + * `startNextQueuedRunForAgent`, not by this index. Widening the key cannot let + * an agent exceed its ceiling, because the slot counter never admits the second + * run. `agent-shared` was a belt over braces that already hold. + * + * 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. */ @@ -6956,7 +6990,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}` }; } diff --git a/server/src/services/workspace-writer-key.ts b/server/src/services/workspace-writer-key.ts index e05d92320834..2930bb9a2904 100644 --- a/server/src/services/workspace-writer-key.ts +++ b/server/src/services/workspace-writer-key.ts @@ -25,9 +25,20 @@ * * - otherwise (`project_primary`, the SHARED project checkout) -> key on the * PROJECT WORKSPACE. `realizeExecutionWorkspace` returns `input.base.baseCwd` - * verbatim 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. + * 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 @@ -57,6 +68,31 @@ * 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; From 508b890abd0699fbd551bff2fef8bd2090e2e056 Mon Sep 17 00:00:00 2001 From: CTO Date: Mon, 21 Sep 2026 22:22:18 +0000 Subject: [PATCH 3/3] docs(heartbeat): the slot counter CAN admit a second run (BLO-19422) The reversal rationale on `withTreeScopedReservationKey` claimed widening the key costs nothing because "the slot counter never admits the second run". It can: `runningCount` comes from `countRunsOccupyingSlots`, which excludes silent runs (BLO-12990). One silent running row leaves `runningRunRows.length === 1` so the zero-rows guard does not fire, `runningCount` collapses to 0, and `availableSlots = 1 - 0 = 1` admits a second run at effective concurrency 1. There, `agent-shared:` was the sole restraint rather than a redundant one, and widening gives it up. This PR's own KNOWN GAP note on `resolveWorkspaceWriterTreeKey` already said so -- the two comments stated opposite things, and this one sits under a "Do not restore this" marker, so its reasoning is what the next reader will act on. Comments only; no behavior change. The trade is unchanged and still correct: the case lost needs a silent run AND an un-backfilled issue, the cross-agent case bought needs no loophole and is the measured default defect. Also: record that the single-writer guarantee depends on migration 0130's index being keyed on `isolation_key` alone -- adding `isolation_mode` would silently un-exclude the mixed-mode pairs this fix relies on, with every test still green. And scope the "any new exit must be reachable from this helper" invariant to the exits it actually reaches; it was untrue for two of four on the day it was written. Co-Authored-By: Claude --- ...-external-lifecycle-concurrency-flag.test.ts | 12 ++++++++---- .../shared-checkout-writer-exclusivity.test.ts | 8 ++++++-- server/src/services/heartbeat.ts | 17 ++++++++++++----- server/src/services/workspace-writer-key.ts | 8 ++++++++ 4 files changed, 34 insertions(+), 11 deletions(-) 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 8acfef40944f..33ad4ac1d57b 100644 --- a/server/src/__tests__/heartbeat-external-lifecycle-concurrency-flag.test.ts +++ b/server/src/__tests__/heartbeat-external-lifecycle-concurrency-flag.test.ts @@ -347,10 +347,14 @@ describe("resolveK8sRunIsolationIdentity: writer key follows the tree, not the r // `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 not this index's job. The per-agent ceiling is enforced at - // dispatch by `availableSlots = effectiveMaxConcurrentRuns - runningCount` - // in `startNextQueuedRunForAgent`, so widening the key cannot let an agent - // exceed its ceiling -- the slot counter never admits the second run. + // - 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 diff --git a/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts b/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts index 508715c1375c..3977315ee73e 100644 --- a/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts +++ b/server/src/__tests__/shared-checkout-writer-exclusivity.test.ts @@ -182,8 +182,12 @@ describe("the key reaches the reservation (end-to-end through the resolver)", () // 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 added to - // that resolver must be reachable from this helper. + // 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, diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index de98360bb234..ee210788dac0 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -6967,12 +6967,19 @@ export function resolveK8sRunIsolationIdentity(input: { * 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 does not hold - * either: the per-agent concurrency ceiling is enforced at dispatch by + * 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. Widening the key cannot let - * an agent exceed its ceiling, because the slot counter never admits the second - * run. `agent-shared` was a belt over braces that already hold. + * `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 diff --git a/server/src/services/workspace-writer-key.ts b/server/src/services/workspace-writer-key.ts index 2930bb9a2904..987b4cc4e687 100644 --- a/server/src/services/workspace-writer-key.ts +++ b/server/src/services/workspace-writer-key.ts @@ -13,6 +13,14 @@ * 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: *