THOTH-GQL-BATCH-01: request-scoped GraphQL batching foundation and mutation request guard - #791
Conversation
Implements ADR-0006 / THOTH-GQL-BATCH-01. The merged state is inert: guard mode OFF, store derived-unavailable, no request-path overhead and unchanged production request acceptance. Store (thoth-api/src/graphql/batching.rs) Request-scoped, owned by one GraphQL request, keyed by the full (top-level response key, loader identity, normalized load shape, parent key) identity, applied uniformly to queries and mutation payloads. Three unambiguous states - NotLoaded, Loaded (including Loaded([])) and LoadFailed - of which only NotLoaded may direct- fallback. Reads are non-destructive, dispatch is set-based over a de-duplicated key set, failures are retained and never retried, and whole-store invalidation is provided. Availability is derived from MutationGuardMode alone, so OFF/OBSERVE + store enabled are structurally unrepresentable. The WIP dispatch de-duplication was replaced with an explicit unique-key loop. Mutation guard (thoth-api/src/graphql/mutation_guard.rs) Central request guard with modes OFF (default), OBSERVE and ENFORCE, wired at the HTTP boundary before ordinary Juniper execution. OFF returns before parsing. Otherwise a baseline eligibility gate reproduces pinned juniper 0.16.2's own pipeline using juniper's own helpers, so a baseline-invalid request yields no decision, no event and no guard error and juniper stays the sole authority for its error. Effective variables mirror the executor's or_insert; @Skip and @include are evaluated against them; fragment expansion is cycle-safe without suppressing distinct occurrences. Scope shim (thoth-api/src/graphql/scope.rs) The single permitted new_error(..) -> path() response-scope extraction site, side-effect-free and fail-closed, with the pinned- Juniper coupling and revalidation obligation documented and tested. Prefetch (thoth-api/src/graphql/prefetch.rs) Look-ahead-driven set-based prefetch covering direct and descendant paths, alias-safe on field_original_name() at every segment, collecting every matching terminal selection and writing into the ordinary terminal namespace. Proven by a test-only loader, schema and mutations; no production field adopts the foundation, no production child resolver and none of the 88 MutationRoot resolvers are modified, and the generated SDL is byte-identical. No migration, schema.rs, policy.rs or dependency change. Merge does not authorize activation: production OBSERVE and ENFORCE each require separate explicit CTO authorization and remain NOT AUTHORIZED.
…changelog Records the implementation evidence required by specification section 14: the per-scope SQL statement-count tables for the direct, descendant and mutation-payload paths at two list sizes; the two-top-level-field result showing 2 dispatches rather than N + N; terminal-loader and intermediate- resolver counts as separate figures; the descendant prefetch representation; the compatibility shim's signature, side-effect freedom and fail-closed behaviour; the baseline eligibility gate and its correspondence to pinned juniper's own pipeline; the baseline-invalid and directive/effective-variable matrices asserted against Juniper's observed execution; the measured zero-resolver, zero-write evidence; observability and redaction evidence; and the generated-SDL and protected-path identity results. Separates implementation/merge readiness from production activation readiness. Production activation remains BLOCKED - runtime-operations / CG-13 and monitoring-threshold evidence unverified. Records the accurate request-path cost (parse and operation selection on every request in OBSERVE/ENFORCE, plus validation on mutations only), characterises the doc(hidden) juniper surfaces without calling them stable public API, and does not claim rollback is certain or deploy-free. Also records one inconvenient finding: the repository's .map_err(Into::into) idiom hits juniper's blanket From<T: Display> impl and drops the extensions.type discriminant, so existing production child resolvers emit none.
…itly Specification section 10 requires query-path behaviour to be proven directly, not only inferred from the mutation cases, because the eligibility gate touches every request in OBSERVE and ENFORCE. Adds four cases: a valid query is never restricted and emits no event in any mode; its response is byte-identical to the no-guard baseline in every mode; an invalid query keeps juniper's canonical error and produces no guard event in any mode; and a query carrying a duplicate top-level response key is accepted, shares one scope across both occurrences, and issues no additional terminal statement for the second. Updates the implementation report's test counts and adds the corresponding evidence section.
…tion report Completes section 3 with every commit from the authorized base, notes that the inherited WIP commit was not rewritten, rebased or force-pushed, and states that the authoritative exact head is the PR head — the head that requires fresh independent cross-model review and separate explicit CTO merge authorization, and which any further movement invalidates.
Exact-head CI verificationHead: Every run below is bound to that exact SHA (verified via
PASS vs SKIPPED. No check was skipped.
|
Independent review of acf4ca2 returned CHANGES REQUIRED. No architecture, production adoption or activation control was changed. 1. Guard rejection no longer has its own HTTP branch. The handler carried `return Ok(HttpResponse::BadRequest().json(rejection))`, which is the extra branch and bespoke status mapping the specification forbids even though the status happened to match. Both paths now produce one GraphQLResponse and fall through the single pre-existing `match result.is_ok()` branch, which is now the only status-producing construction in the handler. Three tests drive the real handler through actix_web::test and show a guard rejection and an ordinary juniper validation failure sharing the status and body convention, plus a success case exercising the 200 arm so the branch is provably common rather than failure-only. No new dependency: the assertions are made against the raw body because this crate has no serde_json, and the precise structural body comparison stays in thoth-api where it does. 2. Two-loader isolation is now proven with two loaders. The previous test instantiated a single loader and asserted a structural property, so it did not satisfy its own name. TestImprintDescendingLoader adds a second closed LoaderIdentity sharing the same parent-key type, value type and shape identity, differing only in the discriminant and in returning rows descending, so a namespace collision returns observably wrong data. Both loaders are dispatched: A's dispatch leaves B NotLoaded, B still needs its own dispatch, each lookup returns its own value, B does not overwrite A, and one (scope, key, shape) holds two entries. A failure under A also leaves B loadable and survives B's dispatch. Verified non-vacuous by temporarily giving B A's identity, which fails the test at the expected assertion. 3. Same-scope multi-site reuse is now proven with two sites. The previous test invoked one site once. TestTwoSiteContainer exposes two sibling fields that each resolve their own parent list and install their own prefetch site, both below one top-level field so both derive the same scope, loader, shape and key set. Per-site outcomes are recorded: the first site is Loaded, the second AlreadyLoaded, on both execute_sync and async execute. Measured terminal-loader SQL for the pair is 1 at n=3 and n=6 with zero fallbacks. Async needed no coordination primitive and none was added; this was measured rather than assumed, and a separate test keeps the same-scope case distinct from correct cross-scope isolation. The implementation report is corrected rather than left standing: the previous "no new handler branch", two-loader and multi-site claims are recorded as unsupported at the reviewed head, with the new tests and actual measured results in their place.
The OFF-mode case posts a real mutation, so it must not be able to write to the shared test database. Anonymous requests stop at PublisherPolicy::can_create before Publisher::create runs; asserting that explicitly proves both that the request reached ordinary execution rather than being turned away by the guard, and that the test performs no write. Verified by row count before and after: unchanged.
Independent-review remediation —
|
| Case | Mode | Status | Body |
|---|---|---|---|
| duplicate top-level response key (guard rejection) | ENFORCE |
400 | {"errors":[…]} with message + locations, no data key |
| unknown field (ordinary juniper validation failure) | OFF |
400 | same shape |
{ __typename } |
ENFORCE |
200 | carries data — proves the branch is common, not failure-only |
| same duplicate document | OFF |
— | no guard rejection; stops at the authorization check, so no write |
Preserved: rejection before any resolver executes, resolver count 0, database writes 0, ordinary validation-style body, HTTP 400 via the common mapping.
2. Two-loader collision isolation — now two actual loaders
The previous test did not satisfy its name. It instantiated one loader and asserted a structural property. TestImprintDescendingLoader adds a second closed LoaderIdentity sharing the same parent-key type (Uuid), value type (Imprint) and shape identity (asserted equal in the test), differing only in the discriminant and in returning rows descending — so a collision returns observably wrong data.
| Step | Observed |
|---|---|
| before any dispatch | A NotLoaded, B NotLoaded |
| after A dispatches | A Loaded; B still NotLoaded |
| B then dispatches | Loaded — B still needs its own dispatch |
| A lookup | ["AAA-first","BBB-middle","CCC-last"] |
| B lookup | ["CCC-last","BBB-middle","AAA-first"] — A not overwritten |
entries under one (scope, key, shape) |
2 |
A failure under loader A also leaves B loadable and survives B's dispatch.
Verified non-vacuous: giving B loader A's identity makes the test fail at exactly loader A's dispatch must not satisfy loader B. The identity was restored immediately.
3. Same-scope multi-site reuse — now two actual sites
The previous test invoked one site once. TestTwoSiteContainer exposes two sibling fields, each resolving its own parent list and installing its own prefetch site, both below one top-level field so both derive the same scope, loader, shape and key set.
| Execution path | site left |
site right |
entries | fallbacks |
|---|---|---|---|---|
juniper::execute_sync |
Loaded (one dispatch) |
AlreadyLoaded |
one per parent | 0 |
async juniper::execute |
Loaded |
AlreadyLoaded |
one per parent | 0 |
Measured terminal-loader SQL through the instrumented pool:
SAME-SCOPE TWO SITES | scope=testTwoSites | rows (n, terminal stmts, fallbacks)
= [(3, 1, 0), (6, 1, 0)]
Total terminal-loader SQL for the pair = 1, both list sizes, both execution paths. All parent results correct and identical between sites.
No coordination primitive was needed, and none was added — this was measured, not assumed. The async path shows one dispatch because the fixture's resolvers are synchronous bodies and the pinned executor drives a selection set through FuturesOrdered polled from a single task, so a site's read-load-write completes before its sibling runs. Had it shown two, request-local coordination would have been added rather than the criterion relaxed. No global/static cache, background task, external lock manager, dependency or production state was introduced.
A separate test keeps this distinct from cross-scope isolation, where two top-level response keys correctly require two dispatches.
Exact-head CI — f6506212
| Check | Result | Duration |
|---|---|---|
classify ×3 |
PASS | 6–9s |
format_check |
PASS | 9s |
lint |
PASS | 1m48s |
build |
PASS | 6m9s |
test |
PASS | 7m56s |
check-changelog |
PASS | 8s |
run_migrations |
PASS | 7m18s |
build_and_push_staging_docker_image |
PASS | 11m25s |
SKIPPED: none. Only the 8 pre-existing thoth_api doc-tests are ignored, as on the base. CI test counts (real postgres:17 + redis:alpine), matching local exactly:
thoth-api 973 passed; 0 failed (869 pre-existing + 104 added)
graphql_permissions 13 passed; 0 failed (unmodified)
thoth-api-server 3 passed; 0 failed (new handler tests)
thoth-export-server 143 passed; 0 failed
thoth-errors 11 passed; 0 failed
thoth-client 4 passed; 0 failed
thoth (bin) 1 passed; 0 failed
TOTAL: 0 failures
Local gate at the same head: cargo fmt --all -- --check, cargo check --workspace, cargo test --workspace, cargo clippy --all --all-targets --all-features -- -D warnings, git diff --check — all exit 0.
Boundaries preserved
Unchanged versus the authorized base df2e2efe: schema.rs, migrations/**, Cargo.toml, Cargo.lock (all three crate manifests), policy.rs, CI workflows, mutation.rs (all 88 MutationRoot resolvers), query.rs, the pre-existing GraphQL test suite, PR #788 and issue #765. Generated SDL still byte-identical (sha256 1e08b46b…), with no test-only type leaked into it. new_error(..) remains a single call site in scope.rs. No BE-02 symbol anywhere. No new dependency, no migration.
Merged default remains MutationGuardMode::Off, store unavailable, no added OFF-mode parsing or validation, request acceptance unchanged. OBSERVE and ENFORCE remain NOT AUTHORIZED; BE-02 runtime remains NOT AUTHORIZED; CG-13 and the monitoring-threshold blockers are untouched and still open.
This is remediation and CI evidence only. It is not a review, not an approval, and not merge authorization. The reviewed SHA has changed, so this head requires a fresh independent review, and merge additionally requires separate explicit CTO authorization bound to the approved exact head.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Remove transient review state from the durable report
Once PR #791 is independently reviewed or merged, this committed paragraph becomes false because it says the current head still requires fresh review and CTO authorization. Keep live review and authorization status in GitHub and make the implementation report state only the durable decision and authority condition, so merging does not immediately leave stale control evidence on the integration branch.
AGENTS.md reference: docs/engineering/AGENTS.md:L56-L59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Authority
THOTH-GQL-BATCH-01- Request-scoped GraphQL batching foundationADR-0006(APPROVED, repository-authoritative)52259739035226006860df2e2efef176716e8c8d523457b30e3deebab770developImplementation authorization is not merge authorization and not production activation authorization.
The merged state is inert
Store availability is derived from the single
MutationGuardModevalue, soOFF + store enabledandOBSERVE + store enabledare structurally unrepresentable — there is no second enable flag anywhere in the codebase.What this adds
batching.rs) — request-scoped, keyed by(top-level response key, loader identity, normalized load shape, parent key)uniformly for queries and mutation payloads. Three unambiguous statesNotLoaded/Loaded(includingLoaded([])) /LoadFailed; onlyNotLoadedmay direct-fallback. Non-destructive reads, set-based dispatch over a de-duplicated key set, retained non-retried failures, whole-store invalidation.mutation_guard.rs) —OFF/OBSERVE/ENFORCE, wired at the HTTP boundary before ordinary Juniper execution.OFFreturns before parsing. A baseline eligibility gate reproduces pinned juniper 0.16.2's own pipeline using juniper's own helpers, so a baseline-invalid request yields no decision, no event and no guard error, and juniper remains the sole authority for its error.scope.rs) — the onlynew_error(..) -> path()response-scope extraction site; side-effect-free, fail-closed, with the pinned-Juniper coupling and revalidation obligation documented and tested.prefetch.rs) — look-ahead-driven set-based prefetch covering direct and descendant paths, alias-safe onfield_original_name()at every segment, collecting every matching terminal selection, writing into the ordinary terminal namespace.Boundaries held
MutationRootresolvers modified;sha256 1e08b46b…, verified by building both);schema.rs,migrations/,Cargo.toml,Cargo.lock,policy.rs, CI workflows all unchanged;unsafe, no private-field access;Evidence (local, exact head)
SQL statement counts observed at the driver via Diesel instrumentation, through a fresh pool constructed after the hook was installed (never the
OnceLocktest pool):N + NBounding the terminal loader does not make the descendant operation globally N+1-free — the legacy intermediate resolver still scales, and is reported separately.
Local checks:
cargo fmt --all -- --check,cargo check --workspace,cargo test --workspace(965 + 171 tests, 0 failures; 96 new),cargo clippy --all --all-targets --all-features -- -D warnings,git diff --check— all pass. Full detail in the implementation report.Activation boundary — read before merging
Merge authorizes neither activation.
Production
OBSERVEandENFORCEeach require separate explicit CTO production activation approval and both remain NOT AUTHORIZED. Production activation is additionally BLOCKED — runtime-operations / CG-13 and monitoring-threshold evidence unverified. Rollback timing and mechanism are unverified; rollback is not claimed to be certain or deploy-free.BE-02runtime remains NOT AUTHORIZED.Review gate
This exact head requires fresh independent cross-model review. The implementing agent has not reviewed or approved its own work. HIGH risk additionally requires separate explicit CTO merge authorization bound to the independently approved exact head; any head movement invalidates it.