From 700a08998a6bd47f8bb981d80f079cf014749ac1 Mon Sep 17 00:00:00 2001 From: x Date: Wed, 14 Oct 2026 16:26:55 -0700 Subject: [PATCH] refactor(AIC-3501): clear Agent Skills state through a core shutdown hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Core lifecycle.py imported the skills module and called skills._clear_state() from shutdown(), which is core hooking into experimental code (TESTING.md §0.3). skills_core now registers clear_state with a new generic shutdown_hooks module when it loads, and shutdown() runs the registered hooks, as the TypeScript SDK does. Importing the package root and calling shutdown() no longer loads any skills or evaluations module; a subprocess test pins that. Co-Authored-By: Claude Opus 5.5 --- packages/client/agents.md | 9 +++- .../src/launchdarkly_ai_server/lifecycle.py | 22 +++------- .../launchdarkly_ai_server/shutdown_hooks.py | 44 +++++++++++++++++++ .../src/launchdarkly_ai_server/skills_core.py | 6 +++ packages/client/tests/test_lifecycle.py | 35 ++++++++++++--- 5 files changed, 93 insertions(+), 23 deletions(-) create mode 100644 packages/client/src/launchdarkly_ai_server/shutdown_hooks.py diff --git a/packages/client/agents.md b/packages/client/agents.md index 203cc83f..6c5319ed 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -686,8 +686,13 @@ three signals are emitted by the `record_*` functions there; nothing else calls the allowlist is enforced in one place. **Injection goes through `skills.py`.** `_set_store`, `_set_emitter_for_testing` and -`_clear_state` delegate to `skills_core`. `set_skill_store`, `shutdown` and tests use -those; none should reach into `skills_core` directly. +`_clear_state` delegate to `skills_core`. `set_skill_store` and tests use those; none +should reach into `skills_core` directly. + +**Core never imports this feature.** `skills_core` registers `clear_state` with +`shutdown_hooks.register_shutdown_hook` when it loads, and `shutdown()` runs the registered +hooks. `lifecycle.py` imports no skills module (TESTING.md §0.3, pinned by +`test_core_does_not_import_experimental_code`). ### Descriptor-pinned filesystem access diff --git a/packages/client/src/launchdarkly_ai_server/lifecycle.py b/packages/client/src/launchdarkly_ai_server/lifecycle.py index 5d4bbadc..ed767400 100644 --- a/packages/client/src/launchdarkly_ai_server/lifecycle.py +++ b/packages/client/src/launchdarkly_ai_server/lifecycle.py @@ -6,9 +6,9 @@ import os from typing import Any -from . import skills from .sdk_info import flush_ai_sdk_info, reset_ai_sdk_info from .sdk_usage import flush_sdk_usage, report_usage, reset_sdk_usage +from .shutdown_hooks import run_shutdown_hooks from .types import InitClientOptions, SDKOptions from .utils import model_stamps_from_meta @@ -354,21 +354,14 @@ def _release_otel_globals() -> None: logger.debug("Could not release the global OTel tracer provider", exc_info=True) -def _clear_experimental_state() -> None: - """Clears experimental feature state. A failure is logged, never raised.""" - try: - skills._clear_state() - except Exception: - logger.warning("Could not clear the Agent Skills state", exc_info=True) - - async def shutdown() -> None: """ Shuts down the singleton client. Idempotent — safe to call multiple times even if the client was never initialized or already shut down. - Also clears any experimental feature state, such as a configured skill - store, and restores process-wide SDK defaults from ``configure``. + Also runs the registered shutdown hooks, which clear the state of any + experimental feature in use, and restores process-wide SDK defaults from + ``configure``. When telemetry was running, this also releases the process-global tracer provider so a later ``init_client`` can install its own — see @@ -380,7 +373,7 @@ async def shutdown() -> None: local_provider = _tracer_provider owned_globals = _owns_otel_globals - _clear_experimental_state() + run_shutdown_hooks() # Null the singleton before any awaits so a second call is a no-op. # Options reset even when no client was initialized: configure() does not @@ -434,9 +427,8 @@ def _reset_for_testing() -> None: _owns_otel_globals = False _sdk_options = SDKOptions() _ai_invoked = False - # Directly, not through _clear_experimental_state: a failed reset should - # fail the test rather than leak state into the next one. - skills._clear_state() + # A failed reset should fail the test rather than leak state into the next one. + run_shutdown_hooks(raise_errors=True) # Mirrors shutdown(): without this a suite that inits more than once leaves # every later span on the first test's provider. if owned_globals: diff --git a/packages/client/src/launchdarkly_ai_server/shutdown_hooks.py b/packages/client/src/launchdarkly_ai_server/shutdown_hooks.py new file mode 100644 index 00000000..06f42a7a --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/shutdown_hooks.py @@ -0,0 +1,44 @@ +""" +Cleanup hooks that ``shutdown()`` runs. + +This is how core reaches experimental state without importing it: an +experimental module registers its own cleanup when it loads, and ``shutdown()`` +runs whatever is registered. Core never imports, names or calls experimental +code (TESTING.md §0.3), so a hook's *name* comes from the module that +registers it. +""" + +from __future__ import annotations + +import logging +from collections.abc import Callable + +# The lifecycle logger: this warning came from shutdown() before the hooks existed. +logger = logging.getLogger("launchdarkly_ai_server.lifecycle") + +_hooks: dict[str, Callable[[], None]] = {} + + +def register_shutdown_hook(name: str, hook: Callable[[], None]) -> None: + """ + Registers *hook* to run on every ``shutdown()``, including before a client + exists. Keyed by *name*, so registering the same feature again replaces the + earlier hook rather than adding another. + """ + _hooks[name] = hook + + +def run_shutdown_hooks(*, raise_errors: bool = False) -> None: + """ + Runs every registered hook. Experimental code must not break a core call, so + a hook that raises is logged and the rest still run. *raise_errors* is for + the test reset, where a failed cleanup should fail the test instead. + """ + for name, hook in list(_hooks.items()): + if raise_errors: + hook() + continue + try: + hook() + except Exception: + logger.warning("Could not clear the %s state", name, exc_info=True) diff --git a/packages/client/src/launchdarkly_ai_server/skills_core.py b/packages/client/src/launchdarkly_ai_server/skills_core.py index a8c8cf03..e5cb6cbc 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_core.py +++ b/packages/client/src/launchdarkly_ai_server/skills_core.py @@ -23,6 +23,7 @@ from dataclasses import dataclass from typing import Any, Literal, Protocol, get_args +from .shutdown_hooks import register_shutdown_hook from .types import Skill, SkillOutcomeReason, SkillReference from .types_validation import is_valid_skill_key, is_valid_skill_version @@ -190,6 +191,11 @@ def clear_state() -> None: _emitter = _NOOP_EMITTER +# Registered on load, so core's ``shutdown()`` clears this state without +# importing any Agent Skills module (TESTING.md §0.3). +register_shutdown_hook("Agent Skills", clear_state) + + def get_store() -> SkillStore | None: """The configured store, or ``None``. The only reader of the global.""" return _store diff --git a/packages/client/tests/test_lifecycle.py b/packages/client/tests/test_lifecycle.py index c1469868..50a7e733 100644 --- a/packages/client/tests/test_lifecycle.py +++ b/packages/client/tests/test_lifecycle.py @@ -4,12 +4,15 @@ """ import os +import subprocess +import sys from typing import Any from unittest.mock import AsyncMock, MagicMock, patch import pytest import launchdarkly_ai_server.lifecycle as lifecycle_module +import launchdarkly_ai_server.shutdown_hooks as shutdown_hooks from launchdarkly_ai_server import get_client, init_client, inspect_config, shutdown from launchdarkly_ai_server.lifecycle import _reset_for_testing from launchdarkly_ai_server.sdk_info import ( @@ -532,18 +535,38 @@ async def test_a_failure_clearing_experimental_state_is_logged_not_raised( stub = _make_stub_client() with patch.object(lifecycle_module, "_setup_telemetry", return_value=None): await init_client(client=stub) + later = MagicMock() + + def failing() -> None: + raise RuntimeError("clear failed") + with ( - patch.object( - lifecycle_module.skills, - "_clear_state", - side_effect=RuntimeError("clear failed"), - ), + patch.dict(shutdown_hooks._hooks, clear=False), caplog.at_level("WARNING", logger="launchdarkly_ai_server.lifecycle"), ): + shutdown_hooks.register_shutdown_hook("Failing Feature", failing) + shutdown_hooks.register_shutdown_hook("Later Feature", later) await shutdown() # must not raise stub.close.assert_called_once() assert lifecycle_module._client is None - assert "Could not clear the Agent Skills state" in caplog.messages + assert "Could not clear the Failing Feature state" in caplog.messages + later.assert_called_once() # one failing hook does not skip the rest + + def test_core_does_not_import_experimental_code(self) -> None: + """Core never hooks into experimental code (TESTING.md §0.3).""" + script = ( + "import asyncio, sys\n" + "import launchdarkly_ai_server\n" + "asyncio.run(launchdarkly_ai_server.shutdown())\n" + "loaded = sorted(m for m in sys.modules if m.startswith(" + "('launchdarkly_ai_server.skills', 'launchdarkly_ai_server.evaluations'," + " 'launchdarkly_ai_server.experimental')))\n" + "print(','.join(loaded))\n" + ) + result = subprocess.run( + [sys.executable, "-c", script], capture_output=True, text=True, check=True + ) + assert result.stdout.strip() == "" async def test_completes_teardown_even_if_flush_throws(self) -> None: stub = _make_stub_client()