Repository navigation
turbo: a resume continues the upload it names, or is refused - #1736
Closed
sebastian-zentral wants to merge 14 commits into
Closed
sebastian-zentral wants to merge 14 commits into
sebastian-zentral wants to merge 14 commits into
Conversation
A command collects something from a machine instead of producing a verdict. Two kinds land with it: sysdiagnose, which takes no options, and file_export, which takes a list of absolute path patterns and an uncompressed max_size. One Command model on BackendInstance with a backend registry, not one model per kind. Script and MSCPCheck each earn a table because Postgres enforces their identity — FKs with DB semantics, typed ODV columns and their constraints. A command has neither: its variation is agent behaviour, which is Python. That is the split stores.Store and probes.Action already use, and a command that grows an FK graduates to its own definition model under the same Job anchor. Job.kind grows one value per backend, flat, so a new command is a config change for an agent rather than a protocol change: an agent already drops a kind it does not know, element by element. Job.definition_relations() replaces the eleven select_related sites that listed the definition relations by hand. Which makes a kind the running release does not know a shape every endpoint has to hold: during a rolling deploy an older instance reads rows a newer one wrote. Job.definition returns None there, and each endpoint an unknown kind can reach handles it rather than dereference it — a machine holding that job would otherwise get a 500 on every poll until the refresh completes, taking the whole batch with it. Config skips the job. Results and status set the entry aside as unknown_job_kind: results records the run, for the same reason kind_mismatch does — the run happened, only the outcome is unusable — and status writes the tracker first, or the next sweep marks a job the agent is still holding as removed. The wire takes `kind` as a short string, not a choice over the kinds THIS release knows, for the same reason. A report of a run that happened is not a malformed entry, and rejecting it drops the entry whole: no acknowledgment for the agent to read, and a shot that never closes. The resolved job decides, which is where the authority already was — results only compares the wire kind against it, and status never reads it at all. A command runs one time only. A backend declares its allowed_modes, the recurring form narrows its job choices to the kinds that accept a recurring schedule, and the recurring serializer says so. A recurring collection is a log shipper, not a job. A backend also declares the artifacts a run produces — name, filename stem, extension, content type, and whether it is optional. file_export declares two: a required manifest of what was collected, and an optional archive, absent when a run matched nothing. The declaration is the server's, never the agent's. The upload plane, which reads it, comes later. file_export matches its patterns with fnmatch(3), which is what macOS gives the agent for free. fnmatch has no recursive wildcard, and it does not reject ** — it reads two stars as one, so a pattern written for recursion collects exactly one directory level and reports no error. The server refuses ** rather than let an operator discover that from a short archive, and the pattern cap is 32 because a tree now costs one pattern per level. The console is read-only for commands, like it is for stores and probe actions: the API validates the per-backend options, and the detail page exists because every job list links a definition's get_absolute_url. The one-time job search covers command names, and the detail page joins a list option rather than rendering its Python repr. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
createOneTimeJob, updateOneTimeJob and deleteOneTimeJob become typed PBAC
actions. createOneTimeJob takes the CONFIGURATION, because the row it creates
does not exist yet. The other two take the ROW, which by then does: it is a
member of its configuration, so one configuration-scoped policy covers all
three, and it carries the job it runs, so an update or delete policy can read
resource.job. All three carry the JOB as an entity in the context, so one policy
can name them all — create has no row to read it from, so the context is the
only thing all three share:
forbid (
principal,
action in [Turbo::Action::"createOneTimeJob",
Turbo::Action::"updateOneTimeJob",
Turbo::Action::"deleteOneTimeJob"],
resource
) when { context.job.kind == "file_export" };
An entity rather than a kind string because it composes: context.job.kind says
what a job_kind key said, and anything else the job comes to declare is readable
without reshaping every action that carries it. The closed set of kinds is not
lost — the values list is declared on Turbo::Job's own kind attribute, where the
schema browser still shows it.
turbo.add_onetimejob, turbo.change_onetimejob and turbo.delete_onetimejob are
RETIRED — unmapped, so nothing can ask these actions through has_legacy_perm
with System::"any" and an empty context. Every request carries a resource. That
is what the console gates its buttons on now, through view-computed booleans
rather than perms.turbo.*, with authorize_one_time_job_rows batching one
decision per row because the rows differ by job. That batching hands the shared
_object_jobs.html partial a list where it used to get a queryset, so its heading
counts the rows with length: a list has no argument-free count(), and the
template engine renders the empty string for a call that needs an argument
instead of raising — the heading read "One-time job ()" on all three definition
pages. The API empties perms_map for POST, PUT and DELETE and decides in
perform_create / perform_update / perform_destroy, on validated objects rather
than primary keys off the wire.
viewOneTimeJob stays legacy-mapped, deliberately: a typed view action would have
to scope a LIST, which means filtering a queryset by policy rather than deciding
one request. A test asserts it stays that way so nobody types it by halves.
context.job is REQUIRED and a policy reading it needs no guard, because the
question the console asks before a job is picked is a preview, and the engine
holds back the part of a policy that reads the missing context instead of
failing on it. permit (…) when { context.job.kind == "sysdiagnose" } therefore
works on its own: it offers the action, allows a sysdiagnose and refuses a
file_export, with no has guard and no companion forbid — through the API as well
as the console, which a test pins in both places.
A preview offers an action, it never gates one: it answers "could be permitted",
and PBACViewMixin and PBACPermission both refuse a request that carries no
context. So the two scheduling forms open on a view permission —
turbo.view_onetimejob under a configuration, turbo.view_enrolledmachine on a
machine page — and the typed action decides on the submit, where the job is
known. The console offer and the job picker are the preview's whole job.
Converted Role policies keep working: they name the actions and leave the
resource open, so they authorize both the preview and the bound request. A role
converted with add_onetimejob but without view_onetimejob no longer opens the
form, which the changelog calls out.
Every kind is covered, not only the commands. A script runs arbitrary code as
root, and add_onetimejob never distinguished it from an mSCP check either.
Scheduling from a machine page now also requires turbo.view_enrolledmachine. The
configuration comes from the enrolled machine, so it is only known once the
serial resolves and cannot be the gate that runs first — and a 404 against a 403
must not reveal which serials are enrolled.
The shared base views expose the configuration as a cached_property, so an
authorization mixin can use it as its resource without any code reading the
database before the mixin runs — resolving it eagerly turned an anonymous
request for an unknown pk into a 404 instead of the login redirect. They no
longer carry PermissionRequiredMixin either: each concrete view declares its own
gate.
The pickers offer only the kinds a policy would permit, so a kind-scoped grant
no longer walks an operator into a 403 with the form lost. The CHOICES are
narrowed, not the field's queryset: narrowing the queryset moves the gate from
authorization to form validation, where a refused kind comes back as "select a
valid choice" without ever reaching the typed check, and nothing is logged as a
permission denial. A forged post still reaches the check and is refused there.
On an update the field keeps its current value, even if a policy would refuse
that kind today.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signing an upload delegates a write, where signing a download is a read of an object the server already has, so the capability gets its own predicate rather than riding on file_storage_has_signed_urls: only the S3 backends do it. Everything declared travels as a signed HEADER — content-length, content-type and the checksum — so the storage itself rejects a body that does not match, and Zentral never reads a byte to find out. That only holds under SigV4, and botocore reaches for it only when the signature version was set EXPLICITLY. Where it is merely resolved from the endpoint — the default in us-east-1, and behind any custom endpoint_url — generate_presigned_url falls back to the legacy V2 query signature: the length is not signed at all, and the checksum becomes a query parameter S3 does not read. Measured, not assumed, and silent in both directions. So the version is forced here rather than trusted, on a client of the storage's own session so that uploads authenticate exactly like downloads. The key goes through the storage's own name normalisation, so the client writes where django-storages reads. With a `location` set, a presigned PUT built from the bare name would put the object outside the namespace that exists(), delete(), url() and a later HEAD all look in. That means calling a private method — but it is the one url() itself uses, and the two paths have to agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One row per (run, artifact), created by the first mint and closed by that run's result. Named for the JOB and not the command: a command is grouped under one model because per-kind reification is not justified for it, which has nothing to do with the reason this plane exists — some jobs produce artifacts, and a reified kind that collects a file writes here too. Keyed like the wire rather than like the ORM — schedule_pk + schedule_mode, no foreign key to either tracker — so one column covers both scheduling models and a recurring upload kind needs no migration. The cost is no cascade, so cleanup will have to find these rows by hand. The multipart columns and the verification axis are here from the start. They cost nothing empty and save two migrations later. Status and verification stay apart on purpose: "the agent says it uploaded" and "the storage agrees" are two facts, and the second must never reopen a shot the first closed. One row per run and artifact is the ONLY uniqueness asserted, and the absence of the other one is deliberate. An index over the uploaded rows of a one-time schedule looks right and is false: a version bump re-arms a one-time job, so a machine can legitimately hold two runs of one shot and upload for both. Such an index makes the second result an IntegrityError, and under ATOMIC_REQUESTS that is a 500 on the whole batch, on every retry, until someone deletes a row by hand. The gate belongs where it can see the version — last_result_at on the tracker, which the mint reads. The artifact declaration and the size ceiling are read through the DEFINITION, never through the command backend: a definition declares what a run of it produces and how large that may be, and that is all the upload plane needs to know. Both are properties on Command rather than getattr() against the model — BackendInstance.__getattr__ raises for a name it does not know, so a getattr with a default would silently answer None and the kind's ceiling would never apply. A Script and an MSCPCheck declare neither: a verdict is not an artifact. get_machine_schedule() is the mint's resolver, and deliberately not the batch one: it creates no tracker row, so a run that mints and never reports leaves no trace in a ledger it does not own. Its rules are the config view's, because a schedule the agent could not have been served is one it must not be able to mint against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A command collects a file, and the file does not travel in the result. The agent asks POST /public/turbo/uploads/ for a destination, sends the artifact there, and reports the upload in the result. Just in time, and never in the config payload: that payload is a pure function of (definition, version), cached and re-served to every in-scope machine, so a URL in it would be expired for the machines that needed it most and a live write credential for the ones that did not. The server owns the key. It is stable for the life of the row, so a retry re-signs the same key and overwrites in place instead of leaving a half-written twin, and the filename a browser saves is settled at the first mint. At ingest the echoed key is COMPARED to the minted one, never taken from the wire. Two destinations, one contract. A presigned PUT signs the length and the digest as headers, so the storage rejects a mismatch and Zentral reads no bytes. The hosted destination has to read them, and checks both itself: the length first, then the digest, which a wrong length also fails but explains less well. That check is not redundant with S3 — it is the only moment a file system storage can ever verify the bytes, because it reports no checksum to read back later. Only S3 can presign today, so a GCS deployment always takes the hosted path. The ceilings are the deployment's, not ours. Without a presigned storage the limit is DATA_UPLOAD_MAX_MEMORY_SIZE, because that is the wall the body actually meets; publishing anything else would spend the artifact, the upload and every retry discovering it. The kind's own maximum clamps under it. The config publishes the result so the agent can refuse a file before collecting it, alongside the digests to fold into its single read pass. No part size: nothing the agent computes depends on the chunk geometry. Every wire value is checked at the boundary, because past it they reach bounded columns and an encoder: a length check on a sha256 is not a format check, and 64 characters of non-hex reach unhexlify as a 500 where the agent needed a 400. Rows are bounded too. Attempts bound a row and nothing bounded the rows — run_id is the agent's, and a gate only closes on a result, so a machine that never reports could mint a fresh write credential for every id it invented. The count excludes the row the request is about, so the bound falls on a new row and never on a retry: reporting the runs already held is the only way the rows ever clear. The result closes the row, on the agent's word alone — whether the storage agrees is a second axis, and it comes later. Partial success is first-class: a file_export that uploads its manifest and gives up on the archive keeps the inventory of what it collected, and an artifact that never happened leaves no row to sit pending. Ingest stays tolerant: an entry that cannot be matched is logged, never a reason to fail a batch whose run did happen. WireError grows a status, because the code alone cannot tell the agent whether to report a failure or say nothing at all: 404 and 410 for a schedule that is gone or a shot already closed, 413 over a maximum, 429 when too much is open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…main A stacked pull request is based on the branch below it, so the branches filter on the pull_request trigger meant it got no checks at all — the review that most needs them, since the diff is the only thing separating it from a PR that would run them. The push trigger keeps its main filter: without it every branch push would run the whole suite a second time next to the pull request run. merge_group keeps its filter too. A merge queue only exists for main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
stat_object() reports the size and the whole-object digests the storage itself recorded, and returns None when there is nothing at the key — only then. Every other failure raises, so a storage that is unreachable, or a key the deployment may not read, is never mistaken for an object that is not there. Two S3 details it depends on, both silent when missed. HeadObject reports a checksum ONLY when the request asks for it, so without ChecksumMode the response is checksum-free and a comparison against it would pass for any object at all. And a missing key surfaces as the bare status 404, not NoSuchKey — HeadObject has no body to carry an error code — so an `except NoSuchKey` would never fire. Both are pinned by tests stubbed against the real service model. The HEAD gets an explicit ceiling instead of botocore's minute-scale defaults, and one attempt. It runs on a device endpoint inside the request transaction, so an unbounded call to a storage that has gone quiet would hold a web worker AND a database connection; a retry would double how long that transaction stays open to buy an answer that was never urgent. sha256_object() reads an object and hashes it, for the storage that records no digest of its own — affordable exactly because that is the storage with the small ceiling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When the agent reports an upload, Zentral asks the storage about the object at that key and compares the size and the SHA-256. The two numbers reach that comparison by two independent paths. One went out at the mint and was signed into the upload, so the storage refused any body that did not hash to it, and comes back as the checksum the storage recorded. The other came home in the result. Verification is where the two copies meet, and on a storage that signs, Zentral reads no part of the file to do it. Both legs are checked. An agent whose result contradicts its own mint is describing a different file, and comparing the stored object against the mint alone would miss it — on a signing storage that comparison is nearly tautological, since S3 enforced it on the way in. verified / mismatch / missing lands on the row's own field, next to the status and never instead of it, and rides the echoed entry into the result event as a boolean. The event now also carries run.id — what an upload row is keyed on, without which the event describes an artifact nothing can find. Some answers are not answers, and none of them is recorded. A storage that timed out, a key we may not read, and an object with no stored digest all leave the row pending, with a log entry: a transient blip that marked artifacts permanently missing would be worse than no answer at all. On a signing storage a missing digest is never a reason to read the object back — that would mean downloading two gigabytes to answer a question. **A mismatch does not reopen the gate.** That is what two axes are for: the shot was consumed when the agent reported, whatever the storage says afterwards. The uploads now close before the wire ref is built rather than after. The verdict has to ride the echoed entry, and enriching the outcome in place afterwards would only have worked by aliasing a dict two callers hold — the shape that has already cost this codebase one class of lost events. The storage is resolved once for the batch and passed down, the way build_upload_destination() already takes it: nothing in the upload plane reaches for the default on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
create_multipart_upload() declares ChecksumAlgorithm: CRC64NVME, and that declaration is the one line of this design that must not be dropped. Without it S3 accepts a WRONG whole-object checksum at completion, and HeadObject afterwards reports a perfectly correct one because S3 computed its own — the object looks verified and is not. ChecksumType and the object size at completion are genuinely optional; the algorithm is not, and their optionality is not license to drop it. CRC64NVME rather than SHA-256 because S3 offers a full-object checksum on a multipart upload only for the CRC algorithms: only those compose from part checksums into a checksum of the whole object. The presigned part goes through the SigV4 client like every other presign here. A part carries no checksum on either storage, so its length is the entire contract — and the legacy V2 presign signs no header at all, which would leave a part free to be any size. Measured again here: a raw client presigns V2 in us-east-1. ListParts paginates. Reading a truncated first page as the whole list would assemble a partial object and call it done, and Zentral's own geometry cannot fill a page — which is exactly why a truncated answer must not be mistaken for a complete one. Two client ceilings now, because the calls are not alike: the ones inside a device request keep the short one, and the assembly gets ten minutes, since both storages document that it can take several. The stat client's constants are renamed to say which of the two they are. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A large artifact goes up in parts. The mint answers mode: "multipart" with the geometry and one signed URL per part — the geometry comes back WITH the URLs, which is the whole reason it is not published or cached anywhere. The agent sends the same body either way and reads the mode off the answer, so there is nothing for it to predict, no part size to cache, and no stale-configuration two-step. The threshold is TWO part sizes. At one there is no resumability to buy: a part_size + 1 artifact is a two-part upload whose second part is one byte, so losing the first part costs the retransmission that restarting a single PUT would have. The whole multipart bill would be paid to protect a one-byte tail. At two, the first failure loses at most half the transfer. A multipart artifact is refused without crc64nvme. sha256 is composite-only on a multipart upload, so the whole-object CRC is the only thing that can validate the assembled object; without it completion would accept whatever arrived. Nothing uses the CRC on a single PUT, and a single PUT is not refused for want of it. Starting the upload is a storage call on a device endpoint, so it answers like one: a ClientError or a timeout is a 503 storage_unavailable, not the HTML page Django renders for an unhandled exception on the one endpoint contracted never to serve one. ATOMIC_REQUESTS would also roll the row back while the storage may already hold the upload id — an orphan for the lifecycle rule. The agent mints again, which is the right thing to do about a storage that was busy. Completion is the server's. The agent posts the three keys that identify the upload and gets a 202; the server lists the parts from the storage — never from the wire — and completes with its own credentials. That is the one place the agent would have needed storage-specific knowledge: collecting ETags, building a completion body, and on S3 finding an error inside a 200 OK. It runs in Celery because completion is documented as taking minutes, and the agent has no use for the answer anyway: it sent the bytes, and whether the storage assembles them lands on the verification axis. Idempotent, so the call can be retried freely. The task maps each answer from the storage onto that axis. A checksum that does not match the bytes is terminal — assembly_failed — because no retry improves it and retrying keeps the parts alive and billed. NoSuchUpload is ambiguous: the storage says it both for an upload that completed already and for one whose parts were aborted, and the object itself is the only thing that tells them apart — by its whole-object CRC, which the HEAD reports and the row holds, so the match is exact where a match on the size alone can be a coincidence. Everything else is transient and gets backoff. That minutes-long completion is also why each writer of the row names the fields it owns. The task loads the upload before ListParts and CompleteMultipartUpload, and the result for the same run arrives inside that window — the normal sequence, not a race to be unlucky about. A full save from either side would put a stale copy back over the other's axis: the status would return to pending for good, since the result was acknowledged and the agent never resends, and the row would go on counting against MAX_PENDING_UPLOADS while reading as a report that never came. update_fields on both, the task writing the verification and the ingest the status. The single-PUT path has never needed this — it decides its verification inline, in the request that writes the status, so it has no second writer. Result ingest does not HEAD a multipart row: completion already validated the whole object, so there is nothing left to ask. What it does instead is notice that a completion is still owed — the agent's call can have been lost, and a result is often the first thing to arrive — and drop the parts when the agent reports giving up. The bucket's AbortIncompleteMultipartUpload rule is the backstop for the report that never comes, and it is now load-bearing. A resume re-signs only the parts the agent names, against the row's OWN part size, fixed when the multipart upload was created. It is refused when it declares a different size or digest: those parts are the bytes the row describes, and re-signing against a new geometry would hand out parts that cannot line up with what is already in flight. The presented upload id is checked against the row rather than believed. Two small changes to shared code the 202 needed. The base POST view grows a response_status, and the turbo_request event posts on any 2xx rather than a literal 200 — otherwise an endpoint that only accepts the work would emit no event at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The page of an enrolled machine lists the artifacts its jobs collected — one row per upload, with its size, both of its axes, and a download once the artifact is in. Two typed actions, sharing one applies_to and differing only by group. viewJobUpload is Admin/User/Viewer: seeing that a job collected a sysdiagnose is an ordinary view. downloadJobUpload is Admin/User, because reading one hands over the data of the person who uses the machine. The distinction the prose already drew, made structural rather than left to policy — a deployment that never writes one still gets it. The USER-only roster guard in tests/server_accounts now records that decision next to the MDM secret readers. An artifact the user may not see is ABSENT from the page rather than merely unlinked, so its name and its size do not leak either. Two decisions per row, batched into one evaluation, the way the scheduling rows already are: the rows differ by machine, by configuration and by job, so a single answer for the page would be wrong. The resource is the upload, and it has TWO containers because they answer different questions. Inventory::Machine answers whose data this is, and brings meta business unit scoping with it. Turbo::Configuration answers whose collection produced it, and is what lets one forbid cover the whole lifecycle — scheduling and artifacts together. With only the machine parent that policy would silently cover the first and miss the second. Both are pinned, and so is the limit of the first: the meta business units of a machine come from its inventory SNAPSHOTS, not from the enrollment secret, so a machine that has enrolled and never posted inventory is in none of them as far as a policy is concerned. Defence in depth, not a boundary — which is exactly why the configuration parent exists. The docs say so rather than leaving it to be discovered. The row is keyed like the wire and holds no foreign key to either scheduling model, so resolve_upload_schedules() resolves the schedule polymorphically — one lookup yielding the configuration and the job, which happen to be the two things both the page and a decision need. An upload whose schedule has since been deleted is dropped: the row survives, because the artifact is still in the bucket, but there is no configuration to name and no job to read, so there is nothing to decide against. The download signs ResponseContentDisposition. The key already ends in the artifact's filename, so a browser saves the right name from the path alone, but a small manifest.json would open in the tab instead of being saved. A 404 for a missing object is expected rather than exceptional: the expiry rule removes objects out from under rows that outlive them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Zentral writes the collected artifacts and never removes them, so how long they are kept is a property of the bucket and not of Zentral. The docs said to configure one rule; there are three, and the one that decides the retention of files holding the data of the person who uses the machine was not among them. An Expiration rule on turbo/uploads/ is what removes an artifact at all, with 14 days offered as a default and the reasoning for it. AbortIncompleteMultipartUpload covers the parts of an upload that never completed, on the whole bucket rather than that prefix, because any client can start a multipart upload. And a versioned bucket needs ExpiredObjectDeleteMarker in a rule of its own, since one Expiration cannot carry both that and a number of days. The 404 the console returns for an artifact a rule removed is documented where the rules are, rather than only where the download is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two bugs the tests could not see, both in the download. The view read select_dist_storage(), and every write in this plane goes to storages["default"]: the mint presigns against it, the hosted PUT saves to it, the verification HEADs it, the completion assembles into it. select_dist_storage is for the MDM artifacts, and a deployment that configures a separate dist storage sent every download to a bucket that never held the file — a 404 from the storage after the redirect. Without a dist storage the two coincide, which is exactly why the tests passed. The new test configures one, and fails without this. And the signed GET passed a disposition to storages that cannot take it. file_storage_has_signed_urls is true for GCS too, so that is the branch a GCP deployment takes: ZentralGoogleCloudStorage overrides url() as url(self, name), so the parameters argument is a TypeError rather than an option the storage ignores, and django storages' own GoogleCloudStorage would hand the dict to generate_signed_url, which spells it response_disposition. A 500 on every download, either way. The disposition is signed on S3 now and nowhere else: a GCS deployment gets a plain signed URL, and a small manifest opens in the tab there. Mapping the option per backend would mean teaching a shared storage class an argument for one caller. One MetaMachine per serial instead of one per row. get_meta_machine_resource caches the parents on the object, so a new one per row was a query per row, on a page where every row carries the same serial. The kind filter runs after the decision now, on the job it already resolved, rather than resolving every schedule a second time to filter first. The docs say that the keys sit under the storage's location when one is set, so the rule needs the prefix that reaches them. A rule on the bare prefix matches nothing, and nothing reports that the artifacts are not being removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A mint carrying upload_id / missing_parts for a run with no row used to create one, start a fresh multipart upload and sign only the missing parts of it, an object that can never assemble. Seen on zaio: the agent resumed under a new run_id, part 2 went into an empty upload, and the completion failed with 64 MiB of 200 MiB. The resume now has to match an existing row on its run and upload id, and is refused with unknown_upload_id before anything is written. An empty upload_id is invalid, since it would match every row whose multipart upload has not started. The completion task checks the stored parts against the row's geometry first, so a gap is recorded as missing_parts by number instead of as the storage's InvalidRequest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Report for CI Build 36580568916Warning No base build found for commit Coverage: 91.088%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
np5
force-pushed
the
turbo-upload-console
branch
from
September 30, 2026 10:17
d532c4c to
05f976c
Compare
Member
|
Changes included in #1686. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1687.
Problem
The agent resumed a multipart upload under a new
run_id. The mint found no row for that run, created one, started a fresh multipart upload, and signed only themissing_partsthe agent asked for. Part 2 went into an empty upload, and the completion failed:The original upload stayed
pendingwith parts 1, 3 and 4 in S3, and neither upload could ever complete. The 2026-09-28 14:01 failure was the same case.Changes
upload_idmust match an existing row on schedule, serial, run, artifact and upload id. Otherwise it is refused withunknown_upload_idbefore anything is written, so no multipart upload is started and no empty pending row counts againstMAX_PENDING_UPLOADS.upload_idisinvalid_upload_id, since it would match every row whose multipart upload has not started.part_sizegeometry before calling CompleteMultipartUpload. A gap is recorded asmissing_partswith the part numbers, instead of surfacing as the storage'sInvalidRequest. The outcome is unchanged (ASSEMBLY_FAILED), with one storage call fewer.A resume under the same run and upload id, as the current agent sends it, works as before.
Tests
tests.turbo: 688 tests pass. New tests cover a resume under an unknown run, a resume without any row, an emptyupload_id, and a completion with a missing part.🤖 Generated with Claude Code