From e224bda65034b992d43ccc7763313ada115c897f Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 18:59:32 -0700 Subject: [PATCH 01/10] Keep the conflict resolver's trust-read timeout over a later stop (#174) When the rebase conflict resolver's issue-trust read timed out on its own (RemoteTimeout, timedOut()) or gh answered it (CommandFailed, ghAnswered()), and a cancel, shutdown or the rebase work deadline became pending while gh was stopping, GitRebaser#resolveConflict reported the stop instead. The preparation was then saved as failed with "Task cancelled.", "Server shutdown." or the deadline text. The resolver's catch now rethrows such an error before it checks for a pending stop, as the gateways, the merge coordinator and the server's trust read do since #48: had the stop come first, the read would have reported the stop. RebaseResourcesUnsettled still comes first, and any other resolver failure still reports a pending stop. Tests: - runner-rebase: a real rebase with a resolver whose trust read gave its timeout or gh's answer before a cancel, and before the rebase work deadline; each asserts that the stop was pending when the error was handled, with a control row (another failure reports the stop). Also a cancel that came first, and retained ownership ahead of a timeout. - pre-merge: a real conflicting rebase through PreMergeCoordinator, with the resolver re-reading authorization as production's does; a timeout or gh answer before a cancel or a shutdown is the saved preparation outcome, and a stop that came first is saved as the stop. - runner-rebase-conflict: the resolver passes an authorization timeout through a later cancel and releases its resources. The answer-first cases fail without the fix with the stop's text. Docs: merge-queue.md (the #174 follow-up note) and pre-merge-rebase.md. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/merge-queue.md | 2 +- docs/implementation/pre-merge-rebase.md | 4 +- runner/rebase.ts | 6 ++ test/pre-merge.test.ts | 112 ++++++++++++++++++++++-- test/runner-rebase-conflict.test.ts | 25 ++++++ test/runner-rebase.test.ts | 96 ++++++++++++++++++++ 6 files changed, 236 insertions(+), 9 deletions(-) diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index 4dfddd99..9b35f2cf 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -33,7 +33,7 @@ A GitHub read that stops reports the first reason it stopped for, and the merge - **Gateway reads.** A GitHub read that runs out of its own time throws `RemoteTimeout` (`github/timeout.ts`): the merge-state inspection, the queue watermark, the queue inspection and the issue read. When the caller aborts later, while gh is still stopping, the read still reports its timeout. A failure gh reported before either abort (its own exit, `CommandFailed`) is reported as itself, not replaced by the abort, except in the rules read, which treats any failure as unknown rules (traced, not tested). When one inspection stage's gh exit stops the other stages, the inspection waits for every stage to settle and then reports that exit, even when its own deadline fired meanwhile (tested). The already-fixed check's result is handled from the moment it starts, so a check that a failed stage stopped and that settles first is not an unhandled rejection, which Node would raise process-wide (found by the #48 tests; it predated #48; tested); a successful answer that arrives after the abort is still discarded, and so is an error the gateway raised while checking a successful answer (for example a different issue returned; traced, not tested). An inspection stage's own validation error that comes before the abort, while the other stages are still settling, is also answered with the abort: resendable, never a wrong saved answer (traced, not tested). The issue gateway's rule also covers execution's issue text and the issue board's refresh; the board's prioritizer still answers a refresh stopped by shutdown with the shutdown reason (traced, not tested). - **Issue-trust reads.** The server wraps a failed trust read as an upstream failure and keeps the timeout as its cause. On the merge path, a trust read that timed out, or that gh answered, before the caller's abort still reports that; the runner actions keep a trust read's own timeout too, even when shutdown started while gh stopped (tested), and answer any other failed trust read as shutting down once shutdown has started (the server is stopping, the runner is closing, or the request was aborted, which only shutdown does). Either way nothing is admitted. - The trust read that gates the runner actions (start, resume, approve-continuation, publish, prepare-merge) is not saved as the action's outcome when it times out (tested for start; the other actions share the same branch): the action answers 503, and the same action ID reads trust again; an outcome already saved under that ID is still the answer, also when shutdown started while gh stopped (tested). - - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver's trust read (`runner/rebase-conflict.ts`) is not covered: `Rebaser` (`runner/rebase.ts`) reports a cancel, shutdown or expired rebase deadline that is pending when the resolver's error is handled in its place. This is unchanged by #48 and left as a follow-up (#174). + - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver's trust read (`runner/rebase-conflict.ts`) keeps the same reason (#174): when it timed out, or gh answered, before a cancel, shutdown or the rebase work deadline, `GitRebaser` (`runner/rebase.ts`) reports that timeout or answer, not the stop that was pending when the resolver's error was handled, and the preparation is saved as failed with it (tested: the resolver passes a timeout through a later cancel in `test/runner-rebase-conflict.test.ts`; a cancel and the rebase work deadline in `test/runner-rebase.test.ts`; a cancel and a shutdown, with the saved outcome, through a real rebase in `test/pre-merge.test.ts`). A cancel or shutdown that came first is still reported as the stop (tested; a deadline that came first is traced, not tested), any other resolver failure still reports a pending stop (tested), and a resolver that kept its resources for startup recovery (`RebaseResourcesUnsettled`) still reports that before any of these (tested). - A publish whose trust read timed out, or that gh answered, before shutdown or cancellation is recorded as failed with that reason, not as stopped (tested for shutdown, `test/runner-publishing.test.ts`); cancellation takes the same path, both leave the publish owed, and a cancelled task's PRs are still closed (traced, not tested). - Execution's issue read reports its own timeout when a cancel arrives after it; the runner decides the attempt's outcome from its recorded cancel, not from that error (traced, not tested). - Out of this change: the issue board's trust toggle still saves a timed-out author read as that action's failure, and planning's issue reads (`web/server.ts`, `web/planning.ts`) still answer a timeout as an upstream failure without that cause. diff --git a/docs/implementation/pre-merge-rebase.md b/docs/implementation/pre-merge-rebase.md index bf2fd10e..b40fd989 100644 --- a/docs/implementation/pre-merge-rebase.md +++ b/docs/implementation/pre-merge-rebase.md @@ -24,7 +24,7 @@ Before any clone or Docker operation, the production adapter records a unique ch The Store records each resolved foreign source SHA before the result ref write. Preparation refuses duplicates, sources outside the captured history, owned sources, and sources that did not actually rewrite. On publication, a foreign mapped commit retains `owner: null`, `origin: foreign`, and `sourceSha`, plus `conflictResolved: true`. Linking therefore keeps its lines in Unplanned while the review UI adds the text label “conflict resolved by agent.” An owned mapped commit instead retains its Px owner and source SHA without foreign-resolution provenance; its preserved message keeps the original plan trailers, and the rewritten mapping lets review recompute whether Px's approval became stale. Every replay preserves the original commit author. -All other conflicts are a review outcome (`RebaseConflict`). Cancellation and failures settle the Git process group before cleanup. Cleanup removes only the validated UUID workspace. A partially created but unregistered worktree is removed only after Git's own worktree list proves it is not registered. Result refs are created atomically from the absent state; a reused attempt ID cannot overwrite or delete a previously retained result, while an ambiguous create is reconciled against the exact attempted value. +All other conflicts are a review outcome (`RebaseConflict`). Cancellation and failures settle the Git process group before cleanup. When the resolver fails while a cancel, shutdown or the rebase work deadline is pending, the rebaser reports the stop, with two exceptions. A resolver that kept its resources for startup recovery reports `RebaseResourcesUnsettled`. A remote read that timed out on its own, or that gh answered, before the stop (the issue-trust read before launch) reports that timeout or answer (#174). Cleanup removes only the validated UUID workspace. A partially created but unregistered worktree is removed only after Git's own worktree list proves it is not registered. Result refs are created atomically from the absent state; a reused attempt ID cannot overwrite or delete a previously retained result, while an ambiguous create is reconciled against the exact attempted value. ## Restart recovery @@ -54,4 +54,4 @@ Every review reload in a preparation awaits a history worker (#170), so each one ## Regression evidence -`test/runner-rebase.test.ts` covers clean one-to-one replay, the already-current no-op, process-group ownership, the whole-operation deadline, owned and foreign conflict resolution, clean sibling files, gitlink-pointer auditing and manual gitlink-conflict routing, embedded-repository refusal, literal replacement-character paths, invalid ledger classification, outside-file, resolver-commit and rebase-control refusal, cancellation before and during Git, exact cleanup, retained refs, and a read-only source checkout. `test/runner-rebase-conflict.test.ts` covers owned-item prompt binding, plan-revision races before launch and copy-back, claim-before-prepare ordering, distinct allocation identities, bounded no-follow host snapshots, nested modify/delete copy-in, copy-back audit, child settlement deadlines, cancellation settlement, cleanup ordering, and retained ownership for unreleased resources. `test/runner-lifecycle-store.test.ts` covers admission, durable process and child ownership, compare-and-swap races, resolved-source validation, and owned/foreign provenance. `test/runner-recovery.test.ts` covers marker validation, live-group fail-closed recovery, exact child-storage recovery, partial-match refusal, and restart ordering. `test/agent-process-group.test.ts` covers delayed-signal suppression after exit and retained ownership for an orphaned group. The real-container suite covers dirty-conflict admission and bounded exact-path export. `test/history.test.ts` and `test/browser/review.spec.ts` cover Unplanned attribution and the visible conflict-resolution label. `test/runner-production.test.ts` covers exact-plan recovery routing and adapter construction. +`test/runner-rebase.test.ts` covers clean one-to-one replay, the already-current no-op, process-group ownership, the whole-operation deadline, owned and foreign conflict resolution, clean sibling files, gitlink-pointer auditing and manual gitlink-conflict routing, embedded-repository refusal, literal replacement-character paths, invalid ledger classification, outside-file, resolver-commit and rebase-control refusal, cancellation before and during Git, the reason a failed resolver reports when a stop is pending (#174), exact cleanup, retained refs, and a read-only source checkout. `test/runner-rebase-conflict.test.ts` covers owned-item prompt binding, plan-revision races before launch and copy-back, claim-before-prepare ordering, distinct allocation identities, bounded no-follow host snapshots, nested modify/delete copy-in, copy-back audit, child settlement deadlines, an authorization read's own timeout kept through a later cancel (#174), cancellation settlement, cleanup ordering, and retained ownership for unreleased resources. `test/runner-lifecycle-store.test.ts` covers admission, durable process and child ownership, compare-and-swap races, resolved-source validation, and owned/foreign provenance. `test/runner-recovery.test.ts` covers marker validation, live-group fail-closed recovery, exact child-storage recovery, partial-match refusal, and restart ordering. `test/agent-process-group.test.ts` covers delayed-signal suppression after exit and retained ownership for an orphaned group. The real-container suite covers dirty-conflict admission and bounded exact-path export. `test/history.test.ts` and `test/browser/review.spec.ts` cover Unplanned attribution and the visible conflict-resolution label. `test/runner-production.test.ts` covers exact-plan recovery routing and adapter construction. diff --git a/runner/rebase.ts b/runner/rebase.ts index a567148f..9136bdc9 100644 --- a/runner/rebase.ts +++ b/runner/rebase.ts @@ -4,6 +4,8 @@ import { fileURLToPath } from 'node:url'; import { runInProcessGroup, type ProcessGroup } from '../agents/process-group.ts'; import type { DockerOutcome } from '../agents/docker.ts'; import { GIT_OPTIONS, gitEnvironment } from '../git/clone.ts'; +import { ghAnswered } from '../github/run-with-input.ts'; +import { timedOut } from '../github/timeout.ts'; import { isUuidV4 } from './lifecycle.ts'; import { ownerOnlyDirectory, type RunnerRepository } from './runner-repository.ts'; import { MAX_INDEX_BYTES } from './verify-checkout.ts'; @@ -440,6 +442,10 @@ export class GitRebaser { catch (error) { // Retained ownership outranks cancellation/deadline reporting: live cleanup must preserve its parent workspace. if (error instanceof RebaseResourcesUnsettled) throw error; + // A remote read that timed out on its own (as the issue-trust read can), or that gh answered, keeps that reason + // though a cancel, shutdown or this deadline became pending while it stopped (#174). Had the stop come first, + // the read would have reported the stop instead: the gateways and the server's trust read keep the first reason. + if (timedOut(error) || ghAnswered(error)) throw error; abortExpired(); controller.signal.throwIfAborted(); throw error; diff --git a/test/pre-merge.test.ts b/test/pre-merge.test.ts index 52708ee0..a58e1035 100644 --- a/test/pre-merge.test.ts +++ b/test/pre-merge.test.ts @@ -17,7 +17,8 @@ import { commandCheckDeps } from '../runner/checks.ts'; import { RunnerCoordinator } from '../runner/coordinator.ts'; import type { InvocationHandle, InvocationResult } from '../agents/contract.ts'; import type { TaskWorkspace, WorkspaceRef } from '../runner/execution.ts'; -import { MIN_REBASE_CLEANUP_TIMEOUT_MS, MIN_REBASE_TIMEOUT_MS, RebaseResourcesUnsettled } from '../runner/rebase.ts'; +import { GitRebaser, MIN_REBASE_CLEANUP_TIMEOUT_MS, MIN_REBASE_TIMEOUT_MS, RebaseResourcesUnsettled } from '../runner/rebase.ts'; +import { ensureCommit, openRunnerRepository } from '../runner/runner-repository.ts'; import { BranchPushRefused } from '../runner/branch-push.ts'; import { UpstreamFailure } from '../runner/lifecycle.ts'; import { RemoteTimeout } from '../github/timeout.ts'; @@ -98,7 +99,12 @@ async function rebaseFixture(rebasedText: string, commandExit?: number, options: runnerCommitted?: boolean; operationTimeoutMs?: number; /** The second argument lets an authorization stop the preparation while its read is settling. */ - authorize?: (signal: AbortSignal, fixture: { coordinator: PreMergeCoordinator }) => Promise; + authorize?: (signal: AbortSignal, fixture: { coordinator: PreMergeCoordinator; service: ReviewService }) => Promise; + /** + * The base moves a.txt too, so the rebase conflicts, and a real GitRebaser replays it with production's hooks. Its + * conflict resolver re-reads authorization as production's does (authorize, refresh, then validate) and resolves a.txt. + */ + conflictRebase?: boolean; reserves?: { processMs?: number; commandMs?: number; pushVisibleMs?: number }; /** The PR API keeps reporting the pre-push head for this many inspections after a push. */ pushVisibleAfter?: number; @@ -144,7 +150,9 @@ async function rebaseFixture(rebasedText: string, commandExit?: number, options: writeFileSync(join(repository, 'a.txt'), 'feature\n'); fixtureGit(repository, 'commit', '-am', 'feature', '-q'); const head = fixtureGit(repository, 'rev-parse', 'HEAD'); fixtureGit(repository, 'switch', '-q', '-C', 'main', base); - writeFileSync(join(repository, 'b.txt'), 'new base\n'); fixtureGit(repository, 'add', '.'); fixtureGit(repository, 'commit', '-qm', 'move base'); + writeFileSync(join(repository, 'b.txt'), 'new base\n'); + if (options.conflictRebase) writeFileSync(join(repository, 'a.txt'), 'main\n'); + fixtureGit(repository, 'add', '.'); fixtureGit(repository, 'commit', '-qm', 'move base'); const onto = fixtureGit(repository, 'rev-parse', 'HEAD'); fixtureGit(repository, 'switch', '-qc', 'rebased'); writeFileSync(join(repository, 'a.txt'), rebasedText); fixtureGit(repository, 'commit', '-am', 'rebased feature', '-q'); @@ -189,6 +197,33 @@ async function rebaseFixture(rebasedText: string, commandExit?: number, options: rebaseAborts++; abortBudgets.push(timeoutMs); if (options.rebaseCleanupFailure) throw options.rebaseCleanupFailure; } } as never; + let gitRebaser: GitRebaser | undefined, resolverReads = 0; + if (options.conflictRebase) { + const runnerRoot = join(root, 'runner'), runnerOwner = 'c'.repeat(32); + const runnerRepository = await openRunnerRepository({ runnerRoot, runnerOwner, repositoryId: 'repo', source: repository }); + await ensureCommit(runnerRepository, head); await ensureCommit(runnerRepository, onto); + const planKey = () => service.store.getTask(identity).planKey; + // The hooks production's assembly (runner/production.ts) gives the rebaser, without its capability wrapper. + const real = new GitRebaser({ repository: runnerRepository, runnerRoot, runnerOwner, + committer: { name: 'Codeboost', email: 'codeboost@example.invalid' }, + onProcessStarting: attemptId => service.store.setRebaseProcessGroup(planKey(), attemptId, null, 'spawning'), + onProcessGroup: (attemptId, group) => service.store.setRebaseProcessGroup(planKey(), attemptId, 'spawning', group), + onProcessGroupSettled: (attemptId, group) => service.store.setRebaseProcessGroup(planKey(), attemptId, group, null), + onProcessUnsettled: (attemptId, group) => service.store.setRebaseProcessGroup(planKey(), attemptId, group, 'unsettled'), + onResultPrepared: (attemptId, result, history, conflicts) => service.store.prepareRebaseResult(planKey(), attemptId, result, history, conflicts), + onResultState: (attemptId, state) => state === 'ready' ? service.store.completeRebaseResult(planKey(), attemptId) + : service.store.setRebaseResultState(planKey(), attemptId, state), + // As production's resolver: a fresh authorization read immediately before the conflict agent would launch. + resolveConflict: async input => { + if (!authorize) throw new Error('This conflict rebase needs an authorization.'); + resolverReads++; + const authorization = await authorize(input.signal!); + await authorization.refresh(); authorization.validate(); + writeFileSync(join(input.repository, 'a.txt'), 'resolved\n'); + } }); + gitRebaser = { run: (input: Parameters[0]) => { rebaseRuns++; rebaseBudgets.push(input.timeoutMs); return real.run(input); }, + abort: (...args: Parameters) => { rebaseAborts++; abortBudgets.push(args[3]); return real.abort(...args); } } as never; + } const checkedHeads: string[] = []; const workspace: TaskWorkspace = { async materialize(attempt, checkedHead) { @@ -248,7 +283,7 @@ async function rebaseFixture(rebasedText: string, commandExit?: number, options: const pushes: { branch: string; from: string; to: string }[] = []; let lagging = 0, lagged = remotePair, branchReads = 0, fetchesAfterPush = 0; let authorizationChecks = 0; - const authorize = options.authorize ? (signal: AbortSignal) => options.authorize!(signal, { coordinator }) : (options.duringAuthorize ? async () => ({ + const authorize = options.authorize ? (signal: AbortSignal) => options.authorize!(signal, { coordinator, service }) : (options.duringAuthorize ? async () => ({ refresh: async () => undefined, validate: () => { remotePair = options.duringAuthorize!(service, ++authorizationChecks) ?? remotePair; }, }) : undefined); @@ -264,7 +299,7 @@ async function rebaseFixture(rebasedText: string, commandExit?: number, options: } coordinator = new PreMergeCoordinator(service, runner, - rebaser, { inspect: async () => { + gitRebaser ?? rebaser, { inspect: async () => { if (lagging > 0) { lagging--; options.duringVisibilityWait?.(service); return lagged; } remoteReads++; // Read 2 is the post-push visibility check; hooks keep counting the later inspections as before the push existed. @@ -326,7 +361,9 @@ async function rebaseFixture(rebasedText: string, commandExit?: number, options: remotePair = { ...remotePair, ...pair }; if (shown) { lagged = { ...remotePair, ...shown }; lagging = reads; } else lagging = 0; }, - rebaseRuns: () => rebaseRuns, rebaseAborts: () => rebaseAborts, rebaseBudgets, abortBudgets }; + rebaseRuns: () => rebaseRuns, rebaseAborts: () => rebaseAborts, rebaseBudgets, abortBudgets, + /** Authorization reads the conflict resolver started (`conflictRebase` only). */ + resolverReads: () => resolverReads }; } it('preserves an approval when a moved base leaves its attributed fingerprint unchanged', async () => { @@ -414,6 +451,69 @@ it.each([ await fixture.runner.close(); }); +// The conflict resolver's own issue-trust read (#174): the read is the second authorization; the first admitted the rebase. +// These run a real Git rebase, with about twenty Git and verifier processes, so they get more than the file's 15 s. +const REAL_REBASE_TEST_TIMEOUT_MS = 60_000; +const resolverTrustAnswers = [ + ['timed out', () => new RemoteTimeout('Issue retrieval timed out.')], + ['was answered by gh', () => new CommandFailed('gh failed (exit 1): HTTP 404: Not Found', 'HTTP 404: Not Found', '')], +] as const; +const resolverStops = { + cancel: { reason: 'Task cancelled.', stop: (service: ReviewService, coordinator: PreMergeCoordinator) => + expect(coordinator.cancelTask(service.store.getTask(service.config.identity).stateVersion, randomUUID())).toBe('stopping') }, + shutdown: { reason: 'Server shutdown.', stop: (_service: ReviewService, coordinator: PreMergeCoordinator) => { void coordinator.close(); } }, +} as const; + +it.each(resolverTrustAnswers.flatMap(([read, answer]) => (['cancel', 'shutdown'] as const).map(stop => [read, stop, answer] as const)))( + "saves a preparation whose conflict resolver's issue-trust read %s before a %s with that answer, not the stop (#174)", async (_read, stopKind, answer) => { + const actionId = randomUUID(), cause = answer(), expected = `Issue trust could not be verified: ${cause.message}`; + const { reason, stop } = resolverStops[stopKind]; + let reads = 0, pending: unknown = null; + const fixture = await rebaseFixture('feature\n', undefined, { actionId, conflictRebase: true, authorize: async (signal, { coordinator, service }) => { + if (++reads === 1) return { refresh: async () => undefined, validate: () => undefined }; + // The read answered (or ran out of time) first; the stop comes while gh is stopping, so it is pending when the + // resolver's error is handled. This is the error the server's readIssueAccess then builds. + stop(service, coordinator); + pending = signal.aborted ? signal.reason : null; + throw new UpstreamFailure(expected, { cause }); + } }); + // The second read was the resolver's, made while the rebase waited on the conflict. + expect(reads).toBe(2); expect(fixture.resolverReads()).toBe(1); + expect((pending as Error | null)?.message).toBe(reason); + expect(fixture.result).toMatchObject({ state: 'failed', reason: expected }); + const identity = fixture.service.config.identity; + expect(fixture.service.store.savedAction(identity, { actionId, kind: 'prepare-merge', request: fixture.actionRequest })?.response) + .toMatchObject({ outcome: 'failed', reason: expected }); + // The live cleanup removed the attempt and its durable marker. + expect(fixture.rebaseRuns()).toBe(1); + expect(fixture.rebaseAborts()).toBe(1); + expect(fixture.service.store.getTask(identity).rebaseInProgress).toBeNull(); + if (stopKind === 'cancel') expect(fixture.service.store.getTask(identity).status).toBe('cancelled'); + await fixture.coordinator.close(); await fixture.runner.close(); + }, REAL_REBASE_TEST_TIMEOUT_MS); + +it.each(resolverTrustAnswers.flatMap(([read, answer]) => (['cancel', 'shutdown'] as const).map(stop => [stop, read, answer] as const)))( + "saves a preparation as stopped by a %s that came before the conflict resolver's issue-trust read %s (#174)", async (stopKind, _read, answer) => { + const actionId = randomUUID(); + const { reason, stop } = resolverStops[stopKind]; + let reads = 0, abortedBeforeRead = false; + const fixture = await rebaseFixture('feature\n', undefined, { actionId, conflictRebase: true, authorize: async (signal, { coordinator, service }) => { + if (++reads === 1) return { refresh: async () => undefined, validate: () => undefined }; + stop(service, coordinator); + abortedBeforeRead = signal.aborted; + // The stop came first: the gateway and readIssueAccess report it, not the answer the read would have given. + if (signal.aborted) throw signal.reason; + throw new UpstreamFailure(`Issue trust could not be verified: ${answer().message}`, { cause: answer() }); + } }); + expect(reads).toBe(2); expect(fixture.resolverReads()).toBe(1); + expect(abortedBeforeRead).toBe(true); + expect(fixture.result).toMatchObject({ state: 'failed', reason }); + expect(fixture.service.store.savedAction(fixture.service.config.identity, { actionId, kind: 'prepare-merge', request: fixture.actionRequest })?.response) + .toMatchObject({ outcome: 'failed', reason }); + expect(fixture.service.store.getTask(fixture.service.config.identity).rebaseInProgress).toBeNull(); + await fixture.coordinator.close(); await fixture.runner.close(); + }, REAL_REBASE_TEST_TIMEOUT_MS); + it('revalidates authorization before starting the delayed local rebase', async () => { const calls: string[] = []; const fixture = await rebaseFixture('feature\n', undefined, { authorize: async () => { diff --git a/test/runner-rebase-conflict.test.ts b/test/runner-rebase-conflict.test.ts index 609bc12f..7ea6b153 100644 --- a/test/runner-rebase-conflict.test.ts +++ b/test/runner-rebase-conflict.test.ts @@ -16,6 +16,8 @@ import { CONFLICT_PROCESS_SETTLEMENT_RESERVE_MS, copyConflictPaths, createForeig import { RebaseResourcesUnsettled, type RebaseConflictInput } from '../runner/rebase.ts'; import type { RunnerRepository } from '../runner/runner-repository.ts'; import { Store } from '../runner/store.ts'; +import { UpstreamFailure } from '../runner/lifecycle.ts'; +import { RemoteTimeout } from '../github/timeout.ts'; const identity = { repositoryId: 'repo', taskId: 'task', planId: 'plan' }; const context: PlanContext = { identity, issue: 22, baseEntries: [{ path: 'conflict.txt', kind: 'file' }], pathKey: p => p, allowedCommands: [] }; @@ -192,6 +194,29 @@ describe('production rebase conflict resolver', () => { expect(x.events).not.toContain('start'); }); + it('reports an authorization read that timed out before a cancel, not the cancel, and releases resources (#174)', async () => { + const f = fixture(), x = deps(f), stop = new AbortController(); + const cause = new RemoteTimeout('Issue retrieval timed out.'); + let pending = false; + const resolve = createForeignConflictResolver({ store: f.store, identity, planKey: f.planKey, + repository: { path: join(f.root, 'bare.git') } as RunnerRepository, runnerOwner: 'a'.repeat(32), + image: () => 'sha256:' + 'b'.repeat(64), token: 'secret', + // The read timed out first; the cancel comes while gh is stopping, as the server's readIssueAccess then reports it. + authorize: async signal => { + stop.abort(new Error('Task cancelled.')); + pending = signal.aborted; + throw new UpstreamFailure(`Issue trust could not be verified: ${cause.message}`, { cause }); + }, + limits: { workBytes: 1, workInodes: 1, metadataBytes: 1, metadataInodes: 1 }, deps: x.d as never }); + const error = await resolve(input(f, stop.signal)).then(() => null, (failure: unknown) => failure as Error); + expect(pending).toBe(true); + expect(error).toBeInstanceOf(UpstreamFailure); + expect(error?.cause).toBe(cause); + expect(x.events).not.toContain('start'); + expect(x.events).toContain('remove'); + expect(f.store.getTask(identity).rebaseInProgress).toMatchObject({ conflict: null, processGroup: null }); + }); + it('refuses launch and releases resources when the authorization hook throws synchronously', async () => { const f = fixture(), x = deps(f); const resolve = createForeignConflictResolver({ store: f.store, identity, planKey: f.planKey, diff --git a/test/runner-rebase.test.ts b/test/runner-rebase.test.ts index cc47ca69..09bd995f 100644 --- a/test/runner-rebase.test.ts +++ b/test/runner-rebase.test.ts @@ -12,6 +12,9 @@ import { splitBoundedIndexRecords, verifyCheckout } from '../runner/verify-check import { readBoundedRebaseStateNames } from '../runner/hash-rebase-state.ts'; import { listGitPaths } from '../runner/list-git-paths.ts'; import { outsideConflictDigest } from '../runner/hash-outside-conflict.ts'; +import { UpstreamFailure } from '../runner/lifecycle.ts'; +import { RemoteTimeout } from '../github/timeout.ts'; +import { CommandFailed } from '../github/run-with-input.ts'; const OWNER = '0123456789abcdef0123456789abcdef'; const roots: string[] = []; @@ -666,6 +669,99 @@ describe('trusted pre-merge rebase', () => { expect(existsSync(join(s.runnerRoot, OWNER, 'rebases', attemptId))).toBe(true); }); + describe('a conflict resolver whose issue-trust read failed, and a stop (#174)', () => { + /** + * The resolver's issue-trust read as production builds it: the issue gateway's bounded read keeps whichever came + * first, its own timeout, gh's answer or the caller's abort, and the server's readIssueAccess reports the abort as + * itself, or wraps any other failure as an UpstreamFailure caused by it. Here `answer` is ready before the read is + * called, so it came first unless the caller's signal was already aborted. + */ + const trustRead = (signal: AbortSignal, answer: Error): Promise => signal.aborted ? Promise.reject(signal.reason) + : Promise.reject(new UpstreamFailure(`Issue trust could not be verified: ${answer.message}`, { cause: answer })); + // The last row is a failure that is neither a timeout nor gh's answer: the same order reports the stop, as before. + const answers = [ + ['its own timeout', () => new RemoteTimeout('Issue retrieval timed out.'), true], + ["gh's answer", () => new CommandFailed('gh failed (exit 1): HTTP 404: Not Found', 'HTTP 404: Not Found', ''), true], + ['a failure of another kind', () => new Error('The trust read failed.'), false], + ] as const; + const run = (s: Awaited>, runner: GitRebaser, attemptId: string, signal?: AbortSignal) => + runner.run({ attemptId, oldBase: s.base, oldHead: s.foreign, oldHistory: s.history, onto: s.onto, signal, + ledger: [{ sha: s.foreign, owner: null, origin: 'foreign' }] }).then(() => null, (error: unknown) => error as Error); + + it.each(answers)('reports %s that came before a cancel, not the cancel', async (_case, answer, kept) => { + const s = await setup('foreign'), attemptId = randomUUID(), stop = new AbortController(); + const cause = answer(), expected = `Issue trust could not be verified: ${cause.message}`; + let pending: unknown; + const runner = createRebaser(s, { resolveForeignConflict: async input => { + const read = trustRead(input.signal!, cause); + // The cancel comes while gh is still stopping, so it is pending when the resolver's error is handled. + stop.abort(new Error('Task cancelled.')); + pending = input.signal!.aborted ? input.signal!.reason : null; + await read; + } }); + const error = await run(s, runner, attemptId, stop.signal); + expect((pending as Error | null)?.message).toBe('Task cancelled.'); + expect(error?.message).toBe(kept ? expected : 'Task cancelled.'); + if (kept) expect(error?.cause).toBe(cause); + // The failed attempt was cleaned up as usual. + expect(existsSync(join(s.runnerRoot, OWNER, 'rebases', attemptId))).toBe(false); + }); + + it.each(answers)('reports %s that came before the rebase work deadline, not the deadline', async (_case, answer, kept) => { + const s = await setup('foreign'), attemptId = randomUUID(), realNow = performance.now.bind(performance); + const cause = answer(), expected = `Issue trust could not be verified: ${cause.message}`; + let skew = 0, overdueMs = 0, aborted: boolean | null = null; + vi.spyOn(performance, 'now').mockImplementation(() => realNow() + skew); + const runner = createRebaser(s, { resolveForeignConflict: async input => { + const read = trustRead(input.signal!, cause); + // The work deadline (whose wall-clock form is input.deadline) passes 1 s before the error is handled, and before + // its timer can run. Cleanup keeps its own 30 s reserve after the work deadline. + skew = input.deadline - Date.now() + 1_000; + overdueMs = Date.now() + (performance.now() - realNow()) - input.deadline; + aborted = input.signal!.aborted; + await read; + } }); + const error = await run(s, runner, attemptId); + expect(overdueMs).toBeGreaterThan(0); + expect(aborted).toBe(false); + expect(error?.message).toBe(kept ? expected : 'The rebase deadline expired during conflict resolution.'); + if (kept) expect(error?.cause).toBe(cause); + expect(existsSync(join(s.runnerRoot, OWNER, 'rebases', attemptId))).toBe(false); + }); + + it.each(answers.slice(0, 2))('reports a cancel that came before the trust read could give %s', async (_case, answer) => { + const s = await setup('foreign'), attemptId = randomUUID(), stop = new AbortController(); + let abortedBeforeRead: boolean | null = null; + const runner = createRebaser(s, { resolveForeignConflict: async input => { + stop.abort(new Error('Task cancelled.')); + abortedBeforeRead = input.signal!.aborted; + await trustRead(input.signal!, answer()); + } }); + const error = await run(s, runner, attemptId, stop.signal); + expect(abortedBeforeRead).toBe(true); + expect(error?.message).toBe('Task cancelled.'); + }); + + it('keeps retained resource ownership ahead of a trust-read timeout and a later cancel', async () => { + const s = await setup('foreign'), attemptId = randomUUID(), stop = new AbortController(); + let pending = false; + const runner = createRebaser(s, { resolveForeignConflict: async input => { + const read = trustRead(input.signal!, new RemoteTimeout('Issue retrieval timed out.')); + stop.abort(new Error('Task cancelled.')); + pending = input.signal!.aborted; + // The resolver's cleanup did not settle, so it retains its resources, as createConflictResolver reports it. + await read.catch((error: Error) => { + throw new RebaseResourcesUnsettled(`Conflict resolution retained resources for startup recovery: ${error.message}`, { cause: error }); + }); + } }); + const error = await run(s, runner, attemptId, stop.signal); + expect(pending).toBe(true); + expect(error).toBeInstanceOf(RebaseResourcesUnsettled); + expect(error?.message).toMatch(/retained resources for startup recovery: Issue trust could not be verified: Issue retrieval timed out/); + expect(existsSync(join(s.runnerRoot, OWNER, 'rebases', attemptId))).toBe(true); + }); + }); + it('routes an owned conflict through the sandbox while preserving its owned provenance', async () => { const s = await setup(true), attemptId = randomUUID(), seen: unknown[] = []; const resolveConflict = vi.fn(async input => { From e42d5021fe34a5c95d14e6abcf75617d1d70e599 Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 19:26:05 -0700 Subject: [PATCH 02/10] Address review round 1 of #197: deadline scope, gh fake shape, wording - merge-queue.md: split the resolver trust-read bullet into a short list (stop after the read, stop before it, the work deadline, other failures, retained resources). Scope the deadline claim to GitRebaser with a resolver that settles. With the production resolver a pending work deadline leaves no time for cleanup, so it usually ends as RebaseResourcesUnsettled, whose message carries the timeout or answer. pre-merge-rebase.md says the same. - runner-rebase-conflict test: the work deadline passes during a trust read that timed out; the resolver starts no cleanup, keeps its child claim, and throws RebaseResourcesUnsettled with the timeout text. - gh fake: test/fixtures/gh-answers.ts gives gh 2.102.0's real shape for `gh api` on a missing issue (exit 1, "gh: Not Found (HTTP 404)" on stderr, GitHub's JSON body on stdout), used by the #174 tests and the #48 pre-merge test it was copied from. Only the class and message are read. - rebase.ts: the comment names its assumption that the issue-trust read is the production resolver's only GitHub read. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/merge-queue.md | 7 ++++++- docs/implementation/pre-merge-rebase.md | 4 ++-- runner/rebase.ts | 8 +++++--- test/fixtures/gh-answers.ts | 13 +++++++++++++ test/pre-merge.test.ts | 6 +++--- test/runner-rebase-conflict.test.ts | 24 ++++++++++++++++++++++++ test/runner-rebase.test.ts | 4 ++-- 7 files changed, 55 insertions(+), 11 deletions(-) create mode 100644 test/fixtures/gh-answers.ts diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index 9b35f2cf..3c701d61 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -33,7 +33,12 @@ A GitHub read that stops reports the first reason it stopped for, and the merge - **Gateway reads.** A GitHub read that runs out of its own time throws `RemoteTimeout` (`github/timeout.ts`): the merge-state inspection, the queue watermark, the queue inspection and the issue read. When the caller aborts later, while gh is still stopping, the read still reports its timeout. A failure gh reported before either abort (its own exit, `CommandFailed`) is reported as itself, not replaced by the abort, except in the rules read, which treats any failure as unknown rules (traced, not tested). When one inspection stage's gh exit stops the other stages, the inspection waits for every stage to settle and then reports that exit, even when its own deadline fired meanwhile (tested). The already-fixed check's result is handled from the moment it starts, so a check that a failed stage stopped and that settles first is not an unhandled rejection, which Node would raise process-wide (found by the #48 tests; it predated #48; tested); a successful answer that arrives after the abort is still discarded, and so is an error the gateway raised while checking a successful answer (for example a different issue returned; traced, not tested). An inspection stage's own validation error that comes before the abort, while the other stages are still settling, is also answered with the abort: resendable, never a wrong saved answer (traced, not tested). The issue gateway's rule also covers execution's issue text and the issue board's refresh; the board's prioritizer still answers a refresh stopped by shutdown with the shutdown reason (traced, not tested). - **Issue-trust reads.** The server wraps a failed trust read as an upstream failure and keeps the timeout as its cause. On the merge path, a trust read that timed out, or that gh answered, before the caller's abort still reports that; the runner actions keep a trust read's own timeout too, even when shutdown started while gh stopped (tested), and answer any other failed trust read as shutting down once shutdown has started (the server is stopping, the runner is closing, or the request was aborted, which only shutdown does). Either way nothing is admitted. - The trust read that gates the runner actions (start, resume, approve-continuation, publish, prepare-merge) is not saved as the action's outcome when it times out (tested for start; the other actions share the same branch): the action answers 503, and the same action ID reads trust again; an outcome already saved under that ID is still the answer, also when shutdown started while gh stopped (tested). - - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver's trust read (`runner/rebase-conflict.ts`) keeps the same reason (#174): when it timed out, or gh answered, before a cancel, shutdown or the rebase work deadline, `GitRebaser` (`runner/rebase.ts`) reports that timeout or answer, not the stop that was pending when the resolver's error was handled, and the preparation is saved as failed with it (tested: the resolver passes a timeout through a later cancel in `test/runner-rebase-conflict.test.ts`; a cancel and the rebase work deadline in `test/runner-rebase.test.ts`; a cancel and a shutdown, with the saved outcome, through a real rebase in `test/pre-merge.test.ts`). A cancel or shutdown that came first is still reported as the stop (tested; a deadline that came first is traced, not tested), any other resolver failure still reports a pending stop (tested), and a resolver that kept its resources for startup recovery (`RebaseResourcesUnsettled`) still reports that before any of these (tested). + - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver also reads issue trust (`runner/rebase-conflict.ts`), just before its agent would start. When that read fails, `GitRebaser` (`runner/rebase.ts`) reports the failure as follows (#174): + - **Cancel or shutdown after the read failed.** If the read timed out, or gh answered, before the cancel or shutdown, the rebase reports that timeout or answer, and the preparation is saved as failed with it. Tested: the resolver in `test/runner-rebase-conflict.test.ts`, the rebaser in `test/runner-rebase.test.ts`, and the saved outcome through a real rebase in `test/pre-merge.test.ts`. + - **Cancel or shutdown before the read.** The read reports the stop, and so does the rebase (tested). + - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not settle then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery, and the preparation is saved with that message, which includes the timeout or answer (tested in `test/runner-rebase-conflict.test.ts`). A deadline that came before the read is traced, not tested. + - **Other failures.** A read failure that is neither a timeout nor gh's answer still reports a stop that is pending when it is handled (tested). + - **Retained resources.** A resolver that keeps its resources for startup recovery (`RebaseResourcesUnsettled`) is reported as that before any of the cases above (tested). - A publish whose trust read timed out, or that gh answered, before shutdown or cancellation is recorded as failed with that reason, not as stopped (tested for shutdown, `test/runner-publishing.test.ts`); cancellation takes the same path, both leave the publish owed, and a cancelled task's PRs are still closed (traced, not tested). - Execution's issue read reports its own timeout when a cancel arrives after it; the runner decides the attempt's outcome from its recorded cancel, not from that error (traced, not tested). - Out of this change: the issue board's trust toggle still saves a timed-out author read as that action's failure, and planning's issue reads (`web/server.ts`, `web/planning.ts`) still answer a timeout as an upstream failure without that cause. diff --git a/docs/implementation/pre-merge-rebase.md b/docs/implementation/pre-merge-rebase.md index b40fd989..18662152 100644 --- a/docs/implementation/pre-merge-rebase.md +++ b/docs/implementation/pre-merge-rebase.md @@ -24,7 +24,7 @@ Before any clone or Docker operation, the production adapter records a unique ch The Store records each resolved foreign source SHA before the result ref write. Preparation refuses duplicates, sources outside the captured history, owned sources, and sources that did not actually rewrite. On publication, a foreign mapped commit retains `owner: null`, `origin: foreign`, and `sourceSha`, plus `conflictResolved: true`. Linking therefore keeps its lines in Unplanned while the review UI adds the text label “conflict resolved by agent.” An owned mapped commit instead retains its Px owner and source SHA without foreign-resolution provenance; its preserved message keeps the original plan trailers, and the rewritten mapping lets review recompute whether Px's approval became stale. Every replay preserves the original commit author. -All other conflicts are a review outcome (`RebaseConflict`). Cancellation and failures settle the Git process group before cleanup. When the resolver fails while a cancel, shutdown or the rebase work deadline is pending, the rebaser reports the stop, with two exceptions. A resolver that kept its resources for startup recovery reports `RebaseResourcesUnsettled`. A remote read that timed out on its own, or that gh answered, before the stop (the issue-trust read before launch) reports that timeout or answer (#174). Cleanup removes only the validated UUID workspace. A partially created but unregistered worktree is removed only after Git's own worktree list proves it is not registered. Result refs are created atomically from the absent state; a reused attempt ID cannot overwrite or delete a previously retained result, while an ambiguous create is reconciled against the exact attempted value. +All other conflicts are a review outcome (`RebaseConflict`). Cancellation and failures settle the Git process group before cleanup. When the resolver fails while a cancel, shutdown or the rebase work deadline is pending, the rebaser reports the stop, with two exceptions. A resolver that kept its resources for startup recovery reports `RebaseResourcesUnsettled`. A remote read that timed out on its own, or that gh answered, before the stop (the issue-trust read before launch) reports that timeout or answer (#174). When the stop is the work deadline, the production resolver has no time left for cleanup, so it usually keeps its resources and reports `RebaseResourcesUnsettled`, whose message includes the timeout or answer. Cleanup removes only the validated UUID workspace. A partially created but unregistered worktree is removed only after Git's own worktree list proves it is not registered. Result refs are created atomically from the absent state; a reused attempt ID cannot overwrite or delete a previously retained result, while an ambiguous create is reconciled against the exact attempted value. ## Restart recovery @@ -54,4 +54,4 @@ Every review reload in a preparation awaits a history worker (#170), so each one ## Regression evidence -`test/runner-rebase.test.ts` covers clean one-to-one replay, the already-current no-op, process-group ownership, the whole-operation deadline, owned and foreign conflict resolution, clean sibling files, gitlink-pointer auditing and manual gitlink-conflict routing, embedded-repository refusal, literal replacement-character paths, invalid ledger classification, outside-file, resolver-commit and rebase-control refusal, cancellation before and during Git, the reason a failed resolver reports when a stop is pending (#174), exact cleanup, retained refs, and a read-only source checkout. `test/runner-rebase-conflict.test.ts` covers owned-item prompt binding, plan-revision races before launch and copy-back, claim-before-prepare ordering, distinct allocation identities, bounded no-follow host snapshots, nested modify/delete copy-in, copy-back audit, child settlement deadlines, an authorization read's own timeout kept through a later cancel (#174), cancellation settlement, cleanup ordering, and retained ownership for unreleased resources. `test/runner-lifecycle-store.test.ts` covers admission, durable process and child ownership, compare-and-swap races, resolved-source validation, and owned/foreign provenance. `test/runner-recovery.test.ts` covers marker validation, live-group fail-closed recovery, exact child-storage recovery, partial-match refusal, and restart ordering. `test/agent-process-group.test.ts` covers delayed-signal suppression after exit and retained ownership for an orphaned group. The real-container suite covers dirty-conflict admission and bounded exact-path export. `test/history.test.ts` and `test/browser/review.spec.ts` cover Unplanned attribution and the visible conflict-resolution label. `test/runner-production.test.ts` covers exact-plan recovery routing and adapter construction. +`test/runner-rebase.test.ts` covers clean one-to-one replay, the already-current no-op, process-group ownership, the whole-operation deadline, owned and foreign conflict resolution, clean sibling files, gitlink-pointer auditing and manual gitlink-conflict routing, embedded-repository refusal, literal replacement-character paths, invalid ledger classification, outside-file, resolver-commit and rebase-control refusal, cancellation before and during Git, the reason a failed resolver reports when a stop is pending (#174), exact cleanup, retained refs, and a read-only source checkout. `test/runner-rebase-conflict.test.ts` covers owned-item prompt binding, plan-revision races before launch and copy-back, claim-before-prepare ordering, distinct allocation identities, bounded no-follow host snapshots, nested modify/delete copy-in, copy-back audit, child settlement deadlines, an authorization read's own timeout kept through a later cancel and carried in the retained-resources message when the work deadline passed (#174), cancellation settlement, cleanup ordering, and retained ownership for unreleased resources. `test/runner-lifecycle-store.test.ts` covers admission, durable process and child ownership, compare-and-swap races, resolved-source validation, and owned/foreign provenance. `test/runner-recovery.test.ts` covers marker validation, live-group fail-closed recovery, exact child-storage recovery, partial-match refusal, and restart ordering. `test/agent-process-group.test.ts` covers delayed-signal suppression after exit and retained ownership for an orphaned group. The real-container suite covers dirty-conflict admission and bounded exact-path export. `test/history.test.ts` and `test/browser/review.spec.ts` cover Unplanned attribution and the visible conflict-resolution label. `test/runner-production.test.ts` covers exact-plan recovery routing and adapter construction. diff --git a/runner/rebase.ts b/runner/rebase.ts index 9136bdc9..3c5faaf5 100644 --- a/runner/rebase.ts +++ b/runner/rebase.ts @@ -442,9 +442,11 @@ export class GitRebaser { catch (error) { // Retained ownership outranks cancellation/deadline reporting: live cleanup must preserve its parent workspace. if (error instanceof RebaseResourcesUnsettled) throw error; - // A remote read that timed out on its own (as the issue-trust read can), or that gh answered, keeps that reason - // though a cancel, shutdown or this deadline became pending while it stopped (#174). Had the stop come first, - // the read would have reported the stop instead: the gateways and the server's trust read keep the first reason. + // A remote read that timed out on its own, or that gh answered, keeps that reason though a cancel, shutdown or + // this deadline became pending while it stopped (#174). Had the stop come first, the read would have reported + // the stop instead: the gateways and the server's trust read keep the first reason. This relies on the + // issue-trust read being the production resolver's only GitHub read. A later gh call in the resolver must also + // report a stop that came first as the stop, or this check would report its answer in the stop's place. if (timedOut(error) || ghAnswered(error)) throw error; abortExpired(); controller.signal.throwIfAborted(); diff --git a/test/fixtures/gh-answers.ts b/test/fixtures/gh-answers.ts new file mode 100644 index 00000000..3efe74ba --- /dev/null +++ b/test/fixtures/gh-answers.ts @@ -0,0 +1,13 @@ +import { CommandFailed } from '../../github/run-with-input.ts'; + +/** + * gh's own exit for an issue read of an issue that does not exist, as `runWithInput` reports it. Checked with gh 2.102.0 + * (`gh api repos/cli/cli/issues/999999999`): exit status 1, `gh: Not Found (HTTP 404)` on stderr, and GitHub's JSON body + * on stdout. The message joins them as `runWithInput` does. The tests that use it read only the error's class (through + * `ghAnswered`) and its message, not the separate stderr and stdout fields. + */ +export function ghIssueNotFound(): CommandFailed { + const stderr = 'gh: Not Found (HTTP 404)'; + const stdout = '{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/issues#get-an-issue","status":"404"}'; + return new CommandFailed(`gh failed (exit 1): ${stderr}\n${stdout}`, stderr, stdout); +} diff --git a/test/pre-merge.test.ts b/test/pre-merge.test.ts index a58e1035..58e65193 100644 --- a/test/pre-merge.test.ts +++ b/test/pre-merge.test.ts @@ -22,7 +22,7 @@ import { ensureCommit, openRunnerRepository } from '../runner/runner-repository. import { BranchPushRefused } from '../runner/branch-push.ts'; import { UpstreamFailure } from '../runner/lifecycle.ts'; import { RemoteTimeout } from '../github/timeout.ts'; -import { CommandFailed } from '../github/run-with-input.ts'; +import { ghIssueNotFound } from './fixtures/gh-answers.ts'; // A passthrough, so a test can act while a preparation's worker history read is in flight. const historyReads = vi.hoisted(() => ({ during: undefined as (() => void) | undefined })); @@ -431,7 +431,7 @@ it('keeps a failed rebase bound to the review that started it when that review c it.each([ ['timed out', () => new RemoteTimeout('Issue retrieval timed out.')], - ['was answered by gh', () => new CommandFailed('gh failed (exit 1): HTTP 404: Not Found', 'HTTP 404: Not Found', '')], + ['was answered by gh', ghIssueNotFound], ] as const)('saves a preparation whose issue-trust read %s before shutdown with that answer, not the shutdown (#48)', async (_case, answer) => { const actionId = randomUUID(); let stoppedFirst = false; @@ -456,7 +456,7 @@ it.each([ const REAL_REBASE_TEST_TIMEOUT_MS = 60_000; const resolverTrustAnswers = [ ['timed out', () => new RemoteTimeout('Issue retrieval timed out.')], - ['was answered by gh', () => new CommandFailed('gh failed (exit 1): HTTP 404: Not Found', 'HTTP 404: Not Found', '')], + ['was answered by gh', ghIssueNotFound], ] as const; const resolverStops = { cancel: { reason: 'Task cancelled.', stop: (service: ReviewService, coordinator: PreMergeCoordinator) => diff --git a/test/runner-rebase-conflict.test.ts b/test/runner-rebase-conflict.test.ts index 7ea6b153..2e7d603a 100644 --- a/test/runner-rebase-conflict.test.ts +++ b/test/runner-rebase-conflict.test.ts @@ -217,6 +217,30 @@ describe('production rebase conflict resolver', () => { expect(f.store.getTask(identity).rebaseInProgress).toMatchObject({ conflict: null, processGroup: null }); }); + it('retains resources, with the timeout in its message, when the work deadline passed during a trust read that timed out (#174)', async () => { + const f = fixture(), x = deps(f), realNow = performance.now.bind(performance); + const cause = new RemoteTimeout('Issue retrieval timed out.'); + let expired = false, removes = 0; + vi.spyOn(performance, 'now').mockImplementation(() => expired ? Number.MAX_SAFE_INTEGER : realNow()); + x.d.remove = async () => { removes++; }; + const resolve = createForeignConflictResolver({ store: f.store, identity, planKey: f.planKey, + repository: { path: join(f.root, 'bare.git') } as RunnerRepository, runnerOwner: 'a'.repeat(32), + image: () => 'sha256:' + 'b'.repeat(64), token: 'secret', + // The read timed out, and the rebase work deadline (this resolver's deadline) passed before its error was handled. + authorize: async () => { + expired = true; + throw new UpstreamFailure(`Issue trust could not be verified: ${cause.message}`, { cause }); + }, + limits: { workBytes: 1, workInodes: 1, metadataBytes: 1, metadataInodes: 1 }, deps: x.d as never }); + const error = await resolve(input(f)).then(() => null, (failure: unknown) => failure as Error); + expect(expired).toBe(true); + // Cleanup has no time left, so it starts nothing and the resolver keeps its resources for startup recovery. + expect(removes).toBe(0); + expect(error).toBeInstanceOf(RebaseResourcesUnsettled); + expect(error?.message).toBe('Conflict resolution retained resources for startup recovery: Issue trust could not be verified: Issue retrieval timed out.'); + expect(f.store.getTask(identity).rebaseInProgress).toMatchObject({ conflict: { source: oid(2) } }); + }); + it('refuses launch and releases resources when the authorization hook throws synchronously', async () => { const f = fixture(), x = deps(f); const resolve = createForeignConflictResolver({ store: f.store, identity, planKey: f.planKey, diff --git a/test/runner-rebase.test.ts b/test/runner-rebase.test.ts index 09bd995f..b82188d4 100644 --- a/test/runner-rebase.test.ts +++ b/test/runner-rebase.test.ts @@ -14,7 +14,7 @@ import { listGitPaths } from '../runner/list-git-paths.ts'; import { outsideConflictDigest } from '../runner/hash-outside-conflict.ts'; import { UpstreamFailure } from '../runner/lifecycle.ts'; import { RemoteTimeout } from '../github/timeout.ts'; -import { CommandFailed } from '../github/run-with-input.ts'; +import { ghIssueNotFound } from './fixtures/gh-answers.ts'; const OWNER = '0123456789abcdef0123456789abcdef'; const roots: string[] = []; @@ -681,7 +681,7 @@ describe('trusted pre-merge rebase', () => { // The last row is a failure that is neither a timeout nor gh's answer: the same order reports the stop, as before. const answers = [ ['its own timeout', () => new RemoteTimeout('Issue retrieval timed out.'), true], - ["gh's answer", () => new CommandFailed('gh failed (exit 1): HTTP 404: Not Found', 'HTTP 404: Not Found', ''), true], + ["gh's answer", ghIssueNotFound, true], ['a failure of another kind', () => new Error('The trust read failed.'), false], ] as const; const run = (s: Awaited>, runner: GitRebaser, attemptId: string, signal?: AbortSignal) => From 6b782f83bc6f4950a7032403f96e21645c4b66bd Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 19:42:59 -0700 Subject: [PATCH 03/10] Address review round 2 of #197: remaining gh fakes, deadline wording, test cost - Replace the last issue-trust-read gh fakes in the old shape (runner-publishing.test.ts x2, merge-queue-poll.test.ts x1) with ghIssueNotFound(), keeping their assertions on the real message. - test/fixtures/gh-answers.ts: ghFailedExit() builds runWithInput's message, 2,000-character slices included; replayGh in test/fixtures/gh/recorded.ts now uses it, so there is one copy. - Drop the unused answer axis from the stop-first tests (pre-merge: 2 real-rebase tests instead of 4; runner-rebase: 1 instead of 2). - merge-queue.md: the production resolver does settle at the work deadline, by reporting retained resources, not the read's failure; a deadline that passes before the read fails, also while it runs, gives a message without the timeout text (traced, not tested). - pre-merge-rebase.md: regression evidence names the pre-merge.test.ts saved-outcome cases. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/merge-queue.md | 2 +- docs/implementation/pre-merge-rebase.md | 2 +- test/fixtures/gh-answers.ts | 21 +++++++++++++++------ test/fixtures/gh/recorded.ts | 5 ++--- test/merge-queue-poll.test.ts | 5 +++-- test/pre-merge.test.ts | 8 +++++--- test/runner-publishing.test.ts | 6 +++--- test/runner-rebase.test.ts | 5 +++-- 8 files changed, 33 insertions(+), 21 deletions(-) diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index 3c701d61..67e3ec8a 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -36,7 +36,7 @@ A GitHub read that stops reports the first reason it stopped for, and the merge - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver also reads issue trust (`runner/rebase-conflict.ts`), just before its agent would start. When that read fails, `GitRebaser` (`runner/rebase.ts`) reports the failure as follows (#174): - **Cancel or shutdown after the read failed.** If the read timed out, or gh answered, before the cancel or shutdown, the rebase reports that timeout or answer, and the preparation is saved as failed with it. Tested: the resolver in `test/runner-rebase-conflict.test.ts`, the rebaser in `test/runner-rebase.test.ts`, and the saved outcome through a real rebase in `test/pre-merge.test.ts`. - **Cancel or shutdown before the read.** The read reports the stop, and so does the rebase (tested). - - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not settle then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery, and the preparation is saved with that message, which includes the timeout or answer (tested in `test/runner-rebase-conflict.test.ts`). A deadline that came before the read is traced, not tested. + - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (tested in `test/runner-rebase-conflict.test.ts`). A deadline that passes before the read fails, including while the read is still running, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources, and that message has no timeout text. - **Other failures.** A read failure that is neither a timeout nor gh's answer still reports a stop that is pending when it is handled (tested). - **Retained resources.** A resolver that keeps its resources for startup recovery (`RebaseResourcesUnsettled`) is reported as that before any of the cases above (tested). - A publish whose trust read timed out, or that gh answered, before shutdown or cancellation is recorded as failed with that reason, not as stopped (tested for shutdown, `test/runner-publishing.test.ts`); cancellation takes the same path, both leave the publish owed, and a cancelled task's PRs are still closed (traced, not tested). diff --git a/docs/implementation/pre-merge-rebase.md b/docs/implementation/pre-merge-rebase.md index 18662152..46b0e34a 100644 --- a/docs/implementation/pre-merge-rebase.md +++ b/docs/implementation/pre-merge-rebase.md @@ -54,4 +54,4 @@ Every review reload in a preparation awaits a history worker (#170), so each one ## Regression evidence -`test/runner-rebase.test.ts` covers clean one-to-one replay, the already-current no-op, process-group ownership, the whole-operation deadline, owned and foreign conflict resolution, clean sibling files, gitlink-pointer auditing and manual gitlink-conflict routing, embedded-repository refusal, literal replacement-character paths, invalid ledger classification, outside-file, resolver-commit and rebase-control refusal, cancellation before and during Git, the reason a failed resolver reports when a stop is pending (#174), exact cleanup, retained refs, and a read-only source checkout. `test/runner-rebase-conflict.test.ts` covers owned-item prompt binding, plan-revision races before launch and copy-back, claim-before-prepare ordering, distinct allocation identities, bounded no-follow host snapshots, nested modify/delete copy-in, copy-back audit, child settlement deadlines, an authorization read's own timeout kept through a later cancel and carried in the retained-resources message when the work deadline passed (#174), cancellation settlement, cleanup ordering, and retained ownership for unreleased resources. `test/runner-lifecycle-store.test.ts` covers admission, durable process and child ownership, compare-and-swap races, resolved-source validation, and owned/foreign provenance. `test/runner-recovery.test.ts` covers marker validation, live-group fail-closed recovery, exact child-storage recovery, partial-match refusal, and restart ordering. `test/agent-process-group.test.ts` covers delayed-signal suppression after exit and retained ownership for an orphaned group. The real-container suite covers dirty-conflict admission and bounded exact-path export. `test/history.test.ts` and `test/browser/review.spec.ts` cover Unplanned attribution and the visible conflict-resolution label. `test/runner-production.test.ts` covers exact-plan recovery routing and adapter construction. +`test/runner-rebase.test.ts` covers clean one-to-one replay, the already-current no-op, process-group ownership, the whole-operation deadline, owned and foreign conflict resolution, clean sibling files, gitlink-pointer auditing and manual gitlink-conflict routing, embedded-repository refusal, literal replacement-character paths, invalid ledger classification, outside-file, resolver-commit and rebase-control refusal, cancellation before and during Git, the reason a failed resolver reports when a stop is pending (#174), exact cleanup, retained refs, and a read-only source checkout. `test/runner-rebase-conflict.test.ts` covers owned-item prompt binding, plan-revision races before launch and copy-back, claim-before-prepare ordering, distinct allocation identities, bounded no-follow host snapshots, nested modify/delete copy-in, copy-back audit, child settlement deadlines, an authorization read's own timeout kept through a later cancel and carried in the retained-resources message when the work deadline passed (#174), cancellation settlement, cleanup ordering, and retained ownership for unreleased resources. `test/pre-merge.test.ts` covers, through a real conflicting rebase, the saved preparation outcome when the resolver's trust read timed out or gh answered before a cancel or shutdown, and when the stop came first (#174). `test/runner-lifecycle-store.test.ts` covers admission, durable process and child ownership, compare-and-swap races, resolved-source validation, and owned/foreign provenance. `test/runner-recovery.test.ts` covers marker validation, live-group fail-closed recovery, exact child-storage recovery, partial-match refusal, and restart ordering. `test/agent-process-group.test.ts` covers delayed-signal suppression after exit and retained ownership for an orphaned group. The real-container suite covers dirty-conflict admission and bounded exact-path export. `test/history.test.ts` and `test/browser/review.spec.ts` cover Unplanned attribution and the visible conflict-resolution label. `test/runner-production.test.ts` covers exact-plan recovery routing and adapter construction. diff --git a/test/fixtures/gh-answers.ts b/test/fixtures/gh-answers.ts index 3efe74ba..90147dc8 100644 --- a/test/fixtures/gh-answers.ts +++ b/test/fixtures/gh-answers.ts @@ -1,13 +1,22 @@ import { CommandFailed } from '../../github/run-with-input.ts'; /** - * gh's own exit for an issue read of an issue that does not exist, as `runWithInput` reports it. Checked with gh 2.102.0 + * gh's failed exit as `runWithInput` (github/run-with-input.ts) reports it: a CommandFailed whose message is + * `gh failed (exit N): ` plus the first 2,000 characters of stderr, then, when stdout is not empty, a newline and the first + * 2,000 characters of stdout. `stderr` and `stdout` are kept whole, already trimmed as runWithInput trims them. + */ +export function ghFailedExit(exitCode: number, stderr: string, stdout: string): CommandFailed { + return new CommandFailed(`gh failed (exit ${exitCode}): ${stderr.slice(0, 2000)}${stdout ? `\n${stdout.slice(0, 2000)}` : ''}`, + stderr, stdout); +} + +/** + * gh's own exit for an issue read of an issue that does not exist. Checked with gh 2.102.0 * (`gh api repos/cli/cli/issues/999999999`): exit status 1, `gh: Not Found (HTTP 404)` on stderr, and GitHub's JSON body - * on stdout. The message joins them as `runWithInput` does. The tests that use it read only the error's class (through - * `ghAnswered`) and its message, not the separate stderr and stdout fields. + * on stdout. The tests that use it read only the error's class (through `ghAnswered`) and its message, not the separate + * stderr and stdout fields. */ export function ghIssueNotFound(): CommandFailed { - const stderr = 'gh: Not Found (HTTP 404)'; - const stdout = '{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/issues#get-an-issue","status":"404"}'; - return new CommandFailed(`gh failed (exit 1): ${stderr}\n${stdout}`, stderr, stdout); + return ghFailedExit(1, 'gh: Not Found (HTTP 404)', + '{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/issues#get-an-issue","status":"404"}'); } diff --git a/test/fixtures/gh/recorded.ts b/test/fixtures/gh/recorded.ts index c1b024f3..99704ba4 100644 --- a/test/fixtures/gh/recorded.ts +++ b/test/fixtures/gh/recorded.ts @@ -1,5 +1,5 @@ import { readFileSync } from 'node:fs'; -import { CommandFailed } from '../../../github/run-with-input.ts'; +import { ghFailedExit } from '../gh-answers.ts'; import type { RunGh } from '../../../github/merge.ts'; /** @@ -35,8 +35,7 @@ export function replayGh(...recordings: RecordedGh[]): RunGh & { calls: string[] // The gateway may turn this failure into unknown rules, so callers also assert `unrecorded` is empty. if (!answer) { unrecorded.push([...args]); throw new Error(`No recorded answer for gh ${args.join(' ')}`); } if (answer.exitCode === 0) return answer.stdout; - throw new CommandFailed(`gh failed (exit ${answer.exitCode}): ${answer.stderr.slice(0, 2000)}${answer.stdout ? `\n${answer.stdout.slice(0, 2000)}` : ''}`, - answer.stderr, answer.stdout); + throw ghFailedExit(answer.exitCode, answer.stderr, answer.stdout); }; return Object.assign(run, { calls, unrecorded }); } diff --git a/test/merge-queue-poll.test.ts b/test/merge-queue-poll.test.ts index 02ca6c60..4ddc85b0 100644 --- a/test/merge-queue-poll.test.ts +++ b/test/merge-queue-poll.test.ts @@ -10,6 +10,7 @@ import { Store } from '../runner/store.ts'; import { createDemo } from '../scripts/demo.ts'; import { startServer } from '../web/server.ts'; import { fixtureGit } from './fixtures/git.ts'; +import { ghIssueNotFound } from './fixtures/gh-answers.ts'; import { RemoteTimeout } from '../github/timeout.ts'; import { CommandFailed } from '../github/run-with-input.ts'; @@ -349,7 +350,7 @@ it('answers a merge click whose issue-trust read gh answered before the merge de // The first read admits the click; the refresh before admission is held until the deadline aborts it. if (++reads === 1) { slowNext = true; return { number, authorLogin: 'member', collaborator: true }; } readStarted(); await new Promise(resolve => options.signal?.addEventListener('abort', resolve, { once: true })); - throw new CommandFailed('gh failed (exit 1): HTTP 404: Not Found', '', ''); + throw ghIssueNotFound(); }, async issueText(number: number) { return { number, title: '', body: '', comments: [] }; }, }; @@ -362,7 +363,7 @@ it('answers a merge click whose issue-trust read gh answered before the merge de }), { github: true, issues }); try { const answer = async (response: Response) => ({ status: response.status, error: (await response.json() as { error: string }).error }); - const expected = { status: 502, error: 'Issue trust could not be verified: gh failed (exit 1): HTTP 404: Not Found' }; + const expected = { status: 502, error: `Issue trust could not be verified: ${ghIssueNotFound().message}` }; expect(await answer(await send())).toEqual(expected); await reading; // Saved: resending the same click replays the answer without reading trust again. diff --git a/test/pre-merge.test.ts b/test/pre-merge.test.ts index 58e65193..4de67425 100644 --- a/test/pre-merge.test.ts +++ b/test/pre-merge.test.ts @@ -492,8 +492,9 @@ it.each(resolverTrustAnswers.flatMap(([read, answer]) => (['cancel', 'shutdown'] await fixture.coordinator.close(); await fixture.runner.close(); }, REAL_REBASE_TEST_TIMEOUT_MS); -it.each(resolverTrustAnswers.flatMap(([read, answer]) => (['cancel', 'shutdown'] as const).map(stop => [stop, read, answer] as const)))( - "saves a preparation as stopped by a %s that came before the conflict resolver's issue-trust read %s (#174)", async (stopKind, _read, answer) => { +// The stop comes before the read can answer, so which answer it would have given does not matter: one is enough. +it.each(['cancel', 'shutdown'] as const)( + "saves a preparation as stopped by a %s that came before the conflict resolver's issue-trust read (#174)", async stopKind => { const actionId = randomUUID(); const { reason, stop } = resolverStops[stopKind]; let reads = 0, abortedBeforeRead = false; @@ -503,7 +504,8 @@ it.each(resolverTrustAnswers.flatMap(([read, answer]) => (['cancel', 'shutdown'] abortedBeforeRead = signal.aborted; // The stop came first: the gateway and readIssueAccess report it, not the answer the read would have given. if (signal.aborted) throw signal.reason; - throw new UpstreamFailure(`Issue trust could not be verified: ${answer().message}`, { cause: answer() }); + const answer = new RemoteTimeout('Issue retrieval timed out.'); + throw new UpstreamFailure(`Issue trust could not be verified: ${answer.message}`, { cause: answer }); } }); expect(reads).toBe(2); expect(fixture.resolverReads()).toBe(1); expect(abortedBeforeRead).toBe(true); diff --git a/test/runner-publishing.test.ts b/test/runner-publishing.test.ts index 296da0ad..b7dbbb7d 100644 --- a/test/runner-publishing.test.ts +++ b/test/runner-publishing.test.ts @@ -4,6 +4,7 @@ import { join } from 'node:path'; import { randomUUID } from 'node:crypto'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { fixtureGit as git } from './fixtures/git.ts'; +import { ghIssueNotFound } from './fixtures/gh-answers.ts'; import { createDemo } from '../scripts/demo.ts'; import { startServer } from '../web/server.ts'; import { Store, type PublishRecord } from '../runner/store.ts'; @@ -27,7 +28,6 @@ import type { PreMergeCoordinator } from '../runner/pre-merge.ts'; import { MergeJobCoordinator, SETTLED_BEFORE_JOB_MERGE, type MergeJobTiming } from '../runner/merge-job.ts'; import type { RemoteMergeState, RequiredCheck } from '../github/merge.ts'; import { RemoteTimeout } from '../github/timeout.ts'; -import { CommandFailed } from '../github/run-with-input.ts'; vi.setConfig({ testTimeout: 30_000 }); const roots: string[] = [], cleanups: (() => Promise | void)[] = []; @@ -631,7 +631,7 @@ describe('publishing a finished task (#103)', () => { it.each([ ['timed out', () => new RemoteTimeout('Issue retrieval timed out.'), /could not be verified: Issue retrieval timed out/], - ['was answered by gh', () => new CommandFailed('gh failed (exit 1): HTTP 404: Not Found', 'HTTP 404: Not Found', ''), /could not be verified: gh failed \(exit 1\): HTTP 404/], + ['was answered by gh', ghIssueNotFound, /could not be verified: gh failed \(exit 1\): gh: Not Found \(HTTP 404\)/], ] as const)('records a publish whose issue-trust read %s before shutdown as failed with that answer, not stopped (#48)', async (_case, answer, expected) => { const w = world(), reading = Promise.withResolvers(), release = Promise.withResolvers(); let accessSignal: AbortSignal | undefined; @@ -2120,7 +2120,7 @@ describe('the one-click merge job (F6b of #22)', () => { it.each([ ['timed out', () => new RemoteTimeout('Issue retrieval timed out.'), 503, 'Issue trust could not be verified: Issue retrieval timed out.'], - ['gh answered', () => new CommandFailed('gh failed (exit 1): HTTP 404: Not Found', '', ''), 502, 'Issue trust could not be verified: gh failed (exit 1): HTTP 404: Not Found'], + ['gh answered', ghIssueNotFound, 502, `Issue trust could not be verified: ${ghIssueNotFound().message}`], ] as const)('keeps a merge click\'s trust read failure (%s) when shutdown starts while gh stops, saving nothing', async (_name, failure, status, error) => { const w = world(); moveUserHead(w); let storeRef: Store | null = null, hang = false, fail!: (error: Error) => void, reading!: () => void; diff --git a/test/runner-rebase.test.ts b/test/runner-rebase.test.ts index b82188d4..885d4804 100644 --- a/test/runner-rebase.test.ts +++ b/test/runner-rebase.test.ts @@ -729,13 +729,14 @@ describe('trusted pre-merge rebase', () => { expect(existsSync(join(s.runnerRoot, OWNER, 'rebases', attemptId))).toBe(false); }); - it.each(answers.slice(0, 2))('reports a cancel that came before the trust read could give %s', async (_case, answer) => { + // The cancel comes before the read can answer, so which answer it would have given does not matter: one is enough. + it('reports a cancel that came before the trust read could answer', async () => { const s = await setup('foreign'), attemptId = randomUUID(), stop = new AbortController(); let abortedBeforeRead: boolean | null = null; const runner = createRebaser(s, { resolveForeignConflict: async input => { stop.abort(new Error('Task cancelled.')); abortedBeforeRead = input.signal!.aborted; - await trustRead(input.signal!, answer()); + await trustRead(input.signal!, new RemoteTimeout('Issue retrieval timed out.')); } }); const error = await run(s, runner, attemptId, stop.signal); expect(abortedBeforeRead).toBe(true); From a75ab812c7aff4c83cfa1a97a92d33adcbbdc12a Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 20:00:06 -0700 Subject: [PATCH 04/10] Address review round 3 of #197: test titles and stop wording Name what each trust-read row reports, so the control row's title no longer says the opposite of its assertion; say that any other stop of the rebase takes the cancel's path; point the saved-message claim at the test that covers it. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/merge-queue.md | 3 ++- docs/implementation/pre-merge-rebase.md | 2 +- test/runner-rebase.test.ts | 11 ++++++----- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index 67e3ec8a..f2eea4dd 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -36,7 +36,8 @@ A GitHub read that stops reports the first reason it stopped for, and the merge - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver also reads issue trust (`runner/rebase-conflict.ts`), just before its agent would start. When that read fails, `GitRebaser` (`runner/rebase.ts`) reports the failure as follows (#174): - **Cancel or shutdown after the read failed.** If the read timed out, or gh answered, before the cancel or shutdown, the rebase reports that timeout or answer, and the preparation is saved as failed with it. Tested: the resolver in `test/runner-rebase-conflict.test.ts`, the rebaser in `test/runner-rebase.test.ts`, and the saved outcome through a real rebase in `test/pre-merge.test.ts`. - **Cancel or shutdown before the read.** The read reports the stop, and so does the rebase (tested). - - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (tested in `test/runner-rebase-conflict.test.ts`). A deadline that passes before the read fails, including while the read is still running, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources, and that message has no timeout text. + - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (the resolver's message is tested in `test/runner-rebase-conflict.test.ts`; that the preparation saves any `RebaseResourcesUnsettled` message is tested in `test/pre-merge.test.ts`). A deadline that passes before the read fails, including while the read is still running, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources, and that message has no timeout text. + - **Other stops.** Any other stop of the rebase, such as the preparation's own deadline, takes the same path as a cancel here (traced, not tested). - **Other failures.** A read failure that is neither a timeout nor gh's answer still reports a stop that is pending when it is handled (tested). - **Retained resources.** A resolver that keeps its resources for startup recovery (`RebaseResourcesUnsettled`) is reported as that before any of the cases above (tested). - A publish whose trust read timed out, or that gh answered, before shutdown or cancellation is recorded as failed with that reason, not as stopped (tested for shutdown, `test/runner-publishing.test.ts`); cancellation takes the same path, both leave the publish owed, and a cancelled task's PRs are still closed (traced, not tested). diff --git a/docs/implementation/pre-merge-rebase.md b/docs/implementation/pre-merge-rebase.md index 46b0e34a..f1208b13 100644 --- a/docs/implementation/pre-merge-rebase.md +++ b/docs/implementation/pre-merge-rebase.md @@ -24,7 +24,7 @@ Before any clone or Docker operation, the production adapter records a unique ch The Store records each resolved foreign source SHA before the result ref write. Preparation refuses duplicates, sources outside the captured history, owned sources, and sources that did not actually rewrite. On publication, a foreign mapped commit retains `owner: null`, `origin: foreign`, and `sourceSha`, plus `conflictResolved: true`. Linking therefore keeps its lines in Unplanned while the review UI adds the text label “conflict resolved by agent.” An owned mapped commit instead retains its Px owner and source SHA without foreign-resolution provenance; its preserved message keeps the original plan trailers, and the rewritten mapping lets review recompute whether Px's approval became stale. Every replay preserves the original commit author. -All other conflicts are a review outcome (`RebaseConflict`). Cancellation and failures settle the Git process group before cleanup. When the resolver fails while a cancel, shutdown or the rebase work deadline is pending, the rebaser reports the stop, with two exceptions. A resolver that kept its resources for startup recovery reports `RebaseResourcesUnsettled`. A remote read that timed out on its own, or that gh answered, before the stop (the issue-trust read before launch) reports that timeout or answer (#174). When the stop is the work deadline, the production resolver has no time left for cleanup, so it usually keeps its resources and reports `RebaseResourcesUnsettled`, whose message includes the timeout or answer. Cleanup removes only the validated UUID workspace. A partially created but unregistered worktree is removed only after Git's own worktree list proves it is not registered. Result refs are created atomically from the absent state; a reused attempt ID cannot overwrite or delete a previously retained result, while an ambiguous create is reconciled against the exact attempted value. +All other conflicts are a review outcome (`RebaseConflict`). Cancellation and failures settle the Git process group before cleanup. When the resolver fails while a cancel, shutdown, the rebase work deadline or any other stop of the rebase (such as the preparation's own deadline) is pending, the rebaser reports the stop, with two exceptions. A resolver that kept its resources for startup recovery reports `RebaseResourcesUnsettled`. A remote read that timed out on its own, or that gh answered, before the stop (the issue-trust read before launch) reports that timeout or answer (#174). When the stop is the work deadline, the production resolver has no time left for cleanup, so it usually keeps its resources and reports `RebaseResourcesUnsettled`, whose message includes the timeout or answer. Cleanup removes only the validated UUID workspace. A partially created but unregistered worktree is removed only after Git's own worktree list proves it is not registered. Result refs are created atomically from the absent state; a reused attempt ID cannot overwrite or delete a previously retained result, while an ambiguous create is reconciled against the exact attempted value. ## Restart recovery diff --git a/test/runner-rebase.test.ts b/test/runner-rebase.test.ts index 885d4804..0fabf839 100644 --- a/test/runner-rebase.test.ts +++ b/test/runner-rebase.test.ts @@ -679,16 +679,17 @@ describe('trusted pre-merge rebase', () => { const trustRead = (signal: AbortSignal, answer: Error): Promise => signal.aborted ? Promise.reject(signal.reason) : Promise.reject(new UpstreamFailure(`Issue trust could not be verified: ${answer.message}`, { cause: answer })); // The last row is a failure that is neither a timeout nor gh's answer: the same order reports the stop, as before. + // Each row: the read's failure, what is reported, the failure itself, and whether the failure is reported. const answers = [ - ['its own timeout', () => new RemoteTimeout('Issue retrieval timed out.'), true], - ["gh's answer", ghIssueNotFound, true], - ['a failure of another kind', () => new Error('The trust read failed.'), false], + ['its own timeout', 'the timeout', () => new RemoteTimeout('Issue retrieval timed out.'), true], + ["gh's answer", "gh's answer", ghIssueNotFound, true], + ['a failure of another kind', 'the stop', () => new Error('The trust read failed.'), false], ] as const; const run = (s: Awaited>, runner: GitRebaser, attemptId: string, signal?: AbortSignal) => runner.run({ attemptId, oldBase: s.base, oldHead: s.foreign, oldHistory: s.history, onto: s.onto, signal, ledger: [{ sha: s.foreign, owner: null, origin: 'foreign' }] }).then(() => null, (error: unknown) => error as Error); - it.each(answers)('reports %s that came before a cancel, not the cancel', async (_case, answer, kept) => { + it.each(answers)('when %s came before a cancel, reports %s', async (_case, _reported, answer, kept) => { const s = await setup('foreign'), attemptId = randomUUID(), stop = new AbortController(); const cause = answer(), expected = `Issue trust could not be verified: ${cause.message}`; let pending: unknown; @@ -707,7 +708,7 @@ describe('trusted pre-merge rebase', () => { expect(existsSync(join(s.runnerRoot, OWNER, 'rebases', attemptId))).toBe(false); }); - it.each(answers)('reports %s that came before the rebase work deadline, not the deadline', async (_case, answer, kept) => { + it.each(answers)('when %s came before the rebase work deadline, reports %s', async (_case, _reported, answer, kept) => { const s = await setup('foreign'), attemptId = randomUUID(), realNow = performance.now.bind(performance); const cause = answer(), expected = `Issue trust could not be verified: ${cause.message}`; let skew = 0, overdueMs = 0, aborted: boolean | null = null; From 864f18450f46420724fa3104fd70c57570ed93e3 Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 20:14:46 -0700 Subject: [PATCH 05/10] Test retained ownership's precedence with a cause that is not a timeout (#197 review round 4) The retained-ownership test's cause was a timeout, so the new timeout check kept it even without the precedence line. A second row with a failure of another kind fails without that line. Co-Authored-By: Claude Opus 5.5 --- test/runner-rebase.test.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/test/runner-rebase.test.ts b/test/runner-rebase.test.ts index 0fabf839..f8fdbc58 100644 --- a/test/runner-rebase.test.ts +++ b/test/runner-rebase.test.ts @@ -744,11 +744,18 @@ describe('trusted pre-merge rebase', () => { expect(error?.message).toBe('Task cancelled.'); }); - it('keeps retained resource ownership ahead of a trust-read timeout and a later cancel', async () => { + // The second row's cause is neither a timeout nor gh's answer, so only the precedence of retained ownership keeps it + // from being reported as the pending cancel. + it.each([ + ['a trust-read timeout', () => new RemoteTimeout('Issue retrieval timed out.'), + /retained resources for startup recovery: Issue trust could not be verified: Issue retrieval timed out/], + ['a failure of another kind', () => new Error('The trust read failed.'), + /retained resources for startup recovery: Issue trust could not be verified: The trust read failed/], + ] as const)('keeps retained resource ownership ahead of %s and a later cancel', async (_case, failure, message) => { const s = await setup('foreign'), attemptId = randomUUID(), stop = new AbortController(); let pending = false; const runner = createRebaser(s, { resolveForeignConflict: async input => { - const read = trustRead(input.signal!, new RemoteTimeout('Issue retrieval timed out.')); + const read = trustRead(input.signal!, failure()); stop.abort(new Error('Task cancelled.')); pending = input.signal!.aborted; // The resolver's cleanup did not settle, so it retains its resources, as createConflictResolver reports it. @@ -759,7 +766,7 @@ describe('trusted pre-merge rebase', () => { const error = await run(s, runner, attemptId, stop.signal); expect(pending).toBe(true); expect(error).toBeInstanceOf(RebaseResourcesUnsettled); - expect(error?.message).toMatch(/retained resources for startup recovery: Issue trust could not be verified: Issue retrieval timed out/); + expect(error?.message).toMatch(message); expect(existsSync(join(s.runnerRoot, OWNER, 'rebases', attemptId))).toBe(true); }); }); From a60b4684a4ee3f8571fe48aad8d0b3390bb1e86f Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 20:23:05 -0700 Subject: [PATCH 06/10] Say that the deadline case covers gh still stopping after the read's own timeout (#197 review round 5) Co-Authored-By: Claude Opus 5.5 --- docs/implementation/merge-queue.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index f2eea4dd..4d099301 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -36,7 +36,7 @@ A GitHub read that stops reports the first reason it stopped for, and the merge - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver also reads issue trust (`runner/rebase-conflict.ts`), just before its agent would start. When that read fails, `GitRebaser` (`runner/rebase.ts`) reports the failure as follows (#174): - **Cancel or shutdown after the read failed.** If the read timed out, or gh answered, before the cancel or shutdown, the rebase reports that timeout or answer, and the preparation is saved as failed with it. Tested: the resolver in `test/runner-rebase-conflict.test.ts`, the rebaser in `test/runner-rebase.test.ts`, and the saved outcome through a real rebase in `test/pre-merge.test.ts`. - **Cancel or shutdown before the read.** The read reports the stop, and so does the rebase (tested). - - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (the resolver's message is tested in `test/runner-rebase-conflict.test.ts`; that the preparation saves any `RebaseResourcesUnsettled` message is tested in `test/pre-merge.test.ts`). A deadline that passes before the read fails, including while the read is still running, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources, and that message has no timeout text. + - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (the resolver's message is tested in `test/runner-rebase-conflict.test.ts`; that the preparation saves any `RebaseResourcesUnsettled` message is tested in `test/pre-merge.test.ts`). A deadline that passes before the read fails, including while the read is still running and after the read's own timeout fired but before gh stopped, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources ("Conflict child settlement exceeded the rebase work deadline."), and that message has no timeout text. - **Other stops.** Any other stop of the rebase, such as the preparation's own deadline, takes the same path as a cancel here (traced, not tested). - **Other failures.** A read failure that is neither a timeout nor gh's answer still reports a stop that is pending when it is handled (tested). - **Retained resources.** A resolver that keeps its resources for startup recovery (`RebaseResourcesUnsettled`) is reported as that before any of the cases above (tested). From 1584cd26cfc19b8090a777f39dd584811f6016a0 Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 20:30:59 -0700 Subject: [PATCH 07/10] Save the action in the unsettled-conflict test, so the cited claim is tested (#197 review round 6) Co-Authored-By: Claude Opus 5.5 --- test/pre-merge.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/pre-merge.test.ts b/test/pre-merge.test.ts index 4de67425..81c5c6b6 100644 --- a/test/pre-merge.test.ts +++ b/test/pre-merge.test.ts @@ -396,9 +396,13 @@ it('marks a failed preparation stale when its final remote inspection was invali await fixture.coordinator.close(); await fixture.runner.close(); }); -it('leaves an unsettled conflict and its rebase marker for startup recovery', async () => { - const fixture = await rebaseFixture('feature\n', undefined, { unsettledRebase: true }); +it('leaves an unsettled conflict and its rebase marker for startup recovery, and saves its message as the action\'s reason', async () => { + const actionId = randomUUID(); + const fixture = await rebaseFixture('feature\n', undefined, { unsettledRebase: true, actionId }); expect(fixture.result).toMatchObject({ state: 'failed', reason: 'Conflict resources remain owned.' }); + // The saved prepare-merge answer carries the RebaseResourcesUnsettled message, as a resend replays it. + expect(fixture.service.store.savedAction(fixture.service.config.identity, { actionId, kind: 'prepare-merge', request: fixture.actionRequest })?.response) + .toMatchObject({ outcome: 'failed', reason: 'Conflict resources remain owned.' }); expect(fixture.coordinator.last).toMatchObject({ state: 'failed', stale: false }); expect(fixture.rebaseAborts()).toBe(0); expect(fixture.service.store.getTask(fixture.service.config.identity).rebaseInProgress).not.toBeNull(); From c709c651883786037089bcc60a24d22d696f6cfe Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 20:41:26 -0700 Subject: [PATCH 08/10] Quote the whole retained-resources message in the deadline case (#197 review round 7) Co-Authored-By: Claude Opus 5.5 --- docs/implementation/merge-queue.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index 4d099301..efae12e4 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -36,7 +36,7 @@ A GitHub read that stops reports the first reason it stopped for, and the merge - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver also reads issue trust (`runner/rebase-conflict.ts`), just before its agent would start. When that read fails, `GitRebaser` (`runner/rebase.ts`) reports the failure as follows (#174): - **Cancel or shutdown after the read failed.** If the read timed out, or gh answered, before the cancel or shutdown, the rebase reports that timeout or answer, and the preparation is saved as failed with it. Tested: the resolver in `test/runner-rebase-conflict.test.ts`, the rebaser in `test/runner-rebase.test.ts`, and the saved outcome through a real rebase in `test/pre-merge.test.ts`. - **Cancel or shutdown before the read.** The read reports the stop, and so does the rebase (tested). - - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (the resolver's message is tested in `test/runner-rebase-conflict.test.ts`; that the preparation saves any `RebaseResourcesUnsettled` message is tested in `test/pre-merge.test.ts`). A deadline that passes before the read fails, including while the read is still running and after the read's own timeout fired but before gh stopped, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources ("Conflict child settlement exceeded the rebase work deadline."), and that message has no timeout text. + - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (the resolver's message is tested in `test/runner-rebase-conflict.test.ts`; that the preparation saves any `RebaseResourcesUnsettled` message is tested in `test/pre-merge.test.ts`). A deadline that passes before the read fails, including while the read is still running and after the read's own timeout fired but before gh stopped, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources ("Conflict resolution retained resources for startup recovery: Conflict child settlement exceeded the rebase work deadline.", or with "The rebase deadline expired during conflict resolution." when the rebaser's own deadline timer fires first), and that message has no timeout text. - **Other stops.** Any other stop of the rebase, such as the preparation's own deadline, takes the same path as a cancel here (traced, not tested). - **Other failures.** A read failure that is neither a timeout nor gh's answer still reports a stop that is pending when it is handled (tested). - **Retained resources.** A resolver that keeps its resources for startup recovery (`RebaseResourcesUnsettled`) is reported as that before any of the cases above (tested). From d99298a49c005a83adc1dc4b76439ee31d92aa5a Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 21:10:16 -0700 Subject: [PATCH 09/10] Name the conditions of the deadline message and the resolver test's case (#197 review round 8) Co-Authored-By: Claude Opus 5.5 --- docs/implementation/merge-queue.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index efae12e4..afc73625 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -34,9 +34,9 @@ A GitHub read that stops reports the first reason it stopped for, and the merge - **Issue-trust reads.** The server wraps a failed trust read as an upstream failure and keeps the timeout as its cause. On the merge path, a trust read that timed out, or that gh answered, before the caller's abort still reports that; the runner actions keep a trust read's own timeout too, even when shutdown started while gh stopped (tested), and answer any other failed trust read as shutting down once shutdown has started (the server is stopping, the runner is closing, or the request was aborted, which only shutdown does). Either way nothing is admitted. - The trust read that gates the runner actions (start, resume, approve-continuation, publish, prepare-merge) is not saved as the action's outcome when it times out (tested for start; the other actions share the same branch): the action answers 503, and the same action ID reads trust again; an outcome already saved under that ID is still the answer, also when shutdown started while gh stopped (tested). - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver also reads issue trust (`runner/rebase-conflict.ts`), just before its agent would start. When that read fails, `GitRebaser` (`runner/rebase.ts`) reports the failure as follows (#174): - - **Cancel or shutdown after the read failed.** If the read timed out, or gh answered, before the cancel or shutdown, the rebase reports that timeout or answer, and the preparation is saved as failed with it. Tested: the resolver in `test/runner-rebase-conflict.test.ts`, the rebaser in `test/runner-rebase.test.ts`, and the saved outcome through a real rebase in `test/pre-merge.test.ts`. + - **Cancel or shutdown after the read failed.** If the read timed out, or gh answered, before the cancel or shutdown, the rebase reports that timeout or answer, and the preparation is saved as failed with it. Tested: the resolver in `test/runner-rebase-conflict.test.ts` (a timeout, then a cancel), the rebaser in `test/runner-rebase.test.ts`, and the saved outcome through a real rebase in `test/pre-merge.test.ts`. - **Cancel or shutdown before the read.** The read reports the stop, and so does the rebase (tested). - - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (the resolver's message is tested in `test/runner-rebase-conflict.test.ts`; that the preparation saves any `RebaseResourcesUnsettled` message is tested in `test/pre-merge.test.ts`). A deadline that passes before the read fails, including while the read is still running and after the read's own timeout fired but before gh stopped, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources ("Conflict resolution retained resources for startup recovery: Conflict child settlement exceeded the rebase work deadline.", or with "The rebase deadline expired during conflict resolution." when the rebaser's own deadline timer fires first), and that message has no timeout text. + - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (the resolver's message is tested in `test/runner-rebase-conflict.test.ts`; that the preparation saves any `RebaseResourcesUnsettled` message is tested in `test/pre-merge.test.ts`). A deadline that passes before the read fails, including while the read is still running and after the read's own timeout fired but before gh stopped, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources ("Conflict resolution retained resources for startup recovery: Conflict child settlement exceeded the rebase work deadline.", or with "The rebase deadline expired during conflict resolution." when the rebaser's own deadline timer fires first and the read reports that stop before the resolver's own deadline timer fires), and that message has no timeout text. - **Other stops.** Any other stop of the rebase, such as the preparation's own deadline, takes the same path as a cancel here (traced, not tested). - **Other failures.** A read failure that is neither a timeout nor gh's answer still reports a stop that is pending when it is handled (tested). - **Retained resources.** A resolver that keeps its resources for startup recovery (`RebaseResourcesUnsettled`) is reported as that before any of the cases above (tested). From 5105e982e82fd3664e924adcba2069c939d310f4 Mon Sep 17 00:00:00 2001 From: mchwang Date: Tue, 20 Oct 2026 21:55:17 -0700 Subject: [PATCH 10/10] Name when the deadline test's deadline passes, and split the deadline sentence (#197 review round 9) Co-Authored-By: Claude Opus 5.5 --- docs/implementation/merge-queue.md | 2 +- test/runner-rebase-conflict.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index afc73625..a57b3035 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -36,7 +36,7 @@ A GitHub read that stops reports the first reason it stopped for, and the merge - The trust read inside merge preparation (`runner/pre-merge.ts`) is different: a timeout or gh answer there settles the preparation as failed, which is saved and replayed, and a shutdown that arrives after it is reported as that timeout or answer (tested: the preparation layer in `test/pre-merge.test.ts`, with the error shape the server's trust read builds, and that read itself through publishing in `test/runner-publishing.test.ts`); a cancel takes the same path (traced, not tested). The rebase conflict resolver also reads issue trust (`runner/rebase-conflict.ts`), just before its agent would start. When that read fails, `GitRebaser` (`runner/rebase.ts`) reports the failure as follows (#174): - **Cancel or shutdown after the read failed.** If the read timed out, or gh answered, before the cancel or shutdown, the rebase reports that timeout or answer, and the preparation is saved as failed with it. Tested: the resolver in `test/runner-rebase-conflict.test.ts` (a timeout, then a cancel), the rebaser in `test/runner-rebase.test.ts`, and the saved outcome through a real rebase in `test/pre-merge.test.ts`. - **Cancel or shutdown before the read.** The read reports the stop, and so does the rebase (tested). - - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (the resolver's message is tested in `test/runner-rebase-conflict.test.ts`; that the preparation saves any `RebaseResourcesUnsettled` message is tested in `test/pre-merge.test.ts`). A deadline that passes before the read fails, including while the read is still running and after the read's own timeout fired but before gh stopped, is traced, not tested: the resolver stops waiting for the read at its deadline and reports retained resources ("Conflict resolution retained resources for startup recovery: Conflict child settlement exceeded the rebase work deadline.", or with "The rebase deadline expired during conflict resolution." when the rebaser's own deadline timer fires first and the read reports that stop before the resolver's own deadline timer fires), and that message has no timeout text. + - **Rebase work deadline.** The rebaser itself keeps a timeout or answer that came before its deadline, when the resolver settles (tested in `test/runner-rebase.test.ts`). The production resolver usually does not report the read's failure itself then: its own deadline is the same work deadline, so its cleanup has no time left. It keeps its resources for startup recovery and reports that, and the preparation is saved with that message, which includes the timeout or answer (the resolver's message is tested in `test/runner-rebase-conflict.test.ts`; that the preparation saves any `RebaseResourcesUnsettled` message is tested in `test/pre-merge.test.ts`). A deadline that passes before the read fails is traced, not tested. This includes a deadline that passes while the read is still running, and one that passes after the read's own timeout fired but before gh stopped. The resolver then stops waiting for the read at its deadline and reports retained resources, and that message has no timeout text. Usually the message is "Conflict resolution retained resources for startup recovery: Conflict child settlement exceeded the rebase work deadline." It ends with "The rebase deadline expired during conflict resolution." instead when the rebaser's own deadline timer fires first and the read reports that stop before the resolver's own deadline timer fires. - **Other stops.** Any other stop of the rebase, such as the preparation's own deadline, takes the same path as a cancel here (traced, not tested). - **Other failures.** A read failure that is neither a timeout nor gh's answer still reports a stop that is pending when it is handled (tested). - **Retained resources.** A resolver that keeps its resources for startup recovery (`RebaseResourcesUnsettled`) is reported as that before any of the cases above (tested). diff --git a/test/runner-rebase-conflict.test.ts b/test/runner-rebase-conflict.test.ts index 2e7d603a..8405c6e8 100644 --- a/test/runner-rebase-conflict.test.ts +++ b/test/runner-rebase-conflict.test.ts @@ -217,7 +217,7 @@ describe('production rebase conflict resolver', () => { expect(f.store.getTask(identity).rebaseInProgress).toMatchObject({ conflict: null, processGroup: null }); }); - it('retains resources, with the timeout in its message, when the work deadline passed during a trust read that timed out (#174)', async () => { + it('retains resources, with the timeout in its message, when the work deadline passed after a trust read timed out, before its error was handled (#174)', async () => { const f = fixture(), x = deps(f), realNow = performance.now.bind(performance); const cause = new RemoteTimeout('Issue retrieval timed out.'); let expired = false, removes = 0;