fix: make teardown idempotent and exception-safe#91
Merged
Conversation
BaseBootstrapper.teardown() now returns immediately when not bootstrapped. This fixes Litestar and FastStream, both of which register self.teardown on a framework shutdown hook while also being callable manually — a user who explicitly calls teardown() would otherwise re-invoke every instrument's teardown when the framework shutdown fired second. Most instruments are not idempotent themselves. Also wrap the shutdown calls inside OpenTelemetryInstrument and LoggingInstrument in try/finally so the cached _tracer_provider / _logger_factory references are reset even when shutdown raises. Without this, a failed shutdown leaves the instrument in a state where a second bootstrap reuses a stale reference. The bootstrapper-level guard prevents the immediate symptom but doesn't help when instruments are used standalone. Regression tests: - test_teardown_is_idempotent: bootstrap, teardown twice, assert each instrument's teardown was called exactly once. - test_opentelemetry_instrument_teardown_resets_tracer_provider_when_shutdown_raises: patch shutdown to raise, assert field resets to None. - test_logging_instrument_teardown_resets_factory_when_close_handlers_raises: patch close_handlers to raise, assert field resets to None. Closes CRIT-3 and TEST-3 from the audit. Resolves the try/finally follow-up flagged in PR #90's code review.
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
Three teardown-robustness fixes bundled into one PR:
BaseBootstrapper.teardown()idempotent. Two-line guard at the top — returns immediately whennot self.is_bootstrapped. Fixes the Litestar/FastStream case where the bootstrapper registersself.teardownon a framework shutdown hook and gets called manually, otherwise double-tearing-down instruments (many of which aren't idempotent themselves).OpenTelemetryInstrument.teardown()exception-safe. Wrapsself._tracer_provider.shutdown()intry/finallyso the cached_tracer_providerresets even when shutdown raises.LoggingInstrument.teardown()exception-safe. Sametry/finallyshape aroundself._logger_factory.close_handlers().Three regression tests added (one per fix); all fail on
main, pass on this branch.Closes CRIT-3 and TEST-3 from an internal audit. Resolves the follow-up flagged in PR #90's code review.
Test plan
just test -- tests/test_free_bootstrap.py -v— all teardown tests pass.just test -- tests/instruments/test_opentelemetry_instrument.py -v— pass.just test -- tests/instruments/test_logging_instrument.py -v— pass.just test— 83/83.just lint— clean.Notes for reviewer
🤖 Generated with Claude Code