From 1123be82f9b54ecc3a9791fd89ff5209c2f3bd50 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 13 Oct 2026 13:24:58 -0400 Subject: [PATCH 1/3] Keep git's background maintenance out of test repositories main's Tests run at 74f5e6123 failed once in engine-free (3.14): test_a_clean_checkout_at_the_declared_commit_is_accepted raised FileNotFoundError on .git/objects/maintenance.lock inside rulespec_tree_digest. git commit starts `git maintenance run --auto`, which takes that lock, detaches and removes it after the commit has returned. The checkout reference hashes .git: the digest lists every file and then reads each, so it can list the lock and find it gone. Every test helper that commits to a throwaway repository now passes test_support.git_repositories.NO_BACKGROUND_MAINTENANCE, so no maintenance process starts. rulespec_tree_digest still refuses a tree that loses a file while it is hashed, now with a ValueError naming the file instead of an unhandled FileNotFoundError. Co-Authored-By: Claude Opus 5.5 --- .../git-fixture-maintenance-lock.fixed.md | 1 + .../engine_free/us/test_us_chronicle_feed.py | 6 +- .../us/test_us_modal_stage_plan_tool.py | 3 +- .../us/test_us_annual_static_aging.py | 4 +- .../src/microcosm/frame/adapters/axiom.py | 23 ++++- .../shared/test_axiom_graph_adapter.py | 96 ++++++++++++++++++- test_support/git_repositories.py | 28 ++++++ .../graph_acceptance_burndown.py | 2 + 8 files changed, 155 insertions(+), 8 deletions(-) create mode 100644 changelog.d/git-fixture-maintenance-lock.fixed.md create mode 100644 test_support/git_repositories.py 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..a0304d054e --- /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 removes 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 a throwaway repository now passes `test_support.git_repositories.NO_BACKGROUND_MAINTENANCE`. `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/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..d458f8b0de 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,7 @@ from hypothesis import HealthCheck, given, settings from hypothesis import strategies as st +from test_support.git_repositories import NO_BACKGROUND_MAINTENANCE from test_support.microcosm_build.us_modal_stage_plan_tool import ( COMMIT, FEED_SHA, @@ -704,7 +705,7 @@ def test_receipt_prices_earlier_preempted_attempts() -> None: def _git(cwd: Path, *args: str) -> str: return subprocess.run( - ["git", *args], + ["git", *NO_BACKGROUND_MAINTENANCE, *args], cwd=cwd, env=_GIT_ENV, check=True, 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..ac4def6aa3 100644 --- a/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py +++ b/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py @@ -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,16 @@ 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. A git " + "process still working in a checkout does 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 +1216,10 @@ 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, and the reference is refused if the lock is removed + while the tree is hashed. 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 +1250,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..0c517c7fcd 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,51 @@ 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().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..52094eb3b4 --- /dev/null +++ b/test_support/git_repositories.py @@ -0,0 +1,28 @@ +"""Git options for the throwaway repositories tests create.""" + +from __future__ import annotations + +#: ``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. ``gc.autoDetach=false`` +#: (git's fallback for ``maintenance.autoDetach``) keeps a maintenance run that +#: does start 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", + "gc.autoDetach=false", +) 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", From 219e0b42f512f9c78630758bbcb05d603313fff6 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 13 Oct 2026 14:32:29 -0400 Subject: [PATCH 2/3] Cover the branch check's fetch and tighten the wording (review) Both independent reviews approved; this takes their non-blocking points. - The modal stage plan test runs _verify_branch, whose own git fetch started background maintenance in its scratch clone. Git run by the code under test reads os.environ, so test_support.git_repositories now also gives the settings as GIT_CONFIG_* variables (NO_BACKGROUND_MAINTENANCE_ENV), and that test sets them. New tests: the branch check starts no maintenance (trace2), and the variables set exactly what the -c options set. - Use maintenance.autoDetach=false, not its fallback gc.autoDetach. - The digest's refusal says what to do for any root, then for a git checkout; the docstrings name the refusal where compiles re-hash. - docs/agent-guide.md: tests that commit or fetch in a throwaway repository pass these settings. Co-Authored-By: Claude Opus 5.5 --- .../git-fixture-maintenance-lock.fixed.md | 2 +- docs/agent-guide.md | 9 +++ .../us/test_us_modal_stage_plan_tool.py | 58 ++++++++++++++++++- .../src/microcosm/frame/adapters/axiom.py | 19 +++--- .../shared/test_axiom_graph_adapter.py | 4 +- test_support/git_repositories.py | 29 ++++++++-- 6 files changed, 103 insertions(+), 18 deletions(-) diff --git a/changelog.d/git-fixture-maintenance-lock.fixed.md b/changelog.d/git-fixture-maintenance-lock.fixed.md index a0304d054e..bdad6c97e4 100644 --- a/changelog.d/git-fixture-maintenance-lock.fixed.md +++ b/changelog.d/git-fixture-maintenance-lock.fixed.md @@ -1 +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 removes 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 a throwaway repository now passes `test_support.git_repositories.NO_BACKGROUND_MAINTENANCE`. `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`. +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 a throwaway repository now passes `test_support.git_repositories.NO_BACKGROUND_MAINTENANCE`. `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..4ea8c55924 100644 --- a/docs/agent-guide.md +++ b/docs/agent-guide.md @@ -152,6 +152,15 @@ 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 a throwaway git repository passes +`test_support.git_repositories.NO_BACKGROUND_MAINTENANCE` to that `git commit` +(`["git", *NO_BACKGROUND_MAINTENANCE, ...]`; a shared helper may pass it to +every command). When the code under test runs the git (a fetch, say), the test +sets `NO_BACKGROUND_MAINTENANCE_ENV` in the environment instead. Without them, +`git commit` and `git fetch` can leave a detached maintenance process that +removes `.git/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_modal_stage_plan_tool.py b/packages/microcosm-build/tests/engine_free/us/test_us_modal_stage_plan_tool.py index d458f8b0de..447fbb6741 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,7 +26,10 @@ from hypothesis import HealthCheck, given, settings from hypothesis import strategies as st -from test_support.git_repositories import NO_BACKGROUND_MAINTENANCE +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, @@ -691,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", "/"), @@ -700,12 +705,13 @@ 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, } def _git(cwd: Path, *args: str) -> str: return subprocess.run( - ["git", *NO_BACKGROUND_MAINTENANCE, *args], + ["git", *args], cwd=cwd, env=_GIT_ENV, check=True, @@ -765,6 +771,54 @@ 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: + 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-frame/src/microcosm/frame/adapters/axiom.py b/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py index ac4def6aa3..234c73962c 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: @@ -1147,10 +1147,11 @@ def rulespec_tree_digest(root: str | Path) -> tuple[str, int]: 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. A git " - "process still working in a checkout does this (its background " - "maintenance holds a lock file under .git); wait for it to " - "finish, or reference a git archive export instead." + 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 does 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) @@ -1218,8 +1219,10 @@ def axiom_engine_ref( commit all give different references, and any later git operation in the 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, and the reference is refused if the lock is removed - while the tree is hashed. Prefer the export. + 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 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 0c517c7fcd..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 @@ -1648,7 +1648,9 @@ def test_a_commit_in_a_test_repository_starts_no_git_maintenance( 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().splitlines()] + events = [ + json.loads(line) for line in trace.read_text(encoding="utf-8").splitlines() + ] started = [ event["argv"] for event in events diff --git a/test_support/git_repositories.py b/test_support/git_repositories.py index 52094eb3b4..3ca490837c 100644 --- a/test_support/git_repositories.py +++ b/test_support/git_repositories.py @@ -2,6 +2,9 @@ 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``. @@ -11,11 +14,11 @@ #: 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. ``gc.autoDetach=false`` -#: (git's fallback for ``maintenance.autoDetach``) keeps a maintenance run that -#: does start 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``. +#: ``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, ...] = ( @@ -24,5 +27,19 @@ "-c", "gc.auto=0", "-c", - "gc.autoDetach=false", + "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 replace any ``GIT_CONFIG_*`` entries already set. +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) + }, + } ) From 3c4182f960a8089c29d659bd161eba319844cca4 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Tue, 13 Oct 2026 14:45:51 -0400 Subject: [PATCH 3/3] Say exactly what the maintenance settings cover (review) Wording from the second review, and one skip guard; no behaviour change. - Changelog and guide: tests that commit or fetch apply the settings as -c options or, where the code under test runs git, as variables; a helper whose environment carries the variables needs no options; code that strips git's local variables drops them. - NO_BACKGROUND_MAINTENANCE_ENV overrides GIT_CONFIG_COUNT entries, not every GIT_CONFIG_* variable. - The digest's refusal says a git process "can" remove the file. - The variables-versus-options test skips without git, like the rest of its section. Co-Authored-By: Claude Opus 5.5 --- .../git-fixture-maintenance-lock.fixed.md | 2 +- docs/agent-guide.md | 17 ++++++++++------- .../us/test_us_modal_stage_plan_tool.py | 2 ++ .../src/microcosm/frame/adapters/axiom.py | 2 +- test_support/git_repositories.py | 5 ++++- 5 files changed, 18 insertions(+), 10 deletions(-) diff --git a/changelog.d/git-fixture-maintenance-lock.fixed.md b/changelog.d/git-fixture-maintenance-lock.fixed.md index bdad6c97e4..4e562c519c 100644 --- a/changelog.d/git-fixture-maintenance-lock.fixed.md +++ b/changelog.d/git-fixture-maintenance-lock.fixed.md @@ -1 +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 a throwaway repository now passes `test_support.git_repositories.NO_BACKGROUND_MAINTENANCE`. `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`. +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 4ea8c55924..2c52453d05 100644 --- a/docs/agent-guide.md +++ b/docs/agent-guide.md @@ -152,13 +152,16 @@ 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 a throwaway git repository passes -`test_support.git_repositories.NO_BACKGROUND_MAINTENANCE` to that `git commit` -(`["git", *NO_BACKGROUND_MAINTENANCE, ...]`; a shared helper may pass it to -every command). When the code under test runs the git (a fetch, say), the test -sets `NO_BACKGROUND_MAINTENANCE_ENV` in the environment instead. Without them, -`git commit` and `git fetch` can leave a detached maintenance process that -removes `.git/objects/maintenance.lock` after the command has returned, and a +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 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 447fbb6741..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 @@ -797,6 +797,8 @@ def test_the_branch_check_starts_no_git_maintenance( 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() diff --git a/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py b/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py index 234c73962c..a5c3f5d9d0 100644 --- a/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py +++ b/packages/microcosm-frame/src/microcosm/frame/adapters/axiom.py @@ -1149,7 +1149,7 @@ def rulespec_tree_digest(root: str | Path) -> tuple[str, int]: 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 does this (its " + "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 diff --git a/test_support/git_repositories.py b/test_support/git_repositories.py index 3ca490837c..e462c1a428 100644 --- a/test_support/git_repositories.py +++ b/test_support/git_repositories.py @@ -32,7 +32,10 @@ #: 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 replace any ``GIT_CONFIG_*`` entries already set. +#: 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),