Skip to content

Commit b4ccfe7

Browse files
authored
fix(selfhost): backlog-vs-fresh fairness, convergence-cap shadowing, and maintenance admission feedback loop (#9233)
* fix(selfhost): add bounded age escape for the backlog-vs-fresh lane priority gate The lane-scoped foreground claim requires beating the best due unclassified priority, but githubWebhookPriority returns 10 (equal to fresh's own priority, above backlog's 9) for nearly every webhook other than a fresh PR open/reopen/ synchronize/ready-for-review event. On any repo with CI at least one such row is essentially always due, so neither lane's priority can ever satisfy the strict `>` gate and both lane claims permanently fall back to plain priority ordering -- the exact starvation the fairness mechanism exists to prevent. Add shouldEscapeLanePriorityGate: once the oldest due row in the lane being claimed has waited at least DEFAULT_FOREGROUND_LANE_MAX_STARVE_AGE_MS (10 minutes, mirroring maintenance-admission's trickle_max_defer_age), the gate is bypassed so an old lane row can win on its own merits. Closes #9153 * fix(selfhost): apply the lane-priority age escape in both queue backends Wires shouldEscapeLanePriorityGate into claimNextForegroundLane for both the Postgres and sqlite queue backends: each computes the oldest due row's age for the lane being claimed (fresh via a dedicated query, backlog by reusing the row set already fetched for repo selection) and falls back to the plain `>=` floor once that age crosses the bounded threshold, instead of staying permanently gated behind an unclassified priority-10 webhook. Includes regression tests for both backends covering the escape arm (an aged backlog row wins preferentially) and the control arm (a fresh backlog row stays gated this cycle), plus a fix to a pre-existing pg-queue test whose placeholder created_at (an epoch-adjacent 1000ms) incidentally tripped the new age escape. Closes #9153 * fix(selfhost): filter repair-exhausted PRs before the backlog-convergence cap selectBacklogConvergenceCandidates sorted oldest-open-first, sliced to `max`, and only then did the caller filter out repair-exhausted PRs -- so five old, permanently-unpublishable PRs could occupy every slot forever, shadowing the 6th+ PR needing convergence behind them indefinitely. Split the pure ordering out into sortedBacklogConvergenceCandidates (unsliced) and have sweepRepoBacklogConvergence walk that full order, skipping exhausted candidates as it goes and stopping once `max` actionable ones are found -- selectBacklogConvergenceCandidates itself becomes a thin slice-after-sort wrapper over the new function for callers that don't need the exhaustion filter. Also logs + records an audit event when every examined candidate is repair-exhausted, so a permanently wedged head of the backlog is visible instead of the sweep silently returning early every cycle. Closes #9154 * fix(selfhost): exclude in-flight jobs from live_job_age_high and cache pressure signals Two compounding defects in maintenance admission: - oldestLiveRunnableAgeMs was computed over rows that are either 'processing' or due-and-pending, so a normal in-flight job (a routine multi-minute AI review) alone pushed the age past maxLiveJobAgeMs for its whole duration, tripping live_job_age_high with no drain escape and collapsing the entire maintenance lane -- including the watchdog/alerter that would report an actual overload -- to the 4-hour trickle backstop. Narrow oldest_runnable's own FILTER to 'pending AND due' rows only, excluding 'processing' entirely. - Every claimed maintenance job recomputes maintenancePressureSignals' four aggregate scans, and a denial returns `true` from processOne(), so the pump loop immediately claims the next due maintenance row and repeats -- a burst of N due maintenance rows means 4N sequential scans in one tight loop, a positive feedback loop (more denials -> more scans -> higher load -> more denials). Add a short-TTL (default 1.5s, configurable via MAINTENANCE_ADMISSION_PRESSURE_CACHE_TTL_MS) memoized wrapper shared by the admission check and the pressureSignals() observability method, in both the Postgres and sqlite backends. Also add a partial index on (is_maintenance, status) for the maintenance-lane aggregate, which previously had no supporting index. Closes #9155
1 parent ad8975e commit b4ccfe7

13 files changed

Lines changed: 863 additions & 53 deletions

apps/loopover-ui/src/lib/selfhost-env-reference.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,10 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [
337337
name: "MAINTENANCE_ADMISSION_MAX_PENDING",
338338
firstReference: "src/selfhost/maintenance-admission.ts",
339339
},
340+
{
341+
name: "MAINTENANCE_ADMISSION_PRESSURE_CACHE_TTL_MS",
342+
firstReference: "src/selfhost/maintenance-admission.ts",
343+
},
340344
{
341345
name: "MIGRATIONS_DIR",
342346
firstReference: "src/server.ts",
@@ -733,6 +737,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [
733737
"| `MAINTENANCE_ADMISSION_MAX_LIVE_AGE_MS` | `src/selfhost/maintenance-admission.ts` |",
734738
"| `MAINTENANCE_ADMISSION_MAX_LIVE_PENDING` | `src/selfhost/maintenance-admission.ts` |",
735739
"| `MAINTENANCE_ADMISSION_MAX_PENDING` | `src/selfhost/maintenance-admission.ts` |",
740+
"| `MAINTENANCE_ADMISSION_PRESSURE_CACHE_TTL_MS` | `src/selfhost/maintenance-admission.ts` |",
736741
"| `MIGRATIONS_DIR` | `src/server.ts` |",
737742
"| `OBSERVABILITY_SMOKE_POLL_MS` | `scripts/smoke-observability-traces.ts` |",
738743
"| `OBSERVABILITY_SMOKE_TIMEOUT_MS` | `scripts/smoke-observability-traces.ts` |",

src/queue/processors.ts

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ import {
258258
isRegateSweepDraining,
259259
selectRegateCandidates,
260260
} from "../settings/agent-sweep";
261-
import { selectBacklogConvergenceCandidates } from "../selfhost/backlog-convergence";
261+
import { BACKLOG_CONVERGENCE_SWEEP_MAX_PRS, sortedBacklogConvergenceCandidates } from "../selfhost/backlog-convergence";
262262
import {
263263
LOW_REST_RATE_LIMIT_REMAINING,
264264
MAINTENANCE_RESERVED_HEADROOM,
@@ -1854,16 +1854,53 @@ export async function sweepRepoBacklogConvergence(
18541854
const sweepInstallationId = repo?.installationId ?? null;
18551855
if (sweepInstallationId == null) return;
18561856
const openPullRequests = await listOpenPullRequests(env, repoFullName);
1857-
const allCandidates = selectBacklogConvergenceCandidates({ pulls: openPullRequests });
1857+
// #9154: the exhaustion filter must run BEFORE the `max` cap, not after -- capping first (the previous
1858+
// shape: selectBacklogConvergenceCandidates already sliced to 5, THEN filtered) let five permanently
1859+
// repair-exhausted old PRs occupy every slot forever, shadowing every other PR needing convergence behind
1860+
// them. Walk the FULL oldest-open-first order and skip exhausted candidates as we go, stopping as soon as
1861+
// `max` actionable ones are found -- bounded to however far into the backlog we actually need to look, not
1862+
// the whole list, in the common case where most of the head is actionable.
1863+
//
18581864
// #orb-retry-storm (backlog-convergence half): needsSurfaceConvergence re-fires on the exact same
18591865
// lastPublishedSurfaceSha-mismatch signal as the main sweep's outage-repair priority path, but this sweeper
18601866
// had no memory of prior attempts at all -- a PR whose gate-check finalize kept failing silently got a fresh
18611867
// full re-review dispatched every ~30 minutes indefinitely. Share the same per-SHA attempt budget as the main
18621868
// sweep (isRegateRepairExhausted) rather than adding an independent cap, since both sweeps competing for the
18631869
// same stuck PR would otherwise double the wasted spend the cap exists to prevent.
1864-
const exhaustedFlags = await Promise.all(allCandidates.map((pr) => isRegateRepairExhausted(env, repoFullName, pr)));
1865-
const candidates = allCandidates.filter((_pr, index) => !exhaustedFlags[index]);
1866-
if (candidates.length === 0) return;
1870+
const orderedCandidates = sortedBacklogConvergenceCandidates(openPullRequests);
1871+
const candidates: PullRequestRecord[] = [];
1872+
let examinedCount = 0;
1873+
for (const pr of orderedCandidates) {
1874+
if (candidates.length >= BACKLOG_CONVERGENCE_SWEEP_MAX_PRS) break;
1875+
examinedCount += 1;
1876+
if (await isRegateRepairExhausted(env, repoFullName, pr)) continue;
1877+
candidates.push(pr);
1878+
}
1879+
if (candidates.length === 0) {
1880+
// Every candidate examined was repair-exhausted -- the sweep is returning early with nothing to show for
1881+
// it. Previously silent (the caller just saw an empty array and bailed); log + audit so a permanently
1882+
// wedged head of the backlog is visible instead of masquerading as "nothing needs convergence".
1883+
if (orderedCandidates.length > 0) {
1884+
console.warn(
1885+
JSON.stringify({
1886+
level: "warn",
1887+
event: "backlog_convergence_sweep_all_exhausted",
1888+
repository: repoFullName,
1889+
examined: examinedCount,
1890+
totalCandidates: orderedCandidates.length,
1891+
}),
1892+
);
1893+
await recordAuditEvent(env, {
1894+
eventType: "agent.sweep.backlog_convergence",
1895+
actor: "loopover",
1896+
targetKey: repoFullName,
1897+
outcome: "denied",
1898+
detail: `backlog-convergence sweep found ${orderedCandidates.length} open PR(s) needing convergence, all repair-exhausted`,
1899+
metadata: { repoFullName, examined: examinedCount, totalCandidates: orderedCandidates.length },
1900+
});
1901+
}
1902+
return;
1903+
}
18671904
// Stamp the backlog-convergence draining marker for EVERY candidate NOW, at dispatch — not in the downstream
18681905
// per-PR job (#4502, mirrors #audit-sweep-dispatch-stamp). This makes getLatestBacklogConvergenceRegatedAt
18691906
// reflect this sweep immediately, so fanOutBacklogConvergenceSweepJobs's in-flight guard skips re-arming this

src/selfhost/backlog-convergence.ts

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -32,25 +32,39 @@ export function needsSurfaceConvergence(pr: Pick<PullRequestRecord, "headSha" |
3232
}
3333

3434
/**
35-
* Select the open PRs a single repo's backlog-convergence sweep should re-enqueue: drop drafts and anything
36-
* whose surface is already published at the current head, then take the `max` PRs that have been open longest
37-
* (oldest `createdAt` first, falling back to the epoch so a PR with no known creation time still sorts
38-
* deterministically rather than being silently dropped) — this is the explicit "oldest open PRs first" fairness
39-
* ordering the backlog-drain lane depends on (see queue-fairness.ts, PR2). Ties broken by PR number. Pure +
40-
* deterministic: same inputs -> same ordered batch.
35+
* Every open PR a repo's backlog-convergence sweep considers, in the order it should serve them: drop drafts
36+
* and anything whose surface is already published at the current head, then order oldest-open-first (oldest
37+
* `createdAt` first, falling back to the epoch so a PR with no known creation time still sorts
38+
* deterministically rather than being silently dropped) — this is the explicit "oldest open PRs first"
39+
* fairness ordering the backlog-drain lane depends on (see queue-fairness.ts, PR2). Ties broken by PR number.
40+
* Deliberately UNSLICED (#9154): selectBacklogConvergenceCandidates below applies the sweep's `max` cap
41+
* directly to this order, which shadows every PR behind the first `max` whenever any of THOSE are
42+
* permanently repair-exhausted (a caller that also needs to skip exhausted candidates -- see
43+
* sweepRepoBacklogConvergence in processors.ts -- must walk this full order and exclude exhausted PRs BEFORE
44+
* capping, not after). Pure + deterministic: same inputs -> same ordered list.
4145
*/
42-
export function selectBacklogConvergenceCandidates(input: {
43-
pulls: PullRequestRecord[];
44-
max?: number;
45-
}): PullRequestRecord[] {
46-
const max = input.max ?? BACKLOG_CONVERGENCE_SWEEP_MAX_PRS;
46+
export function sortedBacklogConvergenceCandidates(pulls: PullRequestRecord[]): PullRequestRecord[] {
4747
const ageKey = (pr: PullRequestRecord): number => {
4848
const created = pr.createdAt ? Date.parse(pr.createdAt) : Number.NaN;
4949
return Number.isFinite(created) ? created : 0;
5050
};
51-
return input.pulls
51+
return pulls
5252
.filter((pr) => pr.state === "open" && !pr.isDraft)
5353
.filter((pr) => needsSurfaceConvergence(pr))
54-
.sort((a, b) => ageKey(a) - ageKey(b) || a.number - b.number)
55-
.slice(0, Math.max(0, max));
54+
.sort((a, b) => ageKey(a) - ageKey(b) || a.number - b.number);
55+
}
56+
57+
/**
58+
* Select the open PRs a single repo's backlog-convergence sweep should re-enqueue: the first `max` PRs (by
59+
* default BACKLOG_CONVERGENCE_SWEEP_MAX_PRS) from sortedBacklogConvergenceCandidates' full oldest-open-first
60+
* order. This convenience wrapper caps WITHOUT regard to repair-exhaustion — fine for a caller that doesn't
61+
* need that filter, but see sortedBacklogConvergenceCandidates' doc comment for why sweepRepoBacklogConvergence
62+
* itself calls that function directly instead. Pure + deterministic.
63+
*/
64+
export function selectBacklogConvergenceCandidates(input: {
65+
pulls: PullRequestRecord[];
66+
max?: number;
67+
}): PullRequestRecord[] {
68+
const max = input.max ?? BACKLOG_CONVERGENCE_SWEEP_MAX_PRS;
69+
return sortedBacklogConvergenceCandidates(input.pulls).slice(0, Math.max(0, max));
5670
}

src/selfhost/maintenance-admission.ts

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,13 @@ export interface MaintenancePressureSignals {
9696
* keeps finding stale work every sweep). This is the field evaluateMaintenanceAdmission's live_pending_high
9797
* check actually gates on. */
9898
liveRunnableNowCount: number;
99-
/** Age in ms of the oldest genuinely-active (processing, or pending AND due) foreground job -- null when
100-
* none qualifies right now. Distinct from oldestLivePendingAgeMs, which is dominated by a job intentionally
101-
* scheduled far in the future and says nothing about how long already-active work has sat unclaimed/running.
102-
* This is the field evaluateMaintenanceAdmission's live_job_age_high check actually gates on. */
99+
/** Age in ms of the oldest DUE-AND-UNCLAIMED (status='pending', run_after<=now) foreground job -- null when
100+
* none qualifies right now. Deliberately EXCLUDES 'processing' rows (#9155): a job actively being worked (a
101+
* normal in-flight AI review routinely takes minutes) must not, by merely running, trip live_job_age_high
102+
* and collapse the entire maintenance lane -- including the watchdog/alerter that would report an actual
103+
* overload -- to the 4-hour trickle backstop. Distinct from oldestLivePendingAgeMs, which is dominated by a
104+
* job intentionally scheduled far in the future and says nothing about how long work has sat unclaimed. This
105+
* is the field evaluateMaintenanceAdmission's live_job_age_high check actually gates on. */
103106
oldestLiveRunnableAgeMs: number | null;
104107
maintenancePendingCount: number;
105108
oldestMaintenancePendingAgeMs: number | null;
@@ -129,6 +132,14 @@ export interface MaintenanceAdmissionConfig {
129132
deferMs: number;
130133
maxDeferAgeMs: number;
131134
maintenanceDrainAgeMs: number;
135+
/** #9155: how long a computed MaintenancePressureSignals snapshot may be reused across successive claim
136+
* attempts before it must be recomputed. A denied maintenance job returns `true` from processOne(), so the
137+
* pump's drain loop immediately claims the next due maintenance row and re-evaluates admission -- without
138+
* this, a burst of N due maintenance rows means 4N sequential aggregate scans in one tight loop (more
139+
* denials -> more scans -> higher DB/host load -> higher hostLoadAvg1PerCore -> more denials, a positive
140+
* feedback loop). A short TTL, not "once per drain pass": pressure is still re-measured often enough to
141+
* react to a genuinely changing queue instead of latching a stale reading for a whole burst. */
142+
pressureSignalsCacheTtlMs: number;
132143
}
133144

134145
const DEFAULT_MAX_LIVE_PENDING_COUNT = 5;
@@ -143,6 +154,9 @@ const DEFAULT_MAX_BACKLOG_CONVERGENCE_PENDING_COUNT = 10;
143154
const DEFAULT_DEFER_MS = 3 * 60_000;
144155
const DEFAULT_MAX_DEFER_AGE_MS = 4 * 60 * 60_000;
145156
const DEFAULT_MAINTENANCE_DRAIN_AGE_MS = 10 * 60_000;
157+
// #9155: within the suggested 1-2s memoization window -- long enough to collapse a burst of denials sharing
158+
// one drain pass, short enough that a real pressure change is still visible within a couple of poll ticks.
159+
const DEFAULT_PRESSURE_SIGNALS_CACHE_TTL_MS = 1_500;
146160

147161
function maintenanceAdmissionEnabled(): boolean {
148162
const raw = (process.env.MAINTENANCE_ADMISSION_ENABLED ?? "").trim().toLowerCase();
@@ -195,6 +209,10 @@ export function resolveMaintenanceAdmissionConfig(): MaintenanceAdmissionConfig
195209
// Never longer than the trickle backstop itself -- a misconfigured drain age above maxDeferAgeMs would be a
196210
// no-op (the trickle would always win first), so clamp it down rather than let it silently do nothing.
197211
maintenanceDrainAgeMs: Math.min(requestedDrainAgeMs, maxDeferAgeMs),
212+
pressureSignalsCacheTtlMs: parsePositiveIntEnv("MAINTENANCE_ADMISSION_PRESSURE_CACHE_TTL_MS", {
213+
min: 0,
214+
fallback: DEFAULT_PRESSURE_SIGNALS_CACHE_TTL_MS,
215+
}),
198216
};
199217
}
200218

0 commit comments

Comments
 (0)