Skip to content

Add consumer reset API#1126

Merged
mtmk merged 5 commits into
mainfrom
consumer-reset-api
May 6, 2026
Merged

Add consumer reset API#1126
mtmk merged 5 commits into
mainfrom
consumer-reset-api

Conversation

@mtmk

@mtmk mtmk commented May 6, 2026

Copy link
Copy Markdown
Member

Add ResetConsumerAsync(stream, consumer, seq=0, ct) on INatsJSContext to call $JS.API.CONSUMER.RESET.<stream>.<consumer> (ADR-60, server v2.14+). Empty-body request rewinds to the ack floor; a non-zero seq reset sets the ack floor stream sequence one below the requested sequence. The response is a ConsumerInfo superset with reset_seq (the next deliverable stream sequence the consumer was reset to).

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 3m 18s —— View job


PR Review: Add consumer reset API

  • Gather context (CLAUDE.md, PR diff, source files)
  • Review for correctness and potential bugs
  • Review for async/await correctness and cancellation token propagation
  • Review API surface and backward compatibility
  • Review NATS protocol correctness
  • Post review summary

The implementation is clean and consistent with how PauseConsumerAsync / UnpinConsumerAsync are structured. One real concern, everything else is fine:

{} vs empty body for seq=0 (minor spec conformance issue)

ADR-60 says the ack-floor-reset path uses an empty body. When seq == 0, new ConsumerResetRequest { Seq = 0 } serializes to {} (the Seq field is suppressed by WhenWritingDefault, leaving an empty JSON object). That's not the same as 0 bytes on the wire. ResumeConsumerAsync uses request: null to achieve a true empty body. In practice the server almost certainly handles {} the same way, but aligning with the spec is straightforward — see the inline suggestion on NatsJSContext.Consumers.cs line 195. Fix this →

Everything else looks good:

  • Async/await pattern and CancellationToken propagation match the rest of the file
  • ThrowIfInvalidStreamName validation is consistent with peer methods; consumer-name validation is intentionally absent (no other method does it)
  • ConsumerResetRequest (internal) / ConsumerResetResponse (public) visibility split is correct
  • SkipIfNatsServer extends FactAttribute — the test is properly registered
  • [Range(ulong.MinValue, ulong.MaxValue)] and [JsonIgnore(Never)] annotations are consistent with the schema-derived style used throughout the Models directory
  • Serializer registration in NatsJSJsonSerializerContext is correct and in alphabetical order

Comment thread src/NATS.Client.JetStream/NatsJSContext.Consumers.cs
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown

@mtmk
mtmk requested review from scottf May 6, 2026 09:54
@mtmk mtmk added this to the 2.14 milestone May 6, 2026

@scottf scottf 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.

LGTM

@mtmk
mtmk merged commit e5aca30 into main May 6, 2026
24 checks passed
@mtmk
mtmk deleted the consumer-reset-api branch May 6, 2026 10:44
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