Skip to content

Fix SSHRemoteJobOperator custom remote_base_dir cleanup validation#69834

Open
axelray-dev wants to merge 4 commits into
apache:mainfrom
axelray-dev:fix/ssh-remote-base-dir-validation-69813
Open

Fix SSHRemoteJobOperator custom remote_base_dir cleanup validation#69834
axelray-dev wants to merge 4 commits into
apache:mainfrom
axelray-dev:fix/ssh-remote-base-dir-validation-69813

Conversation

@axelray-dev

@axelray-dev axelray-dev commented Jul 13, 2026

Copy link
Copy Markdown

Summary

Fixes inconsistent validation of custom remote_base_dir on SSHRemoteJobOperator cleanup (#69813).

The operator accepts a custom base directory when constructing job paths, but cleanup validation only allowed the hardcoded defaults (/tmp/airflow-ssh-jobs and the Windows temp default). Jobs with a custom base then failed at the end with ValueError: Invalid job directory ... Expected path under '/tmp/airflow-ssh-jobs'.

Changes

  • Thread an optional expected base into _validate_job_dir and the cleanup command builders, defaulting to the existing platform defaults.
  • Pass the operator-configured base (RemoteJobPaths.base_dir / remote_base_dir) into cleanup so custom bases validate the same way as defaults.
  • Keep prefix-plus-separator safety checks so cleanup still rejects paths outside the configured base.
  • Add unit tests for custom-base cleanup success and rejection on both POSIX and Windows.

Verification

  • python -m pytest providers/ssh/tests/unit/ssh/utils/test_remote_job.py -q
  • 31 passed

Backwards compatibility

  • Default behavior is unchanged when remote_base_dir is unset.
  • Custom bases that already pass init validation now also pass cleanup validation.

Fixes #69813

@boring-cyborg

boring-cyborg Bot commented Jul 13, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@axelray-dev
axelray-dev marked this pull request as ready for review July 13, 2026 19:00
@achimgaedke

Copy link
Copy Markdown

Suggestion: Remove the _validate_job_dir from the build_posix_cleanup_command - which achieves the same as the PR, but is less code.

Comment thread providers/ssh/src/airflow/providers/ssh/operators/ssh_remote_job.py
Comment thread providers/ssh/src/airflow/providers/ssh/operators/ssh_remote_job.py Outdated
Comment thread providers/ssh/src/airflow/providers/ssh/utils/remote_job.py
Comment thread providers/ssh/src/airflow/providers/ssh/utils/remote_job.py Outdated
@axelray-dev

Copy link
Copy Markdown
Author

Thanks for the detailed notes.

I agree we should keep the cleanup validation. Once job_dir comes back from the trigger event, that prefix check is basically the only guard before rm -rf.

I'll update this PR to:

  • use expected_base is None instead of truthiness
  • catch cleanup validation errors and warn/leave instead of failing the task
  • add an operator-level custom remote_base_dir cleanup test so the execute_complete path is covered

@potiuk
potiuk marked this pull request as draft July 20, 2026 10:15
@potiuk

potiuk commented Jul 20, 2026

Copy link
Copy Markdown
Member

@axelray-dev This PR has been converted to draft because it does not yet meet our Pull Request quality criteria.

Issues found:

  • Pre-commit / static checks: Failing: CI image checks / Static checks. Run prek run --from-ref main --stage pre-commit locally to reproduce and fix.
  • Provider tests: Failing: Provider distributions tests / Compat 2.11.1:P3.10:, Provider distributions tests / Compat 3.0.6:P3.10:, Provider distributions tests / Compat 3.1.8:P3.10:, Provider distributions tests / Compat 3.2.2:P3.10:, Provider distributions tests / Compat 3.3.0:P3.10:. Run breeze testing providers-tests --test-type "Providers[<provider>]" locally.
  • Unresolved review comments: There are 4 unresolved review thread(s) on this PR from @kaxil. For each one: either apply the suggestion in a follow-up commit, or reply in-thread with a brief explanation of why the feedback doesn't apply, or resolve the thread if the feedback is no longer relevant. Once you believe a thread is resolved — whether by pushing a fix or by explaining why the suggestion doesn't apply — it is the author's responsibility to mark it as resolved by clicking the 'Resolve conversation' button at the bottom of the thread. Reviewers don't auto-close their own threads, so unresolved threads read as 'still waiting on the author' and block the PR from moving forward.

Note: Your branch is 126 commits behind main. Some check failures may be caused by changes in the base branch rather than by your PR. Please rebase your branch and push again to get up-to-date CI results.

What to do next:

  • Fix each issue listed above.
  • Make sure static checks pass locally (prek run --from-ref main --stage pre-commit).
  • Mark the PR as "Ready for review" when you're done.

Converting a PR to draft is not a rejection — it is an invitation to bring the PR up to the project's standards so that maintainer review time is spent productively. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the Airflow Slack.


Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

@axelray-dev
axelray-dev force-pushed the fix/ssh-remote-base-dir-validation-69813 branch from 78b532e to b63ef25 Compare July 21, 2026 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SSHRemoteJobOperator: inconsistent "remote_base_dir" validation

5 participants