Improve a race condition in session establishment code

Summary:
If you try to establish several sessions quickly (e.g., by running several
copies of "arc" at once, as in "arc x | arc y"), the current logic has a high
chance of making them all pick the same conduit session to refresh (since it's
the oldest one when each process selects the current sessions). This means they
all issue updates against "conduit-3" (or whatever) and one ends up with a bogus
session.

Instead, do an update against the table with the session key we read, so only
one process wins the race. If we don't win the race, try again until we do or
have tried every session slot.

Test Plan:
  - Wiped conduit sessions, ran arc commands to verify the fresh session case.
  - Ran a bunch of arc piped to itself, e.g. "arc list | arc list | arc list |
...". It succeeds up to the session limit, and above that gets failures as
expected.
  - Manually checked the session table to make sure things seemed reasonable
there.
  - Generally ran a bunch of arc commands.
  - Logged out and logged in on the web interface.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T687

Differential Revision: https://secure.phabricator.com/D1329

conf/default.conf.php

@@ -309,7 +309,7 @@ return array(
 
   // Maximum number of simultaneous Conduit sessions each user is permitted
   // to have.
-  'auth.sessions.conduit'       => 3,
+  'auth.sessions.conduit'       => 5,
 
   // Set this true to enable the Settings -> SSH Public Keys panel, which will
   // allow users to associated SSH public keys with their accounts. This is only

src/applications/people/storage/user/PhabricatorUser.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -240,54 +240,99 @@ class PhabricatorUser extends PhabricatorUserDAO {
         "Session limit for '{$session_type}' must be at least 1!");
     }
 
-    // Load all the currently active sessions.
+    // NOTE: Session establishment is sensitive to race conditions, as when
-    $sessions = queryfx_all(
+    // piping `arc` to `arc`:
-      $conn_w,
+    //
-      'SELECT type, sessionStart FROM %T WHERE userPHID = %s AND type LIKE %>',
+    //   arc export ... | arc paste ...
-      PhabricatorUser::SESSION_TABLE,
+    //
-      $this->getPHID(),
+    // To avoid this, we overwrite an old session only if it hasn't been
-      $session_type.'-');
+    // re-established since we read it.
-
-    // Choose which 'type' we'll actually establish, i.e. what number we're
-    // going to append to the basic session type. To do this, just check all
-    // the numbers sequentially until we find an available session.
-    $establish_type = null;
-    $sessions = ipull($sessions, null, 'type');
-    for ($ii = 1; $ii <= $session_limit; $ii++) {
-      if (empty($sessions[$session_type.'-'.$ii])) {
-        $establish_type = $session_type.'-'.$ii;
-        break;
-      }
-    }
-
-    // If we didn't find an available session, choose the oldest session and
-    // overwrite it.
-    if (!$establish_type) {
-      $sessions = isort($sessions, 'sessionStart');
-      $oldest = reset($sessions);
-      $establish_type = $oldest['type'];
-    }
 
     // Consume entropy to generate a new session key, forestalling the eventual
     // heat death of the universe.
     $session_key = Filesystem::readRandomCharacters(40);
 
-    // UNGUARDED WRITES: Logging-in users don't have CSRF stuff yet.
+    // Load all the currently active sessions.
-    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+    $sessions = queryfx_all(
-
-    queryfx(
       $conn_w,
-      'INSERT INTO %T '.
+      'SELECT type, sessionKey, sessionStart FROM %T
-        '(userPHID, type, sessionKey, sessionStart)'.
+        WHERE userPHID = %s AND type LIKE %>',
-      ' VALUES '.
+      PhabricatorUser::SESSION_TABLE,
-        '(%s, %s, %s, UNIX_TIMESTAMP()) '.
-      'ON DUPLICATE KEY UPDATE '.
-        'sessionKey = VALUES(sessionKey), '.
-        'sessionStart = VALUES(sessionStart)',
-      self::SESSION_TABLE,
       $this->getPHID(),
-      $establish_type,
+      $session_type.'-');
-      $session_key);
+    $sessions = ipull($sessions, null, 'type');
+    $sessions = isort($sessions, 'sessionStart');
+
+    $existing_sessions = array_keys($sessions);
+
+    $retries = 0;
+    while (true) {
+
+      // Choose which 'type' we'll actually establish, i.e. what number we're
+      // going to append to the basic session type. To do this, just check all
+      // the numbers sequentially until we find an available session.
+      $establish_type = null;
+      for ($ii = 1; $ii <= $session_limit; $ii++) {
+        $try_type = $session_type.'-'.$ii;
+        if (!in_array($try_type, $existing_sessions)) {
+          $establish_type = $try_type;
+          $expect_key = $session_key;
+          $existing_sessions[] = $try_type;
+
+          // Ensure the row exists so we can issue an update below. We don't
+          // care if we race here or not.
+          queryfx(
+            $conn_w,
+            'INSERT IGNORE INTO %T (userPHID, type, sessionKey, sessionStart)
+              VALUES (%s, %s, %s, 0)',
+            self::SESSION_TABLE,
+            $this->getPHID(),
+            $establish_type,
+            $session_key);
+          break;
+        }
+      }
+
+      // If we didn't find an available session, choose the oldest session and
+      // overwrite it.
+      if (!$establish_type) {
+        $oldest = reset($sessions);
+        $establish_type = $oldest['type'];
+        $expect_key = $oldest['sessionKey'];
+      }
+
+      // UNGUARDED WRITES: Logging-in users don't have CSRF stuff yet.
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+
+        // This is so that we'll only overwrite the session if it hasn't been
+        // refreshed since we read it. If it has, the session key will be
+        // different and we know we're racing other processes. Whichever one
+        // won gets the session, we go back and try again.
+
+        queryfx(
+          $conn_w,
+          'UPDATE %T SET sessionKey = %s, sessionStart = UNIX_TIMESTAMP()
+            WHERE userPHID = %s AND type = %s AND sessionKey = %s',
+          self::SESSION_TABLE,
+          $session_key,
+          $this->getPHID(),
+          $establish_type,
+          $expect_key);
+
+      unset($unguarded);
+
+      if ($conn_w->getAffectedRows()) {
+        // The update worked, so the session is valid.
+        break;
+      } else {
+        // We know this just got grabbed, so don't try it again.
+        unset($sessions[$establish_type]);
+      }
+
+      if (++$retries > $session_limit) {
+        throw new Exception("Failed to establish a session!");
+      }
+    }
 
     $log = PhabricatorUserLog::newLog(
       $this,