Uptake agent archiving in Sales Order Agent (AB#644549)

[Qasim (qasimikram)]

What & why

Uptake of the agent archiving capability (#8967, plus the follow-up fixes in #10032) in the Sales Order Agent app.

Archiving is a one-way soft delete: the agent must be deactivated first, it stays in the database as read-only, and the platform blocks writes to Agent / AgentData. App-owned tables and app logic are not covered by that gate, so the SOA app has to tell archived agents apart itself.

Instance limit counts active agents only

AllowCreateNewSOAgent counted every SOA Setup row owned by the user against the limit of 20. An archived agent can never be reactivated, so it no longer occupies a slot.

GetSOAUsername looked for a free user name only up to MaxSOAInstances() + 10. Archived agents keep their user name forever, so that bound could be exhausted; it now uses the existing MaxAgentIdentitySuffix() (99).

Identity and mailbox can be reused, but are not suggested

ValidateAgentIdentity (display name, initials) and CheckMailboxUnique (email account plus folder) ignore archived agents, so an administrator can deliberately reuse the identity and mailbox of an archived agent.

The suggested identity for a new agent deliberately does not reuse them. Archived agents keep their place in the series, so an archived SO2 next to a live SO3 and SO4 yields SO5 rather than reusing SO2.

Setup experience is read-only for archived agents

The platform still opens the provider setup page for an archived agent. Page 4400 resolves the archived state on open, makes the page non-editable, shows a notification and skips ApplySetup. The create-task and signature affordances are disabled rather than silently ignored, and the mailbox and folder assist-edits explain why the click did nothing. UpdateAgent and UpdateInstructions are guarded as defence in depth for non-UI callers.

Scheduled tasks do not outlive an active agent

Deactivating an agent from the Agent List or Agent Card never runs SOA code, and archiving is only possible from those pages, so tasks could stay registered and keep running. Two changes:

• SOA Dispatcher and SOA Recovery check first whether the agent is still live. If it is deactivated, archived or gone, they cancel the tasks, clear the ids and stop instead of rescheduling.
• Cancellation is by lookup on Scheduled Task (Run Codeunit + Company + Record) rather than only the two ids stored on the setup row, so duplicates and tasks whose id was lost are removed too. Since RemoveScheduledTask runs on every dispatcher cycle before rescheduling, accumulated duplicates are cleaned up automatically.

A refused cancellation (a session may not cancel a task another user scheduled) does not fail the caller; the task is retried on the next cleanup and the outcome is logged as an error rather than as a completed removal.

Upgrade is archive-aware

• AlwaysUpdateAgentInstructionsOnUpgrade runs on every upgrade with no upgrade tag and calls Agent.SetInstructions for every setup row. For an archived agent that fails, is swallowed by the TryFunction, and logs warning telemetry 0000U1P on every upgrade forever. Archived agents are skipped, as they are in UpgradeAgentIdentity. Both are write paths, so they use the fail-closed check.
• UpgradeSOAKPIToPerAgent attributes legacy KPI rows to a non-archived agent, because the KPI pages skip archived agents. If every agent is archived it falls back to the first setup, since the legacy table is deleted afterwards and skipping would destroy the history.

Smaller archive-aware fixes

• SOA Impl.GetInstanceOffset counted archived siblings when staggering dispatcher start times, pushing live agents further out over time.
• SOA KPI.GetCurrentUserAgentSecurityID could pick an archived agent and open a dead KPI page.
• The awareness notification suppressed itself whenever any SOA Setup row existed. An environment left with only archived agents is now treated like one that never had an agent, in two bounded queries because that path runs for ordinary users.

Reading archived state safely

Agent is a virtual table. The platform builds each record by calling back into the owning app's agent metadata provider (GetInitials, GetSetupPageId, IsManualAgentTaskCreationEnabled and others). Any read of that table, Get, FindSet, Count or a filtered IsEmpty, is served by building records, so a read from one of those methods re-enters it until the stack runs out. Two revisions of this PR did that and produced a NavNCLStackOverflowException; filtering bounds the work but does not avoid the materialisation.

The rule this app follows is therefore absolute: nothing the platform can invoke while it builds an agent record may read the Agent table in any form. IsManualAgentTaskCreationEnabled is kept free of agent reads for that reason, and the platform already excludes archived agents from task creation through CanCurrentUserUseAgent. CreateManualAgentTask runs from the task pane action, outside record building, so it does guard the operation.

IsAgentArchived and IsAgentActive are called only from outside that path. They filter on User Security ID, Agent Metadata Provider and Substate / State, which keeps each check to the single agent it is about rather than the whole table, and stops it from materialising archived agents belonging to other apps.

Two probes exist on purpose. IsAgentArchived is lenient: a session that cannot read the Agent table sees "not archived", so counting and advisory callers are never blocked. MustTreatAgentAsArchived fails closed and backs every guard that blocks an operation, so an unreadable state cannot be used to bypass the read-only rule.

Telemetry

Two new events on the scheduled-task cleanup path: 0000V3K when every task was removed, 0000V3L when one could not be cancelled and was left for the next attempt.

Linked work

AB#644549

How I validated this

• I read the full diff and it contains only changes I intended.
• I built the affected app(s) locally with no new analyzer warnings.
• I ran the change in Business Central and confirmed it behaves as expected.
• I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome

• Compiled Sales Order Agent with the 18.0 AL compiler against the 29.0 symbol set, with the repo base.ruleset.json plus CodeCop and UICop: clean, no errors and no new warnings.
• Ran the change in a local environment. Two defects found this way and fixed here: the stack overflow described above, hit when opening a view that materialises agent records, and the suggested initials reusing an archived agent's place in the series.
• Verified against an environment holding archived agents with leftover SOA Recovery tasks, which is what motivated the lookup-based cancellation.
• No automated tests are included: BCApps has no test project for the Sales Order Agent app, its tests live in the internal repository. Coverage tracked there: a slot freed after archiving, identity and mailbox reuse, the setup page being read-only, manual task creation blocked, upgrade skipping archived agents, scheduled-task cleanup for a deactivated and for an archived agent, and the fail-closed path when the Agent table cannot be read.

Risk & compatibility

• Platform dependency: relies on Agent.Substate from Ability to archive agents #8967. No new app.json dependency was needed.
• Behaviour change, identity: display name, initials and mailbox uniqueness are scoped to non-archived agents, so an archived and a live agent can share a name. Conflict errors still name the offending agent.
• Behaviour change, scheduled tasks: a dispatcher or recovery task belonging to an agent that is not live is now cancelled instead of rescheduled, and cancellation covers every task registered for that setup record rather than only the two remembered ids. A live agent is unaffected.
• No data loss: nothing is deleted and no upgrade step is added. Archived agents keep their tasks, KPIs and logs for auditing, and the legacy KPI migration keeps its history even when every agent is archived.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Error\ Handling}$

The new archived-agent guards in the setup page return immediately from user-invoked handlers without surfacing why the click was ignored. After the initial notification is gone, actions such as "Create task" can degrade into a no-op instead of explicitly telling the user that archived agents are read-only. Disable these affordances when AgentIsArchived, or raise the archived-agent message at the action point.

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.33.4}

[Qasim (qasimikram)]

Fixed in d8c2c64. The archived-agent affordances are now visibly disabled rather than silently ignored: Create task for the agent has Enabled = not AgentIsArchived, and the signature editor is disabled through MailTemplateEditable. Where a handler can still be reached (create-task drilldown, mailbox and folder assist-edits, signature editor) it now shows the archived message instead of returning silently. AgentIsArchived is resolved before UpdateControls so the enabled-state expressions are correct on first render.

[Qasim (qasimikram)]

Pushed 5c8a398 after hitting a NavNCLStackOverflowException while testing this branch. Worth recording here, because it is a trap for any agent app.

What happened. Agent is a virtual table. Its data provider builds every record by calling back into the owning app''s agent metadata provider: AgentVirtualDataProvider.CreateVirtualRecordAsync -> AgentMetadataProvider.GetAgentMetadataAsync, which invokes GetInitials, GetSetupPageId, GetFirstTimeSetupPageId, GetSummaryPageId, IsMultipleFileUploadAllowed and IsManualAgentTaskCreationEnabled (twice). My archived check in IsManualAgentTaskCreationEnabled read the Agent table, which re-entered the same method until the stack ran out. There is no cache or re-entrancy guard on that path.

Why the fix is not just "remove the check". The platform distinguishes two access shapes on this table:

• Get / FindSet build each record, so they run the seven interface callbacks per agent - unsafe from a metadata callback and expensive everywhere else.
• Count / IsEmpty take a fast path straight to the database when every filter is on a field that maps to the underlying agent data table (AgentBaseVirtualDataProvider.CountAccessibleRecordsAsync / AnyAccessibleRecordExistsAsync). No record is built and no AL callback runs.

User Security ID, Agent Metadata Provider and Substate are all source-mapped (AgentVirtualDataProvider.TryGetFieldNoInSourceTable), so IsAgentArchived is now a filtered IsEmpty on those three fields. That makes it safe to call from anywhere and turns the per-row cost into a single keyed lookup - which also resolves the N+1 concern raised on the upgrade thread and the double-read raised on IsAgentArchived, both of which are now moot.

Filtering on the metadata provider matters on its own: without it the check reaches archived agents belonging to other apps and executes their metadata code.

IsManualAgentTaskCreationEnabled stays free of agent lookups even though the new probe would be safe there, because the platform already excludes archived agents (AgentDataSetHelper sets CanCurrentUserUseAgent = ... && !IsArchived) and this method runs for every agent record the platform builds. CreateManualAgentTask still guards the operation itself.

Suggested follow-ups outside this PR, since every agent app will hit this:

1. Agent.IsArchived in the System Application is implemented with GetAgent (a Get), so it carries the same hazard for anyone who calls it from a metadata provider. Reimplementing it as a filtered exists check would make all consumers safe.
2. A provider-scoped helper such as Agent.CountAgents(MetadataProvider, IncludeArchived) would serve the "archived agents do not count towards the instance limit" rule generically.
3. The IAgentMetadata / IAgentManualTaskCreation documentation says only that implementations run in the context of the agent user. It is worth stating that they must not Get or FindSet the Agent table; today the penalty is a stack overflow rather than an error.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade}$

UpgradeSOAKPIToPerAgent still seeds the fallback TargetAgentSecurityID with a plain SOASetup.FindFirst(). In tenants where the first setup now belongs to an archived agent, legacy KPI rows without a stored "User Security ID" will be migrated into that archived agent's summary, while the rest of this PR deliberately excludes archived agents from KPI selection and live usage. Use the same non-archived setup selection here that the runtime code now uses, so migrated KPI totals do not end up attached to an agent the UI skips.

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.33.4}

[Qasim (qasimikram)]

Fixed in 82ce597. UpgradeSOAKPIToPerAgent now seeds the fallback TargetAgentSecurityID with FindFirstNonArchivedSetup instead of a plain FindFirst, so legacy KPI records without a stored User Security ID are attributed to an agent that is still in use. Attaching them to an archived agent would have hidden them, because the KPI page skips archived agents. When every setup belongs to an archived agent the fallback stays empty and those records are counted as skipped and logged, which is the existing behaviour.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Error\ Handling}$

The new inactive-agent cleanup path (RemoveScheduledTasksIfAgentNotActive) is documented as never failing the caller, but RemoveScheduledTask still cancels the two IDs stored on the setup record ("Agent Scheduled Task ID" / "Recovery Scheduled Task ID") with a direct, unguarded TaskScheduler.CancelTask call, while the newly-added CancelScheduledTasksForCodeunit path correctly wraps the same call in the new TryCancelTask TryFunction. If either stored task ID cannot be cancelled by the current session (the same permission edge case the new code explicitly handles for the fallback lookup), CancelTask raises and aborts RemoveScheduledTasksIfAgentNotActive before it commits, clears the IDs, or logs telemetry — undermining the "cleanup must never fail the caller" contract stated in the new method's own doc comment.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        if TaskScheduler.TaskExists(SOASetup."Agent Scheduled Task ID") then begin
            if not TryCancelTask(SOASetup."Agent Scheduled Task ID") then begin
                if CancelErrorCallStack = '' then
                    CancelErrorCallStack := GetLastErrorCallStack();
            end else
                FeatureTelemetry.LogUsage('0000NGN', SOASetupCU.GetFeatureName(), TelemetryAgentScheduledTaskCancelledLbl, TelemetryDimensions);
        end;

        if TaskScheduler.TaskExists(SOASetup."Recovery Scheduled Task ID") then begin
            if not TryCancelTask(SOASetup."Recovery Scheduled Task ID") then begin
                if CancelErrorCallStack = '' then
                    CancelErrorCallStack := GetLastErrorCallStack();
            end else
                FeatureTelemetry.LogUsage('0000NGO', SOASetupCU.GetFeatureName(), TelemetryRecoveryScheduledTaskCancelledLbl, TelemetryDimensions);
        end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.33.4}

[attilatoury]

Review notes, mostly on placement and layering rather than on the behaviour itself. Happy to be told I'm missing context on any of these.

1. RemoveScheduledTasksIfAgentNotActive duplicates a check that already runs three lines later

SOA Setup.State is a FlowField lookup(Agent.State where("User Security ID" = field("User Security ID"))), and CheckSOASetupStillValid already returns false unless that resolves to Enabled. So SOA Dispatcher.RunSOAgent now performs two agent-state checks back to back, reading the same platform state through two different routes — the FlowField, and a direct filtered query on Record Agent.

Since AgentImpl.Archive errors unless State = Disabled, an archived agent is necessarily also disabled, which means CheckSOASetupStillValid was already stopping these dispatchers. What the new code adds that the old check did not is the cancellation of the surviving tasks — that part is real and worth having. But it would be clearer as a single state check feeding a cleanup step, rather than two independent checks with different failure modes sitting next to each other. As written it's easy for a later reader to remove the "redundant" one and silently change behaviour.

2. The cleanup mechanism is general, even though its body is SOA-specific

The body can't move to the System Application — it cancels tasks registered for Codeunit::"SOA Dispatcher"/"SOA Recovery" against a SOA Setup RecordId, which is unavoidably app-specific.

What is general is the approach: lifecycle cleanup implemented as polling from inside the task that is about to run. The comment in the PR states the reason plainly — deactivation removes tasks when it goes through agent setup, so this exists to cover tasks that survived, e.g. when the agent was deactivated from the agent list. That reads as "the platform gives no notification, so each app polls."

AgentImpl.Deactivate/AgentImpl.Archive are right here in this repo. An integration event raised there would give every agent app deterministic teardown instead of each one reinventing this. Was that considered and rejected (timing, VDP constraints, cross-company), or just out of scope? If out of scope, an issue tracking it would help — otherwise the next agent app copies this pattern.

3. IsAgentArchived/IsAgentActive reimplement Agent.IsArchived/Agent.IsActive

The justification in the comments is sound and worth preserving: AgentImpl.GetAgent does a Get on the virtual Agent table, the platform builds the record, and that calls back into this app's metadata provider — so the facade genuinely is unusable from provider-reachable code.

But that's an argument for adding non-building, filter-only lookups to the Agent module, not for copying the workaround into an app. Right now the workaround and the paragraph explaining it live in SOA Setup, where no other agent app can benefit and where nothing stops it drifting from the platform.

It has already drifted: platform IsActive is State = Enabled; SOA Setup.IsAgentActive is State = Enabled and Substate = None. Given the Archive precondition above, that extra filter is either redundant or it's guarding an invariant the platform doesn't actually hold. Worth pinning down which, since the answer determines whether the check is dead code.

4. Three similarly named predicates with three different failure modes

• IsAgentArchived — no read permission ⇒ false (fails open)
• IsAgentActive — no read permission ⇒ true (fails open)
• MustTreatAgentAsArchived — no read permission ⇒ true (fails closed)

The XML docs explain each one, but nothing in the names does, and all three take the same single Guid. A caller picking the wrong one gets a silent behaviour change, not a compile error. Names that encode the policy (e.g. IsAgentArchivedOrUnknown) would make the wrong choice visible at the call site.

5. The fail-open in IsAgentActive may defeat the fix in exactly the scenario it targets

If the task-scheduler session running SOA Dispatcher/SOA Recovery cannot read Agent, IsAgentActive returns true, RemoveScheduledTasksIfAgentNotActive returns false, and the orphaned tasks are never cancelled. CheckSOASetupStillValid would still stop the run via the FlowField, so nothing runs — but the tasks keep rescheduling, which is the problem this PR sets out to solve.

Has this been checked against the permission set those scheduled sessions actually run under? If they always have Agent read access, the fail-open branch is unreachable and could say so; if they don't, this needs another route.

6. Commit() inside a shared helper

RemoveScheduledTasksIfAgentNotActive commits, and it is called from two entry points. The comment justifies it ("the caller is a scheduled task that is about to stop"), which is true of both callers today, but the helper has no way to enforce that and no way to know what the caller had pending. Committing on behalf of callers is worth confining to the entry points themselves.

7. The Boolean return doesn't mean what the name says

Call sites read if SOAImpl.RemoveScheduledTasksIfAgentNotActive(Setup) then exit;. The name suggests "tasks were removed"; the value actually means "agent is not active, stop processing". They coincide today. Either rename to the caller's meaning, or split the query from the command.

8. CountNonArchivedSetups / FindFirstNonArchivedSetup convert set operations into per-record lookups

Several IsEmpty()/Count() calls become FindSet + a filtered Agent query per record — including on ValidateAgentIdentity and the SOA KPI page path. Bounded by the instance cap, so probably fine, but it does mean identity validation and page open now scale with the number of setup records including archived ones, which are precisely the records that accumulate over time and never go away.

9. Minor: naming

SOA Impl follows the BCApps facade/Impl convention (Agent + Agent Impl.) in name only — it's Access = Internal with no public facade in front of it, and has accumulated scheduling, task cancellation, ActiveAgentExistInCurrentCompany and telemetry. Not this PR's problem to fix, but it does mean "it fits the existing contents" isn't much of an argument for placement. This new code is agent-lifecycle logic, and SOA Setup already owns every other agent-state predicate the PR introduces — keeping them together seems more coherent.

None of 2/3/9 need to block the PR if the behaviour is right and there's a follow-up for the platform event. 1, 5 and 6 I'd want answered before merge.

[Qasim (qasimikram)]

Correction to my earlier reply on this thread: the protected cancellation I described could not work, and I have replaced it in 5ffdf1b.

Wrapping TaskScheduler.CancelTask in a [TryFunction] is not valid AL. Cancelling a task deletes its Scheduled Task record, and a database write is not allowed inside a try function, so the code failed at runtime as soon as cleanup ran:

Call to the function 'DELETE' is not allowed inside the call to
'CancelScheduledTasksForRecord' when it is used as a TryFunction.

That applies to the suggestion in this thread as well: the two id-based cancellations cannot be protected that way either, and neither could the sweep that was already using the wrapper.

What the cleanup does now:

• All cancellations are direct calls again, as they were before this PR.
• The outcome is established by re-reading the dispatcher and recovery tasks of the setup record after the cleanup and the commit, instead of being inferred from a caught error. A run that removed everything logs the usage event; a run that left something behind logs the error event. That is a stronger claim than the try-based version, because it reports observed state rather than the absence of an exception.
• Task ids are still collected before anything is cancelled, so removing a row cannot skip the record that follows it.

The remaining exposure is the one this thread raised: if a cancellation is ever refused, the run fails before the ids are cleared. That is the behaviour on main today, and the refusal scenario was a hypothesis rather than something observed. If it proves real, the supported way to isolate it is a small helper codeunit invoked through Codeunit.Run, which permits writes and traps the error. That needs a new object id, so I have left it out rather than adding one speculatively.

[alexei-dobriansky]

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

The platform Agent table gained a Substate (None / Archived). Archiving is a one-way, deactivation-only soft delete; the platform blocks writes to Agent/AgentData, but the app-owned SOA Setup table and app logic are not gated, so this PR makes the Sales Order Agent app archive-aware.

The change is coherent and matches the task. The instance limit and identity-suffix search now exclude or skip archived rows; the setup page resolves the archived state on open, becomes read-only, and every write path (UpdateAgent, UpdateInstructions, CreateManualAgentTask, the assist-edits) is guarded. The key correctness point is handled well: Agent is a virtual table, so the new probes filter and use IsEmpty() only (never Get/FindSet), which avoids the metadata-provider recursion that produced the earlier stack overflow. The lenient (IsAgentArchived/IsAgentActive) vs fail-closed (MustTreatAgentAsArchived) split is applied correctly - counting/advisory callers stay unblocked, blocking guards fail closed. Scheduled-task cleanup targets tasks by SetRange(Record, SOASetup.RecordId), which is correct because the tasks are created with SOASetup.RecordId. Identity/mailbox uniqueness now ignores archived rows, and two live agents still collide, so the scoping is right.

Suggestions

S1 - Refused task cancellation can skip survivor telemetry
CancelScheduledTasksForCodeunit calls TaskScheduler.CancelTask(TaskId) directly. This new path runs from the dispatcher/recovery session for an agent that may have been deactivated from the Agent List by a different user, so a task owned by another user is now a real case. If CancelTask raises when the session may not cancel that task, RemoveScheduledTasksIfAgentNotActive errors before the ScheduledTasksExistForRecord check logs 0000V3L, so the "left for the next attempt" behavior you describe would not happen. If CancelTask can raise on a refused cancellation, wrap it in a TryFunction so the survivor detection and telemetry are always reached.

S2 - New archive-aware branches have no automated coverage in this repo
There are no tests in BCApps for the new branches (slot freed after archiving, identity/mailbox reuse, read-only setup page, blocked task creation, upgrade skip, scheduled-task cleanup, fail-closed read). None of this touches a posting or financial or data-integrity path, and BCApps has no test project for this app, so this is not a blocker. Please keep the internal coverage list in sync with these branches as they change.

Risk assessment and necessity

Risk: The regression surface is agent scheduling and setup, not posting or amounts, so there is no financial exposure. The main behavior change to watch: RemoveScheduledTasksIfAgentNotActive cancels tasks whenever IsAgentActive is false, which includes a merely deactivated (not archived) agent. Resuming such an agent relies on re-activation flowing through SOA setup so ScheduleSOAgent reschedules; a re-enable that never runs SOA code would leave the agent enabled but without tasks. The Commit() in that path is inside a top-level scheduled-task run with no following Error, so it is safe. No public/internal signatures or event surfaces change (new internal procedures are additive), and the upgrade guards leave the tag unset when the Agent table is unreadable, so a later upgrade retries rather than marking the work done - correct and idempotent.

Necessity: The change is required. Without app-side gates, archived Sales Order Agents keep counting against the 20-instance limit, keep their setup page writable, keep suggesting used identities, and keep scheduled tasks running after deactivation - all real defects the platform does not cover for app-owned data. The scope is appropriate for an uptake task.

---

[AI-PR-REVIEW] version=1 promptVersion=2 system=github pr=10308 round=1 by=alexei-dobriansky at=2026-08-18T14:48:33Z lastSha=5ffdf1bfd8bd05b6b0517881215b6fdce5466752 reviewKey=e12f199af6c65ed0651f5ba3bfc3c0a4c4571c9c2f814cb6de9d15ddb83adbca suggestions=S1@fdc98cd1,S2@2cf76bc4