Repository navigation
Conversation
Every served converted download refreshed the copy's mtime so TTL cleanup could measure demand. nginx builds its ETag from mtime+size, so the ETag changed on every request and If-Range never matched, making converted downloads unresumable. Serve the cached copy without touching it, like the ZIP cache does. TTL cleanup now measures from conversion time instead of last serve. Fixes rommapp#5239
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _serve_cached(final_path: Path) -> Path | None: | ||
| """The cached copy, or None while cleanup or a conversion holds its dir. | ||
|
|
||
| Never touches the copy: its mtime feeds nginx's ETag, and refreshing it | ||
| would break resumable downloads. |
There was a problem hiding this comment.
Removing the mtime refresh also removes the download's protection from cleanup. For a cached copy older than SERVE_GRACE_SECONDS, _serve_cached releases its lock before nginx opens the file. If another conversion runs cleanup_stale_conversions to make room during that gap, cleanup can delete the just-requested copy and the download fails. Keep a separate last-served timestamp or equivalent protection without changing the copy's mtime.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/utils/conversion_cache.py
Line: 142-146
Comment:
**Cleanup can delete requested downloads**
Removing the mtime refresh also removes the download's protection from cleanup. For a cached copy older than `SERVE_GRACE_SECONDS`, `_serve_cached` releases its lock before nginx opens the file. If another conversion runs `cleanup_stale_conversions` to make room during that gap, cleanup can delete the just-requested copy and the download fails. Keep a separate last-served timestamp or equivalent protection without changing the copy's mtime.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| assert await get_or_convert(1, f, "psp", "chd") == final | ||
| assert final.stat().st_mtime > time.time() - 60 | ||
| assert final.stat().st_mtime == before |
There was a problem hiding this comment.
The unit tests now expect unchanged mtime, but test_get_refreshes_a_cached_copy_but_head_does_not still requires GET to refresh it. Its fixture sets mtime one hour back, so all three GET cases fail at line 307 with the new serving behavior. Update that endpoint test's mtime and cleanup expectations too.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/tests/utils/test_conversion_cache.py
Line: 133-134
Comment:
**Endpoint tests require removed behavior**
The unit tests now expect unchanged mtime, but `test_get_refreshes_a_cached_copy_but_head_does_not` still requires GET to refresh it. Its fixture sets mtime one hour back, so all three GET cases fail at line 307 with the new serving behavior. Update that endpoint test's mtime and cleanup expectations too.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| """The cached copy, or None while cleanup or a conversion holds its dir. | ||
|
|
||
| Never touches the copy: its mtime feeds nginx's ETag, and refreshing it | ||
| would break resumable downloads. |
There was a problem hiding this comment.
The new _serve_cached docstring has a second paragraph. The repository's comment rule requires one sentence, followed only by an Args: or Returns: block when needed. Keep the non-obvious nginx mtime requirement in one sentence. This repository requirement must be satisfied before merging.
Rule Used: Comment discipline. A comment is one or two lines; a docstring is one sentence plus an Args:/Returns: block when the signature needs it. Flag: multi-paragraph rationale; change history or migration notes ("replaces the manual pattern", "this was drop... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/utils/conversion_cache.py
Line: 143-146
Comment:
**Docstring breaks the comment rule**
The new `_serve_cached` docstring has a second paragraph. The repository's comment rule requires one sentence, followed only by an `Args:` or `Returns:` block when needed. Keep the non-obvious nginx mtime requirement in one sentence. This repository requirement must be satisfied before merging.
**Rule Used:** Comment discipline. A comment is one or two lines; a docstring is one sentence plus an Args:/Returns: block when the signature needs it. Flag: multi-paragraph rationale; change history or migration notes ("replaces the manual pattern", "this was drop... ([source](https://app.greptile.com/romm/-/custom-context?memory=43fcb307-60fc-4cf1-9be8-07a4b8f33a24))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| expired_before = now - converto.cache_ttl_hours * SECONDS_PER_HOUR | ||
| deleted = 0 | ||
| # (last served, size) of the final copies in each dir that survives the TTL pass. | ||
| # (newest mtime, size) of the final copies in each dir that survives the TTL pass. |
There was a problem hiding this comment.
Cleanup now uses conversion age, but cleanup_stale_conversions still says it evicts the least recently served copies. The conversion guide and configuration reference also describe expiry from the last serve and eviction by last use. Update these descriptions to conversion age so admins know that frequent downloads no longer keep a copy cached.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/utils/conversion_cache.py
Line: 381
Comment:
**Cache descriptions promise old behavior**
Cleanup now uses conversion age, but `cleanup_stale_conversions` still says it evicts the least recently served copies. The [conversion guide](https://github.com/rommapp/docs/blob/HEAD/docs/administration/library-conversion.md#the-conversion-cache) and [configuration reference](https://github.com/rommapp/docs/blob/HEAD/docs/reference/configuration-file.md#convertocache_ttl_hours) also describe expiry from the last serve and eviction by last use. Update these descriptions to conversion age so admins know that frequent downloads no longer keep a copy cached.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Serving a converted copy no longer refreshes the copy's mtime (it feeds nginx's ETag), but that also removed the copy's protection from cleanup: a just-served copy could be evicted before nginx opened it. Record serves in a `.last_served` sidecar instead; cleanup already keys off file mtimes, so the sidecar restores the protection without touching the copy. Addresses Greptile P1 findings on rommapp#5242 (cleanup race, stale endpoint test) and the docstring rule P2.
This branch has not been deployed
1 participant
Description
Converted downloads (
GET /api/roms/{id}/content/{file_name}?format=cso) could not be resumed. Every serve of a cached conversion ranos.utimeon the copy so TTL cleanup could measure demand, but nginx builds its ETag from mtime and size, so the ETag changed on every request andIf-Rangenever matched. A client that lost its connection halfway had to download the whole file again._serve_cached()no longer touches the copy it serves, the same approachzip_cache.pyalready uses. Thetouchparameter is removed from_serve_cached,get_cached_converted, andresolve_format_download. TTL cleanup now measures from conversion time rather than last serve.Checklist
Fixes #5239
Testing
backend/tests/utils/test_conversion_cache.py: 70 passed. Addedtest_serving_leaves_mtime_untouchedand reworkedtest_cache_hit_returns_path_without_touching_mtime; both fail on the old code and pass with the fix. The tests that pinned the old touch-on-serve behavior were removed.The full suite was not run here: it needs MariaDB/PostgreSQL through Docker, which is not available in this environment. CI will cover it.
AI assistance disclosure
Per CONTRIBUTING.md: this PR was written primarily by Muse, an AI coding agent, under the contributor's direction. The agent reproduced the bug, wrote the fix and the tests, and drafted this description.