fix(publish-date-workflow-resolution): fix publish date

[syedATdot]

Fixes #36634

Imported from external fork contribution by @riccardoruocco (riccardoruocco/core, branch fix/publish-date-workflow-resolution) so CI can run on it..

[mergify]

⚠️ The sha of the head commit of this PR conflicts with #36594. Mergify cannot evaluate rules on this PR. Once #36594 is merged or closed, Mergify will resume processing this PR. ⚠️

[claude]

Claude finished @fabrizzio-dotCMS's task in 2m 41s —— View job

---

Code Review — PR #36635 (publish date workflow resolution)

• Read the diff and changed files
• Analyze PublishDateUpdater.java changes
• Review new config properties
• Review new test coverage
• Post review feedback

---

I reviewed the workflow-resolution gate, the transaction refactor in processContentInBatch, the new config properties, and the added unit tests. This is a well-constructed, flag-gated change (PUBLISH_JOB_QUEUE_RESPECT_WORKFLOW_RESOLUTION=false by default) with good test coverage. No blocking issues.

New Issues

• 🟡 Medium: PublishDateUpdater.java:673-694 — Global mode permanently holds content that has no current workflow step. When the flag is enabled with an empty schemes list (global mode), configuredSchemes.isEmpty() short-circuits the scheme-matching branch, so findSchemesForContentType is never consulted. If findCurrentStep returns Optional.empty(), resolved is false and the content is skipped on every job run. Content that legitimately has no active workflow step (e.g. never entered a workflow) will never be scheduled-published while the flag is globally on. This matches the documented intent ("publication requires an explicitly resolved current workflow step"), but the asymmetry with scoped mode — where no-step content is allowed unless a configured scheme is assigned (publisherAllowsContentWithoutStepWhenConfiguredWorkflowIsNotAssigned) — is surprising and easy to trip over. Worth confirming this is intended and documenting it in the property comment. Fix this →

• 🟡 Medium: PublishDateUpdater.java:659,674 — Config is read once per contentlet. workflowAllowsScheduledPublish calls Config.getBooleanProperty(...) and configuredWorkflowSchemes() calls Config.getStringProperty(...) on every process() invocation, i.e. once per contentlet in the batch. Config is cached so the cost is small, but re-parsing/splitting the schemes string per item is avoidable — resolving the flag and the parsed scheme set once per batch (or per Publisher instance) would be cleaner. Non-blocking. Assumption: Config.*Property is backed by the in-memory cache; What to verify: no per-call I/O.

Notes (non-blocking)

• Transaction refactor looks correct and is an improvement. Moving the commit trigger from transactionProcessed to transactionVisited (incremented for every visited inode, including missing/failed ones, outside the per-item try) fixes the prior behavior where a long run of failures or missing contentlets could keep a single transaction open indefinitely. The restart guard totalVisited < allInodes.size() is also more accurate than the previous per-search-batch currentIndex + searchBatchSize < allInodes.size(). I traced the boundary cases (commit landing exactly on the final item, transactionStarted toggling across commits, the final tail commit) and did not find a double-commit or leaked-transaction path.
• Fail-closed on lookup errors is sound. workflowAllowsScheduledPublish propagates DotDataException/DotSecurityException; the batch loop catches it per-item and continues, so one content's workflow-lookup failure doesn't abort the queue. Covered by publisherFailsClosedWhenWorkflowLookupFails and batchClosesTransactionWhenProcessorFails.
• Conventions: Config.getBooleanProperty/getStringProperty, Logger, APILocator, and @VisibleForTesting are all used correctly; no SQL, no secrets, no new deps.
• Tests cover the flag-off path, resolved/unresolved steps, no-step content, scoped-by-id/variable matching, unpublisher bypass, and the transaction-close-on-failure path. Good coverage of the new behavior.

• Branch

[fabrizzio-dotCMS]

Suggestion: read the config once per run, not per contentlet

workflowAllowsScheduledPublish currently calls Config.getBooleanProperty(...) and re-parses configuredWorkflowSchemes() (split + stream + collect) for every contentlet. Since Publisher is created once per job run, snapshot these in its constructor:

// Publisher constructor
this.respectWorkflowResolution = Config.getBooleanProperty(RESPECT_WORKFLOW_RESOLUTION_PROPERTY, false);
this.configuredSchemes = configuredWorkflowSchemes(); // parsed once

workflowAllowsScheduledPublish then takes the resolved values instead of reading Config itself.

Alternative: if you prefer all run-scoped config in one place, resolve them at the top of updatePublishExpireDates(...) (the job entry point) and pass them down.

Please don't use a static final Lazy<> here: it caches for the whole JVM lifetime, so changing PUBLISH_JOB_QUEUE_RESPECT_WORKFLOW_RESOLUTION would need a Tomcat restart. Reading once per run keeps the values stable during the run and still picks up changes on the next fire.

[fabrizzio-dotCMS]

Test gap: mid-batch commit triggered by a failure

batchClosesTransactionWhenProcessorFails uses transactionBatchSize=10 with a single inode, so it only exercises the final commit path. The core of the transaction refactor is that a failed/missing record now counts toward the commit boundary mid-batch. Suggest adding a case with several inodes and transactionBatchSize=2 where an early item throws, to lock in that behavior.

[fabrizzio-dotCMS]

Overall the approach is solid and the opt-in flag keeps historical behavior safe by default. Requesting changes so we address a scaling issue plus a couple of smaller items before merge.

Must address

1. Held content is re-scanned every run (scaling). A gated contentlet stays live:false, so it keeps matching PUBLISH_LUCENE_QUERY and is re-fetched (find), re-checked (findCurrentStep), and re-logged on every run — the job runs every minute, so the cost grows linearly with the backlog of unresolved content and never clears on its own. Push the workflow predicate into the publish query so held items are excluded at the index level (+wfstep:(<resolvedStepIds>), resolved at query-build time since resolved is a step-definition property, not an indexed per-content field). Keep workflowAllowsScheduledPublish(...) as the authoritative final gate for index-staleness and the scoped/no-step cases. Details in a separate comment below.

2. Log level for held items. workflowAllowsScheduledPublish(...) logs at WARN per held item per run. A held item is expected steady state, not an anomaly — this becomes per-item-per-minute log spam. Lower to DEBUG (matches the existing "already auto-published and manually unpublished" skip).

Minor

3. Read the config once per run, not per contentlet. Config.getBooleanProperty(...) / configuredWorkflowSchemes() are evaluated inside the per-contentlet process(...) path. Resolve them once when the Publisher is built and reuse. (See inline comment.)

4. Test gap: mid-batch commit triggered by a failure. The transaction-boundary change now commits on visited count including failed/missing contentlets — please add a test that exercises a commit boundary reached via a failure, not only via successful processing. (See inline comment.)

[fabrizzio-dotCMS]

No need to re-read this value over and over again; do it once at the beginning, at the constructor level

[fabrizzio-dotCMS]

On the "records pile up and come back every run" concern — agreed, and I think the right fix is to push the workflow predicate into the search query.

The candidate set is fetched here via an Elasticsearch query:

// PublishDateUpdater.java:449
final List<String> allInodes = contentletAPI.searchIndex(luceneQuery, 0, 0, null,
        systemUser, false).stream().map(ContentletSearch::getInode).collect(Collectors.toList());

Since a held contentlet stays live:false, it keeps matching PUBLISH_LUCENE_QUERY and is re-fetched, re-checked (find + findCurrentStep), and re-logged (WARN) on every run — the job runs every minute, so this cost scales linearly with the backlog of unresolved content and never clears on its own.

To be precise: filtering in the query does not stop the content from accumulating (that's inherent to the feature — we intentionally hold it until the workflow resolves). What it fixes is that the accumulation stops being expensive to skip repeatedly. Per-run cost goes from O(backlog) to O(actually-publishable):

• held items are excluded at the index level → no find, no findCurrentStep, no WARN for them each minute;
• searchIndex(..., 0, 0, ...) no longer materializes the full matching set in memory every run.

Feasibility: the workflow fields are indexed — wfstep / wfscheme (ESMappingConstants), and there's precedent for querying them (WorkflowAPIImpl builds +wfscheme:( … ) +working:true and +(wfstep:… )). The one caveat is that resolved is not a per-content indexed field — it's a property of the step definition (WorkflowStep.resolved). So we can't add +resolved:true; instead we'd resolve the set of resolved step IDs at query-build time and add an inclusion clause, e.g. +wfstep:(<resolvedStepId1> <resolvedStepId2> …).

This should go into the publish query only — built around getPublishLuceneQuery(...) / the PUBLISH_LUCENE_QUERY template (PublishDateUpdater.java:80, :323) — and must not touch the unpublish/expire path (:331), per the issue's constraint that expiration is unaffected.

Suggestion — keep both, they solve different problems:

1. Add the +wfstep:(resolvedStepIds) pre-filter to the publish query → fixes the scale/every-minute concern.
2. Keep workflowAllowsScheduledPublish(...) as the authoritative final gate. The index reflects the last reindex of the content, so there's a small staleness window after a workflow transition; the Java check reads live DB state and also covers the scoped/no-current-step semantics that a pure inclusion filter can't express cleanly (in scoped mode, step-less content outside the configured schemes should still publish).

Net: the query filter is the scaling optimization; the Java gate stays as the correctness safety net.