Skip to content

Conversation

@SgtPooki
Copy link
Collaborator

@SgtPooki SgtPooki commented Feb 11, 2026

Summary
Adds end-to-end abort propagation with shared utilities, introduces job-level timeout enforcement for deal/retrieval jobs, and improves observability/testing around aborted runs and retrieval failures. Updates HTTP timeout defaults and documents new job timeout env vars.

Problem
Long-running deal/retrieval jobs and downstream steps lacked consistent abort handling, causing wasted work and incomplete observability when timeouts or cancellations occur. Abort reasons could be lost, and job metrics didn’t clearly distinguish aborts from failures.

Solution

  • Added abort-utils helpers (createAbortError, awaitWithAbort, delay) with tests.
  • Propagated AbortSignal through deal/retrieval flows, add‑ons, and IPNI polling; prevent new work on abort while preserving partial results.
  • Job runner now enforces per‑job timeouts (deal/retrieval) via AbortController, records handler_result="aborted", and keeps success vs business failure semantics.
  • Retrieval results carry an aborted flag; errors preserve non‑Error abort reasons.
  • Added/updated tests for abort behavior and error preservation.

Notes

  • New env vars: DEAL_JOB_TIMEOUT_SECONDS, RETRIEVAL_JOB_TIMEOUT_SECONDS (defaults 10m/5m) and docs updated.
  • HTTP request timeout defaults reduced to 4m to align with expected transfer throughput.
  • Metrics doc now describes jobs_completed_total handler result values (success, aborted, error).

Fixes #258

Copilot AI review requested due to automatic review settings February 11, 2026 19:50
@SgtPooki SgtPooki linked an issue Feb 11, 2026 that may be closed by this pull request
@FilOzzy FilOzzy added this to FOC Feb 11, 2026
@github-project-automation github-project-automation bot moved this to 📌 Triage in FOC Feb 11, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds end-to-end abort propagation and job-level timeout enforcement for deal/retrieval workflows so long-running jobs can be actively cancelled while improving metrics/logging around abort vs failure.

Changes:

  • Introduces shared abort helpers (abort-utils) and adopts AbortSignal propagation across deal/retrieval flows (including add-ons and IPNI polling).
  • Enforces per-job timeouts in the pg-boss job runner and records handler_result="aborted" for timed-out executions.
  • Updates defaults/docs for job timeouts and HTTP request timeouts, plus adds/updates tests for abort behavior and error preservation.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
docs/environment-variables.md Documents new job timeout env vars and adds them to the quick reference.
apps/backend/src/retrieval/retrieval.service.ts Propagates abort signals through retrieval execution and adjusts batch behavior on abort.
apps/backend/src/retrieval/retrieval.service.spec.ts Updates tests for abort behavior and aligns Deal IDs with UUID strings.
apps/backend/src/retrieval-addons/types.ts Extends retrieval test result shape with an optional aborted flag.
apps/backend/src/retrieval-addons/retrieval-addons.service.ts Adds abort checks, uses shared abort-aware delay, and improves error capture for non-Error throws (partially).
apps/backend/src/retrieval-addons/retrieval-addons.service.spec.ts Adds test ensuring non-Error throws are captured in execution results.
apps/backend/src/metrics-prometheus/metrics-prometheus.module.ts Documents handler_result semantics for jobs_completed_total.
apps/backend/src/jobs/jobs.service.ts Enforces job-level timeouts via AbortController and reports aborted jobs distinctly.
apps/backend/src/jobs/jobs.service.spec.ts Adds metrics and timeout-abort tests for deal/retrieval jobs; updates private-call signatures.
apps/backend/src/deal/deal.service.ts Propagates abort signal into deal creation/upload/IPNI/retrieval checks; preserves non-Error error messages.
apps/backend/src/deal/deal.service.spec.ts Adds coverage for preserving non-Error abort reasons through deal creation failure recording.
apps/backend/src/deal-addons/strategies/ipni.strategy.ts Propagates abort signal through IPNI monitoring/polling and uses abort-aware delay.
apps/backend/src/deal-addons/interfaces/deal-addon.interface.ts Extends onUploadComplete to accept an optional abort signal.
apps/backend/src/deal-addons/deal-addons.service.ts Propagates abort signal through upload-complete add-on handlers and uses awaitWithAbort.
apps/backend/src/config/app.config.ts Adds config schema + loader for job timeout env vars; reduces default HTTP request timeouts.
apps/backend/src/common/abort-utils.ts Adds shared helpers: createAbortError, awaitWithAbort, and abort-aware delay.
apps/backend/src/common/abort-utils.spec.ts Adds unit tests for abort utilities.
apps/backend/.env.example Adds new job timeout vars and updates HTTP timeout defaults/comments.
Comments suppressed due to low confidence (2)

apps/backend/src/retrieval-addons/retrieval-addons.service.ts:207

  • When a retrieval promise rejects in testAllRetrievalMethods, the recorded error uses result.reason?.message || "Unknown error". If a strategy throws a non-Error (the new spec covers this), .message will be undefined and the reason is lost. Prefer result.reason instanceof Error ? result.reason.message : String(result.reason) so execution results preserve the real failure details.
    const executionResults: RetrievalExecutionResult[] = results.map((result, index) => {
      if (result.status === "fulfilled") {
        return result.value;
      } else {
        // Create failed result - retryCount unknown for catastrophic failures
        return {
          url: urlResults[index].url,
          method: urlResults[index].method,
          data: Buffer.alloc(0),
          metrics: {
            latency: 0,
            ttfb: 0,
            throughput: 0,
            statusCode: 0,
            timestamp: new Date(),
            responseSize: 0,
          },
          success: false,
          error: result.reason?.message || "Unknown error",
          retryCount: undefined, // Unknown for catastrophic failures
        };

apps/backend/src/retrieval/retrieval.service.ts:201

  • performAllRetrievals logs "All retrievals failed" at error level for any thrown error, including aborts thrown via signal.throwIfAborted(). This will create noisy failure logs for expected cancellations/timeouts. Consider skipping the error log (or downgrading to warn) when signal?.aborted is true, similar to the batch-level handling above.
    } catch (error) {
      const errorMessage = error instanceof Error ? error.message : String(error);
      this.logger.error(`All retrievals failed for ${deal.pieceCid}: ${errorMessage}`);


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@SgtPooki SgtPooki self-assigned this Feb 11, 2026
@SgtPooki SgtPooki moved this from 📌 Triage to 🔎 Awaiting review in FOC Feb 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🔎 Awaiting review

Development

Successfully merging this pull request may close these issues.

we need to set deal/retrieval max timeout

1 participant