[FIXED] Last msg ID is restored only if it is in duplicate window - #8564

@claude review

Claude finished @sciascid's task in 2m 15s —— View job

Code Review

• Gather context
• Read changed files and diff
• Analyze fix logic
• Review test coverage
• Post review findings

Summary

This PR fixes a real bug: after server restart, mset.lmsgId (used to validate Nats-Expected-Last-Msg-Id headers) was only restored if the last published message was still within the deduplicate window. If the server was restarted after the window expired, the field would be left empty, causing subsequent conditional publishes to be incorrectly rejected.

Fix Analysis (server/stream.go)

Old code — lmsgId was set inside the loop body, but only when seq == state.LastSeq:

if seq == state.LastSeq {
    mset.lmsgId = msgId  // only reached if last msg is in the dedupe window
}

New code — lmsgId is set unconditionally from LoadMsg(state.LastSeq) before any window logic runs:

if state.LastSeq > 0 {
    if sm, err := mset.store.LoadMsg(state.LastSeq, &smv); err == nil {
        mset.lmsgId = getMsgId(sm.hdr)
    }
}

This is correct and minimal. The change in variable declaration placement (moving smv/state to the top) is a necessary consequence and does not affect behavior.

Additional implicit fix: Previously, when Duplicates == 0 (dedup window disabled), the function returned early before ever setting lmsgId. Now lmsgId is restored even for streams with dedup disabled, which is the right behavior since Nats-Expected-Last-Msg-Id is independent of the dedup window.

Edge case acknowledged in review: If the last sequence has no stored message — e.g. SkipMsgNoInterest, or the message was deleted/purged/expired — LoadMsg will return an error and lmsgId will remain empty. The author correctly identified this as a separate, larger problem requiring durable state for the last-seen msg ID, and this PR's scope is appropriate.

Test Analysis (server/jetstream_test.go)

TestJetStreamRestoreLastMsgIDPastDedupeWindow directly exercises the bug:

1. Creates a stream with a 100ms dedupe window
2. Publishes a message with a known msgId ("A")
3. Waits for the message to exit the window via checkFor + GetSeqFromTime
4. Shuts down and restarts the server with the same store directory
5. Sends a publish with Nats-Expected-Last-Msg-Id: A and asserts it succeeds at sequence 2

The test is well-structured and the polling approach to confirm window expiry is robust. Only FileStorage is tested (correct, since memory storage doesn't survive restarts).

Minor observation: The test doesn't cover the case where Duplicates is 0 (the implicit additional fix noted above), but that's a low-risk gap given the simplicity of that path.

Verdict

The fix is correct, focused, and well-tested. The known limitation around non-loadable last messages is an accepted trade-off with reasonable consequences (a false rejection at worst). LGTM.