Skip to content

fix(client): reset connection_failures when a store that gave up is started again - #139

Open
XieX wants to merge 1 commit into
xie/agent-skillsfrom
xie/skills-restart-resets-counter
Open

XieX wants to merge 1 commit into
xie/agent-skillsfrom
xie/skills-restart-resets-counter

Conversation

@XieX

@XieX XieX commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

TL;DR

After a store gives up and you call start() again, diagnostics.connection_failures kept the old run's count. So a healthy restarted store looked like it was already in an outage. This is a one-line fix, and it matches the TypeScript SDK. Found in review of https://github.com/launchdarkly/ai-sdks-monorepo/pull/40#pullrequestreview-5419525312.

Changes

  • _rearm_waiters resets diagnostics.connection_failures, alongside the internal _failures count it already reset. Before this, two recoverable failures and then a 401, followed by start(), gave failed is None and connection_failures == 2, with nothing failed in the new run. TypeScript resets the counter in start() (js-ai-sdk#107).
  • New test test_a_restart_reports_no_failures_before_the_new_run_has_any. It holds the new run's first request open and reads the counter right after start(). The existing test_a_restart_resets_the_failure_count couldn't catch this, because it reads the counter only after the new run fails, and that failure overwrites the stale value. The new test fails with 2 == 0 when the reset is reverted.
  • README (recovery after a 422) and agents.md now say the counter resets on restart.

Spec: TESTING.md §3.25 in https://github.com/launchdarkly/ai-sdks-monorepo/pull/40.

Test plan

  • make lint, make format-check, make typecheck
  • make test: 2201 passed, 11 skipped
  • New test fails with the fix reverted
  • Note: TestWatchSkillsOverTheTransport::test_a_revocation_prunes_without_a_restart failed once in my runs. It also fails on xie/agent-skills without this change (1 in 25 runs), so it's an existing flaky test.

🤖 Generated with Claude Code


Note

Overview
FDv2SkillStore.start() now clears diagnostics.connection_failures when a store that gave up is restarted, alongside the internal _failures counter and backoff state. Before this change, a successful start() could leave failed cleared while diagnostics still showed the previous run’s recoverable failure streak, so operators could misread a healthy restart as an ongoing outage. Behavior is aligned with the TypeScript SDK.

A new test holds the first request of the restarted run open and asserts the counter is 0 immediately after start(), not only after the new run records another failure (which would mask a stale value).

README and agents.md document that recovery via start() resets connection_failures; fatal errors such as HTTP 422 still do not increment that counter.

Reviewed by Cursor Bugbot for commit 62fe384. Bugbot is set up for automated code reviews on this repo. Configure here.

…tarted again

`_rearm_waiters` reset the internal `_failures` count but not
`diagnostics.connection_failures`, so a restarted store reported the old
run's failures until its new run failed or succeeded: after two
recoverable failures and a 401, `start()` left `failed` None with
`connection_failures == 2` and nothing failed in the new run. That
counter is the outage signal now that recoverable failures retry
indefinitely, so a healthy restart read as a store already riding out
an outage. The TypeScript SDK resets it in `start()`.

`test_a_restart_resets_the_failure_count` could not see this: it reads
the counter only after the new run's first failure, which overwrites
the stale value. The new test holds the new run's first request open
and reads the counter right after `start()`; it fails with `2 == 0`
when the reset is reverted.

Spec: launchdarkly/ai-sdks-monorepo#40 (TESTING.md §3.25).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@XieX
XieX requested a review from jeffdupont October 5, 2026 19:43
@jeffdupont jeffdupont mentioned this pull request Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants