Skip to content

fix: paginate S3 directory listings#6630

Open
ShiroKSH wants to merge 1 commit into
FlowiseAI:mainfrom
ShiroKSH:bugfix/paginate-s3-directory-loader
Open

fix: paginate S3 directory listings#6630
ShiroKSH wants to merge 1 commit into
FlowiseAI:mainfrom
ShiroKSH:bugfix/paginate-s3-directory-loader

Conversation

@ShiroKSH

Copy link
Copy Markdown

Summary

  • Load all S3 object-listing pages before downloading documents.
  • Preserve the existing object filter and concurrent download behavior.
  • Add regression coverage for the continuation token passed to the second request.

Root cause

S3 Directory made one ListObjectsV2 request, so prefixes with more than 1,000 matching objects silently omitted later pages.

Validation

  • pnpm --filter flowise-components exec jest nodes/documentloaders/S3Directory/S3Directory.test.ts --runInBand
  • pnpm --filter flowise-components exec eslint nodes/documentloaders/S3Directory/S3Directory.ts nodes/documentloaders/S3Directory/S3Directory.test.ts --ext ts --max-warnings 0
  • pnpm --filter flowise-components exec tsc --noEmit --pretty false
  • pnpm --filter flowise-components build

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces pagination support when listing S3 objects in S3Directory by implementing a looping helper function getS3FileKeys and adds corresponding unit tests. Feedback on these changes highlights two main issues: first, since pagination can now retrieve an unbounded number of keys, passing all of them directly to Promise.all for concurrent processing can lead to memory exhaustion, file descriptor limits, or S3 rate limiting, so a concurrency limit should be implemented; second, the prefix parameter in getS3FileKeys should be marked as optional to match the node inputs and ensure type safety.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

)

const keys: string[] = (listObjectsOutput?.Contents ?? []).filter((item) => item.Key && item.ETag).map((item) => item.Key!)
const keys = await getS3FileKeys(s3Client, bucketName, prefix)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

With the introduction of pagination in getS3FileKeys, the number of retrieved keys is no longer capped at 1,000 and can potentially be much larger (e.g., tens of thousands of files). Passing all these keys directly to Promise.all for concurrent downloading and writing to disk can lead to severe issues such as:

  1. Memory Exhaustion: Buffering thousands of file contents in memory simultaneously.
  2. File Descriptor Limits: Exceeding the OS limit for open files (EMFILE).
  3. Rate Limiting: Getting throttled by the S3 API due to too many concurrent requests.

Consider implementing a concurrency limit (e.g., processing in batches of 10-50 files) to make the download process robust and scalable.

import { LoadOfSheet } from '../MicrosoftExcel/ExcelLoader'
import { PowerpointLoader } from '../MicrosoftPowerpoint/PowerpointLoader'

const getS3FileKeys = async (s3Client: S3Client, bucketName: string, prefix: string): Promise<string[]> => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The prefix parameter is optional in the node inputs (line 104) and can be undefined at runtime. However, the getS3FileKeys function signature types prefix as a required string. To maintain type safety and prevent potential compilation or runtime issues under strict null checks, update the parameter type to string | undefined or make it optional.

Suggested change
const getS3FileKeys = async (s3Client: S3Client, bucketName: string, prefix: string): Promise<string[]> => {
const getS3FileKeys = async (s3Client: S3Client, bucketName: string, prefix?: string): Promise<string[]> => {

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