diff --git a/changelog.d/git-fixture-maintenance-lock.fixed.md b/changelog.d/git-fixture-maintenance-lock.fixed.md new file mode 100644 index 0000000000..4e562c519c --- /dev/null +++ b/changelog.d/git-fixture-maintenance-lock.fixed.md @@ -0,0 +1 @@ +Test repositories no longer start git's background maintenance. `git commit` starts `git maintenance run --auto`, which detaches holding `.git/objects/maintenance.lock` and can remove it after the commit has returned. The Axiom checkout-reference tests hash `.git`, so on a busy runner the digest could list the lock and fail to read it, failing the engine-free job (`FileNotFoundError` in `test_a_clean_checkout_at_the_declared_commit_is_accepted`). Every test that commits to or fetches into a throwaway repository now applies the settings in `test_support.git_repositories`: as `-c` options (`NO_BACKGROUND_MAINTENANCE`) or, where the code under test runs git (the Modal stage branch check's fetch), as environment variables (`NO_BACKGROUND_MAINTENANCE_ENV`). `rulespec_tree_digest` still refuses a tree that loses a file while it is hashed, now with a `ValueError` that names the file and the remedy instead of an unhandled `FileNotFoundError`. diff --git a/docs/agent-guide.md b/docs/agent-guide.md index 0d68f21b65..2c52453d05 100644 --- a/docs/agent-guide.md +++ b/docs/agent-guide.md @@ -152,6 +152,18 @@ Move fixtures and helper functions shared by those files to instead of deriving them from a test module's `__file__`. Keep fixture data in the package's root `tests/fixtures/` or `tests/golden/` directory. +A test that commits to or fetches into a throwaway git repository keeps git's +background maintenance out of it with the settings in +`test_support.git_repositories`: `["git", *NO_BACKGROUND_MAINTENANCE, ...]` for +git the test runs, or `NO_BACKGROUND_MAINTENANCE_ENV` in the environment for +git the code under test runs (a fetch, say). A helper whose environment +already carries the variables needs no options. Code that strips git's +repository-local variables (`git rev-parse --local-env-vars`, as the Axiom +checkout check does) drops the variables. Without these settings, `git commit` +and `git fetch` can leave a detached maintenance process that removes the +repository's `objects/maintenance.lock` after the command has returned, and a +test that lists or hashes `.git` then fails now and then. + `tools/ci_test_plan.py` recursively enumerates and validates this layout. Run `python3 tools/ci_test_plan.py verify` after adding, moving, or splitting tests, and use `python3 tools/ci_test_plan.py describe` to inspect the generated group, diff --git a/packages/microcosm-build/tests/engine_free/us/test_us_chronicle_feed.py b/packages/microcosm-build/tests/engine_free/us/test_us_chronicle_feed.py index 3f9bd565b0..648d3292b5 100644 --- a/packages/microcosm-build/tests/engine_free/us/test_us_chronicle_feed.py +++ b/packages/microcosm-build/tests/engine_free/us/test_us_chronicle_feed.py @@ -26,6 +26,7 @@ load_us_chronicle_feed, us_chronicle_feed_scope_sha256, ) +from test_support.git_repositories import NO_BACKGROUND_MAINTENANCE from test_support.paths import paths_for _TEST_PATHS = paths_for("microcosm-build") @@ -257,7 +258,10 @@ def _row(key: str, record_set: str, period_type: str, period_value, value) -> di def _git(root: Path, *args: str) -> str: return subprocess.run( - ["git", "-C", str(root), *args], check=True, capture_output=True, text=True + ["git", *NO_BACKGROUND_MAINTENANCE, "-C", str(root), *args], + check=True, + capture_output=True, + text=True, ).stdout.strip() diff --git a/packages/microcosm-build/tests/engine_free/us/test_us_modal_stage_plan_tool.py b/packages/microcosm-build/tests/engine_free/us/test_us_modal_stage_plan_tool.py index 90307cd5fb..4ac5c47379 100644 --- a/packages/microcosm-build/tests/engine_free/us/test_us_modal_stage_plan_tool.py +++ b/packages/microcosm-build/tests/engine_free/us/test_us_modal_stage_plan_tool.py @@ -26,6 +26,10 @@ from hypothesis import HealthCheck, given, settings from hypothesis import strategies as st +from test_support.git_repositories import ( + NO_BACKGROUND_MAINTENANCE, + NO_BACKGROUND_MAINTENANCE_ENV, +) from test_support.microcosm_build.us_modal_stage_plan_tool import ( COMMIT, FEED_SHA, @@ -690,6 +694,8 @@ def test_receipt_prices_earlier_preempted_attempts() -> None: # --------------------------------------------------------------------------- # +# The branch check runs git itself, from os.environ, so the settings that +# keep background maintenance out of these repositories travel as variables. _GIT_ENV = { "PATH": os.environ.get("PATH", ""), "HOME": os.environ.get("HOME", "/"), @@ -699,6 +705,7 @@ def test_receipt_prices_earlier_preempted_attempts() -> None: "GIT_AUTHOR_EMAIL": "t@example.com", "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@example.com", + **NO_BACKGROUND_MAINTENANCE_ENV, } @@ -764,6 +771,56 @@ def test_branch_is_verified_against_the_remote( assert result["branch_tip"] == remote["c3"] +def test_the_branch_check_starts_no_git_maintenance( + app, remote, monkeypatch, tmp_path +) -> None: + """Its fetch leaves no maintenance process writing to the scratch clone.""" + + trace = tmp_path / "trace2-events" + for key, value in {**_GIT_ENV, "GIT_TRACE2_EVENT": str(trace)}.items(): + monkeypatch.setenv(key, value) + plan = dataclasses.replace( + plan_lib.parse_plan(_plan_data()), + repo_url=remote["url"], + branch="feature/x", + commit=remote["c3"], + ) + assert app._verify_branch(plan)["branch_verified"] is True + started = [ + event["argv"] + for event in map(json.loads, trace.read_text(encoding="utf-8").splitlines()) + if event["event"] in ("start", "child_start") + ] + # The trace is live: it holds the fetch's own start. + assert any("fetch" in argv for argv in started) + assert [argv for argv in started if {"maintenance", "gc"} & set(argv)] == [] + + +def test_the_maintenance_variables_set_what_the_options_set(tmp_path) -> None: + if shutil.which("git") is None: + pytest.skip("git is not installed") + base = { + key: value + for key, value in _GIT_ENV.items() + if key not in NO_BACKGROUND_MAINTENANCE_ENV + } + + def command_scope(options: tuple[str, ...], extra: dict[str, str]) -> list[str]: + listed = subprocess.run( + ["git", *options, "config", "--list", "--show-scope"], + cwd=tmp_path, + env={**base, **extra}, + check=True, + capture_output=True, + text=True, + ).stdout + return [line for line in listed.splitlines() if line.startswith("command\t")] + + by_options = command_scope(NO_BACKGROUND_MAINTENANCE, {}) + assert len(by_options) == len(NO_BACKGROUND_MAINTENANCE) // 2 + assert command_scope((), dict(NO_BACKGROUND_MAINTENANCE_ENV)) == by_options + + def test_branch_check_argvs_never_touch_the_pinned_clone() -> None: argvs = plan_lib.branch_check_argvs( plan_lib.DEFAULT_REPO_URL, "us-modal-stage-runner", COMMIT, "/tmp/check.git" diff --git a/packages/microcosm-build/tests/engine_workflow/us/test_us_annual_static_aging.py b/packages/microcosm-build/tests/engine_workflow/us/test_us_annual_static_aging.py index cb66df553d..708cd5e8f8 100644 --- a/packages/microcosm-build/tests/engine_workflow/us/test_us_annual_static_aging.py +++ b/packages/microcosm-build/tests/engine_workflow/us/test_us_annual_static_aging.py @@ -14,6 +14,7 @@ from microcosm.build import us_annual_static_aging as annual from microcosm.frame import US_SCHEMA, SignedScale, WeightKind, Weights from microcosm.frame.materialize import put_frame_table, read_frame_table +from test_support.git_repositories import NO_BACKGROUND_MAINTENANCE @pytest.fixture @@ -263,7 +264,8 @@ def test_model_provenance_requires_imported_clean_pinned_source(tmp_path, monkey def git(*args): return subprocess.check_output( - ["git", "-C", str(tmp_path), *args], text=True + ["git", *NO_BACKGROUND_MAINTENANCE, "-C", str(tmp_path), *args], + text=True, ).strip() git("init", "-q") diff --git a/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py b/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py index b3afe7c520..a5c3f5d9d0 100644 --- a/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py +++ b/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py @@ -829,8 +829,8 @@ def _require_pinned_tree(self) -> None: Raises: ValueError: If the module path resolves to another file, the root - holds a symbolic link, or its digest moved since the - reference. + holds a symbolic link, its digest moved since the reference, + or a file under it is removed while it is hashed. """ module = self._module.resolve() if module != self._pinned_module: @@ -1130,7 +1130,9 @@ def rulespec_tree_digest(root: str | Path) -> tuple[str, int]: ``(sha256 hex digest, total bytes of the hashed files)``. Raises: - ValueError: If ``root`` is not a directory. + ValueError: If ``root`` is not a directory, or a file listed under it + is removed before it is read: a tree that changes while it is + hashed has no digest to name. """ path = Path(root) if not path.is_dir(): @@ -1140,7 +1142,17 @@ def rulespec_tree_digest(root: str | Path) -> tuple[str, int]: files = sorted(candidate for candidate in path.rglob("*") if candidate.is_file()) for candidate in files: relative = candidate.relative_to(path).as_posix().encode("utf-8") - content = candidate.read_bytes() + try: + content = candidate.read_bytes() + except FileNotFoundError as exc: + raise ValueError( + f"RuleSpec root {path} changed while it was being hashed: " + f"{relative.decode('utf-8')} was listed and then removed. Hash " + "the tree once nothing is writing to it. If the root is a git " + "checkout, a git process still working in it can do this (its " + "background maintenance holds a lock file under .git); wait " + "for it to finish, or reference a git archive export instead." + ) from exc digest.update(len(relative).to_bytes(8, "little")) digest.update(relative) digest.update(len(content).to_bytes(8, "little")) @@ -1205,7 +1217,12 @@ def axiom_engine_ref( reference specific to one clone: two clones or worktrees of one commit (a worktree's ``.git`` file names an absolute path) and an export of that commit all give different references, and any later git operation in the - checkout moves it. Prefer the export. + checkout moves it. That includes the maintenance git starts in the + background after a commit: until it finishes, its lock file under ``.git`` + is part of the tree. A reference taken then is refused if the lock is + removed between the digest's listing and its read; otherwise it names a + tree that holds the lock, and the adapter's next compile or reference + refuses the root once the lock is gone. Prefer the export. The first call pins the adapter to the tree digest it names and to the file its module path resolves to: it refuses an adapter that has already @@ -1236,7 +1253,8 @@ def axiom_engine_ref( points at); a git checkout root is not its repository's top level, is dirty, contains a submodule, has a file flagged skip-worktree or assume-unchanged, holds bytes its commit does - not, or is at another commit; or the adapter compiled before its + not, or is at another commit; a file under the root is removed + while the tree is hashed; or the adapter compiled before its first reference, or its root or module moved after it. """ if not isinstance(engine, AxiomEngine): diff --git a/packages/microcosm-frame/tests/engine_free/shared/test_axiom_graph_adapter.py b/packages/microcosm-frame/tests/engine_free/shared/test_axiom_graph_adapter.py index e939327fbc..fed236cd60 100644 --- a/packages/microcosm-frame/tests/engine_free/shared/test_axiom_graph_adapter.py +++ b/packages/microcosm-frame/tests/engine_free/shared/test_axiom_graph_adapter.py @@ -69,6 +69,7 @@ ) from microcosm.graph.canonical import canonical_json, sha256_domain from microcosm.graph.keys import _directory_identity, source_content_key +from test_support.git_repositories import NO_BACKGROUND_MAINTENANCE from test_support.paths import paths_for _TEST_PATHS = paths_for("microcosm-frame") @@ -103,7 +104,9 @@ RULESPEC_COMMIT = "c" * 40 # A full override of every git setting a test repository depends on, so the -# developer's global configuration (hooks, signing) never reaches it. +# developer's global configuration (hooks, signing) never reaches it. The +# checkout references below hash ``.git``, so ``_git`` also keeps git's +# background maintenance (and its lock file) out of every test repository. _GIT_ENV = { **os.environ, "GIT_CONFIG_GLOBAL": os.devnull, @@ -115,6 +118,7 @@ def _git(root: Path, *args: str) -> str: completed = subprocess.run( [ "git", + *NO_BACKGROUND_MAINTENANCE, "-c", "user.name=microcosm-test", "-c", @@ -970,6 +974,26 @@ def _write_tree(root: Path, files: Mapping[str, bytes]) -> Path: return root +def _remove_on_first_read( + patch: pytest.MonkeyPatch, victim: Path, *, trigger: Path | None = None +) -> None: + """Remove ``victim`` when a file is first read, as a process releasing a lock. + + The tree digest lists every file and then reads each, so a file removed at + the first read was listed and is gone before its own turn. With + ``trigger``, the removal waits for that file's read. + """ + + read_bytes = Path.read_bytes + + def removing(self: Path) -> bytes: + if trigger is None or self == trigger: + victim.unlink(missing_ok=True) + return read_bytes(self) + + patch.setattr(Path, "read_bytes", removing) + + def _ref(root: Path, **engine_kwargs) -> str: engine = AxiomEngine( root / _MODULE, schema=NZ_SCHEMA, rulespec_roots=(root,), **engine_kwargs @@ -1036,6 +1060,31 @@ def test_a_file_is_not_a_root(self, tmp_path) -> None: with pytest.raises(ValueError, match="not a directory"): rulespec_tree_digest(tmp_path / "file.yaml") + @settings(max_examples=40, deadline=None) + @given(files=rulespec_trees(), data=st.data()) + def test_property_a_file_removed_after_the_listing_is_refused_by_name( + self, tmp_path_factory, files, data + ) -> None: + """No digest names a tree that lost a file while it was hashed.""" + + root = _write_tree(tmp_path_factory.mktemp("tree"), files) + victim = data.draw(st.sampled_from(sorted(files))) + before = rulespec_tree_digest(root) + with pytest.MonkeyPatch.context() as patch: + _remove_on_first_read(patch, root / victim) + with pytest.raises( + ValueError, + match=re.escape( + f"changed while it was being hashed: {victim} was listed" + ), + ): + rulespec_tree_digest(root) + # The file is not skipped: the settled tree has its own digest, the + # one the graph gives it. + settled = rulespec_tree_digest(root) + assert settled != before + assert settled == _directory_identity(root) + class TestAxiomEngineRef: def test_the_reference_is_canonical_json_of_every_pin(self, tmp_path) -> None: @@ -1585,6 +1634,53 @@ def test_a_clean_checkout_at_the_declared_commit_is_accepted( assert self._checkout_ref(root, head) == first assert json.loads(first)["rulespec_commit"] == head + def test_a_commit_in_a_test_repository_starts_no_git_maintenance( + self, checkout, tmp_path, monkeypatch + ) -> None: + """The fixture's repositories stay still once ``_git`` returns. + + A commit otherwise starts ``git maintenance run --auto``, which + detaches holding ``.git/objects/maintenance.lock`` and removes it + later: a file the checkout reference lists and may not find. + """ + + root, _ = checkout + trace = tmp_path / "trace2-events" + monkeypatch.setitem(_GIT_ENV, "GIT_TRACE2_EVENT", str(trace)) + _git(root, "commit", "-q", "--allow-empty", "-m", "traced") + events = [ + json.loads(line) for line in trace.read_text(encoding="utf-8").splitlines() + ] + started = [ + event["argv"] + for event in events + if event["event"] in ("start", "child_start") + ] + # The trace is live: it holds the commit's own start. + assert any("commit" in argv for argv in started) + assert [argv for argv in started if {"maintenance", "gc"} & set(argv)] == [] + assert not (root / ".git/objects/maintenance.lock").exists() + + def test_a_lock_git_removes_while_the_checkout_is_hashed_is_refused( + self, checkout, monkeypatch + ) -> None: + """A checkout git is still working in is refused, and says why.""" + + root, head = checkout + lock = root / ".git/objects/maintenance.lock" + lock.write_bytes(b"") + # Only the digest reads .git, and it reads HEAD before objects/. + _remove_on_first_read(monkeypatch, lock, trigger=root.resolve() / ".git/HEAD") + with pytest.raises( + ValueError, + match=r"changed while it was being hashed: " + r"\.git/objects/maintenance\.lock ", + ): + self._checkout_ref(root, head) + assert not lock.exists() + # Once git has finished, the same checkout is accepted. + assert json.loads(self._checkout_ref(root, head))["rulespec_commit"] == head + def test_a_modified_file_is_refused(self, checkout) -> None: root, head = checkout (root / _MODULE).write_bytes(b"format: rulespec/v1\nrules: [edited]\n") diff --git a/test_support/git_repositories.py b/test_support/git_repositories.py new file mode 100644 index 0000000000..e462c1a428 --- /dev/null +++ b/test_support/git_repositories.py @@ -0,0 +1,48 @@ +"""Git options for the throwaway repositories tests create.""" + +from __future__ import annotations + +from collections.abc import Mapping +from types import MappingProxyType + +#: ``git -c`` options that keep background maintenance out of a test repository. +#: +#: After its own work, ``git commit`` starts ``git maintenance run --auto``. +#: That process takes ``.git/objects/maintenance.lock``, detaches, and removes +#: the lock only when it finishes, which can be after the commit has returned. +#: A test that lists or hashes ``.git`` straight after a commit can therefore +#: list a lock file that is gone by the time it is read. +#: +#: ``maintenance.auto=false`` stops git starting that process, and so does +#: ``gc.auto=0`` when ``maintenance.auto`` is unset. If a maintenance run +#: starts all the same, ``maintenance.autoDetach=false`` keeps it in the +#: foreground, so it releases its lock before the command returns. Read from +#: git 2.55.0: ``prepare_auto_maintenance`` in ``run-command.c`` and +#: ``maintenance_run_tasks`` in ``builtin/gc.c``. +#: +#: Pass them before the subcommand: ``["git", *NO_BACKGROUND_MAINTENANCE, ...]``. +NO_BACKGROUND_MAINTENANCE: tuple[str, ...] = ( + "-c", + "maintenance.auto=false", + "-c", + "gc.auto=0", + "-c", + "maintenance.autoDetach=false", +) + +#: The same settings as ``GIT_CONFIG_COUNT`` environment variables, for git +#: that the code under test runs from ``os.environ``, where no ``-c`` option +#: reaches it. They override any ``GIT_CONFIG_COUNT`` entries already in the +#: environment; a ``GIT_CONFIG_PARAMETERS`` there (an outer ``git -c``) still +#: applies, after them. Code that strips git's repository-local variables +#: (``git rev-parse --local-env-vars``) drops them. +NO_BACKGROUND_MAINTENANCE_ENV: Mapping[str, str] = MappingProxyType( + { + "GIT_CONFIG_COUNT": str(len(NO_BACKGROUND_MAINTENANCE) // 2), + **{ + f"GIT_CONFIG_{part}_{index}": value + for index, setting in enumerate(NO_BACKGROUND_MAINTENANCE[1::2]) + for part, value in zip(("KEY", "VALUE"), setting.split("=", 1), strict=True) + }, + } +) diff --git a/test_support/microcosm_graph/graph_acceptance_burndown.py b/test_support/microcosm_graph/graph_acceptance_burndown.py index 2120391ce9..a513cd6d4c 100644 --- a/test_support/microcosm_graph/graph_acceptance_burndown.py +++ b/test_support/microcosm_graph/graph_acceptance_burndown.py @@ -19,6 +19,7 @@ import pytest +from test_support.git_repositories import NO_BACKGROUND_MAINTENANCE from test_support.paths import paths_for from tools import graph_acceptance_burndown as burndown @@ -115,6 +116,7 @@ def _repository(tmp_path: Path, sources: dict[str, str]) -> Path: subprocess.run( [ "git", + *NO_BACKGROUND_MAINTENANCE, "-c", "user.name=t", "-c",