Skip to content

fix: correct bookmark queue pagination after deletions#7719

Open
cristiandolf wants to merge 1 commit into
elsa-workflows:release/3.6.1from
cristiandolf:fix/bookmark-queue-pagination-3.6.1
Open

fix: correct bookmark queue pagination after deletions#7719
cristiandolf wants to merge 1 commit into
elsa-workflows:release/3.6.1from
cristiandolf:fix/bookmark-queue-pagination-3.6.1

Conversation

@cristiandolf

Copy link
Copy Markdown
Contributor

Summary

Fixes incorrect offset advancement in BookmarkQueueProcessor.ProcessAsync when queue items are deleted during pagination.

The processor paginates with an increasing offset but removes successfully processed items from the store. Incrementing the offset by the full batch size (offset += batchSize) causes remaining items to be skipped within the same processing run.

This bug is present across released 3.x versions that use the current pagination loop (including 3.6.x and 3.7.x). I verified it is still present on 3.8.x as well.

Fix

Advance the offset by the number of non-deleted items in each page:

offset += page.Items.Count - deletedCount

Behavior:

  • If all items in a page are successfully removed, offset does not advance (the next page is read from the same position after the queue shrinks).
  • If some items remain (no matching bookmark yet, or retryable failure), offset advances only past those remaining items so they are not retried for the remainder of the current run; they are picked up again on the next worker cycle.

Version notes for maintainers

3.6.x / 3.7.x

This PR's approach applies directly. deletedCount should count items removed via successful resume (DeleteAsync after responses.Count > 0).

3.8.x

The same pagination bug exists, but ProcessItemAsync has additional paths:

  • retryable failures update the item (SaveAsync) — not a deletion
  • exhausted failures move to dead-letter and then DeleteAsync

For 3.8.x, deletedCount must include deletions from both successful resume and dead-letter, while retryable failures and "no matching bookmark" items should advance the offset (remain in queue).

I can provide a follow-up commit/PR targeting release/3.8.0 / main with the dead-letter-aware variant if preferred.

Tests

Added unit tests in BookmarkQueueProcessorTests:

  • drains queues larger than batch size when all items resume successfully
  • processes subsequent pages when some items in the first page fail
  • retries failed items on the next ProcessAsync invocation
  • (optional) full-page failure scenario

Tests fail against the previous offset += batchSize implementation.

Related

Offset pagination + in-loop deletion anti-pattern. DefaultBookmarkQueuePurger on newer branches already uses FromPage(0, ...); BookmarkQueueProcessor was not updated similarly.

Advance the page offset by the number of non-deleted items per batch instead of always incrementing by batch size. Deleting processed items while paginating with a fixed offset caused remaining queue entries to be skipped within the same processing run. Failed items are skipped for the remainder of the run and retried on the next worker cycle.
@greptile-apps

greptile-apps Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

PR author is not in the allowed authors list.

@cristiandolf

Copy link
Copy Markdown
Contributor Author

Hi @sfmskywalker , I noticed that v3.6.3 and v3.7.1 were recently released. Just wanted to check if there's anything else needed from my end on this PR, or if we can plan to target it for the next patch release? Thanks!

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.

1 participant