bus: row 237 one decision, one open publish; checkPublish refuses publish-busy while another ticket holds the decision, since it is spent only at recordPublish after the push; a broker test and a socket race test with exactly one receive-pack; BUILD.md with the option chosen, M1 and three residuals (duckworth-0068 N1, #1743)

2026-10-20 19:31:48 -05:00
parent cb5fdb064f
commit e424159350
4 changed files with 123 additions and 0 deletions
@@ -0,0 +1,75 @@
+# Row 237: one decision publishes once (#1743)
+
+Megavolt, 2026-10-21 (from `date -u`). Brief: `docs/plans/2026-10-20_agent-credentials.md`
+§ "row 199 follow-up N1", blob dca67d310e1b. Source: Duckworth's row 199 round 1 note N1
+(duckworth-0068). Base: `cb5fdb064` (row 199's landing). Reviewer Duckworth, gate owner Sage.
+
+## The defect
+
+`checkPublish` opens a ticket after checking the decision is unspent. The decision is spent only at
+`recordPublish`, which runs after the bus process has pushed. Two `checkPublish` calls on one decision
+therefore both get tickets, and both push. With different branches, the commit lands on two fork refs:
+`refs/heads/<user>/feat` and `refs/heads/<user>/other`. The second `recordPublish` then refuses
+`decision-consumed`, but by then its push has already reached the fork.
+
+## The fix, and why this option
+
+`checkPublish` refuses `publish-busy` when an open ticket in `#publishes` holds the same business and
+decision (`packages/bus/src/broker.mjs:1038-1040`). It runs after the authority and fact checks, so a
+spent decision still refuses `decision-consumed` first.
+
+The brief's other option, rechecking `#spent` in `publishTarget`, doesn't close the race. Both tickets
+reach `publishTarget` before either push ends, and nothing is spent until `recordPublish`. So both
+rechecks pass and both pushes run. The only state that exists while the first push runs is the open
+ticket, so the check reads that.
+
+A refusal (`publishRefused`) drops the ticket and leaves the decision unspent, so a retry on the same
+decision gets a new ticket. That is row 199's behaviour, unchanged. A within-role publish has no
+decision and isn't affected.
+
+## Tests
+
+- `packages/bus/tests/publish.test.mjs:186`. A second ticket refuses `publish-busy`, on the same branch
+  and on another. After `publishRefused` the retry gets a ticket and a third refuses busy. After
+  `recordPublish` there is one `action.allowed` for git.push.remote and the next call refuses
+  `decision-consumed`. Another agent's decision on the same task isn't held.
+- `packages/cli/tests/publish-socket.test.mjs:269`. End to end through the git socket, the bus runtime
+  and the TLS fake Gitea. While the first publish holds its ticket, a second publish on the same
+  decision to branch `other` gets `publish-busy`. The fake Gitea gets exactly one `git-receive-pack`,
+  the fork has only `refs/heads/coder/feat`, and there is one `action.allowed`.
+
+Runs on this branch, single files:
+- publish.test.mjs: 13/13.
+- publish-socket.test.mjs: 8/8.
+
+## Mutant
+
+M1 removes the `publish-busy` line. Both tests fail:
+- The bus test fails at the first busy assert.
+- The cli test fails because the second publish answers `ok: true` with `ref: refs/heads/coder/other`.
+  That is the defect: two refs on the fork.
+
+Logs are in `/mnt/storage/scratch/megavolt-r237/` (m1-bus.log, m1b-cli.log). `cmp` confirmed
+broker.mjs matched the saved copy after the restore. The first cli mutant run (m1-cli.log) was against
+a hook that recursed on every checkPublish. I fixed the hook to fire only for branch `feat` and reran
+it (m1b-cli.log).
+
+## Residuals, not fixed here
+
+1. **A ticket that never ends holds its decision until the bus restarts.** `#publishes` is in memory.
+   That happens when the host dies between checkPublish and recordPublish or publishRefused. It fails
+   closed (`publish-busy`), and a restart clears it.
+2. **publish-timeout (sage-1440).** The socket files `publishRefused` and drops the ticket, but the bus
+   process's push may still be running. A retry on the same decision can then open a ticket while that
+   push is live. This is the same residual the socket already logs ("the bus process's push may still
+   reach the fork"). Closing it means the runtime's `remotes` must keep the decision busy until its push
+   settles, which reaches into runtime.mjs. That is beyond the brief's two options.
+3. **checkLand has the same shape.** `#landings` takes a ticket per call and the decision is spent at
+   `recordLand`. Whether two landings on one decision can both reach main isn't checked here. That's a
+   follow-up for Sage to row or dismiss.
+
+## Gate
+
+Per the brief: Duckworth approves the manifest, the bus suite and every `scripts/test-*.sh` are green
+on Sage's gate, and M1 is killed. This row also changes a cli test file, so the cli suite (or that file
+alone) should run on the gate too.
@@ -1035,6 +1035,9 @@ export class Broker {
      const decision = args.decision ?? null;
      const result = this.#checkAuthority(s, 'git.push.remote', { decision, target: task_ref });
      const facts = this.#publishFacts(s, task_ref, project, result);
+      // Row 237 (#1743): one decision, one open publish. The decision is spent only at
+      // recordPublish, after the push, so a second ticket on it would push too, maybe to another ref.
+      if (result.decision && [...this.#publishes.values()].some((p) => p.s.business === s.business && p.decision === result.decision)) fail('publish-busy');
      const ticket = randomUUID();
      const dst = `refs/heads/${facts.user}/${branch}`;
      this.#publishes.set(ticket, { s, task_ref, project, decision, dst, ...facts });
@@ -181,6 +181,32 @@ test('the request cannot name the fork, the user or the destination', (t) => {
    assert.throws(() => f.publish(d, 'coder', extra), /invalid-request/);
 });

+// Row 237 (#1743, duckworth-0068 N1): the decision is spent at recordPublish, after the push, so
+// checkPublish refuses a second ticket on a decision that already has one open.
+test('one decision, one open publish: a second ticket refuses publish-busy, whatever its branch', (t) => {
+  const f = setup(t);
+  f.push(A);
+  const d = f.approve();
+  const r = f.publish(d);
+  assert.throws(() => f.publish(d), /publish-busy/);
+  assert.throws(() => f.publish(d, 'coder', { branch: 'other' }), /publish-busy/);
+  // A refused publish frees the decision, which is still unspent; the retry gets a ticket.
+  f.b.publishRefused(r.ticket, { code: 'fork-unavailable' });
+  const retry = f.publish(d, 'coder', { branch: 'other' });
+  assert.throws(() => f.publish(d), /publish-busy/);
+  f.b.recordPublish(retry.ticket, { ref: retry.dst, commit: A, previous: null });
+  assert.equal(f.events('action.allowed').filter((e) => e.body.action === 'git.push.remote' && e.body.operation !== 'decision.raise').length, 1);
+  assert.throws(() => f.publish(d), /decision-consumed/);
+  // Another agent's decision on the same task isn't held by this one's ticket.
+  const f2 = setup(t);
+  f2.push(A);
+  const mine = f2.approve();
+  f2.publish(mine);
+  f2.push(B, A, 'second');
+  const theirs = f2.approve('second');
+  assert.equal(typeof f2.publish(theirs, 'second').ticket, 'string');
+});
+
 test('publishRefused files the code on the task\'s trail and drops the ticket', (t) => {
  const f = setup(t);
  f.push(A);
@@ -266,6 +266,25 @@ test("T3: a push recorded while the bus process pushes refuses decision-mismatch
  assert.ok(w.logs.some((l) => l.includes("wasn't recorded")));
 });

+test("row 237: two publishes race on one decision; the second refuses publish-busy and exactly one push reaches the fork", async (t) => {
+  const w = await setup(t);
+  w.bus.credentials.issue("demo/coder", "gitea", TOKEN, "2099-01-01");
+  const d = w.approve();
+  // The second publish, to another branch, runs while the first holds its ticket.
+  let second;
+  w.hooks.checkPublish = async (m, next) => {
+    const facts = await next();
+    if (m.args.branch === "feat") second = await w.publish(d, { branch: "other" });
+    return facts;
+  };
+  const first = await w.publish(d);
+  assert.deepEqual(second, { ok: false, error: "publish-busy" });
+  assert.equal(first.ok, true);
+  assert.equal(w.gitea.requests.filter((r) => r.path.endsWith("/git-receive-pack")).length, 1);
+  assert.equal(git(w.root, `--git-dir=${w.forkRepo}`, "for-each-ref", "--format=%(refname)", "refs/heads/coder/"), "refs/heads/coder/feat");
+  assert.equal(w.events("action.allowed").filter((e) => e.body.action === "git.push.remote" && e.body.operation !== "decision.raise").length, 1);
+});
+
 test("sage-1440: the publishResult poll is bounded: publish-timeout, filed once, and nothing recorded", async (t) => {
  const w = await setup(t, { publishDeadlineMs: 150 });
  let polls = 0;