Skip to content

fix metric server shutdown using already-canceled context#712

Merged
gertd merged 1 commit intoaserto-dev:mainfrom
ashwinrs:fix/metric-server-shutdown-context
Mar 31, 2026
Merged

fix metric server shutdown using already-canceled context#712
gertd merged 1 commit intoaserto-dev:mainfrom
ashwinrs:fix/metric-server-shutdown-context

Conversation

@ashwinrs
Copy link
Copy Markdown
Contributor

Summary

  • StopServers is called via defer after topazApp.Context is already canceled, so deriving the shutdown timeout context from it causes http.Server.Shutdown to fail immediately with "context canceled".
  • Use context.Background() as the parent for the shutdown timeout context so the metric server (and gateway servers) get the full graceful shutdown window.
  • Remove the now-unused context.Context parameter from StopServers.

Test plan

  • Verify topazd run shuts down cleanly without the "failed to shutdown metric server" error.
  • Confirm metric server drains connections within the 30s timeout on graceful shutdown.

Made with Cursor

StopServers is called via defer after topazApp.Context is canceled,
so deriving the shutdown timeout from that context causes immediate
cancellation. Use context.Background() instead, and remove the now
unused context parameter.

Made-with: Cursor
@gertd gertd self-requested a review March 31, 2026 06:23
@gertd
Copy link
Copy Markdown
Member

gertd commented Mar 31, 2026

Thanks @ashwinrs for the PR. This was outstanding, and hence the reason why main was not tagged yet, sorry!

We discussed this morning and believe the current proposed fix is too narrow; we would want the complete close path to use a new context instance instead.

We will refactor on top of your PR to achieve this.

Thanks again for the support & help!

@gertd gertd merged commit 29c8f25 into aserto-dev:main Mar 31, 2026
4 of 5 checks passed
@gertd
Copy link
Copy Markdown
Member

gertd commented Mar 31, 2026

@ashwinrs

After more debate, that is what you already managed to do, so we took the change as-is, our confusion, thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants