Clarify restart-safe batches and scripted UI handler tests

evaluation/review-fixtures.json

@@ -17,7 +17,8 @@
       "article": "set-defaultimplementation-on-enum"
     },
     "performance": {
-      "article": "use-isempty-for-existence-check"
+      "article": "avoid-commit-inside-loops",
+      "context": "A single worker applies a one-time credit-limit increase over a stable customer set. Failed runs may be retried for the same operation."
     },
     "privacy": {
       "article": "no-pii-in-telemetry-message-string"
@@ -29,7 +30,8 @@
       "article": "telemetry-event-id-stable-unique"
     },
     "testing": {
-      "article": "ui-handlers-in-tests"
+      "article": "ui-handlers-in-tests",
+      "context": "ConfirmPostingAndShowMessage must verify exactly one 'Post this document?' confirmation, reply true, then exactly one 'Posting completed.' message in that order. The Customer Card scenarios verify the selected customer's identity; a credit-limit notification is conditional."
     },
     "upgrade": {
       "article": "initvalue-does-not-update-existing-rows",

microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al

@@ -1,26 +1,61 @@
+query 50127 "Perf Customer Chunk"
+{
+    QueryType = Normal;
+    OrderBy = ascending(CustomerNo);
+
+    elements
+    {
+        dataitem(Customer; Customer)
+        {
+            column(CustomerNo; "No.") { }
+        }
+    }
+}
+
 codeunit 50129 "Perf Sample CommitInLoop Bad"
 {
-    procedure NormalizeCustomerNames()
+    procedure IncreaseCustomerCreditLimits()
     var
-        Customer: Record Customer;
         LastCustomerNo: Code[20];
-        ProcessedCount: Integer;
     begin
-        Customer.SetFilter("No.", '>%1', LastCustomerNo);
-        if Customer.FindSet(true) then
-            repeat
-                Customer.Name := UpperCase(Customer.Name);
-                Customer.Modify();
+        while IncreaseNextChunk(LastCustomerNo) do
+            Commit();
+    end;
 
-                // LastCustomerNo exists only in memory, so a retry cannot exclude
-                // work that was already committed.
-                LastCustomerNo := Customer."No.";
-                ProcessedCount += 1;
+    local procedure IncreaseNextChunk(var LastCustomerNo: Code[20]): Boolean
+    var
+        Customer: Record Customer;
+        TempCustomer: Record Customer temporary;
+        CustomerChunk: Query "Perf Customer Chunk";
+        ChunkStartedAt: DateTime;
+        MaxChunkDuration: Duration;
+    begin
+        ChunkStartedAt := CurrentDateTime();
+        MaxChunkDuration := 60000;
+        CustomerChunk.TopNumberOfRows(500);
+        if LastCustomerNo <> '' then
+            CustomerChunk.SetFilter(CustomerNo, '>%1', LastCustomerNo);
+        CustomerChunk.Open();
+        while CustomerChunk.Read() do begin
+            TempCustomer.Init();
+            TempCustomer."No." := CustomerChunk.CustomerNo;
+            TempCustomer.Insert();
+        end;
+        CustomerChunk.Close();
+
+        if TempCustomer.IsEmpty() then
+            exit(false);
+
+        Customer.LockTable();
+        if TempCustomer.FindSet() then
+            repeat
+                if Customer.Get(TempCustomer."No.") then begin
+                    Customer."Credit Limit (LCY)" += 100;
+                    Customer.Modify();
+                end;
+                LastCustomerNo := TempCustomer."No.";
+            until (TempCustomer.Next() = 0) or (CurrentDateTime() - ChunkStartedAt >= MaxChunkDuration);
 
-                // This still opened a FindSet over the complete remaining tail;
-                // periodic commits do not turn retrieval into bounded TOP X.
-                if ProcessedCount mod 500 = 0 then
-                    Commit();
-            until Customer.Next() = 0;
+        exit(true);
     end;
 }

microsoft/knowledge/performance/avoid-commit-inside-loops.good.al

@@ -14,33 +14,35 @@ query 50127 "Perf Customer Chunk"
 
 codeunit 50128 "Perf Sample CommitInLoop Good"
 {
-    procedure NormalizeCustomerNames()
+    procedure IncreaseCustomerCreditLimits()
     var
-        NormalizeState: Record "Perf Normalize State";
+        CreditLimitState: Record "Perf Credit Limit State";
         LastCustomerNo: Code[20];
     begin
-        if not NormalizeState.Get('CUSTOMER') then begin
-            NormalizeState.Init();
-            NormalizeState.Code := 'CUSTOMER';
-            NormalizeState.Insert();
+        if not CreditLimitState.Get('CUSTOMER') then begin
+            CreditLimitState.Init();
+            CreditLimitState.Code := 'CUSTOMER';
+            CreditLimitState.Insert();
         end;
-        LastCustomerNo := NormalizeState."Last Customer No.";
+        LastCustomerNo := CreditLimitState."Last Customer No.";
 
-        while NormalizeNextChunk(LastCustomerNo) do begin
-            // Persist progress in the same transaction as the completed chunk.
-            NormalizeState."Last Customer No." := LastCustomerNo;
-            NormalizeState.Modify();
+        while IncreaseNextChunk(LastCustomerNo) do begin
+            CreditLimitState."Last Customer No." := LastCustomerNo;
+            CreditLimitState.Modify();
             Commit();
         end;
     end;
 
-    local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]): Boolean
+    local procedure IncreaseNextChunk(var LastCustomerNo: Code[20]): Boolean
     var
         Customer: Record Customer;
         TempCustomer: Record Customer temporary;
         CustomerChunk: Query "Perf Customer Chunk";
-        LastChunkCustomerNo: Code[20];
+        ChunkStartedAt: DateTime;
+        MaxChunkDuration: Duration;
     begin
+        ChunkStartedAt := CurrentDateTime();
+        MaxChunkDuration := 60000;
         CustomerChunk.TopNumberOfRows(500);
         if LastCustomerNo <> '' then
             CustomerChunk.SetFilter(CustomerNo, '>%1', LastCustomerNo);
@@ -49,7 +51,6 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
             TempCustomer.Init();
             TempCustomer."No." := CustomerChunk.CustomerNo;
             TempCustomer.Insert();
-            LastChunkCustomerNo := CustomerChunk.CustomerNo;
         end;
         CustomerChunk.Close();
 
@@ -60,17 +61,17 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
         if TempCustomer.FindSet() then
             repeat
                 if Customer.Get(TempCustomer."No.") then begin
-                    Customer.Name := UpperCase(Customer.Name);
+                    Customer."Credit Limit (LCY)" += 100;
                     Customer.Modify();
                 end;
-            until TempCustomer.Next() = 0;
+                LastCustomerNo := TempCustomer."No.";
+            until (TempCustomer.Next() = 0) or (CurrentDateTime() - ChunkStartedAt >= MaxChunkDuration);
 
-        LastCustomerNo := LastChunkCustomerNo;
         exit(true);
     end;
 }
 
-table 50128 "Perf Normalize State"
+table 50128 "Perf Credit Limit State"
 {
     fields
     {

microsoft/knowledge/performance/avoid-commit-inside-loops.md

@@ -1,30 +1,36 @@
 ---
 bc-version: [all]
 domain: performance
-keywords: [commit, commit-in-loop, per-row-commit, checkpoint, bounded-checkpoint, watermark, topnumberofrows]
+keywords: [commit, commit-in-loop, checkpoint, watermark, retry, idempotent, elapsed-time, batch, topnumberofrows]
 technologies: [al]
 countries: [w1]
 application-area: [all]
 ---
 
-# Do not Commit inside loops
+# Commit batches at restart-safe business boundaries
 
 > Contributions welcome — open a PR to refine or extend this article.
 
 ## Description
 
-Commit ends the current write transaction. Calling it inside a per-row loop usually produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with batching. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`).
+[Commit ends the current write transaction](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/database/database-commit-method). Committing every row can add transaction overhead and prevent whole-operation rollback, but deliberate commits after N completed business units or an elapsed-time threshold can be valid for long-running work. Most loops need no explicit Commit at all; see [implicit transaction boundaries](understand-implicit-transaction-boundary.md).
 
-A durability checkpoint inside an outer batch loop can be valid only when the same transaction persists a progress marker or state that makes retries strictly exclude completed work, the checkpoint follows a complete business unit, and errors propagate instead of being swallowed. Restart safety and bounded retrieval are separate requirements: a persisted watermark can make retries safe, but an outer `FindSet` over the full remaining tail with periodic commits still retrieves the complete set because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next).
+Restart safety concerns business effects, not whether a retry revisits a row. A durable checkpoint or processed state can exclude completed work, while demonstrably idempotent replay or durable deduplication can make revisiting it safe. For example, repeating a pure uppercase-name assignment wastes work but does not by itself demonstrate data corruption; repeating an increment can apply it twice.
 
 ## Best Practice
 
-If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Persist the last selected key in the same transaction as the completed chunk, then commit after the bounded helper returns. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. Let errors escape so failed work is not recorded as complete. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
+Choose the commit cadence separately from the retry strategy. Check the row count or elapsed time only after a complete business unit, and commit any final partial batch. A time threshold checked between units is not a fixed-duration guarantee: retrieval, locking, or a single slow unit can exceed it. Let errors propagate so uncommitted work rolls back.
+
+When correctness depends on excluding completed work, persist its checkpoint or processed state in the same transaction as the corresponding business changes. Resume from that committed state, never from a key merely selected for future processing. Alternatively, establish that replay is idempotent or deduplicated for all effects, including external effects; a missing watermark alone is not a correctness finding.
+
+Bounded retrieval is a separate performance requirement. Periodic commits do not cap a full-tail `FindSet`, because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next). When a bounded next-N batch is needed, a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) can fill a temporary key buffer; process only those keys, not an inclusive range that concurrent inserts could expand. Use a stable ordering key and define how to handle records inserted at or below a committed watermark. Do not require bounded retrieval solely because a loop commits.
+
+The paired samples apply a one-time credit-limit increase with one worker over a stable customer set. Both select at most 500 exact keys and finish a chunk after processing them or reaching a one-minute elapsed-time threshold, whichever is observed first. The good sample commits the last processed key with the increases; the bad sample keeps that key only in memory, so a retry can increase already committed limits again. AL DateTime subtraction produces a [Duration in milliseconds](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/duration/duration-data-type).
 
 See sample: [`avoid-commit-inside-loops.good.al`](avoid-commit-inside-loops.good.al).
 
 ## Anti Pattern
 
-Placing Commit inside `repeat ... until Next() = 0` without persisted progress is almost always a mistake: retries re-enter already committed work, while the cost of starting a transaction on every row dominates the operation. A progress variable held only in memory is not restart-safe. A full-tail `FindSet` with a commit every N rows is not bounded retrieval, even if a persisted watermark makes it restart-safe. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint.
+Committing an incomplete business unit, persisting a checkpoint ahead of its business changes, or replaying committed non-idempotent effects with only an in-memory progress variable and no deduplication. In the last case, identify the effect a retry duplicates rather than treating all repeated work as corruption. Committing every row without a reason can also waste transaction overhead; this is distinct from deliberate row-count or elapsed-time batching.
 
 See sample: [`avoid-commit-inside-loops.bad.al`](avoid-commit-inside-loops.bad.al).

microsoft/knowledge/testing/ui-handlers-in-tests.bad.al

@@ -13,7 +13,6 @@ codeunit 50401 "Test UI Handler Proof Bad"
 
         Page.RunModal(Page::"Customer Card", Customer);
 
-        // This only proves a value assigned before the action stayed true.
         Assert.IsTrue(ActionSucceeded, 'The customer card action failed.');
     end;
 
@@ -51,6 +50,32 @@ codeunit 50401 "Test UI Handler Proof Bad"
         Page.RunModal(Page::"Customer Card", Customer);
     end;
 
+    [Test]
+    [HandlerFunctions('ConfirmHandler,PostMessageHandler')]
+    procedure ConfirmPostingAndShowMessage()
+    begin
+        Assert.IsTrue(RunPostingThatConfirmsAndMessages(), 'The posting confirmation was declined.');
+    end;
+
+    local procedure RunPostingThatConfirmsAndMessages(): Boolean
+    begin
+        if not Confirm('Post this document?', false) then
+            exit(false);
+        Message('Posting completed.');
+        exit(true);
+    end;
+
+    [ConfirmHandler]
+    procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean)
+    begin
+        Reply := true;
+    end;
+
+    [MessageHandler]
+    procedure PostMessageHandler(MessageText: Text[1024])
+    begin
+    end;
+
     [ModalPageHandler]
     procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card")
     begin

microsoft/knowledge/testing/ui-handlers-in-tests.good.al

@@ -30,6 +30,41 @@ codeunit 50400 "Test UI Handler Capture Good"
         Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.');
     end;
 
+    [Test]
+    [HandlerFunctions('ConfirmHandler,PostMessageHandler')]
+    procedure ConfirmPostingAndShowMessage()
+    begin
+        LibraryVariableStorage.Clear();
+        LibraryVariableStorage.Enqueue('Post this document?');
+        LibraryVariableStorage.Enqueue(true);
+        LibraryVariableStorage.Enqueue('Posting completed.');
+
+        Assert.IsTrue(RunPostingThatConfirmsAndMessages(), 'The posting confirmation was declined.');
+
+        LibraryVariableStorage.AssertEmpty();
+    end;
+
+    local procedure RunPostingThatConfirmsAndMessages(): Boolean
+    begin
+        if not Confirm('Post this document?', false) then
+            exit(false);
+        Message('Posting completed.');
+        exit(true);
+    end;
+
+    [ConfirmHandler]
+    procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean)
+    begin
+        Assert.ExpectedConfirm(LibraryVariableStorage.DequeueText(), Question);
+        Reply := LibraryVariableStorage.DequeueBoolean();
+    end;
+
+    [MessageHandler]
+    procedure PostMessageHandler(MessageText: Text[1024])
+    begin
+        Assert.ExpectedMessage(LibraryVariableStorage.DequeueText(), MessageText);
+    end;
+
     [ModalPageHandler]
     procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card")
     begin
@@ -45,5 +80,6 @@ codeunit 50400 "Test UI Handler Capture Good"
     var
         Assert: Codeunit "Library Assert";
         LibrarySales: Codeunit "Library - Sales";
+        LibraryVariableStorage: Codeunit "Library - Variable Storage";
         CapturedCustomerNo: Code[20];
 }