potiuk commented on code in PR #74282: URL: https://github.com/apache/airflow/pull/74282#discussion_r4291297662
########## dev/breeze/src/airflow_breeze/utils/worktree_watcher.py: ########## @@ -0,0 +1,148 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Detached watcher that removes the Docker resources of a deleted Breeze worktree. + +Breeze starts it as a separate process with ``python -I <this file>``. The watcher must keep working +after the worktree is deleted, and that deletion can take Breeze's own virtualenv with it, so this +module only uses the standard library, imports everything up front and never imports +``airflow_breeze``. +""" + +from __future__ import annotations + +import argparse +import contextlib +import fcntl +import subprocess +import time +from pathlib import Path +from typing import IO + +OWNER_LABEL = "org.apache.airflow.breeze=true" +WORKTREE_LABEL = "org.apache.airflow.breeze.worktree" +HOST_LABEL = "org.apache.airflow.breeze.host" +DOCKER_TIMEOUT_SECONDS = 120 + + +def try_lock(lock_file: Path) -> IO[str] | None: + """Take the watcher lock, or return ``None`` when another process holds it.""" + lock_file.parent.mkdir(parents=True, exist_ok=True) + handle = lock_file.open("a") + try: + fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BlockingIOError: + handle.close() + return None + return handle + + +def is_watcher_running(lock_file: Path) -> bool: + handle = try_lock(lock_file) + if handle is None: + return True + handle.close() + return False + + +def is_worktree_missing(worktree: Path) -> bool: Review Comment: Good catch, thanks for testing it for real. Done in [144edf1](https://github.com/apache/airflow/commit/144edf1dfd). `is_worktree_missing` now follows the `.git` → `gitdir` link, as you suggested, so it catches leftover bind-mount folders and a failed `git worktree remove`. Errors other than "not found" are passed up to the caller, so the resources are kept when the check itself fails. There are tests for each case: intact, worktree deleted, folders left behind, `gitdir` gone, and a relative `gitdir`. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting ########## dev/breeze/src/airflow_breeze/utils/docker_command_utils.py: ########## @@ -827,12 +867,21 @@ def remove_stale_worktree_containers() -> None: console_print("[error]Unable to discover containers belonging to deleted worktrees.[/]") return for line in result.stdout.splitlines(): - container_id, _, worktree = line.partition("\t") - if _worktree_is_missing(worktree): + container_id, worktree, host = (line.split("\t") + ["", ""])[:3] + # `docker ps` prints "<no value>" for containers created before the host label existed. + if _belongs_to_deleted_worktree(worktree, "" if host == "<no value>" else host): console_print(f"Removing container {container_id} for deleted worktree {worktree}") run_command(["docker", "rm", "--force", "--volumes", container_id], check=False) +def _belongs_to_deleted_worktree(worktree: str, host: str) -> bool: + # Another host's paths cannot be checked here. Resources created before the host label existed + # have no host and keep the local-path check. + if host and host != get_host_id(): + return False + return _worktree_is_missing(worktree) Review Comment: Agreed. In [144edf1](https://github.com/apache/airflow/commit/144edf1dfd), `_worktree_is_missing` now calls `worktree_watcher.is_worktree_missing`. It keeps only the relative-path guard and the warning when the worktree can't be checked, so the watcher and the sweep can no longer drift apart. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting ########## dev/breeze/src/airflow_breeze/utils/path_utils.py: ########## @@ -640,11 +641,49 @@ def cleanup_python_generated_files(): console_print("[info]Cleaned") +WORKTREE_ISOLATION_ENV = "BREEZE_WORKTREE_ISOLATION" +SUPPRESS_WORKTREE_ISOLATION_FILE = "suppress_worktree_isolation" + + +def get_shared_build_cache_path() -> Path: + """Return the ``.build`` directory of the main checkout, shared by all of its worktrees.""" + main_git_dir = get_main_git_dir_for_worktree() + return main_git_dir.parent / ".build" if main_git_dir else BUILD_CACHE_PATH + + +def is_worktree_isolation_enabled() -> bool: + value = os.environ.get(WORKTREE_ISOLATION_ENV, "").strip().lower() + if value in ("true", "1", "yes", "on"): + return True + if value in ("false", "0", "no", "off"): + return False + return not (get_shared_build_cache_path() / SUPPRESS_WORKTREE_ISOLATION_FILE).exists() + + +def get_isolated_worktree_path() -> Path | None: + """ + Return the path of the linked worktree that owns this checkout's Docker resources. + + ``None`` means the resources are shared: the main checkout, or a worktree with isolation disabled. + """ + if get_main_git_dir_for_worktree() is None or not is_worktree_isolation_enabled(): + return None + return AIRFLOW_ROOT_PATH.resolve() + + def get_default_project_name() -> str: - if get_main_git_dir_for_worktree() is None: + worktree = get_isolated_worktree_path() + if worktree is None: return "breeze" - name = re.sub(r"[^a-z0-9_-]", "-", AIRFLOW_ROOT_PATH.resolve().name.lower()) - return f"breeze-{name}" + name = re.sub(r"[^a-z0-9_-]", "-", worktree.name.lower()) + # Worktrees with the same directory name in different locations or clones must not share a project. + path_hash = hashlib.sha256(str(worktree).encode()).hexdigest()[:6] + return f"breeze-{name}-{path_hash}" + + +def get_host_id() -> str: + """Identify the machine whose filesystem decides whether a labelled worktree still exists.""" + return socket.gethostname() Review Comment: Yes, that works better. Done in [144edf1](https://github.com/apache/airflow/commit/144edf1dfd). `get_host_id()` now creates a random id once and stores it in `~/.config/breeze/host-id`. The file is created with a link that fails if it already exists, so two Breeze runs starting at once still end up with the same id. If the home directory isn't writable (for example in a sandbox), it falls back to the host name. [05ea9a8](https://github.com/apache/airflow/commit/05ea9a8cbe) also treats resources labelled with the current host name as local. That covers resources created before this change or with the fallback, so they still get swept. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
