Fix refresh() not clearing cached state when file was never attached#758
Open
ilang wants to merge 1 commit intoapache:masterfrom
Open
Fix refresh() not clearing cached state when file was never attached#758ilang wants to merge 1 commit intoapache:masterfrom
ilang wants to merge 1 commit intoapache:masterfrom
Conversation
garydgregory
requested changes
Mar 31, 2026
Member
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
-1: This PR is missing a unit test. A future change could undo this behavior without detection.
…ached AbstractFileObject.detach() guards both doDetach() and cache clearing with 'if (attached)'. This prevents clearing cached state (type, parent, children) on objects that were never attached. However, provider-specific cached fields can be populated without going through attach(): - FtpFileObject.childMap via getChildFile() -> doGetChildren() - SftpFileObject.attrs via setStat() in doListChildrenResolved() When refresh() calls detach() on such an object, the stale cached data is silently preserved, causing exists() to return incorrect results. Fix: - FtpFileObject.refresh(): clear childMap explicitly, since doDetach() is guarded by attached and may not run. - SftpFileObject.refresh(): override to clear attrs explicitly, same reason. - AbstractFileObject.detach(): move base-class cache clearing (setFileType, parent, removeChildrenCache) outside the 'if (attached)' guard as a defensive measure. Currently these fields are only populated via code paths that call attach(), but this ensures correctness if future changes introduce paths that bypass attach(). Tests: - FtpRefreshCachedStateTest: verifies exists() returns false after a file is deleted and parent is refreshed (FTP childMap scenario). - SftpRefreshCachedStateTest: verifies exists() returns false after a file is deleted and refreshed (SFTP attrs scenario).
ilang
force-pushed
the
VFS-refresh-not-clearing-cached-state
branch
from
93d09ad to
2b2f45a
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.
Summary
AbstractFileObject.detach()guards bothdoDetach()and cache clearing (setFileType(null),parent = null,removeChildrenCache()) withif (attached). This prevents clearing cached state on objects that were never attached.However, provider-specific cached fields like
FtpFileObject.childMapcan be populated without going throughattach()(e.g. viagetChildFile()→doGetChildren()). Whenrefresh()callsdetach()on such an object, the stale cached data is silently preserved, causingexists()to return incorrect results.Fix
Move cache clearing (
setFileType,parent,removeChildrenCache) outside theif (attached)guard so it always runs. ThedoDetach()call to the provider remains guarded, since providers should not be detached if they were never attached.Root cause
The
if (attached)guard was introduced in the originaldetach()implementation (2005) whenchildMapwas only populated duringattach()→doGetType(). Over the years, code paths were added that populate cached fields withoutattach()(likegetChildFile()→doGetChildren()), making the guard incorrect.Test plan
childMapis correctly cleared afterrefresh()on unattached objects