diff --git a/agents/megavolt/work/queue-237/BUILD.md b/agents/megavolt/work/queue-237/BUILD.md new file mode 100644 index 000000000..48adfa887 --- /dev/null +++ b/agents/megavolt/work/queue-237/BUILD.md @@ -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//feat` and `refs/heads//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. diff --git a/packages/bus/src/broker.mjs b/packages/bus/src/broker.mjs index 52fa91775..06f168417 100644 --- a/packages/bus/src/broker.mjs +++ b/packages/bus/src/broker.mjs @@ -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 }); diff --git a/packages/bus/tests/publish.test.mjs b/packages/bus/tests/publish.test.mjs index 97fa517c2..30a227310 100644 --- a/packages/bus/tests/publish.test.mjs +++ b/packages/bus/tests/publish.test.mjs @@ -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); diff --git a/packages/cli/tests/publish-socket.test.mjs b/packages/cli/tests/publish-socket.test.mjs index 4a905b303..b9b53158a 100644 --- a/packages/cli/tests/publish-socket.test.mjs +++ b/packages/cli/tests/publish-socket.test.mjs @@ -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;