From 12cc71b63415238b0da965d45f54d3232bdc843f Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:53:15 +0200 Subject: [PATCH] fix: hold the pipeline's fixed outputs for a run The pipeline locked only --output-dir while it also writes database.json, install/, README.md and docs/. A run on a scratch output directory could start beside the run on dist/ and rewrite them. Every run now holds the repository's .lock first. --- scripts/artifacts.py | 27 +++++++++++++++++++++++++++ scripts/common.py | 1 + scripts/pipeline.py | 18 +++++++++++++----- tests/test_artifact_lock.py | 30 +++++++++++++++++++++++++++++- wiki/tools.md | 7 +++++-- 5 files changed, 75 insertions(+), 8 deletions(-) diff --git a/scripts/artifacts.py b/scripts/artifacts.py index e0442d53..49627b19 100644 --- a/scripts/artifacts.py +++ b/scripts/artifacts.py @@ -217,6 +217,33 @@ def hold_artifact_lock(directory: str) -> Iterator[None]: else: os.environ[HELD_LOCK_ENV] = previous + +# What a pipeline run writes at fixed paths, whatever its --output-dir. +REPOSITORY_OUTPUTS = ("database.json", "install/", "README.md", "CONTRIBUTING.md", "docs/") + + +@contextlib.contextmanager +def hold_repository_outputs(root: str = ".") -> Iterator[None]: + """Hold the outputs a pipeline writes at fixed paths, for the whole run. + + The output-directory lock let a run on tmp/packtest start beside a run + on dist/: both rewrote database.json, and the second to reach the + manifest step found install/ held after minutes of work, or, when the + steps did not overlap, rewrote the committed manifests from its own + state. The lock file is the repository root's .lock. + """ + stack = contextlib.ExitStack() + try: + stack.enter_context(artifact_lock(root)) + except ArtifactLockBusy as exc: + raise ArtifactLockBusy( + f"another pipeline run is writing {', '.join(REPOSITORY_OUTPUTS)} " + f"(lock: {os.path.join(root, '.lock')}). Wait for it to finish." + ) from exc + with stack: + yield + + def _build_timestamp(db: dict | None = None) -> str: """Timestamp for generated artifacts. diff --git a/scripts/common.py b/scripts/common.py index 35b53465..396a0f23 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -2810,6 +2810,7 @@ from artifacts import ( # noqa: E402,F401 ArtifactLockBusy, artifact_lock, hold_artifact_lock, + hold_repository_outputs, _TIMESTAMP_PATTERNS, _strip_timestamps, ) diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 02971049..67a60f47 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -68,6 +68,7 @@ from common import ( artifact_lock, github_token, hold_artifact_lock, + hold_repository_outputs, load_data_dir_registry, ) from refresh_data_dirs import hold_data_dirs @@ -700,13 +701,20 @@ def main() -> None: # hold lasts until this process exits: taken step by step, it let another # run purge the packs between the build and the integrity check, which # then reported every platform SKIP and passed. + # database.json, install/, README.md and docs/ sit at fixed paths, so + # every run holds them too: a run on a scratch --output-dir otherwise + # rewrote them beside the run on dist/. held = contextlib.ExitStack() - if not args.skip_packs: - try: + try: + held.enter_context( + hold_repository_outputs(str(Path(__file__).resolve().parent.parent)) + ) + if not args.skip_packs: held.enter_context(hold_artifact_lock(args.output_dir)) - except ArtifactLockBusy as exc: - print(f"ERROR: {exc}", file=sys.stderr) - sys.exit(1) + except ArtifactLockBusy as exc: + held.close() + print(f"ERROR: {exc}", file=sys.stderr) + sys.exit(1) all_ok = True total_start = time.monotonic() diff --git a/tests/test_artifact_lock.py b/tests/test_artifact_lock.py index 7e60fd0c..14e1b8de 100644 --- a/tests/test_artifact_lock.py +++ b/tests/test_artifact_lock.py @@ -18,7 +18,12 @@ from pathlib import Path REPO_ROOT = Path(__file__).resolve().parent.parent sys.path.insert(0, str(REPO_ROOT / "scripts")) -from common import ArtifactLockBusy, artifact_lock, hold_artifact_lock # noqa: E402 +from common import ( # noqa: E402 + ArtifactLockBusy, + artifact_lock, + hold_artifact_lock, + hold_repository_outputs, +) def _tempdir(test: unittest.TestCase) -> str: @@ -200,6 +205,29 @@ class PackLockCliTest(unittest.TestCase): self.assertIn("is in use by another run", proc.stderr) +class PipelineHoldsItsFixedOutputs(unittest.TestCase): + """A pipeline on a scratch --output-dir still rewrites database.json, + install/, README.md and docs/: only the output directory was locked.""" + + def test_a_second_pipeline_is_refused_whatever_its_output_dir(self): + scratch = _tempdir(self) + with hold_repository_outputs(str(REPO_ROOT)): + proc = subprocess.run( + [sys.executable, "scripts/pipeline.py", "--offline", "--skip-docs", + "--output-dir", scratch], + capture_output=True, text=True, cwd=str(REPO_ROOT), timeout=120, + ) + self.assertEqual(proc.returncode, 1, proc.stdout + proc.stderr) + self.assertIn("another pipeline run is writing database.json", proc.stderr) + self.assertNotIn("generate database", proc.stdout.lower()) + + def test_two_holds_of_the_repository_exclude_each_other(self): + with hold_repository_outputs(str(REPO_ROOT)), \ + self.assertRaises(ArtifactLockBusy): + with hold_repository_outputs(str(REPO_ROOT)): + pass + + class HeldLockTest(unittest.TestCase): """pipeline.py took the lock step by step: between the pack build and the integrity check dist/ was free, another run's purge emptied it, and the diff --git a/wiki/tools.md b/wiki/tools.md index 976d16dd..fdb2d81b 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -88,8 +88,11 @@ alone, run `python scripts/check_buildbot_system.py`. rewrites nothing, so a diff on a generated file always means content. - **One run per output directory.** Pack generation takes a lock on `/.lock`, and a second run on the same folder stops at once - rather than read a half-written ZIP. Experiments use a scratch - `--output-dir` under `tmp/`. + rather than read a half-written ZIP. Pack experiments with + `generate_pack.py` use a scratch `--output-dir` under `tmp/`. A pipeline + run also writes `database.json`, `install/`, `README.md` and `docs/` at + fixed paths, so it holds the repository's `.lock` as well, and a second + pipeline is refused whatever its `--output-dir`. - **Read the exit status before the output.** A generator that crashed leaves the previous artifact in place, which reads exactly like a run that changed nothing.