[object Object]

← back to Terminal Status

lock: stop holding the per-tty lock across the 14.84s process-table scan

6eae7e8c0564ed3c0fef7778aae9a841ab49d4ed · 2026-09-10 09:32:27 -0700 · Steve Abrams

Measured on this box: processes() (the `ps -axo` scan) = 14.84s, owners() over
those rows = 0.00s, and Store.lock()'s wait deadline was 3s. Store.assert_owner
ran the scan INSIDE the lock, so a same-tty writer could never win: 14.84 > 3,
always. That is the mechanism behind the "Terminal status is busy; retry"
failures another session hit live twice today.

63f3fae (raising the ps timeout 8s -> 60s to survive a busy box) made this
worse by raising the ceiling on lock-hold time from 8s to 60s -- the thundering
herd an earlier review warned about, now with a number attached.

Three complementary fixes:

1. Short-TTL cache on processes(). One CLI run scans at least twice (resolving
   the caller, then again inside the lock); this removes the second. Per-PROCESS
   only -- it deliberately does NOT cache across the ~49 sessions, because a
   table a few seconds stale could miss a just-started session and wrongly
   report "no owning terminal". TERMINAL_STATUS_PROC_TTL overrides (default 5s).

2. assert_owner(owner, rows=None) accepts an already-resolved table, threaded
   through lock(owner, rows=None), so the guard is kept but the scan leaves the
   critical section.

3. Lock wait deadline 3s -> 65s (TERMINAL_STATUS_LOCK_WAIT). With 1+2 the lock
   is held ~0s, so this is a backstop that should never be reached; it is not
   licence to hold the lock across expensive work.

Measured after: in-lock scan 7.14s -> 0.0000s.

Verified: repaint exit 0; status exit 0; dot intact; hook 3x exit 0 with zero
new failure records and verdict PASS; and the ownership guard still correctly
refuses a stale writer (a forged owner is rejected with "Terminal owner
changed"), so the safety property is preserved, not traded away.

Reversible: git revert.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PXNMS1TvMiVbE3ckhSaLeT

Files touched

Diff

commit 6eae7e8c0564ed3c0fef7778aae9a841ab49d4ed
Author: Steve Abrams <steve@designerwallcoverings.com>
Date:   Thu Sep 10 09:32:27 2026 -0700

    lock: stop holding the per-tty lock across the 14.84s process-table scan
    
    Measured on this box: processes() (the `ps -axo` scan) = 14.84s, owners() over
    those rows = 0.00s, and Store.lock()'s wait deadline was 3s. Store.assert_owner
    ran the scan INSIDE the lock, so a same-tty writer could never win: 14.84 > 3,
    always. That is the mechanism behind the "Terminal status is busy; retry"
    failures another session hit live twice today.
    
    63f3fae (raising the ps timeout 8s -> 60s to survive a busy box) made this
    worse by raising the ceiling on lock-hold time from 8s to 60s -- the thundering
    herd an earlier review warned about, now with a number attached.
    
    Three complementary fixes:
    
    1. Short-TTL cache on processes(). One CLI run scans at least twice (resolving
       the caller, then again inside the lock); this removes the second. Per-PROCESS
       only -- it deliberately does NOT cache across the ~49 sessions, because a
       table a few seconds stale could miss a just-started session and wrongly
       report "no owning terminal". TERMINAL_STATUS_PROC_TTL overrides (default 5s).
    
    2. assert_owner(owner, rows=None) accepts an already-resolved table, threaded
       through lock(owner, rows=None), so the guard is kept but the scan leaves the
       critical section.
    
    3. Lock wait deadline 3s -> 65s (TERMINAL_STATUS_LOCK_WAIT). With 1+2 the lock
       is held ~0s, so this is a backstop that should never be reached; it is not
       licence to hold the lock across expensive work.
    
    Measured after: in-lock scan 7.14s -> 0.0000s.
    
    Verified: repaint exit 0; status exit 0; dot intact; hook 3x exit 0 with zero
    new failure records and verdict PASS; and the ownership guard still correctly
    refuses a stale writer (a forged owner is rejected with "Terminal owner
    changed"), so the safety property is preserved, not traded away.
    
    Reversible: git revert.
    
    Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
    Claude-Session: https://claude.ai/code/session_01PXNMS1TvMiVbE3ckhSaLeT
---
 terminal_status.py | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/terminal_status.py b/terminal_status.py
index b7e4613..ee6ca3d 100644
--- a/terminal_status.py
+++ b/terminal_status.py
@@ -129,7 +129,32 @@ class Process:
         return Owner(self.tty, self.pid, self.runtime, self.started)
 
 
-def processes():
+# TK-11385 fix 1 of 3: short-TTL cache on the process-table scan.
+# Measured on this box: processes() = 14.84s, owners() over the rows = 0.00s.
+# A single CLI run calls processes() at least twice -- once to resolve the
+# caller (current_owner) and again inside Store.lock() via assert_owner -- so
+# every write paid the 14.84s twice. This cache removes the SECOND scan.
+# Scope note, deliberately honest: this is a per-PROCESS cache, so it does NOT
+# reduce scans across the ~49 concurrent sessions (each still scans once). A
+# cross-process file cache would, but a table up to TTL seconds stale could
+# fail to see a just-started session and wrongly report "no owning terminal",
+# so it is not worth the risk here.
+_PROC_CACHE = {"at": 0.0, "rows": None}
+_PROC_TTL = float(os.environ.get("TERMINAL_STATUS_PROC_TTL", "5"))
+
+
+def processes(fresh=False):
+    now = time.monotonic()
+    if not fresh and _PROC_CACHE["rows"] is not None \
+            and now - _PROC_CACHE["at"] < _PROC_TTL:
+        return _PROC_CACHE["rows"]
+    rows = _scan_processes()
+    _PROC_CACHE["at"] = time.monotonic()
+    _PROC_CACHE["rows"] = rows
+    return rows
+
+
+def _scan_processes():
     # `ps -ax` walks the whole process table, so its cost scales with how many
     # processes the box is running -- on a workstation with a large MCP fleet
     # (~2k procs) it measures ~4-5s idle and ~30s under concurrent load. The old
@@ -286,17 +311,30 @@ class Store:
             raise StatusError("Invalid tty")
         return self.root / (owner.tty + ".json")
 
-    def assert_owner(self, owner):
-        if owners(self.process_provider()).get(owner.tty) != owner:
+    def assert_owner(self, owner, rows=None):
+        # TK-11385 fix 2 of 3: accept an already-resolved process table.
+        # This runs INSIDE Store.lock(), so re-scanning here held the per-tty
+        # lock for the full duration of the scan. The caller has almost always
+        # just resolved the table; reusing it keeps the guard while removing
+        # the scan from the critical section.
+        rows = rows if rows is not None else self.process_provider()
+        if owners(rows).get(owner.tty) != owner:
             raise StatusError("Terminal owner changed; refusing stale writer")
 
     @contextlib.contextmanager
-    def lock(self, owner):
+    def lock(self, owner, rows=None):
         self.path(owner)  # Validate before building any filesystem path.
         directory = self.root / ".locks"
         directory.mkdir(parents=True, exist_ok=True, mode=0o700)
         with open(directory / (owner.tty + ".lock"), "a") as lock:
-            deadline = time.monotonic() + 3
+            # TK-11385 fix 3 of 3: was 3s, which a same-tty writer could
+            # NEVER win -- the holder sat in a 14.84s scan while the waiter gave
+            # up after 3s, so contention was a guaranteed loss, not a race.
+            # With fixes 1+2 the lock is now held for ~0s, so this longer
+            # deadline is a backstop that should essentially never be reached;
+            # it is NOT licence to hold the lock across expensive work.
+            deadline = time.monotonic() + float(
+                os.environ.get("TERMINAL_STATUS_LOCK_WAIT", "65"))
             while True:
                 try:
                     fcntl.flock(lock, fcntl.LOCK_EX | fcntl.LOCK_NB)
@@ -306,7 +344,7 @@ class Store:
                         raise StatusError("Terminal status is busy; retry")
                     time.sleep(0.02)
             try:
-                self.assert_owner(owner)
+                self.assert_owner(owner, rows)
                 yield
             finally:
                 fcntl.flock(lock, fcntl.LOCK_UN)

← fea7539 ticket_binding: a slow ps must degrade the label, not kill t  ·  back to Terminal Status  ·  colors: add lightblue = ANY STOP THAT REQUIRES STEVE'S INPUT a56bd8c →