Skip to content

bug(core): usage cache validates forever when mtime moves backwards — '>=' should be '===', and the key needs size #426

Description

@edspencer

packages/core/src/state/session-discovery.ts (getSessionUsage, ~line 947 in the built output):

if (cached?.usage && cached.usageMtime && cached.usageMtime >= mtimeStr) {
  return cached.usage;
}
const usage = await extractSessionUsage(filePath);
await this.sessionMetadataStore.setUsage(agentName, sessionId, usage, mtimeStr);

Two defects in the freshness check, in increasing severity.

1. >= should be === — a stale entry can validate permanently

The comparison accepts a cached entry stamped newer than the file it describes. Once that happens the entry is pinned forever: no later mtime can ever be greater, so the cache never expires and extractSessionUsage is never called again. The wrong token totals are served indefinitely, with no error and no expiry.

This is not hypothetical — transcript mtimes move backwards routinely:

  • cp or rsync without -p
  • a restic restore (on the instance I measured, .chats/ is covered by restic snapshots, and restoring is the documented recovery path)
  • copying a data dir to make a clone for testing — the perf clone on that box was made exactly this way

Any of those produce a file older than its cache entry, and from then on the usage numbers are silently frozen at pre-restore values. A restore is precisely the moment you most need correct state.

=== is the correct predicate: the cache describes this exact version of the file, not 'some version no newer than this'.

2. The key should be {mtime, size}, not mtime alone

mtime alone cannot distinguish a file that changed within the filesystem's timestamp granularity, and it is inconsistent with the rest of the codebase — core's own job-index.ts keys on mtime and size, and paddock's JobsDirIndex does too. Only session-metadata and AttributionIndexBuilder rely on mtime alone.

Adding size is also a prerequisite for resumable extraction, which is the direction this cache is heading (transcripts are append-only, so a cache entry of {mtime, size, byteOffset, runningTotals} lets a grown transcript be parsed from byteOffset instead of re-read in full: size > byteOffset → parse the delta, size < byteOffset → full recompute). Size is load-bearing there regardless, so it is worth adding now rather than changing the key twice.

Ask

  • Change >= to ===.
  • Extend the cache key to {mtime, size}; SessionMetadataEntry needs a usageSize (or an equivalent composite stamp) alongside usageMtime.
  • Existing entries lacking the new field should be treated as a per-entry miss that backfills lazily, not as a reason to discard the file.

Ordering

Land #419 first. Until loadMetadata distinguishes absent from unreadable, any schema change here risks the wholesale-file-replacement path and can destroy every customName on the instance. With #419 fixed, this change is safe.

Related: the missing batchSetUsage write-amplification issue (filed separately).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions