Repository navigation
Commit 50d9d90
authored
fix(sleep): thread-safe backend cache + redact exports (#251)
* fix(sleep): thread-safe backend cache + redact exports
- Guard CliBackend _cache/_tokens with a lock on the opt-in parallel replay
path (SKILLOPT_SLEEP_WORKERS>1) so a concurrent miss cannot corrupt state
or lose the token cost metric; the model call stays outside the lock so
parallel workers still overlap.
- Redact report.json (redact_secrets) before staging.
- Redact harvest --output and --json exports (_redact_deep).
- Add tests for CliBackend caching/thread-safety.
* fix(sleep): mapping-key-aware redaction at export boundaries
Address maintainer review on #251:
- Use redact_secrets (mapping-key aware) instead of _redact_deep for harvest
--output/--json and handoff exports, so nested api_key/token mappings are
redacted (not just bare string leaves).
- Redact report_md before writing it alongside report.json.
- Add boundary tests (nested api_key/token + report.md).
* fix(sleep): thread-safe cache access through locked helpers
Address maintainer review on #251 (thread-safety):
- Add _cache_get/_cache_pop/_cache_pop_if locked helpers and route the Pi and
OpenCode _cached_call overrides through them (they previously read/pop the
cache outside the lock).
- tokens_used() now reads _tokens under the lock.
- Popping a failed entry is conditional (_cache_pop_if): a failed caller only
drops its own empty value, never another worker's just-stored success.
- Add tests: barrier-forced overlapping misses stay consistent, and pop-if
does not delete a successful entry.
* fix(sleep): call-local token accounting for parallel replay
Address maintainer review on #251 (last thread-safety item):
- Record each model call's token delta on the calling thread (thread-local),
so parallel replay_one() charges its own cost instead of a before/after
global total that an overlapping worker inflates.
- replay_one reads backend.token_delta() (falling back to the text-length
heuristic for backends that don't track tokens).
- Add tests for call-local and thread-isolated token deltas.
* fix(sleep): never cache empty results; add Pi concurrency test
Address maintainer review on #251 (deepen thread-safety):
- _cached_call no longer caches empty (transient-failure) results and prefers a
concurrently cached success, so an empty/duplicate cannot clobber or delete
another worker's successful entry.
- Add a Pi subclass-level concurrency test (barrier-forced empty-vs-success)
asserting the success survives.
* fix(sleep): key-aware redaction + correct concurrent token accounting
- Redefine _redact_deep to delegate to the key-aware redact_secrets walker so
{"api_key": "x"} is scrubbed at every boundary (--json, digests/snapshot
files, gate_trials, extra, display), not just the --json link.
- Reset _thread_local.delta on cache hit so a later hit doesn't reuse the
previous call's delta.
- Charge every real call's tokens on a concurrent miss (the dedup worker used
to be free, undercounting).
- Add DualBackend.token_delta() so replay_one() reads the target's call cost.
- Regressions: cache-hit delta reset, barrier-forced concurrent charge,
DualBackend token_delta, key-aware _redact_deep.
* test(sleep): raise barrier wait timeout to avoid slow-CI flakiness
- The barrier-forced concurrency tests waited 5s for all workers to reach the
barrier; under a slow/loaded CI that can break the barrier mid-test and turn
a pass into a spurious failure. Raise the wait to 15s (no semantic change).
* docs(sleep): clarify DualBackend.token_delta intent
Document that token_delta() is target-only by design (replay drives the target),
that the optimizer only appears in replay via the rare model-judge fallback
(rule/exact/answer tasks are scored locally, 0 tokens), and that the aggregate
tokens_used() still counts both sub-backends so the total is not undercounted.
* fix(sleep): call-local accounting for attempt_with_tools + refresh redaction test
- Set the thread-local delta in every attempt_with_tools override that charged
_tokens directly (Claude CLI, OpenCode, Codex, Cursor), so replay_one() reads
real call-local usage instead of falling back to a response-length estimate.
- Update the stale test_redact_deep_loses_mapping_key_context to assert redaction
(it was asserting the old leak bug).
- Add tool-replay regressions: attempt_with_tools sets call-local delta, and the
dual-backend path surfaces the target's delta.
* refactor(sleep): centralize token accounting in _record_cost
- Add CliBackend._record_cost(prompt, response) as the single path to charge an
inference's token cost (aggregate _tokens under _lock + call-local
_thread_local.delta), so no path under- or over-counts.
- Route _cached_call miss, all attempt_with_tools overrides
(Claude/OpenCode/Codex/Cursor), and reflect through it; the OpenCode error
path keeps its prompt-only charge.
- No behavior change: identical delta model, just centralized — removing the
duplicated len//4 accounting that caused the #251-class bugs to recur.
* fix(sleep): reset call-local delta on no-call paths + one locked accounting helper
Address the review: all token charging now routes through one locked helper
(_record_cost), and the call-local delta is reset on every no-call / early-return
path (_reset_call_delta), so a reused worker never reports a previous call's
token count — covering the OpenCode disabled-tool-replay early return, cache
hits, and the start of every tool-aware path.
Add regressions: barrier-forced concurrent _record_cost (no lost updates) and
same-thread prior-call -> disabled-tool-replay delta reset.
* fix(sleep): record call-local delta on real-usage (Azure/OpenCode) paths
- Add _record_delta(delta) and route the Azure/OpenCode real-usage accounting
through it, so those backends also set the call-local _thread_local.delta —
the last accounting path that did not (replay_one() saw 0/stale and fell back
to a length estimate for these backends).
* fix(sleep): single-owner token accounting, no double charge
A backend that self-reports provider usage (AzureOpenAI/AzureResponses) must not
be charged twice: _cached_call/_reflect now skip the len//4 estimate when the
call already recorded its own exact usage (via a _charged_in_call marker), so a
30-token provider usage is charged once as 30, not as the ~110 estimate.
- AzureOpenAI/AzureResponses _call accumulate usage across every paid attempt and
record _record_delta(total) once; the call-local delta equals that increment.
- OpenCodeCliBackend.attempt_with_tools error path routes prompt-only cost through
_record_delta (no manual _tokens / _thread_local.delta update).
- The _call str return contract is preserved, so llm_miner/rollout/slow_update
keep working and their Azure cost stays accounted.
- Added fake-client regressions for both Azure backends (single call, empty-retry
+ success accumulation, cache-hit no-charge) + OpenCode error-path.
* refactor(sleep): thread-localize the self-charge marker
Move _charged_in_call from a shared instance attribute to _thread_local.charged_in_call
so parallel workers can never read another worker's marker; the marker already gets
reset to False before every _call on the current thread.
* fix(sleep): finalize accumulated usage on every exhausted-retry exit
AzureOpenAI._call recorded usage_total only when last_exc was 'empty-response'; if
the final attempt raised (after a paid empty response), it returned without
_record_delta, so _cached_call substituted its len//4 estimate and dropped the
known paid usage. Now always record the accumulated usage + set the marker on any
exhausted-retry exit (usage_total 0 = genuinely no paid usage). Regression:
empty-paid-response -> terminal-error keeps the exact 7 tokens.
* fix(sleep): backward-compatible call-local accounting for tokens_used()-only backends
replay_one no longer substitutes a text-length estimate when a backend implements
only the older tokens_used() contract (no token_delta): it reports the same-thread
before/after total difference instead, and DualBackend does the same for a target
that lacks token_delta (snapshotted in attempt/attempt_with_tools). CliBackend and
DualBackend-with-modern-target keep the thread-safe per-call delta, so the shared
total is still never reduced by subtraction for concurrent CliBackend calls.
Added existing-backend regressions (direct + DualBackend, tokens_used-only).
* refactor(sleep): drop unused _cache_pop; note DualBackend snapshot not thread-safe
* fix(sleep): report exact zero cost for cached replay results
A replay served from the backend cache has no new model call: _cached_call() resets the per-thread delta to zero and the aggregate ledger is unchanged. replay_one() was still treating any zero as 'backend does not track tokens' and substituting a text-length estimate, so ReplayResult.tokens disagreed with the backend ledger (102 vs 0 in reproduction). With token_delta() present, the returned cost is authoritative - including a known zero cache hit. Only backends without the per-call interface fall back to before/after totals and the length estimate.
* docs(sleep): spell out the parallel-replay accounting contract
The cumulative-only fallback in DualBackend.token_delta() is sequential-only:
`_target_tokens_before` is shared instance state, so two workers overlapping
in one DualBackend overwrite each other's baseline. That is not the same
guarantee as a target that reports a thread-local per-call delta, and it is not
the same as a bare legacy backend (whose before/after snapshot replay_one takes
on the worker's own thread).
replay_batch()'s docstring now lists the four supported combinations, the
DualBackend constructor note and its token_delta() fallback say which branch is
sequential-only, and replay_one()'s comment no longer implies that any
token_delta()-bearing backend is thread-safe.
Tests: a regression for the modern path - replay_batch(workers=4) through a
DualBackend over a target with a thread-local delta must produce exactly the
same per-task costs as the sequential run, with the tasks spread far enough
apart in size that the check is not vacuous.
* docs(sleep): correct the parallel-replay claim for cumulative-only backends
The first version of this contract filed a bare backend with only the older
tokens_used() contract under "safe to share". It is not: replay_one differences
a counter that the other workers are also spending, so an overlapping worker's
tokens land in this task. Two workers against a backend where every attempt
costs 37 report [74, 37].
Only a thread-local per-call delta is safe to share; every cumulative-total
shape over-counts when workers overlap. The docstring now says that and names
the failure direction - budgets see more spend than actually happened - rather
than implying that legacy backends are fine.
The parallel regression did not bite either: with no real overlap, a backend
that had lost its thread-locality still produced costs identical to the
sequential run. The fake target now blocks on a barrier until every worker is
in flight, so the assertion fails when that locality goes away. Verified by
substituting a cumulative delta (test fails) and by five consecutive runs with
the real one (test passes).
* fix(sleep): 区分用量缺失与权威零,避免误记为 0
provider 的 usage 为 None 时旧代码仍把 charged_in_call 置为 True,
_cached_call 因此跳过 len//4 回退,聚合用量与 call-local 增量双双归零,
一次真实调用被记成免费。现在把是否上报用量与上报的数值分开判断:
usage 缺失落到长度估算,上报 0 仍按权威零计费,多轮重试中已知的部分
用量继续保留。两个 backend(chat completions 与 responses)各自补齐
缺失、权威零、部分重试三类回归。
* fix(sleep): keep unknown usage distinct from zero on exhausted Azure retries
A run of empty responses that all omit usage metadata exhausted the retry
budget without any attempt reporting usage; the exit charged usage_total (0)
and set charged_in_call, so unknown usage was recorded as an authoritative
zero. Charge the accumulated provider usage only when at least one attempt
actually reported it; otherwise leave the marker clear so _cached_call's
len//4 estimate stands in for the unknown cost, as it does on main.
Applies to AzureOpenAIBackend (Chat Completions) and AzureResponsesBackend
(Responses); a reported zero still stays an authoritative zero, known partial
retry usage is preserved, and cache hits remain unchanged.
Regressions: exhausted-empty-without-usage and exhausted-empty-with-reported-
zero for both API variants. Also drops an unused local in the OpenCode error
path and tidies the new test files' imports (no behavior change).
---------
Co-authored-by: WODE25500 <[email protected]>
Co-authored-by: WODE25500 <[email protected]>1 parent 343db22 commit 50d9d90
7 files changed
Lines changed: 1053 additions & 43 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
| 47 | + | |
47 | 48 | | |
48 | 49 | | |
49 | 50 | | |
| |||
333 | 334 | | |
334 | 335 | | |
335 | 336 | | |
336 | | - | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
337 | 345 | | |
338 | | - | |
339 | | - | |
340 | | - | |
341 | | - | |
342 | | - | |
343 | | - | |
344 | | - | |
| 346 | + | |
345 | 347 | | |
346 | 348 | | |
347 | 349 | | |
| |||
483 | 485 | | |
484 | 486 | | |
485 | 487 | | |
486 | | - | |
| 488 | + | |
487 | 489 | | |
488 | 490 | | |
489 | 491 | | |
| |||
807 | 809 | | |
808 | 810 | | |
809 | 811 | | |
810 | | - | |
| 812 | + | |
811 | 813 | | |
812 | | - | |
| 814 | + | |
813 | 815 | | |
814 | 816 | | |
815 | 817 | | |
| |||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
36 | 36 | | |
37 | 37 | | |
38 | 38 | | |
39 | | - | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
40 | 44 | | |
41 | 45 | | |
42 | 46 | | |
43 | 47 | | |
44 | 48 | | |
45 | | - | |
46 | | - | |
47 | | - | |
48 | | - | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
49 | 68 | | |
50 | 69 | | |
51 | 70 | | |
| |||
109 | 128 | | |
110 | 129 | | |
111 | 130 | | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
112 | 154 | | |
113 | 155 | | |
114 | 156 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1130 | 1130 | | |
1131 | 1131 | | |
1132 | 1132 | | |
1133 | | - | |
| 1133 | + | |
1134 | 1134 | | |
1135 | 1135 | | |
1136 | 1136 | | |
1137 | 1137 | | |
1138 | 1138 | | |
1139 | | - | |
| 1139 | + | |
1140 | 1140 | | |
1141 | 1141 | | |
1142 | 1142 | | |
| |||
0 commit comments