Issue #3613466: Runtime entities go through the persistent entity cache, so a stale read can drive a wrong advance

Frank Mablyrequested to merge
issue/orchestra-3613466:3613466-runtime-entities-go into 1.x

Change

  • persistent_cache: FALSE on orchestra_token, orchestra_instance, orchestra_variable and orchestra_work_item, each with a short note on why that row is unsafe to cache.
  • orchestra_workflow_version deliberately stays cached: an immutable snapshot is what a persistent cache is for.
  • docs/execution.md gains a "Runtime state is not persistently cached" section next to the concurrency notes.

Why this is not cosmetic

The engine reads one of these rows to decide a write (advance, park, consume, claim) and writes the decision back at once, so a stale copy produces a wrong write rather than merely stale output. The module already works around entity-layer staleness in the hot path: WorkflowExecutor::advance() re-reads the token state with a raw SELECT instead of going through the entity layer, and explains why in a comment; most raw claim UPDATEs are paired with a resetCache(). SubprocessCoordinator has three raw UPDATEs and no resetCache() at all. With the persistent bin out of the picture, a missed reset can no longer outlive the request that made the write.

Existing installs need one extra step

getStorage() builds the storage from the installed definition (EntityTypeManager::getActiveDefinition()), not from the attribute, so on a site that already has orchestra installed the flag does nothing until the installed definition is refreshed:

$repo = \Drupal::service('entity.last_installed_schema.repository');
$etm = \Drupal::entityTypeManager();
foreach (['orchestra_token', 'orchestra_instance', 'orchestra_variable', 'orchestra_work_item'] as $id) {
  $repo->setLastInstalledDefinition($etm->getDefinition($id));
}

followed by a cache rebuild. A fresh install picks the flag up on its own, and so do kernel tests, which is why the suite passes without this step.

Testing

New RuntimeEntityCachingTest asserts the four flags (and that the version entity keeps its cache), then behaviorally that loading an instance, a token and a variable leaves the entity bin empty. Verified it fails on unfixed code, one of the failures being the bin actually holding a cached token, and passes with the fix. Whole orchestra kernel suite green: 155 test classes, 533 test methods. phpcs and phpcbf clean at --warning-severity=1, cspell clean, phpstan unchanged (the one report in a file touched here is the pre-existing $item->value magic-property churn, identical statement in the base revision).

Two pre-existing notices noticed while sweeping, unrelated and left alone: Only variables should be passed by reference at tests/src/Kernel/CorrelationLookupTest.php:110 and modules/orchestra_api/tests/src/Kernel/LocalOrchestraClientTest.php:228. Happy to clean those up in a follow-up.

Merge request reports