Repository navigation
Tidy tenant-aware Quartz job factory - #9126
Conversation
Share one TenantId reader between the factory and the Execute guard, assert that workflow guard failures do not retry or delete, and replace the primary constructor that produced CS9107. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
sfmskywalker
added
area:runtime
Cursor
|
@elsa-workflows-bot review |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Shared tenant ID resolution extensions/src/scheduling/Elsa.Scheduling.Quartz/Extensions/JobExecutionExtensions.cs, extensions/src/scheduling/Elsa.Scheduling.Quartz/Services/TenantAwareJobFactory.cs |
EnsureScheduledTenant and TenantAwareJobFactory use the shared tenant ID reader. The reader checks trigger data before job data and no longer checks MergedJobDataMap. |
Tenant error test assertions extensions/test/modules/scheduling/Elsa.Scheduling.Quartz.UnitTests/Jobs/ResumeWorkflowJobTests.cs, extensions/test/modules/scheduling/Elsa.Scheduling.Quartz.UnitTests/Jobs/RunWorkflowJobTests.cs |
Tests assert that the specified tenant error paths do not schedule retries, delete jobs, or unschedule triggers. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Possibly related issues
elsa-workflows/elsa-core#9120— The issue describes Quartz timers and Cron triggers firing only for the default tenant. This change reads tenant IDs from trigger data or job data, but the provided summary does not confirm it resolves every reported symptom.
Possibly related PRs
- elsa-workflows/elsa-core#9121 — This PR introduced
TenantAwareJobFactoryandEnsureScheduledTenant, which this change updates to use shared tenant ID extraction.
Suggested reviewers: elsa-workflows-review-bot
Merge Risk: 
8d1a8
-
This tidy-up shares one tenant ID reader between the job factory and the execute guard and adds test assertions. It does not change behavior for tenant IDs supplied through trigger or job data. Merging carries no concrete risk.
Pre-merge checks: 
-
Check name Status Explanation Resolution Description check The description explains the main changes and constraints, but it omits most required template sections, including Purpose, Scope, Problem, Solution, Verification, Expected outcome, Screenshots/Record… Update the description to use the repository template. Add a Purpose summary, select the primary Scope, describe the Problem and Solution, provide verification steps and expected results, address Screenshots/Recordings as applicable, and co… Docstring Coverage Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely identifies the main change to the tenant-aware Quartz job factory. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description explains the main changes and constraints, but it omits most required template sections, including Purpose, Scope, Problem, Solution, Verification, Expected outcome, Screenshots/Recordings, and Checklist.
Resolution
Update the description to use the repository template. Add a Purpose summary, select the primary Scope, describe the Problem and Solution, provide verification steps and expected results, address Screenshots/Recordings as applicable, and complete the Checklist.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 4 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
 [Medium impact] Safe to merge within the reviewed scope; no actionable findings remain. SummaryThis PR shares tenant-ID lookup between the Quartz job factory and the execution guard, replaces the factory’s primary constructor with an explicit constructor, and strengthens rejection tests to check that retries, job deletion, and trigger removal do not occur. No actionable issues were found. Focused before-and-after checks passed all 66 tests on both revisions, including 24 additional tenant-selection contract cases. A separate comparison of the two workflow-job test classes passed all 33 tests on both revisions using the strengthened assertions. The tested behavior preserves trigger-over-job tenant precedence, fallback for missing or invalid tenant values, tenant-scoped job construction, and rejection before workflow execution or scheduler side effects. Safe to merge within the reviewed scope.
Reviews (1) · Last reviewed commit: "refactor(scheduling): tidy tenant-aware ..." · Reviewed by Greptile |
![elsa-workflows-review-bot[bot]](https://avatars.githubusercontent.com/in/5321421?s=60&v=4){kind=small}
There was a problem hiding this comment.
Elsa 3 Code Review: APPROVE + HIGH @ 8d1a809
Code Review, Round 1/6
Scope: follow-up to #9121. One shared TenantId reader for TenantAwareJobFactory and EnsureScheduledTenant, a regular constructor in place of the primary constructor (CS9107), and stronger guard tests.
Correctness
ReadScheduledTenantId(triggerData, jobData)keeps the same order as before: trigger data first, then job data. The factory and the Execute guard now can't drift apart.- The guard no longer falls back to
MergedJobDataMap. Quartz builds that map from the job detail's map plus the trigger's map, so the two direct lookups already cover it. Same behaviour, one less path. - The factory's lookup now also accepts a non-
TryGetStringstring value and tolerates a null map. That is a little more lenient than before and matches what the guard already did. No regression. - The explicit constructor stores
IServiceProvideronce. Lookup, push and retry behaviour inInstantiateJobandTenantLookupFailedJobare unchanged.
Tests
- The
RunWorkflowJobandResumeWorkflowJobguard tests now check that no retry is scheduled and that no job or trigger is deleted or unscheduled. These are the right checks for a fail-loudly guard. - The existing
TenantAwareJobFactoryand module-path tests cover the shared reader through the factory.
Non-blocking
ReadScheduledTenantId(this IJobExecutionContext)is a new public extension on a public class. Both callers are in this assembly, sointernalwould be enough and would keep the public API from growing. Optional.
Bots
- Greptile: 5/5, no actionable findings. No inline bot threads.
CI
All checks on 8d1a8090 are green: ubuntu-latest (compile + test), CodeQL, GitGuardian and CLA. run-affected-tests was skipped by selection, and ubuntu-latest ran the full suite.
Nothing actionable remains.
— Code Review (Elsa 3), via Elsa Review
Small follow-up to #9121:
TenantIdfrom trigger data, then job data, for both the factory and the Execute guard.EnsureScheduledTenantguard tests also assert that no retry is scheduled and that no job or trigger is deleted.TenantAwareJobFactoryuses a regular constructor instead of a primary constructor that capturedIServiceProvidertwice. Behaviour is unchanged.The synchronous tenant lookup stays on the scheduler thread. No timeout or cache was added.
Summary by CodeRabbit