Page the "Register content move redirects" job so it survives large tables (6.x backport of #188)

[kennygutierrez]

Backport of #188 to the CMS 12 / .NET 8 line

This is the release/v6 backport of #188 ("Page the 'Register content move redirects' job so it survives large tables"), which was merged into master and released as v7.1.0 (CMS 13 / .NET 10). Per @valdisiljuconoks's request on #188 to open a PR against the CMS 12 line, this targets the new release/v6 branch. The code change is identical to what shipped in 7.1.0 — the src files merged clean across the CMS 13 rebase, so this is the same diff applied on top of v6.0.0, built and tested on .NET 8. Only the CHANGELOG lineage differs (6.x, no 7.0.0 entry).

Problem

The [Geta NotFoundHandler] Register content move redirects job loads the entire NotFoundHandler.ContentUrlHistory table in a single unbounded query before doing any work:

var movedContent = _contentUrlHistoryLoader.GetAllMoved().ToList();

GetAllMoved() issues a self-join + GROUP BY ... HAVING COUNT(*) > 1 over the whole table with no paging. On sites with a large history table this SELECT exceeds the SqlClient command timeout (the default 30s, which was never overridden). SqlDataExecutor.ExecuteQuery then catches and swallows the timeout, logs it, and falls through to return ds.Tables[0] — but on a failed Fill the DataSet has no tables, so that line throws IndexOutOfRangeException: "Cannot find table 0". The operator sees a confusing "Cannot find table 0" with no hint that the real cause is a query timeout, and the job never completes.

Fix

• SqlDataExecutor: apply a configurable CommandTimeout to every command, and rethrow query failures instead of masking them as "Cannot find table 0". A negative CommandTimeout is clamped to 0 (SqlClient otherwise throws); 0 means no timeout.
• NotFoundHandlerOptions.CommandTimeout (seconds, default 30) so large sites can raise it past the SqlClient default.
• IContentUrlHistoryLoader / SqlContentUrlHistoryRepository: add a paged GetAllMoved(int skip, int take) using OFFSET ... FETCH NEXT over the moved keys, and reimplement the parameterless GetAllMoved() to stream pages so the unbounded query is gone everywhere. (md5_ContentKey is the hash of ContentKey, so each key maps to a single group and is never split across a page boundary.) The paged query short-circuits to an empty result when take <= 0 and clamps a negative skip, so a bad caller can't emit invalid SQL.
• RegisterMovedContentRedirectsJob: process the table page-by-page (stoppable, with progress logging) instead of materialising it all up front. Page size is tunable via OptimizelyNotFoundHandlerOptions.MovedContentBatchSize (default 1000) and is clamped to >= 1. CreateRedirects only writes to the redirects store, not ContentUrlHistory, so the moved set is stable for the run and skip-based paging never skips a key.

Compatibility notes

• IContentUrlHistoryLoader.GetAllMoved(int skip, int take) ships as a default interface method (pages in-memory over GetAllMoved()), so existing third-party implementations keep compiling without any change. Data-backed stores should override it with a server-side paged query, as SqlContentUrlHistoryRepository does — the in-memory default still materialises the full set, so it is a source-compatibility shim, not a scalability path.
• SqlDataExecutor.ExecuteQuery now propagates exceptions that were previously swallowed. This is intentional (so failures surface instead of becoming "Cannot find table 0"), but it changes behaviour for any caller that relied on getting an empty table back on error. The 404 request hot path is unaffected: redirect lookups during a request read the in-memory CustomRedirectCollection (CustomRedirectHandler.Find), not a live query. The change affects collection (re)loads and admin/reporting reads, which now surface a real error instead of silently returning nothing.

Tests

• Repository: paging passes the right skip/take, groups histories by content key, stops paging on a short page, and returns empty without querying when take <= 0.
• Job: processes every page across batches, does not fetch a further page after a partial page, stops after the empty page when the total is an exact multiple of the batch size, clamps a non-positive batch size to one, and continues when an individual item fails.

Built and tested on .NET 8: 60 pass in Geta.NotFoundHandler.Tests, 60 pass in Geta.NotFoundHandler.Optimizely.Tests, 0 failures.

Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces database paging and command timeout configurations to prevent unbounded queries and timeouts when processing large NotFoundHandler.ContentUrlHistory tables. Feedback highlights a potential O(N^2) performance trap in the default interface implementation of GetAllMoved(skip, take) for third-party implementations, and recommends adding defensive null checks for options. Additionally, it is advised to clamp negative command timeouts to a sensible default (e.g., 30 seconds) instead of 0 to avoid infinite timeouts that could cause queries to hang.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

While this default interface implementation provides source compatibility, calling it repeatedly in a loop (as done in RegisterMovedContentRedirectsJob.Execute) creates an $O(N^2)$ performance trap for any third-party implementations that do not override this method.

Each call to GetAllMoved(skip, take) will invoke GetAllMoved() to load the entire dataset from the database, only to skip and take a small batch in memory. For large tables, this will cause massive database load and memory allocations.

Consider documenting this performance implication clearly, or changing the job's execution flow to stream the results of GetAllMoved() directly and batch them in-memory using Enumerable.Chunk if the loader does not support native paging.

[gemini-code-assist]

The options parameter or options.Value could be null, which would cause a NullReferenceException when accessing options.Value.MovedContentBatchSize. It is safer to use defensive guards or null-coalescing operators to handle this.

        public RegisterMovedContentRedirectsJob(
            IAutomaticRedirectsService automaticRedirectsService,
            IContentUrlHistoryLoader contentUrlHistoryLoader,
            IOptions<OptimizelyNotFoundHandlerOptions> options)
        {
            _automaticRedirectsService = automaticRedirectsService ?? throw new ArgumentNullException(nameof(automaticRedirectsService));
            _contentUrlHistoryLoader = contentUrlHistoryLoader ?? throw new ArgumentNullException(nameof(contentUrlHistoryLoader));
            _batchSize = Math.Max(1, options?.Value?.MovedContentBatchSize ?? 1000);
            _jobStatusLogger = new JobStatusLogger(OnStatusChanged);

            IsStoppable = true;
        }

[gemini-code-assist]

The options parameter or options.Value could be null, which would cause a NullReferenceException when accessing options.Value.ConnectionString or options.Value.CommandTimeout. Additionally, clamping a negative CommandTimeout to 0 means that any negative misconfiguration will result in an infinite timeout (no timeout), which can cause database queries to hang indefinitely in the event of deadlocks or severe database contention.

It is safer to use defensive guards and clamp negative values to a sensible default (such as 30 seconds) instead of 0.

        public SqlDataExecutor(
            IOptions<NotFoundHandlerOptions> options,
            ILogger<SqlDataExecutor> logger)
        {
            _connectionString = options?.Value?.ConnectionString ?? throw new ArgumentNullException(nameof(options));
            // Clamp negative values to a default of 30 seconds instead of 0 (infinite timeout) to prevent hanging queries.
            _commandTimeout = (options?.Value?.CommandTimeout >= 0) ? options.Value.CommandTimeout : 30;
            _logger = logger ?? throw new ArgumentNullException(nameof(logger));
        }