Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cashet/redis_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def _encode_commit(commit: Commit) -> bytes:
"tags": commit.tags,
"created_at": commit.created_at.isoformat(),
"claimed_at": commit.claimed_at.isoformat(),
"last_accessed_at": datetime.now(UTC).isoformat(),
"last_accessed_at": commit.claimed_at.isoformat(),
}
return json.dumps(d, separators=(",", ":")).encode()

Expand Down
10 changes: 2 additions & 8 deletions src/cashet/store.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,9 +271,8 @@ def blob_exists(self, hash: str) -> bool:

def put_commit(self, commit: Commit) -> None:
conn = self._connect(immediate=True)
now = datetime.now(UTC).isoformat()
try:
self._put_commit_row(conn, commit, now)
self._put_commit_row(conn, commit, commit.claimed_at.isoformat())
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Medium Possible AttributeError if commit.claimed_at is None

The call commit.claimed_at.isoformat() will raise AttributeError if claimed_at is None. While claimed_at is typically set before put_commit, defensive code should verify it is not None to prevent crashes from malformed Commit objects.

Suggested change
self._put_commit_row(conn, commit, commit.claimed_at.isoformat())
assert commit.claimed_at is not None, "claimed_at must not be None"
self._put_commit_row(conn, commit, commit.claimed_at.isoformat())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 Low Missing documentation or changelog for changed last_accessed_at semantics

The PR changes last_accessed_at to always equal claimed_at, and removes the side-effect update on reads. This is a breaking change for users relying on the old behavior (e.g., for monitoring or custom eviction logic). Add a note to the changelog and update the README or API docs to explain the new semantics.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 Low Potential timezone inconsistency with claimed_at.isoformat()

The call commit.claimed_at.isoformat() may include a timezone offset if claimed_at is timezone-aware with a non-UTC offset. Other timestamps in the store use datetime.now(UTC).isoformat() which always appends +00:00. This could cause comparison issues if the two ISO strings are compared lexicographically. Ensure claimed_at is always in UTC before calling isoformat().

Suggested change
self._put_commit_row(conn, commit, commit.claimed_at.isoformat())
# Normalize to UTC before converting
claimed_utc = commit.claimed_at.astimezone(UTC)
self._put_commit_row(conn, commit, claimed_utc.isoformat())

conn.execute("COMMIT")
except Exception:
conn.execute("ROLLBACK")
Expand Down Expand Up @@ -329,8 +328,7 @@ def _put_commit_row(

def find_by_fingerprint(self, fingerprint: str) -> Commit | None:
conn = self._connect()
now = datetime.now(UTC)
now_iso = now.isoformat()
now_iso = datetime.now(UTC).isoformat()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 Low Dead variable now_iso in find_by_fingerprint

After removing the UPDATE last_accessed_at, the variable now_iso computed on line 332 is no longer used. This wastes a datetime formatting call and may trigger linting warnings. Remove the line.

row = conn.execute(
"""SELECT * FROM commits
WHERE fingerprint = ? AND status IN ('completed', 'cached')
Expand All @@ -341,10 +339,6 @@ def find_by_fingerprint(self, fingerprint: str) -> Commit | None:
).fetchone()
if row is None:
return None
conn.execute(
"UPDATE commits SET last_accessed_at = ? WHERE hash = ?",
(now_iso, row["hash"]),
)
return self._row_to_commit(row)

def find_running_by_fingerprint(self, fingerprint: str) -> Commit | None:
Expand Down
43 changes: 43 additions & 0 deletions tests/test_async_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,49 @@ def do_thing(x: int) -> int:
assert do_thing.__doc__ == "A docstring."
assert hasattr(do_thing, "_cashet_wrapped_func")

async def test_last_accessed_at_derived_from_claimed_at(
self, async_client: AsyncClient
) -> None:
def work() -> int:
return 42

ref = await async_client.submit(work)
assert await ref.load() == 42

conn = async_client.store._core._connect()
row = conn.execute(
"SELECT claimed_at, last_accessed_at FROM commits WHERE hash = ?",
(ref.commit_hash,),
).fetchone()
assert row["claimed_at"] == row["last_accessed_at"]

async def test_cache_hit_does_not_shift_last_accessed_at(
self, async_client: AsyncClient
) -> None:
def work() -> int:
return 42

ref1 = await async_client.submit(work)
assert await ref1.load() == 42

conn = async_client.store._core._connect()
row_before = conn.execute(
"SELECT last_accessed_at FROM commits WHERE hash = ?",
(ref1.commit_hash,),
).fetchone()
la_before = row_before["last_accessed_at"]

ref2 = await async_client.submit(work)
assert await ref2.load() == 42

row_after = conn.execute(
"SELECT last_accessed_at FROM commits WHERE hash = ?",
(ref1.commit_hash,),
).fetchone()
la_after = row_after["last_accessed_at"]

assert la_before == la_after


class TestAsyncContextManager:
async def test_async_with(self) -> None:
Expand Down
39 changes: 39 additions & 0 deletions tests/test_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,45 @@ def make_bytes(n: int) -> bytes:
client.gc(older_than=timedelta(days=1), max_size_bytes=1)
assert client.stats()["total_commits"] == 0

def test_last_accessed_at_derived_from_claimed_at(self, client: Client) -> None:
def work() -> int:
return 42

ref = client.submit(work)
assert ref.load() == 42

conn = client.store._connect()
row = conn.execute(
"SELECT claimed_at, last_accessed_at FROM commits WHERE hash = ?",
(ref.commit_hash,),
).fetchone()
assert row["claimed_at"] == row["last_accessed_at"]

def test_cache_hit_does_not_shift_last_accessed_at(self, client: Client) -> None:
def work() -> int:
return 42

ref1 = client.submit(work)
assert ref1.load() == 42

conn = client.store._connect()
row_before = conn.execute(
"SELECT last_accessed_at FROM commits WHERE hash = ?",
(ref1.commit_hash,),
).fetchone()
la_before = row_before["last_accessed_at"]

ref2 = client.submit(work)
assert ref2.load() == 42

row_after = conn.execute(
"SELECT last_accessed_at FROM commits WHERE hash = ?",
(ref1.commit_hash,),
).fetchone()
la_after = row_after["last_accessed_at"]

assert la_before == la_after


class TestTTL:
def test_ttl_roundtrip(self, client: Client) -> None:
Expand Down
Loading