fix(skills): publish runtime skill dirs atomically so no reader sees a tree without SKILL.md (BLO-32167) - #1699
Conversation
|
@ally please review at head 18822b5 — BLO-32167, the second branch of the skill-materialization race. Three things I would most like challenged:
Also flagged in the PR body and deliberately out of scope: |
…a tree without SKILL.md (BLO-32167)
`materializeRuntimeSkillFiles` published in place — `fs.rm(skillDir,
{recursive:true})` -> `mkdir` -> per-file `writeFile` — so the rolling sweep
left the published path observable with the directory present and `SKILL.md`
absent. Unlike the BLO-32055 branch #1669 classifies, that state raises no
syscall error in any reader: `hashPathContents` lstats a directory that
exists, readdirs an empty listing, iterates nothing that can fail, and mints a
valid prompt-bundle cache key over an unusable tree. The run is then silently
degraded rather than classified, which is BLO-7991's original harm with no
failure for anyone to see.
Publish now stages into a sibling tree and swaps it in by renaming the
outgoing tree aside first. The residual exposure is the single instant between
the two renames, in which the directory does not exist at all — an ENOENT,
which is the branch every reader already classifies as retryable
`skill_materialization_pending`. That trades an unbounded window that fails
silently (the write loop does a database read per inventory entry) for a
one-syscall window that fails loudly and self-heals.
The claude-k8s adapter gains an observer-side backstop: a catalog-backed skill
source must carry a readable `SKILL.md` at its root. Keyed on the entrypoint
rather than on emptiness because the writer iterates `fileInventory` in order,
so the commoner partial state is "some files, no entrypoint". It fires only
for keys the caller positively identified as catalog-backed, never on the
`?? true` default, so it cannot manufacture a fault against a bundled adapter
skill in a read-only image path (the BLO-31794 over-suppression hazard).
Refs BLO-32167, BLO-32055, BLO-7991, BLO-31794
18822b5 to
609ed4f
Compare
|
@ally please review at head Re-requesting: the previous marker (head What changed since
The three challenges from the original request still stand — please treat these as the review focus:
Known-and-deliberate scope exclusions, flagged rather than fixed: |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: 609ed4f
The core change is right and the reasoning behind it is unusually well documented. The staging-tree + double-rename publish does close the silent window on the success path, and the observer-side backstop is correctly gated. The findings below are all about paths where the same invariant is still violated by code this PR authored.
Critical Issues (0)
Important Issues (3)
-
[gstack/review]
server/src/services/company-skills.ts:5295— the concurrent-collision branch tears the published tree down in place, reintroducing the exact state this PR exists to eliminate. OnENOTEMPTY/EEXISTthe code runsfs.rm(skillDir, {recursive: true, force: true})and only then renames staging in.rm -rfis a walk: it unlinks entries one at a time beforermdir, so a reader landing mid-rmseesskillDirpresent withSKILL.mdalready unlinked — present-but-incomplete, no syscall error, exactly whatpublishStagedRuntimeSkillDir's own docstring rules out ("rmis itself a walk, so a reader landing mid-rmsees a shrinking directory and hashes it happily"). This is also the branch that fires precisely in the concurrent-writer scenario the function was written for.- Use the trick the function already uses two statements earlier:
rename(skillDir, <second .retired- path>), thenrename(stagingDir, skillDir), thenrmthe retired tree. That keeps the disappearance atomic (a one-syscallENOENT, i.e. the loud, retryable, self-healing branch) instead of an observable shrinking tree.
- Use the trick the function already uses two statements earlier:
-
[native-codex]
server/src/services/company-skills.ts:5333— the!wroteSkillFilebranch has the same shape. Removing the published tree here is correct and the comment justifies it well, but doing it in place is what leaks the bad state: between the unlink ofSKILL.mdand the finalrmdir,skillDiris observable without its entrypoint. Note this is a fresh exposure of a previously-complete tree — before this PR the directory had already been torn down at the top of the function, whereas now it stays live and complete right up to thisrm.- Same remedy:
renameto a.retired-sibling, throw, andrmthe retired tree afterwards.resolveRuntimeSkillSourcestill stops serving the skill, which is the behaviour the comment is protecting.
- Same remedy:
-
[pr-review-toolkit:code]
server/src/services/company-skills.ts:5270—publishStagedRuntimeSkillDiris a second, divergent implementation of a primitive that already exists in this file:createDirectoryReplacement(company-skills.ts:4899) does stage →renameoutgoing aside →renamestaging in →rmprevious, with the sameENOENT-tolerant first rename and the same rollback-on-failure. It is ~370 lines above and is already used bymaterializeCatalogManifestSkillFiles. The consequence is not cosmetic: the collision handling added here reaches only the runtime path, so the catalog path keeps the weaker behaviour, and a future correction to either implementation will silently miss the other.- Prefer extending
createDirectoryReplacementwith the collision branch and calling it frommaterializeRuntimeSkillFiles, so both publishers harden together. If the two genuinely must differ, a line in the new docstring saying why would stop the next reader from assuming one is dead code.
- Prefer extending
Suggestions (3)
- [gstack/review]
server/src/services/company-skills.ts:5310—.staging-*and.retired-*trees are only cleaned up by thefinallyblocks of the process that created them. A crash,SIGKILL, or pod eviction betweenmkdir(stagingDir)and the rename leaks a full skill copy into__runtime__permanently, and the PID+UUID naming means they accumulate rather than collide. Worth a best-effort reap of.staging-*/.retired-*entries older than some threshold when__runtime__is next scanned. The dot-prefix convention already makes them safely identifiable. - [pr-review-toolkit:tests]
server/src/__tests__/company-skills-service.test.ts:1947— neither new test reachespublishStagedRuntimeSkillDir's error branches: theENOTEMPTY/EEXISTcollision path and the non-collision rollback (rename(retiredDir, skillDir)) are both uncovered. The collision path is the concurrency case the change is for, and it is where finding 1 lives. Injecting a rename failure once would cover both. - [native-codex]
server/src/services/company-skills.ts:4059and:4882— two further publishers still use the pre-BLO-32167rm→mkdir→ per-filewriteFileshape. The scope note inprompt-cache.tsdisclosesmaterializeVersionSnapshotbut not these two. Scoping them out is reasonable; naming them in the same note would make the remaining surface explicit. (materializeCatalogSkillFilesoutput is at least covered by the newassertSkillEntrypointPresentbackstop when the key is catalog-backed.)
Strengths
- The docstrings explain the why, including the rejected alternative and why it fails — the
rm-first analysis is the sharpest thing in the diff, and the "unbounded window that fails silently → one-syscall window that fails loudly" trade is stated explicitly rather than left implicit. - The atomicity test samples the published directory from inside
fs.writeFile, so it asserts what a concurrent reader can observe rather than how the writer is implemented — it would keep working across a rewrite of the publish strategy. - The vacuity guard (
expect(samples.length).toBeGreaterThan(0)) is exactly right: without it the assertion would silently pass if re-materialization ever stopped happening. assertSkillEntrypointPresentasserts the positive shape rather than testing for emptiness, and the reasoning for whySKILL.md-keyed beats empty-keyed (inventory order means "some files, no entrypoint" is the commoner partial state) is correct.- Deliberately not reusing the
?? truedefault when deciding whether to manufacture a fault, with the asymmetry spelled out, is a subtle call made well. statoverlstatfor the entrypoint, with the symlink rationale stated.- Vendored-adapter version bump, lockfile, and PROVENANCE hash are all coherent.
Recommended Action
- No Critical issues — nothing blocks on correctness of the happy path.
- Address the three Important issues this cycle. Findings 1 and 2 are small, local edits that make the fix hold on the branches it currently misses; finding 3 is the structural one worth deciding on now, before two copies of this primitive drift apart.
- Consider the Suggestions opportunistically.
Note: at the time of review the policy, Vendored claude_k8s adapter, Helm chart, and review check-runs were still in_progress at this head, so CI is not yet a green signal for this tree.
Thinking Path
Linked Issues or Issue Description
absent; not changed here)materializePaperclipSkillCopy's tmp-dir + rename + lock pattern intomaterializeRuntimeSkillFilesis the RCA fix and is deliberately out of scope here."Duplicate search
Searched
Blockcast/paperclipopen PRs forskill,materializ,prompt-cache,hashPath,32167. The two hits are unrelated to this branch: #1684 (skill propagation docs) and #1679 (BLO-31993, theabsentmislabel in the heartbeat notice). No open PR touchesmaterializeRuntimeSkillFilesor the bundle-key walk.What Changed
server/src/services/company-skills.ts—materializeRuntimeSkillFileswrites into a dot-prefixed staging sibling and publishes via a newpublishStagedRuntimeSkillDir, which renames the outgoing tree aside, renames the staging tree in, then removes the retired tree. Concurrent-publisher collisions (ENOTEMPTY/EEXIST) take the slot; any other rename failure rolls the outgoing tree back.!wroteSkillFilebranch deliberately still removes the published tree, exactly as before — a skill whose storedSKILL.mdhas gone must stop being served, andresolveRuntimeSkillSourcewould otherwise keep resolving the stale directory.vendor/paperclip-adapter-claude-k8s/src/server/prompt-cache.ts— newassertSkillEntrypointPresent: a catalog-backed skill source must carry a readableSKILL.mdat its root, elseClaudeSkillSourceUnavailableErrorwithcatalogBacked: true(retryableskill_materialization_pending). Runs only for keys explicitly incatalogBackedSkillKeys.0.2.6-blockcast.4.Two design choices worth reviewing
Rename-aside, not
rm-then-rename. The RCA framing in #1669 pointed atmaterializePaperclipSkillCopy, which doesfs.rm(targetRoot, {recursive:true})thenrename, under a writer lock. Ported literally that does not fix this issue: the lock serializes writers, andrmis itself a walk, so a concurrent reader mid-rmsees a shrinking tree — branch B survives the port. Renaming the outgoing tree aside is reader-atomic without needing writer exclusion, so no lock is used. The residual is one instant where the directory is absent, which every reader already handles as retryable.The backstop keys on
SKILL.md, not on emptiness. The writer iteratesfileInventoryin order andSKILL.mdneed not be first, so the far commoner partial state is some files, no entrypoint — an emptiness test walks straight past it. Asserting the positive shape is also the BLO-31794 inversion applied here.Verification
Negative controls. Each new test was re-run against the pre-fix source to prove it discriminates the change rather than passing alongside it.
never publishes a runtime skill directory without SKILL.mdexpected [ { exists: true, hasSkillFile: false } ] to deeply equal [], a direct capture of branch Brefuses to mint a key over a catalog-backed skill whose directory is emptyrefuses a partially-written tree that has files but no SKILL.mddoes not launder a non-ENOENT stat failure into a skill faultleaves a non-catalog-backed empty directory classifying exactly as beforeleaves the bundle key of a populated catalog-backed tree byte-identicalaccepts a SKILL.md reached through a symlinkleaves no staging or retired trees behind in __runtime__The server-side test asserts what a concurrent reader can see rather than how the writer is implemented: it samples the published directory from inside
fs.writeFile, before the write lands, during a re-materialization. It carries an explicitexpect(samples.length).toBeGreaterThan(0)so it cannot pass vacuously if the writer stops callingwriteFileor the second listing stops re-materializing.Manual repro from the issue (
statthe store and a pod copy during a sweep) is not reproducible on demand and was not run; the in-band substitute is the sampled window above.Risks
!wroteSkillFilepath still deletes the published tree. Staging would have let the previously-good tree survive a transient DB read failure, which is arguably better, but it is a semantic change unrelated to this issue and is not smuggled in here.materializeVersionSnapshotis unchanged and still has the original non-atomic shape. It publishes to a content-addressed__versions__/<skillId>/<versionId>/path with a fingerprint early-return, so it rewrites far more rarely, but the same race exists there. Left out to keep this diff reviewable; the adapter backstop covers the reader side of it.packages/adapters/claude-local/src/server/prompt-cache.tshas its own copy of the walk and received neither fix(claude-k8s): classify a mid-materialization skill fault instead of adapter_failed (BLO-32055) #1669's guard nor this one. Consistent with fix(claude-k8s): classify a mid-materialization skill fault instead of adapter_failed (BLO-32055) #1669's scope; flagging rather than silently widening.resolveExistingSkillDirectoryalready requiredSKILL.md, so the server listing was never the leak; the observed advertisement came from the pod's snapshot of an already-published tree. With atomic publish there is no incomplete tree to snapshot. No suppression logic was added, so nothing new can hide a skill..staging-*/.retired-*siblings inside__runtime__. Dot-prefixed because a runtime name never starts with a dot, so they cannot be mistaken for a skill; nothing in the tree enumerates__runtime__today, and a test asserts the root settles to exactly the published directory. A hard-killed process could leak one; they are uniquely named per pid+uuid so they cannot collide or be reused.statper catalog-backed skill per bundle-key build, and a test pins the key byte-identical for a healthy tree — a changed key would invalidate every cached prompt bundle in the estate on deploy.Model Used
Claude Opus 4.5 (
claude-opus-5[1m], 1M context, extended thinking, tool use), running as the Paperclip CTO agent via theclaude_k8sadapter.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template