Handle ENOENT When Fingerprinting Deleted Files

[ObliviousHarmony]

While computing the fingerprint there is a gap between input file globbing and fingerprinting wherein a file might have been deleted. This causes the hash building process to throw an exception and crashes the --watch operation. We ran into this problem while pairing a wireit script's --watch option with webpack --watch. When Webpack started replacing files it would sometimes cause the wireit script to stop the watch process.

This pull request adds some error handling to this process so that wireit will continue working in this case.

[ObliviousHarmony]

Could you take a look here @rictic? (You merged my last PR, sorry for the ping)

[ObliviousHarmony]

What about you @aomarks, would you be able to take a look at this?

[ObliviousHarmony]

Now that we're past all of the holidays, could you take a look @aomarks or @rictic?

[aomarks]

Thanks for this PR! I agree we should do something here since it shouldn't crash. However, I think that ignoring the files that got deleted during this race window might not be the best answer, because in general a file getting deleted in that window means there could be a problem with the configuration/setup that's going to cause wireit to get confused later on too. In this case, it sounds like you have a separate script which is independently writing to the same output files as the wireit script you are watching. I think we should probably invalidate the fingerprint somehow, so that the next time this script is invoked, a full build is forced.

[ObliviousHarmony]

I think we should probably invalidate the fingerprint somehow, so that the next time this script is invoked, a full build is forced.

My first pass at this digested an empty file content so that successive builds without the file in the glob would have a different fingerprint. I went with this approach instead because it felt like a deleted output file would be something considered as part of the build output, however, I can see how that could be potentially problematic. It's probably better to err on the side of caution and ensure the cache is always valid instead of trying to avoid a rebuild that might have actually been necessary.

[ObliviousHarmony]

I took a look at two possible approaches to fingerprinting in this case @aomarks:

• Empty File: This would handle the invalidation gracefully, however, it would deliberately create a collision with an actually empty file.
• Additional Fingerprint Field: We could also add an additional property to the fingerprint that records the failure. This would let us invalidate the entire fingerprint.

The issue here is that, clearly, any assumption we make here is akin to undefined behavior. If we're trying to be safe then it's probably better to not cache anything at all. It makes more sense to fail the execution and convert the exception into an error result so that wireit can handle it gracefully. How would you feel about that?

[aomarks]

The issue here is that, clearly, any assumption we make here is akin to undefined behavior. If we're trying to be safe then it's probably better to not cache anything at all. It makes more sense to fail the execution and convert the exception into an error result so that wireit can handle it gracefully. How would you feel about that?

Yeah, that sounds like probably the thing to do. This also means that subsequent scripts that depend on this one won't be able to run -- which sounds correct. The build is likely in an invalid state after this race condition has been detected, so basically stopping execution at that node in the graph makes sense to me.

[ObliviousHarmony]

How about this @aomarks?

[aomarks]

How about this @aomarks?

This looks great!

(This is a tricky one to test because it's hard to reproduce the situation. We'd need to have some tests where we sub in an fs wrapper that allows us to block readDir from returning or something. I think it's fine without a test for now.)

[ObliviousHarmony]

This is a tricky one to test because it's hard to reproduce the situation

I tested it out locally by breaking the createReadStream path. I wanted to write some tests for it but I've never used uvu and wasn't feeling super confident about putting in that extra time to figure it out.

Would you mind releasing a new version of wireit soon so that all of the changes I've submitted can go out?

[aomarks]

BTW @ObliviousHarmony you should join our Discord if you're not already there! Good place for brainstorming and faster discussion iteration. https://discord.gg/chmdAwvq4e

[aomarks]

I tested it out locally by breaking the createReadStream path. I wanted to write some tests for it but I've never used uvu and wasn't feeling super confident about putting in that extra time to figure it out.

Nice! Yeah, that sounds fine. It would be quite a lot of work to test this properly and probably not worth it right now.

Would you mind releasing a new version of wireit soon so that all of the changes I've submitted can go out?

I did just release v0.14.2 but it didn't include this change. I'll do another one to pick this up too.