Skip to content

Commit b214bbd

Browse files
XieXclaude
andauthored
fix(skills): coalesce a watcher's debounce window into one reconcile (#215)
## What `SkillWatcher` cleared its wake flag *before* waiting out the debounce. A notification that arrived inside the window set the flag again, so a second reconcile followed right after the first: a burst of changes spread across one window ran two reconciles, not one. The TypeScript watcher restarts its timer on every notification and runs one. The debounce now trails. After each window the watcher checks whether another change arrived and, if one did, waits a fresh window. It reconciles only after delivery has been quiet for a whole window. The reconcile reads the store after that last window, so it sees every change in the burst. Nothing is lost. The flag is still cleared before each window, not after the reconcile. A change that lands while a reconcile is running still schedules exactly one more pass. ## Behavior change A steady stream of notifications closer together than `debounce` now **postpones** the reconcile until the stream pauses. Before, the watcher reconciled about once per window throughout the stream. A revocation now takes effect `debounce` seconds after delivery goes quiet, not within `debounce` of its arrival. The README, the module docstring and the `debounce` argument docs say so now. ## Tests - `test_a_burst_of_changes_coalesces_into_a_debounce_window` now pins **one** reconcile for six notifications spread 100 ms apart across a 600 ms window, where it pinned two. It keeps the anti-vacuity setup: the puts come after the listener is attached and are spread out, and the count is an equality check after settling past two windows. It also checks that the single reconcile wrote all six skills. - New: `test_a_change_during_a_reconcile_schedules_one_more`. A store double delivers a new skill just after an armed reconcile has taken its snapshot. The test asserts exactly two reconciles and checks that the late skill reached disk. - Mutation checks, each run against the two tests above: - debounce removed (reconcile straight after the wake): the coalescing test fails - the previous single-window code: the coalescing test fails (2 ≠ 1) - trailing window with the flag cleared after the reconcile: the mid-reconcile test fails `make lint format-check typecheck` and `uv run pytest packages/client tests` pass: 1924 passed, 18 skipped. ## Follow-up for #211 #211 (`SkillSource` / `config_skills`) has not merged. Its `test_the_debounce_spans_both_trigger_kinds` in `tests/test_skills_source.py` pins the old behavior: `reconciles == 2` and `source.reads == 1 + 2`. Once this lands, that test needs the same edit: `reconciles == 1`, `source.reads == 1 + 1`, and the docstring reworded to drop "schedules exactly one further pass". 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **`SkillWatcher`** debounce is now **trailing**: each delivery notification restarts the quiet window, and reconcile runs only after **`debounce`** seconds with no further changes. Bursts that used to trigger about one reconcile per window (e.g. six spaced puts → two passes) collapse to **one** pass that reads the store after the last window. > > The worker loop clears the wake flag **before** each wait (unchanged intent): a change **during** an in-flight reconcile still schedules exactly one follow-up reconcile. Docs in **`README`**, **`skills_watch`**, and **`watch_skills`** now describe “quiet for `debounce`” instead of “within `debounce` of arrival,” including revocation timing. > > Tests expect **one** reconcile for the spread burst, assert all six skills on disk, and add **`test_a_change_during_a_reconcile_schedules_one_more`** for the mid-reconcile case. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 224f1ab. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 715815f commit b214bbd

3 files changed

Lines changed: 67 additions & 19 deletions

File tree

‎packages/client/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -848,7 +848,7 @@ that skips verification.
848848
| `SkillStore` | The structural interface content arrives through: `get_object(kind, key, version=None)`, `all_objects(kind)`, optional `is_initialized()`, `add_listener(kind, fn)` / `remove_listener(kind, fn)`. A store without `is_initialized()` is treated as initialized. Both shipped stores deliver only the skill kind, so `add_listener` on any other kind raises. |
849849
| `InMemorySkillStore(objects=None)` | A dict-backed store with `put(raw)`, for local development and testing. Holds several versions of a key. |
850850
| `FDv2SkillStore(sdk_key, *, base_uri=…, stream_uri=…, mode="stream", …)` | The delivery transport: a store fed by LaunchDarkly over the SDK-facing FDv2 channel. `start()`, `wait_for_skills(timeout)`, `is_initialized()`, `close()`, `diagnostics`, `failed`; also a context manager. `close()` is **final** — `start()` afterwards raises. `poll_interval`, `read_timeout`, `initial_backoff` and `max_backoff` must be positive and finite, and `initial_backoff` may not exceed `max_backoff`. **Server-side only.** See *Receiving skills from LaunchDarkly* above. |
851-
| `watch_skills(skills, root, *, debounce=0.5, on_reconcile=None, …)` | `write_skills` plus a re-reconcile on every delivery change, so revocation takes effect within `debounce` rather than at the next restart. Returns `(initial report, SkillWatcher)`; close the watcher when done. `debounce` is in **seconds**, non-negative and finite. `on_reconcile` receives each *subsequent* report. One watcher per root. |
851+
| `watch_skills(skills, root, *, debounce=0.5, on_reconcile=None, …)` | `write_skills` plus a re-reconcile on every delivery change, so revocation takes effect once delivery has been quiet for `debounce` rather than at the next restart. Returns `(initial report, SkillWatcher)`; close the watcher when done. `debounce` is in **seconds**, non-negative and finite. `on_reconcile` receives each *subsequent* report. One watcher per root. |
852852
| `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `payloads_ignored`, `hashless_objects`, `connection_failures`, `last_error`. |
853853

854854
Configure the store with `set_skill_store(store)`. It applies on every call, before or after

‎packages/client/src/launchdarkly_ai_server/skills_watch.py‎

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@
33
44
``write_skills`` is a one-shot reconcile of what the store holds now.
55
``watch_skills`` re-runs it whenever the store reports a change. With ``"*"``,
6-
a skill revoked in LaunchDarkly is removed from disk within a debounce interval
7-
rather than at the next restart. An explicit list is fixed: a skill the store
8-
answers ``absent`` for keeps its files and reports an error, and a config change
9-
that unpins a skill or moves it to a new version is not seen.
6+
a skill revoked in LaunchDarkly is removed from disk once delivery has been
7+
quiet for a debounce interval, rather than at the next restart. An explicit
8+
list is fixed: a skill the store answers ``absent`` for keeps its files and
9+
reports an error, and a config change that unpins a skill or moves it to a new
10+
version is not seen.
1011
1112
``on_unavailable="keep"`` remains the default, so an outage never deletes the
1213
application's skill files.
@@ -31,8 +32,9 @@
3132

3233
DEFAULT_DEBOUNCE_SECONDS = 0.5
3334
"""
34-
Default debounce, in seconds: how long to wait after a change before
35-
reconciling, so a payload of many skills triggers one reconcile, not one each.
35+
Default debounce, in seconds: how long delivery must stay quiet after a change
36+
before reconciling, so a payload of many skills triggers one reconcile, not one
37+
each.
3638
"""
3739

3840

@@ -105,11 +107,18 @@ def _run(self) -> None:
105107
continue
106108
if self._stop.is_set():
107109
return
108-
# Clear before the debounce so a change arriving mid-debounce
109-
# triggers another pass instead of being swallowed by this one.
110-
self._wake.clear()
111-
if self._stop.wait(self._debounce):
112-
return
110+
# A trailing debounce: wait until a whole window passes with no new
111+
# change, so a burst spread across the window reconciles once. The
112+
# reconcile reads the store after the last window, so it sees every
113+
# change in the burst. Clearing before each window, not after the
114+
# reconcile, means a change arriving during the reconcile itself
115+
# still schedules one more pass instead of being swallowed by it.
116+
while True:
117+
self._wake.clear()
118+
if self._stop.wait(self._debounce):
119+
return
120+
if not self._wake.is_set():
121+
break
113122
self._reconcile_once()
114123

115124
def _reconcile_once(self) -> None:
@@ -206,8 +215,9 @@ async def watch_skills(
206215
the store's lifetime.
207216
208217
Args:
209-
debounce: Seconds to wait after a change before reconciling. Must be a
210-
non-negative finite number.
218+
debounce: Seconds delivery must stay quiet after a change before
219+
reconciling; each further change restarts the wait, so a burst
220+
reconciles once. Must be a non-negative finite number.
211221
on_reconcile: Called with each re-reconcile's report (not the initial
212222
one). Exceptions it raises are logged and do not stop the watcher.
213223

‎packages/client/tests/test_skills_watch.py‎

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,15 +67,15 @@ async def test_the_in_memory_store_can_also_drive_a_watch(
6767
async def test_a_burst_of_changes_coalesces_into_a_debounce_window(
6868
self, tmp_path: Any, make_raw_skill: Any
6969
) -> None:
70-
"""Six notifications spread across one window produce two reconciles.
70+
"""Six notifications spread across one window produce one reconcile.
7171
7272
To discriminate rather than pass vacuously, the puts happen after the
7373
listener is attached, the counter is asserted against an exact figure,
7474
and each put lands in its own scheduler pass (a back-to-back burst would
7575
collapse to one reconcile with or without a debounce).
7676
77-
Two, not one: the wake flag is cleared before the window, so a change
78-
arriving inside it schedules one further pass.
77+
The debounce trails: each change restarts the window, so the reconcile
78+
runs once delivery goes quiet and sees every skill in the burst.
7979
"""
8080
store = InMemorySkillStore()
8181
set_skill_store(store)
@@ -85,10 +85,48 @@ async def test_a_burst_of_changes_coalesces_into_a_debounce_window(
8585
for i in range(6):
8686
store.put(make_raw_skill(key=f"skill-{i}"))
8787
time.sleep(0.1)
88-
assert wait_until(lambda: watcher.reconciles >= 2, timeout=10)
89-
# Settle well past two windows, so a third would have landed by now.
88+
assert wait_until(lambda: watcher.reconciles >= 1, timeout=10)
89+
# Settle well past two windows, so a second would have landed by now.
9090
time.sleep(1.5)
91+
assert watcher.reconciles == 1
92+
for i in range(6):
93+
assert (tmp_path / "s" / f"skill-{i}" / "SKILL.md").exists()
94+
finally:
95+
watcher.close()
96+
97+
async def test_a_change_during_a_reconcile_schedules_one_more(
98+
self, tmp_path: Any, make_raw_skill: Any
99+
) -> None:
100+
"""The wait for quiet ends before the reconcile starts, so a change
101+
landing while it runs is not swallowed by it: exactly one more pass
102+
follows, and that pass writes the late skill."""
103+
104+
class ChangesDuringSnapshot(InMemorySkillStore):
105+
"""Delivers a new skill the moment an armed reconcile has taken its
106+
snapshot, so the snapshot cannot include it."""
107+
108+
def __init__(self) -> None:
109+
super().__init__()
110+
self.armed = False
111+
112+
def all_objects(self, kind: str) -> dict[str, dict[str, Any]]:
113+
objects = super().all_objects(kind)
114+
if self.armed:
115+
self.armed = False
116+
self.put(make_raw_skill(key="late"))
117+
return objects
118+
119+
store = ChangesDuringSnapshot()
120+
set_skill_store(store)
121+
_report, watcher = await watch_skills("*", tmp_path / "s", debounce=0.05)
122+
try:
123+
store.armed = True
124+
store.put(make_raw_skill(key="early"))
125+
assert wait_until(lambda: watcher.reconciles >= 2, timeout=10)
126+
# Settle well past the window, so a third would have landed by now.
127+
time.sleep(0.5)
91128
assert watcher.reconciles == 2
129+
assert (tmp_path / "s" / "late" / "SKILL.md").exists()
92130
finally:
93131
watcher.close()
94132

0 commit comments

Comments
 (0)