refactor: stop using EntityManager.merge to obtain managed entities

server/AGENTS.md

@@ -0,0 +1,150 @@
+# AGENTS.md
+
+Operating rules for AI coding agents working on the Open VSX registry server —
+the Spring Boot application in `src/main/java`, built with Gradle. Claude Code
+loads this file via `CLAUDE.md`; other agents read it directly.
+
+This is the short, must-follow contract. If any rule here conflicts with an
+explicit request from the user, ask before overriding it.
+
+## Persistence: `EntityManager.merge` needs justifying, every time
+
+`merge` copies a **whole detached entity** over the stored row. Any column that
+changed since that entity was loaded is silently reverted. It is a write, even
+where the surrounding code only means to read.
+
+Never reach for `merge` to obtain a managed entity. Use
+`entityManager.find(Type.class, id)` and set the fields you mean to change —
+the setters are what persist inside a transaction.
+
+Before writing or keeping a `merge`, answer all five:
+
+1. **Is an update actually intended here?** If the method only reads, or only
+   needs the entity managed so a lazy association loads, use `find`.
+2. **Is the entity detached?** An already-managed entity makes `merge` a no-op
+   that returns the same instance — the call is noise.
+3. **How stale is it?** An entity loaded before slow work (VSIX parsing, a
+   remote call) and merged afterwards will revert anything a concurrent writer
+   changed in between, `active` included.
+4. **Does anything depend on instance identity?** `UserData#equals` compares
+   every field, so a `merge` can be load-bearing purely by making two
+   references `==`. Compare ids instead — and reject an unassigned id, or an
+   unpersisted entity makes the check fail open.
+5. **Is the row still there?** `find` returns `null` for a deleted row where
+   `merge` would try to resurrect it. Handle the `null`.
+
+`merge` before `remove` works but emits a pointless UPDATE ahead of the DELETE.
+
+This is issue #989, reported as a concurrent write to an extension being
+reverted on a *read* path, leaving extensions inactive. Treat a new `merge` as
+something to argue for in the pull request description.
+
+## Non-negotiables
+
+- **`./gradlew test` must pass before you commit.** Some tests start
+  Testcontainers (Postgres, Elasticsearch, LocalStack), so a working Docker
+  daemon is required; say so rather than skipping them silently.
+- **`./gradlew spotlessCheck` must pass for the files you touched.** It is
+  deliberately not wired into `build`/`check` (`enforceCheck = false`), and the
+  repository has pre-existing violations elsewhere — so run `spotlessApply` and
+  then **revert files you did not otherwise change**, rather than sweeping
+  unrelated formatting into your commit.
+- **New source files need the EPL-2.0 license header** (copy it from any
+  existing file).
+- **Never commit unless the user asks**, and stage only the files you changed
+  (`git add <path>`), never `git add -A` / `git add .`.
+
+## Project shape
+
+- **Java 25** (`libs.versions.toml`). Dependencies are declared in
+  `gradle/libs.versions.toml`, never inline in `build.gradle`.
+- **Nullability is expressed with JSpecify** (`@Nullable`, `@NonNull` from
+  `org.jspecify.annotations`), not Jakarta or Spring annotations.
+- **`src/main/jooq-gen/` is generated and committed.** Never hand-edit it; it
+  is excluded from Spotless. Regenerate it with `./gradlew jooqCodegen` after a
+  schema change — that task reads a live Postgres, so it needs the dev database
+  running.
+- **Flyway migrations** live in `src/main/resources/db/migration` as
+  `V<n>__Description.sql`. A migration that has shipped is immutable — fix a
+  mistake with a new one. They are excluded from the pre-commit hooks.
+- **Configuration properties** are bound in `*Config` classes with `@Value`,
+  each documented with its property name and default, and validated in a
+  `@PostConstruct`. A property with an invalid value should fail startup rather
+  than misbehave later.
+- The server has **no CHANGELOG** — only `cli/` and `webui/` do. Do not invent
+  one; put the reasoning in the commit message and pull request instead.
+
+## Deployment descriptors travel with the config
+
+A property added, renamed or removed in the server has **four** homes that can
+drift apart:
+
+- `src/dev/resources/application.yml`
+- `../deploy/docker/configuration/application.yml`
+- `../deploy/openshift/application.yml`
+- `../deploy/kubernetes/configmap.yaml` (the same document, indented inside
+  `data:`)
+
+The Kubernetes one was added later than the others and has twice been missed by
+changes that updated the rest, leaving keys that silently bind to nothing. When
+you touch configuration, diff all four and say which you changed.
+
+## Changing code requires tests
+
+- JUnit 5 with Mockito and AssertJ under `src/test/java`, mirroring the source
+  package.
+- For a bug fix, **confirm the test fails without the fix** — a regression test
+  that passes either way is not one.
+- Prefer a focused unit test over booting the whole context. A config class can
+  be exercised with `ApplicationContextRunner`; note that a bare runner has no
+  conversion service, so register one
+  (`ApplicationConversionService.getSharedInstance()`) or `Duration` and
+  collection properties will not bind.
+- A change with no matching test update is incomplete. Say so explicitly rather
+  than skipping silently.
+
+## Workflow and code quality
+
+- Read files in full before wide-ranging changes, and before editing files you
+  have not inspected. Do not rely on search snippets.
+- Keep code comments short (1–3 lines): state only the non-obvious constraint or
+  rationale, never narrate what the code does.
+- Ask before removing functionality or code that appears intentional. Do not
+  preserve backward compatibility unless the user asks for it.
+- A configuration property that has never appeared in a release can be renamed
+  outright; one that has shipped needs a fallback to its old key (see
+  `ovsx.access-token.prefix`). Check the tags before assuming either.
+
+## Conventions
+
+- Commit subjects use a conventional-commit prefix: `feat:`, `fix:`, `chore:`,
+  `docs:`, `test:`, `style:`, `build:`, or `ci:`.
+- No emojis in commits, pull requests, issues, or code. Keep prose concise,
+  direct, and technical — no cheerful filler.
+- Answer a user's question before making edits or running implementation
+  commands.
+- When responding to feedback or an analysis, say whether you agree or disagree
+  before describing what you changed.
+
+## Git
+
+Multiple sessions may be running in this cwd at the same time, each modifying
+different files. Git operations that touch unstaged, staged, or untracked files
+outside your own changes will stomp on other sessions' work. Follow these rules:
+
+Committing:
+
+- Only commit files YOU changed in THIS session.
+- Stage explicit paths (`git add <path1> <path2>`); never `git add -A` / `git add .`.
+- Before committing, run `git status` and verify you are only staging your files.
+
+Never run (destroys other agents' work or bypasses checks):
+
+- `git reset --hard`, `git checkout .`, `git clean -fd`, `git stash`,
+  `git add -A`, `git add .`, `git commit --no-verify`.
+
+If rebase conflicts occur:
+
+- Resolve conflicts only in files you modified.
+- If a conflict is in a file you did not modify, abort and ask the user.
+- Never force push.

server/CLAUDE.md

@@ -0,0 +1 @@
+AGENTS.md

server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java

@@ -255,13 +255,16 @@ public ResultJson deactivateAccessToken(UserData user, long id) {
             throw new NotFoundException();
         }
 
-        user = entityManager.merge(user);
-        if (!token.getUser().equals(user)) {
+        // Compare ids, not entities: UserData#equals compares every field - tokens and memberships
+        // included - so comparing entities rejects a caller whose user differs from the stored row in
+        // any way. Rejecting id 0 stops an entity that was never persisted from failing the check open.
+        var tokenUser = token.getUser();
+        if (tokenUser == null || tokenUser.getId() == 0 || tokenUser.getId() != user.getId()) {
             throw new NotFoundException();
         }
 
         token.setActive(false);
-        return ResultJson.success("Deactivated access token for user " + user.getLoginName() + ".");
+        return ResultJson.success("Deactivated access token for user " + tokenUser.getLoginName() + ".");
     }
 
     // REQUIRES_NEW: callers such as LocalRegistryService#createNamespace(NamespaceJson, String) wrap

server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java

@@ -80,7 +80,9 @@ public void changeNamespaceInDatabase(
 
     private void changeExtensionNamespace(Streamable<Extension> extensions, Namespace newNamespace) {
         for (var extension : extensions) {
-            extension = entityManager.merge(extension);
+            // findExtensions ran inside changeNamespaceInDatabase's transaction, so these are
+            // already managed and merge just handed back the same instance; the setter is what
+            // persists the change.
             extension.setNamespace(newNamespace);
         }
     }

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

@@ -12,7 +12,6 @@
 import java.util.LinkedHashSet;
 import java.util.Optional;
 
-import jakarta.persistence.EntityManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Value;
@@ -35,7 +34,6 @@ public class PublisherComplianceChecker {
     protected final Logger logger = LoggerFactory.getLogger(PublisherComplianceChecker.class);
 
     private final TransactionTemplate transactions;
-    private final EntityManager entityManager;
     private final RepositoryService repositories;
     private final ExtensionService extensions;
     private final EclipseService eclipseService;
@@ -45,13 +43,11 @@ public class PublisherComplianceChecker {
 
     public PublisherComplianceChecker(
             TransactionTemplate transactions,
-            EntityManager entityManager,
             RepositoryService repositories,
             ExtensionService extensions,
             EclipseService eclipseService
     ) {
         this.transactions = transactions;
-        this.entityManager = entityManager;
         this.repositories = repositories;
         this.extensions = extensions;
         this.eclipseService = eclipseService;
@@ -102,7 +98,6 @@ private void deactivateExtensions(UserData user) {
             // the version stops being publicly visible here, which the changes feed reports at
             // this instant rather than at the one it was published at
             repositories.recordExtensionVersionChange(version, ExtensionVersionState.INACTIVE, now);
-            entityManager.merge(version);
             var extension = version.getExtension();
             affectedExtensions.add(extension);
             logger.atInfo()
@@ -114,8 +109,10 @@ private void deactivateExtensions(UserData user) {
 
         // Update affected extensions
         for (var extension : affectedExtensions) {
+            // findVersionsByUser runs inside checkPublishers' TransactionTemplate, so these
+            // entities (and the versions above) are already managed and merge only handed back the
+            // same instances; the setters are what persist.
             extensions.updateExtension(extension);
-            entityManager.merge(extension);
         }
     }
 }

server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java

@@ -86,7 +86,17 @@ public MigrationService(
      */
     @Transactional
     public void scheduleMigration(MigrationItem item, Instant scheduledAt) {
-        item = entityManager.merge(item);
+        // find rather than merge: only migrationScheduled below is this method's to write, and
+        // merging the whole detached item reverted anything else that had moved - see #989.
+        var itemId = item.getId();
+        item = entityManager.find(MigrationItem.class, itemId);
+        if (item == null) {
+            // The row can be deleted between the caller reading its batch and this transaction
+            // (MigrationItemCleanupFilter, or one of the Delete_MigrationItems migrations). Skip it
+            // rather than dereferencing null, which would abort the rest of the batch.
+            logger.debug("Migration item {} is gone, nothing to schedule", itemId);
+            return;
+        }
         var jobIdText = item.getJobName() + "->itemId=" + item.getId();
         var jobId = uuidService.generateFromName(jobIdText);
         var handler = JOB_HANDLERS.get(item.getJobName());

server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java

@@ -26,7 +26,15 @@ public RenameDownloadsService(EntityManager entityManager) {
 
     @Transactional
     public FileResource cloneResource(FileResource resource, String name) {
-        resource = entityManager.merge(resource);
+        // Nothing on `resource` is updated here; find still returns a managed entity, so the lazy
+        // getExtension() below resolves just the same - see #989.
+        var resourceId = resource.getId();
+        resource = entityManager.find(FileResource.class, resourceId);
+        if (resource == null) {
+            // Deleted since the job loaded it. The caller uses the clone straight away, so there is
+            // nothing to skip - fail with the id rather than an NPE on the next line.
+            throw new IllegalStateException("Cannot clone file resource " + resourceId + ": it no longer exists");
+        }
         var clone = new FileResource();
         clone.setName(name);
         clone.setStorageType(resource.getStorageType());

server/src/main/java/org/eclipse/openvsx/storage/log/DownloadCountProcessor.java

@@ -111,9 +111,16 @@ public List<Extension> increaseDownloadCounts(Map<Long, Integer> extensionDownlo
     @Transactional // needs transaction for lazy-loading versions
     public void evictCaches(Extension extension) {
         Observation.createNotStarted("DownloadCountProcessor#evictCaches", observations).observe(() -> {
-            var mergedExtension = entityManager.merge(extension);
-            cache.evictExtensionJsons(mergedExtension);
-            cache.evictLatestExtensionVersion(mergedExtension);
+            // find, not merge: this only needs a managed entity so the lazy version collections
+            // can be loaded for eviction, and nothing here updates the extension. merge wrote the
+            // whole (already mutated, possibly stale) row back, which is how a concurrent update
+            // could be silently reverted - the failure mode behind #989.
+            var managedExtension = entityManager.find(Extension.class, extension.getId());
+            if (managedExtension == null) {
+                return;
+            }
+            cache.evictExtensionJsons(managedExtension);
+            cache.evictLatestExtensionVersion(managedExtension);
         });
     }
 

server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java

@@ -168,7 +168,15 @@ public TrustedPublisher registerTrustedPublisher(
         publisher.setProvider(provider.getProviderId());
         publisher.setRegistration(registration);
         publisher.setClaims(claims);
-        publisher.setCreatedBy(entityManager.merge(user));
+        // find, not merge: this needs a managed UserData to point the new row at, and merging the
+        // caller's (detached) user wrote their whole row back as a side effect - see #989.
+        // trusted_publisher.created_by is NOT NULL, so a user that has gone away since the request
+        // was authenticated has to fail here rather than as a constraint violation on persist.
+        var createdBy = entityManager.find(UserData.class, user.getId());
+        if (createdBy == null) {
+            throw new ErrorResultException("Cannot register a trusted publisher for an unknown user.");
+        }
+        publisher.setCreatedBy(createdBy);
         publisher.setCreatedTimestamp(TimeUtil.getCurrentUTC());
         entityManager.persist(publisher);
         return publisher;

server/src/test/java/org/eclipse/openvsx/UserAPITest.java

@@ -216,8 +216,6 @@ void testDeleteAccessToken() throws Exception {
         token.setType(PersonalAccessTokenType.LLT);
         Mockito.when(repositories.findPersonalAccessToken(100))
                 .thenReturn(token);
-        Mockito.when(entityManager.merge(userData))
-                .thenReturn(userData);
 
         mockMvc.perform(
                 post("/user/token/delete/{id}", 100)
@@ -258,6 +256,7 @@ void testDeleteAccessTokenInactive() throws Exception {
     void testDeleteAccessTokenWrongUser() throws Exception {
         mockUserData();
         var userData = new UserData();
+        userData.setId(2);
         userData.setLoginName("wrong_user");
         var token = new PersonalAccessToken();
         token.setId(100);
@@ -923,6 +922,7 @@ void testDeleteDependingExtension() throws Exception {
 
     private UserData mockUserData() {
         var userData = new UserData();
+        userData.setId(1);
         userData.setLoginName("test_user");
         userData.setFullName("Test User");
         userData.setProviderUrl("http://example.com/test");