Skip stale fs store artifacts#4618
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
132d9cc to
190e458
Compare
|
I've re-reviewed the entire PR diff and surrounding code. My prior review already covered this PR comprehensively. Let me verify the key concern from my prior inline comment. Looking at the actual code at lines 732-744, Some("trash") => {
#[cfg(target_os = "windows")]
{
fs::remove_file(path).ok();
}
true
},This means my prior inline comment at line 752 was incorrect — the Windows No new issues found. Prior inline comment correction: The inline comment I left at |
190e458 to
4a1affd
Compare
|
Please word-wrap your commit messages. |
4a1affd to
1b5475a
Compare
|
I'm confused, do we ever actually write |
|
We're not creating/using tmp directories. The problem is we'd skip over tmp/trash files and then later think they were dirs (because they weren't keys) and try to go into it as if it were a dir |
| { | ||
| // Clean up any trash files lying around. | ||
| if ext == "trash" { | ||
| if p.extension().and_then(|ext| ext.to_str()) == Some("trash") { |
There was a problem hiding this comment.
We've ended up short-circuiting this behavior, should we retain it (what are trash files anyway?)
There was a problem hiding this comment.
According to Claude trash files don't ever show up in windows so not really sure what this is meant for
There was a problem hiding this comment.
Oh looks like we actually do this ourselves because of some windows quirks, i guess we need to keep it
There was a problem hiding this comment.
Then we need to remove the trash files in dir_entry_is_store_artifact - we now no longer call dir_entry_is_key from list_all when !dir_entry_is_store_artifact
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4618 +/- ##
==========================================
+ Coverage 86.42% 86.53% +0.10%
==========================================
Files 158 159 +1
Lines 109324 109860 +536
Branches 109324 109860 +536
==========================================
+ Hits 94484 95063 +579
+ Misses 12300 12267 -33
+ Partials 2540 2530 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1b5475a to
bae8edf
Compare
bae8edf to
2b6e669
Compare
|
Not sure what this actually needs backport to, but we'll figure it out when we try to backport. |
2b6e669 to
82e9dd0
Compare
The exhaustive filesystem store listing treated leftover temp and trash files as namespace directories after identifying them as non-keys. Skip those artifacts before recursing so migrations can ignore crash leftovers.
82e9dd0 to
9246d86
Compare
The exhaustive filesystem store listing treated leftover temp and trash files as namespace directories after identifying them as non-keys. Skip those artifacts before recursing so migrations can ignore crash leftovers.
Caught by codex here: lightningdevkit/ldk-node#872 (comment)