fix(sleep): thread-safe backend cache + redact exports - #251
Conversation
- 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.
|
Please add boundary-level tests for both harvest output/file and |
Address maintainer review on microsoft#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).
Address maintainer review on microsoft#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.
Address maintainer review on microsoft#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.
Address maintainer review on microsoft#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.
|
Yifan Yang (@Yif-Yang) — thank you for the thorough review and guidance! I've addressed the feedback:
Thanks again for the detailed review! |
|
Yifan Yang (@Yif-Yang) — thank you for the careful review and guidance. In fact I've done almost all of these submissions through DSH, which is exactly why I have a bold idea. Across your recent PR reviews I noticed that you consistently apply a set of quality baselines: fail-closed handling, structure-aware and boundary-consistent redaction, thread-safety with call-local accounting, validating against real contracts, PR hygiene, and so on. I'd like to distill that into a reusable |
|
Thanks — several original races and the
Please add boundary-level stdout coverage for mapping-key secrets, a same-thread miss→hit regression, a barrier-forced same-key concurrent-miss test that checks paid-call accounting, and a |
- 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.
- 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).
|
Already addressed the review feedback and updated this branch (#251):
Also added a comment on |
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.
|
Thanks — the original cache-hit, concurrent-call, boundary-redaction, and |
…daction 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.
|
Thanks for the re-review. Addressed both blockers: the stale est_redact_deep_loses_mapping_key_context now asserts redaction (was asserting the leak), and every �ttempt_with_tools override that charged _tokens now sets the call-local thread delta (Claude CLI, OpenCode, Codex, Cursor), so |
Sleep-cycle hardening.