Skip to content

extstore s3 driver#2129

Open
cconstable wants to merge 10 commits into
mainfrom
extstore-s3-driver
Open

extstore s3 driver#2129
cconstable wants to merge 10 commits into
mainfrom
extstore-s3-driver

Conversation

@cconstable

@cconstable cconstable commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

What was changed?

  • Adds the built-in S3 driver for External Storage
  • @temporalio/external-storage-s3: S3StorageDriver + S3StorageDriverClient interface, no
    AWS dependency.
  • @temporalio/external-storage-s3-aws-sdk: AwsSdkS3StorageDriverClient, backed by
    @aws-sdk/client-s3 (peer dep).
  • @experimental not yet wired into the converter pipeline.

Why?

  • The two-package split keeps the AWS SDK out of core, so users can supply their own S3
    client.

Checklist

  • New tests added

@cconstable cconstable requested a review from a team as a code owner June 17, 2026 16:00
@cconstable cconstable changed the base branch from main to extstore-foundation June 17, 2026 16:04
@cconstable cconstable force-pushed the extstore-s3-driver branch 2 times, most recently from 6f23506 to 2f945b0 Compare June 17, 2026 16:37
@cconstable cconstable force-pushed the extstore-foundation branch from 1b6c2b9 to 447552b Compare June 17, 2026 16:38
Base automatically changed from extstore-foundation to main June 17, 2026 20:09
@cconstable cconstable force-pushed the extstore-s3-driver branch from fa18b39 to a9e4554 Compare June 18, 2026 15:38
{
"name": "@temporalio/external-storage-s3-aws-sdk",
"version": "1.18.1",
"private": true,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

prevents npm publish from exposing these until extstore is released. we'll want to flip this later.

Comment on lines +94 to +108
async function runAllWithAbortOnError<T>(
external: AbortSignal | undefined,
makeTasks: (signal: AbortSignal) => Promise<T>[]
): Promise<T[]> {
const controller = new AbortController();
const signal = external ? AbortSignal.any([external, controller.signal]) : controller.signal;
const tasks = makeTasks(signal);
try {
return await Promise.all(tasks);
} catch (err) {
controller.abort();
await Promise.allSettled(tasks);
throw err;
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

n.b.

  • S3 driver's runAllWithAbortOnError is concurrent and cancels in-flight sibling requests on the first failure (AWS SDK allows for cancelling mid-request).
  • GCS driver's allSettledOrThrow is concurrent but doesn't attempt mid-flight cancellation. @google-cloud/storage can't cancel an in-flight request. Abort in GCS is only check up-front.

@jmaeagle99 jmaeagle99 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.

LGTM with a few minor comments. Please seek an TS maintainer for review as well.

Comment thread contrib/external-storage-s3/src/__tests__/test-driver.ts Outdated
Comment thread contrib/external-storage-s3/src/__tests__/test-driver.ts Outdated
Comment thread contrib/external-storage-s3/src/driver.ts

describe(): Record<string, string> {
const region = this.client.config?.region;
return typeof region === 'string' && region ? { clientRegion: region } : {};

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.

region might also be a Provider<string | undefined> as documented at https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-client-s3/Interface/ClientInputEndpointParameters/

Do we want to invoke the provider, check if the promise is completed (assuming the promise is cached), and read the value? If this is even possible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that region will be set to the string when the promise resolves which is essentially what you are suggesting. We can't check promise status.

Comment thread contrib/external-storage-s3-aws-sdk/src/aws-sdk-client.ts Outdated
* @experimental
*/
export class AwsSdkS3StorageDriverClient implements S3StorageDriverClient {
constructor(private readonly client: AwsS3Client) {}

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.

I think the way we are using this client, it does not support multipart upload. We might either need to depend on another package or write it ourselves using this client. There is varying recommendations of at what threshold multipart should be used, but it looks like AWS says to definitely use it at 100 MB or larger, which is above our max payload size. So maybe something to defer for now, what for feedback and/or measure with experimentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good callout. I've documented this. Agree on deferring for now.

Comment thread contrib/external-storage-s3/README.md Outdated

> ⚠️ **This package is experimental and may be subject to change.** ⚠️

`@temporalio/external-storage-s3` stores and retrieves Temporal payloads in Amazon S3 via the [External Storage](../../README.md) feature.

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.

Why does External Storage link to the README? did you mean docs: https://docs.temporal.io/external-storage

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea I suppose that is better! I originally had this linking to the in-repo docs.

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.

nice README!

Comment thread contrib/external-storage-s3/src/driver.ts Outdated
Comment thread contrib/external-storage-s3/src/driver.ts Outdated
Comment thread contrib/external-storage-s3/src/__tests__/test-driver.ts
Comment thread contrib/external-storage-s3/src/driver.ts Outdated
Comment thread contrib/external-storage-s3/src/driver.ts Outdated
await Promise.allSettled(tasks);
throw err;
}
}

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.

Remind me - what do we do when an operation fails, is it a task failure and then we retry?

Just thinking of the scenario when store fails partway through. Subsequent retries should attempt to store only the payloads where objectExists == false ?

What about if retries are exhausted and we've partly completed the store operation? I guess there isn't much we can do

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. We fail the task and it must be retried at the task-level. If we are trying store many payloads and one fails, the whole thing fails.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right now, none of the drivers will retry. I think we should have some sensible retry policy to address transient failures. Will chat with the team about this.

@cconstable cconstable force-pushed the extstore-s3-driver branch from eed8180 to e5ae665 Compare June 25, 2026 15:45
async store(context: StorageDriverStoreContext, payloads: Payload[]): Promise<StorageDriverClaim[]> {
const contextSegments = buildContextSegments(context.target);
return runAllWithAbortOnError(context.abortSignal, (signal) => {
const uploads = new Map<string, Promise<void>>();

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.

That's an interesting optimization! Should we do the same for downloads too?

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.

3 participants