Probe the session lock with a shared lock on a read-only channel

README.md

@@ -0,0 +1,30 @@
+# gpufl-agent
+
+Uploads the session logs that gpufl-client writes to the GPUFlight backend.
+
+## Running the agent as a separate user
+
+gpufl-client writes each session under `<log dir>/<session_id>/` and holds an
+exclusive lock on `.gpufl-session.lock` there while it runs. The agent probes
+that lock with a shared lock to decide that the session has finished, so its
+user needs group read on the lock file. The agent also writes markers and
+tombstones in the session directory and deletes uploaded payloads there, so
+its user needs group write on the session directory.
+
+When the agent runs as its own user (for example the systemd user
+`gpufl-agent`):
+
+- Make the session directory setgid and owned by the agent's group, mode 2770
+  (`chgrp gpufl-agent <dir> && chmod 2770 <dir>`), so the files the producer
+  creates in it belong to that group. If the producer creates the session
+  directories itself, do this for the log directory instead: each new session
+  directory inherits the group, but its group write bit comes from the
+  producer's umask, so the producer needs umask 002.
+- The lock file itself works with umask 002 or 022 on new clients: it is
+  created `0660` masked by the umask (0660 or 0640), and either gives the
+  agent's group the read access the probe needs. The directory rule above
+  still decides the umask when the producer creates the session directories.
+- Clients up to 1.3.0rc6 create the lock owner-only (0600). The agent cannot
+  open it, logs a warning, and keeps treating the session as active. With
+  those clients, run `chmod g+rw <session dir>/.gpufl-session.lock` after each
+  run.

src/main/java/com/gpuflight/agent/SessionOwnership.java

@@ -1,5 +1,8 @@
 package com.gpuflight.agent;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.nio.channels.FileChannel;
@@ -10,6 +13,8 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Stream;
 
 /**
@@ -20,8 +25,12 @@
  * is held; only orphan handling and session completion depend on this probe.
  */
 public final class SessionOwnership {
+    private static final Logger log =
+            LoggerFactory.getLogger(SessionOwnership.class);
     public static final String LOCK_FILE = ".gpufl-session.lock";
     public static final String LOSS_PREFIX = ".gpufl-transport-loss.";
+    private static final Set<Path> UNREADABLE_WARNED =
+            ConcurrentHashMap.newKeySet();
 
     public enum State {
         /** Another live process owns the session. */
@@ -36,23 +45,50 @@ public enum State {
 
     private SessionOwnership() {}
 
+    /**
+     * Probes with a shared lock on a read-only channel, so an agent running as
+     * another user needs only read access to the lock file. A shared lock
+     * still conflicts with the writer's exclusive one (fcntl on POSIX,
+     * LockFileEx on Windows).
+     */
     public static State probe(Path sessionDir) {
         Path path = sessionDir.resolve(LOCK_FILE);
         if (!Files.exists(path)) {
             return State.LEGACY_NO_LOCK;
         }
-        try (FileChannel channel = FileChannel.open(
-                path, StandardOpenOption.READ, StandardOpenOption.WRITE)) {
-            try (FileLock ignored = channel.tryLock()) {
+        FileChannel opened;
+        try {
+            opened = FileChannel.open(path, StandardOpenOption.READ);
+        } catch (IOException | RuntimeException unreadable) {
+            warnUnreadableOnce(sessionDir, path, unreadable);
+            return State.UNKNOWN;
+        }
+        try (FileChannel channel = opened) {
+            try (FileLock ignored = channel.tryLock(0L, Long.MAX_VALUE, true)) {
                 return ignored == null ? State.ACTIVE : State.UNOWNED;
             } catch (OverlappingFileLockException activeInThisJvm) {
                 return State.ACTIVE;
             }
-        } catch (IOException | RuntimeException unreadable) {
+        } catch (IOException | RuntimeException lockFailure) {
             return State.UNKNOWN;
         }
     }
 
+    private static void warnUnreadableOnce(
+            Path sessionDir, Path lockPath, Exception cause) {
+        if (!UNREADABLE_WARNED.add(sessionDir.toAbsolutePath().normalize())) {
+            return;
+        }
+        log.warn("Cannot open session lock {} ({}); treating the session as "
+                        + "active. If the agent runs as another user, its "
+                        + "group needs read access to the lock file and write "
+                        + "access to the session directory (the agent writes "
+                        + "markers and tombstones and deletes payloads there). "
+                        + "Clients up to 1.3.0rc6 create the lock owner-only "
+                        + "and need chmod g+rw on it.",
+                lockPath, cause.toString());
+    }
+
     public static boolean hasTransportLoss(Path sessionDir) {
         try (Stream<Path> entries = Files.list(sessionDir)) {
             return entries.anyMatch(path ->

src/test/java/com/gpuflight/agent/SessionOwnershipTest.java

@@ -1,15 +1,23 @@
 package com.gpuflight.agent;
 
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.EnabledOnOs;
+import org.junit.jupiter.api.condition.OS;
 import org.junit.jupiter.api.io.TempDir;
 
+import java.io.BufferedReader;
+import java.io.InputStreamReader;
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.util.concurrent.TimeUnit;
 
 import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assumptions.assumeFalse;
 
 class SessionOwnershipTest {
 
@@ -34,6 +42,49 @@ void osLockOverridesQuietDirectoryHeuristics(@TempDir Path session) throws Excep
                 SessionOwnership.probe(session));
     }
 
+    @Test
+    void writerLockInAnotherProcessProbesActive(@TempDir Path session)
+            throws Exception {
+        Path lockPath = session.resolve(SessionOwnership.LOCK_FILE);
+        Files.writeString(lockPath, "");
+        Process holder = new ProcessBuilder(
+                Path.of(System.getProperty("java.home"), "bin", "java").toString(),
+                "-cp", System.getProperty("java.class.path"),
+                LockHolder.class.getName(), lockPath.toString())
+                .redirectError(ProcessBuilder.Redirect.INHERIT)
+                .start();
+        try (BufferedReader out = new BufferedReader(new InputStreamReader(
+                holder.getInputStream(), StandardCharsets.UTF_8))) {
+            assertEquals("locked", out.readLine());
+            assertEquals(SessionOwnership.State.ACTIVE,
+                    SessionOwnership.probe(session));
+        } finally {
+            holder.getOutputStream().close();
+            assertTrue(holder.waitFor(30, TimeUnit.SECONDS));
+        }
+        assertEquals(SessionOwnership.State.UNOWNED,
+                SessionOwnership.probe(session));
+    }
+
+    @Test
+    @EnabledOnOs({OS.LINUX, OS.MAC})
+    void readOnlyLockFileStillProbesUnowned(@TempDir Path session)
+            throws Exception {
+        Path lockPath = session.resolve(SessionOwnership.LOCK_FILE);
+        Files.writeString(lockPath, "");
+        // What an agent in the client's group sees under umask 022.
+        Files.setPosixFilePermissions(
+                lockPath, PosixFilePermissions.fromString("r--------"));
+        assumeFalse(Files.isWritable(lockPath), "file modes do not bind root");
+        assertEquals(SessionOwnership.State.UNOWNED,
+                SessionOwnership.probe(session));
+
+        Files.setPosixFilePermissions(
+                lockPath, PosixFilePermissions.fromString("---------"));
+        assertEquals(SessionOwnership.State.UNKNOWN,
+                SessionOwnership.probe(session));
+    }
+
     @Test
     void transportLossMarkerIsDurableCompletionGate(@TempDir Path session)
             throws Exception {
@@ -42,4 +93,20 @@ void transportLossMarkerIsDurableCompletionGate(@TempDir Path session)
                 session.resolve(".gpufl-transport-loss.device.9.json"), "{}");
         assertTrue(SessionOwnership.hasTransportLoss(session));
     }
+
+    /**
+     * A writer in another process: an exclusive lock on byte 0, the range the
+     * Windows client locks, held until stdin closes.
+     */
+    static final class LockHolder {
+        public static void main(String[] args) throws Exception {
+            try (FileChannel channel = FileChannel.open(Path.of(args[0]),
+                    StandardOpenOption.READ, StandardOpenOption.WRITE);
+                 FileLock ignored = channel.lock(0L, 1L, false)) {
+                System.out.println("locked");
+                System.out.flush();
+                System.in.read();
+            }
+        }
+    }
 }