fix: serialize deleteExtension and publish with a pessimistic lock (#1919)

* test: cover deleteExtension racing a concurrent publish

Adds tests for an extension delete-all racing a publish of a new version: the concurrently published version must not be silently deleted/orphaned, a delete must fail fast (and ask to retry) while a publish holds the extension lock, and deleting your own version while another publisher has one must leave the extension and that version intact.

Assisted-By: anthropic:claude-opus-4-8[1m]

* fix: serialize deleteExtension and publish with a pessimistic lock

Publishing now takes a waiting FOR UPDATE lock on the extension before adding a version, and deleteExtension takes the same lock with NOWAIT. If a publish holds it, the delete fails fast with a 409 ErrorResultException asking the user to retry, instead of removing the aggregate root (and silently deleting the just-published version) out from under the in-flight publish. On retry the new version exists, so it is no longer a delete-all.

* docs: updating comment

* update wording

---------

Co-authored-by: Thomas Neidhart <thomas.neidhart@eclipse-foundation.org>

Author: gnugomez

server/src/main/java/org/eclipse/openvsx/ExtensionService.java

@@ -28,6 +28,7 @@
 import org.eclipse.openvsx.search.SearchUtilService;
 import org.eclipse.openvsx.util.*;
 import org.jobrunr.scheduling.JobRequestScheduler;
+import org.springframework.dao.PessimisticLockingFailureException;
 import org.springframework.http.HttpStatus;
 import org.springframework.stereotype.Service;
 
@@ -222,9 +223,26 @@ public ResultJson deleteExtension(
             List<TargetPlatformVersionJson> targetVersions,
             UserData user
     ) throws ErrorResultException {
+        // Lock the extension row (NOWAIT) so a delete-all and a concurrent publish can't interleave.
+        // Publishing takes the same lock (waiting); if the lock can not be acquired, this fails fast,
+        // and we ask the user to retry.
+        Extension extension;
+        try {
+            extension = repositories.findExtensionForUpdateNoWait(extensionName, namespaceName);
+        } catch (PessimisticLockingFailureException e) {
+            throw new ErrorResultException(
+                    "Extension " + NamingUtil.toExtensionId(namespaceName, extensionName)
+                            + " can not be locked due to concurrent modification. Please try again.",
+                    HttpStatus.CONFLICT);
+        }
+
+        if (extension == null) {
+            var message = "Extension not found: " + NamingUtil.toExtensionId(namespaceName, extensionName);
+            throw new ErrorResultException(message, HttpStatus.NOT_FOUND);
+        }
+
         var results = new ArrayList<ResultJson>();
-        if(repositories.isDeleteAllVersions(namespaceName, extensionName, targetVersions, user)) {
-            var extension = repositories.findExtension(extensionName, namespaceName);
+        if (repositories.isDeleteAllVersions(namespaceName, extensionName, targetVersions, user)) {
             results.add(deleteExtension(user, extension));
         } else {
             for (var targetVersion : targetVersions) {
@@ -270,7 +288,7 @@ protected ResultJson deleteExtension(UserData user, Extension extension) throws
         }
 
         var deprecatedExtensions = repositories.findDeprecatedExtensions(extension);
-        for(var deprecatedExtension : deprecatedExtensions) {
+        for (var deprecatedExtension : deprecatedExtensions) {
             deprecatedExtension.setReplacement(null);
             cache.evictExtensionJsons(deprecatedExtension);
         }

server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java

@@ -157,7 +157,9 @@ private ExtensionVersion createExtensionVersion(ExtensionProcessor processor, Us
         extVersion.setPublishedWith(token);
         extVersion.setActive(false);
 
-        var extension = repositories.findExtension(extensionName, namespace);
+        // Lock the extension row while adding a version so a concurrent delete-all serializes
+        // against this publish (and fails fast with a retry instead of removing it under us).
+        var extension = repositories.findExtensionForUpdate(extensionName, namespace.getName());
         if (extension == null) {
             extension = new Extension();
             extension.setActive(false);

server/src/main/java/org/eclipse/openvsx/repositories/ExtensionRepository.java

@@ -14,6 +14,7 @@
 import org.eclipse.openvsx.entities.UserData;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.Repository;
+import org.springframework.data.repository.query.Param;
 import org.springframework.data.util.Streamable;
 
 import java.util.Collection;
@@ -29,6 +30,26 @@ public interface ExtensionRepository extends Repository<Extension, Long> {
 
     Extension findByNameIgnoreCaseAndNamespaceNameIgnoreCase(String name, String namespace);
 
+    // Publish takes this lock (waiting) before adding a version. We use a native FOR UPDATE rather
+    // than @Lock(PESSIMISTIC_WRITE) on purpose: on Hibernate 6 + PostgreSQL that maps to the weaker
+    // FOR NO KEY UPDATE, which does NOT conflict with the FOR KEY SHARE a version insert takes, so a
+    // concurrent publish could still insert a row. FOR UPDATE conflicts with it and blocks the insert.
+    // "OF e" scopes the lock to the extension row only (not the joined namespace) — also not
+    // expressible via @Lock.
+    @Query(value = "select e.* from extension e join namespace n on n.id = e.namespace_id"
+            + " where lower(e.name) = lower(:name) and lower(n.name) = lower(:namespace) for update of e",
+            nativeQuery = true)
+    Extension findByNameIgnoreCaseAndNamespaceNameIgnoreCaseForUpdate(@Param("name") String name, @Param("namespace") String namespace);
+
+    // Delete takes this NOWAIT variant so it fails fast (instead of blocking) when a publish holds
+    // the lock, letting us surface a "retry" error rather than removing the extension under a publish.
+    // Native for the same reason as above (FOR UPDATE strength + OF e); NOWAIT could also be done via
+    // a @QueryHint lock-timeout of 0, but we keep both clauses in one explicit native query.
+    @Query(value = "select e.* from extension e join namespace n on n.id = e.namespace_id"
+            + " where lower(e.name) = lower(:name) and lower(n.name) = lower(:namespace) for update of e nowait",
+            nativeQuery = true)
+    Extension findByNameIgnoreCaseAndNamespaceNameIgnoreCaseForUpdateNoWait(@Param("name") String name, @Param("namespace") String namespace);
+
     Streamable<Extension> findByActiveTrue();
 
     Streamable<Extension> findByIdIn(Collection<Long> extensionIds);

server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java

@@ -208,6 +208,15 @@ public Extension findExtension(String name, String namespace) {
         return extensionRepo.findByNameIgnoreCaseAndNamespaceNameIgnoreCase(name, namespace);
     }
 
+    public Extension findExtensionForUpdate(String name, String namespace) {
+        return extensionRepo.findByNameIgnoreCaseAndNamespaceNameIgnoreCaseForUpdate(name, namespace);
+    }
+
+    // Like findExtensionForUpdate but fails fast (NOWAIT) if the row is already locked by a publish.
+    public Extension findExtensionForUpdateNoWait(String name, String namespace) {
+        return extensionRepo.findByNameIgnoreCaseAndNamespaceNameIgnoreCaseForUpdateNoWait(name, namespace);
+    }
+
     public Streamable<Extension> findActiveExtensions(Namespace namespace) {
         return extensionRepo.findByNamespaceAndActiveTrueOrderByNameAsc(namespace);
     }