D: show a push left running by a crash is harmless; document it (#112)

docs/implementation/pull-request-opening.md

@@ -136,9 +136,21 @@ Some repositories do not support draft PRs (for example private repositories on
 - **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.
 
+## A push left running by a crash (#112)
+
+A normal shutdown aborts a publish and awaits its `git push`. If codeboost crashes or is killed instead, the push can keep running, and nothing records its process group, unlike a preparation's. That process is harmless, because its lease (`pushArguments`) pins the exact branch value its own run read. So it can only land while the branch holds that value.
+
+- **It lands before a newer publish reads the branch.** The newer publish reads the leftover's head. That head is a commit the ledger records as codeboost's, so the publish moves the branch on from it.
+- **It lands between the newer publish's read and its push.** The newer push's lease no longer matches, so it is refused (`BranchPushRefused`, "moved while codeboost pushed it"). Nothing is overwritten. The publish is recorded as `refused` and stays owed; the next publish reads the leftover's head and moves on.
+- **It lands after the newer publish moved the branch.** Its own lease no longer matches, so the remote refuses it. The newer head stays.
+- **The branch has moved back to the value it read.** The lease compares the value, not its history. So if the branch returns to that exact commit, because a person force-pushes it back or a later publish pushes that commit again, the leftover lands and moves the branch to its own head, a codeboost commit. An open PR then shows that head, which is not the one recorded, until the next publish pushes the task head again and refreshes the PR. Nothing starts that publish by itself. The branch must hold exactly the commit the crashed run read, and the leftover can only put back a commit codeboost made.
+- **The branch has been deleted, and the run read none.** A run that read no branch pushes with an empty lease. So if a newer publish created the branch and it was deleted since (by a person, or by GitHub after the PR merged), the leftover recreates it at its own head, a codeboost commit. No PR is opened for it, and nothing in codeboost removes it, so a stray `codeboost/…` branch can be left on GitHub until a person deletes it. It overwrites nothing.
+
+The process group is therefore not recorded, and startup does not look for it (decision on #112). The `gh` calls that open, update, draft or close a PR are not covered by this argument. Their effects are recovered by their markers instead: a lost opening, an update in flight, and a close's read by number.
+
 ## Tests
 
-`test/runner-publishing.test.ts` covers the server wiring (#103) with a local bare repository as the remote and GitHub faked: a completed run opens one ready PR with the branch at the task head, a needs-human task gets a draft with its problems, a branch holding someone else's commit is refused and the `publish` action retries it (and its replay reports the outcome), a refused resume that moves the task to needs human publishes a draft while one on a task already there publishes nothing, start, resume and a second publish are refused while a publish runs, shutdown during the push or during the opening closes the Store only after the publish settled, a PR whose reply arrives just after the abort is still recorded before the write gate closes, and the next start publishes once or recovers the opening by its marker. Removing the startup publish, the after-run publish, the publish after a refused resume, the status-change check, the busy guard, the replay update or the shutdown wait, or moving that wait after the write gate, each fails a test. `test/already-fixed.test.ts` and `test/publish.test.ts` cover each row above. Each guard was also broken on purpose, and a test failed each time: own-PR and own-commit exclusion, repository-qualified exclusion, every bound, the ancestor check, the fail-closed catch, the issue-number boundary, the state-version and head re-read, the status check before the no-changes shortcut, the version-owned status change, the open-state check, the settle time, the branch hash, the environment allowlist, reuse of the earlier PR, lost-opening recovery, adoption on the main path and counting that PR as the task's own, drafting a PR that a task which cannot publish still has ready, adopting an abandoned opening's PR that appears after the task stopped, the abort checks on the no-changes path and in that draft step (a lookup that fails, one that answers, a draft change that fails and one that lands), recording a late PR by adoption alone, an abort during the head-mismatch draft change, the `closing` checks before a push and before a ready change, `close()` aborting a publish in progress, the repository check, the definite 422 refusal, a retargeted own PR and another base's PR from the branch, refusals matched on stderr and GitHub's `errors[].message` only, the configured base in recovery and the head settle, the draft step finding the task's PR in any base, closing-only cross-references, both sides of a manual link, the query text, the fence length, and the description bounds.
+`test/runner-publishing.test.ts` covers the server wiring (#103) with a local bare repository as the remote and GitHub faked: a completed run opens one ready PR with the branch at the task head, a needs-human task gets a draft with its problems, a branch holding someone else's commit is refused and the `publish` action retries it (and its replay reports the outcome), a refused resume that moves the task to needs human publishes a draft while one on a task already there publishes nothing, start, resume and a second publish are refused while a publish runs, shutdown during the push or during the opening closes the Store only after the publish settled, a PR whose reply arrives just after the abort is still recorded before the write gate closes, and the next start publishes once or recovers the opening by its marker. Removing the startup publish, the after-run publish, the publish after a refused resume, the status-change check, the busy guard, the replay update or the shutdown wait, or moving that wait after the write gate, each fails a test. `test/runner-branch-push.test.ts` replays a leftover push, with the pusher's own arguments against a real bare remote, before, between and after a newer publish's read and push (#112); dropping the lease fails the last two. `test/already-fixed.test.ts` and `test/publish.test.ts` cover each row above. Each guard was also broken on purpose, and a test failed each time: own-PR and own-commit exclusion, repository-qualified exclusion, every bound, the ancestor check, the fail-closed catch, the issue-number boundary, the state-version and head re-read, the status check before the no-changes shortcut, the version-owned status change, the open-state check, the settle time, the branch hash, the environment allowlist, reuse of the earlier PR, lost-opening recovery, adoption on the main path and counting that PR as the task's own, drafting a PR that a task which cannot publish still has ready, adopting an abandoned opening's PR that appears after the task stopped, the abort checks on the no-changes path and in that draft step (a lookup that fails, one that answers, a draft change that fails and one that lands), recording a late PR by adoption alone, an abort during the head-mismatch draft change, the `closing` checks before a push and before a ready change, `close()` aborting a publish in progress, the repository check, the definite 422 refusal, a retargeted own PR and another base's PR from the branch, refusals matched on stderr and GitHub's `errors[].message` only, the configured base in recovery and the head settle, the draft step finding the task's PR in any base, closing-only cross-references, both sides of a manual link, the query text, the fence length, and the description bounds.
 
 ## Test this document with a reader
 

runner/branch-push.ts

@@ -57,6 +57,19 @@ export function pushUrl(remote: string, env: NodeJS.ProcessEnv = process.env): s
   return `https://${host}/${remote}.git`;
 }
 
+/**
+ * The push's arguments. The lease pins the exact value read (empty: the branch must not exist), so the remote refuses the
+ * push if anyone moved the branch since, including a newer publish. That is what makes a push left running by a crashed
+ * codeboost harmless (#112): it can land only while the branch holds the value its own run read. The lease compares the
+ * value, not its history, so a branch that returns to that value (moved back by a person, or the same commit pushed again) lets it land (a codeboost
+ * commit; the next publish pushes the task head again). An empty lease matches a branch deleted since, so a run that
+ * read none can recreate a deleted branch at its own head: a stray branch with no PR, overwriting nothing. Only this one ref is sent: no tags, no hooks, no submodules.
+ */
+export function pushArguments(input: { url: string; ref: string; read: string | null; head: string }): string[] {
+  return ['push', '--porcelain', '--no-verify', '--no-follow-tags', '--recurse-submodules=no',
+    `--force-with-lease=${input.ref}:${input.read ?? ''}`, '--', input.url, `${input.head}:${input.ref}`];
+}
+
 /** Remove every configured token value and every known token shape. */
 export function redact(text: string, secrets: readonly string[]): string {
   // Longest first, so a token that contains another is removed whole. A value under 8 characters is not a real token,
@@ -110,11 +123,9 @@ export class GitBranchPusher implements BranchPusher {
     if (remote !== null && !owned.has(remote))
       throw new BranchPushRefused(`The branch ${input.branch} holds commit ${remote}, which codeboost did not make. Nothing was pushed.`);
     signal?.throwIfAborted();
-    // The lease pins the exact value read above (empty: the branch must not exist), so GitHub refuses the push if anyone
-    // moved the branch since. Only this one ref is sent: no tags, no hooks, no submodules.
+    // Leased to the value read above (pushArguments).
     try {
-      await this.#git(['push', '--porcelain', '--no-verify', '--no-follow-tags', '--recurse-submodules=no',
-        `--force-with-lease=${ref}:${remote ?? ''}`, '--', this.#url, `${input.head}:${ref}`], signal, true);
+      await this.#git(pushArguments({ url: this.#url, ref, read: remote, head: input.head }), signal, true);
     } catch (error) {
       if ((error as { staleLease?: boolean }).staleLease)
         throw new BranchPushRefused(`The branch ${input.branch} moved while codeboost pushed it. Nothing was pushed.`);

test/runner-branch-push.test.ts

@@ -7,7 +7,7 @@ import { afterEach, describe, expect, it } from 'vitest';
 import { fixtureGit as git } from './fixtures/git.ts';
 import type { PlanIdentity } from '../core/identity.ts';
 import { GH_ENV_ALLOWLIST } from '../github/gh-env.ts';
-import { BranchPushRefused, CREDENTIAL_HELPER, GitBranchPusher, gitFailure, pushEnvironment, pushUrl, redact } from '../runner/branch-push.ts';
+import { BranchPushRefused, CREDENTIAL_HELPER, GitBranchPusher, gitFailure, pushArguments, pushEnvironment, pushUrl, redact } from '../runner/branch-push.ts';
 import { ensureCommit, fetchTaskCommit, openRunnerRepository, type RunnerRepository } from '../runner/runner-repository.ts';
 
 const OWNER = '0123456789abcdef0123456789abcdef';
@@ -109,6 +109,60 @@ describe('GitBranchPusher', () => {
     expect(remoteRefs(s)).toBe(`${s.base} ${REF}`);
   });
 
+  describe('a push left running by a crashed codeboost (#112)', () => {
+    /**
+     * What such a process does: the pusher's own push arguments, from the runner repository, leased to the value that
+     * run read. The process outlives its codeboost, so it lands at any point of a later publish.
+     */
+    const leftover = (s: Setup, head: string, read: string | null) => {
+      try { git(s.repository.path, '-c', 'protocol.file.allow=always', ...pushArguments({ url: s.remote, ref: REF, read, head })); return 'landed'; }
+      catch (error) {
+        // Only the lease's own refusal counts; any other failure would prove nothing about the lease.
+        const output = `${(error as { stdout?: string }).stdout ?? ''}${(error as { stderr?: string }).stderr ?? ''}`;
+        if (/\[rejected\][^\n]*\(stale info\)/.test(output)) return 'refused';
+        throw error;
+      }
+    };
+    it('is refused by its lease when it lands after a newer publish, so it never overwrites the newer head', async () => {
+      const s = await setup(), old = await runnerCommit(s, 'old'), newer = await runnerCommit(s, 'newer');
+      // The crashed run read no branch; a restart's publish created it.
+      await pusher(s, { owned: () => [old, newer] }).instance.push(IDENTITY, { head: newer, branch: BRANCH });
+      expect(leftover(s, old, null)).toBe('refused');
+      expect(remoteRefs(s)).toBe(`${newer} ${REF}`);
+      // The crashed run read an earlier commit of its own; the restart's publish moved the branch on from it.
+      const t = await setup(), a = await runnerCommit(t, 'first'), b = await runnerCommit(t, 'second'), c = await runnerCommit(t, 'third');
+      await pusher(t).instance.push(IDENTITY, { head: a, branch: BRANCH });
+      await pusher(t, { owned: () => [a, b, c] }).instance.push(IDENTITY, { head: c, branch: BRANCH });
+      expect(leftover(t, b, a)).toBe('refused');
+      expect(remoteRefs(t)).toBe(`${c} ${REF}`);
+    });
+    it('makes a newer publish refuse when it lands between that publish\'s read and its push, and the next publish moves on', async () => {
+      const s = await setup(), old = await runnerCommit(s, 'old'), newer = await runnerCommit(s, 'newer');
+      // It lands while the newer publish reads the ledger: after the newer publish's read, before its push.
+      const between = pusher(s, { owned: () => { expect(leftover(s, old, null)).toBe('landed'); return [old, newer]; } });
+      await expect(between.instance.push(IDENTITY, { head: newer, branch: BRANCH })).rejects.toThrow(`The branch ${BRANCH} moved while codeboost pushed it`);
+      expect(remoteRefs(s)).toBe(`${old} ${REF}`);
+      // The next publish reads the leftover's head, a commit the ledger records as codeboost's, and moves the branch on.
+      await pusher(s, { owned: () => [old, newer] }).instance.push(IDENTITY, { head: newer, branch: BRANCH });
+      expect(remoteRefs(s)).toBe(`${newer} ${REF}`);
+    });
+    it('recreates the branch when the run read none and the branch was deleted since a newer publish created it', async () => {
+      const s = await setup(), old = await runnerCommit(s, 'old'), newer = await runnerCommit(s, 'newer');
+      // The crashed run read no branch; a restart's publish created it; then it was deleted (by a person, or by GitHub
+      // after the PR merged). The empty lease matches again, so the leftover lands.
+      await pusher(s, { owned: () => [old, newer] }).instance.push(IDENTITY, { head: newer, branch: BRANCH });
+      git(s.remote, 'update-ref', '-d', REF);
+      expect(leftover(s, old, null)).toBe('landed');
+      expect(remoteRefs(s)).toBe(`${old} ${REF}`);
+    });
+    it('is simply read as codeboost\'s commit when it lands before a newer publish reads the branch', async () => {
+      const s = await setup(), old = await runnerCommit(s, 'old'), newer = await runnerCommit(s, 'newer');
+      expect(leftover(s, old, null)).toBe('landed');
+      await pusher(s, { owned: () => [old, newer] }).instance.push(IDENTITY, { head: newer, branch: BRANCH });
+      expect(remoteRefs(s)).toBe(`${newer} ${REF}`);
+    });
+  });
+
   it('reads only the exact branch, not a longer ref that ends with its name', async () => {
     const s = await setup(), head = await runnerCommit(s, 'one');
     personPushes(s, `refs/heads/other/${REF}`);