feat(tenant): deactivate all agents when a tenant is disabled - #522
Diyanada99x wants to merge 3 commits into
Conversation
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).
|
🔍 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}
hasith
left a comment
hasith
left a comment
There was a problem hiding this comment.
PR Review Report
PR: feat(tenant): deactivate all agents when a tenant is disabled
Author: Diyanada99x
Files Changed: 3 | +84 / -2
Verdict: NEEDS DISCUSSION
Summary
This PR closes a real gap: disabling a tenant previously left its agent activations (Temporal workflows/schedules) running. UpdateTenant now walks active activations and deactivates them one at a time via ActivationService.DeactivateAgentAsync, with best-effort error handling and an audit-message summary. The core logic is sound and appropriately defensive (per-activation try/catch, sanitized logging), but there are two behavioral gaps worth the author's attention before merge, plus no test coverage for the new cascade.
Critical Issues (Must Fix)
No critical issues found.
Warnings (Should Fix)
-
XiansAi.Server.Src/Shared/Services/TenantService.cs:837— The deactivation retry path loses its audit trail.agentSummaryis computed wheneverrequestedEnabled == false(including retries where the tenant is already disabled), but it's only appended to the emitted domain event/audit description whenenabledChangedis also true (line 837'sif). Since retries by definition don't changeEnabled, a retried disable that fails again (or succeeds) produces no event/webhook/audit record at all — the caller has no way to see the retry's outcome except server logs. Note: simply widening the condition toenabledChanged || !string.IsNullOrEmpty(agentSummary)is not a safe drop-in fix — the description text at line 858 assumes a state transition happened ("It was previously {(enabled ? "disabled" : "enabled")}"), so firing on a no-op retry would incorrectly claim the tenant "was previously enabled" when it was already disabled. This needs a distinct message/path for the retry case, which is an author judgment call. -
XiansAi.Server.Src/Shared/Services/TenantService.cs:1132—DeactivateActivationsForTenantAsyncdeactivates activations sequentially, in-band inside theUpdateTenantrequest handler (called synchronously from bothTenantEndpoints.cs:76andAdminTenantEndpoints.cs:411). Each iteration'sDeactivateAgentAsyncperforms real Temporal calls (cancel/terminate workflows, delete schedules) viaCleanupActivationResourcesAsync. For a tenant with many active activations, this can make thePUT/update-tenant request block for a long time and risk client/gateway timeouts, with no bound on the number of activations processed per request. Consider fire-and-forget/background processing (e.g. a queued job) or at least a cap with pagination/continuation, especially since the code already anticipates multi-call retries.
(No suggestion blocks included — both fixes require a design decision from the author rather than a drop-in code change.)
Suggestions (Consider Improving)
-
XiansAi.Server.Src/Shared/Services/TenantService.cs:1111— No tests were added forDeactivateActivationsForTenantAsync/the new disable-cascade behavior (the only touched test file area,TenantServiceMetadataTests.cs, is unrelated — it covers metadata, not enable/disable). Given this changes production behavior for every tenant-disable call (cancelling live workflows), unit tests covering: normal deactivation, partial failure (some activations fail), full lookup failure, and the "already disabled, retry" path would materially reduce risk here.
Review Details
Code Quality
Clean, well-commented addition. The TenantAgentDeactivation record and its Describe() method give a nice compact summary type. Per-activation error handling (try/catch around each DeactivateAgentAsync call) correctly prevents one failure from blocking the rest of the batch.
Security
No security issues found. Tenant/activation ownership is still validated inside DeactivateAgentAsync (tenant-id equality check), all logged identifiers go through LogSanitizer.Sanitize, and no new untrusted input is introduced by this diff (only doc-string / description text changes on the endpoints side).
Test Coverage
No new or updated tests accompany this behavioral change. See Suggestions above.
Performance
See the sequential in-request deactivation loop warning above — the main performance concern in this diff.
Files Reviewed
| File | Lines Changed | Risk | Notes |
|---|---|---|---|
XiansAi.Server.Src/Shared/Services/TenantService.cs |
+82/-1 | 🟡 Medium | New cascade-deactivation logic; audit-gap and in-request performance concerns above |
XiansAi.Server.Src/Features/WebApi/Endpoints/TenantEndpoints.cs |
+1/-1 | 🟢 Low | Doc-string only |
XiansAi.Server.Src/Features/AdminApi/Endpoints/AdminTenantEndpoints.cs |
+2/-0 | 🟢 Low | Doc-string only |
| var deactivated = 0; | ||
| var failed = 0; | ||
|
|
||
| foreach (var activation in activations) |
There was a problem hiding this comment.
Warning: DeactivateActivationsForTenantAsync runs sequentially, in-band, inside the UpdateTenant request handler (called synchronously from both TenantEndpoints.cs and AdminTenantEndpoints.cs). Each iteration's DeactivateAgentAsync performs real Temporal calls (cancel/terminate workflows, delete schedules). For a tenant with many active activations this can make the update-tenant request block for a long time with no bound, risking client/gateway timeouts.
Fix: Consider moving this to background/async processing (e.g. a queued job) or bounding/paginating the batch so a single HTTP request can't be blocked on an unbounded number of Temporal calls.
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Unexpected error deactivating agents of disabled tenant {TenantId}", LogSanitizer.Sanitize(job.TenantId)); | ||
| } |
| catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) | ||
| { | ||
| } |
2 participants
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).