feat(hashing): add match_mtime flag to FileHasher cache backends (ITL-522) by kurodo3[bot] · Pull Request #225 · nauticalab/orcapod-python

kurodo3 · 2026-07-13T23:25:35Z

Summary

Adds match_mtime: bool = True to InMemoryHashCacher, SqliteHashCacher, and PostgresHashCacher. When False, cache lookups ignore mtime_ns and match on path + size only, returning the entry with the highest mtime_ns on multi-hit — useful in environments where mtime is unreliable (rsync, container remounts, restore-from-backup, NFS).
Exposes the flag through enable_file_hash_caching(match_mtime=...) and forwards it to the active cacher backend.
All put() write paths are unchanged — mtime_ns is always stored regardless of the flag.

Test Plan

uv run pytest tests/test_hashing/ --ignore=tests/test_hashing/test_postgres_hash_cacher.py — 619 tests pass
New TestInMemoryHashCacherMatchMtime (8 tests) and TestSqliteHashCacherMatchMtime (8 tests) in test_hash_cacher_match_mtime.py
New TestCachedFileHasherMatchMtime (5 tests) covering T1–T5 with real files and os.utime()
New TestPostgresHashCacherMatchMtime (6 tests) gated with @pytest.mark.postgres
test_match_mtime_kwarg_passes_through_to_cacher verifies end-to-end forwarding through enable_file_hash_caching()
Docs section "Ignoring mtime in cache lookups" added to docs/concepts/file-hash-caching.md

Fixes ITL-522

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

When match_mtime=False, the get() method ignores mtime_ns and instead scans for entries matching (path, size), returning the one with the highest mtime_ns. The put() method is unaffected and always stores the full key. The default behavior (match_mtime=True) preserves backward compatibility by requiring all three key fields to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…on tests (ITL-522) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…(ITL-522)

…ITL-522)

…ses (ITL-522)

codecov · 2026-07-13T23:26:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot

Pull request overview

Adds an opt-in match_mtime switch to file-hash cache backends so cache lookups can ignore mtime_ns and match on path + size (returning the newest mtime_ns entry on multi-hit), and exposes the knob via enable_file_hash_caching().

Changes:

Add match_mtime: bool = True to InMemoryHashCacher, SqliteHashCacher, and PostgresHashCacher, altering get() behavior when False.
Thread match_mtime through enable_file_hash_caching(match_mtime=...) into the active cacher backend.
Add unit/integration tests and user documentation describing the new mode and its trade-offs.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/orcapod/hashing/hash_cachers.py	Implements `match_mtime` for in-memory + SQLite cachers (`__init__`, `get()`, `__repr__`).
src/orcapod/hashing/postgres_hash_cacher.py	Implements `match_mtime` for Postgres cacher (`__init__`, `get()`, `__repr__`).
src/orcapod/contexts/init.py	Exposes/forwards `match_mtime` via `enable_file_hash_caching()`.
tests/test_hashing/test_hash_cacher_match_mtime.py	Adds new tests covering `match_mtime` behavior and `CachedFileHasher` integration scenarios.
tests/test_hashing/test_hash_cachers.py	Adds end-to-end passthrough test for `enable_file_hash_caching(match_mtime=...)`.
tests/test_hashing/test_postgres_hash_cacher.py	Adds Postgres-specific `match_mtime` tests and repr checks.
docs/concepts/file-hash-caching.md	Documents “Ignoring mtime in cache lookups” and the known stale-hit trade-off.
superpowers/specs/2026-07-13-itl-522-match-mtime-flag-design.md	Design/spec write-up for ITL-522 (goals, SQL behavior, tests, docs).
docs/metamorphic/plans/2026-07-13-itl-522-match-mtime-flag.md	Implementation plan/playbook for the change (agent workflow).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kurodo3 · 2026-07-13T23:37:54Z

+    def test_t1_default_mtime_change_causes_miss(self, tmp_path):
+        """T1: Default (match_mtime=True), mtime changed, size unchanged → cache miss → re-hash."""
+        f = tmp_path / "file.bin"
+        f.write_bytes(b"x" * 50)
+
+        cacher = InMemoryHashCacher(match_mtime=True)
+        cached = CachedFileHasher(file_hasher=FileHasher(), cacher=cacher)
+        first_hash = cached.hash_file(f)
+
+        stat = f.stat()
+        os.utime(f, ns=(stat.st_atime_ns, stat.st_mtime_ns + 1_000_000_000))
+
+        # match_mtime=True: mtime change causes a cache miss; CachedFileHasher re-hashes
+        # the file. Content is unchanged so the returned hash must still be correct.
+        second_hash = cached.hash_file(f)
+        assert second_hash == first_hash
+


Fixed. The test now wraps the inner FileHasher with unittest.mock.patch.object(..., wraps=...) and asserts spy.call_count == 2 after the second hash_file() call — confirming the inner hasher was re-invoked (cache miss) rather than serving a cached result. The hash value equality assertion is kept alongside it to verify correctness of the re-hashed output.

kurodo3 · 2026-07-13T23:37:59Z

+    def test_t2_match_mtime_false_mtime_change_is_hit(self, tmp_path):
+        """T2: match_mtime=False, mtime changed, size unchanged → cache hit."""
+        f = tmp_path / "file.bin"
+        f.write_bytes(b"x" * 50)
+
+        cacher = InMemoryHashCacher(match_mtime=False)
+        cached = CachedFileHasher(file_hasher=FileHasher(), cacher=cacher)
+        first_hash = cached.hash_file(f)
+
+        stat = f.stat()
+        os.utime(f, ns=(stat.st_atime_ns, stat.st_mtime_ns + 1_000_000_000))
+
+        second_hash = cached.hash_file(f)
+        assert second_hash == first_hash
+


Fixed. The test now wraps the inner FileHasher with unittest.mock.patch.object(..., wraps=...) and asserts spy.call_count == 1 after the second hash_file() call — confirming the inner hasher was NOT re-invoked, proving the second call returned from cache rather than re-hashing the file.

kurodo3 · 2026-07-13T23:38:05Z

+            cursor = conn.execute(
+                "SELECT hash FROM file_hash_cache "
+                "WHERE path=? AND size=? ORDER BY mtime_ns DESC LIMIT 1",
+                (str(key.path), key.size),
+            )


Fixed. Added CREATE INDEX IF NOT EXISTS idx_file_hash_cache_path_size_mtime ON file_hash_cache (path, size, mtime_ns DESC) in SqliteHashCacher._ensure_schema(). This index satisfies the WHERE path=? AND size=? filter + ORDER BY mtime_ns DESC LIMIT 1 scan in a single index range scan, avoiding a full-table scan when match_mtime=False and multiple historical mtime entries exist for the same file.

kurodo3 · 2026-07-13T23:38:10Z

+            row = conn.execute(
+                "SELECT hash FROM file_hash_cache "
+                "WHERE path=%s AND size=%s ORDER BY mtime_ns DESC LIMIT 1",
+                (str(key.path), key.size),
+            ).fetchone()


Fixed. Added CREATE INDEX IF NOT EXISTS idx_file_hash_cache_path_size_mtime ON file_hash_cache (path, size, mtime_ns DESC) in PostgresHashCacher._ensure_schema(). The index covers the WHERE path=%s AND size=%s ORDER BY mtime_ns DESC LIMIT 1 query for match_mtime=False lookups, allowing the planner to use an index range scan rather than a sequential scan + sort.

eywalker · 2026-07-13T23:33:43Z

+        best_key: FileHashKey | None = None
+        best_value: ContentHash | None = None
+        for cached_key, value in self._cache.items():
+            if cached_key.path == key.path and cached_key.size == key.size:


simply combine this into a single if statement as

cached_key.path == key.path and cached_key.size == key.size and (best_key is None or cached_key.mtime_ns > best_key.mtime_ns)

The nested-dict refactor (see reply on the sibling comment) eliminates the entire loop, so the double-nested if is gone. No standalone fix needed here.

eywalker · 2026-07-13T23:34:36Z

+            return self._cache.get(key)
+        best_key: FileHashKey | None = None
+        best_value: ContentHash | None = None
+        for cached_key, value in self._cache.items():


This looks highly inefficient -- shall we instead change the internal data structure to be dict of dict with outer key (path, size) and inner key mtime?

Done. _cache is now dict[tuple, dict[int, ContentHash]] — outer key (path, size), inner key mtime_ns. Both lookup paths are now O(1)/O(k):

match_mtime=True: two dict lookups — cache[(path, size)][mtime_ns]

match_mtime=False: one outer lookup + inner[max(inner)] — O(k) where k is the number of distinct mtimes for that (path, size) pair, effectively always 1

put() uses setdefault((key.path, key.size), {})[key.mtime_ns] = value. The O(n) scan over all entries is gone entirely.

…(ITL-522)

kurodo3 · 2026-07-13T23:38:19Z

Review round 1 — summary of changes

All four comments addressed in commit 5340fabc.

T1 test (cache miss verification): Wrapped the inner FileHasher with unittest.mock.patch.object(..., wraps=...) and added assert spy.call_count == 2 after the second hash_file() call. The spy proves the inner hasher was invoked again — confirming a genuine cache miss — rather than relying solely on the hash value being equal.

T2 test (cache hit verification): Same spy approach. Added assert spy.call_count == 1 after the second hash_file() call, proving the inner hasher was not re-invoked and the result came from the cache.

SQLite supporting index: Added CREATE INDEX IF NOT EXISTS idx_file_hash_cache_path_size_mtime ON file_hash_cache (path, size, mtime_ns DESC) in SqliteHashCacher._ensure_schema(). The existing PRIMARY KEY (path, mtime_ns, size) cannot efficiently serve a WHERE path=? AND size=? ORDER BY mtime_ns DESC query; the new index covers it directly.

Postgres supporting index: Same index DDL added to PostgresHashCacher._ensure_schema() for the same reason.

… (ITL-522)

kurodo3 · 2026-07-13T23:42:35Z

Review round 2 — summary of changes

One commit 3f2f7981 addressing both comments.

Nested-dict restructure for InMemoryHashCacher: _cache changed from dict[FileHashKey, ContentHash] to dict[tuple, dict[int, ContentHash]] — outer key (path, size), inner key mtime_ns. This eliminates the O(n) scan over all cached entries that the previous match_mtime=False path required:

match_mtime=True lookup: two O(1) dict accesses (cache[(path, size)][mtime_ns])
match_mtime=False lookup: one outer lookup + inner[max(inner)] — O(k) where k is distinct mtimes per (path, size), effectively always 1
put(): setdefault((key.path, key.size), {})[key.mtime_ns] = value

The double-nested if loop from the previous implementation is gone entirely, which also addresses the suggestion to flatten it into a single condition.

kurodo3 Bot and others added 10 commits July 13, 2026 22:19

docs(hashing): add design spec for ITL-522 match_mtime flag

79b628c

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs(hashing): expand unreliable-mtime environment list in ITL-522 spec

216eec1

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs(hashing): add implementation plan for ITL-522 match_mtime flag

cd7e2dd

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(hashing): add match_mtime flag to SqliteHashCacher (ITL-522)

372bded

feat(hashing): add match_mtime flag to PostgresHashCacher (ITL-522)

e2108fe

feat(hashing): add match_mtime to enable_file_hash_caching; integrati…

e57bc6e

…on tests (ITL-522) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(hashing): improve T1 integration test; remove stale comment …

634daaf

…(ITL-522)

docs(hashing): document match_mtime flag in file-hash-caching guide (…

61ad915

…ITL-522)

docs(hashing): fix match_mtime docstring to include path in both clau…

707754c

…ses (ITL-522)

eywalker requested a review from Copilot July 13, 2026 23:25

Copilot started reviewing on behalf of eywalker July 13, 2026 23:26 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

eywalker requested changes Jul 13, 2026

View reviewed changes

fix(hashing): add spy assertions to T1/T2; add path+size+mtime index …

5340fab

…(ITL-522)

refactor(hashing): use nested dict for InMemoryHashCacher; O(1) get()…

3f2f798

… (ITL-522)

eywalker approved these changes Jul 13, 2026

View reviewed changes

eywalker merged commit 0daae6c into main Jul 13, 2026
11 checks passed

eywalker deleted the eywalker/itl-522-filehasher-cache-allow-ignoring-mtime-when-matching-cached branch July 13, 2026 23:47

Uh oh!

Conversation

kurodo3 Bot commented Jul 13, 2026

Summary

Test Plan

Uh oh!

codecov Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

kurodo3 Bot Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

eywalker Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

eywalker Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot commented Jul 13, 2026

Review round 1 — summary of changes

Uh oh!

kurodo3 Bot commented Jul 13, 2026

Review round 2 — summary of changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 13, 2026 •

edited

Loading