fix(packages): reclaim unadopted multipart uploads - #3736
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment has been minimized.
This comment has been minimized.
|
Codex review: needs maintainer review before merge. Reviewed September 16, 2026, 2:03 AM ET / 06:03 UTC (Revision 2). ClawSweeper reviewWhat this changesReclaims newly uploaded package files that no release adopts and limits disposable authenticated-test backends to two active isolates while preserving explicit overrides. Merge readiness✅ Ready for maintainer review No blocking findings. Current main still lacks multipart upload reclamation, and the supplied real-Convex evidence supports the cleanup boundaries. This collaborator-authored PR remains a useful implementation candidate. Priority: P2 Review scores
Verification
How this fits togetherClawHub package publication accepts authenticated multipart uploads, stores their contents in Convex, and attaches them to published or pending releases. Cleanup must distinguish newly uploaded files from reusable staged artifacts and files already owned by releases. flowchart TD
A[Authenticated package upload] --> B[Store and track new blobs]
B --> C{Parsing succeeds?}
C -->|No| D[Reclaim request blobs]
C -->|Yes| E[Validate and publish]
E --> F{New release adopts files?}
F -->|No| D
F -->|Yes| G[Retain release files]
H[Reusable staged tarball] --> E
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Technical reviewBest possible solution: Keep cleanup tied to request provenance and release adoption; durable collection of interrupted uploads can remain separate work. Do we have a high-confidence way to reproduce the issue? Yes: current-main source stores multipart blobs before publication validation and lacks request cleanup on rejection; the contributor additionally supplies real before/after Convex observations. This read-only review did not execute a reproduction. Is this the best way to solve the issue? Yes for package cleanup: the action owns the release-adoption decision, making blanket HTTP deletion unsafe. The isolate cap is an acceptable measured mitigation; raising watchdog limits would weaken the existing test boundary. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against b0fee70b989c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
This branch was successfully deployed
1 participant
Closes #3673.
Multipart package publication stored tarballs and extracted/loose files before validation, leaving them orphaned when publication rejected. Track only request-created blobs, settle every in-flight store before parse cleanup, and transfer cleanup responsibility to the internal publication action. The action reclaims files until a newly committed release adopts them; reused upload-ticket artifacts and committed release files survive retries and later failures.
Thanks to @SebTardif for reporting this and the earlier, closed-unmerged implementation in #3613. This replacement addresses its two deletion-safety findings: reused staged tarballs and errors after publication commits.
Real before/after proof: disposable local Convex 1.44.0, authenticated HTTP multipart requests through the real publication actions and storage. Baseline
be87bfd9d3f8; candidate is this PR. Counts below are new blobs left without a release after the same operation.Successful published and pending releases retained all five expected blobs. The removed-member replay preserved the original release files and reused tarball: all five still downloaded with HTTP 200. Deliberately injected failures after real published-release insertion and pending-release/attempt insertion returned HTTP 400 while all five committed blobs remained readable, before and after. Partial-store failures used a temporary local wrapper that rejects one store while sibling writes still use actual Convex storage. All temporary routes, helpers, and fault injections were removed, and the clean function set was pushed with deploy typechecking before shutdown.
An ambiguous action-RPC failure does not authorize HTTP deletion. Interrupted actions, dispatches that never arrive, or failed best-effort deletions can still leave unreferenced blobs; this change adds no durable orphan collector.
Validation: focused HTTP/package tests (859 passed);
bun run ci:static;bun run ci:unit --maxWorkers=4(6,968 passed, 3 skipped);bun run ci:packages;bun run ci:types-build; standalone schema, CLI, and Convextsc --noEmit; clean local Convex push with typechecking. The default-concurrency full suite twice timed out on an unchanged 80 MiB archive fixture; that fixture passed alone in 1.60 seconds, and the complete coverage gate passed with four workers without changing tests, timeouts, or coverage thresholds. Structured autoreview: no findings.Authenticated CI runtime
The disposable local-auth backend now defaults to two active V8 isolates, preserving explicit overrides (including
0for unlimited). This uses Convex's execution scheduler to reduce CPU contention. The one-second UDF watchdog and existing system/admission deadlines, browser assertions and retries are unchanged.Comparable native Linux ARM64 proof (no x86 emulation), using the same application/backend revisions and fresh state:
This establishes a mitigation under controlled CPU contention, not Blacksmith's specific root cause or an optimal cap. The same hosted timeout symptom also occurred on main without this package patch. Original package storage proof and product code remain unchanged.
Validation: 33 focused runner tests;
bun run ci:static; final docs format check;bun run ci:unit --maxWorkers=4; explicit schema, CLI and Convex TypeScript checks. Independent review found no actionable issues. The final source with no caller override passed the full native test at 3 CPU quota (1.3m), with no watchdog, admission, or system-timeout errors. Full unit coverage passed: 6,969 tests passed / 3 skipped.