From 1da09f2c525a55b0282d464d011e2cd4fdb9d7a3 Mon Sep 17 00:00:00 2001
From: ridiculousfish <corydoras@ridiculousfish.com>
Date: Thu, 16 Jan 2020 15:59:10 -0800
Subject: [PATCH] Ensure new job IDs are never smaller than existing running
 jobs

This makes job IDs "monotone" in the sense that newly spawned jobs
always have larger IDs than existing jobs, as requested in #6053
---
 src/proc.cpp              | 37 +++++++++++--------------------------
 tests/checks/job-ids.fish | 17 +++++++++++++++++
 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/src/proc.cpp b/src/proc.cpp
index e46658382..c20e543a3 100644
--- a/src/proc.cpp
+++ b/src/proc.cpp
@@ -81,42 +81,27 @@ void set_job_control_mode(job_control_t mode) { job_control_mode = mode; }
 
 void proc_init() { signal_set_handlers_once(false); }
 
-// Basic thread safe job IDs. The vector consumed_job_ids has a true value wherever the job ID
-// corresponding to that slot is in use. The job ID corresponding to slot 0 is 1.
-static owning_lock<std::vector<bool>> locked_consumed_job_ids;
+// Basic thread safe sorted vector of job IDs in use.
+static owning_lock<std::vector<job_id_t>> locked_consumed_job_ids;
 
 job_id_t acquire_job_id() {
     auto consumed_job_ids = locked_consumed_job_ids.acquire();
 
-    // Find the index of the first 0 slot.
-    auto slot = std::find(consumed_job_ids->begin(), consumed_job_ids->end(), false);
-    if (slot != consumed_job_ids->end()) {
-        // We found a slot. Note that slot 0 corresponds to job ID 1.
-        *slot = true;
-        return static_cast<job_id_t>(slot - consumed_job_ids->begin() + 1);
-    }
-
-    // We did not find a slot; create a new slot. The size of the vector is now the job ID
-    // (since it is one larger than the slot).
-    consumed_job_ids->push_back(true);
-    return static_cast<job_id_t>(consumed_job_ids->size());
+    // The new job ID should be larger than the largest currently used ID (#6053).
+    job_id_t jid = consumed_job_ids->empty() ? 1 : consumed_job_ids->back() + 1;
+    consumed_job_ids->push_back(jid);
+    return jid;
 }
 
 void release_job_id(job_id_t jid) {
     assert(jid > 0);
     auto consumed_job_ids = locked_consumed_job_ids.acquire();
-    size_t slot = static_cast<size_t>(jid - 1), count = consumed_job_ids->size();
 
-    // Make sure this slot is within our vector and is currently set to consumed.
-    assert(slot < count);
-    assert(consumed_job_ids->at(slot) == true);
-
-    // Clear it and then resize the vector to eliminate unused trailing job IDs.
-    consumed_job_ids->at(slot) = false;
-    while (count--) {
-        if (consumed_job_ids->at(count)) break;
-    }
-    consumed_job_ids->resize(count + 1);
+    // Our job ID vector is sorted, but the number of jobs is typically 1 or 2 so a binary search
+    // isn't worth it.
+    auto where = std::find(consumed_job_ids->begin(), consumed_job_ids->end(), jid);
+    assert(where != consumed_job_ids->end() && "JobID was not in use");
+    consumed_job_ids->erase(where);
 }
 
 job_t *job_t::from_job_id(job_id_t id) {
diff --git a/tests/checks/job-ids.fish b/tests/checks/job-ids.fish
index f0dd0bd81..ecd8ad2c6 100644
--- a/tests/checks/job-ids.fish
+++ b/tests/checks/job-ids.fish
@@ -29,6 +29,23 @@ jobs
 #CHECK: 2{{.*\t}}sleep 200 &
 #CHECK: 1{{.*\t}}sleep 100 &
 
+# Kill job 2; the next job should have job ID 4 because 3 is still in use (#6053).
+
+status job-control interactive
+command kill -9 $tokill[2]
+set -e tokill[2]
+status job-control full
+sleep 400 &
+set -g tokill $tokill $last_pid
+
+jobs
+
+#CHECK: Job	Group{{.*}}
+#CHECK: 4{{.*\t}}sleep 400 &
+#CHECK: 3{{.*\t}}sleep 300 &
+#CHECK: 1{{.*\t}}sleep 100 &
+
+
 status job-control interactive
 
 for pid in $tokill