[MINOR BC] [4.x] Fix FilesystemTenancyBootstrapper discarding configured cache and session paths - #1473
[MINOR BC] [4.x] Fix FilesystemTenancyBootstrapper discarding configured cache and session paths#1473lukinovec wants to merge 16 commits into
Conversation
The tests cover the current (mostly incorrect) scopeCache() behavior (= hardcoding the /framework/cache/data path regardless of what was configured). The 'file cache stores are separated per tenant' is not a regression test -- it covers the default path, which already worked correctly, there were just no tests for it. The rest are regression tests (see the "NOTE ABOUT REGRESSION" comments -- these are temporary, added them just so that it's clear what's currently wrong or broken) that should be fixed by the FS bootstrapper fix in the next commit.
scopeCache() rewrote path and lock_path for every file-driver store to a hardcoded '<storage>/framework/cache/data' path, completely ignoring the store's config. Now, scopeCache() remembers each store's original path and lock_path, scopes these paths for the tenant, and restores them to the stored originals on revert. The store's lock_path was always overwritten by the same hardcoded path. But lock_path is configurable too, AND it's actually optional (unlike path). If it's not configured at all (= it's null or just unset), Laravel automatically falls back to the store's path. So in that case, leave lock_path null instead of assigning the path to it. This is not a *huge* change, assigning path to lock_path would essentially achieve the same thing, BUT if someone explicitly sets lock_path to null in the config, we should just respect that and let Laravel fall back to the path instead of setting the lock_path ourselves. Also, on revert(), the same hardcoded path was used in scopeCache(). So if someone used a custom file driver-based store, cached something in central context, initialized and ended tenancy, the central cache got corrupt (see the 'central cache is not lost when tenancy ends' test).
📝 WalkthroughWalkthroughFile cache stores now retain original paths, apply tenant-specific ChangesFilesystem cache and session scoping
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Tenancy
participant FilesystemTenancyBootstrapper
participant CacheConfig
participant FileStore
Tenancy->>FilesystemTenancyBootstrapper: initialize tenant
FilesystemTenancyBootstrapper->>CacheConfig: scope cache, lock, and session paths
FilesystemTenancyBootstrapper->>FileStore: apply scoped cache and lock paths
Tenancy->>FilesystemTenancyBootstrapper: revert tenant
FilesystemTenancyBootstrapper->>CacheConfig: restore original paths
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1473 +/- ##
============================================
+ Coverage 86.65% 86.75% +0.10%
- Complexity 1220 1228 +8
============================================
Files 186 186
Lines 3589 3601 +12
============================================
+ Hits 3110 3124 +14
+ Misses 479 477 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/Bootstrappers/FilesystemTenancyBootstrapper.php`:
- Around line 220-244: Validate the original cache path in the cache-scoping
flow before passing it to scopeCachePath(); when a file-driver store omits path,
fail with a clear configuration error or skip the store consistently during
bootstrap and revert. Preserve the existing optional lock_path handling and
ensure scopeCachePath() is never called with null.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 31b4c918-c01e-4d38-bdde-b69db2e32054
📒 Files selected for processing (2)
src/Bootstrappers/FilesystemTenancyBootstrapper.phptests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
scopeCache() didn't feel right since it 1) stored thee original paths, 2) actually scoped things. Separate the concerns so that scopeCache() just does that -- scopes cache.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Making it protected could be a minor bc, and it'd be inconsistent with scopeSessions (which is public).
|
@coderabbitai full review |
✅ Action performedFull review finished. |
lukinovec
changed the title
FilesystemTenancyBootstrapper::scopeCache() discarding configured cache pathsFilesystemTenancyBootstrapper::scopeCache() discarding configured cache paths
lukinovec
changed the title
FilesystemTenancyBootstrapper::scopeCache() discarding configured cache pathsFilesystemTenancyBootstrapper::scopeCache() discarding configured cache paths
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (1)
507-508: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a unique temporary directory.
This test recursively deletes the fixed
/tmp/tenancy-cache-testdirectory. Another local process or parallel test run can use that directory. The test can delete unrelated data and can conflict with another run.Generate the path from
sys_get_temp_dir()with a random suffix.Proposed fix
- $path = '/tmp/tenancy-cache-test'; + $path = sys_get_temp_dir() . '/tenancy-cache-test-' . bin2hex(random_bytes(8));🤖 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/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 507 - 508, Update the temporary path setup in the affected test to derive the directory from sys_get_temp_dir() and append a unique random suffix, then continue passing that generated path to File::deleteDirectory. Ensure each test run targets only its own temporary directory instead of the fixed tenancy-cache-test path.
🤖 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.
Outside diff comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 507-508: Update the temporary path setup in the affected test to
derive the directory from sys_get_temp_dir() and append a unique random suffix,
then continue passing that generated path to File::deleteDirectory. Ensure each
test run targets only its own temporary directory instead of the fixed
tenancy-cache-test path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02a24bdd-540d-4eeb-b1ee-43c71eb6dcc5
📒 Files selected for processing (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (1)
471-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that an absent
lock_pathremains null.The current assertions pass if
scopeCache()replaces an absentlock_pathwithpath. Assert thatcache.stores.foo_file.lock_pathisnullafter initialization and aftertenancy()->end().Proposed test assertions
tenancy()->initialize(Tenant::create()); + expect(config('cache.stores.foo_file.lock_path'))->toBeNull(); + expect(Cache::store('foo_file')->put('key', 'tenant'))->toBeTrue(); expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); tenancy()->end(); + expect(config('cache.stores.foo_file.lock_path'))->toBeNull(); + expect(Cache::store('foo_file')->put('key', 'central'))->toBeTrue();🤖 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/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 471 - 500, Update the test around scopeCache() to assert that cache.stores.foo_file.lock_path remains null after tenancy()->initialize() and again after tenancy()->end(). Keep the existing cache put and lock behavior assertions unchanged.
🤖 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 `@src/Bootstrappers/FilesystemTenancyBootstrapper.php`:
- Around line 196-213: In src/Bootstrappers/FilesystemTenancyBootstrapper.php at
the bootstrap-time loop (lines 196-213), record the names of file stores that
are successfully scoped into a new instance property (for example, a scoped
stores list). During revert, update the scopeCache(false) method to iterate over
this captured snapshot of scoped stores instead of reading from the current
tenancy.cache.stores configuration list, ensuring that stores removed
mid-request are still reverted. In
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php at lines 578-618, add
a test case that scopes a file store during bootstrap, then removes it from
tenancy.cache.stores before calling tenancy()->end(), and asserts that the
store's path, lock_path, and resolved FileStore instance are restored to their
central-context values on revert.
---
Outside diff comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 471-500: Update the test around scopeCache() to assert that
cache.stores.foo_file.lock_path remains null after tenancy()->initialize() and
again after tenancy()->end(). Keep the existing cache put and lock behavior
assertions unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5703c71-3aab-4cb7-850a-0f98033cc0f6
📒 Files selected for processing (2)
src/Bootstrappers/FilesystemTenancyBootstrapper.phptests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
Note: 'the original cache paths are only stored on the first bootstrap' test got removed -- it tested that the "Unable to create tenant session directory" exception gets thrown, and that's not in scope of the current PR.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (3)
525-540: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that
lock_pathremainsnull.The cache and lock operations also pass if the bootstrapper replaces an absent
lock_pathwith the scoped cache path. Assert thatcache.stores.foo_file.lock_pathisnullduring tenancy and aftertenancy()->end().Based on upstream contract:
scopeCache()preserves an absentlock_pathasnull.🤖 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/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 525 - 540, Extend the test around the foo_file store configuration to assert that cache.stores.foo_file.lock_path remains null both after tenancy initialization and after tenancy()->end(). Keep the existing cache and lock operation assertions unchanged, verifying scopeCache() preserves the absent lock_path rather than replacing it with the scoped cache path.
493-505: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease each acquired file lock.
Both tests call
get()on non-expiring locks and do not release them. The cleanup only deletes the central configured directory. It does not remove the scoped tenant lock directory. A reused tenant suffix can then make a later lock acquisition fail.
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php#L493-L505: retain the tenant and central lock instances, then callrelease()before cleanup.tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php#L534-L540: retain the tenant and central fallback lock instances, then callrelease()before cleanup.🤖 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/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 493 - 505, Release every acquired non-expiring file lock before cleanup: in tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php lines 493-505, retain the tenant and central lock instances and call release() on both; apply the same change to the tenant and central fallback locks at lines 534-540.
550-551: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse an isolated temporary directory.
Line 550 uses a fixed shared directory.
File::deleteDirectory()deletes all its contents before and after the test. Parallel test workers can delete each other’s active cache data. Local runs can also delete unrelated data at this path. Generate a unique child directory under the system temporary directory for this fixture.Also applies to: 586-586
🤖 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/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 550 - 551, Replace the fixed shared directory path `/tmp/tenancy-cache-test` with a dynamically generated unique temporary directory. Generate a unique child directory under the system temporary directory for the $path variable assignment, then pass this unique path to File::deleteDirectory(). Apply the same fix to both occurrences at lines 550 and 586 to prevent parallel test workers and local runs from deleting each other's or unrelated cache data.
🤖 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.
Outside diff comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 525-540: Extend the test around the foo_file store configuration
to assert that cache.stores.foo_file.lock_path remains null both after tenancy
initialization and after tenancy()->end(). Keep the existing cache and lock
operation assertions unchanged, verifying scopeCache() preserves the absent
lock_path rather than replacing it with the scoped cache path.
- Around line 493-505: Release every acquired non-expiring file lock before
cleanup: in tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php lines
493-505, retain the tenant and central lock instances and call release() on
both; apply the same change to the tenant and central fallback locks at lines
534-540.
- Around line 550-551: Replace the fixed shared directory path
`/tmp/tenancy-cache-test` with a dynamically generated unique temporary
directory. Generate a unique child directory under the system temporary
directory for the $path variable assignment, then pass this unique path to
File::deleteDirectory(). Apply the same fix to both occurrences at lines 550 and
586 to prevent parallel test workers and local runs from deleting each other's
or unrelated cache data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2106cfba-9413-4eb9-8649-04256ab95fb9
📒 Files selected for processing (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
Also add a separate test ('scopeCache ignores changes to tenancy.cache.stores made in tenant context' ) -- the 'central cache is not lost when tenancy ends' covered the skipping mechanism partially, but having a separate test for the tenancy.cache.stores mid-tenant context changes is definitely cleaner and makes more sense.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Bootstrappers/FilesystemTenancyBootstrapper.php (1)
17-18: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear the cache-path snapshot after each revert.
$originalCachePathsand$originalCacheLockPathsremain populated afterrevert(). If central code changes a scoped storepathorlock_pathafter tenancy ends, Line 211 skips the new values. The next bootstrap scopes and restores the stale first values.Clear both maps after the revert loop. Add a regression test that changes both paths after
tenancy()->end(), then initializes another tenant and verifies the new paths are scoped and restored.Proposed fix
$store->setDirectory($path); $store->setLockDirectory($lockPath); } + + if ($suffix === false) { + $this->originalCachePaths = []; + $this->originalCacheLockPaths = []; + } }Also applies to: 211-214
🤖 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/Bootstrappers/FilesystemTenancyBootstrapper.php` around lines 17 - 18, Update FilesystemTenancyBootstrapper::revert() to clear both originalCachePaths and originalCacheLockPaths after completing the revert loop, so each subsequent bootstrap snapshots current path values. Add a regression test covering changes to both path and lock_path after tenancy()->end(), then verify the next tenant scopes those new paths and restores them afterward.tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (1)
579-580: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse a unique cross-platform temporary directory.
/tmp/tenancy-cache-testis shared by test workers. The cleanup can delete artifacts from another run or local data with the same path. The fixed/tmppath also prevents this test from running on platforms without/tmp.Proposed fix
- $path = '/tmp/tenancy-cache-test'; + $path = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'tenancy-cache-' . bin2hex(random_bytes(8));Also applies to: 615-615
🤖 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/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 579 - 580, Replace the hard-coded /tmp/tenancy-cache-test path in the affected test setup and cleanup blocks with a unique, cross-platform temporary directory generated through the project’s existing temporary-directory utility, and reuse that generated path throughout each test run. Apply the same change to the additional occurrence noted in the comment.
🤖 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.
Outside diff comments:
In `@src/Bootstrappers/FilesystemTenancyBootstrapper.php`:
- Around line 17-18: Update FilesystemTenancyBootstrapper::revert() to clear
both originalCachePaths and originalCacheLockPaths after completing the revert
loop, so each subsequent bootstrap snapshots current path values. Add a
regression test covering changes to both path and lock_path after
tenancy()->end(), then verify the next tenant scopes those new paths and
restores them afterward.
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 579-580: Replace the hard-coded /tmp/tenancy-cache-test path in
the affected test setup and cleanup blocks with a unique, cross-platform
temporary directory generated through the project’s existing temporary-directory
utility, and reuse that generated path throughout each test run. Apply the same
change to the additional occurrence noted in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 514d9e23-ec4f-4f08-9d0e-5dbda57b548d
📒 Files selected for processing (2)
src/Bootstrappers/FilesystemTenancyBootstrapper.phptests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
The check could only pass with a `null` path, which is just bad configuration. So no reason to keep this.
Add more meaningful assertions, make the tests clearer (by improving the names, commnets and the test code itself), merge separate tests that don't need to be separate. Also, don't set lock_path in tests that don't deal with locks (except for the generic "file cache stores are separated per tenant" test where we just want to mirror Laravel's default file store config -- though note that keeping the lock_path unset or null there would make no difference).
Fails at :108 (= the bootstrap() behavior), the session path ends with framework/sessions instead of the configured framework/foo_session)). After commenting out :108 and :110, it fails at :115 (= the revert() behavior, instead of reverting to the original configured path -- framework/foo_sessions -- it reverts to the hardcoded framework/sessions path)
…th instead of hardcoding /framework/sessions Also rename tenantCachePath to tenantScopedPath since it's now used both for scoping cache and session paths. Note that the $originalPath local variable -- added that to avoid PHPStan errors.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (1)
323-331: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that initialization throws no exception.
not()->toThrow(ErrorException::class)passes if initialization throws a different exception type. Usenot()->toThrow()because this test claims that non-file and missing stores are skipped without failure.🤖 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/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 323 - 331, Update the assertion in the tenancy initialization test around tenancy()->initialize($tenant1) to use not()->toThrow() without restricting the exception class, ensuring initialization fails for no exception type when non-file and missing stores are skipped.Source: Learnings
🤖 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 `@src/Bootstrappers/FilesystemTenancyBootstrapper.php`:
- Around line 217-225: Update the revert logic in FilesystemTenancyBootstrapper
so scope_cache controls only bootstrapping; always restore every captured cache
store from originalCachePaths and originalCacheLockPaths regardless of the
current flag. Likewise, make scope_sessions affect only bootstrapping and
restore originalSessionPath whenever it was captured, even if the flag changed;
add regressions covering each flag being disabled after tenant bootstrap and
verifying central paths are restored. For the affected sites, apply this at
src/Bootstrappers/FilesystemTenancyBootstrapper.php lines 217-225 and 263-268.
In `@tests/SessionSeparationTest.php`:
- Line 100: Ensure the configured session directory exists before the
File::cleanDirectory call in the session-separation test, using the framework’s
directory-creation helper. Keep the existing cleanup and subsequent
File::files($configuredSessionPath) verification unchanged.
---
Outside diff comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 323-331: Update the assertion in the tenancy initialization test
around tenancy()->initialize($tenant1) to use not()->toThrow() without
restricting the exception class, ensuring initialization fails for no exception
type when non-file and missing stores are skipped.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b4110ec7-e435-474d-b5af-61771690d0ff
📒 Files selected for processing (3)
src/Bootstrappers/FilesystemTenancyBootstrapper.phptests/Bootstrappers/FilesystemTenancyBootstrapperTest.phptests/SessionSeparationTest.php
lukinovec
changed the title
FilesystemTenancyBootstrapper::scopeCache() discarding configured cache pathsFilesystemTenancyBootstrapper::scopeCache() discarding configured cache and session paths
lukinovec
changed the title
FilesystemTenancyBootstrapper::scopeCache() discarding configured cache and session pathsThe assertion could cause false positives/negatives since the exception is not that specific. Just let tenancy()->initialize($tenant1) run and fail loudly, that could tell us more about what's wrong than the original assertion.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (2)
606-606: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the central cache directory assertion.
storage_path('framework/cache/data')is the central cache directory. The earlier test writes to it at Line 323, so this directory can exist even when the external cache path is scoped correctly. Assert the configured store path instead, or check the tenant-scoped default path.Proposed fix
- expect(File::isDirectory(storage_path('framework/cache/data')))->toBeFalse(); + expect(config('cache.stores.foo_file.path')) + ->toBe("{$path}/tenant{$tenant1->id}");🤖 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/Bootstrappers/FilesystemTenancyBootstrapperTest.php` at line 606, Update the directory assertion in the relevant filesystem tenancy test to check the configured external cache store path or tenant-scoped default path, rather than storage_path('framework/cache/data'). Preserve the expectation that the scoped cache directory is absent.
469-572: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up tenant-scoped lock directories.
The tests acquire locks under tenant-scoped paths at Lines 497-498, but cleanup at Lines 517-518 and Line 571 removes only the central paths. If tenant IDs reset between runs, stale tenant lock files can make the next lock assertion fail. Remove the tenant-scoped directories in
finallycleanup as well.🤖 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/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 469 - 572, Update both lock-directory tests around the tenant-scoped paths ($tenantPath and $tenantLockPath) to guarantee cleanup in finally blocks, deleting the tenant directories as well as the existing central paths. Preserve the assertions and ensure cleanup runs even when an assertion or lock acquisition fails.
🤖 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.
Outside diff comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Line 606: Update the directory assertion in the relevant filesystem tenancy
test to check the configured external cache store path or tenant-scoped default
path, rather than storage_path('framework/cache/data'). Preserve the expectation
that the scoped cache directory is absent.
- Around line 469-572: Update both lock-directory tests around the tenant-scoped
paths ($tenantPath and $tenantLockPath) to guarantee cleanup in finally blocks,
deleting the tenant directories as well as the existing central paths. Preserve
the assertions and ensure cleanup runs even when an assertion or lock
acquisition fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c1065352-3b4c-4fe0-ac93-4731cfe796b6
📒 Files selected for processing (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
2 participants
Using a different directory for the
filecache store by settingcache.stores.file.path(either usingconfig([...]), or directly inconfig/cache.php-- doesn't matter) has no effect --FilesystemTenancyBootstrapper::scopeCache()ignorespath/lock_pathentirely and rewrites both to a hardcoded<storage>/framework/cache/datapath on everytenancy()->initialize()/tenancy()->end():Specific issues with hardcoding the path like this:
lock_pathis always overwritten withpath, so a store with a separate lock directory loses that separationrevert()runs the same code, so it doesn't restore what the store was configured with before tenancy initialized -- it just re-applies the same hardcoded default. Central cache ends up using the wrong path after ending tenancy.scopeSessions()has the same bug. It never readssession.files, it hardcodes<storage>/framework/sessionson both bootstrap and revert. So a configured session path gets discarded when tenancy initializes, and it doesn't get reverted back to what it was when tenancy ends. For example, withsession.filesset to/tmp/foo-sessions:So after ending tenancy, sessions don't go back to the configured
/tmp/foo-sessions. They use the hardcoded<storage>/framework/sessionspath, which was never configured anywhere.The fix
In
bootstrap(),scopeCache()captures the original configured paths and scopes those instead of using a hardcoded default.storage/framework/cache/databecomesstorage/tenant1/framework/cache/data).storage_path()-based, there's nothing to swap, so the tenant's suffix just gets appended to the end of the path instead.On
revert(),scopeCache(false)puts the captured paths back intocache.stores.{$name}.path/lock_pathand into the resolved store instance, so central cache uses the path it was configured with again.lock_pathstaysnullwhen a store doesn't configure it, rather than us making it default to the scoped path --FileStorealready falls back topathfor locks in that case, so we can just respect the store's original config.scopeSessions()does the same forsession.files. It captures the configured path duringbootstrap()(once, for the same reason as above), scopes it, and puts it back onrevert()(the defaultstorage/framework/sessionsstill ends up asstorage/tenant1/framework/sessions, so nothing changes for the default config).Also added tests that cover each of the issues above (+ a test for handling paths that aren't
storage_path()-based, and one for a customsession.filespath).POSSIBLE MINOR BC: Someone with
'path' => '/var/cache/foobar'currently gets tenant cache instorage/tenant1/framework/cache/data. After this fix, they get/var/cache/foobar/tenant1, so whatever is already cached in the old directory is orphaned. The same applies to a non-defaultstorage_path()-based path, e.g.'path' => storage_path('framework/cache/data_' . env('TEST_TOKEN', 'default')). The config is now respected while scoping. The same goes for sessions -- with a non-defaultsession.files, tenant sessions move fromstorage/tenant1/framework/sessionsto the configured path scoped for the tenant, so the sessions in the old directory are orphaned.Possible further improvement (not worth pursuing, keeping this here for future reference)
Rather than
tenantScopedPath()hardcoding how a store's path gets scoped, that could go through config, the same waydiskRoot()resolvesroot_overridetemplates. Something like:with
%configured_path%,%suffix%,%storage_path%and%original_storage_path%placeholders. Stores without an entry would keep the behavior described above (no template involved).What that would let people do. Here's the default
filestore fromconfig/cache.php:The changes in this PR scope that path to
storage/tenant1/framework/cache/data, so each tenant's cache directory sits next to their files:With
'file' => '%configured_path%/%suffix%', the cache directory would stay where it's configured and each tenant would get a subdirectory in it:All tenant cache directories in one place, so anything that only concerns cache (skipping it in backups, deleting old files) is one path instead of one per tenant. And apparently, it makes mounting possible -- you can't mount tmpfs on
storage/tenant3/framework/cache/databefore tenant3 exists, but you can mount it once onstorage/framework/cache/dataand every tenant's cache ends up inside the mount.But you can already get the same thing without a new config key. If the store's path is outside
storage_path(), the changes in this PR append the suffix to it automatically:So
path_overridewould only really add one thing -- the same structure while the cache stays insidestorage/. That does matter to someone whose deploy scripts,.gitignoreand volume mounts all assumestorage/, but that's about it.So I don't think we should add it. AFAIK, nobody has ever asked for a configurable cache path (I searched the issues and PRs and couldn't find anything that would suggest that this is something people want). There are also two problems with the idea itself:
pathandlock_pathwould go through the same template, so'%storage_path%/framework/cache/data'brings back the first two bugs from the list above -- the configured path gets thrown away, and both directories end up in the same place. We'd have to require the template to contain%configured_path%.root_override, where the tenant part is%tenant%(the tenant key). Here it'd be%suffix%(suffix_base+ key), so two names for nearly the same thing in one class. I think that could be confusing.(Note that until recently I thought this would be the fix for the
suffix_storage_pathproblem below, but I don't think that anymore -- that one should be fixed by default, not by a config key.)More issues found while looking into this
suffix_storage_path => falseisn't respected byscopeCache()orscopeSessions(). With the setting off, in the tenant context, right aftertenancy()->initialize()and before writing anything:Cache only points the store at the suffixed path and lets
FileStorecreate it on first write.scopeSessions()is worse because it callsmkdir()on every tenancy init, with nosuffix_storage_pathcheck anywhere, whether or not a session is ever written.Also, nothing cleans those directories up.
DeleteTenantStoragereturns early whensuffix_storage_path === false(see theDeleteTenantStoragejob). The early return there is correct -- without it the next guard would catch the case anyway, sincestorage_path()in the tenant context is the central path there, and deleting that would be very bad of course. But it means the cache/session directories understorage/tenant<key>/can't be reached by any cleanup at the moment (so one such directory for every tenant that ever existed). Laravel doesn't do anything with these file sessions either, since it only clears whateversession.filescurrently points at.Turning
scope_cache/scope_sessionsoff wouldn't be a feasible "solution" because for thefiledriver, path scoping is the only isolation there is (FileStore::getPrefix()returns a hardcoded''), so all tenants would share one cache directory. The settings are also about different things --suffix_storage_pathis about whetherstorage_path()itself moves,scope_cache/scope_sessionsabout whether tenant cache and sessions stay separate. A centralstorage_path()with isolated cache sounds like a sensible combination (and I see that's what people actually want -- #196). And sincesuffix_storage_path,scope_cacheandscope_sessionsare all enabled by default in the config, someone who only setssuffix_storage_pathtofalse(which is what the config comment tells you to do on S3) runs into this without ever touching cache or session scoping.I think we could fix this by scoping cache and sessions inside the configured path when
suffix_storage_pathisfalse, sostorage/framework/cache/data/tenant1instead ofstorage/tenant1/framework/cache/data. Tenants stay separated,storage_path()stays central like the user asked for, and there's nostorage/tenant<key>/directory for cleanup to miss.path_overridethingSummary by CodeRabbit
Bug Fixes
Tests