lib/db: Don't put truncated files (ref #6855, ref #6501)

[imsodin]

Calling .putFile with truncate == true and a file with less blocks than blocksIndirectionCutoff puts a fileinfo without blocks. truncate == true is only the case when repairing sequence indexes, so this "shoudn't happen"™. However it does, so it's a real practical issue.

#6855

The tiny change in TestRepairSequence triggers the problem described above. To prevent it, don't ever write truncated files infos. If we wanted to write truncated files, we'd need to always redirect blocklists and check that a block list for the given BlocksHash of the truncated file already exists. Given we only write truncated files when repairing sequences and it's just an irrelevant "optimisation", I don't want to go there and instead just write full file infos.

I also added context to errors in fillFileInfo and adjusted the IsClosed etc. checks to use errors.As, to make the "no file info at all" distinguishable from "blocks/version not found".

I really need to pay more attention to numbers in sentry. The "missing-entry-panic" already affects 30 users on v1.8.0 (https://sentry.syncthing.net/syncthing/syncthing/?query=is%3Aunresolved+release%3Av1.8.0+device+present&statsPeriod=14d) and over 300 in total (https://sentry.syncthing.net/syncthing/syncthing/?query=is%3Aunresolved+device+present&statsPeriod=14d). I usually just notice newly occurring events and then either take action or shelf them...

[imsodin]

This also needs a migration, as existing brokenness (files with non-nil BlocksHash but nil Blocks) will not solve itself. I added a test for that.

I'd like @calmh to have a look at this before merging, as I want to be as sure as possible that I don't just introduce a new kind of truncate-hell here and he dealt (had to deal) with truncate-messes before.

[calmh]

I'm on mobile and away until tomorrow but will take a look then.

I've suggested before that we we should treat entries with missing parts (blocks, version vectors) as simply missing themselves. Conflict resolution should take over and repair it from there?

[calmh]

(Still away, back Monday...)

[calmh]

This looks reasonable, yet I'm not sure I see the need for the migration. After the fixup the file will be in the database with no blocks (I guess?) and the MustRescan flag. We'll hash it on the next scan, or trigger a rescan when pulling it.

If we didn't do the migration and returned NotFound for the file, we'd hash it on next scan or mark it for rescan when pulling (because present on disk but not in database), which is simpler and safer should the problem resurface randomly (database corruption dropping blocks or whatnot)

[calmh]

I think var e *errClosed feels more descriptive than the struct literal thing, here it feels like i need to count the ampersands while stating the type explicitly is less thinking

[imsodin]

Weirdly enought, that change causes test failures but not the panics that are documented on https://golang.org/pkg/errors/#As:

As panics if target is not a non-nil pointer to either a type that implements error, or to any interface type.

[calmh]

So I mean

	var e *errClosed
	return errors.As(err, &e)

right?

[imsodin]

Yep, that breaks our unit tests, see below. I don't really get it, as we actually pass a non-nil pointer to a typed nil-pointer implementing error, which to my understanding should fullfill this:

An error matches target if the error's concrete value is assignable to the value pointed to by target [...]

$ go test -race -short ./lib/db/
--- FAIL: TestIgnoredFiles (0.00s)
    db_test.go:40: key not found
--- FAIL: TestUpdate0to3 (0.00s)
    db_test.go:182: key not found
--- FAIL: TestRepairSequence (0.00s)
    db_test.go:327: key not found
--- FAIL: TestDowngrade (0.00s)
    db_test.go:472: key not found

[calmh]

But that's not possible... https://play.golang.org/p/Em_-8XFeVJj

[imsodin]

I agree, the failures happen on exactly this change though. I pushed it so you can see what the change and error are, maybe there's a typo I fail to see? I can't investigate right now, will do so later if the mystery hasn't been lifted until then.

[calmh]

Comment on why it's important to have this specific number of blocks? (I guess to get the same blocks as another file?)

[imsodin]

We do return a not found error, which makes this work except that when we encounter an err not found for entries in the global list, we panic. For one just dropping the global list entry in this case, possibly leaving other things inconsistent, would also be bad and I think we need at least discoverable failure reports in such cases - I wouldn't want a bug like this to just fly under the radar. And in this case there might even be a catch with just dropping the fileinfo: If only the device in question ever bumped the file version, the new file info created will have e.g. version {A: 1} while everyone else has e.g. {A: 10}, thus everyone elses, possibly older version overrides the current one.

I really want to do failure handling right around the db, meaning try to recover gracefully from everything possible as you say, but also have means to know about failures. And recovering correctly often means that the caller needs to know whether there's been a failure or the file is genuinely not existing.

[calmh]

Fair enough

[calmh]

...notFound :)

[imsodin]

It had to be something like that... Sorry for the wasted time.