fix(security): pre-onboarding hardening — OLTP statement ceiling, GraphQL off-loop, read-path audit, async op lifecycle - #1215
Conversation
…ssion ceiling The extensions engine is one pool for every tenant and deliberately set no statement_timeout, so a statement left to run held a shared connection for its whole duration. Interactive sessions now bind SET LOCAL statement_timeout alongside the search_path on every transaction (SSM-tunable, 30s default); loader syncs, Dagster jobs, migration-time resync and dev scripts opt out explicitly. A cancelled statement surfaces as a 504 with Retry-After on REST and a statement_timeout answer on MCP, never a bare 500 or the SQL.
…ed errors Every extensions resolver is a plain def that opens an OLTP session, and Strawberry executes those inline on the single uvicorn worker's event loop — one slow read stalled every request on the task. User-written sync resolvers now run through run_off_loop under the shared runner limiter; default attribute resolvers and introspection stay inline. Resolver exceptions that are not deliberate GraphQL errors are replaced with a fixed message plus the request id, and a cancelled statement gets its own code.
…the credential The write gates already emitted AuthorizationDenied; a user-graph read refused at the GraphQL or MCP gate did not, so systematic probing through the read surfaces left no trail. Both now emit the same event. Every authentication success publishes a request-scoped principal (user, method, API-key prefix), the logging middleware binds the request id before the route and reads the caller after it, and security events read both at write time — the access log was recording user_id=None on every request.
…minal failure The synchronous envelope reserved its key with SET NX and released it on failure; the async path fronting create-backup, create-subgraph, materialize, change-tier, ingest-file, sync-connection, auto-map-elements and create-graph was lookup-only and cached an optimistic pending envelope that nothing ever cleared — a failed op replayed pending for 24h under its Idempotency-Key. check_idempotency now reserves after a miss, idempotent_dispatch releases the reservation when dispatch fails, and the SSE store (plus the worker reaper's DLQ path) evicts the bound envelope on FAILED/CANCELLED. The operation audit line also carries request id and credential attribution.
…oth build paths, extend it mid-run The blue-green rebuild proceeded unlocked when the lock module or Valkey client failed to construct, and the first-build path took no lock at all — a stale-sensor run and a user materialize could COPY into the same fresh graph. Both build paths now run under one per-graph lock that fails closed (503 + Retry-After at the API when the lock service is unavailable, 409 when held); the lock is compare-token extended per table so a run longer than its TTL aborts before the swap instead of carrying a stale token; the worker's outer lock release is compare-and-delete; tenant sensor runs carry the materialize_db tag so Dagster's per-graph concurrency limit covers them.
…, under a row cap ingest_file read the whole object and fully parsed it just to count rows — on the API event loop, before the small/large routing — so a 100 MB parquet or JSON payload became a multi-GB allocation that stalled every tenant. Parquet is now counted from the footer alone (ranged GET), CSV is streamed record by record, JSON arrays are walked element by element, all in a runner thread behind a body reader bounded by the HEAD-measured size; a per-file row cap (min of the constant and the tier's per-table limit) rejects with 413. The upload presign signs the declared Content-Length and the S3 client is pinned to SigV4 so a mismatched PUT is refused by S3.
|
@claude can you review? |
|
Claude finished @jfrench9's task in 7m 37s —— View job Review of PR #1215Todo list
SummaryThis is a large, well-scoped hardening pass and the core mechanisms are solid: the statement-timeout ceiling is correctly I did find two real correctness gaps in the async-idempotency eviction path (below), both narrow races but directly relevant to the PR's own "retryable under the same key" guarantee. Issues (should fix, or explicitly accept before merge)
Both are narrow-window races, not likely to fire often, but they undercut the PR's headline idempotency guarantee specifically for the long-running operations (backup/materialize) it calls out. Worth at least a follow-up ticket if not fixed here. Suggestions (non-blocking)
VerdictNo blockers on the headline hardening mechanisms (statement timeout, GraphQL offload/masking, materialization lock) — those are correctly implemented and well-tested. I'd want items #1 and #2 addressed or consciously accepted before merge, since they're real gaps in the async-idempotency guarantee the PR explicitly advertises for backup/subgraph/materialize retries; everything else is non-blocking polish. |
…re cannot evict a completed operation's key Review follow-up on #1215. COMPLETED, FAILED and CANCELLED shared one priority tier under a >= guard, so a late or duplicate OPERATION_ERROR after OPERATION_COMPLETED flipped a finished operation to failed and, with the new eviction hook, freed its Idempotency-Key for a duplicate dispatch. The SSE store now refuses any move between distinct terminal statuses and evicts only when its own write is the transition into failure (a lost CAS evicts nothing). The worker reaper's DLQ write is a WATCH/MULTI compare-and-set that leaves an already-terminal operation alone. Ingest closes the S3 body on every exit, including the row-cap early stop.
|
Review follow-up in 2c26c20:
|
1 participant
Summary
Pre-onboarding hardening pass over the extensions data plane — the availability, detectability and async-correctness follow-up to the multi-tenancy resilience review (#1211–#1213). No authorization semantics change; every fix makes the shared plane hold under load, leave a trail on denial, and keep background operations retryable. Design record and residuals:
local/RoboSystems/specs/security/pre-onboarding-hardening.md.extensions_sessionbindsSET LOCAL statement_timeoutwith the search_path (SSMdatabase/EXTENSIONS_STATEMENT_TIMEOUT_MS, default 30s); loader / Dagster / migration paths opt out. A cancelled statement is a 504 +Retry-Afteron REST,statement_timeouton MCP,STATEMENT_TIMEOUTon GraphQL.OffloadSyncResolvers); only deliberateGraphQLErrors reacherrors[]verbatim (MaskUnexpectedErrors, withrequestId).ContentLengthwhen declared and the S3 client is pinned to SigV4 (verified locally thatcontent-lengthjoinsX-Amz-SignedHeaders; the HEAD-measured size in ingest stays authoritative).AuthorizationDenied(same alarm as the write gates). Every auth success publishes a request-scoped principal; the access log now records the caller (it wasuser_id=Noneon every line); audit lines and security events carryrequest_id,auth_method,api_key_prefix.materialize_dbtag on tenant sensor runs.Deploy notes
database/EXTENSIONS_STATEMENT_TIMEOUT_MS(defaults in code;0disables).Test plan
just test-codecleanjust test— 13,362 passed (incl. real-DB tests for the session ceiling)pg_sleep(60)runs on another tenant's session; confirm the API stays responsive and the sleeping session gets 504/STATEMENT_TIMEOUTAuthorizationDeniedin the security stream withapi_key_prefix+request_id