Skip to content

Commit 87f93fc

Browse files
kathapkathap
committed
Adjust review comments
1 parent c9dc2ee commit 87f93fc

5 files changed

Lines changed: 138 additions & 76 deletions

File tree

‎app/jobs/pollable_job_wrapper.rb‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,12 @@ def error(job, exception)
7070
end
7171

7272
def failure(job)
73-
begin
74-
unwrapped_job = Enqueuer.unwrap_job(@handler)
75-
unwrapped_job.recover_from_failure
76-
rescue StandardError => e
77-
logger.error("failure recovery failed: #{e.class}: #{e.message}")
73+
if @handler.respond_to?(:recover_from_failure)
74+
begin
75+
@handler.recover_from_failure
76+
rescue StandardError => e
77+
logger.error("failure recovery failed: #{e.class}: #{e.message}")
78+
end
7879
end
7980

8081
change_state(job, PollableJobModel::FAILED_STATE)

‎app/jobs/v3/services/synchronize_broker_catalog_job.rb‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ def recover_from_failure
4848

4949
class Perform
5050
def initialize(broker_guid, user_audit_info:)
51+
sleep 20
5152
@broker = ServiceBroker.find(guid: broker_guid)
5253
@catalog_updater = VCAP::CloudController::V3::ServiceBrokerCatalogUpdater.new(@broker, user_audit_info:)
5354
end
@@ -56,11 +57,12 @@ def perform
5657
broker.update(state: ServiceBrokerStateEnum::SYNCHRONIZING)
5758

5859
warnings = @catalog_updater.refresh
59-
60+
raise "some error"
6061
broker.update(state: ServiceBrokerStateEnum::AVAILABLE)
6162

6263
warnings
6364
rescue StandardError => e
65+
sleep 20
6466
begin
6567
broker.update(state: ServiceBrokerStateEnum::SYNCHRONIZATION_FAILED)
6668
rescue StandardError

‎spec/unit/jobs/pollable_job_wrapper_spec.rb‎

Lines changed: 38 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -202,52 +202,54 @@ class BigException < StandardError
202202
end
203203
end
204204

205-
context 'when the job implements recover_from_failure' do
206-
let(:broker) do
207-
VCAP::CloudController::ServiceBroker.create(
208-
name: 'recovery-test-broker',
209-
broker_url: 'http://example.org/broker-url',
210-
auth_username: 'username',
211-
auth_password: 'password',
212-
state: VCAP::CloudController::ServiceBrokerStateEnum::SYNCHRONIZING
213-
)
205+
describe '#failure' do
206+
let(:delayed_job) { instance_double(Delayed::Backend::Sequel::Job, guid: 'job-guid') }
207+
let!(:pollable_job) do
208+
VCAP::CloudController::PollableJobModel.make(delayed_job_guid: 'job-guid', state: VCAP::CloudController::PollableJobModel::PROCESSING_STATE)
214209
end
215210

216-
let(:update_request) do
217-
VCAP::CloudController::ServiceBrokerUpdateRequest.create(
218-
name: 'new-name',
219-
service_broker_id: broker.id
220-
)
221-
end
211+
context 'when handler implements recover_from_failure' do
212+
let(:handler) do
213+
instance_double(VCAP::CloudController::V3::UpdateBrokerJob, recover_from_failure: nil, warnings: nil)
214+
end
215+
let(:wrapper) { PollableJobWrapper.new(handler) }
222216

223-
let(:user_audit_info) { instance_double(VCAP::CloudController::UserAuditInfo, { user_guid: Sham.guid }) }
224-
let(:update_job) { VCAP::CloudController::V3::UpdateBrokerJob.new(update_request.guid, broker.guid, 'AVAILABLE', user_audit_info:) }
225-
let(:pollable_job) { PollableJobWrapper.new(update_job) }
217+
it 'calls recover_from_failure and marks the pollable job failed' do
218+
wrapper.failure(delayed_job)
226219

227-
before do
228-
allow_any_instance_of(VCAP::CloudController::V3::UpdateBrokerJob::Perform).to receive(:perform).and_raise(StandardError.new('job failed'))
229-
end
220+
expect(handler).to have_received(:recover_from_failure)
221+
expect(pollable_job.reload.state).to eq(VCAP::CloudController::PollableJobModel::FAILED_STATE)
222+
end
230223

231-
it 'calls recover_from_failure and reverts broker state to AVAILABLE' do
232-
enqueued_job = VCAP::CloudController::Jobs::Enqueuer.new.enqueue(pollable_job)
233-
VCAP::CloudController::PollableJobModel.make(delayed_job_guid: enqueued_job.guid, state: 'PROCESSING')
224+
context 'when recover_from_failure raises an error' do
225+
let(:logger) { instance_double(Steno::Logger, error: nil) }
234226

235-
execute_all_jobs(expected_successes: 0, expected_failures: 1)
227+
before do
228+
allow(handler).to receive(:recover_from_failure).and_raise(StandardError.new('recovery failed'))
229+
allow(Steno).to receive(:logger).with('cc.pollable.job.wrapper').and_return(logger)
230+
end
236231

237-
broker.reload
238-
expect(broker.state).to eq('SYNCHRONIZATION_FAILED')
239-
end
240-
end
232+
it 'logs the error without re-raising' do
233+
expect { wrapper.failure(delayed_job) }.not_to raise_error
234+
expect(logger).to have_received(:error).with(/failure recovery failed/)
235+
end
241236

242-
context 'when the job does not implement recover_from_failure' do
243-
it 'still marks the job as FAILED without error' do
244-
enqueued_job = VCAP::CloudController::Jobs::Enqueuer.new.enqueue(pollable_job)
245-
job_model = VCAP::CloudController::PollableJobModel.make(delayed_job_guid: enqueued_job.guid, state: 'PROCESSING')
237+
it 'still marks the pollable job as failed' do
238+
wrapper.failure(delayed_job)
246239

247-
execute_all_jobs(expected_successes: 0, expected_failures: 1)
240+
expect(pollable_job.reload.state).to eq(VCAP::CloudController::PollableJobModel::FAILED_STATE)
241+
end
242+
end
243+
end
248244

249-
job_model.reload
250-
expect(job_model.state).to eq('FAILED')
245+
context 'when handler does not implement recover_from_failure' do
246+
let(:handler) { double('HandlerWithoutRecovery', warnings: nil) }
247+
let(:wrapper) { PollableJobWrapper.new(handler) }
248+
249+
it 'still marks the pollable job as failed without error' do
250+
expect { wrapper.failure(delayed_job) }.not_to raise_error
251+
expect(pollable_job.reload.state).to eq(VCAP::CloudController::PollableJobModel::FAILED_STATE)
252+
end
251253
end
252254
end
253255
end

‎spec/unit/jobs/v3/services/synchronize_broker_catalog_job_spec.rb‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,70 @@ def incompatible_catalog
201201
allow(catalog).to receive_messages(valid?: true, compatible?: false, incompatibility_errors: incompatibility_errors)
202202
end
203203
end
204+
205+
describe '#recover_from_failure' do
206+
let(:broker) do
207+
ServiceBroker.create(
208+
name: 'test-broker',
209+
broker_url: 'http://example.org/broker-url',
210+
auth_username: 'username',
211+
auth_password: 'password'
212+
)
213+
end
214+
let(:user_audit_info) { instance_double(UserAuditInfo, { user_guid: Sham.guid }) }
215+
216+
subject(:job) do
217+
SynchronizeBrokerCatalogJob.new(broker.guid, user_audit_info:)
218+
end
219+
220+
context 'when broker is in SYNCHRONIZING state' do
221+
before do
222+
broker.update(state: ServiceBrokerStateEnum::SYNCHRONIZING)
223+
end
224+
225+
it 'sets the broker to SYNCHRONIZATION_FAILED' do
226+
expect do
227+
job.recover_from_failure
228+
end.to change { broker.reload.state }.
229+
from(ServiceBrokerStateEnum::SYNCHRONIZING).
230+
to(ServiceBrokerStateEnum::SYNCHRONIZATION_FAILED)
231+
end
232+
end
233+
234+
shared_examples 'does not change the broker state' do |expected_state|
235+
it 'leaves the state unchanged' do
236+
broker.update(state: expected_state)
237+
job.recover_from_failure
238+
239+
expect(broker.reload.state).to eq(expected_state)
240+
end
241+
end
242+
243+
context 'when broker is in a different state' do
244+
include_examples 'does not change the broker state', ServiceBrokerStateEnum::AVAILABLE
245+
include_examples 'does not change the broker state', ServiceBrokerStateEnum::DELETE_IN_PROGRESS
246+
end
247+
248+
context 'when database error occurs during recovery' do
249+
let(:dataset) { instance_double(Sequel::Dataset) }
250+
let(:logger) { instance_double(Steno::Logger, error: nil) }
251+
252+
before do
253+
broker.update(state: ServiceBrokerStateEnum::SYNCHRONIZING)
254+
allow(ServiceBroker).to receive(:where).
255+
with(guid: broker.guid, state: ServiceBrokerStateEnum::SYNCHRONIZING).
256+
and_return(dataset)
257+
allow(dataset).to receive(:update).and_raise(Sequel::DatabaseError.new(RuntimeError.new('connection lost')))
258+
allow(Steno).to receive(:logger).with('cc.background').and_return(logger)
259+
end
260+
261+
it 'logs the error and does not raise' do
262+
expect { job.recover_from_failure }.not_to raise_error
263+
expect(logger).to have_received(:error).with(/Failed to recover broker state/)
264+
expect(broker.reload.state).to eq(ServiceBrokerStateEnum::SYNCHRONIZING)
265+
end
266+
end
267+
end
204268
end
205269
end
206270
end

‎spec/unit/jobs/v3/services/update_broker_job_spec.rb‎

Lines changed: 27 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -448,25 +448,21 @@ module V3
448448

449449
context 'when database disconnects during state rollback' do
450450
let(:catalog_error) { StandardError.new('Catalog fetch failed') }
451+
let(:mock_dataset) { instance_double(Sequel::Postgres::Dataset) }
451452

452453
before do
453454
allow_any_instance_of(VCAP::CloudController::V3::ServiceBrokerCatalogUpdater).to receive(:refresh).and_raise(catalog_error)
454-
455-
mock_dataset = instance_double(Sequel::Postgres::Dataset)
456455
allow(mock_dataset).to receive(:update).and_raise(Sequel::DatabaseDisconnectError.new('connection lost'))
457-
458456
allow(ServiceBroker).to receive(:where).and_call_original
459-
allow(ServiceBroker).to receive(:where).with(id: broker.id).and_return(mock_dataset)
457+
allow(ServiceBroker).to receive(:where).
458+
with(id: update_broker_request.service_broker_id, state: ServiceBrokerStateEnum::SYNCHRONIZING).
459+
and_return(mock_dataset)
460460
end
461461

462-
it 'swallows the database error and re-raises the original catalog error' do
462+
it 're-raises the original error instead of the rollback database error' do
463463
expect { job.perform }.to raise_error(catalog_error)
464464
end
465465

466-
it 'does not raise a database connection error' do
467-
expect { job.perform }.not_to raise_error(Sequel::DatabaseDisconnectError)
468-
end
469-
470466
it 'still cleans up the update request' do
471467
expect { job.perform }.to raise_error(catalog_error)
472468
expect(ServiceBrokerUpdateRequest.where(id: update_broker_request.id).all).to be_empty
@@ -497,48 +493,45 @@ module V3
497493
end
498494

499495
it 'sets the broker to SYNCHRONIZATION_FAILED' do
500-
job.recover_from_failure
501-
502-
broker.reload
503-
expect(broker.state).to eq(ServiceBrokerStateEnum::SYNCHRONIZATION_FAILED)
496+
expect do
497+
job.recover_from_failure
498+
end.to change { broker.reload.state }.
499+
from(ServiceBrokerStateEnum::SYNCHRONIZING).
500+
to(ServiceBrokerStateEnum::SYNCHRONIZATION_FAILED)
504501
end
505502
end
506503

507-
context 'when broker is not in SYNCHRONIZING state' do
508-
before do
509-
broker.update(state: ServiceBrokerStateEnum::AVAILABLE)
510-
end
511-
512-
it 'does not change the broker state' do
504+
shared_examples 'does not change the broker state' do |expected_state|
505+
it 'leaves the state unchanged' do
506+
broker.update(state: expected_state)
513507
job.recover_from_failure
514508

515-
broker.reload
516-
expect(broker.state).to eq(ServiceBrokerStateEnum::AVAILABLE)
509+
expect(broker.reload.state).to eq(expected_state)
517510
end
518511
end
519512

520-
context 'when broker state has changed to something else' do
521-
before do
522-
broker.update(state: ServiceBrokerStateEnum::DELETE_IN_PROGRESS)
523-
end
524-
525-
it 'does not overwrite the newer state' do
526-
job.recover_from_failure
527-
528-
broker.reload
529-
expect(broker.state).to eq(ServiceBrokerStateEnum::DELETE_IN_PROGRESS)
530-
end
513+
context 'when broker is in a different state' do
514+
include_examples 'does not change the broker state', ServiceBrokerStateEnum::AVAILABLE
515+
include_examples 'does not change the broker state', ServiceBrokerStateEnum::DELETE_IN_PROGRESS
531516
end
532517

533518
context 'when database error occurs during recovery' do
519+
let(:dataset) { instance_double(Sequel::Dataset) }
520+
let(:logger) { instance_double(Steno::Logger, error: nil) }
521+
534522
before do
535523
broker.update(state: ServiceBrokerStateEnum::SYNCHRONIZING)
536-
allow(ServiceBroker).to receive(:where).and_raise(Sequel::DatabaseError.new('connection lost'))
537-
allow(Steno).to receive(:logger).and_return(double(error: nil))
524+
allow(ServiceBroker).to receive(:where).
525+
with(guid: broker.guid, state: ServiceBrokerStateEnum::SYNCHRONIZING).
526+
and_return(dataset)
527+
allow(dataset).to receive(:update).and_raise(Sequel::DatabaseError.new(RuntimeError.new('connection lost')))
528+
allow(Steno).to receive(:logger).with('cc.background').and_return(logger)
538529
end
539530

540531
it 'logs the error and does not raise' do
541532
expect { job.recover_from_failure }.not_to raise_error
533+
expect(logger).to have_received(:error).with(/Failed to recover broker state/)
534+
expect(broker.reload.state).to eq(ServiceBrokerStateEnum::SYNCHRONIZING)
542535
end
543536
end
544537
end

0 commit comments

Comments
 (0)