[I] SSHRemoteJobOperator: the default Windows remote_base_dir is never expanded, so the job directory cannot be created [airflow]

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

xFrone opened a new issue, #70438:
URL: https://github.com/apache/airflow/issues/70438

   ### Under which category would you file this issue?
   
   Providers
   
   ### Apache Airflow version
   
   main (`cd7235aff3`), `apache-airflow-providers-ssh` 6.0.0
   
   ### What happened and how to reproduce it?
   
   `SSHRemoteJobOperator` cannot run against a Windows remote when `remote_base_dir` is
   left at its default, because every generated PowerShell command embeds the default
   base directory in a **single-quoted** string, where PowerShell does not expand
   `$env:TEMP`.
   
   `WINDOWS_DEFAULT_BASE_DIR` is defined as a PowerShell expression:
   
   https://github.com/apache/airflow/blob/main/providers/ssh/src/airflow/providers/ssh/utils/remote_job.py#L30
   
   ```python
   WINDOWS_DEFAULT_BASE_DIR = "$env:TEMP\\airflow-ssh-jobs"
   ```
   
   but `build_windows_wrapper_command` interpolates the resulting path into a
   single-quoted assignment:
   
   https://github.com/apache/airflow/blob/main/providers/ssh/src/airflow/providers/ssh/utils/remote_job.py#L289-L290
   
   ```python
       wrapper = f"""$jobDir = '{job_dir}'
   New-Item -ItemType Directory -Force -Path $jobDir | Out-Null
   ```
   
   Single quotes are literal in PowerShell, so `$jobDir` holds the literal text
   `$env:TEMP\airflow-ssh-jobs\...`. PowerShell then parses the leading `$env:` as a
   PSDrive qualifier and fails.
   
   **Reproduce without a remote host** — decode what the provider generates:
   
   ```python
   from airflow.providers.ssh.utils.remote_job import RemoteJobPaths, build_windows_wrapper_command
   import base64
   
   paths = RemoteJobPaths(job_id="af_dag_task_run_try1_abcdef", remote_os="windows")
   cmd = build_windows_wrapper_command(command="echo hello", paths=paths)
   print(base64.b64decode(cmd.split("-EncodedCommand ")[1]).decode("utf-16-le"))
   ```
   
   Output:
   
   ```powershell
   $jobDir = '$env:TEMP\airflow-ssh-jobs\af_dag_task_run_try1_abcdef'
   New-Item -ItemType Directory -Force -Path $jobDir | Out-Null
   $log = '$env:TEMP\airflow-ssh-jobs\af_dag_task_run_try1_abcdef\stdout.log'
   '' | Set-Content -Path $log
   ...
   ```
   
   Running those two lines in Windows PowerShell 5.1 (Windows 11):
   
   ```
   New-Item : Cannot find drive. A drive with the name '$env' does not exist.
       + CategoryInfo          : ObjectNotFound: ($env:String) [New-Item], DriveNotFoundException
       + FullyQualifiedErrorId : DriveNotFound,Microsoft.PowerShell.Commands.NewItemCommand
   ```
   
   Nothing is created — neither a literal `$env:TEMP` folder nor anything under the real
   `%TEMP%`. `Set-Content -Path $log` fails identically, so the job directory and log file
   never exist and the job cannot start.
   
   Cleanup is affected too, but silently: `build_windows_cleanup_command` produces
   
   ```powershell
   Remove-Item -Recurse -Force -Path '$env:TEMP\airflow-ssh-jobs\...' -ErrorAction SilentlyContinue
   ```
   
   which suppresses the same `DriveNotFoundException` and removes nothing.
   
   Setting `remote_base_dir` to a literal path (for example `C:\Temp\airflow-ssh-jobs`)
   avoids all of this, which is presumably why the default has not been hit before.
   
   ### What you think should happen instead?
   
   The default should resolve to the remote user's temporary directory.
   
   The paths are used as literal data in single-quoted PowerShell strings, so the
   expansion cannot happen at string-build time on the Airflow side — the value is only
   known on the remote host. Two directions:
   
   1. Resolve the temporary directory on the remote as part of the existing OS detection
      step (`build_windows_os_detection_command` already runs a probe) and use the
      returned literal path as the default base dir.
   2. Or emit the assignment double-quoted (`$jobDir = "..."`) so PowerShell expands it,
      and quote the remainder appropriately. This is more invasive, since every path is
      currently treated as literal data and a user-supplied `remote_base_dir` containing
      `$` would then be interpreted rather than used verbatim.
   
   Direction 1 looks safer to me because it keeps every emitted path literal and confines
   the change to how the default is obtained.
   
   ### Operating System
   
   Remote: Windows. Verified the PowerShell behaviour on Windows 11 with Windows PowerShell 5.1.
   
   ### Deployment
   
   Other
   
   ### Deployment details
   
   Not deployment specific — reproducible from the provider's command builders alone.
   
   ### Anything else?
   
   Introduced with the operator in #60297. Occurs on every run against a Windows remote
   that does not set `remote_base_dir`.
   
   I could not find an existing issue or open PR covering this. The open PRs touching
   `WINDOWS_DEFAULT_BASE_DIR` (#69834, #69885, #70098, #70233) all address #69813, which
   is a different problem — that the cleanup guard compares against the default base dir
   rather than the configured one.
   
   ### 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](https://github.com/apache/airflow/blob/main/CODE_OF_CONDUCT.md)
   


-- 
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] SSHRemoteJobOperator: the default Windows remote_base_dir is never expanded, so the job directory cannot be created [airflow]

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

xFrone commented on issue #70438:
URL: https://github.com/apache/airflow/issues/70438#issuecomment-5078889232

   After looking at this more closely I think there is a third option that is smaller and
   less invasive than either of the two I listed above, and I would like to check the
   direction before opening a PR.
   
   Keep `$env:TEMP\airflow-ssh-jobs` as an opaque sentinel on the Python side, exactly as
   today, and expand it only at the point where a path is rendered into PowerShell. A
   single helper covers every site:
   
   ```python
   def _powershell_path(path: str) -> str:
       """Render a remote path as a PowerShell expression."""
       if path == WINDOWS_TEMP_PREFIX or path.startswith(WINDOWS_TEMP_PREFIX + "\\"):
           relative = path[len(WINDOWS_TEMP_PREFIX) :].lstrip("\\")
           if not relative:
               return "$env:TEMP"
           return f"(Join-Path $env:TEMP '{relative.replace(chr(39), chr(39) * 2)}')"
       return f"'{path.replace(chr(39), chr(39) * 2)}'"
   ```
   
   so `$jobDir = '{job_dir}'` becomes `$jobDir = {_powershell_path(job_dir)}` and emits
   
   ```powershell
   $jobDir = (Join-Path $env:TEMP 'airflow-ssh-jobs\af_dag_task_run_try1_abcdef')
   ```
   
   Why I prefer this:
   
   - **Every path stays literal data.** A user-supplied `remote_base_dir` never goes
     through expansion, so a `$` in it is still used verbatim. Only the exact default
     prefix is treated as an expression.
   - **No change to `_validate_job_dir`.** Both the job dir and the default base dir keep
     the sentinel on the Python side, so the existing prefix comparison is untouched.
   - **No change to the operator, the trigger, or the event payload.** The resolved path
     never has to travel back to Airflow, which the other two directions would have
     required — cleanup runs from `execute_complete` after deferral, where `_paths` is
     `None`, so a remotely-resolved base dir would have had to be threaded through the
     trigger event.
   - It is confined to the six `build_windows_*` builders in `remote_job.py`.
   
   Verified on Windows PowerShell 5.1: `Join-Path $env:TEMP 'a\b'` resolves correctly,
   is valid as a `-Path` argument, and `''` escaping handles a quote inside the relative
   part.
   
   One consequence worth calling out: user-visible logs and the trigger event would keep
   showing `$env:TEMP\airflow-ssh-jobs\...` rather than the resolved path, since Airflow
   still never learns the real location. I think that is acceptable — it is the status quo
   and it keeps the guard's comparison meaningful — but if you would rather Airflow know
   the real path, then resolving it during OS detection (option 1 above) is the way, at
   the cost of extending the trigger event.
   
   Happy to go with whichever you prefer. If there is no objection I will send a PR for
   this third option.
   


-- 
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] SSHRemoteJobOperator: the default Windows remote_base_dir is never expanded, so the job directory cannot be created [airflow]

["boring-cyborg[bot] (via GitHub)" <gi...@apache.org>]

boring-cyborg[bot] commented on issue #70438:
URL: https://github.com/apache/airflow/issues/70438#issuecomment-5078869796

   Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.
   


-- 
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