37166 bulk file upload backend - #37358
Conversation
There was a problem hiding this comment.
Guys, do we really need this? Why not use the temp API, which gives us a holding place for files before we convert them to permanent assets, or as a sort of fire-and-forget? With the temp api, we could layer on hooks that do things with the files before we ingest them, things like virus scan the asset, de-dupe the asset, upload the asset to S3, whatever. You can just keep dropping things to the tempAPI and in fact you can see this work if you batch select files and drag them onto the content search screen. I'm not quite sure why we need another upload endpoint that is probably not quite as capable.
Multi-file.upload.mp4
Spec-Kit specification for the server side of Content Drive bulk file upload. Content Drive accepts a multi-file selection today, warns that only one file will be uploaded, and discards the rest. Scope is the backend only; the browser half of #37166 is delivered separately, so the spec carries an explicit "Contract Consumed by the Client" section as the boundary between the two. Decisions recorded in the spec: - This feature defines the shared batch-outcome contract (counts + per-file results); #37062 and #37063 consume it rather than the reverse, so this work is not blocked by an unbuilt ticket. - A configurable maximum file count per batch, refused at submission, and a configurable maximum size per file, recorded as a per-file failure that leaves the batch running. No batch-total size limit. - The submitter is notified on any terminal state, both pushed and durably recorded, following the bulk refresh precedent (#36845 / #37131). This keeps the outcome discoverable after navigation without requiring a client-side jobs screen. Directory upload and creating a missing target folder stay out of scope, as #37166 specifies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> # Conflicts: # specs/37166-bulk-file-upload/spec.md
Adds the browser-side specification for #37166, alongside the backend spec already on this branch. The two meet at the backend spec's "Contract Consumed by the Client" (C-001 ... C-006), restated in the frontend spec from the consumer's side so the boundary is reviewable from either document. No server behaviour is re-specified, and the backend spec is unchanged. Six required user stories plus one optional: - P1 Every chosen file is uploaded, from both entry points. The upload control's file input has no `multiple` attribute today, so only drag-and-drop delivers a batch; the ticket's premise that multi-file selection already reaches the client held for the drop path only. - P1 The outcome is honest: counts come from the server, never from the number of files the author picked, and a partial failure names the files. - P1 In-flight state is reported in one place. Content Drive currently announces "this is running" four different ways, the worst of which replaces the entire listing with skeleton rows for an action fired on one row. - P2 Leaving does not lose the batch, carried by the pushed completion signal (C-006), so no jobs screen is required. - P2 Every outcome names what ran and what it ran on. The copy for the missing cases was written and never wired. - P3 One operation does not block another. - P4 OPTIONAL Stopping a run, deferred: a general task manager is the expected home for it. FR-024 was widened so an unrecognised terminal state degrades to an error rather than a green success, which is what makes deferring it safe. Per-row in-flight marking and bulk reindex's reporting are documented as out of scope, with reasons. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clarify that FR-016's per-file "permission denied" reason is a narrower check than the target-folder permission refused at submission (FR-003) — it surfaces via the reused single-file path (FR-006) on a collision with an asset the author can't edit, or a content type they lack permission to create. Also align the frontend spec's reuse-consumer references (FR-032, SC-008) with the backend spec, which already cites both #37062 and #37063. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…oad work (#37166) The frontend spec mentioned the permission only in an acceptance scenario, an edge case and the Legacy section. Nothing required the client to keep honoring it, which is a gap worth closing given this feature edits the upload control and the drop path directly, and the gate lives on both. Content Drive already gates every route into an upload on CAN_ADD_CHILDREN and offers each one disabled *with the reason* rather than hiding it: the upload control, the create menu, the drop zone, and a re-check on the drop target before anything is sent. The site root resolves its permission separately, having no folder to carry it. Adds FR-034 stating that gate must survive unchanged, two acceptance scenarios on User Story 1, and SC-010. As with the batch-size limit, the client-side gate is an affordance and the server stays the point of enforcement (backend FR-003, FR-004). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cator stays quiet (#37166) Two gaps found while sweeping the spec. **The task manager is not hypothetical.** #33331, "[EPIC] Task manager UI to manage large uploads and moves", already owns it, and already claims cancel: "From this task manager UI, we can also enable the user to cancel individual file uploads, or cancel the whole upload or move." The spec said "anticipated as future work" and named nothing. Deferring User Story 5 is therefore alignment with an existing epic rather than a judgment call this spec made, so all six references now cite #33331. **Nothing said what the indicator should announce.** It is a polite live region today, and this feature adds determinate progress, an item name, and concurrent runs to it. A live region that re-announces on every progress tick is hostile to a screen reader user, and the spec was silent on it. FR-035 limits announcements to the events worth hearing - a run starting, ending, or being stopped - and requires a progress value to be available on request rather than pushed. SC-011 makes it verifiable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The out-of-scope note pointed at an unfiled follow-up. It is now #37322, so the reference names it instead of gesturing at it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Applies the substantive findings from the spec review on #37300. FR-018 said this feature defines the batch-outcome shape. It does not: bulk refresh already ships one, and this feature generalizes it. As written a plan author was licensed to invent a second shape, which is the outcome FR-018 exists to prevent. The three deltas are now named (machine-readable reason code, generic item key, one spelling of the counters), and Q1 records that this reverses the dependency direction #37166 states, so that issue needs amending. Nothing covered the uploaded bytes between the accepted submission and the run that consumes them. A new Staged file content group requires the content to outlive the wait, expiry to be a named per-file failure rather than a silent loss, reclaim on any terminal state, cluster readability, and a stated position on total staged bytes per author. FR-006 required creating each file "through the same path the existing single-file upload uses". That path is entered over REST and a background run cannot re-enter it, so the literal reading was unavailable. It now requires observable equivalence, with the reuse point chosen in the plan. SC-003 measured the batch against today's behaviour, which waits for each file to become searchable - the pathology FR-008 removes - making the criterion true by construction. It now asks for an absolute floor. Also records the domain-endpoint decision and the one-call client contract, and adds a Planning Obligations section for the work these findings uncover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ize rule (#37166) Sets FR-010's maximum files per batch to 50, configurable. Corrects the size and type requirements. The spec assumed no per-file size rule existed on the single-file path and that FR-011 was therefore a new control. It is not: ESContentletAPIImpl.validateBinary enforces both a size ceiling (maxFileLength) and an allowed-types rule (accept), declared per content type on the binary field, during the contentlet validation the creation path runs. Reusing them is not merely cheaper, it is what FR-006 requires: a batch that rejected a file the single-file upload accepts would not be observably equivalent. FR-011 and FR-012 now require surfacing that existing rejection as a per-file failure rather than introducing a second limit. The consequence, accepted knowingly, is that where an operator has configured nothing there is no ceiling. Also aligns the Context section with the FR-018 correction from the previous commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…37166) Addresses the review findings from @ihoffmann-dot. The two blockers raised turned out to be one defect. AbandonedJobDetector re-queues a run whose worker stopped reporting, so "what happens on a restart" was already answered - it is retried. Retrying a batch that created 30 of 50 files, without resuming, creates 30 duplicate assets. Bulk refresh avoided this with a no-retry policy because reindexing twice is harmless; creating a file twice is not. FR-036 through FR-040 therefore require resumable execution: completed files recorded durably as the run goes, one notification per batch across attempts, counts covering the whole batch, and a defined answer for a client that lost the connection mid-submission. FR-041 through FR-043 cover the concurrency gap. The spec described collision against an already-existing file, which the product validates today. It said nothing about two runs racing for the same name, where the existing check-then-create can pass for both. Exactly one must win. On size, the secondary finding got worse rather than better: the previous commit made the per-file ceiling the content type's own, which is unset by default, so count x per-file was no longer a bound at all. There is now a configurable batch total refused at submission (FR-013b), and per file the content type's ceiling falls back to a configurable value where none is declared. That fallback makes a batch stricter than a single upload, which is a knowing exception to FR-006 and is recorded as FR-011a rather than left to be found later. FR-016a settles the raw-message question: the reason code is what the client presents, the message is diagnostic and log-only, matching the frontend spec's FR-030. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ze ceiling (#37166) Takes up three additions to the backend spec's client contract. **C-002b, failure reasons.** The server sends a reason code plus a diagnostic message, and the message is never shown - which the frontend spec already required in FR-030, so the two halves were already aligned on the principle but nothing said the client owed copy for each reason. FR-036 names all six (over size, disallowed type, name collision, per-file permission, staged content gone, unclassified) and states that a reason without copy is a hole the author sees. Staged-content-unavailable is new since the first draft. **C-002a, retry after a lost connection.** Resubmitting the same batch cannot duplicate the author's files. FR-037 makes the client treat an uncertain submission as retryable rather than as a failure the author has to reason about, and forbids asking them to go and check the folder first. **C-002, total size ceiling.** Now a sixth submission refusal alongside the file count. The edge case distinguishes it from "too many files": different problem, different fix, so they cannot share one message. Adds SC-012, and syncs the Contract Consumed section with the backend spec's current C-002 / C-002a / C-002b. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#37166) R2-1. FR-037 requires a mid-run durable record of completed files, which the job framework does not provide: parameters are written once and are immutable, progress is a single number, and the result is harvested only at the terminal state. The resume path carries nothing forward and the processor is request-scoped, so a re-queued run starts with every counter at zero. Resumability is therefore either a new framework capability or a store owned by this feature. Recorded as the first Planning Obligation, with the consequence for FR-018 - a shape emitted once at terminal state cannot serve a run that must read its own progress back. Q6 is strengthened by the same reading: the abandoned-job sweep re-queues unconditionally without consulting the retry policy, so no-retry would not have avoided duplicates either. Resumability is the only answer available, not the better of two. Also notes that content import is marked no-retry and creates content from staged uploads, so it is already exposed to this risk and is not a precedent to follow on retry. R2-2. FR-042 asked the plan to close the concurrent-collision window. The storage layer already closes it - identifiers are uniquely constrained on parent path, asset name and host - so the work is catching the violation and mapping it to the collision reason, and FR-043 comes free because a uniqueness constraint contends only on the contended key. As written it licensed building a lock for a race already lost on the caller's behalf. FR-042a adds the case-sensitivity question the constraint raises. R2-3. Q5, FR-013b and FR-013 could not all hold: content arriving in the same call is only measurable by reading it, so "refused before any content is staged" and "do not trust a caller-supplied size" were incompatible. FR-013c splits it into a fast refusal on the declared total and authoritative enforcement during the read; FR-013d requires reclaiming content staged by a refused submission, which FR-033 covered only for runs. R2-4. failedCount is pinned in the text rather than left to the plan; it was already decided in #37166's amendment and in the PR thread. R2-5. FR-035 required the system to "state a position", which no test can fail. Moved verbatim into Planning Obligations as a cross-batch cap decision. R2-10. Adds the acceptance scenario for FR-011a, the one knowing divergence from single-file behaviour, which had no scenario. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ze ceiling (#37166) R2-7. FR-040's two branches protect the author's data equally but do not report equally. Under the collision branch a resubmission re-uploads every byte, produces a second handle, and reports "50 of 50 failed - file already exists" for a batch that succeeded, which the client cannot tell from a genuinely all-collided batch. FR-040a requires the branch to be distinguishable rather than mandating the handle branch, so the plan keeps the cheaper option. R2-8. The allow-list rule is a media-type rule, not an extension rule: the product detects and sniffs the content rather than trusting the name, so a renamed extension does not get past it. FR-012a says so, and FR-012b records the inherited hole - an unresolvable media type skips the check entirely, which the client must not describe as though every file were checked. "Extension" is replaced with "file type" throughout. R2-9. Deriving stable machine-readable reasons is a Planning Obligation. The validation layer distinguishes its cases largely by localized message text - the size and type rejections are the same exception class differing by a translated string - so the per-file reporting this feature promises rests on work not yet accounted for. FR-042a is answered rather than left open. The review's caveat cited the raw-column constraint, but the operative guarantee is a unique index over the lower-cased full path per host, and the pre-check resolves through the same lower-cased path. The two agree: collision is case-insensitive, and Report.pdf and report.pdf are one name. Existing behaviour, not a new decision, so the spec records it rather than asking the plan to choose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#37166) R2-6. The frontend spec had no Planning Obligations section at all, so it was silent on test coverage. Adds one, leading with the Constitution V obligation: the plan must name which layers it exercises and which it does not, with a reason for each omission. The per-story Independent Test steps are acceptance criteria for a reviewer, not a substitute for it. Carries the submission format in from Contract Consumed, and records that User Story 3 rests on C-003, which the backend now carries as its largest piece of hidden work. R2-3. FR-013c's two-stage ceiling has a client consequence nobody had written down: the fast refusal exists only where the caller declares a total, and the caller is the browser. Without a declared total the only enforcement left is the authoritative check taken while the content is read, which refuses the batch after the author has already uploaded it. New FR-038 requires the client to declare it, FR-006 widens from the file count to the size ceiling as well, and the edge case no longer claims either refusal lands before anything is uploaded. R2-7. FR-037 depended on a guarantee it never named. Under the collision branch a successful retry reports "50 of 50 failed - file already exists", so FR-037 and C-002a now carry the FR-040a distinguishability dependency and require such a retry to be reported as the success it is. R2-8. New FR-039 keeps author-facing copy on file type rather than extension, since the server resolves the media type by detection and sniffing rather than by trusting the name. It also forbids telling the author every file is checked, because an unresolvable type skips the check and is accepted. "extension" is gone from User Story 2. R2-2. FR-036 fixes name collision as case-insensitive and forbids suggesting an alternative name that differs only by case. C-006 drift, both specs. C-006 summarised the pushed signal and the durable record as carrying "the run's counts", while FR-014 to FR-016 record counts and per-file results with a reason for each failure. A plan trusting the summary would have shipped a counts-only notification, leaving an author who stepped away with "27 of 30 created" and no way to learn which three failed. Both C-006s now say what the requirements underneath them already required, FR-020 pins what "the outcome" means there, and the frontend's FR-023 covers the transient and the durable surface alike. New FR-023a groups reasons rather than printing one row per file, so a wholly failed batch of 50 reads as one statement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pec (#37166) A consistency read of both specs against each other, rather than against the review threads, surfaced four places where the backend spec contradicts itself and two where it had fallen behind the frontend. SC-008 promised that a refused submission uploads no file. FR-013c.2 enforces the batch total while reading the content and FR-013d requires reclaiming what was staged, so that refusal necessarily happens after bytes arrive - the criterion could not be satisfied as written. It now states what is actually true of every refusal: no batch and no created file. FR-042 cited the raw-column uniqueness constraint while FR-042a named the lower-cased full-path index, each presented as the guarantee. Only the second is binding, and only it supports FR-042a's case-insensitive claim. The total-size edge case still described a refusal "before anything is staged", the wording FR-013c replaced. Assumptions still described the spec before the per-file fallback ceiling and the batch total existed, claiming no size ceiling out of the box and only one new control. There are three. C-002a did not carry FR-040a, though the frontend's mirror of the same contract already depends on it - the one place the two contract sections had stopped being mirrors. User Story 5 likewise stopped at the branch choice, leaving FR-040a with no scenario and so nothing to test it. Also records why FR-035 is absent rather than leaving the gap unexplained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…37166) Six rounds of edits inserted each new group where it fit thematically, so the numbering ran forward and doubled back: a reader looking for FR-016 found it after FR-043. Moves Outcome, Notifying, Progress and Documentation to their numeric places, so the requirements now read FR-001 through FR-043 in sequence. Text is untouched - this commit only moves blocks, and is kept separate from the content fixes so both diffs stay readable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…7166) Phase 1 planning artifacts for the backend half. Committed, unlike the plan and research, because both carry field-level ground truth a reviewer needs after the PR merges rather than process notes. contracts/bulk-upload-api.md is the definition both halves of #37166 reference. The frontend spec records the submission format as unsettled at spec altitude and requires one written agreement; this is it. Field names in it are binding - multipart parts, the form fields, the 202 shape, the outcome shape and the six failure reasons the client maps to copy. data-model.md carries the job parameters, the one new table, and the extracted shared outcome type. The table exists because the job framework persists no mid-run per-item state: parameters are immutable after creation, progress is one float, and the result is harvested only at the terminal state, so a re-queued run would otherwise restart from the first file and duplicate everything it had already created. The same rows that make the run resumable are the per-file results the outcome reports, so nothing is accumulated in memory and lost on interruption. Two decisions worth finding here rather than in review. The target is two mutually exclusive fields, folderId and siteId, rather than one field carrying either - ADR-0020 deprecated an endpoint for exactly that overloading. And the failure reasons are derived by pre-check and exception class, never by parsing message text, because the size and type rejections are the same exception class differing only in a localized string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…37166) Reverses Q5 after review feedback: the client stages files on the product's existing /v1/temp endpoint, which already accepts several in one call, and the batch endpoint takes the references in a JSON body. It no longer accepts content at all. The argument that settled it is that staging is the product's designed intake for files - its own OpenAPI description prescribes this exact pattern for binary fields, and FieldHandlerStrategyFactory already resolves a temp id into a binary field value. A second intake door would have to re-earn whatever that layer gains later. Three complexities fall out rather than move, which is the real signal: - The media type arrives as a resolved fact from staging, so the failure reasons no longer have to be recovered from a validation layer that distinguishes size from type by translated message text. - The batch total becomes a sum at submission, removing the two-stage enforcement, the 413, and the reclaim-on-refusal requirement. SC-008 returns to its plain form: every refusal is decided before anything is committed. - Per-file upload progress and per-file retry become the client's own, at no cost to this half. What does not change: one job, one handle, one authoritative outcome, one durable notification, a resumable run. Those are what a job exists for and no amount of staging reuse provides them. Two corrections to the data model, both surfaced by review questions. The checkpoint table is now job_item_result rather than an upload-specific one: #37062 and #37063 need the same durable per-item record, and a table per bulk action would mean four near-identical schemas. And the retention claim was wrong - it said rows are purged with the job, but nothing purges the job table at all, so retention is now stated rather than inherited, with a note that a policy for finished jobs deserves its own ticket. Also fills in the configuration defaults the contract had left as "see plan", with the reasoning behind each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
64d25f3 to
004c81e
Compare
|
Claude finished @dario-daza's task in 2m 10s —— View job ReviewThis PR is spec-only — The documents are unusually tight and cross-referenced. Field names, counters ( A few things worth confirming before/while planning — none blocking: New Issues
Notes (non-issues, for the record)
No blocking issues. The spec is ready to proceed to · |
Hi @wezell, thanks for the feedback. You're right that this should go through the temp API, we changed the shape to follow that. But we're adding some tracking that the content search flow doesn't have. What the new endpoint does:
That second part is the whole reason the endpoint exists, and it's the thing content search can't do today. Content search does the same work, but it's a for loop living in a browser tab firing one PUBLISH workflow action per file. So if the author closes the tab halfway through, the remaining files simply never happen and nobody is told. And when something does fail, they get a count ("3 of 50 failed") but not which three, so the only way to find out is to compare the folder against their desktop by hand. What we want isn't a different upload. It's the same N ordinary single creates, tracked as one job, so the batch has an identity and something is keeping track of where it got to. That's what buys us:
We are based in Content Import. It already takes the file as form data, stages it on temp underneath, and hands the work to a job. We're doing the same thing, just with N files instead of one CSV, and with a per-file record so we can actually say what happened to each one. We'll also put a ceiling on the batch (file count and total size) which is more than exists today. We'd rather follow the Content Import shape (one call), but if you'd prefer the client staged on feedback-response-bulk-upload-will.mp4 |
#37166) Restores Q5 to a single multipart submission and records why the two-step shape was tried and dropped. Content import is the shipped precedent: it accepts multipart at its own endpoint, calls the staging API internally, and stores the reference in its job parameters. The two-step draft described it as though the client staged. Two defects the two-step shape carried that this one does not: - Staging expiry is measured per file from its last-modified time, so the clock started when the client staged. On a slow connection the earliest files of a large batch could expire before the author reached submit, and an unresolvable reference refused the whole submission. - Nothing below this endpoint bounds a request. With the bytes already on disk at submission, the file-count and total-size ceilings were a policy about what would be processed rather than a bound on what could be written. Also in this change: - FR-003a / SC-012: the same-origin check and the staging enable switch live on the staging layer's REST resource, not its API, and no global filter covers /api/ paths. Both are applied here explicitly, with tests. The comparable shipped endpoint omits both. - FR-010a / FR-013c / FR-013d: ceilings enforced while the body is read, aborting at the ceiling, reclaiming whatever was staged before the abort. - FR-008a: deferred index policy per file plus one batch-level visibility resolution before the completion signal. Per-file WAIT_FOR also flushes the system-wide query cache on every file; deferring alone would report a run as finished while its files were still unfindable by search. - job_item_result keys on (job_id, seq) rather than (job_id, item_key). The spec allows two files of the same name in one batch, which collided on the old key and left a resumed run unable to tell them apart. - Outcome example corrected to processed: 49 — processed counts attempted, and skipped files were never attempted. requestFingerPrint spelling aligned with the product's getRequestFingerprint. Q4 ("a domain endpoint owning its own multipart handling") and Q6 ("import remains a sound precedent for the one-call staging shape") were both left self-contradictory by the two-step reversal and now read correctly again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FR-010's default goes from 50 to 100. The batch total (FR-013b) stays at 1 GB: 100 files of a few megabytes each is roughly 500 MB, so the realistic batch still clears the total with room. 300 was considered and rejected, and Q2 now records why, because the two ceilings are coupled and so is the submission shape: - At a few megabytes per file, 300 puts a realistic batch past the total ceiling. The count cap would become decorative and the size cap would be the one that surprises people. - Under the one-call shape restored in the previous commit, the whole batch travels in a single request, so the file count also governs how long that request stays open. 300 makes a single upload long enough to be a live risk against a proxy timeout — the one property the two-step shape was better at. Also updated so the numbers stay consistent: SC-003's throughput criterion now reads against the configured maximum rather than a hard-coded 50, Q6's restart argument, the job_item_result row-growth sizing, and the reasoning behind the configured defaults. The FR-008a cache-flush argument is stated per file rather than against a fixed batch size. Illustrative examples elsewhere still use 50-file batches; they remain valid under a cap of 100. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Proposed Changes
Checklist
Additional Info
** any additional useful context or info **
Screenshots
This PR fixes: #37166