Problem/Motivation

⚠️Code has been generated using LLM support. It has had some review cycles by a grumpy free range dev.⚠️

I've had some similar issues as described in https://www.drupal.org/project/graphql_compose/issues/3477239 and https://www.drupal.org/project/graphql_compose/issues/3541422
However, I use graphql_core_schema not graphql_compose so these issues seemed to point towards a more central caching issue.

I've started digging trough the code and got suspicious of the cache handling around \Drupal\graphql\Plugin\GraphQL\Schema\SdlSchemaPluginBase because it handles multiple cache items and processes stuff conditionally.
In a cache that can have (single item) random evictions like memcache and / or redis this is often problematic.

I've pointed an LLM towards the suspected problem and it came up with the following mechanism:

getSchema() in graphql's SdlSchemaPluginBase.php:119 builds the schema from four independent cache entries — schema:, extension:, schema_extension:, full: — and CoreComposableSchema has a hidden dependency between two of them:

  • $this->generatedTypes is written in exactly one place: CoreComposableSchema.php:172, inside getSchemaDefinition().
  • getSchemaDefinition() only runs when the schema cache misses (CoreComposableSchema.php:308).
  • getExtensionDocument() reads it at CoreComposableSchema.php:341:
    $extension->getTypeExtensionDefinition($this->generatedTypes ?? []).

So if schema is warm while extension (or full) is cold — which is precisely what a volatile/evicting backend produces — every TypeAwareSchemaExtensionInterface gets an empty type list and bails

  // ImageExtension::getTypeExtensionDefinition()
  if (!in_array('FieldItemTypeImage', $types)) {
    return '';   // <- $types is [] because generatedTypes was never computed
  }

From this I was able to concoct the below reproduction steps to verify this was indeed a valid problem.

Steps to reproduce

  1. Configure graphql server with some additional schemas from either graphql_compose or graphql_core_schema
  2. Ensure you're not running in development mode and that AST caching is enabled.
    This also should trigger assumeValid to be true.
  3. Prepare a GraphQl Query that uses one of the query additions from graphql_compose or graphql_core_schema
  4. Execute the GraphQl Query
  5. Delete the following AST cache entries e.g. via drush:
    ddev drush ev "\Drupal::getContainer()->get('cache.graphql.ast')->deleteMultiple([
        'extension:core_composable:general:en',
        'full:core_composable:general:en',
      ]);"
  6. Execute the GraphQl Query again: Should lead to broken query, either missing type error OR simply missing result properties of that type. This will not recover!
  7. Clear all graphql caches: ddev drush ev "\Drupal::getContainer()->get('cache.graphql.ast')->deleteAll();"
  8. Execute the GraphQl Query again - all fine again

Proposed resolution

Generally speaking a cache relying on multiple items has to expect that single items are lost - due to the ephemeral nature of caching - and be able to detect and recover from such a situation.

Remaining tasks

  1. ✓ Write tests to verify behavior
  2. Write fix - ⚠️Current code has been generated using LLM support. It has had some review cycles by a grumpy free range dev.⚠️
  3. Review
  4. Merge
  5. Profit

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork graphql-3613619

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

das-peter created an issue. See original summary.

das-peter’s picture

Status: Active » Needs work

Digging further through this reveals a whole now world of issues.
While SdlSchemaPluginBase somewhat is the "source" of the issue it is very hard to solve it just there because contrib modules e.g. graphql_core_schema overwrite parts of "it" and do their own caching thing - which runs into the same stale issues as SdlSchemaPluginBase itself has.
I'm not sure if the 5.x branch has the same issues, I can see that it seems to use a different approach but I'm not certain it mitigates the caching problem.
Ultimately it seems to boil down that contrib modules extending SdlSchemaPluginBase have to ensure cache consistency either by not caching intermediaries at all or some other sort of consistency check.
This might become really tricky because we do not have that much control over what contrib does in regards to caching.

das-peter’s picture

Right now the most control we'll ever have over cache consistency seems to be the fact that by default we control the cache.graphql.ast service. So I'm wondering if we could exert control over intermediary cache items by modifying that service instance so that it only allows our fully built schema into a persistent cache and everything else is just stored in a static cache during the process.
However, a look into graphql_compose showed that there is already an example of an extra caching bin being used - and a contrib module with some sanity checks on it's own to probably fix this / similar caching issues:

    // Sanity check. Has any critical component been dropped from cache?
    $has_full_ast = $this->astCache->get($this->getCacheId('full'));
    $has_schema_ast = $this->astCache->get($this->getCacheId('schema'));
    $has_schema = $this->definitionsCache->get('graphql_schema');
    $has_extensions = $this->definitionsCache->get('graphql_schema_extension');

    // If so, register our plugins.
    if ($this->inDevelopment || !$has_full_ast || !$has_schema_ast || !$has_schema || !$has_extensions) {
      // Add GraphQL Compose EntityType plugins to the registry.
      $this->gqlEntityTypeManager->getPluginInstances();
      // Add GraphQL Compose SchemaType plugins to the registry.
      $this->gqlSchemaTypeManager->getPluginInstances();
    }
das-peter’s picture

Issue summary: View changes

I've tried to integrate the above outlined gated ast caching wrapper together with a bunch of documentation regarding the ast caching.
At the moment I've no idea about a less intrusive way that would make it possible to control the ast caching - ideas more than welcome :)

⚠️Current code has been generated using LLM support. I'm not done reviewing it - consider it a draft!⚠️

das-peter’s picture

Assigned: das-peter » Unassigned
Status: Needs work » Needs review

Did my outstanding review - I'm still not able to come up with a better architectural approach that allows a transparent fix of the issue.
The generated code seems clean in terms of what & how.
The in code documentation is quite verbose / extensive even duplicated in parts - but the potential issue almost demands a detailed explanation to ensure nobody "optimizes" this back into a failing state.
The generated tests seem to properly capture the spirit of why this was implemented and test for exactly the things one wants to capture - including the module upgrade behavior which should be as transparent as possible to contrib.
The only concern left is that a contrib module uses a different cache bin / not the wrapped $this->astCache instance - but there's not really anything we can do about this.
I've addressed the open phpstan / phpcs warnings raised by the pipeline.
This code is now running in our test-bed - setting to needs review to hopefully get some more eyes on it :)

das-peter’s picture

Issue summary: View changes
klausi’s picture

Status: Needs review » Postponed (maintainer needs more info)

Thanks, the AI slop pull request is too big to review, so I would not invest time to check that.

Can you try 5.x if that fixes the problem for you? We did some cache changes there.