This is a draft, so I'm judging readiness for human review. It's there — the change is coherent, well-reasoned, and well-tested, with a few things worth flagging before merge.
The PR replaces the controller framework's channel-based "deliver one event per entity" model with a dirtyQueue that coalesces multiple enqueues of the same key into a single work token, and workers always read current entity state from the store at dequeue time rather than processing the snapshot baked into the event. That's a sound architectural move: it eliminates a whole class of stale-read bugs, removes the dropped-event hazard from the old 1000-deep channel, and adds explicit error retry with exponential backoff where there was previously none. The companion change to indexwatch.Watcher — owning reconnect logic with a revision cursor so compactions trigger a fresh snapshot rather than silent loss — is the right place for that concern.
Stop() shutdown ordering. In controller.go, Stop() calls cancel() then watcher.Stop() then queue.Close() then wg.Wait(). The watch-consumer goroutine loops for event := range c.watcher.Updates() — that loop will exit only when the updates channel closes. watcher.Stop() closes updates after its internal goroutine exits, which in turn waits on its own WaitGroup. So the consumer goroutine will unblock once watcher.Stop() returns, and wg.Wait() sees it. The workers are blocked in queue.Get(ctx) — cancel() cancels ctx, causing Get to return false, which exits the workers. queue.Close() is called before wg.Wait() as a belt-and-suspenders: it stops any time.AfterFunc retry timers (good) and closes the wake channel (safe because the workers have already returned false on the cancelled context). This ordering looks sound to me, though the workers racing between ctx.Done() and the queue.wake channel is worth a moment's thought — the Get method checks closed inside the lock on each wake, so it will return false after Close() even if cancel() loses the race. I'm satisfied this is correct.
Enqueue after Stop. The old code guarded against post-stop enqueues by checking c.top.Done(). The new code calls queue.Add, which returns enqueueDropped after queue.Close() — the caller sees a Warn log but no panic. That's acceptable, though the comment in Enqueue's godoc says "Delete snapshots are retained only as tombstones" — callers who Enqueue after Stop should know their enqueue is silently discarded. The behaviour is correct; just document it if this is a public API.
Test assertion mismatch in disk watch tests. In TestDiskVolumeWatchEnqueuesParentByID and TestDiskMountWatchEnqueuesParentByID, the test calls Enqueue (via watch.Update) with no Entity in the event, then asserts event.Entity != nil on the event received by the handler. This works because currentEvent reads the entity from the store before calling the handler. The test comment reads "the target controller should load current parent state", which is exactly right — this is a positive correctness assertion for the new behaviour. Good.
done path when entry.dirty is true after a failure (workqueue.go ~lines 234–241). When a worker fails and entry.dirty is already set, Done promotes dirty to queued and returns false (not retrying). The retry counter and backoff are therefore bypassed. This is intentional — a new signal while a failure was in-flight means there's fresh work that should run immediately rather than wait for backoff. The code is correct but unusual enough to deserve a comment, which is absent here.
retrying entry and the timer pointer after a new urgent signal. When a workUrgent signal arrives for an entry that is retrying, the timer is Stop()ped (line 140) and attempts are reset (line 143). time.Timer.Stop() returns false if the timer already fired, meaning retry() may be running concurrently. In retry(), the pointer comparison entry != expected guards against a stale call, and the entry.retrying check gates re-queuing. Even if retry() wins the lock first, it will re-queue; then the Add call takes the lock, sees entry.retrying = false (set by retry()), entry.queued = true, and falls through to the entry.queued && signal.priority > entry.priority promotion path (line 166–172). Net effect: the entry ends up queued urgently exactly once. I traced this carefully and it is safe.
workqueue.go lines 338–340: the lane slices (q.urgent, q.repair) are never compacted. Promoted entries leave a stale token in the repair lane; popLocked skips them via the pointer/priority check, which is correct and documented. In pathological cases (heavy promotion traffic) the slice grows without bound. This is acceptable for now but worth a note.reportMetrics) starts the ticker, writes once before the first tick, then waits. This means at startup you get an immediate metric write followed by a write 10 seconds later — two writes in the first 10 seconds, then one per 10 seconds thereafter. Probably fine but could produce a confusing spike in dashboards.TestReconcileController_EventProcessing and TestReconcileController_WatchReconnect — those tests relied on mock wiring that no longer fits the architecture. The new TestReconcileControllerCoalescesAndReadsCurrentState covers the important invariant, and indexwatch now has its own tested reconnect logic.Overall the design is solid, the test coverage is meaningfully expanded, and the correctness issues I found are edge cases that are already handled correctly. Ready for human review.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1164,
"verdict": "ready",
"event": "comment",
"summary": "This is a draft, so I'm judging readiness for human review. It's there — the change is coherent, well-reasoned, and well-tested, with a few things worth flagging before merge.\n\n## What this does\n\nThe PR replaces the controller framework's channel-based \"deliver one event per entity\" model with a `dirtyQueue` that coalesces multiple enqueues of the same key into a single work token, and workers always read **current** entity state from the store at dequeue time rather than processing the snapshot baked into the event. That's a sound architectural move: it eliminates a whole class of stale-read bugs, removes the dropped-event hazard from the old 1000-deep channel, and adds explicit error retry with exponential backoff where there was previously none. The companion change to `indexwatch.Watcher` — owning reconnect logic with a revision cursor so compactions trigger a fresh snapshot rather than silent loss — is the right place for that concern.\n\n## Correctness observations\n\n**`Stop()` shutdown ordering.** In `controller.go`, `Stop()` calls `cancel()` then `watcher.Stop()` then `queue.Close()` then `wg.Wait()`. The watch-consumer goroutine loops `for event := range c.watcher.Updates()` — that loop will exit only when the `updates` channel closes. `watcher.Stop()` closes `updates` after its internal goroutine exits, which in turn waits on its own WaitGroup. So the consumer goroutine will unblock once `watcher.Stop()` returns, and `wg.Wait()` sees it. The workers are blocked in `queue.Get(ctx)` — `cancel()` cancels `ctx`, causing `Get` to return `false`, which exits the workers. `queue.Close()` is called before `wg.Wait()` as a belt-and-suspenders: it stops any `time.AfterFunc` retry timers (good) and closes the `wake` channel (safe because the workers have already returned `false` on the cancelled context). This ordering looks sound to me, though the workers racing between `ctx.Done()` and the `queue.wake` channel is worth a moment's thought — the `Get` method checks `closed` inside the lock on each wake, so it will return `false` after `Close()` even if `cancel()` loses the race. I'm satisfied this is correct.\n\n**`Enqueue` after `Stop`.** The old code guarded against post-stop enqueues by checking `c.top.Done()`. The new code calls `queue.Add`, which returns `enqueueDropped` after `queue.Close()` — the caller sees a `Warn` log but no panic. That's acceptable, though the comment in `Enqueue`'s godoc says \"Delete snapshots are retained only as tombstones\" — callers who `Enqueue` after `Stop` should know their enqueue is silently discarded. The behaviour is correct; just document it if this is a public API.\n\n**Test assertion mismatch in disk watch tests.** In `TestDiskVolumeWatchEnqueuesParentByID` and `TestDiskMountWatchEnqueuesParentByID`, the test calls `Enqueue` (via `watch.Update`) with no `Entity` in the event, then asserts `event.Entity != nil` on the event received by the handler. This works because `currentEvent` reads the entity from the store before calling the handler. The test comment reads *\"the target controller should load current parent state\"*, which is exactly right — this is a positive correctness assertion for the new behaviour. Good.\n\n**`done` path when `entry.dirty` is true after a failure** (workqueue.go ~lines 234–241). When a worker fails and `entry.dirty` is already set, `Done` promotes dirty to queued and returns `false` (not retrying). The retry counter and backoff are therefore bypassed. This is intentional — a new signal while a failure was in-flight means there's fresh work that should run immediately rather than wait for backoff. The code is correct but unusual enough to deserve a comment, which is absent here.\n\n**`retrying` entry and the timer pointer after a new urgent signal.** When a `workUrgent` signal arrives for an entry that is `retrying`, the timer is `Stop()`ped (line 140) and `attempts` are reset (line 143). `time.Timer.Stop()` returns false if the timer already fired, meaning `retry()` may be running concurrently. In `retry()`, the pointer comparison `entry != expected` guards against a stale call, and the `entry.retrying` check gates re-queuing. Even if `retry()` wins the lock first, it will re-queue; then the `Add` call takes the lock, sees `entry.retrying = false` (set by `retry()`), `entry.queued = true`, and falls through to the `entry.queued \u0026\u0026 signal.priority \u003e entry.priority` promotion path (line 166–172). Net effect: the entry ends up queued urgently exactly once. I traced this carefully and it is safe.\n\n## Minor things\n\n- `workqueue.go` lines 338–340: the lane slices (`q.urgent`, `q.repair`) are never compacted. Promoted entries leave a stale token in the repair lane; `popLocked` skips them via the pointer/priority check, which is correct and documented. In pathological cases (heavy promotion traffic) the slice grows without bound. This is acceptable for now but worth a note.\n- The metrics reporter goroutine (`reportMetrics`) starts the ticker, writes once *before* the first tick, then waits. This means at startup you get an immediate metric write followed by a write 10 seconds later — two writes in the first 10 seconds, then one per 10 seconds thereafter. Probably fine but could produce a confusing spike in dashboards.\n- Deleted `TestReconcileController_EventProcessing` and `TestReconcileController_WatchReconnect` — those tests relied on mock wiring that no longer fits the architecture. The new `TestReconcileControllerCoalescesAndReadsCurrentState` covers the important invariant, and `indexwatch` now has its own tested reconnect logic.\n\nOverall the design is solid, the test coverage is meaningfully expanded, and the correctness issues I found are edge cases that are already handled correctly. Ready for human review.",
"posted_to_pr": true,
"draft": true
}