fix(close): exempt internal teardown hooks from CloseCallbackBudget - #153
Merged
benitogf merged 1 commit intoJul 28, 2026
Merged
Conversation
CloseCallbackBudget skips not-yet-started close hooks once exhausted, but internal lifecycle hooks (stopResync, LimitFilter.StopCleanup) were registered through the same public path as user callbacks — so a tight budget could skip them, leaking the resync worker past Storage.Close. Close hooks now carry an exempt flag. RegisterCloseHook (public API, unchanged) registers budgeted user hooks; a new internal registerInternalCloseHook marks lifecycle hooks exempt, and runPhase never skips an exempt hook regardless of the budget. stopResync and LimitFilter.StopCleanup route through it. User callbacks stay budgeted exactly as before. Closes #151 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CBosch101
approved these changes
Jul 28, 2026
CBosch101
left a comment
There was a problem hiding this comment.
Fixes #151 correctly: internal lifecycle teardown is now unskippable under a tight CloseCallbackBudget, while user callbacks stay budget-skippable exactly as before. Verified, no blockers.
Verified
Closeloop guard is!hook.exempt && budgetExceeded()(ooo.go:1341-1345) — exempt hooks always run; only non-exempt skips incrementskipped.- Both internal teardown callers routed through the exempt path:
stopResync(ooo.go:913) andLimitFilter.StopCleanup(filters.go:167). No user callers changed — publicRegisterCloseHooksignature and its range guard are unchanged, so no exported-API break. TestCloseCallbackBudgetStopsResyncWorkerdrives the real #151 path — user budget-buster registered beforeStartsostopResynclands after it and is reached only post-exhaustion; assertsresyncClosedand a boundedresyncWgre-join.TestCloseCallbackBudgetExemptsInternalHookscovers exempt-after-exhaustion.- README and the
Closedoc comment both updated consistently.go build ./...andgo vet ./...clean at HEAD.
🤖 Generated with Claude Code
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.
Closes #151.
What & why
Server.Closeruns registered close hooks per phase, and once a positiveCloseCallbackBudgetis exhausted it skips the not-yet-started hooks in that phase. Internal, lifecycle-critical hooks were registered through the same publicRegisterCloseHookpath as user callbacks, so a tight budget could skip them:stopResync(the resync-worker drain,PreShutdown) — if skipped, the worker goroutine outlivesStorage.Close: a goroutine leak and a possible read-after-close on embedded storages.LimitFilter.StopCleanup(PreShutdown) — same exposure.The default budget of 0 skips nothing, so this was latent — but an operator who sets a budget to bound user callbacks would silently break internal shutdown.
The change
Close hooks now carry an
exemptflag:RegisterCloseHook(and the deprecatedRegisterPreClose/RegisterProxyCleanupwrappers) register budgeted user hooks exactly as before.registerInternalCloseHookmarks a hook exempt;runPhasenever skips an exempt hook regardless of the budget, while non-exempt hooks are still skipped once it's exhausted (and the skip-count log counts only those).stopResyncandLimitFilter.StopCleanuproute through the internal path. Proxy-teardown hooks stay budgeted (they're proxy/user-owned).Tests
TestCloseCallbackBudgetStopsResyncWorkerdrives the real defect path: the budget-buster is registered beforeStart, sostopResync(registered duringStart) lands after it in the PreShutdown slice and is only reached after the budget is exhausted. It asserts the worker actually stopped (resyncClosedset only bystopResync, plus a boundedresyncWgjoin). Verified fault-detecting — it fails ifstopResyncis routed back through the budgeted path.TestCloseCallbackBudgetExemptsInternalHooksproves the mechanism directly (exempt sentinels run, a user hook is skipped, an exempt hook registered after the budget is blown still runs).CloseCallbackBudgettests pass unmodified; full suite green undergo test ./... -race -count=1.Docs (
Server.Close"Bound:" section and README) now state the internal-hook exemption.🤖 Generated with Claude Code