diff --git a/packages/client/README.md b/packages/client/README.md index ca0ba47..2898ea7 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -552,13 +552,20 @@ 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 logs it once, keeps asking at its backoff cap, and picks up that first skill without -a restart. `failed` stays `None` throughout, the 422s are kept off `connection_failures` and -`last_error`, and `diagnostics.payload_unavailable` counts them: nonzero and rising beside an -empty store means "there is nothing to deliver", not "delivery is broken". +**A 422 means this connection will never be assigned a skill payload, and delivery stops.** +Every request declares the payload it wants (`kinds=agent-skill`), and LaunchDarkly answers +HTTP 422 when it will not serve one. The cause you can act on is a **view-scoped SDK key**: +a key restricted to a view cannot be assigned a skill payload, so check the key's scoping and +use one that is not view-scoped. The other cause is that Agent Skills delivery is not enabled +for your account, which is not a setting you control — contact LaunchDarkly support if the key +is not the problem. Retrying fixes neither, and LaunchDarkly chose the status so that SDKs stop +rather than hammer the fleet, so the store gives up: `failed` carries the reason, `lastError` is +populated, `connectionFailures` stays at zero (it counts consecutive *recoverable* failures against +the retry bound, which a fatal never spends), and `waitForSkills` resolves `false` immediately +instead of at your timeout. It is **not** the answer for an environment that merely has no skills +yet — with delivery enabled and a non-view-scoped key, an environment holding zero skills is served +an empty payload that commits normally. Because delivery has stopped for good, a process that booted +while the cause was in effect picks up skills only after a restart. **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. @@ -641,7 +648,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`, `payloads_ignored`, `hashless_objects`, `connection_failures`, `payload_unavailable`, `last_error`. | +| `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `payloads_ignored`, `hashless_objects`, `connection_failures`, `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 886dcf1..fb97565 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -267,17 +267,7 @@ strings, held apart on purpose. No `mv`: that parameter selects the *flag* data 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 -— LaunchDarkly creates that payload with the environment's first skill, never in advance. 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. +**HTTP 422 is fatal, and the platform decided that rather than the SDK inferring it.** Delivery answers 422 when a connection's declared kinds exclude every payload it is assigned, and it picked a non-400 4xx *because* LD SDKs treat those as terminal. So `classifyStatus` returns a `FatalTransportError` and the existing give-up path handles it: `failed` and `lastError` are set, `connectionFailures` is untouched (it measures consecutive *recoverable* failures against the retry bound, and a fatal never retries), and `waitForSkills` resolves `false` at once rather than at the timeout. **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 diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index c7c5675..5e9d18f 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -308,16 +308,6 @@ class StoreDiagnostics: """ connection_failures: int = 0 """Recoverable transport failures since the last successful transfer.""" - payload_unavailable: int = 0 - """ - Requests answered "no payload of the kind you asked for" (HTTP 422), which - is the answer until the first skill is created in this project. Cumulative, - and never reset. - - Deliberately not counted as a connection failure: nothing is wrong, there is - nothing to deliver yet. Nonzero and rising alongside an empty store means - this environment has no skills, not that delivery is broken. - """ last_error: str | None = None """The most recent transport error, if any. Human-readable; do not parse.""" @@ -1004,26 +994,6 @@ 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 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. - """ - - class _StaleRequestStateError(_RecoverableTransportError): """ An HTTP 400 for a request carrying client state — the ``basis`` selector, or @@ -1110,12 +1080,13 @@ def _classify_status(status: int, headers: Any) -> Exception: f"LaunchDarkly returned HTTP 400. {_REQUEST_ADVICE}" ) if status == 422: - return _NoSkillPayloadError( - "LaunchDarkly has no Agent Skills payload for this environment " - "(HTTP 422). That is the answer until the first skill is created in " - "this project; when one is, it reaches this store without a restart. " - "If this environment does have skills, check that this SDK key " - "belongs to it." + # Delivery refuses a connection whose declared kinds exclude every + # payload it is assigned, and chose a non-400 4xx precisely so SDKs stop + # instead of retrying. + return _FatalTransportError( + "LaunchDarkly will not deliver Agent Skills on this connection " + "(HTTP 422). The usual cause is a view-scoped SDK key. Check your " + "SDK key or contact LaunchDarkly support. ) if status in (405, 406, 414, 501): return _FatalTransportError( @@ -1699,9 +1670,6 @@ 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 - # 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 --------------------------------------------------------- @@ -1959,32 +1927,6 @@ def _run(self) -> None: except _FatalTransportError as exc: self._give_up(str(exc)) return - except _NoSkillPayloadError as exc: - # 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: - # 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 - self._warned_no_skill_payload = True - if say_it: - logger.warning( - "Skill delivery is idle: %s Retrying every %.1fs. This " - "is logged once.", - exc, - self._max_backoff, - ) - # 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 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 f620989..660e921 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -51,12 +51,12 @@ FDV2_OBJECT_KIND, FDV2_PAYLOAD_KIND, MAX_RESPONSE_BYTES, + StoreDiagnostics, _backoff_delay, _classify_status, _FatalTransportError, _is_skill_event, _iter_sse, - _NoSkillPayloadError, _ProtocolReader, _RecoverableTransportError, _Requester, @@ -1783,19 +1783,6 @@ 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 @@ -2009,123 +1996,172 @@ def test_the_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: + # 422 is fatal, and the platform chose the status to be exactly that: + # a connection whose declared kinds match no payload it is assigned is + # refused, with a code LaunchDarkly SDKs stop on rather than retry. + refused = _classify_status(422, None) + assert isinstance(refused, _FatalTransportError) + assert not isinstance(refused, _RecoverableTransportError) + + def test_every_status_is_either_recoverable_or_fatal(self) -> None: + """ + There is no third class, and the shape matters more than the name. A + status classified as neither broken nor terminal is a retry loop with no + bound and no budget — retrying for the life of the process and invisible + to ``failed`` and ``connection_failures`` alike — which is exactly what + held 422 before it was classified as fatal. + """ + for status in range(300, 600): + classified = _classify_status(status, None) + fatal = isinstance(classified, _FatalTransportError) + recoverable = isinstance(classified, _RecoverableTransportError) + assert fatal ^ recoverable, ( + f"HTTP {status} classified as {type(classified).__name__}, " + "which is neither exactly recoverable nor exactly fatal" + ) + + def test_there_is_no_expected_recoverable_error_class(self) -> None: + """ + Asserted gone by name, because a reintroduction is otherwise visible + only in a log line no test reads. The class existed only to hold 422 and + goes away with that classification. + """ + assert not hasattr(skills_fdv2, "_NoSkillPayloadError") + + def test_diagnostics_does_not_count_an_unavailable_payload(self) -> None: + """ + ``StoreDiagnostics`` is public API from the moment it ships, so its field + list is the contract. A field counting "no payload of the kind you + declared" would count the 422 — and that status is fatal, so there is no + recurring event to accumulate and no running store to accumulate it on. + """ + assert "payload_unavailable" not in StoreDiagnostics.__dataclass_fields__ + assert not hasattr(StoreDiagnostics(), "payload_unavailable") + # The whole list, so a reintroduction under any other name fails too. + assert set(StoreDiagnostics.__dataclass_fields__) == { + "payloads_transferred", + "skill_objects_received", + "objects_ignored", + "objects_revoked", + "payloads_ignored", + "hashless_objects", + "connection_failures", + "last_error", + } + + def test_a_422_on_the_first_response_stops_delivery(self, endpoint: 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. + A 422 means this connection will never be assigned a skill payload, not + that the environment has no skills yet: with delivery enabled and a key + that is not view-scoped, an environment holding zero skills is assigned + an empty payload that commits normally. Every cause of the 422 is + permanent, so the first one is enough to stop on. """ - 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 - ) + endpoint.queue_poll(status=422) + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + # Well above one, to show the bound is not what stopped it. + with poll_store(endpoint, max_consecutive_failures=5) as store: + assert wait_until(lambda: store.failed is not None) + assert "422" in store.failed + # The queued payload is never asked for. + assert len(endpoint.requests) == 1 + assert store.is_initialized() is False - def test_a_422_waits_the_backoff_cap_rather_than_the_initial_delay( - self, endpoint: Any - ) -> None: + def test_the_422_message_names_both_of_its_real_causes(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. + The message is a contract, because it is what a customer pastes into a + support ticket: it has to name the account-level enablement and the key's + scoping so the first person to read it can check both without reading + platform source. - 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. + Asserted on substance rather than prose, so the wording stays free to + improve while a message that drops either cause fails. """ - 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( + endpoint.queue_poll(status=422) + with poll_store(endpoint) as store: + assert wait_until(lambda: store.failed is not None) + message = store.failed.lower() + assert "enabled" in message and "account" in message + assert "view-scoped" in message + # The two things it must not say. Both are what the earlier text said, + # and both are false: the condition is not about whether any skill + # exists, and no skill created later arrives without a restart. + assert "first skill" not in message + assert "without a restart" not in message + # It does name what actually clears the condition. + assert "restart" in message + + def test_a_fatal_422_is_not_counted_against_the_retry_bound( 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. + ``connection_failures`` measures consecutive *recoverable* failures + against the retry bound. A fatal never retries, so counting one would + put a number against a budget nothing will spend and make a store that + gave up on its first response indistinguishable from one that exhausted + its attempts. ``_give_up`` already accounts 401 and 404 this way, and + 422 reaching it means the accounting comes for free — so it is asserted + rather than implemented. """ - store = poll_store(endpoint, max_consecutive_failures=1) endpoint.queue_poll(status=422) + with poll_store(endpoint) as store: + assert wait_until(lambda: store.failed is not None) + assert store.diagnostics.connection_failures == 0 + assert store.diagnostics.last_error is not None + assert store.failed == store.diagnostics.last_error + + def test_a_fatal_422_releases_wait_for_skills_at_once(self, endpoint: Any) -> None: + """ + The observable difference from treating the status as recoverable, and + as much the point of the classification as the stopped retries are: a + boot gated on skills behind a ten-second wait would otherwise pay that + wait on every start, against a store that knew the answer on its first + response. + + The timeout is far longer than the suite would tolerate sitting through, + so the assertion is on the value *and* the elapsed time. + """ 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 + with poll_store(endpoint) as store: + started = time.monotonic() + assert store.wait_for_skills(timeout=30.0) is False + elapsed = time.monotonic() - started + assert elapsed < 5.0, ( + f"waited {elapsed:.1f}s for an answer delivery already had" + ) - async def test_a_422_leaves_the_store_uninitialised_and_prunes_nothing( + async def test_a_store_that_gave_up_on_a_422_prunes_nothing( self, endpoint: Any, tmp_path: Any ) -> None: """ - "LaunchDarkly has no skill payload for this environment" and "this + "LaunchDarkly will not deliver skills to this connection" 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. + reconcile withholds the prune for the whole run. + + The composition is what is under test rather than either half: the + readiness gate already exists, but "delivery gave up, therefore the + store is empty, therefore prune" is the inference an implementation + makes when it has to assemble this behaviour from two sections. It is + also the path a filesystem-agent deployment takes when Agent Skills is + not enabled for the account. """ 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() + endpoint.queue_poll(status=422) + store = poll_store(endpoint) with store: - assert wait_until(lambda: store.diagnostics.payload_unavailable >= 3) + assert wait_until(lambda: store.failed is not None) 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 report.ok is False + assert (stale / "SKILL.md").read_text(encoding="utf-8") == "not ours to delete" assert not any(a.action == "removed" for a in report.actions) def test_a_401_stops_delivery(self, endpoint: Any) -> None: