fix: Avoid sync retry if file/directory was not found

[idsulik]

Description
This PR prevents sync retry if the err is related to a missing file/folder. For example - if you use npm to develop, it removes old files and generates new, and if you change files a lot, then the skaffold will try to sync old files several times, but they're deleted.

User facing changes (remove if N/A)
Before: Retry sync if a file/folder wasn't found.
After: No retry if a file/folder wasn't found.

[idsulik]

@plumpy hi! Could you please check it?

[plumpy]

Can you use os.IsNotExist instead of string matching?

[idsulik]

Good idea, thank you! pushed change

[plumpy]

So this essentially swallows the NotExist error entirely, treats it as though it wasn't an error at all. Is that the correct thing to do here?

The other option is to wrap it in PermanentError which will stop the retries, but still return it from backoff.Retry, and eventually it'll get returned from doDev on line 112.

But maybe that's not the right thing to do because a deleted file is intended and not an error?

[idsulik]

Improved the logic. This approach ensures that:

• Deleted files stop retrying immediately and log an informative message
• Other transient errors get retried (up to 3 times as configured)
• Persistent errors are logged, but don't stop the entire dev loop
• The dev loop continues processing other sync items even if one fails

[plumpy]

I guess this just feels like the wrong place for this. We're way too high up in the stack.

The syncHandler handles syncing every file in s.Copy and s.Delete. There could be dozens of files there. If one of them ends in a IsNotExist error, we didn't actually necessarily finish syncing all the files, right? We just gave up halfway through. There could be other errors we don't know about and other files we never got around to syncing.

So it feels like the syncer is going to have to be the one who decides if an IsNotExist error is relevant or not? If it's not relevant it shouldn't even return the error.

[idsulik]

If you mean this part:

if err != nil {
  log.Entry(ctx).Warnf("Sync failed after retries for %s: %v", s.Image, err)
  continue
}

Then yes, probably you're right, I can rollback the "return" statement

[plumpy]

No, I mean something like this:

1. syncHandler calls Sync() with a list of (for example) 50 files
2. Sync() starts looping through those files
3. on the third file, it can't find the file and returns a file-not-found error
   • notice: Files 4-50 are never synced!
4. your retry code gets this error, calls IsNotExist on it, stops the retry and returns nil, forever hiding the fact that there was an error

   if os.IsNotExist(err) {
     log.Entry(ctx).Infof("Skipping sync for %s: file no longer exists", s.Image)
     return nil
   }
   

5. the caller thinks the files are synced, but most of them are not!

[idsulik]

Thank you for the clarification, you're right, it's not a good idea to swallow the error. Pushed commit where I wrapped not found error in Permanent error, as you suggested before, now it prints the error and continues syncing other files

[idsulik]

And, it is correct now, because it doesn't stop syncing other change sets if something happened with 1 change set.
Instead of return in case of error, it uses continue now