Repository navigation
Keep the HTTP listing cache independent of the first caller's detail - #2229
Merged
Merged
Conversation
Both HTTP files cached whatever shape the first caller happened to ask for,
keyed on the URL alone:
if self.use_listings_cache and url in self.dircache:
out = self.dircache[url]
else:
out = self._ls_real(url, detail=detail, **kwargs)
self.dircache[url] = out
So with use_listings_cache=True a single ls(url, detail=False) left a
list of strings in the cache, and every later caller that needs dicts got
strings instead. That is not an exotic sequence: info, find, walk, expand_path
and cat all funnel through ls(detail=True), while ordinary user code calls
ls(detail=False) on the same URL, so whichever ran first decided for both.
It surfaced as
TypeError: string indices must be integers, not 'str'
from info/find reading the cached entry, or as ls(detail=False) handing back
dicts to a caller that asked for names.
Store the detailed shape and project down for a detail=False caller. One
entry per URL as before, no extra requests, and the cached shape can no
longer depend on call order. detail=True is always a list of dicts with a
"name" key -- _ls_real builds it that way on both the normal and the
trailing-slash-redirect path -- so it is safe to make canonical.
Both copies are fixed together: the requests-based http_sync.HTTPFileSystem
had the same three lines.
martindurant
reviewed
Oct 6, 2026
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
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.
Summary
With
use_listings_cache=True, the first caller'sdetailargument decided the type that every later caller on that URL received. Onels(url, detail=False)was enough to leave a list of strings in the cache, after whichinfo,findandwalk— which all go throughls(detail=True)— got strings where dicts are required.The bug
Both HTTP files cache the listing under the URL alone, storing whichever shape the caller happened to ask for:
The current cache decision
The fix
detailis in neither the cache key nor the cached value, so it is decided once and frozen. This is not an exotic call order —info,find,walk,expand_pathandcatall funnel throughls(detail=True)(spec.py:792,spec.py:528), while ordinary user code callsls(detail=False)on the same URL. Whichever ran first decided for both.Measured against
masterat778f956, with_ls_realstubbed so no network is involved:Measured, both orders
With
use_listings_cache=Falsethe same sequence is correct, so the trigger is exactly the documented cache option.The same three lines are in
http_sync.py:340— therequests-basedHTTPFileSystemhad the identical defect, so this is one bug written twice. Both are fixed here.The fix
Store the detailed shape and project down for a
detail=Falsecaller:One cache entry per URL as before, no extra requests, and the cached shape can no longer depend on call order.
detail=Trueis safe to make canonical because_ls_realalways builds a list of dicts with a"name"key on that path — including the trailing-slash redirect, where it recurses withdetail=Falseand then re-wraps the names into dicts before returning.Validation
6 new tests — the same three cases against each implementation:
Red/green
All 6 are red without the change. The 12 that pass either way are the pre-existing listing-cache tests, and they matter here:
test_list_cache,test_list_cache_with_expiry_time_cached,..._purged,..._max_pathsand..._reuseonly ever driveh.glob(...), which reacheslsexclusively withdetail=True. Nothing in the suite exercised a mixed sequence, which is why this survived.The parametrised test asserts the type of each call rather than a snapshot, in both orders, so a future change cannot reintroduce order-dependence in either direction. A second test checks that the projected names are exactly the ones the detailed listing carries, and that
info/find/globstill work after adetail=Falselisting.Full suite
The
+6is exactly the new tests; no failures either side. I skippedtest_github_paths.pybecause it needs network access for the GitHub API. The tests use the repo's existing localserverfixture, so nothing here reaches the internet.ruff checkandruff format --checkare clean on all four files. One pre-existingFURB110finding inhttp_sync.py:149is unrelated and untouched.A separate issue I did not touch
self.dircache[url] = outalso runs whenuse_listings_cacheisFalse, andHTTPFileSystem.use_listings_cache(defaultFalse) andDirCache.use_listings_cache(defaultTrue) are different objects set from different places — so with default options everyls()writes an entry into an unbounded dict that is never consulted. That is a memory-growth issue rather than a correctness one, it would widen this diff, and picking a policy for it is your call, so I have left it alone.