App secrets store: concurrent saves from two nodes can silently lose a secret (lost update)

Description

Follow-up to #36724 / PR #37053, which fixed the corruption half of the shared App secrets store problem. This issue covers the remaining half: lost updates.

#36724 made saveSecretsStore() publish atomically (write tmp → fsync → ATOMIC_MOVE), so a concurrent reader can only ever see the whole previous store or the whole new one. That removes torn reads. It does not serialize the read-modify-write:

node A: getSecretsStore()          node B: getSecretsStore()
node A: keyStore.setEntry("a", …)  node B: keyStore.setEntry("b", …)
node A: saveSecretsStore()         node B: saveSecretsStore()

Both writes succeed atomically, last writer wins, and one of the two secrets is silently gone. No error, no log. SecretsKeyStoreHelper.saveValue() (:423-432) is exactly this pattern: load the whole store, set one entry, write the whole store back.

Config changes are rare and not done in bulk, so the window is small — but the consequence is a silently missing App credential, the same class of outcome #36724 exists to prevent.

Prior art already in the codebase — and the trap in it

com.dotcms.concurrent.lock.ClusterLockManager is ShedLock 4.33.0 over a JDBC provider on a shared shedlock table:

• The table already ships — postgres.sql:2521 and Task220401CreateClusterLockTable. No schema change needed.
• It is proven in production, not dead code: OSGIUtil uses it at four call sites (:238, :464, :477, :494) to serialize OSGi restarts across the cluster.

It cannot be used as-is. ClusterLockManagerImpl.tryLock() never checks ShedLock's wasExecuted():

final TaskResult<R> taskResult = this.executor.executeWithLock(task, ...);
return null != taskResult ? taskResult.getResult() : null;   // ClusterLockManagerImpl.java:53-54

ShedLock's contract is skip-if-locked, not wait-if-locked — it is designed for scheduled jobs where "another node already did it" is the correct outcome. For an OSGi restart that is fine. For a secrets save it would be catastrophic: the write is silently skipped, tryLock returns null, and the caller believes the secret was persisted. That would be worse than the bug this issue is about.

Acceptance Criteria

• A cluster-wide lock serializes the read-modify-write in SecretsKeyStoreHelper.saveValue() and the create branch of createStoreIfNeeded(), so two nodes cannot both load, mutate and publish the same store concurrently.
• createStoreIfNeeded() re-checks existence after acquiring the lock (double-checked), so two nodes racing at boot cannot both decide to create it.
• A contended or unavailable lock never results in a silently skipped save. Either wait and proceed, or fail loudly — decide and document which, and if ClusterLockManager/ClusterLockManagerImpl is reused, handle wasExecuted() explicitly. Note that class is shared with OSGi restart, so any change to its semantics must not alter that behaviour.
• The lock is on the write path only. Reads must never take it: SecretCachedKeyStoreImpl flushes cluster-wide on every save, so reads are frequent and re-hit the file.
• Decide and document the behaviour when the database is unavailable. Adding a DB-backed lock puts the DB in the secrets write path, which is a new failure mode — it should fail closed with a clear error, not fall through to an unsynchronized write.
• A stale lock from a crashed node cannot block saves indefinitely (ClusterLockManagerImpl defaults to a 600s lockAtMostUntil).
• An integration test demonstrating a lost update on today's code that passes after the fix. Two helper instances = two nodes, each adding a distinct secret from a shared starting state; assert both survive.

Additional Context

Deliberately scoped out of #36724 to keep that PR's blast radius small: it fixes the reported data loss with no new dependency in the write path, whereas this change introduces one and requires touching a class shared with OSGi restart. Discussion of why atomic publish alone is sufficient for corruption but not for lost updates is in PR #37053.

Also worth considering as an alternative to a lock: making the store's structure append-friendly, or narrowing the write to a single entry rather than rewriting the whole keystore, which would remove the read-modify-write entirely. Probably a larger change than a lock, but it addresses the root shape of the problem.

Severity

Medium - Some functionality impacted

Start with SecretsKeyStoreHelper.saveValue() and the create branch of createStoreIfNeeded(), then inspect ClusterLockManager and ClusterLockManagerImpl.tryLock(), especially the wasExecuted() handling and OSGi call sites. Reproduce the two-node lost update with two helper instances, define the lock failure behavior, and add integration coverage showing both secrets survive.