fix(sandbox): docker sandbox native file transfer + reject truncated downloads (#2618) - #2923
Open
larry-zy wants to merge 6 commits into
Open
fix(sandbox): docker sandbox native file transfer + reject truncated downloads (#2618)#2923larry-zy wants to merge 6 commits into
larry-zy wants to merge 6 commits into
Conversation
…downloads (agentscope-ai#2618) Implement SandboxFileTransfer for DockerSandbox via docker cp (host temp file <=> container), so upload/download never pass file bytes through docker exec argv (base64 E2BIG for large files) nor the 512KB stdout truncation cap. - DockerSandbox: supportsFileTransfer/uploadFile/downloadFile with workspace-constrained path validation (rejects root, traversal, outside-workspace, blank/null); truncation-free docker cp round trip. - SandboxBackedFilesystem: fail downloads whose exec fallback output was truncated instead of silently returning partial base64. - Tests: unit coverage for path/boundary/container edge cases; opt-in (-Ddocker.it=true) real round-trip integration tests against a live Docker daemon (>1MB binary bit-for-bit, spaces/quotes, relative/absolute, workspace root '/', missing-file non-zero exit, temp-file cleanup).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
larry-zy
changed the title
…daemon Add an overridable runDockerCliBlocking seam and a recording DockerSandbox subclass so unit tests drive the uploadFile/downloadFile temp-file plumbing (mkdir, docker cp, temp write/read, cleanup on success and cp failure) without requiring -Ddocker.it=true. Raises patch coverage past the codecov gate; the docker cp execution paths were previously exercised only by the opt-in integration tests, which CI does not run.
Buktal
pushed a commit
that referenced
this pull request
Sep 2, 2026
…rdown to stop flaky temp-dir deletion (#2935) ### Problem Two CI runs fail intermittently with JUnit errors that are not test-logic failures but teardown failures: - `Failed to delete temp directory` (Windows, PR #2923) - `Failed to close extension context` / `Failed to delete temp directory` (Ubuntu, PR #2926) Both surface only when a test builds a transient `HarnessAgent`, drives it to completion via `.block()` / `.stream()...block()`, and uses `@TempDir` for its workspace/state home. ### Root cause This is a long-standing race, not a regression of any single commit. The harness memory flush has always been asynchronous: when an agent stream completes, `MemoryFlushMiddleware#onAgent` dispatches the flush on `Schedulers.boundedElastic()` via `subscribe(...)` — i.e. fire-and-forget. The calling test's `.block()` only waits for the business stream, **not** for that background flush. So the timeline is: 1. Test calls `.block()` and returns. 2. JUnit begins `@TempDir` teardown and deletes the temp directory. 3. The async flush on `boundedElastic` is **still writing session/transcript mirror files into that same `@TempDir`** (or still holds open file handles). The result: directory/file deletion fails with `IOException` → wrapped as `JUnitException`. It is timing-dependent (depends on IO speed, scheduler, and how many files are written), which is why it flakes rather than failing deterministically, and why Windows (stricter file locking) fails more readily than Linux. The normal production path avoids this because `HarnessAgent#close()` calls `SessionTree.awaitMirrorQuiescence(...)` + `MemoryBackgroundTasks.awaitQuiescence(...)`. The flaky tests never call `close()` on their transient agent. ### Fix Add test-side quiescence that mirrors what `HarnessAgent#close()` already does, run **after each test method but before the `TempDir` extension deletes the directory**: - `HarnessBackgroundTaskQuiescenceExtension` — an `AfterEachCallback` that calls `SessionTree.awaitMirrorQuiescence(5s)` + `MemoryBackgroundTasks.awaitQuiescence(5s)`. When nothing is in flight both calls return immediately, making it a no-op for tests that never trigger a flush. - `@HarnessQuiescence` — a composed meta-annotation (`@ExtendWith(HarnessBackgroundTaskQuiescenceExtension.class)`) so at-risk tests only need a one-line annotation. - Applied `@HarnessQuiescence` to 16 harness test classes that match the at-risk pattern (`HarnessAgent.builder()` + `.call()`/`.stream()` + `@TempDir`), including the three classes that flaked in CI: `JsonSessionDefaultLocationTest`, `HarnessAgentIntegrationExampleTest`, `HarnessAgentDynamicHookBuilderTest`. Production code is unchanged — the flush remains fire-and-forget so conversation completion is never blocked.
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
1 participant
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.
Problem
Fixes #2618.
SandboxBackedFilesystem.uploadFilesfails deterministically on large files. Three compounding causes:SandboxFileTransferinterface existed but no sandbox implemented it, so every upload fell back to the base64-in-argv path.ARG_MAXexceeded —DockerSandboxpasses that whole command as a singledocker execargv element, blowing past the OS argument limit (~1MB on macOS, 128KB per-arg on Linux).Real-world trigger:
SessionTree.mirrorToFilesystemmirroring 50+ messages produces JSONL >768KB; base64 inflates it ~33% and the upload permanently fails.Fix (Option A from the issue)
DockerSandbox implements SandboxFileTransfer—supportsFileTransfer/uploadFile/downloadFiletransfer bytes via a host temp file +docker cpround trip. File content never passes throughdocker execargv, so there is noARG_MAXlimit and no 512KB stdout truncation cap...traversal, outside-workspace, and blank/null paths.SandboxBackedFilesystem— when a download falls back toexecand the sandbox truncates stdout, returnFileDownloadResponse.fail(...)instead of silently handing back partial base64 (avoids silent data corruption).The dispatch logic in
uploadFiles/downloadFiles(active instanceof SandboxFileTransfer) already existed; it just had no implementation to reach. Docker now actually hits the fast path.Tests
-Ddocker.it=true, 9 passing against a live Docker daemon): >1MB binary bit-for-bit round trip, paths with spaces/quotes, relative/absolute paths, workspace root/, missing-file non-zero exit, temp-file cleanup.All green locally (unit + Docker integration).