Conversation
A `none` intent says the basis is current, not that the connection is finished: later edits arrive on the same stream as put-object / delete-object and payload-transferred with no second server-intent. The reader kept `none` as an intent that carries no objects, so it dropped those events under objectsIgnored and then adopted the new selector as its basis anyway, leaving a revoked skill served across reconnects. Treat `none` as `xfer-changes` with nothing pending, as the base SDKs do (Python's ChangeSetBuilder.expect_changes(), js-core's protocol handler). The foreign-payload check on the following payload-transferred is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Remove the maxConsecutiveFailures option. A count bound turns a few minutes of outage into a process that receives no updates, and no revocations, for the rest of its life, and the base server-side SDKs do not give up on a recoverable error either. Only a fatal status (401, 403, 404, 405, 406, 414, 422, 501, the second or stateless 400, a catastrophic goodbye, an unexpected error) now stops delivery. connectionFailures still counts consecutive recoverable failures, drives the backoff, and is reset by a commit or a `none` intent. The 400 repair no longer needs an exemption from a budget, and is still retried once because the retry drops the state that made it retryable. backoffDelayMs clamps its exponent, so an unbounded attempt number stays finite (and a zero base no longer yields NaN). Tests that gave up after N failures are rewritten to assert retrying past ten, per-drop counting, fails/succeeds/fails reporting one failure, a finite backoff at attempt 10,000, and the option's absence by name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
watchSkills removes a revoked skill's SKILL.md without a restart only
for writeSkills('*'). With an explicit list such as skillRefs(config),
a skill the store answers absent for stays requested as an error action
and is not pruned, and the watcher listens to the skill store only, so
unpinning a skill from a config is not seen either.
Also correct the docs that implied an absent reference makes a reconcile
incomplete: only a missing, uninitialized or throwing store, or an
exhausted timeout, does.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jeffdupont
left a comment
jeffdupont
left a comment
There was a problem hiding this comment.
Parity follow-up to my review of python #131 (launchdarkly/python-ai-sdk#131), against my GA review of #71. I reviewed 9f0b870 against its base, xie/agent-skills-feature-ac9ac7 at 86265f3. yarn test: all workspaces pass, client 1177 passed and 10 skipped, exit 0. yarn typecheck and yarn code:check are clean. I reverted each fix on its own and re-ran the new tests. Without this.intent = INTENT_TRANSFER_CHANGES, all three none tests fail. Without the clamp, the finite-backoff test fails. With the base skills-fdv2.ts put back, the five retry/no-option tests fail.
Blocker status matches Python. (1) Changes after none and (3) giving up after 10 failures are resolved, and the behaviour matches #131. (2) The skill-key grammar is untouched and still needs the SDK-or-API decision. (4) Explicit-list revocation is docs only, which I think is acceptable for 1.0 for the reason in the #131 review: the frozen default is the safe one, and the fixes are additive. Removing maxConsecutiveFailures isn't breaking, because FDv2SkillStore has never been on main (checked with git grep origin/main).
One thing to settle before merge, the same as Python:
initialBackoffMs/maxBackoffMsaren't validated, and with no budget, a zero or negative value spins forever. Reproduced with an always-failing requester: about 860 attempts/s forinitialBackoffMs: 0,maxBackoffMs: 0andmaxBackoffMs: -5, withfailedstayingnull. The base branch stops after 11. The new test even asserts a 0 ms delay as valid output. Inline.
Smaller notes:
- Parity is good on behaviour and names. The
nonehandling, the removed option, the 400 one-retry repair and the fatal set all match. Python clamps the exponent at 62 and JS at 30. Both are fine, since either is far above any realistic cap. connectionFailuresdisagrees with Python on agoodbyeafter a complete answer. JS exempts it (reachedServer, line 1810); Python counts it and setslast_error. This predates the PR, but the counter is now the main outage signal. The spec (launchdarkly/ai-sdks-monorepo#36) should say which is right.- Restart after a fatal error differs. Python's
start()resumes a store that gave up. Herestart()does nothing oncefailedis set (docstring at line 1536), and the give-up log says "until the process restarts". This predates the PR; I'm noting it because both PRs now describe recovery. - These apply to both languages; details in #131:
Retry-Afteris used without jitter (line 1758), so a fleet reconnects together. Backoff resets on everynone, where the base SDK waits for 60s of uptime, so a server that answersnoneand drops gets about 1/s from every process. A response overMAX_RESPONSE_CHARSis now re-downloaded every 15-30s and never setsfailed. - The JS tests are more complete than Python's.
waitForSkillsrunning to its timeout, fail/succeed/fail retrying at the first backoff step, and the rewritten 400 tests are all missing on the Python side. I've asked for them there. - Spec #36 matches this PR on both behaviour changes. I've suggested adding backoff-option validation and
goodbyecounting there.
| @@ -1516,7 +1517,6 @@ export class FDv2SkillStore implements SkillStore { | |||
| } | |||
| this.initialBackoffMs = options.initialBackoffMs ?? 1_000; | |||
There was a problem hiding this comment.
Before, the failure budget stopped a bad value here after 11 attempts. Now nothing does. Reproduced at 9f0b870 with a requester that always throws RecoverableTransportError: initialBackoffMs: 0, maxBackoffMs: 0 and maxBackoffMs: -5 each give about 860 attempts/s, and failed stays null. The Math.min(..., this.maxBackoffMs) at line 1761 makes any non-positive cap a zero wait, and a NaN would do the same through setTimeout.
Could both get the positive-and-finite check that pollIntervalMs has just above, plus initialBackoffMs <= maxBackoffMs? Python #131 needs the same check.
| expect(delay).toBeGreaterThanOrEqual(0); | ||
| } | ||
| expect(backoffDelayMs(10_000, 1000, 30_000, 0)).toBe(30_000); | ||
| expect(backoffDelayMs(10_000, 0, 30_000)).toBe(0); |
There was a problem hiding this comment.
This asserts a zero delay as correct output. The PR description says the clamp fixes 0 * Infinity = NaN for a zero initialBackoffMs, but 0 ms and NaN both mean an immediate retry. If the constructor rejects a non-positive base, this case can go, or become a constructor test that expects the throw.
| } else if (intent === INTENT_TRANSFER_NONE) { | ||
| // The basis is current, and later edits on this connection arrive as | ||
| // objects with no second intent: read on as a delta with nothing pending. | ||
| this.intent = INTENT_TRANSFER_CHANGES; |
There was a problem hiding this comment.
This resolves the #71 blocker and matches Python #131. Reverting just this line fails all three new tests.
This isn't new here, but it now applies for as long as an outage lasts: none returns healthy, and recordSuccess resets the backoff straight away. The base Python SDK resets only after a connection has stayed open 60s (BACKOFF_RESET_INTERVAL); I haven't checked js-core. A server that answers none and then drops gets reconnected at about 1/s by every process. My Python probe saw 27 connections in 20s with the default backoff. I think this belongs in spec #36 rather than in this PR.
With no consecutive-failure bound, initialBackoffMs and maxBackoffMs are the only limit on the retry loop: a zero, negative or NaN value turns every wait into none, and a store pointed at a failing server reconnects as fast as the network allows. Both must now be positive and finite, and initialBackoffMs may not exceed maxBackoffMs; the constructor throws otherwise, as it does for pollIntervalMs (TESTING.md §3.25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s open 60s The backoff delay followed connectionFailures, which a commit or a none intent resets. A degraded server that answers none and then drops every connection was therefore reconnected at initialBackoffMs indefinitely, by every process; with no failure bound, nothing else stopped that. The delay now has its own attempt number. It advances on every reconnect that follows a failure or a dropped stream, a goodbye recycle included, and returns to the first step only when the stream that just ended had been open for BACKOFF_RESET_INTERVAL_MS (60s), as js-core's Backoff does. A completed poll also resets it, since pollIntervalMs already spaces the requests. connectionFailures keeps its meaning: reset by a commit or a none intent. The interval is internal; tests shorten it through the instance's _backoffResetIntervalMs (TESTING.md §3.25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A poll body or streamed event over MAX_RESPONSE_CHARS (64 Mi characters) was a recoverable failure. The payload's size belongs to the environment, not the connection, so every retry was refused the same way: the store re-downloaded up to 64 Mi characters on every backoff step, from every process, for the life of the process, and never set failed. It is now a FatalTransportError, accounted like a 422: failed and lastError are set, connectionFailures does not move, last known good is still served, and exactly one request is made. As with a 422, a store that has given up does not resume; recovery is a restart or a new store, as the README already says for 422 (TESTING.md §3.25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A goodbye after a completed exchange is how the server recycles a long-lived stream, and the delivery loop already exempted it from connectionFailures and lastError. But ProtocolReader warned for every non-silent goodbye before anything knew whether the exchange had completed, so a healthy recycling stream still logged a warning. The reader now logs the goodbye at debug, and the loop logs an exempt reconnect at debug too. A goodbye before any completed exchange still counts, and still warns from the loop. Tests pin both: repeated answer-then-goodbye recycles read connectionFailures 0 and lastError null throughout with no warning, and a goodbye-only server warns (TESTING.md §3.25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Since the backoff step was split from connectionFailures, resetting the counter no longer changes the retry delay. Two test comments and one agents.md bullet still described a counter reset as what would pin a failing server to the initial backoff; they now describe what it does hide, the failure from diagnostics. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
JS counterpart of launchdarkly/python-ai-sdk#131, from review comments on launchdarkly/python-ai-sdk#87 that also apply to #71. Targets
xie/agent-skills-feature-ac9ac7.Changes
noneintent (r4167784120).serverIntent()now sets the intent toxfer-changesonnone, as js-core'sprotocolHandler.tsdoes. Before, aput-object/delete-objectafternonewent toignoreUnderUnknownIntent()and was dropped while the nextpayload-transferredstill advanced the basis, so a skill revoked after a routine reconnect kept being served.maxConsecutiveFailures; retry recoverable failures indefinitely (r4167784131). Only fatal statuses stop delivery and setfailed. The 400-retried-once repair is unchanged.2 ** nreachesInfinityat large attempt numbers, and with a zeroinitialBackoffMsthat gave0 * Infinity = NaN.'*'(r4167784141, docs only), in the README,agents.mdand theskills-watch.tsheader. Also fixes theresolveRequestsdoc andagents.md§4b, which said an unresolved reference makes a run incomplete.Tests
none(reader level), and a listener getting the tombstone on a stream. All three fail with the fix reverted.waitForSkillsruns to its timeout during an outage; fails/succeeds/fails reports 1; an intent-then-drop repeated 5 times readsconnectionFailures === 5; backoff at attempt 10,000 is finite;maxConsecutiveFailuresis absent (source check plus runtime).yarn test(1177 passed, 10 skipped),yarn typecheck,yarn code:checkall pass.Spec: launchdarkly/ai-sdks-monorepo#36
🤖 Generated with Claude Code
Note
Overview
FDv2 skill delivery is reworked so recoverable outages no longer permanently stop updates, post-reconnect deltas apply correctly, and oversized payloads fail like other terminal errors.
ProtocolReadernow treats anoneserver intent as “basis current, deltas still allowed” (xfer-changes), soput-object/delete-objectafter a routine reconnect are applied instead of ignored while the basis advances—fixing missed revocations on live streams.FDv2SkillStoredropsmaxConsecutiveFailures: only fatal transport errors setfailedand end delivery; recoverable failures retry indefinitely withconnectionFailuresas telemetry only. Backoff uses a separatebackoffAttempt(resets after a 60s stable stream or a completed poll), with constructor validation oninitialBackoffMs/maxBackoffMsand a clamped backoff exponent so huge attempt counts stay finite.Poll bodies and SSE events over
MAX_RESPONSE_CHARS(64 Mi) areFatalTransportError(422-style give-up), not retried recoverable reads. Routine streamgoodbyerecycles log at debug; warnings stay on counted failures.Docs and comments narrow on-disk revocation via
watchSkillsto'*'(explicit skill lists do not prune absent skills or config unpins). Tests cover the new protocol, retry, backoff, and fatal oversize paths.Reviewed by Cursor Bugbot for commit 29711fe. Bugbot is set up for automated code reviews on this repo. Configure here.