fix: set informer handler context before registering handlers. Fixes … - #968
Merged
Megh03 merged 1 commit intoSep 21, 2026
Merged
Conversation
quinn-diesel
approved these changes
Sep 21, 2026
Megh03
deleted the
sup-8148-controller-can-panic-during-informer-replay-before
branch
September 21, 2026 16:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #951
Change
Assign
resourceEventHandlerCtxbefore callingAddEventHandler, injobWatcher,podWatcherandcompletionsWatcher.Adds a regression test per watcher that registers against an already started and synced informer. These only fail under
-raceas, a nil context is harmless to the fake clientset, sotests.shgains a-racestep scoped with-run. The scope is deliberate: the watcher constructors write package-level gauge funcs that parallel tests race on, so the package isn't race-clean yet. Follow-up to come; the filter can be dropped once it is.Context
On an already-started informer, client-go starts the listener goroutines inside
AddEventHandlerand then enqueues a synthetic.Addfor every cached object, soOnAddcan run before the assignment and read a nil context. That's the normal startup path: the deduper and limiter have already started and synced the shared Jobs informer by the timejobWatcherregisters. A finished Job with a job acquisition token Secret then reachessecrets.Getwith a nil context and panics inside client-go, crash-looping the controller.Assigning before
AddEventHandlercloses this rather than narrowing it . The write is sequenced before thegostatement that starts the listener, so the memory model guarantees callbacks observe it. All three watchers share the bug;podWatcherandcompletionsWatcherare on the Pods informer, andpodWatcherregisters second whenever the completion watcher is enabled.Verified against a real cluster with 150 finished annotated Jobs: the controller crashed on startup 5 times out of 5 before the fix (exit 2, stack as in #951), 0 out of 5 after, with the token Secret cleaned up within ~3s.
Affiliation (optional, external contributors)
N/A
Disclosures / Credits
Diagnosis and suggested fix from the reporter in #951 were both correct. Used some assistance from Claude to diagnose the fix. The fix has been verified against a real cluster before submission.