Catcrafts/Crafter.Build

Fork 0

Race condition: concurrent builds clobber each other's external dep clones #2

New issue

Closed

opened 2026-05-27 03:44:55 +02:00 by jorijnvdgraaf · 6 comments

jorijnvdgraaf commented

2026-05-27 03:44:55 +02:00

Owner

When two crafter-build invocations run concurrently and both need the same
external dep, they race on the shared .tmp clone directory and one corrupts
the other's in-flight git clone.

Repro

Two projects (A, B) that both declare the same ExternalDependency (directly
or transitively), sharing a common ~/.cache/crafter.build/external/:

cd projectA && crafter-build &
cd projectB && crafter-build &
wait

If the cache miss happens for both at roughly the same moment, one of the
builds fails.

Observed

[N/N] Building external dep <name>
git fetch for '<name>': `git clone --recursive '<url>' '~/.cache/crafter.build/external/<name>-<hash>.tmp'` failed (exit 128): Cloning into '~/.cache/crafter.build/external/<name>-<hash>.tmp'...
fatal: could not open '~/.cache/crafter.build/external/<name>-<hash>.tmp/.git/objects/pack/tmp_pack_XXXXXX' for reading: No such file or directory
fatal: fetch-pack: invalid index-pack output

tmp_pack_* is an in-flight git pack tempfile — it vanishes mid-read because
the other build either rm'd the .tmp directory or finished and renamed it
out from under us.

Likely cause

The cache path is derived from a hash of the dep spec, so concurrent builds
that resolve to the same external compute the same .tmp directory name
(both racing builds picked the identical -<hash>.tmp suffix in the failure
we hit). There's no lock around git clone → rename, so:

build A: git clone … .tmp (in progress)
build B: git clone … .tmp (sees existing dir, or wipes-and-retries)
build A: pack tempfile yanked → invalid index-pack output

Suggested fixes

Per-invocation tmp dir — clone into <dep>.tmp.<pid> (or mktemp -d),
then rename(2) to the canonical cache path on success. rename is atomic
on the same filesystem; the loser of the race just discards its clone.
File lock on the cache dir — flock on
~/.cache/crafter.build/external/<dep>.lock around the clone+rename so
only one builder actually fetches; others wait and reuse the result. Lower
bandwidth usage when many builds start at once.

(1) alone fixes the corruption; (1) + (2) also avoids redundant clones.

Environment

Linux x86_64
Two concurrent crafter-build invocations against sibling projects that
share the user's ~/.cache/crafter.build/external/

When two `crafter-build` invocations run concurrently and both need the same external dep, they race on the shared `.tmp` clone directory and one corrupts the other's in-flight git clone. ## Repro Two projects (A, B) that both declare the same `ExternalDependency` (directly or transitively), sharing a common `~/.cache/crafter.build/external/`: ```sh cd projectA && crafter-build & cd projectB && crafter-build & wait ``` If the cache miss happens for both at roughly the same moment, one of the builds fails. ## Observed ``` [N/N] Building external dep <name> git fetch for '<name>': `git clone --recursive '<url>' '~/.cache/crafter.build/external/<name>-<hash>.tmp'` failed (exit 128): Cloning into '~/.cache/crafter.build/external/<name>-<hash>.tmp'... fatal: could not open '~/.cache/crafter.build/external/<name>-<hash>.tmp/.git/objects/pack/tmp_pack_XXXXXX' for reading: No such file or directory fatal: fetch-pack: invalid index-pack output ``` `tmp_pack_*` is an in-flight git pack tempfile — it vanishes mid-read because the *other* build either rm'd the `.tmp` directory or finished and renamed it out from under us. ## Likely cause The cache path is derived from a hash of the dep spec, so concurrent builds that resolve to the same external compute the same `.tmp` directory name (both racing builds picked the identical `-<hash>.tmp` suffix in the failure we hit). There's no lock around `git clone → rename`, so: - build A: `git clone … .tmp` (in progress) - build B: `git clone … .tmp` (sees existing dir, or wipes-and-retries) - build A: pack tempfile yanked → `invalid index-pack output` ## Suggested fixes 1. **Per-invocation tmp dir** — clone into `<dep>.tmp.<pid>` (or `mktemp -d`), then `rename(2)` to the canonical cache path on success. `rename` is atomic on the same filesystem; the loser of the race just discards its clone. 2. **File lock on the cache dir** — `flock` on `~/.cache/crafter.build/external/<dep>.lock` around the clone+rename so only one builder actually fetches; others wait and reuse the result. Lower bandwidth usage when many builds start at once. (1) alone fixes the corruption; (1) + (2) also avoids redundant clones. ## Environment - Linux x86_64 - Two concurrent `crafter-build` invocations against sibling projects that share the user's `~/.cache/crafter.build/external/`

catbot added the

claude:ready

label

2026-05-27 03:48:08 +02:00

catbot commented

2026-05-27 03:48:47 +02:00

Member

claude:claim:b831bbc6-27c5-4963-87be-6b4dce043c86

catbot added

claude:in-progress

and removed

claude:ready

labels

2026-05-27 03:48:47 +02:00

catbot commented

2026-05-27 03:51:13 +02:00

Member

[progress]

I've found the issue. Let me look at how `Platform` is used to understand `RunCommandChecked` and how PID can be obtained.

**[progress]** ``` I've found the issue. Let me look at how `Platform` is used to understand `RunCommandChecked` and how PID can be obtained. ```

catbot commented

2026-05-27 03:52:14 +02:00

Member

[progress]

I see the race. `FetchGit` at line 106-107 derives `tmpDir` as `cloneDir + ".tmp"` — same suffix for every concurrent invocation that resolved to the same hash. The loser's pack tempfiles get yanked when the winner renames or the next process `remove_all`s the dir at line 108.

Before I patch, one question on scope:
I'll proceed with the minimal correctness fix (1) — per-invocation tmp dir, rename atomically, loser discards. That matches the guidance to not expand scope beyond what the bug requires.

**[progress]** ``` I see the race. `FetchGit` at line 106-107 derives `tmpDir` as `cloneDir + ".tmp"` — same suffix for every concurrent invocation that resolved to the same hash. The loser's pack tempfiles get yanked when the winner renames or the next process `remove_all`s the dir at line 108. Before I patch, one question on scope: I'll proceed with the minimal correctness fix (1) — per-invocation tmp dir, rename atomically, loser discards. That matches the guidance to not expand scope beyond what the bug requires. ```

catbot commented

2026-05-27 03:53:15 +02:00

Member

[progress]

Now I'll make the fix. I'll use a 64-bit random suffix from `std::random_device` so each invocation has a unique tmp dir, then `rename(2)` atomically. If the rename loses to a concurrent winner, we discard our tmp and reuse the winner's clone.
Now build to verify the change compiles.
Let me check if there's an existing build I can use to do a targeted compile of just the changed translation unit.

**[progress]** ``` Now I'll make the fix. I'll use a 64-bit random suffix from `std::random_device` so each invocation has a unique tmp dir, then `rename(2)` atomically. If the rename loses to a concurrent winner, we discard our tmp and reuse the winner's clone. Now build to verify the change compiles. Let me check if there's an existing build I can use to do a targeted compile of just the changed translation unit. ```

catbot commented

2026-05-27 03:55:05 +02:00

Member

[progress]

I'll run the full build to validate the change compiles. This will take a few minutes (it clones glslang first).
Build succeeded but `build.sh` has no `set -e`. Let me re-run the specific compile to confirm clean.

**[progress]** ``` I'll run the full build to validate the change compiles. This will take a few minutes (it clones glslang first). Build succeeded but `build.sh` has no `set -e`. Let me re-run the specific compile to confirm clean. ```

catbot referenced this issue from a pull request that will close it,

2026-05-27 03:55:39 +02:00

Race condition: concurrent builds clobber each other's external dep clones #3

catbot commented

2026-05-27 03:55:39 +02:00

Member

PR opened: #3

PR opened: https://forgejo.catcrafts.net/Catcrafts/Crafter.Build/pulls/3

catbot added

claude:done

and removed

claude:in-progress