From a81479c5cc45a1c6dbb22b20e1b053005696eb8e Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 24 Sep 2026 14:26:09 -0400 Subject: [PATCH 1/2] feat(client): declare ?kinds=agent-skill, and treat a 422 as "no skills yet" Delivery now narrows a connection to the payload kinds it declares and defaults to flags (launchdarkly/streamer#4730), so the store has to ask for the agent-skill payload or receive the environment's flags and no skills at all. The declaration goes on every request, before any basis exists as well as alongside one: it selects what the connection is served rather than describing what the store already holds. It also fixes something that was already wrong. A skill-enabled environment assigns two payloads, so the reader has been warning about the second and reading only the first intent, and the flag payload was re-downloaded and discarded on every reconnect because its basis was never adopted. Declaring one kind makes the connection single-payload, which is the shape the reader is built for. The 422 that comes with it is the interesting half. It is the answer when the credential is assigned no agent-skill payload, which is every project where no skill has ever been created -- gonfalon creates that row with the first skill and never lazily. As an ordinary recoverable failure it would spend max_consecutive_failures and then report "gave up after N consecutive failures: HTTP 422" for an ordinary configuration; as a fatal one, the skill created a minute later would never arrive without a process restart. So it is its own class: _NoSkillPayloadError, caught ahead of _RecoverableTransportError, said once, counted under the new payload_unavailable diagnostic, retried at max_backoff indefinitely, and kept off connection_failures, last_error and failed. The retry is at the cap because _failures deliberately never moves, so the exponential schedule would otherwise sit at the initial delay forever. Nine tests, each of which fails with the source reverted: the declaration on both endpoints and on a from-scratch retry, the two kind constants held apart, the 422's classification, that it never stops delivery and never counts, that it waits the cap and not the initial delay, that a skill arriving after it is picked up, and that it leaves the store uninitialised so a wildcard reconcile prunes nothing. The fake endpoint gained a standing default status, since "every request is answered 422" is not something a queue can express. Also corrects docs that described the payload as classified `generic` and the request as carrying `mv`. Gate from python/: make test (1888 passed, 11 skipped), typecheck, lint, format-check all clean. Co-Authored-By: Claude Opus 5 --- packages/client/README.md | 10 +- packages/client/agents.md | 42 +++- .../src/launchdarkly_ai_server/skills_fdv2.py | 133 +++++++++-- packages/client/tests/test_skills_fdv2.py | 215 +++++++++++++++++- 4 files changed, 366 insertions(+), 34 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index d1966b5..45ddb78 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -542,6 +542,14 @@ that as every skill having been revoked and delete the files it wrote on a previ against a store that has not received a payload reports the retrieval unavailable and leaves everything on disk alone. `report.ok` is `False` in that case, and the error names it. +**A project with no skills yet is a waiting state, not a failure.** Every request declares +the payload it wants (`kinds=agent-skill`), and LaunchDarkly answers HTTP 422 when the +environment has no such payload — which is the case until somebody creates the first skill in +the project. The store says so once, keeps asking at its backoff cap, and picks up that first +skill without a restart. `failed` stays `None` throughout, `connection_failures` stays at +zero, and `diagnostics.payload_unavailable` counts the answers: nonzero and rising beside an +empty store means "there is nothing to deliver", not "delivery is broken". + **Nothing above the store changes.** The accessors, verification, and `write_skills` see raw objects through the `SkillStore` interface and cannot tell which store produced them. @@ -622,7 +630,7 @@ Windows. | `InMemorySkillStore(objects=None)` | A dict-backed store with `put(raw)`, for local development and testing. Holds several versions of a key. | | `FDv2SkillStore(sdk_key, *, base_uri=…, stream_uri=…, mode="stream", …)` | The delivery transport: a store fed by LaunchDarkly over the SDK-facing FDv2 channel. `start()`, `wait_for_skills(timeout)`, `is_initialized()`, `close()`, `diagnostics`, `failed`; also a context manager. `base_uri` and `stream_uri` are separate hosts, defaulting to LaunchDarkly's polling and streaming origins; `base_uri` alone covers both. `close()` is **final** — `start()` afterwards raises. **Server-side only** — a mobile key or client-side environment ID raises. See *Receiving skills from LaunchDarkly* above. | | `watch_skills(skills, root, *, debounce=0.5, on_reconcile=None, …)` | `write_skills` plus a re-reconcile on every delivery change. Returns `(initial report, SkillWatcher)`; close the watcher when done. Revocation then takes effect within `debounce` of arriving rather than at the next restart. `debounce` is in **seconds** and must be non-negative and finite; `on_reconcile` is called with each *subsequent* report, the initial one being returned directly. One watcher per root. | -| `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `hashless_objects`, `connection_failures`, `last_error`. | +| `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `hashless_objects`, `connection_failures`, `payload_unavailable`, `last_error`. | Configure the store with `init_client(options={"skillStore": store})`. With none configured, the accessors raise `RuntimeError` explaining what to do and `write_skills` reports the diff --git a/packages/client/agents.md b/packages/client/agents.md index 77c9c7c..30de942 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -250,12 +250,36 @@ deletes the user's files. ### The delivery transport, and the one field that will bite you `FDv2SkillStore` speaks LaunchDarkly's SDK-facing FDv2 channel (`GET /sdk/poll`, -`GET /sdk/stream`, server-side SDK key in `Authorization`, `basis` + `mv` params, +`GET /sdk/stream`, server-side SDK key in `Authorization`, `kinds` + `basis` params, `If-None-Match`/304). It lives below the store interface and produces raw objects in the shape `skills_core.SkillStore` documents; **nothing above that interface knows it exists**. If a transport change ever seems to require editing an accessor, verification, or `write_skills`, the adapter boundary is wrong. +**Every request declares `kinds=agent-skill`, and that is load-bearing.** Delivery narrows a +connection to the payload kinds it declares and defaults to flags, so a request without it is +served the environment's flag payload and no skills at all — the store would run, report +healthy, and hold nothing. It also makes the connection carry exactly one payload, which is +the shape `_ProtocolReader` is built for: without it, a skill-enabled environment assigns two, +and the reader warns about the second and reads only the first intent. `FDV2_PAYLOAD_KIND` is +the payload's kind and `FDV2_OBJECT_KIND` the kind of the objects inside it — two different +strings, held apart on purpose. No `mv`: that parameter selects the *flag* data model, and +delivery overrides whatever a request asks for with the payload's own default for any +non-flagging payload, so sending it would state a preference that is ignored. + +**HTTP 422 is not a failure.** It is the answer to that declaration when the credential is +assigned no agent-skill payload, which is every project in which no skill has ever been +created — gonfalon creates that payload row with the first skill and never lazily. So +`_classify_status` maps it to `_NoSkillPayloadError`, and `_run` catches that **ahead of** +`_RecoverableTransportError`: counted under `diagnostics.payload_unavailable`, logged once per +store, retried at `max_backoff` indefinitely, and kept off `connection_failures`, +`last_error`, and `failed`. Neither of the two obvious classifications is right — as a +recoverable failure it spends `max_consecutive_failures` and then gives up permanently on an +ordinary configuration; as a fatal one, the skill created a minute later never arrives without +a process restart. The retry is at the *cap* rather than the initial backoff because +`_failures` deliberately never moves, so the exponential schedule would sit at the initial +delay forever. + **The key travels over TLS only, and only to the base URI.** `_require_https_base_uri` refuses a plain `http://` base URI in the constructor — the SDK key would go out in cleartext — with a loopback exemption (`localhost`, `127.0.0.1`, `::1`) because the test suite's fake endpoints @@ -287,7 +311,7 @@ stored `version`, under the stored key `pdf-extraction`. `version` (42) is the v including a flag with nothing to do with skills. Reading it as the skill's version fails **silently**: the object verifies, the hash matches, and the caller gets content under a version number that means nothing. There is no separate field for the skill's version: the -agent-skill payload is a *generic* payload, and generic objects carry only `key`, `kind`, +agent-skill payload is opaque to delivery, and its objects carry only `key`, `kind`, `version` and `object`, exactly like a flag. `_split_wire_key` is the only place the wire key is read, `_store_object_from_put` and `_tombstone_from_delete` both go through it, and `TestVersionTranslation` asserts the translation in both directions. A wire key that will @@ -297,13 +321,13 @@ recognises; only a key with nothing before the delimiter is dropped, since there identity to hold it under. **Skills are identified by `kind == "skill"`; everything else is ignored, not rejected.** -Object kinds on the SDK-facing channel are open strings, and the agent-skill payload is -classified `generic`, so a skill arrives under the kind its producer registered — the bare -category name — not under a broader wrapper kind with a narrowing field. An environment's -payload assignment carries its flag payload alongside its agent-skill payload, so flag and -segment objects arrive as a matter of course. Erroring on an unrecognised kind would turn a -normal payload into a permanent reconnect loop — a flag-delivery outage caused by a skills -rollout. +Object kinds on the SDK-facing channel are open strings, so a skill arrives under the kind its +producer registered — the bare category name — not under a broader wrapper kind with a +narrowing field. The `kinds` declaration above means a flag or segment object should no longer +arrive at all, but the skip stays and stays tested: erroring on an unrecognised kind would +turn a payload that merely gained a new object kind into a permanent reconnect loop — a +flag-delivery outage caused by a skills rollout — and an object nobody expected belongs +outside the skill set rather than in it. **Changes commit at `payload-transferred`, not as objects arrive.** A payload version is the unit of consistency: a half-applied full transfer would publish a state the server never diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index cb8d60c..14a4f8e 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -66,12 +66,31 @@ """ The FDv2 ``kind`` skills are delivered under. -Object kinds on the SDK-facing channel are open strings: the agent-skill payload -is classified ``generic`` and every object in it carries the kind its producer -registered, which for skills is the bare category name. Delivery lower-cases the -kind, so an exact comparison is the whole test. The value happens to equal -``skills_core.SKILL_OBJECT_KIND``; they remain separate constants, because one is -a wire value LaunchDarkly owns and the other is what this SDK asks a store for. +Object kinds on the SDK-facing channel are open strings: every object in the +agent-skill payload carries the kind its producer registered, which for skills is +the bare category name. Delivery lower-cases the kind, so an exact comparison is +the whole test. The value happens to equal ``skills_core.SKILL_OBJECT_KIND``; +they remain separate constants, because one is a wire value LaunchDarkly owns and +the other is what this SDK asks a store for. + +Not to be confused with ``FDV2_PAYLOAD_KIND``: this is the kind of the *objects*, +that is the kind of the *payload* they arrive in. +""" + +FDV2_PAYLOAD_KIND = "agent-skill" +""" +The kind of the FDv2 payload skills are delivered in, declared on every request +as ``?kinds=``. + +Delivery narrows a connection to the payload kinds it declares and defaults to +flags, so this is not an optimisation: a request that omits it receives the +environment's flag payload and no skills at all. Declaring it is also what makes +the connection carry exactly one payload -- the shape ``_ProtocolReader`` is +built for -- since a skill-enabled environment assigns both the flag payload and +this one. + +The wire accepts a comma-separated list, but this store wants the skill payload +and nothing else, so it declares this one kind alone. """ FDV2_KEY_DELIMITER = ":" @@ -287,6 +306,19 @@ class StoreDiagnostics: """ connection_failures: int = 0 """Recoverable transport failures since the last successful transfer.""" + payload_unavailable: int = 0 + """ + Requests LaunchDarkly answered with "no payload of the kind you asked for" + (HTTP 422) -- the answer for a project in which no skill has ever been + created. + + Deliberately not a ``connection_failures``: nothing is wrong, there is + nothing to deliver. Nonzero and rising alongside an empty store is the + difference between "this environment has no skills" and "delivery is + broken", which is the pair this whole type exists to separate. It does not + reset, so a store that was empty and then received its first payload reads + as both, in the order it happened. + """ last_error: str | None = None """The most recent transport error, if any. Human-readable; do not parse.""" @@ -973,6 +1005,28 @@ def __init__(self, message: str, retry_after: float | None = None) -> None: self.retry_after = retry_after +class _NoSkillPayloadError(_RecoverableTransportError): + """ + An HTTP 422: delivery has no payload of the kind this store declared. + + That is the answer for every environment whose project has never had a + skill, because the agent-skill payload is created with the first one and the + declaration then matches nothing the credential is assigned. So it is + neither a failure nor fatal, and is classed as neither: + + - counting it as a failure would spend ``max_consecutive_failures`` and then + give up permanently -- reported as "gave up after N consecutive failures" + -- on a configuration that is merely waiting for its first skill; + - treating it as fatal would mean the skill created a minute later never + arrives, because nothing reopens delivery short of a process restart. + + The loop therefore handles it ahead of ``_RecoverableTransportError``: said + once, counted under ``diagnostics.payload_unavailable``, kept off + ``connection_failures`` and ``last_error``, and retried at the backoff cap + for as long as the store is open. + """ + + class _StaleRequestStateError(_RecoverableTransportError): """ An HTTP 400 for a request carrying client state — the ``basis`` selector, or @@ -1057,6 +1111,17 @@ def _classify_status(status: int, headers: Any) -> Exception: return _StaleRequestStateError( f"LaunchDarkly returned HTTP 400. {_REQUEST_ADVICE}" ) + if status == 422: + # Not "retrying will not fix it" and not a failure either: the payload + # this store asks for does not exist yet. Creating the first skill in + # the project is what fixes it, and delivery keeps asking until then. + return _NoSkillPayloadError( + "LaunchDarkly has no Agent Skills payload for this environment " + "(HTTP 422). This is what it answers until the first skill is " + "created in this project, so delivery keeps asking and picks one up " + "without a restart. If this environment does have skills, check that " + "this SDK key belongs to it." + ) if status in (405, 406, 414, 501): return _FatalTransportError( f"LaunchDarkly returned HTTP {status}, which retrying will not fix. " @@ -1198,19 +1263,25 @@ def interrupt(self) -> None: def _url(self, origin: str, path: str, basis: str | None) -> str: """ - The request URL: the path, plus ``basis`` once a payload has committed. + The request URL: the path, the payload kind this store accepts, and + ``basis`` once a payload has committed. *origin* is the host for this path — polling and streaming have one each. + ``kinds`` is on every request, including the first one, because it + selects what the connection is served rather than describing what it + already holds (see ``FDV2_PAYLOAD_KIND``). + Deliberately no ``mv`` (data model version). That parameter selects the - *flag* data model and the connection rejects any value but the flag - default; the agent-skill payload is generic, is served regardless of it, - and has no model version of its own to ask for. + *flag* data model; delivery overrides whatever a request asks for with + the payload's own default for any non-flagging payload, so sending it + would state a preference that is ignored. """ - if not basis: - return f"{origin}{path}" - return f"{origin}{path}?{urllib.parse.urlencode({'basis': basis})}" + query: dict[str, str] = {"kinds": FDV2_PAYLOAD_KIND} + if basis: + query["basis"] = basis + return f"{origin}{path}?{urllib.parse.urlencode(query)}" def _request( self, origin: str, path: str, basis: str | None, headers: dict[str, str] @@ -1633,6 +1704,11 @@ def __init__( # A stream only ever ends by being dropped, so this is what separates # a recycled healthy connection from one that failed. self._attempt_answered = False + # Whether the "no skill payload for this environment" line has been + # said. Once per store, not once per attempt: the condition persists + # until somebody creates a skill, and delivery keeps asking the whole + # time. + self._warned_no_skill_payload = False # -- lifecycle --------------------------------------------------------- @@ -1890,6 +1966,37 @@ def _run(self) -> None: except _FatalTransportError as exc: self._give_up(str(exc)) return + except _NoSkillPayloadError as exc: + # Ahead of ``_RecoverableTransportError``, which it subclasses, + # because none of that block applies: there is no failure to + # count, no ``last_error`` to leave on a store that is working + # fine, and no budget to spend on a project that has simply not + # created a skill yet. + if self._stop.is_set(): + return + with self._lock: + # Defensive: a 422 refuses the connection before it opens, + # so there is nothing in flight unless an earlier attempt + # left it there. + self._reader._abandon_in_flight() + self._reader.diagnostics.payload_unavailable += 1 + say_it = not self._warned_no_skill_payload + self._warned_no_skill_payload = True + if say_it: + logger.warning( + "Skill delivery is idle: %s Retrying every %.1fs; this " + "is the only time it will be said.", + exc, + self._max_backoff, + ) + # Not backing off from a failure, waiting for somebody to create + # a skill. ``_failures`` never moved, so the exponential + # schedule would hold this at the *initial* delay forever -- the + # cap is both the cheapest place to sit and the one that does + # not depend on a counter this case deliberately leaves alone. + if self._stop.wait(self._max_backoff): + return + continue except _RecoverableTransportError as exc: if self._stop.is_set(): # ``close`` interrupted the request on purpose. Counting it diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index b11ef4e..f620989 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -39,6 +39,7 @@ init_client, skills_fdv2, watch_skills, + write_skills, ) from launchdarkly_ai_server.skills_core import SKILL_OBJECT_KIND from launchdarkly_ai_server.skills_fdv2 import ( @@ -48,12 +49,14 @@ DEFAULT_STREAM_URI, FDV2_KEY_DELIMITER, FDV2_OBJECT_KIND, + FDV2_PAYLOAD_KIND, MAX_RESPONSE_BYTES, _backoff_delay, _classify_status, _FatalTransportError, _is_skill_event, _iter_sse, + _NoSkillPayloadError, _ProtocolReader, _RecoverableTransportError, _Requester, @@ -206,6 +209,12 @@ def __init__(self) -> None: self._streams: list[list[dict[str, Any]]] = [] self._lock = threading.Lock() self.hold_stream_open = False + # What a poll is answered with once the queued script runs out. 304 -- + # "nothing has changed" -- is the right default for a healthy + # environment, but a standing 422 is what an environment with no skill + # payload answers *every* request with, and a queue cannot express + # "every". + self.default_poll_status = 304 # When set, every ``/sdk/stream`` answers 307 to this URL instead of # streaming, so a test can check that the store refuses to follow it. self.redirect_stream_to: str | None = None @@ -290,7 +299,9 @@ def queue_stream(self, payload_events: list[dict[str, Any]]) -> None: def _serve_poll(self, handler: BaseHTTPRequestHandler) -> None: with self._lock: response = ( - self._polls.pop(0) if self._polls else {"status": 304, "events": []} + self._polls.pop(0) + if self._polls + else {"status": self.default_poll_status, "events": []} ) status = response["status"] handler.send_response(status) @@ -381,11 +392,24 @@ class TestObjectIdentification: def test_the_kind_alone_identifies_a_skill(self) -> None: assert _is_skill_event(put_skill()) is True + def test_the_payload_kind_and_the_object_kind_are_separate_constants( + self, + ) -> None: + """ + Two different things, and neither is derived from the other: the store + asks for a payload of kind ``agent-skill`` and reads objects of kind + ``skill`` out of it. Held apart so that a rename of either cannot + silently move the other. + """ + assert FDV2_PAYLOAD_KIND == "agent-skill" + assert FDV2_OBJECT_KIND == "skill" + assert FDV2_PAYLOAD_KIND != FDV2_OBJECT_KIND + def test_the_kind_is_the_bare_category_name(self) -> None: """ - Object kinds on the channel are open strings and the agent-skill payload - is ``generic``, so a skill arrives under the kind its producer - registered — ``skill`` — not under a broader wrapper kind. + Object kinds on the channel are open strings, so a skill arrives under + the kind its producer registered — ``skill`` — not under a broader + wrapper kind, and not under the kind of the payload carrying it. """ assert FDV2_OBJECT_KIND == "skill" @@ -1231,10 +1255,10 @@ def test_the_request_carries_the_sdk_key_and_no_data_model_version( self, endpoint: Any ) -> None: """ - No ``mv``: that parameter selects the *flag* data model, the connection - rejects any value but the flag default, and the generic agent-skill - payload is served regardless of it. Sending ``mv=1`` — the skill - payload's own model version — gets the whole connection refused. + No ``mv``: that parameter selects the *flag* data model, and delivery + overrides whatever a request asks for with the payload's own default for + any non-flagging payload. Sending it would state a preference that is + ignored, so the store states none. """ endpoint.queue_poll(full_payload(("put-object", put_skill()))) with poll_store(endpoint) as store: @@ -1244,6 +1268,28 @@ def test_the_request_carries_the_sdk_key_and_no_data_model_version( assert first["authorization"] == SDK_KEY assert "mv" not in first["query"] + def test_every_request_declares_the_agent_skill_payload_kind( + self, endpoint: Any + ) -> None: + """ + Delivery narrows a connection to the kinds it declares and defaults to + flags, so a request without this parameter is served the environment's + flag payload and no skills at all. It is on the first request as well as + the ones after it: the declaration selects what the connection is served + rather than describing what it already holds, so there is no state for + it to wait on. + """ + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + endpoint.queue_poll(status=304) + with poll_store(endpoint, poll_interval=0.02) as store: + assert store.wait_for_skills(timeout=5) is True + assert wait_until(lambda: len(endpoint.requests) >= 2) + first, second = endpoint.requests[0], endpoint.requests[1] + assert first["query"]["kinds"] == "agent-skill" + assert "basis" not in first["query"] + assert second["query"]["kinds"] == "agent-skill" + assert second["query"]["basis"] == "basis-1" + def test_the_first_request_sends_no_basis(self, endpoint: Any) -> None: endpoint.queue_poll(full_payload(("put-object", put_skill()))) with poll_store(endpoint) as store: @@ -1438,6 +1484,20 @@ def test_the_stream_request_advertises_event_stream(self, endpoint: Any) -> None assert endpoint.requests[0]["path"] == "/sdk/stream" assert endpoint.requests[0]["accept"] == "text/event-stream" + def test_the_stream_request_declares_the_payload_kind(self, endpoint: Any) -> None: + """Asserted separately from polling: the two hosts are different + origins, and nothing stops one path from building its URL without the + declaration that selects what the connection is served.""" + endpoint.hold_stream_open = True + endpoint.queue_stream(full_payload(("put-object", put_skill()))) + store = FDv2SkillStore(SDK_KEY, base_uri=endpoint.base_uri, mode="stream") + try: + store.start() + store.wait_for_skills(timeout=5) + finally: + store.close() + assert endpoint.requests[0]["query"]["kinds"] == "agent-skill" + def test_a_streamed_revocation_arrives_without_a_restart( self, endpoint: Any ) -> None: @@ -1723,6 +1783,19 @@ def stream_store(**kwargs: Any) -> FDv2SkillStore: ) +class _NoWaitStop(threading.Event): + """A ``_stop`` that answers every wait at once. + + Retrying at the backoff cap is the right production behaviour and the wrong + test fixture: a test that only cares what happens *after* the wait should + not sit through one. Kept out of ``poll_store`` so the waits stay real + everywhere they are part of what is being asserted. + """ + + def wait(self, timeout: float | None = None) -> bool: + return super().wait(0.001) + + class TestFailureHandling: def test_a_403_stops_delivery_and_explains_why( self, endpoint: Any, caplog: Any @@ -1858,8 +1931,10 @@ def test_a_400_reconnects_once_from_scratch_and_is_then_fatal( # The premise: the rejected request did carry client state to drop. assert "basis" in endpoint.requests[1]["query"] # The retry was from scratch: no selector and no etag on the way back. + # The kind declaration stays, since it is not client state — it selects + # what the connection is served. retried = endpoint.requests[2] - assert retried["query"] == {} + assert retried["query"] == {"kinds": FDV2_PAYLOAD_KIND} assert retried["if_none_match"] is None # Last known good survives both. assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None @@ -1887,7 +1962,7 @@ def test_a_400_still_reconnects_from_scratch_on_a_spent_budget( assert store.failed is None # The repair went out from scratch rather than never going out at all. repair = endpoint.requests[3] - assert repair["query"] == {} + assert repair["query"] == {"kinds": FDV2_PAYLOAD_KIND} assert repair["if_none_match"] is None def test_a_non_400_after_the_repair_meets_the_spent_budget( @@ -1922,7 +1997,7 @@ def test_a_400_carrying_no_client_state_is_fatal_at_once( assert "400" in store.failed assert len(endpoint.requests) == 1 - def test_the_two_exceptional_statuses_are_classified_apart(self) -> None: + def test_the_exceptional_statuses_are_classified_apart(self) -> None: # The classification is the contract; the end-to-end tests above are # what prove the loop honours it. assert isinstance(_classify_status(404, None), _FatalTransportError) @@ -1934,6 +2009,124 @@ def test_the_two_exceptional_statuses_are_classified_apart(self) -> None: assert isinstance(_classify_status(status, None), _FatalTransportError) assert isinstance(_classify_status(503, None), _RecoverableTransportError) assert not isinstance(_classify_status(503, None), _StaleRequestStateError) + # 422 is the third class: recoverable enough to be retried, but its own + # type so the loop can keep it off the failure budget. + no_payload = _classify_status(422, None) + assert isinstance(no_payload, _NoSkillPayloadError) + assert isinstance(no_payload, _RecoverableTransportError) + assert not isinstance(no_payload, _FatalTransportError) + # The message explains the state rather than reciting the status: this + # is the line a user reads when their skills never show up. + assert "no Agent Skills payload" in str(no_payload) + + def test_a_422_never_stops_delivery_and_is_not_counted_as_a_failure( + self, endpoint: Any, caplog: Any + ) -> None: + """ + A 422 means the credential is assigned no agent-skill payload, which is + every project in which no skill has ever been created. Counting it as a + failure would spend the budget and report an ordinary configuration as + "gave up after N consecutive failures"; so it is counted, said once, and + retried for as long as the store is open. + """ + endpoint.default_poll_status = 422 + store = poll_store(endpoint, max_consecutive_failures=1) + with caplog.at_level("WARNING"): + with store: + # On the diagnostic rather than the request count: the counter + # moves after the response is read, so the fourth request being + # logged does not mean the fourth 422 has been handled. + assert wait_until(lambda: store.diagnostics.payload_unavailable >= 4) + # Well past a bound of one, and still asking. + assert store.failed is None + assert store.diagnostics.payload_unavailable >= 4 + # None of it reads as a failure, because none of it is one. + assert store.diagnostics.connection_failures == 0 + assert store.diagnostics.last_error is None + # Said once, not once per attempt. + idle = [r for r in caplog.records if "Skill delivery is idle" in r.getMessage()] + assert len(idle) == 1 + assert "no Agent Skills payload" in idle[0].getMessage() + # And nothing was logged as a failure. + assert not any( + "Skill delivery failed" in r.getMessage() for r in caplog.records + ) + + def test_a_422_waits_the_backoff_cap_rather_than_the_initial_delay( + self, endpoint: Any + ) -> None: + """ + The exponential schedule is a function of the failure count, which this + case deliberately never advances — so reusing it would hold a store + waiting for its first skill at the *initial* delay forever, polling as + fast as a fresh connection retries. + + Reaches into ``_stop`` because the delay is the thing under test: the + loop's wait is recorded and then not actually waited out, so the + assertion is on the interval asked for rather than on wall-clock timing. + """ + asked: list[float | None] = [] + + class RecordingStop(threading.Event): + def wait(self, timeout: float | None = None) -> bool: + asked.append(timeout) + return super().wait(0.001) + + endpoint.default_poll_status = 422 + store = poll_store(endpoint, initial_backoff=0.01, max_backoff=7.5) + store._stop = RecordingStop() + with store: + assert wait_until(lambda: len(endpoint.requests) >= 3) + assert asked, "the loop never waited" + assert asked[0] == 7.5 + assert 0.01 not in asked + + def test_a_skill_payload_arriving_after_a_422_is_picked_up( + self, endpoint: Any + ) -> None: + """ + The reason this is not fatal. A skill created after the store started is + delivered to the store that was already running, with nothing restarted. + """ + store = poll_store(endpoint, max_consecutive_failures=1) + endpoint.queue_poll(status=422) + endpoint.queue_poll(status=422) + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + store._stop = _NoWaitStop() + with store: + assert store.wait_for_skills(timeout=5) is True + assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None + # Both readings, in the order they happened. + assert store.diagnostics.payload_unavailable == 2 + assert store.diagnostics.payloads_transferred == 1 + assert store.failed is None + + async def test_a_422_leaves_the_store_uninitialised_and_prunes_nothing( + self, endpoint: Any, tmp_path: Any + ) -> None: + """ + "LaunchDarkly has no skill payload for this environment" and "this + environment's every skill was revoked" are the two readings of an empty + answer, and only the second may delete a customer's files. A 422 commits + no payload, so the readiness probe stays false and the wildcard + reconcile withholds the prune. + """ + root = tmp_path / "skills" + stale = root / "left-behind" + stale.mkdir(parents=True) + (stale / "SKILL.md").write_text("not ours to delete", encoding="utf-8") + + endpoint.default_poll_status = 422 + store = poll_store(endpoint, max_consecutive_failures=1) + store._stop = _NoWaitStop() + with store: + assert wait_until(lambda: store.diagnostics.payload_unavailable >= 3) + assert store.is_initialized() is False + assert store.wait_for_skills(timeout=0.1) is False + await init_client(options={"skillStore": store}, client=object()) + report = await write_skills("*", root) + assert (stale / "SKILL.md").exists() + assert not any(a.action == "removed" for a in report.actions) def test_a_401_stops_delivery(self, endpoint: Any) -> None: endpoint.queue_poll(status=401) From b4f29a8561eeb44040fbcb119c2f02f298976129 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 24 Sep 2026 15:28:29 -0400 Subject: [PATCH 2/2] docs(client): say the 422's cause once, on the type that names it "The answer for a project in which no skill has ever been created" appeared seven times in one file: on the diagnostic, on the exception class, in _classify_status, on the once-per-store flag, and twice in the delivery loop. _NoSkillPayloadError now owns the explanation, since it is what the other sites refer to, and each of those states only what is local to it: the diagnostic names the type and keeps the "not a connection_failures" distinction, the except block keeps why it is caught first and why it waits the cap, and _classify_status keeps nothing -- the type it returns and the message it builds already say it twice over. No behaviour change, and the user-facing 422 message is untouched. 15 lines of comment removed. Co-Authored-By: Claude Opus 5 --- .../src/launchdarkly_ai_server/skills_fdv2.py | 69 ++++++++----------- 1 file changed, 27 insertions(+), 42 deletions(-) diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index 14a4f8e..ea357d7 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -74,7 +74,7 @@ the other is what this SDK asks a store for. Not to be confused with ``FDV2_PAYLOAD_KIND``: this is the kind of the *objects*, -that is the kind of the *payload* they arrive in. +that one the kind of the *payload* they arrive in. """ FDV2_PAYLOAD_KIND = "agent-skill" @@ -308,16 +308,13 @@ class StoreDiagnostics: """Recoverable transport failures since the last successful transfer.""" payload_unavailable: int = 0 """ - Requests LaunchDarkly answered with "no payload of the kind you asked for" - (HTTP 422) -- the answer for a project in which no skill has ever been - created. + Requests answered with "no payload of the kind you asked for" + (``_NoSkillPayloadError``). Cumulative, and never reset. Deliberately not a ``connection_failures``: nothing is wrong, there is nothing to deliver. Nonzero and rising alongside an empty store is the difference between "this environment has no skills" and "delivery is - broken", which is the pair this whole type exists to separate. It does not - reset, so a store that was empty and then received its first payload reads - as both, in the order it happened. + broken", which is the pair this whole type exists to separate. """ last_error: str | None = None """The most recent transport error, if any. Human-readable; do not parse.""" @@ -1009,21 +1006,19 @@ class _NoSkillPayloadError(_RecoverableTransportError): """ An HTTP 422: delivery has no payload of the kind this store declared. - That is the answer for every environment whose project has never had a - skill, because the agent-skill payload is created with the first one and the - declaration then matches nothing the credential is assigned. So it is - neither a failure nor fatal, and is classed as neither: - - - counting it as a failure would spend ``max_consecutive_failures`` and then - give up permanently -- reported as "gave up after N consecutive failures" - -- on a configuration that is merely waiting for its first skill; - - treating it as fatal would mean the skill created a minute later never - arrives, because nothing reopens delivery short of a process restart. - - The loop therefore handles it ahead of ``_RecoverableTransportError``: said - once, counted under ``diagnostics.payload_unavailable``, kept off - ``connection_failures`` and ``last_error``, and retried at the backoff cap - for as long as the store is open. + That is the answer for every project in which no skill has ever been + created, since the agent-skill payload row is created with the first one. + Neither of the two obvious classifications is right, which is why this is + its own class: + + - as a failure it would spend ``max_consecutive_failures`` and then give up + permanently -- "gave up after N consecutive failures" -- on a + configuration that is merely waiting for its first skill; + - as fatal, the skill created a minute later would never arrive, because + nothing reopens delivery short of a process restart. + + ``_run`` handles it ahead of ``_RecoverableTransportError``, which it + subclasses. """ @@ -1112,9 +1107,6 @@ def _classify_status(status: int, headers: Any) -> Exception: f"LaunchDarkly returned HTTP 400. {_REQUEST_ADVICE}" ) if status == 422: - # Not "retrying will not fix it" and not a failure either: the payload - # this store asks for does not exist yet. Creating the first skill in - # the project is what fixes it, and delivery keeps asking until then. return _NoSkillPayloadError( "LaunchDarkly has no Agent Skills payload for this environment " "(HTTP 422). This is what it answers until the first skill is " @@ -1704,10 +1696,8 @@ def __init__( # A stream only ever ends by being dropped, so this is what separates # a recycled healthy connection from one that failed. self._attempt_answered = False - # Whether the "no skill payload for this environment" line has been - # said. Once per store, not once per attempt: the condition persists - # until somebody creates a skill, and delivery keeps asking the whole - # time. + # Said once per store rather than once per attempt: the condition holds + # until somebody creates a skill, and delivery keeps asking throughout. self._warned_no_skill_payload = False # -- lifecycle --------------------------------------------------------- @@ -1967,17 +1957,14 @@ def _run(self) -> None: self._give_up(str(exc)) return except _NoSkillPayloadError as exc: - # Ahead of ``_RecoverableTransportError``, which it subclasses, - # because none of that block applies: there is no failure to - # count, no ``last_error`` to leave on a store that is working - # fine, and no budget to spend on a project that has simply not - # created a skill yet. + # Ahead of the recoverable block below, none of which applies: + # nothing failed, so there is no count to advance and no + # ``last_error`` to leave on a store that is working fine. if self._stop.is_set(): return with self._lock: - # Defensive: a 422 refuses the connection before it opens, - # so there is nothing in flight unless an earlier attempt - # left it there. + # A 422 refuses the connection before it opens, so this is + # only for a payload an earlier attempt left in flight. self._reader._abandon_in_flight() self._reader.diagnostics.payload_unavailable += 1 say_it = not self._warned_no_skill_payload @@ -1989,11 +1976,9 @@ def _run(self) -> None: exc, self._max_backoff, ) - # Not backing off from a failure, waiting for somebody to create - # a skill. ``_failures`` never moved, so the exponential - # schedule would hold this at the *initial* delay forever -- the - # cap is both the cheapest place to sit and the one that does - # not depend on a counter this case deliberately leaves alone. + # At the cap rather than on the backoff schedule: ``_failures`` + # deliberately never moves, so the schedule would hold this at + # the *initial* delay forever. if self._stop.wait(self._max_backoff): return continue