From dbd9288399745a8552d4a4d937a1647955d9c79b Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 4 Nov 2026 15:35:57 -0800 Subject: [PATCH] fix(ship-queue): a lock push GitHub applied but answered 502 no longer leaks the land lock (GST-1344) (#3994) * fix(ship-queue): a lock push GitHub applied but answered 502 no longer leaks the land lock (GST-1344) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes Review-Finding: p1/specialist-batched#1 reader=testing Review-Finding: p1/specialist-batched#2 reader=testing Review-Finding: p1/red-team#1 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes Review-Finding: p2/outside-voice#1 reader=outside-voice Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: troubleshooting covers a land lock push GitHub applied but answered 502 (GST-1344) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- .../garrytan-gst-1344-land-lock-502.md | 1 + docs/troubleshooting.md | 11 +++- lib/ship-queue-land-lock.ts | 42 ++++++++++--- test/helpers/flaky-git.ts | 60 ++++++++++++++++++ test/ship-queue-land-direct.test.ts | 61 +++++++++++++++++++ test/ship-queue-land-lock.test.ts | 54 ++++++++++++++++ 6 files changed, 217 insertions(+), 12 deletions(-) create mode 100644 changelog.d/garrytan-gst-1344-land-lock-502.md create mode 100644 test/helpers/flaky-git.ts diff --git a/changelog.d/garrytan-gst-1344-land-lock-502.md b/changelog.d/garrytan-gst-1344-land-lock-502.md new file mode 100644 index 000000000..f956ada7c --- /dev/null +++ b/changelog.d/garrytan-gst-1344-land-lock-502.md @@ -0,0 +1 @@ +- **`land --pin` no longer leaves the land lock stuck in `merging` when GitHub applies a lock push and still answers HTTP 502.** On 2026-11-04, during GitHub 502s, the first land of #3990 reported "the land lock push to origin failed and the lock there is unchanged" while origin's `refs/notes/gstack-land` already held #3990 in `merging`. Its release then failed against the old lease, and two reruns refused the same way until `--break-lock`. Now a lock push that reports failure asks origin (`git ls-remote`) what the ref holds, and treats a push origin applied as written. When a lock fetch fails, the read takes origin's sha from `ls-remote` if this clone already has that commit, so a rerun resumes its own lock instead of leasing a stale sha. When a phase change or release is rejected because origin holds a newer lock from this same run, land adopts that sha and retries once, so a refusal still releases the lock it wrote. A push that origin can't confirm either way now says so instead of claiming the lock is unchanged. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 1b240a916..46246e90f 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -4991,13 +4991,16 @@ uses it while a slot cannot merge yet: checks on the stamp commit are not green, a name in `changelog_fragments.required_checks` is missing or not green there, the PR's base is no longer the train's base, the stamp push failed while the branch still read the pin, `gh pr merge` refused, or the merge is not yet -visible on main. +visible on main. `land` also uses it when a land lock push to origin failed and +origin's lock ref could not be read to tell whether the push landed. **Fix.** Commit and push, wait for CI on that head, rerun `gstack-pregate run --allow-repo-commands` when the tree changed, then rerun `gstack-ship-queue ready --pr `. The block it prints before refusing shows which line is not yet true. For `train land`, do what `NEXT:` says, then rerun -the same `train land` command; it adopts its own journaled stamp commit. +the same `train land` command; it adopts its own journaled stamp commit. For an +unreadable land lock, rerun the same `land` command once origin answers; it +resumes its own lock. ### `QUEUE_PROOF_MISSING` @@ -5140,6 +5143,10 @@ released by the verify that clears the freeze next `land` or `train land` takes the lock over once the freeze record it fetches shows the freeze cleared after the lock was written; `--break-lock` never does. +A lock push that GitHub applied but answered with an error (an HTTP 502) is +checked against origin's ref and counts as written, so a rerun resumes this +run's own lock without `--break-lock`. + ### `QUEUE_TRAIN_UNSTAMPED` diff --git a/lib/ship-queue-land-lock.ts b/lib/ship-queue-land-lock.ts index 9751e3754..5835d3b12 100644 --- a/lib/ship-queue-land-lock.ts +++ b/lib/ship-queue-land-lock.ts @@ -2,7 +2,9 @@ * The repository land lock (docs/ship-policy.md#land, #3599): one ref, refs/notes/gstack-land, whose commit * holds lock.json, shared by `gstack-ship-queue land` (owner `{ pr }`) and `train land` (owner * `{ train, slot, pr }`), so a standalone land and a train slot never both publish onto one main. Every write - * is an exact lease on the sha read, so two writers racing one sha leave one winner. A lock is resumed only by + * is an exact lease on the sha read, so two writers racing one sha leave one winner. A push that reports failure is + * checked against origin's ref (GitHub can apply a push and still answer HTTP 502, GST-1344), and a read whose fetch + * fails takes origin's sha from ls-remote when this clone already holds that commit. A lock is resumed only by * the same owner kind, identity and journal; anything else is a takeover the caller allows (a drift repair, a * train's next slot, the admitted repair train) or a --break-lock under the staleness, mid-push, drift, freeze * and live-train rules. @@ -24,11 +26,19 @@ export const lockLabel = (lock: LandLock): string => (lock.train ? `train ${lock export const ownsLock = (lock: LandLock, owner: LockOwner, journal: string): boolean => lock.pr === owner.pr && (lock.train ?? null) === (owner.train ?? null) && (lock.slot ?? null) === (owner.slot ?? null) && lock.journal === journal; +/** The sha origin's lock ref holds: '' when origin has none, null when origin cannot be read. */ +function originSha(root: string, remote: string): string | null { + const listed = git(root, ['ls-remote', remote, LAND_LOCK_REF]); + if (!listed.ok) return null; + return listed.out.split('\n').map(l => l.split('\t')).find(([, ref]) => ref === LAND_LOCK_REF)?.[0] ?? ''; +} + export function readLock(root: string, remote = 'origin'): LockRead { const fetched = git(root, ['fetch', '-q', remote, `+${LAND_LOCK_REF}:${LAND_LOCK_REF}`]); if (!fetched.ok) { - const listed = git(root, ['ls-remote', remote, LAND_LOCK_REF]); - if (listed.ok && !listed.out.trim()) git(root, ['update-ref', '-d', LAND_LOCK_REF]); + const at = originSha(root, remote); + if (at === '') git(root, ['update-ref', '-d', LAND_LOCK_REF]); + else if (at && git(root, ['cat-file', '-e', `${at}^{commit}`]).ok) git(root, ['update-ref', LAND_LOCK_REF, at]); } const sha = git(root, ['rev-parse', '--verify', '--quiet', LAND_LOCK_REF]).out; if (!sha) return { state: 'absent', sha: '' }; @@ -41,15 +51,18 @@ export function readLock(root: string, remote = 'origin'): LockRead { return { state: 'held', sha, lock: value as LandLock }; } -export function writeLock(root: string, expected: string, value: LandLock | Released, remote = 'origin'): { ok: boolean; sha: string } { +/** Push the lock under the lease. On a failed push, `origin` is the sha origin's ref now holds ('' none, null unreadable); a push origin applied anyway is a success. */ +export function writeLock(root: string, expected: string, value: LandLock | Released, remote = 'origin'): { ok: boolean; sha: string; origin?: string | null } { const blob = git(root, ['hash-object', '-w', '--stdin'], undefined, JSON.stringify(value, null, 2) + '\n').out; const tree = git(root, ['mktree'], undefined, `100644 blob ${blob}\tlock.json\n`).out; const message = 'released' in value ? `land lock released by ${value.by}` : `land lock ${value.train ? `train ${value.train} slot ${value.slot} ` : ''}#${value.pr} ${value.phase}`; const commit = git(root, ['commit-tree', tree, ...(expected ? ['-p', expected] : []), '-m', message]).out; - if (![blob, tree, commit].every(isSha)) return { ok: false, sha: '' }; + if (![blob, tree, commit].every(isSha)) return { ok: false, sha: '', origin: expected }; const push = git(root, ['push', '-q', `--force-with-lease=${LAND_LOCK_REF}:${expected}`, remote, `${commit}:${LAND_LOCK_REF}`]); - if (push.ok) git(root, ['update-ref', LAND_LOCK_REF, commit]); - return { ok: push.ok, sha: commit }; + const origin = push.ok ? commit : originSha(root, remote); + if (origin !== commit) return { ok: false, sha: commit, origin }; + git(root, ['update-ref', LAND_LOCK_REF, commit]); + return { ok: true, sha: commit }; } export interface HeldLandLock { @@ -114,11 +127,20 @@ export function acquireLandLock(o: AcquireOptions): { held: HeldLandLock; lines: } const startedAt = own?.started_at ?? o.now().toISOString(); const always = o.extra?.(own) ?? {}; + const write = (value: LandLock | Released) => { + const w = writeLock(o.root, sha, value, remote); + if (w.ok || w.origin === sha) return w; + const seen = readLock(o.root, remote); + if (seen.state !== 'held' || seen.sha === sha || !ownsLock(seen.lock, o.owner, o.journal) || seen.lock.holder !== o.holder) return w; + sha = seen.sha; + return writeLock(o.root, sha, value, remote); + }; const set = (phase: LandLockPhase, extra: Partial = {}): QueueFailure | null => { const next: LandLock = { pr: o.owner.pr, ...(o.owner.train ? { train: o.owner.train, slot: o.owner.slot } : {}), holder: o.holder, started_at: startedAt, phase, journal: o.journal, phase_at: o.now().toISOString(), ...always, ...extra }; - const w = writeLock(o.root, sha, next, remote); + const w = write(next); if (!w.ok) { - return readLock(o.root, remote).sha === sha + if (w.origin === null) return { code: 'QUEUE_NOT_READY', message: `the land lock push to ${remote} failed and ${remote}'s lock could not be read, so this run cannot tell whether the push landed`, fix: 'rerun the same command once origin answers; it resumes this run\'s own lock' }; + return w.origin === sha ? { code: 'QUEUE_NOT_READY', message: `the land lock push to ${remote} failed and the lock there is unchanged; nothing was pushed` } : { code: 'QUEUE_LAND_LOCKED', message: 'the land lock moved under this run (another lander took it)' }; } @@ -131,7 +153,7 @@ export function acquireLandLock(o: AcquireOptions): { held: HeldLandLock; lines: get lock() { return current!; }, resumed: own, set, - release: () => writeLock(o.root, sha, { released: true, by: o.holder, at: o.now().toISOString() }, remote).ok, + release: () => write({ released: true, by: o.holder, at: o.now().toISOString() }).ok, }; return { held, lines }; } diff --git a/test/helpers/flaky-git.ts b/test/helpers/flaky-git.ts new file mode 100644 index 000000000..cad365fb1 --- /dev/null +++ b/test/helpers/flaky-git.ts @@ -0,0 +1,60 @@ +/** + * A git shim that fails the land lock's network calls the way GitHub did during the 2026-11-04 HTTP 502s + * (GST-1344): a lock push that updates origin's ref and then exits non-zero, and lock fetches or ls-remotes that + * fail outright. Only commands naming refs/notes/gstack-land are touched; everything else runs the real git. + * Arm it with the returned setters; put `dir` first on PATH (or pass `env` to a child). + */ +import { execFileSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; + +export interface FlakyGit { + dir: string; + env: { PATH: string; FLAKY_GIT: string }; + /** The lock pushes (1-based, counted from now) that reach origin and then report an HTTP 502. */ + pushLandsThenFails(...ordinals: number[]): void; + /** The next n lock pushes fail before reaching origin. */ + pushFails(n: number): void; + /** Lock fetches fail while on. */ + fetchFails(on: boolean): void; + /** The next n lock ls-remotes fail. */ + lsRemoteFails(n: number): void; + reset(): void; +} + +export function flakyGit(root: string): FlakyGit { + const dir = path.join(root, 'flaky-git'); + const state = path.join(dir, 'state'); + fs.mkdirSync(state, { recursive: true }); + const real = execFileSync('bash', ['-c', 'command -v git'], { encoding: 'utf8', timeout: 10_000 }).trim(); + fs.writeFileSync(path.join(dir, 'git'), [ + '#!/bin/bash', + `REAL='${real}'`, + `D=':${dir}:'`, + 'P=":$PATH:"; P="${P//"$D"/:}"; P="${P#:}"; export PATH="${P%:}"', + 'S="$FLAKY_GIT"', + 'case " $* " in *refs/notes/gstack-land*) ;; *) exec "$REAL" "$@" ;; esac', + 'bad() { echo "error: RPC failed; HTTP 502 curl 22 The requested URL returned error: 502" >&2; exit 128; }', + 'take() { local n; n=$(cat "$S/$1" 2>/dev/null || echo 0); [ "$n" -gt 0 ] || return 1; echo $((n-1)) > "$S/$1"; }', + 'case "$1" in', + ' push)', + ' n=$(( $(cat "$S/pushes" 2>/dev/null || echo 0) + 1 )); echo $n > "$S/pushes"', + ' take push-fails && bad', + ' if grep -qx "$n" "$S/lands-then-fails" 2>/dev/null; then "$REAL" "$@" || exit $?; bad; fi ;;', + ' fetch) [ -f "$S/fetch-fails" ] && bad ;;', + ' ls-remote) take ls-remote-fails && bad ;;', + 'esac', + 'exec "$REAL" "$@"', + '', + ].join('\n'), { mode: 0o755 }); + const put = (name: string, value: string | null) => { const f = path.join(state, name); if (value === null) fs.rmSync(f, { force: true }); else fs.writeFileSync(f, value); }; + return { + dir, + env: { PATH: `${dir}:${process.env.PATH ?? ''}`, FLAKY_GIT: state }, + pushLandsThenFails: (...ordinals) => { put('pushes', '0'); put('lands-then-fails', ordinals.join('\n') + '\n'); }, + pushFails: n => put('push-fails', String(n)), + fetchFails: on => put('fetch-fails', on ? '1' : null), + lsRemoteFails: n => put('ls-remote-fails', String(n)), + reset: () => { for (const f of fs.readdirSync(state)) fs.rmSync(path.join(state, f), { force: true }); }, + }; +} diff --git a/test/ship-queue-land-direct.test.ts b/test/ship-queue-land-direct.test.ts index 8b306aecc..4a17420f7 100644 --- a/test/ship-queue-land-direct.test.ts +++ b/test/ship-queue-land-direct.test.ts @@ -20,6 +20,7 @@ import { bumpVersion } from '../lib/version-gate-train'; import { openRelease } from '../lib/release-stamp'; import { runMergedChecks } from '../lib/land-merged-checks'; import { readLock, writeLock } from '../lib/ship-queue-land-lock'; +import { flakyGit } from './helpers/flaky-git'; import { runBackstop } from '../scripts/train-backstop'; import { readTrainNotes } from '../scripts/release-cut'; import { gitIn } from '../scripts/train-gate'; @@ -290,6 +291,66 @@ describe('land --pin: direct merge at the PR\'s own green head', () => { expect(dry.out).toContain('DRY-RUN: nothing verified'); }, 120_000); + describe('GST-1344: a lock push GitHub applied but answered with an HTTP 502', () => { + const lockAt = (fx: Fx) => JSON.parse(fx.repo.git('--git-dir', fx.repo.origin, 'show', 'refs/notes/gstack-land:lock.json')); + const flaky = (fx: Fx) => { const g = flakyGit(fx.repo.root); return { g, run: (pr: string, pin: string, extra: string[] = []) => runBin(fx.repo, 'gstack-ship-queue', ['land', '--pr', pr, '--pin', pin, '--base', 'main', ...extra], { ...fx.env, ...g.env, PATH: `${g.dir}:${fx.env.PATH}` }) }; }; + + test('the merging push lands but reports failure while lock fetches fail: land reads origin, sees its own commit, merges and releases', () => { + const fx = setup(); + const { g, run } = flaky(fx); + const pin = openPr(fx, '95'); + g.pushLandsThenFails(2); + g.fetchFails(true); + const r = run('95', pin); + expect(r.err).toBe(''); + expect(r.status).toBe(EXIT.ok); + expect(r.out).toContain('MERGED: #95 as '); + expect(fx.read().lockAtMerge).toMatchObject({ pr: '95', phase: 'merging', journal: 'land-pin:' + pin }); + expect(lockAt(fx)).toMatchObject({ released: true }); + }, 120_000); + + test('origin cannot be read after the push either: the run refuses, and the rerun takes its lease from origin (ls-remote) while fetches still fail, then merges and releases', () => { + const fx = setup(); + const { g, run } = flaky(fx); + const pin = openPr(fx, '96'); + g.pushLandsThenFails(2); + g.fetchFails(true); + g.lsRemoteFails(99); + const first = run('96', pin); + expect(first.status).toBe(EXIT.refused); + expect(first.err).toContain('(QUEUE_NOT_READY)'); + expect(first.err).not.toContain('the lock there is unchanged'); + expect(fx.read().prs['96'].state).toBe('OPEN'); + expect(lockAt(fx)).toMatchObject({ pr: '96', phase: 'merging', journal: 'land-pin:' + pin }); + g.reset(); + g.fetchFails(true); + const again = run('96', pin); + expect(again.err).toBe(''); + expect(again.status).toBe(EXIT.ok); + expect(again.out).toContain('MERGED: #96 as '); + expect(lockAt(fx)).toMatchObject({ released: true }); + }, 120_000); + + test('never left in merging: the merging push lands and origin stays unreadable through the lock read, the post-push check and the re-read (three ls-remotes), so the run refuses, yet its release re-reads origin, finds its own lock and releases it; a rerun lands', () => { + const fx = setup(); + const { g, run } = flaky(fx); + const pin = openPr(fx, '97'); + g.pushLandsThenFails(2); + g.lsRemoteFails(3); + g.fetchFails(true); + const first = run('97', pin); + expect(first.status).toBe(EXIT.refused); + expect(first.err).toContain('(QUEUE_NOT_READY)'); + expect(first.out).not.toContain('LOCK: warn'); + expect(fx.read().prs['97'].state).toBe('OPEN'); + expect(lockAt(fx)).toMatchObject({ released: true }); + g.reset(); + const again = run('97', pin); + expect(again.err).toBe(''); + expect(again.out).toContain('MERGED: #97 as '); + }, 120_000); + }); + test('main moved during the merge with land.merged_checks: they run on the landed tree; a rerun without --allow-repo-commands refuses with REPO_COMMANDS_NOT_ALLOWED and runs nothing', () => { const fx = setup(CHECKED); const pin = openPr(fx, '91'); diff --git a/test/ship-queue-land-lock.test.ts b/test/ship-queue-land-lock.test.ts index 6afefbfce..c7a3af51f 100644 --- a/test/ship-queue-land-lock.test.ts +++ b/test/ship-queue-land-lock.test.ts @@ -12,6 +12,7 @@ import * as path from 'node:path'; import { EXIT } from '../lib/headless-artifacts'; import { acquireLandLock, LAND_LOCK_REF, readLock, writeLock, type LandLock, type LockOwner } from '../lib/ship-queue-land-lock'; import { BASE_POLICY, cleanup, makeFixtureRepo, type FixtureRepo } from './helpers/restamp-fixture'; +import { flakyGit } from './helpers/flaky-git'; import { coordinatorClone } from './helpers/ship-queue-fixture'; import { GREEN_GATE, TRAIN_POLICY, fragmentPr, nonceOf, queue, repoKey, trainFixture, trainJson, wrapLand, type TrainFixture, @@ -117,6 +118,59 @@ describe('the lock module', () => { }); }, 60_000); + test('GST-1344: a push origin applied but answered with an HTTP 502 counts as written; a push that never landed reports the lock unchanged; an unreadable origin says so instead', () => { + const repo = bareRepo(); + const g = flakyGit(repo.root); + withRepoEnv({ ...repo, env: { ...repo.env, ...g.env } }, () => { + const root = repo.work; + const lock: LandLock = { pr: '11', holder: 'h11', started_at: new Date(T0).toISOString(), phase: 'stamping', journal: 'land/11' }; + g.pushLandsThenFails(1); + const w = writeLock(root, '', lock); + expect(w.ok).toBe(true); + expect(repo.git('rev-parse', LAND_LOCK_REF)).toBe(w.sha); + expect(repo.git('--git-dir', repo.origin, 'rev-parse', LAND_LOCK_REF)).toBe(w.sha); + + g.reset(); + const got = take(root, standalone('11'), { journal: 'land/11' }); + expect(failureOf(got)).toBeNull(); + if (!('held' in got)) return; + g.pushFails(1); + expect(got.held.set('waiting')).toMatchObject({ code: 'QUEUE_NOT_READY', message: expect.stringContaining('the lock there is unchanged') }); + g.pushFails(1); + g.lsRemoteFails(2); + g.fetchFails(true); + expect(got.held.set('waiting')).toMatchObject({ code: 'QUEUE_NOT_READY', message: expect.stringContaining('could not be read') }); + g.reset(); + expect(got.held.release()).toBe(true); + expect(readLock(root)).toMatchObject({ state: 'released' }); + }); + }, 60_000); + + test('GST-1344: a phase change rejected because origin holds a newer lock from this same run adopts that sha and retries once; the same journal under another holder is not adopted', () => { + const repo = bareRepo(); + withRepoEnv(repo, () => { + const root = repo.work; + const got = take(root, standalone('12'), { journal: 'land/12' }); + if (!('held' in got)) throw new Error('lock not taken'); + const ahead = (holder: string) => { + const r = readLock(root); + const w = writeLock(root, r.sha, { ...got.held.lock, holder, phase: 'waiting' }); + expect(w.ok).toBe(true); + repo.git('update-ref', LAND_LOCK_REF, r.sha); + return w.sha; + }; + const mine = ahead('test:1'); + expect(got.held.set('pushing')).toBeNull(); + const after = readLock(root); + expect(after).toMatchObject({ state: 'held', lock: { phase: 'pushing' } }); + expect(repo.git('rev-parse', `${after.sha}^`)).toBe(mine); + + const theirs = ahead('other:2'); + expect(got.held.set('merging')).toMatchObject({ code: 'QUEUE_LAND_LOCKED' }); + expect(repo.git('--git-dir', repo.origin, 'rev-parse', LAND_LOCK_REF)).toBe(theirs); + }); + }, 60_000); + test('--break-lock prints the holder, refuses a live train run\'s lock without --break-train-lock, and keeps the staleness, mid-push, between and freeze rules', () => { const repo = bareRepo(); withRepoEnv(repo, () => {