Bound the hydration wait and close the open() registration race - #64
Open
toxicphreAK wants to merge 3 commits into
Open
Bound the hydration wait and close the open() registration race#64toxicphreAK wants to merge 3 commits into
toxicphreAK wants to merge 3 commits into
Conversation
toxicphreAK
force-pushed
the
split/hydration-wait
branch
from
August 27, 2026 09:06
6d12fe3 to
300fe19
Compare
This was referenced Aug 31, 2026
Member
|
This looks very good to me in general, but I think it needs rebasing after the other PRs were merged, there seems to be some overlap in |
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.
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.
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
split/hydration-wait
branch
from
September 2, 2026 18:18
300fe19 to
001473f
Compare
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
2 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.
Split out of #54. Stacked on #61 — the first of the three commits is that PR, so review the last two here and merge #61 first.
Two problems in the wait loop of
openVFSfuse_open().The job was registered by the socket thread only after the send succeeded, while
open()started polling 10 ms afterPostMsg()— which only queues. Miss that window and absence-from-map was read as failure:ENOENTfor a file that plainly exists and is mid-hydration. That's the "first open fails, second works" flakiness. The job is now inserted before the message is posted, and a send failure is published as an explicitFailedstate instead of inferred from a missing entry.The backoff also had no wall-clock ceiling — it grew by roughly the golden ratio per step, so
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 andwaitForJob()blocks on it with a deadline, configurable viahydrationTimeoutSeconds(default 300). A late reply is picked up the moment it lands rather than after the rest of a sleep interval.Failure returns are now
EIO(client reported an error) andETIMEDOUT(no answer).ENOENTreads as "the file is gone" and sends people looking in the wrong place.Incidental:
_transfer_idis nowstd::atomic— FUSE dispatches from several threads and it was incremented unguarded, so two concurrent opens could share a job entry. Config parsing tolerates the missing key, so an older config keeps working.Test. These bugs are timing and framing dependent, so the last commit adds
socketthreadtest: it stands in for the desktop client on a realAF_UNIXsocket and drives the actualSocketThread/SharedMap— split message, 8 KB reply, batched replies, stream still in sync afterwards, 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. It covers both this PR and #61, which is why it sits here rather than there.Also 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 shydrationTimeoutSeconds: 3ETIMEDOUTat 3.08 sOne thing I could not verify: the registration race is fixed by construction — inserting the job before posting makes the window unobservable — but I didn't build a test that reproduces the original failure, since it depends on losing a scheduler race.