I've re-reviewed the PR in full, including the controller framework source and the new tests. My earlier concern has been addressed.
On the re-entrant deadlock question: I read through pkg/controller/controller.go and can confirm the claim in the new comment at lines 208–213 is accurate. The watch callback does c.workQueue <- ev (a buffered channel send) and returns immediately. The handler is invoked only from runWorker → processItem, running in a distinct goroutine. There is no synchronous re-entrant path from inside reconcileAppVersion back to any handler — any store write that triggers a new watch event just enqueues to the work queue and gets picked up by a worker later. The deadlock scenario I flagged is structurally impossible.
The two-part fix holds up:
versionChanged gate in ensurePoolForService correctly limits the "revive drained pool" path to actual deploys, not steady-state resyncs. The new TestAutoModeDrainedPoolOnlyRevivesForNewVersion test exercises both halves — same-version resync leaves DesiredInstances=0, new version raises it to 1 — and that's exactly what matters.
AddonAssociationHandler now calls l.Reconcile, pulling the handler under the per-app mutex. TestAddonAssociationHandlerUsesPerAppLock validates mutual exclusion by externally holding the mutex, confirming the handler blocks, and then confirming it completes once the lock is released. It also confirms that a different app reconciles freely on the same call.
Worker count 1 → 2 is safe: the per-app mutex in Reconcile and CreatePoolForVersion serializes same-app work, and the controller framework's inFlight map additionally serializes same-entity dispatch within a controller.
The comment documenting the framework's async-only dispatch is a clear, permanent answer to the concern, and the code backs it up. Ready to graduate to human review.
Posted to the PR's comment threads when you submit.
pkg/controller/controller.go directly. The claim in the comment you added is correct: the watch callback puts the event into c.workQueue (a buffered channel) and returns — the handler is only ever called from runWorker in a separate goroutine. There is no synchronous path from inside reconcileAppVersion back to the addon handler on the same goroutine. Re-entrant deadlock is structurally impossible here. The comment documents this clearly and the code backs it up. I'm satisfied — resolving this.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1150,
"verdict": "ready",
"event": "comment",
"summary": "I've re-reviewed the PR in full, including the controller framework source and the new tests. My earlier concern has been addressed.\n\n**On the re-entrant deadlock question:** I read through `pkg/controller/controller.go` and can confirm the claim in the new comment at lines 208–213 is accurate. The watch callback does `c.workQueue \u003c- ev` (a buffered channel send) and returns immediately. The handler is invoked only from `runWorker` → `processItem`, running in a distinct goroutine. There is no synchronous re-entrant path from inside `reconcileAppVersion` back to any handler — any store write that triggers a new watch event just enqueues to the work queue and gets picked up by a worker later. The deadlock scenario I flagged is structurally impossible.\n\n**The two-part fix holds up:**\n\n- `versionChanged` gate in `ensurePoolForService` correctly limits the \"revive drained pool\" path to actual deploys, not steady-state resyncs. The new `TestAutoModeDrainedPoolOnlyRevivesForNewVersion` test exercises both halves — same-version resync leaves `DesiredInstances=0`, new version raises it to 1 — and that's exactly what matters.\n\n- `AddonAssociationHandler` now calls `l.Reconcile`, pulling the handler under the per-app mutex. `TestAddonAssociationHandlerUsesPerAppLock` validates mutual exclusion by externally holding the mutex, confirming the handler blocks, and then confirming it completes once the lock is released. It also confirms that a different app reconciles freely on the same call.\n\n- Worker count 1 → 2 is safe: the per-app mutex in `Reconcile` and `CreatePoolForVersion` serializes same-app work, and the controller framework's `inFlight` map additionally serializes same-entity dispatch within a controller.\n\nThe comment documenting the framework's async-only dispatch is a clear, permanent answer to the concern, and the code backs it up. Ready to graduate to human review.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6fA0ec",
"body": "I've now read through `pkg/controller/controller.go` directly. The claim in the comment you added is correct: the watch callback puts the event into `c.workQueue` (a buffered channel) and returns — the handler is only ever called from `runWorker` in a separate goroutine. There is no synchronous path from inside `reconcileAppVersion` back to the addon handler on the same goroutine. Re-entrant deadlock is structurally impossible here. The comment documents this clearly and the code backs it up. I'm satisfied — resolving this.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true,
"draft": true
}This draft fixes a real and well-understood production problem: the minutely resync was cold-starting every idle (scale-to-zero) auto-mode app, which blocked the launcher's single worker while it waited for readiness, and in turn saturated the reconcile queue. The two-part fix is sound:
versionChanged gate in ensurePoolForService — only floor the pool's DesiredInstances back to 1 when the pool's sandboxSpec version actually differs from the incoming version ID. A same-version resync now leaves a drained pool drained. The logic is easy to follow and the intent is captured well in the comment.
AddonAssociationHandler delegates to Reconcile — instead of calling reconcileAppVersion directly (bypassing the per-app mutex), it now calls l.Reconcile(ctx, &core_v1alpha.App{ID: assoc.App}, nil), which re-reads the app from the store, checks addon readiness, and holds the per-app lock for the full duration. This closes the race the old code had between an addon event and a concurrent main reconcile for the same app.
Worker count 1 → 2 — the comment accurately explains the invariant: same-app work is still serialized by the per-app mutex, so two workers give parallelism across different apps without opening any per-app race.
The test suite is strong: TestAutoModeDrainedPoolOnlyRevivesForNewVersion exercises the idle-pool behaviour directly (same-version resync leaves DesiredInstances=0; new-version resync raises it to 1), and TestAddonAssociationHandlerUsesPerAppLock concurrently blocks on a held mutex and verifies that a different app isn't affected — exactly the two properties the change is staking correctness on.
One concrete concern worth calling out before merge:
AddonAssociationHandler now calls l.Reconcile, which tries to acquire appMu for assoc.App. The addon handler runs in the controller framework's goroutine — the same goroutines that can also be dispatching main Reconcile calls for the same app. sync.Mutex is not reentrant in Go, so if the addon handler and the main reconciler both try to acquire the mutex for the same app at the same time, they simply block on each other (no deadlock — one wins, one waits). That part is fine. The concern is the other direction: if the main Reconcile for app A is already holding the lock and — through some path within reconcileAppVersion — triggers a synchronous operation that also dispatches back through the controller framework for app A and ends up calling the addon handler for that same app, you'd have a deadlock. I don't see that path in the code today, but with 2 workers and the addon handler now going through the full Reconcile + EAC round-trips, it's worth confirming that the controller framework's dispatch is always async (never a synchronous re-entrant call into the same goroutine while a lock is held).
The TestAddonAssociationHandlerUsesPerAppLock test does validate the mutual exclusion property by holding the mutex externally and asserting the handler goroutine blocks. It also verifies that releasing the lock lets the handler complete. That's the right test. What it doesn't (and can't easily) test is the re-entrant-deadlock scenario above — worth a quick audit of the controller framework's dispatch path to rule it out before graduating to full review.
Minor nit: the assoc.Encode call in the test at line 4263 is passed but assocEntity is constructed only to satisfy the event — the assoc.App used inside the handler is decoded from the entity, so it's implicitly testing the encode/decode round-trip too. Not a problem, just worth being aware of.
This is solid work and nearly ready. Confirm the no-reentrant-deadlock property in the controller dispatch layer and it's good to go.
Reconcile acquires appMu for assoc.App. Confirm the controller framework's dispatch is always async.This line now routes through Reconcile, which takes the per-app mutex. If the controller framework can ever dispatch an addon-association event synchronously while the same goroutine is already inside Reconcile for the same app (e.g. triggered by a store write done inside reconcileAppVersion), this would deadlock on a non-reentrant sync.Mutex. The test covers the blocking/release flow externally but can't rule out a re-entrant call within the framework itself. Confirm the controller dispatch path is always async/queued before merging.
Verdict: caveats