Skip to content

feat: resumable network volume upload and download - #655

Open
justinwlin wants to merge 20 commits into
mainfrom
feat/network-volume-transfer
Open

justinwlin wants to merge 20 commits into
mainfrom
feat/network-volume-transfer

Conversation

@justinwlin

@justinwlin justinwlin commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

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/download commands (runpod/runpodctl#385). The commands sit under rp volume so they share the group #638 adds; whichever PR merges second folds its commands into the other's runpod/rp_cli/volume.py.

rp volume upload <volume> <local-path> [remote-path]     # volume id or name
rp volume download <volume> <remote-path> [local-path]
from runpod import NetworkVolume
volume = NetworkVolume("my-volume")  # id or name; must already exist
volume.upload("./dataset", "datasets/dataset")
volume.download("outputs/", "./outputs")
# both return {"files_transferred", "files_skipped", "bytes_transferred"}

Resume: if a transfer stops (Ctrl-C, network drop, crash), rerun the same call or command.

  • Unchanged files are skipped: same size, and the destination is not older than the source, compared in whole seconds. force=True / --force transfers every file.
  • Multipart uploads keep their upload id in ~/.runpod/transfers/, and a rerun reuses every part whose MD5 ETag matches the local bytes.
  • If the local file changes mid-upload, the upload is aborted, or the single-request object is deleted, so the rerun sends the new version. If that delete fails, the error says to rerun with --force. If the file vanished instead, the uploaded object is kept.
  • A failed complete is accepted only when the failure was ambiguous and the object's ETag equals the multipart ETag of the uploaded parts.
  • Downloads fetch 64 MB chunks into <name>.rpdownload, with finished chunks recorded in a sidecar pinned to the remote key, size and ETag. Downloaded files get the remote modification time.
  • A folder download is refused if its files would share a local path. That covers case, NFC, . segments, a file that would also be a folder (a next to A/b), and x next to x.rpdownload (or its sidecar and temp files). A symlinked .rpdownload, sidecar, or temp file is refused.
  • Uploading a file onto a remote folder gets a hint to add /. This applies only when a listing confirms the folder; any other HEAD 400 is raised as is.
  • A folder upload skips .rpdownload leftovers and reports them as skipped.
  • One concurrency limit covers every S3 request. Ctrl-C lets requests already in flight finish and starts no new ones.
  • bytes_transferred counts only bytes moved by this run.
  • Retries: botocore's own retries are off and one needs-retry hook 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.
  • A request and its body or part read share one concurrency slot, so --concurrency also bounds the data in flight. Cleanup requests take a slot too.
  • The CLI prints the resume hint only for interruptions, transient failures, and the final-looking errors a rerun gets past (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, or s3_access_key / s3_secret_key saved in the profile in ~/.runpod/config.toml; the first complete pair wins.

Prompts and failures (CLI): checked in order: login, volume, S3 keys.

  • No API key: asks "log in now?" and runs the browser login; in a script: not logged in: run \rp login` or set RUNPOD_API_KEY`.
  • VOLUME left out (rp volume upload ./data): lists your volumes and asks for a number; in a script: pass VOLUME first.
  • No S3 keys: explains they are separate from the API key, asks for both (secret hidden) and offers to save them; in a script the error names the env vars, the --s3-* flags and the console page.
  • Rejected keys: 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.
  • A datacenter without the S3 API fails at once: 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:

  • HeadObject on a directory returns 400, not 404.
  • Part ETags are plain MD5, and the multipart object ETag is the standard MD5 of the part MD5s. Both were verified live.
  • S3 dates have whole-second precision, so the skip rules compare whole seconds.

CI: all checks pass. tests/e2e/test_network_volume_transfer_live.py is opt-in (RUNPOD_E2E_VOLUMES=1 plus 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_uploads folder 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.py collects 120 cases against an in-process fake S3 that matches Runpod's behavior.
  • Each fix has a test that fails when the fix is removed, including Ctrl-C winding down and the shared concurrency limit.
  • Live checks on the latest code, on a temporary 10 GB volume in EU-RO-1, deleted afterwards:
    • The opt-in e2e round trip passes (21 s).
    • The three cases from review: no login gives not logged in: run \rp login` or set RUNPOD_API_KEY; S3 keys without login gives the same; rejected keys give the volume's s3 api rejected the keys (403): check that RUNPOD_S3_ACCESS_KEY and RUNPOD_S3_SECRET_KEY are current ...`.
    • Logged in without S3 keys, in a script, names the env vars, flags and console page.
    • An unknown volume, a missing VOLUME in a script, and a missing remote path each give a one-line message. A volume given by name works.
    • Interactive: the volume list appears when only a path is given, and picking a number downloads from that volume.
    • Ctrl-C exits 130 with the resume hint. Parts already sending finish first (up to about 10 s at the default concurrency), and the rerun reused 268 MB and sent the missing 32 MB. An interrupted download resumed; SHA-256 matches and a rerun skips.
    • Not run live: the S3 key prompt and save, which is covered by the CLI tests.

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.
Comment thread runpod/network_volume_transfer.py Fixed
Comment thread runpod/network_volume_transfer.py Fixed
Comment thread tests/test_network_volume_transfer.py Fixed
Comment thread runpod/network_volume_transfer.py Fixed
Comment thread runpod/network_volume_transfer.py Fixed
Comment thread tests/test_network_volume_transfer.py Fixed
- 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 OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, mixed runpod import). All 6 threads are resolved. Verified fixed at d95e161. _remove now uses unlink(missing_ok=True) (network_volume_transfer.py:178). The LookupError branch 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 e2e and validation checks are not mentioned in the PR body. e2e fails with GraphQL request failed: 401 while runpod_flash deploys mock-worker endpoints (log). validation fails only because e2e failed. The same 401 shows on main CI 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_files pool. Each running file's inner part or chunk pool keeps going, then the outer with ThreadPoolExecutor waits for it. rp_volume_cache.py:730-739 already solves this with a shared stop Event.
  • 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.bin and data/A.bin (or a/./b and a/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. :569 calls head(key) without directory_ok. upload vol model.bin models prints "An error occurred (400) when calling the HeadObject operation".
  • A single-file download to out/ (missing folder) writes a file named out. :711-713. Upload treats a trailing / as "keep the name", so the two directions disagree.
  • volume_id is not escaped in the REST path. :797. The repo escapes IDs with _path_segment (api/ctl_commands.py:17).
  • The max_pool_connections comment 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:1200 says "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.md lists every rp group except network-volume. CHANGELOG.md has a hand-maintained ## Unreleased section.
  • The PR body says 57 tests. The file collects 63 cases.

Nits

  • :447-449, :643-647 bytes_transferred counts resumed bytes that were not sent this run.
  • :33 UPLOADS_DIR = ".s3compat_uploads" needs a comment saying the server creates it. The test also hardcodes the literal instead of using the constant.
  • :736 One key with \ or a//b fails 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:594 builds a real boto3.session.Session(), so a stale AWS_PROFILE on a dev machine fails it.

Simplification opportunities

  1. Drop one retry layer by setting botocore max_attempts to 1. This removes about 1 line and fixes the Medium. Same PR.
  2. Merge the two concurrency tests. This removes about 25 lines. Same PR or a follow-up.

Verified clean

  • Path traversal on download. _safe_target refuses .., a leading /, \, drive letters, and empty segments. It also checks containment after resolve(), 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 _request call 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 LastModified makes 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_for respects both limits. boto3 is a declared dependency and is imported lazily, like rp_upload.py.
  • Test hygiene. Tests patch _sleep, use tmp_path for 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_uploads and count against the volume's quota. Examples: an upload that is never rerun, a failed _abort, or an ambiguous create_multipart_upload retry. Is that expected? If so, a "Things to know" note would help users.
  • Do Runpod's ListObjectsV2 and HeadObject return LastModified at 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.

Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/rp_cli/main.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/rp_cli/main.py Outdated
@justinwlin
justinwlin marked this pull request as ready for review October 26, 2026 21:29
- 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

@OneAsterisk OneAsterisk left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • e2e and validation still fail (R1-1, carried). The cause is the repo's e2e API key, not this PR. The gate passes once e2e is 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 in getaddrinfo. macOS returns EAI_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_transfer and raise the datacenter error there. Keep gaierror retryable mid-transfer. That also removes the CLI's private nvt._is_dns_failure call.

Low

  • Ctrl-C waits out a retry backoff. :173. _with_retries sleeps up to 31 s without checking _stop. During an outage, every worker in backoff delays exit by that long. Pass the event and use stop.wait(delay).
  • The new CLI branches have no tests. rp_cli/main.py:1173-1179. Neither the KeyboardInterrupt exit (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:539 The 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 :421 was shortened for this reason.
  • network_volume_transfer.py:848 The collision message says "case-insensitive filesystem" for an NFC collision too.

Simplification opportunities

  1. Replace _is_dns_failure (15 lines) and the CLI special case with one socket.getaddrinfo check before the first request. Net about −10 lines. Same PR (fixes the Medium).

Verified clean

  • Ctrl-C wind-down. _Stopped is not retryable, so _with_retries stops. Inner pools cancel. guarded records the file. KeyboardInterrupt still 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 --force covers 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_names checks file-vs-file collisions first, then the partial and sidecar twins against both files and parent dirs.
  • Trailing slash. The CLI passes local_path as a plain str (no click.Path), so the / reaches download().
  • Empty volume. download("/") on an empty bucket returns zeros; a missing prefix still raises FileNotFoundError.
  • Folder upload skips .rpdownload, sidecar, and sidecar .tmp leftovers.

Questions for the author

  • A real local file ending in .rpdownload is skipped by folder upload with no skip event 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.

Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/rp_cli/main.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
- 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
Comment thread runpod/network_volume_transfer.py Fixed
Comment thread runpod/network_volume_transfer.py Fixed
- 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 OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. The except OSError: pass has its reason in a comment above the try, 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, ⚠️ test 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_flight is timing-dependent. tests/test_network_volume_transfer.py:1133. assert 10 < 10 (log). The other interpreters were cancelled or passed, and all five passed at c92ac38, so this is a race, not a code regression. The fake raises KeyboardInterrupt in a worker thread. _stop is set only after that propagates through the inner pool, so the other file's workers keep calling upload_part for 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_stopped passes even with the fix reverted. tests/test_network_volume_transfer.py:1007. The autouse fixture stubs _sleep to a no-op. If _with_retries went back to _sleep(delay), the test would still finish in under 0.5 s. Also restore nvt._sleep to time.sleep in this test.
  • The DNS pre-check can block proxy-only networks. network_volume_transfer.py:952. botocore honors HTTPS_PROXY, so the proxy resolves the host. On a network where only the proxy can resolve public names, getaddrinfo fails locally and the transfer is refused before boto would have worked. Skip the check when urllib.request.getproxies() has an https entry, or treat the failure as a warning.

Nits

  • tests/test_network_volume_transfer.py:17 REAL_WAIT = nvt._wait sits between imports, so import runpod.error on line 18 trips pylint wrong-import-position. Move it below the imports.
  • network_volume_transfer.py:193 Put the reason inside the except (see Existing feedback). That clears both CodeQL threads.

Simplification opportunities

None worth a change in this round.

Verified clean

  • Stop-aware retries. After stop is set, _wait returns at once. The next attempt goes through _request, which raises _Stopped. _Stopped is not retryable, so the loop ends. The cleanup delete deliberately skips _request and stop, so it still runs after Ctrl-C.
  • Directory fsync in _write_json_atomic returns early where a directory cannot be opened (Windows). It runs once per 64 MiB chunk or part, so the cost is negligible.
  • _check_local_names compares folded file keys against folded parent keys, so a next to A/b is refused before any get_object.
  • _is_folder reports the original 400 when the listing itself fails, and FakeS3 accepts MaxKeys.
  • test_unresolvable_endpoint_fails_before_the_transfer uses .invalid, which never resolves. monkeypatch.undo() drops only this test's autouse patches.

Questions for the author

  • Is _check_endpoint_resolves meant to run when endpoint_url is 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.

Comment thread tests/test_network_volume_transfer.py Outdated
Comment thread tests/test_network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py
Comment thread tests/test_network_volume_transfer.py Outdated

@OneAsterisk OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at 8c260d2. 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_stopped cannot fail on revert (R3-2, carried). tests/test_network_volume_transfer.py:1039.
  • The new endpoint_check fixture still reads the host's proxy settings. tests/test_network_volume_transfer.py:916. It clears the proxy env vars, but on macOS and Windows getproxies() falls back to system settings when the env is empty. On a developer Mac with a system proxy, the lookup is skipped and test_unresolvable_endpoint_fails_before_the_transfer fails. CI is Linux, so it stays green. Stub urllib.request.getproxies instead (inline).

Nits

  • tests/test_network_volume_transfer.py:17-18 Move REAL_WAIT and REAL_ENDPOINT_CHECK below import runpod.error (R3 nit).

Simplification opportunities

None.

Verified clean

  • Proxy skip. getproxies().get(parsed.scheme or "https") picks the same key botocore uses for an https:// endpoint. NO_PROXY hosts still get the lookup through proxy_bypass.
  • New tests can fail. In the proxy test, getaddrinfo raises. A missing skip would surface as NetworkVolumeTransferError.
  • No real DNS in tests. socket.getaddrinfo is patched on the socket module, which network_volume_transfer imports as a module (:20). The real .invalid lookup and monkeypatch.undo() are gone.

Questions for the author

  • Should _check_endpoint_resolves run when endpoint_url is 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.

Comment thread tests/test_network_volume_transfer.py Outdated
- 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 OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the if keeps the suite green (inline).
  • endpoint_check fixture reads system proxy settings (R4-1, carried). tests/test_network_volume_transfer.py:911. Patch urllib.request.getproxies to return {}.

Nits

  • tests/test_network_volume_transfer.py:1154 Use import threading instead of __import__("threading").

Simplification opportunities

None.

Verified clean

  • Slot re-check. _Stopped is raised inside with self._slots, so the slot is released. _Stopped is not retryable, and the cleanup delete does not go through _request, so it still runs after Ctrl-C.
  • Ctrl-C test bound. KeyboardInterrupt is a BaseException, 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 _stop is set. Without the re-check, a queued worker would make a fourth call.
  • Backoff test. With the real _wait and _sleep, stop.wait returns at once after stop is set.
  • Import order. import runpod.error is with the other imports; the REAL_* 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.

Comment thread runpod/network_volume_transfer.py
Comment thread tests/test_network_volume_transfer.py Outdated
@KAJdev

KAJdev commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

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 network-volume command. It should be under volume. reference #638

@KAJdev

KAJdev commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

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 OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, not network-volume". Verified fixed at f1fa981: runpod/rp_cli/volume.py defines the volume group, mounted in runpod/rp_cli/main.py:1092-1098; network-volume is gone.
  • @KAJdev, conversation: "global functions should be under the NetworkVolume class". Verified fixed at f1fa981: NetworkVolume.upload / .download (runpod/apps/volume.py:87-164); the runpod.upload_to_network_volume / download_from_network_volume exports and aliases are removed. The module-level nvt.upload / nvt.download remain 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_volume re-implements REST pagination with weaker checks — runpod/network_volume_transfer.py:940. run_rest_paginated_request_async (used by list_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_check fixture reads system proxy settings (R4-1, carried). Patch urllib.request.getproxies to return {}.

Nits

  • tests/test_network_volume_transfer.py:1188 __import__("threading") (R5, carried).

Simplification opportunities

  1. Replace the pagination loop in _resolve_volume with the shared helper (about 9 lines removed, and the paging tests become unnecessary). Same PR.

Verified clean

  • CLI wiring. volume is mounted in _mount_groups; no other volume command exists in runpod/cli or runpod/rp_cli. Tests that patch nvt.upload still intercept, because NetworkVolume.upload reads nvt.upload at call time.
  • NetworkVolume(reference, create=False). The transfer methods never create a volume; the docstring says the volume must exist, and _resolve_volume raises "not found".
  • Removed --datacenter. Docs, CLI help, error text, and tests no longer mention it. _check_endpoint_resolves text was updated to match.
  • Exports. __all__ derives from _LAZY_ATTRS, so test_no_module_level_transfer_exports can 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-volumes list 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.

Comment thread runpod/network_volume_transfer.py Outdated
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
OneAsterisk previously approved these changes Oct 26, 2026

@OneAsterisk OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (volume group, NetworkVolume methods): 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 e2e and validation fail on the external 401. Both pass at head. Drop the CI paragraph and update the case count to 96.

Verified clean

  • Helper reuse. block runs 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_name gets the full list, so the not-found and ambiguous-name checks see every page.
  • Error surfacing. QueryError from a bad page shape or a repeated cursor reaches the CLI's except Exception (runpod/rp_cli/volume.py:86) and prints without the resume hint, which is right for a deterministic failure.
  • Test patching. _list_volumes patches runpod.api.rest.run_rest_paginated_request_async. _resolve_volume imports it at call time, so the patch takes effect. The fake's (path, collection, **kwargs) signature matches the call.
  • Imports. List, Dict, Any, and Tuple are still used after the removed loop.
  • CI. CodeQL, test (3.10) to test (3.14), e2e, and validation all 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-volumes list 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.

@justinwlin
justinwlin requested a review from KAJdev October 26, 2026 22:58
_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
OneAsterisk previously approved these changes Oct 26, 2026

@OneAsterisk OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (volume group, NetworkVolume methods) still holds. NetworkVolume.upload/download now call _make_transfer directly, and the module-level upload/download pass-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: NetworkVolume now calls nvt._make_transfer across 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 b085cb1 against b9c8f92. 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, LookupError restart, 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
    • _emit summary counting, CLI resume hint, exit 130 on Ctrl-C
  • No dangling references to the removed nvt.upload / nvt.download in docs/, CHANGELOG.md, README, or runpod/__init__.py. The CLI tests now patch runpod.NetworkVolume instead.
  • a4bc86a fixes a real corruption path. Before it, a stale sidecar (wrong partial size, or a changed ETag) survived while load_or_init truncated 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_first fails without the fix.
  • all(resumable): tested both ways by test_folder_failure_is_resumable_only_when_every_failure_is. Ctrl-C is unaffected: KeyboardInterrupt is not caught by guarded.
  • Chunk index filter (0 <= i < count) also stops a negative saved index from inflating the reuse byte count.
  • CI: CodeQL, test (3.10) to test (3.14), e2e, and validation pass at a4bc86a.

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 NetworkVolume move 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of this file is utilities that arent specific to network volume transfering. Please condense

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread CHANGELOG.md

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont edit this file. this is for release please

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 matches main at c29e0d2. 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 (volume group, NetworkVolume methods): 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. At a4bc86a the slot covered the whole get_object plus Body.read(), and the part read inside upload_part. Now the slot covers only the request until headers arrive. In a folder transfer, each of concurrency file workers runs its own pool of concurrency chunk 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 _call variant 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.io and s3api-ca-mtl-1.runpod.io return NXDOMAIN. botocore retries the resulting EndpointConnectionError 10 times (up to about 111 s of backoff). is_resumable then returns True through BotoConnectionError (: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 _stop until it returns. pool.shutdown (:243) waits for that. With connect_timeout=60 and 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: lower total_max_attempts and retry outside botocore with stop.wait, or register a needs-retry handler that returns None once _stop is 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_json no longer fsyncs the parent directory. :128-137. a4bc86a synced the directory after os.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-862 black moved the # pylint: disable=import-outside-toplevel comments onto the closing ) lines, so they no longer apply to the from line.
  • :720 the "exists but is not a regular file" branch (a directory at x.rpdownload) has no test.
  • runpod/apps/volume.py:117, :147 (carried) nvt._make_transfer is 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 _call and _run without bringing back the old three-pool stack.

Verified clean

  • f7299a9: the refusal runs before any write (only mkdir precedes 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 under MAX_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 in runpod/, tests/ or docs/.
  • _resolve_volume helpers exist with matching signatures (run_rest_paginated_request_async, block, find_by_id_or_name(noun=, error=)).
  • CHANGELOG.md equals origin/main.
  • CI: CodeQL, test (3.10) to test (3.14), e2e and validation all pass at c29e0d2.

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 NetworkVolume move, 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.

Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py
@OneAsterisk
OneAsterisk dismissed stale reviews from themself October 27, 2026 00:54

Superseded by Claude review, round 9

@KAJdev KAJdev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last thing, functionality seems good, but these need prompts and better failure cases

Image Image Image

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread runpod/network_volume_transfer.py
Comment thread runpod/network_volume_transfer.py Outdated
Comment thread runpod/network_volume_transfer.py
Comment thread runpod/network_volume_transfer.py
Comment thread runpod/network_volume_transfer.py
…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:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree: style only. The local imports avoid an import cycle with runpod. No behavior impact.

Comment thread runpod/rp_cli/volume.py
)


def _invoke_transfer(transfer_method, source, target, opts, progress):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree: false positive. Every path in _invoke_transfer returns, calls sys.exit(130), or raises ClickException.

Comment thread runpod/rp_cli/volume.py
def _ensure_logged_in(ctx) -> None:
"""offer the browser login when no api key is set; the volume lookup
needs one."""
import runpod

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree: style only. Same local-import pattern as network_volume_transfer.py:998. No behavior impact.

Comment thread runpod/rp_cli/volume.py

def _prompt_s3_keys():
"""ask for the s3 keys, offering to save them for next time."""
import runpod

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree: style only. Same local-import pattern as network_volume_transfer.py:998. No behavior impact.

return
try:
os.fsync(fd)
except OSError:
Comment thread runpod/rp_cli/volume.py
)


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
Comment thread runpod/rp_cli/volume.py
def _ensure_logged_in(ctx) -> None:
"""offer the browser login when no api key is set; the volume lookup
needs one."""
import runpod
Comment thread runpod/rp_cli/volume.py

def _prompt_s3_keys():
"""ask for the s3 keys, offering to save them for next time."""
import runpod
OneAsterisk
OneAsterisk previously approved these changes Oct 27, 2026

@OneAsterisk OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 matches main. Thread left for @KAJdev.
  • Copilot (5 threads at c29e0d2): all verified fixed at a8431ea. Replied and resolved: part read slot, cleanup slot (_cleanup), edit during complete (_keep_if_unchanged after _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_keys returns the saved pair on every run, so the prompt never comes back. _keys_rejected then tells the user to check RUNPOD_S3_ACCESS_KEY and RUNPOD_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_volume and _check_endpoint run 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-80 and apps/volume.py:109 still say keys come from flags or env vars only. Neither mentions saved s3_* keys, the prompts, or that VOLUME is optional.

Nits

  • rp_cli/volume.py:13 the docstring line is 114 characters. Re-wrap it like its neighbors.
  • rp_cli/volume.py:201-209 with two positional arguments, the first is always VOLUME. So rp volume download outputs/ ./out (no volume) looks up a volume named outputs/. The error says the volume does not exist, which is clear enough. A mention in --help would prevent the trip.
  • tests/e2e/test_network_volume_transfer_live.py:52 the name says "resumes", but the test never interrupts a transfer.

Simplification opportunities

  • None worth a change. _cleanup and _request share only the with self._slots, and merging them would hide the skipped stop check.

Verified clean

  • _retry_delay: botocore emits needs-retry to 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_ATTEMPTS gives 10 tries. A 403 is not transient, so it gets no retry.
  • No slot deadlock: _cleanup is called only from _delete_changed and _abort. Neither runs inside _request (:488, :607, :644, :689).
  • _keys_rejected: it converts inside _request before callers inspect the error. head's 400 path, _resume's NoSuchUpload path and _completed_anyway still see their codes, because 400/404 are not converted.
  • set_credentials: still refuses an existing profile without overwrite. It now keeps the profile's other keys, so a re-login keeps saved S3 keys. _write_config still writes through mkstemp, so the file stays owner-only. Tested in test_set_s3_keys_keeps_the_api_key_and_login_keeps_the_s3_keys.
  • _ensure_logged_in: calls login with its real parameters (rp_cli/main.py:613) and reloads the key from the profile it saved to. _list_volumes maps the REST AuthenticationError to 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 predates NetworkVolume, 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 "

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not addressed at 675087f: _keys_rejected still names only the env vars. Carried into round 11 as Medium.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not addressed at cd56170: this file and line are unchanged. Carried into round 12.

Comment thread runpod/rp_cli/volume.py

_ensure_logged_in(ctx)
reference = reference or _pick_volume()
_ensure_s3_keys(opts)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not addressed at 675087f: _transfer still asks for S3 keys before the volume is resolved. Carried into round 11 as Low.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not addressed at cd56170: this file and line are unchanged. Carried into round 12.

Comment thread runpod/rp_cli/volume.py

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: 114 characters. Re-wrap like the surrounding lines.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not addressed at 675087f: still 114 characters.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
OneAsterisk previously approved these changes Oct 27, 2026

@OneAsterisk OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and CHANGELOG.md: no change. CHANGELOG.md still matches main. 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_dir docstring 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 raises ClickException.
    • import runpod mixed with from runpod... at :998, :123, :175: local imports that avoid an import cycle. Style only.

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:230 is 106 characters after the help edit. Re-wrap it like :248.

Verified clean

  • find_s3_keys rewrite: 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 broken config.toml cannot 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 called saved_s3_keys before 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 at a8431ea or 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.

Comment thread runpod/rp_cli/volume.py
"""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/.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: 106 characters after this edit. Re-wrap like line 248.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 OneAsterisk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the CHANGELOG.md thread: no change since round 11. CHANGELOG.md still matches main. 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 at cd56170.

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 _StoppableBody alone. FakeS3._payload accepts raw bytes too, so dropping the wrapper from _single_put or _Upload still passes. Add a case where upload_part sets transfer._stop before reading Body and assert a resumable "interrupted" error.

Nits

  • tests/test_network_volume_transfer.py:1162 still 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 a RuntimeError, and botocore wraps it in HTTPClientError (httpsession.py:555-558, botocore 1.43.119). The needs-retry hook treats that as resumable. _stop.wait then 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 uses UNSIGNED-PAYLOAD, so the body is not read early for signing. A transient retry rewinds the BytesIO through reset_stream. BytesIO(bytes) shares the buffer, so there is no extra copy.
  • Missing vs rejected API key: _resolve_api_key raises AuthenticationError only with no key, and _raise_for_error raises it only on 401. So runpod.api_key cleanly 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 Body makes botocore add Expect: 100-continue to every PutObject and UploadPart, including empty files (handlers.py add_expect_header). FakeS3 cannot see this. Did a live upload through Runpod's S3 proxy run at cd56170?

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":

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

@OneAsterisk
OneAsterisk dismissed stale reviews from themself October 27, 2026 18:51

Superseded by Claude review, round 12

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants