[4.x] Revert to the previous context when a runForMultiple() or central() callback throws

[GautierDele]

Tenancy::run() reverts to the previous context in a finally since b9cc63f, but runForMultiple() and central() still restore it only after the callback returns. When the callback throws:

• runForMultiple() stays initialized for the tenant whose callback threw, instead of returning to the original tenant or to the central context.
• central() stays in the central context instead of re-initializing the previous tenant, although its docblock says it is atomic and safely reverts to the previous context.

This reaches beyond user code: UpdateOrCreateSyncedResource, DeleteResourcesInTenants and RestoreResourcesInTenants all walk the tenants with runForMultiple(). A write refused in one tenant's database (a NOT NULL or unique constraint, for instance) leaves the rest of the request or job running against that tenant's connection, cache and filesystem.

This PR wraps both in try/finally, the way run() already does. When the callback returns normally nothing changes, including the value central() returns.

Tests:

• runForMultiple reverts to the original context when the closure throws, starting from the central context and from a tenant context
• central helper reverts back to tenant context when the callback throws

All three cases fail on master and pass with the change. PHPStan reports no errors on src/Tenancy.php.

3.x has the same defect, and Tenant::run() there has no finally either. A backport covering all three follows.

Summary by CodeRabbit

• Bug Fixes
  • Tenant context is now restored after callbacks throw errors, including when processing multiple tenants. Central context is restored when no tenant was previously active.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e3c30e05-b39c-44ed-a864-b86246f00c5e

📥 Commits

Reviewing files that changed from the base of the PR and between 34438f9 and 2a77a8e.

📒 Files selected for processing (3)

• src/Tenancy.php
• tests/AutomaticModeTest.php
• tests/RunForMultipleTest.php

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

central() and runForMultiple() now restore the previous tenancy state when callbacks or tenant processing throw. Tests verify exception propagation and state restoration.

Changes

Tenancy cleanup

Layer / File(s)	Summary
Restore tenancy after exceptions src/Tenancy.php, tests/AutomaticModeTest.php, tests/RunForMultipleTest.php	central() restores the previous tenant in a finally block. runForMultiple() restores the original tenant or ends tenancy in a finally block. Tests check that exceptions propagate and tenancy state is restored.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 2a77a

The tenancy-restoration change is ready to merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 2a77a

The change reduces the risk of work continuing under the wrong tenant after a callback fails. Restoration still depends on tenancy cleanup completing successfully, and the behavior of production callers and listeners has not been fully established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Tenant identity affects database connection selection and the cache and filesystem contexts managed by tenancy bootstrappers. A failed restoration can therefore affect more than the tenant value returned to application code.

Trust Boundaries and Controls

• observed — runForMultiple() obtains tenant identity from its supplied tenants or a model cursor, initializes each tenant before invoking the supplied closure, and restores the captured context in finally. The inspected paths do not establish who may influence those tenants or the production callers.

Resilience and Maintainability Implications

• inferred — If restoration itself throws, its exception can replace the callback failure and leave resource state partly transitioned. This is a residual lifecycle limitation, not an established new attack path introduced by the PR.

Hardening Proposals

• proposed — Exercise failed initialization and cleanup listeners while asserting the resulting tenant binding and resource context; the new tests currently assert the binding after callback failures.
• proposed — Clarify whether central() promises restoration when a callback starts centrally and initializes a tenant itself; its current no-previous-tenant branch does not end such a context, including after a throw. This behavior predates the PR.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: restoring the previous tenancy context when runForMultiple() or central() callbacks throw.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks the tenant trail,
Through finally, it will not fail.
A thrown exception takes its course,
The prior state returns to source.
The burrow rests, the tests all pass.}

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.88%. Comparing base (34438f9) to head (2a77a8e).

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1488      +/-   ##
============================================
- Coverage     86.89%   86.88%   -0.01%     
  Complexity     1252     1252              
============================================
  Files           186      186              
  Lines          3654     3653       -1     
============================================
- Hits           3175     3174       -1     
  Misses          479      479              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[stancl]

1. Have you encountered concrete issues or is this an AI generated improvement?
2. "A write refused in one tenant's database (a NOT NULL or unique constraint, for instance) leaves the rest of the request or job running against that tenant's connection, cache and filesystem." isn't the case unless the user would explicitly catch (Exception), ignore the error, and continue with execution without reverting to previous context. The exception (which is not caught currently) would normally terminate execution. That said, we do likely want to make this function revert to the previous context, I believe it's on one of my lists of things to revisit in v4.
3. "central() stays in the central context instead of re-initializing the previous tenant" I'm not sure if this is incorrect. If there is an exception thrown in the central context and tenant-specific logging is used, you'd likely want the exception to be reported in the central context. Would need to consider this in more depth. You could make the case that run() currently does throw the exception in the previous tenant context when invoked from tenant 1 and calling into tenant 2, but the typical (but not exclusive) use of run() is to get into a tenant context from the central context where reverting to throw the exception makes sense. central() also wouldn't normally fail on its own, only if the passed closure has some issue, in which case it makes sense to me to report it in the central context as it's an issue occurring in the central context. Perhaps I would just clarify the docblock that exceptions aren't handled and can skip the reverting logic.

[GautierDele]

Hello @stancl,

1. This PR was generated with the help of AI, mostly for the description and for understanding the repository's history, which I'm not fully aware of. The problem itself is one I encountered in production: synchronization threw due to an error and stopped in the middle

2. You're right, but in an HTTP request Laravel does: Illuminate\Routing\Pipeline::handleException() reports it and renders the error response, then the outer middleware finish (session, cookies) and the terminating callbacks run. All of that still runs in the tenant where runForMultiple() stopped, so a failure during a central request is reported through that tenant's logging. That's what "the rest of the request" meant. I can reduce this PR to runForMultiple() alone, or close it if you'd rather handle it in the v4 list to revisit 😊

3. An exception thrown inside central() is a central-context failure, so reporting it there makes sense, and it's less clear-cut than runForMultiple(). I changed it because its docblock says "Atomic, safely reverts to previous context", but can drop the central() change and reword the docblock to say the previous tenant isn't restored when the callback throws. The case worth stating there is a caller that catches the exception and continues: it keeps running in the central context. Either way works for me, and I'll update the PR to match

Also, do you have any release date estimation for v4 ? Quite interested by it !