chore: safe copy mechanism #480
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
cc @avivkeller / @aduh95 / @flakey5 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #480 +/- ##
==========================================
- Coverage 74.01% 73.86% -0.16%
==========================================
Files 108 109 +1
Lines 10316 10356 +40
Branches 685 686 +1
==========================================
+ Hits 7635 7649 +14
- Misses 2678 2704 +26
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ovflowd
added
the
fast track
|
Requesting fast-track 🙇 |
|
I'd like to point out that I leave comments because I care about the direction of this project, not because I want to nag you. When you dismiss comments with "No" without reason, it makes it very hard to trust your judgement. Now that you've elaborated on your decisions, I can understand why you've made some of the choices you did. I'm still adamant on the list function > glob. There's no need to use the glob library when we have a faster internal function. |
I ack that the quick "No"'s are upsetting. And ack that's definitely not an ideal way to reply on PRs, for that I'm sorry. But questioning my judgment? That's disheartening. |
To clarify, I don't mean your judgement entirely, it's just hard to understand why I given choice was made if the motives aren't explained, which ultimately makes it harder for reviewers to judge whether a given choice was the best option. |
I absolutely get it. It was poor sportsmanship from my side, and to be honest I was stressed with other personal/work matters which led me to have a poor behaviour here. That said, all these questions felt at the time overwhelming which led me to that emotional response. I also did not add on the PR description why I made such changes, which I could have used LLMs to quickly generate for me. My actions came from a position of you questioning me whereas you just couldn't know why I did these changes I'm in the wrong. Your comments rubbed me wrong and I acted childishly, and I'm sorry. (Still it hurts when you said you can't trust me) |
| const sourcePath = join(srcDir, file); | ||
| const targetPath = join(targetDir, file); | ||
|
|
||
| const [sStat, tStat] = await Promise.allSettled([ | ||
| stat(sourcePath), | ||
| stat(targetPath), | ||
| ]); | ||
|
|
||
| const shouldWrite = | ||
| tStat.status === 'rejected' || | ||
| sStat.value.size !== tStat.value.size || | ||
| sStat.value.mtimeMs > tStat.value.mtimeMs; | ||
|
|
||
| if (!shouldWrite) { | ||
| continue; | ||
| } | ||
|
|
||
| const fileContent = await readFile(sourcePath); | ||
|
|
||
| await writeFile(targetPath, fileContent); |
There was a problem hiding this comment.
This is not "safe", there's still a TOCTOU: you call stat without locking the file, so when writeFile is reached the shouldWrite can be outdated (and with several processes running concurrently, it's almost guaranteed to happen).
There was a problem hiding this comment.
Yeah, this PR doesn't prevent concurrent reads/writes, it just "prevents" the fs issues you were facing. It's safe in the sense that it shouldn't fail due to concurrency issues, or very unlikely.
There was a problem hiding this comment.
Well I'm still getting ENOENT
|
I think there's a flaw in the dev process: if we have to fast-track stuff in this repo only to test them later on nodejs/node, it's highly inefficient and poor DX. FWIW I don't think I need the PR to have landed to test it, since we're passing a git hash, I suppose I could give the one that's the head of the PR |
Well, I did test these things and stopped getting ENOENTs. And was getting them before tho. |
|
(I didn't fast-track so you could test, I requested fast-track as I thought it was absolutely going to fix it and be a quick fix). Could you share what errors you're getting now? |
|
@aduh95 you're right, I shouldn't have fast-tracked, I'll go with another solution. Ehh, this still doesn't handle nicely locks nor the files being deleted and created at the same time. |
|
If the final solution works cross platform, it would definitely be worth upstreaming it to |
I'm sorry for the way I responded. I do trust you :-). Let's put this to bed, none of us are perfect, and I'm happy to be on a team with you, even when we have our off days. |
This PR simply uses a safer "only make changes loosly if we fell they need to be done" mechanism. Since git preserves file stats this shouldn't be an issue, or at the very least scenario, will run each only once.