Skip to content

P0: main CI red — Deploy MCP failure on d6c148b #2969

Description

@github-actions

P0: main went red

Workflow: Deploy MCP concluded failure on main.

Failing run: https://github.com/edobry/minsky/actions/runs/31724074028
HEAD SHA: d6c148b31178c732bea5f127f28a1c7e7f67085a
Head commit: fix(mt#4103): Bound every boot-reconciliation await, and make every boot say what happened

Summary

loadPersistedDrivenSessions is fire-and-forget from start-command.ts — nothing awaits it and
nothing times it out. So an await that never settles there does not fail; it simply never finishes,
leaving the driven-session registry empty for the daemon's whole life while every one of the
function's four log lines stays unwritten.

That is not hypothetical. Measured in cockpit-daemon1.log on 2026-08-12: ten boots, and a
boot reconciliation: loaded N line for only seven of them. On the other two — 20:40:13 and
20:55:23 — none of the four outcomes the function can emit appears (loaded N;
listNonTerminalDrivenSessions: failed; no SQL persistence available at boot;
boot reconciliation failed — all three failure strings grep to zero across the entire file). The
resulting empty registry at 20:41 is what let an entity thread silently swap its agent, which was
mt#4093.

What was actually unbounded — four awaits, not one

The task spec named getContextInspectorDb because it also latches. Re-reading the function during
planning found three more, two of them per row:

  1. await getContextInspectorDb() — resolving the handle.
  2. await listNonTerminal(db) — the SELECT.
  3. await probeSpawnCwdAsync(row.cwd) — a bare await stat(cwd). Its own error classifier
    explicitly anticipates "a hung mount's ETIMEDOUT", so a stalling filesystem was a known case —
    but nothing bounded the WAIT, only the error, and a stat on a wedged mount can block without
    ever erroring.
  4. await persistUnrecoverableVerdict(...) — a DB write, per unrecoverable row.

All four are now raced against a bound. The per-row ones degrade that row, not the run.

Key changes

  • ReconciliationOutcome + describeReconciliationOutcome. The run returns a value; a pure
    function turns it into one line and a level. The caller does log[level](message)
    unconditionally, so exactly one line is emitted on every path — including the zero-rows case,
    which previously logged nothing at all.
  • Per-stage naming. A stall reports WHICH stage: resolve-db points an operator at the pool,
    cwd-probe at a wedged filesystem path. "Reconciliation timed out" would send them to the wrong
    place.
  • A caller-scoped cacheNegative: false handle. getBootReconciliationDb replaces the borrowed
    shared getter for this caller only.

Judgment calls

  1. A caller-scoped getter, not a global flip. getContextInspectorDb has ~25 non-test call
    sites across cockpit routes and widgets, and routes/conversation-run-state.ts documents its
    latching as a deliberate contrast — so flipping it would change all of them as a side effect.
    getEntityThreadDb is the established precedent for exactly this move. Verified cheap:
    createCachedSqlDbGetter resolves through the shared getCachedPersistenceProvider, so this is
    a second cache over one pool, not a second pool.
  2. Bounds grounded in observed cadence, not round numbers. A healthy reconciliation completed
    0.6s after PersistenceService initialized on the boots that logged one. 15s for whole-run
    stages is ~25x that; 2s for per-row stages, because those run N times and a shared budget would
    let twenty slow rows consume the run.
  3. raceAgainstTimeout, not a hand-rolled Promise.race. packages/shared/src/timeout.ts
    already exists, is dependency-free, returns a discriminated result rather than throwing (which
    suits reporting a stage), and carries an injectable signal that makes the stall branches testable
    in microseconds. Chose it over the sibling withDeadline, which throws.
  4. The per-row verdict write is bounded AFTER registry.register. A stalled write then costs
    the durable verdict — which the next boot re-derives — and not the registration, which is the
    entire point of reconciliation.
  5. A cwd-probe timeout fails OPEN. It degrades to the same verdict a permission/IO error already
    produces: not-missing, so the row stays reconnecting rather than being retired on a transient
    fault. A timeout is precisely the "cannot tell" case that posture exists for.

Scope held

The class is "an unbounded await in a fire-and-forget boot step". This PR covers every one inside
loadPersistedDrivenSessions. start-command.ts has a sibling with the same posture
(startSseBrokerWarmup) which is NOT in this task's scope and is not touched — naming it rather
than silently widening.

Testing

Execution evidence:

$ bun test --preload ./tests/setup.ts --timeout=15000 src/cockpit/driven-session-launch-persistence.test.ts
 57 pass
 0 fail
 131 expect() calls
Ran 57 tests across 1 file. [116.00ms]

AT1 (a getDb seam that never resolves settles within its bound and names the stage) — "AT1 — a
handle that never resolves settles as a resolve-db stall", asserting the exact outcome value
{kind: "timed-out", stage: "resolve-db", timeoutMs: 15000}. A sibling test does the same for
list-rows. AT2 (zero rows emits a distinguishable outcome; today: nothing) — "AT2 — zero rows
is reported as empty, distinct from a stall", plus a pure test asserting the rendered line says
"no non-terminal sessions to load". AT3 (a seam that fails once then succeeds resolves a real
handle rather than the latched null) — the retry behavior of cacheNegative: false is already
covered by db-providers.test.ts's "retries the probe on every call until success"; what this PR
adds is the WIRING assertion that this caller does not borrow the latching singleton ("AT3 — boot
reconciliation does NOT borrow the latching shared getter"). AT4 (one wedged cwd probe still
lets the other rows register) — "AT4 — one wedged cwd probe degrades that row, not the run", which
asserts both rows are in the registry, the outcome is loaded with degraded naming cwd-probe,
and the wedged row stayed reconnecting rather than being retired.

Five more tests cover the describer: every outcome kind produces a non-empty line (the guarantee the
unconditional log[level] call depends on), degraded stages are deduped, and a stall names both the
stage and the bound.

Negative control — reverted the resolve-db bound to a bare await and removed the empty outcome,
then re-ran at --timeout=3000:

(fail) ... > AT1 — a handle that never resolves settles as a resolve-db stall [3000.00ms]
  ^ this test timed out after 3000ms.
(fail) ... > a SELECT that never resolves settles as a list-rows stall [3000.01ms]
  ^ this test timed out after 3000ms.
(fail) ... > AT2 — zero rows is reported as `empty`, distinct from a stall [0.88ms]
(fail) ... > AT4 — one wedged cwd probe degrades that row, not the run [3000.91ms]
  ^ this test timed out after 3000ms.
(fail) ... > the loader reports 0 for every non-loaded outcome [3000.65ms]
  ^ this test timed out after 3000ms.
 52 pass
 5 fail
Ran 57 tests across 1 file. [12.14s]

Worth reading the FORM of that failure: four of the five fail by hanging rather than by
asserting wrong, which is the production defect exactly — an unbounded await does not fail, it just
never returns. AT2 fails cleanly on the value.

A note on the test design, since it was got wrong once: an injected timeout signal that fires
immediately on every race is useless here — the stages share one seam in sequence, so it always
trips resolve-db and no later stage is ever reached. tripRace(n) trips the Nth race and never
any other, which is what lets a test name the exact await it is stalling.

Live verification

Not applicable, and not deferred: this changes an in-process boot step with no external system, no
render path, and no new integration surface. The stall branches cannot be exercised against a live
daemon without an actual wedged mount or a hung pool — which is precisely why they are driven
through an injected signal instead. The observable production change is a log line per boot, which
§10's deploy check below will show.

Deploy verification

Deploy verification: no deploy-surface file is touched (no infra/**, no Dockerfile, no
railway.json, no deploy workflow, no migration). The post-merge check is that the cockpit service
deploys clean and its boot emits exactly one reconciliation line.

Task: mt#4103 · Sibling: mt#4093 (merged) · Neighbour: mt#4100

Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

What this means

A push to main triggered CI and the workflow above did not conclude success. Per
CLAUDE.md user preference ("main must never be broken"), this is severity-1.

Diagnostic checklist

  1. Open the failing run URL above; identify which job/step failed.
  2. Check whether the offending PR was merged with a known-failing required check
    (operator-API bypass via gh api PUT /merge despite enforce_admins).
  3. Confirm enforce_admins is currently enabled:
    gh api repos/edobry/minsky/branches/main/protection --jq .enforce_admins.enabled
    
    Expected: true post-mt#1938. If false, that is itself a separate finding.

Recovery

  1. Open a hotfix branch off current main.
  2. Apply the smallest fix that turns CI green (often a formatter pass or a config
    flip).
  3. Land via the standard Minsky session flow:
    tasks_create → session_start → session_commit → session_pr_create → /review-pr → session_pr_merge.
  4. Verify the post-merge main build is green within ~5 minutes.
  5. Close this issue with a link to the hotfix PR.

Cross-references

Metadata

Metadata

Assignees

No one assigned

    Labels

    main-redAuto-filed by .github/workflows/main-watch.yml when main CI fails (mt#1938)p0Severity 0: production breakage requiring immediate attention

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions