[gr8man]

Description
In both RedisHandler::save() and PredisHandler::save(), saving a cache item with a TTL previously executed two sequential network round trips:

1. hMset (hmset)
2. expireAt (expireat)

This PR executes both commands atomically in a single network round trip using a small, self-contained Lua script (EVAL):

• Atomicity & Type Safety: Uses redis.pcall('HMSET', ...) inside Lua. If the existing key is not a hash (WRONGTYPE), the script immediately returns 0 without executing EXPIREAT, leaving the existing key's value and TTL intact.
• Fail-safe Fallback: If EVAL is blocked or unsupported in a specific environment, a try/catch block seamlessly falls back to sequential execution (hMset followed by expireAt).
• Regression Tests: Includes regression coverage for non-hash key preservation and for the EVAL-denied fallback path in both handlers.

Benchmark (5,000 save() operations, local Redis instance)

• Sequential (2 round trips: hMset + expireAt): 0.549 s (~0.110 ms per save)
• Atomic Lua script (1 round trip: EVAL): 0.424 s (~0.085 ms per save)
• Local speedup: 1.30x (22.8% latency reduction). Over distributed/cloud networks with non-zero ping, eliminating the second round trip yields an estimated ~50% network latency reduction per save.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[Copilot]

🟡 Changes recommended

Redis and Predis saves can apply expiration even when HMSET fails, changing an existing key’s TTL or potentially deleting it.

1 open issue

• Prevent expiration when a cache write fails · New

What changed in this PR

This PR reduces network round trips when saving expiring cache entries through CodeIgniter’s Redis and Predis handlers.

Changes:

• Pipelines HMSET and EXPIREAT for nonzero TTLs.
• Retains the existing hash format and single-command path for zero TTLs.

Compatibility was assessed under the stricter develop policy because the base branch was unspecified. Runtime validation was left to CI.

File	Description
system/​Cache/​Handlers/​RedisHandler.php	Uses a phpredis pipeline for writes with expiration and checks the write result.
system/​Cache/​Handlers/​PredisHandler.php	Uses a Predis pipeline for writes with expiration and checks the returned status.

_{Review effort: Balanced}

[paulbalandan]

Please update the title and description to reflect the Lua implementation. The current description still describes pipelining and says Lua was deliberately avoided. Please also rerun the benchmark against the final implementation; the pipeline measurements do not establish the performance of this version.

Please add regression coverage for the EVAL-denied fallback in both handlers.

Lastly, please also consider whether using cached EVALs makes sense here (i.e. EVALSHA).

[gr8man]

Thanks @paulbalandan for the review and the detailed pointers!

1. Title & description updated

The PR title and description now reflect the actual implementation (atomic Lua script with EXPIREAT), replacing the outdated pipelining narrative.

2. Benchmark rerun against the final implementation

Measured on 5,000 save() operations against a local Redis instance:

Variant	Total	Per operation
Sequential (hMset + expireAt, 2 RTT)	0.549 s	~0.110 ms
Atomic Lua (EVAL, 1 RTT)	0.424 s	~0.085 ms

Local speedup: 1.30x (22.8% latency reduction). On distributed deployments where Redis sits on a separate host, the saved round trip translates to roughly a 50% reduction of network latency per save.

3. Regression coverage for the EVAL-denied fallback

Added tests in both handlers that replace the client with one whose eval() throws, then assert that save() still returns true, the value is stored, and the handler remains usable:

4. Re: EVALSHA

I considered it, but I would keep plain EVAL here:

• The script is only ~160 bytes, so it fits in a single TCP frame; the payload saving is negligible compared to an extra round trip.
• EVALSHA requires handling the NOSCRIPT error and retrying, and the exception shape differs between phpredis and predis. That adds a second network round trip on every cache flush/restart, plus cache-management complexity on the client side.
• The main benefit of the change (one round trip instead of two) is already achieved by EVAL; EVALSHA would only shave off a constant-sized payload, not a network round trip.

Happy to revisit EVALSHA if you'd prefer it for very large deployments, but I'd avoid the added failure modes in the framework itself.

[michalsn]

When EVAL is disabled via rename-command, phpredis returns false rather than throwing, so the fallback is skipped. Could you handle this case and add a regression test where the mocked eval() returns false? Please distinguish it from integer 0, which indicates a failed HMSET inside the script.

[michalsn]

This changes Predis error handling: a WRONGTYPE error previously raised a ServerException, but now returns false for non-zero TTLs. TTL 0 still throws. Please preserve the existing exception behavior.