Retry and rate-pace TestOps uploads - #818
Conversation
Allure perf metricsGenerated at:
Artifacts: allure-perf-metrics |
Classify TestOps upload failures into transient (5xx/429/timeout),
recoverable ("launch is closed"), or terminal (auth/validation/payload/
conflict) kinds, and retry only the retryable ones with exponential
backoff. Wired into all three upload call sites (test results, global
attachments, global errors) — previously a failed request just logged
and gave up with no retry at all.
Add a leaky-bucket pacer with independent per-window budgets for
requests, files, and bytes sent to TestOps, enabled by default with
sane defaults (20 req/s, 1000 files/s, 1 GiB/s) since the server-side
limits it approximates are real regardless of whether anyone configures
for them. Disable via a new uploadRateLimit: false plugin option, or
tune it via an explicit uploadRateLimit config.
Also makes the `open` command's temp-report cleanup handle SIGTERM, not
just SIGINT, via a small shared signals helper (notifySignals/
waitForAbort) with conventional exit codes and a graceful-shutdown
deadline.
- classifyError only recognized raw AxiosError, but TestOpsClient never sees one: @allurereport/service wraps every HTTP failure into a KnownError/UnknownError first, so retry classification silently fell through to Unknown and withUploadRetry never retried anything for real. - uploadTestResults swallowed its own errors internally, making the retry wrapper around it a no-op; it now lets failures propagate, with logging moved to the plugin-level catch (matching the other upload call sites). - Wire the pacer into every upload call site (global attachments, per-result attachments/fixtures, quality gate), including byte cost for attachment content, not just requests/files. - Add reopenClosedLaunch option: reopens a launch TestOps reports as closed instead of failing the upload outright. - Wrap quality gate upload in withUploadRetry, the one upload path that was missing it.
5b47e9e to
3a989cd
Compare
| onProgress: () => incrementProgress(), | ||
| }), | ||
| { | ||
| onRetry: async (error, attempt) => { |
There was a problem hiding this comment.
try + retry here are wrapping full upload however if 2-3d chunk will fail for example here - retry will duplicate them, is it okay? I mean we should be sure that testops know how to dedup results in this case
There was a problem hiding this comment.
Fixed — retry is now scoped per chunk instead of the whole upload call. Each chunk's POST to /api/upload/test-result retries independently, so a later chunk failing no longer causes earlier, already-acknowledged chunks to be resent.
There was a problem hiding this comment.
Checked this — the server does not dedup by our uuid on re-upload. Test result creation resolves conflicts by internal TEST_RESULT.ID, not by the uuid we send; a repeated chunk gets a brand-new row unless it happens to match a preallocated result via job-run reconciliation (unrelated mechanism). So yes, if a chunk's request actually lands server-side but we never see the response and retry it, that would create a duplicate today. Worth tracking as a follow-up — likely either an idempotency key on the upload endpoint, or having the client only retry on errors that are safe to assume as "not applied" (e.g. connection refused before the request went out) rather than any transient failure.
There was a problem hiding this comment.
Fixed — retry is now scoped per chunk instead of the whole upload call. Each chunk's POST to /api/upload/test-result retries independently, so a later chunk failing no longer causes earlier, already-acknowledged chunks to be resent.
Do we actually need outer retry now, when we cover everything inside operations? As I see we still has another case here:
- chunk 1 POST -> success
- chunk 2 POST -> 503
- inner retry chunk 2 -> 503
- inner retry chunk 2 -> 503
- inner retry chunk 2 -> 503
- client.uploadTestResults throws outside
- outer retry в plugin.ts run full client.uploadTestResults again
- chunk 1 POST -> success second time
… upload A transient failure on chunk N used to make the outer retry replay uploadTestResults() from scratch, resending every chunk that had already been acknowledged by TestOps. Retrying is now scoped to the failing chunk's own post call, so already-succeeded chunks are never resent.
2 participants
Summary
errors.tsclassifies upload failures into transient (5xx/429/timeout), recoverable ("launch is closed"), or terminal (auth/validation/payload/conflict) kinds, recognizing both raw HTTP errors and theKnownError/UnknownErrorwrappers that@allurereport/service's HTTP client actually throws.withUploadRetry()retries only the retryable kinds with exponential backoff, and is wired into all four TestOps upload call sites (test results, global attachments, global errors, quality gate).reopenClosedLaunchoption reopens a launch TestOps reports as closed instead of failing the upload outright.uploadPacer.tsadds a leaky-bucket pacer with independent per-window budgets for requests, files, and bytes sent to TestOps, applied at every upload call site (global attachments, per-result attachments/fixtures, test results, quality gate). Enabled by default with sane defaults (20 req/s, 1000 files/s, 1 GiB/s). Can be disabled viauploadRateLimit: false, or tuned via an explicituploadRateLimitconfig.open.tscommand that only handledSIGINTfor temp-report cleanup — it now handlesSIGTERMtoo, via a small sharedutils/signals.tshelper (notifySignals/waitForAbort) with conventional exit codes (130/143) and a graceful-shutdown deadline.Test plan
yarn vitest runinpackages/plugin-testops— all tests passtsc --noEmit— cleanoxlint— no new warnings introduced