Race condition in toKtx.ts

[mramato]

Describe the bug

toKtx.ts checks if a directory exists before creating it, resulting in a crash/race condition when multiple instances of gltf-transform are run simultaneously. The offending line is:

https://github.com/donmccurdy/glTF-Transform/blob/46a74055d829c8e15b001c3ff55952e0dd1966d0/packages/cli/src/transforms/toktx.ts#L152

I'm also skeptical of gltf-transform always using the same tmp folder across all processes, since it seems like file name collision could be a real problem.

To Reproduce Steps to reproduce the behavior:

1. Ensure $tmp/gltf-transform does not exist.
2. Try to run gltf-transform etc1s in.glb out.glb many times concurrently
3. It might crash with error: EEXIST: file already exists, mkdir '/tmp/gltf-transform' (since this is a race condition it's not 100%).

Expected behavior

• gltf-transform should not crash

Versions:

• Version: 4.10.1
• Environment: CLI on Kubuntu 22.04 installed via npm on Node 20.x

Additional context

In general, it's bad practice to check for an existence of an external resource before using it, specifically because of race conditions like the above. While I'm not intimately familiar with the source, the simple fix should be to remove the if check and use recursive: true, which will handle the "create if not exist" logic correctly for us.

mkdirSync(batchDir, { recursive: true }); 

As I mentioned above, I'm also concerned a well known tmp dir name is always used (gltf-transform) because this means that multiple processes could stomp on each other. Should it create a random name each time (using randomUUID perhaps?)

[donmccurdy]

Thanks @mramato! Within the tmp directory, gltf-transform will generate a UUID (unique to the toktx() operation) and prefix each file with that. So there shouldn't be per-texture conflicts, although it isn't statistically impossible.

I wasn't aware that recursive: false could be used to prevent mkdirSync from throwing an error if the directory already exists – that's good to know, and I agree that the change to remove existsSync should be made.

---

Asides –

In the longer term, I'd like to avoid having a CLI dependency involved in KTX compression at all. Long discussion and open questions:

• 675

You may also find it's faster to do batch processing using a glTF Transform based script rather than the glTF Transform CLI – initializing the CLI session many times has significant overhead. The toktx() function can be imported from @gltf-transform/cli.

[mramato]

Thanks for the quick reply, @donmccurdy.

Within the tmp directory, gltf-transform will generate a UUID (unique to the toktx() operation) and prefix each file with that. So there shouldn't be per-texture conflicts, although it isn't statistically impossible.

That's good to know and makes me way less worried about my current hacked together POC actually working. I can just create the gltf-transform top-level folder at startup.

You may also find it's faster to do batch processing using a glTF Transform based script rather than the glTF Transform CLI – initializing the CLI session many times has significant overhead. The toktx() function can be imported from @gltf-transform/cli.

I was not aware of this, and that is awesome. I will definitely look at going this route. I was doing a quick and dirty POC for KTX in this case, but we actually already use @gltf-transform/core elsewhere so if I can just import toktx from that that's the route I'll go.

[mramato]

Thanks again, @donmccurdy!