Skip to content

fix(events): fail closed when the enqueue fails - #6319

Merged
vincent-pochet merged 1 commit into
mainfrom
fix/ing-606-fail-closed-on-enqueue-failure
Sep 8, 2026
Merged

vincent-pochet merged 1 commit into
mainfrom
fix/ing-606-fail-closed-on-enqueue-failure

Conversation

@vincent-pochet

@vincent-pochet vincent-pochet commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Part of ING-606

Context

Events::CreateService commits the event row and only then enqueues Events::PostProcessJob. The two are not transactional, so when Redis is down the row is committed and the job never runs: no pay-in-advance fee is created, and reposting the same transaction_id answers value_already_exist forever. Events::CreateBatchService has the same shape, and one bulk push failure strands the whole batch.

Description

  • Remove the event, or the whole batch, when its post-processing cannot be enqueued, so the caller's retry succeeds instead of being refused as a duplicate.
  • Move the enqueue ahead of the Kafka produce, so a failed enqueue publishes nothing downstream.
  • Scope the ArgumentError rescue to the timestamp parse: ActiveJob::SerializationError is an ArgumentError, so an enqueue failure was reported as an invalid timestamp.

Hard-deleted rather than discarded: index_unique_transaction_id has no deleted_at predicate, so a discarded event keeps refusing the retry. Enqueuing inside the transaction is worse: the job is pushed before the commit, so every event would race its own commit instead of only the rare accepted-then-failed enqueue.

Behavior change to be aware of

  • Aggregation reads events directly, so a stranded event still billed pay-in-arrears charges correctly. Rejecting it drops that usage unless the caller retries.
  • Narrows the window rather than closing it: a process killed between commit and cleanup still strands the event.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — The enqueue-failure cleanup hard-deletes Event records via delete/delete_all, despite Event including Discard::Model and the repository rule prohibiting hard deletion of soft-deletable models. Rework both paths without bypassing the model’s deletion policy while preserving retryability, and make the specs prove the retry/physical-row behavior (Event.count is default-scoped and would also pass after a discard).

@vincent-pochet
vincent-pochet force-pushed the fix/ing-606-fail-closed-on-enqueue-failure branch from 8de00e4 to 5a6964c Compare September 7, 2026 13:13
@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — the enqueue-failure cleanup hard-deletes Event, despite Event being soft-deletable and the repository explicitly forbidding hard deletion of soft-deletable models. Rework the cleanup/idempotency design through the supported model lifecycle and a dedicated service; the non-partial uniqueness constraint means simply substituting discard is not sufficient.

Add direct job coverage proving that a queued Events::PostProcessJob whose event has disappeared is discarded without invoking post-processing; the new discard_on behavior is currently untested.

## Context

`Events::CreateService` commits the event row and only then enqueues
`Events::PostProcessJob`. The two are not transactional and the rescue clauses
cover only ActiveRecord errors, so when Redis is unavailable the row is committed
and the job never runs. No pay-in-advance fee is ever created, and the caller
cannot repair it: reposting the same transaction_id hits
`index_unique_transaction_id` and answers `value_already_exist` forever.

`Events::CreateBatchService` has the same shape with a wider blast radius, since
`perform_all_later` is a single bulk push: one failure strands the whole batch.

## Description

Removes the event when its post-processing cannot be enqueued, so the error the
caller receives is honest and their retry succeeds instead of being refused as a
duplicate. Nothing is silently accepted and then dropped.

The event is hard-deleted rather than discarded, a deliberate exception to the
usual rule for soft-deletable models: `index_unique_transaction_id` carries no
`deleted_at` predicate, so a discarded event would keep answering the caller's
retry with `value_already_exist` and leave them exactly as stuck.

The enqueue also moves ahead of the Kafka produce, so a failed enqueue leaves
nothing behind downstream either.

`ActiveJob::SerializationError` is an `ArgumentError`, so an enqueue failure fell
into the rescue written for the timestamp parse and was reported to the caller as
an invalid timestamp. That rescue is now scoped to the parse itself.

Note the trade-off: aggregation reads the `events` table directly, so a stranded
event still billed correctly for pay-in-arrears charges. Rejecting it drops that
usage unless the caller retries. This also narrows rather than closes the window,
since a process killed between the commit and the cleanup still strands the event.

Organizations on the ClickHouse events store are unaffected: they persist no row,
so a failure there already left nothing behind.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vincent-pochet
vincent-pochet force-pushed the fix/ing-606-fail-closed-on-enqueue-failure branch from 5a6964c to fce3219 Compare September 7, 2026 14:31
@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — both enqueue-failure handlers hard-delete Event, even though it includes Discard::Model; this bypasses the repository’s soft-deletion contract.

  • Rework the single and batch rollback paths so retryability is preserved without event.delete/delete_all. Also cover an enqueue accepted before an adapter error: the current immediate-raise specs do not catch a queued job left referencing a deleted event.

@vincent-pochet vincent-pochet changed the title fix(events): fail closed when the enqueue fails fix(events): roll the event back when the enqueue fails Sep 7, 2026
@vincent-pochet
vincent-pochet force-pushed the fix/ing-606-fail-closed-on-enqueue-failure branch from fce3219 to 01166eb Compare September 7, 2026 15:03
@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: PASS · CI green

PASS — The transaction/enqueue ordering is consistent across both PostgreSQL ingestion paths while preserving ClickHouse behavior. The focused specs cover rollback and retryability on enqueue failure, Kafka ordering, serialization errors, and the pre-commit deserialization race.

@vincent-pochet
vincent-pochet force-pushed the fix/ing-606-fail-closed-on-enqueue-failure branch from 01166eb to fce3219 Compare September 7, 2026 15:31
@vincent-pochet vincent-pochet changed the title fix(events): roll the event back when the enqueue fails fix(events): fail closed when the enqueue fails Sep 7, 2026

@toommz toommz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch 👏

@vincent-pochet
vincent-pochet merged commit 1c9f290 into main Sep 8, 2026
24 checks passed
@vincent-pochet
vincent-pochet deleted the fix/ing-606-fail-closed-on-enqueue-failure branch September 8, 2026 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants