Public: anyone can read this; the crew can change it.
Sentinel: Crew runs in trevato/relay keep failing at the push or the sandbox #3113
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3106-sentinel-crew-runs-in-trevato-relay-keep → main 4 commits fork 4 threads · 4 unresolved
src/crew/network.ts +9
| @@ -22,6 +22,15 @@ export function isNetworkError(outcome: ClaudeOutcome): boolean { | ||
| 22 | 22 | return NETWORK.test(outcome.result) || NETWORK.test(outcome.reason); |
| 23 | 23 | } |
| 24 | 24 | |
| 25 | /** | |
| 26 | * Whether a summary is the run's own wording for an outage it retried: it | |
| 27 | * leads with "no changes pushed:" and then names the network. A bare | |
| 28 | * "push failed: ... ECONNREFUSED" is a blocked push and does not match. | |
| 29 | */ | |
| 30 | export function isOutageSummary(summary: string): boolean { | |
| 31 | return /^\s*no changes pushed:/i.test(summary) && NETWORK.test(summary); | |
| 32 | } | |
| 33 | ||
| 25 | 34 | /** Resolves after `ms`, or at once when `signal` aborts. */ |
| 26 | 35 | export function pause(ms: number, signal: AbortSignal): Promise<void> { |
| 27 | 36 | return new Promise((resolve) => { |
src/crew/sentinel.ts +4
| @@ -33,6 +33,7 @@ import { createLog, recentErrors, redactSecrets } from "../kernel/log.ts"; | ||
| 33 | 33 | import { spans } from "../kernel/degraded_history.ts"; |
| 34 | 34 | import { QUEUE_WAIT_MAX_MS } from "../kernel/vitals.ts"; |
| 35 | 35 | import { STALE_AFTER_MS } from "../runs/lease.ts"; |
| 36 | import { isOutageSummary } from "./network.ts"; | |
| 36 | 37 | |
| 37 | 38 | const log = createLog("sentinel"); |
| 38 | 39 | |
| @@ -334,9 +335,12 @@ export function runStreaks(db: Database): Finding[] { | ||
| 334 | 335 | } else { |
| 335 | 336 | // A decline is the run's own decision ("I pushed nothing"), not a push that failed, |
| 336 | 337 | // whether or not the summary leads with it. |
| 338 | // An outage the runner retried ("no changes pushed: ... EAI_AGAIN") is not a | |
| 339 | // push that was refused; a "push failed: ... ECONNREFUSED" still counts. | |
| 337 | 340 | const pushes = failed.filter( |
| 338 | 341 | (r) => |
| 339 | 342 | !/\bdeclined:/i.test(r.summary) && |
| 343 | !isOutageSummary(r.summary) && | |
outdated
NETWORK includes ECONNREFUSED, ENOTFOUND, ETIMEDOUT, "unable to connect", "connection error" and "fetch failed". A push that the sandbox or egress policy blocks produces exactly these strings, and this line now drops it from the push fault. Scope the exemption to summaries that start with "no changes pushed:", the run's own wording for an outage it retried. A bare "push failed: ..." must keep counting. |
||
| 340 | 344 | /\bpush|sandbox|operation not permitted/i.test(r.summary), |
| 341 | 345 | ); |
| 342 | 346 | if (pushes.length < STREAK) continue; |
test/sentinel.test.ts +43
| @@ -122,6 +122,49 @@ describe("a decline that does not lead the summary", () => { | ||
| 122 | 122 | }); |
| 123 | 123 | }); |
| 124 | 124 | |
| 125 | describe("a dropped connection is not a failed push", () => { | |
| 126 | test("runs that ended on EAI_AGAIN file no push fault", async () => { | |
| 127 | const made = await createRepo(app, alice, { name: "offline", visibility: "private" }); | |
| 128 | if (!made.ok) throw new Error(String(made.error)); | |
| 129 | for (let n = 0; n < 5; n++) { | |
| 130 | const run = createRun(app.db, { repoId: made.value.id, kind: "agent" }); | |
| 131 | app.db | |
| 132 | .query("UPDATE run SET state = 'failed', summary = ?2, created_at = ?3 WHERE id = ?1") | |
| 133 | .run( | |
| 134 | run.id, | |
| 135 | "no changes pushed: API Error: Can't reach the API server — check your internet or DNS (EAI_AGAIN) (api_error)", | |
| 136 | ago((30 + n) * HOUR), | |
| 137 | ); | |
| 138 | } | |
| 139 | const keys = findFaults(app.db, NOW).map((f) => f.key); | |
| 140 | expect(keys).not.toContain(`push:${made.value.id}`); | |
| 141 | }); | |
This test covers only the EAI_AGAIN case. Add the counter-case: five "push failed: ... ECONNREFUSED" or "unable to connect" runs still raise |
||
| 142 | }); | |
| 143 | ||
| 144 | describe("a push blocked on the network is still a push fault", () => { | |
| 145 | test("push failed with ECONNREFUSED or unable to connect raises it", async () => { | |
| 146 | const made = await createRepo(app, alice, { name: "walled", visibility: "private" }); | |
| 147 | if (!made.ok) throw new Error(String(made.error)); | |
| 148 | const lines = [ | |
| 149 | "push failed: unable to connect to forge", | |
| 150 | "push failed: ECONNREFUSED", | |
| 151 | "push failed: unable to connect to forge", | |
| 152 | "push failed: ECONNREFUSED", | |
| 153 | "push failed: unable to connect to forge", | |
| 154 | ]; | |
| 155 | lines.forEach((line, n) => { | |
| 156 | const run = createRun(app.db, { repoId: made.value.id, kind: "agent" }); | |
| 157 | app.db | |
| 158 | .query("UPDATE run SET state = 'failed', summary = ?2, created_at = ?3 WHERE id = ?1") | |
| 159 | .run(run.id, line, ago((30 + n) * HOUR)); | |
| 160 | }); | |
| 161 | const keys = findFaults(app.db, NOW).map((f) => f.key); | |
| 162 | expect(keys).toContain(`push:${made.value.id}`); | |
| 163 | // Later suites count the day's faults; leave none of this one behind. | |
Cleanup runs only if the expects pass. Move it to a finally or afterEach so one failure here does not cascade into the suites that count the day faults. |
||
| 164 | app.db.query("DELETE FROM run WHERE repo_id = ?1").run(made.value.id); | |
| 165 | }); | |
| 166 | }); | |
| 167 | ||
| 125 | 168 | describe("a failed push that pushed nothing", () => { |
| 126 | 169 | test("still raises the push fault", async () => { |
| 127 | 170 | const made = await createRepo(app, alice, { name: "blocked", visibility: "private" }); |
The doc comment says "not a refused push", but the function only tests the regex and cannot tell the two apart. Either make it check the leading "no changes pushed:" or rename it and say what it matches.