feat(api): bulk upload guards — concurrency cap, stall abort, DB timeouts (#2362)#2370
Open
jh-RLI wants to merge 10 commits into
Open
feat(api): bulk upload guards — concurrency cap, stall abort, DB timeouts (#2362)#2370jh-RLI wants to merge 10 commits into
jh-RLI wants to merge 10 commits into
Conversation
Adds POST /api/v0/tables/<table>/bulk-upload - the tracer bullet of the bulk upload path (slice 2 of #2362): - The request body IS the CSV (text/csv); the server streams it into PostgreSQL COPY FROM STDIN without buffering the file in memory. - Append-only, one transaction per request: a malformed row anywhere rolls back the entire upload. - The delimiter is a required, whitelisted parameter (comma, semicolon, tab) - never inferred from metadata or content. - The CSV header (required) maps columns by name; header names are whitelisted against the table's actual columns and quoted, so no unvalidated identifier ever reaches the SQL. - Same authorization chain as the row API: token auth, write permission, embargo check, table-registry resolution (internal tables are unreachable by construction). - Deliberately bypasses the edit-journal meta tables: bulk-loaded rows have no per-row change history. This trades the (currently unread) per-row provenance for order-of-magnitude ingestion speed; an audit event record follows in a later slice. - COPY is FROM STDIN only; no code path for COPY FROM file/PROGRAM. New HTTP-seam test module api/tests/test_bulk_upload.py (14 tests) covers the happy path per delimiter, auth/permission/embargo denials, all-or-nothing rollback, journal bypass, and target-table containment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Slice 3 of #2362, on top of the bulk upload tracer bullet: - Header preflight before the body streams: reject duplicate column names, names not in the table, and missing required columns (NOT NULL without default), each with a 400 naming the offenders. - Strip a UTF-8 BOM from the header (Excel exports). - FORCE_NULL on all uploaded columns: an empty field is NULL whether quoted or not - a deliberate deviation from COPY's native CSV rule, because many writers quote every field and would silently store empty strings instead of NULLs. - Sanitized failure responses: the database's data-level message plus the CSV line number and column (header-adjusted), never raw SQL, server context dumps, or internal paths - including the no-diagnostics fallback (lost connection), which stays generic. Test module grows to 20 HTTP-seam tests covering each contract rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#2362) Slice 4 of #2362. Clients may include or omit the id column: - Omitted: the table's id sequence assigns ids as usual. - Included: after COPY, still inside the upload's transaction, the id sequence is advanced to the table's max(id) via setval(GREATEST(...)), so a subsequent row-API insert cannot hit a duplicate key. GREATEST plus a pg_advisory_xact_lock on the sequence keep the sequence from ever moving backwards, including under concurrent uploads (setval is non-transactional, so racing reads could otherwise regress it). - Uploads that RAISE the table's max(id) above a generous sanity bound (2^48) are rejected and rolled back, so a single upload cannot exhaust the id sequence for every writer of a shared table. The bound only judges ids introduced by the upload itself: a pre-existing high id (the row API enforces no bound) does not poison the table for future bulk uploads. Test module grows to 25 HTTP-seam tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Slice 5 of #2362. Bulk uploads bypass the per-row edit journal; the new BulkLoadEvent model is their provenance: - Every authenticated, authorized attempt - successful or failed - is recorded: table, user, timestamp, status (success / validation-error / copy-error / embargo), sanitized error message, and bytes received. Decorator-level denials (401/403/404) deliberately create no events so anonymous requests cannot write database rows. - Successes additionally record the row count and the exact id range of the loaded rows (found via xmin = current transaction, so it is correct for both explicit and sequence-assigned ids); the response references the event id. The id range is the forensics handle for block-deleting a mistaken or malicious upload. - Events live in the Django database, so a failure event survives the data transaction's rollback by construction. Event creation is best-effort: a failure to write the audit record is logged but never masks the upload's actual outcome. - Events are listed, filterable, and immutable in the Django admin. Test module grows to 30 HTTP-seam tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Slice 6 of #2362, part 1/3. Content-Encoding: gzip (and legacy x-gzip) request bodies are decompressed in streaming fashion straight into COPY FROM STDIN - gzip.GzipFile pulls compressed chunks on demand, so the whole body is never materialized in memory. Corrupt or truncated gzip aborts with a sanitized 400 (validation-error event) and full rollback, whether it fails at the header or mid-COPY. Tests: gzip round-trip vs plain twin, invalid gzip, truncated gzip mid-body.
Slice 6 of #2362, part 2/3. Adds the size-cap status to BulkLoadEvent (with migration) so uploads rejected by the decompressed size cap are distinguishable in the audit trail and admin filters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Slice 6 of #2362, part 3/3. A configurable cap on DECOMPRESSED bytes per request (settings.BULK_UPLOAD_MAX_BYTES, env-overridable, default 10 GiB) rejects oversized uploads with HTTP 413 naming the cap and a size-cap Bulk Load Event. Counting after decompression is what neutralises gzip bombs: a few-KB body that inflates past the cap is cut off mid-stream, not expanded to disk or RAM. The cap is a backstop - clients are expected to split large datasets into several uploads. psycopg2 surfaces exceptions raised inside COPY's read() as a generic Error, so the stream flags the overflow and the error handler dispatches 413 size-cap vs 400 copy-error on that flag. Tests: plain-body cap breach (413, names the cap, rolled back, event recorded) and gzip bomb cut off at the cap. Module at 35 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ap (#2362) Slice 7 of #2362, part 1/3. New module api/bulk_upload_guard.py: at most one running bulk upload per user plus a configurable global cap (BULK_UPLOAD_MAX_CONCURRENT, default 2). Excess requests get HTTP 429 with Retry-After and create no event - busy rejections are cheap pre-work denials. Slots are always released, including on error paths. The guard state is per worker process (documented: a multi-worker deployment bounds concurrency per process, still capping the damage). The guard module is deliberately its own seam with direct unit tests - concurrency cannot be exercised through the synchronous test client. HTTP-seam tests cover the wiring: per-user 429 and global-cap 429, both with Retry-After.
Slice 7 of #2362, part 2/3. Uploads whose average transfer rate falls below BULK_UPLOAD_MIN_BYTES_PER_SECOND (default 10 KiB/s) after a grace period (BULK_UPLOAD_STALL_GRACE_SECONDS, default 30 s) are aborted with HTTP 408, fully rolled back, and recorded as a stall Bulk Load Event (migration 0051) - a trickling client must not pin a synchronous worker and an open transaction. The StallDetector lives in the guard module with an injectable clock for its unit tests; it only catches trickle (each read eventually returns) - fully hung connections are bounded by the web server's timeouts and the DB session timeouts (next commit). Like the size cap, the abort surfaces through psycopg2 as a generic error, so a flag on the detector decides the failure class.
Slice 7 of #2362, part 3/3. The upload's transaction runs under SET LOCAL statement_timeout (BULK_UPLOAD_STATEMENT_TIMEOUT_MS, default 1 h) and idle_in_transaction_session_timeout (BULK_UPLOAD_IDLE_TX_TIMEOUT_MS, default 60 s), so a client that goes silent mid-protocol cannot hold an open transaction and its locks. SET LOCAL scopes both to the transaction, keeping the pooled connection clean for its next user; the in-code fallbacks mirror the settings defaults and fail closed (never 0/disabled). Together with the concurrency and stall guards this completes the worker-protection story: guard slot -> transfer rate -> statement and idle timeouts, each bounding a different way to pin a worker. Includes the changelog entry for the complete guards feature.
5 tasks
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
1 participant
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.
Summary of the discussion
This PR stacks ontop of #2364 and #2366 and #2367 and
#2368 and
#2369 wait until both are merged and rebase this PR.
Part of #2362 (Slice 7 — guards). The synchronous bulk upload path is
protected by guards rather than infrastructure (ADR 0002). Three commits,
one guard each:
c4689e84): new moduleapi/bulk_upload_guard.py— at most one running upload per user plus a global cap
(
BULK_UPLOAD_MAX_CONCURRENT, default 2). Excess requests → 429 withRetry-After, no event (busy rejections are cheap pre-work denials).
Slots always release, including on error paths (50-failure leak test).
Guard state is per worker process — a multi-worker deployment bounds
concurrency per process (workers × limit), documented in the module.
400f0481): uploads averaging belowBULK_UPLOAD_MIN_BYTES_PER_SECOND(default 10 KiB/s) after a graceperiod (default 30 s) → 408, rolled back, recorded as a
stallevent (migration 0051). Catches trickling clients; fully hung
connections are the web server's and DB timeouts' job.
3b10d788): the upload's transaction runsunder
SET LOCAL statement_timeout(default 1 h) andidle_in_transaction_session_timeout(default 60 s) — transaction-scoped, so pooled connections stay clean; fallbacks fail closed.
The guard module is the design's one below-HTTP seam: 9 direct unit tests
with an injectable clock (concurrency and wall-clock can't be exercised
through the synchronous test client), plus HTTP-seam tests for the wiring
(per-user 429, global-cap 429, stall abort with rollback + event, timeouts
verified on the session). 48 tests across both modules; full suite green.
Known minor gap (accepted): a client above the stall floor but slow enough
to hit
statement_timeout(e.g. 500 MB at 100 KiB/s ≈ 1.4 h) surfaces as ageneric copy-error naming the postgres timeout message, not a dedicated
status. The observability slice's outcome vocabulary can refine this.
Workflow checklist
Automation
Closes #
PR-Assignee
CONTRIBUTING.md
CHANGELOG.md
mkdocs
Reviewer
Reviewer Guidelines