Skip to content

fix(app): wire signal-aware context through controller startup#1787

Open
SarthakB11 wants to merge 1 commit intokagent-dev:mainfrom
SarthakB11:sarth/signal-handlers
Open

fix(app): wire signal-aware context through controller startup#1787
SarthakB11 wants to merge 1 commit intokagent-dev:mainfrom
SarthakB11:sarth/signal-handlers

Conversation

@SarthakB11
Copy link
Copy Markdown

@SarthakB11 SarthakB11 commented May 1, 2026

summary

  • app.Start constructed two independent signal-handling setups: an early ctx := context.Background() used for tracing init and db connect, and a separate ctrl.SetupSignalHandler() passed to mgr.Start later. controller-runtime requires the signal handler be set up exactly once per process, so the second call is a latent panic risk if a future caller adds another signal-handler setup. addresses the long-standing // TODO setup signal handlers at go/core/pkg/app/app.go:297.
  • call ctrl.SetupSignalHandler() once at the top of Start and reuse the returned context for both startup work and mgr.Start. tracing init, db connect, and the manager now share a single signal-aware ctx; the duplicate setup is gone.
  • note: the default migration runner (app.go:462) discards its ctx argument and migrations.RunUp does not accept ctx, so migrations themselves are still not cancellable by this PR. threading ctx through migrations.RunUp is a separate change; happy to follow up.

ai model disclosure

used claude code (claude opus 4.7) to triage and draft the diff. self-reviewed go/core/pkg/app/app.go to confirm the second ctrl.SetupSignalHandler() call site at line 670 and the manager's downstream use of ctx. verified locally via:

go vet ./core/pkg/app/...
./bin/golangci-lint run ./core/pkg/app/...      # 0 issues
go test -race -skip 'TestE2E.*' -count=1 ./core/pkg/app/...   # ok

the controller's Start function used context.Background() for tracing init,
migrations, and db connect, while ctrl.SetupSignalHandler() was only invoked
later for mgr.Start. on sigterm during startup, those early operations
ignored the cancellation and could hang past the pod's terminationGracePeriod.

call ctrl.SetupSignalHandler() once at the top of Start, reuse the returned
context for both startup work and mgr.Start. controller-runtime's signal
handler must be called exactly once per process; collapsing the two call
sites also removes a latent panic risk if a future caller added another
signal-handler setup.

resolves the long-standing 'TODO setup signal handlers' in app.go.

Signed-off-by: SarthakB11 <sarthak.bhardwaj21b@iiitg.ac.in>
Copilot AI review requested due to automatic review settings May 1, 2026 07:15
@github-actions github-actions Bot added the bug Something isn't working label May 1, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR ensures the controller’s startup path uses the same signal-aware context as mgr.Start, so SIGTERM during early initialization (tracing init, migrations, DB connect, etc.) can propagate cancellation and avoid hanging past Kubernetes termination grace periods. It also eliminates a second ctrl.SetupSignalHandler() call to align with controller-runtime’s “call once per process” requirement.

Changes:

  • Initialize a single signal-aware ctx via ctrl.SetupSignalHandler() at the top of app.Start.
  • Reuse that ctx throughout startup and pass it into mgr.Start instead of creating a second signal handler.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread go/core/pkg/app/app.go
Comment on lines +297 to +298
// Reused below for mgr.Start; SetupSignalHandler must be called once per process.
ctx := ctrl.SetupSignalHandler()
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. you're right, the default runner at app.go:462 takes _ context.Context and migrations.RunUp doesn't accept ctx at all, so migrations aren't cancellable by this PR.

I have updated the description, this PR only makes tracing init, db connect, and the manager signal-aware, plus removes the duplicate ctrl.SetupSignalHandler() call. threading ctx through migrations.RunUp is a separate change. happy to open a follow-up if maintainers want it.

@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants