I'm working through linter complaints, these are some fixes. Broad
categories:
1) Ignore errors where we can ignore errors: add "_ = ..." construct.
you can argue that this is annoying noise, but apart from silencing the
linter it *does* serve the purpose of highlighting that an error is
being ignored. I think this is OK, because the linter highlighted some
error cases I wasn't aware of (starting CPU profiles, for example).
2) Untyped constants where we though we had set the type.
3) A real bug where we ineffectually assigned to a shadowed err.
4) Some dead code removed.
There'll be more of these, because not all packages are fixed, but the
diff was already large enough.
This adds a folder option "CopyOwnershipFromParent" which, when set,
makes Syncthing attempt to retain the owner/group information when
syncing files. Specifically, at the finisher stage we look at the parent
dir to get owner/group and then attempt a Lchown call on the temp file.
For this to succeed Syncthing must be running with the appropriate
permissions. On Linux this is CAP_FOWNER, which can be granted by the
service manager on startup or set on the binary in the filesystem. Other
operating systems do other things, but often it's not required to run as
full "root". On Windows this patch does nothing - ownership works
differently there and is generally less of a deal, as permissions are
inherited as ACLs anyway.
There are unit tests on the Lchown functionality, which requires the
above permissions to run. There is also a unit test on the folder which
uses the fake filesystem and hence does not need special permissions.
* lib/fs, lib/model: Improve filesystem operations during tests (fixes#5422)
Introduces MustFilesystem that panics on errors and should be used for operations
during testing which must never fail.
Create temporary directories outside of testdata.
* don't do a filesystem, just a wrapper around os for testing
* fix copyright
This avoids waiting until next ping and timeout until the connection is actually
closed both by notifying the peer of the disconnect and by immediately closing
the local end of the connection after that. As a nice side effect, info level
logging about dropped connections now have the actual reason in it, not a generic
timeout error which looks like a real problem with the connection.
In a recent change (#5201) this return disappeared. The effect is that
we first shortcut the file and then also treat it normally. This results
in to database updates after each other, which are bound to end up in
the same batch. This means we remove one sequence entry and add two.
Not marking the issues as fixed, because I need to do more testing and
there are other discrepancies...
This adds a thin type that holds the state associated with the
leveldb.DB, leaving the huge Instance type more or less stateless. Also
moves some keying stuff into the DB package so that other packages need
not know the keying specifics.
(This does not, yet, fix the cmd/stindex program, in order to keep the
diff size down. Hence the keying constants are still exported.)
* lib/model, cmd/syncthing: Wait for folder restarts to complete (fixes#5233)
This is the somewhat ugly - but on the other hand clear - fix for what
is really a somewhat thorny issue. To avoid zombie folder runners a new
mutex is introduced that protects the RestartFolder operation. I hate
adding more mutexes but the alternatives I can think of are worse.
The other part of it is that the POST /rest/system/config operation now
waits for the config commit to complete. The point of this is that until
the commit has completed we should not accept another config commit. If
we did, we could end up with two separate RestartFolders queued in the
background. While they are both correct, and will run without
interfering with each other, we can't guarantee the order in which they
will run. Thus it could happen that the newer config got committed
first, and the older config commited after that, leaving us with the
wrong config running.
* test
* wip
* hax
* hax
* unflake test
* per folder mutexes
* paranoia
* race
The problem here is that we would update the sequence index before
updating the FileInfos, which would result in a high sequence number
pointing to a low-sequence FileInfo. The index sender would pick up the
high sequence number, send the old file, and think everything was good.
On the receiving side the old file is a no-op and ignored. The file
remains out of sync until another update for it happens.
This fixes that by correcting the order of operations in the database
update: first we remove old sequence index entries, then we update the
FileInfos (which now don't have anything pointing to them) and then we
add the sequence indexes (which the index sender can see).
The other option is to add "proper" transactions where required at the
database layer. I actually have a branch for that, but it's literally
thousands of lines of diff and I'm putting that off for another day as
this solves the problem...
The problem here is that we would update the sequence index before
updating the FileInfos, which would result in a high sequence number
pointing to a low-sequence FileInfo. The index sender would pick up the
high sequence number, send the old file, and think everything was good.
On the receiving side the old file is a no-op and ignored. The file
remains out of sync until another update for it happens.
This fixes that by correcting the order of operations in the database
update: first we remove old sequence index entries, then we update the
FileInfos (which now don't have anything pointing to them) and then we
add the sequence indexes (which the index sender can see).
The other option is to add "proper" transactions where required at the
database layer. I actually have a branch for that, but it's literally
thousands of lines of diff and I'm putting that off for another day as
this solves the problem...
This removes the out of disk space check from CheckHealth. The disk space is now
only checked if there are files to pull, in which case pulling those files is
stopped, but everything else (dirs, links, deletes) keeps running -> can recover
disk space through pulling.
Adds a receive only folder type that does not send changes, and where the user can optionally revert local changes. Also changes some of the icons to make the three folder types distinguishable.
This is an improvement of PR #4493 and related to (and maybe fixing) #4961
and #4475. Maybe fixing, because there is no clear reproducer for that
problem.
The previous PR added a mechanism to resurrect missing parent directories,
if there is a valid child file to be pulled. The same mechanism does not
exist for dirs and symlinks, even though a missing parent can happen for
those items as well. Therefore this PR extends the resurrection to all types
of pulled items.
In addition I moved the IsDeleted branch while iterating over
processDirectly to the existing IsDeleted branch in the WithNeed iteration.
This saves one pointless assignment and IsDeleted query. Also
We have the invalid bit to indicate that a file isn't good. That's enough for remote devices. For ourselves, it would be good to know sometimes why the file isn't good - because it's an unsupported type, because it matches an ignore pattern, or because we detected the data is bad and we need to rescan it.
Or, and this is the main future reason for the PR, because it's a change detected on a receive only device. We will want something like the invalid flag for those changes, but marking them as invalid today means the scanner will rehash them. Hence something more fine grained is required.
This introduces a LocalFlags fields to the FileInfo where we can stash things that we care about locally. For example,
FlagLocalUnsupported = 1 << 0 // The kind is unsupported, e.g. symlinks on Windows
FlagLocalIgnored = 1 << 1 // Matches local ignore patterns
FlagLocalMustRescan = 1 << 2 // Doesn't match content on disk, must be rechecked fully
The LocalFlags fields isn't sent over the wire; instead the Invalid attribute is calculated based on the flags at index sending time. It's on the FileInfo anyway because that's what we serialize to database etc.
The actual Invalid flag should after this just be considered when building the global state and figuring out availability for remote devices. It is not used for local file index entries.
To optimize WithNeed, which is called for the local device whenever an index
update is received. No tracking for remote devices to conserve db space, as
WithNeed is only queried for completion.
I'm trying to slowly clean this up a bit, and moving functionality out
into the folder types and having those methods not reach into model is
part of it. That can mean takign some odd arguments in the meantime,
some of those should probably become interfaces or properties on folder
in the long term.
The functionality was anyway mostly implemented there and not isolated
in the folderScanner type. The attempt to refactor it out in the other
direction wouldn't work given that the event loop and stuff is on
`folder`.
To newer names better reflecting their types and yet sorting together
with folder.go. Doing it now without asking because there are no open
PRs that will get killed by it, and to avoid bikeshedding the names.
The actual pull method (which is really the only thing that differs
between them) is now an interface member which gets overridden by the
subclass.
"Subclass?!" Well, this is dynamic dispatch with overriding, I guess.
Instead of walking and unmarshalling the entire db and sorting the resulting
file infos by sequence, add store device keys by sequence number in the
database. Thus only the required file infos need be unmarshalled and are already
sorted by index.
Bumping the limit to 2 * the max block size (16 MiB) is a slight
increase compared to previously. Nonetheless I think it's good to allow
us to queue one request and have one on the way in, or conversely have
one large block on the way in and be able to ask for smaller blocks from
others at the same time.
