518 deactivating a tenant should bring down all the agents into an deactivated state by Diyanada99x · Pull Request #530 · XiansAiPlatform/XiansAi.Server

Diyanada99x · 2026-10-01T04:26:16Z

No description provided.

Disabling a tenant only flipped Tenant.Enabled and emitted tenant.disabled; its activations stayed active, Temporal workflows kept running and schedules kept firing. UpdateTenant now deactivates every active activation of the tenant whenever enabled=false is sent, one at a time, via ActivationService.DeactivateAgentAsync (cancel/terminate workflows, delete schedules, mark inactive).

into 518-deactivating-a-tenant-should-bring-down-all-the-agents-into-an-deactivated-state

…d tenants

hasith · 2026-10-01T04:26:53Z

🔍 PR Review in Progress

Claude Code is analyzing this pull request. The review will be posted here shortly.

PR Reviewer (1.9.9)

+                catch (Exception ex)
+                {
+                    _logger.LogError(ex, "Unexpected error deactivating agents of disabled tenant {TenantId}", LogSanitizer.Sanitize(job.TenantId));
+                }


+        catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested)
+        {
+        }


hasith

PR Review Report

PR: 518 deactivating a tenant should bring down all the agents into an deactivated state
Author: Diyanada Gunawrdena
Files Changed: 6 | +185 / -3
Verdict: REQUEST CHANGES

Summary

This PR closes a real gap: disabling a tenant previously left its agents' Temporal workflows and schedules running. The fix introduces a background TenantAgentDeactivationService that queues and asynchronously deactivates a disabled tenant's active activations outside the HTTP request path, which is a sound design for avoiding request timeouts. The main concerns are (1) the new service — including a synthetic SysAdmin-privileged tenant context used to drive deactivation — ships with zero unit tests, and (2) a few correctness/robustness edges (tenant re-enabled mid-run, unchecked enqueue result, unvalidated input) that are worth tightening before merge.

Critical Issues (Must Fix)

XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:33 — The new TenantAgentDeactivationService has no unit tests at all, despite containing the entire feature's logic: Enqueue dedup via _pending, background job execution, the re-enabled-before-job-runs guard, and per-activation failure handling. It also constructs a synthetic ITenantContext with a global SysAdmin role (line 111) to drive ActivationService.DeactivateAgentAsync — untested code that fabricates elevated privileges is a higher-than-usual risk to ship without coverage.
```
// Missing: XiansAi.Server.Tests/UnitTests/Shared/Services/TenantAgentDeactivationServiceTests.cs
// covering: Enqueue returns false while a tenant is already pending; ExecuteAsync processes a
// queued job and clears _pending; tenant re-enabled before the job runs is skipped; per-activation
// failures are logged and counted without aborting the batch.
```

(1 critical issue found.)

Warnings (Should Fix)

XiansAi.Server.Tests/UnitTests/Shared/Services/TenantServiceMetadataTests.cs:247 — UpdateTenant_WhenOnlyEnabledChanges_EmitsDisabledNotUpdated disables a tenant but never asserts _agentDeactivationQueue.Enqueue(...) was called, and there's no test confirming a repeat enabled=false queues the tenant again (the behavior the new code comment explicitly documents). Add _agentDeactivationQueue.Verify(x => x.Enqueue(stored.TenantId, It.IsAny<string>()), Times.Once); to this test, plus a new test for the repeat-disable case.
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:131 — tenant.Enabled is only checked once before the deactivation loop starts. For a tenant with many activations (each deactivation makes a multi-second Temporal call), an admin re-enabling the tenant mid-run has no effect — the loop keeps tearing down the remaining activations anyway. Re-check tenant.Enabled (or re-fetch) inside the loop and stop early if it flips back to enabled.
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:131 — Activations are deactivated strictly sequentially inside the single-reader background worker. A tenant with a large activation count (or one slow/hung Temporal call) monopolizes the only worker loop and blocks every other tenant's queued disable job behind it. Consider bounded intra-job parallelism (e.g. Parallel.ForEachAsync with a cap) so one tenant's backlog can't starve others.
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:111 — The synthetic job context is stamped with the global SystemRoles.SysAdmin role and AuthorizedTenantIds = [tenantId] even though nothing in the current call path (GetActiveActivationsAsync, DeactivateAgentAsync) appears to require it. Granting the broadest system role by default is an unaudited privilege widening that becomes exploitable the moment any code later resolved from this scope checks for SysAdmin. Scope the context to the minimum needed (tenant id + actor for audit attribution) unless a specific downstream check is known to require the elevated role.
XiansAi.Server.Src/Shared/Services/TenantService.cs:836 — _agentDeactivationQueue.Enqueue(...)'s return value (false = already pending, or the channel write failed) is discarded. If the write genuinely fails, the tenant is disabled but its agents silently never get queued for deactivation, with nothing surfaced to the caller or logs at the call site. At minimum log a warning here when Enqueue returns false for a reason other than "already pending".
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:56 — Enqueue's requestedBy parameter isn't validated the way tenantId is, even though it's written straight into tenantContext.LoggedInUser for the audit trail of every deactivation performed. A null/blank value flows through unnoticed. Guard it alongside the existing tenantId check.
```
        if (string.IsNullOrWhiteSpace(tenantId) || string.IsNullOrWhiteSpace(requestedBy) || !_pending.TryAdd(tenantId, 0))
```

(6 warnings found.)

Suggestions (Consider Improving)

XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:121 — GetActiveActivationsAsync loads the tenant's entire active-activation set into memory in one call, held for the whole (potentially long) sequential run. Not a problem at current scale, but consider batching/paging for tenants with very large activation counts.
XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs:110 — Audit entries for each deactivation are attributed solely to job.RequestedBy (the admin who disabled the tenant), with no indication the action actually executed asynchronously via the background service, possibly much later. Consider also recording that execution was system-driven (e.g. a performedBy: "system:tenant-deactivation" field) alongside requestedBy, so the audit trail distinguishes "who asked" from "what executed".

Review Details

Code Quality

Overall clean: the new service is well documented (XML comments explain the queue/worker split and its best-effort nature), uses an appropriately configured Channel for the single-writer/multi-reader-producer pattern, and wires DI correctly (one singleton instance exposed under three interfaces). Minor gap: Enqueue's requestedBy isn't null/whitespace-validated like tenantId is (see warning above).

Security

The core design — queuing deactivation off the request path — is sound, but the synthetic ITenantContext built per job grants a full SysAdmin role and tenant-scoped authorization unconditionally (warning above). This isn't exploitable today as far as the current call graph shows, but it's an unaudited privilege grant that should be minimized rather than relied on to stay unused. No secrets, injection, or input-validation issues were found elsewhere in the diff; LogSanitizer.Sanitize is already applied to all logged identifiers.

Test Coverage

This is the biggest gap in the PR: the new TenantAgentDeactivationService — the entire feature — ships with no unit tests, and the one existing test touched by this change only adds the new mock dependency without asserting the new behavior it's supposed to protect. See the Critical and first Warning items above.

Performance

No N+1 queries or unbounded memory growth; GetActiveActivationsAsync is a single query per job and _pending/the channel scale with the number of tenants being disabled, not activations. The sequential, single-worker processing model is an intentional simplicity trade-off per the code's own comments, but it means a large or slow tenant can starve other tenants' queued deactivations (warning above).

Files Reviewed

File	Lines Changed	Risk	Notes
`XiansAi.Server.Src/Shared/Services/TenantAgentDeactivationService.cs`	+160/-0	🔴 High	New background service; impersonated SysAdmin context; untested
`XiansAi.Server.Src/Shared/Services/TenantService.cs`	+14/-1	🟡 Medium	Wires new dependency; enqueues deactivation on disable
`XiansAi.Server.Src/Shared/Configuration/SharedServices.cs`	+5/-0	🟢 Low	DI registration
`XiansAi.Server.Tests/UnitTests/Shared/Services/TenantServiceMetadataTests.cs`	+2/-1	🟡 Medium	Mock wiring added, no new assertions
`XiansAi.Server.Src/Features/AdminApi/Endpoints/AdminTenantEndpoints.cs`	+2/-0	🟢 Low	Docs only
`XiansAi.Server.Src/Features/WebApi/Endpoints/TenantEndpoints.cs`	+1/-1	🟢 Low	Docs only

hasith · 2026-10-01T04:34:32Z

+/// Best-effort: failures are logged only. The queue is in memory, so jobs not yet finished when the
+/// server stops are lost; re-sending enabled=false for the tenant queues it again.
+/// </summary>
+public sealed class TenantAgentDeactivationService : BackgroundService, ITenantAgentDeactivationQueue


CRITICAL: The new TenantAgentDeactivationService has no unit tests at all, despite containing the entire feature's logic: Enqueue dedup via _pending, background job execution, the re-enabled-before-job-runs guard, and per-activation failure handling. It also constructs a synthetic ITenantContext with a global SysAdmin role to drive ActivationService.DeactivateAgentAsync (see line 111) — untested code that fabricates elevated privileges is a higher-than-usual risk to ship without coverage.

Fix: Add XiansAi.Server.Tests/UnitTests/Shared/Services/TenantAgentDeactivationServiceTests.cs covering: Enqueue returns false while a tenant is already pending; ExecuteAsync processes a queued job and clears _pending; a tenant re-enabled before the job runs is skipped; per-activation failures are logged and counted without aborting the batch.

hasith · 2026-10-01T04:34:34Z

+        var deactivated = 0;
+        var failed = 0;
+
+        foreach (var activation in activations)


WARNING: tenant.Enabled is only checked once before the deactivation loop starts. For a tenant with many activations (each deactivation makes a multi-second Temporal call), an admin re-enabling the tenant mid-run has no effect — the loop keeps tearing down the remaining activations anyway.

Fix: Re-check tenant.Enabled (or re-fetch it) inside the loop and stop early if it has flipped back to enabled.

hasith · 2026-10-01T04:34:35Z

+        var deactivated = 0;
+        var failed = 0;
+
+        foreach (var activation in activations)


WARNING: Activations are deactivated strictly sequentially inside the single-reader background worker (SingleReader = true on the channel, one ExecuteAsync loop). A tenant with a large activation count, or one slow/hung Temporal call, monopolizes the only worker loop and blocks every other tenant's queued disable job behind it.

Fix: Consider bounded intra-job parallelism (e.g. Parallel.ForEachAsync with a capped degree of parallelism) so one tenant's backlog can't starve others, while still keeping cross-tenant jobs serialized or on a small worker pool.

hasith · 2026-10-01T04:34:36Z

+        var tenantContext = services.GetRequiredService<ITenantContext>();
+        tenantContext.TenantId = job.TenantId;
+        tenantContext.LoggedInUser = job.RequestedBy;
+        tenantContext.UserRoles = new[] { SystemRoles.SysAdmin };


WARNING: The synthetic job context is stamped with the global SystemRoles.SysAdmin role and AuthorizedTenantIds = [tenantId] even though nothing in the current call path (GetActiveActivationsAsync, DeactivateAgentAsync) appears to require it. Granting the broadest system role by default is an unaudited privilege widening that becomes exploitable the moment any code later resolved from this scope checks for SysAdmin.

Fix: Scope the context to the minimum needed (tenant id + actor for audit attribution) unless a specific downstream check is known to require the elevated role.

hasith · 2026-10-01T04:34:37Z

+            // Re-sending enabled=false queues it again. Re-enabling does not bring agents back.
+            if (result.IsSuccess && requestedEnabled == false)
+            {
+                _agentDeactivationQueue.Enqueue(existingTenant.TenantId, _tenantContext.LoggedInUser);


WARNING: _agentDeactivationQueue.Enqueue(...)'s return value (false = already pending, or the channel write failed) is discarded. If the write genuinely fails, the tenant is disabled but its agents silently never get queued for deactivation, with nothing surfaced to the caller or logs at the call site.

Fix: At minimum, log a warning here when Enqueue returns false for a reason other than "already pending".

hasith · 2026-10-01T04:34:38Z

+
+    public bool Enqueue(string tenantId, string requestedBy)
+    {
+        if (string.IsNullOrWhiteSpace(tenantId) || !_pending.TryAdd(tenantId, 0))


WARNING: Enqueue's requestedBy parameter isn't validated the way tenantId is, even though it's written straight into tenantContext.LoggedInUser for the audit trail of every deactivation performed. A null/blank value flows through unnoticed.

Fix: Guard it alongside the existing tenantId check.

Suggested change

if (string.IsNullOrWhiteSpace(tenantId) || !_pending.TryAdd(tenantId, 0))

if (string.IsNullOrWhiteSpace(tenantId) || string.IsNullOrWhiteSpace(requestedBy) || !_pending.TryAdd(tenantId, 0))

hasith · 2026-10-01T04:34:39Z

+            return;
+        }
+
+        var activations = await services.GetRequiredService<IActivationRepository>().GetActiveActivationsAsync(job.TenantId);


SUGGESTION: GetActiveActivationsAsync loads the tenant's entire active-activation set into memory in one call, held for the whole (potentially long) sequential run. Not a problem at current scale, but it doesn't scale for tenants with very large activation counts.

Fix: Consider batching/paging the activation list for very large tenants rather than materializing it all up front.

hasith · 2026-10-01T04:34:40Z

+        // actor and tenant from the scoped tenant context.
+        var tenantContext = services.GetRequiredService<ITenantContext>();
+        tenantContext.TenantId = job.TenantId;
+        tenantContext.LoggedInUser = job.RequestedBy;


SUGGESTION: Audit entries for each deactivation are attributed solely to job.RequestedBy (the admin who disabled the tenant), with no indication the action actually executed asynchronously via the background service, possibly much later.

Fix: Consider also recording that execution was system-driven (e.g. a performedBy: "system:tenant-deactivation" field) alongside requestedBy, so the audit trail distinguishes "who asked" from "what executed".

hasith · 2026-10-01T04:34:59Z

            _knowledgeRepository.Object,
-            _audit.Object);
+            _audit.Object,
+            _agentDeactivationQueue.Object);


WARNING: UpdateTenant_WhenOnlyEnabledChanges_EmitsDisabledNotUpdated (the test touched by this diff) disables a tenant but never asserts _agentDeactivationQueue.Enqueue(...) was called, and there's no test confirming a repeat enabled=false queues the tenant again (the behavior the new code comment explicitly documents).

Fix: After wiring _agentDeactivationQueue into the constructor here, add _agentDeactivationQueue.Verify(x => x.Enqueue(stored.TenantId, It.IsAny<string>()), Times.Once); to that test, plus a new test for the repeat-disable case.

Diyanada Gunawrdena added 3 commits September 29, 2026 11:02

Merge branch 'main' of https://github.com/XiansAiPlatform/XiansAi.Server

8d7669f

into 518-deactivating-a-tenant-should-bring-down-all-the-agents-into-an-deactivated-state

feat(tenant): implement background deactivation of agents for disable…

2dfa92a

…d tenants

Diyanada99x self-assigned this Oct 1, 2026

Diyanada99x added the ai-dlc/pr/pr-review Ready for AI-DLC PR review label Oct 1, 2026

github-code-quality Bot found potential problems Oct 1, 2026

View reviewed changes

hasith reviewed Oct 1, 2026

View reviewed changes

Diyanada99x closed this Oct 1, 2026

Navigation Menu

518 deactivating a tenant should bring down all the agents into an deactivated state - #530

518 deactivating a tenant should bring down all the agents into an deactivated state#530
Diyanada99x wants to merge 3 commits into
mainfrom
518-deactivating-a-tenant-should-bring-down-all-the-agents-into-an-deactivated-state

Diyanada99x commented Oct 1, 2026

Uh oh!

hasith commented Oct 1, 2026

Uh oh!

hasith left a comment

Uh oh!

hasith Oct 1, 2026

Uh oh!

hasith Oct 1, 2026

Uh oh!

hasith Oct 1, 2026

Uh oh!

hasith Oct 1, 2026

Uh oh!

hasith Oct 1, 2026

Uh oh!

hasith Oct 1, 2026

Uh oh!

hasith Oct 1, 2026

Uh oh!

hasith Oct 1, 2026

Uh oh!

hasith Oct 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if (string.IsNullOrWhiteSpace(tenantId) \|\| !_pending.TryAdd(tenantId, 0))
	if (string.IsNullOrWhiteSpace(tenantId) \|\| string.IsNullOrWhiteSpace(requestedBy) \|\| !_pending.TryAdd(tenantId, 0))

Conversation

Diyanada99x commented Oct 1, 2026

Uh oh!

hasith commented Oct 1, 2026

Uh oh!

hasith left a comment

Choose a reason for hiding this comment

PR Review Report

Summary

Critical Issues (Must Fix)

Warnings (Should Fix)

Suggestions (Consider Improving)

Review Details

Code Quality

Security

Test Coverage

Performance

Files Reviewed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants