refactor(server): remove transactional comment debt

Author: FelixTJDietrich

.changeset/spring-contracts-hold.md

@@ -2,4 +2,4 @@
 "hephaestus": patch
 ---
 
-Rejects mentor chat requests without a message and integration connection requests without a provider kind before processing them.
+Rejects mentor chat requests missing a message and integration connection requests missing a provider kind.

server/openapi.yaml

@@ -9769,7 +9769,6 @@ components:
          <p><code>userInput</code> is intentionally a free-form map so per-kind ConnectionStrategy
          implementations can dictate their own field schema (e.g. GitLab needs <code>pat</code> +
          <code>group_id</code>; GitHub needs nothing because the install URL is server-configured).
-         Provider-specific input validation is the strategy's responsibility.
       properties:
         kind:
           type: string

server/src/main/java/de/tum/cit/aet/hephaestus/agent/mentor/chat/MentorInFlightAccounting.java

@@ -35,12 +35,14 @@ public MentorInFlightAccounting(ChatMessageRepository chatMessageRepository, Llm
 
     @Transactional(propagation = Propagation.REQUIRES_NEW)
     public boolean account(UUID messageId) {
+        // Re-read here so a turn completed after the sweep query is not billed as abandoned.
         ChatMessage message = chatMessageRepository.findById(messageId).orElse(null);
         if (message == null || message.getStatus() != ChatMessage.Status.in_flight) return false;
         JsonNode existingMetadata = message.getMetadata();
         LlmPriceSnapshot price = MentorAdmissionMetadata.readPrice(existingMetadata);
         message.setStatus(ChatMessage.Status.interrupted);
         message.setMetadata(withAbandonedError(existingMetadata));
+        // End the in-flight state before reading usage so the proxy accumulator can no longer change the totals.
         chatMessageRepository.saveAndFlush(message);
         MentorTurnLlmUsage observed = chatMessageRepository
             .findLlmUsageById(message.getId())

server/src/main/java/de/tum/cit/aet/hephaestus/agent/mentor/chat/MentorInFlightReaper.java

@@ -18,19 +18,10 @@
 import org.springframework.stereotype.Component;
 
 /**
- * Interrupts and accounts mentor turns abandoned by a crashed process. Nobody is left to report what
- * such a turn spent — no runner report, no in-process meter — so it is billed from the per-call totals
- * the LLM proxy wrote to its row, and only a turn with no recorded call at all is booked UNVERIFIABLE.
+ * Interrupts and accounts mentor turns abandoned by a crashed process.
  *
- * <p>This sweep writes money, so it is deliberately per-turn rather than per-batch: the accounting collaborator
- * owns one turn in its own {@code REQUIRES_NEW} transaction. A batch rollback would also undo turns
- * already billed in it and leave their {@code in_flight} rows in place, and the partial unique index
- * would then refuse every further turn on those threads.
- *
- * <p>What prevents a double charge is the ledger's {@code (MENTOR_TURN, messageId, 0)} unique
- * constraint behind {@code ON CONFLICT … DO NOTHING}, not any in-process check. The per-turn re-read
- * decides which of the two writes is the CORRECT one: without it, this sweep could bill a turn that has
- * since finished and permanently shadow the normal path's real amount.
+ * <p>Each turn is accounted independently so one failure cannot roll back previously accounted turns
+ * or leave their in-flight rows blocking the thread.
  */
 @ConditionalOnServerRole
 @Component

server/src/main/java/de/tum/cit/aet/hephaestus/integration/core/connection/ConnectionRepository.java

@@ -50,6 +50,10 @@ Optional<Connection> findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
         IntegrationState state
     );
 
+    default Optional<Connection> findActive(long workspaceId, IntegrationKind kind) {
+        return findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(workspaceId, kind, IntegrationState.ACTIVE);
+    }
+
     List<Connection> findByWorkspaceIdAndState(long workspaceId, IntegrationState state);
 
     /**

server/src/main/java/de/tum/cit/aet/hephaestus/integration/core/connection/ConnectionService.java

@@ -39,10 +39,6 @@ public class ConnectionService {
     private final ApplicationEventPublisher eventPublisher;
     private final SyncJobService syncJobService;
 
-    /**
-     * Runs the pre-transition revoke/erase callback in its own {@code REQUIRES_NEW} transaction so a
-     * failing erase cannot mark the lifecycle transaction rollback-only — see {@link #runRevokeIsolated}.
-     */
     private final TransactionTemplate revokeTransactionTemplate;
 
     public ConnectionService(
@@ -64,11 +60,7 @@ public ConnectionService(
 
     @Transactional(readOnly = true)
     public Optional<Connection> findActive(long workspaceId, IntegrationKind kind) {
-        return connectionRepository.findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
-            workspaceId,
-            kind,
-            IntegrationState.ACTIVE
-        );
+        return connectionRepository.findActive(workspaceId, kind);
     }
 
     /**
@@ -86,20 +78,8 @@ public List<Long> findWorkspaceIdsWithActiveConnection(IntegrationKind kind) {
      */
     @Transactional(readOnly = true)
     public Optional<IntegrationKind> findActiveProviderKind(long workspaceId) {
-        boolean github = connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
-                workspaceId,
-                IntegrationKind.GITHUB,
-                IntegrationState.ACTIVE
-            )
-            .isPresent();
-        boolean gitlab = connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
-                workspaceId,
-                IntegrationKind.GITLAB,
-                IntegrationState.ACTIVE
-            )
-            .isPresent();
+        boolean github = connectionRepository.findActive(workspaceId, IntegrationKind.GITHUB).isPresent();
+        boolean gitlab = connectionRepository.findActive(workspaceId, IntegrationKind.GITLAB).isPresent();
         if (github && gitlab) {
             throw new IllegalStateException(
                 "Workspace " +
@@ -116,11 +96,7 @@ public Optional<IntegrationKind> findActiveProviderKind(long workspaceId) {
     @Transactional(readOnly = true)
     public Optional<ConnectionConfig.GitHubAppConfig> findActiveGitHubAppConfig(long workspaceId) {
         return connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
-                workspaceId,
-                IntegrationKind.GITHUB,
-                IntegrationState.ACTIVE
-            )
+            .findActive(workspaceId, IntegrationKind.GITHUB)
             .map(Connection::getConfig)
             .filter(c -> c instanceof ConnectionConfig.GitHubAppConfig)
             .map(c -> (ConnectionConfig.GitHubAppConfig) c);
@@ -129,11 +105,7 @@ public Optional<ConnectionConfig.GitHubAppConfig> findActiveGitHubAppConfig(long
     @Transactional(readOnly = true)
     public Optional<ConnectionConfig.GitHubPatConfig> findActiveGitHubPatConfig(long workspaceId) {
         return connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
-                workspaceId,
-                IntegrationKind.GITHUB,
-                IntegrationState.ACTIVE
-            )
+            .findActive(workspaceId, IntegrationKind.GITHUB)
             .map(Connection::getConfig)
             .filter(c -> c instanceof ConnectionConfig.GitHubPatConfig)
             .map(c -> (ConnectionConfig.GitHubPatConfig) c);
@@ -142,11 +114,7 @@ public Optional<ConnectionConfig.GitHubPatConfig> findActiveGitHubPatConfig(long
     @Transactional(readOnly = true)
     public Optional<ConnectionConfig.GitLabConfig> findActiveGitLabConfig(long workspaceId) {
         return connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
-                workspaceId,
-                IntegrationKind.GITLAB,
-                IntegrationState.ACTIVE
-            )
+            .findActive(workspaceId, IntegrationKind.GITLAB)
             .map(Connection::getConfig)
             .filter(c -> c instanceof ConnectionConfig.GitLabConfig)
             .map(c -> (ConnectionConfig.GitLabConfig) c);
@@ -155,11 +123,7 @@ public Optional<ConnectionConfig.GitLabConfig> findActiveGitLabConfig(long works
     @Transactional(readOnly = true)
     public Optional<ConnectionConfig.SlackConfig> findSlackNotificationConfig(long workspaceId) {
         return connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
-                workspaceId,
-                IntegrationKind.SLACK,
-                IntegrationState.ACTIVE
-            )
+            .findActive(workspaceId, IntegrationKind.SLACK)
             .map(Connection::getConfig)
             .filter(c -> c instanceof ConnectionConfig.SlackConfig)
             .map(c -> (ConnectionConfig.SlackConfig) c);
@@ -168,11 +132,7 @@ public Optional<ConnectionConfig.SlackConfig> findSlackNotificationConfig(long w
     @Transactional(readOnly = true)
     public Optional<ConnectionConfig.OutlineConfig> findActiveOutlineConfig(long workspaceId) {
         return connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
-                workspaceId,
-                IntegrationKind.OUTLINE,
-                IntegrationState.ACTIVE
-            )
+            .findActive(workspaceId, IntegrationKind.OUTLINE)
             .map(Connection::getConfig)
             .filter(c -> c instanceof ConnectionConfig.OutlineConfig)
             .map(c -> (ConnectionConfig.OutlineConfig) c);
@@ -236,7 +196,7 @@ public record OutlineSubscription(long workspaceId, String serverUrl, String sig
     @Transactional(readOnly = true)
     public Optional<BearerToken> findActiveBearerToken(long workspaceId, IntegrationKind kind) {
         return connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(workspaceId, kind, IntegrationState.ACTIVE)
+            .findActive(workspaceId, kind)
             .flatMap(c -> c.credentials(credentialConverter))
             .flatMap(b -> b instanceof BearerToken bt ? Optional.of(bt) : Optional.empty());
     }
@@ -266,11 +226,7 @@ public Optional<Connection> findReferenced(IntegrationRef ref) {
                 ref.instanceKey()
             );
         }
-        return connectionRepository.findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(
-            ref.workspaceId(),
-            ref.kind(),
-            IntegrationState.ACTIVE
-        );
+        return connectionRepository.findActive(ref.workspaceId(), ref.kind());
     }
 
     @Transactional(readOnly = true)
@@ -310,7 +266,7 @@ public Optional<Connection> updateConfig(
         UnaryOperator<ConnectionConfig> mutator
     ) {
         return connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(workspaceId, kind, IntegrationState.ACTIVE)
+            .findActive(workspaceId, kind)
             .map(c -> {
                 ConnectionConfig next = mutator.apply(c.getConfig());
                 if (next == null) {
@@ -340,7 +296,7 @@ public Optional<Connection> updateConfig(
     @Transactional
     public Optional<Connection> rotateBearerToken(long workspaceId, IntegrationKind kind, BearerToken bundle) {
         return connectionRepository
-            .findFirstByWorkspaceIdAndKindAndStateOrderByCreatedAtDesc(workspaceId, kind, IntegrationState.ACTIVE)
+            .findActive(workspaceId, kind)
             .map(c -> {
                 c.setCredentials(bundle, credentialConverter);
                 return connectionRepository.save(c);
@@ -656,38 +612,8 @@ private Connection applyTransition(
     }
 
     /**
-     * Runs the vendor revoke / provider-data erase in its own {@code REQUIRES_NEW} transaction and
-     * absorbs its failure, so "best effort, proceed locally" is actually reachable.
-     *
-     * <p><b>Why a nested transaction and not a plain try/catch.</b> The erasers
-     * ({@code ScmWorkspaceContentEraser}, {@code SlackWorkspaceContentEraser}, the Outline
-     * {@code deleteByWorkspaceId} sweep) are {@code @Transactional} with default {@code REQUIRED}
-     * propagation, so joining the lifecycle transaction would let a {@code DataAccessException} from any
-     * of them — an FK surprise, a statement timeout on a large mirror delete — mark the shared
-     * transaction rollback-only, and the commit would then fail with {@code UnexpectedRollbackException}
-     * even though the transition and audit row were written. Running the callback on its own transaction
-     * confines that poisoning to the callback; catching OUTSIDE the template also absorbs the
-     * {@code UnexpectedRollbackException} raised at the nested commit when a callback swallowed the
-     * failure internally.
-     *
-     * <p><b>Why this cannot deadlock against our own row lock.</b> We hold {@code SELECT … FOR UPDATE}
-     * on the {@code connection} row on the outer connection, and the nested transaction runs on a
-     * second pooled connection — so it must never wait on that row. It does not: no revoke path writes
-     * the {@code connection} row (both {@code OutlineWebhookRegistrar#deregister} and
-     * {@code GitLabWebhookService#deregisterActiveWebhook} refuse to rewrite config there), and the FK
-     * children they DO write are only ever deleted — {@code outline_document},
-     * {@code outline_collection} and {@code outline_document_event} carry a {@code connection_id}, but
-     * PostgreSQL runs no parent-side referential check on a child DELETE, so no {@code FOR KEY SHARE} is
-     * taken on the locked row. Plain reads of {@code connection} from the nested transaction (e.g.
-     * {@code findActiveBearerToken}) are MVCC snapshots and never block on {@code FOR UPDATE}. Adding a
-     * write of {@code connection} — or an INSERT into any table keyed on it — to a revoke path would
-     * break this and self-deadlock.
-     *
-     * <p><b>Accepted consequence.</b> Erase and transition are not atomic. A revoke that erases
-     * successfully still commits even if the transition afterwards fails (duplicate-correlation
-     * short-circuit, illegal transition), leaving erased data behind an ACTIVE connection. Every erase
-     * path is idempotent and the next disconnect re-runs it, so this is recoverable — and it is the
-     * direction to fail in, since the reverse strands the admin with a 500 and an ACTIVE connection.
+     * Isolates vendor revocation from the local lifecycle transition. A vendor failure rolls back its
+     * database work while the local transition proceeds, so the operation is intentionally non-atomic.
      */
     private void runRevokeIsolated(Connection connection, Runnable revoke) {
         try {

server/src/main/java/de/tum/cit/aet/hephaestus/integration/core/connection/api/InitiateConnectionRequestDTO.java

@@ -10,6 +10,5 @@
  * <p>{@code userInput} is intentionally a free-form map so per-kind ConnectionStrategy
  * implementations can dictate their own field schema (e.g. GitLab needs {@code pat} +
  * {@code group_id}; GitHub needs nothing because the install URL is server-configured).
- * Provider-specific input validation is the strategy's responsibility.
  */
 public record InitiateConnectionRequestDTO(@NotNull IntegrationKind kind, Map<String, String> userInput) {}

server/src/main/java/de/tum/cit/aet/hephaestus/integration/outline/lifecycle/OutlineWebhookRegistrar.java

@@ -256,7 +256,7 @@ public void deregister(long workspaceId) {
     public void deregister(long workspaceId, long connectionId) {
         boolean hadSubscription;
         try {
-            hadSubscription = deregisterStrictWithoutTransaction(workspaceId, connectionId);
+            hadSubscription = deregisterStrictInternal(workspaceId, connectionId);
         } catch (RuntimeException e) {
             log.warn("outline.webhook: deregistration failed for connectionId={}: {}", connectionId, e.toString());
             hadSubscription = true;
@@ -268,10 +268,10 @@ public void deregister(long workspaceId, long connectionId) {
 
     @Transactional(propagation = Propagation.NOT_SUPPORTED)
     public boolean deregisterStrict(long workspaceId, long connectionId) {
-        return deregisterStrictWithoutTransaction(workspaceId, connectionId);
+        return deregisterStrictInternal(workspaceId, connectionId);
     }
 
-    private boolean deregisterStrictWithoutTransaction(long workspaceId, long connectionId) {
+    private boolean deregisterStrictInternal(long workspaceId, long connectionId) {
         Optional<Connection> connection = connectionService.findInWorkspace(workspaceId, connectionId);
         if (connection.isEmpty() || !(connection.get().getConfig() instanceof ConnectionConfig.OutlineConfig config)) {
             return false;

server/src/main/java/de/tum/cit/aet/hephaestus/integration/scm/github/pullrequestreview/GitHubPullRequestReviewSyncService.java

@@ -60,9 +60,8 @@
  * Uses typed GraphQL models for type-safe deserialization and delegates
  * persistence to GitHubPullRequestReviewProcessor.
  * <p>
- * GraphQL fetching is non-transactional; persistence is done per-page in
- * {@code REQUIRES_NEW} transactions via self-proxy to isolate deadlock
- * failures and avoid poisoned-transaction retries.
+ * Reviews are fetched outside a transaction and persisted one page at a time so deadlock retries
+ * start clean.
  */
 @Service
 public class GitHubPullRequestReviewSyncService {
@@ -417,11 +416,7 @@ public int syncRemainingReviews(Long scopeId, PullRequest pullRequest, String st
         return totalSynced;
     }
 
-    /**
-     * Persists a page of reviews with transient failure retry. Each attempt runs in a fresh
-     * {@code REQUIRES_NEW} transaction via self-proxy, so a deadlock on one attempt
-     * does not poison subsequent retries.
-     */
+    /** Persists a page of reviews in a new transaction and retries transient failures. */
     private int persistReviewPageWithRetry(
         List<GHPullRequestReview> reviews,
         Long pullRequestId,
@@ -479,19 +474,6 @@ private int persistReviewPageWithRetry(
         return 0;
     }
 
-    /**
-     * Processes a page of review nodes in a {@code REQUIRES_NEW} transaction.
-     * <p>
-     * Called via self-proxy to ensure the transaction annotation is honoured.
-     * If a deadlock occurs, the transaction is rolled back independently without
-     * poisoning any outer transaction.
-     *
-     * @param reviews       the review nodes from the GraphQL response
-     * @param pullRequestId the database ID of the owning pull request
-     * @param scopeId       the scope ID for authentication
-     * @param repository    the repository entity for creating the processing context
-     * @return number of reviews persisted
-     */
     private int processReviewPage(
         List<GHPullRequestReview> reviews,
         Long pullRequestId,

server/src/main/java/de/tum/cit/aet/hephaestus/integration/scm/gitlab/issue/GitLabIssueProcessor.java

@@ -174,20 +174,15 @@ public boolean purgeConfidential(GitLabIssueEventDTO event, ProcessingContext co
     }
 
     /**
-     * Handles an {@code action=update} issue event. Persists the issue via {@link #process} (which
-     * overwrites the label set to the new state), then emits one {@link ScmDomainEvent.IssueLabeled}
+     * Handles an {@code action=update} issue event. Persists the issue, then emits one
+     * {@link ScmDomainEvent.IssueLabeled}
      * per newly-added label — GitLab has no native "labeled" action, so this is how the IssueLabeled
      * trigger reaches parity with GitHub. The added-label delta is read from the webhook's
      * {@code changes.labels} diff, so a plain title/description edit emits nothing.
-     *
-     * <p>{@code @Transactional} so the whole update runs in one transaction: the self-invoked
-     * {@link #process} bypasses its own proxy (Spring AOP self-invocation), so without this its writes
-     * would run with no active transaction.
      */
     @Transactional
     @Nullable
     public Issue processUpdated(GitLabIssueEventDTO event, ProcessingContext context) {
-        // Capture the delta from the payload (independent of the entity's label set, which process() rewrites).
         List<GitLabWebhookLabel> addedLabels = event.addedLabels();
         Issue issue = processInternal(event, context);
         if (issue == null || addedLabels.isEmpty()) {