From b34b45f2ff003dc23e4a42627c4d3a83f009454b Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Thu, 1 Oct 2026 16:27:54 -0400 Subject: [PATCH 1/2] fix(client): a 304 confirms a payload rather than establishing one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A poll adopted the response `ETag` after any body that did not raise, and a 304 published a first payload unconditionally. Between them, a body this SDK could not apply lent its etag to the next request, and the 304 that answered it reported the empty committed set as current: initialized, healthy, and with nothing in a 304 to notice it on. `is_initialized()` is what authorizes `write_skills("*")` to prune, so that store read as an environment whose every skill was revoked and deleted the last known good `SKILL.md` files on disk — the widest version of the hazard the probe exists to prevent. Three claims narrowed to what actually happened: - `payload-transferred` reports a commit only when it applied a pending set. A `none` intent builds none, nor does an `intentCode` this SDK does not recognise, and a foreign payload's contents are declined — those transfers now report nothing, where they reported a commit. They no longer adopt the selector of a payload that was never applied either, which on its own left a store resuming from content it did not hold while every diagnostic read healthy. - A poll adopts the response `ETag` only from a body that completed an exchange: a committed payload, or a `none` intent, which is the server saying the content held is what the etag describes. An unrecognised intent says the opposite — the body carried objects this reader dropped — so leaving the poll unconditional is also what keeps that body arriving and visible instead of silenced behind a 304. - A 304 no longer publishes a first payload. It confirms the payload the store holds; the exchange it stands in for, the `none` intent, does not publish one either. TESTING.md §3.25 defines `is_initialized()` as true "once a payload has committed", and §3.21/§3.22 turn prune suppression on it, so none of this is a new rule — it is the existing one reaching the poll path. The test that pinned the old behaviour justified it as "a reconnect with a cached basis", which this transport has no mechanism for: `_basis` and `_etag` both start as `None` with no injection point, so a 304 reaching a store that holds nothing takes a server answering a request that carried no etag at all. It now fails closed, and the test asserts that. The over-cap poll test was leaning on the same path for its "delivery carries on" assertion; it now gets a real payload on the retry, which proves more. Co-Authored-By: Claude Opus 5 --- packages/client/agents.md | 17 +++ .../src/launchdarkly_ai_server/skills_fdv2.py | 85 +++++++++--- packages/client/tests/test_skills_fdv2.py | 121 +++++++++++++++++- 3 files changed, 198 insertions(+), 25 deletions(-) diff --git a/packages/client/agents.md b/packages/client/agents.md index b3664663..ae5a2680 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -353,6 +353,23 @@ described, and would briefly empty the store — which, with pruning on, is the 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 commit is the only thing that publishes a first payload, and a 304 is not one.** +`is_initialized()` — the fact `write_skills("*")` authorizes a prune on — goes true when a +payload *commits*, so every other answer has to stop short of claiming one. A +`payload-transferred` that applied nothing reports neither a commit nor an up-to-date +answer, and does not adopt the selector of a payload it never applied: a `none` intent +builds no pending set, nor does an `intentCode` this SDK does not recognise, and a foreign +payload's contents are declined. A poll adopts the response `ETag` only from a body that +completed an exchange — a commit, or a `none` intent, which is the server saying the +content held is what the etag describes. And a 304 *confirms* the payload held rather than +establishing one, because the exchange it stands in for cannot establish one either. +Loosen any of the three and the other two carry a store that received nothing into a prune +of every managed `SKILL.md` on disk: an empty committed set reads as an environment that +revoked every skill, and a 304 carries nothing to notice it on. There is no cached basis +to make it safe — `_basis` and `_etag` both start as `None` with no injection point, so a +304 reaching a store that holds nothing takes a server answering a request that carried no +etag at all. + **The first payload intent is read, and is assumed to be the skill payload.** Delivery provides one payload per credential and the protocol requires a client to ignore all but the first payload intent, so `payloads[0]` is both what arrives and what the protocol says to diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index 6cbb73f6..8ccf1a63 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -795,6 +795,10 @@ def _payload_transferred(self, data: Any) -> _TransferOutcome: # whose selector must not become the resume point if it is not the # payload skills arrive on. foreign = self._is_foreign_payload(payload_id) + # Whether this transfer moved the committed set. A ``none`` intent + # builds no pending set, nor does an intent code this SDK does not + # recognise, and a foreign payload's contents are declined below. + applied = not foreign and self._pending is not None if foreign: self._warn_foreign_payload(payload_id) self.diagnostics.payloads_ignored += 1 @@ -833,15 +837,37 @@ def _payload_transferred(self, data: Any) -> _TransferOutcome: version, len(self._committed), ) + if not applied: + # A transfer that applied nothing claims nothing: not a commit, and + # not an up-to-date answer either. + # + # Not a commit, because a commit publishes a first payload, and + # ``is_initialized`` is the fact ``write_skills("*")`` prunes on. An + # empty committed set reported as a payload reads as an environment + # whose every skill was revoked, which deletes the last known good + # copy on disk. + # + # Not up to date either, because only the server can say that, and + # only the ``none`` intent does — on its own event, which has + # already reported it by the time a transfer completing it arrives. + # An intent code this SDK does not recognise says the opposite: the + # body carried objects this reader dropped, so the content held is + # *not* what that body describes, and an etag adopted from it would + # let a 304 report the store as current for as long as the server + # kept re-announcing it. Claiming nothing is what leaves the poll + # unconditional, so the body keeps arriving and keeps being visible. + # + # The selector goes the same way: resuming from a payload this store + # never applied would ask every later connection for changes since + # content it does not hold, with every diagnostic reading healthy. + # + # The transfer is still a wire fact: ``payloads_transferred`` counts + # it above either way. + return _TransferOutcome() return _TransferOutcome( committed=True, changes=changes, - # A declined payload must not move the resume point. Adopting the - # selector of a transfer whose contents this layer just threw away - # would ask the next poll or stream to resume from someone else's - # payload, and skill updates could stop arriving while every - # diagnostic still read healthy. - basis=state if not foreign and isinstance(state, str) and state else None, + basis=state if isinstance(state, str) and state else None, ) def _abandon_in_flight(self) -> None: @@ -2060,10 +2086,15 @@ def _give_up(self, reason: str) -> None: reason, ) - def _apply(self, name: str, data: Any) -> None: + def _apply(self, name: str, data: Any) -> bool: """ Feeds one event to the reader, publishes a commit, and raises the transport error the event calls for, if any. + + Returns whether the event completed an exchange: a committed payload, or + the server confirming that the content held is current. Those are the + two answers that describe what this store now holds, and they are what + ``_poll_once`` adopts an etag on. """ with self._lock: outcome = self._reader.handle(name, data) @@ -2085,6 +2116,7 @@ def _apply(self, name: str, data: Any) -> None: raise _FatalTransportError(outcome.fatal) if outcome.disconnect: raise _RecoverableTransportError(outcome.disconnect) + return outcome.committed or outcome.up_to_date def _poll_once(self) -> None: with self._lock: @@ -2099,20 +2131,37 @@ def _poll_once(self) -> None: result = self._requester.poll(basis, etag) if result.not_modified: logger.debug("Skill payload unchanged (HTTP 304)") - # 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. It is a current answer because the etag that asked for it - # was issued for a body this store applied in full. - self._publish_first_payload() + # A 304 *confirms* the payload this store holds. It cannot establish + # one, and it is not a first payload: the exchange it stands in for + # is the ``none`` intent, which does not publish one either (see + # ``_apply``), and a 304 carries nothing a store holding nothing + # could be initialized from. Publishing here would make + # ``is_initialized`` true over an empty committed set, which is what + # authorises ``write_skills("*")`` to prune, so a 304 answering a + # request that carried no etag — the only way to reach one with + # nothing held — would delete the last known good copy on disk. + # ``_run`` counts the poll as a healthy answer either way. return + completed = False for name, data in result.events: - self._apply(name, data) - with self._lock: - # Adopted only once the whole body has been applied. A body that + completed = self._apply(name, data) or completed + if not completed: + # Adopted only from a body that completed an exchange: one that + # committed a payload, or a ``none`` intent, which is the server + # saying the content held is what the etag describes. A body that # broke off partway — an ``error`` or ``goodbye`` after an announced - # transfer — left the payload it described unapplied, and keeping - # its etag would let the next 304 report a store that is missing - # that payload as current and healthy. + # transfer — raises above and never reaches here. One that merely + # transferred nothing, under an intent code this SDK does not + # recognise, reaches here having committed nothing: its etag + # describes a body whose contents this store does not hold, and + # keeping it would let the next 304 report a store missing that + # payload as current and healthy. + # + # The etag already held is left alone rather than cleared: it was + # earned by a body that did complete, and it still validates that + # content for as long as the basis it was paired with holds. + return + with self._lock: self._etag = result.etag self._etag_basis = basis diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index b9caf25e..bfe5a7cc 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -922,11 +922,19 @@ def test_a_catastrophic_goodbye_is_fatal(self) -> None: ) assert outcome.fatal is not None - def test_transfer_none_holds_everything_and_commits(self) -> None: + def test_transfer_none_holds_everything_and_commits_nothing(self) -> None: + """ + ``none`` is the server saying the payload held is current, so nothing is + applied and nothing is committed. It is a *completed exchange* — which + is what breaks a row of failures, and what lets the next request offer + the body's etag — but it is not a payload. Publishing one would make + ``is_initialized`` true over whatever the store happens to hold, and + that is the fact ``write_skills("*")`` prunes on. + """ held = _SkillObjectSet() reader = _ProtocolReader(held) drive(reader, full_payload(("put-object", put_skill()))) - drive( + intent, outcome = drive( reader, events( ("server-intent", server_intent("none")), @@ -934,6 +942,48 @@ def test_transfer_none_holds_everything_and_commits(self) -> None: ), ) assert len(held) == 1 + assert intent.up_to_date is True + assert outcome.committed is False + # The intent event above already carried the up-to-date answer; the + # transfer completing it adds nothing to report. + assert outcome.up_to_date is False + # Nor does it move the resume point. ``basis-2`` names a payload this + # store was never sent, and resuming from it would ask every later + # connection for changes since a payload it never applied. + assert outcome.basis is None + + def test_a_transfer_that_applied_nothing_is_not_a_commit(self) -> None: + """ + Three shapes reach ``payload-transferred`` with no pending set to apply: + an intent code this SDK does not recognise, a ``none`` intent, and a + lone transfer under no intent at all. None of them applied anything, so + none of them claims anything — neither a commit, which is what publishes + the first payload ``write_skills("*")`` prunes on, nor an up-to-date + answer, which only the server can give and only the ``none`` intent + does, on its own event. + + The transfer is still a wire fact, counted either way. + """ + for payload_events in ( + events( + ("server-intent", server_intent("xfer-future")), + ("put-object", put_skill()), + ("payload-transferred", transferred("basis-1")), + ), + events( + ("server-intent", server_intent("none")), + ("payload-transferred", transferred("basis-1")), + ), + events(("payload-transferred", transferred("basis-1"))), + ): + held = _SkillObjectSet() + reader = _ProtocolReader(held) + outcome = drive(reader, payload_events)[-1] + assert outcome.committed is False + assert outcome.up_to_date is False + assert outcome.basis is None + assert len(held) == 0 + assert reader.diagnostics.payloads_transferred == 1 def test_an_object_arriving_with_no_intent_is_treated_as_a_delta(self) -> None: held = _SkillObjectSet() @@ -1445,15 +1495,68 @@ def test_a_304_keeps_the_held_content(self, endpoint: Any) -> None: assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None assert store.diagnostics.payloads_transferred == 1 assert store.failed is None + # A payload arrived, and a 304 does not take that back. + assert store.is_initialized() is True - def test_a_304_before_any_payload_still_releases_wait_for_skills( + def test_a_304_confirms_a_payload_but_cannot_establish_one( 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.""" + """ + A 304 answers for content this store already holds, and the etag that + asked for it is only ever adopted from a body that completed an + exchange. Reaching one with nothing held therefore takes a server + answering a request that carried no etag at all, and that 304 says + nothing about a payload this store never received. + + Releasing ``wait_for_skills`` on it would make ``is_initialized`` true + over an empty committed set — the fact ``write_skills("*")`` prunes on — + so a reconcile that raced delivery would delete every managed skill on + disk instead of reporting the retrieval unavailable (§3.21). Failing + closed costs a boot that is genuinely waiting nothing it was not already + waiting for. + """ endpoint.queue_poll(status=304) with poll_store(endpoint) as store: - assert store.wait_for_skills(timeout=5) is True + assert store.wait_for_skills(timeout=0.5) is False + assert store.is_initialized() is False + # Not a failure either: the poll was answered, and the store is + # still asking. + assert store.failed is None + assert endpoint.requests[0]["if_none_match"] is None + + def test_an_intent_it_cannot_apply_does_not_lend_its_etag_to_a_304( + self, endpoint: Any + ) -> None: + """ + The chain this closes: a body under a future intent code announces and + transfers a payload this SDK cannot apply, its etag is adopted as though + the body had been applied in full, and the next 304 reports the empty + store it left behind as current. That store is initialized, healthy, and + authorises a prune of every managed skill on disk, with nothing in the + 304 to notice it on. + + An etag is adopted only from a body that completed an exchange, so the + second request carries none and the endpoint's standing 304 cannot + answer for content that never arrived. + """ + endpoint.queue_poll( + events( + ("server-intent", server_intent("xfer-future")), + ("put-object", put_skill()), + ("payload-transferred", transferred("basis-1")), + ), + etag='W/"v1"', + ) + with poll_store(endpoint) as store: + assert wait_until(lambda: len(endpoint.requests) >= 2) + assert store.is_initialized() is False + assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is None + assert endpoint.requests[1]["if_none_match"] is None + # Nor is the selector of a payload it could not apply a resume point. + assert [r["query"].get("basis") for r in endpoint.requests[:2]] == [ + None, + None, + ] def test_a_mixed_payload_over_the_wire_yields_only_the_skill( self, endpoint: Any @@ -2648,6 +2751,7 @@ def test_an_over_cap_poll_body_is_not_applied_and_is_retried( ) -> None: monkeypatch.setattr(skills_fdv2, "MAX_RESPONSE_BYTES", 2048) endpoint.queue_poll(full_payload(("put-object", put_skill(content="x" * 8192)))) + endpoint.queue_poll(full_payload(("put-object", put_skill()))) # A long enough backoff to observe the failure before the retry lands. with poll_store(endpoint, initial_backoff=0.3, max_backoff=0.3) as store: assert wait_until(lambda: store.diagnostics.connection_failures == 1) @@ -2656,7 +2760,10 @@ def test_an_over_cap_poll_body_is_not_applied_and_is_retried( assert store.diagnostics.payloads_transferred == 0 assert store.diagnostics.skill_objects_received == 0 assert store.failed is None - # The retry is an ordinary poll; the endpoint answers it 304. + # The retry is an ordinary poll, and the payload it is answered + # with is what releases the waiter. A 304 could not: nothing has + # committed, and a 304 confirms a payload rather than establishing + # one. assert wait_until(lambda: len(endpoint.requests) >= 2) assert store.wait_for_skills(timeout=5) is True assert wait_until(lambda: store.diagnostics.connection_failures == 0) From 346a552d7ec9fab0a79c8272540414b415dffee9 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Fri, 2 Oct 2026 10:53:18 -0400 Subject: [PATCH 2/2] docs(client): trim the 304/etag comments to what a consumer needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three blocks the previous commit added were arguing the whole case inline — the prune chain, the forward-compatibility reasoning, the cross-language note — in 50-odd lines of comment on a 14-line change. This source gets read by customers and their agents working out how to consume the SDK, and that much nuance about internals they cannot reach is a wall to read past rather than help. Each block now states the rule and the one consequence that is visible from outside: `is_initialized()` / `isInitialized()` goes true on a commit, and that is what authorizes a prune of the files on disk. The long version already has two homes it belongs in — `agents.md` in this package, and TESTING.md §3.25 — so nothing is lost, and the clause that stops a plausible wrong fix ("not up to date either: only the `none` intent says that") is kept. Comments only; no behaviour change. Tests untouched, where the long-form reasoning is the point. Co-Authored-By: Claude Opus 5 --- .../src/launchdarkly_ai_server/skills_fdv2.py | 75 +++++-------------- 1 file changed, 20 insertions(+), 55 deletions(-) diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index 8ccf1a63..4ceae7f9 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -795,9 +795,9 @@ def _payload_transferred(self, data: Any) -> _TransferOutcome: # whose selector must not become the resume point if it is not the # payload skills arrive on. foreign = self._is_foreign_payload(payload_id) - # Whether this transfer moved the committed set. A ``none`` intent - # builds no pending set, nor does an intent code this SDK does not - # recognise, and a foreign payload's contents are declined below. + # A ``none`` intent builds no pending set, nor does an intent code this + # SDK does not recognise, and a foreign payload's contents are declined + # below. applied = not foreign and self._pending is not None if foreign: self._warn_foreign_payload(payload_id) @@ -838,31 +838,12 @@ def _payload_transferred(self, data: Any) -> _TransferOutcome: len(self._committed), ) if not applied: - # A transfer that applied nothing claims nothing: not a commit, and - # not an up-to-date answer either. - # - # Not a commit, because a commit publishes a first payload, and - # ``is_initialized`` is the fact ``write_skills("*")`` prunes on. An - # empty committed set reported as a payload reads as an environment - # whose every skill was revoked, which deletes the last known good - # copy on disk. - # - # Not up to date either, because only the server can say that, and - # only the ``none`` intent does — on its own event, which has - # already reported it by the time a transfer completing it arrives. - # An intent code this SDK does not recognise says the opposite: the - # body carried objects this reader dropped, so the content held is - # *not* what that body describes, and an etag adopted from it would - # let a 304 report the store as current for as long as the server - # kept re-announcing it. Claiming nothing is what leaves the poll - # unconditional, so the body keeps arriving and keeps being visible. - # - # The selector goes the same way: resuming from a payload this store - # never applied would ask every later connection for changes since - # content it does not hold, with every diagnostic reading healthy. - # - # The transfer is still a wire fact: ``payloads_transferred`` counts - # it above either way. + # Nothing applied, so nothing to report — and in particular not a + # commit, because a commit publishes the first payload, and + # ``is_initialized`` (what ``write_skills("*")`` authorises a prune + # on) must not go true over a store that received nothing. Not up to + # date either: only the ``none`` intent says that, on its own event. + # The selector is withheld too — it names a payload never applied. return _TransferOutcome() return _TransferOutcome( committed=True, @@ -2091,9 +2072,8 @@ def _apply(self, name: str, data: Any) -> bool: Feeds one event to the reader, publishes a commit, and raises the transport error the event calls for, if any. - Returns whether the event completed an exchange: a committed payload, or - the server confirming that the content held is current. Those are the - two answers that describe what this store now holds, and they are what + Returns whether the event completed an exchange — a commit, or the + server confirming the content held is current — which is what ``_poll_once`` adopts an etag on. """ with self._lock: @@ -2131,35 +2111,20 @@ def _poll_once(self) -> None: result = self._requester.poll(basis, etag) if result.not_modified: logger.debug("Skill payload unchanged (HTTP 304)") - # A 304 *confirms* the payload this store holds. It cannot establish - # one, and it is not a first payload: the exchange it stands in for - # is the ``none`` intent, which does not publish one either (see - # ``_apply``), and a 304 carries nothing a store holding nothing - # could be initialized from. Publishing here would make - # ``is_initialized`` true over an empty committed set, which is what - # authorises ``write_skills("*")`` to prune, so a 304 answering a - # request that carried no etag — the only way to reach one with - # nothing held — would delete the last known good copy on disk. - # ``_run`` counts the poll as a healthy answer either way. + # A 304 confirms the payload this store holds; it cannot establish + # one. ``is_initialized`` stays false until something commits, so a + # store that has received nothing never authorises a prune of the + # files on disk. ``_run`` counts the poll as a healthy answer. return completed = False for name, data in result.events: completed = self._apply(name, data) or completed if not completed: - # Adopted only from a body that completed an exchange: one that - # committed a payload, or a ``none`` intent, which is the server - # saying the content held is what the etag describes. A body that - # broke off partway — an ``error`` or ``goodbye`` after an announced - # transfer — raises above and never reaches here. One that merely - # transferred nothing, under an intent code this SDK does not - # recognise, reaches here having committed nothing: its etag - # describes a body whose contents this store does not hold, and - # keeping it would let the next 304 report a store missing that - # payload as current and healthy. - # - # The etag already held is left alone rather than cleared: it was - # earned by a body that did complete, and it still validates that - # content for as long as the basis it was paired with holds. + # An etag describes the body it came with, so it is adopted only when + # that body is also what the store now holds: a commit, or a ``none`` + # intent. A transfer this SDK could not apply is neither, and keeping + # its etag would let the next 304 confirm content never applied. Any + # etag already held stays valid, so it is left alone, not cleared. return with self._lock: self._etag = result.etag