diff --git a/docs/implementation/merge-queue.md b/docs/implementation/merge-queue.md index 4dfddd99..a57b3035 100644 --- a/docs/implementation/merge-queue.md +++ b/docs/implementation/merge-queue.md @@ -33,7 +33,13 @@ 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 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 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). - 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..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. 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 @@ -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 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/runner/rebase.ts b/runner/rebase.ts index a567148f..3c5faaf5 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,12 @@ 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, 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(); throw error; diff --git a/test/fixtures/gh-answers.ts b/test/fixtures/gh-answers.ts new file mode 100644 index 00000000..90147dc8 --- /dev/null +++ b/test/fixtures/gh-answers.ts @@ -0,0 +1,22 @@ +import { CommandFailed } from '../../github/run-with-input.ts'; + +/** + * 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 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 { + 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 52708ee0..81c5c6b6 100644 --- a/test/pre-merge.test.ts +++ b/test/pre-merge.test.ts @@ -17,11 +17,12 @@ 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'; -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 })); @@ -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 () => { @@ -359,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(); @@ -394,7 +435,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; @@ -414,6 +455,71 @@ 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', ghIssueNotFound], +] 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); + +// 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; + 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; + 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); + 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-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-conflict.test.ts b/test/runner-rebase-conflict.test.ts index 609bc12f..8405c6e8 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,53 @@ 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('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; + 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 cc47ca69..f8fdbc58 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 { ghIssueNotFound } from './fixtures/gh-answers.ts'; const OWNER = '0123456789abcdef0123456789abcdef'; const roots: string[] = []; @@ -666,6 +669,108 @@ 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. + // Each row: the read's failure, what is reported, the failure itself, and whether the failure is reported. + const answers = [ + ['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)('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; + 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)('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; + 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); + }); + + // 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!, new RemoteTimeout('Issue retrieval timed out.')); + } }); + const error = await run(s, runner, attemptId, stop.signal); + expect(abortedBeforeRead).toBe(true); + expect(error?.message).toBe('Task cancelled.'); + }); + + // 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!, 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. + 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(message); + 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 => {