Skip to content

Commit a3d1e2a

Browse files
thiagohoraclaude
andcommitted
fix: reset the claim cursor whenever the consumer group is recreated
recoverFromNoGroup is reached from both readMessages and claimPendingMessages, but the cursor reset sat at the claim call site only. The read path is the likelier of the two to notice NOGROUP first, since reads run on every tick that is not a claim tick -- so the common case left the cursor pointing into the pending list of a group that no longer exists. Not self-correcting on a busy stream. A scan starting above the recreated PEL's entries only wraps once it exhausts the list, and entries arriving after the stale position keep giving it work at the high end, so the wrap can be deferred indefinitely while the oldest entries go unexamined -- the exact starvation this PR exists to remove. Reset moved into recoverFromNoGroup so both paths get identical treatment and there is one place that owns it. Regression test covers the read path specifically. Mutation-checked: moving the reset back to the claim site alone fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 5b04ebf commit a3d1e2a

2 files changed

Lines changed: 64 additions & 3 deletions

File tree

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/BaseRedisSubscriber.java

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -434,10 +434,9 @@ private Mono<Map<StreamMessageId, Map<String, M>>> claimPendingMessages() {
434434
claimErrors.add(1);
435435
log.error("Error claiming pending messages", throwable);
436436
// A failed scan leaves the cursor where it was, so the next attempt retries the same
437-
// window rather than skipping it. Except on NOGROUP: the group is being recreated, so
438-
// any cursor into the old group's PEL is meaningless.
437+
// window rather than skipping it. The NOGROUP case is different, but recoverFromNoGroup
438+
// resets the cursor itself so both it and the read path get the same treatment.
439439
if (isNoGroupError(throwable)) {
440-
claimCursor = StreamMessageId.MIN;
441440
return recoverFromNoGroup();
442441
}
443442
return Mono.just(Map.of());
@@ -507,6 +506,16 @@ private boolean isNoGroupError(Throwable throwable) {
507506
private Mono<Map<StreamMessageId, Map<String, M>>> recoverFromNoGroup() {
508507
log.warn("Recreating not found consumer group '{}' for stream '{}'",
509508
config.getConsumerGroupName(), config.getStreamName());
509+
// The cursor indexes the OLD group's pending list, so it means nothing once the group is
510+
// recreated. Reset here rather than at the call sites: NOGROUP surfaces from both readMessages
511+
// and claimPendingMessages, and the read path is the likelier of the two to notice it first
512+
// (reads run on every tick that is not a claim tick).
513+
//
514+
// Leaving it stale is not self-correcting on a busy stream. A scan starting above the recreated
515+
// PEL's entries only wraps once it exhausts the list, and entries arriving after the stale
516+
// position keep giving it work at the high end -- so the wrap can be deferred indefinitely while
517+
// the oldest entries, the ones this scan exists to reach, are never examined.
518+
claimCursor = StreamMessageId.MIN;
510519
return createConsumerGroup()
511520
.onErrorResume(throwable -> {
512521
log.error("Failed to recreate consumer group '{}' for stream '{}'",

apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/events/BaseRedisSubscriberUnitTest.java

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,58 @@ void shouldHandleInvalidMessageIdTimestamp() {
622622
@Nested
623623
class NoGroupErrorTests {
624624

625+
/**
626+
* Regression for review feedback on OPIK-8240: {@code recoverFromNoGroup} is reached from BOTH
627+
* {@code readMessages} and {@code claimPendingMessages}, and the cursor reset was originally only
628+
* at the claim call site. The read path is the likelier of the two to notice NOGROUP first, since
629+
* reads run on every tick that is not a claim tick.
630+
*
631+
* <p>A cursor left pointing into the old group's pending list is not self-correcting on a busy
632+
* stream: a scan starting above the recreated PEL's entries only wraps once it exhausts the list,
633+
* and entries arriving after the stale position keep feeding it at the high end, so the oldest
634+
* entries can go unexamined indefinitely -- the exact starvation this fix exists to remove.
635+
*/
636+
@Test
637+
void shouldResetTheClaimCursorWhenTheGroupIsRecreatedViaTheReadPath() {
638+
whenCreateGroupReturnEmpty();
639+
whenRemoveConsumerReturn();
640+
var fastConfig = CONFIG.toBuilder().claimIntervalRatio(2).build();
641+
var subscriber = trackSubscriber(TestRedisSubscriber.createSubscriber(fastConfig, redissonClient));
642+
var starts = new CopyOnWriteArrayList<StreamMessageId>();
643+
var claims = new AtomicInteger();
644+
645+
// First claim advances the cursor well past the start of the PEL.
646+
when(stream.autoClaim(
647+
eq(fastConfig.getConsumerGroupName()),
648+
anyString(),
649+
eq(fastConfig.getPendingMessageDuration().toJavaDuration().toMillis()),
650+
eq(TimeUnit.MILLISECONDS),
651+
any(StreamMessageId.class),
652+
eq(fastConfig.getConsumerBatchSize())))
653+
.thenAnswer(invocation -> {
654+
starts.add(invocation.getArgument(4));
655+
claims.incrementAndGet();
656+
return Mono.just(new AutoClaimResult<>(
657+
new StreamMessageId(9_000L, 0), Map.of(), List.of()));
658+
});
659+
// Reads then hit NOGROUP, which recreates the group underneath us.
660+
when(stream.readGroup(eq(fastConfig.getConsumerGroupName()), anyString(),
661+
any(StreamReadGroupArgs.class)))
662+
.thenAnswer(invocation -> Mono
663+
.error(new RuntimeException("NOGROUP No such key stream or consumer group")));
664+
whenAckReturn();
665+
whenRemoveReturn();
666+
667+
subscriber.start();
668+
669+
await().atMost(AWAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS).until(() -> starts.size() >= 2);
670+
671+
// The recreated group has a fresh PEL, so the next scan must start from the beginning rather
672+
// than from the position it reached in the group that no longer exists.
673+
assertThat(starts.getFirst()).isEqualTo(StreamMessageId.MIN);
674+
assertThat(starts.get(1)).isEqualTo(StreamMessageId.MIN);
675+
}
676+
625677
@Test
626678
void shouldRecoverOnClaimAndNotDie() {
627679
whenCreateGroupReturnEmpty();

0 commit comments

Comments
 (0)