Repository navigation
fix(skills): order the distillation cursor by epoch, not naive-local ISO (#13948) - #13990
Conversation
…ISO (#13948) _select_pending_sessions compared `updated_at <= cursor` lexicographically, on a comment claiming ISO-8601 sorts lexicographically. That holds only at a fixed UTC offset, and the values are naive LOCAL time (datetime.fromtimestamp(st_mtime)). At the autumn DST fallback the local clock repeats an hour, so a session written in the repeated hour renders as a string comparing below an already-advanced cursor and is skipped permanently — up to an hour of conversations, once a year, with nothing in the logs. The cursor's stated guarantee is "bounded re-work, never a silently skipped conversation"; this broke exactly that. session_listing now emits updatedAtEpoch (raw st_mtime) alongside the ISO fields, from both producers that feed list_sessions_fast. Parsing the ISO string back was not an option: during the repeated hour it names two instants, so it is ambiguous precisely where correctness is needed. The ISO fields are untouched, so no API contract changes. The durable cursor migrates rather than resets: _cursor_to_epoch reads the epoch format and falls back to the legacy ISO one. A reset would re-distil the whole corpus at real LLM cost, which is why the issue called it out. A session with no usable timestamp is now skipped with a warning instead of silently — it cannot advance the cursor, so distilling it would repeat forever, but the skip should at least be visible. Three existing tests asserted the raw stored string. They now assert which conversation the cursor names, via _cursor_to_epoch — behaviour, not format.
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
#13948) Review findings on PR #13990, all fixed. The migration docstring claimed "worst case, one hour is re-distilled". That was wrong in both directions: a naive-local string read under a different process timezone shifts by the full offset delta, and resolving it LATE skips conversations silently — the same failure class as the bug being fixed. _iso_to_epoch now returns the earliest instant the string could name, via min(fold=0, fold=1). Inside a DST-repeated hour that is the safe one of the two candidates; outside it both folds agree and the reading is exact, so migration costs nothing in the normal case. I first used a blunt 26h backwards bias and reverted it: it re-distilled a day of conversations on every migration, which is the LLM spend this issue explicitly warned against, and it was guesswork where `fold` gives the exact answer. Also: - non-finite cursors rejected. float("nan") parses, and a nan cursor compares False against everything, silently turning the gate into a no-op so every conversation looks pending on every run. - an unreadable cursor now logs before falling back to a first run. That is the more expensive failure (full-corpus re-distillation) and it was the one case the module handled silently, against its own stated philosophy. - _select_pending_sessions' cursor type hint corrected to float | None. The fold test pins the process timezone with a fixture. Without it the test passed in Riga and failed in UTC — a test that only holds on the machine that wrote it is worse than no test. Suite verified green under TZ=UTC, Europe/Riga, America/New_York, Pacific/Kiritimati and Asia/Kolkata.
Review findings addressed — one was more than a doc nitThe verdict was SAFE with four non-blocking items. Three were small; the first turned out to be a real correctness issue in the migration path, so I want to be explicit about it. 1. The migration could skip silently — fixedMy docstring claimed "worst case, one hour is re-distilled". That was wrong in both directions. A naive-local string read under a different process timezone shifts by the full offset delta, and resolving it late skips every conversation between the true instant and the over-estimate — permanently and silently. That is the same failure class as the bug this PR fixes, reintroduced in the migration path.
Worth recording that my first attempt was worse. I biased the migrated cursor backwards by a blunt 26h (the widest UTC offset span). It killed the mutants, but it re-distilled a full day of conversations on every migration — the exact LLM spend this issue warns about — and it was guesswork in a place where The residual TZ-change exposure (a deployment changing its 2. Non-finite cursors — fixed
3. Silent cursor corruption — fixedAn unreadable cursor fell back to "first run" with no log. That is the more expensive failure (full-corpus re-distillation) and was the one case handled silently, against the module's own stated philosophy. It logs now. 4. Stale type hint — fixed
On the test-portability pointYou were right to check this, and the answer was no. Verified green under five timezones: Mutation battery, re-run under
flake8 clean. Accepted without change
|
1 participant
Closes #13948
Thinking Path
_select_pending_sessionscomparedupdated_at <= cursorlexicographically, justified by a comment stating ISO-8601 sorts lexicographically. That holds only at a fixed UTC offset — and the values are naive local time (datetime.fromtimestamp(st_mtime).isoformat(), the #13856 shape).At the autumn DST fallback the local clock repeats an hour. A session written during the repeated hour renders as a string comparing below a cursor already advanced past it, so it is skipped permanently: up to an hour of conversations, once a year, with nothing in the logs. The cursor's whole stated guarantee is "bounded re-work, never a silently skipped conversation" — this broke precisely that.
Decision: emit the raw mtime rather than parse the ISO string back
The obvious fix is to parse
updatedAtinto an epoch at comparison time. That does not work, and the reason is the bug itself: during the repeated hour the naive-local string names two instants. Parsing it resolves to one of them arbitrarily — ambiguous exactly where correctness is needed.So
session_listingnow emitsupdatedAtEpoch(the rawst_mtime) alongside the existing ISO fields, from both producers that feedlist_sessions_fast. The ISO fields are untouched, so no API contract changes and the frontend is unaffected. ISO-string parsing survives only as a fallback for entries from a producer that does not carry the field, and for migrating the stored cursor.Decision: migrate the durable cursor, never reset it
The cursor lives in Redis and the issue explicitly calls out that a reset re-distils the whole corpus at real LLM spend.
_cursor_to_epochreads the current epoch format and falls back to the legacy ISO one, so an existing deployment migrates in place on the first read. A test pins that a legacy cursor still excludes everything before it — migration must not re-distil either.One behaviour change worth flagging
A session with no usable timestamp was silently skipped; it is now skipped with a warning. It genuinely cannot be distilled — the cursor could not advance past it, so it would repeat every run forever — but "loud and skipped" beats "silent and skipped" for the same class of defect this issue is about.
What Changed
chat_history/session_listing.py—updatedAtEpoch: stat.st_mtimeon bothlist_sessions_fastproducers (the stat-based entry and the orphaned-terminal recovery).services/skill_management/skill_distillation_scheduler.py—_entry_epoch/_iso_to_epoch/_cursor_to_epoch; numeric comparison and sort; cursor read/write in epoch seconds; the false monotonicity comment replaced with what is actually guaranteed.Verification
29 new tests. They use a real DST boundary (
Europe/Riga, 2026-10-25, where 03:30 local occurs twice) rather than a synthetic offset, because the defect only exists where a wall clock repeats. One test asserts the premise — that the two instants render as the same string — so the rest cannot pass vacuously.Mutation-tested rather than merely re-run:
updatedAtEpoch, parse the ambiguous stringTrueis anint— would rewind the cursor to 1970)One mutant survives and is equivalent, not a gap: writing the cursor as a raw float instead of
repr(float(...)). redis-py stringifies on the wire either way, so the round-trip is identical; the explicitrepr(float(...))is kept because it also normalises anintinput.Lint: black, isort, flake8 clean.
Note on this issue's history
PR #13949 is merged and its body says
Closes #13948, but it addresses an unrelated CI problem (git environment isolation inco_change_test.py). The reference is a wrong issue number. This issue was still fully unfixed onDev_new_gui— verified atskill_distillation_scheduler.py:401before starting — and would have been closed by anyone sweeping merged PRs.Model Used
Claude Opus 5