[techmahedy]

Pull Request Checklist

Q	A
Branch?	4.x
Bug fix?	yes
New feature?	yes
Deprecations?	yes
Issues	-
License	MIT

Description

Replaces the native PHP session integration (session_start() on every request, $_SESSION by reference) with a request-scoped, lazy session backed by pluggable stores. This resolves the session problems raised in an external review of the framework and fixes two related defects found while verifying the change.

Problem

An independent review reported the following, and each was traced to the code:

Observation	Cause
A new session file for every cookieless request (about 34,000 files per 20 second run)	The session started in a core provider on every request, and the CSRF token and regeneration timestamp were written immediately, so a session was never empty
Files were never removed (about 83,000 after three runs, with throughput falling from about 1,700 to about 1,100 req/s)	gc_probability was never set, so garbage collection was disabled by the PHP default
Concurrent requests waited on each other	PHP's file handler holds an exclusive lock for the whole request, and session_write_close() was never called
The cookie driver still created server-side files	session_set_save_handler() was never called, so PHP fell back to its default file handler
An invalid driver name exposed file paths with debug off	The exception was thrown during provider registration, before the error renderer could run
No way to disable sessions for API routes	The session started before routing, with no route awareness

What changed

Session core

• Support\Session is lazy. Creating it reads nothing, and the stored session is looked up on first use. A session with no data is never persisted, and no cookie is sent for it.
• Sessions are saved by Application::dispatch() just before the response is sent, instead of at PHP shutdown.
• Session IDs are 40 character lowercase hex strings. Anything else coming from a cookie or setId() is ignored.
• csrf_token() and token() create the token on first use.

Stores (Phaseolies\Session\Stores, contract: SessionStoreInterface)

• FileStore: 256 shard directories by ID prefix, atomic temp-file-and-rename writes, 0600 files, and a bounded sweep.
• CookieStore: AES-256-GCM through the existing Encryption service. It rejects the legacy unauthenticated format, caps the cookie at 4,000 bytes, and names the largest keys when the limit is exceeded.
• ArrayStore: in-memory, for tests.
• SessionManager::extend() registers custom drivers.

Opting out and cleanup

• New stateless config (path patterns such as api/*) and StatelessSession middleware. A stateless request keeps a working in-memory session, stores nothing, and sends no cookie.
• New lottery config for a bounded, probabilistic sweep of expired sessions.
• New session:gc command. session:clear now delegates to the active store.

Cross-platform (Linux, macOS, Windows)

• Directory iteration replaces glob(), so storage paths containing [ ] { } * ? work.
• No read locks. Locks are mandatory on Windows and are unnecessary with atomic writes.
• rename() retries and falls back to an in-place write, for transient Windows file locks.
• destroy() empties the file if it cannot be deleted, so a destroyed session can never remain valid.
• Reads never delete a file that may be mid-write.
• Cookie names are normalized, because PHP rewrites . and spaces to _ in incoming cookie names.
• clearstatcache() is called before time-based checks, for long-lived workers.

Also: the CSRF check now reads the stored token instead of creating one, so a forged request without a session no longer starts a session.

Breaking changes

• Removed ConfigSession, Session\Handlers\*, AbstractSessionHandler, and SessionHandlerInterface. Use SessionManager and SessionStoreInterface.
• Code that reads $_SESSION or calls session_start() must use $request->session(), session(), or the Session facade.
• Existing sessions are not carried over. Users are signed out once after deploying. Old sess_* files are removed by php pool session:gc.
• setId() ignores values that are not well formed session IDs.
• The cookie driver now actually stores the session in the cookie (about 4 KB total, JSON-serializable values only).

Checklist

• Tests have been added or updated
• Documentation has been updated (docs repository (4.x/session.md))
• Code follows the project coding standards
• All tests pass locally

[techmahedy]

@abdulmajidcse check now

[abdulmajidcse]

@techmahedy Nice direction with the lazy store-based sessions. A few things to fix before merge:

Blocking

1. Logout can fail (Session.php:324): expiredCookie() skips the Secure logic from makeCookie(). With same_site=none, Chrome rejects the deletion cookie, so the user stays logged in.
2. A session for every visitor (CsrfTokenMiddleware.php:133): token() on every GET makes the session non-blank, so every anonymous or crawler hit writes a session file.
3. No locking (FileStore.php:83): concurrent requests on one session overwrite each other (lost cart items, lost flashes).
4. JSON in the cookie driver (CookieStore.php:79): objects come back as arrays, and binary data throws (500).
5. Size check before encoding (CookieStore.php:85): the cookie is measured before rawurlencode, so it can still go over 4KB and get dropped.

Minor

• In the HttpException path, a failed commitSession() turns a 404 into a 500 (Application.php:1562).
• Stateless paths never load _token, so every form POST to them gets a 419.
• setId() doesn't destroy the old session.
• Public session classes are removed in 4.x with no deprecation shim.
• The session is serialized twice per commit, and the sweep runs before the response is sent.

[techmahedy]

@abdulmajidcse good catch, please check now

[abdulmajidcse]

@techmahedy Thanks for the quick fixes. The logout cookie, cookie size, JSON safety, 404 path, setId(), double serialize and post-send sweep all look good, and the full suite passes on 86817d8 (5689 tests, 0 failures).

Two blocking issues remain. I reproduced both with failing tests:

1. Concurrent list appends still lose data (mergeChanges()): lists are merged index by index, so two requests that each append to the same list write the same index.

// stored cart = ['x']; A appends 'apple', B appends 'pear', both commit
// result: ["x","pear"], so 'apple' is lost

Suggestion: when the values are lists (array_is_list()), replace them whole instead of merging.

2. A logged-out session comes back (mergeConcurrentChanges()): if a request is still running on session X when logout/invalidate() destroys X, its commit finds no record, skips the merge and writes X back, then re-sends the X cookie, so the user is logged in again.
Suggestion: inside synchronized(), if storedId was set but the record is now gone, don't write it again.

Non-blocking

• The lazy XSRF-TOKEN cookie means SPA/axios clients that never load a form page get a 419 on their first POST. They need a way to get the token, or an upgrade note.
• The shard lock gives up after 2s and saves without the lock, and nothing logs that.

[techmahedy]

@abdulmajidcse for 419 issue, we can use relaxable path and rest I will check

[abdulmajidcse]

@techmahedy Relaxable paths are fine for stateless/webhook routes. For session-authenticated SPA routes, excluding them from CSRF opens a CSRF hole, so they still need a way to get the XSRF-TOKEN (e.g. a /csrf-cookie endpoint or an "always send" option). Waiting on the two blocking ones (list merge, resurrected session) before merge.

[techmahedy]

@abdulmajidcse you are right in this case but till now doppar doesn't support spa natively.

[abdulmajidcse]

@techmahedy Friendly reminder: the two blocking issues from my last review are still open on 86817d8. I re-ran everything today. The full suite passes (5689 tests, 0 failures), but both repro tests still fail:

1. Concurrent list appends lose data (Session.php:289, mergeChanges()): stored cart = ['x'], one request appends apple and another appends pear, and the result is ["x","pear"]. Fix: replace lists whole (array_is_list()) instead of merging by index.
2. Logged-out session is resurrected (Session.php:274, mergeConcurrentChanges()): a request that started before logout writes the destroyed session back and re-sends the old cookie. Fix: inside synchronized(), if storedId was set but the record is gone, don't write it again.

Agreed that the SPA/XSRF point can wait since Doppar doesn't support SPAs natively yet. Happy to re-test as soon as you push.

[techmahedy]

@abdulmajidcse thanks, both are fixed and reproduced with failing tests first.

1. Lists: replacing a list whole would still drop apple (the second writer's list wins), so lists are now merged by what each request added and removed. That also covers two requests creating the same list at once.
2. Resurrected session: inside synchronized(), a missing record for the loaded id now skips the write and sends no cookie. destroy() takes the same lock now, so a logout can't land between the check and the write. A rotated-away id behaves the same way.

Non-blocking: the lock timeout and lock failures are logged, and always_send_xsrf_cookie is an opt-in for clients that never load a form (documented). I also made testAFailingSessionSaveDoesNotTurnAnHttpExceptionIntoAnError independent of test order.

[abdulmajidcse]

@techmahedy CI passes, but there are two session bugs that should be fixed before merge. Both come from the automatic rotation in load().

1. Logout can be undone. In mergeConcurrentChanges(), when the stored record is gone and id !== storedId, the session is saved anyway. After an automatic rotation the new id still holds the old session's data, including the logged-in user's key. So if a logout overlaps with a request that rotates the session, that request writes the login back and sends its cookie, and the user is signed in again.

2. Parallel requests split the session. When several requests load the same session after the rotation threshold, each calls regenerate(true) and gets its own new id. The first one to save destroys the old id, and the others each write a separate copy under their own id, so some changes are lost (flash messages, cart updates). A request that still sends the old cookie then finds no session and receives an expired cookie, which logs the user out.

Fixing #1 alone (dropping the write) makes #2 worse, so they need one fix. Two options:

• On automatic rotation, keep the old id for a short time (about 30–60s) with a pointer to the new one, so overlapping requests follow it.
• Do the rotation inside synchronized() and mark the old record as rotated.

Smaller items, fine for a follow-up PR:

• FileStore::ID_PATTERN uses $, so ids ending in \n pass validation. Use \z or /D.
• lottery => null or [] runs the sweep on every request instead of never.
• Please add a CHANGELOG note that the XSRF-TOKEN cookie is now sent only when the session already has a token, and that session writes are not saved when a request ends with an uncaught exception.

[techmahedy]

@abdulmajidcse thanks, both were real and come from the same root cause, so one fix covers them.

Automatic rotation no longer happens in load(). It runs at save time inside synchronized(), and the old id becomes a pointer to the new one for 60s. Overlapping requests follow it and end up on one id, and a request still sending the old cookie follows it too instead of getting an expired cookie. A missing record is never written back. A logout or destroy() now ends the whole chain, so it can't be undone by a rotation in either order. An explicit regenerate() (login) leaves no pointer, so fixation protection is unchanged.

I reproduced both with failing tests first, and also with real parallel processes: on the previous commit, 12 parallel requests split into 12 sessions (1 of 12 changes kept) and a logout was resurrected in 39 of 40 races. Now 12 of 12 are kept and there are 0 resurrections.

Also fixed: ID_PATTERN now uses \z, and lottery => null/[] disables the sweep.

[abdulmajidcse]

@techmahedy The rotation fix looks good, and CI passes on e9a49ec. The race from my last review didn't come up again. Two bugs should be fixed before merge. I reproduced both with failing tests on e9a49ec.

Blocking

1. A logout is lost if the request then throws (Application.php:1581). Only an HttpResponseException that carries a response commits the session on this path. My last review treated dropped writes as something for the CHANGELOG, but a logout shouldn't be dropped with them.

$session->forget('user');
$session->invalidate();
throw new RuntimeException('boom');
// the stored session still exists, with its data, so the old cookie still works

Authenticate.php:370 is the real logout path, so an exception after that call, such as one from a listener or a DB write, leaves the user logged in. Suggestion: on this path, still apply a pending destroy or invalidate(), even if ordinary writes are dropped.

2. A blank commit deletes a concurrent save (Session.php:288). commitBlank() destroys the stored record without the lock and merge that the non-blank path uses.

// stored: ['url.intended' => '/x']; A and B both load it
$a->forget('url.intended');
$b->put('user', 7);
$b->commit($response);
$a->commit($response);
// the record is gone, so B's login is lost, and A's response expires the cookie

Suggestion: inside synchronized(), only destroy if the record hasn't changed since this request loaded it. Otherwise merge as usual.

Non-blocking

Reproduced with failing tests:

• Session writes during a streamed response are lost (Application.php:1552). The commit runs before send(), so a put() inside a StreamedResponse callback is never saved. Fine if that's intended, but please document it.
• regenerate(true) after setId() leaves the session in use valid (Session.php:1002). previousId ??= keeps the first id, so after load A, setId(B), regenerate(), B survives with its data. Nothing in the framework calls setId(), so this only affects app code.
• setId() keeps the old id after following a rotation pointer (Session.php:1061). getId() returns the pointer's id, not the live one. It should use $record['id']. This also only affects app code.

From reading the code, not reproduced:

• Session changes in error views aren't saved (Application.php:1565). The commit runs before dispatchHttpException() renders the view, so a #csrf token first generated in a 403 page is never stored, and that form should get a 419.
• Validation still creates a session (Rule.php:62). sanitize() flashes the input on every run, even when validation passes. Cookieless requests to routes outside stateless therefore still write a session file and send a cookie, and the input stays on the server. Flashing only on failure would fix it.
• Failed session saves aren't logged (Application.php:1566, 1589). Keeping the original exception is right, but a report() or log line in those catch blocks would make a lost save visible. The lock timeout is already logged.
• Two wildcard matchers (Session.php:189). isStateless() and RequestParser::is() match paths differently, so the same pattern can behave differently for stateless and relaxable paths. One shared helper would avoid that.

The CHANGELOG note from my last review isn't in this PR yet, unless you plan to add it at release. Happy to re-test as soon as you push.

[techmahedy]

@abdulmajidcse i tried to fix all the issues you mentioned, please check now

[abdulmajidcse]

@techmahedy Thank you for the fixes! I re-checked 99373d5: the logout and blank-commit bugs are fixed, tests and CI pass, and parallel-request runs held up. This looks ready to merge from my side, so I will leave the merge to you.

[techmahedy]

@techmahedy Thank you for the fixes! I re-checked 99373d5: the logout and blank-commit bugs are fixed, tests and CI pass, and parallel-request runs held up. This looks ready to merge from my side, so I will leave the merge to you.

@abdulmajidcse it was a great review; thumbs up