sops-checkout-freshness

Playbook — stale /opt/p24-infra checkout corrupts SOPS reads and writes

Issue: #4453 · Found during: #4444 Wasabi IAM rotation (queue row 3318) · Related: #2816 (bms-4 checkout ~750 commits behind), #3329 (vps-i1 divergence self-heal), #5585 (uncommitted n8n workflow export dirtied bms-4 tree, rc=4 aborted the #5577 rotation)

---

Symptom

Any of these, on a credential job:

• A key you just rotated fails to authenticate — InvalidAccessKeyId, 401, invalid_grant — and it looks like a live outage.
• A verify-first / digest-compare job reports drift against a key you know is in sync.
• A drift-sync job reports a healthy no-op on a file you know has changed.
• A rotation “succeeds” but the key reverts on the next secrets-sync run.

The last one is the dangerous one: it is silent.

Root cause

/opt/p24-infra is a shared checkout. Nothing in the worker spawn path used to fetch it, so it only advanced when an unrelated GitHub Actions workflow happened to run (deploy-vps-scripts.yml, plus the #3329 self-heal in three alert workflows). Between those runs it can sit arbitrarily far behind origin/main.

Every credential consumer reads from that path:

Consumer	Path
infra/agent-prompts/worker-secret-manager.md	/opt/p24-infra/secrets/… — reads and commits
infra/agent-prompts/infra-task-request-worker.md	declared cwd /opt/p24-infra
scripts/spawn-infra-task-worker.sh	REPO_DIR=/opt/p24-infra, never clones
scripts/spawn-worker.sh	reads the agent prompt itself from there

Two distinct failure modes, and the quiet one is worse:

1. Read side. You decrypt an already-rotated key and get the OLD value. In #4444 the bms-4 checkout was pinned at 693f48f2; SOPS returned the already-deactivated PINBOX24_W4_WASABI_* value, producing a false InvalidAccessKeyId. The rotation itself had fully succeeded — new key authenticating 200, old key deactivated, verified-rejected and deleted, secrets-sync 12/12 green. The “outage” was an artifact of the reader.
2. Write side. worker-secret-manager.md decrypts, edits, re-encrypts and commits secrets/<file>.env.sops in that same stale checkout — so the commit is built on a stale base and silently reverts every key committed to that file since the checkout last advanced. Nothing fails, nothing logs, and the loss only surfaces when some other key stops working.

The guard (as of #4453)

scripts/lib/ensure-fresh-checkout.sh — fetch, compare HEAD against origin/<ref>, and reset --hard only when the working tree is clean.

bash /opt/p24-infra/scripts/lib/ensure-fresh-checkout.sh /opt/p24-infra main

Exit	Meaning	What to do
0	at origin/main (already current, or cleanly advanced)	proceed
1	bad usage — blank directory argument	fix the caller
2	not a git working tree	wrong path, or the checkout is gone — rebuild it
3	git fetch failed (network / auth)	freshness unknown — stop, do not read
4	behind and dirty — reset refused	see Dirty tree below — stop, do not read
5	git reset --hard failed	disk / permissions — stop, do not read

Wired in at four points:

• scripts/spawn-worker.sh — refreshes before resolving AGENT_PROMPT_PATH, exports P24_SHARED_CHECKOUT_STALE=0/1. Non-fatal: it must not abort the spawn.
• scripts/spawn-infra-task-worker.sh — same, for the legacy path that never clones.
• infra/agent-prompts/worker-secret-manager.md Step 0a — fail-closed, aborts before the SOPS canary.
• infra/agent-prompts/infra-task-request-worker.md Step 0 — fail-closed, aborts before Step 1.

The prompt-level gates are the ones that actually protect credentials; the spawn-script calls are belt-and-braces so the instructions themselves are not read from a stale tree.

Why the canary does not catch this

worker-secret-manager.md Step 1 already canary-decrypts monitoring.env.sops. That proves the age key works — it decrypts a stale file just as happily as a current one. It is not a freshness check, which is why the gate runs before it.

Why the reset is guarded

/opt/p24-infra is a live deployment checkout. The CI-deployed plaintext .env files under it are gitignored, so a clean-tree reset does not touch them — but an unconditional reset would discard any hand-applied hotfix on the box. So a dirty tree returns 4 and the caller decides. This is deliberately unlike deploy-vps-scripts.yml, which resets unconditionally because it owns the deploy.

Why it is safe for a script to reset the checkout it lives in

Both spawn scripts live under /opt/p24-infra/scripts/ and reset /opt/p24-infra while running. git reset --hard replaces a modified file by writing a new one and renaming over the path, which allocates a new inode; the running bash keeps its open fd on the old inode and finishes with consistent semantics. Verified for #4453 against a 200-line-longer replacement. This does not hold for in-place truncate+write (cp, >, tee) — do not “simplify” the reset into one.

Dirty tree (rc=4) — resolution

Do not force it green. rc=4 means someone’s uncommitted change is about to be destroyed.

git -C /opt/p24-infra status
git -C /opt/p24-infra diff

• Change is a real hotfix → land it as a PR from a worktree (docs/playbooks/shared-checkout-worktree-isolation.md), then re-run the job. docs/priorities.md already records that direct edits in this checkout vanish on the next reset — the worktree+PR route is the only durable one.

• Change is a stray artifact (temp file, leftover -tmp.env.sops, editor backup) → remove it, then re-run. Never git checkout -- a secrets/*.env.sops you have not inspected.

• Change is a tracked, auto-regenerated export artifact — most commonly an n8n workflow export under infra-src/n8n-workflows/*.json (+ its sibling .md) that an agent or developer re-exported in this checkout and never committed. This is a known recurring trigger (#5585: an uncommitted pinbox-async-export.json/.md gave rc=4 and aborted the #5577 rotation before any SOPS access). These files carry no secrets and are regenerable from n8n, so:

  1. Confirm the entire dirty set is only infra-src/n8n-workflows/ paths — never assume. git -C /opt/p24-infra status --porcelain | grep -qv '^.. infra-src/n8n-workflows/' && echo "OTHER FILES DIRTY — stop, inspect". If anything else (especially a secrets/*.env.sops) is also dirty, treat it per the bullets above, not this one.
  2. If the export is a real change worth keeping → land it as a PR from a worktree (docs/playbooks/shared-checkout-worktree-isolation.md), the same as any hotfix. n8n workflow JSON is the tracked source of truth, so a genuine export belongs in git, not discarded silently.
  3. If it is a throwaway re-export with no intended change → discard it (git -C /opt/p24-infra checkout -- infra-src/n8n-workflows/) and re-run. Safe here only because step 1 proved no secret file is in the dirty set.

  The durable prevention is discipline, not code: never leave an uncommitted n8n export sitting in the shared checkout — export inside a worktree and PR it. A single stray export fails the rc=4 gate for every credential job on that host until cleared, which is why this is a P1 when it hits a rotation.

• Change is an in-flight SOPS edit from a crashed worker → treat as an incident; verify the file decrypts (sops -d … | head -0) before discarding anything.

Permission denied (rc=5) — shared checkout un-advanceable by a second Linux account

Symptom. On bms-4, every claude-runner-2 secret-manager / infra-task job aborts at the fail-closed Step 0 freshness gate with:

error: unable to unlink old 'CHANGELOG.md': Permission denied
… → git reset --hard failed → rc=5

dev-issue/dev-coder workers are not hit — their shared-checkout check in spawn-worker.sh is non-fatal (P24_SHARED_CHECKOUT_STALE=1, warn+continue) and they run in a fresh /tmp/worker-N clone. Only the fail-closed secret-manager (worker-secret-manager.md §“Checkout freshness gate”) and infra-task (infra-task-request-worker.md Step 0) paths — which operate directly inside /opt/p24-infra — abort.

Root cause (#5673). /opt/p24-infra is a single shared checkout. The root-SSH deploy paths git reset --hard it as root, then chown -R claude-runner:claude-runner so the owner account can advance it (#4462) — but they never restored group-write on the working-tree directories. The top-level dir sits drwxr-xr-x (no group-write) and carries an extended ACL (user:gmail-runner:--- deny + mask::r-x). Once the usage-based load balancer began routing real jobs to the second Linux account claude-runner-2 (#5654/#5670) — a member of the claude-runner group, but not the owner — its git reset --hard cannot unlink tracked files in the non-group-writable directories → Permission denied → rc=5. (.git/ itself is already drwxrwsr-x + core.sharedRepository=group, so git fetch succeeds; only the working-tree reset fails.)

Note a plain chmod g+w on the top-level dir does not fix it: with an extended ACL present, chmod moves the ACL mask, leaving group::r-x — no effective group-write. The fix needs a named-group ACL entry.

Durable fix (in code). scripts/lib/normalize-shared-checkout-perms.sh grants the claude-runner group rwx on every directory via a named-group ACL (g:claude-runner:rwx) plus a matching default ACL (so git-created subdirs inherit it) and setgid. It touches directories only — gitignored deployed secret files (bms-4/.env, root-owned 0600/0640) never participate in reset --hard and keep their modes. It is wired into every root-SSH path that advances the bms-4 checkout, right after the existing chown: deploy-vps-scripts.yml (bms-4 step) and sync-worker-roles.yml (bms-4 step). secrets-sync.yml does not advance the bms-4 checkout dir, so it needs no change.

Apply / re-apply immediately (privileged — needs root or the checkout owner). Merging a scripts/** change auto-triggers deploy-vps-scripts.yml, which re-applies it; to force it now:

gh workflow run deploy-vps-scripts.yml --repo radieu/p24-infra
# or directly on the box as root / claude-runner:
bash /opt/p24-infra/scripts/lib/normalize-shared-checkout-perms.sh /opt/p24-infra

Verify. The top-level dir must show an effective group-write for claude-runner, and a claude-runner-2 reset must succeed:

getfacl -p /opt/p24-infra | grep -E 'group:claude-runner|^mask'   # expect group:claude-runner:rwx + mask::rwx
sudo -u claude-runner-2 git -C /opt/p24-infra reset --hard origin/main   # must not rc=5

chown/ACL fix the symptom. The durable question (issue #5673 fix option 2) is whether the shared checkout should be advanced only by a claude-runner-owned process so per-account reset --hard becomes a no-op; the ACL approach here keeps the current self-advance model working for both accounts and is the lower-risk change.

Verification

# Helper contract
bash scripts/lib/ensure-fresh-checkout.sh --self-test
python3 -m pytest scripts/tests/test_ensure_fresh_checkout.py -v
 
# On a server: confirm the checkout tracks origin/main after a worker spawn
git -C /opt/p24-infra fetch origin main -q
[ "$(git -C /opt/p24-infra rev-parse HEAD)" = "$(git -C /opt/p24-infra rev-parse origin/main)" ] \
  && echo current || echo STALE

Note for future credential workers

The per-job clone at /tmp/worker-<issue>-<repo> created by scripts/spawn-worker.sh is always current (it is a fresh git clone of the remote tips), and spawn-worker.sh already exports a relative $SOPS_FILE that resolves inside it. The agent prompts override that with absolute /opt/p24-infra/... paths. Reading SOPS from the per-job clone instead of the shared checkout would sidestep this whole class of bug — but the secret-manager write path also needs to git commit and git push, which is why it lives in the shared checkout today. Consolidating both onto the per-job clone is the cleaner end state and is not yet done.