From ebb673c9420127f713f3f3ff389d5a0f8778b366 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 02:21:42 +0200 Subject: [PATCH 01/13] Show parked rounds on the dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A round in awaiting_retry whose RetryAt has not passed is correctly not fire-eligible, so QueuedRounds excluded it — and then it vanished from the dashboard entirely. Two PRs parked until 00:07Z rendered byte-identically to an empty queue ("_Nothing queued._", title "idle"), so a reader concluded their enqueue had been dropped. Add parkedRounds: the active rounds that no other part of the dashboard accounts for — not fire-eligible, and not named in the "In flight"/"Feedback wait" rows. That also catches two neighbouring blind spots: every reviewing round past the first (only reviewing[0] gets a row) and a reserved round whose fire slot was cleared. Every active round now appears in exactly one of "In flight", "Feedback wait", "Queue" or "Parked". The queue heading gains a "· N parked" suffix and the empty-state text is suppressed whenever work is parked; RenderTitle reports parked counts so a parked-only state is never "idle". FireEligible/QueuedRounds and firing behaviour are untouched — this is rendering only. Adds internal/state/dashboard_test.go, the first test file in the package. --- internal/state/dashboard.go | 98 +++++++++++++-- internal/state/dashboard_test.go | 201 +++++++++++++++++++++++++++++++ 2 files changed, 292 insertions(+), 7 deletions(-) create mode 100644 internal/state/dashboard_test.go diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index f02b187..30025e0 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -60,6 +60,57 @@ func reviewingRounds(st State) []Round { return out } +// headlineKeys returns the rounds the dashboard's summary table already names +// in its "In flight" / "Feedback wait" rows — the only active rounds rendered +// outside the Queue and Parked sections. +func headlineKeys(st State) map[string]bool { + shown := map[string]bool{} + if slot := st.SlotRound(); slot != nil { + shown[Key(slot.Repo, slot.PR)] = true + } + if rev := reviewingRounds(st); len(rev) > 0 { + shown[Key(rev[0].Repo, rev[0].PR)] = true + } + return shown +} + +// parkedRounds returns the active rounds that no other part of the dashboard +// accounts for: not fire-eligible (so absent from the Queue section) and not +// named in the "In flight"/"Feedback wait" rows. In practice that is an +// awaiting_retry round whose RetryAt is still in the future, plus the tail of a +// multi-round reviewing set and any reserved round whose fire slot was cleared. +// +// Without this, such rounds vanished entirely and a parked-only state rendered +// byte-identically to an empty queue — "no work" and "two PRs parked until +// 00:07Z" must not look the same. Ordered by Seq like the queue. +func parkedRounds(st State, now time.Time) []Round { + shown := headlineKeys(st) + var out []Round + for _, r := range st.Rounds { + if !r.Active() || r.FireEligible(now) || shown[Key(r.Repo, r.PR)] { + continue + } + out = append(out, r) + } + sort.Slice(out, func(i, j int) bool { return out[i].Seq < out[j].Seq }) + return out +} + +// nextRetry returns the earliest RetryAt among rounds, or nil when none of +// them carries one (fmtStamp renders that as "—"). +func nextRetry(rounds []Round) *time.Time { + var best *time.Time + for _, r := range rounds { + if r.RetryAt == nil { + continue + } + if best == nil || r.RetryAt.Before(*best) { + best = r.RetryAt + } + } + return best +} + func firedAtOf(r Round) time.Time { if r.FiredAt != nil { return *r.FiredAt @@ -128,6 +179,7 @@ func RenderDashboard(st State, cfg StoreConfig) string { loc := dashboardLoc(cfg) now := time.Now().UTC() queue := st.QueuedRounds(now) + parked := parkedRounds(st, now) reviewing := reviewingRounds(st) slot := st.SlotRound() blocked := st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now) @@ -144,6 +196,8 @@ func RenderDashboard(st State, cfg StoreConfig) string { fmt.Fprintf(&b, "### 🟡 Awaiting feedback for %s#%d\n\n", reviewing[0].Repo, reviewing[0].PR) case len(queue) > 0: fmt.Fprintf(&b, "### 🟠 %d queued\n\n", len(queue)) + case len(parked) > 0: + fmt.Fprintf(&b, "### 🅿️ %d parked — next retry %s\n\n", len(parked), fmtStamp(nextRetry(parked), loc)) default: fmt.Fprintf(&b, "### 🟢 Idle\n\n") } @@ -189,15 +243,34 @@ func RenderDashboard(st State, cfg StoreConfig) string { fmt.Fprintf(&b, "\n> ⚠️ %s\n", st.Warn) } - fmt.Fprintf(&b, "\n## ⏳ Queue — %d waiting\n\n", len(queue)) - if len(queue) == 0 { - fmt.Fprintf(&b, "_Nothing queued._\n") - } else { + fmt.Fprintf(&b, "\n## ⏳ Queue — %d waiting", len(queue)) + if len(parked) > 0 { + fmt.Fprintf(&b, " · %d parked", len(parked)) + } + fmt.Fprintf(&b, "\n\n") + switch { + case len(queue) > 0: fmt.Fprintf(&b, "| # | PR | enqueued | host |\n|--:|---|---|---|\n") for i, r := range queue { fmt.Fprintf(&b, "| %d | [%s#%d](https://github.com/%s/pull/%d) | %s | `%s` |\n", i+1, r.Repo, r.PR, r.Repo, r.PR, fmtStamp(&r.EnqueuedAt, loc), r.ByHost) } + case len(parked) > 0: + // Never the "nothing queued" empty state: work exists, it is just not + // fire-eligible yet. The parked table below is the honest answer. + fmt.Fprintf(&b, "_Nothing fire-eligible right now._\n") + default: + fmt.Fprintf(&b, "_Nothing queued._\n") + } + + if len(parked) > 0 { + fmt.Fprintf(&b, "\n### 🅿️ Parked — %d not yet eligible\n\n", len(parked)) + fmt.Fprintf(&b, "| PR | commit | phase | attempts | enqueued | retry at | host |\n|---|---|---|--:|---|---|---|\n") + for _, r := range parked { + fmt.Fprintf(&b, "| [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %d | %s | %s | `%s` |\n", + r.Repo, r.PR, r.Repo, r.PR, r.Head, r.Phase, r.Attempts, + fmtStamp(&r.EnqueuedAt, loc), fmtStamp(r.RetryAt, loc), r.ByHost) + } } requested := requestedRounds(st) @@ -221,15 +294,26 @@ func RenderDashboard(st State, cfg StoreConfig) string { func RenderTitle(st State) string { now := time.Now().UTC() queue := len(st.QueuedRounds(now)) + parked := len(parkedRounds(st, now)) + // Parked rounds are real work; a state holding only those must never read + // as "idle" or "queue 0". + count := fmt.Sprintf("queue %d", queue) + if parked > 0 { + count += fmt.Sprintf(" · %d parked", parked) + } switch { case st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now): - return fmt.Sprintf("🐰 crq — blocked · queue %d", queue) + return fmt.Sprintf("🐰 crq — blocked · %s", count) case st.SlotRound() != nil: - return fmt.Sprintf("🐰 crq — reviewing #%d · queue %d", st.SlotRound().PR, queue) + return fmt.Sprintf("🐰 crq — reviewing #%d · %s", st.SlotRound().PR, count) case len(reviewingRounds(st)) > 0: - return fmt.Sprintf("🐰 crq — awaiting feedback · queue %d", queue) + return fmt.Sprintf("🐰 crq — awaiting feedback · %s", count) + case queue > 0 && parked > 0: + return fmt.Sprintf("🐰 crq — %d queued · %d parked", queue, parked) case queue > 0: return fmt.Sprintf("🐰 crq — %d queued", queue) + case parked > 0: + return fmt.Sprintf("🐰 crq — %d parked", parked) default: return "🐰 crq — idle" } diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go new file mode 100644 index 0000000..4b08504 --- /dev/null +++ b/internal/state/dashboard_test.go @@ -0,0 +1,201 @@ +package state + +import ( + "strings" + "testing" + "time" +) + +const nothingQueued = "_Nothing queued._" + +func ptime(t time.Time) *time.Time { return &t } + +// parkedRound is the shape that used to vanish from the dashboard: an +// awaiting_retry round whose RetryAt has not passed yet. +func parkedRound(repo string, pr int, seq int64, now time.Time) Round { + return Round{ + Repo: repo, + PR: pr, + Head: "a9c688a1c", + Seq: seq, + Phase: PhaseAwaitingRetry, + Attempts: 1, + EnqueuedAt: now.Add(-10 * time.Minute), + FiredAt: ptime(now.Add(-9 * time.Minute)), + RetryAt: ptime(now.Add(11 * time.Minute)), + ByHost: "cachyos", + } +} + +func stateWith(rounds ...Round) State { + st := New() + for _, r := range rounds { + st.PutRound(r) + } + return st +} + +// A parked-only state must not render as an empty queue: that is the bug this +// section exists to prevent (a reader concluded their enqueue was dropped). +func TestRenderDashboardParkedOnly(t *testing.T) { + now := time.Now().UTC() + r := parkedRound("kristofferr/ha-adjustable-bed", 480, 1, now) + st := stateWith(r) + + out := RenderDashboard(st, StoreConfig{}) + if strings.Contains(out, nothingQueued) { + t.Fatalf("parked round rendered as an empty queue:\n%s", out) + } + for _, want := range []string{ + "kristofferr/ha-adjustable-bed#480", + "https://github.com/kristofferr/ha-adjustable-bed/pull/480", + "`a9c688a1c`", + fmtStamp(r.RetryAt, time.UTC), // absolute retry_at, not a relative "in 11m" + "`cachyos`", + "1 parked", + } { + if !strings.Contains(out, want) { + t.Errorf("dashboard missing %q:\n%s", want, out) + } + } + + if title := RenderTitle(st); strings.Contains(title, "idle") { + t.Errorf("parked-only state titled idle: %q", title) + } +} + +// Guard against over-correcting: a genuinely empty state keeps its empty-state +// text and its idle title. +func TestRenderDashboardEmpty(t *testing.T) { + st := New() + + out := RenderDashboard(st, StoreConfig{}) + if !strings.Contains(out, nothingQueued) { + t.Errorf("empty state lost its empty-state text:\n%s", out) + } + if strings.Contains(out, "Parked") { + t.Errorf("empty state rendered a parked section:\n%s", out) + } + if got, want := RenderTitle(st), "🐰 crq — idle"; got != want { + t.Errorf("RenderTitle = %q, want %q", got, want) + } +} + +// A fire-eligible round and a parked one land in their own sections, each +// counted exactly once. +func TestRenderDashboardMixed(t *testing.T) { + now := time.Now().UTC() + queued := Round{ + Repo: "kristofferr/coderabbit-queue", + PR: 12, + Head: "beefbeef1", + Seq: 1, + Phase: PhaseQueued, + EnqueuedAt: now.Add(-time.Minute), + ByHost: "cachyos", + } + st := stateWith(queued, parkedRound("kristofferr/ha-adjustable-bed", 480, 2, now)) + + out := RenderDashboard(st, StoreConfig{}) + if !strings.Contains(out, "## ⏳ Queue — 1 waiting · 1 parked") { + t.Errorf("queue heading does not report both counts:\n%s", out) + } + if strings.Contains(out, nothingQueued) { + t.Errorf("non-empty queue rendered the empty state:\n%s", out) + } + + queueBody, parkedBody, ok := strings.Cut(out, "### 🅿️ Parked — 1 not yet eligible") + if !ok { + t.Fatalf("no parked section:\n%s", out) + } + if !strings.Contains(queueBody, "coderabbit-queue#12") { + t.Errorf("fire-eligible round missing from the queue table:\n%s", queueBody) + } + if strings.Contains(queueBody, "ha-adjustable-bed#480") { + t.Errorf("parked round leaked into the queue table:\n%s", queueBody) + } + if strings.Contains(parkedBody, "coderabbit-queue#12") { + t.Errorf("fire-eligible round double-counted in the parked table:\n%s", parkedBody) + } + + if got, want := RenderTitle(st), "🐰 crq — 1 queued · 1 parked"; got != want { + t.Errorf("RenderTitle = %q, want %q", got, want) + } +} + +// An awaiting_retry round whose window has opened is fire-eligible: it belongs +// in the normal queue, not the parked view. +func TestRenderDashboardRetryWindowOpen(t *testing.T) { + now := time.Now().UTC() + r := parkedRound("kristofferr/ha-adjustable-bed", 480, 1, now) + r.RetryAt = ptime(now.Add(-time.Minute)) + st := stateWith(r) + + out := RenderDashboard(st, StoreConfig{}) + if !strings.Contains(out, "## ⏳ Queue — 1 waiting\n") { + t.Errorf("elapsed retry window not counted as waiting:\n%s", out) + } + if strings.Contains(out, "Parked") { + t.Errorf("fire-eligible round rendered as parked:\n%s", out) + } + if got, want := RenderTitle(st), "🐰 crq — 1 queued"; got != want { + t.Errorf("RenderTitle = %q, want %q", got, want) + } +} + +// Every active round must appear in one of "In flight", "Feedback wait", +// "Queue" or "Parked" — the blind spot that hid parked rounds also hid a +// reserved round whose fire slot was cleared and every reviewing round past +// the first. +func TestRenderDashboardAccountsForEveryActiveRound(t *testing.T) { + now := time.Now().UTC() + fired := Round{ + Repo: "kristofferr/a", PR: 1, Head: "aaaaaaaa1", Seq: 1, + Phase: PhaseFired, EnqueuedAt: now.Add(-5 * time.Minute), + FiredAt: ptime(now.Add(-4 * time.Minute)), ByHost: "cachyos", + } + reviewing := Round{ + Repo: "kristofferr/b", PR: 2, Head: "bbbbbbbb2", Seq: 2, + Phase: PhaseReviewing, EnqueuedAt: now.Add(-3 * time.Minute), + FiredAt: ptime(now.Add(-2 * time.Minute)), ByHost: "cachyos", + } + reserved := Round{ // slot-less reserved round (FireSlot cleared by Normalize) + Repo: "kristofferr/c", PR: 3, Head: "cccccccc3", Seq: 3, + Phase: PhaseReserved, EnqueuedAt: now.Add(-time.Minute), + ReservedAt: ptime(now), ByHost: "cachyos", + } + st := stateWith(fired, reviewing, reserved, + parkedRound("kristofferr/d", 4, 4, now)) + + out := RenderDashboard(st, StoreConfig{}) + // The "Recently requested" history lists fired rounds regardless of phase, + // so it must not count as accounting for a live round. + live, _, _ := strings.Cut(out, "## 📨 Recently requested") + for _, r := range st.Rounds { + if !r.Active() { + continue + } + if !strings.Contains(live, Key(r.Repo, r.PR)) { + t.Errorf("active round %s appears nowhere on the dashboard:\n%s", Key(r.Repo, r.PR), out) + } + } + if !strings.Contains(out, "3 not yet eligible") { + t.Errorf("want reviewing tail + reserved + parked in the parked table:\n%s", out) + } +} + +// The parked table honours CRQ_TZ via the shared fmtStamp helper. +func TestRenderDashboardParkedHonoursTimezone(t *testing.T) { + now := time.Now().UTC() + r := parkedRound("kristofferr/ha-adjustable-bed", 480, 1, now) + st := stateWith(r) + + loc, err := time.LoadLocation("Europe/Oslo") + if err != nil { + t.Skipf("tzdata unavailable: %v", err) + } + out := RenderDashboard(st, StoreConfig{Timezone: "Europe/Oslo"}) + if !strings.Contains(out, fmtStamp(r.RetryAt, loc)) { + t.Errorf("parked retry_at not rendered in Europe/Oslo:\n%s", out) + } +} From ab25e40e21a8f18e07627f67afb08f4a12409d0f Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 03:05:56 +0200 Subject: [PATCH 02/13] Render one ordered review queue on the dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard had two ways to be waiting for a fire slot. "Queue" listed fire-eligible rounds; a round in awaiting_retry whose RetryAt had not passed fell out of QueuedRounds and needed a separate "Parked" table to be visible at all. Rendering live fleet state showed why that is the wrong cut: two rounds sat parked with near-identical retry times and nothing said which went first, or how they related to the rounds listed above them. They were never racing. NextEligible takes the lowest Seq among eligible rounds, Seq survives AwaitRetry untouched, and the FireSlot admits one fire fleet-wide — a cooling-down round is already queued, in order. What was wrong is the model: a round's position in line (Seq) and its permission to run (RetryAt) are independent properties, and crq rendered the first as a queue and the second as a lifecycle phase. State.Queue is the single view: every round waiting for a fire, ordered by (ReadyAt, Seq) — the order they actually reach the slot. ReadyAt folds in the account-wide quota block, which fixes a real bug: the parked table printed the round's own RetryAt while DecideFire also gates on Account.BlockedUntil, so a window CodeRabbit later extended left the dashboard promising a retry time the fire gate would refuse. FireEligible, QueuedRounds and NextEligible are untouched; this is a view, not a schema or a rule. The single "In flight"/"Feedback wait" summary rows are replaced by an In flight section listing every reserved/fired/reviewing round. Those rows showed one round each, which hid every reviewing round past the first and any reserved round whose FireSlot Normalize had cleared. In flight and Queue now partition Active() by phase, so "every active round is on the dashboard" holds by construction rather than by a catch-all — pinned by a test. --- internal/state/dashboard.go | 188 ++++++++-------------- internal/state/dashboard_test.go | 262 +++++++++++++++++++++---------- internal/state/state.go | 83 ++++++++++ 3 files changed, 334 insertions(+), 199 deletions(-) diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 30025e0..4008725 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -44,13 +44,20 @@ func minutesUntil(t time.Time, now time.Time) int { return mins } -// reviewingRounds returns the rounds that fired and are still open (fired or -// reviewing), ordered by fire time — the v3 equivalent of the "awaiting -// feedback" set (a fired round whose slot may already be released). -func reviewingRounds(st State) []Round { +// inFlightRounds returns every round crq has already acted on and is still +// carrying: reserved (slot held, command not yet posted), fired, or reviewing — +// ordered by fire time. +// +// Together with State.Queue (queued + awaiting_retry) this PARTITIONS Active() +// by phase, which is what makes "every active round is on the dashboard" true +// by construction. The previous single "Feedback wait" row showed reviewing[0] +// and silently dropped every round behind it, along with any reserved round +// whose fire slot Normalize had cleared. +func inFlightRounds(st State) []Round { var out []Round for _, r := range st.Rounds { - if r.Phase == PhaseFired || r.Phase == PhaseReviewing { + switch r.Phase { + case PhaseReserved, PhaseFired, PhaseReviewing: out = append(out, r) } } @@ -60,57 +67,6 @@ func reviewingRounds(st State) []Round { return out } -// headlineKeys returns the rounds the dashboard's summary table already names -// in its "In flight" / "Feedback wait" rows — the only active rounds rendered -// outside the Queue and Parked sections. -func headlineKeys(st State) map[string]bool { - shown := map[string]bool{} - if slot := st.SlotRound(); slot != nil { - shown[Key(slot.Repo, slot.PR)] = true - } - if rev := reviewingRounds(st); len(rev) > 0 { - shown[Key(rev[0].Repo, rev[0].PR)] = true - } - return shown -} - -// parkedRounds returns the active rounds that no other part of the dashboard -// accounts for: not fire-eligible (so absent from the Queue section) and not -// named in the "In flight"/"Feedback wait" rows. In practice that is an -// awaiting_retry round whose RetryAt is still in the future, plus the tail of a -// multi-round reviewing set and any reserved round whose fire slot was cleared. -// -// Without this, such rounds vanished entirely and a parked-only state rendered -// byte-identically to an empty queue — "no work" and "two PRs parked until -// 00:07Z" must not look the same. Ordered by Seq like the queue. -func parkedRounds(st State, now time.Time) []Round { - shown := headlineKeys(st) - var out []Round - for _, r := range st.Rounds { - if !r.Active() || r.FireEligible(now) || shown[Key(r.Repo, r.PR)] { - continue - } - out = append(out, r) - } - sort.Slice(out, func(i, j int) bool { return out[i].Seq < out[j].Seq }) - return out -} - -// nextRetry returns the earliest RetryAt among rounds, or nil when none of -// them carries one (fmtStamp renders that as "—"). -func nextRetry(rounds []Round) *time.Time { - var best *time.Time - for _, r := range rounds { - if r.RetryAt == nil { - continue - } - if best == nil || r.RetryAt.Before(*best) { - best = r.RetryAt - } - } - return best -} - func firedAtOf(r Round) time.Time { if r.FiredAt != nil { return *r.FiredAt @@ -146,8 +102,9 @@ func requestedRounds(st State) []Round { } // coBotMarks renders a round's co-reviewer trigger bookkeeping for the -// feedback-wait row: ✓ = trigger posted/adopted, ⏳ = post claimed but not -// yet recorded. Empty (byte-identical row) when the round tracks no co-bots. +// in-flight table's triggers column: ✓ = trigger posted/adopted, ⏳ = post +// claimed but not yet recorded. Empty when the round tracks no co-bots; the +// caller supplies the surrounding decoration. func coBotMarks(r Round) string { if len(r.CoBots) == 0 { return "" @@ -170,7 +127,15 @@ func coBotMarks(r Round) string { if len(parts) == 0 { return "" } - return " · triggers: " + strings.Join(parts, ", ") + return strings.Join(parts, ", ") +} + +// dash renders an empty cell as an em dash so a table row never collapses. +func dash(s string) string { + if s == "" { + return "—" + } + return s } // RenderDashboard renders the human-facing dashboard for v3 state: rounds by @@ -178,9 +143,8 @@ func coBotMarks(r Round) string { func RenderDashboard(st State, cfg StoreConfig) string { loc := dashboardLoc(cfg) now := time.Now().UTC() - queue := st.QueuedRounds(now) - parked := parkedRounds(st, now) - reviewing := reviewingRounds(st) + queue := st.Queue(now) + inFlight := inFlightRounds(st) slot := st.SlotRound() blocked := st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now) @@ -192,12 +156,16 @@ func RenderDashboard(st State, cfg StoreConfig) string { fmt.Fprintf(&b, "### 🔴 Blocked — next review in ~%dm\n\n", minutesUntil(*st.Account.BlockedUntil, now)) case slot != nil: fmt.Fprintf(&b, "### 🟡 Reviewing %s#%d\n\n", slot.Repo, slot.PR) - case len(reviewing) > 0: - fmt.Fprintf(&b, "### 🟡 Awaiting feedback for %s#%d\n\n", reviewing[0].Repo, reviewing[0].PR) + case len(inFlight) > 0: + fmt.Fprintf(&b, "### 🟡 Awaiting feedback for %s#%d\n\n", inFlight[0].Repo, inFlight[0].PR) case len(queue) > 0: - fmt.Fprintf(&b, "### 🟠 %d queued\n\n", len(queue)) - case len(parked) > 0: - fmt.Fprintf(&b, "### 🅿️ %d parked — next retry %s\n\n", len(parked), fmtStamp(nextRetry(parked), loc)) + // Nothing ready yet is still queued work, never idle — say when the front + // of the queue opens instead of leaving the reader to guess. + if next := queue[0].ReadyAt; !next.IsZero() { + fmt.Fprintf(&b, "### 🟠 %d queued — next at %s\n\n", len(queue), fmtStamp(&next, loc)) + } else { + fmt.Fprintf(&b, "### 🟠 %d queued\n\n", len(queue)) + } default: fmt.Fprintf(&b, "### 🟢 Idle\n\n") } @@ -226,50 +194,38 @@ func RenderDashboard(st State, cfg StoreConfig) string { fmt.Fprintf(&b, "| **Co-reviewers** | %s |\n", cfg.CoReviewers) } fmt.Fprintf(&b, "| **Last review fired** | %s |\n", fmtStamp(st.LastFired, loc)) - if slot != nil { - fmt.Fprintf(&b, "| **In flight** | [%s#%d](https://github.com/%s/pull/%d) · fired %s · `%s` |\n", - slot.Repo, slot.PR, slot.Repo, slot.PR, fmtStamp(slot.FiredAt, loc), slot.ByHost) - } else { - fmt.Fprintf(&b, "| **In flight** | — |\n") - } - if len(reviewing) > 0 { - r := reviewing[0] - fmt.Fprintf(&b, "| **Feedback wait** | [%s#%d](https://github.com/%s/pull/%d) · `%s` · deadline %s%s |\n", - r.Repo, r.PR, r.Repo, r.PR, r.Head, fmtStamp(r.WaitDeadline, loc), coBotMarks(r)) - } else { - fmt.Fprintf(&b, "| **Feedback wait** | — |\n") - } if st.Warn != "" { fmt.Fprintf(&b, "\n> ⚠️ %s\n", st.Warn) } - fmt.Fprintf(&b, "\n## ⏳ Queue — %d waiting", len(queue)) - if len(parked) > 0 { - fmt.Fprintf(&b, " · %d parked", len(parked)) - } - fmt.Fprintf(&b, "\n\n") - switch { - case len(queue) > 0: - fmt.Fprintf(&b, "| # | PR | enqueued | host |\n|--:|---|---|---|\n") - for i, r := range queue { - fmt.Fprintf(&b, "| %d | [%s#%d](https://github.com/%s/pull/%d) | %s | `%s` |\n", - i+1, r.Repo, r.PR, r.Repo, r.PR, fmtStamp(&r.EnqueuedAt, loc), r.ByHost) + fmt.Fprintf(&b, "\n## 🔬 In flight — %d\n\n", len(inFlight)) + if len(inFlight) == 0 { + fmt.Fprintf(&b, "_None._\n") + } else { + fmt.Fprintf(&b, "| PR | commit | phase | fired | deadline | triggers | host |\n|---|---|---|---|---|---|---|\n") + for _, r := range inFlight { + fmt.Fprintf(&b, "| [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %s | %s | `%s` |\n", + r.Repo, r.PR, r.Repo, r.PR, r.Head, r.Phase, + fmtStamp(r.FiredAt, loc), fmtStamp(r.WaitDeadline, loc), dash(coBotMarks(r)), r.ByHost) } - case len(parked) > 0: - // Never the "nothing queued" empty state: work exists, it is just not - // fire-eligible yet. The parked table below is the honest answer. - fmt.Fprintf(&b, "_Nothing fire-eligible right now._\n") - default: - fmt.Fprintf(&b, "_Nothing queued._\n") } - if len(parked) > 0 { - fmt.Fprintf(&b, "\n### 🅿️ Parked — %d not yet eligible\n\n", len(parked)) - fmt.Fprintf(&b, "| PR | commit | phase | attempts | enqueued | retry at | host |\n|---|---|---|--:|---|---|---|\n") - for _, r := range parked { - fmt.Fprintf(&b, "| [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %d | %s | %s | `%s` |\n", - r.Repo, r.PR, r.Repo, r.PR, r.Head, r.Phase, r.Attempts, - fmtStamp(&r.EnqueuedAt, loc), fmtStamp(r.RetryAt, loc), r.ByHost) + fmt.Fprintf(&b, "\n## ⏳ Queue — %d waiting\n\n", len(queue)) + if len(queue) == 0 { + fmt.Fprintf(&b, "_Nothing queued._\n") + } else { + fmt.Fprintf(&b, "| # | PR | commit | ready | why | attempts | enqueued | host |\n|--:|---|---|---|---|--:|---|---|\n") + for i, e := range queue { + // Absolute stamps only: a relative "in 11m" would re-hash the dashboard + // on every render, and fmtStamp already honours CRQ_TZ. + ready := "now" + if !e.ReadyAt.IsZero() { + at := e.ReadyAt + ready = fmtStamp(&at, loc) + } + fmt.Fprintf(&b, "| %d | [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %d | %s | `%s` |\n", + i+1, e.Repo, e.PR, e.Repo, e.PR, e.Head, ready, dash(e.Why), + e.Attempts, fmtStamp(&e.EnqueuedAt, loc), e.ByHost) } } @@ -291,29 +247,21 @@ func RenderDashboard(st State, cfg StoreConfig) string { return b.String() } +// RenderTitle summarizes the state for the dashboard issue title. The queue +// count is the WHOLE queue, cooling-down rounds included: a state whose only +// work is not yet fire-eligible is queued, never idle. func RenderTitle(st State) string { now := time.Now().UTC() - queue := len(st.QueuedRounds(now)) - parked := len(parkedRounds(st, now)) - // Parked rounds are real work; a state holding only those must never read - // as "idle" or "queue 0". - count := fmt.Sprintf("queue %d", queue) - if parked > 0 { - count += fmt.Sprintf(" · %d parked", parked) - } + queue := len(st.Queue(now)) switch { case st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now): - return fmt.Sprintf("🐰 crq — blocked · %s", count) + return fmt.Sprintf("🐰 crq — blocked · queue %d", queue) case st.SlotRound() != nil: - return fmt.Sprintf("🐰 crq — reviewing #%d · %s", st.SlotRound().PR, count) - case len(reviewingRounds(st)) > 0: - return fmt.Sprintf("🐰 crq — awaiting feedback · %s", count) - case queue > 0 && parked > 0: - return fmt.Sprintf("🐰 crq — %d queued · %d parked", queue, parked) + return fmt.Sprintf("🐰 crq — reviewing #%d · queue %d", st.SlotRound().PR, queue) + case len(inFlightRounds(st)) > 0: + return fmt.Sprintf("🐰 crq — awaiting feedback · queue %d", queue) case queue > 0: return fmt.Sprintf("🐰 crq — %d queued", queue) - case parked > 0: - return fmt.Sprintf("🐰 crq — %d parked", parked) default: return "🐰 crq — idle" } diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 4b08504..4d376e1 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -10,9 +10,9 @@ const nothingQueued = "_Nothing queued._" func ptime(t time.Time) *time.Time { return &t } -// parkedRound is the shape that used to vanish from the dashboard: an -// awaiting_retry round whose RetryAt has not passed yet. -func parkedRound(repo string, pr int, seq int64, now time.Time) Round { +// coolingRound is the shape that used to vanish from the dashboard: a waiting +// round whose own RetryAt has not passed yet. +func coolingRound(repo string, pr int, seq int64, now time.Time, retryIn time.Duration) Round { return Round{ Repo: repo, PR: pr, @@ -22,7 +22,19 @@ func parkedRound(repo string, pr int, seq int64, now time.Time) Round { Attempts: 1, EnqueuedAt: now.Add(-10 * time.Minute), FiredAt: ptime(now.Add(-9 * time.Minute)), - RetryAt: ptime(now.Add(11 * time.Minute)), + RetryAt: ptime(now.Add(retryIn)), + ByHost: "cachyos", + } +} + +func queuedRound(repo string, pr int, seq int64, now time.Time) Round { + return Round{ + Repo: repo, + PR: pr, + Head: "beefbeef1", + Seq: seq, + Phase: PhaseQueued, + EnqueuedAt: now.Add(-time.Minute), ByHost: "cachyos", } } @@ -35,32 +47,54 @@ func stateWith(rounds ...Round) State { return st } -// A parked-only state must not render as an empty queue: that is the bug this -// section exists to prevent (a reader concluded their enqueue was dropped). -func TestRenderDashboardParkedOnly(t *testing.T) { +// queueSection returns just the "## ⏳ Queue" section of a rendered dashboard, +// so a match cannot come from the in-flight table or the requested history. +func queueSection(t *testing.T, out string) string { + t.Helper() + _, after, ok := strings.Cut(out, "## ⏳ Queue") + if !ok { + t.Fatalf("no queue section:\n%s", out) + } + before, _, _ := strings.Cut(after, "\n## ") + return before +} + +// A queue whose rounds are all cooling down must not render as an empty queue: +// that is the bug this design exists to prevent (a reader concluded their +// enqueue had been dropped). +func TestRenderDashboardCoolingDownOnly(t *testing.T) { now := time.Now().UTC() - r := parkedRound("kristofferr/ha-adjustable-bed", 480, 1, now) - st := stateWith(r) + a := coolingRound("kristofferr/ha-adjustable-bed", 480, 1, now, 11*time.Minute) + b := coolingRound("kristofferr/ha-adjustable-bed", 481, 2, now, 12*time.Minute) + st := stateWith(a, b) out := RenderDashboard(st, StoreConfig{}) if strings.Contains(out, nothingQueued) { - t.Fatalf("parked round rendered as an empty queue:\n%s", out) + t.Fatalf("cooling-down rounds rendered as an empty queue:\n%s", out) } + if !strings.Contains(out, "## ⏳ Queue — 2 waiting") { + t.Errorf("queue heading does not count cooling-down rounds:\n%s", out) + } + q := queueSection(t, out) for _, want := range []string{ "kristofferr/ha-adjustable-bed#480", "https://github.com/kristofferr/ha-adjustable-bed/pull/480", "`a9c688a1c`", - fmtStamp(r.RetryAt, time.UTC), // absolute retry_at, not a relative "in 11m" + fmtStamp(a.RetryAt, time.UTC), // absolute ready time, not a relative "in 11m" + WaitCoolingDown, "`cachyos`", - "1 parked", } { - if !strings.Contains(out, want) { - t.Errorf("dashboard missing %q:\n%s", want, out) + if !strings.Contains(q, want) { + t.Errorf("queue section missing %q:\n%s", want, q) } } + // The header must say when the front of the queue opens. + if !strings.Contains(out, "2 queued — next at "+fmtStamp(a.RetryAt, time.UTC)) { + t.Errorf("header does not report the next ready time:\n%s", out) + } - if title := RenderTitle(st); strings.Contains(title, "idle") { - t.Errorf("parked-only state titled idle: %q", title) + if got, want := RenderTitle(st), "🐰 crq — 2 queued"; got != want { + t.Errorf("RenderTitle = %q, want %q", got, want) } } @@ -73,81 +107,106 @@ func TestRenderDashboardEmpty(t *testing.T) { if !strings.Contains(out, nothingQueued) { t.Errorf("empty state lost its empty-state text:\n%s", out) } - if strings.Contains(out, "Parked") { - t.Errorf("empty state rendered a parked section:\n%s", out) + if !strings.Contains(out, "## 🔬 In flight — 0\n\n_None._") { + t.Errorf("empty state lost its empty in-flight section:\n%s", out) } if got, want := RenderTitle(st), "🐰 crq — idle"; got != want { t.Errorf("RenderTitle = %q, want %q", got, want) } } -// A fire-eligible round and a parked one land in their own sections, each -// counted exactly once. -func TestRenderDashboardMixed(t *testing.T) { +// The queue is ordered the way rounds will actually fire: ready rounds first +// (by Seq), then by when each window opens — regardless of Seq. +func TestQueueOrdersByReadyThenSeq(t *testing.T) { now := time.Now().UTC() - queued := Round{ - Repo: "kristofferr/coderabbit-queue", - PR: 12, - Head: "beefbeef1", - Seq: 1, - Phase: PhaseQueued, - EnqueuedAt: now.Add(-time.Minute), - ByHost: "cachyos", - } - st := stateWith(queued, parkedRound("kristofferr/ha-adjustable-bed", 480, 2, now)) + st := stateWith( + coolingRound("kristofferr/a", 1, 1, now, 20*time.Minute), // lowest Seq, latest window + coolingRound("kristofferr/b", 2, 2, now, 5*time.Minute), + queuedRound("kristofferr/c", 3, 3, now), // ready now, highest Seq + coolingRound("kristofferr/d", 4, 4, now, -2*time.Minute), // window already open + ) - out := RenderDashboard(st, StoreConfig{}) - if !strings.Contains(out, "## ⏳ Queue — 1 waiting · 1 parked") { - t.Errorf("queue heading does not report both counts:\n%s", out) + q := st.Queue(now) + var got []int + for _, e := range q { + got = append(got, e.PR) } - if strings.Contains(out, nothingQueued) { - t.Errorf("non-empty queue rendered the empty state:\n%s", out) + // Ready now: c (Seq 3) and d (Seq 4, elapsed RetryAt) — Seq order among them. + // Then b (+5m), then a (+20m). + want := []int{3, 4, 2, 1} + if len(got) != len(want) { + t.Fatalf("Queue returned %v, want %v", got, want) } - - queueBody, parkedBody, ok := strings.Cut(out, "### 🅿️ Parked — 1 not yet eligible") - if !ok { - t.Fatalf("no parked section:\n%s", out) + for i := range want { + if got[i] != want[i] { + t.Fatalf("Queue order = %v, want %v", got, want) + } + } + // An elapsed RetryAt is ready, not cooling down. + if q[1].Why != "" || !q[1].ReadyAt.IsZero() { + t.Errorf("elapsed retry window not treated as ready: %+v", q[1]) + } + if q[2].Why != WaitCoolingDown { + t.Errorf("q[2].Why = %q, want %q", q[2].Why, WaitCoolingDown) } - if !strings.Contains(queueBody, "coderabbit-queue#12") { - t.Errorf("fire-eligible round missing from the queue table:\n%s", queueBody) +} + +// The account-wide quota block gates firing too, so a round whose own RetryAt +// falls inside a longer block must display the block's end, not its own — the +// dashboard must not promise a time DecideFire will refuse. +func TestQueueAccountBlockDominatesRetryAt(t *testing.T) { + now := time.Now().UTC() + r := coolingRound("kristofferr/ha-adjustable-bed", 480, 1, now, 11*time.Minute) + st := stateWith(r) + blockedUntil := now.Add(44 * time.Minute) + st.Account.BlockedUntil = &blockedUntil + + q := st.Queue(now) + if len(q) != 1 { + t.Fatalf("Queue returned %d entries, want 1", len(q)) } - if strings.Contains(queueBody, "ha-adjustable-bed#480") { - t.Errorf("parked round leaked into the queue table:\n%s", queueBody) + if !q[0].ReadyAt.Equal(blockedUntil.UTC()) { + t.Errorf("ReadyAt = %s, want the account block end %s", q[0].ReadyAt, blockedUntil.UTC()) } - if strings.Contains(parkedBody, "coderabbit-queue#12") { - t.Errorf("fire-eligible round double-counted in the parked table:\n%s", parkedBody) + if q[0].Why != WaitAccountBlocked { + t.Errorf("Why = %q, want %q", q[0].Why, WaitAccountBlocked) } - if got, want := RenderTitle(st), "🐰 crq — 1 queued · 1 parked"; got != want { - t.Errorf("RenderTitle = %q, want %q", got, want) + out := queueSection(t, RenderDashboard(st, StoreConfig{})) + if !strings.Contains(out, fmtStamp(&blockedUntil, time.UTC)) { + t.Errorf("queue does not show the account-block end:\n%s", out) + } + if strings.Contains(out, fmtStamp(r.RetryAt, time.UTC)) { + t.Errorf("queue shows the round's own RetryAt, which the fire gate would refuse:\n%s", out) } } -// An awaiting_retry round whose window has opened is fire-eligible: it belongs -// in the normal queue, not the parked view. -func TestRenderDashboardRetryWindowOpen(t *testing.T) { +// A round waiting only because another PR holds the fire slot is ready now; the +// slot is what it waits on. +func TestQueueSlotBusy(t *testing.T) { now := time.Now().UTC() - r := parkedRound("kristofferr/ha-adjustable-bed", 480, 1, now) - r.RetryAt = ptime(now.Add(-time.Minute)) - st := stateWith(r) - - out := RenderDashboard(st, StoreConfig{}) - if !strings.Contains(out, "## ⏳ Queue — 1 waiting\n") { - t.Errorf("elapsed retry window not counted as waiting:\n%s", out) + holder := Round{ + Repo: "kristofferr/a", PR: 1, Head: "aaaaaaaa1", Seq: 1, + Phase: PhaseReserved, EnqueuedAt: now.Add(-2 * time.Minute), + ReservedAt: ptime(now.Add(-time.Minute)), Token: "tok", ByHost: "cachyos", } - if strings.Contains(out, "Parked") { - t.Errorf("fire-eligible round rendered as parked:\n%s", out) + st := stateWith(holder, queuedRound("kristofferr/b", 2, 2, now)) + st.FireSlot = &FireSlot{Key: Key(holder.Repo, holder.PR), Token: "tok"} + + q := st.Queue(now) + if len(q) != 1 || q[0].PR != 2 { + t.Fatalf("Queue = %+v, want only the queued round", q) } - if got, want := RenderTitle(st), "🐰 crq — 1 queued"; got != want { - t.Errorf("RenderTitle = %q, want %q", got, want) + if q[0].Why != WaitSlotBusy { + t.Errorf("Why = %q, want %q", q[0].Why, WaitSlotBusy) } } -// Every active round must appear in one of "In flight", "Feedback wait", -// "Queue" or "Parked" — the blind spot that hid parked rounds also hid a -// reserved round whose fire slot was cleared and every reviewing round past -// the first. -func TestRenderDashboardAccountsForEveryActiveRound(t *testing.T) { +// In flight (reserved/fired/reviewing) and Queue (queued/awaiting_retry) +// partition Active(), so every active round is on the dashboard exactly once. +// The old single "Feedback wait" row hid every reviewing round past the first +// and any reserved round whose fire slot had been cleared. +func TestRenderDashboardPartitionsActiveRounds(t *testing.T) { now := time.Now().UTC() fired := Round{ Repo: "kristofferr/a", PR: 1, Head: "aaaaaaaa1", Seq: 1, @@ -164,30 +223,75 @@ func TestRenderDashboardAccountsForEveryActiveRound(t *testing.T) { Phase: PhaseReserved, EnqueuedAt: now.Add(-time.Minute), ReservedAt: ptime(now), ByHost: "cachyos", } - st := stateWith(fired, reviewing, reserved, - parkedRound("kristofferr/d", 4, 4, now)) + completed := Round{ // not active: the reviewed-head dedup marker + Repo: "kristofferr/e", PR: 5, Head: "eeeeeeee5", Seq: 5, + Phase: PhaseCompleted, EnqueuedAt: now.Add(-time.Hour), ByHost: "cachyos", + } + st := stateWith(fired, reviewing, reserved, completed, + coolingRound("kristofferr/d", 4, 4, now, 30*time.Minute), + queuedRound("kristofferr/f", 6, 6, now)) out := RenderDashboard(st, StoreConfig{}) - // The "Recently requested" history lists fired rounds regardless of phase, - // so it must not count as accounting for a live round. + // The "Recently requested" history lists fired rounds regardless of phase, so + // it must not count as accounting for a live round. live, _, _ := strings.Cut(out, "## 📨 Recently requested") + inFlight, queue, ok := strings.Cut(live, "## ⏳ Queue") + if !ok { + t.Fatalf("no queue section:\n%s", out) + } for _, r := range st.Rounds { + key := Key(r.Repo, r.PR) + inA, inB := strings.Contains(inFlight, key), strings.Contains(queue, key) if !r.Active() { + if inA || inB { + t.Errorf("inactive round %s rendered as live work", key) + } continue } - if !strings.Contains(live, Key(r.Repo, r.PR)) { - t.Errorf("active round %s appears nowhere on the dashboard:\n%s", Key(r.Repo, r.PR), out) + if inA == inB { + t.Errorf("active round %s is in %d sections, want exactly 1:\n%s", key, btoi(inA)+btoi(inB), live) } } - if !strings.Contains(out, "3 not yet eligible") { - t.Errorf("want reviewing tail + reserved + parked in the parked table:\n%s", out) + if !strings.Contains(out, "## 🔬 In flight — 3") { + t.Errorf("want 3 in-flight rounds:\n%s", out) + } + if !strings.Contains(out, "## ⏳ Queue — 2 waiting") { + t.Errorf("want 2 waiting rounds:\n%s", out) + } +} + +func btoi(b bool) int { + if b { + return 1 + } + return 0 +} + +// The in-flight table carries each round's co-reviewer trigger marks. +func TestRenderDashboardInFlightTriggers(t *testing.T) { + now := time.Now().UTC() + r := Round{ + Repo: "kristofferr/a", PR: 1, Head: "aaaaaaaa1", Seq: 1, + Phase: PhaseReviewing, EnqueuedAt: now.Add(-3 * time.Minute), + FiredAt: ptime(now.Add(-2 * time.Minute)), WaitDeadline: ptime(now.Add(18 * time.Minute)), + ByHost: "cachyos", + } + r.SetCoCommand("chatgpt-codex-connector", 42, now.Add(-2*time.Minute)) + st := stateWith(r) + + out := RenderDashboard(st, StoreConfig{}) + if !strings.Contains(out, "chatgpt-codex-connector ✓") { + t.Errorf("in-flight row missing trigger marks:\n%s", out) + } + if !strings.Contains(out, fmtStamp(r.WaitDeadline, time.UTC)) { + t.Errorf("in-flight row missing the wait deadline:\n%s", out) } } -// The parked table honours CRQ_TZ via the shared fmtStamp helper. -func TestRenderDashboardParkedHonoursTimezone(t *testing.T) { +// The ready column honours CRQ_TZ via the shared fmtStamp helper. +func TestRenderDashboardQueueHonoursTimezone(t *testing.T) { now := time.Now().UTC() - r := parkedRound("kristofferr/ha-adjustable-bed", 480, 1, now) + r := coolingRound("kristofferr/ha-adjustable-bed", 480, 1, now, 11*time.Minute) st := stateWith(r) loc, err := time.LoadLocation("Europe/Oslo") @@ -196,6 +300,6 @@ func TestRenderDashboardParkedHonoursTimezone(t *testing.T) { } out := RenderDashboard(st, StoreConfig{Timezone: "Europe/Oslo"}) if !strings.Contains(out, fmtStamp(r.RetryAt, loc)) { - t.Errorf("parked retry_at not rendered in Europe/Oslo:\n%s", out) + t.Errorf("ready time not rendered in Europe/Oslo:\n%s", out) } } diff --git a/internal/state/state.go b/internal/state/state.go index 0e6887a..d51761f 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -596,6 +596,89 @@ func (s *State) QueuedRounds(now time.Time) []Round { return out } +// Queue-entry wait reasons. A waiting round is held by exactly one of these +// (or by nothing, in which case it is next up). +const ( + WaitCoolingDown = "cooling down" // this round's own RetryAt has not passed + WaitAccountBlocked = "account blocked" // the CodeRabbit account quota window dominates + WaitSlotBusy = "slot busy" // another PR's review holds the fire slot +) + +// QueueEntry is one round waiting for a fire, plus when it can actually fire +// and what is holding it. It is a VIEW over Rounds + Account — never persisted, +// so it carries no schema obligations. +type QueueEntry struct { + Round + // ReadyAt is the earliest time this round may fire. Zero means "now". + ReadyAt time.Time + // Why is the gate holding it: one of the Wait* constants, or "" when the + // round is simply next up. + Why string +} + +// roundReadyAt is a waiting round's OWN not-before time: RetryAt while it is +// cooling down, zero (ready now) while it is merely queued. +func roundReadyAt(r Round) time.Time { + if r.Phase == PhaseAwaitingRetry && r.RetryAt != nil { + return r.RetryAt.UTC() + } + return time.Time{} +} + +// Queue returns every round waiting for a fire — queued AND awaiting_retry — in +// the order they will actually reach the slot: by ready time, then by Seq. +// +// There is only one queue. A cooling-down round is not a different species of +// work, it is a queued round with a not-before time, and rendering it as a +// separate list left "nothing queued" and "two PRs parked until 00:07Z" looking +// identical. Ordering by (ReadyAt, Seq) reproduces what firing actually does: a +// round whose window has not opened cannot precede a ready one, and among ready +// rounds NextEligible takes the lowest Seq. +// +// ReadyAt folds in the account-wide quota block, because DecideFire gates on it +// too — showing a round's own RetryAt alone promises a time the fire gate will +// not honour once CodeRabbit extends the window. This is the same max() that +// AccountBlockedUntil computes for the wait path. +// +// MinInterval is deliberately NOT reflected: it is sub-minute pacing, so +// surfacing it would only churn DashboardSHA on every render. +func (s *State) Queue(now time.Time) []QueueEntry { + var blocked time.Time + if s.Account.BlockedUntil != nil && s.Account.BlockedUntil.After(now) { + blocked = s.Account.BlockedUntil.UTC() + } + slotBusy := s.SlotRound() != nil + + var out []QueueEntry + for _, r := range s.Rounds { + if r.Phase != PhaseQueued && r.Phase != PhaseAwaitingRetry { + continue + } + own := roundReadyAt(r) + e := QueueEntry{Round: r, ReadyAt: own} + switch { + case blocked.After(own) && blocked.After(now): + e.ReadyAt = blocked + e.Why = WaitAccountBlocked + case own.After(now): + e.Why = WaitCoolingDown + case slotBusy: + e.Why = WaitSlotBusy + } + if !e.ReadyAt.After(now) { + e.ReadyAt = time.Time{} // ready now + } + out = append(out, e) + } + sort.Slice(out, func(i, j int) bool { + if !out[i].ReadyAt.Equal(out[j].ReadyAt) { + return out[i].ReadyAt.Before(out[j].ReadyAt) + } + return out[i].Seq < out[j].Seq + }) + return out +} + // Normalize repairs invariants after load: map init, expired retry windows // (awaiting_retry with a passed RetryAt is simply fire-eligible; nothing to // do), and a FireSlot pointing at a round that no longer holds it. From 7d3a0ee8008c268e97f1d6ddfefd322f08b682cc Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 14:08:43 +0200 Subject: [PATCH 03/13] Make the ready column a promise firing will keep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased onto the next/wait work and addressed the review round. The queue table advertises when each round will fire. Three cases printed a time the fire gate would have refused: - **Pacing was left out.** DecideFire rejects every round until LastFired + CRQ_MIN_INTERVAL, so a round was shown "ready now" that firing would refuse for up to another 90 seconds — or far longer, since the interval is configurable. It was excluded as sub-minute churn, which was wrong on both counts: it is an absolute boundary, so it does not churn between renders. - **Gates did not compose.** The first matching branch won, so a one-minute cooldown inside a two-hour account block advertised the cooldown. The ready time is now the latest of every gate, and the reason is whichever one binds. - **A slot held by another PR was shown as "ready now".** Its release time is unknowable, so the column stays empty and the reason carries the truth. Also: a `reserved` round with no FireSlot behind it was reported as "Awaiting feedback". It has not posted its command, so no feedback can be coming, and Pump cannot advance it without the slot — the reader was sent looking for a review nobody requested. It is named as a stranded reservation now. MinInterval reaches the renderer through StoreConfig, which is exactly what that struct is for: the fields the store and dashboard need from crq's Config. --- internal/crq/init.go | 2 +- internal/crq/state.go | 5 +- internal/state/dashboard.go | 16 +++-- internal/state/dashboard_test.go | 117 +++++++++++++++++++++++++++++-- internal/state/state.go | 48 +++++++++---- internal/state/store.go | 5 +- 6 files changed, 166 insertions(+), 27 deletions(-) diff --git a/internal/crq/init.go b/internal/crq/init.go index 99420c7..ecc7a56 100644 --- a/internal/crq/init.go +++ b/internal/crq/init.go @@ -43,7 +43,7 @@ func Init(ctx context.Context, cfg Config, gh *ghapi.GitHub, store StateStore) ( if err != nil { return InitResult{}, err } - issue, err := gh.CreateIssue(ctx, cfg.GateRepo, renderTitle(state), body) + issue, err := gh.CreateIssue(ctx, cfg.GateRepo, renderTitle(state, cfg), body) if err != nil { return InitResult{}, err } diff --git a/internal/crq/state.go b/internal/crq/state.go index fd6843c..be37562 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -49,6 +49,7 @@ func (c Config) storeConfig() StoreConfig { Timezone: c.Timezone, Scope: c.Scope, CoReviewers: c.coReviewerSummary(), + MinInterval: c.MinInterval, } } @@ -96,7 +97,9 @@ func DefaultState(cfg Config) State { func renderDashboard(st State, cfg Config) string { return crqstate.RenderDashboard(st, cfg.storeConfig()) } -func renderTitle(st State) string { return crqstate.RenderTitle(st) } +func renderTitle(st State, cfg Config) string { + return crqstate.RenderTitle(st, cfg.storeConfig()) +} func issueBody(st State, cfg Config) (string, error) { return crqstate.IssueBody(st, cfg.storeConfig()) } diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 4008725..7706f7b 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -143,7 +143,7 @@ func dash(s string) string { func RenderDashboard(st State, cfg StoreConfig) string { loc := dashboardLoc(cfg) now := time.Now().UTC() - queue := st.Queue(now) + queue := st.Queue(now, cfg.MinInterval) inFlight := inFlightRounds(st) slot := st.SlotRound() blocked := st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now) @@ -157,7 +157,15 @@ func RenderDashboard(st State, cfg StoreConfig) string { case slot != nil: fmt.Fprintf(&b, "### 🟡 Reviewing %s#%d\n\n", slot.Repo, slot.PR) case len(inFlight) > 0: - fmt.Fprintf(&b, "### 🟡 Awaiting feedback for %s#%d\n\n", inFlight[0].Repo, inFlight[0].PR) + // A reserved round has not posted its command yet, so no feedback can be + // on its way — and with no FireSlot behind it, Pump cannot move it either. + // Calling that a feedback wait sends the reader looking for a review that + // was never requested. + if front := inFlight[0]; front.Phase == PhaseReserved { + fmt.Fprintf(&b, "### 🟠 Stranded reservation on %s#%d — no fire slot backs it\n\n", front.Repo, front.PR) + } else { + fmt.Fprintf(&b, "### 🟡 Awaiting feedback for %s#%d\n\n", inFlight[0].Repo, inFlight[0].PR) + } case len(queue) > 0: // Nothing ready yet is still queued work, never idle — say when the front // of the queue opens instead of leaving the reader to guess. @@ -250,9 +258,9 @@ func RenderDashboard(st State, cfg StoreConfig) string { // RenderTitle summarizes the state for the dashboard issue title. The queue // count is the WHOLE queue, cooling-down rounds included: a state whose only // work is not yet fire-eligible is queued, never idle. -func RenderTitle(st State) string { +func RenderTitle(st State, cfg StoreConfig) string { now := time.Now().UTC() - queue := len(st.Queue(now)) + queue := len(st.Queue(now, cfg.MinInterval)) switch { case st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now): return fmt.Sprintf("🐰 crq — blocked · queue %d", queue) diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 4d376e1..ac80e95 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -93,7 +93,7 @@ func TestRenderDashboardCoolingDownOnly(t *testing.T) { t.Errorf("header does not report the next ready time:\n%s", out) } - if got, want := RenderTitle(st), "🐰 crq — 2 queued"; got != want { + if got, want := RenderTitle(st, StoreConfig{}), "🐰 crq — 2 queued"; got != want { t.Errorf("RenderTitle = %q, want %q", got, want) } } @@ -110,7 +110,7 @@ func TestRenderDashboardEmpty(t *testing.T) { if !strings.Contains(out, "## 🔬 In flight — 0\n\n_None._") { t.Errorf("empty state lost its empty in-flight section:\n%s", out) } - if got, want := RenderTitle(st), "🐰 crq — idle"; got != want { + if got, want := RenderTitle(st, StoreConfig{}), "🐰 crq — idle"; got != want { t.Errorf("RenderTitle = %q, want %q", got, want) } } @@ -126,7 +126,7 @@ func TestQueueOrdersByReadyThenSeq(t *testing.T) { coolingRound("kristofferr/d", 4, 4, now, -2*time.Minute), // window already open ) - q := st.Queue(now) + q := st.Queue(now, 0) var got []int for _, e := range q { got = append(got, e.PR) @@ -161,7 +161,7 @@ func TestQueueAccountBlockDominatesRetryAt(t *testing.T) { blockedUntil := now.Add(44 * time.Minute) st.Account.BlockedUntil = &blockedUntil - q := st.Queue(now) + q := st.Queue(now, 0) if len(q) != 1 { t.Fatalf("Queue returned %d entries, want 1", len(q)) } @@ -193,7 +193,7 @@ func TestQueueSlotBusy(t *testing.T) { st := stateWith(holder, queuedRound("kristofferr/b", 2, 2, now)) st.FireSlot = &FireSlot{Key: Key(holder.Repo, holder.PR), Token: "tok"} - q := st.Queue(now) + q := st.Queue(now, 0) if len(q) != 1 || q[0].PR != 2 { t.Fatalf("Queue = %+v, want only the queued round", q) } @@ -303,3 +303,110 @@ func TestRenderDashboardQueueHonoursTimezone(t *testing.T) { t.Errorf("ready time not rendered in Europe/Oslo:\n%s", out) } } + +// The dashboard's "ready" column is a promise about when firing will accept a +// round, so every gate DecideFire applies has to be reflected. These three cases +// each rendered "ready: now" for a round the fire gate would have refused. + +// Pacing: after a fire, DecideFire rejects everything until LastFired + +// MinInterval. Excluding it as "sub-minute churn" was wrong twice over — it is an +// absolute boundary, so it does not churn between renders, and CRQ_MIN_INTERVAL +// can be configured far beyond 90s. +func TestQueueReflectsThePacingGate(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now)) + fired := now.Add(-30 * time.Second) + st.LastFired = &fired + + q := st.Queue(now, 90*time.Second) + if len(q) != 1 { + t.Fatalf("Queue = %d entries, want 1", len(q)) + } + if want := fired.Add(90 * time.Second); !q[0].ReadyAt.Equal(want) { + t.Errorf("ReadyAt = %v, want the pacing boundary %s", q[0].ReadyAt, want) + } + if q[0].Why != WaitPacing { + t.Errorf("Why = %q, want %q", q[0].Why, WaitPacing) + } + + // Once the interval has elapsed it stops being a gate at all. + if q := st.Queue(now.Add(2*time.Minute), 90*time.Second); len(q) != 1 || !q[0].ReadyAt.IsZero() { + t.Errorf("an elapsed interval must not gate: %+v", q) + } +} + +// A slot held by another PR has no knowable release time. Leaving ReadyAt empty +// rendered as "now", which is the one thing that is definitely false. +func TestQueueDoesNotClaimSlotBlockedRoundsAreReadyNow(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now), queuedRound("kristofferr/b", 2, 2, now)) + st.FireSlot = &FireSlot{Key: "kristofferr/b#2", Token: "tok", Since: now.Add(-time.Minute)} + st.Rounds["kristofferr/b#2"] = func() Round { + r := st.Rounds["kristofferr/b#2"] + r.Phase = PhaseFired + r.Token = "tok" + return r + }() + + for _, e := range st.Queue(now, 0) { + if e.Why != WaitSlotBusy { + continue + } + if !e.ReadyAt.IsZero() { + t.Errorf("a slot-blocked entry must not carry a ready time, got %s", e.ReadyAt) + } + } +} + +// A reserved round has not posted its command, so no feedback can be coming — +// and with no FireSlot behind it, Pump cannot advance it either. Reporting a +// feedback wait sent the reader looking for a review nobody requested. +func TestRenderNamesAStrandedReservation(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now)) + st.Rounds["kristofferr/a#1"] = func() Round { + r := st.Rounds["kristofferr/a#1"] + r.Phase = PhaseReserved + reserved := now.Add(-time.Minute) + r.ReservedAt = &reserved + return r + }() + + got := RenderDashboard(st, StoreConfig{}) + if strings.Contains(got, "Awaiting feedback") { + t.Error("a reserved round with no fire slot is not a feedback wait") + } + if !strings.Contains(got, "Stranded reservation") { + t.Errorf("the stranded reservation must be named, got:\n%s", got) + } +} + +// Gates compose: the ready time is the latest of them, and the reason is +// whichever one binds. Taking the first that matched let a short cooldown hide a +// long account block, advertising a time firing would refuse for hours. +func TestQueueTakesTheLatestBindingGate(t *testing.T) { + now := time.Now().UTC() + st := stateWith(coolingRound("kristofferr/a", 1, 1, now, time.Minute)) + blocked := now.Add(2 * time.Hour) + st.Account.BlockedUntil = &blocked + + q := st.Queue(now, 0) + if len(q) != 1 { + t.Fatalf("Queue = %d entries, want 1", len(q)) + } + if !q[0].ReadyAt.Equal(blocked.UTC()) { + t.Errorf("ReadyAt = %v, want the dominating account block %s", q[0].ReadyAt, blocked.UTC()) + } + if q[0].Why != WaitAccountBlocked { + t.Errorf("Why = %q, want %q", q[0].Why, WaitAccountBlocked) + } + + // And the other way round: a pacing boundary beyond a short cooldown binds. + st2 := stateWith(coolingRound("kristofferr/b", 2, 2, now, time.Second)) + fired := now + st2.LastFired = &fired + q2 := st2.Queue(now, 10*time.Minute) + if len(q2) != 1 || q2[0].Why != WaitPacing || !q2[0].ReadyAt.Equal(fired.Add(10*time.Minute).UTC()) { + t.Errorf("pacing must bind past a shorter cooldown, got %+v", q2) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index d51761f..eb26188 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -602,6 +602,7 @@ const ( WaitCoolingDown = "cooling down" // this round's own RetryAt has not passed WaitAccountBlocked = "account blocked" // the CodeRabbit account quota window dominates WaitSlotBusy = "slot busy" // another PR's review holds the fire slot + WaitPacing = "pacing" // LastFired + CRQ_MIN_INTERVAL has not passed ) // QueueEntry is one round waiting for a fire, plus when it can actually fire @@ -640,33 +641,50 @@ func roundReadyAt(r Round) time.Time { // not honour once CodeRabbit extends the window. This is the same max() that // AccountBlockedUntil computes for the wait path. // -// MinInterval is deliberately NOT reflected: it is sub-minute pacing, so -// surfacing it would only churn DashboardSHA on every render. -func (s *State) Queue(now time.Time) []QueueEntry { +// minInterval is folded in too. It is DecideFire's pacing gate, so leaving it out +// rendered a round "ready: now" that firing would refuse for up to another +// CRQ_MIN_INTERVAL — 90s by default and configurable far longer. It is an +// absolute boundary (LastFired + minInterval), not a countdown, so surfacing it +// does not churn DashboardSHA between renders. +func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { var blocked time.Time if s.Account.BlockedUntil != nil && s.Account.BlockedUntil.After(now) { blocked = s.Account.BlockedUntil.UTC() } slotBusy := s.SlotRound() != nil + // The pacing gate applies to whichever round fires next, so it bounds every + // entry's earliest possible start. + var paced time.Time + if minInterval > 0 && s.LastFired != nil { + if at := s.LastFired.Add(minInterval).UTC(); at.After(now) { + paced = at + } + } var out []QueueEntry for _, r := range s.Rounds { if r.Phase != PhaseQueued && r.Phase != PhaseAwaitingRetry { continue } - own := roundReadyAt(r) - e := QueueEntry{Round: r, ReadyAt: own} - switch { - case blocked.After(own) && blocked.After(now): - e.ReadyAt = blocked - e.Why = WaitAccountBlocked - case own.After(now): - e.Why = WaitCoolingDown - case slotBusy: - e.Why = WaitSlotBusy + // Every gate is a lower bound on when firing will accept this round, so + // the ready time is the LATEST of them and the reason is whichever one + // binds. Picking the first that matched let a later boundary hide behind + // an earlier one — a round cooling down for a minute inside a two-hour + // account block was advertised at the wrong time entirely. + e := QueueEntry{Round: r} + gate := func(at time.Time, why string) { + if at.After(now) && at.After(e.ReadyAt) { + e.ReadyAt, e.Why = at, why + } } - if !e.ReadyAt.After(now) { - e.ReadyAt = time.Time{} // ready now + gate(roundReadyAt(r), WaitCoolingDown) + gate(blocked, WaitAccountBlocked) + gate(paced, WaitPacing) + if e.ReadyAt.IsZero() && slotBusy { + // No time gate binds, but another PR holds the account-wide slot and + // its release time is unknowable. The Why column carries that; the + // ready column must stay empty rather than claim "now". + e.Why = WaitSlotBusy } out = append(out, e) } diff --git a/internal/state/store.go b/internal/state/store.go index e67e557..c8d8e5e 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -42,6 +42,9 @@ type StoreConfig struct { // bots ("" hides the dashboard row, keeping co-bot-less dashboards // byte-identical). CoReviewers string + // MinInterval is DecideFire's pacing gate. The dashboard needs it so a queue + // entry is not advertised as ready before firing would actually accept it. + MinInterval time.Duration } func (c StoreConfig) requireState() error { @@ -267,7 +270,7 @@ func (s *GitStateStore) SyncDashboard(ctx context.Context, st State) error { if err != nil { return err } - title := RenderTitle(st) + title := RenderTitle(st, s.cfg) // Held across read-then-write so two concurrent syncs cannot both observe a // stale issue and both write it. From a2410dca7b080836954fe164f8f93e786cde8118 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 14:34:30 +0200 Subject: [PATCH 04/13] Project pacing along the queue, and let a held slot outrank a timestamp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all the ready column still overstating readiness. **Pacing is serial and was applied in parallel.** The current LastFired + MinInterval boundary was applied to every entry independently, so with a free slot and three ready rounds all three read "ready now" — but firing the front pushes the next out by a whole interval. It is now projected along the queue: each place in line adds an interval, so only the front claims now. **A held fire slot now outranks every projected time.** It was recorded only when no timed gate had produced a ReadyAt, so a cooldown or pacing boundary was shown as a promise while the round could not fire at all. Its release time is unknowable, so no timestamp there is honest. **A stranded reservation is found wherever it sits.** In flight is ordered by fire time, so an older reviewing round hid a later slot-less reserved round and the header reported a plain feedback wait — and a reviewing round alongside a stranded one is the normal shape, not an edge case. --- internal/state/dashboard.go | 20 ++++++++- internal/state/dashboard_test.go | 76 ++++++++++++++++++++++++++++++++ internal/state/state.go | 32 +++++++++++--- 3 files changed, 120 insertions(+), 8 deletions(-) diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 7706f7b..ba08ac0 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -16,6 +16,18 @@ const ( crqProjectURL = "https://github.com/kristofferR/coderabbit-queue" ) +// firstStranded finds an in-flight round that reserved the slot but no longer +// holds it: it cannot receive feedback (no command was posted) and Pump cannot +// advance it (no slot), so it needs naming wherever it sits in the list. +func firstStranded(inFlight []Round) *Round { + for i := range inFlight { + if inFlight[i].Phase == PhaseReserved { + return &inFlight[i] + } + } + return nil +} + func joinScope(scope []string) string { return strings.Join(scope, ",") } @@ -161,8 +173,12 @@ func RenderDashboard(st State, cfg StoreConfig) string { // on its way — and with no FireSlot behind it, Pump cannot move it either. // Calling that a feedback wait sends the reader looking for a review that // was never requested. - if front := inFlight[0]; front.Phase == PhaseReserved { - fmt.Fprintf(&b, "### 🟠 Stranded reservation on %s#%d — no fire slot backs it\n\n", front.Repo, front.PR) + // + // Look past the first row: in flight is ordered by fire time, so an older + // reviewing round hides a later stranded reservation — and a reviewing + // round alongside a stranded one is the normal shape, not the exception. + if stranded := firstStranded(inFlight); stranded != nil { + fmt.Fprintf(&b, "### 🟠 Stranded reservation on %s#%d — no fire slot backs it\n\n", stranded.Repo, stranded.PR) } else { fmt.Fprintf(&b, "### 🟡 Awaiting feedback for %s#%d\n\n", inFlight[0].Repo, inFlight[0].PR) } diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index ac80e95..6ef10fd 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -410,3 +410,79 @@ func TestQueueTakesTheLatestBindingGate(t *testing.T) { t.Errorf("pacing must bind past a shorter cooldown, got %+v", q2) } } + +// Pacing is serial: firing the front of the queue pushes the next one out by a +// whole interval. Applying the current boundary to every entry independently made +// two ready rounds both read "ready now", when only the first was true. +func TestQueueProjectsPacingAlongTheQueue(t *testing.T) { + now := time.Now().UTC() + st := stateWith( + queuedRound("kristofferr/a", 1, 1, now), + queuedRound("kristofferr/b", 2, 2, now), + queuedRound("kristofferr/c", 3, 3, now), + ) + + q := st.Queue(now, 90*time.Second) + if len(q) != 3 { + t.Fatalf("Queue = %d entries, want 3", len(q)) + } + if !q[0].ReadyAt.IsZero() { + t.Errorf("the front of the queue is ready now, got %s", q[0].ReadyAt) + } + for i := 1; i < len(q); i++ { + want := now.Add(time.Duration(i) * 90 * time.Second) + if !q[i].ReadyAt.Equal(want) { + t.Errorf("entry %d ReadyAt = %v, want %s (one interval per place in line)", i, q[i].ReadyAt, want) + } + if q[i].Why != WaitPacing { + t.Errorf("entry %d Why = %q, want %q", i, q[i].Why, WaitPacing) + } + } +} + +// A slot held by another PR has no knowable release time, so it outranks every +// projected timestamp — otherwise a cooldown or pacing boundary is presented as a +// promise while the round cannot fire at all. +func TestQueueLetsSlotBusyDominateATimedGate(t *testing.T) { + now := time.Now().UTC() + st := stateWith(coolingRound("kristofferr/a", 1, 1, now, 5*time.Minute), queuedRound("kristofferr/b", 2, 2, now)) + st.FireSlot = &FireSlot{Key: "kristofferr/b#2", Token: "tok", Since: now.Add(-time.Minute)} + st.Rounds["kristofferr/b#2"] = func() Round { + r := st.Rounds["kristofferr/b#2"] + r.Phase, r.Token = PhaseFired, "tok" + return r + }() + + for _, e := range st.Queue(now, 0) { + if e.Why != WaitSlotBusy { + t.Errorf("entry %d: Why = %q, want %q while the slot is held", e.PR, e.Why, WaitSlotBusy) + } + if !e.ReadyAt.IsZero() { + t.Errorf("entry %d: ReadyAt = %s, want no promised time", e.PR, e.ReadyAt) + } + } +} + +// In flight is ordered by fire time, so an older reviewing round would hide a +// later stranded reservation — and that pairing is the normal shape. +func TestRenderFindsAStrandedReservationBehindAReviewingRound(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now), queuedRound("kristofferr/b", 2, 2, now)) + fired := now.Add(-10 * time.Minute) + st.Rounds["kristofferr/a#1"] = func() Round { + r := st.Rounds["kristofferr/a#1"] + r.Phase, r.FiredAt = PhaseReviewing, &fired + return r + }() + reserved := now.Add(-time.Minute) + st.Rounds["kristofferr/b#2"] = func() Round { + r := st.Rounds["kristofferr/b#2"] + r.Phase, r.ReservedAt = PhaseReserved, &reserved + return r + }() + + got := RenderDashboard(st, StoreConfig{}) + if !strings.Contains(got, "Stranded reservation on kristofferr/b#2") { + t.Errorf("the stranded reservation must be named even behind a reviewing round, got:\n%s", got) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index eb26188..7da297f 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -680,12 +680,6 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { gate(roundReadyAt(r), WaitCoolingDown) gate(blocked, WaitAccountBlocked) gate(paced, WaitPacing) - if e.ReadyAt.IsZero() && slotBusy { - // No time gate binds, but another PR holds the account-wide slot and - // its release time is unknowable. The Why column carries that; the - // ready column must stay empty rather than claim "now". - e.Why = WaitSlotBusy - } out = append(out, e) } sort.Slice(out, func(i, j int) bool { @@ -694,6 +688,32 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { } return out[i].Seq < out[j].Seq }) + + // Pacing is serial, so it has to be projected along the queue rather than + // applied to each entry independently. With a free slot and two ready rounds, + // both looked "ready now" — but firing the first pushes the second out by a + // whole interval, so only the front of the queue was telling the truth. + if minInterval > 0 { + earliest := paced + for i := range out { + if earliest.After(now) && earliest.After(out[i].ReadyAt) { + out[i].ReadyAt, out[i].Why = earliest, WaitPacing + } + start := out[i].ReadyAt + if start.Before(now) || start.IsZero() { + start = now + } + earliest = start.Add(minInterval) + } + } + + // A slot held by another PR outranks every projected time: its release is + // unknowable, so any timestamp here would be a guess presented as a promise. + if slotBusy { + for i := range out { + out[i].ReadyAt, out[i].Why = time.Time{}, WaitSlotBusy + } + } return out } From 55855c7c31b1fccd72273e0fd03f1358cdc55d63 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 14:49:53 +0200 Subject: [PATCH 05/13] Show only times crq can know, and order the queue the way firing does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round two, and all three findings are consequences of round one's pacing fix. **Projected times were anchored to render time.** Deriving "front plus one interval" from `now` gave an unchanged state a different body — and therefore a different DashboardSHA — every minute, which would have turned every render back into a dashboard write. The original code's rationale for omitting pacing was half right after all: an absolute LastFired + MinInterval boundary is stable and worth showing, a projection from render time is not. Rounds behind the front now say so instead of naming a time crq cannot know until the round ahead of them fires. **The order disagreed with what firing does.** NextEligible takes the lowest Seq among rounds eligible at that moment, so a round still cooling down when the front fires can overtake a ready round with a higher Seq. With seq 1 ready, seq 2 cooling for 60s and seq 3 ready on a 90s interval, the queue really runs 1, 2, 3 while sorting by ready time rendered 1, 3, 2. The order is simulated now — advance to when something can run, take the lowest Seq that can, add an interval, repeat — which is the same rule, so the two cannot drift. **The title contradicted the body.** The body names a stranded reservation; the title still reported a feedback wait for a round that never posted a command. The pacing test from round one asserted the churning behaviour, so it now states the corrected rule and additionally pins stability: the same state rendered a minute later must produce the same queue. --- internal/state/dashboard.go | 5 ++ internal/state/dashboard_test.go | 81 +++++++++++++++++++++++++++----- internal/state/state.go | 70 ++++++++++++++++++++------- 3 files changed, 128 insertions(+), 28 deletions(-) diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index ba08ac0..8834ad2 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -283,6 +283,11 @@ func RenderTitle(st State, cfg StoreConfig) string { case st.SlotRound() != nil: return fmt.Sprintf("🐰 crq — reviewing #%d · queue %d", st.SlotRound().PR, queue) case len(inFlightRounds(st)) > 0: + // The body names a stranded reservation; the title must not contradict it + // by reporting a feedback wait for a round that posted no command. + if stranded := firstStranded(inFlightRounds(st)); stranded != nil { + return fmt.Sprintf("🐰 crq — stranded #%d · queue %d", stranded.PR, queue) + } return fmt.Sprintf("🐰 crq — awaiting feedback · queue %d", queue) case queue > 0: return fmt.Sprintf("🐰 crq — %d queued", queue) diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 6ef10fd..546988d 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -411,10 +411,12 @@ func TestQueueTakesTheLatestBindingGate(t *testing.T) { } } -// Pacing is serial: firing the front of the queue pushes the next one out by a -// whole interval. Applying the current boundary to every entry independently made -// two ready rounds both read "ready now", when only the first was true. -func TestQueueProjectsPacingAlongTheQueue(t *testing.T) { +// Pacing is serial, so a round behind the front cannot start until the front +// fires — but crq does not know when that is, and a time derived from render +// time would give an unchanged state a different DashboardSHA every minute (the +// churn that argued against surfacing pacing at all). So the queue says "behind +// an earlier round" rather than naming a time it cannot know. +func TestQueueNamesRoundsBehindTheFrontWithoutInventingATime(t *testing.T) { now := time.Now().UTC() st := stateWith( queuedRound("kristofferr/a", 1, 1, now), @@ -426,16 +428,52 @@ func TestQueueProjectsPacingAlongTheQueue(t *testing.T) { if len(q) != 3 { t.Fatalf("Queue = %d entries, want 3", len(q)) } - if !q[0].ReadyAt.IsZero() { - t.Errorf("the front of the queue is ready now, got %s", q[0].ReadyAt) + if !q[0].ReadyAt.IsZero() || q[0].Why != "" { + t.Errorf("the front is ready now with no gate, got ready=%v why=%q", q[0].ReadyAt, q[0].Why) } for i := 1; i < len(q); i++ { - want := now.Add(time.Duration(i) * 90 * time.Second) - if !q[i].ReadyAt.Equal(want) { - t.Errorf("entry %d ReadyAt = %v, want %s (one interval per place in line)", i, q[i].ReadyAt, want) + if !q[i].ReadyAt.IsZero() { + t.Errorf("entry %d invented a ready time %s; only absolute gates may be shown", i, q[i].ReadyAt) } - if q[i].Why != WaitPacing { - t.Errorf("entry %d Why = %q, want %q", i, q[i].Why, WaitPacing) + if q[i].Why != WaitBehind { + t.Errorf("entry %d Why = %q, want %q", i, q[i].Why, WaitBehind) + } + } + + // Stability is the point: the same state rendered a minute later must produce + // the same queue, or every render becomes a dashboard write. + later := st.Queue(now.Add(time.Minute), 90*time.Second) + for i := range q { + if !later[i].ReadyAt.Equal(q[i].ReadyAt) || later[i].Why != q[i].Why { + t.Errorf("entry %d changed with render time: %v/%q -> %v/%q", + i, q[i].ReadyAt, q[i].Why, later[i].ReadyAt, later[i].Why) + } + } +} + +// The order has to be what firing will actually do. NextEligible takes the lowest +// Seq among rounds eligible at that moment, so a round still cooling down when the +// front fires can overtake a ready round with a higher Seq. Sorting by ready time +// alone rendered 1, 3, 2 where the queue really runs 1, 2, 3. +func TestQueueOrderMatchesWhatFiringWillDo(t *testing.T) { + now := time.Now().UTC() + st := stateWith( + queuedRound("kristofferr/a", 1, 1, now), // ready now + coolingRound("kristofferr/b", 2, 2, now, 60*time.Second), // eligible before the next slot + queuedRound("kristofferr/c", 3, 3, now), // ready now, higher Seq + ) + + var got []int + for _, e := range st.Queue(now, 90*time.Second) { + got = append(got, e.PR) + } + want := []int{1, 2, 3} + if len(got) != len(want) { + t.Fatalf("Queue = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("Queue = %v, want %v (b is eligible by the time the slot frees)", got, want) } } } @@ -486,3 +524,24 @@ func TestRenderFindsAStrandedReservationBehindAReviewingRound(t *testing.T) { t.Errorf("the stranded reservation must be named even behind a reviewing round, got:\n%s", got) } } + +// The title is what a human sees first, so it must not contradict the body: a +// reserved round whose slot was cleared cannot be awaiting feedback. +func TestRenderTitleNamesAStrandedReservation(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now)) + reserved := now.Add(-time.Minute) + st.Rounds["kristofferr/a#1"] = func() Round { + r := st.Rounds["kristofferr/a#1"] + r.Phase, r.ReservedAt = PhaseReserved, &reserved + return r + }() + + got := RenderTitle(st, StoreConfig{}) + if strings.Contains(got, "awaiting feedback") { + t.Errorf("title claims a feedback wait for a stranded reservation: %q", got) + } + if !strings.Contains(got, "stranded") { + t.Errorf("title = %q, want it to name the stranded reservation", got) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index 7da297f..787e86a 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -603,6 +603,7 @@ const ( WaitAccountBlocked = "account blocked" // the CodeRabbit account quota window dominates WaitSlotBusy = "slot busy" // another PR's review holds the fire slot WaitPacing = "pacing" // LastFired + CRQ_MIN_INTERVAL has not passed + WaitBehind = "behind an earlier round" ) // QueueEntry is one round waiting for a fire, plus when it can actually fire @@ -682,28 +683,63 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { gate(paced, WaitPacing) out = append(out, e) } - sort.Slice(out, func(i, j int) bool { - if !out[i].ReadyAt.Equal(out[j].ReadyAt) { - return out[i].ReadyAt.Before(out[j].ReadyAt) + // Order by SIMULATING what firing does, because pacing makes the naive sort + // disagree with it. NextEligible takes the lowest Seq among rounds that are + // eligible *at that moment*, so a round still cooling down when the front + // fires can become eligible before a ready round with a higher Seq — with + // seq 1 ready, seq 2 cooling for 60s and seq 3 ready on a 90s interval, the + // real order is 1, 2, 3, while sorting by ready time renders 1, 3, 2. + clock := now + if paced.After(clock) { + clock = paced + } + ordered := make([]QueueEntry, 0, len(out)) + remaining := out + for len(remaining) > 0 { + // Advance to when something can actually run. + soonest := time.Time{} + for _, e := range remaining { + if own := roundReadyAt(e.Round); own.After(clock) && (soonest.IsZero() || own.Before(soonest)) { + soonest = own + } + } + pick, picked := -1, int64(0) + for i, e := range remaining { + if roundReadyAt(e.Round).After(clock) { + continue + } + if pick < 0 || e.Seq < picked { + pick, picked = i, e.Seq + } + } + if pick < 0 { + if soonest.IsZero() { + break // nothing can ever run; keep the rest in Seq order below + } + clock = soonest + continue + } + ordered = append(ordered, remaining[pick]) + remaining = append(remaining[:pick:pick], remaining[pick+1:]...) + if minInterval > 0 { + clock = clock.Add(minInterval) } - return out[i].Seq < out[j].Seq - }) + } + // Anything unreachable keeps a stable order rather than map order. + sort.Slice(remaining, func(i, j int) bool { return remaining[i].Seq < remaining[j].Seq }) + out = append(ordered, remaining...) - // Pacing is serial, so it has to be projected along the queue rather than - // applied to each entry independently. With a free slot and two ready rounds, - // both looked "ready now" — but firing the first pushes the second out by a - // whole interval, so only the front of the queue was telling the truth. + // Only ABSOLUTE times are shown. A projected "front + one interval" is + // anchored to render time, so an unchanged state would produce a different + // body — and therefore a different DashboardSHA — every minute, which is the + // churn that argued against surfacing pacing in the first place. A round + // queued behind another says so instead of naming a time crq cannot know + // until the round ahead of it actually fires. if minInterval > 0 { - earliest := paced for i := range out { - if earliest.After(now) && earliest.After(out[i].ReadyAt) { - out[i].ReadyAt, out[i].Why = earliest, WaitPacing - } - start := out[i].ReadyAt - if start.Before(now) || start.IsZero() { - start = now + if i > 0 && out[i].ReadyAt.IsZero() { + out[i].Why = WaitBehind } - earliest = start.Add(minInterval) } } From 06c534bc0fc77fc9c9a665023fced72b05bbf97f Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 15:00:28 +0200 Subject: [PATCH 06/13] Start the queue simulation where firing can resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round three, one finding. The simulation began at render time (or the pacing boundary), ignoring the account block — so with a lower-Seq round cooling for an hour, a ready higher-Seq round, and the account blocked for two hours, the queue rendered the ready one first. Both are eligible the moment the block clears, and NextEligible then takes the lowest Seq, so the real order is the other way round. The clock now starts at the later of now, the pacing boundary and the account block: the point where firing can actually resume. --- internal/state/dashboard_test.go | 22 ++++++++++++++++++++++ internal/state/state.go | 7 +++++++ 2 files changed, 29 insertions(+) diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 546988d..d724349 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -545,3 +545,25 @@ func TestRenderTitleNamesAStrandedReservation(t *testing.T) { t.Errorf("title = %q, want it to name the stranded reservation", got) } } + +// An account block that outlasts a cooldown makes both rounds eligible the moment +// it clears, and NextEligible then takes the lowest Seq. Simulating from render +// time instead put the ready higher-Seq round first. +func TestQueueOrdersFromWhenFiringResumes(t *testing.T) { + now := time.Now().UTC() + st := stateWith( + coolingRound("kristofferr/a", 1, 1, now, time.Hour), // lower Seq, cooling + queuedRound("kristofferr/b", 2, 2, now), // ready now, higher Seq + ) + blocked := now.Add(2 * time.Hour) + st.Account.BlockedUntil = &blocked + + q := st.Queue(now, 90*time.Second) + if len(q) != 2 { + t.Fatalf("Queue = %d entries, want 2", len(q)) + } + if q[0].PR != 1 { + t.Errorf("Queue order = [%d %d], want [1 2]: both are eligible when the block clears, so Seq decides", + q[0].PR, q[1].PR) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index 787e86a..ba61e2c 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -689,10 +689,17 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { // fires can become eligible before a ready round with a higher Seq — with // seq 1 ready, seq 2 cooling for 60s and seq 3 ready on a 90s interval, the // real order is 1, 2, 3, while sorting by ready time renders 1, 3, 2. + // Start where firing can actually resume, not at render time. An account block + // that outlasts a round's cooldown makes several rounds eligible together the + // moment it clears, and NextEligible then takes the lowest Seq — so ordering + // from now rendered a ready higher-Seq round ahead of one still cooling. clock := now if paced.After(clock) { clock = paced } + if blocked.After(clock) { + clock = blocked + } ordered := make([]QueueEntry, 0, len(out)) remaining := out for len(remaining) > 0 { From de9ff4e4545de848a6622c4944482efb6432a127 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 16:00:29 +0200 Subject: [PATCH 07/13] Stop the renderer undoing the model, and stop ordering what cannot be ordered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round four. The model had stopped promising times it could not keep; the view and two edge cases had not caught up. **The renderer turned "unknown" back into "now".** Queue clears the ready time for a round blocked behind another PR's slot, and the table printed every zero as "now" — so it read "ready: now | slot busy", contradicting itself across two columns. A gate whose end is unknowable now renders as unknown. **Followers repeated the front's boundary.** Two rounds sharing an account block both showed that same timestamp, but after the front fires pacing pushes the next out by another interval, so the second was advertising a moment firing will refuse. A follower may only show a time strictly later than the round ahead of it could finish, and the floor is built from absolute gates alone so nothing is anchored to render time. **With the slot held, the simulated ORDER was a guess too.** Not just the times: which round is picked when the holder releases depends on which cooldowns have elapsed by then, and that moment is unknowable. It falls back to Seq — the only ordering that is true whenever several rounds become eligible together. **A stranded reservation was hidden behind states that clear by themselves.** An account block or another PR's review resolves on its own; a reserved round with no slot behind it cannot be advanced by Pump at all. It is reported first now, in both the heading and the title. --- internal/state/dashboard.go | 39 ++++++++++---------- internal/state/dashboard_test.go | 61 ++++++++++++++++++++++++++++++++ internal/state/state.go | 26 ++++++++++++-- 3 files changed, 104 insertions(+), 22 deletions(-) diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 8834ad2..e9746fd 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -163,25 +163,20 @@ func RenderDashboard(st State, cfg StoreConfig) string { var b strings.Builder fmt.Fprintf(&b, "# 🐰 crq — CodeRabbit review queue\n\n") + stranded := firstStranded(inFlight) switch { + case stranded != nil: + // Reported before the transient states: a quota window or another PR's + // review clears on its own, but a reserved round with no slot behind it + // cannot be advanced by Pump at all, so hiding it behind them leaves + // permanently stuck work looking ordinary. + fmt.Fprintf(&b, "### 🟠 Stranded reservation on %s#%d — no fire slot backs it\n\n", stranded.Repo, stranded.PR) case blocked: fmt.Fprintf(&b, "### 🔴 Blocked — next review in ~%dm\n\n", minutesUntil(*st.Account.BlockedUntil, now)) case slot != nil: fmt.Fprintf(&b, "### 🟡 Reviewing %s#%d\n\n", slot.Repo, slot.PR) case len(inFlight) > 0: - // A reserved round has not posted its command yet, so no feedback can be - // on its way — and with no FireSlot behind it, Pump cannot move it either. - // Calling that a feedback wait sends the reader looking for a review that - // was never requested. - // - // Look past the first row: in flight is ordered by fire time, so an older - // reviewing round hides a later stranded reservation — and a reviewing - // round alongside a stranded one is the normal shape, not the exception. - if stranded := firstStranded(inFlight); stranded != nil { - fmt.Fprintf(&b, "### 🟠 Stranded reservation on %s#%d — no fire slot backs it\n\n", stranded.Repo, stranded.PR) - } else { - fmt.Fprintf(&b, "### 🟡 Awaiting feedback for %s#%d\n\n", inFlight[0].Repo, inFlight[0].PR) - } + fmt.Fprintf(&b, "### 🟡 Awaiting feedback for %s#%d\n\n", inFlight[0].Repo, inFlight[0].PR) case len(queue) > 0: // Nothing ready yet is still queued work, never idle — say when the front // of the queue opens instead of leaving the reader to guess. @@ -242,10 +237,17 @@ func RenderDashboard(st State, cfg StoreConfig) string { for i, e := range queue { // Absolute stamps only: a relative "in 11m" would re-hash the dashboard // on every render, and fmtStamp already honours CRQ_TZ. + // A zero ReadyAt means "now" only when nothing is holding the round. + // With a gate whose end is unknowable — a slot held elsewhere, or a + // round queued behind another — printing "now" contradicts the very + // column next to it. ready := "now" - if !e.ReadyAt.IsZero() { + switch { + case !e.ReadyAt.IsZero(): at := e.ReadyAt ready = fmtStamp(&at, loc) + case e.Why != "": + ready = "unknown" } fmt.Fprintf(&b, "| %d | [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %d | %s | `%s` |\n", i+1, e.Repo, e.PR, e.Repo, e.PR, e.Head, ready, dash(e.Why), @@ -278,16 +280,15 @@ func RenderTitle(st State, cfg StoreConfig) string { now := time.Now().UTC() queue := len(st.Queue(now, cfg.MinInterval)) switch { + case firstStranded(inFlightRounds(st)) != nil: + // Same precedence as the body: permanently stuck work outranks states that + // clear by themselves. + return fmt.Sprintf("🐰 crq — stranded #%d · queue %d", firstStranded(inFlightRounds(st)).PR, queue) case st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now): return fmt.Sprintf("🐰 crq — blocked · queue %d", queue) case st.SlotRound() != nil: return fmt.Sprintf("🐰 crq — reviewing #%d · queue %d", st.SlotRound().PR, queue) case len(inFlightRounds(st)) > 0: - // The body names a stranded reservation; the title must not contradict it - // by reporting a feedback wait for a round that posted no command. - if stranded := firstStranded(inFlightRounds(st)); stranded != nil { - return fmt.Sprintf("🐰 crq — stranded #%d · queue %d", stranded.PR, queue) - } return fmt.Sprintf("🐰 crq — awaiting feedback · queue %d", queue) case queue > 0: return fmt.Sprintf("🐰 crq — %d queued", queue) diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index d724349..d1cde66 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -567,3 +567,64 @@ func TestQueueOrdersFromWhenFiringResumes(t *testing.T) { q[0].PR, q[1].PR) } } + +// The model clears the ready time when a gate's end is unknowable; the renderer +// must not turn that zero back into "now" beside a "slot busy" reason. +func TestRenderShowsUnknownRatherThanNowForAnUnknowableGate(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now), queuedRound("kristofferr/b", 2, 2, now)) + st.FireSlot = &FireSlot{Key: "kristofferr/b#2", Token: "tok", Since: now.Add(-time.Minute)} + st.Rounds["kristofferr/b#2"] = func() Round { + r := st.Rounds["kristofferr/b#2"] + r.Phase, r.Token = PhaseFired, "tok" + return r + }() + + got := RenderDashboard(st, StoreConfig{}) + if strings.Contains(got, "now | slot busy") { + t.Errorf("a slot-blocked round must not read as ready now:\n%s", got) + } + if !strings.Contains(got, "unknown | slot busy") { + t.Errorf("want an unknown ready time beside the slot-busy reason, got:\n%s", got) + } +} + +// Sharing the front's boundary is not a later time: after the front fires, pacing +// pushes the next round out by another interval, so repeating that timestamp +// advertises a moment firing will refuse. +func TestQueueDoesNotRepeatTheFrontsBoundaryForFollowers(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now), queuedRound("kristofferr/b", 2, 2, now)) + blocked := now.Add(time.Hour) + st.Account.BlockedUntil = &blocked + + q := st.Queue(now, 90*time.Second) + if !q[0].ReadyAt.Equal(blocked.UTC()) { + t.Errorf("the front is gated by the account block, got %v", q[0].ReadyAt) + } + if !q[1].ReadyAt.IsZero() || q[1].Why != WaitBehind { + t.Errorf("the follower must not repeat the block boundary: ready=%v why=%q", q[1].ReadyAt, q[1].Why) + } +} + +// A stranded reservation cannot be advanced by Pump at all, so a quota window or +// another PR's review — both of which clear by themselves — must not hide it. +func TestStrandedReservationOutranksTransientStates(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now)) + reserved := now.Add(-time.Minute) + st.Rounds["kristofferr/a#1"] = func() Round { + r := st.Rounds["kristofferr/a#1"] + r.Phase, r.ReservedAt = PhaseReserved, &reserved + return r + }() + blocked := now.Add(time.Hour) + st.Account.BlockedUntil = &blocked + + if got := RenderDashboard(st, StoreConfig{}); !strings.Contains(got, "Stranded reservation") { + t.Errorf("an account block must not hide a stranded reservation:\n%s", got) + } + if got := RenderTitle(st, StoreConfig{}); !strings.Contains(got, "stranded") { + t.Errorf("title = %q, want the stranded reservation named", got) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index ba61e2c..5c49f85 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -742,20 +742,40 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { // churn that argued against surfacing pacing in the first place. A round // queued behind another says so instead of naming a time crq cannot know // until the round ahead of it actually fires. + // A follower may only show a time that is genuinely later than the round ahead + // of it could finish. Sharing the front's boundary is not that: after the front + // fires, pacing pushes the next one out by another interval, so repeating that + // same timestamp advertises a moment firing will refuse. The floor is built + // from ABSOLUTE gates only, so nothing here is anchored to render time. if minInterval > 0 { + floor := time.Time{} for i := range out { - if i > 0 && out[i].ReadyAt.IsZero() { + if i > 0 && !(floor.IsZero() || out[i].ReadyAt.After(floor)) { + out[i].ReadyAt, out[i].Why = time.Time{}, WaitBehind + } + if at := out[i].ReadyAt; !at.IsZero() { + floor = at.Add(minInterval) + } else { + floor = time.Time{} // ready now: the follower's start is unknowable + } + } + for i := 1; i < len(out); i++ { + if out[i].ReadyAt.IsZero() { out[i].Why = WaitBehind } } } - // A slot held by another PR outranks every projected time: its release is - // unknowable, so any timestamp here would be a guess presented as a promise. + // A slot held by another PR outranks everything. Its release time is + // unknowable, so neither a timestamp nor a simulated POSITION means anything: + // which round is picked depends on which cooldowns have elapsed by then. Fall + // back to Seq — the only ordering that is true whenever several rounds become + // eligible together — and say the slot is why. if slotBusy { for i := range out { out[i].ReadyAt, out[i].Why = time.Time{}, WaitSlotBusy } + sort.Slice(out, func(i, j int) bool { return out[i].Seq < out[j].Seq }) } return out } From 0e7707f738aeac85527ff6046a61bad5c4260976 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 16:10:37 +0200 Subject: [PATCH 08/13] Only call a reservation stranded when no slot backs it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round five, one finding, and a regression from round four: making a stranded reservation outrank every other state meant the happy path started reporting it. Every normal fire passes through PhaseReserved WITH a valid FireSlot while the command is being posted. Testing the phase alone therefore announced "no fire slot backs it" during ordinary operation — loudly, since that state now wins the heading and the title. Holding the slot is the whole distinction, so it is checked: a reserved round that IS the slot holder is mid-fire, and only one whose slot has gone is stranded. The test pins both directions, because the diagnostic is worth nothing if it fires on the path it is meant to distinguish from. --- internal/state/dashboard.go | 22 ++++++++++++++++------ internal/state/dashboard_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index e9746fd..6cfbce4 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -19,11 +19,21 @@ const ( // firstStranded finds an in-flight round that reserved the slot but no longer // holds it: it cannot receive feedback (no command was posted) and Pump cannot // advance it (no slot), so it needs naming wherever it sits in the list. -func firstStranded(inFlight []Round) *Round { +// +// Holding the slot is the whole distinction. Every normal fire passes through +// PhaseReserved WITH a valid slot while the command is being posted, so testing +// the phase alone reported the happy path as stuck — and loudly, since a stranded +// round now outranks every other state. +func firstStranded(st State, inFlight []Round) *Round { + slot := st.SlotRound() for i := range inFlight { - if inFlight[i].Phase == PhaseReserved { - return &inFlight[i] + if inFlight[i].Phase != PhaseReserved { + continue + } + if slot != nil && slot.Repo == inFlight[i].Repo && slot.PR == inFlight[i].PR { + continue // mid-fire, not stranded } + return &inFlight[i] } return nil } @@ -163,7 +173,7 @@ func RenderDashboard(st State, cfg StoreConfig) string { var b strings.Builder fmt.Fprintf(&b, "# 🐰 crq — CodeRabbit review queue\n\n") - stranded := firstStranded(inFlight) + stranded := firstStranded(st, inFlight) switch { case stranded != nil: // Reported before the transient states: a quota window or another PR's @@ -280,10 +290,10 @@ func RenderTitle(st State, cfg StoreConfig) string { now := time.Now().UTC() queue := len(st.Queue(now, cfg.MinInterval)) switch { - case firstStranded(inFlightRounds(st)) != nil: + case firstStranded(st, inFlightRounds(st)) != nil: // Same precedence as the body: permanently stuck work outranks states that // clear by themselves. - return fmt.Sprintf("🐰 crq — stranded #%d · queue %d", firstStranded(inFlightRounds(st)).PR, queue) + return fmt.Sprintf("🐰 crq — stranded #%d · queue %d", firstStranded(st, inFlightRounds(st)).PR, queue) case st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now): return fmt.Sprintf("🐰 crq — blocked · queue %d", queue) case st.SlotRound() != nil: diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index d1cde66..d0fec8d 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -628,3 +628,31 @@ func TestStrandedReservationOutranksTransientStates(t *testing.T) { t.Errorf("title = %q, want the stranded reservation named", got) } } + +// Every normal fire passes through PhaseReserved while its slot is held and the +// command is posted. Reporting that as stranded turns the happy path into a loud +// false alarm, now that stranded outranks every other state. +func TestReservedRoundHoldingTheSlotIsNotStranded(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now)) + reserved := now.Add(-time.Second) + st.Rounds["kristofferr/a#1"] = func() Round { + r := st.Rounds["kristofferr/a#1"] + r.Phase, r.ReservedAt, r.Token = PhaseReserved, &reserved, "tok" + return r + }() + st.FireSlot = &FireSlot{Key: "kristofferr/a#1", Token: "tok", Since: reserved} + + if got := RenderDashboard(st, StoreConfig{}); strings.Contains(got, "Stranded") { + t.Errorf("a mid-fire reservation must not read as stranded:\n%s", got) + } + if got := RenderTitle(st, StoreConfig{}); strings.Contains(got, "stranded") { + t.Errorf("title = %q, want no stranded claim mid-fire", got) + } + + // Once the slot is gone it really is stranded. + st.FireSlot = nil + if got := RenderDashboard(st, StoreConfig{}); !strings.Contains(got, "Stranded reservation") { + t.Errorf("a reservation with no slot must still be named:\n%s", got) + } +} From a7803c10d6b25739b9ebdb52e2820b4fc7b35d4e Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 16:21:03 +0200 Subject: [PATCH 09/13] Stop claiming an order while the slot decides it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round six, three findings. **A reservation sorted by when it was enqueued.** A reserved round has no FiredAt — the command is not posted yet — so the in-flight table fell through to EnqueuedAt and put a PR reserved seconds ago ahead of reviews fired an hour earlier, contradicting its own ordering. It sorts by ReservedAt now. **A co-only round was told to wait for the account window.** It spends no quota, so DecideFire resolves it before that gate ever applies; labelling it "account blocked until T" described a wait the next observation can end immediately. The window no longer gates a round already degraded to its co-reviewers. **The queue stopped claiming positions while the slot is held**, rather than claiming better ones. Two rounds ago that ordering became a simulation, then a Seq fallback; both were still guesses, because which round fires next depends on whose cooldown has elapsed at the moment the slot releases — and that moment is unknown. Refining the guess was the wrong move: the honest answer is that the order is not knowable yet, so the position column says so and the rows remain informative without pretending to a sequence. --- internal/state/dashboard.go | 21 +++++++++- internal/state/dashboard_test.go | 66 ++++++++++++++++++++++++++++++++ internal/state/state.go | 10 ++++- 3 files changed, 93 insertions(+), 4 deletions(-) diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 6cfbce4..b03710b 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "sort" + "strconv" "strings" "time" ) @@ -89,10 +90,18 @@ func inFlightRounds(st State) []Round { return out } +// firedAtOf is when a round entered its in-flight life, for ordering the in-flight +// table. A reserved round has no FiredAt — the command is not posted yet — so +// falling straight through to EnqueuedAt sorted a long-queued PR that was reserved +// seconds ago ahead of reviews fired much earlier, contradicting the table's own +// ordering. func firedAtOf(r Round) time.Time { if r.FiredAt != nil { return *r.FiredAt } + if r.ReservedAt != nil { + return *r.ReservedAt + } return r.EnqueuedAt } @@ -259,8 +268,16 @@ func RenderDashboard(st State, cfg StoreConfig) string { case e.Why != "": ready = "unknown" } - fmt.Fprintf(&b, "| %d | [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %d | %s | `%s` |\n", - i+1, e.Repo, e.PR, e.Repo, e.PR, e.Head, ready, dash(e.Why), + // Only claim a position when one is knowable. While another PR holds the + // slot, which round fires next depends on whose cooldown has elapsed by + // the moment it releases — and that moment is unknown, so every possible + // order (including Seq) is a guess. Say so instead of numbering it. + position := strconv.Itoa(i + 1) + if e.Why == WaitSlotBusy { + position = "—" + } + fmt.Fprintf(&b, "| %s | [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %d | %s | `%s` |\n", + position, e.Repo, e.PR, e.Repo, e.PR, e.Head, ready, dash(e.Why), e.Attempts, fmtStamp(&e.EnqueuedAt, loc), e.ByHost) } } diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index d0fec8d..3cfa7d2 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -656,3 +656,69 @@ func TestReservedRoundHoldingTheSlotIsNotStranded(t *testing.T) { t.Errorf("a reservation with no slot must still be named:\n%s", got) } } + +// A reserved round has no FiredAt yet, so ordering the in-flight table by +// enqueue time put a PR reserved seconds ago ahead of reviews fired much earlier. +func TestInFlightOrdersAReservationByWhenItReserved(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/old", 1, 1, now), queuedRound("kristofferr/new", 2, 2, now)) + firedLongAgo := now.Add(-time.Hour) + st.Rounds["kristofferr/old#1"] = func() Round { + r := st.Rounds["kristofferr/old#1"] + r.Phase, r.FiredAt = PhaseReviewing, &firedLongAgo + return r + }() + enqueuedEvenEarlier := now.Add(-2 * time.Hour) + justReserved := now.Add(-time.Second) + st.Rounds["kristofferr/new#2"] = func() Round { + r := st.Rounds["kristofferr/new#2"] + r.Phase, r.EnqueuedAt, r.ReservedAt = PhaseReserved, enqueuedEvenEarlier, &justReserved + return r + }() + + got := inFlightRounds(st) + if len(got) != 2 || got[0].PR != 1 { + t.Errorf("in flight = %v, want the earlier-fired review first", []int{got[0].PR, got[1].PR}) + } +} + +// While another PR holds the slot, which round fires next depends on whose +// cooldown has elapsed when it releases — and that moment is unknown, so every +// order including Seq is a guess. The table must not number them. +func TestQueueTableDropsPositionsWhileTheSlotIsHeld(t *testing.T) { + now := time.Now().UTC() + st := stateWith(coolingRound("kristofferr/a", 1, 1, now, time.Hour), queuedRound("kristofferr/b", 2, 2, now)) + st.FireSlot = &FireSlot{Key: "kristofferr/c#3", Token: "tok", Since: now.Add(-time.Minute)} + st.Rounds["kristofferr/c#3"] = Round{ + Repo: "kristofferr/c", PR: 3, Head: "ccccccccc", Seq: 3, + Phase: PhaseFired, Token: "tok", EnqueuedAt: now.Add(-2 * time.Minute), FiredAt: &now, + } + + got := RenderDashboard(st, StoreConfig{}) + if strings.Contains(got, "| 1 | [kristofferr/a") || strings.Contains(got, "| 2 | [kristofferr/b") { + t.Errorf("positions must not be claimed while the slot is held:\n%s", got) + } +} + +// A round degraded to its co-reviewers spends no account quota, so DecideFire +// resolves it before the quota gate. Promising it cannot proceed until the window +// closes describes a wait the next observation can end immediately. +func TestQueueDoesNotBlockACoOnlyRoundOnTheAccountWindow(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now)) + st.Rounds["kristofferr/a#1"] = func() Round { + r := st.Rounds["kristofferr/a#1"] + r.CoOnly = true + return r + }() + blocked := now.Add(2 * time.Hour) + st.Account.BlockedUntil = &blocked + + q := st.Queue(now, 0) + if len(q) != 1 { + t.Fatalf("Queue = %d entries, want 1", len(q)) + } + if q[0].Why == WaitAccountBlocked || !q[0].ReadyAt.IsZero() { + t.Errorf("a co-only round must not wait on the account window: ready=%v why=%q", q[0].ReadyAt, q[0].Why) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index 5c49f85..de1c224 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -679,8 +679,14 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { } } gate(roundReadyAt(r), WaitCoolingDown) - gate(blocked, WaitAccountBlocked) - gate(paced, WaitPacing) + // The account window gates the metered review and nothing else. A round + // already degraded to its co-reviewers spends no quota, so DecideFire + // resolves it before that gate — labelling it "account blocked until T" + // promises a wait the next observation can end immediately. + if !r.CoOnly { + gate(blocked, WaitAccountBlocked) + gate(paced, WaitPacing) + } out = append(out, e) } // Order by SIMULATING what firing does, because pacing makes the naive sort From be6924e3d6c1784b6821041e120b567d0e91aa3e Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 16:49:06 +0200 Subject: [PATCH 10/13] Stop predicting a queue order that cannot be predicted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review rounds went into refining this prediction; the fourth explained why there was never one to make. Simulating the queue assumed a slot frees one pacing interval after a fire, but release comes from the bot acknowledging the command or from CRQ_INFLIGHT_TIMEOUT — neither of which pacing knows. With seq 1 ready, seq 2 cooling five minutes and seq 3 ready, an unacknowledged seq 1 leaves both followers eligible together and NextEligible then takes seq 2, the opposite of what any simulation rendered. So the simulation is gone. The queue reports what is known: rows ordered by the gate that binds them and then by Seq, a position for the front alone, and every follower marked as behind an earlier round rather than given a time. A follower cannot start before its own gate, but neither can it start then — that depends on when the round ahead of it finishes — so printing that gate in a "ready" column stated a lower bound as if it were the answer. The header and the front row still carry the block or cooldown that explains the wait. Also fixed: exempting a co-only round from the account block reached its label but not the ordering, because the shared simulation clock still advanced to the block. Such a round spends no quota and DecideFire resolves it before that gate, so immediately actionable work was being listed behind a wait that does not apply to it. The net effect is less code and fewer claims: three of the tests here asserted behaviour of the prediction, and now state the two things that are actually true — which round is next, and that the rest are not ranked. --- internal/state/dashboard.go | 14 ++-- internal/state/dashboard_test.go | 57 +++++++++------ internal/state/state.go | 120 +++++++++---------------------- 3 files changed, 75 insertions(+), 116 deletions(-) diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index b03710b..5b34434 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -268,13 +268,13 @@ func RenderDashboard(st State, cfg StoreConfig) string { case e.Why != "": ready = "unknown" } - // Only claim a position when one is knowable. While another PR holds the - // slot, which round fires next depends on whose cooldown has elapsed by - // the moment it releases — and that moment is unknown, so every possible - // order (including Seq) is a guess. Say so instead of numbering it. - position := strconv.Itoa(i + 1) - if e.Why == WaitSlotBusy { - position = "—" + // Only the front has a knowable position. What fires after it depends on + // when its slot releases — the bot acknowledging, or the in-flight + // timeout — so any number past the first is a guess. List them; do not + // rank them. + position := "—" + if i == 0 && e.Why != WaitSlotBusy { + position = strconv.Itoa(1) } fmt.Fprintf(&b, "| %s | [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %d | %s | `%s` |\n", position, e.Repo, e.PR, e.Repo, e.PR, e.Head, ready, dash(e.Why), diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 3cfa7d2..59a0d08 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -142,12 +142,18 @@ func TestQueueOrdersByReadyThenSeq(t *testing.T) { t.Fatalf("Queue order = %v, want %v", got, want) } } - // An elapsed RetryAt is ready, not cooling down. - if q[1].Why != "" || !q[1].ReadyAt.IsZero() { - t.Errorf("elapsed retry window not treated as ready: %+v", q[1]) + // That d (an ELAPSED RetryAt) sorts among the ready rounds rather than after + // the cooling ones is the point: an expired window is not a wait. + // + // Only the front reports its own gate; everything behind it reports being + // behind, because when it starts depends on when the front finishes. + if q[0].Why != "" || !q[0].ReadyAt.IsZero() { + t.Errorf("the front is ready now: %+v", q[0]) } - if q[2].Why != WaitCoolingDown { - t.Errorf("q[2].Why = %q, want %q", q[2].Why, WaitCoolingDown) + for i := 1; i < len(q); i++ { + if q[i].Why != WaitBehind { + t.Errorf("q[%d].Why = %q, want %q", i, q[i].Why, WaitBehind) + } } } @@ -451,31 +457,36 @@ func TestQueueNamesRoundsBehindTheFrontWithoutInventingATime(t *testing.T) { } } -// The order has to be what firing will actually do. NextEligible takes the lowest -// Seq among rounds eligible at that moment, so a round still cooling down when the -// front fires can overtake a ready round with a higher Seq. Sorting by ready time -// alone rendered 1, 3, 2 where the queue really runs 1, 2, 3. -func TestQueueOrderMatchesWhatFiringWillDo(t *testing.T) { +// Order past the front is not knowable, so the queue stops asserting it. What it +// must still get right is WHICH round is next and that the rest are not ranked: +// slot release comes from the bot acknowledging or from the in-flight timeout, so +// a round cooling now can overtake a ready one with a higher Seq. +func TestQueueRanksOnlyTheFront(t *testing.T) { now := time.Now().UTC() st := stateWith( - queuedRound("kristofferr/a", 1, 1, now), // ready now - coolingRound("kristofferr/b", 2, 2, now, 60*time.Second), // eligible before the next slot - queuedRound("kristofferr/c", 3, 3, now), // ready now, higher Seq + queuedRound("kristofferr/a", 1, 1, now), // ready, lowest Seq + coolingRound("kristofferr/b", 2, 2, now, 5*time.Minute), // cooling + queuedRound("kristofferr/c", 3, 3, now), // ready, higher Seq ) - var got []int - for _, e := range st.Queue(now, 90*time.Second) { - got = append(got, e.PR) + q := st.Queue(now, 90*time.Second) + if q[0].PR != 1 { + t.Errorf("front = %d, want the lowest-Seq ready round", q[0].PR) } - want := []int{1, 2, 3} - if len(got) != len(want) { - t.Fatalf("Queue = %v, want %v", got, want) + if !q[0].ReadyAt.IsZero() || q[0].Why != "" { + t.Errorf("the front is ready now, got ready=%v why=%q", q[0].ReadyAt, q[0].Why) } - for i := range want { - if got[i] != want[i] { - t.Fatalf("Queue = %v, want %v (b is eligible by the time the slot frees)", got, want) + for i := 1; i < len(q); i++ { + if !q[i].ReadyAt.IsZero() || q[i].Why != WaitBehind { + t.Errorf("entry %d must be unranked and untimed, got ready=%v why=%q", i, q[i].ReadyAt, q[i].Why) } } + + // And the table must not number them. + rendered := RenderDashboard(st, StoreConfig{}) + if strings.Contains(rendered, "| 2 | [") || strings.Contains(rendered, "| 3 | [") { + t.Errorf("only the front may carry a position:\n%s", rendered) + } } // A slot held by another PR has no knowable release time, so it outranks every @@ -603,7 +614,7 @@ func TestQueueDoesNotRepeatTheFrontsBoundaryForFollowers(t *testing.T) { t.Errorf("the front is gated by the account block, got %v", q[0].ReadyAt) } if !q[1].ReadyAt.IsZero() || q[1].Why != WaitBehind { - t.Errorf("the follower must not repeat the block boundary: ready=%v why=%q", q[1].ReadyAt, q[1].Why) + t.Errorf("the follower must carry no time at all: ready=%v why=%q", q[1].ReadyAt, q[1].Why) } } diff --git a/internal/state/state.go b/internal/state/state.go index de1c224..aec16af 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -689,99 +689,47 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { } out = append(out, e) } - // Order by SIMULATING what firing does, because pacing makes the naive sort - // disagree with it. NextEligible takes the lowest Seq among rounds that are - // eligible *at that moment*, so a round still cooling down when the front - // fires can become eligible before a ready round with a higher Seq — with - // seq 1 ready, seq 2 cooling for 60s and seq 3 ready on a 90s interval, the - // real order is 1, 2, 3, while sorting by ready time renders 1, 3, 2. - // Start where firing can actually resume, not at render time. An account block - // that outlasts a round's cooldown makes several rounds eligible together the - // moment it clears, and NextEligible then takes the lowest Seq — so ordering - // from now rendered a ready higher-Seq round ahead of one still cooling. - clock := now - if paced.After(clock) { - clock = paced - } - if blocked.After(clock) { - clock = blocked - } - ordered := make([]QueueEntry, 0, len(out)) - remaining := out - for len(remaining) > 0 { - // Advance to when something can actually run. - soonest := time.Time{} - for _, e := range remaining { - if own := roundReadyAt(e.Round); own.After(clock) && (soonest.IsZero() || own.Before(soonest)) { - soonest = own - } - } - pick, picked := -1, int64(0) - for i, e := range remaining { - if roundReadyAt(e.Round).After(clock) { - continue - } - if pick < 0 || e.Seq < picked { - pick, picked = i, e.Seq - } - } - if pick < 0 { - if soonest.IsZero() { - break // nothing can ever run; keep the rest in Seq order below - } - clock = soonest - continue - } - ordered = append(ordered, remaining[pick]) - remaining = append(remaining[:pick:pick], remaining[pick+1:]...) - if minInterval > 0 { - clock = clock.Add(minInterval) - } - } - // Anything unreachable keeps a stable order rather than map order. - sort.Slice(remaining, func(i, j int) bool { return remaining[i].Seq < remaining[j].Seq }) - out = append(ordered, remaining...) - - // Only ABSOLUTE times are shown. A projected "front + one interval" is - // anchored to render time, so an unchanged state would produce a different - // body — and therefore a different DashboardSHA — every minute, which is the - // churn that argued against surfacing pacing in the first place. A round - // queued behind another says so instead of naming a time crq cannot know - // until the round ahead of it actually fires. - // A follower may only show a time that is genuinely later than the round ahead - // of it could finish. Sharing the front's boundary is not that: after the front - // fires, pacing pushes the next one out by another interval, so repeating that - // same timestamp advertises a moment firing will refuse. The floor is built - // from ABSOLUTE gates only, so nothing here is anchored to render time. - if minInterval > 0 { - floor := time.Time{} - for i := range out { - if i > 0 && !(floor.IsZero() || out[i].ReadyAt.After(floor)) { - out[i].ReadyAt, out[i].Why = time.Time{}, WaitBehind - } - if at := out[i].ReadyAt; !at.IsZero() { - floor = at.Add(minInterval) - } else { - floor = time.Time{} // ready now: the follower's start is unknowable - } - } - for i := 1; i < len(out); i++ { - if out[i].ReadyAt.IsZero() { - out[i].Why = WaitBehind + // Order by what is KNOWN — the binding gate, then Seq — and claim a position + // for the front alone. + // + // Four review rounds were spent refining a prediction that cannot be made. + // Simulating the queue assumed the next slot frees one pacing interval after a + // fire, but release comes from the bot acknowledging the command or from + // CRQ_INFLIGHT_TIMEOUT, neither of which pacing knows: with seq 1 ready, seq 2 + // cooling five minutes and seq 3 ready, an unacknowledged seq 1 makes both + // followers eligible together and NextEligible then takes seq 2 — the opposite + // of what any simulation rendered. Ordering past the front is not knowable, so + // this stops asserting it: the rows are listed by what gates them, and only the + // front carries a number (see the renderer, which prints "—" for the rest). + sort.Slice(out, func(i, j int) bool { + if !out[i].ReadyAt.Equal(out[j].ReadyAt) { + // Ready now (zero) sorts ahead of anything with a future gate. + if out[i].ReadyAt.IsZero() != out[j].ReadyAt.IsZero() { + return out[i].ReadyAt.IsZero() } + return out[i].ReadyAt.Before(out[j].ReadyAt) } + return out[i].Seq < out[j].Seq + }) + + // Only the front carries a time. A follower cannot start before its own gate, + // but neither can it start then — that depends on when the round ahead of it + // finishes, which is the unknown above. Showing its gate as a "ready" time + // states a lower bound as though it were the answer, so followers say what is + // actually true: they are behind an earlier round. The header and the front row + // already carry the account block or cooldown that explains the wait. + for i := 1; i < len(out); i++ { + out[i].ReadyAt, out[i].Why = time.Time{}, WaitBehind } - // A slot held by another PR outranks everything. Its release time is - // unknowable, so neither a timestamp nor a simulated POSITION means anything: - // which round is picked depends on which cooldowns have elapsed by then. Fall - // back to Seq — the only ordering that is true whenever several rounds become - // eligible together — and say the slot is why. + // A slot held by another PR gates every entry, including the front: nothing + // fires until it releases, and that moment is unknowable. if slotBusy { for i := range out { - out[i].ReadyAt, out[i].Why = time.Time{}, WaitSlotBusy + if !out[i].Round.CoOnly { + out[i].ReadyAt, out[i].Why = time.Time{}, WaitSlotBusy + } } - sort.Slice(out, func(i, j int) bool { return out[i].Seq < out[j].Seq }) } return out } From d663c7475ed6af9737e1f14dc67010d757ba1a2c Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 16:58:40 +0200 Subject: [PATCH 11/13] Take co-only rounds out of the queue instead of exempting them from it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round running, third place the exemption was missing: the account window, then the ordering clock, now the follower pass — which had already rewritten an eligible co-only round as "behind an earlier round" before the slot check spared it. Exempting gate-by-gate was always going to keep missing one. A co-only round waits for nothing the queue serializes. It spends no account quota, takes no fire slot, and DecideFire resolves it before either gate, so its position behind other rounds is meaningless too. It is now partitioned out before any queue-wide gate is applied, and listed first — actionable work should not sit under rounds waiting for a slot it never needs. Only its own cooldown still binds, because that belongs to the round rather than to the queue. The test asserts the whole exemption at once, with an account block, a pacing boundary and a held slot all active, rather than one gate at a time — which is how the previous two fixes each looked complete. --- internal/state/dashboard_test.go | 44 ++++++++++++ internal/state/state.go | 113 ++++++++++++++++--------------- 2 files changed, 102 insertions(+), 55 deletions(-) diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 59a0d08..4e00080 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -733,3 +733,47 @@ func TestQueueDoesNotBlockACoOnlyRoundOnTheAccountWindow(t *testing.T) { t.Errorf("a co-only round must not wait on the account window: ready=%v why=%q", q[0].ReadyAt, q[0].Why) } } + +// A co-only round waits for nothing the queue serializes: no account quota, no +// fire slot, and DecideFire resolves it before either gate. Exempting it +// gate-by-gate missed a different spot three rounds running — the account window, +// then the ordering, then the follower pass — so this asserts the whole exemption +// at once, with every queue-wide gate active simultaneously. +func TestCoOnlyRoundIsExemptFromEveryQueueGate(t *testing.T) { + now := time.Now().UTC() + st := stateWith( + queuedRound("kristofferr/metered", 1, 1, now), // lower Seq, ordinary round + queuedRound("kristofferr/free", 2, 2, now), // higher Seq, co-only + ) + st.Rounds["kristofferr/free#2"] = func() Round { + r := st.Rounds["kristofferr/free#2"] + r.CoOnly = true + return r + }() + // Every gate the queue can impose, at once. + blocked := now.Add(2 * time.Hour) + st.Account.BlockedUntil = &blocked + fired := now + st.LastFired = &fired + st.FireSlot = &FireSlot{Key: "kristofferr/other#9", Token: "tok", Since: now.Add(-time.Minute)} + st.Rounds["kristofferr/other#9"] = Round{ + Repo: "kristofferr/other", PR: 9, Head: "999999999", Seq: 9, + Phase: PhaseFired, Token: "tok", EnqueuedAt: now.Add(-2 * time.Minute), FiredAt: &fired, + } + + q := st.Queue(now, 90*time.Second) + if len(q) != 2 { + t.Fatalf("Queue = %d entries, want 2", len(q)) + } + free := q[0] + if free.PR != 2 { + t.Fatalf("the co-only round must lead: got #%d — actionable work cannot sit behind a slot it never needs", free.PR) + } + if !free.ReadyAt.IsZero() || free.Why != "" { + t.Errorf("the co-only round is ready now, got ready=%v why=%q", free.ReadyAt, free.Why) + } + // The ordinary round is still governed by all of it. + if q[1].Why != WaitSlotBusy { + t.Errorf("the metered round must report the held slot, got %q", q[1].Why) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index aec16af..4d9fa96 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -662,76 +662,79 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { } } - var out []QueueEntry + // Split by whether the round is waiting for anything the queue serializes. + // + // A co-only round is not. It spends no account quota, takes no fire slot, and + // DecideFire resolves it before either gate — so the account window, the slot, + // and its position behind other rounds are all irrelevant to it. Exempting it + // gate-by-gate was tried and missed a different spot three times running; the + // partition makes the exemption structural, so a gate added later cannot + // silently apply to work it does not govern. + var queued, freeRunning []QueueEntry for _, r := range s.Rounds { if r.Phase != PhaseQueued && r.Phase != PhaseAwaitingRetry { continue } - // Every gate is a lower bound on when firing will accept this round, so - // the ready time is the LATEST of them and the reason is whichever one - // binds. Picking the first that matched let a later boundary hide behind - // an earlier one — a round cooling down for a minute inside a two-hour - // account block was advertised at the wrong time entirely. e := QueueEntry{Round: r} - gate := func(at time.Time, why string) { - if at.After(now) && at.After(e.ReadyAt) { - e.ReadyAt, e.Why = at, why - } + // Its own cooldown binds either way: that is the round's, not the queue's. + if own := roundReadyAt(r); own.After(now) { + e.ReadyAt, e.Why = own, WaitCoolingDown } - gate(roundReadyAt(r), WaitCoolingDown) - // The account window gates the metered review and nothing else. A round - // already degraded to its co-reviewers spends no quota, so DecideFire - // resolves it before that gate — labelling it "account blocked until T" - // promises a wait the next observation can end immediately. - if !r.CoOnly { - gate(blocked, WaitAccountBlocked) - gate(paced, WaitPacing) + if r.CoOnly { + freeRunning = append(freeRunning, e) + continue } - out = append(out, e) - } - // Order by what is KNOWN — the binding gate, then Seq — and claim a position - // for the front alone. - // - // Four review rounds were spent refining a prediction that cannot be made. - // Simulating the queue assumed the next slot frees one pacing interval after a - // fire, but release comes from the bot acknowledging the command or from - // CRQ_INFLIGHT_TIMEOUT, neither of which pacing knows: with seq 1 ready, seq 2 - // cooling five minutes and seq 3 ready, an unacknowledged seq 1 makes both - // followers eligible together and NextEligible then takes seq 2 — the opposite - // of what any simulation rendered. Ordering past the front is not knowable, so - // this stops asserting it: the rows are listed by what gates them, and only the - // front carries a number (see the renderer, which prints "—" for the rest). - sort.Slice(out, func(i, j int) bool { - if !out[i].ReadyAt.Equal(out[j].ReadyAt) { - // Ready now (zero) sorts ahead of anything with a future gate. - if out[i].ReadyAt.IsZero() != out[j].ReadyAt.IsZero() { - return out[i].ReadyAt.IsZero() + // Every remaining gate is a lower bound on when firing will accept this + // round, so the ready time is the LATEST of them and the reason is + // whichever binds. Taking the first that matched let a one-minute cooldown + // hide a two-hour account block. + for _, g := range []struct { + at time.Time + why string + }{{blocked, WaitAccountBlocked}, {paced, WaitPacing}} { + if g.at.After(now) && g.at.After(e.ReadyAt) { + e.ReadyAt, e.Why = g.at, g.why } - return out[i].ReadyAt.Before(out[j].ReadyAt) } - return out[i].Seq < out[j].Seq - }) + queued = append(queued, e) + } - // Only the front carries a time. A follower cannot start before its own gate, - // but neither can it start then — that depends on when the round ahead of it - // finishes, which is the unknown above. Showing its gate as a "ready" time - // states a lower bound as though it were the answer, so followers say what is - // actually true: they are behind an earlier round. The header and the front row - // already carry the account block or cooldown that explains the wait. - for i := 1; i < len(out); i++ { - out[i].ReadyAt, out[i].Why = time.Time{}, WaitBehind + byGateThenSeq := func(list []QueueEntry) { + sort.Slice(list, func(i, j int) bool { + if !list[i].ReadyAt.Equal(list[j].ReadyAt) { + // Ready now (zero) sorts ahead of anything with a future gate. + if list[i].ReadyAt.IsZero() != list[j].ReadyAt.IsZero() { + return list[i].ReadyAt.IsZero() + } + return list[i].ReadyAt.Before(list[j].ReadyAt) + } + return list[i].Seq < list[j].Seq + }) + } + byGateThenSeq(queued) + byGateThenSeq(freeRunning) + + // Order past the front is not knowable and this stops asserting it. Simulating + // the queue assumed a slot frees one pacing interval after a fire, but release + // comes from the bot acknowledging or from CRQ_INFLIGHT_TIMEOUT — neither of + // which pacing knows. So only the front carries a time: a follower cannot start + // before its own gate, but neither can it start then, and printing that gate in + // a "ready" column states a lower bound as though it were the answer. + for i := 1; i < len(queued); i++ { + queued[i].ReadyAt, queued[i].Why = time.Time{}, WaitBehind } - // A slot held by another PR gates every entry, including the front: nothing - // fires until it releases, and that moment is unknowable. + // A held slot gates every queued round, the front included: nothing fires + // until it releases, and that moment is unknowable. if slotBusy { - for i := range out { - if !out[i].Round.CoOnly { - out[i].ReadyAt, out[i].Why = time.Time{}, WaitSlotBusy - } + for i := range queued { + queued[i].ReadyAt, queued[i].Why = time.Time{}, WaitSlotBusy } } - return out + + // Free-running work first: it is actionable now, and burying it under rounds + // waiting for a slot it never needs is what the exemption exists to prevent. + return append(freeRunning, queued...) } // Normalize repairs invariants after load: map init, expired retry windows From d0430338447e1d826195a875bdd308c3b78a8548 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 17:13:02 +0200 Subject: [PATCH 12/13] Claim a next round only when one can actually fire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all from the previous two commits, all the same root error: the table said "#1 is what fires next" in situations where that is not determined. **With nothing eligible, the earliest window is not the next fire.** If no pump runs between two retry times — a polling gap, or the daemon down — both rounds are eligible at the next pass and NextEligible takes the lower Seq, not the earlier window. A position is now claimed only when some round can fire right now, in which case it IS the lowest-Seq ready one. Otherwise the soonest opening is still shown, without saying whose it is. **An empty ready time meant two different things.** "Nothing is holding this round" and "something is holding it whose end is unknowable" both rendered as an empty time and sorted alike, so a slot-blocked round could take the front. They rank apart now. **A cooling co-only round was jumping the queue.** Partitioning it out of the gates was right; concatenating it ahead of everything was not, because its own cooldown still applies. Both groups merge into one list ordered by readiness. Also: a reserved round on a RETRY still carried the previous attempt's FiredAt, since retries preserve that history deliberately — so the in-flight table ordered it by a fire hours old and printed that time for a command this attempt had not posted. One correction to the last commit: dropping a follower's time is right, but it was dropping the REASON too. "Slot busy" and "account blocked" are true and worth saying; only the start time is unknowable. A round with nothing of its own holding it is the one that is purely behind. --- internal/state/dashboard.go | 12 ++++- internal/state/dashboard_test.go | 29 ++++++++--- internal/state/state.go | 89 +++++++++++++++++++++----------- 3 files changed, 93 insertions(+), 37 deletions(-) diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 5b34434..0bf9768 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -96,6 +96,13 @@ func inFlightRounds(st State) []Round { // seconds ago ahead of reviews fired much earlier, contradicting the table's own // ordering. func firedAtOf(r Round) time.Time { + // A reserved round is BETWEEN attempts: a retry deliberately preserves the + // previous FiredAt as history, so reading it here ordered the round by a fire + // that may be hours old — and printed that stale time in a column describing a + // command this attempt has not posted yet. + if r.Phase == PhaseReserved && r.ReservedAt != nil { + return *r.ReservedAt + } if r.FiredAt != nil { return *r.FiredAt } @@ -272,8 +279,11 @@ func RenderDashboard(st State, cfg StoreConfig) string { // when its slot releases — the bot acknowledging, or the in-flight // timeout — so any number past the first is a guess. List them; do not // rank them. + // A position is claimed only for a round that can fire now (see Queue): + // anything else depends on when a pump runs and which windows have + // opened by then. position := "—" - if i == 0 && e.Why != WaitSlotBusy { + if i == 0 && e.ReadyAt.IsZero() && e.Why == "" { position = strconv.Itoa(1) } fmt.Fprintf(&b, "| %s | [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %d | %s | `%s` |\n", diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 4e00080..5a5a42b 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -151,10 +151,14 @@ func TestQueueOrdersByReadyThenSeq(t *testing.T) { t.Errorf("the front is ready now: %+v", q[0]) } for i := 1; i < len(q); i++ { - if q[i].Why != WaitBehind { - t.Errorf("q[%d].Why = %q, want %q", i, q[i].Why, WaitBehind) + if !q[i].ReadyAt.IsZero() { + t.Errorf("q[%d] must carry no time, got %v", i, q[i].ReadyAt) } } + // The ready follower has nothing of its own holding it, so it is purely behind. + if q[1].Why != WaitBehind { + t.Errorf("q[1].Why = %q, want %q", q[1].Why, WaitBehind) + } } // The account-wide quota block gates firing too, so a round whose own RetryAt @@ -476,11 +480,19 @@ func TestQueueRanksOnlyTheFront(t *testing.T) { if !q[0].ReadyAt.IsZero() || q[0].Why != "" { t.Errorf("the front is ready now, got ready=%v why=%q", q[0].ReadyAt, q[0].Why) } + // Behind the front: no time, whatever the reason. A round with a gate of its + // own still reports it; one with nothing holding it is purely behind. for i := 1; i < len(q); i++ { - if !q[i].ReadyAt.IsZero() || q[i].Why != WaitBehind { - t.Errorf("entry %d must be unranked and untimed, got ready=%v why=%q", i, q[i].ReadyAt, q[i].Why) + if !q[i].ReadyAt.IsZero() { + t.Errorf("entry %d must be untimed, got %v", i, q[i].ReadyAt) } } + if q[1].Why != WaitBehind { + t.Errorf("the ready follower has nothing of its own holding it, got %q", q[1].Why) + } + if q[2].Why != WaitCoolingDown { + t.Errorf("the cooling round should still say so, got %q", q[2].Why) + } // And the table must not number them. rendered := RenderDashboard(st, StoreConfig{}) @@ -613,8 +625,13 @@ func TestQueueDoesNotRepeatTheFrontsBoundaryForFollowers(t *testing.T) { if !q[0].ReadyAt.Equal(blocked.UTC()) { t.Errorf("the front is gated by the account block, got %v", q[0].ReadyAt) } - if !q[1].ReadyAt.IsZero() || q[1].Why != WaitBehind { - t.Errorf("the follower must carry no time at all: ready=%v why=%q", q[1].ReadyAt, q[1].Why) + // The follower keeps its reason — the block is real — but loses the time, + // because when it actually starts depends on the round ahead of it. + if !q[1].ReadyAt.IsZero() { + t.Errorf("the follower must carry no time: ready=%v", q[1].ReadyAt) + } + if q[1].Why != WaitAccountBlocked { + t.Errorf("the follower should still report why it waits, got %q", q[1].Why) } } diff --git a/internal/state/state.go b/internal/state/state.go index 4d9fa96..3ab8374 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -699,42 +699,71 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { queued = append(queued, e) } - byGateThenSeq := func(list []QueueEntry) { - sort.Slice(list, func(i, j int) bool { - if !list[i].ReadyAt.Equal(list[j].ReadyAt) { - // Ready now (zero) sorts ahead of anything with a future gate. - if list[i].ReadyAt.IsZero() != list[j].ReadyAt.IsZero() { - return list[i].ReadyAt.IsZero() - } - return list[i].ReadyAt.Before(list[j].ReadyAt) - } - return list[i].Seq < list[j].Seq - }) - } - byGateThenSeq(queued) - byGateThenSeq(freeRunning) - - // Order past the front is not knowable and this stops asserting it. Simulating - // the queue assumed a slot frees one pacing interval after a fire, but release - // comes from the bot acknowledging or from CRQ_INFLIGHT_TIMEOUT — neither of - // which pacing knows. So only the front carries a time: a follower cannot start - // before its own gate, but neither can it start then, and printing that gate in - // a "ready" column states a lower bound as though it were the answer. - for i := 1; i < len(queued); i++ { - queued[i].ReadyAt, queued[i].Why = time.Time{}, WaitBehind - } - - // A held slot gates every queued round, the front included: nothing fires - // until it releases, and that moment is unknowable. + // A held slot gates every queued round: nothing metered fires until it + // releases, and that moment is unknowable. Free-running rounds are untouched — + // they never wanted the slot. if slotBusy { for i := range queued { queued[i].ReadyAt, queued[i].Why = time.Time{}, WaitSlotBusy } } - // Free-running work first: it is actionable now, and burying it under rounds - // waiting for a slot it never needs is what the exemption exists to prevent. - return append(freeRunning, queued...) + // One list, ordered by readiness then Seq — which is NextEligible's own rule + // among rounds that are eligible together. Concatenating the groups instead + // put a cooling co-only round ahead of work that could fire immediately. + out := append(freeRunning, queued...) + // An empty ReadyAt means two different things and they must not sort alike: + // nothing is holding this round, or something is holding it whose end is + // unknowable (a slot another PR holds). Rank them apart, or a slot-blocked + // round sorts as if it were ready and can take the front. + rank := func(e QueueEntry) int { + switch { + case e.ReadyAt.IsZero() && e.Why == "": + return 0 // fire-eligible now + case !e.ReadyAt.IsZero(): + return 1 // waiting until a known time + default: + return 2 // waiting on something with no knowable end + } + } + sort.Slice(out, func(i, j int) bool { + if ri, rj := rank(out[i]), rank(out[j]); ri != rj { + return ri < rj + } + if !out[i].ReadyAt.Equal(out[j].ReadyAt) { + return out[i].ReadyAt.Before(out[j].ReadyAt) + } + return out[i].Seq < out[j].Seq + }) + + // Say which round is next ONLY when one is eligible now — then it is the + // lowest-Seq ready round, exactly what NextEligible picks. With everything + // still cooling, which one fires depends on when a pump happens to run: if + // none runs between two retry times, both are eligible at the next pass and + // the lower Seq wins, not the earlier window. So no front is claimed, and the + // soonest opening is reported without saying whose it is. + // + // Only that front carries a time. Anything behind it starts when the front + // finishes, which is the unknowable part, so naming its own gate there would + // state a lower bound as if it were the answer. + front := len(out) > 0 && rank(out[0]) == 0 + for i := range out { + if front && i == 0 { + continue // the front is the one round whose readiness is knowable + } + if !front && i == 0 { + continue // nothing is eligible; the soonest opening is still worth naming + } + // Drop the TIME but keep the reason. A round's own gate is real and worth + // reporting — "slot busy", "account blocked" — it just is not a start time, + // because the round ahead has to finish first. Only a round with nothing of + // its own holding it is purely behind. + out[i].ReadyAt = time.Time{} + if out[i].Why == "" { + out[i].Why = WaitBehind + } + } + return out } // Normalize repairs invariants after load: map init, expired retry windows From 2aaf898c5b0bc0019ef7f535623a104f95e71fc4 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 17:25:27 +0200 Subject: [PATCH 13/13] A held slot stops everything, free-running rounds included MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, and the second corrects an assumption in the exemption itself. **Exempting co-only rounds from the slot was wrong**, though exempting them from the account window was right. They do not need the slot — but Pump returns as soon as it sees a slot holder, so the quota-free path that would advance them is never reached while one is held. The dashboard was promoting such a round to "#1, ready now" for work the daemon cannot take until the holder is acknowledged. The account window genuinely does not apply to them; the slot, in practice, does. (An agent's own `crq next` can still resolve such a round directly through the quota-free bypass, which is why this describes what the queue will do rather than forbidding anything.) **The in-flight table showed a stale fire time for a reservation.** A retry keeps the previous attempt's FiredAt as history and Reserve does not clear it, so a reserved round printed an earlier attempt's timestamp as though the current command had gone out — and if that post hangs, the misleading value is what stays on the dashboard next to a stranded reservation. Ordering was fixed last round; the rendered cell was not. Unrelated observation while verifying: TestLoopSettleWindowCatchesTrailingWave is flaky. It sleeps 50ms and depends on real 1ms polls, so it loses under parallel load — it failed once and then passed five consecutive full-package runs plus a run against the unmodified tree. Left alone here; noted on the roadmap. --- internal/state/dashboard.go | 17 ++++++++++- internal/state/dashboard_test.go | 51 ++++++++++++++++++++++++++++++++ internal/state/state.go | 15 ++++++++-- 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 0bf9768..6509b1d 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -95,6 +95,21 @@ func inFlightRounds(st State) []Round { // falling straight through to EnqueuedAt sorted a long-queued PR that was reserved // seconds ago ahead of reviews fired much earlier, contradicting the table's own // ordering. +// firedTimeOf is what the in-flight table should print in its "fired" column: the +// time THIS attempt posted its command, or nothing when it has not. +// +// A retry deliberately keeps the previous attempt's FiredAt as history and Reserve +// does not clear it, so a reserved round would otherwise display an earlier +// attempt's timestamp as though the current command had gone out — and if the post +// hangs or the process dies, that misleading value is what stays on the dashboard +// beside the stranded reservation. +func firedTimeOf(r Round) *time.Time { + if r.Phase == PhaseReserved { + return nil + } + return r.FiredAt +} + func firedAtOf(r Round) time.Time { // A reserved round is BETWEEN attempts: a retry deliberately preserves the // previous FiredAt as history, so reading it here ordered the round by a fire @@ -251,7 +266,7 @@ func RenderDashboard(st State, cfg StoreConfig) string { for _, r := range inFlight { fmt.Fprintf(&b, "| [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %s | %s | `%s` |\n", r.Repo, r.PR, r.Repo, r.PR, r.Head, r.Phase, - fmtStamp(r.FiredAt, loc), fmtStamp(r.WaitDeadline, loc), dash(coBotMarks(r)), r.ByHost) + fmtStamp(firedTimeOf(r), loc), fmtStamp(r.WaitDeadline, loc), dash(coBotMarks(r)), r.ByHost) } } diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 5a5a42b..4f309d7 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -794,3 +794,54 @@ func TestCoOnlyRoundIsExemptFromEveryQueueGate(t *testing.T) { t.Errorf("the metered round must report the held slot, got %q", q[1].Why) } } + +// A retry keeps the previous attempt's FiredAt as history, and Reserve does not +// clear it — so a reserved round would print an earlier attempt's timestamp as +// though the current command had gone out. If the post then hangs, that +// misleading value is what stays on the dashboard beside a stranded reservation. +func TestInFlightPrintsNoFireTimeForAReservation(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/a", 1, 1, now)) + previous := now.Add(-3 * time.Hour) + reserved := now.Add(-time.Second) + st.Rounds["kristofferr/a#1"] = func() Round { + r := st.Rounds["kristofferr/a#1"] + r.Phase, r.FiredAt, r.ReservedAt, r.Attempts = PhaseReserved, &previous, &reserved, 1 + return r + }() + + got := RenderDashboard(st, StoreConfig{}) + if strings.Contains(got, fmtStamp(&previous, time.UTC)) { + t.Errorf("the previous attempt's fire time must not be shown for a reservation:\n%s", got) + } +} + +// A held slot stops everything, free-running rounds included — not because they +// need the slot, but because Pump returns as soon as it sees a holder, so the +// quota-free path that would advance them is never reached. +func TestHeldSlotStopsFreeRunningRoundsToo(t *testing.T) { + now := time.Now().UTC() + st := stateWith(queuedRound("kristofferr/free", 1, 1, now)) + st.Rounds["kristofferr/free#1"] = func() Round { + r := st.Rounds["kristofferr/free#1"] + r.CoOnly = true + return r + }() + st.FireSlot = &FireSlot{Key: "kristofferr/other#9", Token: "tok", Since: now.Add(-time.Minute)} + fired := now.Add(-time.Minute) + st.Rounds["kristofferr/other#9"] = Round{ + Repo: "kristofferr/other", PR: 9, Head: "999999999", Seq: 9, + Phase: PhaseFired, Token: "tok", EnqueuedAt: now.Add(-2 * time.Minute), FiredAt: &fired, + } + + q := st.Queue(now, 0) + if len(q) != 1 { + t.Fatalf("Queue = %d entries, want 1", len(q)) + } + if q[0].Why != WaitSlotBusy { + t.Errorf("a co-only round must report the held slot, got %q — the daemon cannot advance it either", q[0].Why) + } + if strings.Contains(RenderDashboard(st, StoreConfig{}), "| 1 | [kristofferr/free") { + t.Error("nothing may be numbered while the slot is held") + } +} diff --git a/internal/state/state.go b/internal/state/state.go index 3ab8374..cc6400f 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -699,13 +699,22 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { queued = append(queued, e) } - // A held slot gates every queued round: nothing metered fires until it - // releases, and that moment is unknowable. Free-running rounds are untouched — - // they never wanted the slot. + // A held slot stops EVERYTHING, free-running rounds included. + // + // Not because they need the slot — they do not — but because Pump returns as + // soon as it sees a slot holder, so the quota-free path that would advance + // them is never reached while one is held. The exemption above is about the + // account window, which genuinely does not apply to them; claiming they are + // ready here would promise action the daemon cannot take until the holder is + // acknowledged. (An agent's own `crq next` can still resolve such a round + // directly, which is why this describes the queue rather than forbidding it.) if slotBusy { for i := range queued { queued[i].ReadyAt, queued[i].Why = time.Time{}, WaitSlotBusy } + for i := range freeRunning { + freeRunning[i].ReadyAt, freeRunning[i].Why = time.Time{}, WaitSlotBusy + } } // One list, ordered by readiness then Seq — which is NextEligible's own rule