From a5e7be339dd31f6401c187b04a5b7dc3bbcc1321 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Sun, 23 Aug 2026 22:31:57 +0200 Subject: [PATCH] fix(factory): skip the orphan-recovery context on a dry-run sweep (#348) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `#githubOrphanRecoveryContext()` read `fleet.roster()` whenever the issue source is GitHub, regardless of `dryRun`. Since #343 a read-only CLI client refuses to mint a workspace identity, and `roster()` mints on demand — so the container's start-disabled `run-once --dry-run` gate logged `could not establish orphan-recovery safety context` on every sweep. The context was already provably unused on that path: `mayRecoverGithubOrphan` in `#performRunOnce` is `!dryRun`, and `#reconcileOrphanedGithubInProgress` refuses under `dryRun` before it so much as looks at the context. Gathering it was not free, though — it required an identity to decide what the sweep WOULD do. Now a dry run skips it and the report says so, via a new `IterationReport.orphanRecoveryDegraded` that distinguishes the benign `dry-run` skip from a live sweep's real `context-unavailable` degradation. The failure counter and warn stay for the live path, where an absent context IS a degradation. Tests are a must-fire / must-not-fire pair plus a control, all on one fixture: a dry-run sweep does not read the roster (measured with a fleet that answers normally, so the zero is the guard and not a refusal); a live sweep still reads it and still recovers the orphan; and a dry run against a client shaped like #343's refusal completes rather than logging a failure. Ablating the guard fails both must-not-fire arms; inverting it fails all three. NOT fixed here, and #348's diagnosis is wrong about it: this does not bring `run-once --dry-run` under its 10-minute budget, because #343 never took it over. See the PR comment for the measurement. Co-Authored-By: Claude Opus 5 --- src/orchestrator/factory.test.ts | 141 +++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 48 ++++++++++- src/types.ts | 11 +++ 3 files changed, 197 insertions(+), 3 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 5da863ed..80177353 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -5049,6 +5049,147 @@ describe('FactoryLoop', () => { } }) + // factory#348. `#343` gave read-only CLI commands a fleet client that refuses + // to mint a workspace identity, and `#githubOrphanRecoveryContext` read + // `fleet.roster()` regardless of `dryRun` — so a GitHub-backed + // `run-once --dry-run` (the container's start-disabled pre-cutover gate) hit + // the refusal and logged a safety-context failure on every sweep. The context + // was provably unused on that path: `mayRecoverGithubOrphan` is `!dryRun` and + // `#reconcileOrphanedGithubInProgress` refuses under `dryRun` before reading + // it. Deciding what a sweep WOULD do must not need an identity to do it with. + describe('orphan-recovery safety context under a read-only fleet client', () => { + // Counts the roster read without changing its answer, so the must-not-fire + // arm below measures the guard and not a refusal. + class RosterCountingFleet extends FakeFleetClient { + rosterCalls = 0 + + override async roster(): ReturnType { + this.rosterCalls += 1 + return await super.roster() + } + } + + // The production shape of #343's refusal: fleet reads are fine once an + // identity exists, but minting one is refused — and `roster()` mints on + // demand. + class ReadOnlyRosterFleet extends FakeFleetClient { + rosterCalls = 0 + + override async roster(): Promise { + this.rosterCalls += 1 + throw new Error( + 'Refusing to register relay agent "factory-cloud-1c88bf23": this fleet client is read-only.', + ) + } + } + + // One orphan-shaped issue: `factory:in-progress`, no live agent, a stranded + // in-flight dispatch row. Live, this is exactly what orphan recovery exists + // to release; dry, it is what gets preserved. + const orphanFixture = async (): Promise<{ + root: string + mount: FakeMountClient + stateStore: InMemoryStateStore + }> => { + const root = await mkdtemp(join(tmpdir(), 'factory-348-dryrun-')) + const path = githubIssuePath('AgentWorkforce', 'pear', 348) + const payload = githubIssueFile(348, { labels: ['factory', 'pear', 'factory:in-progress'] }) + const mount = new FakeMountClient({ [path]: payload }) + mount.setSubRoot('/linear/issues', 'absent') + const stateStore = new InMemoryStateStore({ batchSize: 4 }) + const issue = parseGithubFactoryIssue(path, payload) + await stateStore.recordDispatchAttempt('factory-test', issueKey(issue), { + attempts: 1, + inFlight: true, + terminal: false, + backoffUntilMs: 0, + }) + return { root, mount, stateStore } + } + + const factoryFor = ( + root: string, + mount: FakeMountClient, + fleet: FakeFleetClient, + stateStore: InMemoryStateStore, + ): ReturnType => + createFactory(config({ + issueSource: 'github', + loop: { registryPath: join(root, 'registry.json') }, + }), { + mount, + fleet, + stateStore, + triage: new StaticTriage(), + githubWriteback: new RecordingGithubWriteback(), + probePrGhRunner: async () => ({ stdout: '[]' }), + }) + + // MUST NOT FIRE. The roster read is skipped because the sweep is dry, not + // because the client refused it: this fleet answers `roster()` normally. + it('does not read the roster on a dry-run sweep', async () => { + const { root, mount, stateStore } = await orphanFixture() + const fleet = new RosterCountingFleet() + try { + const factory = factoryFor(root, mount, fleet, stateStore) + + const report = await factory.runOnce({ dryRun: true }) + + expect(fleet.rosterCalls).toBe(0) + expect(report.orphanRecoveryDegraded).toBe('dry-run') + // A deliberate skip, not a failure: the live-path failure counter must + // stay clear so a real degradation is still distinguishable from this. + expect(factory.status().counters.githubOrphanRecoveryContextFailures).toBeUndefined() + expect(factory.status().counters.githubOrphanRecoveryContextSkippedDryRun).toBe(1) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + + // MUST FIRE, and the CONTROL for the arm above. Same fixture, same fleet + // class, same issue — only `dryRun` differs. Without it, the zero above + // could mean the fixture never reaches the orphan-recovery call site at + // all, and would prove nothing. Swapping the two arms fails both: this one + // would stop recovering, that one would start reading the roster. + it('CONTROL: still reads the roster and recovers the orphan on a live sweep', async () => { + const { root, mount, stateStore } = await orphanFixture() + const fleet = new RosterCountingFleet() + try { + const factory = factoryFor(root, mount, fleet, stateStore) + + const report = await factory.runOnce() + + expect(fleet.rosterCalls).toBeGreaterThan(0) + expect(report.orphanRecoveryDegraded).toBeUndefined() + expect(report.dispatched.map((result) => result.issue.key)).toEqual(['348']) + expect(factory.status().counters.githubOrphanedInProgressRecovered).toBe(1) + expect(factory.status().counters.githubOrphanRecoveryContextSkippedDryRun).toBeUndefined() + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + + // The regression itself, in its production shape: the container's + // start-disabled `run-once --dry-run` gate, whose fleet client refuses to + // mint an identity. Before the guard this logged a safety-context failure + // every sweep; now the sweep never asks. + it('completes a dry-run sweep against a client that refuses to mint an identity', async () => { + const { root, mount, stateStore } = await orphanFixture() + const fleet = new ReadOnlyRosterFleet() + try { + const factory = factoryFor(root, mount, fleet, stateStore) + + const report = await factory.runOnce({ dryRun: true }) + + expect(fleet.rosterCalls).toBe(0) + expect(report.orphanRecoveryDegraded).toBe('dry-run') + expect(factory.status().counters.githubOrphanRecoveryContextFailures).toBeUndefined() + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + }) + // Factory admits an issue to dispatch on EITHER the title prefix or the // scope label, but orphan recovery demanded the label alone. An issue // admitted by title could therefore be dispatched and then never un-stuck. diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index dfea6aaa..11e9b7f1 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -2896,7 +2896,13 @@ export class FactoryLoop implements Factory { } const paths = await this.#readyIssuePaths() const orphanRecovery = issueSource === 'github' - ? await this.#githubOrphanRecoveryContext() + ? await this.#githubOrphanRecoveryContext(dryRun) + : undefined + // Preserved-not-reconciled is a fact the caller needs either way, but the + // two causes are not the same event: a dry run skips the context by + // design, a live sweep that has none failed to build one. + const orphanRecoveryDegraded = issueSource === 'github' && orphanRecovery === undefined + ? (dryRun ? 'dry-run' as const : 'context-unavailable' as const) : undefined const pulled: IssueRef[] = [] const triaged: TriageDecision[] = [] @@ -3158,7 +3164,15 @@ export class FactoryLoop implements Factory { } } - report = { pulled, triaged, dispatched, skipped, dryRun, slackDegraded: this.#slackDegraded } + report = { + pulled, + triaged, + dispatched, + skipped, + dryRun, + slackDegraded: this.#slackDegraded, + ...(orphanRecoveryDegraded ? { orphanRecoveryDegraded } : {}), + } return report } catch (error) { this.#logger.warn?.('[factory] run-once failed', { @@ -3177,6 +3191,7 @@ export class FactoryLoop implements Factory { dispatched: report.dispatched.length, skipped: report.skipped.length, slackDegraded: report.slackDegraded ?? false, + orphanRecoveryDegraded: report.orphanRecoveryDegraded, relayfileWaitWarnings: (this.#counters.relayfileOperationWaitWarnings ?? 0) - relayfileWaitWarningsAtStart, relayfileSlowOperations: (this.#counters.relayfileSlowOperations ?? 0) - relayfileSlowOperationsAtStart, relayfileOperationFailures: (this.#counters.relayfileOperationFailures ?? 0) - relayfileOperationFailuresAtStart, @@ -3550,7 +3565,34 @@ export class FactoryLoop implements Factory { } } - async #githubOrphanRecoveryContext(): Promise { + /** + * The safety context orphan recovery needs before it may release a leaked + * `factory:in-progress` claim: who is online, which issues are actively + * owned, and which durable lifecycle rows look abandoned. + * + * A dry run never builds it. `#reconcileOrphanedGithubInProgress` refuses to + * release a claim under `dryRun` before it so much as looks at the context, + * and `mayRecoverGithubOrphan` in `#performRunOnce` is itself `!dryRun` — + * so the context was already provably unused on that path. Gathering it + * anyway was not free: `fleet.roster()` mints this process's workspace + * identity on demand, which is exactly what a read-only client refuses + * (#343) and exactly what a dry run must not need. Deciding what a sweep + * WOULD do must not require an identity to do it with. + * + * Returning `undefined` is therefore the honest answer for a dry run, not a + * failure: the caller reports the sweep as `orphanRecoveryDegraded: + * 'dry-run'` and preserves in-progress issues, which is what a dry run does + * regardless. The failure counter and the warn below stay for the live path, + * where an absent context IS a degradation. + */ + async #githubOrphanRecoveryContext(dryRun: boolean): Promise { + if (dryRun) { + this.#increment('githubOrphanRecoveryContextSkippedDryRun') + this.#logger.info?.('[factory] dry run skipped the orphan-recovery safety context; in-progress issues are preserved', { + reason: 'a dry run never releases an in-progress claim, so it needs no workspace identity to build one', + }) + return undefined + } try { const [registry, roster, lifecycles, waitingClarifications] = await Promise.all([ readFactoryInFlightRegistry(this.#config.loop.registryPath), diff --git a/src/types.ts b/src/types.ts index 8ff24972..68c43fae 100644 --- a/src/types.ts +++ b/src/types.ts @@ -532,6 +532,17 @@ export interface IterationReport { skipped: Array<{ issue: IssueRef; reason: string }> dryRun: boolean slackDegraded?: boolean + /** + * Orphan recovery did not run for this sweep, so `factory:in-progress` + * claims were preserved rather than reconciled. + * + * `dry-run` is expected and benign: a dry run never releases a claim, so it + * deliberately skips building the safety context — which on a read-only + * fleet client would mean minting a workspace identity just to decide what + * the sweep WOULD do. `context-unavailable` is the real degradation: a live + * sweep tried to build the context and could not. + */ + orphanRecoveryDegraded?: 'dry-run' | 'context-unavailable' /** A cross-process owner was already enumerating this workspace. */ discoveryDeferred?: 'sweep-in-flight' error?: { message: string; stack?: string }