Direct artifacts upload by stsewd · Pull Request #13178 · readthedocs/readthedocs.org

stsewd · 2026-07-08T00:55:23Z

Initial implementation of #13134

humitos

This is looking good. This is a preliminary review since I just saw this PR while browsing other stuffs. I know it's in draft.

I'd like to suggest following the APIv3 structure we already have everywhere else and return full objects (or at least re-using the same structure we have) in the API. Example: instead of returning:

build: 12345

we should do

build:
  pk: 12345

https://docs.readthedocs.com/platform/stable/api/v3.html#project-details

The same idea applies to urls and any other field we are returning in this new API.

I would also say that we should re-use UpdateDocsTask and short-circuit it to unzip the file only. That task already handles a lot of known and unknown exceptions. We should re-use that code instead of duplicating it.

humitos · 2026-07-08T09:25:33Z

+    slug = serializers.SlugField(
+        required=False,
+        allow_blank=True,
+        help_text="Optional slug. If not provided, derived from the name.",
+    )


We may need extra validation here. I think we have a specific validator on the Version serializer, but I haven't checked it.

There is a validator in the model, but I ended up dropping this field from the initial implemenation. It requires deciding how to handle syncing of versions. If we automatically sync versions, the slug would already be created.

So, I understand that there is no way to "create a new version" when uploading artifacts, right? The version has to exist already in Read the Docs (meaning that there should be a branch/tag/pr number already created in the repository). Am I correct here?

No, versions can be created on the fly.

humitos · 2026-07-08T09:27:35Z

+    url = serializers.URLField(
+        help_text="URL for uploading the build artifacts.",
+    )
+    # NOTE: url_fields is used to avoid a conflict with the `fields` attribute of the serializer.
+    url_fields = serializers.DictField(
+        source="fields",
+        help_text="Additional fields required for the upload.",
+    )


What about using urls field with nest all the required fields under it? Similar to what we are doing in other serializers: https://docs.readthedocs.com/platform/stable/api/v3.html#project-details -- urls.documentation, urls.home, etc...

Feels like this is a different thing, the fields are part of the response, they are used together with the URL.

humitos · 2026-07-08T09:31:43Z

+
+    build = serializers.DictField()
+    version = serializers.DictField()
+    upload_url = UploadURLSerializer()


Here would be better to use urls. Then we can have:

urls: upload: extra_fields: // data:

That way we avoid creating a new pattern here and re-use the existing one.

they aren't individual URLs, it's just one with the fields for the post request.

humitos · 2026-07-08T09:33:40Z

+    build = serializers.IntegerField(
+        help_text="Build ID returned from the initiate endpoint.",
+    )
+    status = serializers.ChoiceField(
+        choices=["success", "failed"],
+        help_text="Status of the upload after completion.",
+    )


Same here, I would follow the structure we have across the whole APIv3 and return a BuildSerializer instead here as we are doing everywhere else.

This the input for finalizing the upload, this doesn't need a full build object.

Yeah, we can return only the data that's required, but following the structure we already have for the build objects. That way we share the pattern and if we need to add more data here in the future it will looks like a full build object.

Again, this is an input, not an output. The client is giving us the build id and the status of the upload, we aren't returting that.

Gotcha. I thought this was a response.

We should name the field build_id instead of just build, then, to make it explicit.

DRF defaults to using the name of the field for foreign keys when used as inputs for IDs (the only case we have in our code is for subprojects (child is the field for an slug)). If someone else feels this should be renamed, I'd be fine to do it.

stsewd · 2026-08-05T21:41:14Z

This is ready for an initial review. Ignore the upload.sh file, that's only used for testing. I was thinking of having a small GH workflow in this repo to upload on every push/PR to test with our own docs (this would be a different RTD project).

stsewd · 2026-08-05T22:29:20Z

If you want to test this locally, you can get a RTD token from your user (settings > API tokens), and set the required env vars from the upload.sh script (RTD_TOKEN, RTD_PROJECT, RTD_OUTPUT_HTML). RTD_OUTPUT_HTML is where your docs are stored, you can put anything as long as it has an index.html file.

humitos

I'm leaving a few important topics to continue the discussion here, since I think we are still not all in the same page here and we have different ideas in mind. I left more comments in the code itself, but at a high-level, these are the most important ones to me at this point.

API structure responses

My main feedback here is around the API response structure. I really want to follow the API response structure we currently have in APIv3 instead of diverge from there and create a new structure that it's only used for upload artifacts API endpoints.

I understand that there are places where the response should be just a few fields of the build and not the full object, but instead of returning {build: 3, status: cloning} (a complete new response structure); I think it's better follow the current APIv3 structure and return { id: 3 , state: { code: cloning, name: cloning } }. It reduces divergence between API endpoints and allows us to expand the fields returned in the future while keeping a known structure.

Versions allowed to upload

I'm confused about what versions can be uploaded and which ones can not. I understand that with this implementation, any user can upload any version, right? That seems dangerous to me -- the user can easily delete the content of a version by mistake and they cannot go back to the previous state (building it from Git) because we are blocking triggering a build on a version with is_uploaded=True.

I strongly think the user should be in explicit control of this. We should expose a field on the Edit version page the user can switch on/off to allow that particular version to support upload artifacts (#13134 (comment)). False by default.

Allow versions to be created or not

I understood from this thread that we don't want to allow the user to create random versions if they don't exist in Git. However, it seems this PR allows the user to create the version if the slug doesn't exist in the database.

Terminate "not uploaded" builds

We need a way (I think it's not implemented in this PR) to terminate/finish builds that were requested to be uploaded but never completed. We don't necessary need to implement this in this PR, in particular if we are changing the concurrency limit approach on ephemeral builds (see #13203); but something to keep in mind.

humitos · 2026-08-06T09:37:29Z

+    slug = serializers.SlugField(
+        required=False,
+        allow_blank=True,
+        help_text="Optional slug. If not provided, derived from the name.",
+    )


So, I understand that there is no way to "create a new version" when uploading artifacts, right? The version has to exist already in Read the Docs (meaning that there should be a branch/tag/pr number already created in the repository). Am I correct here?

humitos · 2026-08-06T09:42:45Z

+    build = serializers.IntegerField(
+        help_text="Build ID returned from the initiate endpoint.",
+    )
+    status = serializers.ChoiceField(
+        choices=["success", "failed"],
+        help_text="Status of the upload after completion.",
+    )


Yeah, we can return only the data that's required, but following the structure we already have for the build objects. That way we share the pattern and if we need to add more data here in the future it will looks like a full build object.

humitos · 2026-08-06T12:27:39Z

+        # If there are builds triggered/running for this particular project and version,
+        # we cancel all of them and trigger a new one for the latest commit received.
+        for running_build in running_builds:
+            cancel_build(running_build)
+


This method does a lot of stuffs we are already doing in prepare_build:

check if the Project is active

create a Build object

call send_build_status

check for concurrency limits

check for running builds

cancel the current running build if a new one for the same commit/version arrives

There is a bunch of code that could be shared here instead of duplicated. Ideally, I'd like to call prepare_build(upload=True) here and share as much code as possible.

humitos · 2026-08-06T12:34:20Z

+        # Check build is in a valid state for processing.
+        if build.state != BUILD_STATE_TRIGGERED or build.task_id:
+            return Response(
+                {"detail": "Build is already in process."},
+                status=status.HTTP_409_CONFLICT,
+            )


I think I'm not following this. Isn't this logic inverted here?

If there is a build.task_id it means the build was already processed.

If the build.state is not TRIGGERED it means the build was already processed.

Shouldn't it be if build.state not in BUILD_FINAL_STATES instead?

When an upload build is created, it is created in the triggered state, so it's pending to be uploaded. We can also just check for build.task_id, since that means that the build is already queued to post-process.

This is also another case where builds from the upload API benefit from having more states.

Gotcha! The /complete API endpoint has to be called before our build system start processing it. That means the build is TRIGGERED and it was not queued in Celery yet. That makes sense.

We should add a comment here that explains this clearly.

humitos · 2026-08-06T17:52:49Z

+
+        Example: 1234/1111/artifacts.zip
+        """
+        return f"{self.project.id}/{self.id}/artifacts.zip"


I just saw that we are uploading the file under the project_id as the first folder; but we are using project_slug in all the other buckets. Makes sense to keep it consistency and also it's a lot easier to find it while exploring it.

Suggested change

return f"{self.project.id}/{self.id}/artifacts.zip"

return f"{self.project.slug}/{self.id}/artifacts.zip"

I do prefer IDs, they are immutable. slugs can be changed. These are temporary files anyway, if you need to debug something getting the ID or slug is just one query away.

What are the benefits of IDs over slugs here? All the other buckets use project slug, I don't see a reason to have this particular bucket naming it differently here. It only adds confusion.

I do think we should have started with IDs instead of slugs, we now have the problem that if a slug is changed, the files associated with that slug are left there for another project with that slug to take (this isn't often that happens). I don't think we should carry on with that on new features.

That's not wrong, but consistency is more important. We should use slugs like we do for the rest of our storage paths.

I don't agree that consistency should be the reason to keep expanding on a wrong pattern.

* Create bucket for temporary build artifacts uploads Ref readthedocs/readthedocs.org#13178 * This option doesn't exist, it's private by default

…13234) Adds test coverage for `get_s3_build_uploads_scoped_credentials()` (introduced in #13178) in `readthedocs/aws/tests/test_security_token_service.py`, mirroring the existing tests for the build-media and build-tools scoped credentials: - Global (non-AWS/debug) credentials - Scoped credentials with the correct inline policy and session name - Custom `duration` override Stacked on #13178. --- _Generated by [Claude Code](https://claude.ai/code/session_013My5Hgj5E2kuMXyfrZPzJA)_ --------- Co-authored-by: Claude <noreply@anthropic.com>

`prepare_build` (called from `trigger_build`) skips creating a `Build` and returns `(None, None)` when `version.is_uploaded` is `True` — this path didn't have test coverage yet. This adds a test mirroring the existing `test_trigger_skipped_project` case, asserting the build is skipped and `update_docs_task` is never invoked when the version is marked as uploaded. Built on top of #13178. --- *Generated by an AI agent.* --- _Generated by [Claude Code](https://claude.ai/code/session_015dDz3bSrPhVZNMwSJsnGx3)_ --------- Co-authored-by: Claude <noreply@anthropic.com>

Adds test coverage for `RTDS3Storage.generate_presigned_post()` (introduced in #13178) in `readthedocs/storage/tests/test_s3_storage.py`. This follows the existing mock-`_bucket` pattern already used in that file for `delete_directory`/`delete_paths`, mocking `bucket.meta.client.generate_presigned_post` and asserting the `Bucket`, `Key`, `Fields`, `Conditions`, and `ExpiresIn` passed to boto3, both with default arguments and with custom `expires_in`/`min_size`/`max_size` overrides. Stacked on #13178. Note for reviewers: I couldn't run the full local test suite in this session — the checked-out base branch fails to import (`SyntaxError` from Python-2-style `except X, Y:` clauses in several unrelated files, e.g. `readthedocs/core/apps.py`, `readthedocs/integrations/models.py`). This looks like a pre-existing/environment issue unconnected to this change, so I verified the new tests directly against `RTDS3Storage.generate_presigned_post()` outside of the full Django app registry, and ran `ruff format`/`ruff check` on the changed file (both clean). --- _Generated by [Claude Code](https://claude.ai/code/session_01U7muj3LqvSL625pgLBAk4n)_ Co-authored-by: Claude <noreply@anthropic.com>

Adds test coverage for `UploadInitiateView` and `UploadCompleteView` (`readthedocs/upload/api/views.py`), introduced in #13178. This PR targets that branch as its base, since it depends on the `upload` app it adds. Tests focus on the upload API endpoints themselves — authentication, permission checks, the feature flag gate, pending-upload limits, get-or-create version behavior, cancellation of in-flight builds on a new upload, and the concurrency-delayed processing path on complete. Storage (`generate_presigned_post`/`exists`) and the Celery calls (`send_build_status`, `process_uploaded_build`) are mocked throughout, so nothing here exercises the actual build/processing pipeline. A couple of things worth double-checking on review: - `test_cancels_running_builds_for_same_version` asserts on `app.control.revoke` via patching `readthedocs.core.utils.app`, mirroring the pattern used in `readthedocs/builds/tests/test_views.py`. - `test_private_version_not_allowed` relies on `ALLOW_PRIVATE_REPOS=False` being the test-settings default; if that ever changes, the test's expectation should move with it. --- Generated by an AI agent. --- _Generated by [Claude Code](https://claude.ai/code/session_01EZuRwycA8Zk1hgaRNrLbWA)_ --------- Co-authored-by: Claude <noreply@anthropic.com>

humitos · 2026-08-19T11:40:55Z

I re-checked this PR and there are things that are still missing from my point of view to leave it in a mergeable state:

Change the name of the directory where artifacts are uploaded in S3 to use /project.slug/ as prefix
Refactor /initiate endpoint (UploadInitiateView) to call prepare_build(upload=True) instead of duplicating all the same logic in two places
Confirm our decision/position about allow create versions
Move the upload.sh script inside the script/ folder
Delete upload/tasks.py since it's already ported to readthedocs-builder at https://github.com/readthedocs/readthedocs-builder/pull/6
Merge Build: make DAU to work on readthedocs-builder #13219 in to send DAU builds to new builder
Discuss about the name for DAU and update strings accordingly and messages

Note: I've been testing this PR as backend together with new builder implementation and it worked great. We are pretty close to deploy this feature if we can move forward with the previous points 👍🏼

This is based on Santos' PR #13178 to send DAU builds to the new builder's queue. Required by readthedocs/readthedocs-builder#6

stsewd added 3 commits July 7, 2026 19:45

Direct artifacts upload

4cc4fe8

Feature flag

1b2828f

Get it working

90f8da2

humitos reviewed Jul 8, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

stsewd added this to 📍Roadmap Jul 8, 2026

stsewd moved this to In progress in 📍Roadmap Jul 8, 2026

stsewd added 7 commits July 13, 2026 17:51

More like a builder...

85327a7

More checks

039d548

Merge branch 'main' into direct-artifacts-upload

fc0f4d1

More things

aa0aa44

Merge branch 'main' into direct-artifacts-upload

45116d1

Updates

1d5e9df

Merge branch 'main' into direct-artifacts-upload

e22af26

This comment was marked as resolved.

Sign in to view

This comment was marked as off-topic.

Sign in to view

stsewd added 3 commits July 21, 2026 17:50

Updates

160aa56

Use it from env

3307e20

Updates

b66b619

agjohnson assigned stsewd Jul 29, 2026

stsewd added 4 commits August 4, 2026 14:14

Merge branch 'main' into direct-artifacts-upload

fc496dd

Updates

ac1e838

Fix tests

d0d3107

Fix test

bbda75d

Global override broke the extra check

ec66af1

humitos reviewed Aug 6, 2026

View reviewed changes

ericholscher reviewed Aug 6, 2026

View reviewed changes

Comment thread readthedocs/storage/s3_storage.py

humitos reviewed Aug 6, 2026

View reviewed changes

humitos mentioned this pull request Aug 13, 2026

Upload pre-built docs #1083

Open

stsewd added 5 commits August 13, 2026 12:30

Inline

2fc8f5b

Merge branch 'main' into direct-artifacts-upload

5936eb7

No need to return a full response on an error

de03217

Clean up

537b549

We need this

b0270f0

This was referenced Aug 17, 2026

AWS: add tests for build-uploads security token service credentials #13232

Closed

AWS: add tests for build-uploads security token service credentials #13234

Merged

Pass environment variable for bucket name for build uploads readthedocs/common#319

Merged

stsewd and others added 2 commits August 17, 2026 14:16

Merge branch 'main' into direct-artifacts-upload

97dcb47

This was referenced Aug 17, 2026

Builds: test trigger_build for uploaded versions #13235

Merged

Storage: add tests for generate_presigned_post #13236

Merged

stsewd and others added 3 commits August 17, 2026 15:03

Fix tests

9e20f49

stsewd mentioned this pull request Aug 17, 2026

Upload: add tests for the direct artifacts upload API #13237

Merged

humitos reviewed Aug 19, 2026

View reviewed changes

Comment thread readthedocs/upload/api/views.py

humitos and others added 6 commits August 19, 2026 13:38

Build: make DAU to work on readthedocs-builder (#13219)

ade1a84

This is based on Santos' PR #13178 to send DAU builds to the new builder's queue. Required by readthedocs/readthedocs-builder#6

Move

f758924

Make tests green

6d6bba4

Don't use signal

ac40064

More clean up

f7dc503

One more attribute

49effd4

stsewd marked this pull request as ready for review August 19, 2026 22:42

stsewd requested review from a team as code owners August 19, 2026 22:42

	return f"{self.project.id}/{self.id}/artifacts.zip"
	return f"{self.project.slug}/{self.id}/artifacts.zip"

Uh oh!

Uh oh!

Conversation

stsewd commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

humitos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as off-topic.

stsewd commented Aug 5, 2026

Uh oh!

stsewd commented Aug 5, 2026

Uh oh!

humitos left a comment

Choose a reason for hiding this comment

API structure responses

Versions allowed to upload

Allow versions to be created or not

Terminate "not uploaded" builds

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

stsewd commented Jul 8, 2026 •

edited

Loading

humitos commented Aug 19, 2026 •

edited by stsewd

Loading