Skip to content

Conversation

@icecrasher321
Copy link
Collaborator

@icecrasher321 icecrasher321 commented Feb 10, 2026

Summary

  • File ops -- downloadFileFromUrl should also use centralized timeouts.
  • Billing user passthrough
  • Correctly auth access to resources

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Feb 10, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Feb 10, 2026 11:43pm

Request Review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 10, 2026

Greptile Overview

Greptile Summary

This PR centralizes file download timeouts by switching several call sites from fixed 180s defaults to higher/max execution timeouts. Specifically, it increases the job status endpoint’s estimatedDuration, raises the document processor’s file download timeout, and changes downloadFileFromUrl (and base64 hydration) to default to getMaxExecutionTimeout().

The main concern is that getMaxExecutionTimeout() maps to the maximum (enterprise async) timeout, which can make synchronous HTTP requests wait far longer than the server/route can actually sustain, leading to hung requests and poor UX. Consider aligning defaults with the execution context (sync vs async) rather than always using the global max.

Confidence Score: 3/5

  • This PR is moderately safe to merge, but has a real risk of causing excessively long/hung sync requests due to timeout defaults.
  • Changes are small and localized, but defaulting downloadFileFromUrl (and base64 hydration) to the maximum enterprise async timeout can exceed typical HTTP/serverless limits and degrade request behavior unless callers always override the timeout.
  • apps/sim/lib/uploads/utils/file-utils.server.ts (and callers relying on its default timeout)

Important Files Changed

Filename Overview
apps/sim/app/api/jobs/[jobId]/route.ts Bumped estimatedDuration for pending/processing jobs from 180000 to 300000ms; otherwise unchanged.
apps/sim/lib/knowledge/documents/document-processor.ts Increased document file download timeout constant from 180000 to 600000ms.
apps/sim/lib/uploads/utils/file-utils.server.ts Changed downloadFileFromUrl default timeout to getMaxExecutionTimeout(); this likely uses enterprise async max and can cause excessively long/hung sync requests.
apps/sim/lib/uploads/utils/user-file-base64.server.ts Switched default base64 hydration timeout from 180000ms to getMaxExecutionTimeout() (same concern about overly long max-async default unless callers override).

Sequence Diagram

sequenceDiagram
  participant Caller
  participant FU as file-utils.server.ts
  participant DV as input-validation.server
  participant Fetch as secureFetchWithPinnedIP
  participant Storage as storage-service

  Caller->>FU: downloadFileFromUrl(fileUrl, timeoutMs?)
  alt Internal file URL
    FU->>Storage: downloadFile({key, context})
    Storage-->>FU: Buffer
    FU-->>Caller: Buffer
  else External URL
    FU->>DV: validateUrlWithDNS(fileUrl)
    DV-->>FU: {isValid, resolvedIP}
    FU->>Fetch: secureFetchWithPinnedIP(url, resolvedIP, {timeout})
    Fetch-->>FU: Response
    FU-->>Caller: Buffer
  end

  Note over FU: PR sets default timeoutMs = getMaxExecutionTimeout()
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 10, 2026

Additional Comments (1)

apps/sim/lib/uploads/utils/file-utils.server.ts
Max async timeout default

downloadFileFromUrl now defaults timeoutMs to getMaxExecutionTimeout() (enterprise async timeout). This makes a simple file download potentially wait up to the maximum plan’s async limit (currently 5400s) even for sync/server requests, which can exceed route/platform limits and cause hung requests. It should default to an appropriate sync/request timeout (or require callers to pass one), and reserve getMaxExecutionTimeout() for background/async execution contexts.

@icecrasher321
Copy link
Collaborator Author

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@icecrasher321 icecrasher321 changed the title improvement(timeouts): files/base64 should use max timeouts improvement(timeouts): files/base64 should use max timeouts + auth centralization Feb 10, 2026
@icecrasher321
Copy link
Collaborator Author

@cursor review

@icecrasher321
Copy link
Collaborator Author

bugbot run

@icecrasher321
Copy link
Collaborator Author

bugbot run

@icecrasher321
Copy link
Collaborator Author

bugbot run

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@icecrasher321 icecrasher321 merged commit be3cdcf into staging Feb 10, 2026
12 checks passed
@icecrasher321 icecrasher321 deleted the improvement/file-download-timeouts branch February 10, 2026 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant