This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11794

status: use filesystem time boundary to invalidate racy mtime
ClosedPublic

Authored by marmoute on Nov 24 2021, 6:12 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG322525db4c98: status: use filesystem time boundary to invalidate racy mtime
Summary

We record the filesystem time at the start of the status walk and use that as a
boundary to detect files that might be modified during (or right after) the
status run without the mtime allowing that edition to be detected. We
currently do this at a second precision. In a later patch, we will use
nanosecond precision when available.

To cope with "broken" time on the file system where file could be in the
future, we also keep mtime for file over one day in the future. See inline
comment for details.

Large file tests get a bit more confused as we reduce the odds for race
condition.

As a "side effect", the win32text extension is happy again.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Nov 24 2021, 6:12 AM
Alphare requested changes to this revision.Nov 24 2021, 8:48 AM
Alphare added a subscriber: Alphare.

Please do a typo pass on the commit message as discussed in IRC

This revision now requires changes to proceed.Nov 24 2021, 8:48 AM
marmoute retitled this revision from status: use filesystem time boundary to invalidate racy files to status: use filesystem time boundary to invalidate racy mtime.Nov 30 2021, 6:54 PM
marmoute edited the summary of this revision. (Show Details)
marmoute updated this revision to Diff 31229.
Alphare accepted this revision.Dec 1 2021, 5:24 AM
This revision is now accepted and ready to land.Dec 1 2021, 5:24 AM
marmoute updated this revision to Diff 31236.Dec 1 2021, 8:21 AM

Revision Contents
Changeset List

Diff 31256

hgext/largefiles/lfutil.py

lfile, p1_tracked=True, wc_tracked=True, possibly_dirty=True lfile, p1_tracked=True, wc_tracked=True, possibly_dirty=True
) )
return lfdirstate return lfdirstate
def lfdirstatestatus(lfdirstate, repo): def lfdirstatestatus(lfdirstate, repo):
pctx = repo[b'.'] pctx = repo[b'.']
match = matchmod.always() match = matchmod.always()
unsure, s = lfdirstate.status( unsure, s, mtime_boundary = lfdirstate.status(
match, subrepos=[], ignored=False, clean=False, unknown=False match, subrepos=[], ignored=False, clean=False, unknown=False
) )
modified, clean = s.modified, s.clean modified, clean = s.modified, s.clean
wctx = repo[None] wctx = repo[None]
for lfile in unsure: for lfile in unsure:
try: try:
fctx = pctx[standin(lfile)] fctx = pctx[standin(lfile)]
except LookupError: except LookupError:
fctx = None fctx = None
if not fctx or readasstandin(fctx) != hashfile(repo.wjoin(lfile)): if not fctx or readasstandin(fctx) != hashfile(repo.wjoin(lfile)):
modified.append(lfile) modified.append(lfile)
else: else:
clean.append(lfile) clean.append(lfile)
st = wctx[lfile].lstat() st = wctx[lfile].lstat()
mode = st.st_mode mode = st.st_mode
size = st.st_size size = st.st_size
mtime = timestamp.mtime_of(st) mtime = timestamp.mtime_of(st)
cache_data = (mode, size, mtime) cache_data = (mode, size, mtime)
# We should consider using the mtime_boundary
# logic here, but largefile never actually had
# ambiguity protection before, so this confuse
# the tests and need more thinking.
lfdirstate.set_clean(lfile, cache_data) lfdirstate.set_clean(lfile, cache_data)
return s return s
def listlfiles(repo, rev=None, matcher=None): def listlfiles(repo, rev=None, matcher=None):
"""return a list of largefiles in the working copy or the """return a list of largefiles in the working copy or the
specified changeset""" specified changeset"""
if match is None or match.always(): if match is None or match.always():
# Spend a bit of time here to get a list of files we know # Spend a bit of time here to get a list of files we know
# are modified so we can compare only against those. # are modified so we can compare only against those.
# It can cost a lot of time (several seconds) # It can cost a lot of time (several seconds)
# otherwise to update all standins if the largefiles are # otherwise to update all standins if the largefiles are
# large. # large.
lfdirstate = openlfdirstate(ui, repo) lfdirstate = openlfdirstate(ui, repo)
dirtymatch = matchmod.always() dirtymatch = matchmod.always()
unsure, s = lfdirstate.status( unsure, s, mtime_boundary = lfdirstate.status(
dirtymatch, subrepos=[], ignored=False, clean=False, unknown=False dirtymatch, subrepos=[], ignored=False, clean=False, unknown=False
) )
modifiedfiles = unsure + s.modified + s.added + s.removed modifiedfiles = unsure + s.modified + s.added + s.removed
lfiles = listlfiles(repo) lfiles = listlfiles(repo)
# this only loops through largefiles that exist (not # this only loops through largefiles that exist (not
# removed/renamed) # removed/renamed)
for lfile in lfiles: for lfile in lfiles:
if lfile in modifiedfiles: if lfile in modifiedfiles:

hgext/largefiles/overrides.py

@eh.wrapfunction(scmutil, b'addremove') @eh.wrapfunction(scmutil, b'addremove')
def scmutiladdremove(orig, repo, matcher, prefix, uipathfn, opts=None): def scmutiladdremove(orig, repo, matcher, prefix, uipathfn, opts=None):
if opts is None: if opts is None:
opts = {} opts = {}
if not lfutil.islfilesrepo(repo): if not lfutil.islfilesrepo(repo):
return orig(repo, matcher, prefix, uipathfn, opts) return orig(repo, matcher, prefix, uipathfn, opts)
# Get the list of missing largefiles so we can remove them # Get the list of missing largefiles so we can remove them
lfdirstate = lfutil.openlfdirstate(repo.ui, repo) lfdirstate = lfutil.openlfdirstate(repo.ui, repo)
unsure, s = lfdirstate.status( unsure, s, mtime_boundary = lfdirstate.status(
matchmod.always(), matchmod.always(),
subrepos=[], subrepos=[],
ignored=False, ignored=False,
clean=False, clean=False,
unknown=False, unknown=False,
) )
# Call into the normal remove code, but the removing of the standin, we want # Call into the normal remove code, but the removing of the standin, we want
# o | x | o | (*) # o | x | o | (*)
# x | o | o | overwrite (as revert) # x | o | o | overwrite (as revert)
# o | o | o | (*) # o | o | o | (*)
# #
# (*) don't care # (*) don't care
# (*1) deprecated, but used internally (e.g: "rebase --collapse") # (*1) deprecated, but used internally (e.g: "rebase --collapse")
lfdirstate = lfutil.openlfdirstate(repo.ui, repo) lfdirstate = lfutil.openlfdirstate(repo.ui, repo)
unsure, s = lfdirstate.status( unsure, s, mtime_boundary = lfdirstate.status(
matchmod.always(), matchmod.always(),
subrepos=[], subrepos=[],
ignored=False, ignored=False,
clean=True, clean=True,
unknown=False, unknown=False,
) )
oldclean = set(s.clean) oldclean = set(s.clean)
pctx = repo[b'.'] pctx = repo[b'.']

hgext/largefiles/reposetup.py

def sfindirstate(f): def sfindirstate(f):
sf = lfutil.standin(f) sf = lfutil.standin(f)
dirstate = self.dirstate dirstate = self.dirstate
return sf in dirstate or dirstate.hasdir(sf) return sf in dirstate or dirstate.hasdir(sf)
match._files = [f for f in match._files if sfindirstate(f)] match._files = [f for f in match._files if sfindirstate(f)]
# Don't waste time getting the ignored and unknown # Don't waste time getting the ignored and unknown
# files from lfdirstate # files from lfdirstate
unsure, s = lfdirstate.status( unsure, s, mtime_boundary = lfdirstate.status(
match, match,
subrepos=[], subrepos=[],
ignored=False, ignored=False,
clean=listclean, clean=listclean,
unknown=False, unknown=False,
) )
(modified, added, removed, deleted, clean) = ( (modified, added, removed, deleted, clean) = (
s.modified, s.modified,
else: else:
if listclean: if listclean:
clean.append(lfile) clean.append(lfile)
s = wctx[lfile].lstat() s = wctx[lfile].lstat()
mode = s.st_mode mode = s.st_mode
size = s.st_size size = s.st_size
mtime = timestamp.mtime_of(s) mtime = timestamp.mtime_of(s)
cache_data = (mode, size, mtime) cache_data = (mode, size, mtime)
# We should consider using the mtime_boundary
# logic here, but largefile never actually had
# ambiguity protection before, so this confuse
# the tests and need more thinking.
lfdirstate.set_clean(lfile, cache_data) lfdirstate.set_clean(lfile, cache_data)
else: else:
tocheck = unsure + modified + added + clean tocheck = unsure + modified + added + clean
modified, added, clean = [], [], [] modified, added, clean = [], [], []
checkexec = self.dirstate._checkexec checkexec = self.dirstate._checkexec
for lfile in tocheck: for lfile in tocheck:
standin = lfutil.standin(lfile) standin = lfutil.standin(lfile)

hgext/remotefilelog/__init__.py

f2 = actionargs[0] f2 = actionargs[0]
files.append((f2, hex(mctx.filenode(f2)))) files.append((f2, hex(mctx.filenode(f2))))
# batch fetch the needed files from the server # batch fetch the needed files from the server
repo.fileservice.prefetch(files) repo.fileservice.prefetch(files)
return orig(repo, wctx, mctx, force, mresult, *args, **kwargs) return orig(repo, wctx, mctx, force, mresult, *args, **kwargs)
# Prefetch files before status attempts to look at their size and contents # Prefetch files before status attempts to look at their size and contents
def checklookup(orig, self, files): def checklookup(orig, self, files, mtime_boundary):
repo = self._repo repo = self._repo
if isenabled(repo): if isenabled(repo):
prefetchfiles = [] prefetchfiles = []
for parent in self._parents: for parent in self._parents:
for f in files: for f in files:
if f in parent: if f in parent:
prefetchfiles.append((f, hex(parent.filenode(f)))) prefetchfiles.append((f, hex(parent.filenode(f))))
# batch fetch the needed files from the server # batch fetch the needed files from the server
repo.fileservice.prefetch(prefetchfiles) repo.fileservice.prefetch(prefetchfiles)
return orig(self, files) return orig(self, files, mtime_boundary)
# Prefetch the logic that compares added and removed files for renames # Prefetch the logic that compares added and removed files for renames
def findrenames(orig, repo, matcher, added, removed, *args, **kwargs): def findrenames(orig, repo, matcher, added, removed, *args, **kwargs):
if isenabled(repo): if isenabled(repo):
files = [] files = []
pmf = repo[b'.'].manifest() pmf = repo[b'.'].manifest()
for f in removed: for f in removed:

mercurial/context.py

): ):
self._repo.ui.debug( self._repo.ui.debug(
b'ignoring suspect symlink placeholder "%s"\n' % f b'ignoring suspect symlink placeholder "%s"\n' % f
) )
continue continue
sane.append(f) sane.append(f)
return sane return sane
def _checklookup(self, files): def _checklookup(self, files, mtime_boundary):
# check for any possibly clean files # check for any possibly clean files
if not files: if not files:
return [], [], [] return [], [], [], []
modified = [] modified = []
deleted = [] deleted = []
clean = []
fixup = [] fixup = []
pctx = self._parents[0] pctx = self._parents[0]
# do a full compare of any files that might have changed # do a full compare of any files that might have changed
for f in sorted(files): for f in sorted(files):
try: try:
# This will return True for a file that got replaced by a # This will return True for a file that got replaced by a
# directory in the interim, but fixing that is pretty hard. # directory in the interim, but fixing that is pretty hard.
if ( if (
f not in pctx f not in pctx
or self.flags(f) != pctx.flags(f) or self.flags(f) != pctx.flags(f)
or pctx[f].cmp(self[f]) or pctx[f].cmp(self[f])
): ):
modified.append(f) modified.append(f)
elif mtime_boundary is None:
clean.append(f)
else: else:
# XXX note that we have a race windows here since we gather
# the stats after we compared so the file might have
# changed.
#
# However this have always been the case and the
# refactoring moving the code here is improving the
# situation by narrowing the race and moving the two steps
# (comparison + stat) in the same location.
#
# Making this code "correct" is now possible.
s = self[f].lstat() s = self[f].lstat()
mode = s.st_mode mode = s.st_mode
size = s.st_size size = s.st_size
mtime = timestamp.mtime_of(s) file_mtime = timestamp.mtime_of(s)
fixup.append((f, (mode, size, mtime))) cache_info = (mode, size, file_mtime)
file_second = file_mtime[0]
boundary_second = mtime_boundary[0]
# If the mtime of the ambiguous file is younger (or equal)
# to the starting point of the `status` walk, we cannot
# garantee that another, racy, write will not happen right
# after with the same mtime and we cannot cache the
# information.
#
# However is the mtime is far away in the future, this is
# likely some mismatch between the current clock and
# previous file system operation. So mtime more than one days
# in the future are considered fine.
if (
boundary_second
<= file_second
< (3600 * 24 + boundary_second)
):
clean.append(f)
else:
fixup.append((f, cache_info))
except (IOError, OSError): except (IOError, OSError):
# A file become inaccessible in between? Mark it as deleted, # A file become inaccessible in between? Mark it as deleted,
# matching dirstate behavior (issue5584). # matching dirstate behavior (issue5584).
# The dirstate has more complex behavior around whether a # The dirstate has more complex behavior around whether a
# missing file matches a directory, etc, but we don't need to # missing file matches a directory, etc, but we don't need to
# bother with that: if f has made it to this point, we're sure # bother with that: if f has made it to this point, we're sure
# it's in the dirstate. # it's in the dirstate.
deleted.append(f) deleted.append(f)
return modified, deleted, fixup return modified, deleted, clean, fixup
def _poststatusfixup(self, status, fixup): def _poststatusfixup(self, status, fixup):
"""update dirstate for files that are actually clean""" """update dirstate for files that are actually clean"""
poststatus = self._repo.postdsstatus() poststatus = self._repo.postdsstatus()
if fixup or poststatus or self._repo.dirstate._dirty: if fixup or poststatus or self._repo.dirstate._dirty:
try: try:
oldid = self._repo.dirstate.identity() oldid = self._repo.dirstate.identity()
# Even if the wlock couldn't be grabbed, clear out the list. # Even if the wlock couldn't be grabbed, clear out the list.
self._repo.clearpostdsstatus() self._repo.clearpostdsstatus()
def _dirstatestatus(self, match, ignored=False, clean=False, unknown=False): def _dirstatestatus(self, match, ignored=False, clean=False, unknown=False):
'''Gets the status from the dirstate -- internal use only.''' '''Gets the status from the dirstate -- internal use only.'''
subrepos = [] subrepos = []
if b'.hgsub' in self: if b'.hgsub' in self:
subrepos = sorted(self.substate) subrepos = sorted(self.substate)
cmp, s = self._repo.dirstate.status( cmp, s, mtime_boundary = self._repo.dirstate.status(
match, subrepos, ignored=ignored, clean=clean, unknown=unknown match, subrepos, ignored=ignored, clean=clean, unknown=unknown
) )
# check for any possibly clean files # check for any possibly clean files
fixup = [] fixup = []
if cmp: if cmp:
modified2, deleted2, fixup = self._checklookup(cmp) modified2, deleted2, clean_set, fixup = self._checklookup(
cmp, mtime_boundary
)
s.modified.extend(modified2) s.modified.extend(modified2)
s.deleted.extend(deleted2) s.deleted.extend(deleted2)
if clean_set and clean:
s.clean.extend(clean_set)
if fixup and clean: if fixup and clean:
s.clean.extend((f for f, _ in fixup)) s.clean.extend((f for f, _ in fixup))
self._poststatusfixup(s, fixup) self._poststatusfixup(s, fixup)
if match.always(): if match.always():
# cache for performance # cache for performance
if s.unknown or s.ignored or s.clean: if s.unknown or s.ignored or s.clean:

mercurial/dirstate.py

elif subrepos: elif subrepos:
use_rust = False use_rust = False
elif sparse.enabled: elif sparse.enabled:
use_rust = False use_rust = False
elif not isinstance(match, allowed_matchers): elif not isinstance(match, allowed_matchers):
# Some matchers have yet to be implemented # Some matchers have yet to be implemented
use_rust = False use_rust = False
# Get the time from the filesystem so we can disambiguate files that
# appear modified in the present or future.
try:
mtime_boundary = timestamp.get_fs_now(self._opener)
except OSError:
# In largefiles or readonly context
mtime_boundary = None
if use_rust: if use_rust:
try: try:
return self._rust_status( res = self._rust_status(
match, listclean, listignored, listunknown match, listclean, listignored, listunknown
) )
return res + (mtime_boundary,)
except rustmod.FallbackError: except rustmod.FallbackError:
pass pass
def noop(f): def noop(f):
pass pass
dcontains = dmap.__contains__ dcontains = dmap.__contains__
dget = dmap.__getitem__ dget = dmap.__getitem__
# This can happen if we quickly do multiple commits. # This can happen if we quickly do multiple commits.
# Force lookup, so we don't miss such a racy file change. # Force lookup, so we don't miss such a racy file change.
ladd(fn) ladd(fn)
elif listclean: elif listclean:
cadd(fn) cadd(fn)
status = scmutil.status( status = scmutil.status(
modified, added, removed, deleted, unknown, ignored, clean modified, added, removed, deleted, unknown, ignored, clean
) )
return (lookup, status) return (lookup, status, mtime_boundary)
def matches(self, match): def matches(self, match):
""" """
return files in the dirstate (in whatever state) filtered by match return files in the dirstate (in whatever state) filtered by match
""" """
dmap = self._map dmap = self._map
if rustmod is not None: if rustmod is not None:
dmap = self._map._map dmap = self._map._map

mercurial/narrowspec.py

oldincludes, oldexcludes = parseconfig(repo.ui, oldspec) oldincludes, oldexcludes = parseconfig(repo.ui, oldspec)
newincludes, newexcludes = parseconfig(repo.ui, newspec) newincludes, newexcludes = parseconfig(repo.ui, newspec)
oldmatch = match(repo.root, include=oldincludes, exclude=oldexcludes) oldmatch = match(repo.root, include=oldincludes, exclude=oldexcludes)
newmatch = match(repo.root, include=newincludes, exclude=newexcludes) newmatch = match(repo.root, include=newincludes, exclude=newexcludes)
addedmatch = matchmod.differencematcher(newmatch, oldmatch) addedmatch = matchmod.differencematcher(newmatch, oldmatch)
removedmatch = matchmod.differencematcher(oldmatch, newmatch) removedmatch = matchmod.differencematcher(oldmatch, newmatch)
ds = repo.dirstate ds = repo.dirstate
lookup, status = ds.status( lookup, status, _mtime_boundary = ds.status(
removedmatch, subrepos=[], ignored=True, clean=True, unknown=True removedmatch, subrepos=[], ignored=True, clean=True, unknown=True
) )
trackeddirty = status.modified + status.added trackeddirty = status.modified + status.added
clean = status.clean clean = status.clean
if assumeclean: if assumeclean:
clean.extend(lookup) clean.extend(lookup)
else: else:
trackeddirty.extend(lookup) trackeddirty.extend(lookup)

tests/test-dirstate-race.t

$ cat >> $TESTTMP/dirstaterace.py << EOF $ cat >> $TESTTMP/dirstaterace.py << EOF
> from mercurial import ( > from mercurial import (
> context, > context,
> extensions, > extensions,
> ) > )
> def extsetup(ui): > def extsetup(ui):
> extensions.wrapfunction(context.workingctx, '_checklookup', overridechecklookup) > extensions.wrapfunction(context.workingctx, '_checklookup', overridechecklookup)
> def overridechecklookup(orig, self, files): > def overridechecklookup(orig, self, *args, **kwargs):
> # make an update that changes the dirstate from underneath > # make an update that changes the dirstate from underneath
> self._repo.ui.system(br"sh '$TESTTMP/dirstaterace.sh'", > self._repo.ui.system(br"sh '$TESTTMP/dirstaterace.sh'",
> cwd=self._repo.root) > cwd=self._repo.root)
> return orig(self, files) > return orig(self, *args, **kwargs)
> EOF > EOF
$ hg debugrebuilddirstate $ hg debugrebuilddirstate
$ hg debugdirstate $ hg debugdirstate
n 0 -1 unset a n 0 -1 unset a
n 0 -1 unset b n 0 -1 unset b
n 0 -1 unset d n 0 -1 unset d
n 0 -1 unset dir1/c n 0 -1 unset dir1/c