Fix duplicate retention continuations for Waiting jobs - #11257
Fix duplicate retention continuations for Waiting jobs#11257Darrick (darjoo) wants to merge 3 commits into
Conversation
Recognize Waiting retention jobs as pending continuations without restarting them or replacing their scheduled-task IDs. Preserve Ready and On Hold restart behavior and add deterministic regression coverage. AB#649571 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
Team: Integrations
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Replace the separate existence check and bare lookup with a guarded FindFirst under UpdLock. Schedule a continuation when no pending entry matches, while preserving Waiting jobs without restarting them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use a ReadCommitted fast path for Waiting jobs, then guard the update-locked re-read before restarting actionable jobs. Clear stale record identity before creating a replacement continuation. Correct the retention test fixture's attempt counter and remove an ignored using directive that caused AL0789 in Tests-Misc. AB#649571 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| JobQueueEntry.ScheduleJobQueueEntryForLater(Codeunit::"Retention Policy JQ", CurrentDateTime(), JobQueueCategoryTok, '') | ||
| else begin | ||
| JobQueueEntry.SetFilter(Status, '%1|%2|%3', JobQueueEntry.Status::Ready, JobQueueEntry.Status::"On Hold", JobQueueEntry.Status::Waiting); | ||
| if JobQueueEntry.FindFirst() then begin |
There was a problem hiding this comment.
ScheduleContinuation() decides its outcome from two FindFirst() calls over a filter that matches Waiting, Ready, and On Hold entries together, but never establishes a deterministic priority among them. Job Queue Entry is not re-sorted here, so which status is returned first depends on key order, not on status. If a Waiting continuation and a Ready/On Hold continuation ever coexist for the same object, this code can restart the non-waiting row instead of honoring the existing Waiting continuation, leaving a duplicate pending job in place. Consider checking for a Waiting entry with its own filtered lookup before falling back to a separate locked lookup for restartable statuses, so the two cases can't race on row order.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
| Handled := true; | ||
| end; | ||
|
|
||
| internal procedure ScheduleContinuation() |
There was a problem hiding this comment.
Extracting the scheduling logic into an internal procedure (ScheduleContinuation) exposes retention-job scheduling to every companion app listed in BaseApp's internalsVisibleTo, but the procedure no longer enforces the caller's session, user-invoked, or time-window guards that gated the original subscriber-inline logic. Access = Internal is API hygiene, not an authorization boundary, so a privileged scheduling operation reached this way should either stay non-exposed or re-validate its preconditions at the new entry point.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
|
|
||
| RetentionPolicyLog.LogInfo(RetentionPolicyLogCategory::"Retention Policy - Schedule", RescheduleOnLimitExceededLbl); | ||
|
|
||
| ScheduleContinuation(); |
There was a problem hiding this comment.
The info log emitted just before calling ScheduleContinuation() ("The job queue entry was scheduled to run again") is written unconditionally, before the actual outcome is known. After this refactor, ScheduleContinuation() has three distinct outcomes: create a new job entry, restart an existing Ready/On Hold entry, or no-op because a Waiting entry already exists. In the no-op/Waiting branch nothing is (re)scheduled, so the log line records an outcome that didn't happen. Move the log call inside ScheduleContinuation() (or add branch-specific messages) so telemetry reflects what was actually done.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
| [Test] | ||
| [TransactionModel(TransactionModel::AutoRollback)] | ||
| [Scope('OnPrem')] | ||
| procedure RetentionContinuationPreservesWaitingJob() |
There was a problem hiding this comment.
The new retention-continuation tests cover a Waiting-only scenario and a Ready/On-Hold-only scenario, but none seeds a Waiting entry together with a Ready or On Hold entry for the same object. That mixed-state case is exactly the risky one for this change, since ScheduleContinuation()'s FindFirst() over all three statuses is not guaranteed to return the Waiting row first. Add a test that creates both entries at once and asserts the Waiting job is preserved and the other entry is left untouched (not restarted, not duplicated).
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThis change makes retention continuation scheduling treat a Waiting job as pending work, and it extracts the scheduling step into ScheduleContinuation() so tests can call it directly. The main path now preserves an existing Waiting task and still restarts Ready or On Hold jobs. One edge case remains: the lookup mixes Waiting with restartable statuses, so row order can decide the branch when both kinds exist. Problem-solution fitFit: Partial The change matches the reported duplicate-continuation problem and covers the main Waiting, Ready, On Hold, and no-pending-job cases. It does not fully guard the mixed pending-state case where a Waiting entry and a restartable entry are both present for the same retention job. SuggestionsS1 (🔴 High): Check Waiting before restartable jobs Risk assessment and necessityRisk: The changed branch controls retention policy continuation scheduling. A wrong branch can keep extra jobs queued for the same codeunit, causing redundant retention processing and confusing task state. The change is internal and has no schema or public API effect. Necessity: The change is needed because a Waiting continuation means work is already queued. Treating it as missing or secondary can create or preserve redundant jobs during limit-exceeded runs; the scope is otherwise narrow and the tests cover the main states.
|
2 participants
What & why
Retention continuation scheduling only recognizes Ready and On Hold jobs. Once the dispatcher moves an existing continuation to Waiting, another limit-exceeded notification can create a duplicate.
Include Waiting in the pending-job lookup, but skip
Restartfor Waiting entries so their status and scheduled-task ID remain unchanged. Preserve the existing restart behavior for Ready and On Hold jobs. Extract the scheduling step into an internal procedure so its regression coverage does not depend on the overnight scheduling window.Linked work
AB#649571
Azure DevOps Bug 649571
An approved GitHub issue has not been provided.
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Job Queue Entry Tests: repeated requests preserve a Waiting continuation and its task ID; Ready jobs restart; On Hold jobs restart; and a continuation is created when only a running retention job and an unrelated Waiting job exist. They use the existing task-scheduler mock and automatic rollback.Test-ObjectIDsAreValidcheck passed for both modified codeunits, andgit diff --checkpassed.Risk & compatibility
No schema, permission, telemetry, category, or scheduling-window changes. Waiting continuations remain under dispatcher control; existing Ready and On Hold restart behavior is unchanged. The extracted scheduling procedure is internal.