Implement tenant agent deactivation service and integrate with tenant… - #532
Diyanada99x wants to merge 1 commit into
Conversation
|
🔍 PR Review in Progress Claude Code is analyzing this pull request. The review will be posted here shortly. PR Reviewer (1.9.9) |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| catch (Exception ex) | ||
| { | ||
| _logger.LogWarning(ex, "Error deactivating activation {ActivationName} ({ActivationId}) of tenant {TenantId}", | ||
| LogSanitizer.Sanitize(activation.Name), LogSanitizer.Sanitize(activation.Id), | ||
| LogSanitizer.Sanitize(request.TenantId)); | ||
| } |
hasith
left a comment
hasith
left a comment
There was a problem hiding this comment.
PR Review Report
PR: Implement tenant agent deactivation service and integrate with tenant management
Author: Diyanada99x
Files Changed: 6 | +505 / -2
Verdict: REQUEST CHANGES
Summary
This PR adds a background hosted service that deactivates all of a disabled tenant's active agents asynchronously, so the tenant-disable API call returns immediately. The implementation is clean, well-documented, and backed by solid unit tests (13 new test cases covering retry, re-enable, deletion, and per-activation failure isolation). The main concern is a design-level authorization issue: the background job captures and replays the disabling admin's roles/identity onto a live ITenantContext with no re-validation at execution time, and the queue has no bound or dedup, which together warrant a fix before merge (escalated to the full specialist review given the auth/authz surface touched).
Critical Issues (Must Fix)
-
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:95— The background job replays the capturedRequestedBy/UserRoles/UserTypefrom the moment the tenant was disabled onto a fresh scopedITenantContext, then runs the entire downstream call graph (IActivationRepository,IActivationService.DeactivateAgentAsync, audit logging, etc.) under that identity with no re-validation.ITenantContext.UserRoles/UserTypeare read pervasively elsewhere in the codebase to gate authorization. Because the channel is unbounded, in-memory, and has no processing deadline, this can run arbitrarily long after enqueue — if the admin's privileges are revoked in the meantime, the job still executes with the stale, now-invalid roles (a confused-deputy / stale-authorization pattern, CWE-285/CWE-639 analog).// Current (problematic) var tenantContext = services.GetRequiredService<ITenantContext>(); tenantContext.TenantId = request.TenantId; tenantContext.LoggedInUser = request.RequestedBy; tenantContext.UserRoles = request.UserRoles; tenantContext.UserType = request.UserType; // Fix: run as a dedicated internal/system identity scoped only to this // operation, and keep RequestedBy as an audit annotation rather than a // live authorization context. If re-authorization as the original user // is required, re-fetch their *current* roles at processing time and // skip/fail if they no longer hold the required privilege.
Warnings (Should Fix)
-
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:34(queue) andXiansAi.Server.Src/Shared/Services/TenantService.cs:855(enqueue site) — The channel is unbounded with no dedup/cap/rate-limit, and is enqueued on every disable call (not only on a state change, per the inline comment, to support retries). A SysAdmin — or a compromised SysAdmin credential — can repeatedly toggleenabled:falseto grow the queue without bound, each item later triggering a tenant lookup, an activation lookup, and aDeactivateAgentAsynccall per active agent. Consider a bounded channel deduped perTenantId(keep only the latest pending request) and/or rate-limiting the disable path. -
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:129— Active agents are deactivated strictly sequentially (awaitinside aforeach), and eachDeactivateAgentAsynccall does its own DB lookup plus Temporal workflow/schedule cleanup. For tenants with many active agents this background job will take proportionally longer with no concurrency. Consider bounded concurrency (e.g.Parallel.ForEachAsyncwith a modestMaxDegreeOfParallelism) instead of the sequential loop. - Test coverage:
XiansAi.Server.Tests/UnitTests/Shared/Services/TenantAgentDeactivationServiceTests.cshas strong coverage of the main scenarios but is missing a test forEnqueue(null)(verifying theArgumentNullException.ThrowIfNullguard actually fires), andQueuedRequest_IsProcessedByBackgroundLoop:181relies on a fixed 5-secondWaitAsynctimeout for the background-loop integration test, which can be flaky under CI load.
Suggestions (Consider Improving)
-
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:117—GetByTenantIdAsyncfetches every activation (active and historical) for the tenant and filters in memory with.Where(a => a.IsActive), even thoughIActivationRepository.GetActiveActivationsAsync(tenantId)already exists and performs the equivalent filtering at the DB level. Prefer calling that instead to avoid transferring potentially large numbers of inactive historical activation documents. -
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:15—TenantAgentDeactivationRequest.UserRolesis a mutablestring[]. If the caller's array is ever mutated after being captured (e.g. a shared array reference fromITenantContext), the queued snapshot would silently change. Consider copying into an immutable collection (e.g.ImmutableArray<string>or.ToArray()defensive copy) when the request is constructed. -
XiansAi.Server.Src/Shared/Configuration/SharedServices.cs:86— The existingBackgroundTaskServiceregistration is guarded withif (!services.Any(s => s.ServiceType == typeof(IBackgroundTaskService)))"to support test mocks" (see the comment a few lines above). The newTenantAgentDeactivationServiceregistration doesn't follow that same guard — for consistency and to avoid surprises if a test ever substitutesITenantAgentDeactivationServicevia the same DI container, consider mirroring the existing pattern. -
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:36— The channel is configuredSingleReader = true, so one tenant with a very large agent count will serialize behind it the processing of every tenant disabled afterward (no cross-tenant fairness). Likely acceptable given there's no stated SLA, but worth a comment noting the trade-off if it isn't already intentional for ordering guarantees.
Review Details
Code Quality
Overall clean, well-documented code with good null-guard discipline (ArgumentNullException.ThrowIfNull) and proper per-activation error isolation so one failure doesn't block the rest of the batch. Two findings raised by the quality pass (a claimed missing StopAsync override and a claimed null-ErrorMessage logging bug) were independently verified against the base BackgroundService cancellation semantics and LogSanitizer.Sanitize's null-safe implementation respectively, and both turned out to be false positives — not included above.
Security
The original tenant-disable action remains properly gated by existing authorization (EnsureTenantAccessOrThrow / CapabilityActions.TenantsUpdate) before the new enqueue call, and logging consistently sanitizes TenantId/activation name/id/error message via LogSanitizer.Sanitize — no log-injection issues found. The substantive concern is the stale-authorization/confused-deputy risk from replaying captured roles onto a live ITenantContext for delayed, unbounded background execution (see Critical Issues), compounded by the unbounded/un-deduped queue being a DoS amplification vector (see Warnings).
Test Coverage
Strong: 13 new test cases across TenantAgentDeactivationServiceTests.cs (service-level, mocked DI scope) and TenantServiceDisableTests.cs (TenantService-level) cover retry-on-redisable, skip-on-re-enable, skip-on-delete, per-activation failure/exception isolation, and authorization-gated enqueue. Minor gaps noted above (null-arg guard test, integration-test timeout robustness) are non-blocking.
Performance
No critical issues — this work runs off the request path in a background hosted service, so the tenant-disable API's latency and availability are unaffected. The sequential per-agent deactivation loop and the non-DB-pushed-down active-filter are real but moderate throughput concerns for tenants with large agent counts (see Warnings/Suggestions).
Files Reviewed
| File | Lines Changed | Risk | Notes |
|---|---|---|---|
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs |
+157/-0 | 🔴 High | New background service; auth-context replay, unbounded queue |
XiansAi.Server.Src/Shared/Services/TenantService.cs |
+16/-2 | 🟡 Medium | Wires enqueue into disable path |
XiansAi.Server.Tests/UnitTests/Shared/Services/TenantAgentDeactivationServiceTests.cs |
+188/-0 | 🟢 Low | New unit tests |
XiansAi.Server.Tests/UnitTests/Shared/Services/TenantServiceDisableTests.cs |
+138/-0 | 🟢 Low | New unit tests |
XiansAi.Server.Src/Shared/Configuration/SharedServices.cs |
+5/-0 | 🟢 Low | DI registration |
XiansAi.Server.Tests/UnitTests/Shared/Services/TenantServiceMetadataTests.cs |
+2/-1 | 🟢 Low | Constructor arg update for new DI dependency |
| using var scope = _scopeFactory.CreateScope(); | ||
| var services = scope.ServiceProvider; | ||
|
|
||
| var tenantContext = services.GetRequiredService<ITenantContext>(); |
There was a problem hiding this comment.
CRITICAL: The background job replays the captured RequestedBy/UserRoles/UserType from the moment the tenant was disabled onto a fresh scoped ITenantContext, then runs the entire downstream call graph (IActivationRepository, IActivationService.DeactivateAgentAsync, audit logging, etc.) under that identity with no re-validation. ITenantContext.UserRoles/UserType are read pervasively elsewhere in the codebase to gate authorization. Because the channel is unbounded, in-memory, and has no processing deadline, this can run arbitrarily long after enqueue — if the admin's privileges are revoked in the meantime, the job still executes with the stale, now-invalid roles (a confused-deputy / stale-authorization pattern, CWE-285/CWE-639 analog).
Fix: Run the deactivation under a dedicated internal/system identity scoped only to this operation, and keep RequestedBy as an audit annotation (passed explicitly to the audit log) rather than assigning it to tenantContext.LoggedInUser/UserRoles/UserType, which gate real authorization checks elsewhere. If re-authorization as the original user is truly required, re-fetch their current roles at processing time and skip/fail if they no longer hold the required privilege.
| /// </summary> | ||
| public class TenantAgentDeactivationService : BackgroundService, ITenantAgentDeactivationService | ||
| { | ||
| private readonly Channel<TenantAgentDeactivationRequest> _queue = |
There was a problem hiding this comment.
WARNING: The channel is unbounded with no dedup/cap/rate-limit, and (per TenantService.cs around the enqueue call) is enqueued on every disable call, not only on a state change, to support retries. A SysAdmin — or a compromised SysAdmin credential — can repeatedly toggle enabled:false to grow the queue without bound, each item later triggering a tenant lookup, an activation lookup, and a DeactivateAgentAsync call per active agent.
Fix: Use a bounded channel deduped per TenantId (keep only the latest pending request per tenant, e.g. drop-oldest policy) and/or rate-limit the disable path so repeated toggles collapse into a single pending retry instead of growing the queue unboundedly.
| var activationService = services.GetRequiredService<IActivationService>(); | ||
| var deactivated = 0; | ||
|
|
||
| foreach (var activation in active) |
There was a problem hiding this comment.
WARNING: Active agents are deactivated strictly sequentially (await inside a foreach), and each DeactivateAgentAsync call does its own DB lookup plus Temporal workflow/schedule cleanup. For tenants with many active agents this background job will take proportionally longer with no concurrency.
Fix: Consider bounded concurrency (e.g. Parallel.ForEachAsync(active, new ParallelOptions { MaxDegreeOfParallelism = 8 }, ...)) instead of the sequential loop, so independent per-agent deactivations run concurrently while bounding load on Temporal/Mongo.
| try | ||
| { | ||
| _service.Enqueue(Request); | ||
| await done.Task.WaitAsync(TimeSpan.FromSeconds(5)); |
There was a problem hiding this comment.
WARNING: Good coverage of the main scenarios overall, but two gaps are worth closing: (1) there's no test for Enqueue(null) verifying the ArgumentNullException.ThrowIfNull guard actually fires; (2) QueuedRequest_IsProcessedByBackgroundLoop relies on a fixed 5-second WaitAsync timeout for the background-loop integration test, which can be flaky under CI load.
Fix: Add an Enqueue(null) test asserting ArgumentNullException, and consider a longer timeout or a more deterministic synchronization mechanism for the background-loop test.
| } | ||
|
|
||
| var activations = await services.GetRequiredService<IActivationRepository>() | ||
| .GetByTenantIdAsync(request.TenantId); |
There was a problem hiding this comment.
SUGGESTION: GetByTenantIdAsync fetches every activation (active and historical) for the tenant and filters in memory with .Where(a => a.IsActive), even though IActivationRepository.GetActiveActivationsAsync(tenantId) already exists and performs the equivalent filtering at the DB level.
Fix: Call GetActiveActivationsAsync(request.TenantId) instead, avoiding transfer of potentially large numbers of inactive historical activation documents.
| public record TenantAgentDeactivationRequest( | ||
| string TenantId, | ||
| string RequestedBy, | ||
| string[] UserRoles, |
There was a problem hiding this comment.
SUGGESTION: TenantAgentDeactivationRequest.UserRoles is a mutable string[]. If the caller's array is ever mutated after being captured (e.g. a shared array reference from ITenantContext), the queued snapshot would silently change.
Fix: Consider copying into an immutable collection (e.g. ImmutableArray<string> or a defensive .ToArray() copy) when the request is constructed.
| } | ||
|
|
||
| // Deactivates a disabled tenant's agents in the background | ||
| services.AddSingleton<TenantAgentDeactivationService>(); |
There was a problem hiding this comment.
SUGGESTION: The existing BackgroundTaskService registration a few lines above is guarded with if (!services.Any(s => s.ServiceType == typeof(IBackgroundTaskService))) "to support test mocks". The new TenantAgentDeactivationService registration doesn't follow that same guard.
Fix: For consistency (and in case a test ever substitutes ITenantAgentDeactivationService via the same DI container), consider mirroring the existing guarded-registration pattern.
| { | ||
| private readonly Channel<TenantAgentDeactivationRequest> _queue = | ||
| Channel.CreateUnbounded<TenantAgentDeactivationRequest>( | ||
| new UnboundedChannelOptions { SingleReader = true, SingleWriter = false }); |
There was a problem hiding this comment.
SUGGESTION: The channel is configured SingleReader = true, so one tenant with a very large agent count will serialize behind it the processing of every tenant disabled afterward (no cross-tenant fairness).
Fix: Likely acceptable given there's no stated SLA, but worth a short comment confirming this is an intentional trade-off (otherwise consider multiple concurrent readers or a per-tenant batch time cap).
2 participants
… management