Cloonar/coding-lab

Fork 0

autoland: one priority-ordered spawn loop — drain the pipeline before filling it #185

New issue

Closed

opened 2026-07-20 15:16:20 +02:00 by dominik.polakovics · 1 comment

dominik.polakovics commented

2026-07-20 15:16:20 +02:00

Owner

What to build

One spawn loop for the whole AFK/autoland fleet, ordered by pipeline stage, replacing the two independent loops that currently race for the same instance cap.

Why

SchedulerLoop (ScheduleOnce) and ReaperLoop (AutolandOnce, added by #181) are separate goroutines. Both read liveCount < EffectiveCap(repo) against their own snapshot and both launch. Whichever tick fires first takes the slot, so:

The cap can be over-subscribed. Two loops racing to consume one bounded resource is a race by construction, not an edge case.
The lander has arbitrary priority, not high priority. Whether a claim PR gets validated before a fresh issue is started is currently down to tick alignment.

#182 makes this strictly worse by adding fix runs as a third spawner. Doing this refactor first means #182 becomes an addition (one new candidate producer) instead of a modification of a racing structure.

The rule

Drain the pipeline before filling it. Work already in flight outranks starting new work, at every stage:

validate (lander)  >  fix (#182)  >  new AFK work

Rationale: finishing in-flight claims drains the pipeline; starting new work fills it. Without the ordering a repo at cap can spend every slot opening new PRs while validated-but-unlanded ones queue up behind them.

Shape

AutolandOnce stops launching and becomes a candidate producer.
ScheduleOnce's selection likewise becomes a candidate producer for new AFK work.
A single pass merges the candidate lists, sorts by pipeline stage, and launches down the list while under cap — one consumer of the cap, so the race is gone by construction rather than mitigated.
The two loops keep their other duties (reaping, sweeping, the runtime sweep); only the spawn decision merges.
Priority is data (a stage rank on the candidate), not control flow spread across two files — so #182 plugs in by emitting candidates at the fix rank.

Decided along the way

Lander outcomes stop feeding the AFK three-strikes counter. Today reapRun's failure switch is kind-agnostic (its own doc still enumerates only the AFK kinds, so the coupling looks unintended), which produces two wrong behaviours:

A lander death/timeout increments consecutive_failures; at PauseThreshold the repo's AFK auto loop pauses and manual starts 409. Lander flakiness silently suspends unrelated AFK work.
A lander posting reject classifies OutcomeSuccess → ResetRepoFailures. So a lander correctly rejecting a broken AFK PR clears the strikes that broken AFK runs accumulated — the brake gets re-armed by exactly the evidence that should trip it. Under the fix-forward loop this compounds: the pipeline feeds itself.

The budget clock stays shared (that part is deliberate and fine); only the failure counter separates. Whether landers get their own counter or simply none is an implementation call — the requirement is that a lander outcome never moves the AFK counter in either direction.

Acceptance criteria

Exactly one code path consumes EffectiveCap; no launch decision is made outside it
Spawn order is lander > fix > new AFK, pinned by a table test over a mixed candidate set
At cap, a pending lander candidate is launched in preference to a pending new-work candidate — asserted, not incidental
The cap cannot be over-subscribed by concurrent scheduler/reaper ticks (test drives both against one repo at cap)
A lander death/timeout does not increment consecutive_failures; a lander reject does not reset it; AFK run accounting is unchanged
Existing AFK scheduling behaviour (three-strikes pause, ready-for-agent selection, claim rules) is behaviour-identical for repos with autoland off
The candidate list has an explicit extension point for #182's fix runs, exercised by at least a stub candidate at the fix rank

Notes

Blocks/precedes #182 — its brief should be updated to emit candidates into this list rather than adding its own spawner.
Autoland ships default-off (#181/#184), so the current race is latent; this is not a live production defect, it is a correctness debt to clear before the third spawner lands.
Context: the priority rule and the counter decision come from the #184 landing review; see the audit comment on #181.

## What to build One spawn loop for the whole AFK/autoland fleet, ordered by pipeline stage, replacing the two independent loops that currently race for the same instance cap. ## Why `SchedulerLoop` (`ScheduleOnce`) and `ReaperLoop` (`AutolandOnce`, added by #181) are separate goroutines. Both read `liveCount < EffectiveCap(repo)` against their own snapshot and both launch. Whichever tick fires first takes the slot, so: - **The cap can be over-subscribed.** Two loops racing to consume one bounded resource is a race by construction, not an edge case. - **The lander has arbitrary priority, not high priority.** Whether a claim PR gets validated before a fresh issue is started is currently down to tick alignment. #182 makes this strictly worse by adding fix runs as a **third** spawner. Doing this refactor first means #182 becomes an addition (one new candidate producer) instead of a modification of a racing structure. ## The rule Drain the pipeline before filling it. Work already in flight outranks starting new work, at every stage: ``` validate (lander) > fix (#182) > new AFK work ``` Rationale: finishing in-flight claims drains the pipeline; starting new work fills it. Without the ordering a repo at cap can spend every slot opening new PRs while validated-but-unlanded ones queue up behind them. ## Shape - `AutolandOnce` stops launching and becomes a **candidate producer**. - `ScheduleOnce`'s selection likewise becomes a candidate producer for new AFK work. - A single pass merges the candidate lists, sorts by pipeline stage, and launches down the list while under cap — **one consumer of the cap**, so the race is gone by construction rather than mitigated. - The two loops keep their other duties (reaping, sweeping, the runtime sweep); only the *spawn decision* merges. - Priority is data (a stage rank on the candidate), not control flow spread across two files — so #182 plugs in by emitting candidates at the `fix` rank. ## Decided along the way **Lander outcomes stop feeding the AFK three-strikes counter.** Today `reapRun`'s failure switch is kind-agnostic (its own doc still enumerates only the AFK kinds, so the coupling looks unintended), which produces two wrong behaviours: - A lander death/timeout increments `consecutive_failures`; at `PauseThreshold` the repo's **AFK auto loop** pauses and manual starts 409. Lander flakiness silently suspends unrelated AFK work. - A lander posting `reject` classifies `OutcomeSuccess` → `ResetRepoFailures`. So a lander correctly rejecting a broken AFK PR **clears the strikes that broken AFK runs accumulated** — the brake gets re-armed by exactly the evidence that should trip it. Under the fix-forward loop this compounds: the pipeline feeds itself. The budget clock stays shared (that part is deliberate and fine); only the failure counter separates. Whether landers get their own counter or simply none is an implementation call — the requirement is that a lander outcome never moves the AFK counter in either direction. ## Acceptance criteria - [ ] Exactly one code path consumes `EffectiveCap`; no launch decision is made outside it - [ ] Spawn order is `lander > fix > new AFK`, pinned by a table test over a mixed candidate set - [ ] At cap, a pending lander candidate is launched in preference to a pending new-work candidate — asserted, not incidental - [ ] The cap cannot be over-subscribed by concurrent scheduler/reaper ticks (test drives both against one repo at cap) - [ ] A lander death/timeout does not increment `consecutive_failures`; a lander `reject` does not reset it; AFK run accounting is unchanged - [ ] Existing AFK scheduling behaviour (three-strikes pause, ready-for-agent selection, claim rules) is behaviour-identical for repos with autoland off - [ ] The candidate list has an explicit extension point for #182's fix runs, exercised by at least a stub candidate at the `fix` rank ## Notes - Blocks/precedes #182 — its brief should be updated to emit candidates into this list rather than adding its own spawner. - Autoland ships default-off (#181/#184), so the current race is latent; this is not a live production defect, it is a correctness debt to clear before the third spawner lands. - Context: the priority rule and the counter decision come from the #184 landing review; see the audit comment on #181.

dominik.polakovics added the

ready-for-agent

enhancement

labels

2026-07-20 15:16:20 +02:00

dominik.polakovics referenced this issue

2026-07-20 15:19:20 +02:00

autoland engine: per-repo settings + lander runs (auto-validate, auto-merge on clean PASS) #181

dominik.polakovics referenced this issue from a commit

2026-07-20 15:59:42 +02:00

rework(afk): one priority-ordered spawn pass — drain before fill, landers off the strike counter

dominik.polakovics referenced this issue from a pull request that will close it,

2026-07-20 16:00:18 +02:00

rework(afk): one priority-ordered spawn pass — drain the pipeline before filling it #186

dominik.polakovics commented

2026-07-20 16:17:34 +02:00

Author

Owner

This was generated by AI while landing a PR.

Landing audit — PR #186

Verdict: PASS.

Verification signal relied on: Forgejo Actions ci / native — success (6m27s, run 208). Not re-run. Reach confirmed adequate: the hermetic ci-nix gate is path-gated to **/*.nix/flake.lock/go.mod/go.sum, none of which this diff touches, so the native gate (Go build + full go test with real git/tmux/prlimit, plus golangci-lint) exercised every changed line.

Conventions: title rework(afk): … is Conventional-Commits shaped; body carries a working Closes #185; head afk/185 matches the claim-branch pattern; branch is up to date with main (no conflict).

Checked beyond CI:

Lock ordering sound. spawnMu is taken only in SpawnOnce; s.mu only in launch.go:109 / lander.go:127, both reached from inside the pass's launch closures. Order is always spawnMu → mu, never the reverse; no path holds mu on entry to the pass. No deadlock.
No orphaned callers. AutolandOnce / ScheduleOnce are gone from all Go code; only ADR-0049's historical narrative still names them.
Counter guard exactly complete. ActiveAFKRuns (internal/store/afk.go:26) returns precisely {afk_manual, afk_auto, lander} — plain manual is excluded — so reapRun's new AFKManual || AFKAuto guard is a clean 'everything reaped except lander', mirroring the existing done-signal guard at reaper.go:324. Nothing that legitimately fed the counter lost its feed.
Closure capture safe — repo/pull are per-iteration under Go 1.22+ semantics (go 1.26).
The headline AC test is real. TestSpawnOnce_concurrentReaperAndSchedulerTicksCannotOversubscribeCap (spawn_ac_test.go:104) races the literal tick bodies against cap 1 and asserts bounded (<=1), not-starved (==1), AND that the slot went to the lander.
Cap has one consumer. UnderCap removed from both predicates; spawnPass holds the running count, with the locked per-launch guards as backstop.

Non-blocking notes:

Both producers now run on both cadences, so the lander gather's trk.Pulls(ctx) — the unbounded history walk PR #179 exists to window — now also fires on the 45s scheduler tick. Costs nothing today: AutolandEnabled is default-off (repos.go:95), so the pre-filter returns nil with zero forge reads.
spawnMu is held across a pass's forge reads, so an HTTP toggle-on/reset kick can queue behind an in-flight pass — 'the toggle acts now' is now 'acts after the current pass drains'. Benign (kicks are go-dispatched), but a slight softening of the v0-parity promise the handler comment still states.

> *This was generated by AI while landing a PR.* ## Landing audit — PR #186 **Verdict: PASS.** **Verification signal relied on:** Forgejo Actions `ci / native` — success (6m27s, run 208). Not re-run. Reach confirmed adequate: the hermetic `ci-nix` gate is path-gated to `**/*.nix`/`flake.lock`/`go.mod`/`go.sum`, none of which this diff touches, so the native gate (Go build + full `go test` with real git/tmux/prlimit, plus golangci-lint) exercised every changed line. **Conventions:** title `rework(afk): …` is Conventional-Commits shaped; body carries a working `Closes #185`; head `afk/185` matches the claim-branch pattern; branch is up to date with `main` (no conflict). **Checked beyond CI:** - **Lock ordering sound.** `spawnMu` is taken only in `SpawnOnce`; `s.mu` only in `launch.go:109` / `lander.go:127`, both reached from inside the pass's launch closures. Order is always `spawnMu → mu`, never the reverse; no path holds `mu` on entry to the pass. No deadlock. - **No orphaned callers.** `AutolandOnce` / `ScheduleOnce` are gone from all Go code; only ADR-0049's historical narrative still names them. - **Counter guard exactly complete.** `ActiveAFKRuns` (`internal/store/afk.go:26`) returns precisely {afk_manual, afk_auto, lander} — plain `manual` is excluded — so `reapRun`'s new `AFKManual || AFKAuto` guard is a clean 'everything reaped except lander', mirroring the existing done-signal guard at `reaper.go:324`. Nothing that legitimately fed the counter lost its feed. - **Closure capture safe** — `repo`/`pull` are per-iteration under Go 1.22+ semantics (`go 1.26`). - **The headline AC test is real.** `TestSpawnOnce_concurrentReaperAndSchedulerTicksCannotOversubscribeCap` (`spawn_ac_test.go:104`) races the literal tick bodies against cap 1 and asserts bounded (<=1), not-starved (==1), AND that the slot went to the lander. - **Cap has one consumer.** `UnderCap` removed from both predicates; `spawnPass` holds the running count, with the locked per-launch guards as backstop. **Non-blocking notes:** 1. Both producers now run on both cadences, so the lander gather's `trk.Pulls(ctx)` — the unbounded history walk PR #179 exists to window — now also fires on the 45s scheduler tick. Costs nothing today: `AutolandEnabled` is default-off (`repos.go:95`), so the pre-filter returns nil with zero forge reads. 2. `spawnMu` is held across a pass's forge reads, so an HTTP toggle-on/reset kick can queue behind an in-flight pass — 'the toggle acts now' is now 'acts after the current pass drains'. Benign (kicks are `go`-dispatched), but a slight softening of the v0-parity promise the handler comment still states.

dominik.polakovics closed this issue

2026-07-20 16:19:35 +02:00

dominik.polakovics referenced this issue

2026-07-20 17:39:28 +02:00

feat(autoland): fix-forward loop — fix runs on rejection, spawn-count bound, terminal escalation #187