From d3ebf99c7bbeb14641ee4ef74fb6b14cb318ba12 Mon Sep 17 00:00:00 2001 From: Taras Mankovski Date: Mon, 19 Oct 2026 09:50:54 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9C=85=20Make=20P21=20wait=20for=20cancellat?= =?UTF-8?q?ion=20before=20delivering=20authentication=20(#950)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P21 spawned a halter and released the held credential delivery immediately afterwards, so nothing proved that cancellation had reached the pending authentication before the resolver was released. The acquisition could resume, open and be torn down in either order, which is the scheduling the preserved CI failure is consistent with. The injected authentication resource now registers its cleanup before it announces the pending acquisition. That cleanup acknowledges it started, waits for the same owed delivery and marks the delivered session released. The row observes `task.halt()`, waits for that acknowledgement, and asserts the session is unopened, unreleased and the halt still pending before this row — not the cancelled consumer — delivers it. The delivery sits in a yield-free `finally`, so an assertion that failed cannot strand the joined cleanup. Every real observation is unchanged: no counted push, an absent `review/credential` ref, both sessions for the admitted origin, one acknowledged request with no fabricated outcome, and a continuation publishing that request's exact source commit. Co-Authored-By: Claude Opus 5 --- .../tests/retained-repository-push.test.ts | 71 +++++++++++++++---- 1 file changed, 57 insertions(+), 14 deletions(-) diff --git a/packages/git/tests/retained-repository-push.test.ts b/packages/git/tests/retained-repository-push.test.ts index 22a4111c1..8db915d17 100644 --- a/packages/git/tests/retained-repository-push.test.ts +++ b/packages/git/tests/retained-repository-push.test.ts @@ -1884,14 +1884,25 @@ describe("one entry's publication", () => { it("P21: cancelling a pending authentication releases it, and keeps the request", function* () { const fixture = yield* useFixture(); const counted = counters(); - const held = withResolvers(); + const held = withResolvers(); const reached = withResolvers(); + const cleaning = withResolvers(); const session: { opened: boolean; released: boolean } = { opened: false, released: false }; + let returned = false; - // The credential session a live attempt opens for its destination, held - // pending until this row releases it. The managed clone opens one of its - // own first, so only the publication's is held: holding the clone's would - // stop the entry before a publication existed at all. + // The credential session a live attempt opens for its destination, owed a + // delivery this row makes only once cancellation has reached it. The + // managed clone opens one of its own first, so only the publication's is + // held: holding the clone's would stop the entry before a publication + // existed at all. + // + // Its cleanup is registered before the pending acquisition is announced, + // because the halt below is started off that announcement: an `ensure` + // yielded afterwards registers nothing on a scope already unwinding, and + // the cancellation would reach an acquisition with no cleanup on it. That + // cleanup suspends, so it is an `ensure` and not a `finally` — a `finally` + // that yields takes the frame out of return-mode while the scope is being + // torn down. let opened = 0; const sessions = (): Operation => resource(function* (provide) { @@ -1900,13 +1911,18 @@ describe("one entry's publication", () => { yield* provide(UNAUTHENTICATED); return; } - reached.resolve(); - yield* held.operation; - session.opened = true; - yield* ensure(() => { + yield* ensure(function* () { + cleaning.resolve(); + // The same delivery the cancelled acquisition was waiting for. A + // settled resolver answers every later read, so the session this + // attempt was owed arrives here instead of at the initializer the + // halt unwound, and the halt cannot settle until it is released. + yield* held.operation; session.released = true; }); - yield* provide(UNAUTHENTICATED); + reached.resolve(); + const delivered = yield* held.operation; + yield* provide(delivered); }); const stream = new InMemoryStream(); @@ -1923,8 +1939,33 @@ describe("one entry's publication", () => { throw new Error("the publication never opened a session for its destination"); })(), ]); - const halting = yield* spawn(() => task.halt()); - held.resolve(); + // The halt is observed rather than merely started, so `returned` says + // that `task.halt()` came back and not that something asked it to. + const halting = yield* spawn(function* (): Operation { + yield* task.halt(); + returned = true; + }); + try { + // What orders this row is that acknowledgement and nothing else: + // until the cleanup says it started, nothing proves the cancellation + // reached the acquisition this delivery is owed to. + yield* race([ + cleaning.operation, + (function* (): Operation { + yield* sleep(30_000); + throw new Error("the cancelled authentication never started its cleanup"); + })(), + ]); + expect(session.opened).toBe(false); + expect(session.released).toBe(false); + expect(returned).toBe(false); + } finally { + // External arrival, from this row rather than from the acquisition the + // halt unwound. In a `finally` with no yield in it, so an assertion + // that failed above leaves the joined cleanup nothing to wait for. + session.opened = true; + held.resolve(UNAUTHENTICATED); + } yield* halting; }); @@ -1936,10 +1977,12 @@ describe("one entry's publication", () => { // one this row held. expect(new Set(counted.sessions)).toEqual(new Set([fixture.origin])); expect(counted.sessions.length).toBeGreaterThan(1); - // The delivery this attempt's own scope was still owed arrived after the - // halt began, and the halt did not settle before it was released. + // The delivery this attempt's own scope was still owed arrived after that + // scope's cleanup had started, the cleanup released it, and only then did + // the halt return. expect(session.opened).toBe(true); expect(session.released).toBe(true); + expect(returned).toBe(true); expect(subcommands(counted).filter((name) => name === "push")).toEqual([]); expect(yield* remoteCommit(fixture, "review/credential")).toBe(undefined);