Repository navigation
Conversation
* fix: register for the flush wakeup before checking inode state SyncFile checked flushPending and waited on flusherCond without holding the inode lock, so a flush could complete between the state check and the wait and the wakeup would be missed. The sync then blocks until some unrelated flush happens to signal, or forever if none does. Take the inode lock to read the state, acquire flusherMu before releasing it, and only then wait, so completion cannot slip through the gap. Cherry-picked from geesefs fbf052a, with its test. Our fork predates it. The test fails against our current code with a one-second timeout and passes with the fix. (cherry picked from commit fbf052a89e2bc0ba21d7ca8db8b2d0e8b5f4d0a1) * fix: bound the part count instead of indexing past the last part tier completeMultipart derived the part count by taking partNum of the final size and incrementing when the part started below it. At a size that exactly fills the configured tiers that asks partRange for one part beyond the last, which panics with "Part number too large". Introduce numParts and maxParts, and use them, so the count saturates at the maximum instead of running off the end. Cherry-picked from geesefs 8fdec0d, with its tests; the module path in the test is retargeted. Note this supersedes their earlier attempt at the same bug (a5cc92e), which loosened the comparison in partRange and was reverted here for allowing out-of-range parts. We have neither, so only this one should be taken. Not reachable at ordinary file sizes: it needs a file that exactly fills every part tier. (cherry picked from commit 8fdec0de5a9c4b1e0f2b3d4c5a6b7c8d9e0f1a2b) * fix: bound read retries instead of retrying forever by default The read retry loop treats an attempt limit below one as unlimited, and the flag defaulted to zero, so a persistently failing read retried without end and the operation never returned an error to the caller. Default the limit to ten and reject a value below one at startup, matching the validation style already used for part sizes in the same function. Cherry-picked from geesefs ae2c4ed. Behaviour change worth calling out at review: unlimited retries were previously reachable by passing zero, and that now aborts at startup rather than being silently accepted. Anyone deliberately running unlimited has to change the flag. The unlimited branch itself stays in the read loop, since the S3 backend still lowers and restores the limit around its own calls. (cherry picked from commit ae2c4edfcb1a2b3c4d5e6f708192a3b4c5d6e7f8) * fix: do not take a lookup reference for . and .. in readdirplus The readdirplus path referenced every entry it wrote, including the . and .. entries. The kernel does not count those toward nlookup, so each listing leaked a reference on the directory and its parent, and those inodes were never forgotten. Skip the reference for the two self entries. Cherry-picked from geesefs bbedeb0. Our fork predates it. (cherry picked from commit bbedeb027a1b2c3d4e5f60718293a4b5c6d7e8f9) * fix: identify . and .. by directory position, not inode identity The backported reference fix compares the entry against dh.inode and dh.inode.Parent, but ReadDir releases the parent lock before returning, so a rename can change dh.inode.Parent in between. The comparison then misses, the .. entry is referenced, and no forget ever matches it: the leak the fix targets. ReadDir yields . and .. at internal offsets 0 and 1, and dh.Next only advances the position after this block, so the position still identifies this entry and is held under dh.mu throughout. It is also correct for the root, where ReadDir returns the directory itself for .. because there is no parent. Divergence from geesefs bbedeb0 rather than a port; worth sending upstream, which carries the same window. Raised in review on #51.
Startup autodetection sends an unsigned HEAD at the bucket. A 200 means the bucket answers unauthenticated requests, and the code took that, plus the absence of a profile and of AWS_ACCESS_KEY_ID in the environment, as proof the caller wanted anonymous access, and swapped the client onto anonymous credentials. With --iam neither of those is set, because credentials come from the metadata service instead. A mount configured for IAM against a publicly readable bucket therefore dropped to unauthenticated requests without saying so: every subsequent call carried no identity, so writes fail and reads are attributed to nobody. Require that IAM is not in use before inferring anonymous access, and say in the --no-detect help text that anonymous detection is one of the things it turns off, which the old wording omitted. Cherry-picked from geesefs a47ba58. Our fork predates it. (cherry picked from commit a47ba5804b1c2d3e4f5a6b7c8d9e0f1a2b3c4d5e)
…owned uploads (#53) Two defects on the server-side copy path. A self-copy is used only to rewrite metadata, so the code assumed it is cheap regardless of object size and always issued a single CopyObject. Some S3 implementations apply the single-copy size limit to a self-copy too and answer EntityTooLarge, which failed the metadata update outright. Retry those as a multipart copy, keeping the single request wherever the backend allows it. Separately, the multipart copy helper created an upload and returned on error without aborting it. The caller cannot clean up either, because the upload id is never returned on failure, so every failed copy leaked an upload that counts against storage until a lifecycle rule reaps it. The helper now aborts what it created, and leaves caller-supplied uploads alone. Cherry-picked from geesefs b3a2cec, with its tests; the module path in the test is retargeted and our structured logging is kept over upstream's formatted call. Reverting the fix fails three of the six sub-tests. (cherry picked from commit b3a2cec67d8e9f0a1b2c3d4e5f6a7b8c9d0e1f2a)
#54) * fix: make the slurp gap cache work, and remember when a listing ran off the end Lookups on a directory whose stat cache has expired go to the server unless the root's loaded-range list says the key was already covered by a listing. That list has never held anything: markGapLoaded computed the final slice length one short, so the range it inserted was truncated away every time. With the list always empty, every such lookup issued a ListObjects and a HEAD. This dates to the commit that introduced slurp-on-lookup in 2022 and is the same upstream. Once ranges are kept, a second defect surfaces for the reported workload. A writer that keeps creating names sorting after every key on the server makes each lookup list from that name; the listing returns nothing, and the range was recorded up to the last key returned, or as empty. Nothing after it was ever known, so the next name listed again. Record a listing that ran off the end as open-ended instead, using a sentinel that sorts after any UTF-8 key. markGapLoaded is rewritten to keep the list sorted and disjoint by trimming overlapping ranges to their parts outside the new one, which also stops a narrow re-list inside an open-ended range from discarding what is known past its end. Tests cover insertion, overlap trimming, splitting a containing range, the end-of-listing sentinel, and expiry. On the previous code the first assertion fails: marking (a,m] leaves the list empty. * fix: actually emit loaded ranges in the inode debug dump The dump built a map per range and never appended it, so the gaps field was always empty. Append it, and render the end-of-listing sentinel readably. * test: lookups past the last listed key must not list again End-to-end version of the reported symptom: a directory with three files, an expired stat cache, and nine lookups of names sorting after the last key. Every lookup used to cost two ListObjects and a HEAD, because the listing that found nothing past the name was never remembered as covering what follows. Counted through a TestBackend wrapper against a live Tigris bucket. Before the fix the listing count went 3 -> 21 over the nine lookups; after it, the later lookups add no listings and no HEADs. * fix: an explicit refresh must forget the loaded ranges too With the loaded-range cache now actually holding entries, LookUp serves a name from cache whenever a fresh range covers it. The .invalidate refresh reset the subtree's DirTime and AttrTime but left those ranges alone, so the lookup it provoked returned the very entries it had just expired: after a remote overwrite and a refresh, a file still read its old contents. The four notify-refresh tests caught it. resetDirTimeRec now drops every range the root remembers before resetting times. Over-invalidation is deliberate: a refresh is explicit and rare, and the cost is one listing per lookup until ranges are re-learned. The recursion is split into an inner function so the ranges are dropped once, not per node. * fix: drop loaded ranges at the invalidation entry points, not inside the recursive reset resetDirTimeRec took the root lock to drop the loaded ranges, but mount calls it while holding the parent's lock. For a mount directly under the root that is the same mutex, which is not reentrant, so the mount hung and left the root locked against every later lookup. For a nested mount it took the root lock while holding a child's, inverting the parent-before-child order and racing a concurrent lookup for a deadlock. Keep the recursive reset lock-free as it was. Drop the ranges at each entry point instead, where no inode lock is held: RefreshInodeCache, ResetForUnmount, and mount before it takes the parent's lock. dropLoadedRanges documents that it takes only the root lock and must be called with none held. Raised in review on #54.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+342
to
+346
| loadedEnd := nextStartAfter | ||
| if seal || !resp.IsTruncated { | ||
| loadedEnd = gapEndOfListing | ||
| } | ||
| parent.dir.markGapLoaded(NilStr(startWith), loadedEnd) |
There was a problem hiding this comment.
If a directory listing is in flight during an explicit cache refresh, the refresh clears the root’s loaded ranges, but the older listing can finish afterward and record a fresh range. If an object was created after that listing’s snapshot, later lookups in the range return the cached absence instead of checking storage. The refresh therefore fails to reveal the object until the range expires.
Knowledge Base Used: Filesystem objects and operations
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 271f945. Configure here.
| param.StorageClass = s.selectStorageClass(param.Size) | ||
| } | ||
|
|
||
| if !s.gcs && *param.Size > s.config.MultipartCopyThreshold { |
There was a problem hiding this comment.
Fallback copy drops caller ETag
Medium Severity
The EntityTooLarge self-copy fallback always HEADs when StorageClass is unset and then replaces the caller-supplied ETag. Metadata flushes pass knownETag so the multipart copy can If-Match that version; after the overwrite, part copies pin whatever is live now. A remote resize or replace is applied with local metadata instead of returning ERANGE and discarding the stale update.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 271f945. Configure here.
|
🎉 This PR is included in version 1.2.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Note
Medium Risk
Changes default read-retry behavior and core lookup/cache invalidation paths; S3 copy and fsync synchronization fixes reduce failure modes but touch hot paths.
Overview
Fixes directory slurp/gap caching so listings are actually remembered:
markGapLoadedno longer drops inserted ranges, non-truncated listings record an open-ended sentinel (gapEndOfListing) so lookups past the last key avoid repeat List/HEAD storms, overlapping ranges merge/split correctly, anddropLoadedRangesclears root gaps on cache refresh, unmount, and nested mount.S3 backend: metadata self-copies that hit
EntityTooLargeretry via multipart copy; owned multipart copies are aborted on failure; anonymous autodetection is skipped whenUseIAMis enabled.File I/O: shared
numPartslogic for multipart completion at tier boundaries;SyncFileholds the inode lock while deciding whether to wait on the flusher so completed flushes are not missed.Defaults/UX:
--read-retry-attemptsdefaults to 10 (minimum 1); FUSE readdirPlus skips extra refs for./..; debug output labels open-ended gaps.Extensive unit/e2e tests cover gaps, copy self-fallback, part counts, and sync.
Reviewed by Cursor Bugbot for commit 271f945. Bugbot is set up for automated code reviews on this repo. Configure here.