Stop the GraphQL tracer re-executing queries that failed analysis

What does this MR do and why?

Fixes two independent bugs that together caused unbounded recursion (and an ~85 minute CI job hang) whenever a resolver-generated GraphQL argument is marked experiment:. This is why !256398 (merged) had to revert the experiment: markers it added to 26 exclusion-filter arguments across the analytics aggregation engines.

1. The tracer re-executes the query from its own ensure block

This is the production-relevant half of the fix: it protects against any StandardError that escapes the analysis phase, in any environment. Defect 2 below is a preventive fix for one specific, checked-in trigger of it — read this one first.

Gitlab::Graphql::Tracers::InstrumentationTracer#execute_multiplex reads query.result inside ensure (in error_type and log_execute_query). In graphql-ruby 2.6.10, GraphQL::Query#result re-executes the query if it hasn't run yet, and @executed is only set after the analysis phase completes. A StandardError raised during analysis leaves the query unexecuted, so the ensure block's read of query.result re-runs the whole multiplex, hits the same error, and re-enters the same ensure block. Each level logs a line and increments two SLI counters before recursing again, so it loops rather than failing fast — in CI that looked like an ~85 minute job hang rather than a fast crash.

Fix: error_type and log_execute_query now share a result_errors helper that checks query.executed? before reading query.result. The header comment above execute_multiplex now explains why the tracer hooks the multiplex rather than execute_query — even queries that fail validation reach it with query.result set.

Deliberate behavior change: a StandardError that escapes execution but produces no errors array is now counted as an error for the graphql_query_error_rate SLI and excluded from apdex. Previously error_type returned nil for that case, so a failed query was recorded as an apdex success. The exception is multiplex-wide: execute_multiplex's rescued e is the same for every query in the multiplex, so this also reclassifies sibling queries that finished without error. That's intended — one query raising fails the whole request, and none of its query results reach the client.

Not changed: the rescue StandardError => e in execute_multiplex is left as is. A non-StandardError raised during execution — after the analysis phase has already completed, e.g. NotImplementedError — still leaves e nil while query.executed? is true, so error_type returns nil and the query is still counted as an apdex success. That's a pre-existing gap this MR doesn't address; widening the rescue itself is a separate concern (see "Not in this MR").

2. The generated analytics aggregation resolvers have no graphql_name

Gitlab::Graphql::Deprecations turns experiment: into a deprecation_reason. graphql-ruby's FieldUsage analyzer calls argument.definition.path for every deprecated argument, which needs owner.graphql_name. Resolvers::Analytics::Aggregation::EngineResolver.build and AggregationFieldResolver.build create anonymous resolver classes (Class.new(...)) with no graphql_name, so Class#name is nil and graphql-ruby raises GraphQL::RequiredImplementationMissingError: Anonymous class should declare a graphql_name during analysis — exactly the analysis-phase exception that triggers defect 1.

Fix: both build methods now take **graphql_context, like their sibling builders, and set a graphql_name derived from Adapter.types_prefix(graphql_context[:types_prefix]) (e.g. PipelinesAggregationResolver, PipelinesAggregationFieldResolver), matching the prefix already used for the sibling generated types in this tree (PipelinesAggregationScope, PipelinesAggregationResponse, etc.). AggregationScopeType.build passes graphql_context through to AggregationFieldResolver.build so the prefix stays consistent.

Caveats worth flagging:

  • Nothing on master today actually puts experiment: on an argument owned by these resolvers — the adapter hardcodes { required: false, description: } for generated filter arguments. The only route is the block passed to mount_aggregation_engine, which is class_eval'd onto the anonymous resolver class. The new spec exercises that route directly by mounting a block that declares an experiment:-marked argument, rather than relying on existing production code.
  • In production, this specific trigger doesn't currently recurse: LoggerAnalyzer#result rescues the RequiredImplementationMissingError and calls Gitlab::ErrorTracking.track_and_raise_for_dev_exception, which only tracks (and doesn't re-raise) outside development/test. Defect 1 is still a latent hazard for any other exception that escapes the analysis phase, in any environment, which is why both are fixed here.
  • Added a schema-wide spec (spec/graphql/gitlab_schema_spec.rb) that walks every type in GitlabSchema.types, collects field and input-object arguments with a deprecation_reason, and asserts each resolves a path. Other builders in the codebase also create anonymous owners — Resolvers::BaseResolver.single/.last, Resolvers::Analytics::CycleAnalytics::BaseCountResolver.[], Types::RangeInputType.[] — so any of them is one experiment: argument away from the same crash; fixing those is out of scope here, so the guard is scoped to the exact condition FieldUsage checks (deprecated/experiment arguments) rather than all arguments, and it passes on master today. The next such marker will fail at spec time instead of hanging CI.

This is the first graphql_name set on a resolver in the codebase. It's read only by Argument#path for non-mutation resolvers (the gem's other readers of graphql_name are mutation-only), so it isn't part of the emitted schema: bundle exec rake gitlab:graphql:check_docs reports "GraphQL documentation is up to date", and gitlab:graphql:schema:dump output has zero occurrences of AggregationResolver/AggregationFieldResolver.

Not in this MR

  • Re-applying experiment: to the 26 exclusion-filter arguments reverted in !256398 (merged). That needs an experiment option on the filter DSL, adapter forwarding, edits to all 8 engine models, and regenerated docs. This MR makes that safe to do as a follow-up, but doesn't do it.
  • Widening the rescue StandardError => e in execute_multiplex to also catch things like SystemStackError.
  • Switching Types::BaseArgument's array-size error message from owner.name to owner.graphql_name. It currently prints #status for these resolvers, which is a little unclear, but BaseResolver.single/.last also build anonymous resolvers without a graphql_name, so that change would make their error path raise instead.

References

Screenshots or screen recordings

Not applicable. This is a backend GraphQL/query-analysis fix with no UI.

How to set up and validate locally

  1. Run the new specs:
    bundle exec rspec spec/lib/gitlab/graphql/tracers/instrumentation_tracer_spec.rb spec/graphql/resolvers/analytics/aggregation/engine_resolver_spec.rb spec/graphql/gitlab_schema_spec.rb
  2. Full related suite: bundle exec rspec spec/lib/gitlab/graphql/tracers/ spec/graphql/resolvers/analytics/aggregation/ spec/graphql/types/analytics/aggregation/ spec/lib/gitlab/database/aggregation/graphql/ spec/graphql/gitlab_schema_spec.rb — all pass.
  3. Rubocop on the seven touched files — no offenses.
  4. Schema/docs are unaffected: bundle exec rake gitlab:graphql:check_docs reports up to date, and gitlab:graphql:schema:dump has no occurrences of the new resolver names.
  5. End-to-end reproduction of the original issue (not committed): temporarily re-applied experiment: to all 26 <name>Not exclusion-filter arguments (what !256398 (merged) originally did) and, via rails runner, ran the query from the issue: duoWorkflows(statusNot: ["finished"]). Before this MR: SystemStackError. After: a normal result, with one log line per query, e.g. query_analysis.used_deprecated_arguments: ["DuoWorkflowsAggregationResolver.statusNot"].

Note: the request specs under ee/spec/requests/api/graphql/analytics/ could not be run locally (pre-existing local LoadError: sass); CI covers them.

MR acceptance checklist

Evaluated against the MR acceptance checklist. No UI, database, or emitted-schema changes. The only externally observable effect is the graphql_query_error_rate/apdex SLI semantics change described above.

Edited by Chandra Saripaka

Merge request reports

Loading
Loading