No locking when updating files in ~/.bazaar

Bug #525571 reported by Jean-Paul Calderone on 2010-02-22
72
This bug affects 9 people
Affects Status Importance Assigned to Milestone
Bazaar
High
Vincent Ladeuil
Bazaar Subversion Plugin
High
Vincent Ladeuil
Launchpad itself
High
Jelmer Vernooij
bzr (Ubuntu)
Undecided
Unassigned
Lucid
Undecided
Unassigned
Maverick
Undecided
Unassigned

Bug Description

exarkun@boson:/tmp$ svnadmin create foo
exarkun@boson:/tmp$ bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
Initialising Subversion metadata cache in /home/exarkun/.cache/bazaar/svn/261e6e09-138e-40a9-b6e2-6d67e07423dd.
Using repository layout: root
Use 'bzr checkout' to create a working tree in the newly created branches.
exarkun@boson:/tmp$ bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
Using repository layout: root
Use 'bzr checkout' to create a working tree in the newly created branches.
exarkun@boson:/tmp$ bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
Using repository layout: root
Use 'bzr checkout' to create a working tree in the newly created branches.
exarkun@boson:/tmp$ ls foo-bzr/
exarkun@boson:/tmp$ bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr &bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr &bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
[1] 19190
[2] 19191
Using repository layout: root
Using repository layout: root
bzr: ERROR: Error(s) parsing config file /home/exarkun/.bazaar/subversion.conf:
Invalid line at line "3".
Duplicate section name at line 5.
Duplicate keyword name at line 6.
Use 'bzr checkout' to create a working tree in the newly created branches.
Use 'bzr checkout' to create a working tree in the newly created branches.
[1]- Exit 3 bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
[2]+ Done bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
exarkun@boson:/tmp$ cat /home/exarkun/.bazaar/subversion.conf
[261e6e09-138e-40a9-b6e2-6d67e07423dd]
locations = file:///tmp/foo
istedmatrix.com/svn/Twisted
guessed-layout = trunk0
[261e6e09-138e-40a9-b6e2-6d67e07423dd]

Related branches

Jelmer Vernooij (jelmer) wrote :

I'm not sure what the best way is to fix this. I guess we'll have to start locking ~/.bazaar/subversion.conf.

Changed in bzr-svn:
status: New → Triaged
importance: Undecided → Medium
Michael Hudson-Doyle (mwhudson) wrote :

This is one of the few things that requires manual intervention in the import service these days, so can I request a bump in priority?

Changed in launchpad-code:
status: New → Triaged
importance: Undecided → High
Dimitri John Ledkov (xnox) wrote :

lp:gcc lp:gcc/4.5 lp:gcc/4.4 lp:gcc/4.3 are affected.

There quite a few failures with 4.3. and 4.4 getting stuck for 12 hours on incremental import =(

http://launchpadlibrarian.net/46133501/mingw-w64-gcc-4.4.log

Jelmer Vernooij (jelmer) wrote :

See bzrlib/config.py:514

        # FIXME: RBC 20051029 This should refresh the parser and also take a
        # file lock on bazaar.conf.

Changed in bzr:
status: New → Confirmed
importance: Undecided → High
Changed in bzr-svn:
importance: Medium → High
Dimitri John Ledkov (xnox) wrote :

Jelmer Vernooij is it currently broken?

  File "/home/pqm/for_rollouts/production/eggs/bzr-2.1.0-py2.5-linux-x86_64.egg/bzrlib/config.py", line 360, in _get_parser
bzrlib.errors.ParseConfigError: Error(s) parsing config file /home/importd/.bazaar/subversion.conf:
Invalid line at line "575".
Duplicate keyword name at line 576.
Duplicate keyword name at line 577

that's on lp:gcc branches.....

Dimitri John Ledkov (xnox) wrote :

bzrlib.errors.ParseConfigError: Error(s) parsing config file /home/importd/.bazaar/subversion.conf:
Invalid line at line "398".
Import failed:
Traceback (most recent call last):

again gcc...

Tres Seaver (tseaver) wrote :
Download full text (4.0 KiB)

I don't know if this is the same bug, but there seems to be another concurrency-related issue showing up in LPs svn imports. the 'zope.password' import is now failing[1] with the tracceback:

2010-05-10 17:06:46 INFO Starting job.
2010-05-10 17:06:46 INFO Getting exising bzr branch from central store.
2010-05-10 17:06:57 INFO 35 bytes transferred |
2010-05-10 17:06:59 INFO
2010-05-10 17:06:59 WARNING Upgrade to svn 1.5 or higher for faster retrieving of revision properties.
Traceback (most recent call last):
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/scripts/code-import-worker.py", line 82, in <module>
    sys.exit(script.main())
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/scripts/code-import-worker.py", line 77, in main
    return import_worker.run()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/lib/lp/codehosting/codeimport/worker.py", line 437, in run
    return self._doImport()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/lib/lp/codehosting/codeimport/worker.py", line 571, in _doImport
    foreign_branch_tip = foreign_branch.last_revision()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/branch.py", line 501, in last_revision
    last_revmeta, mapping = self.last_revmeta()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/branch.py", line 256, in last_revmeta
    for revmeta, mapping in self._revision_meta_history():
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/util.py", line 142, in next
    return self._next()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/util.py", line 124, in _next
    ret = self._iterator.next()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/repository.py", line 825, in _iter_reverse_revmeta_mapping_history
    (mapping, lhs_mapping) = revmeta.get_appropriate_mappings(mapping)
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 344, in get_appropriate_mappings
    original = self.get_original_mapping()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 843, in get_original_mapping
    self._original_mapping = self.base.get_original_mapping()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 372, in get_original_mapping
    revprops_acceptable=revprops_acceptable)
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 683, in _import_from_props
    if revprops_acceptable(revprops):
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 367, in revprops_acceptable
    return revprops.get(SVN_REVPROP_BZR_ROOT) == self.branch_path
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/util.py", line 67, in get
    self._ensure_init()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/util.py", l...

Read more...

Tim Penhey (thumper) on 2010-05-11
tags: added: code-import

On 10 May 2010 18:12, Tres Seaver <email address hidden> wrote:
> I don't know if this is the same bug, but there seems to be another
> concurrency-related issue showing up in LPs svn imports.  the
> 'zope.password' import is now failing[1] with the tracceback:

Yeap gcc imports were failing with that as well before subversion.conf
was adjusted to include all imports from that svn root id.

Now it "sometimes" fails with a different error as mentioned above =)

@Michael yes, you can. The fix is in three parts:
 a) make sure that we don't have any code paths that set the entire conf without reading it immediately and none that edit the file in place.
 b) add a LockDir around the read-edit-write component of changing the config file (possibly around all edits to ~/.bazaar in fact). I suggest ~/.bazaar/lock as the path of the lock.
 c) We'll need some docs to say that there is a lock being taken, and how to release it if bzr is kill -9'd while holding it. This may mean making 'break-lock' accept the url of a lock as well as the current url of a control dir. Or something.

Tom Haddon (mthaddon) on 2010-05-28
tags: added: canonical-losa-lp
Tom Haddon (mthaddon) wrote :

Got another corruption of .bazaar/subversion.conf on pear just now

Jelmer Vernooij (jelmer) on 2010-06-03
summary: - bzr svn-import fails when invoked concurrently
+ bzrlib.config.IniBasedConfig doesn't lock

On Fri, Jun 4, 2010 at 10:02 AM, Jelmer Vernooij <email address hidden> wrote:
> ** Summary changed:
>
> - bzr svn-import fails when invoked concurrently
> + bzrlib.config.IniBasedConfig doesn't lock

I don't think thats really the problem, the problem is that when we
use that for locations.conf, that we don't lock around it. Fixing this
by making IniBasedConfig lock would be a mistake - it would knock on
into nearly all of our configs - branches, repos, etc.

Fair enough - updated.

summary: - bzrlib.config.IniBasedConfig doesn't lock
+ LocationsConfig doesn't lock around bzrlib.config.IniBasedConfig
Jelmer Vernooij (jelmer) on 2010-06-04
summary: - LocationsConfig doesn't lock around bzrlib.config.IniBasedConfig
+ No locking when updating files in ~/.bazaar

Can you please stop changing the title every hour? xD

On Fri, Jun 4, 2010 at 2:02 PM, Jelmer Vernooij <email address hidden> wrote:

> ** Summary changed:
>
> - LocationsConfig doesn't lock around bzrlib.config.IniBasedConfig
> + No locking when updating files in ~/.bazaar
>
> --
> No locking when updating files in ~/.bazaar
> https://bugs.launchpad.net/bugs/525571
> You received this bug notification because you are a direct subscriber
> of a duplicate bug.
>

--
marcos garcía // <email address hidden>

Vincent Ladeuil (vila) on 2010-06-04
tags: added: udd
Martin Pool (mbp) on 2010-06-07
Changed in bzr:
milestone: none → 2.2.0
Vincent Ladeuil (vila) on 2010-06-24
Changed in bzr:
assignee: nobody → Vincent Ladeuil (vila)
status: Confirmed → In Progress
Vincent Ladeuil (vila) wrote :

Can anyone involved in this bug attached a corrupted subversion.conf file please ?

Jelmer Vernooij (jelmer) wrote :
Vincent Ladeuil (vila) wrote :

Thanks jelmer, now does someone have a corrupted one ?

Robert Collins (lifeless) wrote :

Vincent, the corruption isn't a damaged file, its a file without
content that another concurrent writer added.

Tim Penhey (thumper) wrote :

On Mon, 28 Jun 2010 13:01:10 you wrote:
> Vincent, the corruption isn't a damaged file, its a file without
> content that another concurrent writer added.

Actually I think it is a damaged file as the config reader cannot read the new
one.

Observations have shown it intermingles the lines, so instead of having:

[heading1]
value=some value
[heading2]
value=some other value

we end up with

[heading1]
[heading2]
value=some value
value=some other value

and the reader blows up with duplicate keys.

Robert Collins (lifeless) wrote :

Thanks for clarifying, thats really interesting - and even more worrying :)

-Rob

Vincent Ladeuil (vila) wrote :

>>>>> "Tim" == Tim Penhey <email address hidden> writes:

    Tim> On Mon, 28 Jun 2010 13:01:10 you wrote:
    >> Vincent, the corruption isn't a damaged file, its a file without
    >> content that another concurrent writer added.

Doesn't match the case reported in the bug description.

Well, if by 'file without content' you mean an empty section (aka only
the section header), it matches, but there is no code in configobj to
back this hypothesis (AFAICS, and if it was that may be in a bug in
bzr-svn, but I dont think so).

    Tim> Actually I think it is a damaged file as the config reader
    Tim> cannot read the new one.

More like it.

    Tim> Observations have shown it intermingles the lines, so instead
    Tim> of having:

I like the intermingled lines cause, thanks for giving weight to it :)

    Tim> [heading1]
    Tim> value=some value
    Tim> [heading2]
    Tim> value=some other value

    Tim> we end up with

    Tim> [heading1]
    Tim> [heading2]
    Tim> value=some value
    Tim> value=some other value

    Tim> and the reader blows up with duplicate keys.

But this example still presents complete sections which the reported one
doesn't:

[261e6e09-138e-40a9-b6e2-6d67e07423dd]
locations = file:///tmp/foo
istedmatrix.com/svn/Twisted
guessed-layout = trunk0
[261e6e09-138e-40a9-b6e2-6d67e07423dd]

Here, only the section header is written without any content to put
into (and *that* doesn't make sense).

One can imagine the first process writing its section, the second one
writing its own (explaining that the same section is created twice) and
then the first process truncating the file while closing it and doing so
losing part of the second process section... and finally a third process
crashing while trying to read the result

A bit far fetched though...

Especally when the python code to write such a file is:

            h = open(self.filename, 'wb')
            h.write(output)
            h.close()

'wb' line-buffered ? Huh ? Anyone with a live chicken to spare ?

Anyway, the fix I'm working on should cover this case, but still, seeing
a corrupted file may help reaching a better understanding.

Steve McInerney (spm) wrote :

Died again. Sample:

importd@pear:~/.bazaar$ tail subversion.conf.SAVE
locations = https://pidgin-birthday.svn.sourceforge.net/svnroot/pidgin-birthday
[a60eed84-fc3d-0410-a09c-0d1a7a6707c9]
locations = http://creoleparser.googlecode.com/svn
guessed-layout = trunk0
[cda61777-01e9-0310-a592-d414129be87e]
locations = svn://svn.tartarus.org/sgt
guessed-layout = root
52-e9e23c5afa6a]
locations = svn://svn.alioth.debian.org/svn/pkg-voip
guessed-layout = trunk1

Steve McInerney (spm) wrote :

attached a copy of the corrupted subversion.conf file; vila/rob wanting to check for power of 2 issues.

Vincent Ladeuil (vila) wrote :

@Steve: thanks.

I checked, but that's inconclusive. It doesn't invalidate the fix either anyway.

Vincent Ladeuil (vila) wrote :

A minimal fix that should work around the problem will be part of bzr-2.1.3 when it become available.
A more complete fix is still discussed for bzr.dev.

Changed in bzr:
milestone: 2.2.0 → 2.1.3
status: In Progress → Fix Released
Vincent Ladeuil (vila) on 2010-07-10
Changed in bzr-svn:
status: Triaged → In Progress
assignee: nobody → Vincent Ladeuil (vila)
Jelmer Vernooij (jelmer) on 2010-07-26
Changed in bzr-svn:
status: In Progress → Fix Committed
milestone: none → 1.0.3
status: Fix Committed → In Progress
Jelmer Vernooij (jelmer) on 2010-07-26
Changed in bzr-svn:
status: In Progress → Fix Committed
Jelmer Vernooij (jelmer) on 2010-07-30
Changed in bzr-svn:
status: Fix Committed → Fix Released
Changed in launchpad-code:
assignee: nobody → Jelmer Vernooij (jelmer)
milestone: none → 10.08
tags: added: qa-needstesting
Changed in launchpad-code:
status: Triaged → Fix Committed
Ursula Junque (ursinha) wrote :

This was tested by mwhudson on staging.

tags: added: qa-ok
removed: qa-needstesting
Changed in launchpad-code:
status: Fix Committed → Fix Released
Vincent Ladeuil (vila) wrote :

A more robust fix has landed in bzr.dev, it implements a real lock for config files in ${BZR_HOME}: bazaar.conf and locations.conf.
This is done with a LockableConfig class that can now be used by bzr-svn if needed.

Vincent Ladeuil (vila) wrote :

Dear ~ubuntu-sru, we'd like to get this into lucid-updates.
Per <https://wiki.ubuntu.com/StableReleaseUpdates/MicroReleaseExceptions>, we have an exception to take all of 2.1.3, conditional on running the tests during the package build and on running the tests in 2.1.3.
Some of those will fail in 2.1.3, but they should be fixed in 2.1.4.
For now I propose we build the package, then check there are only the expected failures in the package.

Vincent Ladeuil (vila) on 2010-09-24
tags: added: sru
Vincent Ladeuil (vila) on 2010-10-07
Changed in bzr (Ubuntu):
status: New → Fix Released
Martin Pitt (pitti) wrote :

Reopening Maverick task. As per Vincent this is fixed in 2.2.1, but maverick has 2.2.0.

Changed in bzr (Ubuntu Lucid):
status: New → Triaged
Changed in bzr (Ubuntu Maverick):
status: Fix Released → Triaged
Vincent Ladeuil (vila) on 2010-10-07
Changed in bzr (Ubuntu Maverick):
status: Triaged → Confirmed

Accepted bzr into maverick-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Changed in bzr (Ubuntu Maverick):
status: Confirmed → Fix Committed
tags: added: verification-needed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package bzr - 2.2.1-0ubuntu1

---------------
bzr (2.2.1-0ubuntu1) maverick-proposed; urgency=low

  * New upstream release.
   + Fix regression with upgrading poor-root to rich-root formats. LP: #636930
   + Files in ~/.bazaar are now locked whilst being updated automatically.
     LP: #525571
 -- Max Bowsher <email address hidden> Thu, 30 Sep 2010 09:57:15 +0100

Changed in bzr (Ubuntu Maverick):
status: Fix Committed → Fix Released
Martin Pitt (pitti) wrote :

Natty has 2.3.0 beta 2, assuming that it's fixed there.

Changed in bzr (Ubuntu):
status: Confirmed → Fix Released
Vincent Ladeuil (vila) on 2011-04-04
tags: added: config
Clint Byrum (clint-fewbar) wrote :

Accepted bzr into lucid-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Changed in bzr (Ubuntu Lucid):
status: Triaged → Fix Committed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package bzr - 2.1.4-0ubuntu1

---------------
bzr (2.1.4-0ubuntu1) lucid-proposed; urgency=low

  * Update watch file to use 2.1 series.
  * New upstream release.
   + Fix file descriptors leaks in dirstate compiled extension. LP: #583486
   + Refuse to stack on repositories in incompatible formats. LP: #562380
   + Don't delete nested trees/repos. LP: #572098
   + Fix 'bzr switch' crash when a 'ConfigurableFileMerger' is used. LP: #559436
   + Fix compatibility with older smart servers. LP: #528041
   + Fix symlinks addition. LP: #192859
   + Properly unversion children of unversioned directories. LP: #494221
   + Lock configuration files in '~/.bazaar' for updates. LP: #525571
   + Fix 'bzr commit <symlink>'. LP: #128562
   + Fix `lp:` urls when behind an http proxy. LP: #558343
   + Stop using edge.launchpad.net. LP: #583667
 -- Max Bowsher <email address hidden> Tue, 17 May 2011 09:54:17 +0100

Changed in bzr (Ubuntu Lucid):
status: Fix Committed → Fix Released
Martin Packman (gz) on 2011-11-14
tags: added: affects-twisted
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Related questions