Add Process memory usage and fix process state update (#1516)

* fixes * remove dep. Use Sidekiq approach * move to PSS with a RSS fallback * Fix lint warnings and reformat output * Rescue exceptions * fix linting --------- Co-authored-by: Ben Sheldon [he/him] <[email protected]> Co-authored-by: Ben Sheldon [he/him] <[email protected]>
bensheldon · Nov 21, 2024 · 27c2a6d · 27c2a6d
1 parent c102011
commit 27c2a6d
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 5 deletions.
diff --git a/app/models/good_job/process.rb b/app/models/good_job/process.rb
@@ -12,6 +12,28 @@ class Process < BaseRecord
     STALE_INTERVAL = 30.seconds
     # Interval until the process record is treated as expired
     EXPIRED_INTERVAL = 5.minutes
+    PROCESS_MEMORY = case RUBY_PLATFORM
+                     when /linux/
+                       lambda do |pid|
+                         File.readlines("/proc/#{pid}/smaps_rollup").each do |line|
+                           next unless line.start_with?('Pss:')
+
+                           break line.split[1].to_i
+                         end
+                       rescue Errno::ENOENT
+                         File.readlines("/proc/#{pid}/status").each do |line|
+                           next unless line.start_with?('VmRSS:')
+
+                           break line.split[1].to_i
+                         end
+                       end
+                     when /darwin|bsd/
+                       lambda do |pid|
+                         `ps -o pid,rss -p #{pid.to_i}`.lines.last.split.last.to_i
+                       end
+                     else
+                       ->(_pid) { 0 }
+                     end
 
     self.table_name = 'good_job_processes'
     self.implicit_order_column = 'created_at'
@@ -56,6 +78,13 @@ def self.cleanup
       end
     end
 
+    # @return [Integer]
+    def self.memory_usage(pid)
+      PROCESS_MEMORY.call(pid)
+    rescue StandardError
+      0
+    end
+
     def self.find_or_create_record(id:, with_advisory_lock: false)
       attributes = {
         id: id,
@@ -83,6 +112,7 @@ def self.process_state
       {
         hostname: Socket.gethostname,
         pid: ::Process.pid,
+        memory: memory_usage(::Process.pid),
         proctitle: $PROGRAM_NAME,
         preserve_job_records: GoodJob.preserve_job_records,
         retry_on_unhandled_error: GoodJob.retry_on_unhandled_error,
@@ -98,8 +128,8 @@ def self.process_state
     end
 
     def refresh
-      self.state = self.class.process_state
       reload # verify the record still exists in the database
+      self.state = self.class.process_state
       update(state: state, updated_at: Time.current)
     rescue ActiveRecord::RecordNotFound
       @new_record = true

diff --git a/app/views/good_job/processes/index.html.erb b/app/views/good_job/processes/index.html.erb
@@ -38,6 +38,7 @@
               <span class="badge rounded-pill bg-body-secondary text-secondary"><%= process.state["pid"] %></span>
               <span class="text-muted small">@</span>
               <span class="badge rounded-pill bg-body-secondary text-secondary"><%= process.state["hostname"] %></span>
+              <span class="badge rounded-pill bg-body-secondary text-secondary"><%= (process.state["memory"] / 1024).to_i %> MB</span>
             </div>
           </div>
           <div class="col">

diff --git a/config/brakeman.ignore b/config/brakeman.ignore
@@ -53,7 +53,7 @@
       "check_name": "SQL",
       "message": "Possible SQL injection",
       "file": "app/models/good_job/job.rb",
-      "line": 140,
+      "line": 135,
       "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
       "code": "Arel.sql(\"(CASE #{queues.map.with_index do\n sanitize_sql_array([\"WHEN queue_name = ? THEN ?\", queue_name, index])\n end.join(\" \")} ELSE #{queues.size} END)\")",
       "render_path": null,
@@ -68,8 +68,31 @@
         89
       ],
       "note": "Developer provided value, queue_name, is sanitized."
+    },
+    {
+      "warning_type": "Command Injection",
+      "warning_code": 14,
+      "fingerprint": "dbb80167f57edebfd9da72e1278d425095a0329755e24d3c50e0fda6bb21c097",
+      "check_name": "Execute",
+      "message": "Possible command injection",
+      "file": "app/models/good_job/process.rb",
+      "line": 32,
+      "link": "https://brakemanscanner.org/docs/warning_types/command_injection/",
+      "code": "`ps -o pid,rss -p #{pid.to_i}`",
+      "render_path": null,
+      "location": {
+        "type": "method",
+        "class": "Process",
+        "method": null
+      },
+      "user_input": "pid.to_i",
+      "confidence": "Medium",
+      "cwe_id": [
+        77
+      ],
+      "note": ""
     }
   ],
-  "updated": "2024-07-18 18:05:56 -0700",
-  "brakeman_version": "6.1.2"
+  "updated": "2024-11-16 17:00:20 -0600",
+  "brakeman_version": "6.2.1"
 }
diff --git a/spec/app/models/good_job/process_spec.rb b/spec/app/models/good_job/process_spec.rb
@@ -77,7 +77,8 @@
       process = described_class.create! state: {}, updated_at: 1.day.ago
       expect do
         expect(process.refresh).to be true
-      end.to change(process, :updated_at).to within(1.second).of(Time.current)
+      end.to change(process, :state)
+         .and change(process, :updated_at).to within(1.second).of(Time.current)
     end
 
     context 'when the record has been deleted elsewhere' do