Skip to content

fix(dataplane): fix batch replay oom - #2669

Open
olamilekan000 wants to merge 4 commits into
mainfrom
fix/batch-replay-oom-dos
Open

olamilekan000 wants to merge 4 commits into
mainfrom
fix/batch-replay-oom-dos

Conversation

@olamilekan000

@olamilekan000 olamilekan000 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Change fixes a batch replay OOM risk where the handler defaulted to
a 2B(2000000000) page size and loaded all matching events into memory in one request.
It caps page size at 1000, paginates through results in
BatchReplayEventService, and applies pagination middleware to batch replay
routes.


Note

Medium Risk
Changes batch-replay pagination and HTTP error semantics (409 vs retryable service error), which can affect client retry behavior and how much work is enqueued per request.

Overview
Fixes a batch-replay OOM path that defaulted to a ~2B page size and loaded every matching event in one query.

BatchReplayEventService now ignores caller page size/cursors, pages internally at 1000, and fetches the next page before enqueueing the current one so a later fetch failure does not leave that page already queued (Redis replay IDs would re-fan-out on retry).

Incomplete runs that already enqueued work return 409 with success/failure counts; zero-success failures (ownership skips, fetch errors before any enqueue) stay on the retryable service-error path. Swagger documents the new 409.

Reviewed by Cursor Bugbot for commit 215ac3b. Bugbot is set up for automated code reviews on this repo. Configure here.

@olamilekan000
olamilekan000 force-pushed the fix/batch-replay-oom-dos branch from 91e8f6c to 5c00519 Compare June 17, 2026 20:58
Comment thread services/batch_replay_event.go Outdated
Comment thread services/batch_replay_event.go Outdated
@olamilekan000
olamilekan000 force-pushed the fix/batch-replay-oom-dos branch from 5c00519 to c11a9a0 Compare June 17, 2026 21:10
Comment thread api/api.go Outdated
eventRouter.With(handler.RequireEnabledProject(), handler.RequireEnabledOrganisation()).Post("/broadcast", handler.CreateBroadcastEvent)
eventRouter.With(handler.RequireEnabledProject(), handler.RequireEnabledOrganisation()).Post("/dynamic", handler.CreateDynamicEvent)
eventRouter.With(handler.RequireEnabledProject(), handler.RequireEnabledOrganisation()).Post("/batchreplay", handler.BatchReplayEvents)
eventRouter.With(handler.RequireEnabledProject(), handler.RequireEnabledOrganisation(), middleware.Pagination).Post("/batchreplay", handler.BatchReplayEvents)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This changes batch replay semantics when the caller includes list pagination params. The dashboard filter type already carries next_page_cursor, prev_page_cursor, and direction, and batchReplayEvent() sends the saved queryParams to /events/batchreplay. With this middleware, the replay starts from that cursor while /countbatchreplayevents still counts the full filter set, so the confirmation count can say N events but only the current page/window is replayed.

For a bulk replay endpoint, we should ignore caller cursors and use pagination only internally: start from the first cursor, force direction=next, cap perPage, and loop until done. Alternatively, do not attach middleware.Pagination to this route and let BatchReplayEventService build its own internal pageable.

@olamilekan000
olamilekan000 force-pushed the fix/batch-replay-oom-dos branch from c11a9a0 to ef2a027 Compare June 19, 2026 09:16
@mekilis
mekilis force-pushed the fix/batch-replay-oom-dos branch from ef2a027 to 91397ef Compare August 23, 2026 09:16
@linear

linear Bot commented Aug 23, 2026

Copy link
Copy Markdown

PDE-766

@cursor

cursor Bot commented Aug 23, 2026

Copy link
Copy Markdown

Current version of PR was reviewed by /review-bugbot on Aug 23, 10:09 GMT+1. It flagged 0 findings.

Bugbot on commit 91397ef is skipped.

@mekilis
mekilis force-pushed the fix/batch-replay-oom-dos branch from 91397ef to 0b55638 Compare August 23, 2026 09:17
@mekilis
mekilis enabled auto-merge August 23, 2026 09:17
Comment thread api/handlers/event.go Outdated
@mekilis
mekilis force-pushed the fix/batch-replay-oom-dos branch from 0b55638 to 215ac3b Compare August 23, 2026 10:08

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 215ac3b. Configure here.

nextEvents, nextPagination, nextErr := e.EventRepo.LoadEventsPaged(ctx, e.Filter.Project.UID, &filter)
if nextErr != nil {
return e.fetchError(ctx, nextErr, successes, failures)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fetched page dropped on later failure

Medium Severity

When a later page fetch fails after earlier pages already enqueued work, the already-fetched current page in events is discarded without replayPage, then renderBatchReplayIncomplete returns 409 so clients avoid retrying. Up to one full BatchReplayPageSize window that was successfully loaded is silently omitted from the batch.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 215ac3b. Configure here.

…ull window

a two-billion-row page size exhausted memory. replay now ignores caller
cursors and pages at 1000 so the window matches the confirm count.
the next page is loaded before the current page is queued, and a
partial run returns 409 with status false so clients do not retry
already-enqueued replay jobs.
staticcheck QF1006 on the page walk. behavior is unchanged.
…ueue

ownership skips increment failures without queueing work, so a later
fetch error must stay retryable instead of blocking the remaining window.
… fails

a later page load error was dropping an already-fetched window, and 409
then blocked retry of that window.
@mekilis
mekilis force-pushed the fix/batch-replay-oom-dos branch from 215ac3b to aa3a6b7 Compare August 23, 2026 10:32
@cursor

cursor Bot commented Aug 23, 2026

Copy link
Copy Markdown

Current version of PR was reviewed by /review-bugbot on Aug 23, 11:32 GMT+1. It flagged 0 findings.

Bugbot on commit aa3a6b7 is skipped.

@mekilis
mekilis disabled auto-merge August 23, 2026 10:50
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.

2 participants