Queue fairness: per-fit-class queues, a queue-wait metric, and NOTIFY-driven scanning

[nikhilunni]

Closes #537

Summary

Three mechanical changes to the ADR 0048 session-queue scanner, per the issue's design — no schema migration, no wire/proto change, no SandboxBackend change:

1. Per-fit-class queues (crates/engram-coordinator/src/queue_scanner.rs): the scanner now partitions the FIFO-ordered queue by (mem_budget_mib, cpu_budget_vcpus) — the exact 2D fit place_queued_session uses — and sweeps each class independently, oldest-head-first. A create's NoCapacity stops only its class; a resume's "zero schedulable hosts fleet-wide" still stops the whole sweep (the one legitimate global case). queued_demand/engram_sessions_queued{,_mib} are untouched.
2. Queue-wait metric (crates/engram-coordinator/src/metrics.rs): new engram_queue_wait_seconds histogram (origin=create/resume, outcome=placed/timeout) and engram_queue_head_age_seconds gauge, with a dedicated minutes-scale bucket override.
3. NOTIFY-driven scanning (crates/engram-postgres/src/lib.rs, crates/engram-coordinator/src/pg_listener.rs, lib.rs): a new placement_changed PG NOTIFY fires at 5 discrete placement-feasibility sites (enqueue_session_create/enqueue_session_resume, upsert_host, set_host_cordoned(false), transition_session leaving a memory-reserving state, delete_pending_session). The scanner now parks on a shared Arc<Notify> that pg_listener wakes; ENGRAM_QUEUE_POLL_SECS becomes a 30s fallback (was the 5s primary drive) and a new ENGRAM_QUEUE_RETRY_SECS (default 5s) re-arms a short retry after a sweep that requeued/errored a session.

Acceptance criteria

• HOL is gone (headline) — hol_break_is_per_class_not_global (live-PG): an unfittable older session no longer blocks a fitting newer session of a different class in the same sweep.
• Per-class FIFO preserved — per_class_fifo_head_block_is_scoped_to_its_class: a same-class second member stays blocked behind an unfittable head; a third, smaller class still places the same sweep.
• Autoscaler signal unchanged — asserted inline (queued_demand still counts the blocked head).
• Queue wait measurable — engram_queue_wait_seconds/engram_queue_head_age_seconds wired at the 3 record sites (placed-create, placed-resume, timeout) + head-age gauge in sample_queue_metrics. Not scraped end-to-end in a test (no Prometheus exporter installed in the live-PG harness); verified instead via the durable queue_timeout event's waited_secs payload, which is the same value fed to the histogram — see create_origin_timeout_fails_session_and_records_wait. Flagging as needs-prod-verification for the actual scrape/dashboard.
• Push-driven dequeue — notify_placement_changed_fires_at_every_site (raw sqlx::PgListener observes one placement_changed per site) + scanner_wakes_on_notify_and_places_within_the_wake_not_the_fallback (a real scanner+listener pair, 120s fallback poll, places within 10s of a freeing event).
• Bounded regression, stated — documented in the queue_scanner module doc (30s fallback / 5s retry re-arm / first-sweep startup delay).
• Not claimed: raw capacity waits improve — no code or doc claims this; out of scope per the issue.

Deviations / drift from the issue's anchors

• All file:line anchors in the issue (verified against f6602259) were re-verified against this branch's base (42ed9bb2, a few commits later) and matched almost exactly — no drift worth calling out.
• queue_scanner::run_once is pub (issue anticipated pub(crate)) — the new live-PG tests live in tests/ (a separate crate, matching every other live-PG test in this file), so pub(crate) wouldn't be visible there. Documented in the fn's own doc comment and the test file's header.
• Test isolation: the shared-Postgres, no-cleanup convention already used across this crate's live-PG tests interacts badly with tests that sweep the entire queue table (mine are the first to call run_once/spawn a real scanner). Worked around with cordon_unmeasured_hosts (neutralizes choose_placement_host's "unmeasured host fits any budget" fallback tier being tripped by other tests' zero-allocatable_mib leftovers), unique_fitting_budget_in (per-session-id-derived budgets, disjoint per-test sub-ranges — see the post-review fixups below) so "fits" sessions don't collide with other tests' round fixed budgets or each other, and a noise-tolerant wait_for_reason for the NOTIFY test (the placement_changed channel is global). All discovered and fixed via repeated local runs against the dev Postgres at the exact --test-threads=1 invocation CI uses (ci.yml's "Postgres-gated ignored tests" step) — 3/3 clean runs of the full 17-file live-PG group after the fix.
• Updated ADR 0048's now-stale "FIFO head-of-line blocking is deliberate" invariant + the queue-scanner intro bullet to describe the per-class shape and the NOTIFY-driven dequeue, per CLAUDE.md's "keep it accurate" directive — not requested by the issue but a direct factual contradiction otherwise.
• Commit granularity (undeclared until this edit; review finding 5, CONFIRMED-convention): the issue's own landing plan calls for one commit per separable step (steps 1-2, 3, 4-6, 7); this PR fused steps 1-6 — the pure partition function, the inert metrics constants, and the NOTIFY plumbing (the riskiest piece: it changes prod dequeue-latency characteristics) — into a single commit (dc9a5d07), so a bisect/revert of the NOTIFY wiring alone isn't possible without also reverting per-class partitioning. Declaring it here per the review rather than rewriting already-pushed history.

Post-review fixups (deep review, findings 1–5)

A deep review pass (see PR comments) found 4 inline findings + 1 process finding. All addressed on top of the original 2 commits:

1. [PLAUSIBLE, flake risk] Budget collision between hol_break's leftover B and per_class_fifo's y1 — unique_fitting_budget drew every "definitely fits" class from the same 1024..=1535 MiB range; a never-cleaned-up leftover B could collide with y1 on a later local rerun (~1/512) and steal its host. Fixed: disjoint per-test sub-ranges via unique_fitting_budget_in(id, base).
2. [CONFIRMED, cleanup] Misleading "runs in parallel" header — the queue-fairness test-file header claimed nextest's default parallel-per-process execution, contradicting cordon_unmeasured_hosts's own doc comment 115 lines later (which correctly says CI serializes via --test-threads=1) — and the new 1ms-timeout tests are actually destructive whole-queue sweeps that require that serialization for correctness, not just determinism. Fixed: rewrote the header to state the real contract.
3. [CONFIRMED, undeclared behavior change] Dropped skip-the-first-tick startup guard — the NOTIFY rewrite made run_once fire immediately at coordinator boot instead of after a beat, same regression evac_resumer::spawn deliberately guards against ("give hosts time to heartbeat in"). Fixed: restored the delay, kept it interruptible by a real NOTIFY (so scanner_wakes_on_notify_and_places_within_the_wake_not_the_fallback still holds).
4. [PLAUSIBLE, nit] Unconditional NOTIFY on a no-op resume enqueue — enqueue_session_resume fired placement_changed even when its status='idle'-gated UPDATE matched 0 rows (a racing resume no-op), waking every scanner replica for nothing; inconsistent with delete_pending_session's existing rows_affected() > 0 guard. Fixed: added the same guard.
5. [CONFIRMED-convention] Commit granularity — declared above rather than rewriting already-pushed commits.

Also folded in a test-fixture-dedupe pass (five of the six new live-PG tests repeated the same connect+cordon+build_app_state preamble verbatim; extracted a setup(cordon: bool) helper) flagged during the same review.

Test plan

• cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, cargo hakari verify — all clean.
• cargo nextest run --workspace — clean except 3 pre-existing, unrelated engram-host-agent chunk-cache write-through failures (tracked separately as Regression on main: chunk write-through cache not populated (3 test failures post-#522) #552; confirmed not touched by this diff — no engram-host-agent files in the changeset).
• New unit tests (partition_queue — ordering, class grouping, oldest-head-first, empty) run in-crate, no PG needed.
• New live-PG tests in crates/engram-coordinator/tests/queue_scanner_live_pg.rs — already wired into CI at ci.yml:367 (--test queue_scanner_live_pg, --run-ignored ignored-only, --test-threads=1); no workflow changes needed. Verified 3x clean against the full 17-file "Postgres-gated ignored tests" group locally at the exact CI invocation.
• No .sqlx/ changes needed (engram-postgres uses runtime sqlx::query, no query! macros).
• No migration — queued_at/queue_origin/queue_prompt already exist (migration 0064); the fit-class key is derived from existing columns.

Landing order: #541 -> #536 -> #533 -> #527p1 -> #528 -> #530 -> #537 -> #540 -> #539 -> #526 -> #531 -> #538 -> #529 -> #535

🤖 Generated with Claude Code

https://claude.ai/code/session_01T4WkZddtVw8djsWz2RCcQc

[nikhilunni]

Deep review of the queue-fairness implementation (issue #537). Overall: solid — faithful to the spec, mergeable after considering a couple of test-hygiene and undeclared-deviation nits. The three mechanical changes land exactly as specified: FitClass + pure partition_queue with a proven oldest-head-first ordering, class-scoped NoCapacity breaks with resume's zero-schedulable-hosts case correctly kept global (verified against placement.rs — candidates_for/host_passes_filters filters schedulability, not fit, so the claimed semantics hold), the histogram/gauge at the specified record moments, and the placement_changed NOTIFY at all five sites feeding a shared Arc<Notify> with the 30s fallback + 5s retry re-arm. No type/lint laundering; self-wake loops are structurally avoided (requeue_session is deliberately not a notify site). Surviving findings are concentrated in live-PG test hygiene plus two undeclared deviations, all below.

CI

Green. All lanes pass (CI Gate ✅, including the FC lane at 13m23s and the Postgres-gated ignored-tests step that runs the new queue_scanner_live_pg tests at --test-threads=1). Nothing to diagnose.

Issue compliance

Substantively complete. (a) Per-fit-class queues: partition + per-class FIFO-stop as specified; create NoCapacity breaks only its class, resume Some(false) breaks the whole sweep with the required why-comment; timeout/lease/JoinSet/boot-concurrency semantics preserved; queued_demand untouched; module doc rewritten with the mandated starvation note. (b) Metrics: both constants, the exact bucket override, all three record sites (durable queued→pending flip, post-transition resume dequeue, time_out_session), head-age gauge from the fetched list, no unbounded labels. (c) NOTIFY: all five sites with the spec table's conditions and payloads (note: the unconditional fire at enqueue_session_resume is what the spec's table says — "always" — see finding 4 for the nit), best-effort let _ = post-commit, pg_listener arm + updated info!, Arc<Notify> threaded honestly through both spawn signatures, 5→30s poll default, ENGRAM_QUEUE_RETRY_SECS re-arm via RunSummary. Tests are wired at ci.yml:367 (unchanged) and green. Declared deviations (run_once pub, ADR 0048 doc edit) are justified. Undeclared deviations: (1) commit granularity — the spec's "one logical commit per step where separable (steps 1-2, 3, 4-6, 7)" collapsed into two commits (steps 1–6 in one); (2) the old scanner's deliberate skip-the-first-tick startup behavior was dropped (finding 3). Neither is mentioned in the PR body.

Findings

See inline comments (findings 1–4 anchored to the diff). Finding 5 (no clean diff anchor):

5. [CONFIRMED — convention] Commit granularity collapsed the issue's seams. The issue says "one logical commit per step where separable (steps 1-2, 3, 4-6, 7 are natural commit seams)"; the PR has two commits, with the scanner partition, the inert metrics constants, and the NOTIFY plumbing (the riskiest piece — it changes prod dequeue-latency characteristics) fused into one. A bisect/revert of the NOTIFY wiring can't be separated from the pure partition function. If you keep the two-commit shape, at least declare the deviation in the PR body.

Automated deep review (core-ops batch); findings verified against the branch — treat PLAUSIBLE items as questions.

[nikhilunni]

1. [PLAUSIBLE — flake risk, local reruns] A leftover requeued session from hol_break_is_per_class_not_global can steal this exactly-sized host when its random budget collides with y1's.

The HOL test's B gets placed, its boot fails (prepare_from_row — no enabled_images) and boot_placed_create requeues it; run_once awaits the boot JoinSet, so B is durably back in queued when that test ends, and the suite never cleans up. unique_fitting_budget draws from only 512 values (1024 + (low % 512), cpu always 1), so a leftover B collides with y1's class at ~1/512 per leftover. When they collide, B is the older class head and places first.

I verified this is benign in CI: the run is serial (--test-threads=1, fresh service Postgres per run) and placement_ttl() is 60s, so the HOL test's own exactly-b_mem-sized host is still schedulable seconds later — y1 lands on it even if B takes this host. The failure needs a local rerun against a shared dev Postgres 60s–30min after a previous run (leftover B still queued, its old host stale-filtered by the 60s TTL): then B consumes this host and the y1 must-place assertion fails.

Cheap fix: give each test a disjoint budget sub-range (e.g. HOL 1024..1535, this test 2048..2559 with the host sized to match) so cross-test collision is impossible by construction.

[nikhilunni]

2. [CONFIRMED — cleanup] This header claims parallel per-process execution, but the new 1ms-timeout tests are destructive whole-queue sweeps that are only safe under CI's serialized invocation — and this same file says so 115 lines later.

create_origin_timeout_fails_session_and_records_wait / resume_origin_timeout_returns_to_idle run run_once with timeout = 1ms, which times out (Failed / Idle) every queued row in the shared database, not just their own — run_once checks the timeout per row across all classes before any placement. CI is safe only because ci.yml:372 passes --test-threads=1 (which the cordon_unmeasured_hosts doc comment at ~line 371 correctly states, contradicting this line). A developer who trusts this header and runs the suite with nextest's default parallelism can have a timeout test brick hol_break's session A (asserted Queued after its own run_once) or any other live-pg file's in-flight queued row.

Fix: correct this comment to state the --test-threads=1 contract (and delete the "in parallel" rationale), or make the timeout tests non-destructive (not obviously possible with run_once's whole-queue sweep — the comment fix is the honest one).

[nikhilunni]

3. [CONFIRMED — undeclared behavior change] The rewritten loop drops main's deliberate tick.tick().await; // skip the immediate first tick — run_once now executes immediately at coordinator boot instead of after the first poll interval.

Main's spawn (old queue_scanner.rs:80-82) explicitly skipped the immediate tick; this loop runs run_once first and only then parks on the select!. (The spec's wire-up phrasing — "replace the interval with tokio::select! {...} then run_once" — reads as wait-first, too.) Effect: on every coordinator (re)start, requeue_stale_pending + a full queue sweep + boot spawns run concurrently with the rest of startup, on every replica of a rolling deploy. Likely benign (the host-registry prewarm in run_with_registry_and_local runs before this spawn, and everything reads PG), and arguably even desirable with NOTIFY-driven wakes — but the deletion of a commented-as-deliberate guard deserves a conscious sign-off and a line in the PR body either way.

[nikhilunni]

4. [PLAUSIBLE — nit] enqueue_session_resume NOTIFYs unconditionally even when its status='idle'-gated UPDATE matched 0 rows (the racing-resume no-op the WHERE clause exists for).

To be fair: this matches the issue's NOTIFY table ("always" for the enqueue sites), so it's spec-compliant — but the spec's "always" is motivated by the capacity-freed-between-reserve-and-enqueue race, which only applies when a row was actually enqueued. On the 0-row no-op path this wakes every replica's scanner into a full sweep (requeue_stale_pending UPDATE + whole-queue SELECT + queued_demand aggregate) for nothing, and it's inconsistent with delete_pending_session's rows_affected() > 0 guard 240 lines below. Capturing rows_affected() here is a two-line improvement; fine to defer or decline given the spec wording.

[nikhilunni]

Deep-review fixup pass

Addressed every finding from the deep review (review id 4618582617 + its 4 inline comments). just check is green locally (fmt/clippy/hakari clean; cargo nextest run --workspace clean except the 3 pre-existing, unrelated engram-host-agent chunk-cache write-through failures already called out in the PR body, tracked separately as #552 — zero engram-host-agent files touched by this branch, confirmed via git diff dc9a5d07..HEAD --name-only).

#	Finding	Verdict	Resolution
1	queue_scanner_live_pg.rs:462 — a leftover requeued B from hol_break_is_per_class_not_global can collide with per_class_fifo_head_block_is_scoped_to_its_class's y1 budget on a shared-Postgres rerun (~1/512)	PLAUSIBLE → confirmed real	4f67bb6a — disjoint per-test budget sub-ranges via unique_fitting_budget_in(id, base)
2	queue_scanner_live_pg.rs:257 — header claims nextest's default parallel execution, contradicting the file's own --test-threads=1 requirement 115 lines later; the 1ms-timeout tests are destructive whole-queue sweeps that need that serialization for correctness	CONFIRMED	4f67bb6a — rewrote the header to state the actual contract
3	queue_scanner.rs:181 — the NOTIFY rewrite dropped main's deliberate tick.tick().await; // skip the immediate first tick, so run_once now fires immediately at coordinator boot	CONFIRMED	7653d989 (+ doc note in 5ef59b4e) — restored the startup delay, kept it interruptible by a real placement_changed NOTIFY so scanner_wakes_on_notify_and_places_within_the_wake_not_the_fallback still holds
4	engram-postgres/src/lib.rs:949 — enqueue_session_resume NOTIFYs even on its 0-row status='idle' no-op path, inconsistent with delete_pending_session's rows_affected() > 0 guard	PLAUSIBLE → confirmed real	58b33bb8 — added the same guard
5	Review body — commit granularity: the issue's landing plan wanted separable commits (steps 1-2, 3, 4-6, 7); this PR fused steps 1-6 into one commit, undeclared	CONFIRMED (convention)	Declared in the PR body's "Deviations" section rather than rewriting already-pushed history (dc9a5d07)

Also folded in a test-fixture-dedupe pass flagged during the same review: five of the six new live-PG tests repeated the same three-line connect() + cordon_unmeasured_hosts() + build_app_state() preamble verbatim — extracted into a setup(cordon: bool) helper (4f67bb6a).

PR body: re-walked the acceptance-criteria checklist — all items still hold as originally checked; the "Bounded regression" line now also mentions the restored first-sweep startup delay. Added the "Post-review fixups" section above and the undeclared commit-granularity deviation (finding 5) to "Deviations / drift from the issue's anchors".

Commits: 58b33bb8, 7653d989, 4f67bb6a, 5ef59b4e (pushed to feat/queue-fairness).

CI: just kicked off on push (congested — many concurrent batch-fixup runs in flight across sibling worktrees); not blocking on it here per the fixup-pass process.

[nikhilunni]

CI-failure investigation: engram-sandbox-vz unit tests

The stored job log had expired (Azure BlobNotFound), so I reproduced locally and cross-checked GitHub's job-step metadata instead.

This PR does not touch VZ/core sandbox code. git diff origin/main...HEAD for this branch is scoped entirely to crates/engram-coordinator/, crates/engram-postgres/, and docs/adr/0048-fleet-autoscaling.md (the queue-fairness work) — zero changes to engram-sandbox-vz, engram-core's SandboxBackend/AuxRoDrive, or any other VZ-adjacent surface. (A naive diff against local main looked broader because local main was stale; origin/main already includes the VZ/ADR-0062 slot-ordering commit this branch was based on.)

The failed job died during checkout, not during tests. The job's step timeline (gh api .../actions/jobs/85067748068) shows step 3 (Run actions/checkout@v6) starting at 20:32:31Z with no completed_at, and every subsequent step (build, codesign, the actual cargo nextest run -p engram-sandbox-vz) never started at all. The job ran for exactly 10m1s before being marked failure — consistent with a stuck/hung checkout on the Blacksmith macOS runner (blacksmith-01kwmtx3dwmxzmg7yp9fkzfpsm-6vcpu), not a compile or assertion failure. No timeout-minutes is set on this job, so this reads as an infra-level timeout, not our code.

Local reproduction confirms this. In a worktree at this exact head (5ef59b4e): cargo build -p engram-coordinator -p engram-sandbox-vz --tests → codesign → cargo nextest run -p engram-sandbox-vz --no-fail-fast passed 18/18 (3 repeated runs, no flakiness) and --run-ignored ignored-only passed 5/5. cargo check --workspace --all-targets is clean. Separately, this branch's own prior CI run (bf8d2e00, two commits back) already had a green engram-sandbox-vz unit tests job, and origin/main's own latest push run also has this job green (its one failure was in tests (firecracker, ...), unrelated).

just check is green except for the pre-existing, already-tracked engram-host-agent chunk-cache write-through flake (issue #552, confirmed unrelated — no engram-host-agent files in this diff, matches this PR's own test-plan note).

Conclusion: no code change needed. I kicked a re-run of just the failed job (gh run rerun 28682152390 --job 85067748068) to get a fresh, retrievable log — expect it to go green.

[nikhilunni]

Merge conflict against main resolved (a90800cf) — a purely additive collision in metrics.rs: this PR's queue_wait_buckets/QUEUE_WAIT_SECONDS registration and the already-merged #556's prompt_to_run_started_buckets/PROMPT_TO_RUN_STARTED_SECONDS registration both landed at the same insertion point. Kept both (order between them doesn't matter — Prometheus bucket-override precedence is by Matcher kind, not insertion order). cargo check/fmt/clippy -D warnings/hakari verify all clean post-merge.