-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib/model, gui: properly handle reverting receive-encrypted folders (fixes #8196) #9319
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ package model | |
import ( | ||
"fmt" | ||
"sort" | ||
"time" | ||
|
||
"github.com/syncthing/syncthing/lib/config" | ||
"github.com/syncthing/syncthing/lib/db" | ||
|
@@ -54,12 +55,10 @@ func (f *receiveEncryptedFolder) revert() error { | |
return err | ||
} | ||
defer snap.Release() | ||
var iterErr error | ||
|
||
var dirs []string | ||
snap.WithHaveTruncated(protocol.LocalDeviceID, func(intf protocol.FileIntf) bool { | ||
if iterErr = batch.FlushIfFull(); iterErr != nil { | ||
return false | ||
} | ||
_ = batch.FlushIfFull() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we ignoring these errors? |
||
|
||
fit := intf.(db.FileInfoTruncated) | ||
if !fit.IsReceiveOnlyChanged() || intf.IsDeleted() { | ||
|
@@ -88,23 +87,20 @@ func (f *receiveEncryptedFolder) revert() error { | |
|
||
return true | ||
}) | ||
_ = batch.Flush() | ||
|
||
f.revertHandleDirs(dirs, snap) | ||
// Revert the respective dirs. | ||
f.revertHandleDirs(dirs, snap, batch) | ||
|
||
if iterErr != nil { | ||
return iterErr | ||
} | ||
if err := batch.Flush(); err != nil { | ||
return err | ||
} | ||
_ = batch.Flush() | ||
|
||
// We might need to pull items if the local changes were on valid, global files. | ||
f.SchedulePull() | ||
|
||
return nil | ||
} | ||
|
||
func (f *receiveEncryptedFolder) revertHandleDirs(dirs []string, snap *db.Snapshot) { | ||
func (f *receiveEncryptedFolder) revertHandleDirs(dirs []string, snap *db.Snapshot, batch *db.FileInfoBatch) { | ||
if len(dirs) == 0 { | ||
return | ||
} | ||
|
@@ -113,8 +109,18 @@ func (f *receiveEncryptedFolder) revertHandleDirs(dirs []string, snap *db.Snapsh | |
go f.pullScannerRoutine(scanChan) | ||
defer close(scanChan) | ||
|
||
now := time.Now() | ||
sort.Sort(sort.Reverse(sort.StringSlice(dirs))) | ||
for _, dir := range dirs { | ||
batch.Append(protocol.FileInfo{ | ||
Name: dir, | ||
Type: protocol.FileInfoTypeDirectory, | ||
ModifiedS: now.Unix(), | ||
ModifiedBy: f.shortID, | ||
Deleted: true, | ||
Version: protocol.Vector{}, | ||
LocalFlags: protocol.FlagLocalReceiveOnly, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this the opposite of what we want? As we still have a receive-only entry in the db, while we want to get rid of them. The intention here (not saying it works, clearly it at least partially doesn't) is that we remove the locally changed directly and trigger a scan. The scan picks up the deletions, and as the global is also deleted marks it as not a locally changed item in the db (normal scan behaviour to not produce spurious locally changed items). I guess that's not what's happening in reality? Could you explain what is please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did some digging. And you're right, my approach wasn't right simply put. What I observed, but I think I stumbled upon another issue which may or may not be related but made it slightly annoying an inconsistent, is that three things are involved: 1.) It does remain a 'locally deleted item' unless I manually trigger a scan of the folder afterwards, done via the API (as the watcher is I think per default disabled and the rescan interval an hour). Note, that after this point the state only becomes correct in new clients. Any open client remains faulty, which leads to the following point (related, as this would be required, I think, from how it should work). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, thank! Still no idea how/why that happens, but great to have evidence of that, clearly pins down where something's going wrong. |
||
}) | ||
if err := f.deleteDirOnDisk(dir, snap, scanChan); err != nil { | ||
f.newScanError(dir, fmt.Errorf("deleting unexpected dir: %w", err)) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be updated though? Reverting or manually deleting should replace the locally changed items with non-locally changed items in the db (from revert and/or scan), which in turn updates the counts. And that should reach the UI through the folder summary events. If that's not happening, we should fix that instead.