Repository navigation
feat: resumable network volume upload and download - #655
justinwlin wants to merge 20 commits into
Conversation
Adds runpod.network_volume_transfer (upload/download over the S3-compatible API) and `rp network-volume upload|download`. Multipart uploads resume from a local state file plus ListParts; downloads resume from a chunk sidecar; folder transfers skip unchanged files.
Runpod answers HeadObject on a directory with 400, so download falls through to listing the prefix. The CLI only suggests re-running when the failure is interrupted or transient, not for missing paths or 401/403.
- a failed complete is only accepted when it was ambiguous and the object's etag equals the multipart etag of our parts - parts are reused only when their md5 etag matches the local bytes - abort when the local file changes mid-upload or state can't be saved - download skips need same size and a remote copy no newer than local - sidecar pinned to its remote key; refuse x + x.rpdownload collisions - reject '..' remote paths - requests share one concurrency limit across files and parts
- an ambiguous failed complete needs our multipart etag to match - a single put fails if the file changed across retries - partial-name collisions also cover directories and case folding - every s3 call takes a concurrency slot
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 1
Verdict: Ready after fixes. The resume design holds up, but there are four medium issues: Ctrl-C handling, stacked retries, download target collisions, and an unacknowledged CI failure.
Approval gate: 0 Critical, 1 High, 11 Medium+Low → not approved: 1 High, Medium+Low ≥ 10, and the PR is a draft.
Existing feedback
- github-code-quality and github-advanced-security (CodeQL): 3 findings, each posted twice (empty
except×2, mixedrunpodimport). All 6 threads are resolved. Verified fixed atd95e161._removenow usesunlink(missing_ok=True)(network_volume_transfer.py:178). TheLookupErrorbranch has a comment (:416). The test uses one import style.
PR description vs code
| Claim | Status | Where |
|---|---|---|
rp network-volume upload/download, upload_to_network_volume / download_from_network_volume, return keys |
✅ | rp_cli/main.py:1180-1201, network_volume_transfer.py:268,908-909, __init__.py |
| Skip when same size and destination not older | ✅ | :532-540, :603-607 |
Upload id in ~/.runpod/transfers/, parts reused when the MD5 ETag matches |
✅ | :58-59, :382-392 |
| Upload aborted if the local file changes mid-upload | ✅ | :477-484 (and :358-363 for single PUT) |
| Failed complete accepted only if ambiguous and the multipart ETag matches | ✅ | :503-518 |
| 64 MB download chunks, sidecar pinned to key, size, and ETag | ✅ | :31, :618-626 |
| Remote mtime stamped on downloads | ✅ | :697-699 |
| One concurrency limit covers all requests | ✅ with caveat | All client calls go through _request (:272), but botocore's own retries also run inside the slot (see Medium). The comment at :835 says otherwise (Low). |
| Transient failures retry with backoff, including 524 | ✅ | :89-117 |
| "The CLI prints a resume hint only when rerunning can help" | :143 defaults to resumable, so the hint shows for config errors. Ctrl-C gets no hint (Low). |
|
| "The 57 new tests" | ❌ | The file has 48 test functions, which collect as 63 cases (Low) |
| Live e2e, 1732 passed, 92.6% coverage | Not re-run by this review |
Findings
High
- Failing
e2eandvalidationchecks are not mentioned in the PR body.e2efails withGraphQL request failed: 401whilerunpod_flashdeploys mock-worker endpoints (log).validationfails only becausee2efailed. The same 401 shows onmainCI and on #654, so this PR did not cause it. The likely cause is the repo's e2e API key secret. Fix: add one line to the body saying so, or rerun once the secret is fixed.
Medium
- Ctrl-C during a folder transfer drains every file already in progress. See
network_volume_transfer.py:226. KeyboardInterrupt only reaches the outer_run_filespool. Each running file's inner part or chunk pool keeps going, then the outerwith ThreadPoolExecutorwaits for it.rp_volume_cache.py:730-739already solves this with a sharedstopEvent. - Two retry layers multiply, so a bad endpoint spins for minutes. See
:832. botocore standard mode (10 attempts) runs inside_with_retries(8 attempts), which allows up to 80 attempts per request. botocore's backoff also sleeps while holding a concurrency slot. - Download targets that collide after casefold or normalization are not refused. See
:762.Data/a.binanddata/A.bin(ora/./banda/b) map to one local file on macOS or Windows. Two workers then share one.rpdownload, and a rerun skips the mixed result.
Low
- The resume hint shows for config errors and is missing for Ctrl-C.
:143,rp_cli/main.py:1166. Examples: an unresolvable--datacenter,NotADirectoryError,ParamValidationError. - Uploading a file onto an existing remote folder fails with a raw 400.
:569callshead(key)withoutdirectory_ok.upload vol model.bin modelsprints "An error occurred (400) when calling the HeadObject operation". - A single-file download to
out/(missing folder) writes a file namedout.:711-713. Upload treats a trailing/as "keep the name", so the two directions disagree. volume_idis not escaped in the REST path.:797. The repo escapes IDs with_path_segment(api/ctl_commands.py:17).- The
max_pool_connectionscomment is stale.:835-837. It says metadata calls run alongside the slots, but every call goes through_request. - Download help understates the skip rule.
rp_cli/main.py:1200says "same size". The code also requires the local copy to be no older than the remote. - The feature has no docs or changelog entry.
docs/cli/references/command_line_interface.mdlists everyrpgroup exceptnetwork-volume.CHANGELOG.mdhas a hand-maintained## Unreleasedsection. - The PR body says 57 tests. The file collects 63 cases.
Nits
:447-449,:643-647bytes_transferredcounts resumed bytes that were not sent this run.:33UPLOADS_DIR = ".s3compat_uploads"needs a comment saying the server creates it. The test also hardcodes the literal instead of using the constant.:736One key with\ora//bfails the whole folder before any file starts, unlike the "every file is attempted" rule at:576.tests/test_network_volume_transfer.py"review follow-ups" describes PR history, not behavior. The two concurrency tests (:788,:849) overlap.tests/test_network_volume_transfer.py:594builds a realboto3.session.Session(), so a staleAWS_PROFILEon a dev machine fails it.
Simplification opportunities
- Drop one retry layer by setting botocore
max_attemptsto 1. This removes about 1 line and fixes the Medium. Same PR. - Merge the two concurrency tests. This removes about 25 lines. Same PR or a follow-up.
Verified clean
- Path traversal on download.
_safe_targetrefuses.., a leading/,\, drive letters, and empty segments. It also checks containment afterresolve(), which catches pre-existing symlinks that point outside the root...is refused before any request. - No deadlock. The semaphore is held only around single client calls, and no
_requestcall nests another. - Upload memory stays bounded. Part bodies are read inside the slot.
- Multipart resume. The state key covers endpoint, bucket, key, and local path. Size, mtime, and part size are pinned, and each part is MD5-verified. A missing upload falls back to a fresh one. A failed state write aborts the upload.
- Ambiguous complete. It is accepted only when the error is retryable or 404, the size matches, and the ETag equals the content-derived multipart ETag.
- Download resume. Chunks are fsynced before the sidecar records them. Every range GET re-checks the ETag. Timezone-aware
LastModifiedmakes the stamp correct, and the round trip skips as unchanged. - Edge cases and dependencies. Zero-byte and exactly-64 MiB files work, and
part_size_forrespects both limits. boto3 is a declared dependency and is imported lazily, likerp_upload.py. - Test hygiene. Tests patch
_sleep, usetmp_pathfor the state directory, and set S3 env vars through monkeypatch. Each test builds a fresh fake S3, so nothing leaks between tests.
Questions for the author
- Orphaned multipart parts stay in
.s3compat_uploadsand count against the volume's quota. Examples: an upload that is never rerun, a failed_abort, or an ambiguouscreate_multipart_uploadretry. Is that expected? If so, a "Things to know" note would help users. - Do Runpod's ListObjectsV2 and HeadObject return
LastModifiedat the same precision? If list values have sub-second precision and HEAD truncates, a folder download followed by a single-file upload would not skip.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 116 in / 12,916 out / 3,501,160 cache read / 259,631 cache write = 3,773,823. Estimated cost: $3.04 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (1-hour cache TTL assumed; $2.26 at 5-minute TTL). Counted before posting. Round 1 of this PR. This is automated assistance; a human owns the final call.
- ctrl-c stops files in flight (shared stop event) and the cli prints the resume hint - botocore retries off; _with_retries owns them; 300s socket read timeout - refuse download targets that share a local name (casefold + nfc) and '.' key segments - resume hint only for interruptions and transient failures; a dns failure is neither retried nor resumable and points at --datacenter - uploading a file onto a remote folder explains the trailing slash; a download target ending in / is a folder (also via the public function) - a single put that raced a local edit deletes the object so the re-run sends the new version - --force; skip rule compares whole seconds; missing folder is not found; resumed bytes are not counted as transferred; leftovers not uploaded - escape volume id in the rest path; docs reference and changelog entry
There was a problem hiding this comment.
Claude review, round 2
Verdict: Ready after fixes. Every round 1 finding is fixed, but the DNS fail-fast added for bad --datacenter also turns a network drop into an instant, non-resumable failure.
Approval gate: 0 Critical, 1 High, 3 Medium+Low → not approved: the e2e check still fails (acknowledged and not caused by this PR, but a failing check blocks approval).
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R1-1 | Failing e2e/validation not mentioned in the body |
High | Partially fixed | Body now explains the 401. The check still fails at c92ac38 (log), and main CI has failed since 17:29 UTC today. Carried as High until e2e is green. |
| R1-2 | Ctrl-C drains files in flight | Medium | Fixed | _run_pool sets _stop on interrupt (network_volume_transfer.py:254-256). _request raises _Stopped (:318-319). Test at tests/test_network_volume_transfer.py:1000. |
| R1-3 | botocore retries stack with _with_retries |
Medium | Fixed | total_max_attempts: 1 (:932), asserted in the test. |
| R1-4 | Download targets colliding after casefold/NFC/. |
Medium | Fixed | _fold + _check_local_names (:826-863), ./empty segments refused (:874). Three parametrized cases. |
| R1-5 | Resume hint for config errors, none for Ctrl-C | Low | Fixed | is_resumable is now an allowlist (:150-162). CLI catches KeyboardInterrupt (rp_cli/main.py:1173-1175). |
| R1-6 | Upload onto a remote folder gives raw 400 | Low | Fixed | :628-636 |
| R1-7 | out/ single-file download writes a file named out |
Low | Fixed | :787, and download() passes the str through (:1012). |
| R1-8 | volume_id not escaped |
Low | Fixed | :892-894 |
| R1-9 | Stale max_pool_connections comment |
Low | Fixed | :937-939 |
| R1-10 | Download help understates skip rule | Low | Fixed | rp_cli/main.py:1212-1214 |
| R1-11 | No docs or changelog | Low | Fixed | docs/cli/references/command_line_interface.md, CHANGELOG.md |
| R1-12 | Body says 57 tests | Low | Fixed | Body says 80 cases. Counting parametrized cases in the file gives 80. |
| R1 nits | bytes_transferred, UPLOADS_DIR comment, section header, duplicate concurrency test, AWS_PROFILE |
Nit | Fixed | "reuse" event, :35-36, tests |
| R1 nit | One bad key (\, a//b, now .) fails the whole folder |
Nit | Not addressed | :812 runs _safe_target for every key before _run_files. Still fine as a nit. |
Existing feedback
- github-code-quality and github-advanced-security (CodeQL): 6 threads, all resolved and verified fixed in round 1. No new alerts at
c92ac38. - All 10 round 1 threads were resolved before this round. Each one gets a short "Fixed at
c92ac38" reply for the record.
PR description vs code
| Claim | Status | Where |
|---|---|---|
Skip rule: same size, destination not older, whole seconds; force / --force |
✅ | :268-270, :597, :671-679, rp_cli/main.py:1118-1121 |
| Local change mid-upload aborts (multipart) or deletes (single PUT) | ✅ | :534-541, :409-422 |
Folder download refused on shared local paths (case, NFC, ., x.rpdownload) |
✅ | :826-874 |
| One concurrency limit, Ctrl-C stops files in flight | ✅ with caveat | A worker sleeping in a retry backoff waits out the sleep first (Low) |
bytes_transferred counts only this run |
✅ | "reuse" is not summed (:326-333) |
| "The CLI prints the resume hint only for interruptions and transient failures" | A network drop that fails DNS gets no resume hint and a misleading --datacenter hint (Medium) |
|
| Resume after a network drop | Same Medium: a drop longer than about 1 s stops retries once DNS fails | |
CI note on e2e / validation |
✅ | Log matches: GraphQL request failed: 401 in runpod_flash deploys |
| 80 test cases, 1755 passed, live checks | Case count verified. Test run and live checks not re-run by this review. |
Findings
High
e2eandvalidationstill fail (R1-1, carried). The cause is the repo's e2e API key, not this PR. The gate passes oncee2eis rerun green. No code change needed here.
Medium
- DNS failure is treated as a wrong datacenter, so a network drop stops retrying and loses its resume hint.
network_volume_transfer.py:110. When Wi-Fi drops, the reconnect fails ingetaddrinfo. macOS returnsEAI_NONAME(8), the same errno the test uses for a typo. The first retry after the 1 s backoff then raises without further retries. The CLI prints "check --datacenter" for a single file, and nothing at all for a folder (the aggregated error has no cause chain). Fix: resolve the endpoint host once in_make_transferand raise the datacenter error there. Keepgaierrorretryable mid-transfer. That also removes the CLI's privatenvt._is_dns_failurecall.
Low
- Ctrl-C waits out a retry backoff.
:173._with_retriessleeps up to 31 s without checking_stop. During an outage, every worker in backoff delays exit by that long. Pass the event and usestop.wait(delay). - The new CLI branches have no tests.
rp_cli/main.py:1173-1179. Neither theKeyboardInterruptexit (130 plus hint) nor the DNS hint is exercised. The body says each fix has a test that fails without it.
Nits
network_volume_transfer.py:539The multipart "changed during upload" message still says "run the command again", and the CLI then appends "re-run the same command to resume". The single-PUT message at:421was shortened for this reason.network_volume_transfer.py:848The collision message says "case-insensitive filesystem" for an NFC collision too.
Simplification opportunities
- Replace
_is_dns_failure(15 lines) and the CLI special case with onesocket.getaddrinfocheck before the first request. Net about −10 lines. Same PR (fixes the Medium).
Verified clean
- Ctrl-C wind-down.
_Stoppedis not retryable, so_with_retriesstops. Inner pools cancel.guardedrecords the file.KeyboardInterruptstill reaches the caller. Multipart state and download sidecars survive for the rerun. - Single-PUT change path. The delete goes through
_request, so it honors the slot and the stop flag. A failed delete is swallowed, and--forcecovers the rerun. The test edits to an older mtime, so it fails without the delete. - Whole-second skip matches in both directions. Downloads stamp the exact remote time.
_check_local_nameschecks file-vs-file collisions first, then the partial and sidecar twins against both files and parent dirs.- Trailing slash. The CLI passes
local_pathas a plain str (noclick.Path), so the/reachesdownload(). - Empty volume.
download("/")on an empty bucket returns zeros; a missing prefix still raisesFileNotFoundError. - Folder upload skips
.rpdownload, sidecar, and sidecar.tmpleftovers.
Questions for the author
- A real local file ending in
.rpdownloadis skipped by folder upload with noskipevent or message. Is silent omission intended, or should it be reported in the summary?
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 24 in / 11,796 out / 815,623 cache read / 89,091 cache write = 916,534. Estimated cost: $1.11 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (1-hour cache TTL assumed; $0.80 at 5-minute TTL). Counted before posting. Round 2 of this PR. This is automated assistance; a human owns the final call.
- an unresolvable endpoint fails before the transfer; a dns failure mid-transfer is a network drop and is retried (drops _is_dns_failure) - retry backoff ends early on ctrl-c (stop-aware wait) - single put changed mid-upload: keep the object if the file vanished, and say to re-run with --force if the delete fails - head 400 on an upload target is called a folder only if a listing confirms it; NoSuchUpload and 416 resume - refuse a file that would also be a folder; collision wording covers normalization; leftovers are reported as skipped - state writes sync the directory; cli tests for ctrl-c and the endpoint check
- behind an https proxy (honoured by botocore) the proxy resolves the endpoint, so the local lookup is skipped - endpoint check tests stub getaddrinfo and proxies instead of real dns - explain the ignored directory fsync failure
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 3
Verdict: Needs changes. Every round 2 finding is fixed, but test_ctrl_c_stops_files_in_flight failed on Python 3.11 at this head, and the PR body does not mention it.
Approval gate: 0 Critical, 2 High, 2 Medium+Low → not approved: two failing checks (the PR's own unit test, and the carried e2e failure).
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R1-1 | e2e / validation fail |
High | Not addressed (external) | Same GraphQL request failed: 401 at b923852 (log). The body explains it. A failing check still blocks the gate. |
| R2-1 | DNS failure treated as a wrong datacenter | Medium | Fixed | _is_dns_failure removed. _check_endpoint_resolves runs once in _make_transfer (network_volume_transfer.py:942-957, :982). A mid-transfer gaierror is retryable and resumable (test test_dns_failure_mid_transfer_is_a_network_drop). |
| R2-2 | Ctrl-C waits out a retry backoff | Low | Fixed (code) | _wait uses stop.wait (:151-156). Every _request call goes through self._retry (:334-335). The new test cannot catch a revert (new Low below). |
| R2-3 | New CLI branches untested | Low | Fixed | test_cli_ctrl_c_exits_130_with_resume_hint, test_cli_unresolvable_endpoint_has_no_resume_hint |
| R2 nit | Multipart "changed" message doubles the hint | Nit | Fixed | :570 |
| R2 nit | Collision wording says only "case-insensitive" | Nit | Fixed | :880-881 |
| R1 nit | One bad key fails the whole folder | Nit | Not addressed | Still fine as a nit. |
| R2 Q | .rpdownload silently omitted from folder upload |
Question | Answered | Now emits skip (:640-643) and counts in files_skipped. |
Existing feedback
- github-code-quality and github-advanced-security (CodeQL) on
network_volume_transfer.py:193: Confirmed, Nit. Theexcept OSError: passhas its reason in a comment above thetry, not inside the handler. Two open bot threads. - All round 1 and round 2 threads are resolved. No new human comments.
PR description vs code
| Claim | Status | Where |
|---|---|---|
Single PUT: delete on change, --force hint if the delete fails, keep the object if the file vanished |
✅ | :436-455, tests at tests/test_network_volume_transfer.py:936-975 |
| Folder hint only when a listing confirms the folder | ✅ | :659-667, _is_folder :392-405 |
| Leftovers reported as skipped | ✅ | :640-643 |
| File that would also be a folder is refused | ✅ | :887-892 |
| Ctrl-C ends a retry backoff | ✅ code, |
See Low below |
| DNS mid-transfer retried; bad datacenter fails before the transfer | ✅ | :942-957, :982 |
Resume hint for NoSuchUpload and 416 |
✅ | :146-147. A rerun handles both: NoSuchUpload starts a new upload (:469), a changed object fails the sidecar check (:722-731). |
| "1765 passed" | ❌ | CI at this head: 1 failed, 1764 passed on 3.11 (High below) |
| 90 collected cases, live checks | Not re-run by this review |
Findings
High
test (3.11)fails:test_ctrl_c_stops_files_in_flightis timing-dependent.tests/test_network_volume_transfer.py:1133.assert 10 < 10(log). The other interpreters were cancelled or passed, and all five passed atc92ac38, so this is a race, not a code regression. The fake raisesKeyboardInterruptin a worker thread._stopis set only after that propagates through the inner pool, so the other file's workers keep callingupload_partfor an unbounded time. Make the assertion deterministic (inline comment) and mention CI in the body.e2e/validation(R1-1, carried). External cause; needs a green rerun.
Low
test_retry_backoff_ends_when_stoppedpasses even with the fix reverted.tests/test_network_volume_transfer.py:1007. The autouse fixture stubs_sleepto a no-op. If_with_retrieswent back to_sleep(delay), the test would still finish in under 0.5 s. Also restorenvt._sleeptotime.sleepin this test.- The DNS pre-check can block proxy-only networks.
network_volume_transfer.py:952. botocore honorsHTTPS_PROXY, so the proxy resolves the host. On a network where only the proxy can resolve public names,getaddrinfofails locally and the transfer is refused before boto would have worked. Skip the check whenurllib.request.getproxies()has anhttpsentry, or treat the failure as a warning.
Nits
tests/test_network_volume_transfer.py:17REAL_WAIT = nvt._waitsits between imports, soimport runpod.erroron line 18 trips pylintwrong-import-position. Move it below the imports.network_volume_transfer.py:193Put the reason inside theexcept(see Existing feedback). That clears both CodeQL threads.
Simplification opportunities
None worth a change in this round.
Verified clean
- Stop-aware retries. After
stopis set,_waitreturns at once. The next attempt goes through_request, which raises_Stopped._Stoppedis not retryable, so the loop ends. The cleanup delete deliberately skips_requestandstop, so it still runs after Ctrl-C. - Directory fsync in
_write_json_atomicreturns early where a directory cannot be opened (Windows). It runs once per 64 MiB chunk or part, so the cost is negligible. _check_local_namescompares folded file keys against folded parent keys, soanext toA/bis refused before anyget_object._is_folderreports the original 400 when the listing itself fails, and FakeS3 acceptsMaxKeys.test_unresolvable_endpoint_fails_before_the_transferuses.invalid, which never resolves.monkeypatch.undo()drops only this test's autouse patches.
Questions for the author
- Is
_check_endpoint_resolvesmeant to run whenendpoint_urlis passed explicitly (for example a local S3 stand-in or a proxy setup)?
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 34 in / 10,469 out / 1,100,440 cache read / 71,684 cache write = 1,182,627. Estimated cost: $1.00 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (1-hour cache TTL assumed; $0.79 at 5-minute TTL). Counted before posting. Round 3 of this PR. This is automated assistance; a human owns the final call.
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 4
Verdict: Needs changes. The proxy fix and the CodeQL note are done, and all five unit-test jobs pass at 8c260d2. The flaky Ctrl-C test is unchanged, and e2e still fails.
Approval gate: 0 Critical, 1 High, 3 Medium+Low → not approved: e2e / validation still fail (external 401, carried).
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R1-1 | e2e / validation fail |
High | Not addressed (external) | Same GraphQL request failed: 401 at 8c260d2 (log). validation fails only because e2e failed. |
| R3-1 | test_ctrl_c_stops_files_in_flight is timing-dependent |
High → Medium | Not addressed | Test unchanged (tests/test_network_volume_transfer.py:1165). It passed on all five interpreters this run, so CI is not red, but the race is still there. Downgraded to Medium because the failure is intermittent. |
| R3-2 | test_retry_backoff_ends_when_stopped cannot fail on revert |
Low | Not addressed | :1021-1039 still runs with the autouse _sleep no-op. |
| R3-3 | DNS pre-check blocks proxy-only networks | Low | Fixed | network_volume_transfer.py:956-957 skips the lookup when getproxies() has an entry for the scheme and proxy_bypass(host) is false. That matches how botocore picks a proxy (get_environ_proxies). Tested by test_endpoint_check_skipped_behind_a_proxy. |
| R3 nit | REAL_WAIT between imports (wrong-import-position) |
Nit | Not addressed | :17-19. REAL_ENDPOINT_CHECK now joins it. |
| R3 nit | CodeQL except OSError: pass |
Nit | Fixed | :193-195 now has the reason inside the handler. The return before finally still closes fd. |
| R1 nit | One bad key fails the whole folder | Nit | Not addressed | Still fine as a nit. |
| R3 Q | Should the check run for an explicit endpoint_url? |
Question | Open | Still runs for any endpoint (:990). Only the author can answer. |
Existing feedback
- github-code-quality and github-advanced-security on
network_volume_transfer.py:193: Already addressed at8c260d2. Both threads are resolved. - No new human comments since round 3.
PR description vs code
No body claims changed in this push. The round 3 table still applies, except "1765 passed": unit tests are green on all interpreters at this head.
Findings
High
e2e/validation(R1-1, carried). External 401 from the e2e API key. It needs a green rerun or an owner decision to merge past it.
Medium
- Flaky
test_ctrl_c_stops_files_in_flight(R3-1, carried).tests/test_network_volume_transfer.py:1165. See the existing thread for a deterministic assertion.
Low
test_retry_backoff_ends_when_stoppedcannot fail on revert (R3-2, carried).tests/test_network_volume_transfer.py:1039.- The new
endpoint_checkfixture still reads the host's proxy settings.tests/test_network_volume_transfer.py:916. It clears the proxy env vars, but on macOS and Windowsgetproxies()falls back to system settings when the env is empty. On a developer Mac with a system proxy, the lookup is skipped andtest_unresolvable_endpoint_fails_before_the_transferfails. CI is Linux, so it stays green. Stuburllib.request.getproxiesinstead (inline).
Nits
tests/test_network_volume_transfer.py:17-18MoveREAL_WAITandREAL_ENDPOINT_CHECKbelowimport runpod.error(R3 nit).
Simplification opportunities
None.
Verified clean
- Proxy skip.
getproxies().get(parsed.scheme or "https")picks the same key botocore uses for anhttps://endpoint.NO_PROXYhosts still get the lookup throughproxy_bypass. - New tests can fail. In the proxy test,
getaddrinforaises. A missing skip would surface asNetworkVolumeTransferError. - No real DNS in tests.
socket.getaddrinfois patched on thesocketmodule, whichnetwork_volume_transferimports as a module (:20). The real.invalidlookup andmonkeypatch.undo()are gone.
Questions for the author
- Should
_check_endpoint_resolvesrun whenendpoint_urlis passed explicitly (carried from round 3)?
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 18 in / 4,235 out / 439,534 cache read / 49,146 cache write = 492,933. Estimated cost: $0.57 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (1-hour cache TTL assumed; $0.42 at 5-minute TTL). Counted before posting. Round 4 of this PR. This is automated assistance; a human owns the final call.
- a request re-checks the stop flag after it gets a slot, so nothing starts after an interrupt lands - ctrl-c test blocks later parts on the stop event and asserts a bound that holds under any scheduling - backoff test restores the real sleep so a revert is caught - skip the endpoint dns check for an explicit endpoint_url - move the saved helpers below the imports
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 5
Verdict: Needs changes. All three round 4 code findings on the Ctrl-C and backoff tests are fixed, and the five unit-test jobs pass at 0c8b4d3. e2e still fails on the external 401.
Approval gate: 0 Critical, 1 High, 2 Medium+Low → not approved: e2e / validation still fail (external 401, carried).
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R1-1 | e2e / validation fail |
High | Not addressed (external) | Same GraphQL request failed: 401 while runpod_flash deploys at 0c8b4d3 (log). validation fails only because e2e failed. The body explains it. |
| R3-1 | test_ctrl_c_stops_files_in_flight is timing-dependent |
Medium | Fixed | Parts after the first block in _stop.wait(5) while holding a slot, and _request re-checks _stop after acquiring the slot (network_volume_transfer.py:333-335). <= 3 (tests/test_network_volume_transfer.py:1176) is now a real bound. |
| R3-2 | test_retry_backoff_ends_when_stopped cannot fail on revert |
Low | Fixed | nvt._sleep restored to time.sleep (:1028). A revert would sleep at least 1 s and fail < 0.5. |
| R4-1 | endpoint_check fixture still reads system proxy settings |
Low | Not addressed | :911-926 clears env vars only. On macOS or Windows, getproxies() still reads system settings. The thread was resolved without a change. |
| R3 nit | REAL_WAIT between imports |
Nit | Fixed | :16-20 |
| R3 Q | Should the check run for an explicit endpoint_url? |
Question | Answered | No: :992-995 checks only the derived datacenter endpoint. The new branch has no test (new Low). |
| R1 nit | One bad key fails the whole folder | Nit | Not addressed | Still fine as a nit. |
Existing feedback
- No new human or bot comments since round 4. All threads are resolved. Replies posted on the three fixed threads and on R4-1.
PR description vs code
| Claim | Status | Where |
|---|---|---|
| A datacenter whose endpoint doesn't resolve fails before the transfer | ✅ | :992-995 (derived endpoint only) |
| Check skipped behind an https proxy | ✅ | :959-960 |
| Ctrl-C stops the files in flight | ✅ | _run_pool sets _stop (:266-268), _request checks it before and after the slot |
CI note: e2e 401 is external, validation fails only because of it |
✅ | Matches the log at this head |
| "1765 passed", 90 cases, live checks | Unit jobs green on 3.10 to 3.14. Counts and live checks not re-run by this review. |
Findings
High
e2e/validation(R1-1, carried). External 401 from the e2e API key. It needs a green rerun or an owner decision to merge past it.
Low
- The explicit-endpoint skip has no test.
runpod/network_volume_transfer.py:992. The autouse fixture stubs the check, so dropping or inverting theifkeeps the suite green (inline). endpoint_checkfixture reads system proxy settings (R4-1, carried).tests/test_network_volume_transfer.py:911. Patchurllib.request.getproxiesto return{}.
Nits
tests/test_network_volume_transfer.py:1154Useimport threadinginstead of__import__("threading").
Simplification opportunities
None.
Verified clean
- Slot re-check.
_Stoppedis raised insidewith self._slots, so the slot is released._Stoppedis not retryable, and the cleanup delete does not go through_request, so it still runs after Ctrl-C. - Ctrl-C test bound.
KeyboardInterruptis aBaseException, so_with_retries(except Exception) does not retry it. The maximum is the interrupted part, the other slot holder, and one worker that takes the freed slot before_stopis set. Without the re-check, a queued worker would make a fourth call. - Backoff test. With the real
_waitand_sleep,stop.waitreturns at once afterstopis set. - Import order.
import runpod.erroris with the other imports; theREAL_*captures still run before the autouse fixture.
Questions for the author
None.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 32 in / 8,187 out / 1,021,419 cache read / 68,671 cache write = 1,098,309. Estimated cost: $0.92 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (1-hour cache TTL assumed; $0.71 at 5-minute TTL). Counted before posting. Round 5 of this PR. This is automated assistance; a human owns the final call.
|
Claude reviews are quite noisy so im not sure the state of this PR, but wanted to chime in and say we should not introduce a |
|
I also dislike the global functions. They should be under the NetworkVolume class. |
upload and download are NetworkVolume methods instead of top-level functions, and the cli commands live under rp volume instead of a new rp network-volume group. the volume is found by id or name with the api key, so --datacenter is gone.
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 6
Verdict: Needs changes. The move onto NetworkVolume and rp volume matches what @KAJdev asked for. The new volume lookup hand-rolls pagination the repo already has, and e2e still fails on the external 401.
Approval gate: 0 Critical, 1 High, 3 Medium+Low → not approved: e2e / validation still fail (external 401, carried).
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R1-1 | e2e / validation fail |
High | Not addressed (external) | Same GraphQL request failed: 401 while runpod_flash deploys at f1fa981 (log). Unit jobs pass on 3.10 to 3.14. |
| R5-1 | Explicit-endpoint skip has no test | Low | Not addressed | runpod/network_volume_transfer.py:1006. No test calls _make_transfer with endpoint_url and the real check. |
| R4-1 | endpoint_check fixture reads system proxy settings |
Low | Not addressed | tests/test_network_volume_transfer.py still clears env vars only; getproxies is not patched. |
| R5 nit | __import__("threading") |
Nit | Not addressed | tests/test_network_volume_transfer.py:1188 |
| R1 nit | One bad key fails the whole folder | Nit | Not addressed | Still fine as a nit. |
Existing feedback
- @KAJdev, conversation: "should be under
volume, notnetwork-volume". Verified fixed atf1fa981:runpod/rp_cli/volume.pydefines thevolumegroup, mounted inrunpod/rp_cli/main.py:1092-1098;network-volumeis gone. - @KAJdev, conversation: "global functions should be under the
NetworkVolumeclass". Verified fixed atf1fa981:NetworkVolume.upload/.download(runpod/apps/volume.py:87-164); therunpod.upload_to_network_volume/download_from_network_volumeexports and aliases are removed. The module-levelnvt.upload/nvt.downloadremain as private backers.
PR description vs code
| Claim | Status | Where |
|---|---|---|
rp volume upload/download <volume> takes id or name |
✅ | runpod/rp_cli/volume.py:96-126, _resolve_volume via find_by_id_or_name |
from runpod import NetworkVolume then .upload / .download |
✅ | runpod/__init__.py:55, runpod/apps/volume.py:87,129 |
Volume and datacenter looked up through REST with RUNPOD_API_KEY |
✅ | runpod/network_volume_transfer.py:933-958 |
| Shares the group #638 adds; second to merge folds in | ✅ | #638 also adds runpod/rp_cli/volume.py; expect a conflict there. |
| CI note on the external 401 | ✅ | Matches the log at this head |
| Live checks | The body says they ran before this refactor. The new list-based lookup is not live-checked. |
Findings
High
e2e/validation(R1-1, carried). External 401 from the e2e API key. Needs a green rerun or an owner decision to merge past it.
Medium
_resolve_volumere-implements REST pagination with weaker checks —runpod/network_volume_transfer.py:940.run_rest_paginated_request_async(used bylist_network_volumes,runpod/apps/api.py:446) already retries each page, validates the shape, and rejects any repeated cursor. The new loop only catches an immediate repeat and then returns a partial list, so a name on a later page reads as "not found", or a duplicate name escapes the ambiguity check. An A→B→A cursor cycle loops forever. Reuse it through the existing sync bridge:block(run_rest_paginated_request_async("/v2/network-volumes", "networkVolumes"))(runpod/apps/context.py:120), which is also safe inside a running event loop (inline).
Low
- Explicit-endpoint skip has no test (R5-1, carried).
runpod/network_volume_transfer.py:1006. endpoint_checkfixture reads system proxy settings (R4-1, carried). Patchurllib.request.getproxiesto return{}.
Nits
tests/test_network_volume_transfer.py:1188__import__("threading")(R5, carried).
Simplification opportunities
- Replace the pagination loop in
_resolve_volumewith the shared helper (about 9 lines removed, and the paging tests become unnecessary). Same PR.
Verified clean
- CLI wiring.
volumeis mounted in_mount_groups; no othervolumecommand exists inrunpod/cliorrunpod/rp_cli. Tests that patchnvt.uploadstill intercept, becauseNetworkVolume.uploadreadsnvt.uploadat call time. NetworkVolume(reference, create=False). The transfer methods never create a volume; the docstring says the volume must exist, and_resolve_volumeraises "not found".- Removed
--datacenter. Docs, CLI help, error text, and tests no longer mention it._check_endpoint_resolvestext was updated to match. - Exports.
__all__derives from_LAZY_ATTRS, sotest_no_module_level_transfer_exportscan fail if the names come back. - CHANGELOG. The entry is under Unreleased, so removing the unreleased module-level functions breaks no released API.
Questions for the author
- Could you rerun one live upload by volume name after this refactor? It is the first time the transfer path depends on the
/v2/network-volumeslist shape.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 34 in / 7,685 out / 1,089,395 cache read / 72,788 cache write = 1,169,902. Estimated cost: $0.95 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (1-hour cache TTL assumed; $0.74 at 5-minute TTL). Counted before posting. Round 6 of this PR. This is automated assistance; a human owns the final call.
also pin the explicit-endpoint branch of the dns check with a test and keep system proxy settings out of the endpoint_check fixture.
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 7
Verdict: Approved. Every round 6 finding is fixed at b9c8f92, and every check passes, including e2e and validation.
Approval gate: 0 Critical, 0 High, 1 Medium+Low → approved.
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R1-1 | e2e / validation fail |
High | Fixed | Both pass at b9c8f92 (e2e, 2m45s, a real run). |
| R6-1 | _resolve_volume hand-rolls pagination |
Medium | Fixed | runpod/network_volume_transfer.py:939 now calls block(run_rest_paginated_request_async("/v2/network-volumes", "networkVolumes")). Per-page retry, shape checks, and repeated-cursor rejection come from runpod/api/rest.py:137. |
| R5-1 | Explicit-endpoint skip has no test | Low | Fixed | test_explicit_endpoint_skips_the_dns_check restores the real check, fails DNS, and asserts both sides: no error with endpoint_url, "does not resolve" without it. |
| R4-1 | endpoint_check fixture reads system proxy settings |
Low | Fixed | The fixture swaps urllib.request.getproxies / proxy_bypass for the _environment variants. _check_endpoint_resolves imports them at call time (:960), so the patch applies, and the proxy test still works through HTTPS_PROXY. |
| R5 nit | __import__("threading") |
Nit | Fixed | Now import threading inside the test. |
| R1 nit | One bad key fails the whole folder | Nit | Unchanged | Fine as is. |
Existing feedback
- @KAJdev, conversation (
volumegroup,NetworkVolumemethods): verified fixed in round 6, unchanged since. - All review threads are resolved. Replies confirming the round 6 fixes are posted in the R4-1, R5-1, nit, and R6-1 threads.
PR description vs code
| Claim | Status | Where |
|---|---|---|
Volume looked up by id or name through REST with RUNPOD_API_KEY |
✅ | runpod/network_volume_transfer.py:933-950, through the shared paginated helper |
CI: "e2e fails with GraphQL request failed: 401" |
❌ | e2e and validation pass at b9c8f92. |
| "collects 95 cases" | One test was added, so 96 now. | |
Live checks ran before the NetworkVolume refactor |
✅ | Disclosed. See Questions. |
Findings
Low
- The PR body's CI note is stale. The note says
e2eandvalidationfail on the external 401. Both pass at head. Drop the CI paragraph and update the case count to 96.
Verified clean
- Helper reuse.
blockruns the coroutine on the shared loop thread (runpod/apps/context.py:120), so it is safe from sync callers and from inside a running loop.find_by_id_or_namegets the full list, so the not-found and ambiguous-name checks see every page. - Error surfacing.
QueryErrorfrom a bad page shape or a repeated cursor reaches the CLI'sexcept Exception(runpod/rp_cli/volume.py:86) and prints without the resume hint, which is right for a deterministic failure. - Test patching.
_list_volumespatchesrunpod.api.rest.run_rest_paginated_request_async._resolve_volumeimports it at call time, so the patch takes effect. The fake's(path, collection, **kwargs)signature matches the call. - Imports.
List,Dict,Any, andTupleare still used after the removed loop. - CI. CodeQL,
test (3.10)totest (3.14),e2e, andvalidationall pass.
Questions for the author
- Still open from round 6: could you rerun one live upload by volume name? The transfer path now depends on the
/v2/network-volumeslist shape and the shared helper's strict pagination checks.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 26 in / 6,565 out / 762,883 cache read / 62,605 cache write = 832,079. Estimated cost: $0.79 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (1-hour cache TTL assumed; $0.60 at 5-minute TTL). Counted before posting. Round 7 of this PR. This is automated assistance; a human owns the final call.
_Transfer's long upload and download methods are split into one step each, with _Upload and _Download holding a file's transfer state. the module-level upload/download pass-throughs are gone; NetworkVolume calls _make_transfer directly. fixes: the sidecar's .tmp name now counts as a download name collision, and a folder failure is resumable only when every file failure is.
matches runpodctl: a stop between emptying the partial and saving fresh state left the old sidecar vouching for discarded chunks.
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 8
Verdict: Approved. The refactor in b085cb1 keeps behavior, and the three intended fixes in b085cb1 and a4bc86a are correct and tested. Every check passes at a4bc86a.
Approval gate: 0 Critical, 0 High, 1 Medium+Low → approved.
The watcher triggered this round on b085cb1. a4bc86a landed during the review, so this round covers both commits and posts at a4bc86a.
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R7-1 | PR body's CI note is stale | Low | Not addressed | The body still says e2e and validation fail on a 401. Both pass at a4bc86a (e2e). The case count was updated. |
| R1 nit | One bad key fails the whole folder | Nit | Unchanged | Fine as is. |
All earlier items (R1 to R6) stay fixed. The moved code was re-read for them below.
Existing feedback
- No open review threads. No new human comments since round 7.
- @KAJdev's request (
volumegroup,NetworkVolumemethods) still holds.NetworkVolume.upload/downloadnow call_make_transferdirectly, and the module-levelupload/downloadpass-throughs are gone.
PR description vs code
| Claim | Status | Where |
|---|---|---|
x next to x.rpdownload is refused (now including the sidecar .tmp) |
✅ | TRANSFER_SUFFIXES, used by _check_transfer_names and _is_download_leftover |
| Resume hint only for interruptions and transient failures | ✅ | Folder failures now resume only when every file failure does (_run_files, all(resumable)) |
| Downloads keep finished chunks in a sidecar pinned to key, size, ETag | ✅ | _Download._resumes / load_or_init |
CI: "e2e fails with GraphQL request failed: 401" |
❌ | Passes at a4bc86a. See R7-1. |
| Live checks ran on round-2 code | ✅ | Disclosed. See Questions. |
Findings
Low
- R7-1 (carried): drop the stale CI paragraph from the PR body. No code change needed.
Nits
runpod/apps/volume.py:117,:147:NetworkVolumenow callsnvt._make_transferacross modules. It is the module's real entry point, so a public name (make_transfer) would say so. The repo does not run pylint in CI, so nothing flags it today.
Verified clean
- Refactor is behavior-preserving. I compared every moved block in
b085cb1againstb9c8f92. A separate pass did the same and found no unintended change:- single and folder upload: key derivation, leftover and unchanged skips, folder-400 hint
- multipart: resume, abort on mismatch,
LookupErrorrestart, MD5 part check, range reread per retry, changed-file abort, completed-anyway, state removal - download: destination rules (trailing
/still passed through as a str), ETag-change error, short-read retry, mtime stamp _emitsummary counting, CLI resume hint, exit 130 on Ctrl-C
- No dangling references to the removed
nvt.upload/nvt.downloadindocs/,CHANGELOG.md, README, orrunpod/__init__.py. The CLI tests now patchrunpod.NetworkVolumeinstead. a4bc86afixes a real corruption path. Before it, a stale sidecar (wrong partial size, or a changed ETag) survived whileload_or_inittruncated the partial to full size. A stop at that point left a zero-filled partial that the sidecar marked complete, and the next run would have renamed it into place. Removing the sidecar first closes this.test_discarded_chunks_drop_the_sidecar_firstfails without the fix.all(resumable): tested both ways bytest_folder_failure_is_resumable_only_when_every_failure_is. Ctrl-C is unaffected:KeyboardInterruptis not caught byguarded.- Chunk index filter (
0 <= i < count) also stops a negative saved index from inflating thereusebyte count. - CI: CodeQL,
test (3.10)totest (3.14),e2e, andvalidationpass ata4bc86a.
Questions for the author
- Still open from round 6: could you rerun one live upload and download by volume name? The latest live pass predates the
NetworkVolumemove and this refactor.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 64 in / 11,004 out / 1,955,655 cache read / 159,574 cache write = 2,126,297. Estimated cost: $1.89 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (1-hour cache TTL assumed; $1.41 at 5-minute TTL). Counted before posting. Round 8 of this PR. This is automated assistance; a human owns the final call.
a symlinked .rpdownload, sidecar, or sidecar temp file aimed the write at another file; python has no root confinement, so it could be any file. matches runpodctl.
There was a problem hiding this comment.
most of this file is utilities that arent specific to network volume transfering. Please condense
There was a problem hiding this comment.
Partly addressed at a5e18c3: the module went from 1,145 to 925 lines. The custom retry stack, the DNS pre-check, and two of the three pool helpers are gone. The generic helpers that remain (_read_json/_write_json, _md5_hex, _error, _pages, _run) have no shared equivalent in the repo to move to. Whether this is condensed enough is your call. Note that the condense dropped three earlier fixes (round 9 H1, M1, M2).
There was a problem hiding this comment.
Round 10 note: the module grew from 925 to 1,100 lines in a8431ea. The growth is the round 9 fixes it restored (slot-held reads, endpoint check, retry hook, folder fsync) plus key, login and endpoint handling. It adds no new generic helpers. Whether this is condensed enough is still your call.
There was a problem hiding this comment.
dont edit this file. this is for release please
There was a problem hiding this comment.
Verified fixed at a5e18c3: CHANGELOG.md now matches main (git diff origin/main c29e0d2 -- CHANGELOG.md is empty). Leaving this thread for you to resolve.
botocore's standard retry mode replaces the custom retry and backoff stack, the endpoint dns pre-check is gone (the datacenter comes from the volume record), one thread-pool helper replaces three, and one-use helpers are folded into their callers. CHANGELOG.md is back to main's: release-please owns it.
botocore's standard mode retries 500/502/503/504 only, so the switch to it stopped retrying the 524 a slow complete or upload gets. a needs-retry hook ahead of botocore's adds it.
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 9
Verdict: Needs changes. The symlink fix (f7299a9), the CHANGELOG revert, and the 524 hook (c29e0d2) are correct. The condense refactor (a5e18c3) dropped three earlier fixes.
Approval gate: 0 Critical, 1 High, 5 Medium+Low → not approved: one High. Round 9 withdraws the round 7 and round 8 approvals.
The watcher triggered this round on a5e18c3. c29e0d2 landed during the review, so this round covers f7299a9, a5e18c3 and c29e0d2, and posts at c29e0d2.
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R7-1 | PR body's CI paragraph is stale | Low | Not addressed | The body still says e2e and validation fail on a 401. Every check passes at c29e0d2. |
| R1-1 | Ctrl-C does not stop parts in flight | Medium | Fixed | _call checks _stop inside the slot (:224-228), and _run sets it (:238-240). |
| R2-2 | Backoff sleep ignores the stop event | Low | Regressed | botocore now sleeps inside _call. See M2. |
| R2-1 | Mistyped datacenter fails slowly and says "resume" | Medium | Regressed | The DNS pre-check is gone. See M1. |
| R1-x | One concurrency limit covers every request | (body claim) | Regressed | Body reads and part reads moved outside the slot. See H1. |
| R1 nit | One bad key fails the whole folder | Nit | Unchanged | Fine as is. |
Everything else from R1 to R8 was re-read at c29e0d2 and stays fixed. That covers multipart resume, abort on change, completed-anyway, the sidecar order, name collisions, _safe_target, the skip rules, and the leftover skip.
Existing feedback
- @KAJdev on
CHANGELOG.md: Verified fixed. It matchesmainatc29e0d2. Reply in thread. - @KAJdev on
runpod/network_volume_transfer.py:1(condense): Partly addressed. The module went from 1,145 to 925 lines. The remaining generic helpers (_read_json/_write_json,_md5_hex,_error,_pages,_run) have no shared equivalent in the repo to move to. @KAJdev decides whether this is enough. The condense also caused H1, M1 and M2. Reply in thread. - @KAJdev's conversation requests (
volumegroup,NetworkVolumemethods): still met.
PR description vs code
| Claim | Status | Where |
|---|---|---|
| One concurrency limit covers every S3 request | ❌ | Body and part reads run outside the slot (H1) |
| Ctrl-C lets in-flight requests finish and starts no new ones | True, but "finish" now includes up to 10 botocore attempts (M2) | |
| Retries: botocore standard mode plus a 524 hook | ✅ | :909, :925-935, test_client_retries_the_proxy_524 |
A symlinked .rpdownload, sidecar or temp file is refused |
✅ | :715-723, tested for all three suffixes |
| CLI prints the resume hint only when a rerun gets past the error | ❌ | Not for a volume in a datacenter without the S3 API (M1) |
| "Each fix has a test that fails when the fix is removed" | The tests for the dropped fixes were removed with them | |
CI: e2e fails on a 401 |
❌ | R7-1 |
| 96 cases, 1771 passed | ✅ | Matches the added test |
Findings
High
- H1: Downloaded bodies and upload parts are buffered outside the concurrency limit.
network_volume_transfer.py:782,:609. Ata4bc86athe slot covered the wholeget_objectplusBody.read(), and the part read insideupload_part. Now the slot covers only the request until headers arrive. In a folder transfer, each ofconcurrencyfile workers runs its own pool ofconcurrencychunk workers. So up to concurrency² 64 MiB buffers are live at once (parts can be up to 500 MiB). With the default 4, that is 1 GiB. With--concurrency 16, it is 16 GiB. Fix: do the read inside the slot (for example, a_callvariant that runs a callable under_slots).
Medium
- M1: A volume in a datacenter without the S3 API waits about a minute, then says "re-run to resume".
:909. The S3 API covers only some datacenters (docs).s3api-eu-se-1.runpod.ioands3api-ca-mtl-1.runpod.ioreturn NXDOMAIN. botocore retries the resultingEndpointConnectionError10 times (up to about 111 s of backoff).is_resumablethen returns True throughBotoConnectionError(:108), so the CLI hint is false. The removed_check_endpoint_resolves(fix for R2-1, with its proxy carve-out from R3) handled this. Restore it, or check the datacenter against a known list before building the client. - M2: Ctrl-C can wait minutes during an outage.
:228. botocore runs its retries and backoff sleeps inside_call, and nothing checks_stopuntil it returns.pool.shutdown(:243) waits for that. Withconnect_timeout=60and 10 attempts, a dead network can hold Ctrl-C for about 10 minutes. The 524 hook's sleep has the same gap. This reopens R2-2 at a larger scale. Fix: lowertotal_max_attemptsand retry outside botocore withstop.wait, or register aneeds-retryhandler that returns None once_stopis set.
Low
- L1: botocore's retry quota is shared across every worker.
:909. Standard mode starts each client with 500 tokens. A timeout or connection retry costs 10, and a success refunds 1 (quota.py,standard.py_TIMEOUT_RETRY_REQUEST). About 50 timeout retries during a brief outage drain it. After that, every failure gets one attempt for the rest of a long folder transfer. Files stay resumable, so this is Low. Note it in the body, or keep a per-call retry. - L2:
_write_jsonno longer fsyncs the parent directory.:128-137.a4bc86asynced the directory afteros.replace, so the rename survived a power loss. The commit message does not mention dropping it. Restore it, or say why it is not needed. - R7-1 (carried): remove the stale CI paragraph from the PR body.
Nits
:856-862black moved the# pylint: disable=import-outside-toplevelcomments onto the closing)lines, so they no longer apply to thefromline.:720the "exists but is not a regular file" branch (a directory atx.rpdownload) has no test.runpod/apps/volume.py:117,:147(carried)nvt._make_transferis the module's real entry point. A public name would say so.
Simplification opportunities
- The condense removed about 220 lines. H1 and M2 should be fixable inside
_calland_runwithout bringing back the old three-pool stack.
Verified clean
f7299a9: the refusal runs before any write (onlymkdirprecedes it). Without it, each of the three parametrized cases fails: the partial truncates the target, the sidecar unlink lets the download succeed, and the temp write goes through the link.c29e0d2:register_first("needs-retry.s3", ...)returns a delay only for 524 underMAX_ATTEMPTS. Otherwise it returns None and botocore's handler decides. The test uses a real HTTP server and fails without the hook.- Removed tests: each one maps to removed code (
_with_retries,_wait,_check_endpoint_resolves). No references remain inrunpod/,tests/ordocs/. _resolve_volumehelpers exist with matching signatures (run_rest_paginated_request_async,block,find_by_id_or_name(noun=, error=)).CHANGELOG.mdequalsorigin/main.- CI: CodeQL,
test (3.10)totest (3.14),e2eandvalidationall pass atc29e0d2.
Questions for the author
- Still open from round 6: could you rerun one live upload and download by volume name? The latest live pass predates the
NetworkVolumemove, this condense, and the switch to botocore retries.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 112 in / 17,170 out / 3,490,220 cache read / 240,744 cache write = 3,748,246. Estimated cost: $2.97 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (round 8 rate card; 1-hour cache TTL assumed; $2.25 at 5-minute TTL). Counted before posting. Round 9 of this PR. This is automated assistance; a human owns the final call.
Superseded by Claude review, round 9
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved data-integrity and resource-limit issues can cause incorrect transfers or excessive memory and request usage.
5 open issues
What changed in this PR
Adds resumable network-volume transfers through the S3-compatible API, exposed through NetworkVolume methods and new rp volume commands.
Changes:
- Implements multipart upload recovery, chunked download recovery, unchanged-file skipping, and path-safety checks.
- Adds Python and CLI interfaces with credentials, concurrency controls, progress reporting, and resume hints.
- Documents usage and adds transfer, recovery, and failure-handling tests.
| File | Description |
|---|---|
tests/test_network_volume_transfer.py |
Adds fake-S3 tests for transfers, recovery, safety, and CLI behavior. |
runpod/rp_cli/volume.py |
Defines upload/download commands and progress reporting. |
runpod/rp_cli/main.py |
Registers the volume command group. |
runpod/network_volume_transfer.py |
Implements resumable transfers and S3 client configuration. |
runpod/apps/volume.py |
Exposes upload/download methods on NetworkVolume. |
docs/cli/references/command_line_interface.md |
Documents credentials, commands, resume behavior, and Python usage. |
Review effort: Balanced
…lures rp volume upload/download now asks to log in when no api key is set, lets you pick a volume when VOLUME is left out, and asks for s3 keys (offering to save them in ~/.runpod/config.toml) when none are set. in a script each of these fails with a message naming the fix. rejected keys and a datacenter without the s3 api get their own non-resumable errors. also from review: a request and its body or part read share one slot, cleanup takes a slot too, retries are decided by one stop-aware hook (capped backoff, 524 included, no shared quota), an edit during a multipart complete removes the object, symlinked download subfolders are refused, state writes sync their folder, and an opt-in live e2e test round-trips a temporary volume.
| return | ||
| try: | ||
| os.fsync(fd) | ||
| except OSError: |
There was a problem hiding this comment.
Disagree: the empty except is intended. The _sync_dir docstring says the fsync is best effort. A one-line comment in the except would clear the alert.
|
|
||
| def saved_s3_keys() -> Tuple[Optional[str], Optional[str]]: | ||
| """the s3 keys saved in ~/.runpod/config.toml for the current profile.""" | ||
| import runpod # pylint: disable=import-outside-toplevel,cyclic-import |
There was a problem hiding this comment.
Disagree: style only. The local imports avoid an import cycle with runpod. No behavior impact.
| ) | ||
|
|
||
|
|
||
| def _invoke_transfer(transfer_method, source, target, opts, progress): |
There was a problem hiding this comment.
Disagree: false positive. Every path in _invoke_transfer returns, calls sys.exit(130), or raises ClickException.
| def _ensure_logged_in(ctx) -> None: | ||
| """offer the browser login when no api key is set; the volume lookup | ||
| needs one.""" | ||
| import runpod |
There was a problem hiding this comment.
Disagree: style only. Same local-import pattern as network_volume_transfer.py:998. No behavior impact.
|
|
||
| def _prompt_s3_keys(): | ||
| """ask for the s3 keys, offering to save them for next time.""" | ||
| import runpod |
There was a problem hiding this comment.
Disagree: style only. Same local-import pattern as network_volume_transfer.py:998. No behavior impact.
| return | ||
| try: | ||
| os.fsync(fd) | ||
| except OSError: |
| ) | ||
|
|
||
|
|
||
| def _invoke_transfer(transfer_method, source, target, opts, progress): |
|
|
||
| def saved_s3_keys() -> Tuple[Optional[str], Optional[str]]: | ||
| """the s3 keys saved in ~/.runpod/config.toml for the current profile.""" | ||
| import runpod # pylint: disable=import-outside-toplevel,cyclic-import |
| def _ensure_logged_in(ctx) -> None: | ||
| """offer the browser login when no api key is set; the volume lookup | ||
| needs one.""" | ||
| import runpod |
|
|
||
| def _prompt_s3_keys(): | ||
| """ask for the s3 keys, offering to save them for next time.""" | ||
| import runpod |
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 10
Verdict: Approved. a8431ea fixes every open finding from round 9 and all five Copilot findings, and adds the prompts @KAJdev asked for. One Medium and two Lows remain, all in the new CLI key flow.
Approval gate: 0 Critical, 0 High, 3 Medium+Low → approved. The approval does not override @KAJdev's change request; they decide whether the prompts meet it.
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R9-H1 | Bodies and part reads buffered outside the concurrency limit | High | Fixed | _request holds the slot over work() (network_volume_transfer.py:269-281). _read_range and _read_and_send run inside it. test_body_reads_hold_a_request_slot, test_part_reads_hold_a_request_slot |
| R9-M1 | Datacenter without the S3 API waits, then says "resume" | Medium | Fixed | _check_endpoint (:1031-1050) with the proxy carve-out. The error is non-resumable. test_datacenter_without_the_s3_api_fails_fast |
| R9-M2 | Ctrl-C waits through botocore's retries | Medium | Fixed | total_max_attempts: 1 (:1061), and _retry_delay waits on _stop (:309). Ctrl-C now waits for at most one attempt in flight. test_ctrl_c_ends_a_retry_wait |
| R9-L1 | Shared botocore retry quota | Low | Fixed | botocore's own retries are off. Our hook runs first and returns before the standard handler would spend quota. |
| R9-L2 | _write_json no longer fsyncs the folder |
Low | Fixed | _sync_dir (:170-182), best effort. test_state_writes_sync_the_folder |
| R7-1 | Stale CI paragraph in the body | Low | Fixed | Body says all checks pass. True at a8431ea. |
| R9 nit | pylint disables on the closing ) |
Nit | Fixed | Now on the from lines (:957-973, :999) |
| R9 nit | No test for a folder at x.rpdownload |
Nit | Fixed | Test at tests/test_network_volume_transfer.py:1200 |
| R6 nit | nvt._make_transfer is the real entry point |
Nit | Unchanged | Fine as is |
Existing feedback
- @KAJdev review (needs prompts and better failure cases): Addressed in
a8431ea. The CLI now asks to log in, lists volumes when VOLUME is left out, and asks for S3 keys. It also explains rejected keys and unsupported datacenters. @KAJdev confirms against the screenshots. - @KAJdev on
network_volume_transfer.py:1(condense): Needs @KAJdev. The module grew from 925 to 1,100 lines. The growth is the round 9 restorations plus key, login and endpoint handling. No new generic helpers. - @KAJdev on
CHANGELOG.md: still matchesmain. Thread left for @KAJdev. - Copilot (5 threads at
c29e0d2): all verified fixed ata8431ea. Replied and resolved: part read slot, cleanup slot (_cleanup), edit during complete (_keep_if_unchangedafter_complete,:614-615), body read slot, and symlinked subfolders (_safe_target,:941-948).
PR description vs code
| Claim | Status | Where |
|---|---|---|
| One concurrency limit; request and body or part read share a slot; cleanup takes a slot | ✅ | :269-293 |
Retries: botocore retries off, one needs-retry hook, 10 tries, 524 included, Ctrl-C ends the wait |
✅ | :295-310, :1061, :1099 |
| S3 keys: flags, then env, then saved profile; a pair never mixes sources | ✅ | find_s3_keys (:1007-1019) |
| Prompts: login, then volume, then S3 keys; script-mode errors | ✅ | rp_cli/volume.py:120-218, CLI tests |
| Rejected keys: message, no resume hint | Message is as quoted, but it names only the env vars (M1) | |
EU-SE-1 does not support the s3 api yet (...) |
The real message adds ", or <host> is unreachable from this network" (:1049). Nit |
|
| CI: all checks pass | ✅ | All 10 checks pass at a8431ea |
| e2e is opt-in and round-trips a multipart file and a folder | ✅ | tests/e2e/test_network_volume_transfer_live.py:20-24, :52-74 |
Findings
Medium
- M1: Mistyped saved keys leave the CLI stuck with an error that points at env vars.
network_volume_transfer.py:108,rp_cli/volume.py:187. The key prompt saves by default, and the secret is typed hidden. After a typo,find_s3_keysreturns the saved pair on every run, so the prompt never comes back._keys_rejectedthen tells the user to checkRUNPOD_S3_ACCESS_KEYandRUNPOD_S3_SECRET_KEY, which they never set. Nothing mentions~/.runpod/config.toml. Fix: name every key source in the message ("flags, env vars, or s3_* in ~/.runpod/config.toml"). Better: in the CLI, on a rejection with saved keys, offer to enter new ones.
Low
- L1: The CLI asks for S3 keys before it checks the volume.
rp_cli/volume.py:217.NetworkVolume(...)does not resolve the volume._resolve_volumeand_check_endpointrun later, inside_make_transfer. So with a mistyped volume name, or a volume in a datacenter without the S3 API, the user types and saves keys first and only then learns the transfer cannot run. Fix: resolve the volume and check the endpoint before_ensure_s3_keys. - L2: Docs do not describe the new key flow.
docs/cli/references/command_line_interface.md:76-80andapps/volume.py:109still say keys come from flags or env vars only. Neither mentions saveds3_*keys, the prompts, or that VOLUME is optional.
Nits
rp_cli/volume.py:13the docstring line is 114 characters. Re-wrap it like its neighbors.rp_cli/volume.py:201-209with two positional arguments, the first is always VOLUME. Sorp volume download outputs/ ./out(no volume) looks up a volume namedoutputs/. The error says the volume does not exist, which is clear enough. A mention in--helpwould prevent the trip.tests/e2e/test_network_volume_transfer_live.py:52the name says "resumes", but the test never interrupts a transfer.
Simplification opportunities
- None worth a change.
_cleanupand_requestshare only thewith self._slots, and merging them would hide the skipped stop check.
Verified clean
_retry_delay: botocore emitsneeds-retryto every handler and takes the first non-None result. The standard handler stops at attempt 1 before it touches the quota, so this hook decides alone.attempts >= MAX_ATTEMPTSgives 10 tries. A 403 is not transient, so it gets no retry.- No slot deadlock:
_cleanupis called only from_delete_changedand_abort. Neither runs inside_request(:488,:607,:644,:689). _keys_rejected: it converts inside_requestbefore callers inspect the error.head's 400 path,_resume'sNoSuchUploadpath and_completed_anywaystill see their codes, because 400/404 are not converted.set_credentials: still refuses an existing profile withoutoverwrite. It now keeps the profile's other keys, so a re-login keeps saved S3 keys._write_configstill writes throughmkstemp, so the file stays owner-only. Tested intest_set_s3_keys_keeps_the_api_key_and_login_keeps_the_s3_keys._ensure_logged_in: callsloginwith its real parameters (rp_cli/main.py:613) and reloads the key from the profile it saved to._list_volumesmaps the RESTAuthenticationErrorto the login hint.- Secrets: the hidden secret prompt does not echo it (asserted in
test_cli_prompts_for_keys_and_offers_to_save).
Questions for the author
- Still open from round 6: could you rerun one live upload and download through
rp volume? The opt-in e2e test covers the Python API, but the latest live CLI pass predatesNetworkVolume, the condense, and this round's retry change.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 34 in / 14,409 out / 1,297,862 cache read / 89,291 cache write = 1,401,596. Estimated cost: $1.26 at $4 in / $20 out / $0.20 cache read / $8 cache write per MTok (round 9 rate card; 1-hour cache TTL assumed; $0.99 at 5-minute TTL). Counted before posting. Round 10 of this PR. This is automated assistance; a human owns the final call.
| if status not in (401, 403) and code not in _REJECTED_KEY_CODES: | ||
| return None | ||
| return NetworkVolumeTransferError( | ||
| f"the volume's s3 api rejected the keys ({status or code}): check that " |
There was a problem hiding this comment.
Medium (M1): The CLI saves prompted keys by default, and the secret is typed hidden. After a typo, find_s3_keys returns the saved pair on every run, so the prompt never returns. This message then points at two env vars the user never set, and does not mention ~/.runpod/config.toml. Name every source here, for example: check the --s3-* flags, RUNPOD_S3_ACCESS_KEY / RUNPOD_S3_SECRET_KEY, or s3_access_key / s3_secret_key in ~/.runpod/config.toml. In the CLI, consider offering to re-enter the keys when saved keys are rejected.
There was a problem hiding this comment.
Not addressed at 675087f: _keys_rejected still names only the env vars. Carried into round 11 as Medium.
There was a problem hiding this comment.
Not addressed at cd56170: this file and line are unchanged. Carried into round 12.
|
|
||
| _ensure_logged_in(ctx) | ||
| reference = reference or _pick_volume() | ||
| _ensure_s3_keys(opts) |
There was a problem hiding this comment.
Low (L1): NetworkVolume(reference, create=False) does not resolve anything. The volume lookup and _check_endpoint run later, in _make_transfer. With a mistyped volume name, or a volume in EU-SE-1, the user is asked for (and may save) S3 keys, then gets "no network volume named ..." or "does not support the s3 api". Resolve the volume and check its endpoint before _ensure_s3_keys.
There was a problem hiding this comment.
Not addressed at 675087f: _transfer still asks for S3 keys before the volume is resolved. Carried into round 11 as Low.
There was a problem hiding this comment.
Not addressed at cd56170: this file and line are unchanged. Carried into round 12.
| ``` | ||
|
|
||
| Transfers use the volume's S3-compatible API, so they need S3 API keys | ||
| (separate from the API key): set `RUNPOD_S3_ACCESS_KEY` and |
There was a problem hiding this comment.
Low (L2): This still lists only env vars and flags. Add the saved s3_access_key / s3_secret_key in ~/.runpod/config.toml, the prompts, and that VOLUME can be left out to pick one. NetworkVolume.upload's docstring (runpod/apps/volume.py:109) has the same gap.
There was a problem hiding this comment.
Not addressed at 675087f: this paragraph and apps/volume.py:109 are unchanged. The new --help text explains the picker, which covers part of the gap. Carried into round 11 as Low.
There was a problem hiding this comment.
Not addressed at cd56170: this file and line are unchanged. Carried into round 12.
|
|
||
| Uses the S3-compatible API, which needs S3 API keys (separate from your | ||
| API key): set RUNPOD_S3_ACCESS_KEY and RUNPOD_S3_SECRET_KEY, pass the | ||
| --s3-* flags, or enter them when asked. Re-run an interrupted command to resume it. Parts of an upload that is |
There was a problem hiding this comment.
Nit: 114 characters. Re-wrap like the surrounding lines.
There was a problem hiding this comment.
Not addressed at 675087f: still 114 characters.
There was a problem hiding this comment.
Not addressed at cd56170: this file and line are unchanged. Carried into round 12.
the help now says the volume list appears when only a path is given, and the live e2e test is named for what it checks.
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 11
Verdict: Approved. 675087f changes the key lookup without changing behavior, explains the volume picker in --help, and renames the e2e test. The round 10 Medium and two Lows are still open.
Approval gate: 0 Critical, 0 High, 3 Medium+Low → approved. This approval does not override @KAJdev's change request.
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R10-M1 | Rejected-key error names only the env vars, and saved keys with a typo never prompt again | Medium | Not addressed | _keys_rejected is unchanged (network_volume_transfer.py:104-108) |
| R10-L1 | CLI asks for S3 keys before it resolves the volume | Low | Not addressed | _transfer still runs _ensure_s3_keys before NetworkVolume(...) resolves anything (rp_cli/volume.py:216-219) |
| R10-L2 | Docs leave out saved keys, the prompts and the picker | Low | Not addressed | command_line_interface.md:76-78 and apps/volume.py:109-110 are unchanged. The --help text now explains the picker, which covers part of the gap |
| R10 nit | Two arguments always mean VOLUME first | Nit | Fixed | --help now says "Give only LOCAL_PATH to pick the volume from a list" (rp_cli/volume.py:229-230, :247-248) |
| R10 nit | e2e name says "resumes" | Nit | Fixed | Renamed to test_round_trip_then_rerun_skips |
| R10 nit | rp_cli/volume.py:13 is 114 characters |
Nit | Not addressed | Unchanged |
Existing feedback
- @KAJdev review (prompts and failure cases): no change since round 10. @KAJdev decides whether it is met.
- @KAJdev on
network_volume_transfer.py:1(condense) andCHANGELOG.md: no change.CHANGELOG.mdstill matchesmain. Both threads are left for @KAJdev. - CodeQL and github-code-quality (5 findings, each posted by both bots, at
a8431ea): no real bugs. Replied on the CodeQL threads.- Empty except at
network_volume_transfer.py:177: intended. The_sync_dirdocstring says best effort. A one-line comment would clear the alert. - Mixed returns at
rp_cli/volume.py:80: false positive. Every path returns, exits, or raisesClickException. import runpodmixed withfrom runpod...at:998,:123,:175: local imports that avoid an import cycle. Style only.
- Empty except at
PR description vs code
| Claim | Status | Where |
|---|---|---|
| S3 keys: flags, then env, then saved profile; first complete pair wins | ✅ | find_s3_keys (network_volume_transfer.py:1007-1019). The saved config is now read only when flags and env give no complete pair |
| CI: all checks pass | ✅ | All checks pass at 675087f |
| Rejected keys message as quoted | Accurate, but it names only the env vars (R10-M1) |
Findings
Medium
- R10-M1 (not addressed):
network_volume_transfer.py:108. See the existing thread.
Low
- R10-L1 (not addressed):
rp_cli/volume.py:217. See the existing thread. - R10-L2 (not addressed):
docs/cli/references/command_line_interface.md:77. See the existing thread.
Nits
rp_cli/volume.py:230is 106 characters after the help edit. Re-wrap it like:248.
Verified clean
find_s3_keysrewrite: it keeps the same order, and a pair still never mixes sources. A flag-only access key falls through to env, then to saved keys, as before (test_saved_s3_keys_are_the_last_fallback). The only change:saved_s3_keys()no longer runs when flags or env give a pair. So a brokenconfig.tomlcannot fail a run that has env keys.- New test
test_s3_keys_from_env_skip_the_saved_config: fails on the old eager tuple, because the old code calledsaved_s3_keysbefore it picked a pair. It is not a tautology. all(saved)on(None, None)or a half pair returns None, as before.
Questions for the author
- Still open from round 6: could you rerun one live upload and download through
rp volume? The body lists live CLI checks on the prompts, which covers most of this. Confirm they ran ata8431eaor later.
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 36 in / 7,683 out / 1,046,869 cache read / 67,688 cache write = 1,122,276. Estimated cost: $2.00 from the skill's sum_usage.py rate card (rate card not re-checked this round). Counted before posting. Round 11 of this PR. This is automated assistance; a human owns the final call.
| """Upload a file or folder to a network volume. | ||
|
|
||
| VOLUME is the volume's id or name. Give only LOCAL_PATH to pick the | ||
| volume from a list. REMOTE_PATH defaults to the local name. A folder's contents go under REMOTE_PATH/. |
There was a problem hiding this comment.
Nit: 106 characters after this edit. Re-wrap like line 248.
There was a problem hiding this comment.
Not addressed at cd56170: this file and line are unchanged. Carried into round 12.
…part a part body now raises once ctrl-c sets the stop flag, so the cli exits in a fraction of a second instead of finishing every 64 mib part in flight (23 s live). an api key the api rejects gets its own message instead of "not logged in".
| run_rest_paginated_request_async("/v2/network-volumes", "networkVolumes") | ||
| ) | ||
| except error.AuthenticationError as exc: | ||
| import runpod # pylint: disable=import-outside-toplevel,cyclic-import |
| run_rest_paginated_request_async("/v2/network-volumes", "networkVolumes") | ||
| ) | ||
| except error.AuthenticationError as exc: | ||
| import runpod # pylint: disable=import-outside-toplevel,cyclic-import |
OneAsterisk
left a comment
There was a problem hiding this comment.
Claude review, round 12
Verdict: Needs changes. The code in cd56170 is correct, but the PR body still says Ctrl-C lets in-flight requests finish, and this commit changes that for uploads.
Approval gate: 0 Critical, 1 High, 4 Medium+Low → not approved: the PR body makes a false claim at head. Round 12 withdraws the round 10 and 11 approvals.
Status of prior findings
| # | Finding | Severity | Status | Evidence |
|---|---|---|---|---|
| R10-M1 | Rejected-key error names only the env vars, and saved keys with a typo never prompt again | Medium | Not addressed | _keys_rejected is unchanged (network_volume_transfer.py:115-124) |
| R10-L1 | CLI asks for S3 keys before it resolves the volume | Low | Not addressed | rp_cli/volume.py is unchanged in cd56170 |
| R10-L2 | Docs leave out saved keys, the prompts and the picker | Low | Not addressed | command_line_interface.md and apps/volume.py are unchanged |
| R10 nit | rp_cli/volume.py:13 is 114 characters |
Nit | Not addressed | Unchanged |
| R11 nit | rp_cli/volume.py:230 is 106 characters |
Nit | Not addressed | Unchanged |
Existing feedback
- @KAJdev's change request (prompts and failure cases), the condense thread on
network_volume_transfer.py:1, and theCHANGELOG.mdthread: no change since round 11.CHANGELOG.mdstill matchesmain. All three stay with @KAJdev. - CodeQL and github-code-quality threads from
a8431ea: no change; the round 11 replies stand.
PR description vs code
| Claim | Status | Where |
|---|---|---|
| "Ctrl-C lets requests already in flight finish and starts no new ones" | ❌ | Uploads now stop mid-part: _StoppableBody.read raises once _stop is set (network_volume_transfer.py:248-251). Downloads still finish their chunk |
| Testing: "Parts already sending finish first (up to about 10 s at the default concurrency)" | ❌ | Same change. The commit message says the exit now takes a fraction of a second |
| Prompts and failures list | Leaves out the new API_KEY_REJECTED message (:53-56) |
|
| "Each fix has a test that fails when the fix is removed" | Removing self._body(...) from _single_put or _Upload fails no test (L3) |
|
| CI: all checks pass | 7 of 9 pass at cd56170; test (3.12) and test (3.13) were still running at review time |
Findings
High
- H1: PR body describes the old Ctrl-C behavior.
network_volume_transfer.py:301. The Resume list and the Testing section both say in-flight parts finish. At head, an upload stops mid-part, and only download chunks finish. Update both lines, add the rejected-API-key message to "Prompts and failures", and say whether the live Ctrl-C check ran atcd56170.
Medium
- R10-M1 (not addressed):
network_volume_transfer.py:113. See the existing thread.
Low
- R10-L1 (not addressed):
rp_cli/volume.py:217. See the existing thread. - R10-L2 (not addressed):
docs/cli/references/command_line_interface.md:77. See the existing thread. - L3: the new test does not cover the call sites.
tests/test_network_volume_transfer.py:1554. It tests_StoppableBodyalone.FakeS3._payloadaccepts raw bytes too, so dropping the wrapper from_single_putor_Uploadstill passes. Add a case whereupload_partsetstransfer._stopbefore readingBodyand assert a resumable "interrupted" error.
Nits
tests/test_network_volume_transfer.py:1162still says "only parts already in flight finish". Those parts now raise when they read their body.
Verified clean
- Ctrl-C path through botocore: the body raises inside
http.client. urllib3 does not catch aRuntimeError, and botocore wraps it inHTTPClientError(httpsession.py:555-558, botocore 1.43.119). Theneeds-retryhook treats that as resumable._stop.waitthen returns at once and raises a resumable "interrupted" error. - No truncated part gets reused: the server gets a short body and the connection drops. On resume, only parts whose ETag matches the local MD5 are reused.
- Wire format:
request_checksum_calculation="when_required"means no aws-chunked wrapper. HTTPS usesUNSIGNED-PAYLOAD, so the body is not read early for signing. A transient retry rewinds theBytesIOthroughreset_stream.BytesIO(bytes)shares the buffer, so there is no extra copy. - Missing vs rejected API key:
_resolve_api_keyraisesAuthenticationErroronly with no key, and_raise_for_errorraises it only on 401. Sorunpod.api_keycleanly separates the two cases, and "(401)" is accurate. The error is not resumable, so no resume hint. The parametrized test covers both cases.
Questions for the author
- A file-like
Bodymakes botocore addExpect: 100-continueto every PutObject and UploadPart, including empty files (handlers.pyadd_expect_header). FakeS3 cannot see this. Did a live upload through Runpod's S3 proxy run atcd56170?
Reviewed by Claude (claude-opus-5-5) via the pr-review skill. Tokens: 28 in / 13,231 out / 899,196 cache read / 71,860 cache write = 984,315. Estimated cost: $2.32 from the skill's sum_usage.py rate card (rate card not re-checked this round). Counted before posting. Round 12 of this PR. This is automated assistance; a human owns the final call.
| raise | ||
| raise rejected from exc | ||
|
|
||
| def _body(self, data: bytes) -> "_StoppableBody": |
There was a problem hiding this comment.
High (H1): This wrapper means an upload now stops mid-part on Ctrl-C. The PR body still says "Ctrl-C lets requests already in flight finish", and its Testing section says "Parts already sending finish first (up to about 10 s ...)". Update both lines. Downloads still finish their chunk, so say that too. Also add the new rejected-API-key message to "Prompts and failures".
| assert nvt.find_s3_keys() == ("user_x", "rps_x") | ||
|
|
||
|
|
||
| def test_upload_body_stops_mid_part(tmp_path): |
There was a problem hiding this comment.
Low (L3): This tests _StoppableBody alone. FakeS3._payload also accepts raw bytes, so removing self._body(...) from _single_put or _Upload._part still passes every test. Add a case where fake.upload_part (and put_object) sets transfer._stop before it reads Body. Then assert upload raises a resumable "interrupted" error and stores no part.
| if first: | ||
| raise KeyboardInterrupt | ||
| # every other part waits for the interrupt to land, so what runs | ||
| # after it is deterministic: only parts already in flight finish |
There was a problem hiding this comment.
Nit: stale after cd56170. These parts now raise when original reads the body, so none of them finish. Something like "nothing starts after it, and parts in flight stop at their next body read".
Superseded by Claude review, round 12





Adds resumable file and folder transfer to and from network volumes through the volume's S3-compatible API. It behaves like the matching
runpodctl network-volume upload/downloadcommands (runpod/runpodctl#385). The commands sit underrp volumeso they share the group #638 adds; whichever PR merges second folds its commands into the other'srunpod/rp_cli/volume.py.Resume: if a transfer stops (Ctrl-C, network drop, crash), rerun the same call or command.
force=True/--forcetransfers every file.~/.runpod/transfers/, and a rerun reuses every part whose MD5 ETag matches the local bytes.--force. If the file vanished instead, the uploaded object is kept.<name>.rpdownload, with finished chunks recorded in a sidecar pinned to the remote key, size and ETag. Downloaded files get the remote modification time..segments, a file that would also be a folder (anext toA/b), andxnext tox.rpdownload(or its sidecar and temp files). A symlinked.rpdownload, sidecar, or temp file is refused./. This applies only when a listing confirms the folder; any other HEAD 400 is raised as is..rpdownloadleftovers and reports them as skipped.bytes_transferredcounts only bytes moved by this run.needs-retryhook per transfer decides: up to 10 tries for throttling, 5xx (including the 524 Runpod's proxy returns on slow operations, which botocore skips) and dropped connections, with capped backoff and no shared retry quota. Ctrl-C ends a retry wait. A download body that drops mid-read is fetched again, up to three tries.--concurrencyalso bounds the data in flight. Cleanup requests take a slot too.NoSuchUpload, 416).Credentials: the volume and its datacenter are looked up by id or name with the API key. S3 keys come from the flags/arguments,
RUNPOD_S3_ACCESS_KEY/RUNPOD_S3_SECRET_KEY, ors3_access_key/s3_secret_keysaved in the profile in~/.runpod/config.toml; the first complete pair wins.Prompts and failures (CLI): checked in order: login, volume, S3 keys.
not logged in: run \rp login` or set RUNPOD_API_KEY`.VOLUMEleft out (rp volume upload ./data): lists your volumes and asks for a number; in a script:pass VOLUME first.--s3-*flags and the console page.the volume's s3 api rejected the keys (403): check that RUNPOD_S3_ACCESS_KEY and RUNPOD_S3_SECRET_KEY are current and belong to this account, no resume hint.EU-SE-1 does not support the s3 api yet (https://docs.runpod.io/storage/s3-api), no resume hint.Runpod S3 behavior this code depends on:
CI: all checks pass.
tests/e2e/test_network_volume_transfer_live.pyis opt-in (RUNPOD_E2E_VOLUMES=1plus API and S3 keys): it creates a 10 GB volume, round-trips a multipart file and a folder, checks the rerun skips everything, and deletes the volume. It skips in CI.Note: parts of an upload that is never resumed stay in the volume's hidden
.s3compat_uploadsfolder and count toward its size. The CLI help and the CLI reference say so.Testing
pytest --ignore=tests/e2e: 1795 passed, 92.6% coverage.tests/test_network_volume_transfer.pycollects 120 cases against an in-process fake S3 that matches Runpod's behavior.not logged in: run \rp login` or set RUNPOD_API_KEY; S3 keys without login gives the same; rejected keys givethe volume's s3 api rejected the keys (403): check that RUNPOD_S3_ACCESS_KEY and RUNPOD_S3_SECRET_KEY are current ...`.