isolate: give up and die on file unavailability

client/archiver/directory.go

 // Copyright 2015 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 package archiver
 
 import (
         "bytes"
         "encoding/json"
         "fmt"
@@ skipping 51 matching lines @@
                         return
                 }
         }
         if strings.HasSuffix(root, string(filepath.Separator)) {
                 root = root[:len(root)-1]
         }
         rootLen := len(root) + 1
         err := filepath.Walk(root, func(p string, info os.FileInfo, err error) error {
                 total++
                 if err != nil {
-                        return fmt.Errorf("walk(%s): %s", p, err)
+                        return fmt.Errorf("walk(%q): %v", p, err)

[mithro, 2016/11/29 03:43:44]

This will cause the binary to fail if it tries to walk a directory with a
permission denied error...

I think that is probably the right solution, but we should just double check?

[djd-OOO-Apr2017, 2016/11/29 03:51:55]

Do you have a suggestion on how to check that? Also, it doesn't look like this
change would affect the ultimate behaviour for this type of failure: such an
error is already been reported from walk and ultimately/hopefully cancelling the
upload.

                 }
                 if len(p) <= rootLen {
                         // Root directory.
                         return nil
                 }
                 relPath := p[rootLen:]
                 for _, b := range blacklist {
                         matched, _ := filepath.Match(b, relPath)
                         if !matched {
                                 // Also check at the base file name.
                                 matched, _ = filepath.Match(b, filepath.Base(relPath))
                         }
                         if matched {
                                 // Must not return io.SkipDir for file, filepath.walk() handles this
                                 // badly.
                                 if info.IsDir() {
                                         return filepath.SkipDir
                                 }
                                 return nil
                         }
                 }
                 if info.IsDir() {
                         return nil
                 }
                 c <- &walkItem{fullPath: p, relPath: relPath, info: info}
                 return nil
         })
         if err != nil {
-                c <- &walkItem{err: err}
+                // No point continuing if an error occurred during walk.
+                log.Fatalf("Unable to walk %q: %v", root, err)

[tandrii(chromium), 2016/11/30 14:35:08]

1. a test for this would be nice anyway - panic can be caught...
2. have you seen this
https://bugs.chromium.org/p/chromium/issues/detail?id=480558#c5? It'd be really
awesome to exit with the right error code.

[M-A Ruel, 2016/11/30 15:39:14]

I'm not enthusiastic about hard failing vs cleanly propagating the error up, but
I'm not concerned enough to block the CL as propagating the error would require
a fair amount of refactoring.

log.Fatal() calls os.Exit(1) and doesn't panic. It could be worth calling
log.Printf() + os.Exit(3) or something.

(Sorry for not commenting about this earlier)

         }
 }
 
 // PushDirectory walks a directory at root and creates a .isolated file.
 //
 // It walks the directories synchronously, then returns a *Item to signal when
 // the background work is completed. The Item is signaled once all files are
 // hashed. In particular, the *Item is signaled before server side cache
 // lookups and upload is completed. Use archiver.Close() to wait for
 // completion.
@@ skipping 78 matching lines @@
                                         }
                                 }
                         }
                 }
                 if err != nil {
                         s.SetErr(err)
                 }
         }()
         return s
 }