Skip to content

Add S3 backend to cloudsync#18815

Open
russell-parks wants to merge 12 commits intolibretro:masterfrom
russell-parks:add_s3
Open

Add S3 backend to cloudsync#18815
russell-parks wants to merge 12 commits intolibretro:masterfrom
russell-parks:add_s3

Conversation

@russell-parks
Copy link
Copy Markdown
Contributor

Description

This PR adds an S3-compatible Cloud Sync backend to RetroArch.

S3-compatible object storage is now widely available, and the existing Cloud Sync manifest flow (manifest.server / manifest.local) lets us scope uploads/downloads without requiring bucket list operations. That makes S3 a practical backend while keeping behavior consistent with existing cloud sync architecture.

What this PR adds

  • New S3 Cloud Sync backend with settings for:
    • S3 endpoint URL
    • access key ID
    • secret access key
  • SigV4 request signing for read/write/delete operations
  • Multipart upload support for large files:
    • CreateMultipartUpload
    • UploadPart
    • CompleteMultipartUpload
    • AbortMultipartUpload
  • Multipart plan calculation with S3 limits enforced (part size/count constraints)
  • Fallback to single PUT when multipart initiation encounters transport-level failure
  • URL/region parsing support for common S3-compatible providers (AWS-style and custom endpoint forms)
  • UI/config wiring for S3 cloud sync settings

HTTP/task-layer integration changes required for correctness

During integration/testing, multipart support required a few targeted internal changes:

  • net_http now accepts valid zero-length POST requests when Content-Length == 0
    • needed for multipart initiate requests with empty bodies
  • Added opt-in header access behavior in HTTP task flow
    • needed so multipart part uploads can reliably read response ETag headers
  • Changes were implemented with explicit opt-in patterns to avoid broad behavior changes

Scope and platform enablement

  • This PR currently keeps S3 enablement scoped to macOS Metal configuration.

Notes on multipart threshold strategy

Multipart thresholds are currently baseline-oriented and platform-conditioned in code. This is intended as a functional starting point. Future tuning could include:

  • platform-specific overrides
  • user-configurable threshold
  • adaptive threshold/backoff based on observed throughput/latency

Attribution

This feature was developed with AI-assisted coding/review and human direction for design, troubleshooting, refactoring, and completeness.

Related Issues

No formal issue was opened before implementation.

Integration work in this PR addresses the following discovered runtime issues:

  • multipart initiate requiring zero-length POST acceptance in HTTP layer
  • multipart ETag extraction requiring opt-in response header access behavior

Related Pull Requests

None required in other repositories at this time.

Reviewers

* Ensure we're using C89 standards and other libretro standards
* Use more appropriate logging levels so we don't spam the log
* Fix incorrect S3 part size
Similar to how the WebDAV driver uses async tasks to do network
operations I'm moving the MVP of S3 to do this also.
Copy link
Copy Markdown
Collaborator

@warmenhoven warmenhoven left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor things. I haven't tried compiling or running it yet.

}
}

fprintf(stderr,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I feel like this is better done as additional state on http_t that can then be logged by the caller.

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.

I added this while troubleshooting the -1 http status. I could definitely move it to http_t or even remove it if you prefer, thoughts?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like the idea of being able to introspect what's going on inside of net_http but verbosity/log isn't available and printf/fprintf is not a good answer. So I'd be more in favor of moving it so callers can do the logging. If that ends up being too big or too big for this pr I'd prefer it's just removed entirely (and possibly added in a new pr). Up to you if you want to do that as part of this pr, or just remove it for this one.

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.

Moved this to http_t. Lmk how it looks.

const void *content, size_t content_len, const char *content_type, bool mute,
const char *headers, retro_task_callback_t cb, void *user_data);

void *task_push_http_transfer_with_content_ex(const char *url, const char *method,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

tasks_internal.h isn't public api, it's fine to change it and avoid the _ex suffix.

SETTING_ARRAY("cheevos_leaderboards_enable", settings->arrays.cheevos_leaderboards_enable, true, "", true); /* deprecated */
#endif

#ifdef HAVE_NETWORKING
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why move this down a line?

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.

I was having a problem when I was finding the right place for some of the config and #ifdefs and had forced myself into a faux problem with netplay_mitm_server. Looks like I didn't revert that pushdown, I'll move it back up.

* Support extra prefix in URL
* Cloudflare should support both virtual-host style and path-based URLs
@warmenhoven
Copy link
Copy Markdown
Collaborator

I've merged everything except for the final commit, c289bd1, because I'm still indecisive on what I think a debug API for net_http should look like, and I didn't want to hold up the rest of it waiting on that.

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