From 7363face2d05257456a482c5018d0e78d0007082 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Fri, 4 Sep 2026 11:03:15 -0400 Subject: [PATCH 01/13] =?UTF-8?q?feat(client):=20Agent=20Skills=20?= =?UTF-8?q?=E2=80=94=20the=20FDv2=20delivery=20transport?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds the real transport behind the existing SkillStore seam, on LaunchDarkly's SDK-facing FDv2 channel. Nothing above the seam changed: the accessors, integrity verification, and write_skills are untouched, which is the point of the seam and the evidence it was drawn correctly. The design's original Plan A — a bespoke poller against a gonfalon /private/flagdlv route — is dead. That route is authenticated by Cognito machine-token OAuth scopes with no per-tenant authorization, and the security review ruled out both relaxing its auth and shipping a machine credential to customer hosts. This uses GET /sdk/poll and GET /sdk/stream with the environment's server-side SDK key instead, which is also the channel payload signing will eventually cover. skills_fdv2.py — FDv2SkillStore: authenticate, poll or stream, maintain selector/basis state, deserialize inline-resource/skill objects, hold them keyed by (key, objectVersion), and serve the seam. Bounded retries with capped jittered backoff, Retry-After honoured, If-None-Match/304. Standard library only, so the content path adds no dependency. The trap, stated loudly and asserted in both directions: objectVersion is the skill's own version — the one {key, version} pins — while version is the payload version. Confusing them fails silently. seam_object_from_put is the only place the translation happens. Flag and segment objects arrive on the same connection and are skipped cleanly rather than erroring; erroring is the unknown-kind reconnect loop this feature must not reproduce. Changes commit at payload-transferred, so an interrupted full transfer leaves last known good intact. contentHash is read from the envelope and verification semantics are unchanged. The field has not shipped on the write path yet, so its absence is made loudly diagnosable — an error per object naming missing_content_hash, a summary per wholly-hashless payload, and a StoreDiagnostics counter — rather than a silent empty store. There is deliberately no fallback that skips verification. skills_watch.py — watch_skills/SkillWatcher pull the change-listener re-reconcile forward out of phase 4. A delete-object reaches a live stream in seconds, so a revoked skill's SKILL.md now leaves the disk within a debounce interval instead of at the next restart, which materially improves the review's AV-1. on_unavailable="keep" stays the default, as the review endorses. Server-side only: a mobile key or client-side environment ID raises from the constructor. Skill content is customer-confidential and payload assignment is shared across auth types, so this is the SDK-side half of that boundary. close() shuts the socket down under the blocked read rather than only setting a flag — without it every shutdown of a healthy stream blocks for the full join timeout. 108 tests against an in-process fake FDv2 endpoint that implements the contract: put/delete, objectVersion vs version, mixed and unknown-kind payloads, 304, basis round-tripping, reconnect/backoff, Retry-After, bounded retries, seam parity with InMemorySkillStore, and a missing-contentHash envelope producing withheld skills with the right reason code. Co-Authored-By: Claude Opus 5 --- packages/client/README.md | 61 + packages/client/agents.md | 56 + .../src/launchdarkly_ai_server/__init__.py | 7 + .../src/launchdarkly_ai_server/skills_fdv2.py | 1381 ++++++++++++++ .../launchdarkly_ai_server/skills_watch.py | 263 +++ packages/client/tests/test_skills_fdv2.py | 1595 +++++++++++++++++ 6 files changed, 3363 insertions(+) create mode 100644 packages/client/src/launchdarkly_ai_server/skills_fdv2.py create mode 100644 packages/client/src/launchdarkly_ai_server/skills_watch.py create mode 100644 packages/client/tests/test_skills_fdv2.py diff --git a/packages/client/README.md b/packages/client/README.md index bc8df59..50032d6 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -445,6 +445,64 @@ which OS ran the write. The keys stay valid everywhere else: an AI Config refere named `aux` parses, and its other fields are unaffected. If you have a skill named for a device, rename it. +#### Receiving skills from LaunchDarkly + +`InMemorySkillStore` is for tests and bring-your-own-content. In production, skill content +arrives through `FDv2SkillStore`, which speaks LaunchDarkly's SDK-facing FDv2 delivery +channel — the same `GET /sdk/poll` and `GET /sdk/stream` endpoints the base SDK's FDv2 data +source uses, authenticated with the environment's server-side SDK key. + +```python +from launchdarkly_ai_server import FDv2SkillStore, init_client, watch_skills + +store = FDv2SkillStore(os.environ["LD_SDK_KEY"]).start() +store.wait_for_skills(timeout=10) +await init_client(options={"skillStore": store}) + +# Materialize now, and re-materialize whenever delivery changes. +report, watcher = await watch_skills("*", ".claude/skills") +try: + ... +finally: + watcher.close() + store.close() +``` + +**Nothing above the store changes.** The accessors, integrity verification, and +`write_skills` are transport-agnostic: they see raw objects through the `SkillStore` seam and +cannot tell which store produced them. Everything documented above about verification and +reconcile semantics applies unchanged. + +**Server-side only.** Skills are for server-side agent runtimes and skill content is +customer-confidential. A mobile key (`mob-…`) or a client-side environment ID raises from the +constructor. + +**Streaming is the default, and it is what makes revocation fast.** A `delete-object` reaches +a live stream in seconds; with `mode="poll"` it arrives within one `poll_interval`. Paired +with `watch_skills`, a revoked skill's `SKILL.md` leaves the disk without a restart. During an +outage the store keeps serving the last content it received and `write_skills`' default +`on_unavailable="keep"` leaves managed files alone — an outage must not read as "everything +was revoked". + +**The connection also carries your flags.** A client cannot request only the skill payload, +so a skills-enabled environment delivers flag and segment objects on the same connection. +They are skipped, not evaluated — this store does no evaluation of any kind — and +`diagnostics.objects_ignored` counts them. + +> **Beta caveats, worth knowing before you deploy.** Payload signing does not exist on this +> channel yet, so delivery is TLS-only and the content hash establishes self-consistency, not +> origin authenticity. FDv2 is opt-in per account: without it the endpoints return HTTP 403, +> which the store reports as a fatal error naming the setting. `ld-relay` does not speak the +> FDv2 endpoints, so relay-only deployments cannot receive skills. + +**If every skill comes back empty, check `diagnostics.hashless_objects`.** Verification +requires `contentHash` on the delivered object and withholds anything without one, so a +nonzero count there means skills are being withheld rather than that the environment has +none. The store logs an error per hashless object and one summary per wholly-hashless +payload, both naming the reason. There is deliberately no fallback that skips verification: a +hash the SDK computed from the content it was handed would certify the content against +itself. + **Total path length is yours to bound, not the SDK's.** The 255-byte bound above is per *component*; the root is your path, so `` + `` + `/SKILL.md` can still exceed Windows' 260-character `MAX_PATH` with a perfectly legal key. Choose a short managed root on @@ -460,6 +518,9 @@ Windows. | `write_skills(skills, root, *, prune=True, timeout=10.0, on_unavailable="keep")` | Materialize skills under `root`, returning a `ReconcileReport`. `prune` removes formerly-managed skills no longer requested. `on_unavailable="raise"` raises instead of reporting when content cannot be retrieved. Raises `ValueError` for an unusable root, a negative `timeout`, or an unrecognised `on_unavailable`. **Performs synchronous filesystem I/O — see the note below.** | | `SkillStore` | The structural interface content arrives through: `get_object(kind, key, version=None)`, `all_objects(kind)`, optional `add_listener(kind, fn)`. | | `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=…, mode="stream", …)` | The delivery transport: a store fed by LaunchDarkly over the SDK-facing FDv2 channel. `start()`, `wait_for_skills(timeout)`, `close()`, `diagnostics`, `failed`; also a context manager. **Server-side only** — a mobile key or client-side environment ID raises. See *Receiving skills from LaunchDarkly* above. | +| `watch_skills(skills, root, …)` | `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. | +| `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `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 2558f9c..73e81d5 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -30,6 +30,8 @@ No other `launchdarkly-ai-*` package may define or duplicate these. They import | `src/launchdarkly_ai_server/types_validation.py` | `parse_ai_config` — validates flag variation shape; `is_valid_skill_key` / `is_valid_skill_version` / `skill_key_rejection_reason` (the canonical key-grammar explanation every layer quotes) | | `src/launchdarkly_ai_server/skills.py` | Agent Skills, retrieval half — `skill_refs`, `get_skill`/`get_skills`/`all_skills`, `InMemorySkillStore`, and the store/telemetry injection points `_set_store` / `_set_emitter_for_testing` | | `src/launchdarkly_ai_server/skills_core.py` | Shared skills internals — the `SkillStore` seam, module state, the telemetry seam and its three recorders, integrity verification, and store resolution. Imported by both `skills.py` and the materialization layer; imports neither | +| `src/launchdarkly_ai_server/skills_fdv2.py` | Agent Skills, delivery transport — the FDv2 protocol, the `objectVersion`/`version` translation, the held object set, and `FDv2SkillStore`. Sits **below** the store seam; imports `skills_core` only, and nothing imports it | +| `src/launchdarkly_ai_server/skills_watch.py` | Agent Skills, eager re-reconcile — `watch_skills` / `SkillWatcher`, wiring the store's change listener to `write_skills`. Sits **above** `skills_fs` and modifies none of it | | `src/launchdarkly_ai_server/skills_fs.py` | Agent Skills, materialization half — `write_skills`, request resolution, the manifest format and on-disk filenames, per-skill reconcile, and pruning | | `src/launchdarkly_ai_server/safe_fs.py` | Descriptor-pinned filesystem primitives — `atomic_write`, `unlink_file`, `pinned_directory`, `open_directory_nofollow`, `open_or_create_directory`, `SymlinkRefused`, and the `*at()` capability probe. Owns the descriptor-vs-path platform split; knows nothing about skills | | `src/launchdarkly_ai_server/utils.py` | `parse_template`, `parse_json_with_possible_fences`, `create_handler`, `parse_usage`, `make_track_data`, `to_ld_context` | @@ -217,6 +219,60 @@ one place that collapses the result to one object per key, because both whole-st consumers need it — `all_skills`, since a list holding two versions of one key is not a set of skills, and the `"*"` reconcile, since `//SKILL.md` is a single path. +### 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, +`If-None-Match`/304). It lives below the seam and produces raw objects in the shape +`skills_core.SkillStore` documents; **nothing above the seam knows it exists**. The transport +design was replaced wholesale late in this feature's life and cost zero changes above this +line, which is the strongest evidence the seam is drawn correctly. If a transport change ever +seems to require editing an accessor, verification, or `write_skills`, the adapter boundary is +wrong. + +**`objectVersion` is the skill's version. `version` is the payload's.** On the wire a skill +`put-object` carries both, and they are not interchangeable: + +```json +{"key":"pdf-extraction","kind":"inline-resource","category":"skill", + "objectVersion":3,"version":42, + "object":{"contentType":"text/markdown","content":"…","contentHash":"…","name":"…"}} +``` + +`objectVersion` (3) is what a `{key, version}` reference pins and what becomes the seam's +`version`. `version` (42) is the version of the *payload* the object arrived in — it moves +when anything in the environment moves, 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. Flags and segments +carry only `version` and omit both `category` and `objectVersion`, which is exactly why the +two fields look interchangeable. `seam_object_from_put` is the only place the translation +happens, and `TestVersionTranslation` asserts it in both directions. + +**Skills are identified by `kind == "inline-resource" && category == "skill"`; everything else +is ignored, not rejected.** An environment's payload assignment carries the flagging payload +alongside the agent-skill payload, so flag and segment objects arrive as a matter of course. +Erroring on an unrecognised kind is the unknown-kind reconnect loop this feature must not +reproduce — a flag-delivery outage caused by a skills rollout. + +**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 +described, and would briefly empty the store — which, with pruning on, is the difference +between a reconcile and deleting a customer's skill files. An interrupted transfer therefore +leaves last known good intact, and listeners fire once per commit. + +**A hashless object is held, not dropped.** Verification withholds it with +`missing_content_hash`; the transport's job is to make that loud (an error per object, a +summary per wholly-hashless payload, `diagnostics.hashless_objects`) rather than to work +around it. Dropping it at the transport would report `absent` — indistinguishable from "no +such skill" — and would let a prune delete the last known-good copy on disk. Never synthesize +a hash from the delivered content: that certifies the content against itself and verifies +nothing. + +**`close` interrupts the socket, it does not just set a flag.** The delivery thread spends its +life blocked in a read that no flag can reach, and closing a response from another thread does +not unblock CPython's buffered reader. `_interrupt_read` shuts the socket down underneath it. +Without that, every shutdown of a *healthy* stream blocks for the full join timeout. + ### The reported outcome vocabulary, and the `Resolution` mapping `get_skill` returns `Skill | None`; `get_skill_result` returns a frozen `SkillOutcome` diff --git a/packages/client/src/launchdarkly_ai_server/__init__.py b/packages/client/src/launchdarkly_ai_server/__init__.py index 753e1e9..0f98711 100644 --- a/packages/client/src/launchdarkly_ai_server/__init__.py +++ b/packages/client/src/launchdarkly_ai_server/__init__.py @@ -46,6 +46,7 @@ skill_refs, ) from .skills_core import SkillStore +from .skills_fdv2 import FDv2SkillStore, StoreDiagnostics from .skills_fs import ( MANIFEST_FILENAME, MANIFEST_VERSION, @@ -53,6 +54,7 @@ OnUnavailable, write_skills, ) +from .skills_watch import SkillWatcher, watch_skills from .tracking import execute_and_stream, execute_and_track, wrap_tool_handlers from .types import ( NATIVE_TOOL_KEY, @@ -237,6 +239,11 @@ "write_skills", "SkillStore", "InMemorySkillStore", + # skills — the FDv2 delivery transport, and the eager re-reconcile it enables + "FDv2SkillStore", + "StoreDiagnostics", + "watch_skills", + "SkillWatcher", # skills — the three closed-set unions a typed consumer needs to name "ReconcileActionKind", "OnUnavailable", diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py new file mode 100644 index 0000000..e579ae2 --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -0,0 +1,1381 @@ +""" +Agent Skills — the FDv2 delivery transport. + +The store implementation that actually talks to LaunchDarkly. It sits *below* +the ``SkillStore`` seam, not above it: it produces raw wire objects in the shape +``skills_core`` documents, and everything above — the accessors, integrity +verification, the ``Skill`` dataclass, materialization — is unchanged and +unaware of it. That is the whole point of the seam, and the fact that replacing +the transport design wholesale cost nothing above this line is the evidence it +was drawn in the right place. + +Layering:: + + launchdarkly_ai_server + └─ SkillStore protocol (skills_core) ── duck-typed accessor surface + └─ FDv2SkillStore (this module) ── deserialize, hold, serve + └─ the SDK-facing FDv2 channel on FDCore + GET /sdk/poll, GET /sdk/stream, authenticated with the + environment's server-side SDK key + +Dependencies run one way. This module imports ``skills_core`` for the seam's +kind constant and nothing else from the feature; ``skills.py`` and +``skills_fs.py`` do not import it. It uses only the standard library, so the +content path adds no dependency to a package whose sole runtime dependency is +``opentelemetry-api`` and whose LaunchDarkly base-SDK dependency is optional. + +**There is no bespoke private route here, deliberately.** An earlier design had +this adapter poll ``/private/flagdlv/payloads/{id}/latest/obj/skill/{key}``. +Those are gonfalon private endpoints authenticated by Cognito machine-token +OAuth scopes with no per-tenant authorization; the security review ruled out +both relaxing that auth and shipping a machine credential to a customer host. +This transport uses the genuinely SDK-facing channel instead, which is also the +channel payload signing will eventually cover. Do not reintroduce the private +route. + +What this module does *not* do, on purpose: + +- **It does not verify content.** Verification lives at the accessor boundary in + ``skills_core`` so that it applies to every store equally, including + ``InMemorySkillStore`` and a customer's own. A transport that verified would + make integrity depend on which store you configured. +- **It does not skip verification when the wire envelope has no + ``contentHash``.** See ``_SkillObjectSet.put`` and ``StoreDiagnostics``: a + hashless object is stored verbatim and *withheld* by verification with + ``missing_content_hash``, and this module's job is to make that outcome loud + rather than to paper over it. +- **It does not evaluate anything.** No flags, no segments, no targeting. Skills + have no targeting; the SDK key fully determines the payload. +""" + +from __future__ import annotations + +import json +import logging +import random +import re +import socket +import threading +import urllib.error +import urllib.parse +import urllib.request +from collections.abc import Callable +from dataclasses import dataclass, field +from typing import Any, Literal + +from .skills_core import SKILL_OBJECT_KIND +from .types_validation import is_valid_skill_version + +logger = logging.getLogger(__name__) + + +# --------------------------------------------------------------------------- +# The wire contract +# --------------------------------------------------------------------------- + +FDV2_OBJECT_KIND = "inline-resource" +""" +The FDv2 ``kind`` skills are delivered under. + +Distinct from ``skills_core.SKILL_OBJECT_KIND`` (``"skill"``), which is the +*seam* value the SDK asks a store for. Translating this pair — kind +``inline-resource`` plus category ``skill`` — onto that single value is exactly +the adapter's job, and the reason ``SKILL_OBJECT_KIND`` is documented as a seam +string rather than as the wire contract. +""" + +FDV2_OBJECT_CATEGORY = "skill" +"""The ``category`` that narrows ``inline-resource`` to an agent skill.""" + +DEFAULT_BASE_URI = "https://sdk.launchdarkly.com" +"""Where the SDK-facing FDv2 endpoints live. Overridable for Federal instances, +private instances, and the fake endpoint the tests run against.""" + +POLL_PATH = "/sdk/poll" +STREAM_PATH = "/sdk/stream" + +SDK_DATA_MODEL_VERSION = 1 +""" +The ``mv`` request parameter — the SDK data model version this adapter speaks. + +Overridable through ``FDv2SkillStore(data_model_version=...)`` because it is the +one request parameter this side cannot verify: the LaunchDarkly base SDK's own +FDv2 data source does not send ``mv`` at all today, and the streamer branch that +carries skills is unmerged, so the value the server expects has not been +observed. Confirm it with FDN before Beta rather than trusting this default. +""" + +_EVENT_SERVER_INTENT = "server-intent" +_EVENT_PUT_OBJECT = "put-object" +_EVENT_DELETE_OBJECT = "delete-object" +_EVENT_PAYLOAD_TRANSFERRED = "payload-transferred" +_EVENT_HEARTBEAT = "heart-beat" +_EVENT_GOODBYE = "goodbye" +_EVENT_ERROR = "error" + +_INTENT_TRANSFER_FULL = "xfer-full" +_INTENT_TRANSFER_CHANGES = "xfer-changes" +_INTENT_TRANSFER_NONE = "none" + +_ENVELOPE_FIELDS = ("contentType", "content", "contentHash", "name", "description") +""" +The skill object envelope's fields, copied through verbatim. + +``contentHash`` is listed here and is the field the whole content path waits on; +see ``StoreDiagnostics``. Nothing here is coerced, defaulted, or normalised — +everything a store serves is untrusted input and is revalidated above the seam, +so a transport that "helpfully" filled in a field would be forging the very +thing verification exists to check. +""" + +Mode = Literal["stream", "poll"] + +_MOBILE_KEY_PREFIX = "mob-" +_SERVER_KEY_PREFIX = "sdk-" +_CLIENT_SIDE_ID = re.compile(r"\A[0-9a-f]{20,}\Z") +""" +A client-side environment ID: bare lowercase hex, no prefix. Server-side keys +and mobile keys both carry a prefix, so "hex with no prefix" is an +unambiguous client-side credential rather than a heuristic. +""" + + +# --------------------------------------------------------------------------- +# Server-side only +# --------------------------------------------------------------------------- + + +def _require_server_side_credential(sdk_key: str) -> None: + """ + Refuses a mobile key or a client-side environment ID. + + Skills are for server-side agent runtimes. The payload assignment that + carries them is shared by every auth type, so the skill payload ID is + appended for mobile and environment-ID auth too — which means a client-side + credential may well *succeed* against these endpoints and deliver + customer-confidential skill content to a client-side process. Failing here + is the SDK-side half of that boundary; excluding skills at assignment time + is the platform-side half, and is an open ask on FDN (design §3.1c). + + Raises ``ValueError`` rather than logging, because there is no degraded mode + that is correct: a store built on the wrong credential should not exist. + """ + if not isinstance(sdk_key, str) or not sdk_key.strip(): + raise ValueError( + "FDv2SkillStore requires a LaunchDarkly server-side SDK key " + "(sdk-...); none was given." + ) + key = sdk_key.strip() + if key.startswith(_MOBILE_KEY_PREFIX): + raise ValueError( + "FDv2SkillStore was given a mobile key (mob-...). Agent Skills are a " + "server-side feature: skill content is customer-confidential and is " + "never delivered to a mobile or client-side process. Use the " + "environment's server-side SDK key (sdk-...)." + ) + if _CLIENT_SIDE_ID.match(key): + raise ValueError( + "FDv2SkillStore was given what looks like a client-side environment " + "ID. Agent Skills are a server-side feature: skill content is " + "customer-confidential and is never delivered to a client-side " + "process. Use the environment's server-side SDK key (sdk-...)." + ) + if not key.startswith(_SERVER_KEY_PREFIX): + # Not rejected: private instances and test doubles issue credentials that + # do not carry the public prefix, and refusing them would break a + # deployment that is perfectly correct. The two shapes above are refused + # because they are unambiguously *not* server-side. + logger.warning( + "The credential given to FDv2SkillStore does not look like a " + "LaunchDarkly server-side SDK key (sdk-...). Skills are delivered " + "only to server-side credentials; if this is a client-side or mobile " + "credential the connection will be rejected or will deliver nothing." + ) + + +# --------------------------------------------------------------------------- +# Diagnostics — and the contentHash gap in particular +# --------------------------------------------------------------------------- + + +@dataclass +class StoreDiagnostics: + """ + What the transport has seen. Read-only from a caller's perspective. + + Not part of the ``SkillStore`` seam — nothing above the seam reads this — but + the difference between "this environment has no skills" and "every skill was + withheld" is the single most confusing failure this feature can produce, and + a counter a caller can assert on beats reading logs. + """ + + payloads_transferred: int = 0 + """Completed ``payload-transferred`` commits since the store started.""" + skill_objects_received: int = 0 + """``put-object`` events identified as skills, across all payloads.""" + objects_ignored: int = 0 + """Objects skipped because they were not skills — flags, segments, and any + future kind. Skipping is the contract, not a failure; the count exists so a + mixed payload is visibly mixed.""" + objects_revoked: int = 0 + """``delete-object`` events applied to skills.""" + hashless_objects: int = 0 + """ + Skill objects whose envelope carried no ``contentHash``. + + **Nonzero means skills are being withheld.** Verification withholds a + hashless object with ``missing_content_hash``, so every one of these is a + skill that will never resolve. The field exists so that outcome is a number + a caller can read rather than an empty store they have to explain. + """ + connection_failures: int = 0 + """Recoverable transport failures since the last successful transfer.""" + last_error: str | None = None + """The most recent transport error, if any. Human-readable; do not parse.""" + + +# --------------------------------------------------------------------------- +# Deserialization — where objectVersion is not version +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class _Tombstone: + """A ``delete-object`` narrowed to the identity it revokes.""" + + key: str + object_version: int | None + + +def is_skill_event(data: Any) -> bool: + """ + Whether one ``put-object`` / ``delete-object`` payload is a skill. + + ``kind == "inline-resource" and category == "skill"``, and nothing else. Both + halves are required: ``inline-resource`` is a broad kind that may carry other + categories, and flags and segments omit ``category`` entirely. + + Every other kind is **ignored, not rejected**. An environment's payload + assignment carries the flagging payload alongside the agent-skill payload, so + a connection delivers flag and segment objects as a matter of course. Erroring + on them would turn a normal payload into a permanent failure — which is + exactly the unknown-kind reconnect loop this feature must not reproduce. + """ + if not isinstance(data, dict): + return False + return ( + data.get("kind") == FDV2_OBJECT_KIND + and data.get("category") == FDV2_OBJECT_CATEGORY + ) + + +def seam_object_from_put(data: dict[str, Any]) -> dict[str, Any] | None: + """ + Translates one FDv2 skill ``put-object`` into a seam-shaped raw object. + + ``None`` when the event cannot be filed at all — only when ``key`` is not a + string, since a keyless object has no identity to store it under and no key + to attribute a failure to. Every other defect is carried through verbatim so + that *verification* withholds it, with a reason code and an integrity signal, + rather than the transport dropping it silently. A silent drop is + indistinguishable from "no such skill" and would additionally let a prune + delete the last known-good copy on disk. + + **The translation this whole module exists to get right:** + + wire ``objectVersion`` → seam ``version`` (the skill's own version) + wire ``version`` → dropped (the *payload* version) + + ``objectVersion`` is what a ``{key, version}`` reference pins. ``version`` is + the version of the payload the object arrived in — it changes when anything + in the environment changes, including a flag that has nothing to do with + skills. Reading it as the skill's version resolves the wrong content with no + error anywhere: the object verifies, the hash matches, and the caller is + handed a skill under a version number that means nothing. Flags and segments + carry only ``version``, which is why the two fields look interchangeable and + are not. + """ + key = data.get("key") + if not isinstance(key, str) or not key: + logger.warning( + "An FDv2 skill put-object carried no string 'key' and could not be " + "stored under any identity; it was dropped." + ) + return None + + raw: dict[str, Any] = {"key": key} + + # The single translation. Written as a membership test rather than a `.get` + # default so an explicitly-null objectVersion stays null and reaches + # verification as `invalid_version`, instead of being invented here. + if "objectVersion" in data: + raw["version"] = data["objectVersion"] + + envelope = data.get("object") + if isinstance(envelope, dict): + for wire_field in _ENVELOPE_FIELDS: + if wire_field in envelope: + raw[wire_field] = envelope[wire_field] + return raw + + +def tombstone_from_delete(data: dict[str, Any]) -> _Tombstone | None: + """ + Narrows one FDv2 skill ``delete-object`` to the identity it revokes. + + A delete for an inline resource **is revocation** — the object leaves the + payload, this store drops it, the accessors stop resolving it, and the next + reconcile prunes its files. Same ``objectVersion`` translation as a put. + + ``object_version`` of ``None`` means the delete named no usable version, and + is read as "revoke every version of this key". That is the safe direction: + the alternative is ignoring an unparseable revocation and continuing to serve + content LaunchDarkly has withdrawn. + """ + key = data.get("key") + if not isinstance(key, str) or not key: + logger.warning( + "An FDv2 skill delete-object carried no string 'key'; it was ignored." + ) + return None + object_version = data.get("objectVersion") + return _Tombstone( + key=key, + object_version=object_version + if is_valid_skill_version(object_version) + else None, + ) + + +# --------------------------------------------------------------------------- +# The held object set +# --------------------------------------------------------------------------- + + +class _SkillObjectSet: + """ + Raw skill objects held in memory, keyed by ``(key, objectVersion)``. + + Lookup semantics are deliberately identical to ``InMemorySkillStore``'s, down + to the fall-through to a version-less entry, so that the store a caller + configures cannot change how a pinned reference resolves. They are + reimplemented here rather than inherited because the transport needs two + operations a hand-populated store does not have — ``delete`` and the atomic + ``replace`` a full transfer requires — and reaching into another store's + privates to get them would couple the two far harder than a test that asserts + they agree. ``test_skills_fdv2.py`` carries that parity test. + + Several versions of one key coexist, because they coexist in a real payload: + the newest version of every skill plus every version a variation currently + pins. An object too malformed to carry a usable version is still held, under + its key alone, so verification withholds it with a signal rather than the + transport dropping it into indistinguishable absence. + """ + + def __init__(self) -> None: + self._versions: dict[str, dict[int, dict[str, Any]]] = {} + self._loose: dict[str, dict[str, Any]] = {} + + def put(self, raw: dict[str, Any]) -> None: + key = raw["key"] + version = raw.get("version") + if is_valid_skill_version(version): + self._versions.setdefault(key, {})[version] = raw + else: + self._loose[key] = raw + + def delete(self, tombstone: _Tombstone) -> list[dict[str, Any]]: + """ + Removes what *tombstone* revokes; returns the raw objects that went away. + + A tombstone with no usable version removes every version of the key — see + ``tombstone_from_delete`` for why that is the safe reading. + """ + removed: list[dict[str, Any]] = [] + if tombstone.object_version is None: + held = self._versions.pop(tombstone.key, {}) + removed.extend(held.values()) + loose = self._loose.pop(tombstone.key, None) + if loose is not None: + removed.append(loose) + return removed + + held = self._versions.get(tombstone.key, {}) + gone = held.pop(tombstone.object_version, None) + if gone is not None: + removed.append(gone) + if not held: + self._versions.pop(tombstone.key, None) + return removed + + def get(self, key: str, version: int | None) -> dict[str, Any] | None: + held = self._versions.get(key, {}) + if version is not None: + # Fall through to the version-less entry when the pin matches nothing + # well-formed, so a malformed object reaches verification and is + # withheld with a signal rather than reading as simply absent. + return held.get(version) or self._loose.get(key) + if held: + return held[max(held)] + return self._loose.get(key) + + def snapshot(self) -> dict[str, dict[str, Any]]: + """One entry per ``(key, version)``, under keys opaque to the SDK.""" + out: dict[str, dict[str, Any]] = { + f"{key}:{version}": raw + for key, versions in self._versions.items() + for version, raw in versions.items() + } + out.update(self._loose) + return out + + def all_raw(self) -> list[dict[str, Any]]: + return list(self.snapshot().values()) + + def replace_with(self, other: _SkillObjectSet) -> None: + """Adopts *other*'s contents wholesale — how a full transfer commits.""" + self._versions = other._versions + self._loose = other._loose + + def copy(self) -> _SkillObjectSet: + clone = _SkillObjectSet() + clone._versions = {key: dict(v) for key, v in self._versions.items()} + clone._loose = dict(self._loose) + return clone + + def __len__(self) -> int: + return sum(len(v) for v in self._versions.values()) + len(self._loose) + + +# --------------------------------------------------------------------------- +# The protocol state machine — pure, no I/O +# --------------------------------------------------------------------------- + + +@dataclass +class _Change: + """One committed change, as handed to a listener.""" + + raw: dict[str, Any] + + +@dataclass +class _TransferOutcome: + """What one event did. Aggregated by the caller; nothing here does I/O.""" + + committed: bool = False + changes: list[dict[str, Any]] = field(default_factory=list) + basis: str | None = None + fatal: str | None = None + disconnect: str | None = None + + +class _ProtocolReader: + """ + Applies FDv2 events to an object set. Pure — no sockets, no threads, no clock. + + Split out so the protocol is testable without a server: every wire case in + ``test_skills_fdv2.py`` drives this directly, and the HTTP layer above it only + has to turn bytes into ``(event name, data)`` pairs. + + **Changes are buffered and committed at ``payload-transferred``**, matching + how the base SDK's FDv2 data source applies a change set. A payload version + is the unit of consistency: applying half of one would publish a state the + server never described, and on a full transfer it would briefly empty the + store — which, with pruning on, is the difference between a reconcile and + deleting a customer's skill files. Listeners therefore fire once per commit, + not once per object, which is also exactly the granularity the re-reconcile + wants. + """ + + def __init__(self, committed: _SkillObjectSet) -> None: + self._committed = committed + self._intent: str | None = None + self._pending: _SkillObjectSet | None = None + self._changes: list[dict[str, Any]] = [] + self.diagnostics = StoreDiagnostics() + + # -- events ------------------------------------------------------------ + + def handle(self, name: str, data: Any) -> _TransferOutcome: + """Routes one event. Unknown event names are ignored, by contract.""" + if name == _EVENT_SERVER_INTENT: + return self._server_intent(data) + if name == _EVENT_PUT_OBJECT: + return self._put_object(data) + if name == _EVENT_DELETE_OBJECT: + return self._delete_object(data) + if name == _EVENT_PAYLOAD_TRANSFERRED: + return self._payload_transferred(data) + if name == _EVENT_ERROR: + return self._error(data) + if name == _EVENT_GOODBYE: + return self._goodbye(data) + if name == _EVENT_HEARTBEAT: + return _TransferOutcome() + logger.debug("Ignoring unknown FDv2 event '%s'", name) + return _TransferOutcome() + + def _server_intent(self, data: Any) -> _TransferOutcome: + payloads = data.get("payloads") if isinstance(data, dict) else None + if not isinstance(payloads, list) or not payloads: + return _TransferOutcome( + disconnect="server-intent carried no payload description" + ) + first = payloads[0] + intent = first.get("intentCode") if isinstance(first, dict) else None + self._intent = intent + self._changes = [] + if intent == _INTENT_TRANSFER_FULL: + # A fresh set: the payload about to arrive replaces everything held. + # Built alongside the live set rather than in place, so an interrupted + # transfer leaves last-known-good intact. + self._pending = _SkillObjectSet() + elif intent == _INTENT_TRANSFER_CHANGES: + self._pending = self._committed.copy() + elif intent == _INTENT_TRANSFER_NONE: + self._pending = None + else: + logger.debug("Ignoring FDv2 server-intent with intentCode %r", intent) + self._pending = None + return _TransferOutcome() + + def _target(self) -> _SkillObjectSet | None: + if self._pending is None and self._intent in ( + _INTENT_TRANSFER_FULL, + _INTENT_TRANSFER_CHANGES, + ): + # An object arrived before any server-intent. Treat it as a delta + # against what we hold rather than dropping it. + self._pending = self._committed.copy() + return self._pending + + def _put_object(self, data: Any) -> _TransferOutcome: + if not is_skill_event(data): + self.diagnostics.objects_ignored += 1 + return _TransferOutcome() + if self._pending is None and self._intent is None: + self._intent = _INTENT_TRANSFER_CHANGES + target = self._target() + if target is None: + return _TransferOutcome() + + raw = seam_object_from_put(data) + if raw is None: + return _TransferOutcome() + target.put(raw) + self._changes.append(raw) + self.diagnostics.skill_objects_received += 1 + if not isinstance(raw.get("contentHash"), str): + self.diagnostics.hashless_objects += 1 + _warn_hashless(raw) + return _TransferOutcome() + + def _delete_object(self, data: Any) -> _TransferOutcome: + if not is_skill_event(data): + self.diagnostics.objects_ignored += 1 + return _TransferOutcome() + if self._pending is None and self._intent is None: + self._intent = _INTENT_TRANSFER_CHANGES + target = self._target() + if target is None: + return _TransferOutcome() + + tombstone = tombstone_from_delete(data) + if tombstone is None: + return _TransferOutcome() + target.delete(tombstone) + self.diagnostics.objects_revoked += 1 + # A tombstone, not a skill object: it carries identity and no content, so + # a listener that only needs "something changed" works unchanged while one + # that reads content sees no `content` key. Documented on + # ``FDv2SkillStore.add_listener``. + self._changes.append( + {"key": tombstone.key, "version": tombstone.object_version} + ) + return _TransferOutcome() + + def _payload_transferred(self, data: Any) -> _TransferOutcome: + state = data.get("state") if isinstance(data, dict) else None + version = data.get("version") if isinstance(data, dict) else None + if self._pending is not None: + hashless_before = self.diagnostics.hashless_objects + self._committed.replace_with(self._pending) + _warn_if_nothing_can_verify(self._committed, hashless_before) + self._pending = None + self._intent = None + changes = self._changes + self._changes = [] + self.diagnostics.payloads_transferred += 1 + logger.debug( + "FDv2 payload transferred: payload version %s, %d skill object(s) held", + version, + len(self._committed), + ) + return _TransferOutcome( + committed=True, + changes=changes, + basis=state if isinstance(state, str) and state else None, + ) + + def _error(self, data: Any) -> _TransferOutcome: + reason = data.get("reason") if isinstance(data, dict) else None + # An error abandons the in-flight payload and keeps what is committed. + self._pending = None + self._intent = None + self._changes = [] + return _TransferOutcome(disconnect=f"server sent error: {reason}") + + def _goodbye(self, data: Any) -> _TransferOutcome: + reason = data.get("reason") if isinstance(data, dict) else None + catastrophe = bool(data.get("catastrophe")) if isinstance(data, dict) else False + silent = bool(data.get("silent")) if isinstance(data, dict) else False + self._pending = None + self._intent = None + self._changes = [] + if not silent: + logger.info("FDv2 connection closing: %s", reason) + if catastrophe: + return _TransferOutcome( + fatal=f"server sent a catastrophic goodbye: {reason}" + ) + return _TransferOutcome(disconnect=f"server said goodbye: {reason}") + + +_HASHLESS_ADVICE = ( + "The delivered skill object carries no 'contentHash', so integrity " + "verification withholds it with reason_code 'missing_content_hash' and its " + "content will never resolve. This is not a fault in this store and not " + "something the SDK can work around: verification hashes the verbatim bytes " + "and compares, and there is nothing to compare against. The field is " + "specified as an additive sha256-over-verbatim-UTF-8 value on the skill " + "envelope (LaunchDarkly AIC-2905) and has not shipped yet. Until it does, " + "expect an empty result from every skill accessor." +) + +_warned_hashless: set[tuple[str, Any]] = set() +_warned_lock = threading.Lock() + + +def _warn_hashless(raw: dict[str, Any]) -> None: + """ + One ERROR per ``(key, version)`` whose envelope had no ``contentHash``. + + At ERROR rather than WARN, and per object rather than once per process, + because this is the difference between a broken deployment and an + empty-by-design one — the exact confusion the blocking gap produces. Deduped + so a re-delivered payload does not multiply it; a store that is restarted + reports again. + """ + identity = (raw["key"], raw.get("version")) + with _warned_lock: + if identity in _warned_hashless: + return + _warned_hashless.add(identity) + logger.error( + "Skill '%s' version %s arrived without a contentHash and will be withheld. %s", + raw["key"], + raw.get("version"), + _HASHLESS_ADVICE, + extra={"ld_skill_key": raw["key"], "ld_skill_version": raw.get("version")}, + ) + + +def _warn_if_nothing_can_verify( + committed: _SkillObjectSet, hashless_before_this_payload: int +) -> None: + """ + One ERROR per committed payload in which *nothing* the store now holds can + possibly verify. + + ``log_withholding_summary`` already reports a wholly-withheld batch at the + accessor boundary, but only once a caller asks. This fires at delivery time, + so the condition is visible in a process that boots, materializes nothing, + and exits — which is the shape a skills deployment fails in. + """ + del hashless_before_this_payload # counted for the store, not for this check + held = committed.all_raw() + if not held: + return + hashless = [raw for raw in held if not isinstance(raw.get("contentHash"), str)] + if len(hashless) != len(held): + return + logger.error( + "All %d skill object(s) in the delivered payload arrived without a " + "contentHash. No skill content will resolve from this store. %s", + len(held), + _HASHLESS_ADVICE, + ) + + +# --------------------------------------------------------------------------- +# HTTP +# --------------------------------------------------------------------------- + + +class _FatalTransportError(Exception): + """A failure retrying cannot fix: bad credential, forbidden, wrong URI.""" + + +class _RecoverableTransportError(Exception): + """A failure worth retrying. Carries a server-requested delay when given one.""" + + def __init__(self, message: str, retry_after: float | None = None) -> None: + super().__init__(message) + self.retry_after = retry_after + + +_FORBIDDEN_ADVICE = ( + "FDv2 is opt-in per account: the 'fdv2-protocol-control' setting defaults to " + "'forbid', which is served as HTTP 403. Skill delivery over this channel " + "needs that flag flipped for the account, and needs the FDCore/streamer " + "inline-resource support merged and deployed." +) + + +def _retry_after_seconds(headers: Any) -> float | None: + """``Retry-After`` in seconds, when the server sent a usable one.""" + if headers is None: + return None + try: + raw = headers.get("Retry-After") + except AttributeError: + return None + if raw is None: + return None + try: + seconds: float = float(str(raw).strip()) + except ValueError: + # The HTTP-date form is legal and rare; falling back to our own backoff + # is better than parsing a date to honour it approximately. + return None + return max(0.0, seconds) + + +def _classify_status(status: int, headers: Any) -> Exception: + """Turns an HTTP error status into the right exception type.""" + if status == 401: + return _FatalTransportError( + "LaunchDarkly rejected the SDK key (HTTP 401). Skill delivery cannot " + "start. Check that the key is the environment's server-side SDK key." + ) + if status == 403: + return _FatalTransportError( + f"LaunchDarkly returned HTTP 403. {_FORBIDDEN_ADVICE}" + ) + if status == 404: + return _FatalTransportError( + "LaunchDarkly returned HTTP 404 for the FDv2 endpoint. Check the base " + "URI, and that this instance serves /sdk/poll and /sdk/stream." + ) + if status in (400, 405, 406, 414, 501): + return _FatalTransportError( + f"LaunchDarkly returned HTTP {status}, which retrying will not fix. " + "The request this adapter sent was not understood; the 'mv' data " + f"model version ({SDK_DATA_MODEL_VERSION}) is the parameter most " + "likely to be wrong." + ) + return _RecoverableTransportError( + f"LaunchDarkly returned HTTP {status}", _retry_after_seconds(headers) + ) + + +def _interrupt_read(response: Any) -> None: + """ + Best-effort interruption of a read blocked on *response*, from another thread. + + Closing the response is not enough: CPython's buffered reader stays parked in + ``readline`` until bytes arrive, so a ``close`` from another thread does not + unblock it. Shutting the *socket* down underneath it does, immediately. + + Reaching the socket means walking urllib's private attribute chain, so every + step is guarded and a failure here is silent by design. It is an + optimisation, not a correctness requirement: the delivery thread is a daemon + and ``close``'s join timeout is the backstop, so the worst case of this not + finding a socket is a shutdown that takes as long as the join allows. + """ + for path in (("fp", "raw", "_sock"), ("fp", "_sock"), ("_sock",)): + found: Any = response + for name in path: + found = getattr(found, name, None) + if found is None: + break + if found is not None and hasattr(found, "shutdown"): + try: + found.shutdown(socket.SHUT_RDWR) + except OSError: + pass + return + + +class _StreamConnection: + """ + One open streaming connection: an event iterator plus a way to interrupt it. + + Exists because ``close`` runs on a *different* thread from the read. The + delivery thread spends nearly all its life blocked in a socket read on a + long-lived stream, where a stop flag it cannot check is no use. Without an + interruption a store's ``close`` would block for its whole join timeout on + every shutdown of a *healthy* stream — a hang in the caller's shutdown path, + paid every time. + """ + + def __init__(self, response: Any) -> None: + self._response = response + self.events = _iter_sse(response) + + def close(self) -> None: + """Interrupts the read. Safe to call from any thread, and twice.""" + _interrupt_read(self._response) + try: + self._response.close() + except Exception: + pass + + +@dataclass(frozen=True) +class _PollResult: + not_modified: bool + events: list[tuple[str, Any]] + etag: str | None + + +class _Requester: + """ + The only place this module opens a socket. + + Standard library only, on purpose: this package's sole runtime dependency is + ``opentelemetry-api`` and its LaunchDarkly base-SDK dependency is optional, so + the content path must not smuggle in an HTTP client. + """ + + def __init__( + self, + sdk_key: str, + base_uri: str, + *, + connect_timeout: float, + read_timeout: float, + data_model_version: int, + opener: Any = None, + ) -> None: + self._sdk_key = sdk_key + self._base_uri = base_uri.rstrip("/") + self._read_timeout = read_timeout + self._connect_timeout = connect_timeout + self._data_model_version = data_model_version + # Injectable so the tests drive a fake endpoint without a socket; the + # default is urllib's global opener. + self._opener = opener or urllib.request.build_opener() + + def _url(self, path: str, basis: str | None) -> str: + params: dict[str, str] = {"mv": str(self._data_model_version)} + if basis: + params["basis"] = basis + return f"{self._base_uri}{path}?{urllib.parse.urlencode(params)}" + + def _request( + self, path: str, basis: str | None, headers: dict[str, str] + ) -> urllib.request.Request: + all_headers = {"Authorization": self._sdk_key, **headers} + return urllib.request.Request( + self._url(path, basis), headers=all_headers, method="GET" + ) + + def poll(self, basis: str | None, etag: str | None) -> _PollResult: + """ + One ``GET /sdk/poll``. Honours ``If-None-Match`` and returns 304 as a + first-class outcome rather than as an error. + """ + headers = {"Accept": "application/json"} + if etag: + headers["If-None-Match"] = etag + request = self._request(POLL_PATH, basis, headers) + try: + with self._opener.open(request, timeout=self._read_timeout) as response: + status = getattr(response, "status", None) or response.getcode() + if status == 304: + return _PollResult(not_modified=True, events=[], etag=etag) + body = response.read() + new_etag = response.headers.get("ETag") or etag + except urllib.error.HTTPError as exc: + if exc.code == 304: + # urllib raises on 304 when no redirect handler swallows it. + return _PollResult(not_modified=True, events=[], etag=etag) + raise _classify_status(exc.code, exc.headers) from exc + except _FatalTransportError: + raise + except Exception as exc: + raise _RecoverableTransportError( + f"polling request failed: {type(exc).__name__}: {exc}" + ) from exc + + return _PollResult( + not_modified=False, events=_decode_poll_body(body), etag=new_etag + ) + + def stream(self, basis: str | None) -> _StreamConnection: + """ + Opens ``GET /sdk/stream``. + + Returns a ``_StreamConnection`` rather than a bare generator so the + caller can interrupt a blocked read from another thread; see that class. + """ + request = self._request( + STREAM_PATH, + basis, + {"Accept": "text/event-stream", "Cache-Control": "no-cache"}, + ) + try: + response = self._opener.open(request, timeout=self._read_timeout) + except urllib.error.HTTPError as exc: + raise _classify_status(exc.code, exc.headers) from exc + except _FatalTransportError: + raise + except Exception as exc: + raise _RecoverableTransportError( + f"streaming request failed: {type(exc).__name__}: {exc}" + ) from exc + return _StreamConnection(response) + + +def _decode_poll_body(body: bytes) -> list[tuple[str, Any]]: + """ + Unwraps ``{"events": [...]}``. + + Polling and streaming carry the *identical* event objects — polling just wraps + them in an envelope — which is why the protocol state machine above is shared + and neither mode has its own copy of the semantics. + """ + try: + parsed = json.loads(body.decode("utf-8")) + except (UnicodeDecodeError, json.JSONDecodeError) as exc: + raise _RecoverableTransportError( + f"polling response was not valid JSON: {exc}" + ) from exc + if not isinstance(parsed, dict) or not isinstance(parsed.get("events"), list): + raise _RecoverableTransportError("polling response had no 'events' array") + events: list[tuple[str, Any]] = [] + for entry in parsed["events"]: + if not isinstance(entry, dict): + continue + name = entry.get("event") + if isinstance(name, str): + events.append((name, entry.get("data"))) + return events + + +def _iter_sse(response: Any) -> Any: + """ + Decodes an SSE body into ``(event name, data)`` pairs. + + Minimal on purpose — this consumes one LaunchDarkly endpoint, not the whole + spec: ``event:``/``data:`` fields, multi-line ``data`` joined with newlines, + a blank line dispatching, and ``:`` comments skipped. + """ + try: + name: str | None = None + data_lines: list[str] = [] + for raw_line in response: + line = raw_line.decode("utf-8", errors="replace").rstrip("\r\n") + if line == "": + if name is not None: + payload = "\n".join(data_lines) + try: + parsed = json.loads(payload) if payload else None + except json.JSONDecodeError: + logger.warning( + "Discarding FDv2 '%s' event whose data was not JSON", name + ) + parsed = None + else: + yield name, parsed + name = None + data_lines = [] + continue + if line.startswith(":"): + continue + field_name, _, value = line.partition(":") + value = value[1:] if value.startswith(" ") else value + if field_name == "event": + name = value + elif field_name == "data": + data_lines.append(value) + finally: + try: + response.close() + except Exception: + pass + + +# --------------------------------------------------------------------------- +# Backoff +# --------------------------------------------------------------------------- + + +def backoff_delay( + attempt: int, *, base: float, maximum: float, jitter: float = 0.5 +) -> float: + """ + Exponential backoff with decorrelating jitter, capped at *maximum*. + + Jitter is subtractive over the *whole* range rather than added on top, so the + cap is a real ceiling: a fleet of agent processes restarted together must not + reconnect in lockstep, and must not exceed the interval the cap promises. + """ + # float(2 ** n) rather than 2 ** n: the integer power is untyped to mypy, + # and the whole expression is a duration, not a count. + ceiling: float = min(maximum, base * float(2 ** max(0, attempt - 1))) + return ceiling * (1.0 - jitter * random.random()) + + +# --------------------------------------------------------------------------- +# The store +# --------------------------------------------------------------------------- + + +class FDv2SkillStore: + """ + A ``SkillStore`` fed by LaunchDarkly's SDK-facing FDv2 delivery channel. + + The transport half of Agent Skills. Constructed with the environment's + server-side SDK key, started explicitly, and passed to ``init_client``:: + + store = FDv2SkillStore(sdk_key=os.environ["LD_SDK_KEY"]) + store.start() + store.wait_for_skills(timeout=10) + await init_client(options={"skillStore": store}) + + skill = await get_skill("pdf-extraction") + ... + store.close() + + It also works as a context manager, which is the shape to prefer when the + process's lifetime is a block. + + **Server-side only.** Skills are for server-side agent runtimes and skill + content is customer-confidential. A mobile key or a client-side environment + ID is refused in the constructor — see ``_require_server_side_credential``. + + **Delivery is in the background; retrieval is not.** ``SkillStore`` is a + synchronous seam, so a daemon thread owns the connection and fills memory, + and ``get_object`` only ever reads what has already arrived. Nothing here + blocks a retrieval on the network. The corollary is that a process which + calls ``get_skill`` immediately after ``start()`` may see an empty store; + ``wait_for_skills`` is how you order boot against the first payload. + + **Last known good survives an outage.** A transport failure never empties the + store and never makes ``get_object`` raise: it keeps serving what it last + received, which is what makes ``write_skills(on_unavailable="keep")`` + correct. ``diagnostics`` and ``failed`` report the degradation. + + **What arrives is untrusted.** This store holds raw wire objects verbatim and + verifies nothing — integrity verification lives at the accessor boundary so + it applies to every store equally. In particular an object with no + ``contentHash`` is held and then *withheld* by verification; see + ``StoreDiagnostics.hashless_objects``. + """ + + def __init__( + self, + sdk_key: str, + *, + base_uri: str = DEFAULT_BASE_URI, + mode: Mode = "stream", + poll_interval: float = 30.0, + connect_timeout: float = 10.0, + read_timeout: float = 300.0, + initial_backoff: float = 1.0, + max_backoff: float = 30.0, + max_consecutive_failures: int = 10, + data_model_version: int = SDK_DATA_MODEL_VERSION, + _requester: Any = None, + ) -> None: + """ + *mode* is ``"stream"`` by default. Prefer it: a ``delete-object`` reaches a + live stream in seconds, which is what makes revocation seconds-latent + instead of restart-latent, and is why the change-listener re-reconcile is + worth wiring at all. ``"poll"`` exists for environments that cannot hold a + long-lived connection, and revocation there is one ``poll_interval`` late. + + *max_consecutive_failures* bounds the retry loop. On exceeding it the + transport stops, logs an error, and the store keeps serving last known + good rather than pretending to be live — ``failed`` reports it. + """ + _require_server_side_credential(sdk_key) + if mode not in ("stream", "poll"): + raise ValueError(f'mode must be "stream" or "poll", got {mode!r}') + if poll_interval <= 0: + raise ValueError(f"poll_interval must be positive, got {poll_interval!r}") + + self._mode: Mode = mode + self._poll_interval = poll_interval + self._initial_backoff = initial_backoff + self._max_backoff = max_backoff + self._max_consecutive_failures = max_consecutive_failures + + self._objects = _SkillObjectSet() + self._reader = _ProtocolReader(self._objects) + self._lock = threading.RLock() + self._listeners: dict[str, list[Callable[[dict[str, Any]], Any]]] = {} + + self._basis: str | None = None + self._etag: str | None = None + + self._requester = _requester or _Requester( + sdk_key.strip(), + base_uri, + connect_timeout=connect_timeout, + read_timeout=read_timeout, + data_model_version=data_model_version, + ) + + self._stop = threading.Event() + self._first_payload = threading.Event() + self._thread: threading.Thread | None = None + self._failed_reason: str | None = None + self._connection: Any = None + """The open streaming connection, so ``close`` can interrupt its read.""" + + # -- lifecycle --------------------------------------------------------- + + def start(self) -> FDv2SkillStore: + """ + Starts the delivery thread. Idempotent; returns ``self`` so it chains. + + Does not block: use ``wait_for_skills`` when boot ordering matters. + """ + with self._lock: + if self._thread is not None and self._thread.is_alive(): + return self + self._stop.clear() + self._thread = threading.Thread( + target=self._run, name="ld-ai-skills-fdv2", daemon=True + ) + self._thread.start() + return self + + def close(self, timeout: float = 5.0) -> None: + """ + Stops delivery. Idempotent, and safe to call from any thread. + + Held content is *not* dropped: a closed store still answers from what it + received, so shutting the transport down does not turn into an integrity + failure or an empty reconcile mid-flight. ``shutdown()`` is what detaches + the store from the accessors. + """ + self._stop.set() + # Interrupt the read before joining. The delivery thread is normally + # blocked in a socket read that no flag can reach, so without this the + # join below waits out its full timeout on every shutdown. + with self._lock: + connection = self._connection + if connection is not None: + connection.close() + thread = self._thread + if ( + thread is not None + and thread.is_alive() + and thread is not threading.current_thread() + ): + thread.join(timeout=timeout) + + def __enter__(self) -> FDv2SkillStore: + return self.start() + + def __exit__(self, *_exc: Any) -> None: + self.close() + + def wait_for_skills(self, timeout: float = 10.0) -> bool: + """ + Blocks until the first payload has been committed, or *timeout* elapses. + + ``True`` means a payload arrived — not that any skill in it verified, and + not that the environment has any skills. Boot ordering is all this + answers; ``diagnostics`` answers the rest. + """ + return self._first_payload.wait(timeout=timeout) + + @property + def failed(self) -> str | None: + """Why delivery stopped for good, or ``None`` while it is running.""" + with self._lock: + return self._failed_reason + + @property + def diagnostics(self) -> StoreDiagnostics: + """A snapshot of what the transport has seen. See ``StoreDiagnostics``.""" + with self._lock: + return StoreDiagnostics(**vars(self._reader.diagnostics)) + + # -- the SkillStore seam ---------------------------------------------- + + def get_object( + self, kind: str, key: str, version: int | None = None + ) -> dict[str, Any] | None: + if kind != SKILL_OBJECT_KIND: + return None + with self._lock: + return self._objects.get(key, version) + + def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: + if kind != SKILL_OBJECT_KIND: + return {} + with self._lock: + return self._objects.snapshot() + + def add_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None: + """ + Registers *fn* to be called once per committed change. + + Fires **once per changed object at payload-transferred**, not as objects + stream in: a payload version is the unit of consistency, and a listener + that reacted to a half-applied full transfer would see the store briefly + empty. ``skills_watch.watch_skills`` is the intended consumer. + + A put notifies with the raw skill object. A revocation notifies with a + ``{"key", "version"}`` tombstone — it names what went away and carries no + content, since there is none. A listener that only needs "something + changed" works with both; one that reads content must check for + ``content`` rather than assume it. + + *fn* runs on the delivery thread. Keep it cheap and non-blocking: work + done there delays the next event. An exception it raises is logged and + swallowed, because a broken listener must not be able to kill delivery. + """ + with self._lock: + self._listeners.setdefault(kind, []).append(fn) + + def _notify(self, changes: list[dict[str, Any]]) -> None: + with self._lock: + listeners = list(self._listeners.get(SKILL_OBJECT_KIND, [])) + for raw in changes: + for listener in listeners: + try: + listener(raw) + except Exception: + logger.error( + "A skill store change listener raised; delivery continues", + exc_info=True, + ) + + # -- the delivery loop ------------------------------------------------- + + def _run(self) -> None: + failures = 0 + while not self._stop.is_set(): + try: + if self._mode == "stream": + self._stream_once() + else: + self._poll_once() + failures = 0 + with self._lock: + self._reader.diagnostics.connection_failures = 0 + except _FatalTransportError as exc: + self._give_up(str(exc)) + return + except _RecoverableTransportError as exc: + failures += 1 + with self._lock: + self._reader.diagnostics.connection_failures = failures + self._reader.diagnostics.last_error = str(exc) + if failures > self._max_consecutive_failures: + self._give_up( + f"gave up after {failures} consecutive failures; " + f"last error: {exc}" + ) + return + delay = exc.retry_after + if delay is None: + delay = backoff_delay( + failures, base=self._initial_backoff, maximum=self._max_backoff + ) + logger.warning( + "Skill delivery failed (%s); retrying in %.1fs", exc, delay + ) + if self._stop.wait(delay): + return + continue + except Exception as exc: # pragma: no cover - belt and braces + self._give_up(f"unexpected error in skill delivery: {exc!r}") + logger.error("Unexpected error in skill delivery", exc_info=True) + return + + if self._mode == "poll" and self._stop.wait(self._poll_interval): + return + + def _give_up(self, reason: str) -> None: + with self._lock: + self._failed_reason = reason + self._reader.diagnostics.last_error = reason + logger.error( + "Skill delivery has stopped and will not retry: %s. The store keeps " + "serving the last content it received; skills will not update until " + "the process restarts with a working connection.", + reason, + ) + # Unblock anyone waiting on a first payload that is never coming, rather + # than making them eat the full timeout. + self._first_payload.set() + + def _apply(self, name: str, data: Any) -> _TransferOutcome: + with self._lock: + outcome = self._reader.handle(name, data) + if outcome.committed and outcome.basis is not None: + self._basis = outcome.basis + if outcome.committed: + self._first_payload.set() + if outcome.changes: + self._notify(outcome.changes) + return outcome + + def _poll_once(self) -> None: + with self._lock: + basis, etag = self._basis, self._etag + result = self._requester.poll(basis, etag) + with self._lock: + self._etag = result.etag + if result.not_modified: + logger.debug("Skill payload unchanged (HTTP 304)") + # A 304 is a successful, current answer: the payload we hold is the + # payload the server has. It counts as a first payload so a boot that + # reconnects with a cached basis is not blocked on a transfer the + # server has no reason to send. + self._first_payload.set() + return + for name, data in result.events: + outcome = self._apply(name, data) + if outcome.fatal: + raise _FatalTransportError(outcome.fatal) + if outcome.disconnect: + raise _RecoverableTransportError(outcome.disconnect) + + def _stream_once(self) -> None: + with self._lock: + basis = self._basis + connection = self._requester.stream(basis) + with self._lock: + self._connection = connection + try: + for name, data in connection.events: + if self._stop.is_set(): + return + outcome = self._apply(name, data) + if outcome.fatal: + raise _FatalTransportError(outcome.fatal) + if outcome.disconnect: + raise _RecoverableTransportError(outcome.disconnect) + except Exception: + if self._stop.is_set(): + # `close` interrupted the read on purpose; unwinding quietly is + # the point, not a failure to report or retry. + return + raise + finally: + connection.close() + with self._lock: + self._connection = None + # A stream that ends without a goodbye is a dropped connection, not a + # completed operation: reconnect through the backoff path. + raise _RecoverableTransportError("the FDv2 stream closed unexpectedly") diff --git a/packages/client/src/launchdarkly_ai_server/skills_watch.py b/packages/client/src/launchdarkly_ai_server/skills_watch.py new file mode 100644 index 0000000..f7b5e2b --- /dev/null +++ b/packages/client/src/launchdarkly_ai_server/skills_watch.py @@ -0,0 +1,263 @@ +""" +Agent Skills — re-reconcile on delivery, so revocation does not wait for a restart. + +``write_skills`` is a one-shot reconcile: it materializes what the store holds +now. That was the whole story while the only transport was a hand-populated +store, and the design accordingly deferred an eager re-reconcile — revocation +would take effect at the next process restart, which the security review filed +as AV-1. + +A streaming FDv2 connection changes the premise. A ``delete-object`` reaches a +live connection in **seconds**, and the store already publishes a change +listener, so the gap between "LaunchDarkly revoked this skill" and "its +``SKILL.md`` is off the agent's disk" collapses from a process lifetime to a +debounce interval. That is the single largest resilience improvement available +at this layer, which is why it is here rather than in a later phase. + +``on_unavailable="keep"`` stays the default, deliberately and per the review: an +outage must not read as "everything was revoked". A watcher that pruned on a +failed retrieval would convert every transport blip into deletion of a +customer's skill files. + +Layering: this module sits *above* ``skills_fs`` and calls ``write_skills`` +without modifying it. Nothing in the reconcile, the accessors, or verification +knows this file exists. +""" + +from __future__ import annotations + +import asyncio +import logging +import os +import threading +from collections.abc import Callable, Sequence +from typing import Any + +from .skills_core import SKILL_OBJECT_KIND, get_store +from .skills_fs import OnUnavailable, write_skills +from .types import ReconcileReport, Skill, SkillReference + +logger = logging.getLogger(__name__) + +DEFAULT_DEBOUNCE_SECONDS = 0.5 +""" +How long a change waits for its neighbours before a reconcile runs. + +A full payload transfer commits many objects at once and the listener fires per +object, so without coalescing a payload of forty skills would run forty +reconciles against one root. Half a second is far below the seconds-scale +latency this feature is trying to achieve and far above the microseconds a +commit's listener calls take. +""" + + +class SkillWatcher: + """ + A running re-reconcile. Returned by ``watch_skills``; stop it with ``close``. + + One watcher owns one root. **Do not point two watchers at the same root**, + and do not run ``write_skills`` against a watched root concurrently: the + reconcile's own contract is one root, one reconcile at a time, because two + interleaved runs lose the loser's manifest entries and leave the files it + wrote unmanaged. This class enforces that for its *own* reconciles — they run + on a single worker thread, serialized — and cannot enforce it against a + caller who reconciles the same root by hand. + """ + + def __init__( + self, + request: Sequence[Skill | SkillReference | str] | str, + root: str | os.PathLike[str], + *, + prune: bool, + timeout: float, + on_unavailable: OnUnavailable, + debounce: float, + on_reconcile: Callable[[ReconcileReport], Any] | None, + ) -> None: + self._request = request + self._root = root + self._prune = prune + self._timeout = timeout + self._on_unavailable = on_unavailable + self._debounce = debounce + self._on_reconcile = on_reconcile + + self._wake = threading.Event() + self._stop = threading.Event() + self._reconciles = 0 + self._lock = threading.Lock() + self._thread = threading.Thread( + target=self._run, name="ld-ai-skills-reconcile", daemon=True + ) + self._thread.start() + + # -- the listener the store calls ------------------------------------- + + def notify(self, _raw: Any = None) -> None: + """ + The store's change listener. Records that something changed; runs nothing. + + Deliberately trivial. It is called on the delivery thread, where a + reconcile — which does synchronous filesystem I/O, an fsync per file, and + a manifest rewrite — would stall event processing for the duration and, + on a stream, let the connection's read buffer back up behind a disk write. + The argument is ignored: a put's raw object and a revocation's tombstone + both mean the same thing here, which is "the store is not what it was". + """ + self._wake.set() + + # -- the worker -------------------------------------------------------- + + def _run(self) -> None: + while not self._stop.is_set(): + if not self._wake.wait(timeout=0.5): + continue + if self._stop.is_set(): + return + # Coalesce the rest of the burst. Clearing *before* the sleep rather + # than after is what makes a change arriving mid-debounce trigger the + # next pass instead of being swallowed by this one. + self._wake.clear() + if self._stop.wait(self._debounce): + return + self._reconcile_once() + + def _reconcile_once(self) -> None: + try: + report = asyncio.run( + write_skills( + self._request, + self._root, + prune=self._prune, + timeout=self._timeout, + on_unavailable=self._on_unavailable, + ) + ) + except Exception: + # A watcher that died on one bad reconcile would silently stop + # tracking revocations, which is worse than a noisy one. + logger.error( + "A skill re-reconcile raised; the watcher continues", exc_info=True + ) + return + + with self._lock: + self._reconciles += 1 + changed = [ + action + for action in report.actions + if action.action in ("written", "updated", "removed", "error") + ] + if changed: + logger.info( + "Re-reconciled skills after a delivery change: %d action(s) of note", + len(changed), + ) + if self._on_reconcile is not None: + try: + self._on_reconcile(report) + except Exception: + logger.error("A watch_skills callback raised", exc_info=True) + + # -- lifecycle --------------------------------------------------------- + + @property + def reconciles(self) -> int: + """How many re-reconciles have completed since the watcher started. + + Excludes the initial reconcile ``watch_skills`` awaits, which is the + caller's own result.""" + with self._lock: + return self._reconciles + + def close(self, timeout: float = 15.0) -> None: + """ + Stops watching. Idempotent. Does not undo anything already on disk. + + Waits out an in-flight reconcile rather than interrupting one, because a + reconcile killed between its content writes and its manifest rewrite is + the one case the manifest format has to recover from — worth avoiding when + we control the timing. + """ + self._stop.set() + self._wake.set() + if self._thread.is_alive() and self._thread is not threading.current_thread(): + self._thread.join(timeout=timeout) + + def __enter__(self) -> SkillWatcher: + return self + + def __exit__(self, *_exc: Any) -> None: + self.close() + + +async def watch_skills( + skills: Sequence[Skill | SkillReference | str] | str, + root: str | os.PathLike[str], + *, + prune: bool = True, + timeout: float = 10.0, + on_unavailable: OnUnavailable = "keep", + debounce: float = DEFAULT_DEBOUNCE_SECONDS, + on_reconcile: Callable[[ReconcileReport], Any] | None = None, +) -> tuple[ReconcileReport, SkillWatcher]: + """ + Reconciles now, then re-reconciles whenever delivery changes. + + Every argument that ``write_skills`` takes means the same thing here and is + passed straight through; the reconcile's semantics are untouched. Returns the + initial reconcile's report — so a caller can fail fast on a bad root or a + corrupt manifest exactly as they would with ``write_skills`` — paired with a + ``SkillWatcher`` to close when the process is done:: + + report, watcher = await watch_skills("*", "/etc/agent/skills") + try: + ... + finally: + watcher.close() + + A revocation delivered over a streaming connection then prunes the skill's + files within ``debounce`` of arriving, rather than at the next restart. + + Requires a store that implements the optional ``add_listener`` half of the + seam. Raises ``RuntimeError`` when no store is configured, and when the + configured store has no ``add_listener`` — the second case failing loudly + rather than degrading to a one-shot reconcile, because a watcher that + silently never fires looks exactly like a watcher whose skills never changed. + """ + store = get_store() + if store is None: + raise RuntimeError( + "watch_skills needs a configured skill store. Configure one with " + 'init_client(options={"skillStore": store}).' + ) + add_listener = getattr(store, "add_listener", None) + if not callable(add_listener): + raise RuntimeError( + "watch_skills needs a skill store that implements add_listener(kind, " + "fn); the configured store does not, so delivery changes cannot be " + "observed. Use write_skills for a one-shot reconcile, or configure a " + "store with a delivery transport (FDv2SkillStore)." + ) + if debounce < 0: + raise ValueError(f"debounce must not be negative, got {debounce!r}") + + # The initial reconcile runs first and on the caller's thread, so its report + # is the caller's to inspect and a bad root raises out of `watch_skills` + # rather than into a worker thread's log. + report = await write_skills( + skills, root, prune=prune, timeout=timeout, on_unavailable=on_unavailable + ) + + watcher = SkillWatcher( + skills, + root, + prune=prune, + timeout=timeout, + on_unavailable=on_unavailable, + debounce=debounce, + on_reconcile=on_reconcile, + ) + add_listener(SKILL_OBJECT_KIND, watcher.notify) + return report, watcher diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py new file mode 100644 index 0000000..310be91 --- /dev/null +++ b/packages/client/tests/test_skills_fdv2.py @@ -0,0 +1,1595 @@ +""" +Tests for the FDv2 skill delivery transport. + +Two layers, deliberately: + +- **A real fake endpoint.** ``_FakeFDv2Endpoint`` is an in-process + ``ThreadingHTTPServer`` that implements the wire contract — ``basis`` and + ``mv`` query parameters, ``Authorization``, ``If-None-Match``/304, the + ``{"events": [...]}`` polling envelope, and SSE for streaming. The store under + test opens real sockets against it, so request construction and header + handling are exercised rather than mocked. This is what stands in for a live + server while the backend work is unmerged. +- **The protocol reader driven directly.** Wire semantics — which objects are + skills, ``objectVersion`` versus ``version``, revocation, mixed payloads — are + asserted against ``_ProtocolReader``, which has no I/O, so those cases read as + the contract they are instead of as a server script. +""" + +from __future__ import annotations + +import hashlib +import json +import threading +import time +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from typing import Any, ClassVar +from urllib.parse import parse_qs, urlparse + +import pytest + +from launchdarkly_ai_server import ( + FDv2SkillStore, + InMemorySkillStore, + all_skills, + get_skill, + get_skill_result, + init_client, + watch_skills, +) +from launchdarkly_ai_server.skills_core import SKILL_OBJECT_KIND +from launchdarkly_ai_server.skills_fdv2 import ( + FDV2_OBJECT_CATEGORY, + FDV2_OBJECT_KIND, + _ProtocolReader, + _RecoverableTransportError, + _SkillObjectSet, + backoff_delay, + is_skill_event, + seam_object_from_put, + tombstone_from_delete, +) +from launchdarkly_ai_server.skills_fdv2 import _warned_hashless as _hashless_dedupe + +pytestmark = pytest.mark.usefixtures("reset_skill_state") + +SDK_KEY = "sdk-00000000-0000-4000-8000-000000000000" +SKILL_BODY = "---\nname: PDF Extraction\n---\nExtract text from PDFs.\n" + + +def _hash(content: str) -> str: + return hashlib.sha256(content.encode("utf-8")).hexdigest() + + +# --------------------------------------------------------------------------- +# Wire builders — one place that knows the shape, so a contract change is one edit +# --------------------------------------------------------------------------- + + +def put_skill( + key: str = "pdf-extraction", + *, + object_version: Any = 3, + payload_version: int = 42, + content: str = SKILL_BODY, + content_hash: Any = None, + omit_hash: bool = False, + name: str = "PDF Extraction", +) -> dict[str, Any]: + """One skill ``put-object`` event's data, exactly as §2.3 specifies it.""" + envelope: dict[str, Any] = { + "contentType": "text/markdown", + "content": content, + "name": name, + "description": "Extracts text", + } + if not omit_hash: + envelope["contentHash"] = ( + content_hash if content_hash is not None else _hash(content) + ) + return { + "key": key, + "kind": FDV2_OBJECT_KIND, + "category": FDV2_OBJECT_CATEGORY, + "objectVersion": object_version, + "version": payload_version, + "object": envelope, + } + + +def delete_skill( + key: str = "pdf-extraction", *, object_version: Any = 3, payload_version: int = 43 +) -> dict[str, Any]: + return { + "key": key, + "kind": FDV2_OBJECT_KIND, + "category": FDV2_OBJECT_CATEGORY, + "objectVersion": object_version, + "version": payload_version, + } + + +def put_flag(key: str = "my-flag", version: int = 17) -> dict[str, Any]: + """A flag ``put-object``: no ``category``, no ``objectVersion``.""" + return { + "key": key, + "kind": "flag", + "version": version, + "object": { + "key": key, + "version": version, + "on": True, + "variations": [True, False], + }, + } + + +def put_segment(key: str = "beta-users", version: int = 4) -> dict[str, Any]: + return { + "key": key, + "kind": "segment", + "version": version, + "object": {"key": key, "version": version, "included": []}, + } + + +def server_intent( + code: str = "xfer-full", payload_id: str = "agent-skill" +) -> dict[str, Any]: + return { + "payloads": [ + {"id": payload_id, "target": 1, "intentCode": code, "reason": "test"} + ] + } + + +def transferred(state: str = "basis-1", version: int = 42) -> dict[str, Any]: + return {"state": state, "version": version} + + +def events(*pairs: tuple[str, Any]) -> list[dict[str, Any]]: + return [{"event": name, "data": data} for name, data in pairs] + + +def full_payload( + *object_events: tuple[str, Any], state: str = "basis-1" +) -> list[dict[str, Any]]: + return events( + ("server-intent", server_intent("xfer-full")), + *object_events, + ("payload-transferred", transferred(state)), + ) + + +# --------------------------------------------------------------------------- +# The fake endpoint +# --------------------------------------------------------------------------- + + +class _FakeFDv2Endpoint: + """ + An in-process server implementing the SDK-facing FDv2 contract. + + Scripted per request: ``queue_poll`` appends a response for the next + ``/sdk/poll``, ``queue_stream`` appends a sequence of SSE events for the next + ``/sdk/stream``. Every request's method, path, query and headers are recorded + in ``requests`` so the tests can assert on what the store actually sent — + which is the only way ``basis`` round-tripping and ``If-None-Match`` can be + checked at all. + """ + + def __init__(self) -> None: + self.requests: list[dict[str, Any]] = [] + self._polls: list[dict[str, Any]] = [] + self._streams: list[list[dict[str, Any]]] = [] + self._lock = threading.Lock() + self.hold_stream_open = False + self._release = threading.Event() + + endpoint = self + + class Handler(BaseHTTPRequestHandler): + protocol_version = "HTTP/1.1" + + def log_message(self, *_args: Any) -> None: + return + + def do_GET(self) -> None: + parsed = urlparse(self.path) + query = {k: v[0] for k, v in parse_qs(parsed.query).items()} + with endpoint._lock: + endpoint.requests.append( + { + "path": parsed.path, + "query": query, + "authorization": self.headers.get("Authorization"), + "if_none_match": self.headers.get("If-None-Match"), + "accept": self.headers.get("Accept"), + } + ) + if parsed.path == "/sdk/poll": + endpoint._serve_poll(self) + elif parsed.path == "/sdk/stream": + endpoint._serve_stream(self) + else: + self.send_response(404) + self.send_header("Content-Length", "0") + self.end_headers() + + class Server(ThreadingHTTPServer): + # Handler threads are not joined on shutdown: a test that ends while + # a stream is deliberately held open should not pay for the hold. + daemon_threads = True + + self._server = Server(("127.0.0.1", 0), Handler) + # A short poll interval so `shutdown` is prompt: the default 0.5s is + # paid at the teardown of every test that touches the endpoint. + self._thread = threading.Thread( + target=lambda: self._server.serve_forever(poll_interval=0.01), daemon=True + ) + self._thread.start() + + @property + def base_uri(self) -> str: + host, port = self._server.server_address[:2] + return f"http://{host}:{port}" + + # -- scripting --------------------------------------------------------- + + def queue_poll( + self, + payload_events: list[dict[str, Any]] | None = None, + *, + status: int = 200, + etag: str | None = None, + retry_after: str | None = None, + ) -> None: + with self._lock: + self._polls.append( + { + "status": status, + "events": payload_events or [], + "etag": etag, + "retry_after": retry_after, + } + ) + + def queue_stream(self, payload_events: list[dict[str, Any]]) -> None: + with self._lock: + self._streams.append(payload_events) + + # -- serving ----------------------------------------------------------- + + def _serve_poll(self, handler: BaseHTTPRequestHandler) -> None: + with self._lock: + response = ( + self._polls.pop(0) if self._polls else {"status": 304, "events": []} + ) + status = response["status"] + handler.send_response(status) + if response.get("etag"): + handler.send_header("ETag", response["etag"]) + if response.get("retry_after"): + handler.send_header("Retry-After", response["retry_after"]) + if status in (200,): + body = json.dumps({"events": response["events"]}).encode("utf-8") + handler.send_header("Content-Type", "application/json") + handler.send_header("Content-Length", str(len(body))) + handler.end_headers() + handler.wfile.write(body) + return + handler.send_header("Content-Length", "0") + handler.end_headers() + + def _serve_stream(self, handler: BaseHTTPRequestHandler) -> None: + with self._lock: + payload_events = self._streams.pop(0) if self._streams else [] + handler.send_response(200) + handler.send_header("Content-Type", "text/event-stream") + handler.send_header("Cache-Control", "no-cache") + handler.send_header("Transfer-Encoding", "chunked") + handler.end_headers() + for event in payload_events: + chunk = ( + f"event: {event['event']}\ndata: {json.dumps(event.get('data'))}\n\n" + ).encode() + handler.wfile.write(f"{len(chunk):X}\r\n".encode() + chunk + b"\r\n") + handler.wfile.flush() + if self.hold_stream_open: + # Keeps the connection up so a test can assert on the store's state + # without racing the reconnect path. Released on ``close`` so the + # hold costs the suite nothing once the test is done with it. + self._release.wait(timeout=10) + handler.wfile.write(b"0\r\n\r\n") + + def close(self) -> None: + self._release.set() + self._server.shutdown() + self._server.server_close() + + +@pytest.fixture +def endpoint() -> Any: + server = _FakeFDv2Endpoint() + yield server + server.close() + + +@pytest.fixture(autouse=True) +def _clear_hashless_dedupe() -> Any: + """The hashless-object ERROR is deduped per process; per test here.""" + _hashless_dedupe.clear() + yield + _hashless_dedupe.clear() + + +def poll_store(endpoint: Any, **kwargs: Any) -> FDv2SkillStore: + return FDv2SkillStore( + SDK_KEY, + base_uri=endpoint.base_uri, + mode="poll", + poll_interval=kwargs.pop("poll_interval", 0.05), + initial_backoff=kwargs.pop("initial_backoff", 0.01), + max_backoff=kwargs.pop("max_backoff", 0.05), + read_timeout=kwargs.pop("read_timeout", 5.0), + **kwargs, + ) + + +def wait_until(predicate: Any, timeout: float = 5.0) -> bool: + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return True + time.sleep(0.01) + return predicate() + + +# --------------------------------------------------------------------------- +# Identifying skill objects, and ignoring everything else +# --------------------------------------------------------------------------- + + +class TestObjectIdentification: + def test_kind_and_category_together_identify_a_skill(self) -> None: + assert is_skill_event(put_skill()) is True + + def test_a_flag_is_not_a_skill(self) -> None: + assert is_skill_event(put_flag()) is False + + def test_a_segment_is_not_a_skill(self) -> None: + assert is_skill_event(put_segment()) is False + + def test_inline_resource_of_another_category_is_not_a_skill(self) -> None: + """``inline-resource`` is a broad kind; the category is load-bearing.""" + other = put_skill() + other["category"] = "prompt-template" + assert is_skill_event(other) is False + + def test_skill_category_under_another_kind_is_not_a_skill(self) -> None: + other = put_skill() + other["kind"] = "some-future-kind" + assert is_skill_event(other) is False + + def test_a_flag_shaped_object_with_no_category_is_not_a_skill(self) -> None: + """Flags and segments omit ``category`` entirely — the documented shape.""" + assert "category" not in put_flag() + assert "objectVersion" not in put_flag() + + @pytest.mark.parametrize("value", [None, "skill", 3, [], ()]) + def test_non_dict_events_are_not_skills(self, value: Any) -> None: + assert is_skill_event(value) is False + + +# --------------------------------------------------------------------------- +# objectVersion is not version. This is the whole ballgame. +# --------------------------------------------------------------------------- + + +class TestVersionTranslation: + def test_object_version_becomes_the_seam_version(self) -> None: + raw = seam_object_from_put(put_skill(object_version=3, payload_version=42)) + assert raw is not None + assert raw["version"] == 3 + + def test_the_payload_version_never_reaches_the_seam(self) -> None: + """ + The failure this asserts against is silent: a store that read ``version`` + would serve verifiable content under a version number that means nothing, + and every pinned reference would resolve to the wrong thing with no error. + """ + raw = seam_object_from_put(put_skill(object_version=3, payload_version=42)) + assert raw is not None + assert raw["version"] != 42 + assert 42 not in raw.values() + + def test_the_two_are_distinguished_even_when_the_payload_version_is_lower( + self, + ) -> None: + raw = seam_object_from_put(put_skill(object_version=99, payload_version=1)) + assert raw is not None + assert raw["version"] == 99 + + def test_a_missing_object_version_is_not_defaulted_from_the_payload(self) -> None: + wire = put_skill() + del wire["objectVersion"] + raw = seam_object_from_put(wire) + assert raw is not None + assert "version" not in raw + + def test_an_explicitly_null_object_version_is_carried_through_as_null(self) -> None: + """Carried, not invented: verification reports ``invalid_version``.""" + raw = seam_object_from_put(put_skill(object_version=None)) + assert raw is not None + assert raw["version"] is None + + def test_a_delete_translates_object_version_too(self) -> None: + tombstone = tombstone_from_delete( + delete_skill(object_version=3, payload_version=43) + ) + assert tombstone is not None + assert tombstone.object_version == 3 + + def test_a_delete_with_no_usable_object_version_revokes_every_version(self) -> None: + tombstone = tombstone_from_delete(delete_skill(object_version=None)) + assert tombstone is not None + assert tombstone.object_version is None + + def test_a_keyless_put_is_dropped_because_it_has_no_identity(self) -> None: + wire = put_skill() + del wire["key"] + assert seam_object_from_put(wire) is None + + def test_the_envelope_is_copied_verbatim(self) -> None: + raw = seam_object_from_put(put_skill()) + assert raw is not None + assert raw["content"] == SKILL_BODY + assert raw["contentHash"] == _hash(SKILL_BODY) + assert raw["name"] == "PDF Extraction" + assert raw["contentType"] == "text/markdown" + + def test_an_absent_envelope_field_is_absent_rather_than_defaulted(self) -> None: + wire = put_skill() + del wire["object"]["name"] + raw = seam_object_from_put(wire) + assert raw is not None + assert "name" not in raw + + +# --------------------------------------------------------------------------- +# The protocol reader +# --------------------------------------------------------------------------- + + +def drive(reader: _ProtocolReader, payload_events: list[dict[str, Any]]) -> list[Any]: + return [reader.handle(e["event"], e.get("data")) for e in payload_events] + + +class TestProtocolReader: + def test_a_full_transfer_commits_at_payload_transferred(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + outcomes = drive(reader, full_payload(("put-object", put_skill()))) + assert len(held) == 1 + assert outcomes[-1].committed is True + assert outcomes[-1].basis == "basis-1" + + def test_nothing_is_visible_before_payload_transferred(self) -> None: + """A payload version is the unit of consistency; half of one is not a state.""" + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive( + reader, + events( + ("server-intent", server_intent("xfer-full")), + ("put-object", put_skill()), + ), + ) + assert len(held) == 0 + + def test_an_interrupted_full_transfer_leaves_last_known_good_intact(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive(reader, full_payload(("put-object", put_skill(object_version=1)))) + assert held.get("pdf-extraction", None) is not None + + # A second full transfer starts and never completes. + drive( + reader, + events( + ("server-intent", server_intent("xfer-full")), + ("put-object", put_skill(object_version=2)), + ), + ) + still_held = held.get("pdf-extraction", None) + assert still_held is not None + assert still_held["version"] == 1 + + def test_a_full_transfer_replaces_rather_than_merges(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive(reader, full_payload(("put-object", put_skill("first")))) + drive( + reader, full_payload(("put-object", put_skill("second")), state="basis-2") + ) + assert held.get("first", None) is None + assert held.get("second", None) is not None + + def test_a_change_transfer_applies_deltas_over_what_is_held(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive(reader, full_payload(("put-object", put_skill("first")))) + drive( + reader, + events( + ("server-intent", server_intent("xfer-changes")), + ("put-object", put_skill("second")), + ("payload-transferred", transferred("basis-2")), + ), + ) + assert held.get("first", None) is not None + assert held.get("second", None) is not None + + def test_a_delete_object_revokes_the_skill(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive(reader, full_payload(("put-object", put_skill(object_version=3)))) + drive( + reader, + events( + ("server-intent", server_intent("xfer-changes")), + ("delete-object", delete_skill(object_version=3)), + ("payload-transferred", transferred("basis-2")), + ), + ) + assert held.get("pdf-extraction", None) is None + assert reader.diagnostics.objects_revoked == 1 + + def test_a_delete_notifies_with_a_tombstone_carrying_no_content(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive(reader, full_payload(("put-object", put_skill()))) + outcomes = drive( + reader, + events( + ("server-intent", server_intent("xfer-changes")), + ("delete-object", delete_skill()), + ("payload-transferred", transferred("basis-2")), + ), + ) + (change,) = outcomes[-1].changes + assert change == {"key": "pdf-extraction", "version": 3} + assert "content" not in change + + def test_a_delete_for_one_version_leaves_the_other_held(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive( + reader, + full_payload( + ("put-object", put_skill(object_version=2)), + ("put-object", put_skill(object_version=3)), + ), + ) + drive( + reader, + events( + ("server-intent", server_intent("xfer-changes")), + ("delete-object", delete_skill(object_version=3)), + ("payload-transferred", transferred("basis-2")), + ), + ) + assert held.get("pdf-extraction", 2) is not None + assert held.get("pdf-extraction", None)["version"] == 2 + + def test_flag_and_segment_objects_are_skipped_cleanly(self) -> None: + """ + The mixed payload is the normal case, not an edge one: an environment's + assignment carries the flagging payload alongside the agent-skill payload. + """ + held = _SkillObjectSet() + reader = _ProtocolReader(held) + outcomes = drive( + reader, + full_payload( + ("put-object", put_flag("flag-a")), + ("put-object", put_skill("pdf-extraction")), + ("put-object", put_segment("beta-users")), + ("put-object", put_flag("flag-b")), + ("delete-object", put_flag("flag-c")), + ), + ) + assert len(held) == 1 + assert held.get("pdf-extraction", None) is not None + assert reader.diagnostics.objects_ignored == 4 + assert reader.diagnostics.skill_objects_received == 1 + assert all(o.fatal is None and o.disconnect is None for o in outcomes) + + def test_an_unknown_kind_is_ignored_rather_than_fatal(self) -> None: + """ + Erroring here is the unknown-kind reconnect loop this feature must not + reproduce — a flag-delivery outage caused by a skills rollout. + """ + held = _SkillObjectSet() + reader = _ProtocolReader(held) + exotic = { + "key": "x", + "kind": "quantum-widget", + "version": 1, + "object": {"a": 1}, + } + outcomes = drive(reader, full_payload(("put-object", exotic))) + assert len(held) == 0 + assert all(o.fatal is None and o.disconnect is None for o in outcomes) + + def test_an_unknown_event_name_is_ignored(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + outcome = reader.handle("some-future-event", {"anything": True}) + assert outcome.fatal is None + assert outcome.disconnect is None + + def test_a_heartbeat_does_nothing(self) -> None: + reader = _ProtocolReader(_SkillObjectSet()) + outcome = reader.handle("heart-beat", None) + assert outcome == type(outcome)() + + def test_an_error_event_abandons_the_in_flight_payload(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive(reader, full_payload(("put-object", put_skill(object_version=1)))) + outcomes = drive( + reader, + events( + ("server-intent", server_intent("xfer-full")), + ("put-object", put_skill(object_version=2)), + ( + "error", + {"payloadId": "agent-skill", "reason": "backend unavailable"}, + ), + ), + ) + assert outcomes[-1].disconnect is not None + assert held.get("pdf-extraction", None)["version"] == 1 + + def test_a_goodbye_asks_for_a_reconnect(self) -> None: + reader = _ProtocolReader(_SkillObjectSet()) + outcome = reader.handle("goodbye", {"reason": "rebalancing", "silent": False}) + assert outcome.disconnect is not None + assert outcome.fatal is None + + def test_a_catastrophic_goodbye_is_fatal(self) -> None: + reader = _ProtocolReader(_SkillObjectSet()) + outcome = reader.handle( + "goodbye", {"reason": "no", "silent": False, "catastrophe": True} + ) + assert outcome.fatal is not None + + def test_transfer_none_holds_everything_and_commits(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive(reader, full_payload(("put-object", put_skill()))) + drive( + reader, + events( + ("server-intent", server_intent("none")), + ("payload-transferred", transferred("basis-2")), + ), + ) + assert len(held) == 1 + + def test_an_object_arriving_with_no_intent_is_treated_as_a_delta(self) -> None: + held = _SkillObjectSet() + reader = _ProtocolReader(held) + drive( + reader, + events( + ("put-object", put_skill()), + ("payload-transferred", transferred("basis-1")), + ), + ) + assert len(held) == 1 + + +# --------------------------------------------------------------------------- +# Seam parity with InMemorySkillStore +# --------------------------------------------------------------------------- + + +class TestSeamParity: + """ + The two stores must resolve identically. ``_SkillObjectSet`` reimplements the + lookup rather than inheriting it — see its docstring for why — so this is the + test that stops the two from drifting. + """ + + RAWS: ClassVar[list[dict[str, Any]]] = [ + {"key": "a", "version": 1, "content": "x", "contentHash": _hash("x")}, + {"key": "a", "version": 4, "content": "y", "contentHash": _hash("y")}, + {"key": "b", "version": 2, "content": "z", "contentHash": _hash("z")}, + {"key": "malformed", "version": "not-a-version", "content": "q"}, + ] + + def _both(self) -> tuple[InMemorySkillStore, _SkillObjectSet]: + memory = InMemorySkillStore() + objects = _SkillObjectSet() + for raw in self.RAWS: + memory.put(dict(raw)) + objects.put(dict(raw)) + return memory, objects + + @pytest.mark.parametrize( + "key,version", + [ + ("a", None), + ("a", 1), + ("a", 4), + ("a", 9), + ("b", 2), + ("b", None), + ("missing", None), + ("missing", 1), + ("malformed", None), + ("malformed", 7), + ], + ) + def test_get_agrees(self, key: str, version: int | None) -> None: + memory, objects = self._both() + assert memory.get_object(SKILL_OBJECT_KIND, key, version) == objects.get( + key, version + ) + + def test_snapshot_agrees(self) -> None: + memory, objects = self._both() + assert memory.all_objects(SKILL_OBJECT_KIND) == objects.snapshot() + + +# --------------------------------------------------------------------------- +# The store against the fake endpoint +# --------------------------------------------------------------------------- + + +class TestPollingAgainstTheEndpoint: + def test_a_polled_skill_becomes_retrievable_through_the_accessors( + self, endpoint: Any + ) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + with poll_store(endpoint) as store: + assert store.wait_for_skills(timeout=5) is True + raw = store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") + assert raw is not None + assert raw["version"] == 3 + + def test_the_request_carries_the_sdk_key_and_the_data_model_version( + self, endpoint: Any + ) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + first = endpoint.requests[0] + assert first["path"] == "/sdk/poll" + assert first["authorization"] == SDK_KEY + assert first["query"]["mv"] == "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: + store.wait_for_skills(timeout=5) + assert "basis" not in endpoint.requests[0]["query"] + + def test_the_basis_from_payload_transferred_is_echoed_on_the_next_request( + self, endpoint: Any + ) -> None: + endpoint.queue_poll( + full_payload(("put-object", put_skill()), state="selector-abc") + ) + endpoint.queue_poll(status=304) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + assert wait_until(lambda: len(endpoint.requests) >= 2) + assert endpoint.requests[1]["query"]["basis"] == "selector-abc" + + def test_the_basis_advances_across_successive_payloads(self, endpoint: Any) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill()), state="basis-1")) + endpoint.queue_poll( + events( + ("server-intent", server_intent("xfer-changes")), + ("put-object", put_skill("second")), + ("payload-transferred", transferred("basis-2")), + ) + ) + endpoint.queue_poll(status=304) + with poll_store(endpoint): + assert wait_until(lambda: len(endpoint.requests) >= 3) + bases = [r["query"].get("basis") for r in endpoint.requests[:3]] + assert bases == [None, "basis-1", "basis-2"] + + def test_an_etag_is_returned_as_if_none_match(self, endpoint: Any) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill())), etag='W/"v1"') + endpoint.queue_poll(status=304) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + assert wait_until(lambda: len(endpoint.requests) >= 2) + assert endpoint.requests[1]["if_none_match"] == 'W/"v1"' + + def test_a_304_keeps_the_held_content(self, endpoint: Any) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill())), etag='W/"v1"') + endpoint.queue_poll(status=304) + endpoint.queue_poll(status=304) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + assert wait_until(lambda: len(endpoint.requests) >= 3) + assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None + assert store.diagnostics.payloads_transferred == 1 + assert store.failed is None + + def test_a_304_before_any_payload_still_releases_wait_for_skills( + self, endpoint: Any + ) -> None: + """A reconnect with a cached basis has nothing to transfer; boot must not + block on a payload the server has no reason to send.""" + endpoint.queue_poll(status=304) + with poll_store(endpoint) as store: + assert store.wait_for_skills(timeout=5) is True + + def test_a_mixed_payload_over_the_wire_yields_only_the_skill( + self, endpoint: Any + ) -> None: + endpoint.queue_poll( + full_payload( + ("put-object", put_flag("flag-a")), + ("put-object", put_segment("beta")), + ("put-object", put_skill("pdf-extraction")), + ("put-object", put_flag("flag-b")), + ) + ) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + held = store.all_objects(SKILL_OBJECT_KIND) + assert len(held) == 1 + assert next(iter(held.values()))["key"] == "pdf-extraction" + assert store.diagnostics.objects_ignored == 3 + + def test_a_revocation_over_the_wire_removes_the_skill(self, endpoint: Any) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + endpoint.queue_poll( + events( + ("server-intent", server_intent("xfer-changes")), + ("delete-object", delete_skill()), + ("payload-transferred", transferred("basis-2")), + ) + ) + endpoint.queue_poll(status=304) + with poll_store(endpoint) as store: + assert wait_until( + lambda: ( + store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is None + and store.diagnostics.objects_revoked == 1 + ) + ) + + def test_the_store_asks_for_only_the_kind_it_serves(self, endpoint: Any) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + assert store.get_object("flag", "pdf-extraction") is None + assert store.all_objects("flag") == {} + + +class TestStreamingAgainstTheEndpoint: + def test_a_streamed_payload_lands(self, endpoint: Any) -> None: + 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", initial_backoff=0.01 + ) + try: + store.start() + assert store.wait_for_skills(timeout=5) is True + assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None + finally: + store.close() + + def test_the_stream_request_advertises_event_stream(self, endpoint: Any) -> None: + 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]["path"] == "/sdk/stream" + assert endpoint.requests[0]["accept"] == "text/event-stream" + + def test_a_streamed_revocation_arrives_without_a_restart( + self, endpoint: Any + ) -> None: + endpoint.hold_stream_open = True + endpoint.queue_stream( + full_payload(("put-object", put_skill())) + + events( + ("server-intent", server_intent("xfer-changes")), + ("delete-object", delete_skill()), + ("payload-transferred", transferred("basis-2")), + ) + ) + store = FDv2SkillStore(SDK_KEY, base_uri=endpoint.base_uri, mode="stream") + try: + store.start() + assert wait_until( + lambda: ( + store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is None + and store.diagnostics.objects_revoked == 1 + ) + ) + finally: + store.close() + + def test_a_dropped_stream_reconnects_with_the_basis_it_reached( + self, endpoint: Any + ) -> None: + endpoint.queue_stream( + full_payload(("put-object", put_skill()), state="basis-1") + ) + endpoint.queue_stream(events(("heart-beat", None))) + store = FDv2SkillStore( + SDK_KEY, + base_uri=endpoint.base_uri, + mode="stream", + initial_backoff=0.01, + max_backoff=0.05, + ) + try: + store.start() + assert wait_until(lambda: len(endpoint.requests) >= 2) + finally: + store.close() + assert endpoint.requests[1]["query"]["basis"] == "basis-1" + + def test_close_returns_promptly_while_a_stream_is_open(self, endpoint: Any) -> None: + """ + The delivery thread is blocked in a socket read that no stop flag can + reach, so ``close`` closes the connection under it. Without that, every + shutdown of a healthy stream waits out the join timeout. + """ + 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") + store.start() + assert store.wait_for_skills(timeout=5) is True + started = time.monotonic() + store.close(timeout=5.0) + assert time.monotonic() - started < 1.0 + + def test_an_interrupted_stream_is_not_reported_as_a_failure( + self, endpoint: Any + ) -> None: + 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") + store.start() + store.wait_for_skills(timeout=5) + store.close() + assert store.failed is None + + def test_content_survives_a_reconnect(self, endpoint: Any) -> None: + endpoint.queue_stream(full_payload(("put-object", put_skill()))) + endpoint.queue_stream(events(("heart-beat", None))) + store = FDv2SkillStore( + SDK_KEY, base_uri=endpoint.base_uri, mode="stream", initial_backoff=0.01 + ) + try: + store.start() + assert wait_until(lambda: len(endpoint.requests) >= 2) + assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None + finally: + store.close() + + +# --------------------------------------------------------------------------- +# Failure handling +# --------------------------------------------------------------------------- + + +class _ScriptedConnection: + """Stands in for ``_StreamConnection``: an event iterator plus a close.""" + + def __init__(self, payload_events: Any) -> None: + self.events = iter(payload_events) + self.closed = False + + def close(self) -> None: + self.closed = True + + +class _ScriptedRequester: + """Raises a scripted sequence, so backoff is asserted without real sockets.""" + + def __init__(self, *outcomes: Any) -> None: + self.outcomes = list(outcomes) + self.calls: list[tuple[str | None, str | None]] = [] + + def poll(self, basis: str | None, etag: str | None) -> Any: + self.calls.append((basis, etag)) + outcome = ( + self.outcomes.pop(0) if self.outcomes else _RecoverableTransportError("x") + ) + if isinstance(outcome, Exception): + raise outcome + return outcome + + def stream(self, basis: str | None) -> Any: + self.calls.append((basis, None)) + outcome = ( + self.outcomes.pop(0) if self.outcomes else _RecoverableTransportError("x") + ) + if isinstance(outcome, Exception): + raise outcome + return _ScriptedConnection(outcome) + + +class TestFailureHandling: + def test_a_403_stops_delivery_and_names_the_protocol_control_flag( + self, endpoint: Any, caplog: Any + ) -> None: + endpoint.queue_poll(status=403) + with caplog.at_level("ERROR"): + with poll_store(endpoint) as store: + assert wait_until(lambda: store.failed is not None) + assert "403" in store.failed + assert "fdv2-protocol-control" in store.failed + assert any("fdv2-protocol-control" in r.getMessage() for r in caplog.records) + + def test_a_401_stops_delivery(self, endpoint: Any) -> None: + endpoint.queue_poll(status=401) + with poll_store(endpoint) as store: + assert wait_until(lambda: store.failed is not None) + assert "401" in store.failed + + def test_a_fatal_failure_releases_wait_for_skills_rather_than_hanging( + self, endpoint: Any + ) -> None: + endpoint.queue_poll(status=401) + with poll_store(endpoint) as store: + assert store.wait_for_skills(timeout=5) is True + assert store.failed is not None + + def test_a_fatal_failure_keeps_last_known_good_servable( + self, endpoint: Any + ) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + endpoint.queue_poll(status=403) + with poll_store(endpoint) as store: + assert wait_until(lambda: store.failed is not None) + assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None + + def test_a_500_is_retried(self, endpoint: Any) -> None: + endpoint.queue_poll(status=500) + endpoint.queue_poll(status=503) + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + with poll_store(endpoint) as store: + assert store.wait_for_skills(timeout=5) is True + assert store.failed is None + assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None + + def test_a_retry_resets_the_failure_count_on_success(self, endpoint: Any) -> None: + endpoint.queue_poll(status=500) + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + endpoint.queue_poll(status=304) + with poll_store(endpoint) as store: + assert store.wait_for_skills(timeout=5) + assert wait_until(lambda: store.diagnostics.connection_failures == 0) + + def test_retries_are_bounded(self) -> None: + store = FDv2SkillStore( + SDK_KEY, + mode="poll", + poll_interval=0.01, + initial_backoff=0.001, + max_backoff=0.002, + max_consecutive_failures=3, + _requester=_ScriptedRequester(), + ) + try: + store.start() + assert wait_until(lambda: store.failed is not None) + assert "3 consecutive failures" in store.failed or "gave up" in store.failed + finally: + store.close() + + def test_a_retry_after_header_is_honoured(self) -> None: + requester = _ScriptedRequester( + _RecoverableTransportError("slow down", retry_after=0.25), + ) + store = FDv2SkillStore( + SDK_KEY, + mode="poll", + poll_interval=10.0, + initial_backoff=5.0, + _requester=requester, + ) + try: + started = time.monotonic() + store.start() + assert wait_until(lambda: len(requester.calls) >= 2, timeout=3) + elapsed = time.monotonic() - started + # The server asked for 0.25s; our own backoff would have been 5s. + assert 0.2 <= elapsed < 3.0 + finally: + store.close() + + def test_a_retry_after_header_is_parsed_off_the_wire(self, endpoint: Any) -> None: + endpoint.queue_poll(status=429, retry_after="0") + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + with poll_store(endpoint, initial_backoff=5.0) as store: + # If Retry-After were ignored the 5s backoff would blow the timeout. + assert store.wait_for_skills(timeout=3) is True + + def test_backoff_is_exponential_and_capped(self) -> None: + assert backoff_delay(1, base=1.0, maximum=30.0, jitter=0.0) == 1.0 + assert backoff_delay(2, base=1.0, maximum=30.0, jitter=0.0) == 2.0 + assert backoff_delay(3, base=1.0, maximum=30.0, jitter=0.0) == 4.0 + assert backoff_delay(20, base=1.0, maximum=30.0, jitter=0.0) == 30.0 + + def test_jitter_never_exceeds_the_cap(self) -> None: + for attempt in range(1, 12): + for _ in range(50): + assert 0.0 <= backoff_delay(attempt, base=1.0, maximum=5.0) <= 5.0 + + def test_a_malformed_polling_envelope_is_recoverable_not_fatal( + self, endpoint: Any + ) -> None: + store = FDv2SkillStore( + SDK_KEY, + mode="poll", + poll_interval=0.01, + initial_backoff=0.001, + _requester=_ScriptedRequester( + _RecoverableTransportError("polling response had no 'events' array") + ), + ) + try: + store.start() + assert wait_until(lambda: store.diagnostics.connection_failures >= 1) + assert store.failed is None + finally: + store.close() + + def test_a_listener_that_raises_does_not_kill_delivery(self, endpoint: Any) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill("first")))) + endpoint.queue_poll( + events( + ("server-intent", server_intent("xfer-changes")), + ("put-object", put_skill("second")), + ("payload-transferred", transferred("basis-2")), + ) + ) + endpoint.queue_poll(status=304) + with poll_store(endpoint) as store: + store.add_listener(SKILL_OBJECT_KIND, lambda _raw: 1 / 0) + assert wait_until( + lambda: store.get_object(SKILL_OBJECT_KIND, "second") is not None + ) + assert store.failed is None + + +# --------------------------------------------------------------------------- +# The contentHash gap +# --------------------------------------------------------------------------- + + +class TestMissingContentHash: + """ + The blocking backend gap, asserted as behaviour rather than assumed. + + An envelope with no ``contentHash`` must produce a *withheld* skill with the + ``missing_content_hash`` reason — loudly, diagnosably, and without a crash. + There is deliberately no fallback that skips verification: a hash the SDK + computed from the content it was handed would certify the content against + itself and verify nothing. + """ + + async def test_a_hashless_skill_is_withheld_with_the_right_reason( + self, endpoint: Any + ) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill(omit_hash=True)))) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + await init_client(options={"skillStore": store}, client=object()) + + outcome = await get_skill_result("pdf-extraction") + assert outcome.skill is None + assert outcome.reason == "integrity_failure" + assert await get_skill("pdf-extraction") is None + assert await all_skills() == [] + + async def test_the_object_is_still_held_so_the_outcome_is_not_absent( + self, endpoint: Any + ) -> None: + """ + Holding it is what makes the failure diagnosable. Dropping it at the + transport would report ``absent`` — indistinguishable from "no such + skill" — and would additionally let a prune delete the last known-good + copy already on disk. + """ + endpoint.queue_poll(full_payload(("put-object", put_skill(omit_hash=True)))) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + raw = store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") + assert raw is not None + assert "contentHash" not in raw + await init_client(options={"skillStore": store}, client=object()) + assert (await get_skill_result("pdf-extraction")).reason != "absent" + + def test_the_store_counts_hashless_objects(self, endpoint: Any) -> None: + endpoint.queue_poll( + full_payload( + ("put-object", put_skill("a", omit_hash=True)), + ("put-object", put_skill("b", omit_hash=True)), + ("put-object", put_skill("c")), + ) + ) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + assert store.diagnostics.hashless_objects == 2 + assert store.diagnostics.skill_objects_received == 3 + + def test_a_hashless_object_logs_an_error_naming_the_reason_code( + self, endpoint: Any, caplog: Any + ) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill(omit_hash=True)))) + with caplog.at_level("ERROR"): + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + rendered = "\n".join(r.getMessage() for r in caplog.records) + assert "missing_content_hash" in rendered + assert "pdf-extraction" in rendered + assert "contentHash" in rendered + + def test_a_wholly_hashless_payload_says_so_once( + self, endpoint: Any, caplog: Any + ) -> None: + endpoint.queue_poll( + full_payload( + ("put-object", put_skill("a", omit_hash=True)), + ("put-object", put_skill("b", omit_hash=True)), + ) + ) + with caplog.at_level("ERROR"): + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + summaries = [ + r + for r in caplog.records + if "No skill content will resolve" in r.getMessage() + ] + assert len(summaries) == 1 + assert "All 2 skill object(s)" in summaries[0].getMessage() + + def test_a_partly_hashed_payload_does_not_claim_total_failure( + self, endpoint: Any, caplog: Any + ) -> None: + endpoint.queue_poll( + full_payload( + ("put-object", put_skill("a", omit_hash=True)), + ("put-object", put_skill("b")), + ) + ) + with caplog.at_level("ERROR"): + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + rendered = "\n".join(r.getMessage() for r in caplog.records) + assert "No skill content will resolve" not in rendered + + async def test_a_hash_that_does_not_match_is_a_different_failure( + self, endpoint: Any + ) -> None: + """``missing_content_hash`` and ``hash_mismatch`` must not collapse: one is + a backend gap and the other is possible tampering.""" + endpoint.queue_poll( + full_payload( + ("put-object", put_skill(content_hash=_hash("something else"))) + ) + ) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + assert store.diagnostics.hashless_objects == 0 + await init_client(options={"skillStore": store}, client=object()) + assert ( + await get_skill_result("pdf-extraction") + ).reason == "integrity_failure" + + async def test_a_hashed_skill_resolves_end_to_end(self, endpoint: Any) -> None: + """The positive control: everything above is a gap, not a broken adapter.""" + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + await init_client(options={"skillStore": store}, client=object()) + + skill = await get_skill("pdf-extraction") + assert skill is not None + assert skill.key == "pdf-extraction" + assert skill.version == 3 + assert skill.content == SKILL_BODY.encode("utf-8") + assert skill.content_hash == _hash(SKILL_BODY) + assert skill.name == "PDF Extraction" + + async def test_a_pinned_reference_resolves_to_the_pinned_object_version( + self, endpoint: Any + ) -> None: + endpoint.queue_poll( + full_payload( + ("put-object", put_skill(object_version=2, content="v2 body")), + ("put-object", put_skill(object_version=5, content="v5 body")), + ) + ) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + await init_client(options={"skillStore": store}, client=object()) + + pinned = await get_skill("pdf-extraction", version=2) + assert pinned is not None + assert pinned.content == b"v2 body" + newest = await get_skill("pdf-extraction") + assert newest is not None + assert newest.version == 5 + + async def test_the_payload_version_is_not_resolvable_as_a_skill_version( + self, endpoint: Any + ) -> None: + """ + The end-to-end form of the ``objectVersion``/``version`` assertion. + + Asking for the payload version resolves nothing — reported ``absent``, + because the store answers "I hold no such version" rather than answering + with the wrong one. The version that *does* resolve is ``objectVersion``. + """ + endpoint.queue_poll( + full_payload( + ("put-object", put_skill(object_version=3, payload_version=42)) + ) + ) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + await init_client(options={"skillStore": store}, client=object()) + by_payload_version = await get_skill_result("pdf-extraction", version=42) + assert by_payload_version.skill is None + assert by_payload_version.reason == "absent" + assert await get_skill("pdf-extraction", version=3) is not None + + +# --------------------------------------------------------------------------- +# Server-side only +# --------------------------------------------------------------------------- + + +class TestServerSideOnly: + def test_a_mobile_key_is_refused(self) -> None: + with pytest.raises(ValueError, match="mobile key"): + FDv2SkillStore("mob-00000000-0000-4000-8000-000000000000") + + def test_a_client_side_environment_id_is_refused(self) -> None: + with pytest.raises(ValueError, match="client-side"): + FDv2SkillStore("0123456789abcdef01234567") + + def test_an_empty_credential_is_refused(self) -> None: + with pytest.raises(ValueError, match="server-side SDK key"): + FDv2SkillStore(" ") + + def test_a_server_side_key_is_accepted(self) -> None: + assert FDv2SkillStore(SDK_KEY) is not None + + def test_an_unrecognised_credential_shape_warns_but_is_allowed( + self, caplog: Any + ) -> None: + """Private instances and test doubles issue keys without the public prefix.""" + with caplog.at_level("WARNING"): + FDv2SkillStore("my-private-instance-credential") + assert any("server-side SDK key" in r.message for r in caplog.records) + + def test_an_unknown_mode_is_refused(self) -> None: + with pytest.raises(ValueError, match="stream"): + FDv2SkillStore(SDK_KEY, mode="mobile") # type: ignore[arg-type] + + +# --------------------------------------------------------------------------- +# The eager re-reconcile +# --------------------------------------------------------------------------- + + +class TestWatchSkills: + async def test_a_revocation_prunes_without_a_restart( + self, endpoint: Any, tmp_path: Any + ) -> None: + """ + AV-1, closed at this layer. The store's change listener drives the + reconcile, so the file goes away seconds after the ``delete-object`` + rather than at the next process start. + """ + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + endpoint.queue_poll( + events( + ("server-intent", server_intent("xfer-changes")), + ("delete-object", delete_skill()), + ("payload-transferred", transferred("basis-2")), + ) + ) + endpoint.queue_poll(status=304) + + with poll_store(endpoint, poll_interval=0.2) as store: + store.wait_for_skills(timeout=5) + await init_client(options={"skillStore": store}, client=object()) + report, watcher = await watch_skills( + "*", tmp_path / "skills", debounce=0.05 + ) + try: + written = tmp_path / "skills" / "pdf-extraction" / "SKILL.md" + assert written.exists() + assert any(a.action == "written" for a in report.actions) + assert wait_until(lambda: not written.exists(), timeout=10) + finally: + watcher.close() + + async def test_a_new_version_is_rewritten_without_a_restart( + self, endpoint: Any, tmp_path: Any + ) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill(content="first")))) + endpoint.queue_poll( + events( + ("server-intent", server_intent("xfer-full")), + ("put-object", put_skill(object_version=4, content="second")), + ("payload-transferred", transferred("basis-2")), + ) + ) + endpoint.queue_poll(status=304) + + with poll_store(endpoint, poll_interval=0.2) as store: + store.wait_for_skills(timeout=5) + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + try: + written = tmp_path / "s" / "pdf-extraction" / "SKILL.md" + assert written.read_text() == "first" + assert wait_until(lambda: written.read_text() == "second", timeout=10) + finally: + watcher.close() + + async def test_a_burst_of_changes_coalesces_into_few_reconciles( + self, endpoint: Any, tmp_path: Any + ) -> None: + endpoint.queue_poll( + full_payload(*[("put-object", put_skill(f"skill-{i}")) for i in range(12)]) + ) + endpoint.queue_poll(status=304) + with poll_store(endpoint) as store: + store.wait_for_skills(timeout=5) + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.1) + try: + time.sleep(0.5) + # Twelve objects committed in one payload fire twelve listener + # calls; without coalescing that is twelve reconciles of one root. + assert watcher.reconciles <= 2 + finally: + watcher.close() + + async def test_the_default_keeps_last_known_good_during_an_outage( + self, endpoint: Any, tmp_path: Any + ) -> None: + """``on_unavailable="keep"`` is the endorsed default: an outage must not + read as "everything was revoked".""" + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + endpoint.queue_poll(status=500) + with poll_store(endpoint, poll_interval=0.05) as store: + store.wait_for_skills(timeout=5) + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + try: + written = tmp_path / "s" / "pdf-extraction" / "SKILL.md" + assert written.exists() + # ``last_error`` rather than ``connection_failures``: the counter + # resets on the next successful poll, so asserting on it races + # the retry that is supposed to happen. + assert wait_until( + lambda: store.diagnostics.last_error is not None, timeout=10 + ) + time.sleep(0.3) + assert written.exists() + finally: + watcher.close() + + async def test_a_store_with_no_listener_support_is_refused_loudly( + self, tmp_path: Any + ) -> None: + class NoListeners: + def get_object(self, *_a: Any, **_k: Any) -> None: + return None + + def all_objects(self, _kind: str) -> dict[str, Any]: + return {} + + await init_client(options={"skillStore": NoListeners()}, client=object()) + with pytest.raises(RuntimeError, match="add_listener"): + await watch_skills("*", tmp_path / "s") + + async def test_no_store_configured_raises(self, tmp_path: Any) -> None: + with pytest.raises(RuntimeError, match="configured skill store"): + await watch_skills("*", tmp_path / "s") + + async def test_the_in_memory_store_can_also_drive_a_watch( + self, tmp_path: Any + ) -> None: + """The watcher is wired to the seam, not to the FDv2 store.""" + store = InMemorySkillStore() + store.put( + { + "key": "a", + "version": 1, + "content": "body", + "contentHash": _hash("body"), + } + ) + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + try: + written = tmp_path / "s" / "a" / "SKILL.md" + assert written.read_text() == "body" + store.put( + { + "key": "a", + "version": 2, + "content": "new body", + "contentHash": _hash("new body"), + } + ) + assert wait_until(lambda: written.read_text() == "new body", timeout=10) + finally: + watcher.close() + + +# --------------------------------------------------------------------------- +# Lifecycle +# --------------------------------------------------------------------------- + + +class TestLifecycle: + def test_start_is_idempotent(self, endpoint: Any) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + store = poll_store(endpoint) + try: + assert store.start() is store + assert store.start() is store + assert store.wait_for_skills(timeout=5) + finally: + store.close() + + def test_close_is_idempotent(self, endpoint: Any) -> None: + store = poll_store(endpoint) + store.start() + store.close() + store.close() + + def test_a_closed_store_still_answers_from_what_it_received( + self, endpoint: Any + ) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + store = poll_store(endpoint) + store.start() + store.wait_for_skills(timeout=5) + store.close() + assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None + + def test_wait_for_skills_times_out_rather_than_hanging(self) -> None: + store = FDv2SkillStore( + SDK_KEY, mode="poll", poll_interval=60, _requester=_ScriptedRequester() + ) + try: + assert store.wait_for_skills(timeout=0.05) is False + finally: + store.close() + + def test_the_store_satisfies_the_seam_before_it_starts(self) -> None: + store = FDv2SkillStore(SDK_KEY) + assert store.get_object(SKILL_OBJECT_KIND, "anything") is None + assert store.all_objects(SKILL_OBJECT_KIND) == {} From bad7ecd591b89abc9fb5cc9abf7a1b26f94b9300 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Fri, 4 Sep 2026 11:16:41 -0400 Subject: [PATCH 02/13] test(client): assert the retry bound exactly, matching the TS port Co-Authored-By: Claude Opus 5 --- packages/client/tests/test_skills_fdv2.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index 310be91..1ccf53a 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -1094,7 +1094,9 @@ def test_retries_are_bounded(self) -> None: try: store.start() assert wait_until(lambda: store.failed is not None) - assert "3 consecutive failures" in store.failed or "gave up" in store.failed + # Four, not three: the bound is the number of failures *tolerated*, + # so the run that exceeds it is the one that gives up. + assert "gave up after 4 consecutive failures" in store.failed finally: store.close() From bc579347abcea85efc0bd6596a7084b0829e402e Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Wed, 9 Sep 2026 12:32:26 -0400 Subject: [PATCH 03/13] docs(client): make the Agent Skills transport comments customer-facing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This repo is public and customers read the source while consuming the SDK, so the comments, docstrings, and user-visible log strings in the FDv2 transport are reworked to be instructional rather than internal. Removes internal references: the abandoned private-route paragraph, the ticket number and internal setting name that appeared in two log strings customers see (the missing-contentHash error and the HTTP 403 advice, both of which now point at LaunchDarkly support), and the review/design-doc pointers in skills_watch. Removes development history: the "an earlier design did X", "the transport was replaced wholesale", and "deferred to a later phase" framing. Replaces internal jargon with formal terms — "seam" becomes "interface" throughout, along with "duck-typed", "load-bearing", "belt and braces", "blip", and "smuggle in". The 403 test asserted on the internal setting name in the message and now asserts on the replacement wording; TestSeamParity is renamed to TestInterfaceParity. No behaviour changes. Co-Authored-By: Claude Opus 5 --- packages/client/README.md | 10 +- packages/client/agents.md | 22 +-- .../src/launchdarkly_ai_server/skills_fdv2.py | 174 ++++++++---------- .../launchdarkly_ai_server/skills_watch.py | 31 ++-- packages/client/tests/test_skills_fdv2.py | 47 ++--- 5 files changed, 127 insertions(+), 157 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 50032d6..ccae5b3 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -469,8 +469,8 @@ finally: ``` **Nothing above the store changes.** The accessors, integrity verification, and -`write_skills` are transport-agnostic: they see raw objects through the `SkillStore` seam and -cannot tell which store produced them. Everything documented above about verification and +`write_skills` are transport-agnostic: they see raw objects through the `SkillStore` +interface and cannot tell which store produced them. Everything documented above about verification and reconcile semantics applies unchanged. **Server-side only.** Skills are for server-side agent runtimes and skill content is @@ -491,9 +491,9 @@ They are skipped, not evaluated — this store does no evaluation of any kind > **Beta caveats, worth knowing before you deploy.** Payload signing does not exist on this > channel yet, so delivery is TLS-only and the content hash establishes self-consistency, not -> origin authenticity. FDv2 is opt-in per account: without it the endpoints return HTTP 403, -> which the store reports as a fatal error naming the setting. `ld-relay` does not speak the -> FDv2 endpoints, so relay-only deployments cannot receive skills. +> origin authenticity. The FDv2 protocol is opt-in per account: without it the endpoints +> return HTTP 403, which the store reports as a fatal error explaining what to do. `ld-relay` +> does not speak the FDv2 endpoints, so relay-only deployments cannot receive skills. **If every skill comes back empty, check `diagnostics.hashless_objects`.** Verification requires `contentHash` on the delivered object and withholds anything without one, so a diff --git a/packages/client/agents.md b/packages/client/agents.md index 73e81d5..abac360 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -30,7 +30,7 @@ No other `launchdarkly-ai-*` package may define or duplicate these. They import | `src/launchdarkly_ai_server/types_validation.py` | `parse_ai_config` — validates flag variation shape; `is_valid_skill_key` / `is_valid_skill_version` / `skill_key_rejection_reason` (the canonical key-grammar explanation every layer quotes) | | `src/launchdarkly_ai_server/skills.py` | Agent Skills, retrieval half — `skill_refs`, `get_skill`/`get_skills`/`all_skills`, `InMemorySkillStore`, and the store/telemetry injection points `_set_store` / `_set_emitter_for_testing` | | `src/launchdarkly_ai_server/skills_core.py` | Shared skills internals — the `SkillStore` seam, module state, the telemetry seam and its three recorders, integrity verification, and store resolution. Imported by both `skills.py` and the materialization layer; imports neither | -| `src/launchdarkly_ai_server/skills_fdv2.py` | Agent Skills, delivery transport — the FDv2 protocol, the `objectVersion`/`version` translation, the held object set, and `FDv2SkillStore`. Sits **below** the store seam; imports `skills_core` only, and nothing imports it | +| `src/launchdarkly_ai_server/skills_fdv2.py` | Agent Skills, delivery transport — the FDv2 protocol, the `objectVersion`/`version` translation, the held object set, and `FDv2SkillStore`. Sits **below** the store interface; imports `skills_core` only, and nothing imports it | | `src/launchdarkly_ai_server/skills_watch.py` | Agent Skills, eager re-reconcile — `watch_skills` / `SkillWatcher`, wiring the store's change listener to `write_skills`. Sits **above** `skills_fs` and modifies none of it | | `src/launchdarkly_ai_server/skills_fs.py` | Agent Skills, materialization half — `write_skills`, request resolution, the manifest format and on-disk filenames, per-skill reconcile, and pruning | | `src/launchdarkly_ai_server/safe_fs.py` | Descriptor-pinned filesystem primitives — `atomic_write`, `unlink_file`, `pinned_directory`, `open_directory_nofollow`, `open_or_create_directory`, `SymlinkRefused`, and the `*at()` capability probe. Owns the descriptor-vs-path platform split; knows nothing about skills | @@ -223,12 +223,10 @@ of skills, and the `"*"` reconcile, since `//SKILL.md` is a single pa `FDv2SkillStore` speaks LaunchDarkly's SDK-facing FDv2 channel (`GET /sdk/poll`, `GET /sdk/stream`, server-side SDK key in `Authorization`, `basis` + `mv` params, -`If-None-Match`/304). It lives below the seam and produces raw objects in the shape -`skills_core.SkillStore` documents; **nothing above the seam knows it exists**. The transport -design was replaced wholesale late in this feature's life and cost zero changes above this -line, which is the strongest evidence the seam is drawn correctly. If a transport change ever -seems to require editing an accessor, verification, or `write_skills`, the adapter boundary is -wrong. +`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. **`objectVersion` is the skill's version. `version` is the payload's.** On the wire a skill `put-object` carries both, and they are not interchangeable: @@ -239,7 +237,7 @@ wrong. "object":{"contentType":"text/markdown","content":"…","contentHash":"…","name":"…"}} ``` -`objectVersion` (3) is what a `{key, version}` reference pins and what becomes the seam's +`objectVersion` (3) is what a `{key, version}` reference pins and what becomes the stored `version`. `version` (42) is the version of the *payload* the object arrived in — it moves when anything in the environment moves, including a flag with nothing to do with skills. Reading it as the skill's version fails **silently**: the object verifies, the hash matches, @@ -249,10 +247,10 @@ two fields look interchangeable. `seam_object_from_put` is the only place the tr happens, and `TestVersionTranslation` asserts it in both directions. **Skills are identified by `kind == "inline-resource" && category == "skill"`; everything else -is ignored, not rejected.** An environment's payload assignment carries the flagging payload -alongside the agent-skill payload, so flag and segment objects arrive as a matter of course. -Erroring on an unrecognised kind is the unknown-kind reconnect loop this feature must not -reproduce — a flag-delivery outage caused by a skills rollout. +is ignored, not rejected.** 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. **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 e579ae2..c51bb4d 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -1,38 +1,26 @@ """ Agent Skills — the FDv2 delivery transport. -The store implementation that actually talks to LaunchDarkly. It sits *below* -the ``SkillStore`` seam, not above it: it produces raw wire objects in the shape +The store implementation that talks to LaunchDarkly. It sits *below* the +``SkillStore`` interface: it produces raw wire objects in the shape ``skills_core`` documents, and everything above — the accessors, integrity -verification, the ``Skill`` dataclass, materialization — is unchanged and -unaware of it. That is the whole point of the seam, and the fact that replacing -the transport design wholesale cost nothing above this line is the evidence it -was drawn in the right place. +verification, the ``Skill`` dataclass, materialization — is unaware of it. Layering:: launchdarkly_ai_server - └─ SkillStore protocol (skills_core) ── duck-typed accessor surface - └─ FDv2SkillStore (this module) ── deserialize, hold, serve - └─ the SDK-facing FDv2 channel on FDCore + └─ SkillStore protocol (skills_core) ── the interface accessors call + └─ FDv2SkillStore (this module) ── deserialise, hold, serve + └─ LaunchDarkly's SDK-facing FDv2 channel GET /sdk/poll, GET /sdk/stream, authenticated with the environment's server-side SDK key -Dependencies run one way. This module imports ``skills_core`` for the seam's -kind constant and nothing else from the feature; ``skills.py`` and +Dependencies run one way. This module imports ``skills_core`` for the +interface's kind constant and nothing else from the feature; ``skills.py`` and ``skills_fs.py`` do not import it. It uses only the standard library, so the content path adds no dependency to a package whose sole runtime dependency is ``opentelemetry-api`` and whose LaunchDarkly base-SDK dependency is optional. -**There is no bespoke private route here, deliberately.** An earlier design had -this adapter poll ``/private/flagdlv/payloads/{id}/latest/obj/skill/{key}``. -Those are gonfalon private endpoints authenticated by Cognito machine-token -OAuth scopes with no per-tenant authorization; the security review ruled out -both relaxing that auth and shipping a machine credential to a customer host. -This transport uses the genuinely SDK-facing channel instead, which is also the -channel payload signing will eventually cover. Do not reintroduce the private -route. - What this module does *not* do, on purpose: - **It does not verify content.** Verification lives at the accessor boundary in @@ -42,8 +30,8 @@ - **It does not skip verification when the wire envelope has no ``contentHash``.** See ``_SkillObjectSet.put`` and ``StoreDiagnostics``: a hashless object is stored verbatim and *withheld* by verification with - ``missing_content_hash``, and this module's job is to make that outcome loud - rather than to paper over it. + ``missing_content_hash``. Making that outcome visible is this module's job; + working around it is not. - **It does not evaluate anything.** No flags, no segments, no targeting. Skills have no targeting; the SDK key fully determines the payload. """ @@ -77,11 +65,11 @@ """ The FDv2 ``kind`` skills are delivered under. -Distinct from ``skills_core.SKILL_OBJECT_KIND`` (``"skill"``), which is the -*seam* value the SDK asks a store for. Translating this pair — kind -``inline-resource`` plus category ``skill`` — onto that single value is exactly -the adapter's job, and the reason ``SKILL_OBJECT_KIND`` is documented as a seam -string rather than as the wire contract. +Distinct from ``skills_core.SKILL_OBJECT_KIND`` (``"skill"``), which is the value +the SDK asks a store for. Translating this pair — kind ``inline-resource`` plus +category ``skill`` — onto that single value is the adapter's job, which is why +``SKILL_OBJECT_KIND`` is documented as an interface value rather than as the wire +contract. """ FDV2_OBJECT_CATEGORY = "skill" @@ -98,11 +86,8 @@ """ The ``mv`` request parameter — the SDK data model version this adapter speaks. -Overridable through ``FDv2SkillStore(data_model_version=...)`` because it is the -one request parameter this side cannot verify: the LaunchDarkly base SDK's own -FDv2 data source does not send ``mv`` at all today, and the streamer branch that -carries skills is unmerged, so the value the server expects has not been -observed. Confirm it with FDN before Beta rather than trusting this default. +Override it with ``FDv2SkillStore(data_model_version=...)`` if a LaunchDarkly +instance expects a different value. """ _EVENT_SERVER_INTENT = "server-intent" @@ -121,11 +106,10 @@ """ The skill object envelope's fields, copied through verbatim. -``contentHash`` is listed here and is the field the whole content path waits on; -see ``StoreDiagnostics``. Nothing here is coerced, defaulted, or normalised — -everything a store serves is untrusted input and is revalidated above the seam, -so a transport that "helpfully" filled in a field would be forging the very -thing verification exists to check. +Nothing here is coerced, defaulted, or normalised: everything a store serves is +untrusted input and is revalidated above the store interface, so a transport +that filled in a missing field would be forging the very thing verification +exists to check. """ Mode = Literal["stream", "poll"] @@ -149,13 +133,11 @@ def _require_server_side_credential(sdk_key: str) -> None: """ Refuses a mobile key or a client-side environment ID. - Skills are for server-side agent runtimes. The payload assignment that - carries them is shared by every auth type, so the skill payload ID is - appended for mobile and environment-ID auth too — which means a client-side - credential may well *succeed* against these endpoints and deliver - customer-confidential skill content to a client-side process. Failing here - is the SDK-side half of that boundary; excluding skills at assignment time - is the platform-side half, and is an open ask on FDN (design §3.1c). + Skills are for server-side agent runtimes, and skill content is + customer-confidential. Payload assignment is shared across credential types, + so a client-side credential may well *succeed* against these endpoints and + deliver skill content to a client-side process. Refusing one here is what + keeps that from happening. Raises ``ValueError`` rather than logging, because there is no degraded mode that is correct: a store built on the wrong credential should not exist. @@ -203,10 +185,9 @@ class StoreDiagnostics: """ What the transport has seen. Read-only from a caller's perspective. - Not part of the ``SkillStore`` seam — nothing above the seam reads this — but - the difference between "this environment has no skills" and "every skill was - withheld" is the single most confusing failure this feature can produce, and - a counter a caller can assert on beats reading logs. + Not part of the ``SkillStore`` interface — nothing above it reads this — but + "this environment has no skills" and "every skill was withheld" are easy to + mistake for each other, and a counter is easier to assert on than a log line. """ payloads_transferred: int = 0 @@ -225,8 +206,7 @@ class StoreDiagnostics: **Nonzero means skills are being withheld.** Verification withholds a hashless object with ``missing_content_hash``, so every one of these is a - skill that will never resolve. The field exists so that outcome is a number - a caller can read rather than an empty store they have to explain. + skill whose content will not resolve. """ connection_failures: int = 0 """Recoverable transport failures since the last successful transfer.""" @@ -256,10 +236,9 @@ def is_skill_event(data: Any) -> bool: categories, and flags and segments omit ``category`` entirely. Every other kind is **ignored, not rejected**. An environment's payload - assignment carries the flagging payload alongside the agent-skill payload, so - a connection delivers flag and segment objects as a matter of course. Erroring - on them would turn a normal payload into a permanent failure — which is - exactly the unknown-kind reconnect loop this feature must not reproduce. + assignment carries its flag payload alongside its agent-skill payload, so a + connection delivers flag and segment objects as a matter of course; erroring + on them would turn a normal payload into a permanent reconnect loop. """ if not isinstance(data, dict): return False @@ -271,7 +250,8 @@ def is_skill_event(data: Any) -> bool: def seam_object_from_put(data: dict[str, Any]) -> dict[str, Any] | None: """ - Translates one FDv2 skill ``put-object`` into a seam-shaped raw object. + Translates one FDv2 skill ``put-object`` into the raw object shape that the + ``SkillStore`` interface defines. ``None`` when the event cannot be filed at all — only when ``key`` is not a string, since a keyless object has no identity to store it under and no key @@ -281,10 +261,10 @@ def seam_object_from_put(data: dict[str, Any]) -> dict[str, Any] | None: indistinguishable from "no such skill" and would additionally let a prune delete the last known-good copy on disk. - **The translation this whole module exists to get right:** + **The one translation this adapter must get right:** - wire ``objectVersion`` → seam ``version`` (the skill's own version) - wire ``version`` → dropped (the *payload* version) + wire ``objectVersion`` → stored ``version`` (the skill's own version) + wire ``version`` → dropped (the *payload* version) ``objectVersion`` is what a ``{key, version}`` reference pins. ``version`` is the version of the payload the object arrived in — it changes when anything @@ -359,11 +339,10 @@ class _SkillObjectSet: Lookup semantics are deliberately identical to ``InMemorySkillStore``'s, down to the fall-through to a version-less entry, so that the store a caller configures cannot change how a pinned reference resolves. They are - reimplemented here rather than inherited because the transport needs two - operations a hand-populated store does not have — ``delete`` and the atomic - ``replace`` a full transfer requires — and reaching into another store's - privates to get them would couple the two far harder than a test that asserts - they agree. ``test_skills_fdv2.py`` carries that parity test. + reimplemented rather than inherited because the transport needs two + operations a hand-populated store does not have: ``delete``, and the atomic + ``replace`` a full transfer requires. ``test_skills_fdv2.py`` asserts that the + two stores resolve identically. Several versions of one key coexist, because they coexist in a real payload: the newest version of every skill plus every version a variation currently @@ -484,8 +463,7 @@ class _ProtocolReader: server never described, and on a full transfer it would briefly empty the store — which, with pruning on, is the difference between a reconcile and deleting a customer's skill files. Listeners therefore fire once per commit, - not once per object, which is also exactly the granularity the re-reconcile - wants. + not once per object. """ def __init__(self, committed: _SkillObjectSet) -> None: @@ -645,12 +623,11 @@ def _goodbye(self, data: Any) -> _TransferOutcome: _HASHLESS_ADVICE = ( "The delivered skill object carries no 'contentHash', so integrity " "verification withholds it with reason_code 'missing_content_hash' and its " - "content will never resolve. This is not a fault in this store and not " - "something the SDK can work around: verification hashes the verbatim bytes " - "and compares, and there is nothing to compare against. The field is " - "specified as an additive sha256-over-verbatim-UTF-8 value on the skill " - "envelope (LaunchDarkly AIC-2905) and has not shipped yet. Until it does, " - "expect an empty result from every skill accessor." + "content will not resolve. The SDK cannot work around this: verification " + "hashes the delivered bytes and compares them against the envelope's " + "'contentHash', and there is nothing to compare against. 'contentHash' is a " + "sha256 over the verbatim UTF-8 content. Contact LaunchDarkly support if " + "skills in your environment arrive without one." ) _warned_hashless: set[tuple[str, Any]] = set() @@ -662,10 +639,9 @@ def _warn_hashless(raw: dict[str, Any]) -> None: One ERROR per ``(key, version)`` whose envelope had no ``contentHash``. At ERROR rather than WARN, and per object rather than once per process, - because this is the difference between a broken deployment and an - empty-by-design one — the exact confusion the blocking gap produces. Deduped - so a re-delivered payload does not multiply it; a store that is restarted - reports again. + because an empty accessor result is otherwise indistinguishable from an + environment that has no skills. Deduped so a re-delivered payload does not + multiply it; a restarted process reports again. """ identity = (raw["key"], raw.get("version")) with _warned_lock: @@ -691,7 +667,7 @@ def _warn_if_nothing_can_verify( ``log_withholding_summary`` already reports a wholly-withheld batch at the accessor boundary, but only once a caller asks. This fires at delivery time, so the condition is visible in a process that boots, materializes nothing, - and exits — which is the shape a skills deployment fails in. + and exits, which is a common way a skills deployment fails. """ del hashless_before_this_payload # counted for the store, not for this check held = committed.all_raw() @@ -726,10 +702,9 @@ def __init__(self, message: str, retry_after: float | None = None) -> None: _FORBIDDEN_ADVICE = ( - "FDv2 is opt-in per account: the 'fdv2-protocol-control' setting defaults to " - "'forbid', which is served as HTTP 403. Skill delivery over this channel " - "needs that flag flipped for the account, and needs the FDCore/streamer " - "inline-resource support merged and deployed." + "The FDv2 protocol is opt-in per LaunchDarkly account and is served as HTTP " + "403 while it is off. Skill delivery needs it enabled; contact LaunchDarkly " + "support to enable it for your account." ) @@ -814,10 +789,9 @@ class _StreamConnection: Exists because ``close`` runs on a *different* thread from the read. The delivery thread spends nearly all its life blocked in a socket read on a - long-lived stream, where a stop flag it cannot check is no use. Without an - interruption a store's ``close`` would block for its whole join timeout on - every shutdown of a *healthy* stream — a hang in the caller's shutdown path, - paid every time. + long-lived stream, where a stop flag it cannot check is of no use; without an + interruption, closing a *healthy* stream would block the caller's shutdown + path for the whole join timeout. """ def __init__(self, response: Any) -> None: @@ -846,7 +820,7 @@ class _Requester: Standard library only, on purpose: this package's sole runtime dependency is ``opentelemetry-api`` and its LaunchDarkly base-SDK dependency is optional, so - the content path must not smuggle in an HTTP client. + the content path must not add an HTTP client dependency. """ def __init__( @@ -1055,13 +1029,13 @@ class FDv2SkillStore: **Server-side only.** Skills are for server-side agent runtimes and skill content is customer-confidential. A mobile key or a client-side environment - ID is refused in the constructor — see ``_require_server_side_credential``. + ID is refused in the constructor. **Delivery is in the background; retrieval is not.** ``SkillStore`` is a - synchronous seam, so a daemon thread owns the connection and fills memory, - and ``get_object`` only ever reads what has already arrived. Nothing here - blocks a retrieval on the network. The corollary is that a process which - calls ``get_skill`` immediately after ``start()`` may see an empty store; + synchronous interface, so a daemon thread owns the connection and fills + memory, and ``get_object`` only ever reads what has already arrived. Nothing + here blocks a retrieval on the network. The corollary is that a process + which calls ``get_skill`` immediately after ``start()`` may see an empty store; ``wait_for_skills`` is how you order boot against the first payload. **Last known good survives an outage.** A transport failure never empties the @@ -1071,7 +1045,7 @@ class FDv2SkillStore: **What arrives is untrusted.** This store holds raw wire objects verbatim and verifies nothing — integrity verification lives at the accessor boundary so - it applies to every store equally. In particular an object with no + it applies to every store equally. In particular, an object with no ``contentHash`` is held and then *withheld* by verification; see ``StoreDiagnostics.hashless_objects``. """ @@ -1093,10 +1067,10 @@ def __init__( ) -> None: """ *mode* is ``"stream"`` by default. Prefer it: a ``delete-object`` reaches a - live stream in seconds, which is what makes revocation seconds-latent - instead of restart-latent, and is why the change-listener re-reconcile is - worth wiring at all. ``"poll"`` exists for environments that cannot hold a - long-lived connection, and revocation there is one ``poll_interval`` late. + live stream in seconds, so a revoked skill stops resolving in seconds + rather than at the next restart. ``"poll"`` exists for environments that + cannot hold a long-lived connection, and revocation there is one + ``poll_interval`` late. *max_consecutive_failures* bounds the retry loop. On exceeding it the transport stops, logs an error, and the store keeps serving last known @@ -1208,7 +1182,7 @@ def diagnostics(self) -> StoreDiagnostics: with self._lock: return StoreDiagnostics(**vars(self._reader.diagnostics)) - # -- the SkillStore seam ---------------------------------------------- + # -- the SkillStore interface ----------------------------------------- def get_object( self, kind: str, key: str, version: int | None = None @@ -1297,7 +1271,7 @@ def _run(self) -> None: if self._stop.wait(delay): return continue - except Exception as exc: # pragma: no cover - belt and braces + except Exception as exc: # pragma: no cover - defensive self._give_up(f"unexpected error in skill delivery: {exc!r}") logger.error("Unexpected error in skill delivery", exc_info=True) return @@ -1316,7 +1290,7 @@ def _give_up(self, reason: str) -> None: reason, ) # Unblock anyone waiting on a first payload that is never coming, rather - # than making them eat the full timeout. + # than making them wait out the full timeout. self._first_payload.set() def _apply(self, name: str, data: Any) -> _TransferOutcome: @@ -1368,8 +1342,8 @@ def _stream_once(self) -> None: raise _RecoverableTransportError(outcome.disconnect) except Exception: if self._stop.is_set(): - # `close` interrupted the read on purpose; unwinding quietly is - # the point, not a failure to report or retry. + # `close` interrupted the read on purpose: unwind quietly rather + # than reporting a delivery failure and retrying. return raise finally: diff --git a/packages/client/src/launchdarkly_ai_server/skills_watch.py b/packages/client/src/launchdarkly_ai_server/skills_watch.py index f7b5e2b..d14d69a 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_watch.py +++ b/packages/client/src/launchdarkly_ai_server/skills_watch.py @@ -2,22 +2,18 @@ Agent Skills — re-reconcile on delivery, so revocation does not wait for a restart. ``write_skills`` is a one-shot reconcile: it materializes what the store holds -now. That was the whole story while the only transport was a hand-populated -store, and the design accordingly deferred an eager re-reconcile — revocation -would take effect at the next process restart, which the security review filed -as AV-1. +now. With a hand-populated store that is sufficient, and a revocation takes +effect at the next process restart. A streaming FDv2 connection changes the premise. A ``delete-object`` reaches a -live connection in **seconds**, and the store already publishes a change -listener, so the gap between "LaunchDarkly revoked this skill" and "its -``SKILL.md`` is off the agent's disk" collapses from a process lifetime to a -debounce interval. That is the single largest resilience improvement available -at this layer, which is why it is here rather than in a later phase. +live connection in **seconds**, and the store publishes a change listener, so +wiring the two together collapses the gap between "LaunchDarkly revoked this +skill" and "its ``SKILL.md`` is off the agent's disk" from a process lifetime to +a debounce interval. -``on_unavailable="keep"`` stays the default, deliberately and per the review: an -outage must not read as "everything was revoked". A watcher that pruned on a -failed retrieval would convert every transport blip into deletion of a -customer's skill files. +``on_unavailable="keep"`` stays the default: an outage must not read as +"everything was revoked". A watcher that pruned on a failed retrieval would +convert every transport failure into deletion of a customer's skill files. Layering: this module sits *above* ``skills_fs`` and calls ``write_skills`` without modifying it. Nothing in the reconcile, the accessors, or verification @@ -221,10 +217,11 @@ async def watch_skills( files within ``debounce`` of arriving, rather than at the next restart. Requires a store that implements the optional ``add_listener`` half of the - seam. Raises ``RuntimeError`` when no store is configured, and when the - configured store has no ``add_listener`` — the second case failing loudly - rather than degrading to a one-shot reconcile, because a watcher that - silently never fires looks exactly like a watcher whose skills never changed. + ``SkillStore`` interface. Raises ``RuntimeError`` when no store is configured, + and when the configured store has no ``add_listener`` — the second case + failing loudly rather than degrading to a one-shot reconcile, because a + watcher that silently never fires looks exactly like a watcher whose skills + never changed. """ store = get_store() if store is None: diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index 1ccf53a..c33582d 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -8,8 +8,7 @@ ``mv`` query parameters, ``Authorization``, ``If-None-Match``/304, the ``{"events": [...]}`` polling envelope, and SSE for streaming. The store under test opens real sockets against it, so request construction and header - handling are exercised rather than mocked. This is what stands in for a live - server while the backend work is unmerged. + handling are exercised rather than mocked. - **The protocol reader driven directly.** Wire semantics — which objects are skills, ``objectVersion`` versus ``version``, revocation, mixed payloads — are asserted against ``_ProtocolReader``, which has no I/O, so those cases read as @@ -76,7 +75,7 @@ def put_skill( omit_hash: bool = False, name: str = "PDF Extraction", ) -> dict[str, Any]: - """One skill ``put-object`` event's data, exactly as §2.3 specifies it.""" + """One skill ``put-object`` event's data, in the shape the wire delivers it.""" envelope: dict[str, Any] = { "contentType": "text/markdown", "content": content, @@ -361,7 +360,7 @@ def test_a_segment_is_not_a_skill(self) -> None: assert is_skill_event(put_segment()) is False def test_inline_resource_of_another_category_is_not_a_skill(self) -> None: - """``inline-resource`` is a broad kind; the category is load-bearing.""" + """``inline-resource`` is a broad kind, so the category is required too.""" other = put_skill() other["category"] = "prompt-template" assert is_skill_event(other) is False @@ -382,7 +381,7 @@ def test_non_dict_events_are_not_skills(self, value: Any) -> None: # --------------------------------------------------------------------------- -# objectVersion is not version. This is the whole ballgame. +# objectVersion is not version # --------------------------------------------------------------------------- @@ -585,7 +584,7 @@ def test_a_delete_for_one_version_leaves_the_other_held(self) -> None: def test_flag_and_segment_objects_are_skipped_cleanly(self) -> None: """ The mixed payload is the normal case, not an edge one: an environment's - assignment carries the flagging payload alongside the agent-skill payload. + assignment carries its flag payload alongside its agent-skill payload. """ held = _SkillObjectSet() reader = _ProtocolReader(held) @@ -607,8 +606,9 @@ def test_flag_and_segment_objects_are_skipped_cleanly(self) -> None: def test_an_unknown_kind_is_ignored_rather_than_fatal(self) -> None: """ - Erroring here is the unknown-kind reconnect loop this feature must not - reproduce — a flag-delivery outage caused by a skills rollout. + Erroring on an unrecognised kind would turn a normal payload into a + permanent reconnect loop — a flag-delivery outage caused by a skills + rollout. """ held = _SkillObjectSet() reader = _ProtocolReader(held) @@ -692,11 +692,11 @@ def test_an_object_arriving_with_no_intent_is_treated_as_a_delta(self) -> None: # --------------------------------------------------------------------------- -# Seam parity with InMemorySkillStore +# Interface parity with InMemorySkillStore # --------------------------------------------------------------------------- -class TestSeamParity: +class TestInterfaceParity: """ The two stores must resolve identically. ``_SkillObjectSet`` reimplements the lookup rather than inheriting it — see its docstring for why — so this is the @@ -1030,7 +1030,7 @@ def stream(self, basis: str | None) -> Any: class TestFailureHandling: - def test_a_403_stops_delivery_and_names_the_protocol_control_flag( + def test_a_403_stops_delivery_and_explains_why( self, endpoint: Any, caplog: Any ) -> None: endpoint.queue_poll(status=403) @@ -1038,8 +1038,8 @@ def test_a_403_stops_delivery_and_names_the_protocol_control_flag( with poll_store(endpoint) as store: assert wait_until(lambda: store.failed is not None) assert "403" in store.failed - assert "fdv2-protocol-control" in store.failed - assert any("fdv2-protocol-control" in r.getMessage() for r in caplog.records) + assert "opt-in" in store.failed + assert any("opt-in" in r.getMessage() for r in caplog.records) def test_a_401_stops_delivery(self, endpoint: Any) -> None: endpoint.queue_poll(status=401) @@ -1183,7 +1183,7 @@ def test_a_listener_that_raises_does_not_kill_delivery(self, endpoint: Any) -> N class TestMissingContentHash: """ - The blocking backend gap, asserted as behaviour rather than assumed. + A skill delivered without a ``contentHash``, asserted as behaviour. An envelope with no ``contentHash`` must produce a *withheld* skill with the ``missing_content_hash`` reason — loudly, diagnosably, and without a crash. @@ -1287,8 +1287,9 @@ def test_a_partly_hashed_payload_does_not_claim_total_failure( async def test_a_hash_that_does_not_match_is_a_different_failure( self, endpoint: Any ) -> None: - """``missing_content_hash`` and ``hash_mismatch`` must not collapse: one is - a backend gap and the other is possible tampering.""" + """``missing_content_hash`` and ``hash_mismatch`` must not collapse: one + means the envelope carried no hash, the other means the content did not + match the hash it carried.""" endpoint.queue_poll( full_payload( ("put-object", put_skill(content_hash=_hash("something else"))) @@ -1303,7 +1304,7 @@ async def test_a_hash_that_does_not_match_is_a_different_failure( ).reason == "integrity_failure" async def test_a_hashed_skill_resolves_end_to_end(self, endpoint: Any) -> None: - """The positive control: everything above is a gap, not a broken adapter.""" + """The positive control: a well-formed envelope resolves end to end.""" endpoint.queue_poll(full_payload(("put-object", put_skill()))) with poll_store(endpoint) as store: store.wait_for_skills(timeout=5) @@ -1405,9 +1406,8 @@ async def test_a_revocation_prunes_without_a_restart( self, endpoint: Any, tmp_path: Any ) -> None: """ - AV-1, closed at this layer. The store's change listener drives the - reconcile, so the file goes away seconds after the ``delete-object`` - rather than at the next process start. + The store's change listener drives the reconcile, so the file goes away + seconds after the ``delete-object`` rather than at the next process start. """ endpoint.queue_poll(full_payload(("put-object", put_skill()))) endpoint.queue_poll( @@ -1479,8 +1479,8 @@ async def test_a_burst_of_changes_coalesces_into_few_reconciles( async def test_the_default_keeps_last_known_good_during_an_outage( self, endpoint: Any, tmp_path: Any ) -> None: - """``on_unavailable="keep"`` is the endorsed default: an outage must not - read as "everything was revoked".""" + """``on_unavailable="keep"`` is the default: an outage must not read as + "everything was revoked".""" endpoint.queue_poll(full_payload(("put-object", put_skill()))) endpoint.queue_poll(status=500) with poll_store(endpoint, poll_interval=0.05) as store: @@ -1522,7 +1522,8 @@ async def test_no_store_configured_raises(self, tmp_path: Any) -> None: async def test_the_in_memory_store_can_also_drive_a_watch( self, tmp_path: Any ) -> None: - """The watcher is wired to the seam, not to the FDv2 store.""" + """The watcher is wired to the ``SkillStore`` interface, not to the FDv2 + store.""" store = InMemorySkillStore() store.put( { From 4eca85e1edf1417a0efc9f84cdc9e65acc30d000 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 14:02:43 -0400 Subject: [PATCH 04/13] =?UTF-8?q?fix(client):=20Agent=20Skills=20=E2=80=94?= =?UTF-8?q?=20three=20ways=20the=20FDv2=20loop=20stopped=20delivering=20wh?= =?UTF-8?q?ile=20reporting=20healthy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stream mode never reset the consecutive-failure count. A streaming connection only ever ends by being dropped, so the reset placed after the loop body returned was unreachable in stream mode, and every healthy server-recycled connection counted as a failure. After max_consecutive_failures + 1 recycles the store gave up for the process lifetime, including revocations. The count now lives on the store and is reset at each committed payload; the reset on a normal return is kept so a polled HTTP 304 still counts as a success. An overflowing Retry-After killed the delivery thread. float() accepts inf and out-of-range literals, and Event.wait(inf) raises OverflowError from inside the retry handler, which no sibling handler catches. The thread died with `failed` still None. The parser now ignores non-finite values, and the honoured delay is clamped to max_backoff so a header cannot park delivery for longer than the cap promises. close() during a slow connect hung for the full join timeout. The connection is published only after stream() returns, so a close() landing in that window had nothing to interrupt and the thread went on to block in the read. The loop now re-checks the stop flag as soon as the connection is published. Tests cover all three in stream mode, which previously had no failure-counting coverage at all. Co-Authored-By: Claude Fable 5.1 --- .../src/launchdarkly_ai_server/skills_fdv2.py | 59 +++++- packages/client/tests/test_skills_fdv2.py | 181 ++++++++++++++++++ 2 files changed, 233 insertions(+), 7 deletions(-) diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index c51bb4d..fd37dd3 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -40,6 +40,7 @@ import json import logging +import math import random import re import socket @@ -724,6 +725,11 @@ def _retry_after_seconds(headers: Any) -> float | None: # The HTTP-date form is legal and rare; falling back to our own backoff # is better than parsing a date to honour it approximately. return None + if not math.isfinite(seconds): + # ``float`` accepts "inf", "nan" and out-of-range literals such as + # "1e309". None of them is a delay, and an infinite one would overflow + # the wait that honours it, so treat them like the date form. + return None return max(0.0, seconds) @@ -1072,9 +1078,14 @@ def __init__( cannot hold a long-lived connection, and revocation there is one ``poll_interval`` late. + *max_backoff* caps every delay between retries, including one the server + asks for with ``Retry-After``; a header cannot park delivery for longer + than the cap promises. + *max_consecutive_failures* bounds the retry loop. On exceeding it the transport stops, logs an error, and the store keeps serving last known - good rather than pretending to be live — ``failed`` reports it. + good rather than pretending to be live — ``failed`` reports it. Only + failures in a row count: a committed payload resets the count. """ _require_server_side_credential(sdk_key) if mode not in ("stream", "poll"): @@ -1110,6 +1121,16 @@ def __init__( self._failed_reason: str | None = None self._connection: Any = None """The open streaming connection, so ``close`` can interrupt its read.""" + self._failures = 0 + """ + Recoverable failures since the last committed payload. + + Held on the store rather than in the loop because the reset belongs at + the commit, not at the return: a streaming connection only ever ends by + being dropped, so a loop that reset on return would count every healthy, + server-recycled connection as a failure and eventually give up on a + transport that never failed. + """ # -- lifecycle --------------------------------------------------------- @@ -1236,22 +1257,24 @@ def _notify(self, changes: list[dict[str, Any]]) -> None: # -- the delivery loop ------------------------------------------------- def _run(self) -> None: - failures = 0 while not self._stop.is_set(): try: if self._mode == "stream": self._stream_once() else: self._poll_once() - failures = 0 - with self._lock: - self._reader.diagnostics.connection_failures = 0 + # A poll that returned is a current answer even when it committed + # nothing (HTTP 304), so it counts as a success in its own right. + # A stream never returns normally; its successes are counted where + # they happen, at each commit in ``_apply``. + self._record_success() except _FatalTransportError as exc: self._give_up(str(exc)) return except _RecoverableTransportError as exc: - failures += 1 with self._lock: + self._failures += 1 + failures = self._failures self._reader.diagnostics.connection_failures = failures self._reader.diagnostics.last_error = str(exc) if failures > self._max_consecutive_failures: @@ -1261,10 +1284,16 @@ def _run(self) -> None: ) return delay = exc.retry_after - if delay is None: + if delay is None or not math.isfinite(delay): delay = backoff_delay( failures, base=self._initial_backoff, maximum=self._max_backoff ) + # ``Retry-After`` is a request, and ``max_backoff`` is a promise. + # The header comes from whatever answered on the error path, + # which may be a proxy or a CDN rather than LaunchDarkly, and a + # value in the hours would park delivery (and revocation) for + # that long. The promise wins. A zero still means "now". + delay = min(delay, self._max_backoff) logger.warning( "Skill delivery failed (%s); retrying in %.1fs", exc, delay ) @@ -1279,6 +1308,11 @@ def _run(self) -> None: if self._mode == "poll" and self._stop.wait(self._poll_interval): return + def _record_success(self) -> None: + with self._lock: + self._failures = 0 + self._reader.diagnostics.connection_failures = 0 + def _give_up(self, reason: str) -> None: with self._lock: self._failed_reason = reason @@ -1299,6 +1333,10 @@ def _apply(self, name: str, data: Any) -> _TransferOutcome: if outcome.committed and outcome.basis is not None: self._basis = outcome.basis if outcome.committed: + # A connection that transferred a payload succeeded, whatever it does + # afterwards: the give-up bound counts failures in a row, and a + # commit breaks the row. + self._record_success() self._first_payload.set() if outcome.changes: self._notify(outcome.changes) @@ -1332,6 +1370,13 @@ def _stream_once(self) -> None: with self._lock: self._connection = connection try: + # ``close`` may have run while the connect above was in flight. It + # found no connection to interrupt then, so this is the last chance + # to notice before the read below blocks for as long as the server + # stays quiet. Either ``close`` saw the connection and interrupted + # it, or it set the stop flag before this check: there is no window. + if self._stop.is_set(): + return for name, data in connection.events: if self._stop.is_set(): return diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index c33582d..8d37562 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -42,6 +42,7 @@ FDV2_OBJECT_KIND, _ProtocolReader, _RecoverableTransportError, + _retry_after_seconds, _SkillObjectSet, backoff_delay, is_skill_event, @@ -1029,6 +1030,69 @@ def stream(self, basis: str | None) -> Any: return _ScriptedConnection(outcome) +class _RecyclingRequester: + """ + A healthy server that recycles connections: every ``stream`` call succeeds, + transfers a full payload, and then ends the connection, as LaunchDarkly and + any proxy in between do to a long-lived stream. + """ + + def __init__(self) -> None: + self.connections = 0 + + def stream(self, basis: str | None) -> Any: + self.connections += 1 + return _ScriptedConnection( + [ + (e["event"], e["data"]) + for e in full_payload( + ("put-object", put_skill()), state=f"basis-{self.connections}" + ) + ] + ) + + +class _BlockingConnection: + """A stream that never produces an event until it is closed.""" + + def __init__(self) -> None: + self._closed = threading.Event() + + @property + def events(self) -> Any: + self._closed.wait() + return iter(()) + + def close(self) -> None: + self._closed.set() + + +class _SlowConnectRequester: + """ + A ``stream`` whose connect does not return until the test releases it, + standing in for a slow TLS handshake, followed by a read that never yields. + """ + + def __init__(self) -> None: + self.entered = threading.Event() + self.release = threading.Event() + + def stream(self, basis: str | None) -> Any: + self.entered.set() + self.release.wait(timeout=10) + return _BlockingConnection() + + +def stream_store(**kwargs: Any) -> FDv2SkillStore: + return FDv2SkillStore( + SDK_KEY, + mode="stream", + initial_backoff=kwargs.pop("initial_backoff", 0.001), + max_backoff=kwargs.pop("max_backoff", 0.002), + **kwargs, + ) + + class TestFailureHandling: def test_a_403_stops_delivery_and_explains_why( self, endpoint: Any, caplog: Any @@ -1100,6 +1164,62 @@ def test_retries_are_bounded(self) -> None: finally: store.close() + def test_recycled_stream_connections_are_not_failures(self) -> None: + # A streaming connection only ever ends by being dropped, so a loop + # that counted every drop as a failure would give up on a healthy + # server after max_consecutive_failures + 1 recycles, and delivery + # (including revocation) would silently stop for the process lifetime. + requester = _RecyclingRequester() + store = stream_store(max_consecutive_failures=3, _requester=requester) + try: + store.start() + assert wait_until(lambda: requester.connections >= 8) + assert store.failed is None + assert store.diagnostics.payloads_transferred >= 8 + # A drop is a failure until the next commit clears it, so the count + # may read 1 mid-reconnect. What it must never do is climb. + assert store.diagnostics.connection_failures <= 1 + assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None + finally: + store.close() + + def test_a_stream_commit_resets_the_failure_count(self) -> None: + payload = [ + (e["event"], e["data"]) for e in full_payload(("put-object", put_skill())) + ] + requester = _ScriptedRequester( + _RecoverableTransportError("x"), + _RecoverableTransportError("x"), + _RecoverableTransportError("x"), + payload, + ) + store = stream_store(max_consecutive_failures=3, _requester=requester) + try: + store.start() + assert store.wait_for_skills(timeout=5) + # Three failures reach the bound, then a commit, then the exhausted + # requester fails on every reconnect. The count must start again at + # the commit: the stream's own drop is failure one, and three more + # connects are owed before giving up. Carrying the three over would + # give up on the drop itself, with no further connect at all. + assert wait_until(lambda: store.failed is not None) + assert "gave up after 4 consecutive failures" in store.failed + assert "last error: x" in store.failed + assert len(requester.calls) == 7 + finally: + store.close() + + def test_stream_retries_are_bounded(self) -> None: + store = stream_store( + max_consecutive_failures=3, _requester=_ScriptedRequester() + ) + try: + store.start() + assert wait_until(lambda: store.failed is not None) + assert "gave up after 4 consecutive failures" in store.failed + finally: + store.close() + def test_a_retry_after_header_is_honoured(self) -> None: requester = _ScriptedRequester( _RecoverableTransportError("slow down", retry_after=0.25), @@ -1128,6 +1248,51 @@ def test_a_retry_after_header_is_parsed_off_the_wire(self, endpoint: Any) -> Non # If Retry-After were ignored the 5s backoff would blow the timeout. assert store.wait_for_skills(timeout=3) is True + @pytest.mark.parametrize("raw", ["inf", "Infinity", "-inf", "nan", "1e309"]) + def test_a_non_finite_retry_after_is_ignored(self, raw: str) -> None: + assert _retry_after_seconds({"Retry-After": raw}) is None + + def test_retry_after_parsing_keeps_its_edges(self) -> None: + assert _retry_after_seconds({"Retry-After": "0"}) == 0.0 + assert _retry_after_seconds({"Retry-After": "-5"}) == 0.0 + assert ( + _retry_after_seconds({"Retry-After": "Wed, 21 Oct 2015 07:28:00 GMT"}) + is None + ) + assert _retry_after_seconds({"Retry-After": "2.5"}) == 2.5 + + @pytest.mark.parametrize("retry_after", [float("inf"), float("nan"), 86400.0]) + def test_an_unreasonable_retry_after_neither_kills_delivery_nor_parks_it( + self, retry_after: float + ) -> None: + # An infinite wait would overflow inside the retry handler and kill the + # thread with `failed` still None; a day-long one would be honoured to + # the second. Both must fall back to the max_backoff cap and carry on. + requester = _ScriptedRequester( + _RecoverableTransportError("slow down", retry_after=retry_after), + [ + (e["event"], e["data"]) + for e in full_payload(("put-object", put_skill())) + ], + ) + store = stream_store(max_backoff=0.05, _requester=requester) + try: + store.start() + assert store.wait_for_skills(timeout=3) is True + assert store.failed is None + assert store._thread is not None and store._thread.is_alive() + finally: + store.close() + + def test_a_non_finite_retry_after_off_the_wire_falls_back_to_backoff( + self, endpoint: Any + ) -> None: + endpoint.queue_poll(status=429, retry_after="inf") + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + with poll_store(endpoint) as store: + assert store.wait_for_skills(timeout=3) is True + assert store.failed is None + def test_backoff_is_exponential_and_capped(self) -> None: assert backoff_delay(1, base=1.0, maximum=30.0, jitter=0.0) == 1.0 assert backoff_delay(2, base=1.0, maximum=30.0, jitter=0.0) == 2.0 @@ -1573,6 +1738,22 @@ def test_close_is_idempotent(self, endpoint: Any) -> None: store.close() store.close() + def test_close_during_a_slow_connect_returns_promptly(self) -> None: + # Before the connect returns there is no connection for close() to + # interrupt. If the delivery thread then enters the read anyway, close() + # sits out its whole join timeout on a stream that will never speak. + requester = _SlowConnectRequester() + store = stream_store(_requester=requester) + store.start() + assert requester.entered.wait(timeout=5) + threading.Timer(0.1, requester.release.set).start() + started = time.monotonic() + store.close(timeout=5.0) + elapsed = time.monotonic() - started + assert elapsed < 2.0 + assert store._thread is not None + assert not store._thread.is_alive() + def test_a_closed_store_still_answers_from_what_it_received( self, endpoint: Any ) -> None: From 98569778da45f0b222543d5fdcdd581f2b82ec20 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 14:23:40 -0400 Subject: [PATCH 05/13] =?UTF-8?q?fix(client):=20Agent=20Skills=20=E2=80=94?= =?UTF-8?q?=20dedupe=20the=20hashless-object=20ERROR=20per=20store,=20not?= =?UTF-8?q?=20per=20process?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_warn_hashless` deduped on a module-global set, so a store recreated in the same process (reconnect wrapper, config reload, credential rotation) never reported a hashless `(key, version)` again, and two stores in one process suppressed each other's diagnostics. The set also grew for the life of the process. The docstring promised per-store behaviour the code did not deliver. The dedupe set now lives on `_ProtocolReader`, alongside the per-store `StoreDiagnostics`. The lock is dropped: `handle` only runs under the owning store's lock, on that store's single delivery thread. Tests drive two readers directly to show a recreated store reports again, a re-delivered payload still logs once per store, and two live stores do not quieten each other. The autouse fixture that cleared the global set is gone with the global. Co-Authored-By: Claude Fable 5.1 --- .../src/launchdarkly_ai_server/skills_fdv2.py | 58 +++++++++-------- packages/client/tests/test_skills_fdv2.py | 62 ++++++++++++++++--- 2 files changed, 84 insertions(+), 36 deletions(-) diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index fd37dd3..ac07861 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -473,6 +473,11 @@ def __init__(self, committed: _SkillObjectSet) -> None: self._pending: _SkillObjectSet | None = None self._changes: list[dict[str, Any]] = [] self.diagnostics = StoreDiagnostics() + # Identities already reported by ``_warn_hashless``. Held per reader, so a + # store that is recreated in the same process reports again and two + # stores never quieten each other. No lock: ``handle`` only runs under + # the owning store's lock, on that store's single delivery thread. + self._warned_hashless: set[tuple[str, Any]] = set() # -- events ------------------------------------------------------------ @@ -547,7 +552,7 @@ def _put_object(self, data: Any) -> _TransferOutcome: self.diagnostics.skill_objects_received += 1 if not isinstance(raw.get("contentHash"), str): self.diagnostics.hashless_objects += 1 - _warn_hashless(raw) + self._warn_hashless(raw) return _TransferOutcome() def _delete_object(self, data: Any) -> _TransferOutcome: @@ -620,6 +625,31 @@ def _goodbye(self, data: Any) -> _TransferOutcome: ) return _TransferOutcome(disconnect=f"server said goodbye: {reason}") + # -- diagnostics --------------------------------------------------------- + + def _warn_hashless(self, raw: dict[str, Any]) -> None: + """ + One ERROR per ``(key, version)`` whose envelope had no ``contentHash``. + + At ERROR rather than WARN, and per object rather than once per process, + because an empty accessor result is otherwise indistinguishable from an + environment that has no skills. Deduped within this reader so a + re-delivered payload does not multiply it; a store that is recreated, + in this process or another, reports again, and stores for different + environments in one process do not share the dedupe. + """ + identity = (raw["key"], raw.get("version")) + if identity in self._warned_hashless: + return + self._warned_hashless.add(identity) + logger.error( + "Skill '%s' version %s arrived without a contentHash and will be withheld. %s", + raw["key"], + raw.get("version"), + _HASHLESS_ADVICE, + extra={"ld_skill_key": raw["key"], "ld_skill_version": raw.get("version")}, + ) + _HASHLESS_ADVICE = ( "The delivered skill object carries no 'contentHash', so integrity " @@ -631,32 +661,6 @@ def _goodbye(self, data: Any) -> _TransferOutcome: "skills in your environment arrive without one." ) -_warned_hashless: set[tuple[str, Any]] = set() -_warned_lock = threading.Lock() - - -def _warn_hashless(raw: dict[str, Any]) -> None: - """ - One ERROR per ``(key, version)`` whose envelope had no ``contentHash``. - - At ERROR rather than WARN, and per object rather than once per process, - because an empty accessor result is otherwise indistinguishable from an - environment that has no skills. Deduped so a re-delivered payload does not - multiply it; a restarted process reports again. - """ - identity = (raw["key"], raw.get("version")) - with _warned_lock: - if identity in _warned_hashless: - return - _warned_hashless.add(identity) - logger.error( - "Skill '%s' version %s arrived without a contentHash and will be withheld. %s", - raw["key"], - raw.get("version"), - _HASHLESS_ADVICE, - extra={"ld_skill_key": raw["key"], "ld_skill_version": raw.get("version")}, - ) - def _warn_if_nothing_can_verify( committed: _SkillObjectSet, hashless_before_this_payload: int diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index 8d37562..7790a30 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -49,7 +49,6 @@ seam_object_from_put, tombstone_from_delete, ) -from launchdarkly_ai_server.skills_fdv2 import _warned_hashless as _hashless_dedupe pytestmark = pytest.mark.usefixtures("reset_skill_state") @@ -315,14 +314,6 @@ def endpoint() -> Any: server.close() -@pytest.fixture(autouse=True) -def _clear_hashless_dedupe() -> Any: - """The hashless-object ERROR is deduped per process; per test here.""" - _hashless_dedupe.clear() - yield - _hashless_dedupe.clear() - - def poll_store(endpoint: Any, **kwargs: Any) -> FDv2SkillStore: return FDv2SkillStore( SDK_KEY, @@ -1346,6 +1337,17 @@ def test_a_listener_that_raises_does_not_kill_delivery(self, endpoint: Any) -> N # --------------------------------------------------------------------------- +def _per_object_hashless_errors(caplog: Any) -> list[Any]: + """The per-object ERROR, as distinct from the whole-payload summary.""" + return [ + r + for r in caplog.records + if r.levelname == "ERROR" + and "arrived without a contentHash" in r.getMessage() + and "No skill content will resolve" not in r.getMessage() + ] + + class TestMissingContentHash: """ A skill delivered without a ``contentHash``, asserted as behaviour. @@ -1414,6 +1416,48 @@ def test_a_hashless_object_logs_an_error_naming_the_reason_code( assert "pdf-extraction" in rendered assert "contentHash" in rendered + def test_a_redelivered_hashless_object_logs_once_per_store( + self, caplog: Any + ) -> None: + """Re-delivering the same ``(key, version)`` to one store must not + multiply the ERROR: a polling store sees every object on every poll.""" + reader = _ProtocolReader(_SkillObjectSet()) + payload = full_payload(("put-object", put_skill(omit_hash=True))) + with caplog.at_level("ERROR"): + drive(reader, payload) + drive(reader, payload) + assert len(_per_object_hashless_errors(caplog)) == 1 + + def test_a_recreated_store_reports_the_same_hashless_object_again( + self, caplog: Any + ) -> None: + """ + The dedupe belongs to the store, not the process. A host that rebuilds + its store (reconnect wrapper, config reload, credential rotation) must + get the ERROR again, since it is the loudest signal that a deployment is + broken rather than empty by design. + """ + payload = full_payload(("put-object", put_skill(omit_hash=True))) + with caplog.at_level("ERROR"): + drive(_ProtocolReader(_SkillObjectSet()), payload) + first = len(_per_object_hashless_errors(caplog)) + drive(_ProtocolReader(_SkillObjectSet()), payload) + assert first == 1 + assert len(_per_object_hashless_errors(caplog)) == 2 + + def test_two_live_stores_do_not_suppress_each_other(self, caplog: Any) -> None: + """Two stores in one process (say, two environments) each report.""" + one = _ProtocolReader(_SkillObjectSet()) + two = _ProtocolReader(_SkillObjectSet()) + payload = full_payload(("put-object", put_skill(omit_hash=True))) + with caplog.at_level("ERROR"): + drive(one, payload) + drive(two, payload) + # And each still dedupes its own re-deliveries. + drive(one, payload) + drive(two, payload) + assert len(_per_object_hashless_errors(caplog)) == 2 + def test_a_wholly_hashless_payload_says_so_once( self, endpoint: Any, caplog: Any ) -> None: From 7279fc944cadeca6e7c4199559f729d40a517df5 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 14:30:34 -0400 Subject: [PATCH 06/13] =?UTF-8?q?fix(client):=20Agent=20Skills=20=E2=80=94?= =?UTF-8?q?=20drop=20the=20no-op=20connect=5Ftimeout,=20bound=20polls=20ho?= =?UTF-8?q?nestly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FDv2SkillStore accepted connect_timeout and forwarded it to _Requester, which stored it and never read it: both poll and stream passed only read_timeout to urllib. So FDv2SkillStore(key, connect_timeout=2.0) did nothing, and a poll against a host that accepts and never answers hung for read_timeout (300s) rather than the 10s the parameter advertised. Removed rather than wired up. urllib's timeout is the socket timeout for the whole operation, so bounding the connect separately from the reads means a custom connection class and handler in a module that is deliberately standard-library only. One timeout that is honoured beats two where one lies. Removing it leaves the gap it was presumably meant to cover: 300s is right for a stream (heartbeats arrive well inside it, and the timeout is per read) but far too long for a single poll request. read_timeout now defaults per mode — DEFAULT_POLL_TIMEOUT (10s) bounding the whole poll, DEFAULT_STREAM_READ_TIMEOUT (300s) bounding each stream read — and an explicit value overrides either. A non-positive value is rejected. Tests drive a socket that accepts and never responds, and measure that a poll and a stream open both fail in roughly read_timeout; the store records the failure and keeps retrying. The constructor is asserted to reject connect_timeout. README and agents.md document the single timeout and why there is no second one. Co-Authored-By: Claude Fable 5.1 --- packages/client/README.md | 9 ++ packages/client/agents.md | 8 ++ .../src/launchdarkly_ai_server/skills_fdv2.py | 49 ++++++- packages/client/tests/test_skills_fdv2.py | 134 ++++++++++++++++++ 4 files changed, 195 insertions(+), 5 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index ccae5b3..a13af1d 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -484,6 +484,15 @@ outage the store keeps serving the last content it received and `write_skills`' `on_unavailable="keep"` leaves managed files alone — an outage must not read as "everything was revoked". +**One network timeout, and its default depends on the mode.** `read_timeout` bounds every +socket operation of a request — connecting, waiting for headers, and each read — because the +standard library offers no separate connect timeout, and the store deliberately adds no HTTP +client dependency to provide one. In `mode="poll"` it therefore bounds the whole request and +defaults to 10 seconds, so a poll against a host that never answers fails in that time and is +retried. In `mode="stream"` it bounds each wait for the next bytes and defaults to 300 seconds: +a stream is meant to sit idle between events, and LaunchDarkly's heartbeats arrive well inside +that, so a stream silent for longer has genuinely gone. Pass `read_timeout` to override either. + **The connection also carries your flags.** A client cannot request only the skill payload, so a skills-enabled environment delivers flag and segment objects on the same connection. They are skipped, not evaluated — this store does no evaluation of any kind — and diff --git a/packages/client/agents.md b/packages/client/agents.md index abac360..36d6f05 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -266,6 +266,14 @@ such skill" — and would let a prune delete the last known-good copy on disk. N a hash from the delivered content: that certifies the content against itself and verifies nothing. +**There is one network timeout, not two.** `urllib`'s `timeout` is the socket timeout for the +whole operation, so connect, headers and each read share it, and the module cannot bound the +connect separately without a custom connection class it should not carry. `read_timeout` is +therefore the only knob, and its default is per mode (`DEFAULT_POLL_TIMEOUT` for a whole poll +request, `DEFAULT_STREAM_READ_TIMEOUT` for the gap between reads on a stream). Do not add a +parameter that the standard library cannot honour; `TestTimeouts` measures the bound against a +socket that accepts and never answers. + **`close` interrupts the socket, it does not just set a flag.** The delivery thread spends its life blocked in a read that no flag can reach, and closing a response from another thread does not unblock CPython's buffered reader. `_interrupt_read` shuts the socket down underneath it. diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index ac07861..e93563f 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -91,6 +91,19 @@ instance expects a different value. """ +DEFAULT_POLL_TIMEOUT = 10.0 +""" +Default ``read_timeout`` in ``"poll"`` mode: the bound on one whole +``GET /sdk/poll``, from opening the connection to reading the last byte. +""" + +DEFAULT_STREAM_READ_TIMEOUT = 300.0 +""" +Default ``read_timeout`` in ``"stream"`` mode: the longest a live stream may go +silent before it is treated as dead. LaunchDarkly sends heartbeats well inside +this, so an idle stream this long genuinely has gone away. +""" + _EVENT_SERVER_INTENT = "server-intent" _EVENT_PUT_OBJECT = "put-object" _EVENT_DELETE_OBJECT = "delete-object" @@ -838,7 +851,6 @@ def __init__( sdk_key: str, base_uri: str, *, - connect_timeout: float, read_timeout: float, data_model_version: int, opener: Any = None, @@ -846,7 +858,15 @@ def __init__( self._sdk_key = sdk_key self._base_uri = base_uri.rstrip("/") self._read_timeout = read_timeout - self._connect_timeout = connect_timeout + """ + The one timeout, applied to every socket operation of a request. + + ``urllib`` has no separate connect timeout: its ``timeout`` becomes the + socket timeout for the whole operation, so connecting, waiting for headers + and each body read are all bounded by this same value. For a poll that + makes it the bound on the whole request; for a stream it is the longest + gap tolerated between two reads. + """ self._data_model_version = data_model_version # Injectable so the tests drive a fake endpoint without a socket; the # default is urllib's global opener. @@ -1067,8 +1087,7 @@ def __init__( base_uri: str = DEFAULT_BASE_URI, mode: Mode = "stream", poll_interval: float = 30.0, - connect_timeout: float = 10.0, - read_timeout: float = 300.0, + read_timeout: float | None = None, initial_backoff: float = 1.0, max_backoff: float = 30.0, max_consecutive_failures: int = 10, @@ -1082,6 +1101,19 @@ def __init__( cannot hold a long-lived connection, and revocation there is one ``poll_interval`` late. + *read_timeout* is the only network timeout, and it bounds every socket + operation of a request: connecting, waiting for headers, and each read. + There is no separate connect timeout because the standard library offers + none, so a host that accepts and never answers, or never accepts, fails + after ``read_timeout`` too. What the value means therefore depends on the + mode, and so does its default. In ``"poll"`` mode it bounds the whole + request and defaults to ``DEFAULT_POLL_TIMEOUT`` (10s). In ``"stream"`` + mode it bounds each wait for the next bytes and defaults to + ``DEFAULT_STREAM_READ_TIMEOUT`` (300s): a stream is meant to sit idle + between events, and LaunchDarkly's heartbeats arrive well inside that. + Pass a value to override the default for either mode; it must be + positive. + *max_backoff* caps every delay between retries, including one the server asks for with ``Retry-After``; a header cannot park delivery for longer than the cap promises. @@ -1096,6 +1128,14 @@ def __init__( raise ValueError(f'mode must be "stream" or "poll", got {mode!r}') if poll_interval <= 0: raise ValueError(f"poll_interval must be positive, got {poll_interval!r}") + if read_timeout is None: + read_timeout = ( + DEFAULT_STREAM_READ_TIMEOUT + if mode == "stream" + else DEFAULT_POLL_TIMEOUT + ) + elif not (math.isfinite(read_timeout) and read_timeout > 0): + raise ValueError(f"read_timeout must be positive, got {read_timeout!r}") self._mode: Mode = mode self._poll_interval = poll_interval @@ -1114,7 +1154,6 @@ def __init__( self._requester = _requester or _Requester( sdk_key.strip(), base_uri, - connect_timeout=connect_timeout, read_timeout=read_timeout, data_model_version=data_model_version, ) diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index 7790a30..35cc90d 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -19,6 +19,7 @@ import hashlib import json +import socket import threading import time from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer @@ -38,10 +39,13 @@ ) from launchdarkly_ai_server.skills_core import SKILL_OBJECT_KIND from launchdarkly_ai_server.skills_fdv2 import ( + DEFAULT_POLL_TIMEOUT, + DEFAULT_STREAM_READ_TIMEOUT, FDV2_OBJECT_CATEGORY, FDV2_OBJECT_KIND, _ProtocolReader, _RecoverableTransportError, + _Requester, _retry_after_seconds, _SkillObjectSet, backoff_delay, @@ -1821,3 +1825,133 @@ def test_the_store_satisfies_the_seam_before_it_starts(self) -> None: store = FDv2SkillStore(SDK_KEY) assert store.get_object(SKILL_OBJECT_KIND, "anything") is None assert store.all_objects(SKILL_OBJECT_KIND) == {} + + +# --------------------------------------------------------------------------- +# Timeouts +# --------------------------------------------------------------------------- + + +class _BlackHole: + """ + A listening socket that accepts connections and never sends a byte. + + This is the host ``read_timeout`` exists for: the TCP handshake completes, so + nothing fails fast, and then no response ever comes. A request against it can + only end by timing out, which makes the elapsed time a direct measurement of + the timeout actually applied. + """ + + def __init__(self) -> None: + self._listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + self._listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + self._listener.bind(("127.0.0.1", 0)) + self._listener.listen(8) + self._accepted: list[socket.socket] = [] + self._stop = threading.Event() + self._thread = threading.Thread(target=self._accept_forever, daemon=True) + self._thread.start() + host, port = self._listener.getsockname() + self.base_uri = f"http://{host}:{port}" + + def _accept_forever(self) -> None: + self._listener.settimeout(0.05) + while not self._stop.is_set(): + try: + conn, _ = self._listener.accept() + except OSError: + continue + self._accepted.append(conn) + + def close(self) -> None: + self._stop.set() + self._thread.join(timeout=2) + for conn in self._accepted: + conn.close() + self._listener.close() + + +@pytest.fixture +def black_hole() -> Any: + server = _BlackHole() + yield server + server.close() + + +class TestTimeouts: + """ + ``read_timeout`` is the only network timeout, and every request honours it. + + The bounds asserted here are loose on purpose: the point is that a request + against an unresponsive host fails in roughly ``read_timeout`` rather than in + minutes, and that a regression back to a much longer default fails this + suite quickly instead of hanging it. + """ + + def test_a_poll_against_an_unresponsive_host_fails_within_read_timeout( + self, black_hole: Any + ) -> None: + requester = _Requester( + SDK_KEY, black_hole.base_uri, read_timeout=0.3, data_model_version=1 + ) + started = time.monotonic() + with pytest.raises(_RecoverableTransportError) as excinfo: + requester.poll(None, None) + elapsed = time.monotonic() - started + assert 0.2 <= elapsed < 2.0 + assert "timed out" in str(excinfo.value) + + def test_a_stream_against_an_unresponsive_host_fails_within_read_timeout( + self, black_hole: Any + ) -> None: + requester = _Requester( + SDK_KEY, black_hole.base_uri, read_timeout=0.3, data_model_version=1 + ) + started = time.monotonic() + with pytest.raises(_RecoverableTransportError): + requester.stream(None) + assert time.monotonic() - started < 2.0 + + def test_the_store_reports_the_timeout_and_keeps_going( + self, black_hole: Any + ) -> None: + store = FDv2SkillStore( + SDK_KEY, + base_uri=black_hole.base_uri, + mode="poll", + poll_interval=0.05, + initial_backoff=0.01, + max_backoff=0.05, + read_timeout=0.3, + ) + try: + store.start() + assert wait_until(lambda: store.diagnostics.connection_failures >= 1) + assert store.failed is None + assert "timed out" in (store.diagnostics.last_error or "") + finally: + store.close() + + def test_the_default_bound_depends_on_the_mode(self) -> None: + assert DEFAULT_POLL_TIMEOUT == 10.0 + assert DEFAULT_STREAM_READ_TIMEOUT == 300.0 + polling = FDv2SkillStore(SDK_KEY, mode="poll") + streaming = FDv2SkillStore(SDK_KEY, mode="stream") + assert polling._requester._read_timeout == DEFAULT_POLL_TIMEOUT + assert streaming._requester._read_timeout == DEFAULT_STREAM_READ_TIMEOUT + + @pytest.mark.parametrize("mode", ["poll", "stream"]) + def test_an_explicit_read_timeout_overrides_the_default(self, mode: Any) -> None: + store = FDv2SkillStore(SDK_KEY, mode=mode, read_timeout=42.0) + assert store._requester._read_timeout == 42.0 + + @pytest.mark.parametrize("value", [0.0, -1.0, float("inf"), float("nan")]) + def test_a_non_positive_read_timeout_is_rejected(self, value: float) -> None: + with pytest.raises(ValueError, match="read_timeout"): + FDv2SkillStore(SDK_KEY, read_timeout=value) + + def test_there_is_no_separate_connect_timeout(self) -> None: + # ``urllib`` cannot bound the connect separately from the reads, so the + # constructor does not offer a parameter that would only pretend to. + with pytest.raises(TypeError): + FDv2SkillStore(SDK_KEY, connect_timeout=2.0) # type: ignore[call-arg] From 9ebe826dc12c7156b61f186b3d4ce6be0f244aea Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 15:01:18 -0400 Subject: [PATCH 07/13] =?UTF-8?q?feat(client):=20Agent=20Skills=20?= =?UTF-8?q?=E2=80=94=20let=20a=20closed=20SkillWatcher=20detach=20from=20i?= =?UTF-8?q?ts=20store?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the optional remove_listener(kind, fn) half of the SkillStore listener interface to InMemorySkillStore and FDv2SkillStore, and have SkillWatcher.close() unregister notify. Until now a closed watcher stayed in the store's listener list for the store's lifetime, so repeated watchers accumulated and every committed change walked a list of dead listeners. SkillWatcher now takes the store, registers before starting its worker, and detaches once under a guard so close() stays idempotent. A store that offers add_listener but not remove_listener still works; the watcher probes and skips detaching rather than failing the close. Co-Authored-By: Claude Fable 5.1 --- packages/client/README.md | 2 +- packages/client/agents.md | 10 +- .../src/launchdarkly_ai_server/skills.py | 16 +++ .../src/launchdarkly_ai_server/skills_core.py | 16 ++- .../src/launchdarkly_ai_server/skills_fdv2.py | 21 ++++ .../launchdarkly_ai_server/skills_watch.py | 36 ++++++- packages/client/tests/test_skills.py | 29 +++++ packages/client/tests/test_skills_fdv2.py | 102 ++++++++++++++++++ 8 files changed, 219 insertions(+), 13 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index a13af1d..862b38f 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -525,7 +525,7 @@ Windows. | `get_skills(refs)` | Batch form. Accepts `SkillReference` values and bare key strings (string = latest). Results follow input order; missing or unverifiable entries are omitted. | | `all_skills()` | Every verified skill the store holds, one per key at its newest version. | | `write_skills(skills, root, *, prune=True, timeout=10.0, on_unavailable="keep")` | Materialize skills under `root`, returning a `ReconcileReport`. `prune` removes formerly-managed skills no longer requested. `on_unavailable="raise"` raises instead of reporting when content cannot be retrieved. Raises `ValueError` for an unusable root, a negative `timeout`, or an unrecognised `on_unavailable`. **Performs synchronous filesystem I/O — see the note below.** | -| `SkillStore` | The structural interface content arrives through: `get_object(kind, key, version=None)`, `all_objects(kind)`, optional `add_listener(kind, fn)`. | +| `SkillStore` | The structural interface content arrives through: `get_object(kind, key, version=None)`, `all_objects(kind)`, optional `add_listener(kind, fn)` / `remove_listener(kind, fn)`. | | `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=…, mode="stream", …)` | The delivery transport: a store fed by LaunchDarkly over the SDK-facing FDv2 channel. `start()`, `wait_for_skills(timeout)`, `close()`, `diagnostics`, `failed`; also a context manager. **Server-side only** — a mobile key or client-side environment ID raises. See *Receiving skills from LaunchDarkly* above. | | `watch_skills(skills, root, …)` | `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. | diff --git a/packages/client/agents.md b/packages/client/agents.md index 36d6f05..9c1f965 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -200,11 +200,11 @@ Three layers, in increasing order of blast radius: ### The store seam, and why version is part of the lookup -`SkillStore` is `get_object(kind, key, version=None)`, `all_objects(kind)`, and an optional -`add_listener(kind, fn)`. Version is part of the **lookup identity**, not a filter applied -to the answer, and that is load-bearing: a delivery payload carries the newest version of -every skill *plus* every version any variation currently pins, so two versions of one key -coexist routinely. A seam keyed by key alone would answer a pinned reference with the newest +`SkillStore` is `get_object(kind, key, version=None)`, `all_objects(kind)`, and the optional +pair `add_listener(kind, fn)` / `remove_listener(kind, fn)`. Version is part of the **lookup +identity**, not a filter applied to the answer, and that is load-bearing: a delivery payload +carries the newest version of every skill *plus* every version any variation currently pins, +so two versions of one key coexist routinely. A seam keyed by key alone would answer a pinned reference with the newest object, and the caller would then have to reject it — turning the primary use case, a version-pinned attachment, into a missing skill. `version=None` asks for the newest held. diff --git a/packages/client/src/launchdarkly_ai_server/skills.py b/packages/client/src/launchdarkly_ai_server/skills.py index cb21986..9016ab9 100644 --- a/packages/client/src/launchdarkly_ai_server/skills.py +++ b/packages/client/src/launchdarkly_ai_server/skills.py @@ -167,6 +167,22 @@ def add_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None: """ self._listeners.setdefault(kind, []).append(fn) + def remove_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None: + """ + Unregisters *fn* from *kind*, so a subsequent ``put`` no longer calls it. + + Removes one occurrence: a callable registered twice must be removed twice. + Removing a callable that is not registered is a no-op, not an error, so a + consumer that detaches on close can do so unconditionally. + """ + listeners = self._listeners.get(kind) + if listeners is None: + return + try: + listeners.remove(fn) + except ValueError: + return + # --------------------------------------------------------------------------- # Reference discovery diff --git a/packages/client/src/launchdarkly_ai_server/skills_core.py b/packages/client/src/launchdarkly_ai_server/skills_core.py index 74cb706..2216679 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_core.py +++ b/packages/client/src/launchdarkly_ai_server/skills_core.py @@ -151,11 +151,17 @@ class SkillStore(Protocol): Duck-typed on purpose, mirroring how the LaunchDarkly client interface works in this package: pass any object carrying these methods. - ``add_listener(kind, fn)`` is part of the seam but - **optional**, which is why it is deliberately not declared here: a Protocol - member is required for structural compatibility, so declaring it would reject - every store that does not implement it. Nothing in this module calls it — it - exists for the delivery transport to push updates through. + ``add_listener(kind, fn)`` and ``remove_listener(kind, fn)`` are part of the + interface but **optional**, which is why they are deliberately not declared + here: a Protocol member is required for structural compatibility, so declaring + them would reject every store that does not implement them. Nothing in this + module calls either — they exist for the delivery transport to push updates + through, and for a consumer such as ``watch_skills`` to stop receiving them. + A store that implements ``add_listener`` should implement ``remove_listener`` + too; consumers probe for it and skip detaching when it is absent, so an + older store keeps working at the cost of a listener that lives as long as + the store does. ``remove_listener`` removes one occurrence of *fn* under + *kind* and is a no-op when *fn* is not registered. The raw objects a store serves are wire-shaped, with camelCase field names identical across language implementations:: diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index e93563f..2406e20 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -1284,6 +1284,27 @@ def add_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None: with self._lock: self._listeners.setdefault(kind, []).append(fn) + def remove_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None: + """ + Unregisters *fn* from *kind*, so later committed changes no longer call it. + + Safe to call from any thread, including from inside a listener: the + listener list is copied under the lock before a commit's notifications + run, so a removal during one commit takes effect from the next. + + Removes one occurrence: a callable registered twice must be removed twice. + Removing a callable that is not registered is a no-op, not an error, so + ``SkillWatcher.close`` can detach unconditionally. + """ + with self._lock: + listeners = self._listeners.get(kind) + if listeners is None: + return + try: + listeners.remove(fn) + except ValueError: + return + def _notify(self, changes: list[dict[str, Any]]) -> None: with self._lock: listeners = list(self._listeners.get(SKILL_OBJECT_KIND, [])) diff --git a/packages/client/src/launchdarkly_ai_server/skills_watch.py b/packages/client/src/launchdarkly_ai_server/skills_watch.py index d14d69a..d9461ca 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_watch.py +++ b/packages/client/src/launchdarkly_ai_server/skills_watch.py @@ -58,12 +58,19 @@ class SkillWatcher: wrote unmanaged. This class enforces that for its *own* reconciles — they run on a single worker thread, serialized — and cannot enforce it against a caller who reconciles the same root by hand. + + The watcher owns its registration on *store*: it registers ``notify`` when + constructed and unregisters it in ``close``, so a closed watcher is no longer + reachable from the store and can be collected. *store* must implement + ``add_listener``; ``remove_listener`` is probed for and, when the store does + not offer it, the listener stays registered for the store's lifetime. """ def __init__( self, request: Sequence[Skill | SkillReference | str] | str, root: str | os.PathLike[str], + store: Any, *, prune: bool, timeout: float, @@ -86,6 +93,14 @@ def __init__( self._thread = threading.Thread( target=self._run, name="ld-ai-skills-reconcile", daemon=True ) + + # Register before starting the worker: ``notify`` only sets an event, so a + # change that lands in between is picked up as soon as the worker runs, + # and a store whose ``add_listener`` raises leaves no thread behind. + self._store = store + self._registered = False + store.add_listener(SKILL_OBJECT_KIND, self.notify) + self._registered = True self._thread.start() # -- the listener the store calls ------------------------------------- @@ -175,12 +190,27 @@ def close(self, timeout: float = 15.0) -> None: reconcile killed between its content writes and its manifest rewrite is the one case the manifest format has to recover from — worth avoiding when we control the timing. + + Detaches ``notify`` from the store first, so no further change reaches a + watcher that is shutting down and the store no longer holds a reference to + it. A store without the optional ``remove_listener`` is left as it is + rather than failing the close. """ + self._detach() self._stop.set() self._wake.set() if self._thread.is_alive() and self._thread is not threading.current_thread(): self._thread.join(timeout=timeout) + def _detach(self) -> None: + with self._lock: + if not self._registered: + return + self._registered = False + remove_listener = getattr(self._store, "remove_listener", None) + if callable(remove_listener): + remove_listener(SKILL_OBJECT_KIND, self.notify) + def __enter__(self) -> SkillWatcher: return self @@ -221,7 +251,9 @@ async def watch_skills( and when the configured store has no ``add_listener`` — the second case failing loudly rather than degrading to a one-shot reconcile, because a watcher that silently never fires looks exactly like a watcher whose skills - never changed. + never changed. The optional ``remove_listener`` lets ``SkillWatcher.close`` + detach from the store; a store without it still works, but each closed + watcher then stays registered for the store's lifetime. """ store = get_store() if store is None: @@ -250,11 +282,11 @@ async def watch_skills( watcher = SkillWatcher( skills, root, + store, prune=prune, timeout=timeout, on_unavailable=on_unavailable, debounce=debounce, on_reconcile=on_reconcile, ) - add_listener(SKILL_OBJECT_KIND, watcher.notify) return report, watcher diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index 6470f0c..d3f8309 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -548,6 +548,35 @@ def test_put_does_not_notify_other_kind_listeners( assert seen == [] + def test_remove_listener_stops_put_notifying_it(self, make_raw_skill: Any) -> None: + s = InMemorySkillStore() + seen: list[dict[str, Any]] = [] + s.add_listener("skill", seen.append) + s.remove_listener("skill", seen.append) + + s.put(make_raw_skill(key="a")) + + assert seen == [] + + def test_remove_listener_removes_one_occurrence(self, make_raw_skill: Any) -> None: + s = InMemorySkillStore() + seen: list[dict[str, Any]] = [] + s.add_listener("skill", seen.append) + s.add_listener("skill", seen.append) + s.remove_listener("skill", seen.append) + + s.put(make_raw_skill(key="a")) + + assert len(seen) == 1 + + def test_remove_listener_of_an_unregistered_callable_is_a_no_op(self) -> None: + s = InMemorySkillStore() + s.remove_listener("skill", print) + s.add_listener("skill", print) + s.remove_listener("flag", print) + s.remove_listener("skill", print) + s.remove_listener("skill", print) + class TestStoreConfiguration: """Store wiring on the lifecycle layer.""" diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index 35cc90d..e9f0ed9 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -1764,6 +1764,108 @@ async def test_the_in_memory_store_can_also_drive_a_watch( watcher.close() +class TestWatcherDetachesOnClose: + """``SkillWatcher.close`` unregisters ``notify``, so a closed watcher is + neither called nor kept alive by the store.""" + + @staticmethod + def _skill_listeners(store: Any) -> list[Any]: + return list(store._listeners.get(SKILL_OBJECT_KIND, [])) + + async def test_a_closed_watcher_is_no_longer_notified( + self, endpoint: Any, tmp_path: Any + ) -> None: + endpoint.queue_poll(full_payload(("put-object", put_skill(content="first")))) + endpoint.queue_poll(status=304) + endpoint.queue_poll( + events( + ("server-intent", server_intent("xfer-full")), + ("put-object", put_skill(object_version=4, content="second")), + ("payload-transferred", transferred("basis-2")), + ) + ) + endpoint.queue_poll(status=304) + + with poll_store(endpoint, poll_interval=0.1) as store: + store.wait_for_skills(timeout=5) + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + assert watcher.notify in self._skill_listeners(store) + + watcher.close() + + assert watcher.notify not in self._skill_listeners(store) + written = tmp_path / "s" / "pdf-extraction" / "SKILL.md" + assert wait_until( + lambda: ( + store.get_object(SKILL_OBJECT_KIND, "pdf-extraction", 4) is not None + ), + timeout=10, + ) + time.sleep(0.3) + assert written.read_text() == "first" + assert watcher.reconciles == 0 + + async def test_close_twice_does_not_raise(self, tmp_path: Any) -> None: + store = InMemorySkillStore() + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + watcher.close() + watcher.close() + assert self._skill_listeners(store) == [] + + async def test_repeated_watchers_leave_no_listeners_behind( + self, tmp_path: Any + ) -> None: + store = InMemorySkillStore() + await init_client(options={"skillStore": store}, client=object()) + for _ in range(5): + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + assert len(self._skill_listeners(store)) == 1 + watcher.close() + assert self._skill_listeners(store) == [] + + async def test_a_store_without_remove_listener_still_closes( + self, tmp_path: Any + ) -> None: + """``remove_listener`` is optional: an older store keeps working, at the + cost of the listener staying registered.""" + + class AddOnly: + def __init__(self) -> None: + self.listeners: list[Any] = [] + + def get_object(self, *_a: Any, **_k: Any) -> None: + return None + + def all_objects(self, _kind: str) -> dict[str, Any]: + return {} + + def add_listener(self, _kind: str, fn: Any) -> None: + self.listeners.append(fn) + + store = AddOnly() + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + assert store.listeners == [watcher.notify] + + watcher.close() + watcher.close() + + assert store.listeners == [watcher.notify] + + def test_fdv2_remove_listener_of_an_unregistered_callable_is_a_no_op( + self, endpoint: Any + ) -> None: + with poll_store(endpoint) as store: + store.remove_listener(SKILL_OBJECT_KIND, print) + store.add_listener(SKILL_OBJECT_KIND, print) + store.remove_listener("flag", print) + store.remove_listener(SKILL_OBJECT_KIND, print) + store.remove_listener(SKILL_OBJECT_KIND, print) + assert self._skill_listeners(store) == [] + + # --------------------------------------------------------------------------- # Lifecycle # --------------------------------------------------------------------------- From 9d4c930bb3a3fd3ed1bd24051d884a8823b305d9 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 15:06:30 -0400 Subject: [PATCH 08/13] =?UTF-8?q?docs(client):=20Agent=20Skills=20?= =?UTF-8?q?=E2=80=94=20finish=20the=20customer-facing=20prose=20pass?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes out the remaining convention drift in the FDv2 transport and watcher modules: British spelling for deserialisation/serialised, the one PR-added "seam" in agents.md, a missing `import os` in the README example, and the FDv2SkillStore.close docstring now names the package-level shutdown() coroutine so it no longer reads as a method on the store. Restores the public note that `mv` is the one request parameter not confirmed against a live server. Co-Authored-By: Claude Fable 5.1 --- packages/client/README.md | 2 ++ packages/client/agents.md | 2 +- .../client/src/launchdarkly_ai_server/skills_fdv2.py | 9 ++++++--- .../client/src/launchdarkly_ai_server/skills_watch.py | 2 +- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 862b38f..0325667 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -453,6 +453,8 @@ channel — the same `GET /sdk/poll` and `GET /sdk/stream` endpoints the base SD source uses, authenticated with the environment's server-side SDK key. ```python +import os + from launchdarkly_ai_server import FDv2SkillStore, init_client, watch_skills store = FDv2SkillStore(os.environ["LD_SDK_KEY"]).start() diff --git a/packages/client/agents.md b/packages/client/agents.md index 9c1f965..92c049b 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -204,7 +204,7 @@ Three layers, in increasing order of blast radius: pair `add_listener(kind, fn)` / `remove_listener(kind, fn)`. Version is part of the **lookup identity**, not a filter applied to the answer, and that is load-bearing: a delivery payload carries the newest version of every skill *plus* every version any variation currently pins, -so two versions of one key coexist routinely. A seam keyed by key alone would answer a pinned reference with the newest +so two versions of one key coexist routinely. A store keyed by key alone would answer a pinned reference with the newest object, and the caller would then have to reject it — turning the primary use case, a version-pinned attachment, into a missing skill. `version=None` asks for the newest held. diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index 2406e20..d0ce6d6 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -87,6 +87,8 @@ """ The ``mv`` request parameter — the SDK data model version this adapter speaks. +Of the request parameters this adapter sends, it is the one whose value could +not be confirmed against a live server, so treat the default as provisional. Override it with ``FDv2SkillStore(data_model_version=...)`` if a LaunchDarkly instance expects a different value. """ @@ -229,7 +231,7 @@ class StoreDiagnostics: # --------------------------------------------------------------------------- -# Deserialization — where objectVersion is not version +# Deserialisation — where objectVersion is not version # --------------------------------------------------------------------------- @@ -1199,8 +1201,9 @@ def close(self, timeout: float = 5.0) -> None: Held content is *not* dropped: a closed store still answers from what it received, so shutting the transport down does not turn into an integrity - failure or an empty reconcile mid-flight. ``shutdown()`` is what detaches - the store from the accessors. + failure or an empty reconcile mid-flight. Detaching the store from the + accessors is the job of the package-level ``launchdarkly_ai_server.shutdown()`` + coroutine, not of this method. """ self._stop.set() # Interrupt the read before joining. The delivery thread is normally diff --git a/packages/client/src/launchdarkly_ai_server/skills_watch.py b/packages/client/src/launchdarkly_ai_server/skills_watch.py index d9461ca..e421df6 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_watch.py +++ b/packages/client/src/launchdarkly_ai_server/skills_watch.py @@ -56,7 +56,7 @@ class SkillWatcher: reconcile's own contract is one root, one reconcile at a time, because two interleaved runs lose the loser's manifest entries and leave the files it wrote unmanaged. This class enforces that for its *own* reconciles — they run - on a single worker thread, serialized — and cannot enforce it against a + on a single worker thread, serialised — and cannot enforce it against a caller who reconciles the same root by hand. The watcher owns its registration on *store*: it registers ``notify`` when From 784bd5d9fc4b04ac362953875ce6dead847edb1e Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 15:10:14 -0400 Subject: [PATCH 09/13] =?UTF-8?q?refactor(client):=20Agent=20Skills=20?= =?UTF-8?q?=E2=80=94=20remove=20four=20pieces=20of=20dead=20code=20from=20?= =?UTF-8?q?the=20FDv2=20transport?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop the unused `_Change` dataclass. Listeners have always received the raw skill object or a `{"key", "version"}` tombstone, as documented on `FDv2SkillStore.add_listener`; nothing constructed or referenced the class. - Drop the `hashless_before_this_payload` parameter that `_warn_if_nothing_can_verify` discarded on entry, and the caller's local that existed only to pass it. - Drop the unreachable `except _FatalTransportError: raise` clauses in `_Requester.poll` and `_Requester.stream`. `_classify_status` raises from inside the sibling `HTTPError` handler, which the same `try` never catches, and no test double raises it through the opener. No behaviour change. The recoverable catch-all is untouched. Co-Authored-By: Claude Fable 5.1 --- .../src/launchdarkly_ai_server/skills_fdv2.py | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index d0ce6d6..ba53527 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -447,13 +447,6 @@ def __len__(self) -> int: # --------------------------------------------------------------------------- -@dataclass -class _Change: - """One committed change, as handed to a listener.""" - - raw: dict[str, Any] - - @dataclass class _TransferOutcome: """What one event did. Aggregated by the caller; nothing here does I/O.""" @@ -598,9 +591,8 @@ def _payload_transferred(self, data: Any) -> _TransferOutcome: state = data.get("state") if isinstance(data, dict) else None version = data.get("version") if isinstance(data, dict) else None if self._pending is not None: - hashless_before = self.diagnostics.hashless_objects self._committed.replace_with(self._pending) - _warn_if_nothing_can_verify(self._committed, hashless_before) + _warn_if_nothing_can_verify(self._committed) self._pending = None self._intent = None changes = self._changes @@ -677,9 +669,7 @@ def _warn_hashless(self, raw: dict[str, Any]) -> None: ) -def _warn_if_nothing_can_verify( - committed: _SkillObjectSet, hashless_before_this_payload: int -) -> None: +def _warn_if_nothing_can_verify(committed: _SkillObjectSet) -> None: """ One ERROR per committed payload in which *nothing* the store now holds can possibly verify. @@ -689,7 +679,6 @@ def _warn_if_nothing_can_verify( so the condition is visible in a process that boots, materializes nothing, and exits, which is a common way a skills deployment fails. """ - del hashless_before_this_payload # counted for the store, not for this check held = committed.all_raw() if not held: return @@ -909,8 +898,6 @@ def poll(self, basis: str | None, etag: str | None) -> _PollResult: # urllib raises on 304 when no redirect handler swallows it. return _PollResult(not_modified=True, events=[], etag=etag) raise _classify_status(exc.code, exc.headers) from exc - except _FatalTransportError: - raise except Exception as exc: raise _RecoverableTransportError( f"polling request failed: {type(exc).__name__}: {exc}" @@ -936,8 +923,6 @@ def stream(self, basis: str | None) -> _StreamConnection: response = self._opener.open(request, timeout=self._read_timeout) except urllib.error.HTTPError as exc: raise _classify_status(exc.code, exc.headers) from exc - except _FatalTransportError: - raise except Exception as exc: raise _RecoverableTransportError( f"streaming request failed: {type(exc).__name__}: {exc}" From a2a7fb75973af572c89f4fe64391d0161b7820e8 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 15:20:12 -0400 Subject: [PATCH 10/13] =?UTF-8?q?test(client):=20Agent=20Skills=20?= =?UTF-8?q?=E2=80=94=20give=20skills=5Fwatch=20its=20own=20test=20module?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the watch_skills / SkillWatcher tests out of test_skills_fdv2.py into test_skills_watch.py, matching the one-test-file-per-module convention. The watcher is wired to the SkillStore interface, not to the FDv2 transport, so the moved tests drive it from InMemorySkillStore and small store doubles. The three cases that only mean something with a transport underneath — a wire-level revocation, an objectVersion bump, and an outage — stay in test_skills_fdv2.py as TestWatchSkillsOverTheTransport. The remove_listener no-op test is a store test and moves to TestListenerRegistration there. No assertion changes. 141 tests before and after (133 + 8). Co-Authored-By: Claude Fable 5.1 --- packages/client/tests/test_skills_fdv2.py | 168 ++----------------- packages/client/tests/test_skills_watch.py | 181 +++++++++++++++++++++ 2 files changed, 195 insertions(+), 154 deletions(-) create mode 100644 packages/client/tests/test_skills_watch.py diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index e9f0ed9..2549ac9 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -1610,11 +1610,19 @@ def test_an_unknown_mode_is_refused(self) -> None: # --------------------------------------------------------------------------- -# The eager re-reconcile +# The eager re-reconcile, end to end over the transport # --------------------------------------------------------------------------- -class TestWatchSkills: +class TestWatchSkillsOverTheTransport: + """ + ``watch_skills`` against a live ``FDv2SkillStore``. The watcher's own + behaviour — debounce, refusal of a store without ``add_listener``, detaching + on close — is covered in ``test_skills_watch.py`` against the in-memory + store; these are the cases that only mean something with a transport + underneath: a wire-level revocation, an ``objectVersion`` bump, and an outage. + """ + async def test_a_revocation_prunes_without_a_restart( self, endpoint: Any, tmp_path: Any ) -> None: @@ -1670,25 +1678,6 @@ async def test_a_new_version_is_rewritten_without_a_restart( finally: watcher.close() - async def test_a_burst_of_changes_coalesces_into_few_reconciles( - self, endpoint: Any, tmp_path: Any - ) -> None: - endpoint.queue_poll( - full_payload(*[("put-object", put_skill(f"skill-{i}")) for i in range(12)]) - ) - endpoint.queue_poll(status=304) - with poll_store(endpoint) as store: - store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) - _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.1) - try: - time.sleep(0.5) - # Twelve objects committed in one payload fire twelve listener - # calls; without coalescing that is twelve reconciles of one root. - assert watcher.reconciles <= 2 - finally: - watcher.close() - async def test_the_default_keeps_last_known_good_during_an_outage( self, endpoint: Any, tmp_path: Any ) -> None: @@ -1714,146 +1703,17 @@ async def test_the_default_keeps_last_known_good_during_an_outage( finally: watcher.close() - async def test_a_store_with_no_listener_support_is_refused_loudly( - self, tmp_path: Any - ) -> None: - class NoListeners: - def get_object(self, *_a: Any, **_k: Any) -> None: - return None - - def all_objects(self, _kind: str) -> dict[str, Any]: - return {} - - await init_client(options={"skillStore": NoListeners()}, client=object()) - with pytest.raises(RuntimeError, match="add_listener"): - await watch_skills("*", tmp_path / "s") - - async def test_no_store_configured_raises(self, tmp_path: Any) -> None: - with pytest.raises(RuntimeError, match="configured skill store"): - await watch_skills("*", tmp_path / "s") - - async def test_the_in_memory_store_can_also_drive_a_watch( - self, tmp_path: Any - ) -> None: - """The watcher is wired to the ``SkillStore`` interface, not to the FDv2 - store.""" - store = InMemorySkillStore() - store.put( - { - "key": "a", - "version": 1, - "content": "body", - "contentHash": _hash("body"), - } - ) - await init_client(options={"skillStore": store}, client=object()) - _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) - try: - written = tmp_path / "s" / "a" / "SKILL.md" - assert written.read_text() == "body" - store.put( - { - "key": "a", - "version": 2, - "content": "new body", - "contentHash": _hash("new body"), - } - ) - assert wait_until(lambda: written.read_text() == "new body", timeout=10) - finally: - watcher.close() +# --------------------------------------------------------------------------- +# Listener registration +# --------------------------------------------------------------------------- -class TestWatcherDetachesOnClose: - """``SkillWatcher.close`` unregisters ``notify``, so a closed watcher is - neither called nor kept alive by the store.""" +class TestListenerRegistration: @staticmethod def _skill_listeners(store: Any) -> list[Any]: return list(store._listeners.get(SKILL_OBJECT_KIND, [])) - async def test_a_closed_watcher_is_no_longer_notified( - self, endpoint: Any, tmp_path: Any - ) -> None: - endpoint.queue_poll(full_payload(("put-object", put_skill(content="first")))) - endpoint.queue_poll(status=304) - endpoint.queue_poll( - events( - ("server-intent", server_intent("xfer-full")), - ("put-object", put_skill(object_version=4, content="second")), - ("payload-transferred", transferred("basis-2")), - ) - ) - endpoint.queue_poll(status=304) - - with poll_store(endpoint, poll_interval=0.1) as store: - store.wait_for_skills(timeout=5) - await init_client(options={"skillStore": store}, client=object()) - _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) - assert watcher.notify in self._skill_listeners(store) - - watcher.close() - - assert watcher.notify not in self._skill_listeners(store) - written = tmp_path / "s" / "pdf-extraction" / "SKILL.md" - assert wait_until( - lambda: ( - store.get_object(SKILL_OBJECT_KIND, "pdf-extraction", 4) is not None - ), - timeout=10, - ) - time.sleep(0.3) - assert written.read_text() == "first" - assert watcher.reconciles == 0 - - async def test_close_twice_does_not_raise(self, tmp_path: Any) -> None: - store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) - _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) - watcher.close() - watcher.close() - assert self._skill_listeners(store) == [] - - async def test_repeated_watchers_leave_no_listeners_behind( - self, tmp_path: Any - ) -> None: - store = InMemorySkillStore() - await init_client(options={"skillStore": store}, client=object()) - for _ in range(5): - _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) - assert len(self._skill_listeners(store)) == 1 - watcher.close() - assert self._skill_listeners(store) == [] - - async def test_a_store_without_remove_listener_still_closes( - self, tmp_path: Any - ) -> None: - """``remove_listener`` is optional: an older store keeps working, at the - cost of the listener staying registered.""" - - class AddOnly: - def __init__(self) -> None: - self.listeners: list[Any] = [] - - def get_object(self, *_a: Any, **_k: Any) -> None: - return None - - def all_objects(self, _kind: str) -> dict[str, Any]: - return {} - - def add_listener(self, _kind: str, fn: Any) -> None: - self.listeners.append(fn) - - store = AddOnly() - await init_client(options={"skillStore": store}, client=object()) - _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) - assert store.listeners == [watcher.notify] - - watcher.close() - watcher.close() - - assert store.listeners == [watcher.notify] - def test_fdv2_remove_listener_of_an_unregistered_callable_is_a_no_op( self, endpoint: Any ) -> None: diff --git a/packages/client/tests/test_skills_watch.py b/packages/client/tests/test_skills_watch.py new file mode 100644 index 0000000..b44ffea --- /dev/null +++ b/packages/client/tests/test_skills_watch.py @@ -0,0 +1,181 @@ +""" +Tests for ``watch_skills`` / ``SkillWatcher`` — the eager re-reconcile. + +The watcher is wired to the ``SkillStore`` interface, not to any one transport: +it needs a store that implements ``add_listener``, and nothing more. These tests +therefore drive it from ``InMemorySkillStore``, whose ``put`` notifies its +listeners synchronously, and from small hand-written store doubles. The +end-to-end path — a ``delete-object`` arriving over a live FDv2 connection and +pruning a skill's files — is exercised in ``test_skills_fdv2.py``, where the fake +endpoint lives. + +Every test writes only inside pytest's ``tmp_path``. The watcher runs a real +worker thread, so tests wait on observable outcomes rather than on fixed sleeps +wherever the outcome is something that *does* happen; a fixed sleep is used only +to assert that something does *not*. +""" + +from __future__ import annotations + +import time +from typing import Any + +import pytest + +from launchdarkly_ai_server import InMemorySkillStore, init_client, watch_skills +from launchdarkly_ai_server.skills_core import SKILL_OBJECT_KIND + +pytestmark = pytest.mark.usefixtures("reset_skill_state") + + +def wait_until(predicate: Any, timeout: float = 5.0) -> bool: + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return True + time.sleep(0.01) + return predicate() + + +# --------------------------------------------------------------------------- +# Starting a watch, and what it refuses +# --------------------------------------------------------------------------- + + +class TestWatchSkills: + async def test_the_in_memory_store_can_also_drive_a_watch( + self, tmp_path: Any, make_raw_skill: Any + ) -> None: + """The watcher is wired to the ``SkillStore`` interface, not to the FDv2 + store.""" + store = InMemorySkillStore() + store.put(make_raw_skill(key="a", version=1, content="body")) + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + try: + written = tmp_path / "s" / "a" / "SKILL.md" + assert written.read_text() == "body" + store.put(make_raw_skill(key="a", version=2, content="new body")) + assert wait_until(lambda: written.read_text() == "new body", timeout=10) + finally: + watcher.close() + + async def test_a_burst_of_changes_coalesces_into_few_reconciles( + self, tmp_path: Any, make_raw_skill: Any + ) -> None: + store = InMemorySkillStore() + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.1) + try: + for i in range(12): + store.put(make_raw_skill(key=f"skill-{i}")) + time.sleep(0.5) + # Twelve objects put back to back fire twelve listener calls; without + # coalescing that is twelve reconciles of one root. + assert watcher.reconciles <= 2 + finally: + watcher.close() + + async def test_a_store_with_no_listener_support_is_refused_loudly( + self, tmp_path: Any + ) -> None: + class NoListeners: + def get_object(self, *_a: Any, **_k: Any) -> None: + return None + + def all_objects(self, _kind: str) -> dict[str, Any]: + return {} + + await init_client(options={"skillStore": NoListeners()}, client=object()) + with pytest.raises(RuntimeError, match="add_listener"): + await watch_skills("*", tmp_path / "s") + + async def test_no_store_configured_raises(self, tmp_path: Any) -> None: + with pytest.raises(RuntimeError, match="configured skill store"): + await watch_skills("*", tmp_path / "s") + + +# --------------------------------------------------------------------------- +# Closing a watch +# --------------------------------------------------------------------------- + + +class TestWatcherDetachesOnClose: + """``SkillWatcher.close`` unregisters ``notify``, so a closed watcher is + neither called nor kept alive by the store.""" + + @staticmethod + def _skill_listeners(store: Any) -> list[Any]: + return list(store._listeners.get(SKILL_OBJECT_KIND, [])) + + async def test_a_closed_watcher_is_no_longer_notified( + self, tmp_path: Any, make_raw_skill: Any + ) -> None: + store = InMemorySkillStore() + store.put(make_raw_skill(key="pdf-extraction", version=1, content="first")) + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + assert watcher.notify in self._skill_listeners(store) + + watcher.close() + + assert watcher.notify not in self._skill_listeners(store) + store.put(make_raw_skill(key="pdf-extraction", version=4, content="second")) + written = tmp_path / "s" / "pdf-extraction" / "SKILL.md" + assert wait_until( + lambda: ( + store.get_object(SKILL_OBJECT_KIND, "pdf-extraction", 4) is not None + ), + timeout=10, + ) + time.sleep(0.3) + assert written.read_text() == "first" + assert watcher.reconciles == 0 + + async def test_close_twice_does_not_raise(self, tmp_path: Any) -> None: + store = InMemorySkillStore() + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + watcher.close() + watcher.close() + assert self._skill_listeners(store) == [] + + async def test_repeated_watchers_leave_no_listeners_behind( + self, tmp_path: Any + ) -> None: + store = InMemorySkillStore() + await init_client(options={"skillStore": store}, client=object()) + for _ in range(5): + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + assert len(self._skill_listeners(store)) == 1 + watcher.close() + assert self._skill_listeners(store) == [] + + async def test_a_store_without_remove_listener_still_closes( + self, tmp_path: Any + ) -> None: + """``remove_listener`` is optional: an older store keeps working, at the + cost of the listener staying registered.""" + + class AddOnly: + def __init__(self) -> None: + self.listeners: list[Any] = [] + + def get_object(self, *_a: Any, **_k: Any) -> None: + return None + + def all_objects(self, _kind: str) -> dict[str, Any]: + return {} + + def add_listener(self, _kind: str, fn: Any) -> None: + self.listeners.append(fn) + + store = AddOnly() + await init_client(options={"skillStore": store}, client=object()) + _report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05) + assert store.listeners == [watcher.notify] + + watcher.close() + watcher.close() + + assert store.listeners == [watcher.notify] From 75de089154cd5c1b95abbb5abcef1e1582751c94 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 15:37:16 -0400 Subject: [PATCH 11/13] =?UTF-8?q?fix(client):=20Agent=20Skills=20=E2=80=94?= =?UTF-8?q?=20name=20the=20delivery=20store=20in=20the=20no-store=20messag?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NO_STORE_MESSAGE offered only InMemorySkillStore, so the first thing a user saw on a missing store pointed them at the development store. Now that FDv2SkillStore is the production path, name it first and keep the in-memory store as what it is: local development and testing. Deferred earlier as "a one-line follow-up rather than touching reviewed code here". That reason no longer holds — adding remove_listener already edits this file — and it is cheaper to fix than to explain. Callers match on "skill store", which the new wording keeps. Co-Authored-By: Claude Opus 5 --- .../src/launchdarkly_ai_server/skills_core.py | 13 +++++++++++-- packages/client/tests/test_skills.py | 12 ++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/packages/client/src/launchdarkly_ai_server/skills_core.py b/packages/client/src/launchdarkly_ai_server/skills_core.py index 2216679..1bfc6ac 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_core.py +++ b/packages/client/src/launchdarkly_ai_server/skills_core.py @@ -134,9 +134,18 @@ NO_STORE_MESSAGE = ( "No skill store is configured, so skill content cannot be retrieved. Configure " - 'one with init_client(options={"skillStore": store}) — InMemorySkillStore is ' - "available for local development and testing." + 'one with init_client(options={"skillStore": store}) — FDv2SkillStore receives ' + "content from LaunchDarkly, and InMemorySkillStore is available for local " + "development and testing." ) +""" +The first thing a user sees when no store is configured, so it names both stores. + +``FDv2SkillStore`` comes first because it is the answer in production, and a +message that offered only ``InMemorySkillStore`` would point a deployment at the +development store. Callers match on "skill store"; keep that phrase if the +wording changes. +""" # --------------------------------------------------------------------------- diff --git a/packages/client/tests/test_skills.py b/packages/client/tests/test_skills.py index d3f8309..a07ea6a 100644 --- a/packages/client/tests/test_skills.py +++ b/packages/client/tests/test_skills.py @@ -603,6 +603,18 @@ async def test_all_skills_raises_actionably_when_no_store(self) -> None: with pytest.raises(RuntimeError, match="skill store"): await all_skills() + async def test_the_no_store_message_names_the_delivery_store_first(self) -> None: + """ + A deployment that hits this message must be pointed at the store that + receives content from LaunchDarkly, not only at the development one. + """ + with pytest.raises(RuntimeError) as reported: + await get_skill("a") + message = str(reported.value) + assert "FDv2SkillStore" in message + assert "InMemorySkillStore" in message + assert message.index("FDv2SkillStore") < message.index("InMemorySkillStore") + async def test_shutdown_clears_the_store( self, make_raw_skill: Any, mock_ld_client: Any ) -> None: From bea40e2fe06acd1429ed7621da60d6e5265fdc1d Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 15:37:26 -0400 Subject: [PATCH 12/13] =?UTF-8?q?refactor(client):=20Agent=20Skills=20?= =?UTF-8?q?=E2=80=94=20mark=20the=20FDv2=20module's=20internal=20helpers?= =?UTF-8?q?=20private?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four module-level helpers carried public names while being used only inside skills_fdv2.py and its tests, and were never exported from the package root: is_skill_event -> _is_skill_event seam_object_from_put -> _store_object_from_put tombstone_from_delete -> _tombstone_from_delete backoff_delay -> _backoff_delay tombstone_from_delete was the clearest tell — a public-looking function returning the private _Tombstone. They now read consistently with _SkillObjectSet, _ProtocolReader, and _Requester alongside them. The rename also retires the last "seam" in the module. The prose pass converted every sentence but could not reach the identifier, which agents.md quotes as the single place the objectVersion translation happens; that reference follows the new name. "store" matches the docstring, which already describes the result as the shape the SkillStore interface defines. No behaviour change, and no assertion changes. Co-Authored-By: Claude Opus 5 --- packages/client/agents.md | 2 +- .../src/launchdarkly_ai_server/skills_fdv2.py | 20 ++++---- packages/client/tests/test_skills_fdv2.py | 50 +++++++++---------- 3 files changed, 36 insertions(+), 36 deletions(-) diff --git a/packages/client/agents.md b/packages/client/agents.md index 92c049b..8a1e7d0 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -243,7 +243,7 @@ when anything in the environment moves, including a flag with nothing to do with 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. Flags and segments carry only `version` and omit both `category` and `objectVersion`, which is exactly why the -two fields look interchangeable. `seam_object_from_put` is the only place the translation +two fields look interchangeable. `_store_object_from_put` is the only place the translation happens, and `TestVersionTranslation` asserts it in both directions. **Skills are identified by `kind == "inline-resource" && category == "skill"`; everything else diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index ba53527..8e79682 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -243,7 +243,7 @@ class _Tombstone: object_version: int | None -def is_skill_event(data: Any) -> bool: +def _is_skill_event(data: Any) -> bool: """ Whether one ``put-object`` / ``delete-object`` payload is a skill. @@ -264,7 +264,7 @@ def is_skill_event(data: Any) -> bool: ) -def seam_object_from_put(data: dict[str, Any]) -> dict[str, Any] | None: +def _store_object_from_put(data: dict[str, Any]) -> dict[str, Any] | None: """ Translates one FDv2 skill ``put-object`` into the raw object shape that the ``SkillStore`` interface defines. @@ -315,7 +315,7 @@ def seam_object_from_put(data: dict[str, Any]) -> dict[str, Any] | None: return raw -def tombstone_from_delete(data: dict[str, Any]) -> _Tombstone | None: +def _tombstone_from_delete(data: dict[str, Any]) -> _Tombstone | None: """ Narrows one FDv2 skill ``delete-object`` to the identity it revokes. @@ -384,7 +384,7 @@ def delete(self, tombstone: _Tombstone) -> list[dict[str, Any]]: Removes what *tombstone* revokes; returns the raw objects that went away. A tombstone with no usable version removes every version of the key — see - ``tombstone_from_delete`` for why that is the safe reading. + ``_tombstone_from_delete`` for why that is the safe reading. """ removed: list[dict[str, Any]] = [] if tombstone.object_version is None: @@ -543,7 +543,7 @@ def _target(self) -> _SkillObjectSet | None: return self._pending def _put_object(self, data: Any) -> _TransferOutcome: - if not is_skill_event(data): + if not _is_skill_event(data): self.diagnostics.objects_ignored += 1 return _TransferOutcome() if self._pending is None and self._intent is None: @@ -552,7 +552,7 @@ def _put_object(self, data: Any) -> _TransferOutcome: if target is None: return _TransferOutcome() - raw = seam_object_from_put(data) + raw = _store_object_from_put(data) if raw is None: return _TransferOutcome() target.put(raw) @@ -564,7 +564,7 @@ def _put_object(self, data: Any) -> _TransferOutcome: return _TransferOutcome() def _delete_object(self, data: Any) -> _TransferOutcome: - if not is_skill_event(data): + if not _is_skill_event(data): self.diagnostics.objects_ignored += 1 return _TransferOutcome() if self._pending is None and self._intent is None: @@ -573,7 +573,7 @@ def _delete_object(self, data: Any) -> _TransferOutcome: if target is None: return _TransferOutcome() - tombstone = tombstone_from_delete(data) + tombstone = _tombstone_from_delete(data) if tombstone is None: return _TransferOutcome() target.delete(tombstone) @@ -1004,7 +1004,7 @@ def _iter_sse(response: Any) -> Any: # --------------------------------------------------------------------------- -def backoff_delay( +def _backoff_delay( attempt: int, *, base: float, maximum: float, jitter: float = 0.5 ) -> float: """ @@ -1337,7 +1337,7 @@ def _run(self) -> None: return delay = exc.retry_after if delay is None or not math.isfinite(delay): - delay = backoff_delay( + delay = _backoff_delay( failures, base=self._initial_backoff, maximum=self._max_backoff ) # ``Retry-After`` is a request, and ``max_backoff`` is a promise. diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index 2549ac9..ee62fef 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -43,15 +43,15 @@ DEFAULT_STREAM_READ_TIMEOUT, FDV2_OBJECT_CATEGORY, FDV2_OBJECT_KIND, + _backoff_delay, + _is_skill_event, _ProtocolReader, _RecoverableTransportError, _Requester, _retry_after_seconds, _SkillObjectSet, - backoff_delay, - is_skill_event, - seam_object_from_put, - tombstone_from_delete, + _store_object_from_put, + _tombstone_from_delete, ) pytestmark = pytest.mark.usefixtures("reset_skill_state") @@ -347,24 +347,24 @@ def wait_until(predicate: Any, timeout: float = 5.0) -> bool: class TestObjectIdentification: def test_kind_and_category_together_identify_a_skill(self) -> None: - assert is_skill_event(put_skill()) is True + assert _is_skill_event(put_skill()) is True def test_a_flag_is_not_a_skill(self) -> None: - assert is_skill_event(put_flag()) is False + assert _is_skill_event(put_flag()) is False def test_a_segment_is_not_a_skill(self) -> None: - assert is_skill_event(put_segment()) is False + assert _is_skill_event(put_segment()) is False def test_inline_resource_of_another_category_is_not_a_skill(self) -> None: """``inline-resource`` is a broad kind, so the category is required too.""" other = put_skill() other["category"] = "prompt-template" - assert is_skill_event(other) is False + assert _is_skill_event(other) is False def test_skill_category_under_another_kind_is_not_a_skill(self) -> None: other = put_skill() other["kind"] = "some-future-kind" - assert is_skill_event(other) is False + assert _is_skill_event(other) is False def test_a_flag_shaped_object_with_no_category_is_not_a_skill(self) -> None: """Flags and segments omit ``category`` entirely — the documented shape.""" @@ -373,7 +373,7 @@ def test_a_flag_shaped_object_with_no_category_is_not_a_skill(self) -> None: @pytest.mark.parametrize("value", [None, "skill", 3, [], ()]) def test_non_dict_events_are_not_skills(self, value: Any) -> None: - assert is_skill_event(value) is False + assert _is_skill_event(value) is False # --------------------------------------------------------------------------- @@ -383,7 +383,7 @@ def test_non_dict_events_are_not_skills(self, value: Any) -> None: class TestVersionTranslation: def test_object_version_becomes_the_seam_version(self) -> None: - raw = seam_object_from_put(put_skill(object_version=3, payload_version=42)) + raw = _store_object_from_put(put_skill(object_version=3, payload_version=42)) assert raw is not None assert raw["version"] == 3 @@ -393,7 +393,7 @@ def test_the_payload_version_never_reaches_the_seam(self) -> None: would serve verifiable content under a version number that means nothing, and every pinned reference would resolve to the wrong thing with no error. """ - raw = seam_object_from_put(put_skill(object_version=3, payload_version=42)) + raw = _store_object_from_put(put_skill(object_version=3, payload_version=42)) assert raw is not None assert raw["version"] != 42 assert 42 not in raw.values() @@ -401,42 +401,42 @@ def test_the_payload_version_never_reaches_the_seam(self) -> None: def test_the_two_are_distinguished_even_when_the_payload_version_is_lower( self, ) -> None: - raw = seam_object_from_put(put_skill(object_version=99, payload_version=1)) + raw = _store_object_from_put(put_skill(object_version=99, payload_version=1)) assert raw is not None assert raw["version"] == 99 def test_a_missing_object_version_is_not_defaulted_from_the_payload(self) -> None: wire = put_skill() del wire["objectVersion"] - raw = seam_object_from_put(wire) + raw = _store_object_from_put(wire) assert raw is not None assert "version" not in raw def test_an_explicitly_null_object_version_is_carried_through_as_null(self) -> None: """Carried, not invented: verification reports ``invalid_version``.""" - raw = seam_object_from_put(put_skill(object_version=None)) + raw = _store_object_from_put(put_skill(object_version=None)) assert raw is not None assert raw["version"] is None def test_a_delete_translates_object_version_too(self) -> None: - tombstone = tombstone_from_delete( + tombstone = _tombstone_from_delete( delete_skill(object_version=3, payload_version=43) ) assert tombstone is not None assert tombstone.object_version == 3 def test_a_delete_with_no_usable_object_version_revokes_every_version(self) -> None: - tombstone = tombstone_from_delete(delete_skill(object_version=None)) + tombstone = _tombstone_from_delete(delete_skill(object_version=None)) assert tombstone is not None assert tombstone.object_version is None def test_a_keyless_put_is_dropped_because_it_has_no_identity(self) -> None: wire = put_skill() del wire["key"] - assert seam_object_from_put(wire) is None + assert _store_object_from_put(wire) is None def test_the_envelope_is_copied_verbatim(self) -> None: - raw = seam_object_from_put(put_skill()) + raw = _store_object_from_put(put_skill()) assert raw is not None assert raw["content"] == SKILL_BODY assert raw["contentHash"] == _hash(SKILL_BODY) @@ -446,7 +446,7 @@ def test_the_envelope_is_copied_verbatim(self) -> None: def test_an_absent_envelope_field_is_absent_rather_than_defaulted(self) -> None: wire = put_skill() del wire["object"]["name"] - raw = seam_object_from_put(wire) + raw = _store_object_from_put(wire) assert raw is not None assert "name" not in raw @@ -1289,15 +1289,15 @@ def test_a_non_finite_retry_after_off_the_wire_falls_back_to_backoff( assert store.failed is None def test_backoff_is_exponential_and_capped(self) -> None: - assert backoff_delay(1, base=1.0, maximum=30.0, jitter=0.0) == 1.0 - assert backoff_delay(2, base=1.0, maximum=30.0, jitter=0.0) == 2.0 - assert backoff_delay(3, base=1.0, maximum=30.0, jitter=0.0) == 4.0 - assert backoff_delay(20, base=1.0, maximum=30.0, jitter=0.0) == 30.0 + assert _backoff_delay(1, base=1.0, maximum=30.0, jitter=0.0) == 1.0 + assert _backoff_delay(2, base=1.0, maximum=30.0, jitter=0.0) == 2.0 + assert _backoff_delay(3, base=1.0, maximum=30.0, jitter=0.0) == 4.0 + assert _backoff_delay(20, base=1.0, maximum=30.0, jitter=0.0) == 30.0 def test_jitter_never_exceeds_the_cap(self) -> None: for attempt in range(1, 12): for _ in range(50): - assert 0.0 <= backoff_delay(attempt, base=1.0, maximum=5.0) <= 5.0 + assert 0.0 <= _backoff_delay(attempt, base=1.0, maximum=5.0) <= 5.0 def test_a_malformed_polling_envelope_is_recoverable_not_fatal( self, endpoint: Any From 4cbe02e3d228b5dbda444602c4191a70360c53ef Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 10 Sep 2026 16:00:01 -0400 Subject: [PATCH 13/13] =?UTF-8?q?refactor(client):=20Agent=20Skills=20?= =?UTF-8?q?=E2=80=94=20one=20home=20per=20argument=20in=20the=20FDv2=20tra?= =?UTF-8?q?nsport?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module made the same five points in many places: hashless objects are held not dropped, changes commit at payload-transferred, there is one network timeout, close must interrupt the socket, and standard library only. Each now lives in agents.md for the rationale and in the one docstring nearest the decision for the code-level reason; everywhere else points at those. The bare string literals after attribute assignments, which read as docstrings but are no-op expressions, become comments. Two small code duplications go with it: the put and delete handlers share their skill-event preamble, and the poll and stream loops no longer each map a transfer outcome onto an exception. The README section loses the paragraphs that restated agents.md. Co-Authored-By: Claude Fable 5.1 --- packages/client/README.md | 25 +- .../src/launchdarkly_ai_server/skills_fdv2.py | 625 +++++++----------- 2 files changed, 245 insertions(+), 405 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 0325667..42b1f40 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -470,10 +470,8 @@ finally: store.close() ``` -**Nothing above the store changes.** The accessors, integrity verification, and -`write_skills` are transport-agnostic: they see raw objects through the `SkillStore` -interface and cannot tell which store produced them. Everything documented above about verification and -reconcile semantics applies unchanged. +**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. **Server-side only.** Skills are for server-side agent runtimes and skill content is customer-confidential. A mobile key (`mob-…`) or a client-side environment ID raises from the @@ -487,13 +485,9 @@ outage the store keeps serving the last content it received and `write_skills`' was revoked". **One network timeout, and its default depends on the mode.** `read_timeout` bounds every -socket operation of a request — connecting, waiting for headers, and each read — because the -standard library offers no separate connect timeout, and the store deliberately adds no HTTP -client dependency to provide one. In `mode="poll"` it therefore bounds the whole request and -defaults to 10 seconds, so a poll against a host that never answers fails in that time and is -retried. In `mode="stream"` it bounds each wait for the next bytes and defaults to 300 seconds: -a stream is meant to sit idle between events, and LaunchDarkly's heartbeats arrive well inside -that, so a stream silent for longer has genuinely gone. Pass `read_timeout` to override either. +socket operation of a request, connecting included. In `mode="poll"` it bounds the whole +request and defaults to 10 seconds; in `mode="stream"` it bounds each wait for the next bytes +and defaults to 300 seconds, well beyond LaunchDarkly's heartbeat interval. **The connection also carries your flags.** A client cannot request only the skill payload, so a skills-enabled environment delivers flag and segment objects on the same connection. @@ -507,12 +501,9 @@ They are skipped, not evaluated — this store does no evaluation of any kind > does not speak the FDv2 endpoints, so relay-only deployments cannot receive skills. **If every skill comes back empty, check `diagnostics.hashless_objects`.** Verification -requires `contentHash` on the delivered object and withholds anything without one, so a -nonzero count there means skills are being withheld rather than that the environment has -none. The store logs an error per hashless object and one summary per wholly-hashless -payload, both naming the reason. There is deliberately no fallback that skips verification: a -hash the SDK computed from the content it was handed would certify the content against -itself. +withholds any delivered object without a `contentHash`, so a nonzero count means skills are +being withheld rather than that the environment has none. The store also logs an error per +hashless object naming the reason. There is deliberately no fallback that skips verification. **Total path length is yours to bound, not the SDK's.** The 255-byte bound above is per *component*; the root is your path, so `` + `` + `/SKILL.md` can still exceed diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index 8e79682..660da34 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -15,25 +15,25 @@ GET /sdk/poll, GET /sdk/stream, authenticated with the environment's server-side SDK key -Dependencies run one way. This module imports ``skills_core`` for the -interface's kind constant and nothing else from the feature; ``skills.py`` and -``skills_fs.py`` do not import it. It uses only the standard library, so the -content path adds no dependency to a package whose sole runtime dependency is -``opentelemetry-api`` and whose LaunchDarkly base-SDK dependency is optional. +Dependencies run one way: this module imports ``skills_core`` for the +interface's kind constant and nothing else from the feature, and nothing in the +feature imports it. It uses only the standard library, so it adds no dependency +to a package whose sole runtime dependency is ``opentelemetry-api``. -What this module does *not* do, on purpose: +Three things this module does *not* do, on purpose: - **It does not verify content.** Verification lives at the accessor boundary in - ``skills_core`` so that it applies to every store equally, including - ``InMemorySkillStore`` and a customer's own. A transport that verified would - make integrity depend on which store you configured. -- **It does not skip verification when the wire envelope has no - ``contentHash``.** See ``_SkillObjectSet.put`` and ``StoreDiagnostics``: a - hashless object is stored verbatim and *withheld* by verification with - ``missing_content_hash``. Making that outcome visible is this module's job; - working around it is not. -- **It does not evaluate anything.** No flags, no segments, no targeting. Skills - have no targeting; the SDK key fully determines the payload. + ``skills_core`` so that it applies to every store equally, including a + customer's own. +- **It does not work around a missing ``contentHash``.** A hashless object is + held verbatim and *withheld* by verification with ``missing_content_hash``. + This module's job is to make that outcome loud — see ``StoreDiagnostics``. +- **It does not evaluate anything.** Flag and segment objects that share the + connection are skipped and counted, nothing more. + +The design rationale — why ``objectVersion`` is not ``version``, why changes +commit at ``payload-transferred``, why there is one network timeout — is in +``agents.md`` under *The delivery transport*. """ from __future__ import annotations @@ -64,47 +64,34 @@ FDV2_OBJECT_KIND = "inline-resource" """ -The FDv2 ``kind`` skills are delivered under. - -Distinct from ``skills_core.SKILL_OBJECT_KIND`` (``"skill"``), which is the value -the SDK asks a store for. Translating this pair — kind ``inline-resource`` plus -category ``skill`` — onto that single value is the adapter's job, which is why -``SKILL_OBJECT_KIND`` is documented as an interface value rather than as the wire -contract. +The FDv2 ``kind`` skills are delivered under. Together with +``FDV2_OBJECT_CATEGORY`` it maps onto the single interface value +``skills_core.SKILL_OBJECT_KIND``; that translation is this adapter's job. """ FDV2_OBJECT_CATEGORY = "skill" """The ``category`` that narrows ``inline-resource`` to an agent skill.""" DEFAULT_BASE_URI = "https://sdk.launchdarkly.com" -"""Where the SDK-facing FDv2 endpoints live. Overridable for Federal instances, -private instances, and the fake endpoint the tests run against.""" +"""Where the SDK-facing FDv2 endpoints live. Overridable for Federal and private +instances.""" POLL_PATH = "/sdk/poll" STREAM_PATH = "/sdk/stream" SDK_DATA_MODEL_VERSION = 1 """ -The ``mv`` request parameter — the SDK data model version this adapter speaks. - -Of the request parameters this adapter sends, it is the one whose value could -not be confirmed against a live server, so treat the default as provisional. -Override it with ``FDv2SkillStore(data_model_version=...)`` if a LaunchDarkly -instance expects a different value. +The ``mv`` request parameter. The one request parameter whose value could not be +confirmed against a live server, so treat the default as provisional and +override it with ``FDv2SkillStore(data_model_version=...)`` if needed. """ DEFAULT_POLL_TIMEOUT = 10.0 -""" -Default ``read_timeout`` in ``"poll"`` mode: the bound on one whole -``GET /sdk/poll``, from opening the connection to reading the last byte. -""" +"""Default ``read_timeout`` in ``"poll"`` mode: the bound on one whole request.""" DEFAULT_STREAM_READ_TIMEOUT = 300.0 -""" -Default ``read_timeout`` in ``"stream"`` mode: the longest a live stream may go -silent before it is treated as dead. LaunchDarkly sends heartbeats well inside -this, so an idle stream this long genuinely has gone away. -""" +"""Default ``read_timeout`` in ``"stream"`` mode: the longest gap tolerated +between two reads. LaunchDarkly's heartbeats arrive well inside this.""" _EVENT_SERVER_INTENT = "server-intent" _EVENT_PUT_OBJECT = "put-object" @@ -120,12 +107,9 @@ _ENVELOPE_FIELDS = ("contentType", "content", "contentHash", "name", "description") """ -The skill object envelope's fields, copied through verbatim. - -Nothing here is coerced, defaulted, or normalised: everything a store serves is -untrusted input and is revalidated above the store interface, so a transport -that filled in a missing field would be forging the very thing verification -exists to check. +The skill object envelope's fields, copied through verbatim. Nothing is coerced +or defaulted: a transport that filled in a missing field would be forging the +very thing verification exists to check. """ Mode = Literal["stream", "poll"] @@ -133,11 +117,8 @@ _MOBILE_KEY_PREFIX = "mob-" _SERVER_KEY_PREFIX = "sdk-" _CLIENT_SIDE_ID = re.compile(r"\A[0-9a-f]{20,}\Z") -""" -A client-side environment ID: bare lowercase hex, no prefix. Server-side keys -and mobile keys both carry a prefix, so "hex with no prefix" is an -unambiguous client-side credential rather than a heuristic. -""" +"""A client-side environment ID: bare lowercase hex. Server-side and mobile keys +both carry a prefix, so this shape is unambiguous rather than heuristic.""" # --------------------------------------------------------------------------- @@ -149,14 +130,10 @@ def _require_server_side_credential(sdk_key: str) -> None: """ Refuses a mobile key or a client-side environment ID. - Skills are for server-side agent runtimes, and skill content is - customer-confidential. Payload assignment is shared across credential types, - so a client-side credential may well *succeed* against these endpoints and - deliver skill content to a client-side process. Refusing one here is what - keeps that from happening. - - Raises ``ValueError`` rather than logging, because there is no degraded mode - that is correct: a store built on the wrong credential should not exist. + Skill content is customer-confidential, and payload assignment is shared + across credential types, so a client-side credential may well *succeed* + against these endpoints. Raises rather than logs: a store built on the wrong + credential should not exist. """ if not isinstance(sdk_key, str) or not sdk_key.strip(): raise ValueError( @@ -179,10 +156,8 @@ def _require_server_side_credential(sdk_key: str) -> None: "process. Use the environment's server-side SDK key (sdk-...)." ) if not key.startswith(_SERVER_KEY_PREFIX): - # Not rejected: private instances and test doubles issue credentials that - # do not carry the public prefix, and refusing them would break a - # deployment that is perfectly correct. The two shapes above are refused - # because they are unambiguously *not* server-side. + # Not rejected: private instances and test doubles issue credentials + # without the public prefix. Only the two unambiguous shapes above are. logger.warning( "The credential given to FDv2SkillStore does not look like a " "LaunchDarkly server-side SDK key (sdk-...). Skills are delivered " @@ -192,7 +167,7 @@ def _require_server_side_credential(sdk_key: str) -> None: # --------------------------------------------------------------------------- -# Diagnostics — and the contentHash gap in particular +# Diagnostics # --------------------------------------------------------------------------- @@ -201,9 +176,9 @@ class StoreDiagnostics: """ What the transport has seen. Read-only from a caller's perspective. - Not part of the ``SkillStore`` interface — nothing above it reads this — but - "this environment has no skills" and "every skill was withheld" are easy to - mistake for each other, and a counter is easier to assert on than a log line. + Not part of the ``SkillStore`` interface. It exists because "this environment + has no skills" and "every skill was withheld" are easy to mistake for each + other, and a counter is easier to assert on than a log line. """ payloads_transferred: int = 0 @@ -211,18 +186,16 @@ class StoreDiagnostics: skill_objects_received: int = 0 """``put-object`` events identified as skills, across all payloads.""" objects_ignored: int = 0 - """Objects skipped because they were not skills — flags, segments, and any - future kind. Skipping is the contract, not a failure; the count exists so a - mixed payload is visibly mixed.""" + """Objects skipped because they were not skills: flags, segments, and any + future kind. Skipping is the contract, not a failure.""" objects_revoked: int = 0 """``delete-object`` events applied to skills.""" hashless_objects: int = 0 """ Skill objects whose envelope carried no ``contentHash``. - **Nonzero means skills are being withheld.** Verification withholds a - hashless object with ``missing_content_hash``, so every one of these is a - skill whose content will not resolve. + **Nonzero means skills are being withheld**: verification withholds every one + of these with ``missing_content_hash``. """ connection_failures: int = 0 """Recoverable transport failures since the last successful transfer.""" @@ -247,14 +220,10 @@ def _is_skill_event(data: Any) -> bool: """ Whether one ``put-object`` / ``delete-object`` payload is a skill. - ``kind == "inline-resource" and category == "skill"``, and nothing else. Both - halves are required: ``inline-resource`` is a broad kind that may carry other - categories, and flags and segments omit ``category`` entirely. - - Every other kind is **ignored, not rejected**. An environment's payload - assignment carries its flag payload alongside its agent-skill payload, so a - connection delivers flag and segment objects as a matter of course; erroring - on them would turn a normal payload into a permanent reconnect loop. + Both halves are required: ``inline-resource`` may carry other categories, + and flags and segments omit ``category`` entirely. Every other kind is + ignored, not rejected, because flag and segment objects share the connection + and erroring on them would turn a normal payload into a reconnect loop. """ if not isinstance(data, dict): return False @@ -266,30 +235,23 @@ def _is_skill_event(data: Any) -> bool: def _store_object_from_put(data: dict[str, Any]) -> dict[str, Any] | None: """ - Translates one FDv2 skill ``put-object`` into the raw object shape that the + Translates one FDv2 skill ``put-object`` into the raw object shape the ``SkillStore`` interface defines. - ``None`` when the event cannot be filed at all — only when ``key`` is not a - string, since a keyless object has no identity to store it under and no key - to attribute a failure to. Every other defect is carried through verbatim so - that *verification* withholds it, with a reason code and an integrity signal, - rather than the transport dropping it silently. A silent drop is - indistinguishable from "no such skill" and would additionally let a prune - delete the last known-good copy on disk. - **The one translation this adapter must get right:** wire ``objectVersion`` → stored ``version`` (the skill's own version) wire ``version`` → dropped (the *payload* version) - ``objectVersion`` is what a ``{key, version}`` reference pins. ``version`` is - the version of the payload the object arrived in — it changes when anything - in the environment changes, including a flag that has nothing to do with - skills. Reading it as the skill's version resolves the wrong content with no - error anywhere: the object verifies, the hash matches, and the caller is - handed a skill under a version number that means nothing. Flags and segments - carry only ``version``, which is why the two fields look interchangeable and - are not. + ``objectVersion`` is what a ``{key, version}`` reference pins; ``version`` + moves whenever anything in the environment moves. Confusing them fails + silently: the object verifies and the caller gets content under a version + number that means nothing. + + Returns ``None`` only when ``key`` is not a string, since a keyless object + has no identity to store it under. Every other defect is carried through + verbatim so that verification withholds it with a reason code rather than + the transport dropping it into indistinguishable absence. """ key = data.get("key") if not isinstance(key, str) or not key: @@ -301,9 +263,8 @@ def _store_object_from_put(data: dict[str, Any]) -> dict[str, Any] | None: raw: dict[str, Any] = {"key": key} - # The single translation. Written as a membership test rather than a `.get` - # default so an explicitly-null objectVersion stays null and reaches - # verification as `invalid_version`, instead of being invented here. + # A membership test rather than a `.get` default, so an explicitly-null + # objectVersion stays null and reaches verification as `invalid_version`. if "objectVersion" in data: raw["version"] = data["objectVersion"] @@ -317,16 +278,13 @@ def _store_object_from_put(data: dict[str, Any]) -> dict[str, Any] | None: def _tombstone_from_delete(data: dict[str, Any]) -> _Tombstone | None: """ - Narrows one FDv2 skill ``delete-object`` to the identity it revokes. - - A delete for an inline resource **is revocation** — the object leaves the - payload, this store drops it, the accessors stop resolving it, and the next - reconcile prunes its files. Same ``objectVersion`` translation as a put. + Narrows one FDv2 skill ``delete-object`` to the identity it revokes, with + the same ``objectVersion`` translation as a put. - ``object_version`` of ``None`` means the delete named no usable version, and - is read as "revoke every version of this key". That is the safe direction: - the alternative is ignoring an unparseable revocation and continuing to serve - content LaunchDarkly has withdrawn. + An ``object_version`` of ``None`` means the delete named no usable version + and is read as "revoke every version of this key". That is the safe + direction: the alternative is continuing to serve content LaunchDarkly has + withdrawn. """ key = data.get("key") if not isinstance(key, str) or not key: @@ -352,19 +310,15 @@ class _SkillObjectSet: """ Raw skill objects held in memory, keyed by ``(key, objectVersion)``. - Lookup semantics are deliberately identical to ``InMemorySkillStore``'s, down - to the fall-through to a version-less entry, so that the store a caller - configures cannot change how a pinned reference resolves. They are - reimplemented rather than inherited because the transport needs two - operations a hand-populated store does not have: ``delete``, and the atomic - ``replace`` a full transfer requires. ``test_skills_fdv2.py`` asserts that the - two stores resolve identically. - - Several versions of one key coexist, because they coexist in a real payload: - the newest version of every skill plus every version a variation currently - pins. An object too malformed to carry a usable version is still held, under - its key alone, so verification withholds it with a signal rather than the - transport dropping it into indistinguishable absence. + Lookup semantics are identical to ``InMemorySkillStore``'s, down to the + fall-through to a version-less entry, so that the store a caller configures + cannot change how a pinned reference resolves; ``TestInterfaceParity`` + asserts it. Reimplemented rather than inherited because the transport needs + ``delete`` and the atomic ``replace_with`` a full transfer requires. + + An object too malformed to carry a usable version is still held, under its + key alone, so verification withholds it with a signal rather than the + transport dropping it. """ def __init__(self) -> None: @@ -380,12 +334,7 @@ def put(self, raw: dict[str, Any]) -> None: self._loose[key] = raw def delete(self, tombstone: _Tombstone) -> list[dict[str, Any]]: - """ - Removes what *tombstone* revokes; returns the raw objects that went away. - - A tombstone with no usable version removes every version of the key — see - ``_tombstone_from_delete`` for why that is the safe reading. - """ + """Removes what *tombstone* revokes; returns the raw objects that went away.""" removed: list[dict[str, Any]] = [] if tombstone.object_version is None: held = self._versions.pop(tombstone.key, {}) @@ -406,9 +355,8 @@ def delete(self, tombstone: _Tombstone) -> list[dict[str, Any]]: def get(self, key: str, version: int | None) -> dict[str, Any] | None: held = self._versions.get(key, {}) if version is not None: - # Fall through to the version-less entry when the pin matches nothing - # well-formed, so a malformed object reaches verification and is - # withheld with a signal rather than reading as simply absent. + # Fall through to the version-less entry so a malformed object + # reaches verification rather than reading as simply absent. return held.get(version) or self._loose.get(key) if held: return held[max(held)] @@ -460,19 +408,13 @@ class _TransferOutcome: class _ProtocolReader: """ - Applies FDv2 events to an object set. Pure — no sockets, no threads, no clock. - - Split out so the protocol is testable without a server: every wire case in - ``test_skills_fdv2.py`` drives this directly, and the HTTP layer above it only - has to turn bytes into ``(event name, data)`` pairs. - - **Changes are buffered and committed at ``payload-transferred``**, matching - how the base SDK's FDv2 data source applies a change set. A payload version - is the unit of consistency: applying half of one would publish a state the - server never described, and on a full transfer it would briefly empty the - store — which, with pruning on, is the difference between a reconcile and - deleting a customer's skill files. Listeners therefore fire once per commit, - not once per object. + Applies FDv2 events to an object set. Pure — no sockets, no threads, no clock — + so every wire case is testable without a server. + + **Changes are buffered and committed at ``payload-transferred``.** A payload + version is the unit of consistency: applying half of one would publish a + state the server never described, and on a full transfer would briefly empty + the store. Listeners therefore fire once per commit, not once per object. """ def __init__(self, committed: _SkillObjectSet) -> None: @@ -481,10 +423,8 @@ def __init__(self, committed: _SkillObjectSet) -> None: self._pending: _SkillObjectSet | None = None self._changes: list[dict[str, Any]] = [] self.diagnostics = StoreDiagnostics() - # Identities already reported by ``_warn_hashless``. Held per reader, so a - # store that is recreated in the same process reports again and two - # stores never quieten each other. No lock: ``handle`` only runs under - # the owning store's lock, on that store's single delivery thread. + # Identities already reported by ``_warn_hashless``. Per reader, so a + # recreated store reports again and two stores never quieten each other. self._warned_hashless: set[tuple[str, Any]] = set() # -- events ------------------------------------------------------------ @@ -519,39 +459,40 @@ def _server_intent(self, data: Any) -> _TransferOutcome: self._intent = intent self._changes = [] if intent == _INTENT_TRANSFER_FULL: - # A fresh set: the payload about to arrive replaces everything held. - # Built alongside the live set rather than in place, so an interrupted - # transfer leaves last-known-good intact. + # Built alongside the live set rather than in place, so an + # interrupted transfer leaves last known good intact. self._pending = _SkillObjectSet() elif intent == _INTENT_TRANSFER_CHANGES: self._pending = self._committed.copy() - elif intent == _INTENT_TRANSFER_NONE: - self._pending = None else: - logger.debug("Ignoring FDv2 server-intent with intentCode %r", intent) + if intent != _INTENT_TRANSFER_NONE: + logger.debug("Ignoring FDv2 server-intent with intentCode %r", intent) self._pending = None return _TransferOutcome() - def _target(self) -> _SkillObjectSet | None: - if self._pending is None and self._intent in ( - _INTENT_TRANSFER_FULL, - _INTENT_TRANSFER_CHANGES, - ): - # An object arrived before any server-intent. Treat it as a delta - # against what we hold rather than dropping it. + def _target_for(self, data: Any) -> _SkillObjectSet | None: + """ + The pending set a skill object event applies to, or ``None`` when the + event is not a skill or the current intent carries no objects. + + An object arriving with no ``server-intent`` at all is treated as a + delta against what is held rather than dropped. + """ + if not _is_skill_event(data): + self.diagnostics.objects_ignored += 1 + return None + if self._pending is None: + if self._intent is None: + self._intent = _INTENT_TRANSFER_CHANGES + if self._intent not in (_INTENT_TRANSFER_FULL, _INTENT_TRANSFER_CHANGES): + return None self._pending = self._committed.copy() return self._pending def _put_object(self, data: Any) -> _TransferOutcome: - if not _is_skill_event(data): - self.diagnostics.objects_ignored += 1 - return _TransferOutcome() - if self._pending is None and self._intent is None: - self._intent = _INTENT_TRANSFER_CHANGES - target = self._target() + target = self._target_for(data) if target is None: return _TransferOutcome() - raw = _store_object_from_put(data) if raw is None: return _TransferOutcome() @@ -564,24 +505,16 @@ def _put_object(self, data: Any) -> _TransferOutcome: return _TransferOutcome() def _delete_object(self, data: Any) -> _TransferOutcome: - if not _is_skill_event(data): - self.diagnostics.objects_ignored += 1 - return _TransferOutcome() - if self._pending is None and self._intent is None: - self._intent = _INTENT_TRANSFER_CHANGES - target = self._target() + target = self._target_for(data) if target is None: return _TransferOutcome() - tombstone = _tombstone_from_delete(data) if tombstone is None: return _TransferOutcome() target.delete(tombstone) self.diagnostics.objects_revoked += 1 - # A tombstone, not a skill object: it carries identity and no content, so - # a listener that only needs "something changed" works unchanged while one - # that reads content sees no `content` key. Documented on - # ``FDv2SkillStore.add_listener``. + # A tombstone carries identity and no content; see + # ``FDv2SkillStore.add_listener`` for what listeners should expect. self._changes.append( {"key": tombstone.key, "version": tombstone.object_version} ) @@ -609,21 +542,22 @@ def _payload_transferred(self, data: Any) -> _TransferOutcome: basis=state if isinstance(state, str) and state else None, ) - def _error(self, data: Any) -> _TransferOutcome: - reason = data.get("reason") if isinstance(data, dict) else None - # An error abandons the in-flight payload and keeps what is committed. + def _abandon_in_flight(self) -> None: + """Drops the in-flight payload and keeps what is committed.""" self._pending = None self._intent = None self._changes = [] + + def _error(self, data: Any) -> _TransferOutcome: + reason = data.get("reason") if isinstance(data, dict) else None + self._abandon_in_flight() return _TransferOutcome(disconnect=f"server sent error: {reason}") def _goodbye(self, data: Any) -> _TransferOutcome: reason = data.get("reason") if isinstance(data, dict) else None catastrophe = bool(data.get("catastrophe")) if isinstance(data, dict) else False silent = bool(data.get("silent")) if isinstance(data, dict) else False - self._pending = None - self._intent = None - self._changes = [] + self._abandon_in_flight() if not silent: logger.info("FDv2 connection closing: %s", reason) if catastrophe: @@ -638,12 +572,8 @@ def _warn_hashless(self, raw: dict[str, Any]) -> None: """ One ERROR per ``(key, version)`` whose envelope had no ``contentHash``. - At ERROR rather than WARN, and per object rather than once per process, - because an empty accessor result is otherwise indistinguishable from an - environment that has no skills. Deduped within this reader so a - re-delivered payload does not multiply it; a store that is recreated, - in this process or another, reports again, and stores for different - environments in one process do not share the dedupe. + ERROR rather than WARN because an empty accessor result is otherwise + indistinguishable from an environment that has no skills. """ identity = (raw["key"], raw.get("version")) if identity in self._warned_hashless: @@ -671,13 +601,11 @@ def _warn_hashless(self, raw: dict[str, Any]) -> None: def _warn_if_nothing_can_verify(committed: _SkillObjectSet) -> None: """ - One ERROR per committed payload in which *nothing* the store now holds can - possibly verify. + One ERROR per committed payload in which *nothing* held can possibly verify. - ``log_withholding_summary`` already reports a wholly-withheld batch at the - accessor boundary, but only once a caller asks. This fires at delivery time, - so the condition is visible in a process that boots, materializes nothing, - and exits, which is a common way a skills deployment fails. + ``log_withholding_summary`` reports the same condition at the accessor + boundary, but only once a caller asks. This fires at delivery time, so it is + visible in a process that boots, materializes nothing, and exits. """ held = committed.all_raw() if not held: @@ -718,7 +646,13 @@ def __init__(self, message: str, retry_after: float | None = None) -> None: def _retry_after_seconds(headers: Any) -> float | None: - """``Retry-After`` in seconds, when the server sent a usable one.""" + """ + ``Retry-After`` in seconds, when the server sent a usable one. + + The HTTP-date form, and non-finite values such as ``inf`` or ``1e309`` that + ``float`` accepts, fall back to our own backoff: none of them is a delay, + and an infinite one would overflow the wait that honours it. + """ if headers is None: return None try: @@ -730,13 +664,8 @@ def _retry_after_seconds(headers: Any) -> float | None: try: seconds: float = float(str(raw).strip()) except ValueError: - # The HTTP-date form is legal and rare; falling back to our own backoff - # is better than parsing a date to honour it approximately. return None if not math.isfinite(seconds): - # ``float`` accepts "inf", "nan" and out-of-range literals such as - # "1e309". None of them is a delay, and an infinite one would overflow - # the wait that honours it, so treat them like the date form. return None return max(0.0, seconds) @@ -752,17 +681,12 @@ def _classify_status(status: int, headers: Any) -> Exception: return _FatalTransportError( f"LaunchDarkly returned HTTP 403. {_FORBIDDEN_ADVICE}" ) - if status == 404: - return _FatalTransportError( - "LaunchDarkly returned HTTP 404 for the FDv2 endpoint. Check the base " - "URI, and that this instance serves /sdk/poll and /sdk/stream." - ) if status in (400, 405, 406, 414, 501): return _FatalTransportError( f"LaunchDarkly returned HTTP {status}, which retrying will not fix. " "The request this adapter sent was not understood; the 'mv' data " f"model version ({SDK_DATA_MODEL_VERSION}) is the parameter most " - "likely to be wrong." + f"likely to be wrong." ) return _RecoverableTransportError( f"LaunchDarkly returned HTTP {status}", _retry_after_seconds(headers) @@ -774,14 +698,10 @@ def _interrupt_read(response: Any) -> None: Best-effort interruption of a read blocked on *response*, from another thread. Closing the response is not enough: CPython's buffered reader stays parked in - ``readline`` until bytes arrive, so a ``close`` from another thread does not - unblock it. Shutting the *socket* down underneath it does, immediately. - - Reaching the socket means walking urllib's private attribute chain, so every - step is guarded and a failure here is silent by design. It is an - optimisation, not a correctness requirement: the delivery thread is a daemon - and ``close``'s join timeout is the backstop, so the worst case of this not - finding a socket is a shutdown that takes as long as the join allows. + ``readline`` until bytes arrive. Shutting the *socket* down underneath it + unblocks it immediately. Reaching the socket means walking urllib's private + attribute chain, so every step is guarded and failure is silent: the + delivery thread is a daemon and ``close``'s join timeout is the backstop. """ for path in (("fp", "raw", "_sock"), ("fp", "_sock"), ("_sock",)): found: Any = response @@ -799,13 +719,8 @@ def _interrupt_read(response: Any) -> None: class _StreamConnection: """ - One open streaming connection: an event iterator plus a way to interrupt it. - - Exists because ``close`` runs on a *different* thread from the read. The - delivery thread spends nearly all its life blocked in a socket read on a - long-lived stream, where a stop flag it cannot check is of no use; without an - interruption, closing a *healthy* stream would block the caller's shutdown - path for the whole join timeout. + One open streaming connection: an event iterator plus a way to interrupt it + from another thread, which is what ``FDv2SkillStore.close`` needs. """ def __init__(self, response: Any) -> None: @@ -830,11 +745,12 @@ class _PollResult: class _Requester: """ - The only place this module opens a socket. + The only place this module opens a socket. Standard library only, on purpose. - Standard library only, on purpose: this package's sole runtime dependency is - ``opentelemetry-api`` and its LaunchDarkly base-SDK dependency is optional, so - the content path must not add an HTTP client dependency. + *read_timeout* is applied to every socket operation of a request. ``urllib`` + has no separate connect timeout: its ``timeout`` becomes the socket timeout + for the whole operation, so connecting, waiting for headers and each body + read are all bounded by the same value. """ def __init__( @@ -849,18 +765,8 @@ def __init__( self._sdk_key = sdk_key self._base_uri = base_uri.rstrip("/") self._read_timeout = read_timeout - """ - The one timeout, applied to every socket operation of a request. - - ``urllib`` has no separate connect timeout: its ``timeout`` becomes the - socket timeout for the whole operation, so connecting, waiting for headers - and each body read are all bounded by this same value. For a poll that - makes it the bound on the whole request; for a stream it is the longest - gap tolerated between two reads. - """ self._data_model_version = data_model_version - # Injectable so the tests drive a fake endpoint without a socket; the - # default is urllib's global opener. + # Injectable so tests can drive a fake endpoint without a socket. self._opener = opener or urllib.request.build_opener() def _url(self, path: str, basis: str | None) -> str: @@ -878,10 +784,7 @@ def _request( ) def poll(self, basis: str | None, etag: str | None) -> _PollResult: - """ - One ``GET /sdk/poll``. Honours ``If-None-Match`` and returns 304 as a - first-class outcome rather than as an error. - """ + """One ``GET /sdk/poll``. A 304 is a first-class outcome, not an error.""" headers = {"Accept": "application/json"} if etag: headers["If-None-Match"] = etag @@ -908,12 +811,7 @@ def poll(self, basis: str | None, etag: str | None) -> _PollResult: ) def stream(self, basis: str | None) -> _StreamConnection: - """ - Opens ``GET /sdk/stream``. - - Returns a ``_StreamConnection`` rather than a bare generator so the - caller can interrupt a blocked read from another thread; see that class. - """ + """Opens ``GET /sdk/stream``.""" request = self._request( STREAM_PATH, basis, @@ -932,11 +830,8 @@ def stream(self, basis: str | None) -> _StreamConnection: def _decode_poll_body(body: bytes) -> list[tuple[str, Any]]: """ - Unwraps ``{"events": [...]}``. - - Polling and streaming carry the *identical* event objects — polling just wraps - them in an envelope — which is why the protocol state machine above is shared - and neither mode has its own copy of the semantics. + Unwraps ``{"events": [...]}``. Polling and streaming carry identical event + objects, which is why the protocol reader is shared between the two modes. """ try: parsed = json.loads(body.decode("utf-8")) @@ -960,9 +855,8 @@ def _iter_sse(response: Any) -> Any: """ Decodes an SSE body into ``(event name, data)`` pairs. - Minimal on purpose — this consumes one LaunchDarkly endpoint, not the whole - spec: ``event:``/``data:`` fields, multi-line ``data`` joined with newlines, - a blank line dispatching, and ``:`` comments skipped. + Minimal on purpose: ``event:``/``data:`` fields, multi-line ``data`` joined + with newlines, a blank line dispatching, and ``:`` comments skipped. """ try: name: str | None = None @@ -1008,14 +902,13 @@ def _backoff_delay( attempt: int, *, base: float, maximum: float, jitter: float = 0.5 ) -> float: """ - Exponential backoff with decorrelating jitter, capped at *maximum*. + Exponential backoff with jitter, capped at *maximum*. - Jitter is subtractive over the *whole* range rather than added on top, so the - cap is a real ceiling: a fleet of agent processes restarted together must not - reconnect in lockstep, and must not exceed the interval the cap promises. + Jitter is subtractive over the whole range rather than added on top, so the + cap is a real ceiling: a fleet restarted together must not reconnect in + lockstep, and must not exceed the interval the cap promises. """ - # float(2 ** n) rather than 2 ** n: the integer power is untyped to mypy, - # and the whole expression is a duration, not a count. + # float(2 ** n): the integer power is untyped to mypy. ceiling: float = min(maximum, base * float(2 ** max(0, attempt - 1))) return ceiling * (1.0 - jitter * random.random()) @@ -1029,8 +922,8 @@ class FDv2SkillStore: """ A ``SkillStore`` fed by LaunchDarkly's SDK-facing FDv2 delivery channel. - The transport half of Agent Skills. Constructed with the environment's - server-side SDK key, started explicitly, and passed to ``init_client``:: + Constructed with the environment's server-side SDK key, started explicitly, + and passed to ``init_client``:: store = FDv2SkillStore(sdk_key=os.environ["LD_SDK_KEY"]) store.start() @@ -1041,29 +934,25 @@ class FDv2SkillStore: ... store.close() - It also works as a context manager, which is the shape to prefer when the - process's lifetime is a block. - - **Server-side only.** Skills are for server-side agent runtimes and skill - content is customer-confidential. A mobile key or a client-side environment - ID is refused in the constructor. - - **Delivery is in the background; retrieval is not.** ``SkillStore`` is a - synchronous interface, so a daemon thread owns the connection and fills - memory, and ``get_object`` only ever reads what has already arrived. Nothing - here blocks a retrieval on the network. The corollary is that a process - which calls ``get_skill`` immediately after ``start()`` may see an empty store; - ``wait_for_skills`` is how you order boot against the first payload. - - **Last known good survives an outage.** A transport failure never empties the - store and never makes ``get_object`` raise: it keeps serving what it last - received, which is what makes ``write_skills(on_unavailable="keep")`` - correct. ``diagnostics`` and ``failed`` report the degradation. - - **What arrives is untrusted.** This store holds raw wire objects verbatim and - verifies nothing — integrity verification lives at the accessor boundary so - it applies to every store equally. In particular, an object with no - ``contentHash`` is held and then *withheld* by verification; see + It also works as a context manager. + + **Server-side only.** A mobile key or a client-side environment ID is + refused in the constructor. + + **Delivery is in the background; retrieval is not.** A daemon thread owns + the connection and fills memory, and ``get_object`` only ever reads what has + already arrived. A process that calls ``get_skill`` immediately after + ``start()`` may see an empty store; ``wait_for_skills`` orders boot against + the first payload. + + **Last known good survives an outage.** A transport failure never empties + the store and never makes ``get_object`` raise, which is what makes + ``write_skills(on_unavailable="keep")`` correct. ``diagnostics`` and + ``failed`` report the degradation. + + **What arrives is untrusted.** Raw wire objects are held verbatim and + verified at the accessor boundary, not here. In particular an object with no + ``contentHash`` is held and then *withheld*; see ``StoreDiagnostics.hashless_objects``. """ @@ -1082,33 +971,24 @@ def __init__( _requester: Any = None, ) -> None: """ - *mode* is ``"stream"`` by default. Prefer it: a ``delete-object`` reaches a - live stream in seconds, so a revoked skill stops resolving in seconds - rather than at the next restart. ``"poll"`` exists for environments that - cannot hold a long-lived connection, and revocation there is one + *mode* is ``"stream"`` by default. Prefer it: a ``delete-object`` reaches + a live stream in seconds. ``"poll"`` exists for environments that cannot + hold a long-lived connection, and revocation there is one ``poll_interval`` late. - *read_timeout* is the only network timeout, and it bounds every socket - operation of a request: connecting, waiting for headers, and each read. - There is no separate connect timeout because the standard library offers - none, so a host that accepts and never answers, or never accepts, fails - after ``read_timeout`` too. What the value means therefore depends on the - mode, and so does its default. In ``"poll"`` mode it bounds the whole - request and defaults to ``DEFAULT_POLL_TIMEOUT`` (10s). In ``"stream"`` - mode it bounds each wait for the next bytes and defaults to - ``DEFAULT_STREAM_READ_TIMEOUT`` (300s): a stream is meant to sit idle - between events, and LaunchDarkly's heartbeats arrive well inside that. - Pass a value to override the default for either mode; it must be - positive. + *read_timeout* is the only network timeout and bounds every socket + operation of a request, so its meaning and default follow the mode: in + ``"poll"`` it bounds the whole request (``DEFAULT_POLL_TIMEOUT``); in + ``"stream"`` it bounds each wait for the next bytes + (``DEFAULT_STREAM_READ_TIMEOUT``). Must be positive when given. *max_backoff* caps every delay between retries, including one the server - asks for with ``Retry-After``; a header cannot park delivery for longer - than the cap promises. + asks for with ``Retry-After``. *max_consecutive_failures* bounds the retry loop. On exceeding it the transport stops, logs an error, and the store keeps serving last known - good rather than pretending to be live — ``failed`` reports it. Only - failures in a row count: a committed payload resets the count. + good; ``failed`` reports it. Only failures in a row count: a committed + payload resets the count. """ _require_server_side_credential(sdk_key) if mode not in ("stream", "poll"): @@ -1149,18 +1029,13 @@ def __init__( self._first_payload = threading.Event() self._thread: threading.Thread | None = None self._failed_reason: str | None = None + # The open streaming connection, so ``close`` can interrupt its read. self._connection: Any = None - """The open streaming connection, so ``close`` can interrupt its read.""" + # Recoverable failures since the last committed payload. Reset at the + # commit rather than when a connection returns: a stream only ever ends + # by being dropped, so resetting on return would count every healthy, + # server-recycled connection as a failure. self._failures = 0 - """ - Recoverable failures since the last committed payload. - - Held on the store rather than in the loop because the reset belongs at - the commit, not at the return: a streaming connection only ever ends by - being dropped, so a loop that reset on return would count every healthy, - server-recycled connection as a failure and eventually give up on a - transport that never failed. - """ # -- lifecycle --------------------------------------------------------- @@ -1185,15 +1060,12 @@ def close(self, timeout: float = 5.0) -> None: Stops delivery. Idempotent, and safe to call from any thread. Held content is *not* dropped: a closed store still answers from what it - received, so shutting the transport down does not turn into an integrity - failure or an empty reconcile mid-flight. Detaching the store from the - accessors is the job of the package-level ``launchdarkly_ai_server.shutdown()`` - coroutine, not of this method. + received. Detaching the store from the accessors is the job of the + package-level ``launchdarkly_ai_server.shutdown()`` coroutine. """ self._stop.set() - # Interrupt the read before joining. The delivery thread is normally - # blocked in a socket read that no flag can reach, so without this the - # join below waits out its full timeout on every shutdown. + # The delivery thread is normally blocked in a socket read that no flag + # can reach; without this the join waits out its full timeout. with self._lock: connection = self._connection if connection is not None: @@ -1217,8 +1089,7 @@ def wait_for_skills(self, timeout: float = 10.0) -> bool: Blocks until the first payload has been committed, or *timeout* elapses. ``True`` means a payload arrived — not that any skill in it verified, and - not that the environment has any skills. Boot ordering is all this - answers; ``diagnostics`` answers the rest. + not that the environment has any skills. ``diagnostics`` answers the rest. """ return self._first_payload.wait(timeout=timeout) @@ -1252,37 +1123,28 @@ def all_objects(self, kind: str) -> dict[str, dict[str, Any]]: def add_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None: """ - Registers *fn* to be called once per committed change. - - Fires **once per changed object at payload-transferred**, not as objects - stream in: a payload version is the unit of consistency, and a listener - that reacted to a half-applied full transfer would see the store briefly - empty. ``skills_watch.watch_skills`` is the intended consumer. + Registers *fn* to be called once per changed object, at + ``payload-transferred`` rather than as objects stream in. A put notifies with the raw skill object. A revocation notifies with a - ``{"key", "version"}`` tombstone — it names what went away and carries no - content, since there is none. A listener that only needs "something - changed" works with both; one that reads content must check for - ``content`` rather than assume it. - - *fn* runs on the delivery thread. Keep it cheap and non-blocking: work - done there delays the next event. An exception it raises is logged and - swallowed, because a broken listener must not be able to kill delivery. + ``{"key", "version"}`` tombstone carrying no content, so a listener that + reads content must check for ``content`` rather than assume it. + + *fn* runs on the delivery thread. Keep it cheap and non-blocking. An + exception it raises is logged and swallowed, because a broken listener + must not be able to kill delivery. """ with self._lock: self._listeners.setdefault(kind, []).append(fn) def remove_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None: """ - Unregisters *fn* from *kind*, so later committed changes no longer call it. - - Safe to call from any thread, including from inside a listener: the - listener list is copied under the lock before a commit's notifications - run, so a removal during one commit takes effect from the next. + Unregisters *fn* from *kind*. Safe to call from any thread, including + from inside a listener: a removal during one commit takes effect from + the next. - Removes one occurrence: a callable registered twice must be removed twice. - Removing a callable that is not registered is a no-op, not an error, so - ``SkillWatcher.close`` can detach unconditionally. + Removes one occurrence; removing a callable that is not registered is a + no-op, so ``SkillWatcher.close`` can detach unconditionally. """ with self._lock: listeners = self._listeners.get(kind) @@ -1316,9 +1178,8 @@ def _run(self) -> None: else: self._poll_once() # A poll that returned is a current answer even when it committed - # nothing (HTTP 304), so it counts as a success in its own right. - # A stream never returns normally; its successes are counted where - # they happen, at each commit in ``_apply``. + # nothing (HTTP 304). A stream never returns normally; its + # successes are counted at each commit in ``_apply``. self._record_success() except _FatalTransportError as exc: self._give_up(str(exc)) @@ -1340,11 +1201,9 @@ def _run(self) -> None: delay = _backoff_delay( failures, base=self._initial_backoff, maximum=self._max_backoff ) - # ``Retry-After`` is a request, and ``max_backoff`` is a promise. - # The header comes from whatever answered on the error path, - # which may be a proxy or a CDN rather than LaunchDarkly, and a - # value in the hours would park delivery (and revocation) for - # that long. The promise wins. A zero still means "now". + # ``Retry-After`` is a request and ``max_backoff`` is a promise. + # The header may come from a proxy rather than LaunchDarkly, and + # a value in the hours would park revocation for that long. delay = min(delay, self._max_backoff) logger.warning( "Skill delivery failed (%s); retrying in %.1fs", exc, delay @@ -1375,24 +1234,28 @@ def _give_up(self, reason: str) -> None: "the process restarts with a working connection.", reason, ) - # Unblock anyone waiting on a first payload that is never coming, rather - # than making them wait out the full timeout. + # Unblock anyone waiting on a first payload that is never coming. self._first_payload.set() - def _apply(self, name: str, data: Any) -> _TransferOutcome: + def _apply(self, name: str, data: Any) -> None: + """ + Feeds one event to the reader, publishes a commit, and raises the + transport error the event calls for, if any. + """ with self._lock: outcome = self._reader.handle(name, data) if outcome.committed and outcome.basis is not None: self._basis = outcome.basis if outcome.committed: - # A connection that transferred a payload succeeded, whatever it does - # afterwards: the give-up bound counts failures in a row, and a - # commit breaks the row. + # A commit breaks the row of consecutive failures. self._record_success() self._first_payload.set() if outcome.changes: self._notify(outcome.changes) - return outcome + if outcome.fatal: + raise _FatalTransportError(outcome.fatal) + if outcome.disconnect: + raise _RecoverableTransportError(outcome.disconnect) def _poll_once(self) -> None: with self._lock: @@ -1402,18 +1265,12 @@ def _poll_once(self) -> None: self._etag = result.etag if result.not_modified: logger.debug("Skill payload unchanged (HTTP 304)") - # A 304 is a successful, current answer: the payload we hold is the - # payload the server has. It counts as a first payload so a boot that - # reconnects with a cached basis is not blocked on a transfer the - # server has no reason to send. + # A 304 counts as a first payload, so a boot that reconnects with a + # cached basis is not blocked on a transfer the server will not send. self._first_payload.set() return for name, data in result.events: - outcome = self._apply(name, data) - if outcome.fatal: - raise _FatalTransportError(outcome.fatal) - if outcome.disconnect: - raise _RecoverableTransportError(outcome.disconnect) + self._apply(name, data) def _stream_once(self) -> None: with self._lock: @@ -1422,31 +1279,23 @@ def _stream_once(self) -> None: with self._lock: self._connection = connection try: - # ``close`` may have run while the connect above was in flight. It - # found no connection to interrupt then, so this is the last chance - # to notice before the read below blocks for as long as the server - # stays quiet. Either ``close`` saw the connection and interrupted - # it, or it set the stop flag before this check: there is no window. + # ``close`` may have run while the connect was in flight and found + # no connection to interrupt; this is the last chance to notice + # before the read below blocks. if self._stop.is_set(): return for name, data in connection.events: if self._stop.is_set(): return - outcome = self._apply(name, data) - if outcome.fatal: - raise _FatalTransportError(outcome.fatal) - if outcome.disconnect: - raise _RecoverableTransportError(outcome.disconnect) + self._apply(name, data) except Exception: if self._stop.is_set(): - # `close` interrupted the read on purpose: unwind quietly rather - # than reporting a delivery failure and retrying. + # ``close`` interrupted the read on purpose. return raise finally: connection.close() with self._lock: self._connection = None - # A stream that ends without a goodbye is a dropped connection, not a - # completed operation: reconnect through the backoff path. + # A stream that ends without a goodbye is a dropped connection. raise _RecoverableTransportError("the FDv2 stream closed unexpectedly")