Fix #1127: switch off takeover of a lock whose pid file cannot be read (v0.45.2)

changelog.d/1119.fixed.md

@@ -8,9 +8,9 @@
   `save.lock`/`consolidation.lock`/`staging.lock` away from a holder that
   was still alive, 60x sooner than any other takeover path allows (#1119).
   A fast read failure is now treated the same as a read that never
-  completed: refused on its own, but still recoverable through the
-  existing age-gated takeover once the lock directory is older than
-  `REMEMBER_LOCK_MAX_HOLD_SECONDS`.
+  completed: refused on its own. (The age-gated takeover this originally
+  fell back to for a persistently unreadable pid file was itself removed
+  in the same release -- see #1127.)
 - Corrects the v0.45.1 changelog entry for #1101/#1103, which claimed a
   confirmed-empty pid file is "reclaimed too, exactly as it already was for
   the no-pid-file-at-all case" -- not true for a pid file that reads empty

changelog.d/1122.fixed.md

@@ -10,10 +10,9 @@
   the winner's `save.lock`/`consolidation.lock`/`staging.lock` and
   producing two simultaneous holders -- the same class #1119/#1120 shipped
   as a non-blocking trap.d fragment one release earlier and then had to
-  patch immediately as a regression. The re-read now also compares the
-  renamed file's inode against what was captured just before the rename:
-  a match is the same file whose first read already failed too, safe to
-  steal; a mismatch, or either lookup coming back empty, hands back rather
-  than guess -- `stat` needs no read permission on the file's content,
-  unlike the re-read itself, so this works even when neither pid file is
-  readable (#1122).
+  patch immediately as a regression. The re-read compared the renamed
+  file's inode against what was captured just before the rename, and
+  `stat` needs no read permission on the file's content to do that, unlike
+  the re-read itself (#1122). (This whole takeover path -- the one this
+  inode check protected -- was removed in the same release after five
+  review rounds kept finding new two-holder races in it; see #1127.)

changelog.d/1127.removed.md

@@ -0,0 +1,74 @@
+- Removed: automatic takeover of a save/consolidation/staging lock whose
+  pid file cannot be read (#1127) -- the dataless cloud-sync placeholder case
+  (OneDrive, iCloud: the remote copy of a synced file was deleted after
+  the placeholder itself synced). Since v0.45.1 (#1101, #1103), a lock
+  left in that state was eventually taken over automatically once it had
+  sat untouched long enough. Five review rounds in a row (#1119, #1122,
+  #1124, #1125, and the reviews on pull request #1126) each found a new
+  way for two processes to end up believing they both held the same
+  lock through that takeover, however it was tightened in response to
+  the previous finding. Rather than continue patching an unsafe design,
+  this takeover is switched off: a pid file that cannot be read, at any
+  age, is refused rather than stolen or adopted, the same as before
+  v0.45.1. Two further review rounds on this fix's own pull request
+  (#1129) found three more races in the behaviour this change keeps --
+  closed in the same set of commits: an EMPTY pid file (not merely
+  unreadable) is now also never grounds to steal, at any age, because
+  every legitimate winner's own pid write passes through a briefly-empty
+  state that an unrelated dead holder's permanently-empty pid file is
+  indistinguishable from; releasing a lock now refuses whenever its own
+  read of the pid file comes back empty, rather than treating an empty
+  read as "ownership unproven, but proceed anyway"; and every place
+  this file restores a claim it took back (an abandoned-claim cleanup,
+  a failed steal's hand-back, a confirmed-stealer's pid put-back during
+  release) now does so with a hard link instead of a plain rename, so
+  it can never silently overwrite a pid a faster process wrote into the
+  same spot in the meantime.
+  How you would notice this happening: a save, a consolidation round,
+  or an append to today's running notes quietly stops completing --
+  nothing crashes and nothing shows an error unless debug logging is
+  on, so the way most people notice is that nothing new is being
+  remembered for a while.
+
+  Before touching anything, make sure it is actually safe to clear a
+  lock: closing the Claude window is not enough on its own, because
+  saving and consolidating can both keep running in the background for
+  a short while after you close it (consolidation in particular can
+  take up to three minutes). Close Claude, then wait at least five
+  minutes before clearing anything. If you want to double-check rather
+  than just wait: on macOS or Linux, open a terminal and run
+  `ps aux | grep -i save-session` and `ps aux | grep -i run-consolidation`
+  -- if either prints a matching line (other than the `grep` command
+  itself), one is still running and you should wait longer. On Windows,
+  open Task Manager, go to "Details", and look for a process whose
+  command line mentions `save-session` or `run-consolidation` (the
+  "Command line" column may need to be added via "Select columns").
+
+  Finding the right folder to clear takes one more step, because where
+  memory lives is not always the same folder your project is in -- it
+  can be redirected elsewhere, and some project setups store memory
+  somewhere other than the folder you have open. The reliable way to
+  find it: run the `/remember:doctor` command -- it prints a line
+  starting with `REMEMBER_DIR` naming the exact folder. (The same
+  folder also shows up as `REMEMBER_DIR=...` in this plugin's own
+  debug log, if you have that turned on.) Inside that folder is a
+  `tmp` subfolder, and inside `tmp` are the three lock folders this can
+  affect: `save.lock`, `consolidation.lock` and `staging.lock`. Once
+  you have confirmed nothing is still running, delete whichever of
+  those three folders exists (just that one small lock folder, not
+  anything else in the memory folder) and the next save or
+  consolidation will work normally again.
+  Everything else from v0.45.1 is unchanged: the bounded (2-second) pid
+  read, reclaiming a dead holder's pid file that reads back as garbage
+  (non-numeric, but not empty), the Ubuntu 26.04 `noclobber` guard, and
+  the age-gated takeover of a holder that is still alive but has held
+  the lock past `REMEMBER_LOCK_MAX_HOLD_SECONDS`. That last one still
+  requires a pid file that reads back successfully and non-empty, and
+  is kept because both sides of it (the steal and the ordinary
+  release) resolve their race through the same atomic-rename mechanism
+  already proven for the confirmed-dead-holder case, now with the
+  empty-read ambiguity above closed on both sides too.
+- Compatibility: compatible - a lock that used to recover on its own
+  after a long wait now instead needs a manual delete of the lock
+  folder; no command, config key or file format changes, and no
+  caller of `lock_acquire`/`lock_release` changes its own interface.