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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
