[renechoi]

What

Fixes the sporadic touch failure reported in #4591, and the same assertion in the sibling copies of that test.

Why

The reported failure is:

SentinelAllKindOfValuesCommandsIT.touch()[2] -- Time elapsed: 0.012 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <0> but was: <1>
	at redis.clients.jedis.commands.unified.AllKindOfValuesCommandsTestBase.touch(AllKindOfValuesCommandsTestBase.java:399)

Line 399 is the OBJECT IDLETIME read immediately after TOUCH:

assertEquals(1, jedis.touch(bfoo1));
assertEquals(0L, jedis.objectIdletime(bfoo1).longValue());

OBJECT IDLETIME is derived from the LRU clock, whose resolution is one second. TOUCH stamps the key with the current clock value and the read then subtracts two second-granular values, so when the clock ticks between the two commands the result is 1 rather than 0. How closely the commands are issued does not matter — only whether a tick falls between them. The client is behaving correctly here; the assertion is simply stricter than what the command guarantees.

Reproduced against the 8.8 test environment by repeating that same back-to-back TOUCH / OBJECT IDLETIME pair, with nothing between the two calls, until one pair straddled a tick:

REPRO_HIT after 1542 iterations: objectIdletime=1
REPRO_HIT after 2654 iterations: objectIdletime=1

which is the value the assertion sees on a CI failure.

Changes

Assert <= 1 instead of == 0. The reported failure is the binary half of the unified test, but the same assertion exists in four other places, so all five are updated:

• commands/unified/AllKindOfValuesCommandsTestBase#touch — String and binary
• commands/unified/cluster/ClusterAllKindOfValuesCommandsTest#touch — overrides the base test
• commands/jedis/AllKindOfValuesCommandsTest#touch — String and binary in the legacy copy

The tolerant form matches what ObjectCommandsTest#objectIdletime already does for the same command (assertThat(time, lessThanOrEqualTo(10L))).

The surrounding assertions are untouched, so TOUCH return values are still checked exactly, and the idle time is still asserted to be at (or next to) zero rather than merely non-negative.

Verification

make start version=8.8, then the four classes that run this test, in full rather than just the one method:

Class	Per-run result
SentinelAllKindOfValuesCommandsIT	45 run, 0 failures, 3 skipped
ClusterAllKindOfValuesCommandsTest	45 run, 0 failures, 3 skipped
RedisClientAllKindOfValuesCommandsTest	45 run, 0 failures, 3 skipped
AllKindOfValuesCommandsTest (legacy)	54 run, 0 failures, 3 skipped

432 run, 0 failures, 0 errors, 27 skipped in total across the RESP parameterizations, BUILD SUCCESS.

One note

dumpAndRestore has the same shape a bit further down — RESTORE ... IDLETIME 1000 followed by assertEquals(1000, objectIdletime(...)) — in both the unified base and the legacy copy. It is exposed to the same tick, though I have not seen it reported. I left it alone to keep this focused on #4591; happy to fold it in here or send it separately, whichever you prefer.

Fixes #4591

---

Note

Low Risk
Test-only assertion and import changes; no library or runtime behavior is modified.

Overview
Fixes intermittent CI failures in touch integration tests (#4591) by aligning assertions with Redis’s one-second LRU clock for OBJECT IDLETIME.

After TOUCH, tests no longer require idle time to be exactly 0; they assert <= 1 via Hamcrest lessThanOrEqualTo, so a clock tick between TOUCH and OBJECT IDLETIME does not fail the run. The same change is applied across the unified base, cluster override, and legacy AllKindOfValuesCommandsTest (string and binary keys).

The legacy jedis test class also switches several Hamcrest matchers to static imports (e.g. hasItem, greaterThanOrEqualTo) instead of Matchers.*.

^{Reviewed by Cursor Bugbot for commit 5008f32. Bugbot is set up for automated code reviews on this repo. Configure here.}

[kdelay]

I looked at this from the completeness and coverage angle, and checked the OBJECT IDLETIME behaviour locally against Redis 8.0.2.

The root cause description matches what the server does: OBJECT IDLETIME is derived from the cached LRU clock (server.lruclock, refreshed in serverCron) at a one second resolution, so a tick landing between TOUCH and the read reports 1. lessThanOrEqualTo(1L) is also the tightest bound that stays deterministic here, and it is consistent with the range-matcher style already used by ObjectCommandsTest#objectIdletime and CommandObjectsServerManagementCommandsTest#testObjectIdletime.

Completeness: I went through every objectIdletime usage in the test tree, and all five brittle exact-zero assertions are covered here.

• commands/jedis/AllKindOfValuesCommandsTest#touch (String + binary)
• commands/unified/AllKindOfValuesCommandsTestBase#touch (String + binary)
• commands/unified/cluster/ClusterAllKindOfValuesCommandsTest#touch (String only; that override has no binary section)

The remaining callers (ObjectCommandsTest, CommandObjectsServerManagementCommandsTest, and the mocked tests) already assert ranges or mock the reply, so nothing equivalent is left behind.

One observation that is orthogonal to the flake, but relevant to what the test claims to verify: the assertion does not distinguish a touched key from an untouched one. Each key is SET immediately before, so its idle time is already 0 by the time TOUCH runs. Measured on 8.0.2, SET foo1 bar1 followed by OBJECT IDLETIME foo1 as two separate round trips returned 0 in 3000/3000 iterations, so the assertion passes just as well with the touch() call deleted. That was equally true of the old assertEquals(0L, ...), so this PR loses no coverage. It only means touch() exercises the reply value of TOUCH but not its effect on idle time.

Covering the effect would need a real idle window before the TOUCH. The cheapest deterministic shape I could confirm on 8.0.2:

SET foo1 bar1
(wait ~2s)
OBJECT IDLETIME foo1   -> 2     (assert >= 1)
TOUCH foo1
OBJECT IDLETIME foo1   -> 0     (assert <= 1)

The cost is a two second sleep in the suite, which may not be a trade worth making in this test. Would you prefer to keep touch() about the reply values only and cover the reset separately in ObjectCommandsTest, or is a sleep acceptable here?

[renechoi]

Thanks, that completeness pass matches what I found: the same five exact-zero assertions, and the remaining objectIdletime callers already assert ranges or mock the reply.

On the second point, I agree with the observation and would rather not act on it in this PR.

You are right that the assertion cannot tell a touched key from an untouched one. It is visible in the test without needing a server: SET foo1 bar1 is two lines above, so the key's idle time is already 0 when TOUCH runs, and the old assertEquals(0L, ...) had exactly the same blind spot. So this PR neither adds nor removes coverage there, which is the property I care about for a flake fix.

What makes me want to keep it out of here is the cost, which multiplies more than it looks. The assertions sit in parameterized classes, so a 2 s sleep at each of the five sites is executed 16 times in a full build:

• unified/AllKindOfValuesCommandsTestBase#touch (String + binary) is inherited by RedisClientAllKindOfValuesCommandsTest and SentinelAllKindOfValuesCommandsIT, 2 sites x 2 classes x 2 RESP versions = 8
• unified/cluster/ClusterAllKindOfValuesCommandsTest#touch overrides it and has the String section only, 1 x 2 = 2
• commands/jedis/AllKindOfValuesCommandsTest#touch (String + binary) runs over jedisRespVersions, 2 x 3 = 6

That is a little over half a minute of pure waiting added to the suite to assert one property, and it buys a test whose determinism depends on a timing margin, which is the category of thing this PR is trying to remove.

If the reset behaviour is worth covering, I would rather it be its own test that pays the sleep once, and I do not think it can extend an existing one: ObjectCommandsTest#objectIdletime only pushes a key and asserts lessThanOrEqualTo(10L), so it does not cover the reset either. There is also no way to age a key without real wall-clock time; DEBUG SLEEP blocks the server thread, which is what advances lruclock in serverCron, so it does not help.

So my preference is to keep touch() about the TOUCH reply values, land the flake fix for #4591 as is, and leave the reset to a separate test if the maintainers want it. Happy to be overruled if they would rather see it here.

[ggivo]

@renechoi
Thanks for digging into it. The fix looks good, and will merge it once we have a green pipeline

So my preference is to keep touch() about the TOUCH reply values, land the flake fix for #4591 as is, and leave the reset to a separate test if the maintainers want it. Happy to be overruled if they would rather see it here.

@renechoi @kdelay 2s per test run is an expensive one, and I think this is better validated in server-side tests. From a client perspective, validating the response of touch/objectidletime should be sufficient

@renechoi @kdelay Thanks again for the contribution!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5008f32fdd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Raise the idle-time tolerance above one second

For Redis-backed runs where the JVM or test host is paused for more than a second after TOUCH (for example under GC or loaded CI/sentinel/cluster scheduling), OBJECT IDLETIME can legitimately advance to 2+ before this read, so this still leaves the flake in place; ObjectCommandsTest.objectIdletime already allows up to 10 seconds for the same command. Please use that wider bounded tolerance for these updated TOUCH assertions rather than <= 1.

Useful? React with 👍 / 👎.

[renechoi]

Thanks, that settles the sleep question for me.

On the automated suggestion above, to widen the bound from 1 s to 10 s: I would rather keep lessThanOrEqualTo(1L) here, but I am happy to widen it if you prefer.

1. The change as written is strictly monotone against the assertion it replaces. Every run that passes assertEquals(0L, ...) today also passes lessThanOrEqualTo(1L), so it can only remove failures, never add them. The case the suggestion describes, a multi-second stall between the TOUCH and the read, already fails on master, so <= 1 does not leave that behind. It just does not widen the bound to cover it.

2. The failure in test: Sporadic test failures AllKindOfValuesCommandsTestBase.touch:399 — expected <0> but was <1> #4591 is the one second LRU tick, not a stall. Reproducing it without any artificial delay, by issuing the same TOUCH / OBJECT IDLETIME pair in a loop, the first non-zero value came out as 1, at iteration 1542 and 2654 of two independent runs. <= 1 is the tightest bound that still covers a tick landing between the two commands.

3. ObjectCommandsTest#objectIdletime is doing a different job. It checks that the command returns a plausible number after an lpush, and nothing there depends on the value being small. In touch() the key is SET two lines above the TOUCH, so its idle time is near zero regardless, and a 10 s bound would accept essentially any reply a healthy server can produce.

If a value above 1 ever does turn up in CI, widening is a one-line follow-up with an actual data point behind it. Say the word if you would rather not revisit it and I will change it now.

[ggivo]

Thanks, that settles the sleep question for me.

On the automated suggestion above, to widen the bound from 1 s to 10 s: I would rather keep lessThanOrEqualTo(1L) here, but I am happy to widen it if you prefer.

Let's keep it to 1

[renechoi]

Thanks for the quick review and the merge, and for the import cleanup on the branch.