docs(architecture): propose request-scoped GraphQL batching (ADR-0006) - #789
Merged
ja573 merged 11 commits intoAug 8, 2026
Merged
Conversation
Add ADR-0006 at PROPOSED, recording the CTO's request-scoped batching / set-based loading direction as a shared Thoth GraphQL decision, and the bounded THOTH-GQL-BATCH-01 runtime implementation specification at DRAFT. The concrete mechanism is determined against the pinned stack rather than assumed. A deferred-dispatch DataLoader is rejected on evidence: juniper_codegen 0.16.0 wraps non-async resolver bodies in future::ready(..) so they evaluate before any future is polled, the sync resolver generated for an async fn field is a panic! while the whole GraphQL test suite runs through execute_sync, and neither FuturesOrdered selection-set driver exposes a batch-dispatch signal. The selected mechanism is look-ahead-driven set-based prefetch into request-scoped state, which is synchronous and so behaves identically under the production async execute path and the sync test path, with no new dependency and no execution-model change. Existing child resolvers are not migrated; legacy remediation is evidence-led follow-up work. BE-02 is a dependent of the batching foundation, not a dependency of it. Documentation only. ADR-0006 is PROPOSED and authorizes nothing, the runtime implementation is not authorized, PR #788 and issue #765 are unmodified, and no runtime, schema, migration, dependency or workflow file changes.
The verified count of 63 is of resolver methods taking `context: &Context` in thoth-api/src/graphql/model.rs, which includes non-child resolvers. Describing all 63 as per-parent child resolvers overstated the finding, so the count is retained only where it is accurate and the legacy-policy statements refer to the existing child resolvers without it. Also corrects a module-count detail in the implementation-impact table.
Independent architecture review returned CHANGES REQUIRED with three P1 findings and one P2. Option A / A2 - look-ahead-driven set-based prefetch into request-scoped state - is unchanged; B, C and D were not reconsidered. P1 cache identity: the store was keyed by (loader, parent key), which collides for the argument-bearing child fields Thoth already has. Identity is now (loader identity, normalized load shape, parent key), with typed loader-specific shapes, one constructor shared by prefetch and lookup, and explicit default normalization - Juniper look-ahead reads only literal AST arguments and never applies schema defaults, while the child resolver receives the default-applied value. BE-02's argument-free field takes a Unit shape; no production field gains an argument. P1 failure state: the draft required a failed prefetch both to leave keys absent and to suppress the fallback that absence triggers. Replaced with a three-state store - NotLoaded falls back, Loaded (including empty) never queries, LoadFailed returns the error with no retry - the failure recorded once per dispatch, the parent list field still resolving, and a GraphQL-visible equivalence contract over errors[].path, null propagation and extensions.type rather than error text. P1 path coverage: correctness and N+1 compliance are now distinct. Publisher fans out through Imprint.publisher and Contact.publisher as well as the publishers root query, so a loader-backed field with one prefetch site can still issue a query per parent. Adopting tasks owe an exact-base path inventory, coverage or escalation, and per-path measurement; the BE-02 inventory belongs to BE-02. P2 measurement: statement counts must use a pool constructed after the instrumentation hook, not the process-wide OnceLock test pool. Documentation only. ADR-0006 remains PROPOSED, THOTH-GQL-BATCH-01 remains DRAFT and unauthorized, PR #788 and issue #765 are unmodified, and the changed head requires a fresh independent exact-head review.
Specify descendant prefetch and escalate the mutation-payload N+1 boundary. ADR-0006 required adopting tasks to cover every material fan-out path while A2 could only express a direct-child prefetch, so paths such as QueryRoot.imprints -> Imprint.publisher -> Publisher.distributionPlatforms were mandated but unsupported. Section 4.19 extends A2 so a prefetch site may target a direct child or a descendant, settling the selection path, terminal loader identity, terminal load-shape constructor and a key projector from the resolved list item, with recursive alias-safe traversal matching field_original_name() at every segment, every matching terminal selection collected, and results stored under the ordinary terminal identity so no second cache namespace exists. Adds a four-condition key-projection security rule and separates loader-backed-field compliance from legacy intermediate resolver performance, so bounding a terminal loader is never reported as making an operation globally N+1-free. Withdraws the rule confining prefetch sites to resolvers unreachable from MutationRoot payloads, which contradicted the coverage rule now that updatePublisher -> Publisher -> contacts -> publisher -> distributionPlatforms is a live fan-out. Operation-scoped mutation batching was investigated against the pinned sources and is not implementable: a resolver cannot determine the operation type through stable public Juniper API, the only public route to the execution path is an off-label Executor::new_error plus ExecutionError::path, and the pinned async path drives mutation root fields through FuturesOrdered with no OperationType-aware serialization, unlike the serial sync path. The decision is escalated to the CTO as M1 or M2 with neither selected, and no silent exclusion is written: mutation-payload paths stay correct via the NotLoaded fallback, are recorded as blocked rather than excluded, and the hold is stated as temporary. Documentation only. ADR-0006 remains PROPOSED, THOTH-GQL-BATCH-01 remains DRAFT and HIGH risk, and BE-02 remains blocked.
Resolve the mutation-payload N+1 boundary that left the previous head blocked. The CTO selected uniform top-level-response-key scoping. Loader state is now owned by one GraphQL request but partitioned by the current top-level GraphQL response key, giving the store identity (top-level response key, loader identity, normalized load shape, parent key), applied uniformly to queries and mutation payloads. No resolver detects operation type and the raw document is never parsed to derive scope. Storage lifetime and reuse namespace are now distinct: the store still lives on the request-scoped Context and never crosses requests, while reuse is confined to one top-level response key. The withdrawn MutationRoot rule, the temporary hold and the M1/M2 decision set are removed rather than annotated. Mutation-payload fan-out is an ordinary covered path using the same adoption algorithm as query paths, and no production mutation resolver is modified because correctness comes from scope isolation rather than invalidation on write. That isolation holds even though the pinned async path drives mutation root fields concurrently through FuturesOrdered while the sync path is serial. Scope is derived through one isolated pinned-Juniper compatibility shim over Executor::new_error plus ExecutionError::path, evidenced side-effect-free, failing closed to the NotLoaded fallback rather than to a shared namespace, and carrying a revalidation obligation on any relevant Juniper change. Scope keys are response keys and therefore aliases, never normalized to the schema field name; repeated response keys share one scope, since Juniper does not merge selections while validation rejects incompatible same-key selections, so no source-position component is added. The accepted cost is recorded, not hidden: the same key beneath two top-level query response keys loads once per scope, bounded by the operation's top-level structure and independent of parent count. The risk classification was re-run against the framework and remains HIGH. Documentation only. ADR-0006 remains PROPOSED, THOTH-GQL-BATCH-01 remains DRAFT.
The previously reviewed head claimed a top-level GraphQL response key uniquely identifies a mutation execution. Reproduced against pinned juniper 0.16.2, that is false: OverlappingFieldsCanBeMerged permits compatible repeats, the sync executor resolves each Selection::Field occurrence and the async executor pushes one future per occurrence, and merge_key_into reconciles the results afterwards. One response key can therefore drive several mutation resolver executions sharing one scope, breaking the read-after-write isolation invariant. An execution-occurrence scope is rejected on evidence: for a duplicate response key whose payloads share one fragment, the terminal resolver's path() and location() are both identical across two distinct writes. Correcting the execution layer is rejected as architecture expansion. Adopt two coordinated controls: a central request-boundary guard rejecting mutation operations with a duplicate executable top-level response key, and the existing response-key-scoped store, whose one-to-one correspondence with a write now depends on that guard and which must be unavailable without it. Correct the rollout: the guard is live on the common request path at merge, so the foundation is no longer inert. Add a kill switch, sweep THOTH-GQL-BATCH-01 for stale unscoped identities, split multi-site reuse into same-scope reuse and cross-scope isolation, and re-derive the risk classification. Documentation only. ADR-0006 remains PROPOSED and now asks the CTO to approve the request-boundary restriction as its own decision. THOTH-GQL-BATCH-01 remains DRAFT and unauthorized.
Remediates the independent review of ef3a895. F2 remains the selected architecture and was not reopened. Rollout: the guard was active on every mutation request from the merge commit behind a kill switch defaulting to enabled, with CTO merge authorization treated as activation authorization. That conflicts with release-gates.md, which prefers safe disabled-by-default post-merge behaviour and requires production activation of HIGH-risk work to carry preview acceptance, a controlled pilot, monitoring, rollback, an activation owner, an observation period and explicit CTO approval. The guard now has OFF / OBSERVE / ENFORCE modes. OFF is the default and the merged state: it evaluates nothing and the store is unavailable, so merge changes no production behaviour. OBSERVE evaluates exactly as ENFORCE would, rejects nothing, and records would-be rejections. ENFORCE rejects and is the only mode in which the store may be available. The coupling store-available => mode ENFORCE must be structurally unrepresentable otherwise. ENFORCE needs its own CTO production activation approval, distinct from merge authorization. The claim that a comparison period adds no evidence is withdrawn: the open question is whether real traffic contains documents ENFORCE would reject, which no test answers and which matters because external clients cannot be enumerated. OBSERVE is the controlled pilot, and a non-zero count blocks ENFORCE. Directive evaluation used raw request variables, but the pinned executor applies operation variable defaults first via or_insert. Reproduced against the pinned sources: the raw form over-rejects six of thirteen documents, the effective map matches actual execution in all thirteen. Also records that pinned juniper rejects non-null variables with defaults, so defaulted-variable tests must declare them nullable. Corrects the one remaining unscoped binding identity: descendant results are stored under (scope, loader, shape, terminal key). Reconciles observability, which said "Required logs: none" while the guard required a record per rejection, and re-derives risk as HIGH without the withdrawn at-merge-activation grounds. Documentation only. ADR-0006 remains PROPOSED; THOTH-GQL-BATCH-01 remains DRAFT with implementation NOT AUTHORIZED.
…ation Remediates the independent review of 78cd44e. F2, the OFF/OBSERVE/ENFORCE staging, effective variables and scoped identities are all retained and not reopened. P1 validation ordering. The guard was specified to run before data.execute(&st, &ctx).await, but GraphQLRequest::execute delegates to juniper's crate-level execute, and both execute and execute_sync perform parse, document validation, operation selection and input-variable validation inside that call. A guard placed before it therefore sees documents juniper would reject, so the ADR's claim that such a document "never reaches the guard" was false. This was load-bearing: a document can be both invalid and duplicate-shaped, letting ENFORCE replace juniper's canonical error and letting OBSERVE record would-be rejections for traffic juniper would never execute, corrupting the very evidence that gates ENFORCE. Successful ordinary validation is now a prerequisite for duplicate-key analysis. A baseline eligibility gate reproduces juniper's own pipeline stages in order using its own public helpers; any error means the guard performs no analysis, emits no event, returns no error, and lets juniper produce the canonical response. The gate is an eligibility gate, not a replacement executor. Verified by compiling and running it against pinned juniper 0.16.2 on public APIs only: eight baseline-invalid duplicate-shaped documents were all excluded while juniper produced its canonical error and ran zero resolvers. Withdraws the understated "one additional parse" cost in favour of duplicate parse and validation on the guarded path, with OFF required to short-circuit ahead of it. Also sweeps stale active-at-merge language, replaces the vague "store unavailable whenever the guard is not applied" with the ENFORCE-tied form, and reconciles the decision register's dependency chain through preview, OBSERVE and ENFORCE before BE-02. Documentation only. ADR-0006 remains PROPOSED; THOTH-GQL-BATCH-01 remains DRAFT with implementation NOT AUTHORIZED.
Remediates the independent review of b1a4b6c. F2 and the baseline-validation eligibility gate are retained and not reopened. Blast radius. The gate's cost was described as bounded to mutations, but it must parse and select an operation before it can know the operation kind, so in OBSERVE/ENFORCE it touches every GraphQL request. A safe earlier discriminator was investigated and found: operation type is determinable after parse plus get_operation alone, verified with zero mismatches against juniper's own typing across valid and invalid documents of both kinds. The fast path is adopted, but it does not eliminate the cost -- validation is roughly three fifths of gate cost, leaving parse and selection on every request. The three costs are now stated separately and all availability, latency and threshold text is written against the common request path. OBSERVE authorization. OBSERVE parses and selects for every request, validates and analyses mutations and emits logs, so it is live production behaviour. Both OFF -> OBSERVE and OBSERVE -> ENFORCE now require their own explicit CTO production activation approval, neither implied by merge authorization nor by the other. Rollback. The claims that rollback is certain and deploy-free are withdrawn. The clap Arg::env pattern proves a configuration input exists and nothing about reload, restart, propagation, cross-replica atomicity, ownership or verification, all unmapped under open CG-13. Activation is blocked pending ten feature-specific answers, including partial-fleet handling, which is load-bearing because store availability derives from the mode. Monitoring. The collision stream is the compatibility signal only. Service-health signals must be verified to exist before OBSERVE, with thresholds derived from existing baselines; since none is authoritative here, no number is invented and the status is recorded as blocked. Also corrects the Juniper API wording: the gate's surfaces are doc(hidden), unlike the shim and look-ahead APIs. Risk is now recorded as implementation HIGH plus activation readiness BLOCKED. Documentation and control only. ADR-0006 remains PROPOSED; THOTH-GQL-BATCH-01 remains DRAFT with implementation NOT AUTHORIZED.
Documentation-consistency sweep only. No architecture decision is reopened: F2, the eligibility gate, the non-mutation fast path, effective variables, scoped identities, descendant prefetch, the compatibility shim, the three-state store and the SQL evidence model are unchanged. Activation wording. The final model has two production activations, but stale text still framed activation as ENFORCE-only: the risk table and classification paragraph, a bounding factor saying behaviour changes only at ENFORCE, the HIGH-risk controls list, the task metadata, section 11.6's observation period, the implementation-report requirement, and an ADR consequence. Corrected so that OFF -> OBSERVE is itself production activation, because it adds live request-path behaviour, and OBSERVE -> ENFORCE is a second activation because it additionally changes accepted mutation-request semantics. OBSERVE is operational activation; ENFORCE is operational activation plus a client-visible acceptance change. Each needs its own explicit CTO approval, and none of merge, OBSERVE or ENFORCE authorization implies another. Observation is now two stages. The OBSERVE window must evaluate both compatibility and operational health, both must pass before ENFORCE, and collision events are recorded as the compatibility signal only. Performance wording. The ADR still said the guard adds one document parse per mutation request. Corrected to parse and operation selection on every GraphQL request, with document and input validation plus duplicate-key traversal on mutations only, queries exiting through the fast path, and OFF adding none of it. CG-13 remains open and monitoring thresholds remain unverified; the control-gap document was not touched. ADR-0006 remains PROPOSED; THOTH-GQL-BATCH-01 remains DRAFT with implementation NOT AUTHORIZED.
The task's approval section still said merge authorization is not production activation authorization because "the transition to guard mode ENFORCE requires its own explicit CTO approval". Naming only ENFORCE contradicted the rest of the same task, which already requires separate CTO production activation approval for OFF -> OBSERVE because OBSERVE activates live request-path behaviour. Corrected to state the full binding rule: merge authorization is not OBSERVE activation authorization is not ENFORCE activation authorization. OFF -> OBSERVE requires explicit CTO production activation approval; OBSERVE -> ENFORCE requires a second, separate one; neither is authorized by merge approval or by the other. The ADR-0005 terminal-evidence rule immediately following is unchanged, and no approval identifier is introduced. A semantic sweep of the task, ADR and decision register found no other current normative statement naming ENFORCE alone. ADR-0006 and the decision register needed no change. One ADR occurrence is historical and already marked superseded, and was left as written. Documentation consistency only. No architecture, runtime or production change; CG-13 remains unresolved and monitoring thresholds unverified. ADR-0006 remains PROPOSED; THOTH-GQL-BATCH-01 remains DRAFT with implementation NOT AUTHORIZED.
ja573
marked this pull request as ready for review
August 8, 2026 11:06
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Member
Author
CTO specification approval - THOTH-GQL-BATCH-01I approve the This approval is bound to the exact specification content whose blob SHA is This is specification approval only. It does not authorize runtime implementation, creation of Under ADR-0005, this GitHub record is terminal approval evidence; no approval-state-only repository commit or recursive approval PR is required solely to transcribe this approval. The next gate remains a freshly verified exact |
Member
Author
CTO implementation authorization - THOTH-GQL-BATCH-01On 2026-08-08 the CTO explicitly authorized implementation of
Pre-authorization verification established that this SHA was exactly current Authorized implementation branch:
This authorization covers the bounded implementation defined by the approved THOTH-GQL-BATCH-01 specification only. It does not authorize merge, deployment, production activation, Production activation readiness remains BLOCKED on the runtime-operations prerequisite (CG-13 or bounded successor controls) and verified monitoring/threshold evidence. |
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
One final normative authorization-consistency correction. Documentation and control only. No architecture decision is reopened, no runtime code changes, and no production action occurred.
The previous architecture and production-control remediation remains intact and passed independent review. One task note still named
ENFORCEalone; this revision corrects it.Contains:
ADR-0006 - Request-scoped GraphQL batching and set-based child loading, atPROPOSED;THOTH-GQL-BATCH-01implementation specification, atDRAFT,HIGHrisk, implementationNOT AUTHORIZED, production activation readinessBLOCKED;The correction
THOTH-GQL-BATCH-01section 17 (Approval) previously read:Naming only
ENFORCEcontradicted the rest of the same task, which already requires separate CTO production activation approval forOFF -> OBSERVE— becauseOBSERVEactivates live request-path behaviour.Corrected to the full binding rule:
Neither activation is authorized by merge approval or by the other. The existing
ADR-0005terminal-evidence rule immediately follows, unchanged, and no approval identifier is introduced into the repository.Sweep result
A semantic sweep of the current normative task, ADR and decision-register text found no other statement naming
ENFORCEalone. Every surviving activation-authorization statement names both transitions or pairs them explicitly —ADR-0006invariant 32, sections 7.2.1 and 7.2.1.1, the approval-section consequence, the decision register'sADR-0006row and dependency narrative, and the task's metadata, HIGH-risk controls list, section 11.2 and section 11.5.One
ADR-0006occurrence is historical and already marked superseded — the narrative describing the earlieref3a895a…remediation, which records that onlyENFORCEthen carried the requirement and already states that a later remediation extended it toOBSERVE, with section 7.2.1 controlling. It was left as written.ADR-0006and the decision register required no change; both were already independently verified as consistent.Unchanged
F2 top-level-response-key architecture and store identity · baseline-validation eligibility gate · non-mutation fast path · effective-variable handling · scoped failure identities · descendant prefetch · compatibility shim ·
NotLoaded/Loaded/LoadFailed· set-based SQL and actual query-count evidence ·OFF/OBSERVE/ENFORCE· store availability only inENFORCE· the two-stage observation model · the performance model · conservative BE-02 dependency ordering.Performance model (unchanged): in
OBSERVE/ENFORCE, parse and operation selection on every GraphQL request; document/schema validation, input-variable validation and duplicate-key traversal on mutations only; queries and subscriptions exit through the fast path after parse and selection;OFFadds none of it.Observation model (unchanged): the
OBSERVEwindow requires both compatibility evidence and operational service-health evidence beforeENFORCE, with collision events remaining compatibility telemetry only;ENFORCEobservation continues actual rejections, legitimate-client incidents, latency/error/availability, and mode/fleet correctness.Activation blockers — unchanged
The CG-13 control-gap document is not modified. No numeric threshold is invented. No claim is made that runtime rollback is certain, immediate or deploy-free.
Risk
Unchanged.
Boundaries
BE-02specification: unmodifiedADR-0006: unmodified — the sweep found no current contradictiondocs/publisher-services/task-status.md, ADR-0001 – ADR-0005: unmodifiedthoth-api/src/**,thoth-api-server/src/**,thoth-client/**, Cargo files, migrations,schema.rs, CI workflows, repository settings, production configuration: unchangedfeature/shared-architecture/graphql-batching: not createdADR-0006 is
PROPOSED.THOTH-GQL-BATCH-01runtime implementation is NOT AUTHORIZED. ProductionOBSERVEandENFORCEactivation are NOT AUTHORIZED.BE-02runtime implementation is NOT AUTHORIZED. This revision corrects one remaining normative authorization inconsistency only — the head has changed, so no previous review decision carries forward, and this PR requires a fresh independent exact-head review before architecture approval, merge authorization, runtime implementation authorization, or production activation.