← back to Terminal Status
TK-11831: harden the ps-enrichment cache against the codex-check findings
c6b317d72a67f2bffcb669c66ba4049356edad73 · 2026-09-16 14:14:32 -0700 · Steve Abrams
A second-model adversarial pass (gpt-5.3-codex, per the standing codex-check
rule) found real defects in 28cf4f6 -- two of which produce a WRONG ticket
binding rather than merely a missed cache, i.e. the confidently-wrong-dot
class. Fixed, each with a test that is mutation-verified to go RED:
1. STICKY NEGATIVE CACHE. A pid absent from the ps output (it exited mid-call,
or ps was truncated) was cached as "" and then never re-read, so ONE
transient miss left that tab unbound for the whole TTL. The uncached path
retried on the very next paint -- a cache must never be WORSE than what it
replaces. Now only a CONCLUSIVE read is cached: for argv the pid must have
come back with a non-empty args; for env the pid must be present AND its
args prefix must match (so the remainder really is the environment). A
settled pid with genuinely no TERM_SESSION_ID is a real answer and is still
cached, so those pids stop re-ps'ing and the win is kept.
2. POISON OVERWRITE / LOST UPDATE. ~68 processes read-modify-write this file
with no lock. os.replace gives an untearable READ but does not serialise
RMW, so a slow writer merging onto its START-OF-PASS snapshot could drop --
or blank -- an entry a peer had already learned. Now the merge re-reads the
file at write time and NEVER downgrades: an empty value cannot overwrite a
non-empty one. Values are immutable per process, so two writers can only
ever agree; that makes concurrent writes commutative and lock-free without
a CAS, because no writer can lose information.
3. UNVALIDATED CACHE VALUES. The uncached path can only ever put a str into
the argv map, so a hand-edited {"a": null} served as a hit was a cache
answer DIFFERING from the uncached one. Values are now type-checked on read
and anything unexpected is simply not a hit.
4. KEY now includes runtime as well as pid+start, so a pid that exec'd into a
different runtime misses instead of serving the previous image's argv.
5. Cache file written 0600 (it holds argv snippets + TERM_SESSION_ID), and the
TTL comment corrected: a steady all-hit pass writes nothing and therefore
does NOT refresh `t`. The TTL is a hygiene bound, not a correctness one --
identity is pinned by the key, and expiry costs ONE batched ps over the
missing pids, not one per session.
TESTS: 49 pass (was 45; +4). All SEVEN mutations go RED on throwaway copies --
M1 pid-reuse key, M2 cache bypassed, M3 lossy env, M4 sticky negative,
M5 poison overwrite, M6 stale-snapshot merge, M7 unvalidated values. Control
GREEN. A positive-only test on a cache proves nothing.
A/B re-measured after hardening, alternating under live load 44-62 (n=6):
ORIG mean 1.79s worst 3.74s -> NEW mean 0.64s worst 0.91s
bindings diverged on 0/6 rows.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014BD2ty1QgqWc2QVUVtYPRQ
Files touched
M test_terminal_status.pyM ticket_binding.py
Diff
commit c6b317d72a67f2bffcb669c66ba4049356edad73
Author: Steve Abrams <steve@designerwallcoverings.com>
Date: Wed Sep 16 14:14:32 2026 -0700
TK-11831: harden the ps-enrichment cache against the codex-check findings
A second-model adversarial pass (gpt-5.3-codex, per the standing codex-check
rule) found real defects in 28cf4f6 -- two of which produce a WRONG ticket
binding rather than merely a missed cache, i.e. the confidently-wrong-dot
class. Fixed, each with a test that is mutation-verified to go RED:
1. STICKY NEGATIVE CACHE. A pid absent from the ps output (it exited mid-call,
or ps was truncated) was cached as "" and then never re-read, so ONE
transient miss left that tab unbound for the whole TTL. The uncached path
retried on the very next paint -- a cache must never be WORSE than what it
replaces. Now only a CONCLUSIVE read is cached: for argv the pid must have
come back with a non-empty args; for env the pid must be present AND its
args prefix must match (so the remainder really is the environment). A
settled pid with genuinely no TERM_SESSION_ID is a real answer and is still
cached, so those pids stop re-ps'ing and the win is kept.
2. POISON OVERWRITE / LOST UPDATE. ~68 processes read-modify-write this file
with no lock. os.replace gives an untearable READ but does not serialise
RMW, so a slow writer merging onto its START-OF-PASS snapshot could drop --
or blank -- an entry a peer had already learned. Now the merge re-reads the
file at write time and NEVER downgrades: an empty value cannot overwrite a
non-empty one. Values are immutable per process, so two writers can only
ever agree; that makes concurrent writes commutative and lock-free without
a CAS, because no writer can lose information.
3. UNVALIDATED CACHE VALUES. The uncached path can only ever put a str into
the argv map, so a hand-edited {"a": null} served as a hit was a cache
answer DIFFERING from the uncached one. Values are now type-checked on read
and anything unexpected is simply not a hit.
4. KEY now includes runtime as well as pid+start, so a pid that exec'd into a
different runtime misses instead of serving the previous image's argv.
5. Cache file written 0600 (it holds argv snippets + TERM_SESSION_ID), and the
TTL comment corrected: a steady all-hit pass writes nothing and therefore
does NOT refresh `t`. The TTL is a hygiene bound, not a correctness one --
identity is pinned by the key, and expiry costs ONE batched ps over the
missing pids, not one per session.
TESTS: 49 pass (was 45; +4). All SEVEN mutations go RED on throwaway copies --
M1 pid-reuse key, M2 cache bypassed, M3 lossy env, M4 sticky negative,
M5 poison overwrite, M6 stale-snapshot merge, M7 unvalidated values. Control
GREEN. A positive-only test on a cache proves nothing.
A/B re-measured after hardening, alternating under live load 44-62 (n=6):
ORIG mean 1.79s worst 3.74s -> NEW mean 0.64s worst 0.91s
bindings diverged on 0/6 rows.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014BD2ty1QgqWc2QVUVtYPRQ
---
test_terminal_status.py | 56 ++++++++++++++++++++++++
ticket_binding.py | 111 +++++++++++++++++++++++++++++++++++++-----------
2 files changed, 143 insertions(+), 24 deletions(-)
diff --git a/test_terminal_status.py b/test_terminal_status.py
index 0edeb3f..8e30485 100644
--- a/test_terminal_status.py
+++ b/test_terminal_status.py
@@ -716,6 +716,62 @@ class EnrichmentCacheTests(unittest.TestCase):
self.cache.write_text(junk)
self.assertEqual(self.discover()[0], expected, "corrupt cache: " + repr(junk))
+ def test_inconclusive_read_is_never_cached_as_a_negative(self):
+ # CODEX-CHECK FINDING (sticky negative cache). If a pid is absent from the
+ # ps output -- it exited mid-call, or ps was truncated -- that is "not yet
+ # known", NOT "known to have no argv". Caching "" for it would make ONE
+ # transient miss STICKY for the whole TTL, leaving that tab unbound for an
+ # hour, where the uncached path simply retried on the next paint. A cache
+ # must never be WORSE than the thing it replaces.
+ real = tb.subprocess.run
+
+ def blind(cmd, *a, **kw):
+ out = real(cmd, *a, **kw)
+ if isinstance(cmd, (list, tuple)) and cmd and cmd[0] == "ps":
+ return subprocess.CompletedProcess(cmd, 0, "", "") # pid absent
+ return out
+ with patch.object(tb.subprocess, "run", blind):
+ self.discover()
+ spy, calls = self.counted()
+ with spy:
+ self.discover()
+ self.assertEqual(len(calls), 2,
+ "an inconclusive read must be retried, not cached as empty")
+
+ def test_merge_never_downgrades_a_peer_value(self):
+ # CODEX-CHECK FINDING (poison overwrite). ~68 processes read-modify-write
+ # this file with no lock. A slow writer merging onto its START-OF-PASS
+ # snapshot could blank an entry a peer had already learned -- and a blanked
+ # entry is a WRONG ticket binding, not merely a missed cache. Values are
+ # immutable per process, so "never downgrade" makes concurrent writes
+ # commutative: no writer can lose information, lock-free.
+ tb._enrich_cache_merge({"K": {"a": "claude TK-1", "s": "REAL"}})
+ tb._enrich_cache_merge({"K": {"a": "", "s": ""}}) # the poison pass
+ kept = tb._enrich_cache_read()["K"]
+ self.assertEqual(kept["a"], "claude TK-1")
+ self.assertEqual(kept["s"], "REAL", "empty must never overwrite a real answer")
+
+ def test_merge_reads_fresh_and_keeps_a_concurrent_peers_entry(self):
+ # The other half of the same race: merging must start from the file as it
+ # is NOW, not from this pass's stale snapshot, or a peer's brand-new key is
+ # silently dropped by the full-file replace.
+ stale = tb._enrich_cache_read() # empty snapshot, as a slow pass holds
+ tb._enrich_cache_merge({"PEER": {"s": "PEER-TSID"}}) # peer lands mid-flight
+ tb._enrich_cache_merge({"MINE": {"s": "MY-TSID"}}) # our late write-back
+ self.assertEqual(stale, {})
+ after = tb._enrich_cache_read()
+ self.assertIn("PEER", after, "a concurrent peer's entry must survive our write")
+ self.assertIn("MINE", after)
+
+ def test_non_string_cache_values_are_not_served(self):
+ # CODEX-CHECK FINDING: the uncached path can only ever put a str into the
+ # argv map. A hand-edited {"a": null} served as a hit would be a cache
+ # answer that DIFFERS from the uncached one -- the one thing this may
+ # never do -- and would blow up downstream string ops.
+ self.cache.write_text(json.dumps({"entries": {
+ "K": {"a": None, "s": 7, "t": tb.time.time()}}}))
+ self.assertEqual(tb._enrich_cache_read(), {}, "typed junk is not a hit")
+
def test_stale_entries_age_out(self):
# A pid-keyed cache on a box up for days must not grow without bound or
# serve an entry whose process is long gone; the TTL is the backstop.
diff --git a/ticket_binding.py b/ticket_binding.py
index c48299a..93c3f24 100644
--- a/ticket_binding.py
+++ b/ticket_binding.py
@@ -39,13 +39,20 @@ _PS_TIMEOUT = float(os.environ.get("TERMINAL_STATUS_PS_TIMEOUT", "90"))
_PS_CACHE_PATH = os.environ.get(
"TERMINAL_STATUS_PS_CACHE",
os.path.join(tempfile.gettempdir(), "terminal-status-psenrich-%d.json" % os.getuid()))
+# The TTL is a HYGIENE bound (stop a days-up box hoarding entries), NOT a
+# correctness one: the key already pins identity, so a dead pid's entry is
+# simply never looked up again. Note a steady all-hit pass writes nothing and
+# therefore does NOT refresh `t` -- entries age out on when they were LEARNED.
+# That is fine and deliberate: expiry costs one batched `ps` over the missing
+# pids, not one per session.
_PS_CACHE_TTL = float(os.environ.get("TERMINAL_STATUS_PS_CACHE_TTL", "3600"))
_PS_CACHE_MAX = 512
def _enrich_key(owner):
- """pid + start time: a recycled pid misses instead of reading a stale argv."""
- return "%d:%s" % (owner.pid, owner.started)
+ """pid + start time + runtime: a recycled pid (or a pid that exec'd into a
+ different runtime) MISSES instead of serving the previous process's argv."""
+ return "%d:%s:%s" % (owner.pid, owner.started, owner.runtime)
def _enrich_cache_read():
@@ -56,20 +63,66 @@ def _enrich_cache_read():
entries = blob.get("entries") if isinstance(blob, dict) else None
if not isinstance(entries, dict):
return {}
+ # Validate the VALUES, not just the container. A hand-edited or partly
+ # written file with {"a": null} would otherwise be served straight into
+ # the argv map, where the uncached path can only ever put a str -- a
+ # cache hit that differs from the uncached answer, which is the one
+ # thing this cache may never do. Anything unexpected is simply not a hit.
floor = time.time() - _PS_CACHE_TTL
- return {k: v for k, v in entries.items()
- if isinstance(v, dict) and float(v.get("t", 0)) > floor}
+ clean = {}
+ for key, value in entries.items():
+ if not isinstance(value, dict):
+ continue
+ try:
+ if float(value.get("t", 0)) <= floor:
+ continue
+ except (TypeError, ValueError):
+ continue
+ kept = {f: value[f] for f in ("a", "s")
+ if isinstance(value.get(f), str)}
+ if kept:
+ kept["t"] = value["t"]
+ clean[key] = kept
+ return clean
except (OSError, ValueError, TypeError, AttributeError):
return {}
-def _enrich_cache_write(entries):
- """Atomic replace, so ~68 concurrent writers can never read a torn file."""
+def _enrich_cache_merge(learned):
+ """Publish `learned` without ever clobbering a peer's better answer.
+
+ `os.replace` gives an untearable READ, but it does not serialise a
+ read-modify-write across ~68 processes. Merging onto the snapshot this pass
+ STARTED with would let a slow writer replace the whole file and silently
+ drop -- or worse, blank -- an entry a peer had already learned. A lost key
+ only costs an extra `ps`; a blanked one costs a WRONG ticket binding.
+
+ Two properties make that impossible here without a lock. First, re-read the
+ file NOW rather than reusing the start-of-pass snapshot. Second, never
+ downgrade: a stored non-empty value is never replaced (values are immutable
+ per process, so two writers can only ever agree) and an empty value never
+ overwrites a non-empty one. Concurrent writes are therefore commutative --
+ last-writer-wins is harmless because no writer can lose information.
+ """
+ entries = _enrich_cache_read() # FRESH, not the start-of-pass snapshot
+ now = time.time()
+ for key, value in learned.items():
+ entry = dict(entries.get(key, {}))
+ for field in ("a", "s"):
+ if field not in value:
+ continue
+ if value[field] == "" and entry.get(field):
+ continue # never downgrade a peer's real answer
+ entry[field] = value[field]
+ entry["t"] = now
+ entries[key] = entry
if len(entries) > _PS_CACHE_MAX: # newest-first; a long-lived box can't grow forever
- entries = dict(sorted(entries.items(), key=lambda kv: -float(kv[1].get("t", 0)))[:_PS_CACHE_MAX])
+ entries = dict(sorted(entries.items(),
+ key=lambda kv: -float(kv[1].get("t", 0)))[:_PS_CACHE_MAX])
tmp = "%s.%d.tmp" % (_PS_CACHE_PATH, os.getpid())
try:
- with open(tmp, "w") as handle:
+ fd = os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
+ with os.fdopen(fd, "w") as handle: # 0600: argv snippets + TERM_SESSION_ID
json.dump({"entries": entries}, handle)
os.replace(tmp, _PS_CACHE_PATH)
except (OSError, ValueError, TypeError):
@@ -219,8 +272,15 @@ def discover(root, rows, live, chain, argv=None, env=None):
parts = line.strip().split(None, 1)
if len(parts) == 2:
argv[int(parts[0])] = parts[1]
+ # Cache only a CONCLUSIVE read. A pid absent from the output (it
+ # exited mid-call, or ps was truncated) is "not yet known", NOT
+ # "known to have no argv" -- storing "" for it would make one
+ # transient miss STICKY for the whole TTL, so an unlucky session
+ # would sit unbound for an hour. The uncached path retried on the
+ # very next paint; the cache must not be worse than what it replaces.
for pid, owner in missing.items():
- _learned.setdefault(_enrich_key(owner), {})["a"] = argv.get(pid, "")
+ if argv.get(pid):
+ _learned.setdefault(_enrich_key(owner), {})["a"] = argv[pid]
except (OSError, subprocess.TimeoutExpired) as exc:
# Label enrichment unavailable; the dot itself still paints.
try:
@@ -255,38 +315,41 @@ def discover(root, rows, live, chain, argv=None, env=None):
output = subprocess.run(["ps", "eww", "-o", "pid=,command=",
"-p", ",".join(str(pid) for pid in missing)],
capture_output=True, text=True, timeout=_PS_TIMEOUT)
+ # `settled` = pids whose env we could actually READ: the pid came
+ # back AND its args prefix matched, so the remainder really is the
+ # environment. A pid that is absent, or whose prefix did not match
+ # (a stale argv), is INCONCLUSIVE -- we learn nothing about it and
+ # must not cache a negative. A settled pid with no TERM_SESSION_ID
+ # is a genuine answer and IS cached, so those pids stop re-ps'ing.
+ settled = set()
for line in output.stdout.splitlines():
parts = line.strip().split(None, 1)
if len(parts) != 2:
continue
pid, full = int(parts[0]), parts[1]
args = (argv or {}).get(pid, "")
- remainder = full[len(args):] if args and full.startswith(args) else ""
+ if not (args and full.startswith(args)):
+ continue
+ settled.add(pid)
+ remainder = full[len(args):]
m = re.search(r"TERM_SESSION_ID=(\S+)", remainder)
if m:
env[pid] = m.group(1)
for pid, owner in missing.items():
- _learned.setdefault(_enrich_key(owner), {})["s"] = env.get(pid, "")
+ if pid in settled:
+ _learned.setdefault(_enrich_key(owner), {})["s"] = env.get(pid, "")
except (OSError, subprocess.TimeoutExpired) as exc:
try:
from terminal_status import _record_enum_blind
_record_enum_blind("ticket_binding ps eww: " + type(exc).__name__)
except Exception:
pass
- # TK-11831: persist whatever this pass had to learn, MERGED over the entry
- # already on disk -- a pass that only ran the argv block must not drop a
- # sibling's cached TERM_SESSION_ID (and vice versa). Refreshing "t" keeps a
- # still-live session from ageing out mid-run. Write-back is best-effort: if
- # it fails, the next pass simply re-reads ps, which is today's behaviour.
+ # TK-11831: publish whatever this pass had to learn. _enrich_cache_merge
+ # re-reads the file and refuses to downgrade, so a concurrent peer can
+ # neither be dropped nor blanked. Best-effort: if the write fails, the next
+ # pass simply re-reads ps, which is exactly today's behaviour.
if _learned:
- now = time.time()
- merged = dict(_cache)
- for key, value in _learned.items():
- entry = dict(merged.get(key, {}))
- entry.update(value)
- entry["t"] = now
- merged[key] = entry
- _enrich_cache_write(merged)
+ _enrich_cache_merge(_learned)
for tty, owner in live.items():
if result[tty]:
continue
← 28cf4f6 TK-11831: cache discover()'s two ps enrichment calls (the pe
·
back to Terminal Status
·
backfill: always-on dot self-heal (repaint dropped dots + gr 3790923 →