feat(benchmark): make bulk sample upload resumable

[RapidPoseidon]

Addresses customer feedback on bulk sample upload: 12 of 1,487 benchmark samples failed permanently over a mobile connection, and the failed pairs were only recoverable by scraping the logs.

Pairs with rapidata-backend#5015 (merged), which does the deduplication this relies on.

Why it failed

Each sample is two requests — POST /asset/file (the bytes, disk-cached) then POST /participant/{id}/sample (small JSON) — both inside one retry loop. The three attempts backed off 2**attempt, so all of them landed within ~3 seconds of each other, well inside the congestion window that caused the failure. add_model then logged the failures and dropped them.

What changed

Resumable upload. add_model runs a recovery sweep when anything fails, and participant.retry_missing(media, identifiers) runs it on demand: ask the server which identifiers are short, re-send every asset belonging to those identifiers, and let the backend reject the ones already there. It loops until nothing is short, stopping early if a round fails to close the gap.

The client does not decide what is a duplicate. An earlier revision of this PR compared the caller's paths against the originalFilename the API reports. That cannot work — originalFilename only holds the last path segment, so runA/0001.png and runB/0001.png are indistinguishable. @LinoGiger caught it and the heuristic is gone rather than patched. The backend now fingerprints each sample's asset by content and returns 409 for one the participant already holds; the SDK treats that as uploaded and does not retry it. That is what makes re-sending a whole identifier safe, and why the client never has to work out which of its assets is missing.

missing_counts(identifiers) reports the shortfall per identifier. It deliberately does not name individual media — that is precisely the question the client cannot answer correctly. Reasons for genuine failures still come back on FailedUpload, which carries the media, the identifier, the reason and the trace id.

Trace ids. A Kestrel request-timeout 408 is produced before any handler runs, so it has no problem+json body carrying traceId — but it does carry x-trace-id. That header is now preferred: TraceIdMiddleware sets it from Activity.Current.TraceId (the bare id) while the body's traceId comes from Activity.Current.Id (the full traceparent), so reading the header first reports one searchable shape regardless of which layer produced the error.

Jittered backoff at both the per-sample and transport layers, so a worker pool that fails together stops retrying in lockstep.

Three smaller fixes in the same path: sample paging capped at the server's MaxPageSize of 100 (it rejects rather than clamps, so the earlier 500 would have 400'd every call); intermediate retry attempts logged at INFO (they were at DEBUG behind suppress_rapidata_error_logging, so every transient failure that later succeeded was invisible); and the progress bar now advances on failures instead of stalling short of the total.

Behaviour change

upload_media previously returned tuple[list[str], list[str]] documented as "successful and failed identifiers", but actually returned asset paths in both. It now returns tuple[list[str], list[FailedUpload[SampleUpload]]] — identifiers that uploaded, and rich failures. No caller in this repo or the docs used the return value.

Separately, the backend change means one participant can no longer hold the same asset twice under the same prompt. That was previously legal; it would only double-weight the prompt in matchup sampling.

Still open

httpx.ReadError / WriteError are never retried at the transport layer (_is_retryable_error covers ConnectError/RemoteProtocolError/ConnectTimeout/ReadTimeout only), and 408 is not in _RETRYABLE_STATUS_CODES. Both are worth fixing and are now safe to fix, since the backend rejects duplicates — but the transport retry is blind to HTTP method, so it deserves its own PR rather than riding along here.

Testing

• uv run pyright src/rapidata/rapidata_client — 0 errors
• uv run pytest tests/ — 82 passed. The 4 failures in tests/rapidata_client/audience/ are pre-existing on main (verified by stashing this branch); they are untouched by this change.
• uv run --group docs mkdocs build — clean

🔗 Session: https://poseidon.rapidata.internal/chat/session-4cbbfac5

[LinoGiger]

why are we adding max workers as a parameter here when we have the global config that gets used everywhere else?

[RapidPoseidon]

Removed — upload_media reads the global rapidata_config.upload.maxWorkers again. Reasoning in the upload_config.py thread: the parameter only existed to let the sweep run lower, and that turned out not to be what makes the sweep work.

[LinoGiger]

same question for this

[RapidPoseidon]

Same — removed from retry_missing too. It now just calls upload_media, which uses the global config.

[LinoGiger]

i don't get why this would help? why can't we just use the normal max workers?

[RapidPoseidon]

You are right — removed, along with the per-call max_workers parameters on upload_media / retry_missing. Uploads now read rapidata_config.upload.maxWorkers like everywhere else.

Working through why I added it, it does not survive: the sweep set is the failures, so it is small by construction. In the customer's run it was 12 items — at that size a cap of 8 vs 25 changes nothing, because you never have more than len(missing) requests in flight anyway. And by then the assets are already in the local upload cache, so the sweep is small JSON POSTs, not the large bodies that produced the 408s.

What actually does the recovery is the server diff, the sweep landing outside the congestion window, and the jitter. The concurrency drop was the weakest of the four and cost a public config field to get. Their manual recovery used maxWorkers=8, which is what put the number in my head, but I think the timing and the warm cache are what made it work, not the 8.

Anyone who genuinely is uplink-bound can still set rapidata_config.upload.maxWorkers — the MRI docs now point at that.

[LinoGiger]

shouldn't this logic be the other way around?

[RapidPoseidon]

Yes — flipped, good catch. It is now trace_id or self._trace_id_from_details(details).

I checked the backend rather than guess, and the two sources are not the same string:

• Rapidata.Shared.API/Middleware/TraceIdMiddleware.cs:17 sets the header from Activity.Current?.TraceId.ToString() — the bare 32-hex trace id.
• The body's traceId comes from the stock DefaultProblemDetailsFactory (there is no custom one in rapidata-backend), which uses Activity.Current?.Id — the full traceparent, 00-<trace>-<span>-<flags>.

So body-first meant the reported id changed shape depending on which layer produced the error: a traceparent when a handler ran, a bare id when a Kestrel/LB timeout meant none did. Header-first gives one consistent value, and it is the form that goes straight into a trace search without stripping the wrapper — which is the whole point of the customer's request.

My original comment said the body was "canonical", which was the wrong axis to reason on. Comment rewritten to explain the format difference.

[LinoGiger]

i think this can not be above 100

[RapidPoseidon]

Correct, and it was worse than a limit — it would have failed outright. Rapidata.Shared.Query/Validators/PaginationValidator.cs does RuleFor(x => x!.Size).LessThanOrEqualTo(pagingSettings.Value.MaxPageSize), and Rapidata.Shared.Configuration/global-appsettings.json:49 sets MaxPageSize: 100. So it rejects rather than clamps: every uploaded_identifier_counts call would have 400'd, taking retry_missing and the whole automatic sweep down with it.

Fixed to 100, matching the prompt and leaderboard loops already in rapidata_benchmark.py, with a named constant and a comment so the next person does not read it as a tuning knob.

Worth noting why this got through: my tests mocked uploaded_identifier_counts wholesale, so the paging loop had no coverage at all. Added test_uploaded_identifier_counts_pages_within_the_server_page_limit, which drives the real loop against a mocked API and asserts every request stays at or below 100.

[LinoGiger]

not sure i like to expose this, cause it's only valid for when the participant gets instantiated and stuff is uploaded. when the participant is fetched, this is always empty, no?

[RapidPoseidon]

You are right, and it is exactly as bad as you describe: benchmark.participants constructs these objects from the API, so failed_samples would be [] there — indistinguishable from "uploaded cleanly". A property on a long-lived entity describing a transient operation.

Removed it, along with the backing state. In its place there is now participant.missing_samples(assets, identifiers), which asks the server which of your intended pairs are absent. That is correct on any participant however it was obtained, and it cannot go stale — it is the same diff retry_missing runs, just exposed.

The per-failure detail (reason, trace id) is not lost: upload_media and retry_missing still return FailedUpload[SampleUpload] objects, and add_model logs format_error_details() for each residual failure at ERROR. What the customer actually needed was the pairs, so they could re-upload without reconstructing them from logs — and missing_samples / retry_missing give them that without any stored state.

Happy to reinstate a stored accessor if you would rather have the reasons reachable from add_model programmatically; I left it out because it reintroduces the staleness you flagged.

[RapidPoseidon]

Confirmation from @LinoGiger's question territory, via Luca (leaderboard owner) — asked out-of-band whether multiple samples per (participantId, identifier) are intentional:

yes this is intentional. we support multiple samples for the same prompt

That validates the diff design in missing_samples: it counts samples per identifier rather than testing set membership, so supplying the same prompt N times and having one of the N fail is detected correctly. A set-based diff would have treated one uploaded sample as satisfying all N, and a backend uniqueness constraint on the pair would have been the wrong fix. Covered by test_missing_samples_counts_repeated_identifiers_individually.

The corollary is that the backend cannot dedupe defensively, so nothing server-side stops a blind retry from doubling a sample. That is why retry_missing verifies before re-uploading, and why the transport-layer retry work (adding 408 / httpx.ReadError) stays out of this PR. Follow-up with Luca on whether that endpoint should take an Idempotency-Key is in flight; either way it is a separate change.

[RapidPoseidon]

Correction: the diff was matching on the wrong thing

@LinoGiger caught that the identifier-count diff is not sufficient, and he is right. Superseding my earlier comment on this — the design it said Luca had validated was validated only on the "several samples per prompt is legal" point, not on the matching being adequate.

The bug. missing_samples counted how many samples the server held per identifier. That answers how many of an identifier's samples landed, never which. With a1.jpg, a2.jpg, a3.jpg all under identifier a, if a1 failed and a2 succeeded the server reports {a: 1}; the loop consumed that count against a1 in list order, reported a2 missing, and left a1 alone. The sweep then re-uploaded a2 — the exact duplicate the verify-before-retry design exists to prevent — and a1 was never uploaded at all. My test only passed because the failing asset happened to be last in the list.

The fix (2fd9cd0): match on the media as well as the identifier. The server reports assets in a form this repo already knows how to read — RapidataBenchmark.__extract_asset_url does the same thing for prompt assets: remote URLs round-trip verbatim as sourceUrl, local files come back as the bare originalFilename, text as its content. Multiplicity is still counted per pair, so the same media supplied twice still needs two samples.

Where an asset cannot be read back (unexpected shape, metadata the backend stops sending), the sample is counted as covering an intended pair rather than as absent. That direction is deliberate: an unsent sample is returned to the caller and visible, whereas a duplicate silently over-weights the prompt.

One case remains ambiguous and is documented on the method: two different paths sharing a filename under the same identifier (a/1.png vs b/1.png) are indistinguishable once the server has reduced them to originalFilename. Multiplicity is still right, so nothing is duplicated or dropped — only the choice of which of the two to re-upload could be wrong.

Tests: test_missing_samples_identifies_which_asset_is_absent is the regression guard (old code returns [a2, a3], new returns [a1, a3]), plus coverage for URL / basename / text matching, repeated media, and the unreadable-asset fallback. 85 passing, pyright clean.

[LinoGiger]

this method doesn't really work? cause the original filename isn't necessarily the same as the path for the upload cause it only saves the last bit. can't we use the cache first and see if we have the filename there?

[RapidPoseidon]

Agreed — and rather than reach for the cache, the whole heuristic is now gone.

You are right that it cannot work: originalFilename only holds the last path segment, so runA/0001.png and runB/0001.png are indistinguishable once the server has reduced them. The cache would have helped for local files with a warm cache, but not for a cold one, and not uniformly with URLs — so it would have been a better guess, still a guess.

Instead the decision moved to the backend, which is what we agreed after this came up in chat: rapidata-backend#5015 (merged) gives each sample a content fingerprint from the asset store's own hash — not the filename, so an uploaded file and a URL ingest of the same bytes compare equal — with a unique index on (participantId, identifier, assetFingerprint) and a 409 for a sample the participant already holds.

So the SDK is now your version: ask which identifiers are short, re-send every asset belonging to them, and let the server reject the ones already there. 409 counts as uploaded and is not retried. The client never has to know which of an identifier's assets is missing.

Deleted: _server_asset_key, _local_asset_key, and the pair-level diff. missing_samples is replaced by missing_counts(identifiers), which reports the shortfall per identifier — deliberately not naming individual media, since that is precisely the question the client cannot answer correctly. Reasons for genuine failures still come back on FailedUpload.

retry_missing loops until nothing is short and stops early if a round fails to close the gap, so a sample the server keeps refusing cannot spin.

82 tests pass, pyright clean, docs build. Ready for another look.