Fix hydration race, socket framing, and cross-desktop ignore defaults - #54
Closed
toxicphreAK wants to merge 6 commits into
Closed
Fix hydration race, socket framing, and cross-desktop ignore defaults#54toxicphreAK wants to merge 6 commits into
toxicphreAK wants to merge 6 commits into
Conversation
name(PinStates) returned the strings belonging to name(States) for its first two cases, and inverted at that: OnlineOnly printed as "hydrated" and AlwaysLocal as "dehydrated". The only caller is the openvfs_stat diagnostic tool, which prints state and pin state side by side -- so the tool integrators reach for when pin state handling misbehaves reported the exact opposite of the truth. The two vocabularies are now distinguishable at a glance as well. Fixes #4
readSocket() read at most 1023 bytes per call and left message reassembly to a FIXME. The socket is a SOCK_STREAM and carries no message boundaries, so any reply longer than the buffer -- or merely split across two reads by the kernel -- was parsed as two independent messages. Both halves then failed to parse, the original reply was lost, and the waiting open() sat out its full backoff. Worse, the effect is self-sustaining: once the stream desynchronises every subsequent message on the connection is misparsed, so a single long reply broke hydration until restart. A V2/HYDRATE_FILE_RESULT carrying a path plus a free-form arguments.error string clears 1 KB without trying. processSocketInput() now drains the socket into a persistent buffer, dispatches only complete newline terminated messages, and keeps the remainder for the next read. handleReceivedMsg() correspondingly handles a single message and no longer splits on newlines itself, which also retires its trailing-empty-fragment case. The buffer is bounded so a peer that never sends a newline cannot grow it without limit. Fixes #2
Two problems in the wait loop of openVFSfuse_open(), both on the hottest path in the project. First, the job was registered by the socket thread only after the send succeeded, while open() started polling the map 10 ms after PostMsg() -- which merely queues. If the socket thread had not been scheduled and had not completed its write() within that window, the lookup missed, and absence-from-map was read as failure: open() returned ENOENT for a file that plainly exists and was in the middle of being hydrated. That is the "the first open fails, the second works" flakiness. The job is now inserted before the message is posted, so no waiter can observe the absence of a job it just posted, and a send failure is published explicitly as a Failed state instead of being inferred from a missing entry -- two conditions the old code collapsed into one. Second, the backoff grew by roughly the golden ratio per step with no wall-clock ceiling: MaxCnt of 20 bounded the number of polls, not the time. By iteration 9 a single sleep was ~36 s, and a wedged client left open() blocked in uninterruptible sleep for what is effectively forever, with the calling application unkillable. Late replies also waited out a whole sleep interval before being noticed, so latency was dominated by poll granularity rather than by the download. SharedMap now carries a condition variable and SharedMap::waitForJob() blocks on it with a deadline: a result is observed the moment the socket thread publishes it, and the wait is bounded in seconds rather than in polls. The timeout is configurable via hydrationTimeoutSeconds, since a large file over a slow link is legitimately slow while an unresponsive client is not. The failure returns are now honest as well: EIO when the client reports an error, ETIMEDOUT when it does not answer. Applications and users both read ENOENT as "the file is gone", which sent us looking in the wrong place. Also made _transfer_id atomic. FUSE dispatches from several threads and the counter was incremented unguarded, so two concurrent opens could be handed the same id and share one job entry -- the same class of bug, noticed while fixing the above. Config parsing now tolerates missing keys so that an older config file keeps working. Fixes #1 Fixes #3
This was referenced Aug 18, 2026
The shipped config blocked only KDE components from triggering a hydration, so on GNOME, Cinnamon, MATE or Xfce nothing was blocked at all: nautilus and its thumbnailer helpers, tracker/localsearch, gvfsd and the shell search providers open dehydrated files as a matter of routine. Browsing a folder downloaded everything in it and indexing downloaded the entire sync root -- silently, and on what is probably the most common Linux desktop. The list now covers the common file managers, the freedesktop thumbnailer convention (a "thumbnailer" suffix catches the -thumbnailer binaries and ffmpegthumbnailer alike), the GNOME/tracker and localsearch indexers, gvfs helpers, locate's updatedb and ClamAV. /usr/bin/dolphin moved from byName to an endsWith entry: matching the binary name rather than an absolute path holds across distributions that install elsewhere. This is a starting point rather than an exhaustive list -- see INTEGRATION.md. Enumerating every indexer, thumbnailer and antivirus in existence does not converge, so a permit-list of the few applications that should be allowed to trigger a download may scale better; that is a design decision for the maintainers and is deliberately not attempted here. The errno returned to a blocked caller is likewise left at EPERM. Fixes #5
src/openvfs/ is one library in one directory but gave three different
licensing answers: four files GPL-3.0-or-later, openvfs.cpp
GPL-2.0-or-later, and three files in src/openvfsfuse/ with no SPDX tag
at all. libopenvfs is what downstream sync clients link against, so its
license determines the license of the combined binary they ship --
anyone doing that assessment got a different answer depending on which
file they opened first.
openvfs.cpp is aligned with the other four and with the top-level
LICENSE, the missing tags are added to main.cpp and sharedmap.{h,cpp}
(which already carried the full GPL-3 notice in prose), and the tag in
strtools.h moves above the include guard like everywhere else.
socketthread.{h,cpp} are deliberately left alone: they carry third-party
MIT code by David Lafreniere alongside the GPL-3 notice, and pinning a
single machine-readable expression on that combination is the
maintainers' call, not a drive-by fix. A REUSE.toml would make the whole
tree checkable with `reuse lint` and is worth doing as a follow-up.
Fixes #6
The bugs fixed in this branch are timing and framing dependent, which is exactly the kind that comes back unnoticed. The test stands in for the desktop client on a real AF_UNIX socket and drives the actual SocketThread and SharedMap. It covers a message split across two writes, a reply far larger than any single read buffer, several replies batched into one write, the stream still being in sync afterwards, a silent client timing out within its deadline, a late reply being observed without a growing backoff, and PostMsg reporting a message dropped during shutdown. Verified to fail against the pre-fix framing code. The repository had ctest wired up but no tests, so this also gives the existing "Run tests" CI step something to run.
toxicphreAK
force-pushed
the
fix/integration-findings
branch
from
August 18, 2026 14:36
10d650f to
0096111
Compare
This was referenced Aug 20, 2026
Member
|
Thank you very much for this work. I took a first view and all looks very good to me. Please give us some more time for review. |
TheOneRing
requested changes
Aug 24, 2026
TheOneRing
left a comment
TheOneRing
left a comment
Collaborator
There was a problem hiding this comment.
Please split this pr.
There are many unrelated changes in this pr, and they should be discussed in separate pull requests.
Comment on lines
+87
to
90
| return "onlineonly"; | ||
| case PinStates::AlwaysLocal: | ||
| return "dehydrated"; | ||
| return "alwayslocal"; | ||
| case PinStates::Inherited: |
Collaborator
There was a problem hiding this comment.
This is a breaking change, without many benefits.
This was referenced Aug 27, 2026
Author
|
Split as requested. Rebased on current main, each one builds:
The test covers both the framing and the timeout, so it sits in #64 rather than in #61. I dropped the One thing worth flagging: the Closing this in favour of the four above. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six fixes found while integrating openVFS into the Nextcloud desktop client. One commit per issue; they can be taken separately.
Hydration path (
openvfsfuse.cpp,sharedmap.*)open()intermittently returnedENOENTfor files that exist. The job was registered by the socket thread after the send succeeded, whileopen()started polling 10 ms afterPostMsg()— which only queues. Miss that window and absence-from-map was read as failure. The job is now inserted before the message is posted, and a send failure is published as an explicitFailedstate rather than inferred from a missing entry.MaxCnt = 20bounded the number of polls, not the time. By iteration 9 a single sleep was ~36 s, and a wedged client leftopen()in uninterruptible sleep indefinitely.SharedMapnow carries a condition variable;waitForJob()blocks on it with a deadline, configurable viahydrationTimeoutSeconds(default 300).EIO(client reported an error) andETIMEDOUT(no answer).ENOENTreads as "the file is gone" and sends people looking in the wrong place.Protocol (
socketthread.cpp)readSocket()read 1023 bytes with no reassembly. On aSOCK_STREAM, a reply longer than the buffer — or merely split across two reads — was parsed as two messages, both failed, and the reply was lost. The desync was self-sustaining, so one longarguments.errorstring broke hydration until restart. A persistent buffer now dispatches only complete newline-terminated messages and keeps the remainder.Defaults and diagnostics
ignoreAppswas KDE-only, so on GNOME/Cinnamon/MATE/Xfce nothing was blocked: browsing a folder downloaded everything in it, indexing downloaded the sync root. The default now covers the common file managers, the freedesktop thumbnailer convention, tracker/localsearch, gvfs helpers,updatedband ClamAV.name(PinStates)returned thename(States)strings, inverted —OnlineOnlyprinted as"hydrated",AlwaysLocalas"dehydrated". Its only caller isopenvfs_stat, the tool integrators reach for when pin-state handling misbehaves.src/openvfs/gave three different licensing answers for one library, andlibopenvfsis what downstream clients link against. Aligned toGPL-3.0-or-later, plus the three missing headers.Tests
ctestwas wired up but had no tests, so the existing "Run tests" CI step had nothing to run.socketthreadteststands in for the desktop client on a realAF_UNIXsocket and drives the actualSocketThread/SharedMap: split messages, an 8 KB reply, batched replies, stream resync, timeout bounded by wall clock, prompt pickup of a late reply, andPostMsgreporting a drop during shutdown. Verified to fail against the pre-fix framing code; clean under ThreadSanitizer.Separately, and not part of the diff, the whole path was exercised against a real FUSE mount with a stub client on the socket API, opening an actual dehydrated placeholder:
open()succeeds, 4096 bytes, 0.27 sarguments.errorEIOin 0.26 s (the old 1023-byte read desynced the stream here)hydrationTimeoutSeconds: 3ETIMEDOUTat 3.08 signoreAppsEPERM, and zero hydration requests reach the clientWhat I could not verify
ignoreAppslist was checked against the thumbnailers actually installed on my machine (GNOME): all four/usr/share/thumbnailersentries are covered, includinggnome-thumbnail-font, which does not match the-thumbnailerconvention. The KDE, Xfce, Cinnamon and MATE entries are from documentation and are unverified against a live install — worth a second pair of eyes from someone running those.open()registration race is fixed by construction: inserting the job before posting makes the window unobservable. I did not build a test that reproduces the original failure, since it depends on losing a scheduler race.name(PinStates)is covered by inspection only; there is no test foropenvfs_statoutput.Deliberately left to you
ignoreAppsstays a deny-list. Enumerating every indexer, thumbnailer and antivirus does not converge; a permit-list of what should trigger a download is a smaller, more stable set — but that is a design decision.EPERM. WhetherENODATAor serving placeholder content behaves better across thumbnailers has real tradeoffs.socketthread.{h,cpp}keep no SPDX tag: they carry third-party MIT code alongside the GPL-3 notice, and pinning one expression on that is yours to decide. AREUSE.tomlwould make the tree checkable withreuse lint.Incidental
_transfer_idis nowstd::atomic— FUSE dispatches from several threads and it was incremented unguarded, so two concurrent opens could share one job entry. Config parsing tolerates missing keys, so an older config file keeps working.Each finding is written up in detail in the commit messages. Happy to split this into separate PRs, or to file the findings as issues here first, if that suits your workflow better.