At a high level, this is what I've done and why:
- I'm moving the protobuf generation for the `protocol`, `discovery` and
`db` packages to the modern alternatives, and using `buf` to generate
because it's nice and simple.
- After trying various approaches on how to integrate the new types with
the existing code, I opted for splitting off our own data model types
from the on-the-wire generated types. This means we can have a
`FileInfo` type with nicer ergonomics and lots of methods, while the
protobuf generated type stays clean and close to the wire protocol. It
does mean copying between the two when required, which certainly adds a
small amount of inefficiency. If we want to walk this back in the future
and use the raw generated type throughout, that's possible, this however
makes the refactor smaller (!) as it doesn't change everything about the
type for everyone at the same time.
- I have simply removed in cold blood a significant number of old
database migrations. These depended on previous generations of generated
messages of various kinds and were annoying to support in the new
fashion. The oldest supported database version now is the one from
Syncthing 1.9.0 from Sep 7, 2020.
- I changed config structs to be regular manually defined structs.
For the sake of discussion, some things I tried that turned out not to
work...
### Embedding / wrapping
Embedding the protobuf generated structs in our existing types as a data
container and keeping our methods and stuff:
```
package protocol
type FileInfo struct {
*generated.FileInfo
}
```
This generates a lot of problems because the internal shape of the
generated struct is quite different (different names, different types,
more pointers), because initializing it doesn't work like you'd expect
(i.e., you end up with an embedded nil pointer and a panic), and because
the types of child types don't get wrapped. That is, even if we also
have a similar wrapper around a `Vector`, that's not the type you get
when accessing `someFileInfo.Version`, you get the `*generated.Vector`
that doesn't have methods, etc.
### Aliasing
```
package protocol
type FileInfo = generated.FileInfo
```
Doesn't help because you can't attach methods to it, plus all the above.
### Generating the types into the target package like we do now and
attaching methods
This fails because of the different shape of the generated type (as in
the embedding case above) plus the generated struct already has a bunch
of methods that we can't necessarily override properly (like `String()`
and a bunch of getters).
### Methods to functions
I considered just moving all the methods we attach to functions in a
specific package, so that for example
```
package protocol
func (f FileInfo) Equal(other FileInfo) bool
```
would become
```
package fileinfos
func Equal(a, b *generated.FileInfo) bool
```
and this would mostly work, but becomes quite verbose and cumbersome,
and somewhat limits discoverability (you can't see what methods are
available on the type in auto completions, etc). In the end I did this
in some cases, like in the database layer where a lot of things like
`func (fv *FileVersion) IsEmpty() bool` becomes `func fvIsEmpty(fv
*generated.FileVersion)` because they were anyway just internal methods.
Fixes#8247
I came accross this in another context and didn't investigate fully, but
literally ten lines above this code, in another method, we say that
filesets _must_ be created under the lock. It's either one or the other
and I'm taking the safer route here.
---------
Co-authored-by: Simon Frei <freisim93@gmail.com>
### Purpose
As discussed in #9686
Syncthing currently does not check folderstate on remote device before
pulling. If no devices have a valid folderstate (i.e all devices have
the folder paused) it will still attempt to pull. On large folders this
will cause a hanging "Syncing" status.
This checks whether at least one connected device has the file available
and has a valid folderstate.
### Testing
Tested locally on multiple devices.
We're new to Go (all our stuff is Python) so please bear with!
Interested if there may be a better place to slot this in.
Thanks,
Jon
---------
Co-authored-by: Simon Frei <freisim93@gmail.com>
This should prevent the panic that occurred in this test run:
https://github.com/syncthing/syncthing/actions/runs/11095876010/job/30825046810
```
2024-09-29T21:01:53.5425372Z === RUN TestIssue4357
2024-09-29T21:01:53.5505943Z panic: runtime error: integer divide by zero [recovered]
2024-09-29T21:01:53.5512200Z panic: runtime error: integer divide by zero
2024-09-29T21:01:53.5516633Z
2024-09-29T21:01:53.5523018Z goroutine 2655 [running]:
2024-09-29T21:01:53.5524157Z github.com/thejerf/suture/v4.(*Supervisor).runService.func2.2()
2024-09-29T21:01:53.5527176Z /home/runner/go/pkg/mod/github.com/thejerf/suture/v4@v4.0.5/supervisor.go:563 +0xd0
2024-09-29T21:01:53.5530556Z panic({0x1080d20?, 0x1851290?})
2024-09-29T21:01:53.5564723Z /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.1.linux-amd64/src/runtime/panic.go:785 +0x132
2024-09-29T21:01:53.5566616Z github.com/syncthing/syncthing/lib/model.(*model).numHashers(0xc0006f6180, {0x117dc1a, 0x7})
2024-09-29T21:01:53.5568061Z /home/runner/work/syncthing/syncthing/lib/model/model.go:2581 +0x210
2024-09-29T21:01:53.5569912Z github.com/syncthing/syncthing/lib/model.(*folder).scanSubdirsChangedAndNew(0xc00c38c808, {0x0, 0x0, 0x0}, 0xc0003fc060)
2024-09-29T21:01:53.5571612Z /home/runner/work/syncthing/syncthing/lib/model/folder.go:653 +0x250
2024-09-29T21:01:53.5573010Z github.com/syncthing/syncthing/lib/model.(*folder).scanSubdirs(0xc00c38c808, {0x0, 0x0, 0x0})
2024-09-29T21:01:53.5574447Z /home/runner/work/syncthing/syncthing/lib/model/folder.go:512 +0xd0f
2024-09-29T21:01:53.5576011Z github.com/syncthing/syncthing/lib/model.(*folder).scanTimerFired(0xc00c38c808)
2024-09-29T21:01:53.5577367Z /home/runner/work/syncthing/syncthing/lib/model/folder.go:916 +0x46
2024-09-29T21:01:53.5579010Z github.com/syncthing/syncthing/lib/model.(*folder).Serve(0xc00c38c808, {0x1307650, 0xc0006a0910})
2024-09-29T21:01:53.5580428Z /home/runner/work/syncthing/syncthing/lib/model/folder.go:205 +0xd7e
2024-09-29T21:01:53.5581624Z github.com/thejerf/suture/v4.(*Supervisor).runService.func2()
2024-09-29T21:01:53.5582978Z /home/runner/go/pkg/mod/github.com/thejerf/suture/v4@v4.0.5/supervisor.go:567 +0x249
2024-09-29T21:01:53.5584400Z created by github.com/thejerf/suture/v4.(*Supervisor).runService in goroutine 2651
2024-09-29T21:01:53.5585872Z /home/runner/go/pkg/mod/github.com/thejerf/suture/v4@v4.0.5/supervisor.go:541 +0x32a
2024-09-29T21:01:53.5661413Z FAIL github.com/syncthing/syncthing/lib/model 5.510s
```
### Testing
I have not been able to reproduce the panic throughout a few minutes of
continuously running the test without this fix, but judging by the
traceback it seems to only happen if the test happens to delete the
folder from config at the same time `scanTimerFired` triggers.
I can see already in our Sentry data that there are a fair amount of
these warnings, and mostly the shape of it. Asking users to report them
will likely cause a lot of reporting effort to fairly little additional
value. We can do that when/if we have something more targeted to ask
for.
### Purpose
This PR contains the set of changes needed to make Syncthing work on iOS
for [my iOS app for
Syncthing](https://github.com/pixelspark/sushitrain).
Most changes originate from [the Mobius Sync
fork](http://github.com/MobiusSync/syncthing/tree/ios). I have removed
the changes from their fork that are not strictly needed for my app
(i.e. their changes to the GUI and command line utilities, for instance)
and squashed it all in a single commit.
In summary, the changes are:
* Resolve non-absolute paths to the 'Documents' folder (basically the
only one an app can/should write user data to by default on iOS)
* Tweaking of build flags/conditions for iOS (i.e. determine which
basicfs_watch, ignoreresult variant to build for iOS)
* Disable upgrade mechanism on iOS
* Make `RequestGlobal` and `PullerProgress` public symbols
* Expose syncthing.app's Model instance (app.M)
* Add no-op stub for SetLowPriority on iOS
I would very much appreciate these changes to be (eventually) merged to
mainline syncthing, as this would allow my iOS app to track the mainline
source code directly and removes the need (for me at least) for
maintaining a separate fork. Perhaps the Mobius folks can also benefit
from this (although as noted this branch does not contain their changes
to e.g. the GUI).
### Testing
This branch has been tested with the iOS app and appears to work fine.
The full set of MobiusSync changes has been used before with success.
### Screenshots
n/a
### Documentation
There should be no visible changes for users due to this set of changes.
---------
Co-authored-by: Simon Pickup <simon@pickupinfinity.com>
### Purpose
Avoid the issue where the folder marker is deleted by overzealous
cleanup tools because it's just a useless, empty directory.
We create a small file containing a an admonishment to not delete the
directory, and some metadata that is just for human consumption at the
moment. (But it would parse as a valid yaml file if we wanted to read
this, at some point.)
This will only apply when _creating_ a folder marker, that is, existing
setups will not gain the file automatically. Obviously, when using a
custom folder marker none of this applies.
Also, slightly adjust the permission bits for the folder marker directory and file on Unixes, making sure the group & write bits are unset.
### Testing
I've created and deleted a few folders and it appears to behave as I
expect.
### Screenshots
```
jb@ok:~/somefolder % ls -la
total 0
drwxr-xr-x 3 jb staff 96 May 1 08:52 ./
drwx------ 12 jb staff 384 May 1 08:52 ../
drwxr-xr-x 3 jb staff 96 May 1 08:52 .stfolder/
jb@ok:~/somefolder % ls -l .stfolder
total 8
-rw-r--r-- 1 jb staff 122 May 1 08:52 syncthing-folder-39a4b0.txt
jb@ok:~/somefolder % cat .stfolder/syncthing-folder-39a4b0.txt
# This directory is a Syncthing folder marker.
# Do not delete.
folderID: xtdca-cudyf
created: 2024-05-01T08:52:49+02:00
```
Adds a bool flag to `scanIfItemChanged()` to indicate when the scan was initiated from a delete function, and if so, tell `IsEquivalentOptional()` to ignore Xattrs and Ownership regardless of the global setting.
I tested this with my sledgehammer and it seems to pass.
The new test has a flakiness factor on slow platforms, where the close
on the sending connection races with the last index message, potentially
messing up the count. This adds a wait to ensure that all sent messages
are received, or the test will eventually fail with a timeout.
This makes a couple of small improvements to the folder summary
mechanism:
- The folder summary includes the local and remote sequence numbers in
clear text, rather than some odd sum that I'm not sure what it was
intended to represent.
- The folder summary event is generated when appropriate, regardless of
whether there is an event listener. We did this before because
generating it was expensive, and we wanted to avoid doing it
unnecessarily. Nowadays, however, it's mostly just reading out
pre-calculated metadata, and anyway, it's nice if it shows up reliably
when running with -verbose.
The point of all this is to make it easier to use these events to judge
when devices are, in fact, in sync. As-is, if I'm looking at two
devices, it's very difficult to reliably determine if they are in sync
or not. The reason is that while we can ask device A if it thinks it's
in sync, we can't see if the answer is "yes" because it has processed
all changes from B, or if it just doesn't know about the changes from B
yet. With proper sequence numbers in the event we can compare the two
and determine the truth. This makes testing a lot easier.
This is a refactor of the protocol/model interface to take the actual
message as the parameter, instead of the broken-out fields:
```diff
type Model interface {
// An index was received from the peer device
- Index(conn Connection, folder string, files []FileInfo) error
+ Index(conn Connection, idx *Index) error
// An index update was received from the peer device
- IndexUpdate(conn Connection, folder string, files []FileInfo) error
+ IndexUpdate(conn Connection, idxUp *IndexUpdate) error
// A request was made by the peer device
- Request(conn Connection, folder, name string, blockNo, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error)
+ Request(conn Connection, req *Request) (RequestResponse, error)
// A cluster configuration message was received
- ClusterConfig(conn Connection, config ClusterConfig) error
+ ClusterConfig(conn Connection, config *ClusterConfig) error
// The peer device closed the connection or an error occurred
Closed(conn Connection, err error)
// The peer device sent progress updates for the files it is currently downloading
- DownloadProgress(conn Connection, folder string, updates []FileDownloadProgressUpdate) error
+ DownloadProgress(conn Connection, p *DownloadProgress) error
}
```
(and changing the `ClusterConfig` to `*ClusterConfig` for symmetry;
we'll be forced to use all pointers everywhere at some point anyway...)
The reason for this is that I have another thing cooking which is a
small troubleshooting change to check index consistency during transfer.
This required adding a field or two to the index/indexupdate messages,
and plumbing the extra parameters in umpteen changes is almost as big a
diff as this is. I figured let's do it once and avoid having to do that
in the future again...
The rest of the diff falls out of the change above, much of it being in
test code where we run these methods manually...
### Purpose
This PR changes behaviour of syncthing related to `receive-only`
folders, which I believe to be a bug since I wouldn't expect the current
behaviour. With the current syncthing codebase, a file of a
`receive-only` folder that is only modified locally can cause the
creation of a `.sync-conflict` file.
### Testing
Consider this szenario: Setup two paired clients that sync a folder with
a given file (e.g. `Test.txt`). One of the clients configures the folder
to be `receive-only`. Now, change the contents of the file for the
receive-only client **_twice_**.
With the current syncthing codebase, this leads to the creation of a
`.sync-conflict` file that contains the modified contents, while the
regular `Test.txt` file is reset to the cluster's provided contents.
This is due to a `protocol.FileInfo#ShouldConflict` check, that is
succeeding on the locally modified file.
This PR changes this behaviour to not reset the file and not cause the
creation of a `.sync-conflict`. Instead, the second content update is
treated the same as the first content update.
This PR also contains a test that fails on the current codebase and
succeeds with the changes introduced in this PR.
### Screenshots
This is not a GUI change
### Documentation
This is not a user visible change.
## Authorship
Your name and email will be added automatically to the AUTHORS file
based on the commit metadata.
#### Thanks to all the syncthing folks for this awesome piece of
software!
On kqueue-systems, folders listen for folder summaries to (be able to)
warn for potential high resource usage. However, it listened for any
folder summary and not for the summary which matches the folder it's
about. This could cause that an unwatched folder causes a folder summary
containing more files than the threshold (10k), and the listening folder
(with the watcher enabled) triggers the warning.
This makes sure that only the folder summaries which are relevant to the
specific folder are being handled.
### Testing
- Fire up some kqueue-system (freebsd, I used).
- add folder A, disable the watcher, add 10001 files
- add folder B with the watcher enabled, no files are needed here
Before the change:
- add an item to folder A, trigger a rescan to speed up the process
- wait some seconds...warning triggered by folder B's
summarySubscription
After the change:
- Only a warning is triggered if the received folder summary matches the
folder which listens for the summaries
Cleanup after #9275.
This renames `fmut` -> `mut`, removes the deadlock detector and
associated plumbing, renames some things from `...PRLocked` to
`...RLocked` and similar, and updates comments.
Apart from the removal of the deadlock detection machinery, no
functional code changes... i.e. almost 100% diff noise, have fun
reviewing.
I'm tired of the fmut/pmut shenanigans. This consolidates both under one
lock; I'm not convinced there are any significant performance
differences with this approach since we're literally just protecting map
juggling...
- The locking goes away when we were already under an appropriate fmut
lock.
- Where we had fmut.RLock()+pmut.Lock() it gets upgraded to an
fmut.Lock().
- Otherwise s/pmut/fmut/.
In order to avoid diff noise for an important change I did not do the
following cleanups, which will be filed in a PR after this one, if
accepted:
- Renaming fmut to just mut
- Renaming methods that refer to being "PRLocked" and stuff like that
- Removing the no longer relevant deadlock detector
- Comments referring to pmut and locking sequences...
Before introducing the service map and using it for folder runners, the
entries in folderCfgs and folderRunners for the same key/folder were
removed under a single lock. Stopping the folder happens separately
before that with just the read lock. Now with the service map stopping
the folder and removing it from the map is a single operation. And that
still happens with just a read-lock. However even with a full lock it’s
still problematic: After the folder stopped, the runner isn’t present
anymore while the folder-config still is and sais the folder isn't
paused.
The index handler in turn looks at the folder config that is not paused,
thus assumes the runner has to be present -> nil deref on the runner.
A better solution might be to push most of these fmut maps into the
folder - they anyway are needed in there. Then there's just a single
map/source of info that's necessarily consistent. That's quite a bit of
work though, and probably/likely there will be corner cases there too.
LastSeen for a device was only updated when they connected. This now
updates it when they disconnect, so that we remember the last time we
actually saw them. When asking the API for current stats, currently
connected devices get a last seen value of the current time.
I don't really understand under what circumstances, but sometimes these
calls panic with a "panic: counter cannot decrease in value" because the
value passed to Add() was negative.
This adds the ability to have multiple concurrent connections to a single device. This is primarily useful when the network has multiple physical links for aggregated bandwidth. A single connection will never see a higher rate than a single link can give, but multiple connections are load-balanced over multiple links.
It is also incidentally useful for older multi-core CPUs, where bandwidth could be limited by the TLS performance of a single CPU core -- using multiple connections achieves concurrency in the required crypto calculations...
Co-authored-by: Simon Frei <freisim93@gmail.com>
Co-authored-by: tomasz1986 <twilczynski@naver.com>
Co-authored-by: bt90 <btom1990@googlemail.com>
Instead of separately tracking the token.
Also changes serviceMap to have a channel version of RemoveAndWait, so
that it's possible to do the removal under a lock but wait outside of
the lock. And changed where we do that in connection close, reversing
the change that happened when I added the serviceMap in 40b3b9ad1.
* lib/versioner: Factor out DefaultPath constant.
Replace several instances where .stversions is named literally to all
use the same definition in the versioner package. Exceptions are the
packages where a cyclic dependency on versioner is impossible, or some
tests which combine the versions base path with other components.
* lib/versioner: Fix comment about trash can in simple versioner.
* lib/versioner: Fix wrong versioning type string in error message.
The error message shows the folder type instead of the versioning
type, although the correct field is used in the comparison.
refactor: replace empty slice literal with `var`
An empty slice can be represented by `nil` or an empty slice literal. They are
functionally equivalent — their `len` and `cap` are both zero — but the `nil`
slice is the preferred style. For more information about empty slices,
see [Declaring Empty Slices](https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices).
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
refactor: fix unused method receiver
Methods with unused receivers can be a symptom of unfinished refactoring or a bug. To keep
the same method signature, omit the receiver name or '_' as it is unused.
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
