kaxil commented on code in PR #73990:
URL: https://github.com/apache/airflow/pull/73990#discussion_r4153927973
##########
providers/common/ai/src/airflow/providers/common/ai/toolsets/sandbox.py:
##########
@@ -506,8 +594,77 @@ def _close(self) -> None:
if holder is not None:
self._release(self._attached_handle, holder)
return
+ export = bool(self._exports) and not run_failed
if sandbox is None:
+ if export and not self._forked:
+ # The run never called a tool, so it never provisioned a
sandbox, or the
+ # sandbox ended under its last command and nothing replaced
it. Either
+ # way the files it was to leave behind do not exist.
+ raise SandboxTerminalError(
+ "The run ended with no sandbox standing, so none of the
files it was to export "
+ f"exist: {', '.join(repr(path) for path in
self._exports)}."
+ )
return
+ try:
+ if export:
+ self._export(sandbox)
+ finally:
+ self._destroy(sandbox)
+
+ def _export(self, sandbox: str) -> None:
+ """
+ Copy every file in ``exports`` out of the sandbox, failing the task on
the first that cannot be.
+
+ Before teardown, and never best effort: a task that promised a file
and did not
+ deliver it must fail, or its downstream task finds nothing and cannot
tell why.
+ """
+ written: list[ObjectStoragePath] = []
+ for path, destination in self._exports.items():
+ target = self._export_target(destination)
Review Comment:
Good catch, fixed in 92fc387de27. Each file now goes to a staging key next
to its destination first (`<name>.<random>.partial`). Nothing moves into place
until all of them are copied. That move is a rename on local disk and a
server-side copy on object storage. If an export fails, partway through a file
or on the second of two, the destinations stay as they were and the staging
keys get deleted. Added tests for your ORIGINAL case and the two-file case.
##########
providers/common/ai/src/airflow/providers/common/ai/sandbox/sbx.py:
##########
@@ -55,6 +58,10 @@
# Helpers return a status or a directory listing, never bulk file content, so a
# small cap is enough to bound what a hostile guest can push into worker
memory.
_HELPER_OUTPUT_CAP = 1024 * 1024
+# Seconds an export may go without a byte arriving before it is ended. A
stall, not a
+# budget for the whole file: measured, 200 MB streams out of a local microVM
in under
+# three seconds, and a larger file only takes longer.
+_EXPORT_STALL_TIMEOUT = 120.0
Review Comment:
Yes, it only caught a stream that stops completely. `execution_timeout`
would cap it, but it is unset unless the author or `[core]
default_task_execution_timeout` sets one. I added a deadline for the whole
copy: `max_export_bytes` at 1 MiB/s, minimum 120 s. For the default 1 GiB that
is about 17 minutes. It applies to the base default, the sbx stream and the
OpenSandbox download. The 120 s stall check is still there too.
--
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]
