Repository navigation
Codex: dedupe fork-replayed history by content, not position - #37
Conversation
Fork/spawn child rollouts replay the parent session's history with every copied line's timestamp rewritten to the fork instant, so the positional dedup key (session, timestamp, line index) counted replayed token_count events again — inflating token totals, LIMITS samples, and MODES effort mix (issue #36). - Usage events: keyed on (session, last_token_usage vector, total_token_usage vector). The cumulative is non-decreasing between compactions, so the pair identifies one turn's consumption across every file it appears in; re-emissions with an unchanged cumulative collapse too. Logs without the cumulative fall back to the positional key. - Rate-limit samples: same usage fingerprint plus the snapshot's own fields (used_percent, window_minutes, resets_at) so a verbatim replay collapses while a genuine re-notification survives. - Effort events: keyed by turn_context.payload.turn_id, which replays copy verbatim; positional fallback for logs predating turn_id. - merge_into: keyed duplicates keep the earliest observed timestamp — scan order (archived_sessions sorts before sessions wholesale) can't let a replay shift an event's day attribution. The five dedup loops fold into a shared dedupe_into helper. - CACHE_VERSION 9 -> 10: cached FileEvents carry the old positional keys, so parsing semantics changed even though the layout did not. Known remainder (follow-up): session touches and task durations from replayed lines are still counted. Co-Authored-By: Claude Fable 5 <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request transitions Codex event deduplication (for usage, rate-limit, and effort events) from positional keys to content-based keys. This prevents replayed parent history in fork/spawn child rollouts from being counted multiple times when timestamps are rewritten and line positions are shifted. Additionally, the event merging logic is updated to preserve the earliest observed timestamp, and the cache version is bumped to 10. The review feedback points out a potential bug in total_usage where an explicit JSON null value for total_token_usage would be treated as Some(&Value::Null) rather than falling back to the positional key, which could cause unrelated events to incorrectly collapse under a static fingerprint.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e932675837
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address second-model review of the GH-36 fix: - Gate the semantic-key path on a fingerprintable usage object (total_tokens parses as an integer) — total_token_usage null/{} would fingerprint as all-zero and falsely collide distinct turns. Same gate on the rate-limit key's last_token_usage. - Clamp used_percent before keying so an out-of-range raw value can't mint a second key for the same stored sample value. - Soften the CHANGELOG attribution claim: earliest-timestamp-wins holds when both original and replay are scanned; an out-of-window original leaves the replay's fork-instant timestamp (known remainder). - Pin the review-raised invariants in tests: malformed-cumulative fallback, rate-limit re-notification survival, cross-turn re-emission collapse (why turn_id is not in the usage key), same-timestamp distinct turn_ids, cache_write discrimination, and Claude fragment merge (larger volume + earliest timestamp + merged metadata). Co-Authored-By: Claude Fable 5 <[email protected]>
Layer a parse-time skip under the content-based keys (GH-36) — the approach ccusage took for the same bug (ccusage/ccusage#950) — with a broader trigger: a second session_meta marks a fork-shaped rollout (catches Desktop thread branches that carry no thread_spawn marker), and event lines stamped in the same second as that meta are the replay burst. - Replayed history now contributes nothing even when the parent rollout is outside the scan (deleted or past the mtime window), where cross-file dedup has no original to collapse into and previously the replay was counted once at the fork instant. - Durations (unkeyed) and session touches from replayed lines stop counting, closing two known remainders. - Content-based keys stay as the safety net for burst lines that leak past the fork second, whole-file copies, and re-emissions. Real-data check: same-instant A/B with and without the skip yields byte-identical Codex totals on an all-parents-in-scan corpus — the skip only changes outcomes where originals are missing from the scan. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Correction to the description above: "ccusage took the same approach for this bug" was imprecise — I inferred that from the issue discussion in ccusage/ccusage#950, not from ccusage's code, and the two implementations use different mechanisms. Context: #49. |
Fixes #36. Codex fork/spawn copies parent history into the child rollout with timestamps rewritten to the fork instant, so the positional dedup key
(session, timestamp, line_index)counted replayed events again — inflating token totals, LIMITS, and MODES. Not orchestrator-specific; details and reproduction in #36.Fix
last_token_usagevector,total_token_usagevector). Re-emissions with an unchanged cumulative collapse too. Positional fallback when the cumulative is absent.turn_id, which replays copy verbatim; positional fallback.merge_into: keyed duplicates keep the earliest observed timestamp — file scan order can't shift day attribution.CACHE_VERSION9 → 10 so the fix reaches existing installs.session_meta) additionally skip the replay burst at the fork instant wholesale (ccusage took the same approach for this bug in Bug: Massive token overcounting for Codex subagent sessions (91x inflation) ccusage/ccusage#950; the second-meta trigger also covers Desktop thread branches without athread_spawnmarker). Replayed history contributes nothing — tokens, rate-limit samples, efforts, durations, session touches — even when the parent rollout is outside the scan window. The content keys stay as the safety net for burst lines leaking past the fork second, whole-file copies, and re-emissions.Verification
fmt / clippy (pedantic, zero warnings) / 107 tests incl. new fork fixtures. Real data: 30-day Codex total 2.1B → 930.6M, matching an independent semantic-dedup measurement of the same logs; other provider tabs byte-identical.
Follow-up (out of scope): a
token_countwhose cumulative is unchanged but whose last-usage vector differs is still summed (~0.7% of local volume, semantics ambiguous — see the review thread).🤖 Generated with Claude Code