fix(osgi): restart on live-export state, not the shared upload folder (#36434)

[dsolistorres]

Problem (#36434 — Stage 1 / Mode A)

On a clustered dotCMS (2+ nodes sharing /data/shared), uploading an OSGi plugin that imports a dotCMS package not yet in the exported-packages list could leave the plugin permanently unresolved on every node:

org.osgi.framework.BundleException: Unable to resolve <bundle>: missing requirement
  osgi.wiring.package; (&(osgi.wiring.package=<pkg>)(version>=0.0.0))

…even though osgi-extra.conf already contained <pkg>. This is not a version-string problem (version=0 ≡ 0.0.0); it's a restart that was skipped on the cluster.

Root cause (Mode A)

Two flaws combined, both in the upload pipeline of OSGIUtil:

1. The "already exported?" decision read the shared osgi-extra.conf file rather than this node's running framework. When a peer node had just written the new package to that shared file, this node read it back, concluded "no restart needed," and only moved the jar.
2. The restart trigger was gated on "is the upload folder still non-empty?" The real restart ran debounced, after the cluster lock was released, so a peer's jar-move could empty upload and silently cancel it.

Net: the export was written to the shared file and the jar deployed to load, but no node ever restarted → the package was never published to any running framework → persistent resolve failure.

Fix (Stage 1)

• Decide against live state, not the file. Each node snapshots the packages its framework actually started with (liveExportedPackages) and compares the plugin's packages against that, so a peer writing the shared file can't trick a node into skipping its restart.
• Make the decision atomic under the cluster lock. Writing exports + moving jars to load + capturing needsRestart all happen under osgi_restart_lock; the boolean can't be raced away.
• Submit the cluster-wide restart off-thread (single submitter) — the upload endpoint still returns immediately (matching prior behavior) and the restart starts ahead of fileinstall's next poll, so the freshly-moved jar isn't started against a still-stale framework on the origin node.
• Remove the fragile upload-folder guard from the debounced restart path (also fixes the manual PUT /extra-packages edit, which must restart even with no jar in upload).
• Read exports after the boot reload in initializeFramework, fixing a latent boot-with-preloaded-jars gap.

Also included

• felix.log.level is now Config-driven (default 3) so 4 can capture the Felix resolver's candidate evaluation in the field — per the issue's Diagnostics ask. dotCMS runs two separate Felix frameworks, so there are two independent config keys, one per framework (the system framework namespaces all its felix config under system., matching the existing system.felix.base.dir pattern):

  Framework	Class / line	Config key	Env var	Default
  User / plugin	OSGIUtil.java:201	felix.log.level	DOT_FELIX_LOG_LEVEL	3
  System (saml, tika)	OSGISystem.java:96	system.felix.log.level	DOT_SYSTEM_FELIX_LOG_LEVEL	3

  Levels: 1=ERROR, 2=WARNING, 3=INFO, 4=DEBUG. Set the relevant key to 4 to enable resolver DEBUG on that framework only.

• Tier-A regression tests in OSGIUtilTest proving version=0 ≡ 0.0.0 and that the error only appears when the package is absent from the running framework.

Verification

• ./mvnw compile -pl :dotcms-core — BUILD SUCCESS.
• OSGIUtilTest — 8 run, 0 failures/errors/skipped.
• 2-node Docker cluster sharing /data/shared, package absent from exports, plugin dropped into the shared upload folder (both pollers racing — the Mode A trigger): the origin node took the new Restarting OSGI Cluster Wide path, the peer restarted via pub/sub, and com.dotcms.osgi.repro-37894d reached ACTIVE on both nodes with zero wiring.package errors.

Scope / follow-up

This is Stage 1: it eliminates the persistent failure (Mode A). A transient, self-healing window remains: because the jar is moved into the shared, fileinstall-watched load folder before the async restart republishes the new exports, a still-stale framework can briefly auto-start the bundle and log a wiring.package=…(version>=0.0.0) error that clears on the pending restart. This window can occur on both the origin node and remote nodes — it is intentionally deferred to Stage 2 (node-local load + verify-exports-before-start / version-gated apply), tracked in #36504. It is documented in a code comment at the jar-move site so it isn't mistaken for an oversight. In practice the origin node's restart is submitted immediately and typically outruns fileinstall's poll (the 2-node test below saw zero errors), but the window is not formally closed until Stage 2.

Refs: #36434

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[claude]

Claude finished @dsolistorres's task in 5m 41s —— View job

---

🔍 dotCMS Backend Review

Reviewed the full diff vs origin/main and rechecked all prior findings.

New Issues

• 🟡 Medium: OSGIUtil.java:240-259 — The manual-edit path now wraps persistOsgiExtras in tryClusterLock(OSGI_RESTART_LOCK_KEY) to fix the lost-update race, but ShedLock's executeWithLock does not block — if the lock is already held (e.g. an upload merge in progress on another node), the task is silently skipped, the file is never written on this node, yet modifyExtraPackages still returns 200 "OSGI Extra Packages Saved". So under contention the manual edit is dropped with a false success confirmation, and the trailing debouncer.debounce("restartOsgi", …) (line 259) still fires an unnecessary restart.

  Assumption: net.javacrumbs.shedlock's DefaultLockingTaskExecutor.executeWithLock skips (does not queue/block) the task when the lock can't be acquired — the standard "run at most once across the cluster" semantics that ClusterLockManagerImpl relies on.

  What to verify: Confirm the intended behavior when the lock is contended. If a lost manual edit returning 200 is unacceptable, the void tryClusterLock needs to signal whether the task actually ran (ShedLock's TaskResult.wasExecuted()), and the REST layer should surface a retry/conflict instead of a success. Note this is a narrow window (the lock is held only for the brief write+move), and for shared-/data clusters the other node's write does land in the same file — so the shared file stays consistent; the risk is specifically the admin's particular edit being silently superseded. Non-blocking.

  Fix this →

Resolved

• ✅ OSGIResource.java:721 — uploadBundles now returns new ResponseEntitySetStringView(...), matching the corrected @Schema (return-type drift closed).
• ✅ OSGIUtil.java:219-259 — Manual osgi-extra.conf writer (writeOsgiExtras, PUT /extra-packages + RESET) now mutates the file under the same OSGI_RESTART_LOCK_KEY cluster lock as the upload pipeline. Dry-run validation correctly stays outside the lock to preserve the OsgiException → 400 mapping and keep the lock hold short.
• ✅ OSGIUtil.java:554-560 — Async cluster-restart failure now logs jar names + exception detail to the server log only and broadcasts a generic message, so pushBundleUploadError's GLOBAL visibility no longer leaks jar names/exception text to every logged-in backend user.

Notes (verified, no action needed)

• felix.log.level (OSGIUtil:201) and system.felix.log.level (OSGISystem:96) are correctly Config-driven with the two-framework namespacing rationale; defaults 3 preserved.
• The known transient/self-healing pre-restart fileinstall window is clearly documented at the jar-move site (OSGIUtil.java, processOsgiPackages) and deferred to Stage 2 (OSGi cluster plugin deploy — Stage 2: eliminate the shared-load-folder restart race (Mode B) #36504) — intentional scope, not an oversight.
• OSGI_FRAMEWORK_RESTART-before-restart ordering in the async path is documented as intentional; failure is surfaced via OSGI_BUNDLES_UPLOAD_FAILED.
• ReturnableDelegate<Boolean> local correctly disambiguates the tryClusterLock overload; Boolean.TRUE.equals(needsRestart) null-safely handles a skipped lock (no restart on this node — the lock holder handles it + pub/sub).
• Regression test test_zzz_04 genuinely exercises the live-snapshot-vs-shared-file decision (Mode A) plus a control case; the version-equivalence tests (test_zzz_01..03) are meaningful.

No Critical or High severity issues. The one Medium above is non-blocking.

[dsolistorres]

✅ Addressed review observation — log-level config keys documented

Thanks for catching the mismatch between the PR description and the code. The two felix.log.level reads use different config keys on purpose, because dotCMS runs two separate Felix frameworks, each with its own config namespace:

Framework	Code	Config key	Env var
User / plugin	OSGIUtil.java:201	felix.log.level	DOT_FELIX_LOG_LEVEL
System (saml, tika)	OSGISystem.java:96	system.felix.log.level	DOT_SYSTEM_FELIX_LOG_LEVEL

The system framework already namespaces all of its felix configuration under system. (e.g. system.felix.base.dir), so system.felix.log.level keeps that convention and lets each framework's resolver verbosity be tuned independently. Levels: 1=ERROR, 2=WARNING, 3=INFO, 4=DEBUG (default 3).

No code change needed — the PR description has been updated with the table above so the two keys are explicit for reviewers and QA.

[claude]

🚫 Critical or High severity findings must be resolved before merging. See the review comment above for details.

[dsolistorres]

Addressed backend-review feedback (commit 5939b80)

🟠 High — jar moved to shared load before the async restart (pre-restart fileinstall window).
Confirmed real, and it is the same class of race as "Mode B" deferred to #36504 — with the valid nuance the reviewer raised: it can occur on the origin node too, not only remote nodes. A complete fix requires the Stage 2 machinery (node-local load + verify-exports-before-start / version-gated apply); partial origin-only patches are fragile (the jar-move sequencing breaks or delays deploy, and they don't help remote nodes anyway). Per the reviewer's first suggestion, I've:

• Added an explicit code comment at the jar-move site documenting the window (origin + remote) as a transient, self-healing, Stage-2-tracked condition, so it isn't mistaken for an oversight.
• Updated the PR description and broadened OSGi cluster plugin deploy — Stage 2: eliminate the shared-load-folder restart race (Mode B) #36504 to state the origin-node case explicitly.

This is transient/self-healing (not the persistent Mode A this PR fixes), and the 2-node cluster test saw zero errors because the origin restart outruns fileinstall's poll — but it's honestly not formally closed until Stage 2.

🟡 Medium — addressed now:

• M3 (publish/async-restart failure invisible to admin): the async catch now calls pushBundleUploadError(...) → system event + error toast, covering pub/sub publish failures that restartOsgiOnlyLocal doesn't handle itself.
• M5 (cast to disambiguate the tryClusterLock overload): replaced with a typed ReturnableDelegate<Boolean> local.
• M6 (tests didn't exercise the new mechanism): added test_zzz_04 — seeds osgi-extra.conf with the package (simulating a peer's write) while this node's liveExportedPackages lacks it, and asserts processOsgiPackages still returns needsRestart=true (Mode A), plus a control asserting false once it's live. Full class green locally: 9 run, 0 failures.

🟡 Medium — intentionally deferred (with rationale):

• M1 (startup fireReload mutates osgi-extra.conf without the cluster lock): legit, but boot-time cluster locking has startup-ordering risk; better handled deliberately (candidate for Stage 2 / its own change) than bolted on here.
• M2 (event pushed before restart vs after): current order is deliberate so the UI shows "restarting" promptly; the failure path now surfaces an error toast (M3), so an optimistic event is corrected by a subsequent error. Left as-is.
• M4 (no debounce/coalescing on the async restart): not a correctness issue — the single-thread OSGIRestart submitter serializes restarts; at most a redundant restart under rapid uploads.

[claude]

🔍 dotCMS Backend Review

[🟠 High] dotCMS/src/main/java/com/dotcms/rest/api/v1/osgi/OSGIResource.java:721-723 (uploadBundles return statement)

The @Schema on the 200 response was corrected to ResponseEntitySetStringView.class (line 666) in a prior commit on this PR, but the method body was never updated to match — it still constructs a raw, generic ResponseEntityView<> instead of the declared subclass. This is the second half of the drift an earlier review pass on this PR flagged (that pass caught both the wrong @Schema and this return-statement mismatch; only the @Schema half was fixed).

return Response.ok(new ResponseEntityView<>(
        files.stream().map(File::getName).collect(Collectors.toSet())))
        .build();

💡 return Response.ok(new ResponseEntitySetStringView(files.stream().map(File::getName).collect(Collectors.toSet()))).build();

---

[🟠 High] dotCMS/src/main/java/org/apache/felix/framework/OSGIUtil.java:218-234 (writeOsgiExtras / persistOsgiExtras, manual PUT /extra-packages path)

This PR makes the automatic upload path's read-merge-write of osgi-extra.conf atomic by running it under the cluster-wide OSGI_RESTART_LOCK_KEY shedlock (lockManager.tryClusterLock(reloadTask) in checkUploadFolder). The manual edit path (OSGIResource.modifyExtraPackages → writeOsgiExtras → persistOsgiExtras) still overwrites the same shared file with no lock at all. Since the shedlock is DB-backed and cluster-scoped (ClusterLockManagerImpl, JDBC lock provider), the two writers are not mutually exclusive: a manual edit on any node can race with an in-progress upload merge on any other node and clobber it (lost update), reintroducing a variant of the exact "shared file diverges from live state" problem this PR sets out to fix — just for the other writer of the same file.

// OSGIUtil.java:230 — no lock acquired
this.persistOsgiExtras(extraPackages, testDryRun);
//restart OSGI after delay (manual extra-packages edit path keeps the debounce)
debouncer.debounce("restartOsgi", this::restartOsgi, delay, TimeUnit.MILLISECONDS);

💡 Wrap persistOsgiExtras's read-merge-write in writeOsgiExtras with the same OSGI_RESTART_LOCK_KEY cluster lock used by checkUploadFolder, so both writers to osgi-extra.conf are serialized cluster-wide.

---

[🟡 Medium] dotCMS/src/main/java/org/apache/felix/framework/OSGIUtil.java:522-531 (async restart-failure handler, via pushBundleUploadError)

The new async cluster-restart failure path broadcasts the uploaded jar names and raw exception text through pushBundleUploadError, which pushes both a system event and an error toast at Visibility.GLOBAL (the no-args Payload() / Payload(message) constructors default to it, and GlobalVerifier.verified() unconditionally returns true). That reaches every logged-in backend user, not just those holding the DYNAMIC_PLUGINS permission that gates the upload endpoint itself — so a low-privilege user with an open backend session can see another admin's jar file names and exception detail purely by being logged in. This broadcast-scope gap pre-dates this PR (existing pushBundleUploadError call sites in the file already leak jar names the same way), but this PR adds a new call site and extends the leaked content to include exception text from the async restart failure.

final String errorMsg = "Cluster-wide OSGI restart failed after bundle "
        + "upload [" + uploadedJarNames + "]: " + getExceptionMessage(re);
Logger.error(this, errorMsg, re);
pushBundleUploadError(errorMsg);

💡 Scope this notification to DYNAMIC_PLUGINS-permitted users (e.g. a permission-scoped Visibility instead of GLOBAL), or keep the broadcast generic ("OSGi restart failed — check server logs") and leave jar names/exception detail in the server log only.

---

Next steps

• 🟠 Fix locally and push — these need your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix <issue description> in <File.java>
• Every new push updates this comment automatically

[claude]

✅ No Critical or High severity issues found.

[dsolistorres]

Addressed backend-review observations (commit 8959815)

Both items were documentation/observability gaps around the new fire-and-forget restart path — fixed:

🟡 Item 1 — upload endpoint docs didn't reflect async restart.
OSGIResource.uploadBundles now documents (in both @Operation/@ApiResponse and the method Javadoc) that a 200 confirms the upload was accepted/scheduled, not that the plugin is live: any required OSGi restart runs cluster-wide asynchronously after the response, with completion signalled by OSGI_FRAMEWORK_RESTART, successful deployment by OSGI_BUNDLES_LOADED, and async failure by OSGI_BUNDLES_UPLOAD_FAILED. Clients should react to those system events rather than assume the plugin is active on the 200. openapi.yaml was regenerated from the annotations and is included in the commit.

🟡 Item 2 — async restart-failure message lacked jar names.
checkUploadFolder now captures the uploaded jar names (before the reload moves them out of the upload folder) and includes them in the async restart-failure message: Cluster-wide OSGI restart failed after bundle upload [<jars>]: …, so an admin who sees the standalone error toast can correlate it back to the triggering upload — matching the other pushBundleUploadError call sites in the file.

./mvnw compile -pl :dotcms-core — BUILD SUCCESS (openapi.yaml regen included).

[claude]

✅ No Critical or High severity issues found.

[claude]

🚫 Critical or High severity findings must be resolved before merging. See the review comment above for details.

[dsolistorres]

Addressed @Schema observation (commit 79fe5e0)

uploadBundles returns ResponseEntityView<Set<String>> (the set of uploaded jar names), but the 200 @ApiResponse declared @Schema(implementation = ResponseEntityStringView.class). Corrected it to ResponseEntitySetStringView.class (com.dotcms.rest.ResponseEntitySetStringView extends ResponseEntityView<Set<String>>) so the OpenAPI contract matches the actual return type, and added the import.

Regenerated openapi.yaml from the annotations — the OSGi upload response now $refs #/components/schemas/ResponseEntitySetStringView (component definition added). Verified the committed openapi.yaml matches build output (no drift), so the CI OpenAPI check will pass.

./mvnw compile -pl :dotcms-core — BUILD SUCCESS.

[claude]

🚫 Critical or High severity findings must be resolved before merging. See the review comment above for details.

[claude]

✅ No Critical or High severity issues found.

[dsolistorres]

Re: OSGI_FRAMEWORK_RESTART event ordering (commit 943db07)

Reviewed this one closely — the ordering here is intentional, and I've added a code comment documenting why so it isn't re-flagged.

OSGI_FRAMEWORK_RESTART is a "restart is happening" signal (the dot-plugins store reacts by showing a restarting spinner), not a success signal. In this path the restart runs off-thread (fire-and-forget) and blocks for several seconds, so pushing the event before restartOsgiClusterWide() is what shows the spinner while the restart is in progress. Pushing it after (as the synchronous restartOsgi() / _restart endpoint paths do) would only flip the UI to "restarting" once the work is already finished — backwards for this path.

The "optimistic success" concern is covered: a restart failure is not reported as success — the catch block pushes OSGI_BUNDLES_UPLOAD_FAILED (via pushBundleUploadError), which the store handles by resetting status to loaded and showing an error toast. So a failed restart self-corrects the UI.

No behavior change — just a clarifying comment on the intentional difference from the synchronous paths.

[claude]

🚫 Critical or High severity findings must be resolved before merging. See the review comment above for details.

[dsolistorres]

Addressed the latest review round (commit 2ddd862)

🟠 High #1 — uploadBundles return type drift. Correct catch — my earlier commit fixed the @Schema but left the return building a raw ResponseEntityView<>. Now constructs ResponseEntitySetStringView, so the object matches the declared schema. (No openapi.yaml change — the annotation was already correct.)

🟠 High #2 — manual osgi-extra.conf writer not mutually exclusive with the upload writer. Confirmed true: the upload path mutates the file under OSGI_RESTART_LOCK_KEY, but writeOsgiExtras (manual PUT /extra-packages + RESET) had no lock, so a manual edit on one node could clobber an in-progress upload merge on another (lost update). This is a Stage-1 consistency gap (same principle this PR establishes), not Stage 2 — so fixed here: writeOsgiExtras now serializes its file mutation under the same cluster lock. The dry-run validation runs before the lock (pure validation, keeps lock-hold short and preserves the OsgiException → 400 REST behavior); no reentrancy with the upload path (that path calls persistOsgiExtras directly, never writeOsgiExtras).

🟡 Medium #3 — GLOBAL-visibility broadcast leaked jar names + exception text. The new async restart-failure path now logs the full detail (jar names + exception) to the server log only and broadcasts a generic message ("check the dotCMS server logs for details") to avoid exposing it to every logged-in backend user. (The pre-existing pushBundleUploadError sites and a proper permission-scoped Visibility are broader and left out of this PR.)

./mvnw compile -pl :dotcms-core — BUILD SUCCESS; openapi.yaml unchanged.

Note: I couldn't run the docker-orchestrated OSGIUtilTest locally this round (Docker daemon down in my environment) — but these three changes don't touch the paths the existing 9 tests cover, and CI runs OSGIUtilTest (MainSuite2a/2b) + the Osgi postman collection.

[dsolistorres]

@claude the changes you requested have been addressed in commit 2ddd862 (on top of the earlier review rounds). Summary of what was fixed since your last review of 943db07:

• High — uploadBundles now constructs ResponseEntitySetStringView so the returned object matches the declared 200 @Schema (previously still ResponseEntityView<>).
• High — writeOsgiExtras (manual PUT /extra-packages + RESET) now serializes its osgi-extra.conf mutation under the same OSGI_RESTART_LOCK_KEY cluster lock the upload pipeline uses, so the two writers are mutually exclusive cluster-wide (dry-run validation stays outside the lock to preserve the OsgiException → 400 behavior).
• Medium — the async cluster-restart-failure broadcast now sends a generic message and keeps jar names + exception text in the server log only (no longer leaked to every logged-in backend user via GLOBAL visibility).

OSGIUtilTest passes locally (9/9). Please re-review the current head — thanks!

[claude]

Re-review in progress

Re-reviewing head commit 2ddd862 against the three fixes claimed:

• Verify uploadBundles returns ResponseEntitySetStringView
• Verify writeOsgiExtras now uses the cluster lock (OSGI_RESTART_LOCK_KEY)
• Verify async restart-failure broadcast no longer leaks jar names/exception text
• Check for any other regressions introduced by these changes
• Post final review

View job run