Skip to content

fix: use dynamic lockMaxRetries limit in RedisHandler#10295

Open
gr8man wants to merge 7 commits into
codeigniter4:developfrom
gr8man:fix-redis-handler-lock-retries
Open

fix: use dynamic lockMaxRetries limit in RedisHandler#10295
gr8man wants to merge 7 commits into
codeigniter4:developfrom
gr8man:fix-redis-handler-lock-retries

Conversation

@gr8man

@gr8man gr8man commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Description

In RedisHandler::lockSession(), there was a bug where a hardcoded value of 300 was used to check if the maximum lock acquisition attempts were reached:

if ($attempt === 300) {

This bypassed the actual $this->lockMaxRetries property configured by the user. As a result, if a developer configured a smaller lockAttempts limit (e.g., 3), the check would fail to trigger because 3 !== 300, causing the method to incorrectly return true (success) despite not acquiring the lock. Conversely, it would hardcode the failure point to exactly 300 attempts regardless of higher configurations.

This PR replaces the hardcoded 300 with $this->lockMaxRetries in the condition and the associated error log message, ensuring the configured limits are appropriately enforced. A test has also been added to verify that the failure condition is triggered correctly after the maximum configured attempts.

Checklist:

  • Secure coding practices
  • Tests have been added to cover this change
  • All tests pass

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Some small changes are required, as well as adding a changelog entry here: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.7.4.rst

Comment thread system/Session/Handlers/RedisHandler.php Outdated
@michalsn michalsn added the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 10, 2026

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the changelog entry.

- **API:** Fixed a bug in Transformers where the root request's ``fields`` and ``include`` query parameters leaked into nested transformers created inside ``include*()`` methods, causing incorrect field filtering, unexpected includes, or infinite recursion.
- **Database:** Fixed a bug where ``updateBatch()`` could be called after Query Builder ``where()`` conditions, even though it's not supported. In this situation, now the ``DatabaseException`` is thrown.
- **HTTP:** Fixed a bug where the User Agent library reported Safari's WebKit version instead of the browser version from the ``Version`` token.
- **Session:** Fixed a bug in ``RedisHandler`` where missing ``$lockRetryInterval`` and ``$lockMaxRetries`` properties in a custom ``Session`` configuration file could trigger an ``Undefined property`` warning or error.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- **Session:** Fixed a bug in ``RedisHandler`` where missing ``$lockRetryInterval`` and ``$lockMaxRetries`` properties in a custom ``Session`` configuration file could trigger an ``Undefined property`` warning or error.
- **Session:** Fixed a bug in ``RedisHandler`` where the configured ``$lockRetryInterval`` and ``$lockMaxRetries`` values were not respected when acquiring session locks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants