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)
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 decision = args.decision ?? null;
|
||||||
const result = this.#checkAuthority(s, 'git.push.remote', { decision, target: task_ref });
|
const result = this.#checkAuthority(s, 'git.push.remote', { decision, target: task_ref });
|
||||||
const facts = this.#publishFacts(s, task_ref, project, result);
|
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 ticket = randomUUID();
|
||||||
const dst = `refs/heads/${facts.user}/${branch}`;
|
const dst = `refs/heads/${facts.user}/${branch}`;
|
||||||
this.#publishes.set(ticket, { s, task_ref, project, decision, dst, ...facts });
|
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/);
|
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) => {
|
test('publishRefused files the code on the task\'s trail and drops the ticket', (t) => {
|
||||||
const f = setup(t);
|
const f = setup(t);
|
||||||
f.push(A);
|
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")));
|
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) => {
|
test("sage-1440: the publishResult poll is bounded: publish-timeout, filed once, and nothing recorded", async (t) => {
|
||||||
const w = await setup(t, { publishDeadlineMs: 150 });
|
const w = await setup(t, { publishDeadlineMs: 150 });
|
||||||
let polls = 0;
|
let polls = 0;
|
||||||
|
|||||||
Reference in New Issue
Block a user