Conversation
…ed CephFS (BLO-33641)
Every agent pod start ran:
GUARD_DIR="${CLAUDE_CONFIG_DIR:-$HOME/.claude}"
... | base64 -d > "$GUARD_DIR/paperclip-env-guard.mjs"
On the k8s adapter $HOME is /paperclip, the shared ReadWriteMany CephFS
volume every agent pod mounts, so that redirect is an O_TRUNC against a
single inode contended by ~92 CephFS clients.
Measured in production: 71 of 72 blocked MDS ops were 'setattr size=0'
against this one inode, with the oldest aging past 1800s. Pods scheduled
fine, then hung in Init:0/1 behind ceph-csi's per-volume NodeStageVolume
lock until the 600s startup budget expired, which Paperclip records as
k8s_pod_schedule_failed - 253 in 12h.
The two .mjs files are byte-identical for every pod and are only ever
read locally, so nothing is lost by keeping them per-pod. They now go to
/runtime-cache/paperclip-guard (the emptyDir job-manifest.ts already
mounts unconditionally), with a fallback to $GUARD_DIR when that path is
unavailable, and PAPERCLIP_GUARD_SCRIPT_DIR to override.
settings.json must stay in CLAUDE_CONFIG_DIR for Claude Code to read it,
so the merge script now writes only when the resulting PreToolUse list
actually differs - steady state is a pure read instead of a third
unconditional truncate on the shared volume. Guard entries are matched by
filename rather than full path, so a settings.json still pointing at the
old in-$HOME location is migrated rather than accumulating a second entry.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Omar Ramadan <omar@blockcast.net>
1 similar comment
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8732a84
Critical Issues (0)
Important Issues (2)
- [native-codex]
vendor/paperclip-adapter-claude-k8s/src/server/env-guard.ts:872— The sharedsettings.jsonnow records a literal hook command pointing into/runtime-cache/paperclip-guard, but/runtime-cacheis anemptyDirunique to each pod. The file is still shared across pods, so each pod startup races to rewrite the same shared settings file with its own pod-local path; after another pod wins, this pod invokes a nonexistent script and the env-dump guard is silently bypassed (the hook fails open).- Keep the shared hook target stable, or make the configuration itself pod-local. If the hook must remain in shared
settings.json, use a stable shared entrypoint that dispatches to the current pod-local script rather than embedding a per-pod absolute path, and add a multi-pod/shared-settings regression test.
- Keep the shared hook target stable, or make the configuration itself pod-local. If the hook must remain in shared
- [gstack/review]
vendor/paperclip-adapter-claude-k8s/src/server/env-guard.ts:837— The guard’s remediation text still tells blocked users to runnode $HOME/.claude/safe-env-inspect.mjs, but this change writessafe-env-inspect.mjsto$GUARD_SCRIPT_DIR(normally/runtime-cache/paperclip-guard). The documented escape hatch therefore points at a nonexistent or stale shared-volume file after deployment.- Construct the remediation command from the same runtime guard-script directory, or print the configured path; add an assertion that the message matches the installed helper location.
Suggestions (1)
- [pr-review-toolkit]
vendor/paperclip-adapter-claude-k8s/src/server/env-guard.test.ts:349— Add a test that executes the generated setup shell with a separate guard directory and verifies the installed hook points there and remains usable after a second pod uses the same settings directory.
Strengths
- The diff is narrowly scoped and adds regression coverage for both per-pod script placement and steady-state settings idempotency.
- Filename-based migration avoids accumulating duplicate guard hooks when upgrading existing shared settings.
- The runtime-cache volume is mounted in both init and main containers, and the existing hook semantics remain otherwise unchanged.
Recommended Action
- Fix Important issues before merge.
- Address the shared-settings/path consistency issue this cycle.
- Consider the additional cross-pod regression coverage.
…ed helper Ally review on #1814 raised two Important findings against the per-pod guard-script move (BLO-33641): 1. settings.json is shared across pods but the hook command embedded this pod's script dir. A pod that fell back to CLAUDE_CONFIG_DIR would rewrite the shared file and point every other pod at a path it does not have, where the hook died as a bare module-resolution error (silent fail-open). The recorded command is now ENV_GUARD_HOOK_COMMAND, identical for every pod: it resolves /runtime-cache/paperclip-guard first, then the CLAUDE_CONFIG_DIR fallback, and reports a guard missing from both on stderr (still exit 0 per the recorded BLO-22514 fail-open decision). 2. The block message still told users to run $HOME/.claude/safe-env-inspect.mjs, which no longer receives the file. It now names the helper beside the guard that actually ran. Tests first (3 red -> green, 283/283): command is byte-identical across differing PAPERCLIP_GUARD_SCRIPT_DIR values and embeds neither; the command dispatches to the config-dir copy through `sh -c`; a missing guard is reported rather than silent; the remediation names the sibling helper. Also updates PROVENANCE.md (row + integrity hash) and bumps the vendored package to 0.2.6-blockcast.6 -- the `Vendored claude_k8s adapter` job was red on the stale hash. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Omar Ramadan <omar@blockcast.net>
|
Addressed both Important findings in c229325 (tests written first, 3 red → 283/283 green):
Also: PROVENANCE.md row + integrity hash updated and the vendored package bumped to 🤖 Generated with Claude Code |
|
Closing as superseded by #1810 (merged 2026-09-13T09:58Z), which fixes the same BLO-33641 root cause — the fleet-wide 🤖 Generated with Claude Code |
Thinking Path
Linked Issues or Issue Description
Refs BLO-33641 (Paperclip issue tracker).
Bug. Agent runs fail with
k8s_pod_schedule_failed:Despite the error name this is not a scheduling failure — there were zero
FailedSchedulingevents and no node pressure (no Memory/Disk/PID pressure on any of the 17 nodes). Pods schedule immediately, then hang.Production measurements
k8s_pod_schedule_failed, 6 hk8s_pod_schedule_failed, 12 hFailedMounteventssetattr size=0on one inodeEvery stuck pod reported the same ceph-csi error — the per-volume in-flight lock, not Multi-Attach:
pvc-729b17f4-…ispaperclip/paperclip-data(cephfs,ReadWriteMany). Dumping the MDS blocked ops showed 71 of 72 weresetattr size=0against inode0x100070a0300, all parked atflag_point: acquired locks. Resolving that inode:One shared file, eight clients, seven of them blocked waiting for the file caps the truncate needs.
Where it comes from
The agent pod spec confirms the overlap —
HOME=/paperclip, and/paperclipis the shared PVC:So all three writes land on the shared volume on every single pod start.
What Changed
env-guard.ts— the two.mjsfiles now go to$GUARD_SCRIPT_DIR, defaulting to/runtime-cache/paperclip-guard. That emptyDir is mounted unconditionally byjob-manifest.ts(RUNTIME_CACHE_MOUNT_PATH), so it is always available;PAPERCLIP_GUARD_SCRIPT_DIRoverrides it, and a failedmkdir -pfalls back to$GUARD_DIR(previous behavior).env-guard.ts—settings.jsonstays inCLAUDE_CONFIG_DIR(Claude Code must find it there), but the merge script now computes the desiredPreToolUselist and writes only when it differs from what is already on disk. Steady state is a pure read rather than a third unconditional truncate.env-guard.ts— guard entries are matched by filename (endsWith("paperclip-env-guard.mjs")) rather than exact path, so an existingsettings.jsonstill pointing at the old in-$HOMElocation is migrated to the new path instead of accumulating a second hook entry. Without this, the path change alone would have appended a duplicate on every existing volume.env-guard.test.ts— two regression tests (below).The scripts are byte-identical for every pod and are only ever read locally by the PreToolUse hook, so nothing is lost by keeping them per-pod. This does not change what the guard does, only where it lives.
Verification
Both tests were written first and observed failing against unmodified
env-guard.ts:After the change:
Re-verified after rebasing onto current master: 279/279.
The existing idempotency test (
merges the PreToolUse hook idempotently, preserving existing hooks) is unchanged and still passes, so the pre-existing contract — run twice, exactly one Bash guard entry, unrelatedStophook preserved — still holds.Pre-existing failures in this package, not caused by this PR
job-manifest.test.tsandtsc --noEmitboth fail in a standalone checkout ofvendor/paperclip-adapter-claude-k8s, becausevendor/is not a member of the rootpnpm-workspace.yaml, so@paperclipai/adapter-utilsand@types/nodedo not resolve there. I confirmed this is independent of the change rather than assuming it:Identical counts. The 3 in
env-guard.tsareCannot find name 'Buffer'on the three pre-existingBuffer.from(...)lines, which this PR does not touch. CI resolves these normally.Risks
Low.
/runtime-cacheis unavailable,mkdir -p ... || GUARD_SCRIPT_DIR="$GUARD_DIR"restores exactly the old locations, so the worst case is today's behavior.settings.jsonwritten by an older image gets one entry pointing at the new path, not two.settings.jsonis still written when it genuinely needs to change (first run, corrupt/missing file, stale path), so a pod can never start without the guard installed.|| echo "... settings merge skipped" >&2), so a merge error cannot block a run from starting — same as before.job-manifest.test.tsasserts the guard install precedesccrotate next, and the guard writes still come first in the joined fragment.This does not fix the CephFS condition on its own. BLO-33641 also found the MDS has
mds_max_caps_per_client = 1048576with clients sitting just under it andrecall_caps: 0, and 96 % of the MDS cache pinned — so cap pressure needs a separate operational fix. What this PR removes is the single-inode contention point that converted that pressure into failed pod starts.Model Used
Claude Opus 5 (1M context) — model ID
claude-opus-5[1m], via Claude Code. Extended thinking enabled; tool use (Bash,kubectl/psql/cephagainst the live sfo12 cluster for every measurement above, vitest, tsc). Written test-first.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code