Skip to content

fix(Session): release lock state on close and check session ID in MemcachedHandler - #10760

Open
banghamid wants to merge 5 commits into
codeigniter4:developfrom
banghamid:fix/memcached-session-lock
Open

banghamid wants to merge 5 commits into
codeigniter4:developfrom
banghamid:fix/memcached-session-lock

Conversation

@banghamid

Copy link
Copy Markdown
Contributor

Description
In MemcachedHandler:

  1. close() previously called $this->memcached->delete($this->lockKey) directly instead of $this->releaseLock(), leaving $this->lockKey non-null and $this->lock set to true after the lock key was deleted from Memcached. When the same MemcachedHandler instance was reused (e.g., session_write_close() followed by session_start(), or across session ID regeneration), the subsequent read() called lockSession(), saw isset($this->lockKey) === true, and called $this->memcached->replace($this->lockKey, ...) on the already-deleted key—which returned false and caused read() to fail and return ''.
  2. lockSession(string $sessionID) only checked if (isset($this->lockKey)) rather than verifying that $this->lockKey matches the lock key for $sessionID (unlike RedisHandler::lockSession()).

This PR updates MemcachedHandler::close() to release the lock via $this->releaseLock() and updates MemcachedHandler::lockSession() to check $this->lockKey === $lockKey before refreshing the existing lock TTL.

Checklist:

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

@carson-codeigniter4 carson-codeigniter4 Bot added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 11, 2026
@banghamid
banghamid marked this pull request as draft November 11, 2026 10:15
@banghamid
banghamid marked this pull request as ready for review November 11, 2026 10:15
@banghamid
banghamid marked this pull request as draft November 11, 2026 10:16
@banghamid
banghamid marked this pull request as ready for review November 11, 2026 10:18
@gr8man

gr8man commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Thanks @banghamid for the clear diagnosis and fix!

The core change is solid and follows the pattern established in RedisHandler (releaseLock() on close() and checking $this->lockKey === $lockKey in lockSession()). Resetting $this->lockKey = null and $this->lock = false correctly prevents the failed replace() attempt when reusing the handler instance.

A few suggestions regarding the tests before merging:

  1. Test Idempotency / State Leakage:
    Both testReopenAndReadWithSameHandlerInstance and testRegenerateSessionIdWithSameHandlerInstance assert $this->assertSame('', $handler->read($sessionId)) as a precondition, but write() persists the session data with a 7200s TTL and close() only removes the :lock key. Running the test suite twice against a persistent local Memcached daemon will fail on that first assertion.
    Suggestion: Use a dynamic ID per run (e.g. '666666' . bin2hex(random_bytes(8))) and/or clean up the test keys in tearDown().

  2. Session Regeneration Lifecycle:
    In real PHP session regeneration, PHP invokes write($newSessionId, $data) while the lock for $oldSessionId is still held, which triggers the if ($id !== $this->sessionID) branch in write(). In testRegenerateSessionIdWithSameHandlerInstance, close() is explicitly called before opening the new ID. Consider adding a test case that calls write($newSessionId, $data) directly without calling close() first to exercise that path.

  3. Minor Nit:
    In lockSession():

    // PHP 7 reuses the SessionHandler object on regeneration,

@banghamid

banghamid commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks @gr8man for checking this! Good points on both test cases.

Just pushed an update:

  • Switched the new tests to dynamic session IDs and added cleanup in tearDown(), so running them multiple times against a local Memcached instance won't hit leftover keys.
  • Added testRegenerateSessionIdWriteWithoutClose() to test calling write($newSessionId, $data) while the old session lock is still held (without calling close() first), covering the $this->sessionID !== $id path.

Also noticed point 3 got cut off right after the PHP 7 comment — guessing you meant dropping the 7 since CI4 is on 8.2+ now? Changed it to PHP for now, let me know if you meant something else there.

@gr8man

gr8man commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

You need to fix the tests. composer cs-fix and rector

@banghamid
banghamid force-pushed the fix/memcached-session-lock branch from 464627c to ed78e22 Compare November 11, 2026 22:46
@banghamid

Copy link
Copy Markdown
Contributor Author

I've updated the PR with the suggested changes (close() simplified, MemcachedHandlerTest cleaned up with unique session IDs in tearDown(), plus testRegenerateSessionIdWriteWithoutClose()).

All static analysis, Rector, and PHP CS Fixer checks are green on ed78e22. The only failure is phpcpd / Code Copy-Paste Detection with exit code 127 — it looks like an upstream CI issue because https://phar.phpunit.de/phpcpd-6.0.3.phar is currently returning 404/522 during setup-php, so phpcpd never gets installed on the runner.

@gr8man

gr8man commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Rerun test :
git commit --allow-empty -m "trigger ci"
git push

@banghamid

banghamid commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Re-triggered CI as suggested @gr8man — all 56 checks (including \phpcpd\) are green now on \e9e202e7\. Ready for another look whenever you get a chance!

This branch has not been deployed

No deployments
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