Skip to content

Commit

Permalink
🐞 Eliminate spurious UnknownError from CC workers
Browse files Browse the repository at this point in the history
- Previously, the old records cleanup generates UnknownErrors when there are no
  records & it has been directed to preserve the most recent.  Although
  these UnknownErrors are harmless, they pollute logs, and can cause consternation.
- These UnknownErrors are not visible to the user via API calls; they appear only
  in the worker logs.
- We now check to make sure that there is at least one record before
  attempting to preserve it.

fixes:

```json
{
  "error_code": "UnknownError",
  "description": "An unknown error occurred.",
  "code": 10001,
  "test_mode_info": {
    "description": "undefined method `id' for nil:NilClass",
    "error_code": "CF-id"
  }
}
```

[finishes #172773653]

Co-authored-by: Reid Mitchell <[email protected]>
Co-authored-by: Brian Cunnie <[email protected]>
  • Loading branch information
reidmit and Brian Cunnie committed May 11, 2020
1 parent 9997811 commit 7eb2257
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 2 deletions.
6 changes: 4 additions & 2 deletions lib/database/old_record_cleanup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ def delete

old_records = model.dataset.where(Sequel.lit('created_at < ?', cutoff_date))
if keep_at_least_one_record
last_id = model.order(:id).last.id
old_records = old_records.where(Sequel.lit('id < ?', last_id))
last_record = model.order(:id).last
if last_record
old_records = old_records.where(Sequel.lit('id < ?', last_record.id))
end
end
logger.info("Cleaning up #{old_records.count} #{model.table_name} table rows")

Expand Down
9 changes: 9 additions & 0 deletions spec/unit/lib/database/old_record_cleanup_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@
expect { stale_event2.reload }.to raise_error(Sequel::NoExistingObject)
end

context "when there are no records at all but you're trying to keep at least one" do
it "doesn't keep one because there aren't any to keep" do
record_cleanup = Database::OldRecordCleanup.new(VCAP::CloudController::ServiceUsageEvent, 1, keep_at_least_one_record: true)

expect { record_cleanup.delete }.not_to raise_error
expect(VCAP::CloudController::ServiceUsageEvent.count).to eq(0)
end
end

it 'only retrieves the current timestamp from the database once' do
expect(VCAP::CloudController::Event.db).to receive(:fetch).with('SELECT CURRENT_TIMESTAMP as now').once.and_call_original
record_cleanup = Database::OldRecordCleanup.new(VCAP::CloudController::Event, 1)
Expand Down

0 comments on commit 7eb2257

Please sign in to comment.