bus: one decision, one open publish; checkPublish refuses publish-busy while another ticket holds the decision (row 237, #1743)
The decision is spent only at recordPublish, after the push, so two checkPublish calls on one decision could both push and land the commit on two fork refs. checkPublish now refuses publish-busy while an open ticket holds the same business and decision. A refusal drops the ticket and leaves the decision unspent. Broker test and a socket race test with exactly one git-receive-pack. Residuals 1-3 in BUILD.md; residual 3 (checkLand) is a follow-up row. Built by Megavolt (megavolt/row237e42415935, duckworth-0068 N1), reviewed by Duckworth (approve, comment 29810, manifest bb777cb0). Sage's gate on816d48b03plus these paths: M1 killed, every package suite and every scripts/test-*.sh green.
This commit is contained in:
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user