[I] GitDagBundle: an interrupted clone leaves a version directory that can never check out and is never repaired [airflow]

["8silvergun (via GitHub)" <gi...@apache.org>]

8silvergun opened a new issue, #72759:
URL: https://github.com/apache/airflow/issues/72759

   ### Apache Airflow Provider(s)
   
   git
   
   ### Versions of Apache Airflow Providers
   
   apache-airflow-providers-git 0.4.3 (also present in 0.3.1 and on main)
   
   ### Apache Airflow version
   
   3.2.1 (code path unchanged on main)
   
   ### Operating System
   
   Linux (containerized)
   
   ### Deployment
   
   Official Apache Airflow Helm Chart
   
   ### Deployment details
   
   `[dag_processor] dag_bundle_storage_path` points at a volume shared by the Dag processor and all worker pods.
   
   ### What happened
   
   If a process is killed while `GitDagBundle` is cloning a bundle version, it leaves behind a directory that GitPython can open as a repository but that cannot resolve the configured branch or tag `tracking_ref`, and subsequent initialization attempts do not repair or replace it. Every later task pinned to that bundle version then fails during parsing with:
   
   ```
   GitCommandError: Cmd('git') failed due to: exit code(1)
     cmdline: git checkout <tracking_ref>
     stderr: 'error: pathspec '<tracking_ref>' did not match any file(s) known to git'
   ```
   
   The leftover directory looks like this:
   
   - `versions/<sha>/.git/` exists, with `HEAD`, `config`, `description`, `branches/`, `hooks/`, `info/` and a partially populated `objects/`
   - `.git/refs/` contains no files and there is no `.git/packed-refs`
   - `.git/HEAD` still contains the initial value `git clone` writes (`ref: refs/heads/<init.defaultBranch>`) rather than the cloned branch
   - the working tree is empty (no Dag files)
   
   `git clone` writes the `.git` skeleton and objects before it writes refs and checks out the working tree, so a process killed in that window leaves exactly this state. The bare mirror and the remote are both healthy - only the per-version clone is truncated.
   
   The failure is not self-healing:
   
   1. `_is_pruned_worktree()` treats "no `.git`" as a completed version, so a leftover `.git` makes it return `False`.
   2. `_local_repo_has_version()` returns `False` for the truncated repo, so it does not take that shortcut either.
   3. `_clone_repo_if_required()` only clones when `repo_path` does not exist. The path does exist, so it skips cloning and reopens the truncated repository.
   4. `self.repo.git.checkout(self.tracking_ref)` then fails. It sits outside the `@retry` / `shutil.rmtree` block inside `_clone_repo_if_required()`, so nothing cleans the directory up and nothing retries.
   
   Because the version directory is never repaired or replaced, tasks pinned to that bundle version continue to fail until the version directory is repaired or removed. A later bundle version may move newly scheduled tasks away from the poisoned version, but it does not fix the version itself. With shared bundle storage the blast radius is every worker, not one host.
   
   One aggravating factor: the failure happens in `task_runner.parse()`, before the Dag is parsed, so task-level `on_failure_callback` / `on_retry_callback` never run. The failures are silent as far as Dag-level alerting is concerned.
   
   ### This may already be fixed by #71535
   
   #71535 (open, closes #71388) adds a fetch before the checkout:
   
   ```python
   if not self._has_version(self.repo, self.tracking_ref):
       self.repo.remotes.origin.fetch()
   self.repo.git.checkout(self.tracking_ref)
   ```
   
   That looks like it would recover the state described here as well, for a different reason than it was written for:
   
   - `_has_version(repo, tracking_ref)` cannot resolve `tracking_ref` in a repository with no refs, so it returns `False` and the fetch runs.
   - `git clone` writes the remote configuration before it transfers objects, so an interruption during the object transfer can leave `remote.origin.url` configured. On such a leftover this can be confirmed with:
   
     ```
     git -C versions/<sha> config --get remote.origin.url
     ```
   
     which points at the bundle's healthy local bare mirror.
   - `origin` here is the bundle's local bare mirror, so the fetch is local and cheap, and it repopulates `refs/remotes/origin/*`, after which the checkout resolves.
   
   If that is right, this issue is a second, unrelated trigger that #71535 happens to cover, and it would be worth an explicit regression test so the recovery is not lost in a later refactor. I would be glad to contribute that test to #71535 rather than open a competing fix.
   
   One residual case #71535 would not cover: if the process is killed in the much narrower window before `git clone` writes the remote configuration, the leftover has no `origin` and `self.repo.remotes.origin` raises `AttributeError`. Whether that is worth handling is a separate question, and it is not what was observed here.
   
   ### What you think should happen instead
   
   An interrupted clone should not permanently poison a bundle version. `GitDagBundle` should recover when an existing per-version repository cannot resolve the configured tracking ref, either by restoring the missing refs from its healthy local bare origin or, if that is not possible, by discarding and recreating the clone.
   
   ### How to reproduce
   
   1. Configure a `GitDagBundle` with `[dag_processor] dag_bundle_storage_path` on storage that survives the process.
   2. Start a task (or any bundle initialization) that has to clone a new version, and kill the process with `SIGKILL` a few seconds into the clone, while `objects/` is being populated and before the working tree appears.
   3. Confirm the leftover state: `ls versions/<sha>` is empty, `git -C versions/<sha> show-ref` prints nothing, `cat versions/<sha>/.git/HEAD` still shows the `init.defaultBranch` value rather than the tracked branch.
   4. Run any task pinned to that bundle version. It fails with the `pathspec ... did not match any file(s) known to git` error above, and keeps failing on every retry and every other task on that version.
   
   Removing `versions/<sha>` by hand restores normal operation, because `_clone_repo_if_required()` then performs a real clone.
   
   ### Anything else
   
   Preferred outcome, in order:
   
   1. If #71535 does cover this, add an interrupted-clone regression test there: initialize a versioned bundle with a named branch such as `main` as `tracking_ref`, remove all loose and packed refs from `versions/<sha>/.git`, leave HEAD pointing at a nonexistent branch, empty the working tree, verify that `origin` remains configured, then initialize the same version again and assert that both the expected version and the Dag files are restored. That test fails on `main` today and should pass with #71535. Happy to contribute it to that PR.
   2. If #71535 turns out not to cover it, the narrow alternative is to remove `repo_path` before the existence check in `_clone_repo_if_required()` when it contains a `.git` with no references, so the normal clone path runs. A repository with no refs cannot resolve a branch or tag `tracking_ref`, so the normal initialization checkout cannot succeed until the refs are restored or the clone is recreated.
   
   Moving `self.repo.git.checkout(self.tracking_ref)` inside the `@retry` / `rmtree` block of `_clone_repo_if_required()` would make any failing checkout discard the clone and retry once, which is broader and would cover more of these cases at once, but it also changes behaviour for failures that are not clearly recoverable.
   
   ### Are you willing to submit PR?
   
   - [x] Yes I am willing to submit a PR!
   
   ### Code of Conduct
   
   - [x] I agree to follow this project's Code of Conduct
   


-- 
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: commits-unsubscribe@airflow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] GitDagBundle: an interrupted clone leaves a version directory that can never check out and is never repaired [airflow]

["8silvergun (via GitHub)" <gi...@apache.org>]

8silvergun commented on issue #72759:
URL: https://github.com/apache/airflow/issues/72759#issuecomment-5863865306

   Thanks for the additional production case and for splitting prevention from recovery.
   
   This makes the scope clearer to me:
   
   - #72759 originally captured one concrete interrupted-clone state: a repository with no usable refs that is permanently reused.
   - #72922 demonstrates that the same underlying failure can leave a different corrupted working-tree state, so fetch-before-checkout alone is not sufficient for the whole failure class.
   - #72923 prevents new half-written working repositories from becoming visible at `repo_path`.
   - A recovery path such as #72787 is still useful for repositories already poisoned by an older provider version or another checkout failure.
   
   So I no longer think #71535 alone should be considered sufficient coverage for #72759.
   
   From my side, the original production failure and non-self-healing mechanism in #72759 are still accurately represented. I’m happy for the maintainers to decide whether #72759 should eventually be closed by the prevention change, a recovery change, or the combination of both.


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] GitDagBundle: an interrupted clone leaves a version directory that can never check out and is never repaired [airflow]

["hkc-8010 (via GitHub)" <gi...@apache.org>]

hkc-8010 commented on issue #72759:
URL: https://github.com/apache/airflow/issues/72759#issuecomment-5630507280

   We hit the same root cause in production with a different leftover state, which matters for your
   "does #71535 cover this" question.
   
   Airflow 3.3.0, providers-git 0.4.x, long-lived worker pods with pod-local bundle storage. SIGTERM
   landed 315 ms into the working clone, and `git clone` had got far enough to write files into the
   working tree but not far enough to record them in the index. So instead of
   `pathspec ... did not match any file(s) known to git`, the checkout aborted on untracked files:
   
   ```
   git checkout master
   error: The following untracked working tree files would be overwritten by checkout:
           .agents/instructions/airflow-patterns.md
           <...>
   Aborting
   ```
   
   Every one of the 30 tasks that landed on that pod afterwards failed at
   `task_runner.py parse` -> `git.py:215`, and it stayed broken until the worker was replaced. Your
   four reasons it does not self-heal apply unchanged.
   
   The relevant part: #71535 would not recover this one. `_has_version(repo, tracking_ref)` fails and
   the fetch runs, the refs come back from the local bare mirror, and then the checkout still aborts
   because the untracked files are still sitting in the working tree. #72787's broader move of the
   checkout into the retry and `rmtree` block does recover it. That seems like a concrete argument
   for the broader fix rather than the narrow fetch, at least for this failure family.
   
   Your point about the residual case, where the clone dies before the remote configuration is
   written and `self.repo.remotes.origin` raises `AttributeError`, is the one that convinced me the
   recovery path is not the whole answer. I have written that up in #72922 with a PR that stages the
   clone in a sibling directory and renames it into place, so nothing lands at `repo_path` until the
   clone is finished. That removes the residual case too, and composes with #72787 rather than
   competing with it.
   


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org