If we fail to take the rename shortcut we may crash on a later loop,
because we do trickiness with the indexes but the original buckets[key]
in "range buckets[key]" isn't re-evaluated so i exceeds the max index.
This adds indirection of large version vectors in the same manner as we
already to block lists. The effect is the same: less duplicated data in
some situations.
To mitigate the impact for when this indirection
wouldn't be needed I've added an indirection cutoff for both blocks and
the new version vector stuff: we don't do the indirection at all for
small block lists or small version vectors, instead storing it directly
like we used to do. This is faster for small files and small setups.
Storing assets as []byte requires every compiled-in asset to be copied
into writable memory at program startup. That currently takes up 1.6MB
per syncthing process. Strings stay in the RODATA section and should be
shared between processes running the same binary.
This makes version vector values clock based instead of just incremented
from zero. The effect is that a vector that is created from scratch
(after database reset) will have a higher value for the local device
than what it could have been previously, causing a conflict. That is, if
we are A and we had
{A: 42, B: 12}
in the old scheme, a reset and rescan would give us
{A: 1}
which is a strict ancestor of the older file (this might be wrong). With
the new scheme we would instead have
{A: someClockTime, b: otherClockTime}
and the new version after reset would become
{A: someClockTime+delta}
which is in conflict with the previous entry (better).
In case the clocks are wrong (current time is less than the value in the
vector) we fall back to just simple increment like today.
This scheme is ineffective if we suffer a database reset while at the
same time setting the clock back far into the past. It's however no
worse than what we already do.
This loses the ability to emit the "added" event, as we can't look for
the magic 1 entry any more. That event was however already broken
(#5541).
Another place where we infer meaning from the vector itself is in
receive only folders, but there the only criteria is that the vector is
one item long and includes just ourselves, which remains the case with
this change.
* wip
This changes the build script to build all the things in one go
invocation, instead of one invocation per cmd. This is a lot faster
because it means more things get compiled concurrently. It's especially
a lot faster when things *don't* need to be rebuilt, possibly because it
only needs to build the dependency map and such once instead of once per
binary.
In order for this to work we need to be able to pass the same ldflags to
all the binaries. This means we can't set the program name with an
ldflag.
When it needs to rebuild everything (go clean -cache):
( ./old-build -gocmd go1.14.2 build all 2> /dev/null; ) 65.82s user 11.28s system 574% cpu 13.409 total
( ./new-build -gocmd go1.14.2 build all 2> /dev/null; ) 63.26s user 7.12s system 1220% cpu 5.766 total
On a subsequent run (nothing to build, just link the binaries):
( ./old-build -gocmd go1.14.2 build all 2> /dev/null; ) 26.58s user 7.53s system 582% cpu 5.853 total
( ./new-build -gocmd go1.14.2 build all 2> /dev/null; ) 18.66s user 2.45s system 1090% cpu 1.935 total
This makes the GC runner a service that will stop fairly quickly when
told to.
As a bonus, STTRACE=app will print the service tree on the way out,
including any errors they've flagged.
So, in a funny plot twist, it turns out that WriteFile in Go 1.13
doesn't actually set the read only bit on Windows when called with
permissions 0444 so my test was broken. With an improved test it turns
out that Rename does not, in fact, overwrite a read-only file on
Windows. This adds a fix for that.
(Rename might get improved in Go 1.15...)
This adds the functionality to run a user search with a filter for LDAP
authentication. The search is done after successful bind, as the binding
user. The typical use case is to limit authentication to users who are
member of a group or under a certain OU. For example, to only match
users in the "Syncthing" group in otherwise default Active Directory
set up for example.com:
<searchBaseDN>CN=Users,DC=example,DC=com</searchBaseDN>
<searchFilter>(&(sAMAccountName=%s)(memberOf=CN=Syncthing,CN=Users,DC=example,DC=com))</searchFilter>
The search filter is an "and" of two criteria (with the ampersand being
XML quoted),
- "(sAMAccountName=%s)" matches the user logging in
- "(memberOf=CN=Syncthing,CN=Users,DC=example,DC=com)" matches members
of the group in question.
Authentication will only proceed if the search filter matches precisely
one user.
The previous implementation was very generic; its tests didn't cover the
actual alphabet for device IDs.
Benchmark results on amd64:
name old time/op new time/op delta
Luhnify-8 1.00µs ± 1% 0.28µs ± 4% -72.38% (p=0.000 n=9+10)
Unluhnify-8 992ns ± 2% 274ns ± 1% -72.39% (p=0.000 n=10+9)
As of the latest database checker we are again putting files without
blocks. I'm not 100% convinced that's a great idea, but we also do it
for ignored files apparently so it looks like we probably should support
it. This adds an escape hatch that must be manually enabled...
- In the few places where we wrap errors, use the new Go 1.13 "%w"
construction instead of %s or %v.
- Where we create errors with constant strings, consistently use
errors.New and not fmt.Errorf.
- Remove capitalization from errors in the few places where we had that.
If we decide to recalculate the metadata we shouldn't start from
whatever we loaded from the database, as that data is wrong. We should
start from a clean slate.
I was working on indirecting version vectors, and that resulted in some
refactoring and improving the existing block indirection stuff. We may
or may not end up doing the version vector indirection, but I think
these changes are reasonable anyhow and will simplify the diff
significantly if we do go there. The main points are:
- A bunch of renaming to make the indirection and GC not about "blocks"
but about "indirection".
- Adding a cutoff so that we don't actually indirect for small block
lists. This gets us better performance when handling small files as it
cuts out the indirection for quite small loss in space efficiency.
- Being paranoid and always recalculating the hash on put. This costs
some CPU, but the consequences if a buggy or malicious implementation
silently substituted the block list by lying about the hash would be bad.
One of the causes of "panic: database is closed" is that we try to send
summaries after it's been closed. Calculating summaries can take a long
time and if we have a lot of folders it's not unreasonable to think
that we might be stopped in this loop, so prepare to bail here.
* push
During NAT discovery we block for 10s (NatTimeoutS) before returning.
This is mostly noticeable when Ctrl-C:ing a Syncthing directly after
startup as we wait for those ten seconds before shutting down. This
makes it check the context a little bit more frequently.
This adds metadata updates to the same write batch as the underlying
file change. The odds of a metadata update going missing is greatly
reduced.
Bonus change: actually commit the transaction in recalcMeta.
lib/db: Recover sequence number and metadata on startup (fixes#6335)
If we crashed after writing new file entries but before updating
metadata in the database the sequence number and metadata will be wrong.
This fixes that.
We could potentially get a snapshot and then fail to get a releaser,
leaking the snapshot. This takes the releaser first and makes sure to
release it on snapshot error.
The readWriteTransaction offered both commit() (the one to use) and
Commit() (via embedding) where the latter didn't close the read
transaction. This removes the lower cased variant in order to prevent
the mistake.
The only place where the mistake was made was the new gc runner, where
it would leave a read snapshot open forever.
During some other work I discovered these tests weren't great, so I've
rewritten them to be a little better. The real changes here are:
- Don't play games with not starting the folder and such, and don't
construct a fake folder instance -- just use the one the model has. The
folder starts and scans but the folder contents are empty at this point
so that's fine.
- Use a fakefs instead of a temp dir.
- To support the above, implement a fakefs option `?content=true` to
make the fakefs actually retain written content. Use sparingly,
obviously, but it means the fakefs can usually be used instead of an
on disk real directory.
This adds a new config with the simple and concise name
maxConcurrentIncomingRequestKiB. This limits how many bytes we have "in
the air" in the form of response data being read and processed.
After some testing I think that not having this limiter is seldom a
great idea and thus I propose a default value of 256 MiB for this new
setting.
I also refactored the folder IO limiter to be a model/folder attribute
instead of a package global.
Adds a new folder state "Waiting to Sync" in the same vein as the
existing "Waiting to Scan". This vastly improves performances in the
rare cases when there are lots and lots of folders operating.
Also retain the interval over restarts by storing last GC time in the
database. This to make sure that GC eventually happens even if the
interval is configured to a long time (say, a month).
* lib/db: Deduplicate block lists in database (fixes#5898)
This moves the block list in the database out from being just a field on
the FileInfo to being an object of its own. When putting a FileInfo we
marshal the block list separately and store it keyed by the sha256 of
the marshalled block list. When getting, if we are not doing a
"truncated" get, we do an extra read and unmarshal for the block list.
Old block lists are cleared out by a periodic GC sweep. The alternative
would be to use refcounting, but:
- There is a larger risk of getting that wrong and either dropping a
block list in error or keeping them around forever.
- It's tricky with our current database, as we don't have dirty reads.
This means that if we update two FileInfos with identical block lists in
the same transaction we can't just do read/modify/write for the ref
counters as we wouldn't see our own first update. See above about
tracking this and risks about getting it wrong.
GC uses a bloom filter for keys to avoid heavy RAM usage. GC can't run
concurrently with FileInfo updates so there is a new lock around those
operation at the lowlevel.
The end result is a much more compact database, especially for setups
with many peers where files get duplicated many times.
This is per-key-class stats for a large database I'm currently working
with, under the current schema:
```
0x00: 9138161 items, 870876 KB keys + 7397482 KB data, 95 B + 809 B avg, 1637651 B max
0x01: 185656 items, 10388 KB keys + 1790909 KB data, 55 B + 9646 B avg, 924525 B max
0x02: 916890 items, 84795 KB keys + 3667 KB data, 92 B + 4 B avg, 192 B max
0x03: 384 items, 27 KB keys + 5 KB data, 72 B + 15 B avg, 87 B max
0x04: 1109 items, 17 KB keys + 17 KB data, 15 B + 15 B avg, 69 B max
0x06: 383 items, 3 KB keys + 0 KB data, 9 B + 2 B avg, 18 B max
0x07: 510 items, 4 KB keys + 12 KB data, 9 B + 24 B avg, 41 B max
0x08: 1349 items, 12 KB keys + 10 KB data, 9 B + 8 B avg, 17 B max
0x09: 194 items, 0 KB keys + 123 KB data, 5 B + 634 B avg, 11484 B max
0x0a: 3 items, 0 KB keys + 0 KB data, 14 B + 7 B avg, 30 B max
0x0b: 181836 items, 2363 KB keys + 10694 KB data, 13 B + 58 B avg, 173 B max
Total 10426475 items, 968490 KB keys + 9202925 KB data.
```
Note 7.4 GB of data in class 00, total size 9.2 GB. After running the
migration we get this instead:
```
0x00: 9138161 items, 870876 KB keys + 2611392 KB data, 95 B + 285 B avg, 4788 B max
0x01: 185656 items, 10388 KB keys + 1790909 KB data, 55 B + 9646 B avg, 924525 B max
0x02: 916890 items, 84795 KB keys + 3667 KB data, 92 B + 4 B avg, 192 B max
0x03: 384 items, 27 KB keys + 5 KB data, 72 B + 15 B avg, 87 B max
0x04: 1109 items, 17 KB keys + 17 KB data, 15 B + 15 B avg, 69 B max
0x06: 383 items, 3 KB keys + 0 KB data, 9 B + 2 B avg, 18 B max
0x07: 510 items, 4 KB keys + 12 KB data, 9 B + 24 B avg, 41 B max
0x09: 194 items, 0 KB keys + 123 KB data, 5 B + 634 B avg, 11484 B max
0x0a: 3 items, 0 KB keys + 0 KB data, 14 B + 17 B avg, 51 B max
0x0b: 181836 items, 2363 KB keys + 10694 KB data, 13 B + 58 B avg, 173 B max
0x0d: 44282 items, 1461 KB keys + 61081 KB data, 33 B + 1379 B avg, 1637399 B max
Total 10469408 items, 969939 KB keys + 4477905 KB data.
```
Class 00 is now down to 2.6 GB, with just 61 MB added in class 0d.
There will be some additional reads in some cases which theoretically
hurts performance, but this will be more than compensated for by smaller
writes and better compaction.
On my own home setup which just has three devices and a handful of
folders the difference is smaller in absolute numbers of course, but
still less than half the old size:
```
0x00: 297122 items, 20894 KB keys + 306860 KB data, 70 B + 1032 B avg, 103237 B max
0x01: 115299 items, 7738 KB keys + 17542 KB data, 67 B + 152 B avg, 419 B max
0x02: 1430537 items, 121223 KB keys + 5722 KB data, 84 B + 4 B avg, 253 B max
...
Total 1947412 items, 151268 KB keys + 337485 KB data.
```
to:
```
0x00: 297122 items, 20894 KB keys + 37038 KB data, 70 B + 124 B avg, 520 B max
0x01: 115299 items, 7738 KB keys + 17542 KB data, 67 B + 152 B avg, 419 B max
0x02: 1430537 items, 121223 KB keys + 5722 KB data, 84 B + 4 B avg, 253 B max
...
0x0d: 18041 items, 595 KB keys + 71964 KB data, 33 B + 3988 B avg, 101109 B max
Total 1965447 items, 151863 KB keys + 139628 KB data.
```
* wip
* wip
* wip
* wip
Makes the logic a bit clearer and safer. This also sneakily redefines
the 0 interval to also mean disabled, whereas it previously meant ...
sometimes default to 1s, sometimes just spin.
* lib/db, lib/protocol: Compact FileInfo and BlockInfo alignment
This fixes the following two lint warnings
FileInfo: struct of size 160 bytes could be of size 136 bytes
BlockInfo: struct of size 48 bytes could be of size 40 bytes
by reordering fields in alignment order (64 bit fields, then 32 bit
fields, then 16 bit fields (if any), then small ones). The end result is
a slightly less aesthetically pleasing struct field order, but since
these are the objects we often juggle in bulk and keep large queues of I
think it's worth it.
It's a micro optimization, but a cheap one.
This adds error returns to model methods called by the protocol layer.
Returning an error will cause the connection to be torn down as the
message couldn't be handled. Using this to signal that a folder isn't
currently available will then cause a reconnection a few moments later,
when it'll hopefully work better.
Tested manually by running with STRECHECKDBEVERY=0 on a nontrivially
sized setup. This panics reliably before this patch, but just causes a
disconnect/reconnect now.
As foretold by the prophecy, "once the database refactor is merged, then
shall appear a request to propagate errors from the store known
throughout the land as the NamedspacedKV, and it shall be good".
This PR does two things, because one lead to the other:
- Move the leveldb specific stuff into a small "backend" package that
defines a backend interface and the leveldb implementation. This allows,
potentially, in the future, switching the db implementation so another
KV store should we wish to do so.
- Add proper error handling all along the way. The db and backend
packages are now errcheck clean. However, I drew the line at modifying
the FileSet API in order to keep this manageable and not continue
refactoring all of the rest of Syncthing. As such, the FileSet methods
still panic on database errors, except for the "database is closed"
error which is instead handled by silently returning as quickly as
possible, with the assumption that we're anyway "on the way out".
* lib/versioner: Reduce surface area
This is a refactor while I was anyway rooting around in the versioner.
Instead of exporting every possible implementation and the factory and
letting the caller do whatever, this now encapsulates all that and
exposes a New() that takes a config.VersioningConfiguration.
Given that and that we don't know (from the outside) how a versioner
works or what state it keeps, we now just construct it once per folder
and keep it around. Previously it was recreated for each restore
request.
* unparam
* wip
We incorrectly gave a too small buffer to lz4.Compress, causing it to
allocate in some cases (when the data actually becomes larger when
compressed). This then panicked when passed to the buffer pool.
This ensures a buffer that is large enough, and adds tripwires closer to
the source in case this ever pops up again. There is a test that
exercises the issue.
* gui, lib/api: Adds support for prefers-color-scheme on default theme (fixes#6115)
- Renames current default theme into a new "light" theme
- Modifies assets serving to allow getting assets from different themes
* lib/api: Serve assets from arbitrary theme when path starts with "theme-assets"
* lib/api: Moves constant out of function
* Loads light theme in browsers without support for prefers-color-scheme
* gui: Disables dark theme when printing
* Prevents repeated injection and adds support for older browsers
The CSS is always loaded if there is no support for `matchMedia`.
* Fix bufferpool puts (ref #4976)
There was a logic error in Put() which made us put all large blocks into
segment zero, where we subsequently did not look for them.
I also added a lowest threshold, as we otherwise allocate a 128KiB
buffer when we need 24 bytes for a header and such.
* wip
* wip
* wip
* wip
* wip
* wip
* wip
* wip
* wip
* wip
* smaller stress
* cap/len
* wip
* wip
This adds a certificate lifetime parameter to our certificate generation
and hard codes it to twenty years in some uninteresting places. In the
main binary there are a couple of constants but it results in twenty
years for the device certificate and 820 days for the HTTPS one. 820 is
less than the 825 maximum Apple allows nowadays.
This also means we must be prepared for certificates to expire, so I add
some handling for that and generate a new certificate when needed. For
self signed certificates we regenerate a month ahead of time. For other
certificates we leave well enough alone.
The relay and discosrv didn't use the new lib/build package, now they
do. Conversely the lib/build package wasn't aware there might be other
users and hard coded the program name - now it's set by the build
script
This adds a field `guiAddressUsed` to the system status response, that
holds the current listening address actually in use. This may be
different from the one stored in the config because it may have been
overridden by environment or command line flag.
The GUI now checks this field to see if we are listening on localhost.
If we are not, the authentication required warning is displayed,
regardless of the *configured* listening address.
This is an experiment in testing, based on the advise to always call
t.Parallel() at the start of every test. Doing so makes tests run in
parallel, which is usually faster, but also exposes package level state
and potential race conditions better.
To support this I had to redesign the CSRF manager to not be package
global, which was indeed an improvement. And tests run five times faster
now.
This splits large writes into smaller ones when using a rate limit,
making them into a legitimate trickle rather than large bursts with a
long time in between.
This is the result of:
- Changing build.go to take the protobuf version from the modules
instead of hardcoded
- `go get github.com/gogo/protobuf@v1.3.0` to upgrade
- `go run build.go proto` to regenerate our code
Assume a folder error was set due to bad ignores on the latest scan.
Previously, doing a manual rescan would result in:
1. Clearing the folder error, which schedules (immediately) an fs
watcher restart
2. Attempting to load the ignores, which fails, so we set a folder
error and bail.
3. Now the fs watcher restarts, as scheduled, so we trigger a scan.
Goto 1.
This change fixes this by not clearing the error until the error is
actually cleared, that is, if both the health check and ignore loading
succeeds.
This introduces a better set of defaults for large databases. I've
experimentally determined that it results in much better throughput in a
couple of scenarios with large databases, but I can't give any
guarantees the values are always optimal. They're probably no worse than
the defaults though.
NATSymmetricUDPFirewall actually is not NAT at all, but means the machine has a global IP address and an UDP firewall in front (RFC calls it Symmetric UDP Firewall). This is punchable fine, both theoretically and also practically in testing.
