[local] Serve a file from one open handle so Content-Length matches the body

source/file-server/src/main/java/nl/aerius/fileserver/local/LocalFileController.java

@@ -17,12 +17,17 @@
 package nl.aerius.fileserver.local;
 
 import java.io.IOException;
+import java.nio.channels.Channels;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.annotation.Profile;
-import org.springframework.core.io.FileUrlResource;
+import org.springframework.core.io.InputStreamResource;
 import org.springframework.core.io.Resource;
 import org.springframework.http.ContentDisposition;
 import org.springframework.http.HttpHeaders;
@@ -65,12 +70,17 @@ public ResponseEntity<Resource> getFile(final @PathVariable String uuid, final @
       final String file = storageService.getFile(uuid, filename);
 
       LOG.debug("Returning file: {}", file);
-      final FileUrlResource resource = new FileUrlResource(file);
+      // Open the file once and take both the length and the bytes from that one handle. A FileUrlResource reads the length
+      // for the Content-Length header first and opens the file later, so a concurrent overwrite in between makes Tomcat
+      // send a truncated (or never completed) body. An open handle keeps the old file even after it is renamed over.
+      final SeekableByteChannel channel = Files.newByteChannel(Path.of(file), StandardOpenOption.READ);
+      final long contentLength = channel.size();
+      final InputStreamResource resource = new InputStreamResource(Channels.newInputStream(channel));
       final HttpHeaders headers = new HttpHeaders();
       final ContentDisposition contentDisposition = ContentDisposition.attachment().filename(filename).build();
 
       headers.setContentDisposition(contentDisposition);
-      return ResponseEntity.ok().headers(headers).body(resource);
+      return ResponseEntity.ok().headers(headers).contentLength(contentLength).body(resource);
     } catch (final IOException e) {
       LOG.trace("IOException when trying to get a file", e);
     } catch (final RuntimeException e) {

source/file-server/src/test/java/nl/aerius/fileserver/local/LocalFileControllerTest.java

@@ -17,6 +17,7 @@
 package nl.aerius.fileserver.local;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.ArgumentMatchers.isNotNull;
@@ -32,7 +33,11 @@
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+import java.util.List;
 import java.util.UUID;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
@@ -120,6 +125,47 @@ void testGetFile() throws Exception {
     assertEquals(content, response.getContentAsString(), "Expects file content to be in data");
   }
 
+  @Test
+  void testGetFileWhileOverwritten() throws Exception {
+    // A file that is atomically replaced while it is being served must still come back whole: the Content-Length header and
+    // the body have to describe the same version of the file. Reading the length first and opening the file later serves the
+    // new bytes under the old length, which the client then sees as a truncated body.
+    final String tempFilename = UUID.randomUUID().toString();
+    final Path file = new File(tempDir, tempFilename).toPath();
+    final String small = "{\"errors\":[],\"warnings\":[]}";
+    final String large = "{\"errors\":[\"" + "x".repeat(4000) + "\"]}";
+    Files.writeString(file, small);
+    doReturn(file.toAbsolutePath().toString()).when(storageService).getFile(UUID_CODE, tempFilename);
+
+    final AtomicBoolean running = new AtomicBoolean(true);
+    final Thread writer = new Thread(() -> {
+      try {
+        while (running.get()) {
+          for (final String content : List.of(large, small)) {
+            final Path staged = Files.createTempFile(tempDir.toPath(), tempFilename, ".tmp");
+            Files.writeString(staged, content);
+            Files.move(staged, file, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+          }
+        }
+      } catch (final Exception e) {
+        throw new IllegalStateException(e);
+      }
+    });
+    writer.start();
+    try {
+      for (int i = 0; i < 2000; i++) {
+        final MockHttpServletResponse response = mvc.perform(get(HTTP_LOCALHOST + UUID_CODE + "/" + tempFilename)).andExpect(status().isOk())
+            .andReturn().getResponse();
+        final String body = response.getContentAsString();
+        assertEquals(body.length(), response.getContentLength(), "Content-Length must match the body that was served (iteration " + i + ")");
+        assertTrue(body.equals(small) || body.equals(large), "Body must be one complete version of the file (iteration " + i + ")");
+      }
+    } finally {
+      running.set(false);
+      writer.join();
+    }
+  }
+
   @Test
   void testGetFile404Missing() throws Exception {
     doThrow(new FileNotFoundException()).when(storageService).getFile(any(), any());