fix(redis,stream): shutdown-aware XREAD ctx + clock-epoch defence (#620 follow-up)#631
fix(redis,stream): shutdown-aware XREAD ctx + clock-epoch defence (#620 follow-up)#631
Conversation
…nce + comment fix Follow-up to PR #620 (already merged): address Gemini's round-final medium-priority review (#620 (review)), which landed after the merge. Each change ships with a regression test. Defensive cast on time.Now().UnixMilli(): - nextXAddID's "*" branch cast time.Now().UnixMilli() to uint64 directly. If the system clock is ever set before the Unix epoch (1970-01-01) UnixMilli returns a negative int64 and the cast wraps to a value near math.MaxUint64, which then makes the future-ms branch chase that pathological value forever — wedging every subsequent XADD '*' on the stream until the clock recovers. - New helper safeUnixMilliToUint64(ms int64) uint64 clamps negative values to 0; nextXAddID now goes through it. The lastMs/lastSeq monotonic guarantee then carries the stream forward via bumpStreamID. Shutdown-aware XREAD context plumbing: - xread's $-resolution context (resolveCtx) now derives from r.handlerContext() instead of context.Background(), so an in-flight resolve aborts promptly on graceful shutdown rather than surviving past the drain window. - xreadBusyPoll's per-iteration context (iterCtx) likewise reroots on r.handlerContext(). Without this, a 5-second BLOCK would ignore Close() and run to completion. The loop also short- circuits at the top of each iteration when handlerCtx is cancelled — relying on iterCtx alone would still spin at the redisBusyPollBackoff cadence because isXReadIterCtxError silently translates the per-iteration cancel into "empty iteration". - Net effect: XREAD ... BLOCK <duration> on a server in shutdown returns the standard null-on-timeout reply within ~one redisBusyPollBackoff (~10 ms) instead of waiting up to <duration>. Comment alignment: - resolveXReadDollarID's doc comment said "legacy blobs fall back to a full load", but PR #620 deliberately removed that fallback in favour of the "discard-on-read, delete-on-write" contract. The comment is now consistent with what dollarIDFromState actually does. Tests: - TestSafeUnixMilliToUint64 covers the clamp at 0, MaxInt64 pass-through, and the three negative cases (-1, large negative, math.MinInt64). - TestRedis_StreamXReadShutdownShortCircuits seeds a stream, fires XREAD ... BLOCK 5s in a goroutine, calls redisServer.Close() ~50 ms later, and asserts the XREAD returns redis.Nil within 2 s. Pre-fix the test would block for the full 5 s; post-fix it unblocks on the next iteration boundary. Build / vet / lint clean.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of Redis stream operations by handling edge cases in system clock values and ensuring graceful shutdown. It introduces a safe conversion for Unix milliseconds to prevent pathological stream ID generation when the system clock is set before the Unix epoch. Additionally, it updates XREAD busy-polling to use the server's handler context, allowing blocking reads to abort promptly during shutdown. Feedback was provided to ensure that auto-generated IDs do not result in the forbidden 0-0 value when the clock is at epoch zero.
| nowMs := safeUnixMilliToUint64(time.Now().UnixMilli()) | ||
| if !hasLast || nowMs > lastMs { | ||
| return strconv.FormatUint(nowMs, 10) + "-0", nil | ||
| } |
There was a problem hiding this comment.
The auto-generated ID logic can produce 0-0 if the system clock is at or before the Unix epoch (clamped to 0 by safeUnixMilliToUint64) and the stream is empty. Redis explicitly forbids 0-0 as a stream ID. When nowMs is 0 and the stream has no previous entries, the sequence should start at 1 to ensure the ID is greater than 0-0 (i.e., 0-1).
| nowMs := safeUnixMilliToUint64(time.Now().UnixMilli()) | |
| if !hasLast || nowMs > lastMs { | |
| return strconv.FormatUint(nowMs, 10) + "-0", nil | |
| } | |
| nowMs := safeUnixMilliToUint64(time.Now().UnixMilli()) | |
| if !hasLast || nowMs > lastMs { | |
| // Redis forbids 0-0; if the clock is at epoch 0, start at 0-1. | |
| seq := "0" | |
| if nowMs == 0 { | |
| seq = "1" | |
| } | |
| return strconv.FormatUint(nowMs, 10) + "-" + seq, nil | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a969c3d09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !hasLast || nowMs > lastMs { | ||
| return strconv.FormatUint(nowMs, 10) + "-0", nil |
There was a problem hiding this comment.
XADD '*' from emitting stream ID 0-0
Clamping negative wall-clock milliseconds to 0 means the !hasLast path now returns "0-0" when the system clock is pre-epoch. This conflicts with the same function’s invariant that IDs <= 0-0 are invalid/unreachable via XREAD ... 0, so the first auto-generated entry can become invisible to normal consumers in exactly the edge case this patch targets. In this branch, ensure the generated ID is strictly greater than 0-0 (for example by bumping to 0-1 when nowMs == 0).
Useful? React with 👍 / 👎.
Gemini medium + Codex P2 round 1 on PR #631: the original change clamped a pre-epoch clock to nowMs=0 via safeUnixMilliToUint64, but on a fresh stream the auto-ID branch then returned "0-0" — the very ID the same function rejects as invalid for explicitly-requested IDs and which XREAD ... 0 treats as the empty after-marker. Fix: extract the auto-ID branch into autoXAddID(nowMs, hasLast, ...) so it can be unit-tested without the wall clock; bump seq to 1 when nowMs is 0 so the first auto-generated entry is "0-1" rather than "0-0". The existing bumpStreamID-on-collision path is unchanged. Tests: - TestAutoXAddID covers the fresh-stream branch (sane clock, clamped clock → 0-1), the same-ms collision (bump to seq+1), the clock-behind-lastMs case (carry from lastMs/lastSeq, not nowMs), the seq-at-MaxUint64 carry to ms+1, and the ID-space-exhausted error. Build / vet / lint clean.
|
Addressed Gemini medium + Codex P2 (両方とも同じ問題) in f726981. 指摘: 修正: auto-ID ブランチを
Build / vet / lint clean. /gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the reliability of Redis stream operations by handling edge cases related to system clock inaccuracies and server shutdown. It introduces autoXAddID to prevent invalid "0-0" stream IDs and safeUnixMilliToUint64 to clamp negative timestamps. Furthermore, it updates the XREAD implementation to use the server's handler context, ensuring that blocking operations abort promptly during shutdown rather than waiting for timeouts. I have no feedback to provide.
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Follow-up to merged PR #620, addressing the Gemini medium-priority review that landed after the merge. Three changes, all with regression tests:
safeUnixMilliToUint64defendsnextXAddID's*branch against a system clock set before the Unix epoch (a negativeUnixMilliwould wrap to amath.MaxUint64-adjacent value and wedge every subsequentXADD '*'chasing that pathological future-ms).xread's$-resolve andxreadBusyPoll's per-iteration ctx now derive fromr.handlerContext()instead ofcontext.Background(), so a serverClose()aborts in-flight BLOCK XREADs within ~oneredisBusyPollBackoff(10 ms) instead of running until the BLOCK deadline. The busy-poll loop also short-circuits at the top of each iteration whenhandlerCtxis cancelled — necessary becauseisXReadIterCtxErrorwould otherwise silently translate the per-iteration cancel into "empty iteration" and let the loop spin at backoff cadence.resolveXReadDollarID's doc comment said "legacy blobs fall back to a full load"; PR perf(redis): stream entry-per-key layout for O(new) XREAD #620 deliberately removed that fallback in favour of the "discard-on-read, delete-on-write" contract. Comment now matches the implementation.Test plan
TestSafeUnixMilliToUint64(unit) — clamp-at-0 for negatives (-1, large negative,MinInt64); pass-through for0, positive,MaxInt64.TestRedis_StreamXReadShutdownShortCircuits(e2e) —XREAD ... BLOCK 5sin a goroutine,redisServer.Close()after 50 ms, assert reply isredis.Nilwithin 2 s.TestRedis_StreamXReadShortBlockReturnsNullNotErrorandTestRedis_StreamXReadIterCtxDeadlineReturnsNullstill pass — the shutdown short-circuit doesn't perturb the existing BLOCK-timeout-returns-null path./gemini review
@codex review