Skip to content

Codex: dedupe fork-replayed history by content, not position - #37

Merged
miiiiiiich merged 3 commits into
mainfrom
fix/codex-fork-replay-dedup
Jul 21, 2026
Merged

miiiiiiich merged 3 commits into
mainfrom
fix/codex-fork-replay-dedup

Conversation

@miiiiiiich

@miiiiiiich miiiiiiich commented Jul 21, 2026 •

Copy link
Copy Markdown
Owner

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

  • Usage events: keyed on (session, last_token_usage vector, total_token_usage vector). Re-emissions with an unchanged cumulative collapse too. Positional fallback when the cumulative is absent.
  • Rate-limit samples: same fingerprint plus the snapshot's own fields, so replays collapse but genuine re-notifications survive.
  • Effort events: keyed by 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_VERSION 9 → 10 so the fix reaches existing installs.
  • Fork-shaped rollouts (second 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 a thread_spawn marker). 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_count whose 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

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]>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/collector/codex.rs Resolved

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/collector/codex.rs Resolved
Comment thread src/collector/codex.rs Resolved
Comment thread src/collector/mod.rs Resolved
miiiiiiich and others added 2 commits July 21, 2026 17:43
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]>
@miiiiiiich
miiiiiiich merged commit 89d6a03 into main Jul 21, 2026
12 checks passed
@miiiiiiich miiiiiiich mentioned this pull request Jul 21, 2026
@miiiiiiich
miiiiiiich deleted the fix/codex-fork-replay-dedup branch July 28, 2026 14:38
@miiiiiiich

Copy link
Copy Markdown
Owner Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parent session history in forked Codex logs is double-counted

1 participant