Merge lp:~mbp/bzr/220464-stale-locks into lp:bzr

[Martin Pool]

optionally detect and steal dead locks from the same machine and user (bug 220464)

bzr automatically detects stale locks originating from other processes on the same machine.

This is a somewhat old branch, but I think it's actually all finished, aside from not handling detection of live/dead processes on Windows. There was a list thread about that but it looks a bit nontrivial, so I'd like to handle that separately.

I think the config variable needs to be renamed to match our current convention.

[Martin Pool (mbp)]

A few more points here:

* In general, this won't apply to hpss smart server locks, because they are supposed to be held by the server process. (Doing so should make it more likely they'll be cleaned up if the client abruptly disconnects.) In that case you see something like this:

mbp@grace% ./bzr push
Using saved push location: lp:~mbp/bzr/220464-stale-locks
Unable to obtain lock held by <email address hidden>
at crowberry [process #27519], acquired 0 seconds ago.
See "bzr help break-lock" for more.
bzr: ERROR: Could not acquire lock "(remote lock)": bzr+ssh://bazaar.launchpad.net/~mbp/bzr/220464-stale-locks/

* We could do an interactive "do you want to break this" ui but that's out of scope.

* In related bug 257217 I'm quite inclined to say a crash-only design will be safer and simpler: if something tries to kill bzr just let it happen and we'll break the lock next time.

* I should test this interactively too....

[Martin Pool (mbp)]

This is a little tricky to hit interactively from the command line, but you can get it pretty well from the Python shell:

>>> b.lock_write()
Unable to obtain lock file:///home/mbp/bzr/work/ held by Martin Pool <email address hidden>
at grace [process #7658], acquired 42 seconds ago.
Will continue to try until 10:35:52, unless you press Ctrl-C.
See "bzr help break-lock" for more.
^CTraceback (most recent call last):
....
KeyboardInterrupt

now release lock from another window

>>> b.lock_write()
Stole lock file:///home/mbp/bzr/work/.bzr/branches/220464-stale-locks/.bzr/branch/lock from dead process LockHeldInfo({'nonce': u'gxkq1gpt3t36aketelkt', 'start_time': u'1306456480', 'hostname': u'grace', 'pid': u'7658', 'user': u'Martin Pool <email address hidden>'}).
BranchWriteLockResult(0uqc649u5zeziqwzni06, <bound method BzrBranch7.unlock of BzrBranch7(file:///home/mbp/bzr/work/.bzr/branches/220464-stale-locks/)>)

One thing that demonstrates is that it should probably print the dead process info in a somewhat more baked form...

[Andrew Bennetts (spiv)]

I like this a lot. You seem to have taken a lot of care with the various possible cases, and you've done some nice refactoring and tidying too.

My only real question is if it's appropriate for is_lock_holder_known_dead to raise if os.kill raises something other than ESRCH or EPERM? On one hand other errnos shouldn't happen, at least on Linux, but even if they do aborting the bzr command entirely is probably a bit unfriendly to the user. Emitting a warning and then returning False might be better?

Apart from that I just have unimportant pedantry:

In test_auto_break_stale_lock:

646 + self.assertRaises(errors.LockBroken,
647 + l1.unlock)

That would fit comfortably on just one line :)

Ditto in test_auto_break_stale_lock_configured_off.

713 +* Information about held lockdir locks returned from eg `LockDir.peek` is
714 + now represented as a `LockHeldInfo` object, rather than a plain Python
715 + dict.

I think to be pendantically correct ReST you want double-backticks for an inline literal. Single backticks means interpreted text with the default role, which I think is unset in our sphinx conf. Hmm, I suppose it would be reasonable to set it to something that automatically links to our API docs, I think pydoctor provides an “api” role for this…

review: Approve

[Andrew Bennetts (spiv)]

I agree btw that the output isn't particularly pretty. Something formatted more like the usual LockContention output would be nicer. But I'd much rather have this with ugly output than not have it at all.

[Martin Pool (mbp)]

> I like this a lot. You seem to have taken a lot of care with the various
> possible cases, and you've done some nice refactoring and tidying too.

Thanks!

> My only real question is if it's appropriate for is_lock_holder_known_dead to
> raise if os.kill raises something other than ESRCH or EPERM? On one hand
> other errnos shouldn't happen, at least on Linux, but even if they do aborting
> the bzr command entirely is probably a bit unfriendly to the user. Emitting a
> warning and then returning False might be better?

Historically we have not tended to do that; I think Rob argued that these warnings are not normally going to be checked by tests and so could either cause false passes, or could have latent problems where the warning is emitted all the time.

However in this case I think users would almost certainly not thank us for having bzr stop there unnecessarily. If we do want to test it, there are ways.

> I think to be pendantically correct ReST you want double-backticks for an
> inline literal. Single backticks means interpreted text with the default
> role, which I think is unset in our sphinx conf. Hmm, I suppose it would be
> reasonable to set it to something that automatically links to our API docs, I
> think pydoctor provides an “api” role for this…

I think we should do that. Using double backticks for shell or computer output, and single for API references seems like the best way to spend our backtick budget. (Unless we want to get all docbooky and specifically mark roles on computer input and output, but that seems too longwinded.)

[Martin Packman (gz)]

See <lp:~gz/bzr/220464-stale-locks> for some changes built on this branch. It includes fixes for some trivial things, and an implementation (well, two really, the pywin32 ones needs testing) of process deadness detection for windows.

The current os.kill detection gets broken pretty thoroughly by Python 2.7 which includes a function of that name on windows that doesn't behave anything like the posix one. Currently my branch doesn't have any fallback (`lambda pid: False` would be reasonable) as posix+windows should cover everything.

review: Needs Fixing

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/27/2011 02:06 AM, Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/220464-stale-locks into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #220464 in Bazaar: "Bazaar doesn't detect its own stale locks"
> https://bugs.launchpad.net/bzr/+bug/220464
>
> For more details, see:
> https://code.launchpad.net/~mbp/bzr/220464-stale-locks/+merge/62582
>
> bzr automatically detects stale locks originating from other processes on the same machine.
>
> This is a somewhat old branch, but I think it's actually all finished, aside from not handling detection of live/dead processes on Windows. There was a list thread about that but it looks a bit nontrivial, so I'd like to handle that separately.
>
> I think the config variable needs to be renamed to match our current convention.

Unfortunately on newer Python's os.kill *is* available on windows, but
doesn't work like you want it to. So we probably need a "sys.platform ==
'windows'" check as well.
+ if getattr(os, 'kill', None) is None:
+ # Probably not available on Windows.
+ # XXX: How should we check for process liveness there?
+ return False

 review: needsfixing

Otherwise it looks good to me.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3fW3wACgkQJdeBCYSNAAP/IwCeJ2ATSFX6BWF7zWoTxA9ztlV7
jMAAnRiKL5tYKqJsOtt7h9o7duA/zqjB
=fBtG
-----END PGP SIGNATURE-----

review: Needs Fixing

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/27/2011 04:29 AM, Martin [gz] wrote:
> Review: Needs Fixing
> See <lp:~gz/bzr/220464-stale-locks> for some changes built on this branch. It includes fixes for some trivial things, and an implementation (well, two really, the pywin32 ones needs testing) of process deadness detection for windows.
>
> The current os.kill detection gets broken pretty thoroughly by Python 2.7 which includes a function of that name on windows that doesn't behave anything like the posix one. Currently my branch doesn't have any fallback (`lambda pid: False` would be reasonable) as posix+windows should cover everything.
>
>

I thought that was also in python2.6. Regardless, it needs to be done
differently.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3fW+UACgkQJdeBCYSNAAOMswCeNmvO/1LuTFKZjMGGZON+1YBb
dQcAoLq19easU8FMCfcEDJxqcLCZHzAi
=kwJ1
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

@jam: gz kindly provided what looks like a working Windows version of a factored-out is_local_pid_dead, so I think we're now ok there. Please let me know if you spot anything else.

[Martin Pool (mbp)]

I'd like some rereview on this, at least just so John can look at gz's windows code.

[Robert Collins (lifeless)]

A small question - without reading the diff (sorry, but it seemed a
little large).

Will this result in Launchpad smart servers thinking a lock is held by
a dead, when there are two smart server *machines* with one backend
store mounted over (NFS|OCFS) and the active process is on a different
machine?

-Rob

[Martin Pool (mbp)]

On 3 June 2011 18:56, Robert Collins <email address hidden> wrote:
> A small question - without reading the diff (sorry, but it seemed a
> little large).
>
> Will this result in Launchpad smart servers thinking a lock is held by
> a dead, when there are two smart server *machines* with one backend
> store mounted over (NFS|OCFS) and the active process is on a different
> machine?

Only if they have the same hostname, which they shouldn't.

More of a worry is if some manages to have a client machine with the
same hostname as the ssh server, and the names are stored
not-fully-qualified, and therefore they get confused about identity.

Perhaps we need something stronger than that. We could put a random
per-user key into their home directory and into the lock file, though
that would be a larger change.

[Robert Collins (lifeless)]

The impact of a mistake could be pretty bad, so perhaps we should look
at something stronger before doing this?

[Martin Packman (gz)]

As windows and posix should cover everything, I think this is fine now.

> I'd like some rereview on this, at least just so John can look at gz's windows
> code.

I'd like that too, was coded a little too late at night.

A few notes it:

* I did a ctypes and pywin32 version out of habit. Now we're 2.6 only, there's not much point in having pywin32 code around (apart from where it's easier to spell).
* As Martin changed the posix version to never raise an exception, should the windows versions change too? Really, I think I'd prefer the caller to catch and log rather than the function, but pywin32 makes that annoying by using a different exception type.
* OpenProcess requests the PROCESS_TERMINATE process access right. It might make more sense to ask for PROCESS_QUERY_INFORMATION or something else.
* Could add code later to get the process executable name too with GetProcessImageFileName, as we have the handle open anyway. Being able to say there's a running bzr.exe holding the lock rather than some other process that's inherited the pid may be useful.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[Martin Pool (mbp)]

On 3 June 2011 19:34, Robert Collins <email address hidden> wrote:
> The impact of a mistake could be pretty bad, so perhaps we should look
> at something stronger before doing this?

I wrote some background about other types of locking improvements we
can do, but we probably all know that. This patch is just about
trying to detect locks belong to processes that are sure to be dead.
As I said above, the main risk is that we'll see a lock actually from
another machine and assume it's actually local. It is a little hard
because there is on unix no obvious guaranteed-unique machine
identifier.

Things we could do to guard against that:

0- don't use this approach at all
1- put in the host default ip (also not guaranteed unique; likely to
often be seen as 127.0.0.1; also arguably a privacy problem to send
this to lp)
2- disable this on the smart server; punt back to the client to decide
if the process is live or not (maybe a good idea anyhow)
3- make up a unique per-user nonce; store it in .bazaar; put it into
lock files (not a perfect solution for home on NFS for instance)
4- don't automatically break it; just ask the user if they want to do
so, or point out that it probably is dead
5- grab some other random data that might both be stable across
multiple processes on a single machine, and across reboots, and also
unique to machines (this does exist on windows, and might be available
in Python)

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 6/6/2011 12:02 PM, Vincent Ladeuil wrote:
> The proposal to merge lp:~mbp/bzr/220464-stale-locks into lp:bzr has been updated.
>
> Status: Approved => Needs review
>
> For more details, see:
> https://code.launchpad.net/~mbp/bzr/220464-stale-locks/+merge/62582

Vila, you changed this to needs-review after approving it and sending it
to pqm. Did it bounce?

I'll try to take a look at it tomorrow, but my WinAPI isn't all that strong.

I do think gz's comment about access to kill vs access to get info is
relevant. Specifically, if User A does the push, User B cannot kill
their processes. Would that lead to us thinking the process is already
dead, and unlocking things underneath them?

John
=:->
 review: needsinfo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3s5IYACgkQJdeBCYSNAAObsACg0rZOrcMV24l6VkfbXQf6TtBw
rmcAn0OtBiyVbXAvzGp1Ldburw9E0CP5
=PEAW
-----END PGP SIGNATURE-----

review: Needs Information

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 6/3/2011 11:34 AM, Robert Collins wrote:
> The impact of a mistake could be pretty bad, so perhaps we should look
> at something stronger before doing this?
>

I think the potential for trouble is moderate here, mostly because of
the lifetimes of locks. If two people race on the *repository* lock, you
could get strange corruption. (Both feel they uploaded their content,
but only one of them ends up with their file mentioned in pack-names.)

That, however is meant to be a very fast lock, and shouldn't be opened
by VFS anymore. (I'm sure it could still happen, but we probably would
be more productive fixing those cases rather than spending too much time
on the locking code.)

If a Branch/WT lock gets out of sync, it should be a lot easier to
recover. The locks there seem a bit more advisory. (I'm pushing to this
branch, so don't try to push to it until I'm done.) Because the actual
mutations should be pretty close to atomic. (overwrite
.bzr/branch/last_revision, etc.)

gz saying that we may not be able to detect other people's processes as
being alive is worrying.

Having the shared host be named "ubuntu" and your local host be named
"ubuntu" could be a problem. Would it be reasonable as a first step to
only support local host transports? (So we auto-unlock
file://path/to/branch, but not bzr+ssh://remote-host/path/to/branch.)

I specifically mention "ubuntu" because I believe all ubuntu installs
default to naming the machine "ubuntu" and someone has to actively
change the machine name. (May not be as true anymore.) I think some of
this stems from using a live-install where the machine needs a name
before actually installing.

What about a compromise, and auto-unlocking local objects only, until we
get something like the unique random id in .bazaar, etc. (Note that you
could still get bitten if someone copies their .bazaar folder...)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3s5hsACgkQJdeBCYSNAAM7vACfSMvettIMp/bIGOpf/Ki7zm+d
yeMAoMEnK2O/8IpiABRLcS67TxlJD+eD
=Rcq8
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

<snip/>

    > Vila, you changed this to needs-review after approving it and sending it
    > to pqm. Did it bounce?

Yes (mail forwarded) and poolie said on IRC he wanted more feedback on
it before landing anyway.

[Robert Collins (lifeless)]

On Mon, Jun 6, 2011 at 9:52 PM, Martin Pool <email address hidden> wrote:
> On 3 June 2011 19:34, Robert Collins <email address hidden> wrote:
>> The impact of a mistake could be pretty bad, so perhaps we should look
>> at something stronger before doing this?
>
> I wrote some background about other types of locking improvements we
> can do, but we probably all know that.  This patch is just about
> trying to detect locks belong to processes that are sure to be dead.
> As I said above, the main risk is that we'll see a lock actually from
> another machine and assume it's actually local.  It is a little hard
> because there is on unix no obvious guaranteed-unique machine
> identifier.
>
> Things we could do to guard against that:
>
> 0- don't use this approach at all
> 1- put in the host default ip (also not guaranteed unique; likely to
> often be seen as 127.0.0.1; also arguably a privacy problem to send
> this to lp)
> 2- disable this on the smart server; punt back to the client to decide
> if the process is live or not (maybe a good idea anyhow)
> 3- make up a unique per-user nonce; store it in .bazaar; put it into
> lock files (not a perfect solution for home on NFS for instance)
> 4- don't automatically break it; just ask the user if they want to do
> so, or point out that it probably is dead
> 5- grab some other random data that might both be stable across
> multiple processes on a single machine, and across reboots, and also
> unique to machines (this does exist on windows, and might be available
> in Python)

Right. So I'm worried about one of these cases incorrectly happening
to Launchpad:
 - Launchpad smart server 1 thinking smart server 2 is dead
 - Launchpad smart server 1 thinking sftp client 2 is dead
 - sftp client 1 thinking Launchpad smart server 2 is dead
 - sftp client 1 thinking sftp client 2 is dead

However, These are sorted in what I think is most likely frequency of
use - that is we use the smart server a lot.

Having this go wrong on a repository would be pretty bad. Having it go
wrong on a branch (which has a greater window of opportunity per
John's mail) would probably be less destructive.

For the first case, where Launchpad is running both smart servers, we
can configure a smart server nonce so the servers can detect each
other, if thats how you'd like to prevent misdetected dead processes.

Or we can use a network aware lock inside the data centre (e.g. in
memcache/a db/ ...).

-Rob

[Martin Pool (mbp)]

On 7 June 2011 06:03, Robert Collins <email address hidden> wrote:
> Right. So I'm worried about one of these cases incorrectly happening
> to Launchpad:
>  - Launchpad smart server 1 thinking smart server 2 is dead
>  - Launchpad smart server 1 thinking sftp client 2 is dead
>  - sftp client 1 thinking Launchpad smart server 2 is dead
>  - sftp client 1 thinking sftp client 2 is dead
>
> However, These are sorted in what I think is most likely frequency of
> use - that is we use the smart server a lot.
>
> Having this go wrong on a repository would be pretty bad. Having it go
> wrong on a branch (which has a greater window of opportunity per
> John's mail) would probably be less destructive.
>
> For the first case, where Launchpad is running both smart servers, we
> can configure a smart server nonce so the servers can detect each
> other, if thats how you'd like to prevent misdetected dead processes.
>
> Or we can use a network aware lock inside the data centre (e.g. in
> memcache/a db/ ...).

When the smart server takes a lock, it puts its own (ie the server
side) hostname and pid into the lock file. (I checked in the source
and by taking a lock on lp.) So the first case should never happen
unless Canonical IS starts giving multiple servers the same name, or
there is some other kind of bizarre split-brain case.

hostname: crowberry
nonce: x4bgo6vp5iixo3o8sl4o
pid: 16838
start_time: 1307421845
user: <email address hidden>

I don't think using a network aware lock is especially useful; backing
the whole repository storage on something other than a disk might be
but I don't see much reason to move just the locks.

The second two cases can potentially happen if someone configures
their machine to have the same hostname as one of the Launchpad
servers, and they access the repo over sftp (or nosmart ssh). This is
probably unlikely considering how obscure Canonical's host names are
but it's possible.

I'd like to use the fqdn but I can't see an easy reliable way to get
it in Python. (It really wants to say localhost.localdomain and
that's just making things worse.) We could put this in configuration
but that will take some work to roll out to lp.

The last case should only happen when someone has configured two
machines with the same hostname and they're both accessing the same
branch at the same time, which is kind of asking for trouble.

We need to bear in mind that at the moment people are encouraged to
break locks by hand and they may well make worse decisions than bzr
can.

One easy step would be to check the user name too; I'll try that now.

[Andrew Bennetts (spiv)]

Martin Pool wrote:
[...]
> We need to bear in mind that at the moment people are encouraged to
> break locks by hand and they may well make worse decisions than bzr
> can.

Especially in the presence of a smart server, where it's not clear to
users whether the server or the client “owns” the lock. Confusingly,
the client does (via the nonce handed back from the server) but it's the
server's details that appear in the lock info. I've certainly seen
users confused about this on IRC.

I suppose ideally the lock info generated by the server would indicate
“on behalf of bzr smart client from 1.2.3.4:9876” or something like
that. I think LP's server already tweaks BZR_EMAIL to at least use the
LP username rather than codehosting system user.

[Martin Pool (mbp)]

It does seem to use the user's email address which I guess comes from
the lp account.

Also, I think lock breaking at the moment is done over vfs and it
would be good to abstract that.

Martin

[Martin Packman (gz)]

> I do think gz's comment about access to kill vs access to get info is
> relevant. Specifically, if User A does the push, User B cannot kill
> their processes. Would that lead to us thinking the process is already
> dead, and unlocking things underneath them?

No, it should fail safe. In this circumstance, you get ERROR_ACCESS_DENIED which we take to mean the process may exist, so don't break the lock. That's just slightly worse information than if we can get a process handle.

[John A Meinel (jameinel)]

I would like to see the constants in the win32 functions pre-defined as constants, so

PROCESS_TERMINATE = 1
ERROR_ACCESS_DENIED = 5

etc.

But that is a pretty trivial cleanup.
if self.get('hostname') == 'localhost':

I wonder if we should handle "get()" returning None? what about "get('hostname').startswith('localhost') In case it returns "localhost.localdomain".

Anyway, overall I think I've balanced enough to be okay with this. I think the checks Martin added is pretty strict, so should be enough.

review: Approve

[Martin Pool (mbp)]

Based on the irc discussion, we might start this by just warning in these cases.

[Martin Pool (mbp)]

sent to pqm by email

[Martin Pool (mbp)]

sent to pqm by email

[Martin Pool (mbp)]

sent to pqm by email

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'bzrlib/commit.py'
--- bzrlib/commit.py	2011-05-17 14:27:30 +0000
+++ bzrlib/commit.py	2011-06-13 21:24:39 +0000
@@ -52,7 +52,6 @@
 from bzrlib import (
     debug,
     errors,
-    revision,
     trace,
     tree,
     ui,

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-06-01 12:31:04 +0000
+++ bzrlib/config.py	2011-06-13 21:24:39 +0000
@@ -75,7 +75,6 @@
 import re
 from cStringIO import StringIO
 
-import bzrlib
 from bzrlib import (
     atomicfile,
     bzrdir,
@@ -372,16 +371,18 @@
                 value = self._expand_options_in_string(value)
         return value
 
-    def get_user_option_as_bool(self, option_name, expand=None):
-        """Get a generic option as a boolean - no special process, no default.
+    def get_user_option_as_bool(self, option_name, expand=None, default=None):
+        """Get a generic option as a boolean.
 
+        :param expand: Allow expanding references to other config values.
+        :param default: Default value if nothing is configured
         :return None if the option doesn't exist or its value can't be
             interpreted as a boolean. Returns True or False otherwise.
         """
         s = self.get_user_option(option_name, expand=expand)
         if s is None:
             # The option doesn't exist
-            return None
+            return default
         val = ui.bool_from_string(s)
         if val is None:
             # The value can't be interpreted as a boolean
@@ -2701,6 +2702,8 @@
                         ' the configuration file'),
         ]
 
+    _see_also = ['configuration']
+
     @commands.display_command
     def run(self, name=None, all=False, directory=None, scope=None,
             remove=False):

=== modified file 'bzrlib/help_topics/en/configuration.txt'
--- bzrlib/help_topics/en/configuration.txt	2011-05-24 14:40:29 +0000
+++ bzrlib/help_topics/en/configuration.txt	2011-06-13 21:24:39 +0000
@@ -462,6 +462,14 @@
 These settings are only needed if the SMTP server requires authentication
 to send mail.
 
+locks.steal_dead
+~~~~~~~~~~~~~~~~
+
+If set to true, bzr will automatically break locks held by processes from
+the same machine and user that are no longer alive.  Otherwise, it will
+print a message and you can break the lock manually, if you are satisfied
+the object is no longer in use.
+
 mail_client
 ~~~~~~~~~~~
 

=== modified file 'bzrlib/lockdir.py'
--- bzrlib/lockdir.py	2011-05-16 13:39:39 +0000
+++ bzrlib/lockdir.py	2011-06-13 21:24:39 +0000
@@ -92,6 +92,10 @@
 >>> # do something here
 >>> l.unlock()
 
+Some classes of stale locks can be predicted by checking: the host name is the
+same as the local host name; the user name is the same as the local user; the
+process id no longer exists.  The check on user name is not strictly necessary
+but helps protect against colliding host names.
 """
 
 
@@ -103,16 +107,19 @@
 # the existing locking code and needs a new format of the containing object.
 # -- robertc, mbp 20070628
 
+import errno
 import os
 import time
 
 from bzrlib import (
+    config,
     debug,
     errors,
     lock,
     osutils,
+    ui,
+    urlutils,
     )
-import bzrlib.config
 from bzrlib.decorators import only_raises
 from bzrlib.errors import (
         DirectoryNotEmpty,
@@ -130,7 +137,6 @@
         )
 from bzrlib.trace import mutter, note
 from bzrlib.osutils import format_delta, rand_chars, get_host_name
-import bzrlib.ui
 
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
@@ -162,7 +168,8 @@
 
     __INFO_NAME = '/info'
 
-    def __init__(self, transport, path, file_modebits=0644, dir_modebits=0755):
+    def __init__(self, transport, path, file_modebits=0644, dir_modebits=0755,
+        extra_holder_info=None):
         """Create a new LockDir object.
 
         The LockDir is initially unlocked - this just creates the object.
@@ -171,6 +178,10 @@
 
         :param path: Path to the lock within the base directory of the
             transport.
+
+        :param extra_holder_info: If passed, {str:str} dict of extra or
+            updated information to insert into the info file when the lock is
+            taken.
         """
         self.transport = transport
         self.path = path
@@ -181,8 +192,9 @@
         self._held_info_path = self._held_dir + self.__INFO_NAME
         self._file_modebits = file_modebits
         self._dir_modebits = dir_modebits
-
         self._report_function = note
+        self.extra_holder_info = extra_holder_info
+        self._warned_about_lock_holder = None
 
     def __repr__(self):
         return '%s(%s%s)' % (self.__class__.__name__,
@@ -203,7 +215,6 @@
         except (TransportError, PathError), e:
             raise LockFailed(self, e)
 
-
     def _attempt_lock(self):
         """Make the pending directory and attempt to rename into place.
 
@@ -216,8 +227,8 @@
 
         :returns: The nonce of the lock, if it was successfully acquired.
 
-        :raises LockContention: If the lock is held by someone else.  The exception
-            contains the info of the current holder of the lock.
+        :raises LockContention: If the lock is held by someone else.  The
+            exception contains the info of the current holder of the lock.
         """
         self._trace("lock_write...")
         start_time = time.time()
@@ -226,17 +237,24 @@
         except (errors.TransportError, PathError), e:
             self._trace("... failed to create pending dir, %s", e)
             raise LockFailed(self, e)
-        try:
-            self.transport.rename(tmpname, self._held_dir)
-        except (errors.TransportError, PathError, DirectoryNotEmpty,
-                FileExists, ResourceBusy), e:
-            self._trace("... contention, %s", e)
-            self._remove_pending_dir(tmpname)
-            raise LockContention(self)
-        except Exception, e:
-            self._trace("... lock failed, %s", e)
-            self._remove_pending_dir(tmpname)
-            raise
+        while True:
+            try:
+                self.transport.rename(tmpname, self._held_dir)
+                break
+            except (errors.TransportError, PathError, DirectoryNotEmpty,
+                    FileExists, ResourceBusy), e:
+                self._trace("... contention, %s", e)
+                other_holder = self.peek()
+                self._trace("other holder is %r" % other_holder)
+                try:
+                    self._handle_lock_contention(other_holder)
+                except:
+                    self._remove_pending_dir(tmpname)
+                    raise
+            except Exception, e:
+                self._trace("... lock failed, %s", e)
+                self._remove_pending_dir(tmpname)
+                raise
         # We must check we really got the lock, because Launchpad's sftp
         # server at one time had a bug were the rename would successfully
         # move the new directory into the existing directory, which was
@@ -262,6 +280,33 @@
                 (time.time() - start_time) * 1000)
         return self.nonce
 
+    def _handle_lock_contention(self, other_holder):
+        """A lock we want to take is held by someone else.
+
+        This function can: tell the user about it; possibly detect that it's
+        safe or appropriate to steal the lock, or just raise an exception.
+
+        If this function returns (without raising an exception) the lock will
+        be attempted again.
+
+        :param other_holder: A LockHeldInfo for the current holder; note that
+            it might be None if the lock can be seen to be held but the info
+            can't be read.
+        """
+        if (other_holder is not None):
+            if other_holder.is_lock_holder_known_dead():
+                if self.get_config().get_user_option_as_bool(
+                    'locks.steal_dead',
+                    default=False):
+                    ui.ui_factory.show_user_warning(
+                        'locks_steal_dead',
+                        lock_url=urlutils.join(self.transport.base, self.path),
+                        other_holder_info=unicode(other_holder))
+                    self.force_break(other_holder)
+                    self._trace("stole lock from dead holder")
+                    return
+        raise LockContention(self)
+
     def _remove_pending_dir(self, tmpname):
         """Remove the pending directory
 
@@ -287,14 +332,14 @@
             self.create(mode=self._dir_modebits)
             # After creating the lock directory, try again
             self.transport.mkdir(tmpname)
-        self.nonce = rand_chars(20)
-        info_bytes = self._prepare_info()
+        info = LockHeldInfo.for_this_process(self.extra_holder_info)
+        self.nonce = info.get('nonce')
         # We use put_file_non_atomic because we just created a new unique
         # directory so we don't have to worry about files existing there.
         # We'll rename the whole directory into place to get atomic
         # properties
         self.transport.put_bytes_non_atomic(tmpname + self.__INFO_NAME,
-                                            info_bytes)
+            info.to_bytes())
         return tmpname
 
     @only_raises(LockNotHeld, LockBroken)
@@ -344,9 +389,10 @@
     def break_lock(self):
         """Break a lock not held by this instance of LockDir.
 
-        This is a UI centric function: it uses the bzrlib.ui.ui_factory to
+        This is a UI centric function: it uses the ui.ui_factory to
         prompt for input if a lock is detected and there is any doubt about
-        it possibly being still active.
+        it possibly being still active.  force_break is the non-interactive
+        version.
 
         :returns: LockResult for the broken lock.
         """
@@ -355,16 +401,16 @@
             holder_info = self.peek()
         except LockCorrupt, e:
             # The lock info is corrupt.
-            if bzrlib.ui.ui_factory.get_boolean(u"Break (corrupt %r)" % (self,)):
+            if ui.ui_factory.get_boolean(u"Break (corrupt %r)" % (self,)):
                 self.force_break_corrupt(e.file_data)
             return
         if holder_info is not None:
-            lock_info = '\n'.join(self._format_lock_info(holder_info))
-            if bzrlib.ui.ui_factory.confirm_action(
-                "Break %(lock_info)s", 'bzrlib.lockdir.break', 
-                dict(lock_info=lock_info)):
+            if ui.ui_factory.confirm_action(
+                u"Break %(lock_info)s",
+                'bzrlib.lockdir.break',
+                dict(lock_info=unicode(holder_info))):
                 result = self.force_break(holder_info)
-                bzrlib.ui.ui_factory.show_message(
+                ui.ui_factory.show_message(
                     "Broke lock %s" % result.lock_url)
 
     def force_break(self, dead_holder_info):
@@ -374,18 +420,19 @@
         it still thinks it has the lock there will be two concurrent writers.
         In general the user's approval should be sought for lock breaks.
 
-        dead_holder_info must be the result of a previous LockDir.peek() call;
-        this is used to check that it's still held by the same process that
-        the user decided was dead.  If this is not the current holder,
-        LockBreakMismatch is raised.
-
         After the lock is broken it will not be held by any process.
         It is possible that another process may sneak in and take the
         lock before the breaking process acquires it.
 
+        :param dead_holder_info:
+            Must be the result of a previous LockDir.peek() call; this is used
+            to check that it's still held by the same process that the user
+            decided was dead.  If this is not the current holder,
+            LockBreakMismatch is raised.
+
         :returns: LockResult for the broken lock.
         """
-        if not isinstance(dead_holder_info, dict):
+        if not isinstance(dead_holder_info, LockHeldInfo):
             raise ValueError("dead_holder_info: %r" % dead_holder_info)
         self._check_not_locked()
         current_info = self.peek()
@@ -413,10 +460,10 @@
 
     def force_break_corrupt(self, corrupt_info_lines):
         """Release a lock that has been corrupted.
-        
+
         This is very similar to force_break, it except it doesn't assume that
         self.peek() can work.
-        
+
         :param corrupt_info_lines: the lines of the corrupted info file, used
             to check that the lock hasn't changed between reading the (corrupt)
             info file and calling force_break_corrupt.
@@ -470,7 +517,8 @@
 
         peek() reads the info file of the lock holder, if any.
         """
-        return self._parse_info(self.transport.get_bytes(path))
+        return LockHeldInfo.from_info_file_bytes(
+            self.transport.get_bytes(path))
 
     def peek(self):
         """Check if the lock is held by anyone.
@@ -489,35 +537,6 @@
     def _prepare_info(self):
         """Write information about a pending lock to a temporary file.
         """
-        # XXX: is creating this here inefficient?
-        config = bzrlib.config.GlobalConfig()
-        try:
-            user = config.username()
-        except errors.NoWhoami:
-            user = osutils.getuser_unicode()
-        s = rio.Stanza(hostname=get_host_name(),
-                   pid=str(os.getpid()),
-                   start_time=str(int(time.time())),
-                   nonce=self.nonce,
-                   user=user,
-                   )
-        return s.to_string()
-
-    def _parse_info(self, info_bytes):
-        lines = osutils.split_lines(info_bytes)
-        try:
-            stanza = rio.read_stanza(lines)
-        except ValueError, e:
-            mutter('Corrupt lock info file: %r', lines)
-            raise LockCorrupt("could not parse lock info file: " + str(e),
-                              lines)
-        if stanza is None:
-            # see bug 185013; we fairly often end up with the info file being
-            # empty after an interruption; we could log a message here but
-            # there may not be much we can say
-            return {}
-        else:
-            return stanza.as_dict()
 
     def attempt_lock(self):
         """Take the lock; fail if it's already held.
@@ -598,24 +617,16 @@
                 else:
                     start = 'Lock owner changed for'
                 last_info = new_info
-                formatted_info = self._format_lock_info(new_info)
+                msg = u'%s lock %s %s.' % (start, lock_url, new_info)
                 if deadline_str is None:
                     deadline_str = time.strftime('%H:%M:%S',
-                                                 time.localtime(deadline))
-                user, hostname, pid, time_ago = formatted_info
-                msg = ('%s lock %s '        # lock_url
-                    'held by '              # start
-                    '%s\n'                  # user
-                    'at %s '                # hostname
-                    '[process #%s], '       # pid
-                    'acquired %s.')         # time ago
-                msg_args = [start, lock_url, user, hostname, pid, time_ago]
+                                                    time.localtime(deadline))
                 if timeout > 0:
                     msg += ('\nWill continue to try until %s, unless '
-                        'you press Ctrl-C.')
-                    msg_args.append(deadline_str)
+                        'you press Ctrl-C.'
+                        % deadline_str)
                 msg += '\nSee "bzr help break-lock" for more.'
-                self._report_function(msg, *msg_args)
+                self._report_function(msg)
             if (max_attempts is not None) and (attempt_count >= max_attempts):
                 self._trace("exceeded %d attempts")
                 raise LockContention(self)
@@ -676,23 +687,6 @@
             raise LockContention(self)
         self._fake_read_lock = True
 
-    def _format_lock_info(self, info):
-        """Turn the contents of peek() into something for the user"""
-        start_time = info.get('start_time')
-        if start_time is None:
-            time_ago = '(unknown)'
-        else:
-            time_ago = format_delta(time.time() - int(info['start_time']))
-        user = info.get('user', '<unknown>')
-        hostname = info.get('hostname', '<unknown>')
-        pid = info.get('pid', '<unknown>')
-        return [
-            user,
-            hostname,
-            pid,
-            time_ago,
-            ]
-
     def validate_token(self, token):
         if token is not None:
             info = self.peek()
@@ -710,3 +704,158 @@
         if 'lock' not in debug.debug_flags:
             return
         mutter(str(self) + ": " + (format % args))
+
+    def get_config(self):
+        """Get the configuration that governs this lockdir."""
+        # XXX: This really should also use the locationconfig at least, but
+        # that seems a bit hard to hook up at the moment. -- mbp 20110329
+        return config.GlobalConfig()
+
+
+class LockHeldInfo(object):
+    """The information recorded about a held lock.
+
+    This information is recorded into the lock when it's taken, and it can be
+    read back by any process with access to the lockdir.  It can be used, for
+    example, to tell the user who holds the lock, or to try to detect whether
+    the lock holder is still alive.
+
+    Prior to bzr 2.4 a simple dict was used instead of an object.
+    """
+
+    def __init__(self, info_dict):
+        self.info_dict = info_dict
+
+    def __repr__(self):
+        """Return a debugging representation of this object."""
+        return "%s(%r)" % (self.__class__.__name__, self.info_dict)
+
+    def __unicode__(self):
+        """Return a user-oriented description of this object."""
+        d = self.to_readable_dict()
+        return (
+            u'held by %(user)s on %(hostname)s (process #%(pid)s), '
+            u'acquired %(time_ago)s' % d)
+
+    def to_readable_dict(self):
+        """Turn the holder info into a dict of human-readable attributes.
+
+        For example, the start time is presented relative to the current time,
+        rather than as seconds since the epoch.
+
+        Returns a list of [user, hostname, pid, time_ago] all as readable
+        strings.
+        """
+        start_time = self.info_dict.get('start_time')
+        if start_time is None:
+            time_ago = '(unknown)'
+        else:
+            time_ago = format_delta(
+                time.time() - int(self.info_dict['start_time']))
+        user = self.info_dict.get('user', '<unknown>')
+        hostname = self.info_dict.get('hostname', '<unknown>')
+        pid = self.info_dict.get('pid', '<unknown>')
+        return dict(
+            user=user,
+            hostname=hostname,
+            pid=pid,
+            time_ago=time_ago)
+
+    def get(self, field_name):
+        """Return the contents of a field from the lock info, or None."""
+        return self.info_dict.get(field_name)
+
+    @classmethod
+    def for_this_process(cls, extra_holder_info):
+        """Return a new LockHeldInfo for a lock taken by this process.
+        """
+        info = dict(
+            hostname=get_host_name(),
+            pid=str(os.getpid()),
+            nonce=rand_chars(20),
+            start_time=str(int(time.time())),
+            user=get_username_for_lock_info(),
+            )
+        if extra_holder_info is not None:
+            info.update(extra_holder_info)
+        return cls(info)
+
+    def to_bytes(self):
+        s = rio.Stanza(**self.info_dict)
+        return s.to_string()
+
+    @classmethod
+    def from_info_file_bytes(cls, info_file_bytes):
+        """Construct from the contents of the held file."""
+        lines = osutils.split_lines(info_file_bytes)
+        try:
+            stanza = rio.read_stanza(lines)
+        except ValueError, e:
+            mutter('Corrupt lock info file: %r', lines)
+            raise LockCorrupt("could not parse lock info file: " + str(e),
+                lines)
+        if stanza is None:
+            # see bug 185013; we fairly often end up with the info file being
+            # empty after an interruption; we could log a message here but
+            # there may not be much we can say
+            return cls({})
+        else:
+            return cls(stanza.as_dict())
+
+    def __cmp__(self, other):
+        """Value comparison of lock holders."""
+        return (
+            cmp(type(self), type(other))
+            or cmp(self.info_dict, other.info_dict))
+
+    def is_locked_by_this_process(self):
+        """True if this process seems to be the current lock holder."""
+        return (
+            self.get('hostname') == get_host_name()
+            and self.get('pid') == str(os.getpid())
+            and self.get('user') == get_username_for_lock_info())
+
+    def is_lock_holder_known_dead(self):
+        """True if the lock holder process is known to be dead.
+
+        False if it's either known to be still alive, or if we just can't tell.
+
+        We can be fairly sure the lock holder is dead if it declared the same
+        hostname and there is no process with the given pid alive.  If people
+        have multiple machines with the same hostname this may cause trouble.
+
+        This doesn't check whether the lock holder is in fact the same process
+        calling this method.  (In that case it will return true.)
+        """
+        if self.get('hostname') != get_host_name():
+            return False
+        if self.get('hostname') == 'localhost':
+            # Too ambiguous.
+            return False
+        if self.get('user') != get_username_for_lock_info():
+            # Could well be another local process by a different user, but
+            # just to be safe we won't conclude about this either.
+            return False
+        pid_str = self.info_dict.get('pid', None)
+        if not pid_str:
+            mutter("no pid recorded in %r" % (self, ))
+            return False
+        try:
+            pid = int(pid_str)
+        except ValueError:
+            mutter("can't parse pid %r from %r"
+                % (pid_str, self))
+            return False
+        return osutils.is_local_pid_dead(pid)
+
+
+def get_username_for_lock_info():
+    """Get a username suitable for putting into a lock.
+
+    It's ok if what's written here is not a proper email address as long
+    as it gives some clue who the user is.
+    """
+    try:
+        return config.GlobalConfig().username()
+    except errors.NoWhoami:
+        return osutils.getuser_unicode()

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2011-05-26 08:05:45 +0000
+++ bzrlib/osutils.py	2011-06-13 21:24:39 +0000
@@ -2448,3 +2448,30 @@
             if os.access(f, os.X_OK):
                 return f
     return None
+
+
+def _posix_is_local_pid_dead(pid):
+    """True if pid doesn't correspond to live process on this machine"""
+    try:
+        # Special meaning of unix kill: just check if it's there.
+        os.kill(pid, 0)
+    except OSError, e:
+        if e.errno == errno.ESRCH:
+            # On this machine, and really not found: as sure as we can be
+            # that it's dead.
+            return True
+        elif e.errno == errno.EPERM:
+            # exists, though not ours
+            return False
+        else:
+            mutter("os.kill(%d, 0) failed: %s" % (pid, e))
+            # Don't really know.
+            return False
+    else:
+        # Exists and our process: not dead.
+        return False
+
+if sys.platform == "win32":
+    is_local_pid_dead = win32utils.is_local_pid_dead
+else:
+    is_local_pid_dead = _posix_is_local_pid_dead

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-06-09 15:13:54 +0000
+++ bzrlib/tests/__init__.py	2011-06-13 21:24:39 +0000
@@ -1070,9 +1070,13 @@
         # break some locks on purpose and should be taken into account by
         # considering that breaking a lock is just a dirty way of releasing it.
         if len(acquired_locks) != (len(released_locks) + len(broken_locks)):
-            message = ('Different number of acquired and '
-                       'released or broken locks. (%s, %s + %s)' %
-                       (acquired_locks, released_locks, broken_locks))
+            message = (
+                'Different number of acquired and '
+                'released or broken locks.\n'
+                'acquired=%s\n'
+                'released=%s\n'
+                'broken=%s\n' %
+                (acquired_locks, released_locks, broken_locks))
             if not self._lock_check_thorough:
                 # Rather than fail, just warn
                 print "Broken test %s: %s" % (self, message)

=== modified file 'bzrlib/tests/test_lockdir.py'
--- bzrlib/tests/test_lockdir.py	2011-05-13 12:51:05 +0000
+++ bzrlib/tests/test_lockdir.py	2011-06-13 21:24:39 +0000
@@ -17,6 +17,7 @@
 """Tests for LockDir"""
 
 import os
+import sys
 import time
 
 import bzrlib
@@ -24,6 +25,7 @@
     config,
     errors,
     lock,
+    lockdir,
     osutils,
     tests,
     transport,
@@ -35,9 +37,13 @@
     LockFailed,
     LockNotHeld,
     )
-from bzrlib.lockdir import LockDir
+from bzrlib.lockdir import (
+    LockDir,
+    LockHeldInfo,
+    )
 from bzrlib.tests import (
     features,
+    TestCase,
     TestCaseWithTransport,
     )
 from bzrlib.trace import note
@@ -48,6 +54,7 @@
 # implementation are tested separately.  (The main requirement is just that
 # they don't allow overwriting nonempty directories.)
 
+
 class TestLockDir(TestCaseWithTransport):
     """Test LockDir operations"""
 
@@ -141,8 +148,8 @@
         self.addCleanup(lf1.unlock)
         # lock is held, should get some info on it
         info1 = lf1.peek()
-        self.assertEqual(set(info1.keys()),
-                         set(['user', 'nonce', 'hostname', 'pid', 'start_time']))
+        self.assertEqual(set(info1.info_dict.keys()),
+            set(['user', 'nonce', 'hostname', 'pid', 'start_time']))
         # should get the same info if we look at it through a different
         # instance
         info2 = LockDir(t, 'test_lock').peek()
@@ -161,7 +168,7 @@
         self.addCleanup(lf1.unlock)
         info2 = lf2.peek()
         self.assertTrue(info2)
-        self.assertEqual(info2['nonce'], lf1.nonce)
+        self.assertEqual(info2.get('nonce'), lf1.nonce)
 
     def test_30_lock_wait_fail(self):
         """Wait on a lock, then fail
@@ -184,24 +191,16 @@
             # it should only take about 0.4 seconds, but we allow more time in
             # case the machine is heavily loaded
             self.assertTrue(after - before <= 8.0,
-                    "took %f seconds to detect lock contention" % (after - before))
+                "took %f seconds to detect lock contention" % (after - before))
         finally:
             lf1.unlock()
         self.assertEqual(1, len(self._logged_reports))
-        self.assertEqual(self._logged_reports[0][0],
-            '%s lock %s held by %s\n'
-            'at %s [process #%s], acquired %s.\n'
-            'Will continue to try until %s, unless '
-            'you press Ctrl-C.\n'
-            'See "bzr help break-lock" for more.')
-        start, lock_url, user, hostname, pid, time_ago, deadline_str = \
-            self._logged_reports[0][1]
-        self.assertEqual(start, u'Unable to obtain')
-        self.assertEqual(user, u'[e-mail]')
-        # skip hostname
-        self.assertContainsRe(pid, r'\d+')
-        self.assertContainsRe(time_ago, r'.* ago')
-        self.assertContainsRe(deadline_str, r'\d{2}:\d{2}:\d{2}')
+        self.assertContainsRe(self._logged_reports[0][0],
+            r'Unable to obtain lock .* held by jrandom@example\.com on .*'
+            r' \(process #\d+\), acquired .* ago\.\n'
+            r'Will continue to try until \d{2}:\d{2}:\d{2}, unless '
+            r'you press Ctrl-C.\n'
+            r'See "bzr help break-lock" for more.')
 
     def test_31_lock_wait_easy(self):
         """Succeed when waiting on a lock with no contention.
@@ -349,12 +348,15 @@
         ld.create()
         ld.lock_write()
         ld.transport.put_bytes_non_atomic('test_lock/held/info', '\0')
+
         class LoggingUIFactory(bzrlib.ui.SilentUIFactory):
             def __init__(self):
                 self.prompts = []
+
             def get_boolean(self, prompt):
                 self.prompts.append(('boolean', prompt))
                 return True
+
         ui = LoggingUIFactory()
         self.overrideAttr(bzrlib.ui, 'ui_factory', ui)
         ld2.break_lock()
@@ -372,12 +374,15 @@
         ld.create()
         ld.lock_write()
         ld.transport.delete('test_lock/held/info')
+
         class LoggingUIFactory(bzrlib.ui.SilentUIFactory):
             def __init__(self):
                 self.prompts = []
+
             def get_boolean(self, prompt):
                 self.prompts.append(('boolean', prompt))
                 return True
+
         ui = LoggingUIFactory()
         orig_factory = bzrlib.ui.ui_factory
         bzrlib.ui.ui_factory = ui
@@ -416,18 +421,17 @@
         lf1.unlock()
         self.assertFalse(t.has('test_lock/held/info'))
 
-    def test__format_lock_info(self):
+    def test_display_form(self):
         ld1 = self.get_lock()
         ld1.create()
         ld1.lock_write()
         try:
-            info_list = ld1._format_lock_info(ld1.peek())
+            info_list = ld1.peek().to_readable_dict()
         finally:
             ld1.unlock()
-        self.assertEqual(info_list[0], u'[e-mail]')
-        # info_list[1] is hostname. we skip this.
-        self.assertContainsRe(info_list[2], '^\d+$') # pid
-        self.assertContainsRe(info_list[3], r'^\d+ seconds? ago$') # time_ago
+        self.assertEqual(info_list['user'], u'[e-mail]')
+        self.assertContainsRe(info_list['pid'], '^\d+$')
+        self.assertContainsRe(info_list['time_ago'], r'^\d+ seconds? ago$')
 
     def test_lock_without_email(self):
         global_config = config.GlobalConfig()
@@ -481,8 +485,10 @@
         # should be nothing before we start
         ld1.create()
         t = self.get_transport().clone('test_lock')
+
         def check_dir(a):
             self.assertEquals(a, t.list_dir('.'))
+
         check_dir([])
         # when held, that's all we see
         ld1.attempt_lock()
@@ -505,9 +511,10 @@
         t.put_bytes('test_lock/held/info', '')
         lf = LockDir(t, 'test_lock')
         info = lf.peek()
-        formatted_info = lf._format_lock_info(info)
+        formatted_info = info.to_readable_dict()
         self.assertEquals(
-            ['<unknown>', '<unknown>', '<unknown>', '(unknown)'],
+            dict(user='<unknown>', hostname='<unknown>', pid='<unknown>',
+                time_ago='(unknown)'),
             formatted_info)
 
     def test_corrupt_lockdir_info(self):
@@ -646,3 +653,108 @@
         ld2.force_break(holder_info)
         lock_path = ld.transport.abspath(ld.path)
         self.assertEqual([], self._calls)
+
+
+class TestLockHeldInfo(TestCase):
+    """Can get information about the lock holder, and detect whether they're
+    still alive."""
+
+    def test_repr(self):
+        info = LockHeldInfo.for_this_process(None)
+        self.assertContainsRe(repr(info), r"LockHeldInfo\(.*\)")
+
+    def test_unicode(self):
+        info = LockHeldInfo.for_this_process(None)
+        self.assertContainsRe(unicode(info),
+            r'held by .* on .* \(process #\d+\), acquired .* ago')
+
+    def test_is_locked_by_this_process(self):
+        info = LockHeldInfo.for_this_process(None)
+        self.assertTrue(info.is_locked_by_this_process())
+
+    def test_is_not_locked_by_this_process(self):
+        info = LockHeldInfo.for_this_process(None)
+        info.info_dict['pid'] = '123123123123123'
+        self.assertFalse(info.is_locked_by_this_process())
+
+    def test_lock_holder_live_process(self):
+        """Detect that the holder (this process) is still running."""
+        info = LockHeldInfo.for_this_process(None)
+        self.assertFalse(info.is_lock_holder_known_dead())
+
+    def test_lock_holder_dead_process(self):
+        """Detect that the holder (this process) is still running."""
+        info = LockHeldInfo.for_this_process(None)
+        info.info_dict['pid'] = '123123123'
+        if sys.platform == 'win32':
+            self.knownFailure(
+                'live lock holder detection not implemented yet on win32')
+        self.assertTrue(info.is_lock_holder_known_dead())
+
+    def test_lock_holder_other_machine(self):
+        """The lock holder isn't here so we don't know if they're alive."""
+        info = LockHeldInfo.for_this_process(None)
+        info.info_dict['hostname'] = 'egg.example.com'
+        info.info_dict['pid'] = '123123123'
+        self.assertFalse(info.is_lock_holder_known_dead())
+
+    def test_lock_holder_other_user(self):
+        """Only auto-break locks held by this user."""
+        info = LockHeldInfo.for_this_process(None)
+        info.info_dict['user'] = '[e-mail]'
+        info.info_dict['pid'] = '123123123'
+        self.assertFalse(info.is_lock_holder_known_dead())
+
+    def test_no_good_hostname(self):
+        """Correctly handle ambiguous hostnames.
+
+        If the lock's recorded with just 'localhost' we can't really trust
+        it's the same 'localhost'.  (There are quite a few of them. :-)
+        So even if the process is known not to be alive, we can't say that's
+        known for sure.
+        """
+        self.overrideAttr(lockdir, 'get_host_name',
+            lambda: 'localhost')
+        info = LockHeldInfo.for_this_process(None)
+        info.info_dict['pid'] = '123123123'
+        self.assertFalse(info.is_lock_holder_known_dead())
+
+
+class TestStaleLockDir(TestCaseWithTransport):
+    """Can automatically break stale locks.
+
+    :see: https://bugs.launchpad.net/bzr/+bug/220464
+    """
+
+    def test_auto_break_stale_lock(self):
+        """Locks safely known to be stale are just cleaned up.
+
+        This generates a warning but no other user interaction.
+        """
+        # This is off by default at present; see the discussion in the bug.
+        # If you change the default, don't forget to update the docs.
+        config.GlobalConfig().set_user_option('locks.steal_dead', True)
+        # Create a lock pretending to come from a different nonexistent
+        # process on the same machine.
+        l1 = LockDir(self.get_transport(), 'a',
+            extra_holder_info={'pid': '12312313'})
+        token_1 = l1.attempt_lock()
+        l2 = LockDir(self.get_transport(), 'a')
+        token_2 = l2.attempt_lock()
+        # l1 will notice its lock was stolen.
+        self.assertRaises(errors.LockBroken,
+            l1.unlock)
+        l2.unlock()
+
+    def test_auto_break_stale_lock_configured_off(self):
+        """Automatic breaking can be turned off"""
+        l1 = LockDir(self.get_transport(), 'a',
+            extra_holder_info={'pid': '12312313'})
+        token_1 = l1.attempt_lock()
+        self.addCleanup(l1.unlock)
+        l2 = LockDir(self.get_transport(), 'a')
+        # This fails now, because dead lock breaking is off by default.
+        self.assertRaises(LockContention,
+            l2.attempt_lock)
+        # and it's in fact not broken
+        l1.confirm()

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2011-05-27 05:16:48 +0000
+++ bzrlib/ui/__init__.py	2011-06-13 21:24:39 +0000
@@ -154,6 +154,8 @@
             "It is recommended that you upgrade by "
             "running the command\n"
             "  bzr upgrade %(basedir)s"),
+        locks_steal_dead=(
+            u"Stole dead lock %(lock_url)s %(other_holder_info)s."),
         )
 
     def __init__(self):
@@ -304,13 +306,13 @@
         try:
             template = self._user_warning_templates[warning_id]
         except KeyError:
-            fail = "failed to format warning %r, %r" % (warning_id, message_args)
-            warnings.warn(fail)   # so tests will fail etc
+            fail = "bzr warning: %r, %r" % (warning_id, message_args)
+            warnings.warn("no template for warning: " + fail)   # so tests will fail etc
             return fail
         try:
             return template % message_args
         except ValueError, e:
-            fail = "failed to format warning %r, %r: %s" % (
+            fail = "bzr unprintable warning: %r, %r, %s" % (
                 warning_id, message_args, e)
             warnings.warn(fail)   # so tests will fail etc
             return fail

=== modified file 'bzrlib/win32utils.py'
--- bzrlib/win32utils.py	2011-05-18 16:42:48 +0000
+++ bzrlib/win32utils.py	2011-06-13 21:24:39 +0000
@@ -576,3 +576,41 @@
         return argv
 else:
     get_unicode_argv = None
+
+
+if has_win32api:
+    def _pywin32_is_local_pid_dead(pid):
+        """True if pid doesn't correspond to live process on this machine"""
+        try:
+            handle = win32api.OpenProcess(1, False, pid) # PROCESS_TERMINATE
+        except pywintypes.error, e:
+            if e[0] == 5: # ERROR_ACCESS_DENIED
+                # Probably something alive we're not allowed to kill
+                return False
+            elif e[0] == 87: # ERROR_INVALID_PARAMETER
+                return True
+            raise
+        handle.close()
+        return False
+    is_local_pid_dead = _pywin32_is_local_pid_dead
+elif has_ctypes and sys.platform == 'win32':
+    from ctypes.wintypes import BOOL, DWORD, HANDLE
+    _kernel32 = ctypes.windll.kernel32
+    _CloseHandle = ctypes.WINFUNCTYPE(BOOL, HANDLE)(
+        ("CloseHandle", _kernel32))
+    _OpenProcess = ctypes.WINFUNCTYPE(HANDLE, DWORD, BOOL, DWORD)(
+        ("OpenProcess", _kernel32))
+    def _ctypes_is_local_pid_dead(pid):
+        """True if pid doesn't correspond to live process on this machine"""
+        handle = _OpenProcess(1, False, pid) # PROCESS_TERMINATE
+        if not handle:
+            errorcode = ctypes.GetLastError()
+            if errorcode == 5: # ERROR_ACCESS_DENIED
+                # Probably something alive we're not allowed to kill
+                return False
+            elif errorcode == 87: # ERROR_INVALID_PARAMETER
+                return True
+            raise ctypes.WinError(errorcode)
+        _CloseHandle(handle)
+        return False
+    is_local_pid_dead = _ctypes_is_local_pid_dead

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-06-13 17:18:13 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-06-13 21:24:39 +0000
@@ -43,6 +43,13 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Bazaar can now detect when a lock file is held by a dead process
+  originating from the same machine, and steal the lock after printing a
+  message to the user.  This is off by default, for safety, but can be
+  turned on by setting the configuration variable ``locks.steal_dead`` to
+  ``True``.
+  (Martin Pool, #220464)
+
 * Credentials in the log output produced by ``-Dhttp`` are masked so users
   can more freely post them in bug reports. (Vincent Ladeuil, #723074)
 
@@ -89,6 +96,10 @@
 .. Changes that may require updates in plugins or other code that uses
    bzrlib.
 
+* Information about held lockdir locks returned from eg `LockDir.peek` is
+  now represented as a `LockHeldInfo` object, rather than a plain Python
+  dict.
+
 Internals
 *********