From 5311586341ef36156b8eb2d3a727425289c6b52e Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sun, 31 May 2026 23:53:12 +0300 Subject: [PATCH] fix: make teardown idempotent and exception-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lite_bootstrap/bootstrappers/base.py | 2 ++ .../instruments/logging_instrument.py | 6 ++++-- .../instruments/opentelemetry_instrument.py | 6 ++++-- tests/instruments/test_logging_instrument.py | 19 +++++++++++++++++++ .../test_opentelemetry_instrument.py | 19 +++++++++++++++++++ tests/test_free_bootstrap.py | 16 ++++++++++++++++ 6 files changed, 64 insertions(+), 4 deletions(-) diff --git a/lite_bootstrap/bootstrappers/base.py b/lite_bootstrap/bootstrappers/base.py index 9dd997b..938c251 100644 --- a/lite_bootstrap/bootstrappers/base.py +++ b/lite_bootstrap/bootstrappers/base.py @@ -80,6 +80,8 @@ def bootstrap(self) -> ApplicationT: return self._prepare_application() def teardown(self) -> None: + if not self.is_bootstrapped: + return self.is_bootstrapped = False errors: list[tuple[str, BaseException]] = [] for one_instrument in reversed(self.instruments): diff --git a/lite_bootstrap/instruments/logging_instrument.py b/lite_bootstrap/instruments/logging_instrument.py index d183b34..d12c216 100644 --- a/lite_bootstrap/instruments/logging_instrument.py +++ b/lite_bootstrap/instruments/logging_instrument.py @@ -207,5 +207,7 @@ def teardown(self) -> None: h.close() root_logger.setLevel(logging.WARNING) if self._logger_factory is not None: - self._logger_factory.close_handlers() - object.__setattr__(self, "_logger_factory", None) + try: + self._logger_factory.close_handlers() + finally: + object.__setattr__(self, "_logger_factory", None) diff --git a/lite_bootstrap/instruments/opentelemetry_instrument.py b/lite_bootstrap/instruments/opentelemetry_instrument.py index bbd9fa1..bcad0ee 100644 --- a/lite_bootstrap/instruments/opentelemetry_instrument.py +++ b/lite_bootstrap/instruments/opentelemetry_instrument.py @@ -138,5 +138,7 @@ def teardown(self) -> None: else: one_instrumentor.uninstrument() if self._tracer_provider is not None: - self._tracer_provider.shutdown() - object.__setattr__(self, "_tracer_provider", None) + try: + self._tracer_provider.shutdown() + finally: + object.__setattr__(self, "_tracer_provider", None) diff --git a/tests/instruments/test_logging_instrument.py b/tests/instruments/test_logging_instrument.py index 6023e6f..2d8d26e 100644 --- a/tests/instruments/test_logging_instrument.py +++ b/tests/instruments/test_logging_instrument.py @@ -1,6 +1,8 @@ import logging from io import StringIO +from unittest.mock import patch +import pytest import structlog from opentelemetry.trace import get_tracer @@ -117,3 +119,20 @@ def test_memory_logger_factory_error() -> None: error_message = "error message" test_logger.error(error_message) assert error_message in test_stream.getvalue() + + +def test_logging_instrument_teardown_resets_factory_when_close_handlers_raises() -> None: + instrument = LoggingInstrument( + bootstrap_config=LoggingConfig(logging_buffer_capacity=0), + ) + instrument.bootstrap() + factory = instrument._logger_factory # noqa: SLF001 + assert factory is not None + + with ( + patch.object(factory, "close_handlers", side_effect=RuntimeError("boom")), + pytest.raises(RuntimeError, match="boom"), + ): + instrument.teardown() + + assert instrument._logger_factory is None # noqa: SLF001 diff --git a/tests/instruments/test_opentelemetry_instrument.py b/tests/instruments/test_opentelemetry_instrument.py index 3f3e68b..6c041d8 100644 --- a/tests/instruments/test_opentelemetry_instrument.py +++ b/tests/instruments/test_opentelemetry_instrument.py @@ -1,5 +1,7 @@ from unittest.mock import patch +import pytest + from lite_bootstrap.instruments.opentelemetry_instrument import ( InstrumentorWithParams, OpentelemetryConfig, @@ -49,3 +51,20 @@ def test_opentelemetry_instrument_teardown_shuts_down_tracer_provider() -> None: mock_shutdown.assert_called_once_with() assert instrument._tracer_provider is None # noqa: SLF001 + + +def test_opentelemetry_instrument_teardown_resets_tracer_provider_when_shutdown_raises() -> None: + instrument = OpenTelemetryInstrument( + bootstrap_config=OpentelemetryConfig(opentelemetry_log_traces=True), + ) + instrument.bootstrap() + tracer_provider = instrument._tracer_provider # noqa: SLF001 + assert tracer_provider is not None + + with ( + patch.object(tracer_provider, "shutdown", side_effect=RuntimeError("boom")), + pytest.raises(RuntimeError, match="boom"), + ): + instrument.teardown() + + assert instrument._tracer_provider is None # noqa: SLF001 diff --git a/tests/test_free_bootstrap.py b/tests/test_free_bootstrap.py index 1fb1df4..184511e 100644 --- a/tests/test_free_bootstrap.py +++ b/tests/test_free_bootstrap.py @@ -108,3 +108,19 @@ def test_free_bootstrapper_with_missing_instrument_dependency( ) -> None: with emulate_package_missing(package_name), pytest.warns(UserWarning, match=package_name): FreeBootstrapper(bootstrap_config=free_bootstrapper_config) + + +def test_teardown_is_idempotent(free_bootstrapper_config: FreeBootstrapperConfig) -> None: + bootstrapper = FreeBootstrapper(bootstrap_config=free_bootstrapper_config) + bootstrapper.bootstrap() + + first = MagicMock() + second = MagicMock() + bootstrapper.instruments = [first, second] + + bootstrapper.teardown() + bootstrapper.teardown() + + first.teardown.assert_called_once() + second.teardown.assert_called_once() + assert not bootstrapper.is_bootstrapped