Race condition: concurrent GRPO jobs corrupt the shared model overlay directory #21

Problem

nvflow/lib/rl/create_overlay.py creates a symlinked model-overlay directory with a patched config.json, used whenever a rollout config sets hf_config_overrides (e.g. YaRN long-context overrides). The overlay path is content-addressed only by model_path + hf_config_overrides (_overlay_path() in nvflow/lib/rl/helpers.py:437-448) — it does not vary per seed or chunk.

rollout() submits one Slurm job per (seed, chunk_id) pair (nvflow/lib/rl/rollout.py:1264, the full cross-product of seeds × chunks — e.g. 4 seeds × 8 chunks = 32 jobs). Every one of those jobs runs create_overlay.py against the exact same path, with no coordination between them:

# create_overlay.py
for name in os.listdir(model_path):
    link = os.path.join(overlay_path, name)
    if os.path.exists(link) or os.path.islink(link):
        os.unlink(link)
    target = os.path.join(model_path, name)
    os.symlink(os.path.relpath(target, overlay_path), link)   # <-- TOCTOU: two jobs interleaving here -> FileExistsError

...
with open(cfg_path, "w") as f:      # <-- not atomic; a second job can read a torn config.json mid-write
    json.dump(cfg, f, indent=2)

with open(marker, "w") as f:        # <-- marker written last, so the race window covers the whole first-time build
    f.write(expected)

Trigger: any GRPO/rollout config with hf_config_overrides set and more than one concurrent seed/chunk job — intermittent FileExistsError crashes, or vLLM in another job reading a truncated/invalid config.json, non-deterministically, only on the first (cold-cache) launch of that model+overrides combination.

Why this is easy to fix without adding a locking protocol

The overlay's target content is deterministic and identical across every concurrent job building it — all of them compute the exact same symlink targets and the exact same merged config.json for a given (model_path, hf_config_overrides) pair. So no coordination/locking is actually needed — each individual file write just needs to be atomic, since "last writer wins" converges to the same correct final state regardless of ordering:

• Symlinks: replace os.unlink() + os.symlink() with create-at-a-uniquely-named-temp-path + os.replace(tmp_link, link). os.replace() is atomic on POSIX and works for symlinks (any directory entry), same as it's already used correctly elsewhere in this codebase for regular files (e.g. enrich_rollouts.py, shuffle_jsonl.py, grounding_feature_gate.py all use tmp-file + os.replace()).
• config.json and the marker file: same tmp-file + os.replace() pattern, matching the existing convention in this repo rather than introducing something new.

This should fully eliminate the race with a small, self-contained diff to create_overlay.py alone — no lock files, no leader-election, no changes to job submission/scheduling in rollout.py.

Why I'm opening this as an issue rather than a PR

This touches concurrent Slurm job behavior, which I'd want a maintainer's sanity check on before submitting code — in particular, confirming os.replace() behaves as expected for symlinks on the shared filesystem this actually runs on (Lustre, per INSTALL.md), since atomic rename semantics can vary across network filesystems. Happy to submit the PR once the approach sounds right, or to adjust it if there's a reason the current approach was chosen that I'm missing.