Skip to content

Conversation

@marbens-arch
Copy link
Member

A full index transfer is required after we dropped all files.

Purpose

Fixes #10469.

Testing

Unit test and I tested the migration manually.

A full index transfer is required as we just dropped all files.

Signed-off-by: Marcus B Spencer <[email protected]>
@github-actions github-actions bot added the bug A problem with current functionality, as opposed to missing functionality (enhancement) label Nov 29, 2025
Signed-off-by: Marcus B Spencer <[email protected]>
Copy link
Member

@calmh calmh left a 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]>
@marbens-arch marbens-arch force-pushed the reset-index-id-when-dropping-all-files branch from f8d8f71 to cc9b557 Compare November 30, 2025 14:20
Copy link
Member

@imsodin imsodin left a 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.

@imsodin
Copy link
Member

imsodin commented Nov 30, 2025

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 folderDB.DropDevice even on a index ID reset when the device isn't unshared - just re-inserts the device index with a bumped integer.

Signed-off-by: Marcus B Spencer <[email protected]>
@marbens-arch
Copy link
Member Author

marbens-arch commented Nov 30, 2025

@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 folderDB.DropDevice is used when unsharing, because the problem is older than per-folder databases, and when one big SQLite database was used I think DropAllFiles was correct.

@imsodin
Copy link
Member

imsodin commented Nov 30, 2025

You likely mean this change, right? cf1cf85
Looks like the behaviour was the same before it, i.e. it also didn't remove the index IDs on DropAllFiles. Anyway, doesn't really matter when it changed, it clearly isn't right/needs fixing which you do here.

On my wider topic on removing the device idx - mostly FYI in case anyone is interested and to sort my thoughts, not relevant to merging this PR: In v1 the DB device idx was global we never dropped: Naturally not when dropping a index ID or unsharing a folder (as it wasn't folder scoped), but also not even if a device was entirely removed. In v2 before the folder split the device index was also global, and never removed: The method DropDevice existed, but was not used. And it still exists and isn't used now, and newer was. So a little opportunity for cleanup exists there (though looking at the cleanup/config change logic in model.go is headache inducing (I may say that, a lot of that is mine 🙃 )) :)

@marbens-arch
Copy link
Member Author

You likely mean this change, right? cf1cf85

@imsodin Yes, that's the change I was talking about.

Looks like the behaviour was the same before it, i.e. it also didn't remove the index IDs on DropAllFiles.

Indeed. I pointed that out above:

the problem is older than per-folder databases

Copy link
Member

@imsodin imsodin left a 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]>
Copy link
Member

@calmh calmh left a 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):

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.

@imsodin
Copy link
Member

imsodin commented Dec 1, 2025

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug A problem with current functionality, as opposed to missing functionality (enhancement)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Index IDs persist after unsharing folder

3 participants