fix(processing): Close temp tarball before reading it

[jam-jee]

Summary

FrameworkProcessor._package_code read and then deleted its temporary tar.gz while the NamedTemporaryFile handle was still open. On Windows that raises PermissionError: [WinError 32] because the open handle still holds the file, so every FrameworkProcessor.run() on Windows failed before the job was submitted.

The fix closes the handle first, reads the archive through a with open(...) context, and unlinks it in a finally block so the temp file is removed on every path (including upload failure). Behaviour on Linux/macOS is unchanged.

The LF line-ending fixes for sm_train.sh and the repack launcher that were originally bundled with this change are already covered by open PRs #6255 / #6088 (sm_train.sh) and #6313 (repack launcher), so they are deliberately not repeated here.

Issues fixed

Fixes #5873

Testing

New unit test in sagemaker-core/tests/unit/test_processing.py: test_package_code_closes_temp_handle_before_unlink -- fails on master, passes with this change.

tests/unit/test_processing.py: 124 passed. black -l 100 and flake8 clean on changed files.

Integration tests

None added. The defect is a Windows file lock on an open temp handle; CI runs Linux, where the old code also passed, so an integ test could not reproduce it. The unit test is the regression proof, and the tarball round-trip itself is exercised end-to-end by the FrameworkProcessor integ test added in #6318.

---

X-AI-Prompt: Fix S-effort PySDK V3 bugs, windows theme
X-AI-Tool: Kiro

[github-actions]

Claude finished @jam-jee's task in 47s —— View job

---

PR Review: Close temp tarball before reading it

• Read the diff and changed files
• Analyze correctness, best practices, security, performance, tests

Verdict

This is a clean, correct bug fix. It properly addresses the Windows PermissionError: [WinError 32] in FrameworkProcessor._package_code and, as a bonus, fixes a pre-existing file-handle leak. I recommend merging.

What the fix does well

1. Root-cause fix (processing.py:1292-1293). Creating the NamedTemporaryFile with delete=False and immediately tmp.close()-ing it means no handle is held while tarfile.open(tmp.name, ...) reopens the path — the exact scenario that fails on Windows.

2. Fixes a latent handle leak (processing.py:1316-1317). The old code did body=open(tmp.name, "rb").read(), which never closed the read handle. Wrapping it in with open(...) as tar_file: closes it deterministically. Good catch.

3. More robust cleanup (processing.py:1327-1328). Moving os.unlink(tmp.name) into a finally block ensures the temp file is removed on every path — including when the tar build or S3 upload raises. Previously an upload failure would leak the temp file. tmp.name always exists once NamedTemporaryFile returns, so the unconditional unlink in finally is safe.

Test

test_package_code_closes_temp_handle_before_unlink targets the exact regression: it tracks every open .tar.gz handle and asserts none remain open at os.unlink time. This is a meaningful behavioral assertion rather than a smoke test, and it reproduces the reported failure on master. Nicely scoped.

Minor / non-blocking observations

• The whole archive is still read into memory via .read() before upload. That's unchanged from before (not a regression), but for very large source_dir/dependency bundles it could be worth streaming in a future change — out of scope here.
• The test's patch("builtins.open", ...) is global for the duration of the call; it correctly filters to .tar.gz paths so unrelated opens (tarfile internals, etc.) aren't affected. Fine as-is.

No correctness, security, or backward-compatibility concerns. Behavior on Linux/macOS is unchanged.

Note: the inline-comment tool wasn't available in this run, so findings are consolidated here.
• branch fix/windows-line-endings

[github-actions]

Claude finished @rsareddy0329's task in 35s —— View job

---

PR Review: Close temp tarball before reading it

• Read the diff (/tmp/pr.diff) and changed files
• Analyze correctness, best practices, security, performance, tests

Verdict

This is a correct, well-scoped bug fix. It resolves the Windows PermissionError: [WinError 32] in FrameworkProcessor._package_code and, as a side benefit, closes a pre-existing file-handle leak and makes temp-file cleanup robust against failures. I'd merge it.

Why the fix is correct

1. Root cause (processing.py:1292-1293). Switching to NamedTemporaryFile(delete=False) + immediate tmp.close() means no handle is held on the path while tarfile.open(tmp.name, "w:gz") reopens it. That open handle was exactly what made os.unlink fail on Windows. Linux/macOS behavior is unchanged.

2. Fixes a latent leak (processing.py:1316-1317). The old body=open(tmp.name, "rb").read() never closed its read handle; the new with open(...) as tar_file: closes it deterministically.

3. Robust cleanup (processing.py:1327-1328). Moving os.unlink(tmp.name) into finally guarantees removal on every path, including a tarfile build error or an S3 upload exception (previously those leaked the temp file). tmp.name always exists once NamedTemporaryFile returns, so the unconditional unlink in finally is safe.

Test

test_package_code_closes_temp_handle_before_unlink asserts the actual invariant that was broken — no .tar.gz handle is open at os.unlink time — rather than a smoke check, and it reproduces the failure on master. Good, targeted regression coverage. The global patch("builtins.open", ...) correctly filters to .tar.gz paths so unrelated opens aren't disturbed.

Non-blocking observations

• The whole archive is still read into memory via .read() before upload. Unchanged from before (not a regression); streaming would help for very large bundles but is out of scope here.
• Minor: the fix means tmp is briefly a closed handle whose file lingers on disk between close() and the finally unlink — intentional and necessary for the reopen, just worth noting.

No correctness, security, or backward-compatibility concerns; the return value and public behavior are unchanged.

Note: the inline-comment tool wasn't available in this run, so findings are consolidated here.
• branch fix/windows-line-endings