Always preserve job records created by cron (#767)

README.md

@@ -203,17 +203,10 @@ Usage:
 Options:
   [--before-seconds-ago=SECONDS] # Destroy records finished more than this many seconds ago (env var:  GOOD_JOB_CLEANUP_PRESERVED_JOBS_BEFORE_SECONDS_AGO, default: 1209600 (14 days))
 
-Destroys preserved job records.
+Manually destroys preserved job records.
 
-By default, GoodJob destroys job records when the job is performed and this
-command is not necessary.
-
-However, when `GoodJob.preserve_job_records = true`, the jobs will be
-preserved in the database. This is useful when wanting to analyze or
-inspect job performance.
-
-If you are preserving job records this way, use this command regularly
-to destroy old records and preserve space in your database.
+By default, GoodJob automatically destroys job records when the job is performed
+and this command is not required to be used.
 ```
 
 ### Configuration options
@@ -460,7 +453,9 @@ GoodJob's concurrency control strategy for `perform_limit` is "optimistic retry
 
 GoodJob can enqueue jobs on a recurring basis that can be used as a replacement for cron.
 
-Cron-style jobs are run on every GoodJob process (e.g. CLI or `async` execution mode) when `config.good_job.enable_cron = true`, but GoodJob's cron uses unique indexes to ensure that only a single job is enqueued at the given time interval.
+Cron-style jobs are run on every GoodJob process (e.g. CLI or `async` execution mode) when `config.good_job.enable_cron = true`.
+
+GoodJob's cron uses unique indexes to ensure that only a single job is enqueued at the given time interval. In order for this to work, GoodJob must preserve cron-created job records; these records will be automatically deleted like any other preserved record.
 
 Cron-format is parsed by the [`fugit`](https://github.com/floraison/fugit) gem, which has support for seconds-level resolution (e.g. `* * * * * *`) and natural language parsing (e.g. `every second`).
 

app/models/good_job/execution.rb

@@ -300,7 +300,7 @@ def perform
       reenqueued = result.retried? || retried_good_job_id.present?
       if result.unhandled_error && GoodJob.retry_on_unhandled_error
         save!
-      elsif GoodJob.preserve_job_records == true || reenqueued || (result.unhandled_error && GoodJob.preserve_job_records == :on_unhandled_error)
+      elsif GoodJob.preserve_job_records == true || reenqueued || (result.unhandled_error && GoodJob.preserve_job_records == :on_unhandled_error) || cron_key.present?
         self.finished_at = Time.current
         save!
       else

lib/good_job.rb

@@ -54,9 +54,10 @@ module GoodJob
   # @!attribute [rw] preserve_job_records
   #   @!scope class
   #   Whether to preserve job records in the database after they have finished (default: +true+).
-  #   By default, GoodJob deletes job records after the job is completed successfully.
   #   If you want to preserve jobs for latter inspection, set this to +true+.
   #   If you want to preserve only jobs that finished with error for latter inspection, set this to +:on_unhandled_error+.
+  #   If you do not want to preserve jobs, set this to +false+.
+  #   When using GoodJob's cron functionality, job records will be preserved for a brief time to prevent duplicate jobs.
   #   @return [Boolean, Symbol, nil]
   mattr_accessor :preserve_job_records, default: true
 

lib/good_job/cli.rb

@@ -128,17 +128,10 @@ def start
     # @!macro thor.desc
     desc :cleanup_preserved_jobs, "Destroys preserved job records."
     long_desc <<~DESCRIPTION
-      Destroys preserved job records.
+      Manually destroys preserved job records.
 
-      By default, GoodJob destroys job records when the job is performed and this
-      command is not necessary.
-
-      However, when `GoodJob.preserve_job_records = true`, the jobs will be
-      preserved in the database. This is useful when wanting to analyze or
-      inspect job performance.
-
-      If you are preserving job records this way, use this command regularly
-      to destroy old records and preserve space in your database.
+      By default, GoodJob automatically destroys job records when the job is performed
+      and this command is not required to be used.
 
     DESCRIPTION
     method_option :before_seconds_ago,

lib/good_job/configuration.rb

@@ -221,8 +221,7 @@ def queue_select_limit
       )&.to_i
     end
 
-    # Whether to destroy discarded jobs when cleaning up preserved jobs.
-    # This configuration is only used when {GoodJob.preserve_job_records} is +true+.
+    # Whether to automatically destroy discarded jobs that have been preserved.
     # @return [Boolean]
     def cleanup_discarded_jobs?
       return rails_config[:cleanup_discarded_jobs] unless rails_config[:cleanup_discarded_jobs].nil?
@@ -231,8 +230,7 @@ def cleanup_discarded_jobs?
       true
     end
 
-    # Number of seconds to preserve jobs when using the +good_job cleanup_preserved_jobs+ CLI command.
-    # This configuration is only used when {GoodJob.preserve_job_records} is +true+.
+    # Number of seconds to preserve jobs before automatic destruction.
     # @return [Integer]
     def cleanup_preserved_jobs_before_seconds_ago
       (
@@ -243,7 +241,7 @@ def cleanup_preserved_jobs_before_seconds_ago
       ).to_i
     end
 
-    # Number of jobs a {Scheduler} will execute before cleaning up preserved jobs.
+    # Number of jobs a {Scheduler} will execute before automatically cleaning up preserved jobs.
     # @return [Integer, nil]
     def cleanup_interval_jobs
       value = (
@@ -254,7 +252,7 @@ def cleanup_interval_jobs
       value.present? ? value.to_i : nil
     end
 
-    # Number of seconds a {Scheduler} will wait before cleaning up preserved jobs.
+    # Number of seconds a {Scheduler} will wait before automatically cleaning up preserved jobs.
     # @return [Integer, nil]
     def cleanup_interval_seconds
       value = (

spec/app/models/good_job/execution_spec.rb

@@ -432,6 +432,22 @@ def job_params
       end
     end
 
+    context 'when the job is a cron job and records are not preserved' do
+      before do
+        allow(GoodJob).to receive(:preserve_job_records).and_return(false)
+        TestJob.queue_adapter = GoodJob::Adapter.new(execution_mode: :inline)
+        good_job.update(cron_key: "test_key", cron_at: Time.current)
+      end
+
+      it 'preserves the job record anyway' do
+        good_job.perform
+        expect(good_job.reload).to have_attributes(
+          performed_at: within(1.second).of(Time.current),
+          finished_at: within(1.second).of(Time.current)
+        )
+      end
+    end
+
     it 'raises an error if the job is attempted to be re-run' do
       good_job.update!(finished_at: Time.current)
       expect { good_job.perform }.to raise_error described_class::PreviouslyPerformedError