Cacheability and cost on paths that pay for nothing

Fixes the six findings filed on #3621292: cacheability that was claimed and not delivered, and work paid for on paths that could not use it.

All six were real. Five are fixed as defects; one (F087) is a duplication task with no behaviour change.

F037 · Every service injected a prebuilt logger channel

24 service definitions across 8 modules injected @logger.channel.orchestra (or orchestra_client's own channel), and eight plugins read the same channel out of the container in create(). logger.channel_base is a factory service (['@logger.factory', 'get']), so each of those forced logger.factory, request_stack and current_user to be built and a LoggerChannel instantiated whenever the host service was instantiated — on every request that touches the engine, the payment step, the inbox notifications or the mail channels, and the overwhelming majority of those log nothing.

Every host now injects @logger.factory and asks for the channel inside the branch that logs. OrchestraLoggerTrait::getOrchestraLogger() names the shared channel once for the whole project, the way orchestra_interaction_webform already asked the factory for its own; orchestra_client, with a single log site on a channel of its own, asks inline as that plugin does. Both logger.channel.* definitions are deleted — a channel service exists only to be injected.

Channel names, messages and levels are unchanged, so the lines still reach the orchestra channel docs/audit.md describes. The test capture needed nothing: LogCaptureTrait already attaches its recorder through logger.factory->addLogger(). Two callers that build a service by hand now hand it the factory, and the audit-trail unit test stubs the factory to return the channel it asserts on.

Test (new, tests/src/Unit/LoggerFactoryInjectionTest.php) — the rule is read from the shipped files, because a new service is wired by copying the one next to it. Red against the unfixed wiring:

1) LoggerFactoryInjectionTest::testServiceDefinitionsNameNoLoggerChannel
orchestra_audit_trail.services.yml names a logger channel; inject
@logger.factory and ask it for the channel at the log site.

2) LoggerFactoryInjectionTest::testPluginsReadNoLoggerChannelFromTheContainer
ActionTask.php asks the container for a logger channel; ask for
'logger.factory' and get the channel where it logs.

F017 · A merging join resolved each branch's value twice

evaluate() gathers one collected value per arrived arc to build the join state (a quorum votes on it), and mergeArrivals() then read the same variable from the same tokens again. Each read is a getInstanceVariables() call, which runs TokenLineage::getLineageIds() — an unbounded "every token of this instance" query plus a loadMultiple() — before its own variable query. On top of that, getCommonAncestorId() ran that same whole-instance token query once per joined token.

The firing cohort is exactly one token per arrived arc (a second token on an arc is left waiting as surplus), so the map evaluate() already built is the merged list, in the same order the second loop produced. mergeArrivals() becomes getMergeWrite(), which formats what was collected instead of collecting again. getCommonAncestorId() batch-loads the instance's tokens once and walks each lineage over that one map; the walk moves into a private helper it shares with getLineageIds().

Measured on the arrival that fires the join, counting SELECTs against the variable and token tables: a 3-branch join goes from 9 and 30 reads to 6 and 25, a 5-branch join from 13 and 38 to 8 and 29 — one variable read per branch instead of two, two token reads instead of four.

Test (JoinMergeTest::testTheFiringArrivalReadsEachBranchOnce), asserting the per-branch slope between a 3- and a 5-branch join, and that every branch's vote is still in the merged list. Red against the unfixed coordinator:

A branch must cost the firing arrival one read of the variables, not one per
resolution.
Failed asserting that 4 is equal to 3 or is less than 3.

and, with the coordinator fixed but the lineage walk still per token:

A branch must not cost the firing arrival a second read of every token of the
instance.
Failed asserting that 6 is equal to 5 or is less than 5.

F049 · The token hook claimed a variable write it did not cover

The hook enumerated "a status change, a workflow rename, a variable write or the branch advancing" as what must invalidate a cached render, and added only the instance, the workflow and the branch token. [orchestra:var:NAME] reads its value from orchestra_variable rows, whose tags none of those imply, so a consumer rendering one into a cacheable build — an ECA model, orchestra_easy_email, or any third party, since this hook is the module's public token API — keeps serving the value the variable had when the render was first cached. No shipped page shows it, because the shipped consumers opt out of caching.

Resolution is a query over those rows, so the dependency is the entity type's list tag, not a per-row one: a write to an existing row invalidates both its own tag and the list tag, while a name written for the first time — which resolved to nothing when the render was cached — invalidates only the list tag. It is added where the variable tokens are resolved, so text carrying none of them declares no dependency on the variable rows and is not invalidated by every write the engine makes. docs/tokens.md now names the variables in the metadata it tells a module to apply (and calls that argument the fourth, which is what Token::replace() takes).

Test (ProcessTokensTest::testVariableTokenDeclaresTheVariableRows), red against the unfixed hook:

A resolved variable token must make the render depend on the variable rows.
Failed asserting that an array contains 'orchestra_variable_list'.

F093 · An instance index a wider one already served

(definition, state) is a strict leftmost prefix of the (definition, state, changed) index declared below it, so no query can reach it that the wider one does not already serve on its leading columns — including migration's (definition + state) lookup. The instance table grows for the life of a site, so that index was maintained on every insert and every state change for nothing. Dropped, with its rationale folded into the wider index's comment next to the same argument the class already makes for the initiator index.

Test — RuntimeEntityIndexTest owns the declared-index list, so the name comes off it and joins the redundant-index assertions, now a loop naming the wider index that serves each. Red against the unfixed schema:

The orchestra_instance__definition_state index is redundant:
orchestra_instance__definition_state_changed serves it on its leading columns.
Failed asserting that true is false.

F095 · The incident table carried no index for its dashboards

docs/incidents.md advertises a dashboard of open incidents as "just a view", and docs/views.md spells the recipe out - "Open incidents across tenants: on incidents, filter state = open and drop the Current tenant filter" - while orchestra_views also gives orchestra_incident the same "Current tenant" filter as its siblings. The table declared only (instance, state), so both dashboards were a full scan.

Shipped as (state, tenant): leading with state serves the cross-tenant recipe, and carrying tenant serves the scoped one just as well as the other order would, because both predicates are equalities and an index serves two equalities whichever way round they are declared. One index for both shapes rather than two to maintain on every write. Every in-code query (openIncidentCount(), retention, the cleanup cascade, the operator list) leads with instance, so nothing else moves.

The first attempt on this branch shipped (tenant, state) and claimed it removed a filesort; a follow-up commit corrected both. An index with no ordering column removes no filesort, and neither dashboard is shipped, so neither declares an order to trail.

Test, in the same class:

Missing incident index: orchestra_incident__state_tenant
Failed asserting that false is true.

F087 · NotificationAudience::resolve() restated the grouped resolution (task)

resolve() repeated resolveByDisposition()'s whole derivation — skip a row with no plugin or an unknown one, instantiate the plugin with its settings, key the recipients by key() — and its result is exactly the union of the three groups that method returns, because that method leaves a recipient in one group only. Two loops meant audience resolution could drift between the park-on-arrival dispatcher (the flat list) and the Notify node (the grouped one). resolve() now merges the three groups, and says in its docblock why the merge is the union.

No behaviour and no query changes: the same rows are read by the same plugins once, and the merge is three array unions over keys already deduped. The only difference is the order of a flat list that goes to channels with no notion of disposition, which is not a contract worth pinning — so this one ships without a new test: an assertion on the recipient set passes against both versions, and one that passes both ways is worse than none. Proved by the existing tests over both call paths instead: ParkNotificationDispatchTest (6 tests, 83 assertions) and NotifyNodeTest (5 tests, 86 assertions), both green.

Found by auditing the branch itself

Six more, fixed in follow-up commits on the same branch:

• The service-wiring sweep read Drupal core. LoggerFactoryInjectionTest walked the module root recursively; drupal.org CI builds the docroot INSIDE the checkout, so on CI it also read web/core and vendor and failed on core's own development.services.yml and ThemeInstaller.php (pipeline job 12031329). It now names the directories the project owns. GuardedRowUpdateTest walked the root the same way, so the walk moved into a shared ShippedFilesTrait and both read it from there. The count floor is replaced by what the failure was about: the sweep must have read the engine's own service file at the module root AND a submodule's below it.
• OrchestraLoggerTrait is @internal but RoleAudienceBase is @api. A protected trait method is inherited, so the helper became part of the surface a third-party audience plugin extends, while its own docblock says it may change. It is private now; a class that logs uses the trait and says so.
• [orchestra:status-label] declared nothing either. Same defect as the variable rows, in the same method: the label is a status term's, and none of the three dependencies the hook adds implies that term. It now takes the status list tag where the label is resolved. The bare date tokens were checked and deliberately left alone - a date format's only invalidation tag is rendered, which RenderCache puts on every entry, so declaring it would cost a config load for an invalidation that already happens.
• The incident index led with the wrong column (above).
• The merge's one-value-per-arc rule was unpinned. Building the merge write from the per-arc map rests on the firing cohort covering exactly one token per arc; no test drove two tokens onto one arc with a merging join. One does now. It is a contract test - it passes against the per-token loop too, which is the point.
• The lineage cap was described twice, verbatim, after the walker was extracted.

A second round over those fixes found two more, also fixed here:

• The status dependency was gated on a label coming back, so an instance whose status resolves to nothing declared nothing - and creating the term is exactly what makes a label appear where the token had cleared. It is declared before resolving now, which is what the variable tokens beneath it already do.
• docs/tokens.md listed the metadata one dependency short after the status tag was added - the same kind of false statement the hook's comment was filed for.

A third round found one candidate and withdrew it: [orchestra:node-label] reads the pinned version snapshot rather than the workflow, and that snapshot is re-saved in place when a workflow's config translation is corrected - but LanguageConfigOverride::save() invalidates the tag of the config it overrides, which is the workflow tag the hook already declares. Nothing goes stale, so nothing was added; the comment now records both withdrawals so the next reader does not add a dependency that buys nothing.

Also audited and withdrawn: the bare date tokens (above), and the equivalence claims behind the join refactor and resolve() - an exhaustive model of both merge algorithms over one arriving token plus 0-3 waiting tokens on 3 arcs (1554 cases) found no divergence, and resolve()'s single production caller passes rows that carry no delivery at all.

Checks

• phpcs --standard=Drupal (php,module,inc,install,theme,profile,engine,info,test,yml): 1010 files, exit 0.
• phpcs --standard=DrupalPractice: 816 files, exit 0.
• phpstan analyse at the project's level 5: no errors.
• cspell with the gitlab_templates config (en-US, allowCompoundWords: false), over the 54 files this branch touches: clean.
• CI, on the pushed branch: phpunit green over all 261 classes (202 kernel, 27 unit, 24 functional, 8 functional-javascript), plus phpcs, phpstan, cspell, eslint, stylelint, composer-lint and translations. The manual test-only changes lane was played and fails naming this branch's own assertions - the variable rows, the status terms, the index list, the service wiring and the join read budget - which is the proof those tests pin something. (The over-delivered-arc test passes there, as a contract test should.)
• Kernel, run locally on the impacted classes: RuntimeEntityIndexTest, ProcessTokensTest, JoinMergeTest, JoinTest, DiscriminatorQuorumTest, MatchingBasisJoinTest, LoopJoinTest, JoinCancellationTest, JoinStallRecoveryTest, TokenVariableTest, NotifyNodeTest, ParkNotificationDispatchTest, EngineFailurePathTest, DeadEndBranchTest, InstanceRetentionTest, RolesVariableAudienceTest, NotificationMailChannelTest, RemoteTokenProviderTest, plus the OrchestraAuditTrailSubscriberTest and LoggerFactoryInjectionTest unit classes — all green. The rest of the suite is CI's.

No update hooks: pre-1.0, and both schema changes are picked up by a reinstall.