Repository: borgbackup/borg

Issue:
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.



Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=h1) Report
> Merging [#2924](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=desc) into [master](https://codecov.io/gh/borgbackup/borg/commit/a1840e9238e7024c8a6d37fd41dcff3c001325f4?src=pr&el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `71.42%`.

[![Impacted file tree graph](https://codecov.io/gh/borgbackup/borg/pull/2924/graphs/tree.svg?width=650&height=150&src=pr&token=LssEpmW3o4)](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #2924      +/-   ##
==========================================
+ Coverage   86.21%   86.25%   +0.04%     
==========================================
  Files          23       23              
  Lines        8799     8832      +33     
  Branches     1481     1486       +5     
==========================================
+ Hits         7586     7618      +32     
+ Misses        820      818       -2     
- Partials      393      396       +3
```


| [Impacted Files](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/borg/platform/base.py](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=tree#diff-c3JjL2JvcmcvcGxhdGZvcm0vYmFzZS5weQ==) | `86.74% <100%> (ø)` | :arrow_up: |
| [src/borg/repository.py](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=tree#diff-c3JjL2JvcmcvcmVwb3NpdG9yeS5weQ==) | `87.43% <66.66%> (-0.07%)` | :arrow_down: |
| [src/borg/locking.py](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=tree#diff-c3JjL2JvcmcvbG9ja2luZy5weQ==) | `85.38% <69.23%> (-1.8%)` | :arrow_down: |
| [src/borg/helpers.py](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=tree#diff-c3JjL2JvcmcvaGVscGVycy5weQ==) | `87.86% <80%> (+0.03%)` | :arrow_up: |
| [src/borg/archive.py](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=tree#diff-c3JjL2JvcmcvYXJjaGl2ZS5weQ==) | `83.76% <0%> (+0.62%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=footer). Last update [a1840e9...c1d1832](https://codecov.io/gh/borgbackup/borg/pull/2924?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/borg/fuse.py b/src/borg/fuse.py
index 7868eb37..129e3b20 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 faf63cec..a226b228 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 31495791..14209c5b 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 c3d69674..0fc09275 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"""
+        assert 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"""
+        assert 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):
+        assert 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 ba938e66..f56c03be 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 f13b3662..b7e439ab 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 4b389821..3344de16 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 4352d864..7c3cf1b0 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 b8689e2c..0144e1aa 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 b443bdb4..a61e4e94 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):


Test patch:
diff --git a/src/borg/testsuite/locking.py b/src/borg/testsuite/locking.py
index fe8676d4..64a79281 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}