bug: make deposit/withdraw/transfer transactional with row locks (sfind-baf6d2f9)

src/main/java/com/example/bankapp/controller/BankController.java

@@ -64,7 +64,7 @@ public String withdraw(@RequestParam BigDecimal amount, Model model) {
             accountService.withdraw(account, amount);
         } catch (RuntimeException e) {
             model.addAttribute("error", e.getMessage());
-            model.addAttribute("account", account);
+            model.addAttribute("account", accountService.findAccountByUsername(username));
             return "dashboard";
         }
 
@@ -88,7 +88,7 @@ public String transferAmount(@RequestParam String toUsername, @RequestParam BigD
             accountService.transferAmount(fromAccount, toUsername, amount);
         } catch (RuntimeException e) {
             model.addAttribute("error", e.getMessage());
-            model.addAttribute("account", fromAccount);
+            model.addAttribute("account", accountService.findAccountByUsername(username));
             return "dashboard";
         }
 

src/main/java/com/example/bankapp/service/AccountService.java

@@ -4,6 +4,9 @@
 import com.example.bankapp.model.Transaction;
 import com.example.bankapp.repository.AccountRepository;
 import com.example.bankapp.repository.TransactionRepository;
+import jakarta.persistence.EntityManager;
+import jakarta.persistence.LockModeType;
+import jakarta.persistence.PersistenceContext;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.security.core.GrantedAuthority;
 import org.springframework.security.core.authority.SimpleGrantedAuthority;
@@ -12,6 +15,7 @@
 import org.springframework.security.core.userdetails.UsernameNotFoundException;
 import org.springframework.security.crypto.password.PasswordEncoder;
 import org.springframework.stereotype.Service;
+import org.springframework.transaction.annotation.Transactional;
 
 import java.math.BigDecimal;
 import java.time.LocalDateTime;
@@ -31,6 +35,9 @@ public class AccountService implements UserDetailsService {
     @Autowired
     private TransactionRepository transactionRepository;
 
+    @PersistenceContext
+    private EntityManager entityManager;
+
     public Account findAccountByUsername(String username) {
         return accountRepository.findByUsername(username).orElseThrow(() -> new RuntimeException("Account not found"));
     }
@@ -48,33 +55,25 @@ public Account registerAccount(String username, String password) {
     }
 
 
+    @Transactional
     public void deposit(Account account, BigDecimal amount) {
-        account.setBalance(account.getBalance().add(amount));
-        accountRepository.save(account);
-
-        Transaction transaction = new Transaction(
-                amount,
-                "Deposit",
-                LocalDateTime.now(),
-                account
-        );
-        transactionRepository.save(transaction);
+        Account locked = lockAccount(account.getId());
+        locked.setBalance(locked.getBalance().add(amount));
+
+        transactionRepository.save(new Transaction(amount, "Deposit", LocalDateTime.now(), locked));
+        account.setBalance(locked.getBalance());
     }
 
+    @Transactional
     public void withdraw(Account account, BigDecimal amount) {
-        if (account.getBalance().compareTo(amount) < 0) {
+        Account locked = lockAccount(account.getId());
+        if (locked.getBalance().compareTo(amount) < 0) {
             throw new RuntimeException("Insufficient funds");
         }
-        account.setBalance(account.getBalance().subtract(amount));
-        accountRepository.save(account);
-
-        Transaction transaction = new Transaction(
-                amount,
-                "Withdrawal",
-                LocalDateTime.now(),
-                account
-        );
-        transactionRepository.save(transaction);
+        locked.setBalance(locked.getBalance().subtract(amount));
+
+        transactionRepository.save(new Transaction(amount, "Withdrawal", LocalDateTime.now(), locked));
+        account.setBalance(locked.getBalance());
     }
 
     public List<Transaction> getTransactionHistory(Account account) {
@@ -100,38 +99,42 @@ public Collection<? extends GrantedAuthority> authorities() {
         return Arrays.asList(new SimpleGrantedAuthority("USER"));
     }
 
+    @Transactional
     public void transferAmount(Account fromAccount, String toUsername, BigDecimal amount) {
-        if (fromAccount.getBalance().compareTo(amount) < 0) {
+        Long toId = accountRepository.findByUsername(toUsername)
+                .orElseThrow(() -> new RuntimeException("Recipient account not found"))
+                .getId();
+
+        // Lock both rows in ascending id order so opposite-direction transfers cannot deadlock.
+        Long fromId = fromAccount.getId();
+        Account first = lockAccount(Math.min(fromId, toId));
+        Account second = lockAccount(Math.max(fromId, toId));
+        Account from = fromId <= toId ? first : second;
+        Account to = fromId <= toId ? second : first;
+
+        if (from.getBalance().compareTo(amount) < 0) {
             throw new RuntimeException("Insufficient funds");
         }
 
-        Account toAccount = accountRepository.findByUsername(toUsername)
-                .orElseThrow(() -> new RuntimeException("Recipient account not found"));
-
-        // Deduct from sender's account
-        fromAccount.setBalance(fromAccount.getBalance().subtract(amount));
-        accountRepository.save(fromAccount);
-
-        // Add to recipient's account
-        toAccount.setBalance(toAccount.getBalance().add(amount));
-        accountRepository.save(toAccount);
-
-        // Create transaction records for both accounts
-        Transaction debitTransaction = new Transaction(
-                amount,
-                "Transfer Out to " + toAccount.getUsername(),
-                LocalDateTime.now(),
-                fromAccount
-        );
-        transactionRepository.save(debitTransaction);
-
-        Transaction creditTransaction = new Transaction(
-                amount,
-                "Transfer In from " + fromAccount.getUsername(),
-                LocalDateTime.now(),
-                toAccount
-        );
-        transactionRepository.save(creditTransaction);
+        from.setBalance(from.getBalance().subtract(amount));
+        to.setBalance(to.getBalance().add(amount));
+
+        transactionRepository.save(new Transaction(
+                amount, "Transfer Out to " + to.getUsername(), LocalDateTime.now(), from));
+        transactionRepository.save(new Transaction(
+                amount, "Transfer In from " + from.getUsername(), LocalDateTime.now(), to));
+        fromAccount.setBalance(from.getBalance());
+    }
+
+    // refresh() with a lock issues SELECT ... FOR UPDATE and overwrites any stale copy already in the
+    // persistence context (open-in-view keeps the controller's Account managed for the whole request).
+    private Account lockAccount(Long id) {
+        Account account = entityManager.find(Account.class, id);
+        if (account == null) {
+            throw new RuntimeException("Account not found");
+        }
+        entityManager.refresh(account, LockModeType.PESSIMISTIC_WRITE);
+        return account;
     }
 
 }

src/test/java/com/example/bankapp/controller/BankControllerConcurrencyTest.java

@@ -0,0 +1,151 @@
+package com.example.bankapp.controller;
+
+import com.example.bankapp.repository.AccountRepository;
+import com.example.bankapp.repository.TransactionRepository;
+import com.example.bankapp.service.AccountService;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.web.server.LocalServerPort;
+import org.springframework.test.context.ActiveProfiles;
+
+import java.math.BigDecimal;
+import java.net.URI;
+import java.net.http.HttpClient;
+import java.net.http.HttpRequest;
+import java.net.http.HttpResponse;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Drives the real HTTP endpoints so the requests go through open-in-view, where the controller's
+ * Account stays managed for the whole request.
+ */
+@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
+@ActiveProfiles("test")
+class BankControllerConcurrencyTest {
+
+    @LocalServerPort
+    private int port;
+
+    @Autowired
+    private AccountService accountService;
+
+    @Autowired
+    private AccountRepository accountRepository;
+
+    @Autowired
+    private TransactionRepository transactionRepository;
+
+    private final HttpClient http = HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NEVER).build();
+
+    @BeforeEach
+    void cleanDatabase() {
+        transactionRepository.deleteAll();
+        accountRepository.deleteAll();
+    }
+
+    @Test
+    void concurrentWithdrawRequestsCannotOverdraw() throws Exception {
+        accountService.registerAccount("mallory", "pw12345");
+        String session = login("mallory", "pw12345");
+        post(session, "/deposit", "amount=100");
+
+        fireConcurrently(20, i -> post(session, "/withdraw", "amount=100"));
+
+        assertThat(balanceOf("mallory")).isEqualByComparingTo("0.00");
+        assertThat(transactionRepository.findByAccountId(idOf("mallory")))
+                .filteredOn(t -> t.getType().equals("Withdrawal"))
+                .hasSize(1);
+    }
+
+    @Test
+    void concurrentTransferRequestsCannotSpendTheSameBalanceTwice() throws Exception {
+        accountService.registerAccount("mallory", "pw12345");
+        for (int i = 0; i < 10; i++) {
+            accountService.registerAccount("accomplice" + i, "pw12345");
+        }
+        String session = login("mallory", "pw12345");
+        post(session, "/deposit", "amount=100");
+
+        fireConcurrently(10, i -> post(session, "/transfer", "toUsername=accomplice" + i + "&amount=100"));
+
+        assertThat(balanceOf("mallory")).isEqualByComparingTo("0.00");
+        BigDecimal received = BigDecimal.ZERO;
+        for (int i = 0; i < 10; i++) {
+            received = received.add(balanceOf("accomplice" + i));
+        }
+        assertThat(received).isEqualByComparingTo("100.00");
+    }
+
+    @Test
+    void failedTransferRendersThePersistedBalance() throws Exception {
+        accountService.registerAccount("sender", "pw12345");
+        String longName = "r".repeat(250);
+        accountService.registerAccount(longName, "pw12345");
+        String session = login("sender", "pw12345");
+        post(session, "/deposit", "amount=100");
+
+        String page = http.send(form(session, "/transfer", "toUsername=" + longName + "&amount=40"),
+                HttpResponse.BodyHandlers.ofString()).body();
+
+        assertThat(balanceOf("sender")).isEqualByComparingTo("100.00");
+        assertThat(page).contains("Current Balance: $100.00").doesNotContain("Current Balance: $60.00");
+    }
+
+    private interface Request {
+        void send(int index) throws Exception;
+    }
+
+    private void fireConcurrently(int count, Request request) {
+        List<CompletableFuture<Void>> futures = new ArrayList<>();
+        for (int i = 0; i < count; i++) {
+            int index = i;
+            futures.add(CompletableFuture.runAsync(() -> {
+                try {
+                    request.send(index);
+                } catch (Exception e) {
+                    throw new IllegalStateException(e);
+                }
+            }));
+        }
+        CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join();
+    }
+
+    private String login(String username, String password) throws Exception {
+        HttpResponse<Void> response = http.send(form(null, "/login", "username=" + username + "&password=" + password),
+                HttpResponse.BodyHandlers.discarding());
+        assertThat(response.headers().firstValue("Location")).hasValueSatisfying(l -> assertThat(l).endsWith("/dashboard"));
+        return response.headers().allValues("Set-Cookie").stream()
+                .filter(c -> c.startsWith("JSESSIONID="))
+                .map(c -> c.substring(0, c.indexOf(';')))
+                .findFirst()
+                .orElseThrow();
+    }
+
+    private void post(String session, String path, String body) throws Exception {
+        http.send(form(session, path, body), HttpResponse.BodyHandlers.discarding());
+    }
+
+    private HttpRequest form(String session, String path, String body) {
+        HttpRequest.Builder builder = HttpRequest.newBuilder(URI.create("http://localhost:" + port + path))
+                .header("Content-Type", "application/x-www-form-urlencoded")
+                .POST(HttpRequest.BodyPublishers.ofString(body));
+        if (session != null) {
+            builder.header("Cookie", session);
+        }
+        return builder.build();
+    }
+
+    private Long idOf(String username) {
+        return accountRepository.findByUsername(username).orElseThrow().getId();
+    }
+
+    private BigDecimal balanceOf(String username) {
+        return accountRepository.findByUsername(username).orElseThrow().getBalance();
+    }
+}