fix(sandbox): keep unbounded UnixLocal workspace I/O off the event loop - #4700
Open
ayaangazali wants to merge 1 commit into
Open
fix(sandbox): keep unbounded UnixLocal workspace I/O off the event loop#4700ayaangazali wants to merge 1 commit into
ayaangazali wants to merge 1 commit into
Conversation
persist_workspace tars the whole workspace, hydrate_workspace extracts it, and rm(recursive=True) removes it, all synchronously inside async methods. The loop is blocked for the full duration, so a resume cannot even be cancelled: the cancellation never gets scheduled. Moving that work to a thread on its own is not safe here. asyncio.to_thread() does not stop its worker when the awaiting task is cancelled, and restore_snapshot_into_workspace_on_resume closes the archive stream in a finally as soon as its await returns, so a surviving extractor would read a closed stream and write into a workspace that resume then clears. Run the work in a worker thread and keep waiting for it even while cancelled, matching the mutation semantics the session backends already rely on in agents.memory.sqlite_session. Cancellation latency is unchanged, since the loop was blocked for the same work before, while the loop stays responsive and the archive and workspace are only released once nothing is still writing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UnixLocalSandboxSessiondoes three pieces of unbounded filesystem work synchronously insideasync defmethods:persist_workspacetars the whole workspace,hydrate_workspaceextracts it, andrm(recursive=True)removes it. All three sit on the snapshot path, so the event loop is blocked for the full duration of a workspace-sized archive or extract.The blocking is severe enough that a resume cannot be cancelled at all. Without this change, the regression test below fails with
DID NOT RAISE CancelledError: the loop never gets a chance to deliver the cancellation, so the task runs to completion instead.Moving that work to a thread is not sufficient on its own, which is what closed #4678.
asyncio.to_thread()does not stop its worker when the awaiting task is cancelled, andrestore_snapshot_into_workspace_on_resumecloses the archive stream in afinallyas soon as its await returns:So a cancelled hydrate that returns early leaves a live extractor reading a closed stream and writing into a workspace that resume then clears.
This change runs the work in a worker thread and keeps waiting for that worker even while cancelled, so ownership of the archive and the workspace root is never released while something is still writing. That is the same mutation semantic the session backends already depend on:
agents.memory.sqlite_session._await_mutationis used by the SQLite, SQLAlchemy, MongoDB and Redis sessions for exactly this reason, and the helper here follows it rather than inventing a second answer.Scope is the lifecycle boundary rather than a lint rule. Only the three unbounded sites that own shared workspace state are moved. The bounded
mkdir,existsandresolvecalls are deliberately left alone, since being on the loop is not a demonstrated defect for them and they sit on the exec confinement path.Tradeoff worth stating plainly: cancellation is not made prompt, only safe. A caller that cancels mid-extract still waits for the extract to finish. That is not a regression, because today the loop is blocked for that same work and cancellation cannot be delivered at all. Making cancellation prompt needs a cooperative stop inside
safe_extract_tarfileandshutil.rmtree, which is a larger change and a separate one.Test plan
tests/sandbox/test_unix_local.py::test_hydrate_workspace_cancellation_waits_for_the_extracting_workercovers the ordering that was missing: it starts a hydrate whose extractor blocks, cancels the awaiting task once the worker has actually started, and asserts thatCancelledErrorreaches the caller only after the worker recorded completion, with the archive stream still open at that point.Verified it fails without the source change by reverting
src/: it fails withDID NOT RAISE CancelledError, which is the blocked-loop symptom itself..agents/skills/code-change-verification/scripts/run.shpasses end to end: format, lint, typecheck and the full suite, including all 1447 sandbox tests.Issue number
Fixes #4675
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PRI picked this up after #4678 was closed, and I tried to build to the shape you described there rather than re-slice the same patch. The part I would most like checked is the decision to wait for the worker instead of trying to interrupt it. Waiting seemed like the only honest option given a thread cannot be stopped, and it matches what the session backends already do, but it does mean a cancel can block for as long as the extract takes. I'm a freshman in college, so if you would rather see cooperative stopping inside the tar and rmtree paths before this lands, I'm glad to take that direction instead.