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: — andCoreComposableSchemahas a hidden dependency between two of them:
$this->generatedTypesis written in exactly one place: CoreComposableSchema.php:172, insidegetSchemaDefinition().getSchemaDefinition()only runs when theschemacache misses (CoreComposableSchema.php:308).getExtensionDocument()reads it at CoreComposableSchema.php:341:
$extension->getTypeExtensionDefinition($this->generatedTypes ?? []).So if
schemais warm whileextension(orfull) 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
- Configure graphql server with some additional schemas from either graphql_compose or graphql_core_schema
- Ensure you're not running in development mode and that AST caching is enabled.
This also should triggerassumeValidto be true. - Prepare a GraphQl Query that uses one of the query additions from graphql_compose or graphql_core_schema
- Execute the GraphQl Query
- 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', ]);" - 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!
- Clear all graphql caches:
ddev drush ev "\Drupal::getContainer()->get('cache.graphql.ast')->deleteAll();" - 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
- ✓ Write tests to verify behavior
- Write fix - ⚠️Current code has been generated using LLM support. It has had some review cycles by a grumpy free range dev.⚠️
- Review
- Merge
- Profit
User interface changes
None.
API changes
None.
Data model changes
None.
Issue fork graphql-3613619
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
Comment #3
das-peter commentedDigging further through this reveals a whole now world of issues.
While
SdlSchemaPluginBasesomewhat 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 asSdlSchemaPluginBaseitself 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
SdlSchemaPluginBasehave 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.
Comment #4
das-peter commentedRight now the most control we'll ever have over cache consistency seems to be the fact that by default we control the
cache.graphql.astservice. 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:
Comment #5
das-peter commentedI'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!⚠️
Comment #6
das-peter commentedDid 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->astCacheinstance - 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 :)
Comment #7
das-peter commentedComment #8
klausiThanks, 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.