fix(sources): reserve the engine's cursor/pageSize bind names from run-time parameters (#253)

Run-time (execution) parameters were bound BEFORE the engine's own @cursor and
@pageSize, and AddParameter skips a name that is already bound — so a caller
passing a parameter named "cursor" pinned the keyset cursor to their value for
every page. A standard keyset query (WHERE (@cursor IS NULL OR Id > @cursor)
ORDER BY Id) then returns the same first page forever: the runner's page loop
only stops on HasMore=false, so the run never terminates and keeps appending to
its temp file. Reachable from one report-run request.

Bind the engine's reserved names first, so the existing first-wins rule protects
them; run-time parameters still beat the source's static ones for every other
name. Applied to both the read and the row-count query. Covered by a Sqlite
integration test verified to fail without the fix (page 2 repeated page 1).

Author: thiagoluga

CHANGELOG.md

@@ -58,6 +58,13 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
   the host — D20 — this is a nudge, not a behaviour change).
 
 ### Fixed
+- **A run-time parameter can no longer take over the engine's keyset cursor.** Execution parameters
+  were bound before the engine's own `@cursor`/`@pageSize`, and the binder keeps whichever name was
+  bound first — so a report run supplying a parameter called `cursor` pinned it for every page. The
+  keyset query then returned the same first page indefinitely and the run never terminated (the page
+  loop only stops when a source reports no more data), growing its staging file the whole time. The
+  engine's reserved names are now bound first; run-time parameters still override the source's static
+  ones for every other name.
 - A job whose source times out is now recorded as **Failed**, not "Cancelled". The worker caught
   every `OperationCanceledException`, including the `TaskCanceledException` an `HttpClient.Timeout`
   raises (its token is not the run's), so a real failure was labelled as an operator-initiated

src/Sources/NeoReports.Sources.Common/AdoKeysetSource.cs

@@ -90,20 +90,23 @@ public async Task<BatchResult<T>> ReadBatchAsync(BatchContext context, Cancellat
         command.CommandText = _sql;
         _configureCommand?.Invoke(command);
 
+        // The engine's own bind names go FIRST so nothing can take them over: AddParameter skips a
+        // name that is already bound, so whichever is added first wins. A run-time parameter called
+        // "cursor" would otherwise pin the keyset cursor to a caller-supplied value for every page —
+        // the query keeps returning the same first page and the run never terminates. "pageSize" is
+        // reserved for the same reason (it caps rows per page without requiring TOP/OFFSET/LIMIT/
+        // ROWNUM in the user's SQL, and drives the has-more decision).
+        AddParameter(command, "cursor", DecodeCursor(context.Cursor), _parameterPrefix);
+        AddParameter(command, "pageSize", _pageSize, _parameterPrefix);
+
         // Merge run-time (execution) parameters with the source's static parameters. Run-time values
         // are added FIRST so that when a report supplies a per-run override of a name the author also
-        // set statically (a tenant id, a date range), the run-time value wins — AddParameter skips a
-        // name that is already bound, so whichever is added first takes precedence.
+        // set statically (a tenant id, a date range), the run-time value wins — same first-wins rule.
         foreach (KeyValuePair<string, object?> kvp in context.Execution.Parameters)
             AddParameter(command, kvp.Key, kvp.Value, _parameterPrefix);
         foreach (KeyValuePair<string, object?> kvp in _parameters)
             AddParameter(command, kvp.Key, kvp.Value, _parameterPrefix);
 
-        AddParameter(command, "cursor", DecodeCursor(context.Cursor), _parameterPrefix);
-
-        // Cap rows per page without requiring TOP/OFFSET/LIMIT/ROWNUM in the user's SQL.
-        AddParameter(command, "pageSize", _pageSize, _parameterPrefix);
-
         var records = new List<T>(_pageSize);
         string? lastKey = null;
         var read = 0;
@@ -152,16 +155,17 @@ public async Task<BatchResult<T>> ReadBatchAsync(BatchContext context, Cancellat
         command.CommandText = $"SELECT COUNT(*) FROM (\n{innerSql}{_countInnerSuffix}\n) q";
         _configureCommand?.Invoke(command);
 
-        // Same run-time-wins precedence as ReadBatchAsync (execution parameters added first), so the
+        // Same ordering as ReadBatchAsync: the engine's reserved names first (so a run-time parameter
+        // can't take them over), then run-time parameters, then the source's static ones — so the
         // progress count reads with the exact values the real read will use.
+        AddParameter(command, "cursor", null, _parameterPrefix);
+        AddParameter(command, "pageSize", _pageSize, _parameterPrefix);
+
         foreach (KeyValuePair<string, object?> kvp in execution.Parameters)
             AddParameter(command, kvp.Key, kvp.Value, _parameterPrefix);
         foreach (KeyValuePair<string, object?> kvp in _parameters)
             AddParameter(command, kvp.Key, kvp.Value, _parameterPrefix);
 
-        AddParameter(command, "cursor", null, _parameterPrefix);
-        AddParameter(command, "pageSize", _pageSize, _parameterPrefix);
-
         object? result = await command.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false);
         return Convert.ToInt64(result, CultureInfo.InvariantCulture);
     }

tests/NeoReports.Sources.Sqlite.IntegrationTests/SqliteKeysetSourceTests.cs

@@ -17,8 +17,28 @@ public class SqliteKeysetSourceTests : IClassFixture<SqliteFileFixture>
         "SELECT Id, Customer, Amount, Date FROM Sales " +
         "WHERE (@cursor IS NULL OR Id > @cursor) ORDER BY Id";
 
-    private static ReportExecutionContext Exec() =>
-        new("job", "sales", null, NullLogger.Instance, CancellationToken.None);
+    private static ReportExecutionContext Exec(IReadOnlyDictionary<string, object?>? parameters = null) =>
+        new("job", "sales", parameters, NullLogger.Instance, CancellationToken.None);
+
+    [Fact]
+    public async Task A_run_time_parameter_cannot_take_over_the_engine_cursor()
+    {
+        // Regression: run-time parameters used to be bound BEFORE the engine's own @cursor, and
+        // AddParameter skips an already-bound name — so a caller passing "cursor" pinned it for every
+        // page. The keyset query then kept returning the same first page and the run never advanced
+        // (an unbounded loop writing to disk, since the page loop only stops on HasMore=false).
+        var hijack = new Dictionary<string, object?> { ["cursor"] = null };
+        var source = Source.Sqlite(_fixture.ConnectionString, Sql).Keyset<Sale, long>(v => v.Id, pageSize: 10);
+
+        var first = await source.ReadBatchAsync(new BatchContext(Exec(hijack), 10, null, 1), CancellationToken.None);
+        var second = await source.ReadBatchAsync(
+            new BatchContext(Exec(hijack), 10, first.NextCursor, 2), CancellationToken.None);
+
+        first.Records.Count.ShouldBe(10);
+        second.Records.Count.ShouldBe(10);
+        // The second page must move past the first — not repeat it.
+        second.Records[0].Id.ShouldBeGreaterThan(first.Records[^1].Id);
+    }
 
     [Fact]
     public async Task Reads_all_pages_in_order_without_gaps_or_duplicates()