Summary
#416 delivered the comment-triggered re-check (@github2gerrit check), which
transfers an approved fork pull request. A scheduled sweep would remove the
comment step entirely — a maintainer approves in the ordinary way and the next
sweep notices — which was the original zero-touch design.
A sweep was implemented in #418 and then removed from it, because a
repository-wide job cannot be serialised against the per-pull-request runs it
duplicates. This issue records what a correct implementation has to satisfy,
so the next attempt starts from the constraints rather than rediscovering them.
Why the straightforward version does not work
Concurrency. The sweep runs in g2g-<repo>-schedule while a comment or
synchronize run for the same pull request runs in g2g-<repo>-<number>.
cancel-in-progress: false does not serialise across different groups, and
GitHub concurrency is per job, so one repository-wide job cannot share a lock
with per-pull-request runs.
Both runs can read the same approval and race to create the same Gerrit change.
The push half of that race is already covered — _enforce_approved_head
refuses a head that moved after the gate ran — but two runs at the same
approved commit are not. That is likeliest precisely when the feature is used:
an impatient maintainer comments @github2gerrit check while the sweep is
already transferring. Two concurrent first transfers can leave two Gerrit
changes for one pull request, and ALLOW_DUPLICATES defaults to true, under
which duplicate detection logs and continues.
Scope. A sweep must not visit every open pull request. Re-running the
pipeline for all of them on every interval would push to Gerrit repeatedly, and
again ALLOW_DUPLICATES is not a brake. The narrowing developed in #418 was to
skip heads where head_is_trusted — a same-repository pull request never
passed through the gate, so a sweep has nothing to notice about it. Note the
predicate: a pull request with unresolved provenance must still be visited,
since the gate applies to those and skipping them would strand them.
Idempotency. Even correctly scoped, a sweep re-runs the pipeline for a fork
pull request that has already been transferred. That is #419.
Requirements
Approaches worth weighing
Fan out to per-pull-request jobs. A sweep job enumerates candidates and a
matrix job processes each, so each lands in its own concurrency group. This
solves serialisation directly. Cost: a second job, matrix plumbing, and the
enumeration job needs its own token scope.
Idempotency instead of locking. If #419 gives the sweep a reliable "already
transferred at this commit" signal, a losing racer becomes a no-op rather than
a duplicate. Cheaper, but it makes correctness depend on a best-effort comment
marker, which is weaker than a lock.
The two combine well: fan-out for serialisation, the marker to avoid pointless
work.
Not urgent
The comment doorbell already delivers the feature. This is convenience, and it
should not be rushed — it produced the most serious finding in each of three
review rounds on #418.
Summary
#416 delivered the comment-triggered re-check (
@github2gerrit check), whichtransfers an approved fork pull request. A scheduled sweep would remove the
comment step entirely — a maintainer approves in the ordinary way and the next
sweep notices — which was the original zero-touch design.
A sweep was implemented in #418 and then removed from it, because a
repository-wide job cannot be serialised against the per-pull-request runs it
duplicates. This issue records what a correct implementation has to satisfy,
so the next attempt starts from the constraints rather than rediscovering them.
Why the straightforward version does not work
Concurrency. The sweep runs in
g2g-<repo>-schedulewhile a comment orsynchronizerun for the same pull request runs ing2g-<repo>-<number>.cancel-in-progress: falsedoes not serialise across different groups, andGitHub concurrency is per job, so one repository-wide job cannot share a lock
with per-pull-request runs.
Both runs can read the same approval and race to create the same Gerrit change.
The push half of that race is already covered —
_enforce_approved_headrefuses a head that moved after the gate ran — but two runs at the same
approved commit are not. That is likeliest precisely when the feature is used:
an impatient maintainer comments
@github2gerrit checkwhile the sweep isalready transferring. Two concurrent first transfers can leave two Gerrit
changes for one pull request, and
ALLOW_DUPLICATESdefaults totrue, underwhich duplicate detection logs and continues.
Scope. A sweep must not visit every open pull request. Re-running the
pipeline for all of them on every interval would push to Gerrit repeatedly, and
again
ALLOW_DUPLICATESis not a brake. The narrowing developed in #418 was toskip heads where
head_is_trusted— a same-repository pull request neverpassed through the gate, so a sweep has nothing to notice about it. Note the
predicate: a pull request with unresolved provenance must still be visited,
since the gate applies to those and skipping them would strand them.
Idempotency. Even correctly scoped, a sweep re-runs the pipeline for a fork
pull request that has already been transferred. That is #419.
Requirements
for the same pull request, or is otherwise made safe against a concurrent
transfer at the same commit
head_is_trustedso unresolved provenance is still visiteddocumented
Approaches worth weighing
Fan out to per-pull-request jobs. A sweep job enumerates candidates and a
matrix job processes each, so each lands in its own concurrency group. This
solves serialisation directly. Cost: a second job, matrix plumbing, and the
enumeration job needs its own token scope.
Idempotency instead of locking. If #419 gives the sweep a reliable "already
transferred at this commit" signal, a losing racer becomes a no-op rather than
a duplicate. Cheaper, but it makes correctness depend on a best-effort comment
marker, which is weaker than a lock.
The two combine well: fan-out for serialisation, the marker to avoid pointless
work.
Not urgent
The comment doorbell already delivers the feature. This is convenience, and it
should not be rushed — it produced the most serious finding in each of three
review rounds on #418.