diff --git a/docs/implementation/pull-request-opening.md b/docs/implementation/pull-request-opening.md index 6115e433..77720677 100644 --- a/docs/implementation/pull-request-opening.md +++ b/docs/implementation/pull-request-opening.md @@ -133,7 +133,7 @@ Some repositories do not support draft PRs (for example private repositories on - **Base branch.** `github.baseBranch` is required with a runner block. The `gh` calls run with the same environment as the push (its `GH_HOST` and credentials). The CLI checks it before it takes the lock, and `setUpRunner` checks it again before anything is created. - **Mode.** The task's status decides it. A `running` task whose every plan item has completed at the current revision gets a ready PR. A task in `needs human`, by any path (a safety finding, the time budget), gets a draft PR. Its problems say what sent it there now: its current attempt's safety finding (or, when that finding's save failed, the attempt's diagnostic that carries it), else "time budget ran out" when the budget has passed, else that attempt's diagnostic. A budget that ran out after a safety finding does not hide the finding. An earlier attempt's finding, which a person may have dealt with, is not used. Any other status, an active attempt, a run still in progress, or a publish already in progress refuses. A draft waits only for an owed safety finding, not an owed scope pause: a draft invites no review, and a task in needs human cannot record a pause. A ready publish also needs every item of the current plan revision to have completed at that revision. The Store's publish guard checks this before every guarded write, so a revision that adds an item while a publish awaits GitHub refuses it. So does work an earlier run still owes: a safety finding not yet acted on, or a scope pause never recorded (its write failed, or the process stopped). Every plan item has run, so no later run would pay it, and a ready PR would show unreviewed out-of-scope changes. A run's end, startup and a refused `publish` action therefore have the executor pay it first (`ItemExecutor.payOwed`, as a run does before its first item). The task then moves to needs amendment or needs human, and a draft follows for the latter. For a task already in needs human, only an owed safety finding is paid. That draft names the paid finding, which only this process holds. It does so only while the task stays in needs human on the same current attempt. A later run that returns the task to needs human has its own reason. - **When.** Once a run started by `start` or `resume` has ended, with no attempt active (`assertPublishable` refuses one). On the `publish` runner action (`expectedStateVersion` and an action ID, like the other runner actions), which retries a refused or failed publish. When a refused `start` or `resume` moves the task to needs human (an expired budget is committed with the refusal); a refused action that leaves the status as it was publishes nothing, so pressing resume on a task already in needs human never pushes. Once at startup, if a publish is owed: the CLI starts it through `publishOwed`, which first runs the check that the runner lock still names the database, because a publish pushes; if the check fails, nothing starts. A task recorded as `not published` by the v10 upgrade stays unpublished until a person uses the `publish` action, or until a run starts after the upgrade (its end is the first state the upgrade did not decide), or a person's `start` or `resume` moves the task. Otherwise a publish is owed when the task can be published and no publish has settled (`opened`, `possibly already fixed`, `draft skipped`, `draft unsupported`, `no changes`) at the task's current state version. So a refusal, a failure or a stop leaves it owed, and the next start recovers a lost opening by its marker. A publish that ends with an opening still in flight (its own reply was lost, or it was refused while one settles) is tried again at that opening's own deadline, plus a second (`settleRemaining`; a timer, cleared at shutdown). A short retry, after 30 seconds, follows a publish refused because GitHub's PR list lags behind a recorded PR, one refused by a guard (mostly a race: the task, its review or its head changed during the publish; a refusal that persists is in the mode too, so the retry finds nothing owed), one refused just as an opening's deadline passed, and one whose open or update left work behind. That last case covers GitHub showing another head than the one pushed (the PR is made a draft and the task stays running), and a draft change that failed. Such an `opened` outcome is recorded with `reconcile`, which keeps the publish owed. Retries are bounded: one timer per task, and per chain at most one retry at an opening's deadline (while it is still ahead) and one short retry. A chain ends when a publish settles. A new one starts with a run's end, a person's action or a restart, never with a retry's own publish. Whatever is still unsettled after them is stuck for a reason a person must fix (two PRs carry a marker, the repository changed, GitHub keeps failing every open), so the next publish waits for a run's end, a restart or the `publish` action. -- **Outcome.** Each publish runs in the background, one per task, and records its outcome in `publish_outcomes` (schema v10): the outcome kind, or `refused` (a guard, `BranchPushRefused`, `OpeningUnsettled`, `PullRequestMisplaced`, `PullRequestRefused`), `failed` or `stopped` (shutdown), with its message (any token value or token shape removed, as for the push) and the task's state version. The record does not move the task's state version. The same transaction replaces the `publish` action's saved response with this outcome, so resending its action ID reports the outcome, not `publishing`. Before it starts, every publish (after a run, at startup or on the action) records itself as `publishing`, its durable in-flight ownership. If that write fails, the publish does not run. A `publishing` record left at startup is a publish whose process stopped before it recorded an outcome. Startup publishes again if one is owed, and otherwise settles the record as `stopped`. When only an action reply is stuck (its publish never recorded itself as started), the reply reports the outcome on record, and that record is left as it is. A failure while paying owed work does not stop this; the task's PR records show what that publish did. A reply that still says `publishing` belongs to an action whose process stopped (a crash) before its publish recorded an outcome. Any later outcome of the task settles it too, since one publish runs per task. At startup, if no publish is owed, it is settled as `stopped` ("interrupted before it recorded an outcome"), so no replay says `publishing` for ever. The v10 upgrade records every task already `running` or in `needs human` as `not published`, a settled outcome: the first start after the upgrade opens no PR a person did not ask for, and the `publish` action publishes it. `GET /api/runner` shows it as `publish.last`, with `publish.active` and `publish.publishable`. +- **Outcome.** Each publish runs in the background, one per task, and records its outcome in `publish_outcomes` (schema v10): the outcome kind, or `refused` (a guard, `BranchPushRefused`, `OpeningUnsettled`, `PullRequestMisplaced`, `PullRequestRefused`), `failed` or `stopped` (shutdown), with its message (any token value or token shape removed, as for the push) and the task's state version. The record does not move the task's state version. Its state version is the one the publish last saw (`seenVersion`, #114), which every outcome carries. Where a path ends right after a Store write or guarded read, it is the version read there. Where a GitHub call came after the last write (a draft change that failed and left the PR ready, or one refused because drafts are unsupported), it is the version from before that call. So a change someone else made during the call still leaves a publish owed. A refusal, a failure or a stop is stamped with the current version, and stays owed anyway. The same transaction replaces the `publish` action's saved response with this outcome, so resending its action ID reports the outcome, not `publishing`. Before it starts, every publish (after a run, at startup or on the action) records itself as `publishing`, its durable in-flight ownership. If that write fails, the publish does not run. A `publishing` record left at startup is a publish whose process stopped before it recorded an outcome. Startup publishes again if one is owed, and otherwise settles the record as `stopped`. When only an action reply is stuck (its publish never recorded itself as started), the reply reports the outcome on record, and that record is left as it is. A failure while paying owed work does not stop this; the task's PR records show what that publish did. A reply that still says `publishing` belongs to an action whose process stopped (a crash) before its publish recorded an outcome. Any later outcome of the task settles it too, since one publish runs per task. At startup, if no publish is owed, it is settled as `stopped` ("interrupted before it recorded an outcome"), so no replay says `publishing` for ever. The v10 upgrade records every task already `running` or in `needs human` as `not published`, a settled outcome: the first start after the upgrade opens no PR a person did not ask for, and the `publish` action publishes it. `GET /api/runner` shows it as `publish.last`, with `publish.active` and `publish.publishable`. - **Start and resume** refuse while a publish of the task is in progress, so a new item cannot move the head being pushed. A second `publish` is refused too, and the refusal is saved under its action ID. Cancel task is not refused: a draft PR that GitHub opens after a cancel is recorded, and closing it is the later "close the draft on cancel" slice. - **Demo.** A demo never publishes, even with a `github` block or an injected publisher. diff --git a/runner/publish.ts b/runner/publish.ts index f6d61836..a52c54a4 100644 --- a/runner/publish.ts +++ b/runner/publish.ts @@ -22,7 +22,7 @@ export interface PublishConfig { /** An earlier opening's outcome is still unknown; nothing new is opened until it settles. Retry later. */ export class OpeningUnsettled extends Error {} export const DEFAULT_SETTLE_MS = 10 * 60_000; -export type PublishOutcome = +export type PublishOutcome = ( /** `leftReady`: the PR opened or was updated, but could not be made a draft although its task is not in review. */ | { kind: 'opened'; number: number; url: string; draft: boolean; status: string; leftReady?: number } /** `leftReady`: the task's earlier PR could not be made a draft because the repository does not support drafts. */ @@ -38,7 +38,14 @@ export type PublishOutcome = * The task head is its base: there is nothing to open a PR for. A running task moves to needs human. `leftReady`: the * task's earlier PR could not be made a draft because the repository does not support drafts. */ - | { kind: 'no changes'; leftReady?: number }; + | { kind: 'no changes'; leftReady?: number }) & { + /** + * The task state version this publish last saw (#114): where a path ends right after a Store write or guarded read, + * the version read there, with no await since; where a GitHub call came after its last write, the version from before + * that call, since a change someone else made during it was not seen. Required, so every return path decides it. + */ + seenVersion: number; +}; const marker = (openingId: string) => ``; const pullRequestList = (prs: readonly { number: number }[]) => `Pull requests ${prs.map(pr => `#${pr.number}`).join(' and ')}`; @@ -225,7 +232,8 @@ export class PullRequestPublisher { // A cancelled publish still records a draft change that landed (above), but moves no task. signal?.throwIfAborted(); if (!draft) this.#store.transitionTask(identity, stateVersion, 'needs human'); - return leftReady === undefined ? { kind: 'no changes' } : { kind: 'no changes', leftReady }; + const seenVersion = this.#store.getTask(identity).stateVersion; + return leftReady === undefined ? { kind: 'no changes', seenVersion } : { kind: 'no changes', leftReady, seenVersion }; } const own = new Set(prs.filter(pr => pr.number !== null && pr.repository.toLowerCase() === this.#config.repository.toLowerCase()).map(pr => pr.number!)); if (live) own.add(live.number); @@ -250,13 +258,19 @@ export class PullRequestPublisher { const check = this.#store.recordAlreadyFixed(identity, stateVersion, { snapshotId: snapshot.id, reviewVersion, draft, result }); if (drafted && earlier!.state === 'opened') this.#store.recordPullRequestDraft(identity, earlier!.openingId, drafted.number, drafted.draft, { stateVersion: check.stateVersion, reviewVersion: check.reviewVersion }); const ready = leftReady === undefined ? {} : { leftReady }; - if (result.outcome !== 'clear') return draft ? { kind: 'draft skipped', result, ...ready } : { kind: 'possibly already fixed', result, ...ready }; + if (result.outcome !== 'clear') { + // Right after recording the check (and any draft change), with no await since. + const seenVersion = this.#store.getTask(identity).stateVersion; + return draft ? { kind: 'draft skipped', result, ...ready, seenVersion } : { kind: 'possibly already fixed', result, ...ready, seenVersion }; + } if (earlier && live) { // A needs-human task's ready PR becomes a draft first, before anything else about it changes: if the repository // has no drafts, the refusal comes while the PR is still exactly as it was (no push, no new description). if (draft && !live.draft) { + // Read before the call: nothing is written after it on the refusal path (#114). + const seenVersion = this.#store.getTask(identity).stateVersion; try { await this.#pulls.markDraft(live.number, { base: this.#config.baseBranch, headBranch: branch, marker: marker(earlier.openingId) }, signal); } - catch (error) { if (error instanceof DraftsUnsupported) return { kind: 'draft unsupported', number: live.number }; throw error; } + catch (error) { if (error instanceof DraftsUnsupported) return { kind: 'draft unsupported', number: live.number, seenVersion }; throw error; } signal?.throwIfAborted(); } // The push is a refresh's first content write (it moves the open PR's head), so the refresh is recorded before it; @@ -300,7 +314,7 @@ export class PullRequestPublisher { if (error instanceof PullRequestRefused) { this.#store.abandonPullRequestOpening(identity, opening.openingId); throw error; } if (!(error instanceof DraftsUnsupported)) throw error; this.#store.abandonPullRequestOpening(identity, opening.openingId); - return { kind: 'draft unsupported', number: null }; + return { kind: 'draft unsupported', number: null, seenVersion: this.#store.getTask(identity).stateVersion }; } const status = this.#store.recordPullRequestOpened(identity, opening.openingId, pr); return this.#settleHead(identity, opening.openingId, pr, snapshot.head, draft, status, branch, signal); @@ -315,7 +329,10 @@ export class PullRequestPublisher { async #settleHead(identity: PlanIdentity, openingId: string, pr: { number: number; url: string; headSha: string; draft: boolean }, head: string, draft: boolean, status: string, branch: string, signal?: AbortSignal): Promise { const base = this.#config.baseBranch; - const opened = { kind: 'opened' as const, number: pr.number, url: pr.url, draft: pr.draft, status }; + // The version right after the open or refresh was recorded (no await since); on the leftReady path nothing is written + // after the draft call, so a change made during it was not seen (#114). + const seenVersion = this.#store.getTask(identity).stateVersion; + const opened = { kind: 'opened' as const, number: pr.number, url: pr.url, draft: pr.draft, status, seenVersion }; // Left ready only for a ready publish whose task is now in review at the pushed head; a draft publish's PR is always // a draft, whatever GitHub returned. if (pr.draft || (!draft && status === 'in review' && pr.headSha === head)) return opened; @@ -327,7 +344,8 @@ export class PullRequestPublisher { // A fact about the PR, recorded against the versions read right now (no await since). this.#store.recordPullRequestDraft(identity, openingId, drafted.number, drafted.draft, { stateVersion: this.#store.getTask(identity).stateVersion, reviewVersion: this.#store.reviewVersion(identity) }); - return { kind: 'opened', number: drafted.number, url: drafted.url, draft: drafted.draft, status: this.#store.getTask(identity).status }; + const task = this.#store.getTask(identity); + return { kind: 'opened', number: drafted.number, url: drafted.url, draft: drafted.draft, status: task.status, seenVersion: task.stateVersion }; } /** The task may keep a ready PR: it is in review, approved (GitHub merges no draft) or merged. Read with no await since. */ diff --git a/runner/publishing.ts b/runner/publishing.ts index 3d378a63..07d09763 100644 --- a/runner/publishing.ts +++ b/runner/publishing.ts @@ -247,11 +247,12 @@ export class TaskPublishing { // is recorded, startup finds the marker and publishes again or settles it as interrupted. No marker, no publish. try { this.#write(() => this.#store.recordPublish(identity, { outcome: IN_FLIGHT, draft, message: 'A pull request is being published.' })); } catch (error) { console.error(`Could not record the publish as started, so it did not run: ${JSON.stringify(message(error, this.#secrets))}`); return; } - let record: Omit, unsettled = false; + let record: Omit, seenVersion: number | undefined, unsettled = false; try { // The problems are read when the publish starts: the task is in needs human, and its last attempt says why. const outcome = await this.#publisher.publish(identity, draft ? { problems: this.#problems(identity) } : {}); record = describe(outcome, draft); + seenVersion = outcome.seenVersion; } catch (error) { // A guard refusal is mostly a race (the task, its review or its head changed during the publish): one short retry // publishes the new state. One that persists is in mode() too, so the retry finds nothing owed. @@ -259,7 +260,7 @@ export class TaskPublishing { const stopped = error instanceof ShuttingDownError || (this.#closing && (error as Error)?.name === 'AbortError'); record = { outcome: stopped ? 'stopped' : REFUSALS.some(type => error instanceof type) ? 'refused' : 'failed', draft, message: message(error, this.#secrets) }; } - try { this.#write(() => this.#store.recordPublish(identity, record, actionId)); } + try { this.#write(() => this.#store.recordPublish(identity, record, actionId, seenVersion)); } catch (error) { console.error(`Could not record the publish outcome: ${JSON.stringify(message(error, this.#secrets))}`); } this.#retryIfUnsettled(identity, record, unsettled); }; diff --git a/runner/store.ts b/runner/store.ts index 7d7a9935..730fb9f5 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -1326,10 +1326,13 @@ export class Store { * Record a publish's outcome. An observation, not a task change: the task's state version does not move, so a refused * publish leaves the task exactly as it was. Settlement writes go through the shutdown capability. */ - recordPublish(identity: PlanIdentity, record: Omit, actionId?: string): PublishRecord { + recordPublish(identity: PlanIdentity, record: Omit, actionId?: string, seenVersion?: number): PublishRecord { const key = identityKey(identity); return this.#transaction(() => { - const saved: PublishRecord = { ...record, message: record.message.slice(0, 2000), stateVersion: this.#task(key).state_version as number, at: new Date().toISOString() }; + // The version the publish last saw, when it reported one (#114); otherwise the current one, which it ended on. + const current = this.#task(key).state_version as number; + if (seenVersion !== undefined && (!Number.isSafeInteger(seenVersion) || seenVersion > current)) throw new Error('Invalid seen state version.'); + const saved: PublishRecord = { ...record, message: record.message.slice(0, 2000), stateVersion: seenVersion ?? current, at: new Date().toISOString() }; this.#run('INSERT INTO publish_outcomes (plan_key,data) VALUES (?,?) ON CONFLICT(plan_key) DO UPDATE SET data=excluded.data', key, encode(saved)); // The same transaction refreshes the publish action's replay, as for a merge, so a resent click reports this outcome. // A reply still saying `publishing` is an action whose publish never recorded an outcome: its process stopped (a diff --git a/test/publish.test.ts b/test/publish.test.ts index 95268c1d..a7466c87 100644 --- a/test/publish.test.ts +++ b/test/publish.test.ts @@ -164,7 +164,7 @@ describe('opening the task PR', () => { const store = runningTask(); const result: AlreadyFixedResult = { outcome: 'found', baseHead: oid(9), matches: [{ kind: 'pull request', repository: 'owner/repo', number: 401, state: 'OPEN', draft: false }] }; const { publisher, log } = harness(store, { results: [result] }); - expect(await publisher.publish(identity)).toEqual({ kind: 'possibly already fixed', result }); + expect(await publisher.publish(identity)).toEqual({ kind: 'possibly already fixed', result , seenVersion: expect.any(Number) }); expect(log).toEqual(['find ', 'check']); expect(store.getTask(identity).status).toBe('possibly already fixed'); expect(store.taskPullRequests(identity)).toEqual([]); @@ -197,7 +197,7 @@ describe('opening the task PR', () => { it('opens no PR when the task changed nothing, and moves it to needs human', async () => { const store = runningTask({ head: oid(1) }); const { publisher, log } = harness(store); - expect(await publisher.publish(identity)).toEqual({ kind: 'no changes' }); + expect(await publisher.publish(identity)).toEqual({ kind: 'no changes' , seenVersion: expect.any(Number) }); // The branch is looked up first (a foreign PR there would be refused); nothing is checked, pushed or opened. expect(log).toEqual(['find ']); expect(store.getTask(identity).status).toBe('needs human'); @@ -212,7 +212,7 @@ describe('opening the task PR', () => { store.settleAttempt(identity, attempt.id, { firstReason: null, exitCode: 0, valid: true }); store.recordHistory(identity, { revision: 1, snapshotId: store.getSnapshot(identity).id }, oid(1), oid(1), []); const again = harness(store, { live, next }); - expect(await again.publisher.publish(identity)).toEqual({ kind: 'no changes' }); + expect(await again.publisher.publish(identity)).toEqual({ kind: 'no changes' , seenVersion: expect.any(Number) }); expect(again.log).toContain('draft 100'); expect(store.taskPullRequests(identity)).toMatchObject([{ number: 100, draft: true }]); expect(store.getTask(identity).status).toBe('needs human'); @@ -470,7 +470,7 @@ describe('guards found by the independent review', () => { store.markRunning(identity, attempt.id); store.settleAttempt(identity, attempt.id, { firstReason: null, exitCode: 0, valid: true }); store.recordHistory(identity, { revision: 1, snapshotId: store.getSnapshot(identity).id }, oid(1), oid(1), []); - expect(await harness(store, { live, next, draftsUnsupported: true }).publisher.publish(identity)).toEqual({ kind: 'no changes', leftReady: 100 }); + expect(await harness(store, { live, next, draftsUnsupported: true }).publisher.publish(identity)).toEqual({ kind: 'no changes', leftReady: 100 , seenVersion: expect.any(Number) }); expect(store.getTask(identity).status).toBe('needs human'); }); it('refuses a PR-number mismatch before the no-changes draft change', async () => { @@ -507,12 +507,35 @@ describe('guards found by the independent review', () => { await expect(harness(store, { live, next, copies: [copy], closed: new Set([pr.number]) }).publisher.publish(identity)).rejects.toThrow(/cancelled/); expect(store.taskPullRequests(identity)).toMatchObject([{ number: 100, draft: true, headSha: oid(2), refresh: null }]); }); + it('reports the version it saw when the last GitHub call came after its last Store write (#114)', async () => { + // leftReady: GitHub shows another head than the one pushed, and making the PR a draft fails; a change made during + // that call was not seen by this publish. + const store = runningTask(); + let before = -1; + const left = harness(store, { open: async () => ({ number: 100, url: 'https://github.com/owner/repo/pull/100', headSha: oid(7), draft: false }), draftFails: true, + onDraft: () => { before = store.getTask(identity).stateVersion; store.cancelTask(identity, before, crypto.randomUUID()); } }); + expect(await left.publisher.publish(identity)).toMatchObject({ kind: 'opened', leftReady: 100, seenVersion: before }); + expect(store.getTask(identity).stateVersion).toBeGreaterThan(before); + // draft unsupported on the main path: the task's ready PR cannot become a draft, and nothing is written after. + const other = runningTask(), live = new Map(), next = { value: 100 }; + await harness(other, { live, next }).publisher.publish(identity); + rerun(other); + other.transitionTask(identity, other.getTask(identity).stateVersion, 'needs human'); + // The draft step for a task that cannot keep a ready PR tries first; the main path's own draft change is the second. + let seen = -1, drafts = 0; + const again = harness(other, { live, next, draftsUnsupported: true, onDraft: () => { + if (++drafts !== 2) return; + seen = other.getTask(identity).stateVersion; other.cancelTask(identity, seen, crypto.randomUUID()); + } }); + expect(await again.publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: 100, seenVersion: seen }); + expect(other.getTask(identity).stateVersion).toBeGreaterThan(seen); + }); it('reports drafts unsupported when a lost draft opening cannot be turned back into a draft', async () => { const store = runningTask(), live = new Map(), next = { value: 100 }; store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs human'); await expect(harness(store, { live, next, openTimesOut: true }).publisher.publish(identity, { problems: ['x'] })).rejects.toThrow('timeout'); for (const [m, pr] of live) live.set(m, { ...pr, draft: false }); - expect(await harness(store, { live, next, draftsUnsupported: true }).publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: 100 }); + expect(await harness(store, { live, next, draftsUnsupported: true }).publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: 100, seenVersion: expect.any(Number) }); expect(store.taskPullRequests(identity)).toMatchObject([{ state: 'opened', number: 100 }]); }); it('refuses to recover an opening recorded for another repository', async () => { @@ -775,11 +798,11 @@ describe('a repository without draft PRs', () => { const store = runningTask(); store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs human'); const { publisher } = harness(store, { draftsUnsupported: true }); - expect(await publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: null }); + expect(await publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: null , seenVersion: expect.any(Number) }); expect(store.taskPullRequests(identity).map(pr => pr.state)).toEqual(['abandoned']); expect(store.getTask(identity).status).toBe('needs human'); // Nothing is left in flight: the next publish does not wait for a settle time. - expect(await harness(store, { draftsUnsupported: true }).publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: null }); + expect(await harness(store, { draftsUnsupported: true }).publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: null , seenVersion: expect.any(Number) }); }); it('leaves the existing ready PR exactly as it is: the draft refusal comes before any push or description change', async () => { const store = runningTask(), live = new Map(), next = { value: 100 }; @@ -787,7 +810,7 @@ describe('a repository without draft PRs', () => { rerun(store); store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs human'); const again = harness(store, { live, next, draftsUnsupported: true }); - expect(await again.publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: 100 }); + expect(await again.publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: 100, seenVersion: expect.any(Number) }); // The draft step comes first, so the refusal leaves the PR exactly as it was: no push, no new description. expect(again.log.some(line => line.startsWith('push') || line.startsWith('refresh'))).toBe(false); expect(store.taskPullRequests(identity)).toMatchObject([{ number: 100, refresh: null, headSha: oid(2) }]); @@ -1615,7 +1638,7 @@ describe('shutdown and PRs left ready', () => { const store = runningTask({ head: oid(1) }); store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs human'); const version = store.getTask(identity).stateVersion; - expect(await harness(store).publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'no changes' }); + expect(await harness(store).publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'no changes' , seenVersion: expect.any(Number) }); expect(store.getTask(identity)).toMatchObject({ status: 'needs human', stateVersion: version }); }); it('moves no task on the no-changes path when the publish is aborted during a refused draft change', async () => { @@ -1824,7 +1847,7 @@ describe('shutdown and PRs left ready', () => { // The task changed since the opening (same status, new version), so the opening is not this publish's own. store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs human'); const again = harness(store, { live, next, draftsUnsupported: true }); - expect(await again.publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: 100 }); + expect(await again.publisher.publish(identity, { problems: ['x'] })).toEqual({ kind: 'draft unsupported', number: 100, seenVersion: expect.any(Number) }); expect(again.log).toContain('check'); }); it('converges after a base change once the person retargets the PR, even when the update is then lost', async () => { diff --git a/test/runner-lifecycle-store.test.ts b/test/runner-lifecycle-store.test.ts index c7669cb0..58f3d9f9 100644 --- a/test/runner-lifecycle-store.test.ts +++ b/test/runner-lifecycle-store.test.ts @@ -704,6 +704,12 @@ describe('publish outcomes (#103)', () => { expect(reopened.getTask(identity)).toEqual(before); expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 10 }); }); + it('stamps an outcome with the version the publish saw, and refuses one it cannot have seen (#114)', () => { + const { store } = queued(), version = store.getTask(identity).stateVersion; + expect(store.recordPublish(identity, { outcome: 'opened', draft: false, message: 'x' }, undefined, version)).toMatchObject({ stateVersion: version }); + expect(() => store.recordPublish(identity, { outcome: 'opened', draft: false, message: 'x' }, undefined, store.getTask(identity).stateVersion + 1)).toThrow('Invalid seen state version.'); + expect(store.lastPublish(identity)).toMatchObject({ stateVersion: version }); + }); it('records a task already running at the upgrade as not published, so the first start publishes nothing for it', () => { const { path, store } = queued(); admit(store); expect(store.getTask(identity).status).toBe('running'); diff --git a/test/runner-publishing.test.ts b/test/runner-publishing.test.ts index aac2b77a..4cac8fbf 100644 --- a/test/runner-publishing.test.ts +++ b/test/runner-publishing.test.ts @@ -40,6 +40,8 @@ class FakeGitHub { prs: Pr[] = []; calls: string[] = []; onOpen?: (signal?: AbortSignal) => Promise; + /** Runs inside a draft change, before it applies; a throw fails the change. */ + onDraft?: () => void; /** PRs GitHub's list does not show yet (it lags behind them). */ hidden = new Set(); readonly remote: string; @@ -75,7 +77,7 @@ class FakeGitHub { findOwned: async input => this.prs.filter(pr => pr.open && !this.hidden.has(pr.number) && pr.headBranch === input.headBranch && input.markers.includes(pr.marker)) .map(pr => ({ ...this.#view(pr), marker: pr.marker, base: pr.base })), readPull: async number => { const pr = this.prs.find(candidate => candidate.number === number)!; return { open: pr.open, headBranch: pr.headBranch, base: pr.base, marker: pr.marker }; }, - markDraft: async number => { const pr = this.prs.find(candidate => candidate.number === number)!; pr.draft = true; return this.#view(pr); }, + markDraft: async number => { this.onDraft?.(); const pr = this.prs.find(candidate => candidate.number === number)!; pr.draft = true; return this.#view(pr); }, refresh: async (number, input) => { this.calls.push(`refresh ${input.draft ? 'draft' : 'ready'}`); const pr = this.prs.find(candidate => candidate.number === number)!; @@ -417,6 +419,23 @@ describe('publishing a finished task (#103)', () => { expect(store.lastPublish(identity)).toMatchObject({ outcome: 'failed', message: 'gh failed: Authorization: token [token]' }); }); + it('stamps the outcome with the version the publish saw, not one changed during its last GitHub call (#114)', async () => { + const w = world(); + let seen = -1; + // GitHub shows another head than the one pushed, so the PR is to be made a draft; that call fails, and someone else + // changes the task while it runs. + w.github.onOpen = async () => { const pr = w.github.prs.at(-1)!; git(w.remote, 'update-ref', `refs/heads/${pr.headBranch}`, git(w.demo.repository, 'rev-list', '--max-parents=0', 'HEAD')); }; + const { app, identity, store } = await serve(w, { before: completeAll }); + w.github.onDraft = () => { + seen = store.getTask(identity).stateVersion; + store.cancelTask(identity, seen, randomUUID()); + throw new Error('GitHub timed out.'); + }; + await publishSettled(app, identity); + expect(store.lastPublish(identity)).toMatchObject({ outcome: 'opened', stateVersion: seen, message: expect.stringContaining('may still be ready for review') }); + expect(store.getTask(identity).stateVersion).toBeGreaterThan(seen); + }); + describe('a publish\'s in-flight record', () => { it('is written before any GitHub call', async () => { const w = world();