zozo123 commented on PR #71672: URL: https://github.com/apache/airflow/pull/71672#issuecomment-5665548746
Rebased onto current `main` (the `uv.lock` conflict is resolved — regenerated with the pinned `uv` 0.12.10 so the diff stays at the 19 lines the new extra needs) and pushed fixes for every thread from @kaxil's review. Replies are inline on each one.  ### The main change The output-bounding wrapper is much smaller. It used four background jobs, four fifos and a `wait`, and that `wait` could only finish once the capture reached end-of-input — so a command that backgrounds a process stalled until the child exited. This was not theoretical: `sleep 20 & echo started` took **20.1s** in a real microVM, and past the deadline the backend destroys the sandbox, so the agent lost its files over a command whose foreground part had already finished. Each stream is now redirected to a scratch file and the last `max_output_bytes` emitted with `tail -c`, so only the foreground command is waited on. The same command is **0.3s** live. The fifos, drains and `wait` are gone, along with the `0`/`1` header protocol — the wrapper is asked for one byte over the budget, so an over-budget stream is itself the proof that bytes were dropped. The trade is that total output lands on the sandbox's own ephemeral disk rather than streaming through a fixed window, which is now stated in the docstring and the docs. Also fixed: `mkdir -m 700` instead of a `umask` left in force for the agent's command; signal traps that `exit` instead of cleaning up and carrying on; truncation that drops a partial leading line (and a partial leading *record* in `list_directory`, which has no newline to align on); the sandbox name bound before creation with best-effort delete if creation fails ambiguously; retries kept on `delete_sandbox`; a failed post-timeout cleanup downgraded from task failure to a warning, since `delete_after` reclaims it; and unrecognised exec statuses treated as terminal rather than still-running. ### Two things the live runs turned up - **`SandboxSpec.env` set at creation is visible to later execs**, including through the wrapper's `sh -lc`. That answers the open question on that thread — the create-time call is sufficient, and there is now a test so it cannot be dropped silently. - **The server does not appear to enforce `timeout_secs`.** `sleep 30` with `timeout_secs=2` ran the full 30s and reported `completed` with exit 0. The client-side polling deadline is what actually bounds a command, and that path already destroys the sandbox and reports a timeout, so behaviour is correct — but the `"timeout"` status is not something to rely on. ### Verification - **65 unit tests pass**, up from 45. The new `TestCommandWrapper` executes `_COMMAND_WRAPPER` through a local `sh` with no API key, so the riskiest code is covered in ordinary CI — that was the gap that let all of these through. I wrote those tests against the old wrapper first and confirmed they fail on it. - **The shipped system test passes** against a live microVM, as does a broader end-to-end pass covering spec env, backgrounded commands, permissions, truncation, persistence, deny-all egress, the timeout path and idempotent teardown. - Provider still imports without the optional SDK, and first use raises the actionable "install the extra" error. - Ruff check and format clean at the repo's pinned version. No sandboxes were left behind by any of the runs. -- 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]
