Repository: borgbackup/borg
Pull request: migrate locks to child PID when daemonize is used
fixes #2923 

the pid change due to daemonize didn't work well with the stale lock detection (as the initial/parent process was gone). thus we better transfer the locks to the new pid.

this is only needed for client-side (local) repos because the daemonize happens there.
remote repos are locked by borg serve and the pid problem does not happen there.

Problem statement:
lock.roster left behind after umount in 1.1.0 rc1
I have been testing v1.1.0 rc1 and found the following issue:
Before doing a backup I check if a backup is neccessary by mounting the repo and comparing the latest archive with the source.
Then the repo is unmounted.
At the beginning of the next backup I get a message that lock.roster files are removed for PID ...
A workaround is doing a borg break-lock after each borg umount.

Hints from issue discussion:
```
$ borg -V
borg 1.1.0rc1
$ borg mount repo mnt
$ ls mnt
1  2  new  testing
$ borg umount mnt
$ ls -l repo
total 668
-rw------- 1 tw tw    209 Jul 21 22:57 config
drwx------ 3 tw tw   4096 Jul 21 22:57 data
-rw------- 1 tw tw     64 Aug  6 02:09 hints.17
-rw------- 1 tw tw 656458 Aug  6 02:09 index.17
-rw------- 1 tw tw    190 Aug  6 02:09 integrity.17
-rw------- 1 tw tw     46 Aug  6 23:59 lock.roster
-rw------- 1 tw tw     73 Jul 21 22:57 README
$ borg create repo::checkingthelock .
Removed stale shared roster lock for pid 7081.
Removed stale shared roster lock for pid 7081.
```
does not happen when `borg mount -f` is used.
Hmm, the stale detection is malfunctioning if borg mount forks into the background:
```
$ borg mount repo mnt
$ borg create repo::checkingthelock7 .
Removed stale shared roster lock for pid 8289.
Removed stale shared roster lock for pid 8289.
```
Here it is killing the lock although the borg mount is still alive (just not having pid 8289 any more, but the forked process' pid).
from platform/posix.py:
```
def get_process_id():
    """
    Return identification tuple (hostname, pid, thread_id) for 'us'. If this is a FUSE process, then the PID will be
    that of the parent, not the forked FUSE child.
```
Well, that can't work with the stale detection...

@cwygerde thanks for finding this.

this was undiscovered for long as it only affected borg mount (no other command forks into the background).
also, it only happens when the stale lock killer is active (which is the default since a while in 1.1).

a fix is in PR #2924.
I think you are fixing another (more critical) bug in #2924.

This ticket initially describes 1.1 leaving lock.roster behind after umount, 1.0 does not do this.
It has both the same root cause, that the PID in the lock was not the alive process, but the dead grandparent's PID. The code worked ok even with that, except when stale lock detection was active.
running the code from #2924:

```
$ borg -V
borg 1.2.0.dev35+gfe7c101
$ borg mount repo mnt
$ ls mnt
1  checkingthelock    checkingthelock11  checkingthelock13  checkingthelock3  checkingthelock5  checkingthelock7  new
2  checkingthelock10  checkingthelock12  checkingthelock2   checkingthelock4  checkingthelock6  checkingthelock8  testing
$ borg umount mnt
$ ls -l repo
total 664
-rw------- 1 tw tw    209 Jul 21 22:57 config
drwx------ 3 tw tw   4096 Jul 21 22:57 data
-rw------- 1 tw tw     66 Aug  7 05:01 hints.65
-rw------- 1 tw tw 656458 Aug  7 05:01 index.65
-rw------- 1 tw tw    190 Aug  7 05:01 integrity.65
-rw------- 1 tw tw     73 Jul 21 22:57 README
$ borg create repo::checkingthelock15 scripts/
```

```
$ borg mount repo mnt
$ borg create repo::checkingthelock16 scripts/
Failed to create/acquire the lock /home/tw/w/borg/repo/lock (timeout).
```

Commit message: migrate locks to child PID when daemonize is used

also:

increase platform api version due to change in get_process_id behaviour.

Patch to review:
diff --git a/src/borg/fuse.py b/src/borg/fuse.py
index 7868eb3782..129e3b20b7 100644
--- a/src/borg/fuse.py
+++ b/src/borg/fuse.py
@@ -22,6 +22,7 @@
 from .helpers import daemonize, hardlinkable, signal_handler, format_file_size
 from .item import Item
 from .lrucache import LRUCache
+from .remote import RemoteRepository
 
 # Does this version of llfuse support ns precision?
 have_fuse_xtime_ns = hasattr(llfuse.EntryAttributes, 'st_mtime_ns')
@@ -285,7 +286,10 @@ def mount(self, mountpoint, mount_options, foreground=False):
         self._create_filesystem()
         llfuse.init(self, mountpoint, options)
         if not foreground:
-            daemonize()
+            old_id, new_id = daemonize()
+            if not isinstance(self.repository_uncached, RemoteRepository):
+                # local repo and the locking process' PID just changed, migrate it:
+                self.repository_uncached.migrate_lock(old_id, new_id)
 
         # If the file system crashes, we do not want to umount because in that
         # case the mountpoint suddenly appears to become empty. This can have
diff --git a/src/borg/helpers/checks.py b/src/borg/helpers/checks.py
index faf63cecdc..a226b22810 100644
--- a/src/borg/helpers/checks.py
+++ b/src/borg/helpers/checks.py
@@ -28,7 +28,7 @@ def check_extension_modules():
         raise ExtensionModuleError
     if borg.crypto.low_level.API_VERSION != '1.1_02':
         raise ExtensionModuleError
-    if platform.API_VERSION != platform.OS_API_VERSION != '1.1_01':
+    if platform.API_VERSION != platform.OS_API_VERSION != '1.1_02':
         raise ExtensionModuleError
     if item.API_VERSION != '1.1_03':
         raise ExtensionModuleError
diff --git a/src/borg/helpers/process.py b/src/borg/helpers/process.py
index 314957916f..14209c5b55 100644
--- a/src/borg/helpers/process.py
+++ b/src/borg/helpers/process.py
@@ -11,7 +11,12 @@
 
 
 def daemonize():
-    """Detach process from controlling terminal and run in background"""
+    """Detach process from controlling terminal and run in background
+
+    Returns: old and new get_process_id tuples
+    """
+    from ..platform import get_process_id
+    old_id = get_process_id()
     pid = os.fork()
     if pid:
         os._exit(0)
@@ -27,6 +32,8 @@ def daemonize():
     os.dup2(fd, 0)
     os.dup2(fd, 1)
     os.dup2(fd, 2)
+    new_id = get_process_id()
+    return old_id, new_id
 
 
 class SignalException(BaseException):
diff --git a/src/borg/locking.py b/src/borg/locking.py
index c3d69674aa..c21518d93d 100644
--- a/src/borg/locking.py
+++ b/src/borg/locking.py
@@ -202,6 +202,16 @@ def break_lock(self):
                 os.unlink(os.path.join(self.path, name))
             os.rmdir(self.path)
 
+    def migrate_lock(self, old_id, new_id):
+        """migrate the lock ownership from old_id to new_id"""
+        if self.id == old_id:
+            new_unique_name = os.path.join(self.path, "%s.%d-%x" % new_id)
+            if self.is_locked() and self.by_me():
+                with open(new_unique_name, "wb"):
+                    pass
+                os.unlink(self.unique_name)
+            self.id, self.unique_name = new_id, new_unique_name
+
 
 class LockRoster:
     """
@@ -271,6 +281,25 @@ def modify(self, key, op):
         roster[key] = list(list(e) for e in elements)
         self.save(roster)
 
+    def migrate_lock(self, key, old_id, new_id):
+        """migrate the lock ownership from old_id to new_id"""
+        if self.id == old_id:
+            # need to temporarily switch off stale lock killing as we want to
+            # rather migrate than kill them (at least the one made by old_id).
+            killing, self.kill_stale_locks = self.kill_stale_locks, False
+            try:
+                try:
+                    self.modify(key, REMOVE)
+                except KeyError:
+                    # entry was not there, so no need to add a new one, but still update our id
+                    self.id = new_id
+                else:
+                    # old entry removed, update our id and add a updated entry
+                    self.id = new_id
+                    self.modify(key, ADD)
+            finally:
+                self.kill_stale_locks = killing
+
 
 class Lock:
     """
@@ -373,3 +402,14 @@ def got_exclusive_lock(self):
     def break_lock(self):
         self._roster.remove()
         self._lock.break_lock()
+
+    def migrate_lock(self, old_id, new_id):
+        if self.id == old_id:
+            self.id = new_id
+            if self.is_exclusive:
+                self._lock.migrate_lock(old_id, new_id)
+                self._roster.migrate_lock(EXCLUSIVE, old_id, new_id)
+            else:
+                with self._lock:
+                    self._lock.migrate_lock(old_id, new_id)
+                    self._roster.migrate_lock(SHARED, old_id, new_id)
diff --git a/src/borg/platform/base.py b/src/borg/platform/base.py
index ba938e66a1..f56c03be48 100644
--- a/src/borg/platform/base.py
+++ b/src/borg/platform/base.py
@@ -15,7 +15,7 @@
 are correctly composed into the base functionality.
 """
 
-API_VERSION = '1.1_01'
+API_VERSION = '1.1_02'
 
 fdatasync = getattr(os, 'fdatasync', os.fsync)
 
diff --git a/src/borg/platform/darwin.pyx b/src/borg/platform/darwin.pyx
index f13b366207..b7e439ab67 100644
--- a/src/borg/platform/darwin.pyx
+++ b/src/borg/platform/darwin.pyx
@@ -4,7 +4,7 @@ from ..helpers import user2uid, group2gid
 from ..helpers import safe_decode, safe_encode
 from .posix import swidth
 
-API_VERSION = '1.1_01'
+API_VERSION = '1.1_02'
 
 cdef extern from "sys/acl.h":
     ctypedef struct _acl_t:
diff --git a/src/borg/platform/freebsd.pyx b/src/borg/platform/freebsd.pyx
index 4b38982125..3344de1658 100644
--- a/src/borg/platform/freebsd.pyx
+++ b/src/borg/platform/freebsd.pyx
@@ -4,7 +4,7 @@ from ..helpers import posix_acl_use_stored_uid_gid
 from ..helpers import safe_encode, safe_decode
 from .posix import swidth
 
-API_VERSION = '1.1_01'
+API_VERSION = '1.1_02'
 
 cdef extern from "errno.h":
     int errno
diff --git a/src/borg/platform/linux.pyx b/src/borg/platform/linux.pyx
index 4352d864a2..7c3cf1b0b5 100644
--- a/src/borg/platform/linux.pyx
+++ b/src/borg/platform/linux.pyx
@@ -13,7 +13,7 @@ from .posix import swidth
 from libc cimport errno
 from libc.stdint cimport int64_t
 
-API_VERSION = '1.1_01'
+API_VERSION = '1.1_02'
 
 cdef extern from "sys/types.h":
     int ACL_TYPE_ACCESS
diff --git a/src/borg/platform/posix.pyx b/src/borg/platform/posix.pyx
index b8689e2c29..0144e1aa3b 100644
--- a/src/borg/platform/posix.pyx
+++ b/src/borg/platform/posix.pyx
@@ -18,23 +18,21 @@ def swidth(s):
         return str_len
 
 
-# only determine the PID and hostname once.
-# for FUSE mounts, we fork a child process that needs to release
-# the lock made by the parent, so it needs to use the same PID for that.
-_pid = os.getpid()
+# for performance reasons, only determine the hostname once.
 # XXX this sometimes requires live internet access for issuing a DNS query in the background.
 _hostname = '%s@%s' % (socket.getfqdn(), uuid.getnode())
 
 
 def get_process_id():
     """
-    Return identification tuple (hostname, pid, thread_id) for 'us'. If this is a FUSE process, then the PID will be
-    that of the parent, not the forked FUSE child.
+    Return identification tuple (hostname, pid, thread_id) for 'us'.
+    This always returns the current pid, which might be different from before, e.g. if daemonize() was used.
 
     Note: Currently thread_id is *always* zero.
     """
     thread_id = 0
-    return _hostname, _pid, thread_id
+    pid = os.getpid()
+    return _hostname, pid, thread_id
 
 
 def process_alive(host, pid, thread):
diff --git a/src/borg/repository.py b/src/borg/repository.py
index b443bdb4ef..a61e4e9491 100644
--- a/src/borg/repository.py
+++ b/src/borg/repository.py
@@ -346,6 +346,11 @@ def get_transaction_id(self):
     def break_lock(self):
         Lock(os.path.join(self.path, 'lock')).break_lock()
 
+    def migrate_lock(self, old_id, new_id):
+        # note: only needed for local repos
+        if self.lock is not None:
+            self.lock.migrate_lock(old_id, new_id)
+
     def open(self, path, exclusive, lock_wait=None, lock=True):
         self.path = path
         if not os.path.isdir(path):
diff --git a/src/borg/testsuite/locking.py b/src/borg/testsuite/locking.py
index fe8676d4a4..64a7928116 100644
--- a/src/borg/testsuite/locking.py
+++ b/src/borg/testsuite/locking.py
@@ -3,6 +3,7 @@
 
 import pytest
 
+from ..helpers import daemonize
 from ..platform import get_process_id, process_alive
 from ..locking import TimeoutTimer, ExclusiveLock, Lock, LockRoster, \
                       ADD, REMOVE, SHARED, EXCLUSIVE, LockTimeout, NotLocked, NotMyLock
@@ -76,6 +77,19 @@ def test_kill_stale(self, lockpath, free_pid):
             with pytest.raises(LockTimeout):
                 ExclusiveLock(lockpath, id=our_id, kill_stale_locks=True, timeout=0.1).acquire()
 
+    def test_migrate_lock(self, lockpath):
+        old_id, new_id = ID1, ID2
+        assert old_id[1] != new_id[1]  # different PIDs (like when doing daemonize())
+        lock = ExclusiveLock(lockpath, id=old_id).acquire()
+        assert lock.id == old_id  # lock is for old id / PID
+        old_unique_name = lock.unique_name
+        assert lock.by_me()  # we have the lock
+        lock.migrate_lock(old_id, new_id)  # fix the lock
+        assert lock.id == new_id  # lock corresponds to the new id / PID
+        new_unique_name = lock.unique_name
+        assert lock.by_me()  # we still have the lock
+        assert old_unique_name != new_unique_name  # locking filename is different now
+
 
 class TestLock:
     def test_shared(self, lockpath):
@@ -155,6 +169,22 @@ def test_kill_stale(self, lockpath, free_pid):
             with pytest.raises(LockTimeout):
                 Lock(lockpath, id=our_id, kill_stale_locks=True, timeout=0.1).acquire()
 
+    def test_migrate_lock(self, lockpath):
+        old_id, new_id = ID1, ID2
+        assert old_id[1] != new_id[1]  # different PIDs (like when doing daemonize())
+
+        lock = Lock(lockpath, id=old_id, exclusive=True).acquire()
+        assert lock.id == old_id
+        lock.migrate_lock(old_id, new_id)  # fix the lock
+        assert lock.id == new_id
+        lock.release()
+
+        lock = Lock(lockpath, id=old_id, exclusive=False).acquire()
+        assert lock.id == old_id
+        lock.migrate_lock(old_id, new_id)  # fix the lock
+        assert lock.id == new_id
+        lock.release()
+
 
 @pytest.fixture()
 def rosterpath(tmpdir):
@@ -207,3 +237,14 @@ def test_kill_stale(self, rosterpath, free_pid):
         other_killer_roster = LockRoster(rosterpath, kill_stale_locks=True)
         # Did not kill us, since we're alive
         assert other_killer_roster.get(SHARED) == {our_id, cant_know_if_dead_id}
+
+    def test_migrate_lock(self, rosterpath):
+        old_id, new_id = ID1, ID2
+        assert old_id[1] != new_id[1]  # different PIDs (like when doing daemonize())
+        roster = LockRoster(rosterpath, id=old_id)
+        assert roster.id == old_id
+        roster.modify(SHARED, ADD)
+        assert roster.get(SHARED) == {old_id}
+        roster.migrate_lock(SHARED, old_id, new_id)  # fix the lock
+        assert roster.id == new_id
+        assert roster.get(SHARED) == {new_id}


Code review comments:
diff_hunk: @@ -202,6 +202,16 @@ def break_lock(self):
                 os.unlink(os.path.join(self.path, name))
             os.rmdir(self.path)
 
+    def migrate_lock(self, old_id, new_id):
+        """migrate the lock ownership from old_id to new_id"""
+        if self.id == old_id:
path: src/borg/locking.py
text: @user1:
I don't quite see how this could be false? Perhaps make it an `assert self.id == old_id` instead?

@author:
well, it could be wrong if the caller does something wrong.
yes, could be also an assertion.