Skip to content

fix: stop re-listing on every lookup of a name past the last known key - #54

Merged
efirs merged 5 commits into
mainfrom
fix/slurp-gap-end
Sep 29, 2026
Merged

efirs merged 5 commits into
mainfrom
fix/slurp-gap-end

Conversation

@efirs

@efirs efirs commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Symptom

With a writer creating small files whose names sort after everything already in the directory, tigrisfs keeps issuing ListObjects. Once the directory's stat cache expires, each lookup of the next new name goes back to the server, and it never stops.

Reproduced against a live Tigris bucket with a counting backend wrapper: three files in a directory, stat cache expired, then nine lookups of names past the last key. Listings went from 3 to 21, two per lookup, plus a HEAD each. With this change the later lookups add no listings and no HEADs.

Cause

Two defects in the slurp gap cache, the per-root list of key ranges a previous listing already covered, which LookUp consults before going to the server.

The cache has never stored anything. markGapLoaded computed the final slice length one short, so the range it inserted was truncated away on every call. Marking (a, m] into an empty list left it empty. With the list always empty, every lookup on an expired directory listed and HEADed regardless of what earlier listings had established. This dates to the 2022 upstream commit that introduced slurp-on-lookup, has no test there or here, and is identical in geesefs today.

Even with ranges stored, running off the end was not remembered. A listing from name N that returned nothing proves nothing follows N, but the range was recorded as empty, or bounded at the last key returned when there was one. So the very next name, sorting after N, was not covered and listed again. That is the appending-writer case in the report.

Change

  • markGapLoaded is rewritten to keep the list sorted and disjoint. Overlapping ranges are trimmed to the parts outside the new one and keep their own load time, so a narrow re-list inside an open-ended range does not discard what is known past its end.
  • A listing that ran off the end (not truncated, or sealed) is recorded up to a sentinel that sorts after any UTF-8 key, so every later name is covered until the range ages out at the stat cache TTL.
  • The inode debug dump built a map per range and never appended it, so the gaps field was always empty. Fixed in passing, since it is how you would observe this in the field.

checkGapLoaded and the lookup path are unchanged. The exclusive start is deliberate: StartAfter never returns the key it starts from, so the looked-up name itself is not covered by its own listing. That is pre-existing and separate; a lookup of a name that a prior listing started at still falls through to a HEAD.

Tests

  • Unit tests for the range list: insertion, overlap trimming, splitting a containing range, the end-of-listing sentinel across UTF-8 extremes, and TTL eviction. On the previous code the first assertion fails outright.
  • An end-to-end GoofysTest counting listings and HEADs through the TestBackend wrapper. Run against nrt: passes with the fix, fails without it with lists went 3 -> 21. It also passes in-process against nrt over FUSE; the first CI run exercised it against s3proxy.

Upstream geesefs carries both defects unchanged and is worth a report.


Note

Medium Risk
Changes core metadata caching and lookup behavior (including post-refresh correctness); wrong gap logic could hide remote creates until TTL or cause extra S3 traffic, but behavior aligns with existing stat-cache TTL semantics.

Overview
Fixes the slurp gap cache so LookUp can skip redundant ListObjects/HEAD when a prior listing already covered a key range.

markGapLoaded was broken (final slice length off by one), so loaded ranges were never retained; listObjectsSlurp now records open-ended ranges with a gapEndOfListing sentinel when a listing is complete or sealed, so “nothing after this name” is cached instead of re-listing on every later name.

markGapLoaded is rewritten to maintain sorted, disjoint ranges and split overlaps without dropping knowledge past a narrow re-list inside a wider range.

Cache invalidation clears those ranges via dropLoadedRanges on refresh, unmount, and mount (before subtree reset), so stale negative/positive answers are not served after .invalidate. The inode debug gaps dump now appends entries and labels the sentinel.

Unit tests cover range bookkeeping; an e2e test asserts repeated lookups past the last key do not increase list/HEAD counts.

Reviewed by Cursor Bugbot for commit 8b6d9ce. Bugbot is set up for automated code reviews on this repo. Configure here.

Refresh interaction, found by CI

With the range list now populated, the first CI run failed three notify-refresh tests: overwrite a file remotely, set .invalidate, read it back, and get the old contents. The refresh reset the subtree's DirTime and AttrTime but left the loaded ranges alone, so the lookup it provoked was served from a still-fresh range and never went to the server. The ranges are now dropped at each invalidation entry point, at a moment when no inode lock is held: RefreshInodeCache, ResetForUnmount, and mount before it takes the parent's lock (9c099a1, then 8b6d9ce after review found that dropping them inside the recursive reset deadlocked a mount over an existing directory, since mount calls it with the parent's lock held). Verified locally in-process against nrt: with only the gap fix the two notify tests fail on exactly the CI assertion; with the refresh fix all four pass alongside the listing-count test.

Visibility of files created by other clients

An open-ended range means a file another client creates past a completed listing is not seen by lookups until the range ages out, and the same holds for a name inside a bounded range. That is the expected behaviour of the stat cache TTL and what it was designed for: a range records that a listing covered those keys at a point in time, and until it ages past --stat-cache-ttl (30 s default) lookups inside it are answered from cache, positive or negative, exactly as LookUpCached already answers "not found" from a fresh directory cache. A listing of the directory or an explicit .invalidate picks the file up sooner.

It looks new only because it was broken before: with the range list always empty, every lookup on an expired directory went to the server, which defeated the negative caching the TTL is meant to provide and caused the listing storm this PR fixes. Workloads that need faster visibility of remote changes tune the TTL, at the cost of the listings this change removes.

…ff 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.
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.
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.
@greptile-apps

greptile-apps Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Changes how directory listing cache tracks loaded ranges.

The PR appears safe to merge; no new actionable issue or outstanding previous finding was established.

Summary

The PR repairs slurp-gap range storage, records completed listings through the end of the keyspace, and clears loaded ranges during explicit cache invalidation.

  • Adds range-bookkeeping and lookup-count regression tests.
  • Fixes the inode dump’s gap entries.
  • Moves mount-time range clearing outside the parent inode lock, addressing the previously reported deadlock.

Reviews (3) · Last reviewed commit: "fix: drop loaded ranges at the invalidat..."

Comment thread core/dir.go
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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9c099a1. Configure here.

Comment thread core/dir.go
Comment thread core/dir.go
…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.
@efirs
efirs merged commit 271f945 into main Sep 29, 2026
9 of 10 checks passed
@efirs
efirs deleted the fix/slurp-gap-end branch September 29, 2026 01:10
@tigrisdata-argocd-bot

Copy link
Copy Markdown

🎉 This PR is included in version 1.2.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants