Repository navigation
feat(upload): add single-file connection-fault retry opt-out - #370
Conversation
Quality Keeper — Advisory ReviewSource:
Quality Keeper assessed the repository, executed checks, and collected results, covering 19 quality expectations. Five repository-quality areasHard expectations keep their exact Evidence State. Advisory applicability questions are separate. Implementation or suite presence is not proof of a trusted run.
Observability — 0 hard, 0 advisoryCan operators understand and diagnose repository-owned runtime behavior? Hard expectations None selected. Applicability Review Gaps (advisory) None. Product Analytics — 0 hard, 0 advisoryDoes repository-owned product analytics instrumentation behave safely and as declared? Hard expectations None selected. Applicability Review Gaps (advisory) None. User Feedback — 0 hard, 0 advisoryDo repository-owned feedback and support-intake flows behave safely and accessibly? Hard expectations None selected. Applicability Review Gaps (advisory) None. Reliability — 3 hard, 0 advisoryDoes repository-owned software fail, recover, time out, and report health predictably? Hard expectations
Applicability Review Gaps (advisory) None. Security — 4 hard, 0 advisoryDo static controls and repository-owned security behaviors protect the intended boundaries? Hard expectations
Applicability Review Gaps (advisory) None. Other registered quality — 12 hard, 0 advisoryRegistered Testing Types not assigned to one of the five focused areas. Hard expectations
Applicability Review Gaps (advisory) None. Result
CoveredQuality Keeper found acceptable evidence for one or more expectations in these Testing Types.
Needs attentionQuality Keeper does not yet have acceptable evidence for these Testing Types. Some have partial checks that need to run, be fixed, or be connected. Others need new proof or seeded tests. In both cases, Quality Keeper cannot count them as covered yet.
Quality Keeper changes no Evidence State. Pending, missing, executed-but-not-exercised, and review items are not passing, and seeded scaffolds are next actions, not evidence, until applied and re-evaluated. Full breakdown (19 expectations)
New to these terms? Start with the Quality Keeper Standards Wiki to learn what each testing type means, why it applies, and what counts as evidence. |
sophiecarreras
left a comment
There was a problem hiding this comment.
Review of #370 (read-only; nothing was run). No blocking findings. CI is green; the skipped live-contract and audit jobs are not counted as passing.
What was checked
- With
retryUploadConnectionFaults: false, no path re-sends a single-file upload POST after a connection fault: the gate is atretry.ts:288and:332, the transport never replays upload POSTs (transport.ts:510-518,:686-691), and part uploads are gated off by design (a retried part replaces the same part number). - With the default (
true), the result is identical to the previous behaviour (#344), so existing retry behaviour is unchanged. - Tests: each claimed behaviour has a test that fails if its line is reverted.
Suggestions (non-blocking)
- Docs: say that with the option off, faults that happen before the request is sent (for example connection refused on a stale pooled upload URL) are also not retried.
uploadInProgressis set before the POST (retry.ts:262-270), so any network error counts. - Docs: README line 147 and the CHANGELOG mention only 503. Replays after 408, 500, 502 and 504 responses still happen through
B2Error.retryable(retry.ts:335-336) even with the option off. That matters for Object Lock buckets, andMIGRATION.md:50already says "not exactly-once". - Hardening: the
= truedefault parameters inbucket.ts:433andobject.ts:155mean a future call site that forgets the argument silently ignores the client setting.src/redaction.test.ts:51andsrc/redaction.node.test.ts:58already omit it. Consider removing the default so the compiler catches it.
Not covered: real B2 behaviour after a dropped POST, because there is no live run in this PR.
Context
Add an escape hatch for single-file upload connection-fault retries while retaining normal recovery. Replaying a POST that B2 already stored can create duplicate versions, and Object Lock retention or legal hold can prevent deleting them.
The default remains on. The default-on versus opt-in decision is pending the team's sign-off; this PR only adds the escape hatch.
Root cause
The upload retry helper treated every non-SSRF, non-response-body
NetworkErroras retryable. Disabling connection-fault replay required settingretry.maxRetries: 0, which also disabled server, auth-token, and upload-URL recovery.Changes
retryUploadConnectionFaults(defaulttrue) to client and per-upload options. Per-call values override the client default, including an explicittrue.retryResponseBodyFailuresplumbing with a default resolver, a required resolved upload-layer boolean, and forwarding through the part helper and all multipart/write-stream call sites. The single-file phase gate preserves multipart behavior.falseonly to single-file upload-callback connection faults, including wrapped transport errors and timeouts before response headers. Preserve upload-URL acquisition, server/auth/upload-URL retries, backoff, SSRF handling, and multipart/write-stream retry policy. Multipart retries replace the same part number.retryResponseBodyFailuresoption. Enabling that option can still replay a stored POST with connection-fault retries disabled; this is not an exactly-once guarantee.Verification
pnpm install --frozen-lockfile && pnpm run verifypassed:The gate also passed lint, TSDoc lint, typecheck, example typecheck, build, generated docs, and package/release/docs-consistency checks. Biome emitted its existing schema-version informational diagnostic; no warning or error failed the gate. The audit counts above are wrapper tests, not a separate live dependency audit.
Each mutation below was applied alone, failed its targeted assertions, and was reverted. The restored focused suite passed all 37 tests.
The resolved-option follow-up also rechecked the
return truemutation (11 failed, 26 passed) and a resolver default offalse(1 failed, 2 passed). RemovinguploadPartWithFreshUrlforwarding failed typecheck with TS2345, because the resolved field is required; restoring it passed typecheck and full verification.Test limit: the simulator does not persist single-file retention headers. The Object Lock-enabled case verifies the enabled bucket, outgoing retention settings, and one stored version; it does not prove server-side retention enforcement. No live B2 run is claimed.
Closes #368