fix(sleep): thread-safe backend cache + redact exports - #251

report.json is now correctly passed through the mapping-aware redactor, but the export-redaction and thread-safety fixes are still incomplete in three places.

1. Harvest output uses _redact_deep(payload), while _redact_deep() only recurses into values and loses the mapping-key context. As a result, _redact_deep({"api_key": "plain-secret", "nested": {"token": "other"}}) returns both secrets unchanged. Please use the existing mapping-key-aware redact_secrets(payload) at every structured output boundary.
2. write_staging() redacts report.json but writes caller-provided report_md verbatim. Edit content/rationale can therefore still expose credentials. Please redact the Markdown before writing it as well.
3. Not all cache/token access uses the new lock: the Pi and OpenCode overrides still inspect and pop _cache directly, and tokens_used() reads _tokens without the lock. More importantly, parallel replay_one() attributes one call's tokens from a shared global before/after total, so overlapping workers can charge another worker's tokens to the wrong result. Please route cache/token access through locked helpers, prevent a failed caller from deleting another caller's successful cache entry, and use call-local accounting for per-result tokens.

Please add boundary-level tests for both harvest output/file and report.md, including nested api_key/token mappings. The concurrency tests should use barriers/events to force overlapping misses and assert exact call/cache/token outcomes, including a concurrent Pi/OpenCode empty-result versus successful-result case. The current immediate echo test does not guarantee overlap and can pass without exercising these races.

Yifan Yang (@Yif-Yang) — thank you for the thorough review and guidance! I've addressed the feedback:

• Structured outputs now use the mapping-key-aware redact_secrets (not _redact_deep, which loses the key context); report_md is also redacted before writing.
• Thread-safety hardened: cache/token access goes through locked helpers (_cache_get/_cache_pop/_cache_pop_if); tokens_used() is locked; a failed caller only conditionally pops its own empty value (_cache_pop_if), never another worker's success; the base no longer caches empty (transient-failure) results.
• Switched to call-local token accounting (thread-local delta) so parallel replay no longer misattributes tokens through a global before/after total.
• Added tests: nested api_key/token redaction, report.md, barrier-forced concurrency, pop-if not deleting a success, Pi subclass empty-vs-success, and thread-isolated token deltas — all pass.

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 review-standards: a general quality standard plus a pre-PR self-check CLI that contributors run before submitting, so they spend less time on rework and you receive fewer low-quality PRs — a win-win. I'd credit you as the original source of these standards. Looking forward to hearing from you whenever you get a chance.

Thanks — several original races and the report.md boundary are fixed, but four correctness gaps remain on this head.

1. cmd_harvest --json still calls _redact_deep(payload). That function recurses into values and loses mapping-key context, so {"api_key":"top-secret"} is still emitted unchanged on JSON stdout. Please use the mapping-aware redact_secrets(payload) at this final boundary too.
2. A cache hit returns before resetting self._thread_local.delta. Reusing a worker thread after a real call therefore makes a later cache hit report the previous call's token delta.
3. Concurrent misses for the same key both make paid model calls, but when one worker finds the other's cached success it sets its own delta = 0 and does not add that real call to _tokens. The cache contents are safe, but actual usage is undercounted unless in-flight calls are coalesced or every real call is charged.
4. DualBackend has no token_delta(). Consequently replay_one() falls back to a response-length estimate and loses the target backend's real call-local cost.

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 DualBackend replay accounting test. These are the remaining blockers; the other changes from the previous review look addressed.

Already addressed the review feedback and updated this branch (#251):

• Commits 37fe5b6 / d555992: _redact_deep now delegates to the key-aware redact_secrets walker (fixing every output boundary: --json, digests/snapshot files, gate_trials, extra, display); cache hits reset the thread-local delta; every real call on a concurrent miss is charged; DualBackend gained token_delta(); barrier tests added (with a longer timeout to avoid slow-CI flakiness).
  Please re-review, thanks.

Also added a comment on DualBackend.token_delta() documenting that target-only is intentional: replay drives the target, and the optimizer only appears via the rare model-judge fallback (rule/exact/answer tasks are scored locally); the aggregate tokens_used() still counts both, so the total is not undercounted.

Thanks — the original cache-hit, concurrent-call, boundary-redaction, and DualBackend changes are mostly addressed. Two blockers remain. First, the branch’s own full suite fails at tests/test_export_redaction.py::test_redact_deep_loses_mapping_key_context; that stale regression still asserts that the secret leaks and must be updated to assert redaction. Second, real tool-aware replay overrides update _tokens directly but do not set the new thread-local delta, so replay_one() receives zero or stale call-local usage and falls back to a response-length estimate. Please make call-local accounting cover attempt_with_tools() as well and add a tool-replay regression, including the dual-backend path.

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
eplay_one() sees real call-local usage. Added tool-replay regressions incl. the dual-backend path. Commit 3c6f95.

Thanks — the tool-aware paths now set a call-local delta, but two accounting issues remain. OpenCodeCliBackend.attempt_with_tools() returns early when tool replay is disabled without resetting the thread-local delta, so a reused worker can report the previous call token count. Also, the Claude, OpenCode, Codex, and Cursor tool-aware paths still update _tokens directly outside _lock, unlike _cached_call(). Please route all token charging and per-call delta updates through one locked helper, reset the delta on every no-call/early-return path, and add a same-thread prior-call → disabled-tool-replay regression plus barrier-forced concurrent tool-call accounting coverage.

Thanks to Teacher Yifan for the careful review, please wait a moment.

Thanks for the re-review - both accounting issues are resolved (commits 3ef661, c8ff7f8, 8a17aa6):

• All token charging now goes through one locked helper _record_cost (aggregate _tokens under _lock + call-local _thread_local.delta), so no path updates _tokens outside the lock.
• The call-local delta is reset on every no-call / early-return path (_reset_call_delta), covering the OpenCode disabled-tool-replay early return and cache hits - a reused worker can no longer report the previous call's cost.
• The Azure/OpenCode real-usage paths also record call-local delta via _record_delta.
• Added regressions: barrier-forced concurrent _record_cost (no lost updates) and same-thread prior-call ? disabled-tool-replay delta reset.

Thanks — the previous early-return and locking issues are fixed, but one accounting blocker remains on 8a17aa6.

CliBackend._cached_call() in skillopt_sleep/backend.py always calls _record_cost(prompt, out) after _call() (lines 393–395). However, AzureOpenAIBackend._call() already records provider usage through _record_delta() (lines 2476–2483), and AzureResponsesBackend._call() does the same (lines 2584–2595). Each successful Azure call is therefore charged twice, and the exact call-local usage is overwritten by the length estimate.

Minimal reproduction with a fake chat-completions response reporting 10 prompt + 20 completion tokens, a 400-character prompt, and a 40-character response:

provider_usage=30
tokens_used=140
token_delta=110

A two-attempt reproduction (7 tokens on an empty response, then 11 on success) similarly reports:

provider_usage=18
tokens_used=38
token_delta=20

Thus both aggregate cost and ReplayResult.tokens are incorrect. The newly added focused suites still pass (19 passed), because they only exercise length-estimated CliBackend implementations.

Please establish one owner for accounting: preferably have _call() return its provider-reported usage and let _cached_call() record it once, falling back to the length estimate only when usage is unavailable. Retry usage should be accumulated across every paid attempt, and the final call-local delta should equal that same accumulated increment.

Please add fake-client regressions for both AzureOpenAIBackend and AzureResponsesBackend, covering a single successful call, an empty-response retry followed by success, and a cache hit (delta == 0, aggregate unchanged).

Also, OpenCodeCliBackend.attempt_with_tools() still updates _tokens and _thread_local.delta manually on its error path (lines 1440–1445). Please route that path through _record_delta() as well and cover it with an error-path regression.

Yifan Yang (@Yif-Yang) — addressed the remaining accounting blocker on ef7b806.

Summary:

• Single owner, no double charge: a backend that self-reports provider usage (AzureOpenAI / AzureResponses) records its exact, accumulated usage once inside _call and flips a per-thread marker; _cached_call and _reflect then skip their len//4 estimate. Previously the estimate ran on top of the provider usage, so a 30-token usage was charged as ~110 and the exact call-local delta was overwritten.
• Retry accumulation: Azure _call sums usage across every paid attempt; the final call-local delta equals that accumulated increment (e.g. empty-then-success → 7 + 30 = 37).
• Cache hit: no charge, delta reset to 0, aggregate unchanged.
• OpenCode error path: attempt_with_tools now routes the prompt-only cost through _record_delta (no more manual _tokens / _thread_local.delta update).
• Thread-safety: the marker is thread-local (_thread_local.charged_in_call), so parallel workers never read another worker's marker.

Added fake-client regressions (tests/test_azure_usage_accounting.py) for AzureOpenAI and AzureResponses covering a single successful call, an empty-response retry followed by success, and a cache hit, plus an OpenCode error-path regression — 7/7 pass; the full suite is green (1372 passed).

Thanks for the single-owner accounting fix. Re-reviewing ef7b80622b71, the successful retry cases are fixed, but a paid empty response followed by exhausted exception retries still loses the exact provider usage.

Offline fake-client reproduction through _cached_call():

prompt: 400 characters
attempt 1: empty completion, provider usage = 7 prompt + 0 completion tokens
attempts 2-5: simulated exceptions
response: ""
expected aggregate and call-local usage: 7
actual tokens_used(): 100
actual token_delta(): 100

AzureOpenAIBackend._call() accumulates the 7 tokens in usage_total, but only records the total on success or when the final last_exc is "empty-response". If the last attempt raises, it returns without _record_delta() or the accounting marker. _cached_call() then substitutes its 100-token length estimate, dropping the already known paid usage.

Please finalize the accumulated usage on every exhausted-retry exit (as the Responses path already does), while distinguishing genuinely unavailable usage from known zero/known partial usage. Add an empty-paid-response -> terminal-error regression, including the cache/aggregate delta checks. The full shipped suite passes (1521 passed, 9 skipped); this added negative-path reproduction fails.

Yifan Yang (@Yif-Yang) — fixed on 06e5f16. AzureOpenAIBackend._call() now finalizes the accumulated provider usage on every exhausted-retry exit, not only when the final attempt was an empty response: a paid empty response followed by terminal-exception retries keeps the exact usage, and _cached_call() no longer substitutes its length estimate. Added a regression (empty-paid -> terminal-error) asserting aggregate and call-local delta both equal the paid usage. 24 azure-accounting tests pass.

Thanks for 06e5f166a624. The paid-empty-response -> exhausted-error-retry issue from my previous comment is fixed: the independent regression now passes. The full suite on a test merge with main at 79124b37e9a6 also passes (1526 passed, 9 skipped).

There is still a separate backward-compatibility regression in replay_one() that needs fixing before merge. Backend exposes tokens_used() but does not define token_delta(). This PR stops reading the existing total before/after an attempt and instead treats a missing token_delta() as zero, then substitutes a text-length estimate. DualBackend.token_delta() does the same when its target has only the existing interface.

I reproduced this offline with a minimal Backend subclass implementing the existing contract:

initial tokens_used(): 100
each attempt: add 37 to tokens_used(), return "done"
task intent: "done"; empty skill and memory
expected ReplayResult.tokens per attempt: 37
current main: 37
this head: 2 (the length estimate)

The independent matrix covers direct/wrapped (DualBackend) use and plain/tool-aware replay: 4 passed on current main, 4 failed on this head. This is not limited to hypothetical interface shape: the in-tree OpenClawDeepSeekBackend also implements tokens_used() without token_delta(); I am not claiming a live OpenClaw run was performed.

Please introduce the call-local accounting contract compatibly, migrate supported implementations/wrappers or provide an appropriate compatibility path, and add these existing-backend regressions alongside the parallel-replay tests. Do not simply restore unconditional shared-total subtraction for concurrent CliBackend calls, since that would reintroduce the race this PR is meant to solve. Accurate reported per-attempt cost must not silently become a different estimate just because an existing backend lacks the new optional method.

Yifan Yang (@Yif-Yang) — fixed on e869f55. Backward-compatible call-local accounting:

• replay_one() now uses the backend's per-call token_delta() when present (CliBackend / DualBackend-with-modern-target keep the thread-safe per-call delta), and for a backend that only implements the older tokens_used() contract it reports the same-thread before/after total difference instead of substituting a text-length estimate.
• DualBackend.token_delta() does the same for a target that lacks the method (the target total is snapshotted in attempt/attempt_with_tools). CliBackend's shared total is never reduced by subtraction for concurrent calls, so the parallel race this PR fixed is not reintroduced.
• Added existing-backend regressions (direct tokens_used()-only + DualBackend-wrapping-one) asserting the per-attempt cost is the real 37, not the length estimate.

Yifan Yang (@Yif-Yang) Follow-up to e869f55: a self-review of replay_one() found one more spot where per-attempt cost disagreed with the backend ledger. A cache hit resets token_delta() to 0 and leaves tokens_used() unchanged, but replay_one() still treated any zero as "backend does not track tokens" and substituted a text-length estimate (repro: first replay 190, cached second replay 102).

With token_delta() present, the returned cost is now authoritative -- including a known zero for a cache hit; only backends without the per-call interface fall back to before/after totals and the length estimate. Added a replay_one()-level regression asserting the cached replay reports 0 and does not invoke the model again. Related suites: 30 passed locally. Commit f2282ab. CI on the new head is awaiting maintainer approval.

Re-reviewed f2282abf5d0d. The two accounting issues from my earlier comments are now covered by passing independent checks: paid usage survives exhausted retries, and direct/DualBackend legacy tokens_used() implementations retain the reported per-attempt cost rather than the length estimate. The new cache-hit regression also correctly distinguishes a known zero cost from missing accounting.

The cache/accounting/export selection, including the independent regressions, passes 35 tests on Linux/Python 3.11. I am not continuing to treat those earlier reproductions as unfixed.

One useful follow-up is to clarify the concurrency contract for legacy cumulative-only targets. The new inline note about a shared DualBackend snapshot should not be read as the same guarantee as a modern target's call-local delta, and replay_batch() normally shares a backend across workers. Please document the supported combinations clearly and retain a regression for the modern DualBackend parallel path; do not describe cumulative-only fallback accounting as universally thread-safe merely because the sequential compatibility cases pass. This is a scope/coverage clarification, not a newly reproduced concurrency failure in this test slice.

Official CI for this exact head is still awaiting maintainer approval. The local results resolve the cited accounting regressions, not the final full-review/CI merge gate.

Yifan Yang (@Yif-Yang) — addressed on d2fc598; the branch now also merges main at 79124b37e9a6 (efd29a8), which is the base the numbers below are from.

Documentation of the supported combinations:

• replay_batch()'s docstring now lists all four cases and which may be shared across workers: CliBackend (thread-local per-call delta); DualBackend over a target that defines token_delta() (delegates to that target's thread-local delta); a bare backend with only tokens_used() (replay_one snapshots the total on the worker's own thread, so its difference is call-local); and DualBackend over a tokens_used()-only target, which is not safe to share because _target_tokens_before is shared instance state — give each worker its own DualBackend or keep workers=1.
• DualBackend.__init__ and the token_delta() fallback say the same thing where the snapshot is actually taken, and replay_one()'s comment no longer reads as "anything exposing token_delta() is thread-safe".
• Your point is stated explicitly rather than implied: the legacy fallback is call-local in replay_one but not inside DualBackend, so "legacy backends are fine" does not generalize to a legacy target behind a DualBackend. Sequential support is claimed; the concurrency guarantee is not.

Regression for the modern DualBackend parallel path:

• test_dual_backend_modern_target_parallel_replay_is_call_local runs replay_batch(workers=4) through a DualBackend over a target with a thread-local delta and requires each task's cost to equal the sequential run exactly. Task sizes are spread 25 characters apart, so the check cannot pass vacuously if the per-task costs collapse.

Verification on Linux / Python 3.11, on the merge with main (scratch branch, deleted afterwards; the workflow file is not part of this PR):

• focused (cache / accounting / pi backends): 49 passed
• the parallel regression alone, five consecutive runs: 1 passed each time
• full suite: 1527 passed, 12 skipped
• run: https://github.com/WODE25500/SkillOpt/actions/runs/34728208914

Nothing here claims thread-safety for the cumulative-only fallback, and no test asserts it.

Yifan Yang (@Yif-Yang) — correcting myself on e02f7d3. My previous message filed a bare backend with only tokens_used() under "safe to share across workers". That is wrong, and it is the same over-claim you warned about, just in a different row of the table.

replay_one differences a counter the other workers are also spending, so an overlapping worker's tokens land in this task's difference. Measured on Linux, two workers, against a backend where every attempt really costs 37:

legacy backend, per-attempt cost 37: sequential=[37, 37] parallel=[74, 37]

So the contract now reads: only a thread-local per-call delta is safe to share — CliBackend, or a DualBackend whose target has one. Both cumulative-total shapes, the bare legacy backend and the DualBackend fallback, are sequential-only, and they over-count rather than under-count, so token and cost budgets see more spend than actually happened. replay_batch() says that, and replay_one()'s comment no longer implies that a same-thread difference is call-local on its own.

The regression was weaker than I claimed as well. It compared a parallel run against a sequential one, but with no real overlap a backend that had lost its thread-locality still produced costs identical to sequential, so it would not have caught the regression it was written for. The fake target now blocks on a barrier until every worker is in flight. Verified both ways: substituting a cumulative delta fails the test ({'t3': 610} != {'t3': 821}), and the real one passes five consecutive runs.

Re-verified on Linux / Python 3.11 (scratch branch, deleted afterwards; workflow file not part of this PR):

• focused (cache / accounting / pi backends): 49 passed
• the parallel regression alone, five consecutive runs: 1 passed each time
• over-count reproduced on that host, not just locally
• full suite: 1527 passed, 12 skipped
• run: https://github.com/WODE25500/SkillOpt/actions/runs/34728413945

If you would rather have the guard than the documentation — replay_batch() refusing or warning when workers > 1 and the backend has no call-local delta — say so and I will add it. I kept this to the scope you set as a coverage clarification.

Thank you for working through the concurrent accounting and export-hardening feedback.

Re-reviewed e02f7d3cc52a. The shipped cache/accounting/export selection passes 161 tests (1 optional skip), and the complete suite passes 1,526 tests (13 skips) locally. The corrected concurrency documentation and forced-overlap modern DualBackend regression are useful.

The remaining unknown-versus-zero usage case still needs a fix. Both AzureOpenAIBackend._call() and AzureResponsesBackend._call() set charged_in_call=True even when a successful SDK response has usage=None. _cached_call() therefore skips its fallback, and both aggregate and call-local usage become zero.

Independent offline reproduction using the SDK's own ChatCompletion and Response objects:

prompt length: 400; successful response length: 40; usage: None
main 79124b37e9a6: tokens_used = 110 (length fallback)
this head:        tokens_used = 0, token_delta = 0

Both SDK usage fields are optional. The regression passes twice on main and fails twice on this head; these are fake-client calls, with no provider access.

Please track whether usage is actually available separately from its numeric value. Preserve authoritative zero for a reported zero/cache hit, preserve known partial retry usage, and retain estimated accounting for successful attempts without usage metadata. Add both API variants to the regression suite. Upstream CI is still awaiting approval; the passing shipped suite does not cover this case.