Cacheability and cost on paths that pay for nothing

Problem/Motivation

Work the module does on requests that never need it: a logger channel built on every service instantiation, a join that asks the same question twice, and two index decisions that do not match the queries.

Found by a complete file-by-file audit of the whole module at commit bc205f4f: 1130 tracked files, 1126 read in full, 4 binary assets recorded as skipped. Every linter was already clean at that commit, so none of this is something a tool reports: phpcs Drupal over 1008 files and DrupalPractice over 814 both at zero, PHPStan level 5 with no errors, cspell against the CI configuration over 1061 files with no issues, eslint with no errors, stylelint with no problems, composer validate, and the per-project translation check reporting every string translated in the catalog of the project that ships it.

Findings

Medium

1. Every service injects a prebuilt logger channel, so the channel is constructed on requests that never log (orchestra.services.yml line 161, performance). 26 service definitions across 8 modules inject '@logger.channel.orchestra' (or orchestra_client's own channel) and none injects '@logger.factory'; core defines logger.channel_base as factory: ['@logger.factory', 'get'], so injecting it forces logger.factory, request_stack and current_user to be built and a LoggerChannel instantiated whenever any of those services is instantiated. Every request that touches the engine, the payment step, the inbox notifications or the mail channels pays for constructing a logger channel and its dependency graph, on the overwhelming majority of requests that log nothing; the project standard is to inject the factory and call get() at the log site. Fix: Inject @logger.factory everywhere and call $this->loggerFactory->get('orchestra') at each log site; a small module trait can name the channel once. Delete the logger.channel.orchestra and logger.channel.orchestra_client service definitions once nothing injects them. Tests capturing logs must switch to $this->container->get('logger.factory')->addLogger($buffering) and compare levels against RfcLogLevel::*, not the PSR-3 name.
2. A merging quorum join resolves each branch's collect variable twice, each resolution re-querying the whole token set (src/JoinCoordinator.php line 126, performance). evaluate() reads the collect settings key from every arrived branch, then mergeArrivals() reads the same key from the same tokens again, and each getInstanceVariables() call runs TokenLineage::getLineageIds(), an unbounded per-instance token query plus loadMultiple(). A user completing the last branch of a 5-arc quorum join in synchronous mode waits on roughly fifteen identical every-token-of-this-instance queries, each returning every token the run has created. Fix: Build the per-arc collected values once in evaluate() and pass the map to mergeArrivals(); load the instance's tokens once in getCommonAncestorId() and walk parents in memory.

Low

1. NotificationAudience::resolve() restates resolveByDisposition()'s whole loop (src/NotificationAudience.php line 46, duplication). Both methods repeat the same derivation - skip a missing plugin id, instantiate the audience with its settings, resolve its recipients, key them by key() - and resolve()'s result is exactly the union of the three groups resolveByDisposition() returns. A maintainer fixing audience resolution has to change two loops, and the park-on-arrival dispatcher and the Notify node then behave differently. Fix: Make resolve() call resolveByDisposition() and merge the three groups.
2. The orchestra token hook claims to cover a variable write in its cache metadata, and does not (src/Hook/OrchestraTokensHooks.php line 184, cacheability). The comment enumerates a status change, a workflow rename, a variable write or the branch advancing as what must invalidate a cached render, but only the instance, workflow and branch token are added as cacheable dependencies; the orchestra_variable rows that [orchestra:var:NAME] reads contribute no tag. Any consumer that renders [orchestra:var:*] into a cacheable build - an ECA model, orchestra_easy_email, or a third party, since this hook is the module's public token API - serves the value the variable had when the render was first cached. No shipped page is affected today because the shipped consumers opt out of caching, which is why it has gone unnoticed. Fix: Add the variable rows' cacheability to the bubbleable metadata when variable tokens were resolved, or narrow the comment to the three dependencies actually added.
3. orchestra_instance__definition_state is a redundant prefix of definition_state_changed (src/ProcessInstanceStorageSchema.php line 33, performance). (definition, state) is a strict leftmost prefix of the (definition, state, changed) index declared fourteen lines below it, so it can never serve a query the wider index does not already serve. Every insert into and every state change of the instance table - the table the docblock says grows for the life of a site - maintains one secondary index no query can use. Fix: Drop the narrower index and fold its rationale into the wider one's comment; the class already applies this principle for the initiator index.
4. The incident table carries no tenant index, though a tenant-scoped incident dashboard is advertised (src/IncidentStorageSchema.php line 19, performance). orchestra_incident declares only (instance, state), while every sibling runtime table indexes (tenant, state) for the same shape, and the module ships an orchestra_incident 'Current tenant' Views filter. A site builder who follows docs/incidents.md and builds the advertised multi-tenant dashboard of open incidents gets a full table scan plus filesort on every page. Fix: Add orchestra_incident__tenant_state to be ['tenant', 'state'] and name the dashboard in the docblock.

Proposed resolution

One merge request for this issue, one commit per finding, each commit naming the finding it closes. Every defect gets a test that fails against the unfixed code, so the ground is closed and the next audit pass has to look somewhere new.

Remaining tasks

• Fix each finding above, with its test.
• Run every linter the pipeline runs, and the impacted test classes, before pushing.
• Play the next-major lane by hand, since its composer job is manual and a skipped lane reads as green.

User interface changes

None.

API changes

None.

AI-Generated: Yes (Claude Code was used to run this audit and to draft this issue summary. I reviewed the findings against the source myself before posting; the code and tests will follow on the merge request.)