Skip to content

Fix retention policy deletion-limit stop propagation - #11262

Open
Darrick (darjoo) wants to merge 1 commit into
mainfrom
darjoo-retention-deletion-limit
Open

Fix retention policy deletion-limit stop propagation#11262
Darrick (darjoo) wants to merge 1 commit into
mainfrom
darjoo-retention-deletion-limit

Conversation

@darjoo

@darjoo Darrick (darjoo) commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What & why

Background retention runs execute each policy in an isolated Codeunit.Run call. The child returns the cumulative deletion count but loses EndCurrentRun, so the outer loop continues processing later policies and can raise additional limit events after the batch should have stopped.

Return the stop flag through the temporary retention policy setup record alongside the deletion count, and import it only after successful child execution. This preserves per-policy error isolation while allowing the existing outer-loop stop condition to take effect.

Linked work

AB#649569

Azure DevOps Bug 649569: Retention policies continue past the deletion limit because EndCurrentRun is not propagated

Approved GitHub issue: not provided; the source work item is in Azure DevOps.

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 (required - be specific: scenarios, commands, screenshots for UI changes)

  • git diff --check passed.
  • Added AL regression cases for reaching the exact limit, stopping with records remaining, continuing below the limit, and continuing after a policy error. The limit cases also assert that the limit event is raised only once, and explicitly start a subsequent run to process remaining records.
  • AL compilation and runtime execution are pending: Docker is unavailable, and no usable local AL compiler or module symbol cache was available. The subsequent-run assertions do not establish automatic job-queue scheduling. This PR is a draft pending Business Central execution.

Risk & compatibility

  • Adds internal Boolean field 101, End Current Run, to Retention Policy Setup for temporary result transport, following the existing deletion-count field pattern. Existing setup records are not rewritten by the execution path.
  • Leaves the configured deletion allowance, per-policy transaction/error handling, event signature, and continuation scheduler unchanged. The existing limit event is raised before the stop flag returns to the outer loop.
  • ApplyAllRetentionPolicies context propagation is a separate follow-up and is not included here. This change does not redesign scheduling/fairness or guarantee a hard cap for custom or indirect-permission deletion implementations.

Return EndCurrentRun through the temporary retention policy setup record so background processing stops after the deletion-limit event. Preserve per-policy error isolation and cover limit boundaries, continued processing below the limit, policy errors, and subsequent runs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@darjoo
Darrick (darjoo) marked this pull request as ready for review September 9, 2026 10:50
@darjoo
Darrick (darjoo) requested a review from a team September 9, 2026 10:50
@darjoo
Darrick (darjoo) requested a review from a team as a code owner September 9, 2026 10:50
@github-actions github-actions Bot added the Team: Integrations GitHub request for Integrations area label Sep 9, 2026
@github-actions github-actions Bot added this to the Version 30.0 milestone Sep 9, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does

This change carries the retention run stop flag back from the isolated child codeunit to the outer automatic loop. It uses the same temporary setup record pattern that already carries the deleted-record count, so the product fix is narrow and aimed at the right boundary.

The product code imports the flag only after a successful child run, which keeps per-policy error isolation. The added tests cover the right scenarios, but they currently fail during setup, so the PR is not merge-ready yet.

Problem-solution fit

Fit: Strong

The reported problem is that automatic retention processing can continue to later policies after the deletion limit is reached. The code change returns the missing stop state to the loop that owns the stop condition, without changing the deletion limit or event contract.

Suggestions

S1 (🔴 High): Refresh allowed tables before creating setup
The new limit tests fail during setup because table 138700 is not allowed. Call RetentionPolicyTestLibrary.RaiseOnRefreshAllowedTables() before inserting the policies so the tests can verify the limit behavior.

Risk assessment and necessity

Risk: The product change is limited to retention policy background processing and an internal transport field. The main risk is merge readiness: the regression tests that should protect this data-deletion path fail before they exercise the changed behavior.

Necessity: The change is needed because an exhausted deletion budget should stop the current automatic run instead of processing later policies and raising repeated limit events. The scope is appropriate, but the failing test setup must be fixed before merge.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11262 round=1 by=alexei-dobriansky at=2026-09-09T22:30:06Z lastSha=3b1bc3f2cb02468821d0e817d5acb711ab31c8e1 reviewKey=3068e1daf4a3cb383e19e2df89b10115c911bf20761dd8170f28683839d2186c suggestions=S1@63edbc1d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: System Application Team: Integrations GitHub request for Integrations area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants