Fix test‑data extraction cache by judging archive file sha256 (#1844) - #2884
Fix test‑data extraction cache by judging archive file sha256 (#1844)#2884Helen-Mars wants to merge 15 commits into
Conversation
Greptile SummaryThis PR refreshes extracted Git LFS test data when its archive changes. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (14): Last reviewed commit: "Merge branch 'main' into fix/1844-reextr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| extracted_path.rename(backup_path) | ||
|
|
||
| try: | ||
| # Move the new extraction into the final location. | ||
| new_extracted.rename(extracted_path) |
There was a problem hiding this comment.
extracted_path.rename(backup_path) removes the directory path that prior get_data() callers hold before the replacement is published. A concurrent reader that opens or checks a nested file in the interval before new_extracted.rename(extracted_path) completes can receive FileNotFoundError. Staging protects against failed decompression, but these two renames do not preserve a continuously available published path. Coordinate refreshes with readers or publish the completed extraction through an atomic indirection.
There was a problem hiding this comment.
I understand this concurrency problem.
This PR solves two P‑1‑level bugs: avoiding stat() when archive missing and preventing data‑loss during decompression failures.
I plan to implement file‑lock in a follow‑up PR to solve the concurrent‑access issue.
There was a problem hiding this comment.
Thanks for the contribution! I'm proposing an alternative path here to address the freshness check.
- on first extraction add a metadata file into extracted folder root recording the original artifacts md5 sum
- use this checksum for freshness comparison instead of flaky
mtime.
@paul-nechifor what do you think?
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #2884 +/- ##
==========================================
- Coverage 72.35% 71.83% -0.53%
==========================================
Files 1024 1038 +14
Lines 92038 97071 +5033
Branches 8420 9172 +752
==========================================
+ Hits 66598 69731 +3133
- Misses 23168 25003 +1835
- Partials 2272 2337 +65
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 91 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Yeah, I tried to do this initially, (I think using modified time), but then I realised it's not so easy so I created a ticket to do it later. I imagine maintaining our own metadata is more reliable. One of the issues with timestamps is that you don't necessarily want the newest one. For example if you switch back to an old branch, you want the old archive. Using a hash is better, but then you have to maintain this. Is it possible to get the SHA1 hash from git so you don't have to compute the sum yourself (like md5)? But then how do you know if the dir is up to date or not? Do you hash the dir? I haven't really thought everything through... but it looks like you're handling it, so I'll leave it to you. 🙃 Thanks! |
|
@Helen-Mars Hi let me know if you need more context! You can re-request review once you are done addressing the comments. |
I implemented md5 freshness check, would you please review again, I hope to receive your reply. |
|
Yes can you address greptile's comment first? It seems legit. |
I fixed all Greptile AI problems, could you please review again when you are free .😊 |
TomCC7
left a comment
TomCC7
left a comment
There was a problem hiding this comment.
hi @Helen-Mars md5 handling looks fine to me, but the program logic can be improved, can you address?
| extracted_path, | ||
| ) | ||
|
|
||
| # Extract the updated archive into a temporary staging directory so |
There was a problem hiding this comment.
I feel like there's no need to be this defensive here. Artifacts are expected to be working, using old artifacts only hinders the issue. Also this logic is duplicated with the extraction logic below, discarding the defensive logic can make the program lot cleaner.
1. verify if local artifact is stale
2. if no just early return
3. if yes backup the artifact
4. extract the new artifact
There was a problem hiding this comment.
I agree with you. The current logic is a little redundant and I prefer way you gived, which is more simple and easy to maintain. I will update it next version.
There was a problem hiding this comment.
don't think this is resolved as well....
|
|
||
| metadata_path = _get_archive_metadata_path(extracted_path) | ||
|
|
||
| metadata = {"archive_md5": md5_str} |
There was a problem hiding this comment.
there's two way to avoid magic key here IMO.
- define
MD5_KEY="archive_md5"on top of program - or define a pydantic model like
LFSArchiveMetadataand leverage it's json serialization and we just serialize and dump to file during write, and deserialize back to python object during read. https://pydantic.dev/docs/validation/dev/concepts/models/
For extensibility (in case we need more metadata fields) I recommend option 2 here.
There was a problem hiding this comment.
Yeah, using a pydantic model is better for safety and extension. I will optimize it next commit.
There was a problem hiding this comment.
don't think this is addressed....
|
also would be good if you can fix ci btw |
I will try to do it.😊 |
I make sense, this exception is redundant. Here archive file exists. Co-authored-by: cc <55869557+TomCC7@users.noreply.github.com>
…up tests - Replace mtime with MD5 checksum for staleness detection - Use LFSArchiveMetadata (Pydantic) for read/write validation - Remove redundant FileNotFoundError/NotADirectoryError catches - Remove staging and rollback logic for simpler refresh flow - Add nested path existence check before return - Update tests for all scenarios (missing, up-to-date, updated, nested, single-file)
I finished corresponding fix and update including extra CI bug following your advice, looking forward to your reply.😊 |
TomCC7
left a comment
TomCC7
left a comment
There was a problem hiding this comment.
@Helen-Mars are you sure you've addressed the comments? Did you forgot to push or something?
Let me check again, are you available in discord? could I talk about something fixing problem in discord. I think it is more direct. |
Yes can just dm me. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
I am sorry I didn't update local to remote, I updated it just now. Hope you review again and receive your reply. |
| extracted_path, | ||
| backup_path, | ||
| ) | ||
| extracted_path.rename(backup_path) |
There was a problem hiding this comment.
_pull_lfs_archive(), _decompress_archive(), and metadata writing have succeeded. If the updated archive cannot be pulled, is corrupt, or extraction fails due to an I/O error, the prior usable data remains only at the UUID backup path and later get_data() calls cannot return the cache. Extract and validate a replacement in staging, then publish it only after success; retain or restore the existing path on failure.
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!
There was a problem hiding this comment.
Most of the time we should assure that LFS file is valid and Can be compressed successfully. When some mistake occurs due to an IO error caused from coding, Throwing an exception is more valid for us to recheck and fix our code. We backup user's file just give them chance to get their files Rather than do too much Over defensive coding.
There was a problem hiding this comment.
That's a fair point — for a developer tooling context working with LFS archives, fail-fast exceptions are more appropriate than silent recovery. If the archive is corrupt or extraction fails, surfacing that loudly helps catch bugs early, and the backup serves its purpose as a user data safety net rather than a transparent recovery mechanism. The original comment over-applied a resilience pattern that fits end-user services better than internal dev tooling.
Shall I remember this as a rule: "Prefer fail-fast exceptions over silent recovery in developer tooling utilities; backups are for user data safety, not transparent state restoration"?
Helen-Mars
changed the title
TomCC7
added
the
first-time-contributor
|
@Helen-Mars Hi thanks for the effort! I have no more comment rn. However we are discussing a different model internally and that might reduce the logic a lot. I'll update here later. |
ok |
3 participants
Problem
Test data archives managed through Git LFS were not re-extracted when the archive was updated after the initial extraction.
The existing logic returned the already extracted path whenever it existed, which could leave stale test data in place after a newer archive was downloaded.
Closes #1844
Solution
Compare the sha256 checksum of the LFS archive with the checksum recorded in the metadata file within the extracted directory for freshness check.
Remove and re-extract the existing directory or file when the archive is newer.
Return the existing extracted path when the archive has not changed.
Write the current archive's sha256 checksum into a metadata file inside the extracted root directory after extraction to avoid repeated re-extraction.
Add tests covering:
How to Test
Or run all related tests:
Contributor License Agreement