Skip to content

fix(ssh): validate job dir cleanup against configured remote_base_dir - #70802

Closed
mmaxjr wants to merge 1 commit into
apache:mainfrom
mmaxjr:fix/ssh-remote-job-custom-base-dir-cleanup
Closed

fix(ssh): validate job dir cleanup against configured remote_base_dir#70802
mmaxjr wants to merge 1 commit into
apache:mainfrom
mmaxjr:fix/ssh-remote-job-custom-base-dir-cleanup

Conversation

@mmaxjr

@mmaxjr mmaxjr commented Jul 31, 2026

Copy link
Copy Markdown

Fixes #69813

Problem

SSHRemoteJobOperator lets users set a custom remote_base_dir, and it's
correctly validated for safety (_validate_base_dir) and used to build
job_dir via RemoteJobPaths. Execution and monitoring both work fine
with it.

But _cleanup_remote_job calls build_posix_cleanup_command/
build_windows_cleanup_command, which always validated job_dir against
the hardcoded default base directory (_validate_job_dir), not the one
actually configured. So cleanup for any job using a custom
remote_base_dir always raised:

SSHRemoteJobOperator._validate_base_dir() lets a user set a custom
remote_base_dir, and RemoteJobPaths correctly builds job_dir under it.
But at cleanup time, _validate_job_dir() always checked job_dir against
the hardcoded default base directory instead of the one actually
configured, so cleanup raised "Invalid job directory ... Expected path
under '/tmp/airflow-ssh-jobs'" for any job that used a custom
remote_base_dir - execution and monitoring worked fine, only cleanup
broke.

_validate_job_dir(), build_posix_cleanup_command(), and
build_windows_cleanup_command() now take an optional base_dir, and the
operator passes its own self.remote_base_dir through at cleanup time.
Defaults to the previous hardcoded behavior when base_dir is omitted,
so other callers are unaffected.

Fixes apache#69813

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@boring-cyborg

boring-cyborg Bot commented Jul 31, 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

@potiuk

potiuk commented Aug 1, 2026

Copy link
Copy Markdown
Member

Thanks — this is a close call, and sorry to close it. Three PRs independently fixed this bug; I am going with #69885, open since 14 July.

Yours is good work and better in two respects: the docstring actually explains why a caller with a custom base dir must pass it, and you linked the originating issue #69813. #69885 has the edge on coverage though — it pins the rejection case on Windows as well as POSIX, whereas this covers the negative path for POSIX only. On a validation guard the refusals are the part most worth locking down.

Also worth knowing: #70091 has since rewritten these same command builders on main to use shlex.quote, so all three branches conflicted. I resolved that on #69885 rather than sending it back.

I would rather not lose your docstring — if you would like to carry it and the issue link over as a follow-up once #69885 lands, that would genuinely improve the result.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

@potiuk potiuk closed this Aug 1, 2026
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

2 participants