-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix(db): drop index IDs when dropping all files (fixes #10469) #10478
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
base: main
Are you sure you want to change the base?
fix(db): drop index IDs when dropping all files (fixes #10469) #10478
Conversation
A full index transfer is required as we just dropped all files. Signed-off-by: Marcus B Spencer <[email protected]>
Signed-off-by: Marcus B Spencer <[email protected]>
calmh
left a comment
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.
Cool
Signed-off-by: Marcus B Spencer <[email protected]>
f8d8f71 to
cc9b557
Compare
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.
Edit: Retracting the review requesting changes. The below is still potential relevant, but if it applies it could be a separate change/fix - see followup comment below.
Original: Thinking about naming nits, I realized besides the adjusted folderDB.DropAllFiles there's also folderDB.DropDevice - the difference is that it cleanes up the device and index ID, which in turn removes files with DB hooks. This PR removes makes DropAllFiles do almost the same thing, except preserving the device index in the folder specific table. For the referenced bug #10469 that shouldn't be relevant as the scenario involves unsharing the device - that should remove the device and index ID. I don't yet see if the bug is that some model code should be calling folderDB.DropDevice (not exposed currently) e.g. here while other model code still needs DropAllFiles (some/all cases in the index handler), or if DropDevice and DropAllFiles should be the same thing, aka always drop the device index too - still looking into that. In any case leaving behind the device index on device unshare seems like a mistake.
As usual I started with nitty, not very relevant comments (inline below) before realizing the above, leaving them in place just in case.
|
Followup to the above review comment: #10478 (review) Retracting the requesting changes review - while the question of dropping the device index remains (see below), the change in this PR is a step in the right direction regardless. Indeed strictly speaking I think there are two distinct scenarios in the model with distinct requirements: One to drop all files and index ID for a device, in cases where the folder stays shared with the device but needs a index/file reset. And another to do that plus drop the device index on unshare. Otherwise we keep that device index forever in the folder device table (unless that's intentional?). We could also simplify code and always drop all information/use |
Signed-off-by: Marcus B Spencer <[email protected]>
|
@imsodin I thought about this too when climbing up the "ladder" to hunt down the bug. I didn't consider the bug to be that |
|
You likely mean this change, right? cf1cf85 On my wider topic on removing the device |
imsodin
left a comment
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.
Explicitly approving mainly to make it clear that all the noise I produced above does not mean that I have concerns with this PR - still please wait with merging for @calmh to have a look again.
Signed-off-by: Marcus B Spencer <[email protected]>
calmh
left a comment
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.
Yeah, no, I don't think this is precisely correct. There are cases where we expect to be able to do DropAllFiles without dropping the index ID, for example when getting the initial index from a device where we've already set the index ID (and in fact already run DropAllFiles once, I think):
syncthing/lib/model/indexhandler.go
Line 374 in cde867c
| if err := s.sdb.DropAllFiles(s.folder, deviceID); err != nil { |
This should probably be a more targeted change specifically where we're calling it after unsharing / when detecting an unshared situation.
|
Argh right. Also the sequence is tracked together with the index ID in the DB, and afaik in the (hypothetical) scenario where the remote announces an index ID and non-zero sequence but then still sends an index (not update), we'd want to keep the index ID but set the sequence to zero. Something like this should afaik fix the the bug and deal with the sequence: #10480 ? |
A full index transfer is required after we dropped all files.
Purpose
Fixes #10469.
Testing
Unit test and I tested the migration manually.