fix(secrets-broker): bw is not concurrency-safe — serialise, and never return an empty secret with exit 0

Reported by svos-dev after parallelising four vault reads in SVOS's systemd
wrapper. Reproduced here and it is worse than reported: four concurrent secret get
calls for distinct items returned empty strings with exit code 0, zero of four
succeeding against their one of four. No error, no timeout, no diagnostic.

The shape is the problem, not the race. A caller treating an empty optional secret
as 'not configured' degrades silently and never learns otherwise - it cost SVOS the
ability to page the operator while the process logged a clean startup line.

Root cause is session establishment, not item reads. Every invocation runs bw
unlock, and concurrent unlocks against the shared appdata dir invalidate each
other. The damage then surfaces downstream as an empty listing or an empty item
body, which is why a per-call lock is useless: by the time the read runs the
session it holds is already dead. So the lock wraps the whole command instead.

Three changes. The command-level lock makes concurrent callers queue. cmd_get now
refuses an empty value rather than printing it, since a stored secret is never
legitimately zero-length. And find() no longer coerces empty stdout to '[]' - that
turned a broken read into a confident 'no such secret', the same silent-wrong-answer
shape one layer up.

Verified: four parallel reads of four real items now return all four correctly,
serialised at the honest ~17s each. A name that genuinely does not exist still
fails loudly, so the guard did not simply mute the negative case.

Also replaces the copy at ~/.local/bin/secret with a symlink to this file. It was a
plain copy in sync by luck, and every edit here silently left the live tool behind.

services/secrets-broker/secret

@@ -21,6 +21,7 @@ Commands:
 """
 import argparse
 import base64
+import fcntl
 import glob
 import hashlib
 import json
@@ -68,7 +69,27 @@ def load_env():
     return env
 
 
+BW_LOCK = Path(os.environ.get("SECRET_LOCK", "/tmp/.secrets-broker-bw.lock"))
+
+
 def bw(args, env, session=None, stdin=None, check=True):
+    """Run one `bw` command. Serialisation is at the COMMAND level — see `main`.
+
+    ⚠ `bw` is NOT concurrency-safe against a shared session. Measured 2026-09-15:
+    four parallel `secret get` calls for distinct items returned **empty strings
+    with exit code 0** — 0 of 4 succeeded here, 1 of 4 in svos-dev's run that
+    reported it. No error, no timeout, no diagnostic. Sequentially all four are
+    correct. That is the worst possible failure shape for a credential tool: a
+    caller treating "empty optional secret" as "not configured" degrades silently
+    and never knows (it cost SVOS its ability to page the operator, and nothing
+    said so).
+
+    The lock makes concurrent callers queue instead of race, so callers no longer
+    have to know. It is held for the whole `bw` call — vault reads are ~17s, so
+    parallelism was never buying much anyway. Paired with the empty-value guard in
+    `cmd_get`: the lock prevents the race, the guard makes any future silent-empty
+    loud rather than invisible.
+    """
     cmd = [BW] + args + (["--session", session] if session else [])
     r = subprocess.run(cmd, env=env, input=stdin, capture_output=True)
     if check and r.returncode != 0:
@@ -97,7 +118,18 @@ def encode(obj, env):
 
 
 def find(env, s, name):
-    for it in json.loads(bw(["list", "items", "--search", name], env, s).stdout.decode() or "[]"):
+    raw = bw(["list", "items", "--search", name], env, s).stdout.decode().strip()
+    # ⚠ Empty stdout is a FAILED listing, not an empty result set. The old code said
+    # `or "[]"`, which turned a broken read into a confident "no such secret" — the
+    # same silent-wrong-answer shape as the empty-value bug below. bw returns nothing
+    # here when its session has been invalidated underneath it.
+    if not raw:
+        raise BwError(
+            "`bw list items` returned NOTHING (not an empty list). The vault session "
+            "was probably invalidated mid-call — most often by a concurrent `secret` "
+            "invocation. Retry sequentially."
+        )
+    for it in json.loads(raw):
         if it.get("name") == name:
             return it
     return None
@@ -212,6 +244,15 @@ def cmd_get(a):
                 return print(f["value"])
         die(f"no field '{a.field}' on {a.name}")
     data = read_secret(env, s, a.name)
+    # ⚠ An EMPTY value with exit 0 is the silent-failure surface — see bw() above.
+    # A stored secret is never legitimately zero-length, so refuse rather than hand
+    # a caller "" that it will read as "not configured".
+    if not data:
+        die(f"{a.name} resolved to an EMPTY value. A stored secret is never empty, so "
+            "this is a failed read, not an unset secret. Most likely cause: a "
+            "concurrent `secret` call — bw is not safe under parallel invocation and "
+            "returns empty with success. Retry sequentially; if it persists the item "
+            "has no note/field/attachment body.")
     if a.file:
         Path(a.file).write_bytes(data)
         os.chmod(a.file, 0o600)
@@ -320,10 +361,32 @@ def main():
     pb = sub.add_parser("backfill", help="scan THIS box's local secret files and upsert each")
     pb.add_argument("--dry-run", action="store_true"); pb.set_defaults(fn=cmd_backfill)
     a = p.parse_args()
+    # ⚠ SERIALISE THE WHOLE COMMAND, not individual `bw` calls.
+    #
+    # Every invocation runs `bw unlock` to mint a session (see `session`), and
+    # concurrent unlocks against the shared BITWARDENCLI_APPDATA_DIR invalidate each
+    # other. The damage then surfaces DOWNSTREAM — `bw list items` returns empty
+    # stdout, or an item read returns an empty body — so a per-call lock cannot help:
+    # by the time the read runs, the session it holds is already dead.
+    #
+    # Measured 2026-09-15: four parallel `secret get` calls for distinct items
+    # returned EMPTY STRINGS WITH EXIT 0. Zero of four succeeded here; one of four in
+    # svos-dev's run that reported it. No error, no timeout, no diagnostic — and a
+    # caller that treats an empty optional secret as "not configured" then degrades
+    # silently and never knows. It cost SVOS its ability to page the operator.
+    #
+    # Vault reads are ~17s each, so parallelism was never buying much. Correctness
+    # first: callers queue, and no caller has to know any of this.
+    BW_LOCK.touch(exist_ok=True)
+    lk = open(BW_LOCK, "a")
+    fcntl.flock(lk, fcntl.LOCK_EX)
     try:
         a.fn(a)
     except BwError as e:
         die(str(e))
+    finally:
+        fcntl.flock(lk, fcntl.LOCK_UN)
+        lk.close()
 
 
 if __name__ == "__main__":