diff --git a/scripts/bash/create-new-feature.sh b/scripts/bash/create-new-feature.sh index abdb2194b1..ef2a1854ab 100644 --- a/scripts/bash/create-new-feature.sh +++ b/scripts/bash/create-new-feature.sh @@ -167,6 +167,21 @@ fit_branch_name() { printf '%s' "$branch_name" } +# After a lost exclusive mkdir, pick the next sequential number. +rescan_sequential_feature() { + local highest + highest=$(get_highest_from_specs "$SPECS_DIR") + if [ "$highest" -eq "$MAX_FEATURE_NUMBER" ]; then + echo "Error: feature number must be between 0 and $MAX_FEATURE_NUMBER, got '9223372036854775808'" >&2 + exit 1 + fi + BRANCH_NUMBER=$((highest + 1)) + FEATURE_NUM=$(printf "%03d" "$((10#$BRANCH_NUMBER))") + BRANCH_NAME=$(fit_branch_name "$FEATURE_NUM" "$BRANCH_SUFFIX") + FEATURE_DIR="$SPECS_DIR/$BRANCH_NAME" + SPEC_FILE="$FEATURE_DIR/spec.md" +} + # Quote a value for POSIX shell reuse, byte-identical to Python's shlex.quote # so the persistence hints match the Python variant exactly (printf %q output # differs between bash versions and from shlex.quote for spaces/metachars). @@ -355,7 +370,29 @@ if [ "$DRY_RUN" != true ]; then fi fi - mkdir -p "$FEATURE_DIR" + # Exclusive create: plain mkdir fails with EEXIST if another invocation + # reserved the same FEATURE_DIR after the exists check above. Rescan + # and retry before writing spec.md so the loser cannot overwrite it. + while true; do + if [ "$ALLOW_EXISTING" = true ] && [ -d "$FEATURE_DIR" ]; then + break + fi + if mkdir "$FEATURE_DIR" 2>/dev/null; then + break + fi + if [ ! -d "$FEATURE_DIR" ]; then + echo "Error: could not create feature directory '$FEATURE_DIR'" >&2 + exit 1 + fi + if [ "$ALLOW_EXISTING" = true ]; then + break + fi + if [ "$USE_TIMESTAMP" = true ]; then + >&2 echo "Error: Feature directory '$FEATURE_DIR' already exists. Rerun to get a new timestamp or use a different --short-name." + exit 1 + fi + rescan_sequential_feature + done if [ "$NEEDS_SPEC" = true ]; then if [ "$SPEC_TEMPLATE_FOUND" = true ]; then diff --git a/scripts/powershell/create-new-feature.ps1 b/scripts/powershell/create-new-feature.ps1 index e7a68c4076..b48f7e2501 100644 --- a/scripts/powershell/create-new-feature.ps1 +++ b/scripts/powershell/create-new-feature.ps1 @@ -268,7 +268,39 @@ if (-not $DryRun) { $content = Resolve-TemplateContent -TemplateName 'spec-template' -RepoRoot $repoRoot } - New-Item -ItemType Directory -Path $featureDir -Force | Out-Null + # Exclusive create: New-Item without -Force fails if another invocation + # reserved the same FEATURE_DIR after the exists check above. Rescan + # and retry before writing spec.md so the loser cannot overwrite it. + while ($true) { + if ($AllowExistingBranch -and (Test-Path -LiteralPath $featureDir -PathType Container)) { + break + } + try { + New-Item -ItemType Directory -Path $featureDir | Out-Null + break + } catch { + if (-not (Test-Path -LiteralPath $featureDir -PathType Container)) { + throw + } + if ($AllowExistingBranch) { + break + } + if ($Timestamp) { + Write-Error "Error: Feature directory '$featureDir' already exists. Rerun to get a new timestamp or use a different -ShortName." + exit 1 + } + $highestNumber = Get-HighestNumberFromSpecs -SpecsDir $specsDir + if ($highestNumber -eq [long]::MaxValue) { + Write-Error "Error: feature number must be between 0 and $([long]::MaxValue), got '9223372036854775808'" + exit 1 + } + $resolvedNumber = $highestNumber + 1 + $featureNum = ('{0:000}' -f $resolvedNumber) + $branchName = Get-FittedBranchName -FeatureNum $featureNum -BranchSuffix $branchSuffix + $featureDir = Join-Path $specsDir $branchName + $specFile = Join-Path $featureDir 'spec.md' + } + } if ($needsSpec) { if ($null -ne $content) { diff --git a/scripts/python/create_new_feature.py b/scripts/python/create_new_feature.py index f36064afbb..29a2c2b94e 100644 --- a/scripts/python/create_new_feature.py +++ b/scripts/python/create_new_feature.py @@ -394,7 +394,37 @@ def main(argv: list[str] | None = None) -> int: print(f"Error: {exc}", file=sys.stderr) return 1 - feature_dir.mkdir(parents=True, exist_ok=True) + # Exclusive create: mkdir without exist_ok fails if another invocation + # reserved the same FEATURE_DIR after the exists check above. Rescan + # and retry before writing spec.md so the loser cannot overwrite it. + while True: + if args.allow_existing and feature_dir.is_dir(): + break + try: + feature_dir.mkdir(parents=True) + break + except FileExistsError: + if args.allow_existing: + break + if args.use_timestamp: + print( + f"Error: Feature directory '{feature_dir}' already exists. " + "Rerun to get a new timestamp or use a different --short-name.", + file=sys.stderr, + ) + return 1 + number = _get_highest_from_specs(specs_dir) + 1 + if number > _MAX_FEATURE_NUMBER: + print( + f"Error: feature number must be between 0 and " + f"{_MAX_FEATURE_NUMBER}, got '{number}'", + file=sys.stderr, + ) + return 1 + feature_num = f"{number:03d}" + branch_name = _fit_branch_name(feature_num, branch_suffix) + feature_dir = specs_dir / branch_name + spec_file = feature_dir / "spec.md" if needs_spec: if template_content is not None: diff --git a/tests/test_create_new_feature_python_parity.py b/tests/test_create_new_feature_python_parity.py index 41122b1f5f..fc36c26ce1 100644 --- a/tests/test_create_new_feature_python_parity.py +++ b/tests/test_create_new_feature_python_parity.py @@ -2,7 +2,10 @@ from __future__ import annotations +import os import re +import shutil +import subprocess from pathlib import Path import pytest @@ -14,6 +17,7 @@ HAS_POWERSHELL, bash_cmd, break_wrap_layer, + clean_env, install_composition_stack, install_scripts, json_stdout, @@ -1065,3 +1069,186 @@ def test_all_variants_corrected_prefix_skips_timestamp_collision(repo: Path) -> assert json_stdout(py)["FEATURE_NUM"] == "20260320" for result in (bash, ps, py): assert "using 20260320 instead" in result.stderr + + +def _shared_specs_repos(tmp_path: Path) -> tuple[Path, Path, Path]: + """Two project roots that share one specs/ directory via symlink.""" + repo_a = _setup_repo(tmp_path, "proj-a") + repo_b = _setup_repo(tmp_path, "proj-b") + shared = tmp_path / "shared-specs" + shared.mkdir() + try: + (repo_a / "specs").symlink_to(shared, target_is_directory=True) + (repo_b / "specs").symlink_to(shared, target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("Symlinks are not available in this environment") + (repo_a / ".specify" / "templates" / "spec-template.md").write_text( + "ALPHA-SPEC\n", encoding="utf-8" + ) + (repo_b / ".specify" / "templates" / "spec-template.md").write_text( + "BETA-SPEC\n", encoding="utf-8" + ) + return repo_a, repo_b, shared + + +def _mkdir_barrier_env(tmp_path: Path, n_waiters: int = 2) -> dict[str, str]: + """Pause FEATURE_DIR mkdir until n_waiters arrive so the TOCTOU window is forced. + + Two processes that have already scanned the same max spec number then both + call mkdir on the same specs/NNN-name path. mkdir -p / exist_ok=True lets + both succeed and the second write overwrites spec.md. + """ + env = clean_env() + barrier = tmp_path / "mkdir-barrier" + barrier.mkdir() + released = barrier / "released" + real_mkdir = shutil.which("mkdir") + if real_mkdir is None: + pytest.skip("mkdir not available") + bin_dir = tmp_path / "mkdir-bin" + bin_dir.mkdir() + wrapper = bin_dir / "mkdir" + wrapper.write_text( + f"""#!/bin/sh +path_last="" +for arg in "$@"; do + path_last="$arg" +done +base=$(basename -- "$path_last") +parent=$(basename -- "$(dirname -- "$path_last")") +case "$parent/$base" in + specs/[0-9]*-*) + if [ ! -f "{released}" ]; then + i=0 + while ! mkdir "{barrier}/w-$i" 2>/dev/null; do + i=$((i + 1)) + if [ "$i" -gt 100 ]; then + break + fi + done + n=0 + while [ "$n" -lt 200 ]; do + count=$(ls -d "{barrier}"/w-* 2>/dev/null | wc -l) + if [ "$count" -ge {n_waiters} ]; then + touch "{released}" + break + fi + if [ -f "{released}" ]; then + break + fi + sleep 0.05 + n=$((n + 1)) + done + fi + ;; +esac +exec "{real_mkdir}" "$@" +""", + encoding="utf-8", + ) + wrapper.chmod(0o755) + env["PATH"] = f"{bin_dir}{os.pathsep}{env.get('PATH', '')}" + env["SPECKIT_MKDIR_BARRIER"] = str(barrier) + env["SPECKIT_MKDIR_BARRIER_N"] = str(n_waiters) + site = tmp_path / "mkdir-site" + site.mkdir() + (site / "sitecustomize.py").write_text( + """ +import os +import time +from pathlib import Path + +_orig_mkdir = os.mkdir +_barrier = Path(os.environ["SPECKIT_MKDIR_BARRIER"]) +_released = _barrier / "released" +_n_waiters = int(os.environ.get("SPECKIT_MKDIR_BARRIER_N", "2")) + + +def _is_feature_dir(path): + p = Path(path) + return p.parent.name == "specs" and "-" in p.name and p.name[0:1].isdigit() + + +def _gated_mkdir(path, mode=0o777, *args, **kwargs): + if _is_feature_dir(path) and not _released.exists(): + (_barrier / f"w-py-{os.getpid()}-{time.time_ns()}").mkdir(exist_ok=True) + for _ in range(200): + if _released.exists(): + break + waiters = list(_barrier.glob("w-*")) + if len(waiters) >= _n_waiters: + _released.touch() + break + time.sleep(0.05) + return _orig_mkdir(path, mode, *args, **kwargs) + + +os.mkdir = _gated_mkdir +""", + encoding="utf-8", + ) + env["PYTHONPATH"] = f"{site}{os.pathsep}{env.get('PYTHONPATH', '')}" + return env + + +def _popen( + cmd: list[str], repo: Path, env: dict[str, str] +) -> subprocess.Popen[str]: + return subprocess.Popen( + cmd, + cwd=repo, + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + + +def _wait( + proc: subprocess.Popen[str], cmd: list[str] +) -> subprocess.CompletedProcess[str]: + stdout, stderr = proc.communicate(timeout=20) + return subprocess.CompletedProcess(cmd, proc.returncode, stdout, stderr) + + +@requires_bash +@pytest.mark.parametrize("variant", ["bash", "python"]) +def test_concurrent_reservations_do_not_share_spec_directory( + tmp_path: Path, variant: str +) -> None: + """Two concurrent reservations must not land in the same FEATURE_DIR. + + create-new-feature scans max(specs)+1, checks that FEATURE_DIR is absent, + then creates it with mkdir -p (bash) / New-Item -Force (powershell) / + Path.mkdir(exist_ok=True) (python). That create is not exclusive, so two + invocations can share the directory and the second overwrites spec.md. + """ + repo_a, repo_b, shared = _shared_specs_repos(tmp_path) + env = _mkdir_barrier_env(tmp_path) + args = ("--json", "--short-name", "user-auth", "Add user authentication") + if variant == "bash": + cmd_a = bash_cmd(repo_a, SCRIPT, *args) + cmd_b = bash_cmd(repo_b, SCRIPT, *args) + else: + cmd_a = py_cmd(repo_a, SCRIPT, *args) + cmd_b = py_cmd(repo_b, SCRIPT, *args) + + proc_a = _popen(cmd_a, repo_a, env) + proc_b = _popen(cmd_b, repo_b, env) + result_a = _wait(proc_a, cmd_a) + result_b = _wait(proc_b, cmd_b) + + assert result_a.returncode == 0, result_a.stderr + assert result_b.returncode == 0, result_b.stderr + + data_a = json_stdout(result_a) + data_b = json_stdout(result_b) + assert data_a["BRANCH_NAME"] != data_b["BRANCH_NAME"] + assert {data_a["FEATURE_NUM"], data_b["FEATURE_NUM"]} == {"001", "002"} + + names = sorted(path.name for path in shared.iterdir() if path.is_dir()) + assert names == ["001-user-auth", "002-user-auth"] + contents = { + (shared / name / "spec.md").read_text(encoding="utf-8") for name in names + } + assert contents == {"ALPHA-SPEC\n", "BETA-SPEC\n"}