From 4ce33c9758af421662b737e2d16d7f3362b2a967 Mon Sep 17 00:00:00 2001 From: Rhea Ghosh Date: Tue, 9 Jan 2024 14:11:34 -0600 Subject: [PATCH] taildrop: remove breaking abstraction layers for apple (#10728) Removes the avoidFinalRename logic and all associated code as it is no longer required by the Apple clients. Enables resume logic to be usable for Apple clients. Fixes tailscale/corp#14772 Signed-off-by: Rhea Ghosh --- cmd/tailscaled/taildrop.go | 1 - ipn/backend.go | 1 + ipn/ipnlocal/local.go | 29 ++++++++--------------------- taildrop/resume.go | 6 ------ taildrop/send.go | 31 ++++++++++--------------------- taildrop/taildrop.go | 10 +--------- 6 files changed, 20 insertions(+), 58 deletions(-) diff --git a/cmd/tailscaled/taildrop.go b/cmd/tailscaled/taildrop.go index 11f026e27..39fe54373 100644 --- a/cmd/tailscaled/taildrop.go +++ b/cmd/tailscaled/taildrop.go @@ -27,7 +27,6 @@ func configureTaildrop(logf logger.Logf, lb *ipnlocal.LocalBackend) { } else { logf("%s Taildrop: using %v", dg, path) lb.SetDirectFileRoot(path) - lb.SetDirectFileDoFinalRename(true) } } diff --git a/ipn/backend.go b/ipn/backend.go index ad5dbd4bf..269360400 100644 --- a/ipn/backend.go +++ b/ipn/backend.go @@ -175,6 +175,7 @@ type PartialFile struct { // in-progress '*.partial' file's path when the peerapi isn't // being used; see LocalBackend.SetDirectFileRoot. PartialPath string `json:",omitempty"` + FinalPath string `json:",omitempty"` // Done is set in "direct" mode when the partial file has been // closed and is ready for the caller to rename away the diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index a06a66a54..a5f3ae693 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -265,9 +265,8 @@ type LocalBackend struct { // It's also used on several NAS platforms (Synology, TrueNAS, etc) // but in that case DoFinalRename is also set true, which moves the // *.partial file to its final name on completion. - directFileRoot string - directFileDoFinalRename bool // false on macOS, true on several NAS platforms - componentLogUntil map[string]componentLogState + directFileRoot string + componentLogUntil map[string]componentLogState // c2nUpdateStatus is the status of c2n-triggered client update. c2nUpdateStatus updateStatus currentUser ipnauth.WindowsToken @@ -540,17 +539,6 @@ func (b *LocalBackend) SetDirectFileRoot(dir string) { b.directFileRoot = dir } -// SetDirectFileDoFinalRename sets whether the peerapi file server should rename -// a received "name.partial" file to "name" when the download is complete. -// -// This only applies when SetDirectFileRoot is non-empty. -// The default is false. -func (b *LocalBackend) SetDirectFileDoFinalRename(v bool) { - b.mu.Lock() - defer b.mu.Unlock() - b.directFileDoFinalRename = v -} - // ReloadConfig reloads the backend's config from disk. // // It returns (false, nil) if not running in declarative mode, (true, nil) on @@ -3877,13 +3865,12 @@ func (b *LocalBackend) initPeerAPIListener() { ps := &peerAPIServer{ b: b, taildrop: taildrop.ManagerOptions{ - Logf: b.logf, - Clock: tstime.DefaultClock{Clock: b.clock}, - State: b.store, - Dir: fileRoot, - DirectFileMode: b.directFileRoot != "", - AvoidFinalRename: !b.directFileDoFinalRename, - SendFileNotify: b.sendFileNotify, + Logf: b.logf, + Clock: tstime.DefaultClock{Clock: b.clock}, + State: b.store, + Dir: fileRoot, + DirectFileMode: b.directFileRoot != "", + SendFileNotify: b.sendFileNotify, }.New(), } if dm, ok := b.sys.DNSManager.GetOK(); ok { diff --git a/taildrop/resume.go b/taildrop/resume.go index 1296df181..dfcc408c3 100644 --- a/taildrop/resume.go +++ b/taildrop/resume.go @@ -64,9 +64,6 @@ func (m *Manager) PartialFiles(id ClientID) (ret []string, err error) { if m == nil || m.opts.Dir == "" { return nil, ErrNoTaildrop } - if m.opts.DirectFileMode && m.opts.AvoidFinalRename { - return nil, nil // resuming is not supported for users that peek at our file structure - } suffix := id.partialSuffix() if err := rangeDir(m.opts.Dir, func(de fs.DirEntry) bool { @@ -90,9 +87,6 @@ func (m *Manager) HashPartialFile(id ClientID, baseName string) (next func() (Bl } noopNext := func() (BlockChecksum, error) { return BlockChecksum{}, io.EOF } noopClose := func() error { return nil } - if m.opts.DirectFileMode && m.opts.AvoidFinalRename { - return noopNext, noopClose, nil // resuming is not supported for users that peek at our file structure - } dstFile, err := joinDir(m.opts.Dir, baseName) if err != nil { diff --git a/taildrop/send.go b/taildrop/send.go index d96c3542c..0dff71b24 100644 --- a/taildrop/send.go +++ b/taildrop/send.go @@ -31,6 +31,7 @@ type incomingFile struct { w io.Writer // underlying writer sendFileNotify func() // called when done partialPath string // non-empty in direct mode + finalPath string // not used in direct mode mu sync.Mutex copied int64 @@ -92,13 +93,6 @@ func (m *Manager) PutFile(id ClientID, baseName string, r io.Reader, offset, len return err } - avoidPartialRename := m.opts.DirectFileMode && m.opts.AvoidFinalRename - if avoidPartialRename { - // Users using AvoidFinalRename are depending on the exact filename - // of the partial files. So avoid injecting the id into it. - id = "" - } - // Check whether there is an in-progress transfer for the file. partialPath := dstPath + id.partialSuffix() inFileKey := incomingFileKey{id, baseName} @@ -111,6 +105,7 @@ func (m *Manager) PutFile(id ClientID, baseName string, r io.Reader, offset, len } if m.opts.DirectFileMode { inFile.partialPath = partialPath + inFile.finalPath = dstPath } return inFile }) @@ -128,10 +123,6 @@ func (m *Manager) PutFile(id ClientID, baseName string, r io.Reader, offset, len defer func() { f.Close() // best-effort to cleanup dangling file handles if err != nil { - if avoidPartialRename { - os.Remove(partialPath) // best-effort - return - } m.deleter.Insert(filepath.Base(partialPath)) // mark partial file for eventual deletion } }() @@ -179,16 +170,9 @@ func (m *Manager) PutFile(id ClientID, baseName string, r io.Reader, offset, len } fileLength := offset + copyLength - // Return early for avoidPartialRename since users of AvoidFinalRename - // are depending on the exact naming of partial files. - if avoidPartialRename { - inFile.mu.Lock() - inFile.done = true - inFile.mu.Unlock() - m.totalReceived.Add(1) - m.opts.SendFileNotify() - return fileLength, nil - } + inFile.mu.Lock() + inFile.done = true + inFile.mu.Unlock() // File has been successfully received, rename the partial file // to the final destination filename. If a file of that name already exists, @@ -221,6 +205,10 @@ func (m *Manager) PutFile(id ClientID, baseName string, r io.Reader, offset, len } // Avoid the final rename if a destination file has the same contents. + // + // Note: this is best effort and copying files from iOS from the Media Library + // results in processing on the iOS side which means the size and shas of the + // same file can be different. if dstLength == fileLength { partialSum, err := computePartialSum() if err != nil { @@ -240,6 +228,7 @@ func (m *Manager) PutFile(id ClientID, baseName string, r io.Reader, offset, len // Choose a new destination filename and try again. dstPath = NextFilename(dstPath) + inFile.finalPath = dstPath } if maxRetries <= 0 { return 0, errors.New("too many retries trying to rename partial file") diff --git a/taildrop/taildrop.go b/taildrop/taildrop.go index c9717c2d4..9ad0e1a7e 100644 --- a/taildrop/taildrop.go +++ b/taildrop/taildrop.go @@ -90,15 +90,6 @@ type ManagerOptions struct { // copy them out, and then delete them. DirectFileMode bool - // AvoidFinalRename specifies whether in DirectFileMode - // we should avoid renaming "foo.jpg.partial" to "foo.jpg" after reception. - // - // TODO(joetsai,rhea): Delete this. This is currently depended upon - // in the Apple platforms since it violates the abstraction layer - // and directly assumes how taildrop represents partial files. - // Right now, file resumption does not work on Apple. - AvoidFinalRename bool - // SendFileNotify is called periodically while a file is actively // receiving the contents for the file. There is a final call // to the function when reception completes. @@ -244,6 +235,7 @@ func (m *Manager) IncomingFiles() []ipn.PartialFile { DeclaredSize: f.size, Received: f.copied, PartialPath: f.partialPath, + FinalPath: f.finalPath, Done: f.done, }) return true