-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: derive last_accessed_at from claimed_at to unify timestamp tracking #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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()) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
|
||||||
| conn.execute("COMMIT") | ||||||
| except Exception: | ||||||
| conn.execute("ROLLBACK") | ||||||
|
|
@@ -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() | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| row = conn.execute( | ||||||
| """SELECT * FROM commits | ||||||
| WHERE fingerprint = ? AND status IN ('completed', 'cached') | ||||||
|
|
@@ -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: | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
| 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: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.