taildrop: fix theoretical race condition in fileDeleter.Init (#9876)

It is possible that upon a cold-start, we enqueue a partial file
for deletion that is resumed shortly after startup.

If the file transfer happens to last longer than deleteDelay,
we will delete the partial file, which is unfortunate.
The client spent a long time uploading a file,
only for it to be accidentally deleted.
It's a very rare race, but also a frustrating one
if it happens to manifest.

Fix the code to only delete partial files that
do not have an active puts against it.

We also fix a minor bug in ResumeReader
where we read b[:blockSize] instead of b[:cs.Size].
The former is the fixed size of 64KiB,
while the latter is usually 64KiB,
but may be less for the last block.

Updates tailscale/corp#14772

Signed-off-by: Joe Tsai <joetsai@digital-static.net>

taildrop/delete.go

@@ -27,8 +27,8 @@
 type fileDeleter struct {
 	logf  logger.Logf
 	clock tstime.DefaultClock
-	event func(string) // called for certain events; for testing only
 	dir   string
+	event func(string) // called for certain events; for testing only
 
 	mu     sync.Mutex
 	queue  list.List
@@ -46,11 +46,11 @@ type deleteFile struct {
 	inserted time.Time
 }
 
-func (d *fileDeleter) Init(logf logger.Logf, clock tstime.DefaultClock, event func(string), dir string) {
+func (d *fileDeleter) Init(m *Manager, eventHook func(string)) {
-	d.logf = logf
+	d.logf = m.opts.Logf
-	d.clock = clock
+	d.clock = m.opts.Clock
-	d.dir = dir
+	d.dir = m.opts.Dir
-	d.event = event
+	d.event = eventHook
 
 	// From a cold-start, load the list of partial and deleted files.
 	d.byName = make(map[string]*list.Element)
@@ -59,19 +59,30 @@ func (d *fileDeleter) Init(logf logger.Logf, clock tstime.DefaultClock, event fu
 	d.group.Go(func() {
 		d.event("start init")
 		defer d.event("end init")
-		rangeDir(dir, func(de fs.DirEntry) bool {
+		rangeDir(d.dir, func(de fs.DirEntry) bool {
 			switch {
 			case d.shutdownCtx.Err() != nil:
 				return false // terminate early
 			case !de.Type().IsRegular():
 				return true
-			case strings.Contains(de.Name(), partialSuffix):
+			case strings.HasSuffix(de.Name(), partialSuffix):
-				d.Insert(de.Name())
+				// Only enqueue the file for deletion if there is no active put.
-			case strings.Contains(de.Name(), deletedSuffix):
+				nameID := strings.TrimSuffix(de.Name(), partialSuffix)
+				if i := strings.LastIndexByte(nameID, '.'); i > 0 {
+					key := incomingFileKey{ClientID(nameID[i+len("."):]), nameID[:i]}
+					m.incomingFiles.LoadFunc(key, func(_ *incomingFile, loaded bool) {
+						if !loaded {
+							d.Insert(de.Name())
+						}
+					})
+				} else {
+					d.Insert(de.Name())
+				}
+			case strings.HasSuffix(de.Name(), deletedSuffix):
 				// Best-effort immediate deletion of deleted files.
 				name := strings.TrimSuffix(de.Name(), deletedSuffix)
-				if os.Remove(filepath.Join(dir, name)) == nil {
+				if os.Remove(filepath.Join(d.dir, name)) == nil {
-					if os.Remove(filepath.Join(dir, de.Name())) == nil {
+					if os.Remove(filepath.Join(d.dir, de.Name())) == nil {
 						break
 					}
 				}

taildrop/delete_test.go

@@ -67,8 +67,12 @@ func TestDeleter(t *testing.T) {
 	}
 	eventHook := func(event string) { eventsChan <- event }
 
+	var m Manager
 	var fd fileDeleter
-	fd.Init(t.Logf, tstime.DefaultClock{Clock: clock}, eventHook, dir)
+	m.opts.Logf = t.Logf
+	m.opts.Clock = tstime.DefaultClock{Clock: clock}
+	m.opts.Dir = dir
+	fd.Init(&m, eventHook)
 	defer fd.Shutdown()
 	insert := func(name string) {
 		t.Helper()

taildrop/resume.go

@@ -145,7 +145,7 @@ func ResumeReader(r io.Reader, hashNext func() (BlockChecksum, error)) (int64, i
 		}
 
 		// Read the contents of the next block.
-		n, err := io.ReadFull(r, b[:blockSize])
+		n, err := io.ReadFull(r, b[:cs.Size])
 		b = b[:n]
 		if err == io.EOF || err == io.ErrUnexpectedEOF {
 			err = nil

taildrop/retrieve.go

@@ -112,16 +112,15 @@ func (m *Manager) DeleteFile(baseName string) error {
 		err := os.Remove(path)
 		if err != nil && !os.IsNotExist(err) {
 			err = redactError(err)
-			// Put a retry loop around deletes on Windows. Windows
+			// Put a retry loop around deletes on Windows.
-			// file descriptor closes are effectively asynchronous,
+			//
-			// as a bunch of hooks run on/after close, and we can't
+			// Windows file descriptor closes are effectively asynchronous,
-			// necessarily delete the file for a while after close,
+			// as a bunch of hooks run on/after close,
-			// as we need to wait for everybody to be done with
+			// and we can't necessarily delete the file for a while after close,
-			// it. (on Windows, unlike Unix, a file can't be deleted
+			// as we need to wait for everybody to be done with it.
-			// if it's open anywhere)
+			// On Windows, unlike Unix, a file can't be deleted if it's open anywhere.
-			// So try a few times but ultimately just leave a
+			// So try a few times but ultimately just leave a "foo.jpg.deleted"
-			// "foo.jpg.deleted" marker file to note that it's
+			// marker file to note that it's deleted and we clean it up later.
-			// deleted and we clean it up later.
 			if runtime.GOOS == "windows" {
 				if bo == nil {
 					bo = backoff.NewBackoff("delete-retry", logf, 1*time.Second)

taildrop/taildrop.go

@@ -135,7 +135,7 @@ func (opts ManagerOptions) New() *Manager {
 		opts.SendFileNotify = func() {}
 	}
 	m := &Manager{opts: opts}
-	m.deleter.Init(opts.Logf, opts.Clock, func(string) {}, opts.Dir)
+	m.deleter.Init(m, func(string) {})
 	m.emptySince.Store(-1) // invalidate this cache
 	return m
 }
@@ -250,10 +250,6 @@ func (m *Manager) IncomingFiles() []ipn.PartialFile {
 	return files
 }
 
-// redacted is a fake path name we use in errors, to avoid
-// accidentally logging actual filenames anywhere.
-const redacted = "redacted"
-
 type redactedError struct {
 	msg   string
 	inner error
@@ -270,6 +266,7 @@ func (re *redactedError) Unwrap() error {
 func redactString(s string) string {
 	hash := adler32.Checksum([]byte(s))
 
+	const redacted = "redacted"
 	var buf [len(redacted) + len(".12345678")]byte
 	b := append(buf[:0], []byte(redacted)...)
 	b = append(b, '.')