diff --git a/app/poros/webhooks/retry_failed_event.rb b/app/poros/webhooks/retry_failed_event.rb index d77421c2..1532952e 100644 --- a/app/poros/webhooks/retry_failed_event.rb +++ b/app/poros/webhooks/retry_failed_event.rb @@ -21,22 +21,38 @@ def call private + # Un éxito se registra aunque el evento se haya descartado mientras corría: + # la venta o el envío ya se procesaron, y `succeeded` es tan terminal como + # `discarded` —no vuelve a la cola—, pero además dice la verdad. def mark_succeeded - @event.update!(status: :succeeded, attempts: @event.attempts + 1, - next_retry_at: nil, last_error: nil, claimed_at: nil) + @event.with_lock do + @event.update!(status: :succeeded, attempts: @event.attempts + 1, + next_retry_at: nil, last_error: nil, claimed_at: nil) + end @event end + # El intento falló, pero el evento pudo cambiar de manos mientras corría: un + # operador lo descartó (o lo reencoló) desde la API. Esa decisión gana. Antes + # se escribía encima sin releer, el evento volvía a `pending` con un + # `next_retry_at` y el barrido lo seguía reintentando aunque lo hubieran + # descartado (hallazgo de la auditoría de TESIS-89). + # + # `with_lock` relee la fila con FOR UPDATE: la decisión del operador y este + # resultado no se pueden cruzar a mitad de camino. def mark_failed(error) - @event.attempts += 1 - exhausted = @event.attempts_exhausted? + @event.with_lock do + next unless @event.processing? - @event.update!( - status: exhausted ? :dead : :pending, - next_retry_at: exhausted ? nil : FailedEvent.next_retry_at(@event.attempts), - claimed_at: nil, - **failure_details(error) - ) + @event.attempts += 1 + exhausted = @event.attempts_exhausted? + @event.update!( + status: exhausted ? :dead : :pending, + next_retry_at: exhausted ? nil : FailedEvent.next_retry_at(@event.attempts), + claimed_at: nil, + **failure_details(error) + ) + end @event end end diff --git a/spec/poros/webhooks/retry_failed_event_spec.rb b/spec/poros/webhooks/retry_failed_event_spec.rb index 91b5c96e..7c6acada 100644 --- a/spec/poros/webhooks/retry_failed_event_spec.rb +++ b/spec/poros/webhooks/retry_failed_event_spec.rb @@ -54,6 +54,35 @@ def retry_event = described_class.new(failed_event: event).call end end + # Hallazgo de auditoría (TESIS-89): el worker cargó el evento antes de que un + # operador lo descartara, y al terminar escribía su resultado encima. El + # evento volvía a `pending` y el barrido lo seguía reintentando. + context 'when an operator discards the event while the retry runs' do + def discard_meanwhile + worker = described_class.new(failed_event: event) + Webhooks::DiscardFailedEvent.new(failed_event: FailedEvent.find(event.id)).call + worker + end + + it 'keeps it discarded when the attempt fails', :aggregate_failures do + stub_request(:post, url).to_return(status: 503, body: 'unavailable') + + discard_meanwhile.call + + expect(event.reload).to have_attributes(status: 'discarded', next_retry_at: nil) + end + + # Ya se procesó: `succeeded` es tan terminal como `discarded` y dice la verdad. + it 'records the success when the attempt went through' do + stub_request(:post, url).to_return(status: 200, headers: { 'Content-Type' => 'application/json' }, + body: { estado: 'Entregado' }.to_json) + + discard_meanwhile.call + + expect(event.reload.status).to eq('succeeded') + end + end + context 'when the replay fails and attempts remain' do before { stub_request(:post, url).to_return(status: 503, body: 'unavailable') }