Skip to content

fix: make schema-sync directory renames EXDEV-safe for docker builds#2357

Open
mikemeier wants to merge 1 commit into
adcontextprotocol:mainfrom
gotom-io:fix/exdev-safe-schema-sync
Open

fix: make schema-sync directory renames EXDEV-safe for docker builds#2357
mikemeier wants to merge 1 commit into
adcontextprotocol:mainfrom
gotom-io:fix/exdev-safe-schema-sync

Conversation

@mikemeier

Copy link
Copy Markdown

Problem

npm run sync-schemas (and therefore schemas:ensure / build:lib) fails inside any docker build where the schema cache is not pre-populated.

During an image build, the working tree is overlayfs. renameSync on a directory that came from a lower image layer (e.g. via COPY . .) fails with EXDEV: cross-device link not permitted even though src and dest are on the same mount — overlayfs cannot rename lower-layer directories. This hits copySkillTree when it snapshots an existing skill dir (skills/adcp-brand -> skills/adcp-brand.previous) and can equally hit replaceTree.

The failure is then misreported: the catch in sync() swallows the error and prints AdCP <version> was not reachable from adcontextprotocol.org, and the GitHub-dist fallback 404s (no dist/ on main), so the build dies with a misleading network error. Bind mounts and normal checkouts are unaffected, which makes this look flaky/environment-specific.

Fix

  • Add moveTreeSync: try renameSync, and on EXDEV fall back to cpSync(recursive) + rmSync — overlayfs handles copy+delete fine. Used at all three directory-rename sites (replaceTree x2, copySkillTree).
  • Include the original error message in the GitHub-dist fallback warning so extraction failures are no longer misreported as network failures.

Notes

  • scripts/-only change (dev tooling, not published in files), so no changeset per CONTRIBUTING policy — happy to add one if you want it anyway.
  • Repro: docker build a context containing the repo, run npm run sync-schemas with an empty schemas/cache/. Verified fixed in node:22-alpine builds.

🤖 Generated with Claude Code

During docker build /app is overlayfs, and renameSync on a directory that
came from a lower image layer (COPY . .) fails with EXDEV even though src
and dest are on the same mount. This broke sync-schemas (and thus
schemas:ensure / build:lib) in any image build without a pre-populated
schema cache, misreported as 'AdCP not reachable' by the fallback path.

Fall back to copy+delete on EXDEV and include the original error in the
GitHub-dist fallback warning so extraction failures are no longer
misreported as network failures.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aao-secretariat

Copy link
Copy Markdown

⚠️ Argus review could not complete (no verdict or review body was produced). A human reviewer should take this PR. View workflow run

@andybevan-scope3

Copy link
Copy Markdown
Collaborator

Reviewed — the fix itself is solid, approving on the code. One thing before merge: please add a patch changeset.

You'll need a changes though - Even though scripts/ isn’t in package.json files, sync-schemas runs inside build:lib and produces published artifacts (skills/, compliance/cache/), and a broken container build is release-relevant — so this needs a changeset (and the Changeset Check job will flag it anyway). npm run changeset, pick patch.

On the code, confirmed the diagnosis holds up:

Existing mitigation at sync-schemas.ts:370 (temp workdir inside the repo) covers the cross-mount EXDEV case but not the overlayfs lower-layer case — so this fix is complementary, not redundant.
All three directory-rename sites converted (replaceTree ×2, copySkillTree); none missed.
cpSync is a safe fallback — copies, not renames, so it can’t itself throw EXDEV. Node 16.7+; repo is node >=20.
Callers preserve the “dest doesn’t exist” invariant, so copy+delete can’t merge into a stale tree.
Symlink safety preserved; typechecks clean.
The improved fallback warning is a good catch too — misreporting EXDEV as a network failure was a real trap.

@bokelley

Copy link
Copy Markdown
Contributor

Thanks @andybevan-scope3 — agreed on the rationale. sync-schemas is part of build:lib and its output (skills/**, compliance/cache/**) is in the published files, so a docker-build breakage here is release-relevant.

I wasn't able to push directly to the fork branch (session scope is limited to the upstream repo), but I've staged the changeset at adcontextprotocol/adcp-client:fix/exdev-safe-schema-sync as commit 4dc10eb. You can cherry-pick it onto this branch:

git cherry-pick 4dc10eb
git push

Or add the file manually:

.changeset/exdev-safe-schema-sync.md

---
"@adcp/sdk": patch
---

Fixed `sync-schemas` crashing with EXDEV inside docker builds. Directory renames now fall back to copy+delete when `renameSync` fails on overlayfs lower-layer directories. Also corrects the misleading "was not reachable" fallback warning to include the original error.

Once that's pushed the Changeset Check should go green and the PR will be ready to merge.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants