fix: login requiring two clicks to work

deno.json

@@ -10,6 +10,7 @@
     "dev": "vite",
     "build": "vite build",
     "start": "deno serve -A _fresh/server.js",
+    "test": "deno test -A --env-file=.env tests/",
     "update": "deno run -A -r jsr:@fresh/update ."
   },
   "lint": {

routes/api/auth/password.ts

@@ -75,6 +75,17 @@ export const handler: RouteHandler<User, State> = {
       });
     }
 
+    // This ensures the middleware can find the session after redirect, preventing
+    // the race condition where eventual consistency hasn't caught up yet
+    const verifySession = await kv.get<User>(sessionKey, {
+      consistency: "strong",
+    });
+
+    if (!verifySession.value) {
+      logger.error("Session verification failed after write", { sessionId });
+      throw new Error("Failed to verify session creation");
+    }
+
     await kv.delete([UserKeys.TEMPORARY_LOGIN, email]);
     logger.debug("User logged in! Redirecting to home", { email });
 

tests/db/models/user-race-condition.test.ts

@@ -0,0 +1,161 @@
+import { assertEquals, assertExists } from "@std/assert";
+import UserService, { Keys, User } from "@/db/models/user.ts";
+import { kv } from "@/db/kv.ts";
+
+const SESSION_TTL = 90 * 24 * 60 * 60 * 1000;
+
+// Clean up test data after each test
+async function cleanup(email: string, sessionId?: string) {
+  const user = await UserService.getByEmail(email);
+  if (user) {
+    await kv.delete([Keys.USERS, user.id]);
+    await kv.delete([Keys.USERS_BY_EMAIL, email]);
+  }
+  if (sessionId) {
+    await kv.delete([Keys.USERS_SESSION, sessionId]);
+  }
+}
+
+Deno.test(
+  "Race Condition Timing Test - Issue #17 Demonstration",
+  async (t) => {
+    const testEmail = `timing-test-${crypto.randomUUID()}@example.com`;
+    const sessionId = crypto.randomUUID();
+
+    await t.step("cleanup before tests", async () => {
+      await cleanup(testEmail, sessionId);
+    });
+
+    await t.step(
+      "NEGATIVE: immediate eventual consistency read may fail after atomic write",
+      async () => {
+        // Arrange: Create a user
+        const user = await UserService.create({ email: testEmail });
+
+        // Act: Simulate exact sequence from password.ts
+        // 1. Write session atomically
+        const sessionKey = [Keys.USERS_SESSION, sessionId];
+        const sessionRes = await kv
+          .atomic()
+          .check({ key: sessionKey, versionstamp: null })
+          .set(sessionKey, user, { expireIn: SESSION_TTL })
+          .commit();
+
+        assertEquals(sessionRes.ok, true);
+
+        // 2. Immediately read with eventual consistency (this is what happens first in getBySessionId)
+        const eventualUser = await kv.get<User>(sessionKey, {
+          consistency: "eventual",
+        });
+
+        // This demonstrates the race condition:
+        // - In production/deployed environments, eventual consistency might not have caught up
+        // - In local/test environments, it often works immediately due to single-node setup
+        console.log(
+          `Immediate eventual read after atomic write: ${
+            eventualUser.value ? "SUCCESS" : "FAILED (race condition)"
+          }`,
+        );
+
+        // The current code has a fallback to strong consistency, which masks the issue:
+        if (eventualUser.value === null) {
+          const strongUser = await kv.get<User>(sessionKey);
+          assertExists(
+            strongUser.value,
+            "Fallback to strong consistency should work",
+          );
+        }
+
+        // However, the issue is that we shouldn't need this fallback for FRESH sessions
+        // The fix should ensure strong consistency for newly created sessions
+      },
+    );
+
+    await t.step(
+      "POSITIVE: verify current UserService.getBySessionId has fallback",
+      async () => {
+        // Act: Use the actual UserService method
+        const user = await UserService.getBySessionId(sessionId);
+
+        // Assert: The fallback mechanism in getBySessionId should handle this
+        assertExists(
+          user,
+          "UserService.getBySessionId should find the session (via fallback)",
+        );
+
+        // Note: This test passes because getBySessionId has a fallback to strong consistency
+        // However, for optimal performance and reliability, we should:
+        // 1. Verify the session is readable with strong consistency BEFORE redirecting
+        // 2. Or use a method that guarantees immediate availability after write
+      },
+    );
+
+    await t.step("cleanup after tests", async () => {
+      await cleanup(testEmail, sessionId);
+    });
+  },
+);
+
+Deno.test("Proposed Fix Verification - Issue #17", async (t) => {
+  const testEmail = `fix-test-${crypto.randomUUID()}@example.com`;
+  const sessionId = crypto.randomUUID();
+
+  await t.step("cleanup before tests", async () => {
+    await cleanup(testEmail, sessionId);
+  });
+
+  await t.step(
+    "POSITIVE: verify session exists with strong consistency before redirect",
+    async () => {
+      // Arrange: Create user
+      const user = await UserService.create({ email: testEmail });
+
+      // Act: Write session and immediately verify with strong consistency
+      const sessionKey = [Keys.USERS_SESSION, sessionId];
+      const sessionRes = await kv
+        .atomic()
+        .check({ key: sessionKey, versionstamp: null })
+        .set(sessionKey, user, { expireIn: SESSION_TTL })
+        .commit();
+
+      assertEquals(sessionRes.ok, true);
+
+      // This is what the fix should do: verify with STRONG consistency
+      // BEFORE sending the redirect response
+      const verifyUser = await kv.get<User>(sessionKey, {
+        consistency: "strong",
+      });
+
+      // Assert: Strong consistency read should always work immediately
+      assertExists(
+        verifyUser.value,
+        "Strong consistency verification should confirm session exists",
+      );
+      assertEquals(verifyUser.value.id, user.id);
+
+      // Now it's safe to redirect, because we KNOW the session is available
+      // for the middleware to read (even with eventual consistency)
+    },
+  );
+
+  await t.step(
+    "POSITIVE: after strong consistency verification, eventual consistency should work",
+    async () => {
+      // After the strong consistency read in the previous step,
+      // even eventual consistency should work
+      const sessionKey = [Keys.USERS_SESSION, sessionId];
+      const eventualUser = await kv.get<User>(sessionKey, {
+        consistency: "eventual",
+      });
+
+      assertExists(
+        eventualUser.value,
+        "After strong consistency verification, eventual should work",
+      );
+    },
+  );
+
+  await t.step("cleanup after tests", async () => {
+    await cleanup(testEmail, sessionId);
+  });
+});

tests/db/models/user.test.ts

@@ -0,0 +1,176 @@
+import { assertEquals, assertExists } from "@std/assert";
+import UserService, { Keys, User } from "@/db/models/user.ts";
+import { kv } from "@/db/kv.ts";
+
+// Clean up test data after each test
+async function cleanup(email: string, sessionId?: string) {
+  const user = await UserService.getByEmail(email);
+  if (user) {
+    await kv.delete([Keys.USERS, user.id]);
+    await kv.delete([Keys.USERS_BY_EMAIL, email]);
+  }
+  if (sessionId) {
+    await kv.delete([Keys.USERS_SESSION, sessionId]);
+  }
+}
+
+Deno.test("UserService - Session Management", async (t) => {
+  const testEmail = `test-${crypto.randomUUID()}@example.com`;
+  const sessionId = crypto.randomUUID();
+
+  await t.step("cleanup before tests", async () => {
+    await cleanup(testEmail, sessionId);
+  });
+
+  await t.step(
+    "POSITIVE: should immediately retrieve session after creation with strong consistency",
+    async () => {
+      // Arrange: Create a user
+      const user = await UserService.create({ email: testEmail });
+      assertExists(user);
+      assertEquals(user.email, testEmail);
+
+      // Act: Set session and immediately try to retrieve it
+      await UserService.setSession(user, sessionId);
+
+      // Assert: Session should be immediately available
+      // This test verifies that after setting a session, we can immediately read it
+      // which is critical for the first-login redirect to work properly
+      const retrievedUser = await UserService.getBySessionId(sessionId);
+      assertExists(retrievedUser, "Session should be immediately retrievable");
+      assertEquals(retrievedUser.id, user.id);
+      assertEquals(retrievedUser.email, user.email);
+    },
+  );
+
+  await t.step(
+    "NEGATIVE: should handle non-existent session gracefully",
+    async () => {
+      // Arrange: Use a session ID that doesn't exist
+      const nonExistentSessionId = crypto.randomUUID();
+
+      // Act: Try to retrieve non-existent session
+      const result = await UserService.getBySessionId(nonExistentSessionId);
+
+      // Assert: Should return null, not throw an error
+      // This ensures the middleware can handle missing sessions gracefully
+      assertEquals(
+        result,
+        null,
+        "Non-existent session should return null, not throw",
+      );
+    },
+  );
+
+  await t.step(
+    "POSITIVE: getBySessionId should work with eventual consistency for existing sessions",
+    async () => {
+      // Arrange: Session already exists from previous test
+      // Wait a bit to ensure eventual consistency has caught up
+      await new Promise((resolve) => setTimeout(resolve, 100));
+
+      // Act: Retrieve with eventual consistency (should work for older sessions)
+      const retrievedUser = await UserService.getBySessionId(sessionId);
+
+      // Assert: Should still retrieve the session
+      // This verifies backward compatibility with the eventual consistency check
+      assertExists(
+        retrievedUser,
+        "Existing session should be retrievable with eventual consistency",
+      );
+    },
+  );
+
+  await t.step(
+    "NEGATIVE: should not create duplicate sessions",
+    async () => {
+      // Arrange: Get the user
+      const user = await UserService.getByEmail(testEmail);
+      assertExists(user);
+
+      // Act: Try to set session with the same ID again
+      await UserService.setSession(user, sessionId);
+
+      // Assert: Should still have only one session (verified by retrieval)
+      const retrievedUser = await UserService.getBySessionId(sessionId);
+      assertExists(retrievedUser);
+      assertEquals(retrievedUser.id, user.id);
+    },
+  );
+
+  await t.step("cleanup after tests", async () => {
+    await cleanup(testEmail, sessionId);
+  });
+});
+
+Deno.test(
+  "UserService - Race Condition Reproduction (Issue #17)",
+  async (t) => {
+    const testEmail = `race-test-${crypto.randomUUID()}@example.com`;
+    const sessionId = crypto.randomUUID();
+
+    await t.step("cleanup before tests", async () => {
+      await cleanup(testEmail, sessionId);
+    });
+
+    await t.step(
+      "NEGATIVE: demonstrates the race condition with eventual consistency",
+      async () => {
+        // Arrange: Create a user
+        const user = await UserService.create({ email: testEmail });
+
+        // Act: Set session using atomic write
+        const sessionKey = [Keys.USERS_SESSION, sessionId];
+        const SESSION_TTL = 90 * 24 * 60 * 60 * 1000;
+        const sessionRes = await kv
+          .atomic()
+          .check({ key: sessionKey, versionstamp: null })
+          .set(sessionKey, user, { expireIn: SESSION_TTL })
+          .commit();
+
+        assertEquals(sessionRes.ok, true, "Session write should succeed");
+
+        // Act: Immediately try to read with eventual consistency (simulates the bug)
+        const eventualUser = await kv.get<User>(sessionKey, {
+          consistency: "eventual",
+        });
+
+        // Assert: This test documents the race condition
+        // In the actual bug, eventualUser.value might be null immediately after write
+        // causing the middleware to fail to find the user on first redirect
+        // Note: This test may pass or fail depending on timing, which demonstrates
+        // the non-deterministic nature of the race condition
+        console.log(
+          `Eventual consistency result: ${
+            eventualUser.value ? "found" : "not found"
+          } (race condition may occur)`,
+        );
+
+        // The fix should ensure we don't rely on eventual consistency for fresh sessions
+      },
+    );
+
+    await t.step(
+      "POSITIVE: strong consistency read always works immediately after write",
+      async () => {
+        // Arrange: Session was just written in previous test
+        const sessionKey = [Keys.USERS_SESSION, sessionId];
+
+        // Act: Read with strong consistency
+        const strongUser = await kv.get<User>(sessionKey);
+
+        // Assert: Strong consistency should always find the just-written session
+        // This is the behavior we need for the fix
+        assertExists(
+          strongUser.value,
+          "Strong consistency read should always work after write",
+        );
+        assertEquals(strongUser.value.email, testEmail);
+      },
+    );
+
+    await t.step("cleanup after tests", async () => {
+      await cleanup(testEmail, sessionId);
+    });
+  },
+);