Skip to content

fix: aws storage construction#895

Merged
yamijuan merged 1 commit intomainfrom
juan/aws
Mar 3, 2026
Merged

fix: aws storage construction#895
yamijuan merged 1 commit intomainfrom
juan/aws

Conversation

@yamijuan
Copy link
Contributor

@yamijuan yamijuan commented Mar 3, 2026

Description

Fixed get_dailyco_storage() to construct AwsStorage explicitly with only role_arn, instead of using Storage.get_instance() which auto-collects all DAILYCO_STORAGE_AWS_* settings. This prevents the ValueError: cannot use both aws_role_arn and access keys crash when both DAILYCO_STORAGE_AWS_ROLE_ARN and DAILYCO_STORAGE_AWS_ACCESS_KEY_ID/SECRET_ACCESS_KEY are configured in the same environment.

The root cause: Storage.get_instance(settings_prefix="DAILYCO_STORAGE_") iterates all settings matching that prefix and passes them to AwsStorage(**config). After the platform-agnostic source storage feature added DAILYCO_STORAGE_AWS_ACCESS_KEY_ID and SECRET_ACCESS_KEY (for worker reads via get_source_storage()), the get_dailyco_storage() factory started passing both role_arn AND access keys to the constructor, hitting its mutual-exclusion validation.

The fix explicitly constructs AwsStorage with only the fields get_dailyco_storage() needs (bucket, region, role_arn), keeping it cleanly separated from get_source_storage("daily") which uses only access keys.

Added 11 new tests covering all storage factory construction cases.

Related Issue

Follow-up to the platform-agnostic source storage PR. The poll daily meetings worker was crashing because get_dailyco_storage() picked up the newly added worker access keys alongside the existing role_arn.

Motivation and Context

In selfhosted deployments with Daily.co, two separate credential sets coexist under the DAILYCO_STORAGE_AWS_* prefix:

  • ROLE_ARN: Used by the Daily API to write recordings to S3 (via get_dailyco_storage())
  • ACCESS_KEY_ID / SECRET_ACCESS_KEY: Used by Hatchet workers to read/delete recordings from S3 (via get_source_storage("daily"))

AwsStorage intentionally rejects having both auth methods simultaneously. The fix ensures each factory only passes the credentials it needs.

How Has This Been Tested?

29 tests pass (uv run pytest tests/test_storage.py -v), including 11 new tests:

get_dailyco_storage() (5 tests):

  • Constructs with role_arn, no access keys
  • No conflict when access keys are also in settings (key regression test)
  • Falls back to us-east-1 when region is None
  • Raises ValueError without bucket
  • Exposes role_credential, bucket_name, region properties

get_whereby_storage() (2 tests):

  • Constructs with access keys via Storage.get_instance()
  • Raises ValueError without bucket

get_transcripts_storage() (2 tests):

  • Garage endpoint with path-style addressing
  • Standard AWS S3

get_source_storage() (1 test):

  • Falls back to us-east-1 when region is None

Coexistence (1 test):

  • All three factories (get_transcripts_storage, get_dailyco_storage, get_source_storage) work simultaneously with the real selfhosted config (Garage + role_arn + access keys)

@yamijuan yamijuan changed the title fix aws storage construction fix: aws storage construction Mar 3, 2026
@yamijuan yamijuan merged commit f5ec2d2 into main Mar 3, 2026
6 of 7 checks passed
@yamijuan yamijuan deleted the juan/aws branch March 3, 2026 18:04
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.

2 participants