feat(content): add bulk refresh endpoint and enable the Refresh quick action - #37131
Open
rjvelazco wants to merge 21 commits into
Open
Conversation
POST /api/v1/content/_bulkrefresh is the bulk counterpart of the existing
single-item PUT /api/v1/content/_refresh/{identifierOrInode}, which is
unchanged. Not a full index rebuild -- POST /api/v1/esindex/reindex is a
different operation and stays untouched.
Job-backed: the POST answers 202 with a job id and a status URL, and the
client polls that URL until the job is terminal. Reindexing a large
selection synchronously would hold a request open for minutes.
Indexing is deliberately synchronous per item (IndexPolicy.WAIT_FOR). The
default DEFER only enqueues into dist_reindex_journal and returns, which is
why the single-item endpoint can answer true with nothing reindexed. This
endpoint exists so that "done" means done.
Work is done per identifier across all its versions, not per submitted
inode: content missing from search is rarely confined to one language, so
three selected language rows of the same contentlet are one reindex. Each
result still names every inode that resolved to it, so a client can settle
each row it selected.
An inode that no longer resolves is a per-item failure, not a request
error -- a selection can go stale between the click and the submit, and one
dead row must not cost the caller the rest of the batch. Failures are caught
per identifier for the same reason.
Not modelled as a workflow action or a SystemAction: bulk fire resolves its
target set by searching the index, which is circular for an operation whose
job is to fix the index. Content missing from the index cannot be found by
an index search, so the items that most need this are exactly the ones that
path could never reach. The processor goes DB-first via findAllVersions.
Gated on CMS Power User or CMS Administrator, matching the legacy button in
view_contentlets.jsp, so nobody who could press it loses access and nobody
who could not gains it. Capped by CONTENT_BULK_REFRESH_MAX_ITEMS (default
500) and accepts no Lucene query: an unbounded set plus synchronous writes
is a self-inflicted full reindex.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Unit tests drive the processor through mocked collaborators: per-identifier de-duplication, per-item failure isolation (including a DotSecurityException from findAllVersions as an item failure rather than a job failure), the cancel boundary, and the WAIT_FOR policy reaching the index call. Counters are read back through getResultMetadata -- the same path production reads -- rather than a test-only accessor. The integration test that matters most deletes a contentlet from the index behind dotCMS's back and asserts it is findable again once the job reports SUCCESS. That is what actually proves indexing is synchronous: under DEFER the job would report SUCCESS having only written a journal row, and this assertion would fail. The cancel test asserts the invariant rather than winning the race. Whether cancellation lands mid-flight or the run finishes first, the counters still close over total; a test that depended on that timing would be flaky rather than informative. Postman covers the contract outside the UI: submit, poll to terminal, and the 400/404 paths. Registered in the default-split group alongside ContentImportResource, its job-backed peer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refresh was rendered but disabled in the Content Drive Action Center. It is
now selectable for any selection containing at least one contentlet,
regardless of live, archived or locked state -- none of those affect whether
the index copy of a contentlet is stale, which is all a reindex fixes.
DotBulkRefreshService submits the selection and polls the job to completion,
emitting the final counters once. The first poll is immediate so a fast job
is not held back by the interval, polling stops on the terminal value
itself, and a five-minute cap keeps a job on a dead node from leaving the
toolbar claiming work is in progress for the rest of the session.
No live counters. The status endpoint reports a progress float but nothing
item-wise while the job runs, so the outcome lands once, at the end.
The run reports through the same store settle path as every other quick
action, so the existing toast, grid reload and selection clear all apply
unchanged -- but with its own copy. The default partial-outcome message
names workflow causes ("you may not have permission, or the content is
locked", "not on their workflow step"); neither is why a reindex falls
short, and a shortfall explained by the wrong cause sends the user off to
fix something that was never the problem. So
DotContentDriveActionExecutionResult gained an optional partialDetailKey and
Refresh supplies its own. Absent, the default still applies, leaving
workflow actions byte-identical.
A finished job that reports no counters is routed to the error handler
rather than given a substituted count: the inode count would claim every
item was reindexed and zero would claim none were, and the first errs in
the reassuring direction.
Not gated client-side, even though the endpoint requires CMS Power User or
CMS Administrator. core-web has no power-user awareness -- currentUserIsAdmin
comes from getCurrentUser().admin and there is no role list -- so the only
gate available would hide Refresh from exactly the users the legacy button
was written for. A visible action that answers 403 is a better failure than
a capability silently withheld from people who have it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
14 tasks
github-actions
Bot
added
Area : Backend
Contributor
|
Claude finished @rjvelazco's task in 6m 8s —— View job Code review — bulk refresh (reindex the selection)Reviewed the latest state ( I re-checked each finding from the earlier rounds against the current code and they hold up:
I did not find any new blocking issues introduced by this PR. Non-blocking notes
Looks good to me. ✅ |
…ick-action-that-reindexes-the-selection
Two CI failures from the first run, both in test wiring rather than in the endpoint. The Postman collection read entity.result.metadata. JobView.result() is serialised by OptionalJobResultSerializer, which flattens the metadata map straight into `result` rather than nesting it under a `metadata` key -- so that path is undefined over HTTP, and reading .successCount off it threw a TypeError. The Java accessor really is JobResult.metadata(), which is why the integration tests are right and only the HTTP layer was wrong. Left a note at each call site so the "obvious" fix back to .metadata is not made later. BulkRefreshResourceIntegrationTest was in no @suite, so CI silently never ran it -- 14 tests that would have stayed dead weight in this PR and every future one. Registered in Junit5Suite1 next to ContentImportResourceIntegrationTest, its closest analogue and the other job-backed endpoint in that suite. Also dropped a setTimeout from the poll loop that paced nothing (Newman does not honour it) and added a terminal-state assertion to the no-results poll, so exhausting the poll cap reports that it stopped waiting instead of throwing on a null result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cess Code review found two Critical bugs, both in the frontend's reading of the job status response. The client read `result.metadata`, which does not exist over HTTP. OptionalJobResultSerializer flattens the job's metadata map straight into `result`, so that path was always undefined: refresh() always emitted null, executeRefresh always took the no-counters branch, and every successful reindex surfaced as an error toast with the grid never reloading. The same flattening had already been found and fixed in the Postman collection last commit, and documented in a comment there -- the TypeScript model was never brought in line, and the service spec built its fixture in the nested shape, so the suite went green against a contract the server never emits. The fixture is now the real wire shape and a test pins the flattened read specifically. The client also discarded job.state. A job that dies mid-run still carries the counters it had reached, so an all-zero result from FAILED_PERMANENTLY was indistinguishable from a clean run over nothing and rendered as "Refresh ran on 0 item(s)" in green. State now travels with the counters, and a run is only reported when it settled in SUCCESS or CANCELED *and* its counters close over total -- which also catches a job that stopped after 3 of 10 and would otherwise have hidden the 7 never attempted. Three silent-failure modes in the poll pipeline: - switchMap cancelled a status call that outlived the 1500ms interval, so under sustained latency no poll ever completed and only the overall timeout ended the run. Now exhaustMap. - A single transient poll failure abandoned a run the server was still progressing. Now retried three times. - A bare TimeoutError has no `status`, so the shared HTTP error handler matched no handler and said nothing at all -- indicator cleared, grid stale, no explanation. Now normalised to a 504. Status and cancel were gated only on "is a backend user", which made the 403 on submit decorative: any backend user could cancel a Power User's in-flight reindex, or read a job back and recover the submitted inode list and the submitter's id from its parameters. Both now require canRefresh. Also: the class javadoc overclaimed. WAIT_FOR guarantees the write is attempted rather than enqueued, but the underlying bulk call logs per-document failures without throwing, so SUCCESS is not a per-document guarantee. Scope of the claim is now stated. Removed SSE references left behind when SSE was dropped, gave DotBulkRefreshJobState a real union instead of one collapsing to string, guarded NaN progress on an empty work list, and restored alphabetical order in the data-access barrel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Run 3 failed on "expected [ 'SUCCESS', 'CANCELED', ... ] to include 'PENDING'": the poll loop burned all 60 attempts while the job was still queued, and gave up before it had even started running. Removing the no-op setTimeout last commit was correct — Newman does not honour it — but it left the loop with no pacing whatsoever, so 60 polls fired in under a second. Counting attempts was never the right bound either; what matters is elapsed time. The loop now waits a real 500ms per poll and gives up after 60 seconds of wall clock rather than after N attempts. The pacing is a short synchronous wait because that is the only thing that actually delays inside a Postman sandbox. It runs once per poll, so each script execution stays far below any sandbox timeout even though the loop as a whole can span a minute. The sibling ContentImportResource collection keeps a setTimeout that likewise does nothing; what carries it is its own elapsed-time bound, which is the part worth copying. Failure now names the last state seen, so "queued and never picked up" is distinguishable from "ran and died" instead of both reading as an opaque "did not reach terminal". The rest of the collection passed: 64 assertions executed with this as the only failure, and Check Terminal Result's assertions passed, which confirms the entity.result path fix from the previous commit is correct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ration test First real execution of these tests: 7 of 12 errored, in two distinct ways, both in the test's own setup rather than in the endpoint. Five timed out after 120s waiting for a terminal state. The job queue accepts jobs but does not process anything until it is explicitly started, so all 25 jobs created during the run sat at state=PENDING and the processor never ran once -- zero "reindexing" log lines against 25 "created by user" lines. ContentImportResourceIntegrationTest shares this suite and never noticed, because it only asserts job *creation*: 24 tests in under nine seconds. This was the first test here that actually needed a processor to execute. JobQueueManagerAPIIntegrationTest already had the answer -- start() plus awaitStart() behind an isStarted() guard -- so that is what this now does. The power user case failed differently and instantly: loadRoleByKey answers null for a role this database does not have, rather than throwing, and doesUserHaveRole(user, null) is then silently false. So the lookup was passed straight into user creation, producing a "power user" holding no such role and a permission check that could only ever fail. Now created if absent, mirroring TestUserUtils.getOrCreateAdminRole. Worth noting for review: canRefresh is the same expression as view_contentlets.jsp:209, so the production gate is faithful to the legacy one. But both inherit that silent null -- on an install without the CMS Power User role, every Power User is denied and nothing says why. Not introduced here, and not changed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zJaaal
reviewed
Aug 20, 2026
zJaaal
left a comment
zJaaal
left a comment
Member
There was a problem hiding this comment.
Review
Read the full diff and cross-checked the backend against the APIs it calls. This is unusually well-documented work and the architecture calls hold up: job-backed over synchronous, per-identifier over per-inode, and the reasoning for not modelling this as a workflow action or SystemAction is correct (bulk fire really does resolve its target set by searching the index, which is circular here).
One finding is worth blocking on, the rest are cleanups. Inline comments have the detail.
🔴 The cache eviction is a no-op and defeats the operation. contentletCache.remove(identifier) against an inode-keyed cache evicts nothing, and findAllVersions then reads back through that same cache. A stale cached version gets written to the index, which is the failure this endpoint exists to fix. Same call exists in the single-item _refresh, so it's inherited rather than new.
🟠 Every status poll returns the full submitted inode list (JobContract.parameters() isn't @JsonIgnored), ~200 times per run at up to 500 UUIDs each.
🟠 The integration test starts the shared job queue and never stops it, in a suite that also runs ContentImportResourceIntegrationTest and JobProcessorDiscoveryTest.
🟡 Also: the toast counts identifiers where the dialog counted rows; BulkRefreshHelper's no-arg constructor builds a bean that NPEs; status/cancel accept any job id regardless of queue; the two 200 responses are missing @Schema against the repo rule; the poll retries 401/403/404.
Verified as correct
Worth stating explicitly, since these are the load-bearing claims:
IndexPolicy.WAIT_FORgenuinely bypasses the journal (ContentletIndexAPIImpl.java:2308). TheinTransaction()branch below it doesn't apply here:processNextJobis@CloseDBIfOpened, not@WrapInTransaction.- The "counters sit directly on
result, not undermetadata" comment indot-content-drive.model.tsis exactly right perOptionalJobResultSerializer. @DependentplusprocessorInstancesByJobId.computeIfAbsentgives one processor instance per job, so the mutable counters don't leak between runs.Jdk8Moduleis registered on the job queue's mapper, soOptional<String> identifier()round-trips through the persisted metadata.- Path routing (matches the
/v1/content/_importprecedent), CDI discovery, and exception mapping all resolve:IllegalArgumentException→ 400,DotSecurityException→ 403,DoesNotExistException→ 404, and the form'sValueInstantiationException→ 400. - The
FAILED→FAILED_PERMANENTLYtransition the frontend depends on does happen:handleJobFailuremarks FAILED and requeues, thenprocessJobWithRetrypromotes it viahandleNonRetryableFailedJob. Worth knowing the gap between the two is bounded only by the 5-minute UI timeout, so a slow promotion surfaces to the user as the 504 "stopped waiting" copy rather than as a failure.
On the open items in the description
The two that look blocking are the ones already flagged: the integration and Postman suites have never been run, and test_bulkRefresh_makesContentMissingFromTheIndexFindableAgain is the only thing standing behind the central WAIT_FOR claim. The no-client-side-role-gate decision is defensible as argued. AC #1 is a one-comment fix on the issue.
🤖 Reviewed by Claude Code, posted by @zJaaal.
rjvelazco
marked this pull request as draft
August 20, 2026 20:26
…onse Review found the cache eviction was a no-op that defeated the operation, and it was right. ContentletCache is keyed by inode: ContentletCacheImpl stores under add(inode, contentlet) and remove(Contentlet) delegates to remove(getInode()). Removing by identifier evicted nothing. That matters more here than almost anywhere, because findAllVersions reads back *through* that cache -- ESContentFactoryImpl.findContentlets serves hits straight from it -- so a stale cached version was loaded and written to the index. The comment above the call claimed it prevented exactly that. Now the submitted inodes and every returned version are evicted by inode, and a second read supplies what gets indexed, so the indexed object came from the database rather than the cache. Inherited from the single-item _refresh, which still has it. The status endpoint returned JobView, which serializes parameters() -- for this queue the entire submitted inode list plus the submitter's id, up to 500 UUIDs, on every one of the ~200 polls a run attracts. Replaced with a view carrying id, state, progress and the result metadata, keeping the flattened result shape clients already read. The polling subscription needed takeUntilDestroyed. take(1) sits on the outer observable, which does not emit until the poll loop has finished, so it could not stop an infinite timer; the store is component-scoped, so leaving the portlet orphaned a poll that kept firing, could raise a global error dialog minutes later about invisible work, and would patch a store already replaced. Also: versionsIndexed now counts inside the loop so a partial failure reports the versions that did land; status and cancel reject job ids belonging to other queues rather than reaching across them; both 200s declare a @Schema per the repo rule; the poll no longer retries 401/403/404, which cannot improve on being asked again; the integration test hands the job queue back in the state it found it; and its cancel test no longer swallows every exception. Two pushed back on. The no-arg constructor on BulkRefreshHelper mirrors ContentImportHelper:93 exactly -- the cited precedent does have one -- and removing it risks an unproxyable-bean deployment failure, so it stays with the reason written down. And creating the CMS Power User role when absent is not a dead branch: this database genuinely lacks it, which is why that test failed before the helper existed, so failing loudly would only reinstate the failure. It now logs when it fires. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The client polled GET /_bulkrefresh/{jobId} every 1.5s for up to five minutes
-- roughly 200 requests per run -- because SSE had been removed and nothing
else told it the job had finished. That endpoint is now gone, and completion
arrives over the websocket the admin UI already holds open.
A listener on JobCompletedEvent pushes a BULK_REFRESH_COMPLETED system event
carrying the run's counters, scoped with Visibility.USER to whoever submitted
it, and records a notification so the outcome survives navigating away. The
Content Drive store subscribes and feeds the existing settle path, so the
toast copy, severity, grid reload and selection clear all behave exactly as
before.
JobCompletedEvent rather than the processor tail: it fires for every terminal
state including permanent failure, so the failure path needs no
catch-and-rethrow. JobFailedEvent is deliberately unused -- it signals a
*retryable* failure, where the job returns to the queue and is not finished.
And we push our own event rather than relying on job events reaching the
browser, because those travel as CLUSTER_WIDE_EVENT, which is explicitly
excluded from the websocket.
A custom event rather than the generic MESSAGE toast keeps i18n on the client
instead of having the server compose user-facing copy, renders in Content
Drive's own toast, and lets the same handler trigger the grid reload.
Improves on the legacy batch reindex rather than copying it. That one sets no
IndexPolicy, so it defers to the journal and its "finished successfully" means
enqueued, not searchable; it runs inline on the request thread at 50 items or
fewer; it announces completion to every CMS Administrator in the system user's
locale even when every item failed; and it raises one notification per failed
inode. This keeps WAIT_FOR, always backgrounds, tells only the submitter, and
words the notification on what actually happened.
Deleted with the endpoint: the status view types, the whole RxJS poll pipeline
(timer/exhaustMap/retry/takeWhile/last/timeout), and five now-unused model
types. The Postman collection loses its poll requests, so completion can no
longer be observed over HTTP -- that assertion now rests on the integration
test alone.
Verified: eslint clean on data-access, dotcms-models and portlets-content-drive;
nx format:check clean; 24 backend unit tests and 1172 frontend tests passing,
including a test that the socket subscription is actually wired rather than
just the reporter it calls.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zJaaal
added
the
PR: docker image
Contributor
🐳 PR Docker test imageLatest build for commit docker pull dotcms/dotcms-test:pr-37131-issue-36845-workflow-center-add-a-refresh-quick-action-that-reindexes-the-selection
docker pull dotcms/dotcms-test:pr-37131-issue-36845-workflow-center-add-a-refresh-quick-action-that-reindexes-the-selection_ed8e4ba |
…ick-action-that-reindexes-the-selection
Reported from manual testing: the toolbar sat on "applying refresh to 2 items" forever even though the POST returned 202 and the job finished. Two separate causes. The first was already fixed: before the listener was registered at startup it was never constructed, so the completion event was never pushed at all. Any build predating that commit shows exactly this. The second is this commit. Removing the poll loop removed its five-minute timeout with it, and nothing replaced it, so if the completion event never arrives there is no request whose failure surfaces and the run stays marked in flight for the rest of the session. That marker is the in-flight guard on every quick action here, so a stuck refresh also locked out Lock, Unlock and Add to Bundle, with only a page reload to escape. The event can legitimately go missing without anything being broken: system events disabled, the socket dropped between submit and completion, or the job abandoned on a dead node. A bounded wait now clears the marker and reports that we stopped waiting rather than claiming an outcome, since there are no honest counts to report. The run is held by object identity, not a flag, so a timer belonging to a finished run cannot clear a later one -- a flag would already have been cleared by the first settle. Three tests cover it, one of which caught its own flaw first: ticking the full timeout fired both runs' timers at once, which proved nothing until the two runs were staggered in time. Verified: eslint clean on the three touched core-web projects, nx format:check clean, 24 backend and 1279 frontend tests passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n fires Four changes to Refresh and one to every action. Refresh no longer sets actionExecution. That single field drove the toolbar's "Applying ... to N item(s)" indicator and locked the Action Center, and neither suits a job that runs for minutes and reports no progress. So the indicator is gone for Refresh, the dialog stays usable while a reindex runs, and a toast at trigger says the work continues in the background. The completion toast is untouched, because the shell effect reads only actionExecutionResult. Double-firing is still refused, via a dedicated refreshInFlight flag rather than the shared marker: it guards the reindex against itself without blocking Lock, Unlock, Add to Bundle or workflow actions. The five-minute timeout is repointed at that flag rather than deleted -- its old job disappeared with the indicator, but the new flag can get stuck the same way if the completion event never arrives. reportRefreshCompleted no longer clears actionExecution on any branch. That was harmless while a reindex held the only lock; now that it does not, a reindex completing mid-Lock would have un-gated Lock early and published a Refresh toast over it. There is a regression test for that specific collision. Firing any action now clears the grid selection, at the single hand-off point every action already funnels through -- and deliberately not when the dialog is merely dismissed, since the user may still be building a selection. This also closes existing desync: selection previously only cleared via loadItems in the result effect, so it never cleared on the reindex timeout, on any catchError branch, or when loadItems hit an early return, where the boxes stayed checked permanently. Clearing the store alone would not have unchecked anything. The shell passed no [selection], leaving the grid in its uncontrolled mode where it owns its own checked set and only drops it when the items reference changes. The shell now binds it, putting the grid in the controlled mode the action preview already uses. Verified that pagination, sort and filter still clear: they route through the search effect to loadItems, which empties the selection in the store. Also removed three console.log lines that were sitting uncommitted in the tree. Verified: eslint clean across data-access, dotcms-models, portlets-content-drive and ui (import-order errors the auto-fixer caught are fixed), nx format:check clean, 1283 + 197 + 5 frontend tests and 24 backend tests passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A review scoped to stale artefacts found that iterating on this feature left
real drift, plus two bugs that reproduce the exact misleading success the
endpoint was written to remove.
Bugs first.
The trigger toast fired unconditionally. executeRefresh returns early when a
reindex is already in flight, but the toast and the hand-off ran regardless -- so
firing Refresh again within the five-minute window told the user a reindex had
started, cleared their selection, and submitted nothing.
A settled run's timeout could abort a later one. Each executeRefresh armed a
five-minute timer that was never cancelled on completion, so run 1's timer fired
long after run 1 finished, saw run 2's flag set, and cleared it with a bogus 504.
This was a regression I introduced: the identity check that used to prevent it
went away with the switch to a boolean flag, and I deleted its test in the same
edit -- a test whose own comment had warned that a flag would not survive this.
The timer is now held and released wherever a run ends. Both bugs have tests
that fail against the previous commit.
Then the staleness. statusUrl was still built, returned, typed, documented,
asserted in the integration test and asserted in Postman -- pointing at the
GET /_bulkrefresh/{jobId} endpoint that was deleted when polling was replaced by
push. Every client that read it would have got a 404. Removed end to end, along
with the now-unused request parameter it needed, an orphaned javax.ws.rs.GET
import, and two Postman variables left over from the deleted request.
Also corrected roughly a dozen comments that still described the polling design,
including two that contradicted a correct comment a few lines below, and one
security claim that was simply false: the javadoc said the role gate stopped a
backend user reading the submitted inode list, but the generic
GET /api/v1/jobs/{jobId}/status returns the whole job with no role check. That
exposure is pre-existing and untouched here; only the claim of protection was
new, and it is now stated accurately.
Kept deliberately: the cancel endpoint, and the per-item result machinery.
includeItemResults is hardcoded false so the records are never produced in
practice, but they remain readable through the generic job-status endpoint, so
the comments now say that plainly instead of claiming a drill-down that does not
exist.
Dropped one tautological test -- it asserted the service never calls a status
endpoint, which the single-request service body makes impossible to fail.
Verified: eslint clean across four core-web projects, nx format:check clean,
1285 + 4 + 197 frontend tests and 24 backend tests passing, openapi regenerated
with exactly two paths and no statusUrl in the submit schema.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing called it. The Content Drive UI never wired a cancel control, and with the
polling status endpoint already gone there was no client left holding a job id.
Removed POST /v1/content/_bulkrefresh/{jobId}/cancel, the helper's cancelJob and
getJob (getJob existed only to queue-scope cancellation), and the resource's
authorized() — cancel was its only caller, since submit does its own role check in
BulkRefreshHelper.submit. Dropped the Postman request and the two integration tests
whose production methods no longer exist.
The processor stays Cancellable: a stuck reindex is still killable through the
generic POST /api/v1/jobs/{jobId}/cancel, so the invariant test moved onto that path
rather than being deleted with the endpoint.
Also corrected the submit operation description, which still claimed to return "the
URLs to follow the run" after statusUrl was removed, and carried the job-parameters
exposure caveat over to canRefresh rather than losing it with authorized().
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The five-minute timer only existed to un-wedge refreshInFlight, and that guard only existed to stop a double-fire. Neither survives the move to a backgrounded action: nothing on screen waits for a reindex any more, so there is nothing for a deadline to unblock, and a client giving up after five minutes reported a 504 about a run it had no information about — most likely one that had succeeded, and one the server records in the notification bell whether or not the socket delivered the event. Removing the timer alone would have been worse than keeping it: a lost completion event would leave the flag set and the Refresh button a silent no-op for the rest of the session. So the pair goes together. The double-fire it guarded is now handled by firing clearing the selection — executeQuickAction and executeRefresh both return early on an empty set, so a second run takes a deliberate re-selection, and running a reindex twice is wasteful rather than wrong. Tests: the three timer tests collapse into one asserting nothing is left ticking, the guard test becomes "a second reindex can be fired", and the component's "refused while in flight" case becomes an emptied preview — the remaining way to reach the toast with nothing submitted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zJaaal
reviewed
Aug 24, 2026
zJaaal
left a comment
zJaaal
left a comment
Member
There was a problem hiding this comment.
Code review of the bulk refresh endpoint and the Refresh quick action. 9 findings, posted inline, most severe first.
Highest impact: the permission gate resolves a role key that does not exist, so it degrades to admin-only, and the frontend consumes the completion event with no jobId correlation. That missing correlation is the root of three separate inline findings (clearing an unrelated action's guard, closing a dialog mid-edit, reacting to another tab's run); keeping the jobId from the 202 and filtering on it collapses all three into one fix.
Checked and found correct, so no need to re-verify: per-job processor instances (the AtomicInteger counters are safe), @Dependent scope against JobProcessorDiscovery, JobCompletedEvent firing on permanent failure, WAIT_FOR writing synchronously, the exception mappers producing the documented 400/403, the refreshed-partial placeholder order, and the dot-folder-list-view controlled [selection] mode.
Review authored by Claude (Claude Code), posted via @zJaaal's account.
zJaaal
approved these changes
Aug 24, 2026
zJaaal
left a comment
zJaaal
left a comment
Member
There was a problem hiding this comment.
Last feedback, good to merge after resolving my latests comments
…ick-action-that-reindexes-the-selection
…ick-action-that-reindexes-the-selection
…empty reindexes
Seven of nine review findings, plus notes on the two handled differently.
Correlation is the big one. BULK_REFRESH_COMPLETED is scoped to the submitting user,
not to a tab, so a grid reacted to runs it never started — toasting counts for content
it had not selected. The listener now puts the job id in the payload and the store keeps
the id from its own 202, ignoring everything else. That closes three findings at once.
Two ways the settle path disturbed unrelated work:
- The SUCCESS branch went through onSettled, which clears actionExecution. A reindex
landing while a Lock was in flight wiped Lock's indicator and reopened its replay
guard, so Lock could be fired again over rows already changing. It now publishes only
the result.
- The shell closed the dialog on any result. A pushed outcome can arrive minutes later,
while the user is filling in another action's form. Results now carry `backgrounded`,
and the shell neither closes the dialog nor reloads the grid under an open one.
Also: an identifier whose versions vanished between resolution and reindex was counted
a success with zero index writes; a user-cancelled run was reported as a failure at
ERROR, disagreeing with the toast that has always treated CANCELED as a partial; and an
absent request body answered 500 instead of the documented 400.
The role gate keeps its behaviour and loses its silence. `loadRoleByKey("CMS Power
User")` finds nothing in the starter data, so the gate is admin-only in practice — but
view_contentlets.jsp:209 makes the identical call, so this matches legacy rather than
regressing it. Granting real Power Users access would widen the gate beyond legacy,
which is a product decision; the miss is now logged instead of failing quietly.
The double findAllVersions is gone, but not the suggested way: evicting only by inode
would have skipped the page, site and relationship caches that remove(Contentlet)
invalidates. Cheap VersionableAPI inodes evict before the read so it is cold, the
Contentlets evict after so the derived caches go — one permission-filtered read instead
of two, nothing lost.
Not done: adding `max` to @SiZe. Bean validation runs after Jackson has deserialized the
body, so it cannot stop the list being materialized; only a container body limit can.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rjvelazco
enabled auto-merge
August 24, 2026 14:29
dario-daza
reviewed
Aug 24, 2026
dario-daza
approved these changes
Aug 24, 2026
Answers a review question on the suite registration. The split between this suite and the MainSuites is by test framework, not by subject area, and the file gave no hint either way. The MainSuites run @RunWith(MainBaseSuite.class) over JUnit 4's @SuiteClasses, so a jupiter test listed there contributes zero tests and nothing reports that it did not run. That is not hypothetical: this PR's integration test sat in no suite at all for a while, passing locally while CI never executed it, and a green pipeline that runs none of your new tests is the worst version of that failure. Comment only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rjvelazco
disabled auto-merge
August 24, 2026 17:25
rjvelazco
enabled auto-merge
August 24, 2026 17:25
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #36845
Adds a Refresh quick action to the Content Drive Action Center that reindexes every selected contentlet, and the bulk endpoint behind it.
The single-item
PUT /api/v1/content/_refresh/{identifierOrInode}already existed; there was no bulk equivalent over REST (only the legacy Strutscmd=full_reindex_list). This adds one rather than looping_refreshN times from the client — a client-side loop gives no cancellation, no survival across a page reload, and no server-side bound on cost.Endpoints
POST/api/v1/content/_bulkrefresh202+{jobId, statusUrl, submitted}GET/api/v1/content/_bulkrefresh/{jobId}POST/api/v1/content/_bulkrefresh/{jobId}/cancelKey decisions
Job-backed, not synchronous. Reindexing a 500-item selection synchronously would hold a request open for minutes.
202rather than200because the work is accepted, not done — a client must not be able to read the response as "reindexed".Indexing is synchronous per item (
IndexPolicy.WAIT_FOR). The defaultDEFERonly enqueues intodist_reindex_journaland returns, which is why the single-item endpoint can answertruewith nothing reindexed. This endpoint exists so that "done" means done — and there's an integration test that deletes content from the index and asserts it's findable again once the job reportsSUCCESS, which only holds if the write is synchronous.Per identifier, not per inode. Content missing from search is rarely confined to one language, so three selected language rows of the same contentlet are one reindex. Each result still names every inode that resolved to it, so the client can settle each row the user selected.
Not a workflow action or a new
SystemAction. Bulk fire resolves its target set by searching the index (WorkflowAPIImplbuilds a+inode:( … ) +(wfstep:…)Lucene query), which is circular for an operation whose job is to fix the index — content missing from the index cannot be found by an index search, so the items that most need this are exactly the ones that path could never reach. The processor goes DB-first viafindAllVersions. It would also inherit hardcodedIndexPolicy.DEFER, silentwfstepskips, and workflow side effects (actionlets, new versions, task/history rows) for an operation that changes no content.Unresolvable inodes are per-item failures, not request errors. A selection can go stale between the click and the submit; one dead row must not cost the caller the rest of the batch.
Role-gated server-side on CMS Power User or CMS Administrator, matching the legacy
view_contentlets.jspcheck, so nobody who could press the old button loses access and nobody who could not gains it. "Always available" in the issue means not gated by content state — that still holds.Capped by
CONTENT_BULK_REFRESH_MAX_ITEMS(default 500), and no Lucene query is accepted: an unbounded set plus synchronous writes is a self-inflicted full reindex.Frontend
Refresh was already rendered but disabled. It's now selectable for any selection with ≥1 contentlet regardless of live/archived/locked state, goes straight to the preview (a reindex needs no configuration), and reports through the existing store settle path — so the toast, grid reload and selection clear all come unchanged.
It gets its own toast copy. The default partial-outcome message names workflow causes — "failed (you may not have permission, or the content is locked by another user), skipped (this action is not on their workflow step)". Neither is why a reindex falls short, and a shortfall explained by the wrong cause sends the user off to fix something that was never the problem. So
DotContentDriveActionExecutionResultgained an optionalpartialDetailKey; absent, the default still applies, leaving workflow actions byte-identical.Final toast:
Testing
portlets-content-drive(full)dot-bulk-refreshserviceBulkRefreshResource)Backend unit tests were confirmed failing first (14 errors, all
UnsupportedOperationException) before implementation, per the constitution's TDD gate.test_bulkRefresh_makesContentMissingFromTheIndexFindableAgain, which is what proves theWAIT_FORpolicy actually took effect.Video
video.mov
video.mov
Reviewer notes / open items
core-webhas no power-user awareness —currentUserIsAdmincomes fromgetCurrentUser().adminand there's no role list — so the only gate available would hide Refresh from exactly the users the legacy button was written for. A visible action that answers 403 through the existing error handler beats silently withholding a capability from people who have it. Flagging in case you'd rather add a lookup.progressfloat but nothing item-wise mid-run, so the dialog can show a percentage but not "5 of 12". Deliberate, per PO discussion. Adding them means surfacing the processor's counters viaJobQueueManagerAPI.getInstance(jobId)on the status endpoint.SSEMonitorUtiloverload it needed was reverted, so that shared util has zero diff againstmain.PushPublishServicefails 4 tests (2026-08-20expected vs2026-08-19received) — its spec builds the expected date fromnew Date().toISOString()(UTC) while the service uses local time, so it fails every day after ~20:00 local. Untouched by this PR; worth its own ticket.openapi.yamlis regenerated (+132) and committed alongside the annotations, per the repo rule.🤖 Generated with Claude Code