kaxil commented on code in PR #73352:
URL: https://github.com/apache/airflow/pull/73352#discussion_r4050429341
##########
providers/ssh/src/airflow/providers/ssh/utils/remote_job.py:
##########
@@ -271,37 +271,43 @@ def build_windows_wrapper_command(
env_setup += f"$env:{key} = '{escaped_value}'; "
def ps_escape(s: str) -> str:
+ return s.replace("`", "``").replace('"', '`"')
+
+ def ps_escape_sq(s: str) -> str:
return s.replace("'", "''")
job_dir = ps_escape(paths.job_dir)
- log_file = ps_escape(paths.log_file)
- exit_code_file = ps_escape(paths.exit_code_file)
- exit_code_tmp = ps_escape(paths.exit_code_tmp_file)
- pid_file = ps_escape(paths.pid_file)
- status_file = ps_escape(paths.status_file)
+ sep = paths.sep
+ job_id = ps_escape_sq(paths.job_id)
escaped_command = ps_escape(command)
Review Comment:
The command goes into the `@'...'@` here-string on line 304, and that form
is verbatim, so PowerShell does no escape processing and these backticks reach
job.ps1 as literal characters. Once job.ps1 is parsed the escaped quotes no
longer quote anything, so what was inside them gets read as PowerShell. Running
the generated wrapper on a windows-latest runner, `python -c "print(1+1)"`
reaches Python as `print 2` and fails with a SyntaxError, and a `.ps1` target
receives `--msg "hello world"` as three arguments instead of two. With
`escaped_command = command` and nothing else changed, both of those pass.
Worth saying why a manual run can look clean: PowerShell re-quotes arguments
for native commands, so `script.py foo "hello world"` does survive. It is
quoted text holding PowerShell-significant characters, and any `.ps1` target,
that breaks.
##########
providers/ssh/src/airflow/providers/ssh/utils/remote_job.py:
##########
@@ -271,37 +271,43 @@ def build_windows_wrapper_command(
env_setup += f"$env:{key} = '{escaped_value}'; "
def ps_escape(s: str) -> str:
+ return s.replace("`", "``").replace('"', '`"')
+
+ def ps_escape_sq(s: str) -> str:
return s.replace("'", "''")
job_dir = ps_escape(paths.job_dir)
- log_file = ps_escape(paths.log_file)
- exit_code_file = ps_escape(paths.exit_code_file)
- exit_code_tmp = ps_escape(paths.exit_code_tmp_file)
- pid_file = ps_escape(paths.pid_file)
- status_file = ps_escape(paths.status_file)
+ sep = paths.sep
+ job_id = ps_escape_sq(paths.job_id)
escaped_command = ps_escape(command)
- job_id = ps_escape(paths.job_id)
- child_script = f"""$ErrorActionPreference = 'Continue'
-$env:LOG_FILE = '{log_file}'
-$env:STATUS_FILE = '{status_file}'
+ # Use $d and $b for directories to avoid passing long paths multiple times.
+ # Write command to disk to avoid double base64 encoding.
+ # Add-Content writes each pipeline object to disk immediately (no block
buffering),
+ # giving real-time log updates.
+ # Launch as Win32_Process so that it doesn't die when SSH exits.
+ child_script = f"""$ErrorActionPreference = "Continue"
+$b = $PSScriptRoot
+$env:LOG_FILE = "$b{sep}stdout.log"
+$env:STATUS_FILE = "$b{sep}status"
{env_setup}
-{escaped_command}
+{escaped_command} 2>&1 | Add-Content -Path $env:LOG_FILE
Review Comment:
`2>&1 | Add-Content` binds to the last statement only, and the job has no
console once it is launched through WMI, so output from anything earlier is
discarded. On the same runner, `Write-Output 'one'; Write-Output 'two'` logged
only `two`, and a two-line command logged only its second line. Wrapping the
command, `& { ... } 2>&1 | Add-Content -Path $env:LOG_FILE`, captured both, and
a command exiting 3 still wrote exit_code 3, so the exit-code capture survives
that change.
##########
providers/ssh/src/airflow/providers/ssh/utils/remote_job.py:
##########
@@ -271,37 +271,43 @@ def build_windows_wrapper_command(
env_setup += f"$env:{key} = '{escaped_value}'; "
def ps_escape(s: str) -> str:
+ return s.replace("`", "``").replace('"', '`"')
Review Comment:
`$` is not escaped, and the move to double quotes means it now interpolates.
The double quotes are what make the default base dir work (`$env:TEMP` stayed
literal in the old single-quoted form), but `remote_base_dir` is a template
field, so a path holding a `$` quietly relocates the job. With it set to
`C:\ptest\$batch` on the runner, the job dir was created at `C:\ptest\<job_id>`
while the operator and trigger kept looking under `C:\ptest\$batch\<job_id>`,
so no logs, no exit code, and cleanup aims at a path that was never created.
`_validate_base_dir` rejects only `..` and null bytes today. The same escape
pair repeats at lines 342, 379, 409, 462 and 498, so it probably wants to be
one shared helper.
##########
providers/ssh/src/airflow/providers/ssh/utils/remote_job.py:
##########
@@ -271,37 +271,43 @@ def build_windows_wrapper_command(
env_setup += f"$env:{key} = '{escaped_value}'; "
def ps_escape(s: str) -> str:
+ return s.replace("`", "``").replace('"', '`"')
+
+ def ps_escape_sq(s: str) -> str:
return s.replace("'", "''")
job_dir = ps_escape(paths.job_dir)
- log_file = ps_escape(paths.log_file)
- exit_code_file = ps_escape(paths.exit_code_file)
- exit_code_tmp = ps_escape(paths.exit_code_tmp_file)
- pid_file = ps_escape(paths.pid_file)
- status_file = ps_escape(paths.status_file)
+ sep = paths.sep
+ job_id = ps_escape_sq(paths.job_id)
escaped_command = ps_escape(command)
- job_id = ps_escape(paths.job_id)
- child_script = f"""$ErrorActionPreference = 'Continue'
-$env:LOG_FILE = '{log_file}'
-$env:STATUS_FILE = '{status_file}'
+ # Use $d and $b for directories to avoid passing long paths multiple times.
+ # Write command to disk to avoid double base64 encoding.
+ # Add-Content writes each pipeline object to disk immediately (no block
buffering),
+ # giving real-time log updates.
+ # Launch as Win32_Process so that it doesn't die when SSH exits.
+ child_script = f"""$ErrorActionPreference = "Continue"
+$b = $PSScriptRoot
+$env:LOG_FILE = "$b{sep}stdout.log"
+$env:STATUS_FILE = "$b{sep}status"
{env_setup}
-{escaped_command}
+{escaped_command} 2>&1 | Add-Content -Path $env:LOG_FILE
$ec = $LASTEXITCODE
if ($null -eq $ec) {{ $ec = 0 }}
-Set-Content -NoNewline -Path '{exit_code_tmp}' -Value $ec
-Move-Item -Force -Path '{exit_code_tmp}' -Destination '{exit_code_file}'
+Set-Content -NoNewline -Path "$b{sep}exit_code.tmp" -Value $ec
+Move-Item -Force -Path "$b{sep}exit_code.tmp" -Destination "$b{sep}exit_code"
"""
- child_script_bytes = child_script.encode("utf-16-le")
- encoded_script = base64.b64encode(child_script_bytes).decode("ascii")
- wrapper = f"""$jobDir = '{job_dir}'
-New-Item -ItemType Directory -Force -Path $jobDir | Out-Null
-$log = '{log_file}'
-'' | Set-Content -Path $log
+ wrapper = f"""$d = "{job_dir}"
+New-Item -ItemType Directory -Force -Path $d | Out-Null
+$sf = "$d{sep}job.ps1"
+@'
+{child_script}'@ | Set-Content -Path $sf -Encoding UTF8
+'' | Set-Content -Path "$d{sep}stdout.log"
Review Comment:
Separate from the redirect, the log does not round-trip non-ASCII. With the
other issues patched so the command actually ran, `python -c "print('héllo')"`
landed in stdout.log as `hTllo` on the Windows runner. `Add-Content` re-encodes
through PowerShell's output encoding while `build_windows_log_tail_command`
reads the bytes back as UTF-8, so non-ASCII output reaches the task log
corrupted. Worth pinning the encoding at both ends.
##########
providers/ssh/src/airflow/providers/ssh/utils/remote_job.py:
##########
@@ -271,37 +271,43 @@ def build_windows_wrapper_command(
env_setup += f"$env:{key} = '{escaped_value}'; "
def ps_escape(s: str) -> str:
+ return s.replace("`", "``").replace('"', '`"')
+
+ def ps_escape_sq(s: str) -> str:
return s.replace("'", "''")
job_dir = ps_escape(paths.job_dir)
- log_file = ps_escape(paths.log_file)
- exit_code_file = ps_escape(paths.exit_code_file)
- exit_code_tmp = ps_escape(paths.exit_code_tmp_file)
- pid_file = ps_escape(paths.pid_file)
- status_file = ps_escape(paths.status_file)
+ sep = paths.sep
+ job_id = ps_escape_sq(paths.job_id)
escaped_command = ps_escape(command)
- job_id = ps_escape(paths.job_id)
- child_script = f"""$ErrorActionPreference = 'Continue'
-$env:LOG_FILE = '{log_file}'
-$env:STATUS_FILE = '{status_file}'
+ # Use $d and $b for directories to avoid passing long paths multiple times.
+ # Write command to disk to avoid double base64 encoding.
+ # Add-Content writes each pipeline object to disk immediately (no block
buffering),
+ # giving real-time log updates.
+ # Launch as Win32_Process so that it doesn't die when SSH exits.
+ child_script = f"""$ErrorActionPreference = "Continue"
+$b = $PSScriptRoot
+$env:LOG_FILE = "$b{sep}stdout.log"
+$env:STATUS_FILE = "$b{sep}status"
{env_setup}
-{escaped_command}
+{escaped_command} 2>&1 | Add-Content -Path $env:LOG_FILE
$ec = $LASTEXITCODE
if ($null -eq $ec) {{ $ec = 0 }}
-Set-Content -NoNewline -Path '{exit_code_tmp}' -Value $ec
-Move-Item -Force -Path '{exit_code_tmp}' -Destination '{exit_code_file}'
+Set-Content -NoNewline -Path "$b{sep}exit_code.tmp" -Value $ec
+Move-Item -Force -Path "$b{sep}exit_code.tmp" -Destination "$b{sep}exit_code"
"""
- child_script_bytes = child_script.encode("utf-16-le")
- encoded_script = base64.b64encode(child_script_bytes).decode("ascii")
- wrapper = f"""$jobDir = '{job_dir}'
-New-Item -ItemType Directory -Force -Path $jobDir | Out-Null
-$log = '{log_file}'
-'' | Set-Content -Path $log
+ wrapper = f"""$d = "{job_dir}"
+New-Item -ItemType Directory -Force -Path $d | Out-Null
+$sf = "$d{sep}job.ps1"
+@'
+{child_script}'@ | Set-Content -Path $sf -Encoding UTF8
+'' | Set-Content -Path "$d{sep}stdout.log"
-$p = Start-Process -FilePath 'powershell.exe' -ArgumentList @('-NoProfile',
'-NonInteractive', '-EncodedCommand', '{encoded_script}')
-RedirectStandardOutput $log -RedirectStandardError $log -PassThru -WindowStyle
Hidden
-Set-Content -NoNewline -Path '{pid_file}' -Value $p.Id
+$cmd = "powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass
-File `"$sf`""
+$p = ([wmiclass]'Win32_Process').Create($cmd, $null, $null)
Review Comment:
Two things about this call. `Create` reports failure in its return value
rather than throwing: aimed at a missing executable on a Windows runner it
returned 9 with a null `ProcessId`, so line 310 writes a 0-byte pid file and
line 311 still prints the job id. The operator raises only on a non-zero exit
status and downgrades a job id mismatch to a warning, so submission looks
successful and the trigger then polls for an `exit_code` file that never
arrives.
The second argument is the working directory, and `$null` means the child
does not inherit the SSH session's. On the runner the job reported
`C:\Windows\system32` as its working directory while the submitting shell sat
elsewhere, so any command using a relative path will not resolve.
`(Get-Location).Path` in place of that `$null` restored it. The docstring on
line 251 still describes Start-Process as well.
--
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]
