|
| 1 | +--- |
| 2 | +summary: Make a second bootstrapper on an already-bootstrapped application fail loudly — `bootstrap()` raises `ConfigurationError` instead of dying inside Litestar with a duplicate-route error or silently double-registering FastAPI's routes. |
| 3 | +--- |
| 4 | + |
| 5 | +# Design: Fail fast when a second bootstrapper targets the same application |
| 6 | + |
| 7 | +## Summary |
| 8 | + |
| 9 | +`_attach_teardown_once` guards teardown attachment, and only that. A second |
| 10 | +bootstrapper constructed on the same application still applies every instrument |
| 11 | +again when `bootstrap()` is called, because nothing stops it. Litestar dies with |
| 12 | +a confusing error from deep inside the framework; FastAPI silently registers a |
| 13 | +second copy of the health and metrics routes. Make `bootstrap()` raise |
| 14 | +`ConfigurationError` on the bootstrapper whose attach was skipped. |
| 15 | + |
| 16 | +## Motivation |
| 17 | + |
| 18 | +The construction-time warning already tells the user what the supported model |
| 19 | +is — "construct one `<Bootstrapper>` per application" — but nothing enforces it, |
| 20 | +and the two frameworks fail in opposite, equally unhelpful ways. Both |
| 21 | +reproduced on the current tree: |
| 22 | + |
| 23 | +```python |
| 24 | +first = LitestarBootstrapper(bootstrap_config=config_a) |
| 25 | +second = LitestarBootstrapper(bootstrap_config=dataclasses.replace(config_a)) # warns |
| 26 | +first.bootstrap() |
| 27 | +second.bootstrap() |
| 28 | +``` |
| 29 | + |
| 30 | +``` |
| 31 | +ImproperlyConfiguredException: 500: Handler already registered for path '/health' and http method OPTIONS |
| 32 | +``` |
| 33 | + |
| 34 | +Nothing in that message points at the real mistake. The FastAPI equivalent is |
| 35 | +worse, because it does not fail at all: the second `bootstrap()` returns the |
| 36 | +same app object with its route count grown from 6 to 8 — a shadowed duplicate |
| 37 | +of the health-check and metrics routes, a second `PrometheusInstrument` |
| 38 | +registering against the same collector registry, and a second set of instrument |
| 39 | +state whose teardown is not wired to anything. |
| 40 | + |
| 41 | +The warning is emitted at construction, but the damage happens at `bootstrap()`, |
| 42 | +which is exactly where nothing checks. |
| 43 | + |
| 44 | +## Design |
| 45 | + |
| 46 | +`_attach_teardown_once` already detects the case; it just does not record it. |
| 47 | +Have it remember, and have `bootstrap()` refuse: |
| 48 | + |
| 49 | +```python |
| 50 | +# BaseBootstrapper |
| 51 | +_attach_skipped: bool = False |
| 52 | +``` |
| 53 | + |
| 54 | +```python |
| 55 | +# BaseBootstrapper._attach_teardown_once |
| 56 | +def _attach_teardown_once(self, target: object, attach: typing.Callable[[], object]) -> None: |
| 57 | + if getattr(target, self._TEARDOWN_MARKER, False): |
| 58 | + warnings.warn(...) # unchanged |
| 59 | + self._attach_skipped = True |
| 60 | + return |
| 61 | + ... |
| 62 | +``` |
| 63 | + |
| 64 | +```python |
| 65 | +# BaseBootstrapper.bootstrap |
| 66 | +def bootstrap(self) -> ApplicationT: |
| 67 | + if self._attach_skipped: |
| 68 | + msg = ( |
| 69 | + f"{type(self).__name__} shares its application with another lite-bootstrap " |
| 70 | + f"bootstrapper, which has already applied its instruments. Construct one " |
| 71 | + f"{type(self).__name__} per application." |
| 72 | + ) |
| 73 | + raise ConfigurationError(msg) |
| 74 | + ... |
| 75 | +``` |
| 76 | + |
| 77 | +`ConfigurationError` already exists in `lite_bootstrap/exceptions.py` and is what |
| 78 | +`FastAPIConfig.__post_init__` raises for a comparable misuse. |
| 79 | + |
| 80 | +The construction-time warning stays, so the documented warn-and-skip invariant |
| 81 | +for the teardown seam is unchanged — this adds a second, louder gate at the |
| 82 | +point where instruments would actually be applied. `FreeBootstrapper` never |
| 83 | +calls `_attach_teardown_once`, so it is unaffected. |
| 84 | + |
| 85 | +The warning's wording needs a small update: it currently ends by saying this |
| 86 | +bootstrapper's teardown "will not run on shutdown", which understates the new |
| 87 | +behavior — the bootstrapper cannot be used at all. |
| 88 | + |
| 89 | +## Non-goals |
| 90 | + |
| 91 | +- Making instrument application idempotent so a second bootstrapper becomes |
| 92 | + harmless. That means per-instrument state on user-supplied objects and a much |
| 93 | + wider change; the supported model is one bootstrapper per application. |
| 94 | +- Letting the second `bootstrap()` warn and return the app unchanged. It would |
| 95 | + hand back an app whose instruments were never applied and whose teardown is |
| 96 | + not wired — a quieter trap than the one being fixed. |
| 97 | +- Detecting two bootstrappers built on two *different* `AppConfig` objects that |
| 98 | + happen to share route paths. That is ordinary route-collision territory and |
| 99 | + belongs to the framework. |
| 100 | + |
| 101 | +## Testing |
| 102 | + |
| 103 | +`just test -k "second_bootstrapper"`, extending the existing |
| 104 | +`test_second_<framework>_bootstrapper_on_same_*_warns_not_stacks` tests, which |
| 105 | +today stop at construction and never call `bootstrap()`: |
| 106 | + |
| 107 | +- Litestar: the second `bootstrap()` raises `ConfigurationError` naming the |
| 108 | + bootstrapper class, and the first application still works (its health route |
| 109 | + responds). |
| 110 | +- FastAPI: same, and the app's route count is unchanged by the failed second |
| 111 | + bootstrap — the assertion that pins the silent-duplication half of the defect. |
| 112 | +- FastStream and FastMCP: same raise, since they share the seam. |
| 113 | +- `FreeBootstrapper`: two bootstrappers still work, since it has no attach |
| 114 | + target. This pins that the guard did not overreach. |
| 115 | + |
| 116 | +Then `just lint-ci` and the full `just test`. |
| 117 | + |
| 118 | +## Risk |
| 119 | + |
| 120 | +**A caller today relies on double-bootstrapping.** Unlikely and already broken: |
| 121 | +on Litestar it raises, on FastAPI it produces shadowed duplicate routes. The |
| 122 | +release note should still call it out, since the FastAPI case currently |
| 123 | +"works". |
| 124 | + |
| 125 | +**The flag survives on a bootstrapper that could otherwise be reused.** Once |
| 126 | +`_attach_skipped` is set, that bootstrapper is permanently unusable. That is |
| 127 | +intended — the application it was given is owned by someone else, and nothing |
| 128 | +about that changes later. |
| 129 | + |
| 130 | +**Promotion:** `architecture/bootstrappers.md` records that the attach marker |
| 131 | +now gates instrument application as well as teardown attachment, and what the |
| 132 | +second bootstrapper gets. |
0 commit comments