Repository navigation
Conversation
_upload_file opened the destination with "wb" before streaming, so an upload that failed part-way (disk full, a size limit, the process killed) destroyed the previous file and left a partial one. Stream into a sibling .<name>.<random>.tmp file and os.replace it onto the destination only on success, removing it on failure. An overwrite keeps the file's permissions and owner, a read-only file is still refused, and a symlink's target is replaced as before. Drop the F14.upload-atomic known-bug marker. Fixes #5889 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review — PR #5902, head 2c2670e
Scope: in-repo. This changes the agent-server's own /api/file/upload handler (openhands-agent-server/.../file_router.py), which this repository owns; it does not need a product/architecture decision or a move.
State checked on this head: uv run pytest tests/agent_server/test_file_router.py → 96 passed. All GitHub Actions checks for 2c2670e are green (build, agent-server-tests, tests 24.x, agent-server-api, REST-API breakage, etc.). Linked issue #5889 is open with bug, priority:low, ready-for-dev, and its acceptance criteria match the diff. No prior review by this account exists on this head.
The core fix is sound. Streaming into a sibling .<name>.<random>.tmp and os.replace-ing it only after the stream completes makes a failed overwrite preserve the previous file and leave no temp file behind. I confirmed the failure path (previous content intact, no .*.tmp leftover), the permission-preservation path, and the symlink path locally. Two findings below; the first is material.
1. Overwrite uploads break on Windows (material)
_atomic_write calls os.chown(...) unguarded (line 113). os.chown does not exist on Windows, and contextlib.suppress(OSError) does not catch the resulting AttributeError, so every overwrite of an existing file raises and _upload_file maps it to HTTP 500. New-file uploads are unaffected (previous is None). The unit tests pass only because they run on Linux. The agent server is built and smoke-tested on Windows (build-binary-and-test matrix includes windows-latest in .github/workflows/server.yml; release-binaries.yml builds a windows-2022 binary), and file_router.py itself already carries Windows branches (_list_root_locations, os.name == "nt"). This is a supported runtime, so this is a regression introduced by the diff. Guard the call (if hasattr(os, "chown"), or contextlib.suppress(AttributeError, OSError)) or drop it.
2. Temp file is world-readable during the write
open(tmp_path, "wb") (line 108) creates the scratch file with the process umask (typically 0644) and only os.chmods it to the destination's mode after the stream finishes (line 111). When overwriting a private file (e.g. 0600), the incoming content is readable by other local users in .<name>.<random>.tmp for the duration of the upload — I measured 0644 mid-write for a 0600 target. The previous in-place open(target_path, "wb") kept 0600 throughout, so this is a confidentiality regression for private uploads. Create the temp with a restrictive mode and widen to the destination's mode just before os.replace.
🔄 CHANGES REQUESTED
| if previous: | ||
| os.chmod(tmp_path, stat.S_IMODE(previous.st_mode)) | ||
| with contextlib.suppress(OSError): | ||
| os.chown(tmp_path, previous.st_uid, previous.st_gid) |
There was a problem hiding this comment.
os.chown does not exist on Windows, and contextlib.suppress(OSError) does not catch the AttributeError it raises there. Because this runs inside the try, the exception escapes _atomic_write, the temp file is cleaned up, and _upload_file returns HTTP 500 for every overwrite of an existing file on Windows (new-file uploads are fine, since previous is None). The agent server is a supported Windows runtime — build-binary-and-test covers windows-latest and a windows-2022 release binary is produced — and this file already branches on os.name == "nt". Guard the call (e.g. if hasattr(os, "chown") or contextlib.suppress(AttributeError, OSError)) or drop it.
| raise PermissionError(errno.EACCES, os.strerror(errno.EACCES), str(dest)) | ||
| tmp_path = dest.with_name(f".{dest.name}.{uuid4().hex}.tmp") | ||
| try: | ||
| with open(tmp_path, "wb") as f: |
There was a problem hiding this comment.
The temp file is created with the process umask (typically 0644) and only chmod-ed to the destination's mode on line 111, after the whole stream is written. For an overwrite of a private file (e.g. 0600) the new content is therefore world-readable in .<name>.<random>.tmp for the duration of the upload — I measured 0644 mid-write for a 0600 target. The old in-place open(target_path, "wb") kept 0600 throughout, so this weakens confidentiality for private uploads. Consider creating the temp with a restrictive mode (e.g. os.open(tmp_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)) and widening it to the destination's mode just before os.replace.
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review — PR #5902, head 7a865b1
Scope: in-repo and correctly placed. _atomic_write changes only the agent-server's own /api/file/upload handler, which this repository owns; no OpenAPI contract change (the endpoint, request, and FileUploadResult are unchanged), so no clients/typescript/ work is required. Not a draft on the gate.
Issue #5889 acceptance: delivered. The overwrite now streams into a sibling temp file, os.replace runs only after the stream completes, failures remove the temp, and read-only targets are still refused. All four acceptance points from the issue (previous content survives a failed overwrite, no temp file left behind, permissions kept on success, atomic replacement) are covered by the new tests: test_failed_overwrite_keeps_previous_file, test_upload_overwrite_keeps_file_mode, test_upload_does_not_overwrite_read_only_file, test_upload_through_symlink_replaces_link_target. I ran uv run pytest tests/agent_server/test_file_router.py on this head in the workspace: 104 passed. The PR also carries recorded live before/after map-run evidence (xfail on main, pass on this head), which satisfies the live-evidence bar for this runtime-behavior fix. Symlink-through writes, new-file umask semantics, upload-onto-directory error mapping, and hard-link semantics (disclosed in the PR notes) all behave as documented. The F14-files.md feature-map updates match the implemented behavior.
Checks: all substantive GitHub Actions checks are green on 7a865b1. The only failing check is unresolved-review-threads, which reflects the two unresolved review threads carried over from the earlier review of this branch (on 2c2670e), not a code failure.
Blocking findings on the current head (both re-verified against 7a865b1; the code is unchanged in these spots since the earlier review, and the threads remain unresolved):
-
Windows: every overwrite of an existing file fails with HTTP 500.
os.chowndoes not exist on Windows;os.chown(...)raisesAttributeError, whichcontextlib.suppress(OSError)does not catch. The exception escapes_atomic_write'stry, the temp file is deleted, and_upload_file's generic handler returns 500 — so overwriting an existing file never succeeds on Windows, a regression from main's in-place write. I confirmed the escape empirically withos.chownremoved, exactly reproducing the code path (write succeeds,chmodsucceeds,chownraisesAttributeError, temp removed). Windows is a supported agent-server runtime:build-binary-and-testbuilds and smoke-runsdist/openhands-agent-server.exeonwindows-latestandrelease-binaries.ymlships awindows-2022binary,file_router.pyhas no POSIX gate, and sibling agent-server modules branch onos.name. CI cannot catch this:agent-server-testsis pinned to Ubuntu and the Windows suite only runstests/tools. Fix by guarding the call (if hasattr(os, "chown")orcontextlib.suppress(AttributeError, OSError)) or dropping it. -
Temp file holds new content at umask permissions for the whole upload.
open(tmp_path, "wb")creates the temp with the process umask (measured0644in this environment), and thechmodto the destination's mode only runs after the entire stream is written. For an overwrite of a private file (e.g.0600), the new content is world-readable in.<name>.<random>.tmpfor the duration of the upload — main's in-place write kept0600throughout, so this weakens confidentiality for private-file uploads. The same applies to a leftover temp after an abrupt kill, which sits at umask mode instead of the destination's. The repo's ownatomic_write_textinopenhands-sdk/utils/files.pysets the restrictive mode before writing; doing the same here (create restrictive, widen to the destination's mode just beforeos.replace) removes the window.
Both are fixable in a few lines without changing the design. Not approving while they stand.
🔄 CHANGES REQUESTED
| if previous: | ||
| os.chmod(tmp_path, stat.S_IMODE(previous.st_mode)) | ||
| with contextlib.suppress(OSError): | ||
| os.chown(tmp_path, previous.st_uid, previous.st_gid) |
There was a problem hiding this comment.
os.chown does not exist on Windows, and contextlib.suppress(OSError) does not catch the AttributeError it raises there. Because this runs inside the try, the exception escapes _atomic_write, the temp file is removed, and _upload_file returns HTTP 500 for every overwrite of an existing file on Windows (new-file uploads are fine, since previous is None) — a regression from main's in-place write. The agent server is a supported Windows runtime: build-binary-and-test builds and smoke-runs dist/openhands-agent-server.exe on windows-latest, release-binaries.yml ships a windows-2022 binary, this file has no POSIX gate, and sibling agent-server modules branch on os.name. CI misses it because agent-server-tests is pinned to Ubuntu and the Windows suite only runs tests/tools. Guard the call (e.g. if hasattr(os, "chown") or contextlib.suppress(AttributeError, OSError)) or drop it.
| raise PermissionError(errno.EACCES, os.strerror(errno.EACCES), str(dest)) | ||
| tmp_path = dest.with_name(f".{dest.name}.{uuid4().hex}.tmp") | ||
| try: | ||
| with open(tmp_path, "wb") as f: |
There was a problem hiding this comment.
The temp file is created with the process umask (typically 0644) and is only chmod-ed to the destination's mode after the whole stream is written. For an overwrite of a private file (e.g. 0600) the new content is world-readable in .<name>.<random>.tmp for the duration of the upload — I measured 0644 mid-write for a 0600 target on this head. The old in-place open(target_path, "wb") kept 0600 throughout, so this weakens confidentiality for private uploads; a temp left by an abrupt kill also sits at umask mode instead of the destination's. Consider creating the temp with a restrictive mode (e.g. os.open(tmp_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)) and widening it to the destination's mode just before os.replace, as the repo's own atomic_write_text already sets its mode before writing.
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Summary
Read-only targets remain refused; new files retain umask-based permissions.
Why
A failed upload overwrite should leave the previous complete file available.
Issue Number
Fixes #5889
How to Test
Previously recorded verification:
Reproduction commands, results, and limits
uv run pytest tests/agent_server/test_file_router.py: 94 passed, 2 skipped (the read-only test skips as root; run as an unprivileged user, all 12 upload tests pass).test_failed_overwrite_keeps_previous_filefails on main.AGENT_SERVER_VERIFY_HOMEprivate to this run:control-agent-server map run --file F14 --only F14.upload-atomic --fresh --repeat 2 --checkout <main checkout>->xfailin both attempts.--checkout->xpassin both attempts.control-agent-server map checkok;map run --file F14 --only F14.upload-atomic --fresh->pass(re-run after the final code changes).uv run pre-commit run --files <changed files>: all hooks pass.Video/Screenshots
No UI change; reproduction evidence is above.
Type
Notes
Rename replacement creates a new inode: other hard links retain the old content, and the destination directory must be writable. There is no
fsyncpower-loss guarantee; an abrupt kill can leave a temporary file. Upload-onto-directory error mapping is unchanged. #5856 separately adds the returned upload size.Visual explanation by Codex using HumanLayer show-me.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:7a865b1-pythonRun
All tags pushed for this build
About Multi-Architecture Support
7a865b1-python) is a multi-arch manifest supporting both amd64 and arm647a865b1-python-amd64) are also available if neededJev-Fast-Audit
⚡ Jev fast audit · estimates · 0.48s · commit 7a865b1
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: complete supplied coverage; 11/11 hunks, 3/3 files.
All estimates and evidence