Make session persistence lifecycle-safe and Laravel-aligned - #456
Merged
Conversation
Record the settled Session design, upstream research, owner decisions, rejected alternatives, implementation boundaries, and complete regression matrix. Keep the package-specific recovery and anti-overengineering rules beside the implementation plan so long-running work can restore its full context without rereading the framework-wide audit history.
Give each Store instance its own coroutine-scoped identity, attributes, and started slots while keeping the active request store on the established fixed context key. Synchronize only that active identity through the HTTP testing bridge and restore the non-null Laravel session ID contract. Make JSON error-bag marshalling and flash aging pure, write serialized snapshots before publishing live state, and reject false handler writes without corrupting retry state. Cover sibling stores, reused object IDs, copied request contexts, serialization failures, cache write failures, encrypted stores, and consecutive JSON saves.
Use the request session as the startup commit flag so failed reads cannot schedule an empty after-response write. Preserve the primary rendered failure when a retry also fails, while retaining persistence after route and render failures. Delegate blocked-route acquisition and cleanup to Cache lock callback ownership so timeout paths never release an unacquired lock and request failures remain primary over release failures. Add focused middleware and Testbench-backed persistence regressions for every failure boundary.
Replace the JSON transport envelope with private PHP serialization so arbitrary serialized session bytes round-trip without UTF-8 loss. Decode untrusted cookies with classes disabled and accept only the exact string payload and integer expiry shape. Cover binary data, expiration, malformed frames, object rejection, strict field types, real CookieJar transport, coroutine isolation, and current test typing without adding a compatibility decoder or a second framing layer.
Require Filesystem writes to return the complete byte count before reporting a successful session save. Use Finder pathnames directly during garbage collection and count only files that were actually deleted. Align the concrete Filesystem delete signature with its existing contract and sibling implementations, keep destroy idempotent, and cover successful, false, partial, empty, and garbage-collection paths in isolated temporary storage.
Refresh the local existence decision after a cold read so direct writes update existing rows instead of attempting duplicate inserts. Reset the object-specific existence slot at construction and cloning so reused PHP object IDs cannot silently update zero rows. Remove the unsafe unused connection mutator, retain fresh pooled connection resolution, document the shared container mutator as boot/test-only, and cover direct update plus deterministic constructor and clone identity reuse against the database harness.
Declare the complete Session configuration surface at its framework owner, including the JSON application default, route-block settings, and a dedicated application-scoped Redis session prefix. Apply Redis connection and prefix changes only to the cloned cache store, preserve explicit null, empty, and zero prefix behavior, reject incompatible stores clearly, remove drift-prone call-site defaults, and delete the dead unsupported cache-driver wrapper. Cover configuration conversion, direct PHP construction, clone isolation, and every prefix boundary.
Bring the Array and Null session handlers into line with SessionHandlerInterface and the already typed Cookie, File, Database, and Cache handlers. Add only the missing parameter and return types, preserving valid covariance and existing runtime behavior without widening the public surface or performing a broad unrelated typing sweep.
Rely on Hypervel auto-singletoning for StartSession, remove the empty provider boot hook, and keep only the canonical manager and store bindings. Delete verified unmatched PHPStan suppressions from Session authentication and Filesystem adapter creation while preserving the real route and magic-proxy guards. Update the touched authentication coverage to the repository typing convention.
Declare the extensions, Container, and Symfony components used directly by the Session split package instead of relying on transitive installation. Add package metadata coverage for the complete direct runtime dependency set so future source changes cannot silently drift the standalone manifest.
Document JSON serialization, deliberate PHP object-session opt-in, worker-local array sessions, Redis session prefixes, blocking configuration, and truthful custom handler write and garbage-collection contracts in the existing task-oriented guide style. Record the intentionally omitted unsupported Laravel cache-backed drivers and point custom implementations to Session::extend() so future upstream synchronization does not reintroduce the deleted dead wrapper.
Rename the heartbeat connection regression for the explicit Pool::flush() close protocol it exercises. The Pool lifecycle deliberately removed unreachable destructors in favor of deterministic close ownership, so retaining a destructor-named test made the supported behavior harder to verify and search.
Record the owner-approved framework-wide review of unmatched inline ignores and global patterns with reportUnmatchedIgnoredErrors enabled. Require each suppression to be traced before removal, prohibit runtime contortions or wider types merely to satisfy PHPStan, and leave the permanent strict-setting decision with that dedicated audit rather than expanding Session work.
Record session-01 through session-22 and filesystem-12 with their final owners, regressions, rejected alternatives, performance profile, Laravel-facing result, and completed validation. Mark Session complete, route the next standalone Queue audit through its exact prerequisite work units, close verified Cache, Log, Pool, Redis, Support, and Filesystem revalidation markers, and keep the dependency index grouped in completion order.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis change completes the session lifecycle audit, isolates coroutine-backed session state, makes persistence failure-aware, corrects middleware and handler boundaries, adds Redis/blocking configuration, updates session cache storage and package metadata, and expands documentation and regression coverage. ChangesSession lifecycle and persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR makes session persistence lifecycle-safe for long-lived workers and aligns the supported API and configuration surface with Laravel.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/session/src/DatabaseSessionHandler.php | Resets object-specific existence state during construction and cloning and refreshes it after cold reads; framework-owned ID changes still reset existence through Store migration. |
| src/session/src/Store.php | Introduces per-object coroutine state, non-null lazy IDs, and commit-after-write publication while preserving existence resets during session migration. |
| src/session/src/Middleware/StartSession.php | Uses request session attachment as the startup commit flag and preserves primary request failures across locking and after-response persistence. |
| src/session/src/CookieSessionHandler.php | Replaces the JSON envelope with strict binary-safe serialization and disables class construction while decoding. |
| src/session/src/FileSessionHandler.php | Requires complete writes and reports garbage collection according to successful deletions. |
| src/session/src/SessionManager.php | Applies Redis connection and prefix configuration to an isolated cloned cache repository and rejects incompatible stores. |
Reviews (3): Last reviewed commit: "fix(cache): preserve literal session cac..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/Integration/Session/DatabaseSessionHandlerTest.php (1)
202-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate the generic tracking fixture in a test-specific namespace.
TrackingDatabaseSessionHandleris collision-prone in the shared test namespace. Place it under a namespace ending inDatabaseSessionHandlerTest.As per coding guidelines, “Use test-specific namespaces for collision-prone generic helper classes, with the test class name as the final namespace segment.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Integration/Session/DatabaseSessionHandlerTest.php` around lines 202 - 220, Move the generic TrackingDatabaseSessionHandler fixture into a test-specific namespace whose final segment is DatabaseSessionHandlerTest, and update any references in the test accordingly while preserving its insert and update tracking behavior.Source: Coding guidelines
src/session/src/SessionManager.php (1)
147-203: 🩺 Stability & Availability | 🔵 TrivialVerify migration path for users with previously-published
config/session.php.
serialization,block,block_lock_seconds, andblock_wait_secondsare now read with typed getters and no inline default (string()/boolean()/integer()without a fallback), which throwInvalidArgumentExceptionif the key is missing. This is correct per the guideline to keep defaults in the config file, but any app that already ranvendor:publishon the pre-PRconfig/session.php(missing these new keys) will start throwing on every session access after upgrading, instead of silently falling back as before.Please confirm the shipped
src/foundation/config/session.phpdeclares these keys with sensible defaults, and that the upgrade docs call out this as a breaking change for apps with an already-published session config.Based on learnings, this repo's guideline states: "Use typed configuration getters such as
string(),integer(),float(),boolean(), andarray()for non-nullable values, with defaults declared in configuration files and merged throughmergeConfigFrom()."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/src/SessionManager.php` around lines 147 - 203, Verify the shipped session configuration defines defaults for serialization, block, block_lock_seconds, and block_wait_seconds so the typed getters in SessionManager methods buildSession, shouldBlock, defaultRouteBlockLockSeconds, and defaultRouteBlockWaitSeconds remain safe. Update the upgrade documentation to explicitly call this a breaking change for applications using an older published config/session.php, instructing them to republish or add the missing keys.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docs/plans/2026-07-27-0530-session-lifecycle-persistence-and-current-laravel-parity.md`:
- Line 143: Escape or reword the literal union pipes in the finding tables:
update `string|false` at
docs/plans/2026-07-27-0530-session-lifecycle-persistence-and-current-laravel-parity.md:143,
`array|string` at the same file:160, and the corresponding union text at
docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md:1261,
using wording such as “string-or-false” or escaped pipes so each row remains
valid Markdown.
In `@src/session/src/CookieSessionHandler.php`:
- Around line 49-59: Update the read() method around the unserialize fallback so
legacy non-array cookie-session envelope values are decoded before the current
data/expires validation. Preserve support for the existing serialized array
format, and only return an empty string after both modern and legacy decoding
paths fail validation.
---
Nitpick comments:
In `@src/session/src/SessionManager.php`:
- Around line 147-203: Verify the shipped session configuration defines defaults
for serialization, block, block_lock_seconds, and block_wait_seconds so the
typed getters in SessionManager methods buildSession, shouldBlock,
defaultRouteBlockLockSeconds, and defaultRouteBlockWaitSeconds remain safe.
Update the upgrade documentation to explicitly call this a breaking change for
applications using an older published config/session.php, instructing them to
republish or add the missing keys.
In `@tests/Integration/Session/DatabaseSessionHandlerTest.php`:
- Around line 202-220: Move the generic TrackingDatabaseSessionHandler fixture
into a test-specific namespace whose final segment is
DatabaseSessionHandlerTest, and update any references in the test accordingly
while preserving its insert and update tracking behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6264f7ef-1548-412d-9825-1b71a54b0b47
📒 Files selected for processing (40)
docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-07-27-0530-session-lifecycle-persistence-and-current-laravel-parity.mddocs/todo.mdsrc/boost/docs/session.mdsrc/contracts/src/Session/Session.phpsrc/filesystem/src/Filesystem.phpsrc/filesystem/src/FilesystemManager.phpsrc/foundation/config/session.phpsrc/foundation/src/Testing/Concerns/InteractsWithSession.phpsrc/foundation/src/Testing/Concerns/MakesHttpRequests.phpsrc/session/README.mdsrc/session/composer.jsonsrc/session/src/ArraySessionHandler.phpsrc/session/src/CookieSessionHandler.phpsrc/session/src/DatabaseSessionHandler.phpsrc/session/src/FileSessionHandler.phpsrc/session/src/Middleware/AuthenticateSession.phpsrc/session/src/Middleware/StartSession.phpsrc/session/src/NullSessionHandler.phpsrc/session/src/SessionManager.phpsrc/session/src/SessionServiceProvider.phpsrc/session/src/Store.phpsrc/support/src/Facades/Session.phptests/Foundation/Testing/Concerns/MakesHttpRequestsTest.phptests/Integration/Session/CookieSessionHandlerTest.phptests/Integration/Session/DatabaseSessionHandlerTest.phptests/Integration/Session/SessionPersistenceTest.phptests/Pool/HeartbeatConnectionTest.phptests/Session/CacheBasedSessionHandlerTest.phptests/Session/CookieSessionHandlerCoroutineSafetyTest.phptests/Session/CookieSessionHandlerTest.phptests/Session/EncryptedSessionStoreTest.phptests/Session/FileSessionHandlerTest.phptests/Session/Middleware/AuthenticateSessionTest.phptests/Session/Middleware/StartSessionTest.phptests/Session/PackageMetadataTest.phptests/Session/SessionConfigTest.phptests/Session/SessionManagerTest.phptests/Session/SessionStoreTest.php
💤 Files with no reviewable changes (3)
- src/filesystem/src/FilesystemManager.php
- src/foundation/src/Testing/Concerns/InteractsWithSession.php
- src/session/src/SessionServiceProvider.php
Escape union separators only where inline code appears inside Markdown table cells. Raw pipes were being parsed as additional columns, truncating the Session, Filesystem, Bus, and Console audit records in rendered documentation. Leave identical notation in normal prose untouched, where escaping would render a visible backslash. The five repaired rows now retain their expected column counts and code spans through GitHub's GFM renderer.
Collaborator
Author
|
Thanks @greptile-apps and @coderabbitai. I traced every finding against the current branch, the framework lifecycle, and current Laravel.
No runtime or test changes were needed from this review round. |
|
All six points check out against the PR and the review threads:
No outstanding findings from this review round. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Documentation table rows containing union types ---'
rg -n -F -C 1 '|' docs/plans -g '*.md' | rg -n -C 1 'array\|string|string\|array|\?string\|' || true
printf '%s\n' '--- Relevant file locations ---'
fd -a -t f \
'DatabaseSessionHandler.php' \
'CookieSessionHandler.php' \
'Store.php' \
'session.php' \
'DatabaseSessionHandlerTest.php' \
'SessionConfigTest.php' \
'CookieSessionHandlerTest.php' \
'2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md' \
'2026-07-12-0900-framework-coroutine-state-lifecycle-audit.md' \
.
printf '%s\n' '--- Session implementation outlines ---'
for file in \
src/session/src/DatabaseSessionHandler.php \
src/session/src/CookieSessionHandler.php \
src/session/src/Store.php \
src/foundation/config/session.php \
tests/Integration/Session/DatabaseSessionHandlerTest.php \
tests/Session/CookieSessionHandlerTest.php \
tests/Session/SessionConfigTest.php
do
if [ -f "$file" ]; then
echo "### $file"
ast-grep outline "$file" --items all || true
fi
done
printf '%s\n' '--- Targeted implementation and test references ---'
rg -n -C 3 \
-e 'function (read|write|__clone|migrate)\b' \
-e 'TrackingDatabaseSessionHandler\b' \
-e 'allowed_classes' \
-e "SESSION_(BLOCK|PREFIX|SERIALIZATION)" \
-e "'(block|prefix|serialization)'" \
src/session/src/DatabaseSessionHandler.php \
src/session/src/CookieSessionHandler.php \
src/session/src/Store.php \
src/foundation/config/session.php \
tests/Integration/Session/DatabaseSessionHandlerTest.php \
tests/Session/CookieSessionHandlerTest.php \
tests/Session/SessionConfigTest.php || trueLength of output: 25271 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Commit cb2847797: changed files and Markdown hunks ---'
git show --no-ext-diff --format=fuller --stat cb2847797
git show --no-ext-diff --format= --unified=4 cb2847797 -- 'docs/**/*.md'
printf '%s\n' '--- Store migration and database handler write/existence lifecycle ---'
sed -n '88,125p' src/session/src/DatabaseSessionHandler.php
sed -n '235,280p' src/session/src/DatabaseSessionHandler.php
sed -n '595,625p' src/session/src/Store.php
printf '%s\n' '--- GFM table separator counts for added/removed Markdown rows in cb2847797 ---'
git show --no-ext-diff --format= --unified=0 cb2847797 -- 'docs/**/*.md' |
python3 -c '
import sys
for raw in sys.stdin:
if not raw.startswith(("+", "-")) or raw.startswith(("+++", "---")):
continue
row = raw[1:].rstrip("\n")
if "|" not in row:
continue
# Pipes inside escaped content do not split a GFM cell.
separators = sum(ch == "|" and (i == 0 or row[i - 1] != "\\") for i, ch in enumerate(row))
print(f"{raw[0]} separators={separators}: {row}")
'Length of output: 20568
I agree the remaining items are intentional, supported design choices rather than changes needed in this PR. 🐇✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Store the session cache collection as a flat map so dotted cache keys are no longer interpreted as Session attribute paths. This prevents neighboring keys from nesting into, overwriting, or deleting one another while preserving expiration metadata during increments. Document that the session cache inherits the configured Session serializer and record the JSON-backed store's object-fidelity limitation without adding another encoding or validation layer. Add focused regressions for dotted-key coexistence, deletion, enumeration, and expiration-preserving increments, and update the audit ledger with the final boundaries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change makes Session state and persistence safe for long-lived workers while restoring the current supported Laravel surface.
Session stores now keep their mutable state per Store object and coroutine. Saves build storage snapshots without mutating live state, persist them first, and publish flash and error-bag changes only after the handler confirms the write. Middleware uses the request session as the startup commit flag, delegates lock cleanup to Cache, and prevents a failed after-response retry from replacing the request failure that was already reported and rendered.
The handler layer is tightened at the same time: cookie sessions use a strict binary-safe envelope, file sessions verify complete writes and truthful garbage collection, database sessions reset object-specific existence state across construction and cloning, and Redis sessions receive an isolated application-scoped prefix on the cloned cache store.
For more details, see: docs/plans/2026-07-27-0530-session-lifecycle-persistence-and-current-laravel-parity.md
Why
Hypervel caches Session drivers for the worker lifetime. The previous fixed coroutine keys therefore allowed two Store objects in one coroutine to share IDs, attributes, and started state. PHP object-ID reuse could also expose stale handler state after a driver was forgotten and rebuilt.
Persistence had a separate publication problem. Flash data and JSON error bags were modified before the handler write committed. A throwing or false write left the live Store in a partially saved state, so an after-response retry could age flash data twice, lose validation errors, or accept persistence loss as success.
These are lifecycle ownership issues rather than reasons to clone the manager, add a registry, or introduce a session transaction abstraction. The changes keep the existing manager and handler design, then fix each state transition at the lowest owner.
Store and request lifecycle
getId()contract with lazy per-coroutine generation.SessionHandlerInterface::write()result as persistence failure.Middleware ownership
Lock::block(..., $callback)for blocked routes, preserving request-failure precedence and avoiding release after acquisition timeout.Handler correctness
Configuration and Laravel parity
config/session.php.SESSION_PREFIX, defaulting toapp_id() . '_session:'."0".Cleanup and package completeness
Session::extend()alternative.Performance
Normal Store state access remains one coroutine-context lookup against a key allocated once per Store. The lazy ID path adds only an absent-slot branch. Save adds no lock, retry, container lookup, yield, or I/O beyond the existing handler operation; PHP copy-on-write touches only the snapshots that change.
Redis prefix setup occurs once during driver construction and performs no command or pool checkout. The database direct-write cold path adds one in-memory lookup while avoiding duplicate-insert failure and unnecessary database work. File writes compare the byte count already returned by the filesystem.
No registry, state machine, compatibility decoder, retry system, worker cache, or hot-path synchronization is introduced.
Validation
Summary by CodeRabbit
session.serialization(jsonorphp), Redissession.prefix, and optional session request blocking (lock/store timing).get/put/increment/forgetwith better key handling (including dotted keys).getId()is now guaranteed to return a non-nullstring; filesystemdeletenow acceptsarray|string.