refactor: derive last_accessed_at from claimed_at to unify timestamp tracking

[jolovicdev]

Summary

Unifies last_accessed_at with claimed_at across SQLite and Redis backends, eliminating a redundant timestamp and the side-effect write in find_by_fingerprint.

Problem

The SQLite store tracked two independent timestamps:

• claimed_at — set when a worker acquired the claim
• last_accessed_at — updated on every find_by_fingerprint read

This caused two issues:

1. Write contention on reads — find_by_fingerprint did an UPDATE commits SET last_accessed_at = ?, turning every cache lookup into a write.
2. Drift — put_commit passed a fresh now for last_accessed_at, which could diverge from claimed_at if the commit row was updated by a heartbeat or status change elsewhere.

Changes

• src/cashet/store.py
  • put_commit now derives last_accessed_at from commit.claimed_at.isoformat() instead of a separate now.
  • find_by_fingerprint is now side-effect-free; the UPDATE last_accessed_at has been removed.
• src/cashet/redis_store.py
  • _encode_commit uses commit.claimed_at for last_accessed_at to keep both backends consistent.
• tests/test_store.py & tests/test_async_client.py
  • test_last_accessed_at_derived_from_claimed_at — verifies the invariant after task completion.
  • test_cache_hit_does_not_shift_last_accessed_at — verifies that repeated cache hits no longer move the access timestamp.

Why this is safe

Callers that need to "touch" a commit (cache-hit promotion, heartbeat) already call put_commit, which writes the row with the current claimed_at. Removing the implicit UPDATE inside find_by_fingerprint just makes the behavior explicit and idempotent.

Testing

All 298 tests pass (45 skipped for Redis).

[ds-review]

Automated PR review encountered an error and could not complete.

[jolovicdev]

@ds-review review

[jolovicdev]

Recreating to fix GitHub UI visibility.

[ds-review]

PR Review

PR: refactor: derive last_accessed_at from claimed_at to unify timestamp tracking

Important

Verdict: Request changes - 4 actionable findings, highest severity P2.

Findings (4)

<img alt="P2 Medium" src="https://img.shields.io/badge/P2-Medium-ca8a04?style=flat-square" height="20" align="absmiddle"

src/cashet/redis_store.py:123

Redis find_by_fingerprint still touches access time; backfill uses stale claimed_at

The PR removes the side-effect UPDATE in SQLite's find_by_fingerprint, but the Redis variant still calls _touch_commit which updates the access sorted set with current time, violating the intended backend consistency. Moreover, the backfill function _backfill_access_index reads last_accessed_at from the JSON, now set to claimed_at (which may be older than the original access score). During a backfill, it overwrites the access sorted set entry with this older timestamp, causing LRU eviction to consider still-active entries as old.

** ****

src/cashet/store.py:275

put_commit no longer advances last_accessed_at and may fail on None claimed_at

put_commit now uses commit.claimed_at.isoformat() instead of current time, so callers relying on put_commit for cache-hit promotion or heartbeats will not refresh the access timestamp, potentially causing premature eviction. Additionally, if commit.claimed_at is None (e.g., unclaimed commit), this will raise AttributeError. Previously, the timestamp was computed independently.

** De

src/cashet/store.py:331

Dead code: now_iso assignment no longer used

The assignment to now_iso is no longer used because the subsequent UPDATE statement was removed. This wastes a system call and can mislead maintainers.

** Te

tests/test_store.py:1109

Tests use private _connect() to check timestamps

The new tests use client.store._connect() to directly query the database. This depends on internal implementation details (private method _connect) and may break if the store connection mechanism changes. Using a public API to retrieve the commit and check timestamps would be more robust.

How To Recheck

Reply @ds-review recheck under the relevant inline finding after pushing a fix.

[ds-review]

put_commit no longer advances last_accessed_at and may fail on None claimed_at

put_commit now uses commit.claimed_at.isoformat() instead of current time, so callers relying on put_commit for cache-hit promotion or heartbeats will not refresh the access timestamp, potentially causing premature eviction. Additionally, if commit.claimed_at is None (e.g., unclaimed commit), this will raise AttributeError. Previously, the timestamp was computed independently.

Suggested change

	self._put_commit_row(conn, commit, commit.claimed_at.isoformat())
	self._put_commit_row(conn, commit, (commit.claimed_at or datetime.now(UTC)).isoformat())

[ds-review]

Redis find_by_fingerprint still touches access time; backfill uses stale claimed_at

The PR removes the side-effect UPDATE in SQLite's find_by_fingerprint, but the Redis variant still calls _touch_commit which updates the access sorted set with current time, violating the intended backend consistency. Moreover, the backfill function _backfill_access_index reads last_accessed_at from the JSON, now set to claimed_at (which may be older than the original access score). During a backfill, it overwrites the access sorted set entry with this older timestamp, causing LRU eviction to consider still-active entries as old.

[ds-review]

Dead code: now_iso assignment no longer used

The assignment to now_iso is no longer used because the subsequent UPDATE statement was removed. This wastes a system call and can mislead maintainers.

[ds-review]

Tests use private _connect() to check timestamps

The new tests use client.store._connect() to directly query the database. This depends on internal implementation details (private method _connect) and may break if the store connection mechanism changes. Using a public API to retrieve the commit and check timestamps would be more robust.