Page the "Register content move redirects" job so it survives large tables

[kennygutierrez]

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.

Rebased onto master (Optimizely CMS 13.1 / Commerce 15 / .NET 10, #187). All tests pass on the new target framework: 60 in Geta.NotFoundHandler.Tests, 60 in Geta.NotFoundHandler.Optimizely.Tests.

Generated with Claude Code

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[kennygutierrez]

Friendly bump 👋 @wezz @marisks — would you be able to take a look at this when you have a moment?

Context: the "Register content move redirects" job runs an unbounded SELECT over NotFoundHandler.ContentUrlHistory, which blows past the 30s SqlCommand timeout once that table gets large (we hit this in production with ~125k rows). This PR pages the job (OFFSET/FETCH via GetAllMoved(skip, take), batch size configurable through OptimizelyNotFoundHandlerOptions.MovedContentBatchSize, default 1000), adds NotFoundHandlerOptions.CommandTimeout, and makes SqlDataExecutor.ExecuteQuery rethrow instead of masking the timeout as "Cannot find table 0".

Two compat notes I'd value your steer on:

1. Adding GetAllMoved(int, int) to IContentUrlHistoryLoader is technically breaking for third-party implementers — happy to move it to an extension/overload instead if you'd prefer to avoid the interface change.
2. ExecuteQuery now propagates previously-swallowed exceptions, which is a behavior change (intentional — the swallow is what hid the timeout).

Glad to rebase or adjust to fit your conventions. Thanks!

[ivanmarkovic1402]

Thanks for this - it's a well-scoped fix for a real problem, and the write-up (including the root cause of the misleading "Cannot find table 0") is excellent.

I checked it against the code and the core correctness argument holds: CreateRedirects only writes to the redirects store and never touches NotFoundHandler.ContentUrlHistory, so the moved set is stable for the run and skip-based paging can't skip or duplicate a key. Paging by grouped keys (not rows) is the right call, and since md5_ContentKey is derived from ContentKey, a key can't be split across a page boundary. CommandTimeout defaults to 30s (the SqlClient default), so there's no behavior change unless configured, and the ExecuteQuery rethrow is strictly better than the previous behavior. Tests are solid.

A few things I'd like to see before merge, none of them architectural:

1. Clamp MovedContentBatchSize to >= 1 - a misconfigured 0/negative value emits invalid FETCH NEXT 0 SQL and now fails the job.
2. Make the interface change deliberate and bump the version. Adding GetAllMoved(int, int) to IContentUrlHistoryLoader breaks third-party implementers; a default interface implementation (you're on .NET 10) keeps it source-compatible, or keep the paged query on the repository. This plus the ExecuteQuery rethrow warrants a CHANGELOG entry and an appropriate semver bump.
3. CHANGELOG entry for the new options (CommandTimeout, MovedContentBatchSize) and the two behavior changes (the IContentUrlHistoryLoader addition and ExecuteQuery now propagating exceptions). The PR updates README.md but not CHANGELOG.md.

One non-blocking follow-up: the paged query re-aggregates the whole table on every page and the OFFSET grows with each page, so total DB work scales with page count. It fixes the timeout, but materializing the moved-key set once (or keyset pagination over the indexed md5_ContentKey) would scale better on very large tables — happy for that to be a separate PR.

Overall the approach is sound and I'm in favor of merging once the batch-size clamp, the interface/versioning decision, and the CHANGELOG are addressed.

[kennygutierrez]

Thanks for the thorough review @ivanmarkovic1402 — much appreciated. Pushed 966068e addressing all three blockers:

1. Clamp MovedContentBatchSize >= 1. RegisterMovedContentRedirectsJob now sets _batchSize = Math.Max(1, options.Value.MovedContentBatchSize), so a misconfigured 0/negative can't emit FETCH NEXT 0 ROWS ONLY and fail the job now that ExecuteQuery rethrows.

2. Interface change is now source-compatible. I went with the default interface implementation you suggested: GetAllMoved(int skip, int take) defaults to GetAllMoved().Skip(skip).Take(take), so existing IContentUrlHistoryLoader implementers keep compiling. SqlContentUrlHistoryRepository still overrides it with the server-side OFFSET/FETCH query. One small note — the project targets net8.0 (not .NET 10), but default interface methods are fully supported there (C# 12), so this works as-is.

3. CHANGELOG. Added an ## [Unreleased] section documenting the two new options (CommandTimeout, MovedContentBatchSize), the paged GetAllMoved, and the ExecuteQuery rethrow behavior change. I left the concrete version number to your release process — the repo has no version field and CHANGELOG.md was last at 1.1.0, so I didn't want to guess the next tag. With the break neutralized by the default impl, this should be a minor bump.

Also added the two tests you flagged: a guard test for the batch-size clamp and an exact-multiple-of-batch-size case (one trailing empty fetch, then stop). All 58 Optimizely tests pass.

On the non-blocking perf point (per-page re-aggregation + growing OFFSET) — agreed, and happy to do the keyset-pagination-over-md5_ContentKey version as a follow-up PR so this one stays scoped to fixing the timeout.

[kennygutierrez]

Rebased onto current master (the CMS 13.1 / Commerce 15 / .NET 10 upgrade from #187). Only CHANGELOG.md conflicted — the touched source files auto-merged cleanly — so my ## [Unreleased] entry now sits above the new ## [7.0.0] section. As a side note, this settles the earlier .NET 10 point: the repo is now on .NET 10, so the GetAllMoved(int, int) default interface method is fully supported and the addition stays source-compatible.

Pushed a small hardening commit on top:

• SqlContentUrlHistoryRepository.GetAllMoved(skip, take) now short-circuits to an empty result (no DB round-trip) when take <= 0 and clamps a negative skip, so a direct caller can't emit invalid OFFSET/FETCH SQL — complementing the existing Math.Max(1, ...) batch-size clamp in the job.
• SqlDataExecutor clamps a negative CommandTimeout to 0 (SqlClient throws on a negative value; 0 = no timeout), documented on the option.
• Added a test for the take <= 0 guard.

All tests pass on .NET 10: 60 in Geta.NotFoundHandler.Tests and 60 in Geta.NotFoundHandler.Optimizely.Tests.

I also tidied the PR description to clarify two points that could read ambiguously: the IContentUrlHistoryLoader addition is source-compatible (default interface method), and the ExecuteQuery rethrow does not affect the 404 request hot path — redirect lookups during a request read the in-memory CustomRedirectHandler.Find / CustomRedirectCollection, not a live query.

The performance follow-up (temp table or keyset pagination over the indexed md5_ContentKey, to avoid re-aggregating the whole table per page) remains deferred to a separate PR as discussed.

[kennygutierrez]

Thanks for merging this, @ivanmarkovic1402 🙏

One follow-up: since the fix released in v7.1.0, it's only available on the CMS 13 / .NET 10 line. Teams still on Optimizely CMS 12 / .NET 8 are on the 6.x package series, and this large-ContentUrlHistory timeout is exactly the kind of thing that bites on long-lived CMS 12 sites — but there doesn't appear to be a 6.x maintenance branch to pick up the fix.

Would you consider backporting #188 to a 6.1.0 release? The change was originally built and tested against net8.0 (before this branch was rebased onto the CMS 13 upgrade), and the touched src files merged cleanly across that upgrade, so a cherry-pick onto the v6.0.0 base should be low-risk.

Happy to open the backport PR myself if that would help — just let me know if you'd like a 6.x branch to target.

[valdisiljuconoks]

Please, open PR against CMS12 version, we would merge it in. CMS12 version is not primary target for us anymore, but we can maintain it in the background while there is active usage for it.

Thx!

[kennygutierrez]

Thanks @valdisiljuconoks — really appreciate you keeping the CMS 12 line maintained. 🙏

I've already reconstructed the backport on the 6.x base: it's the exact same change as #188, rebuilt on top of the v6.0.0 commit (net8.0, no CMS 13 / .NET 10 code). I verified it's equivalent — git diff v6.0.0..backport matches #188's net contribution file-for-file (same 10 files, +333/−29; the only differences are line-number offsets from the CMS 13 upgrade). All tests pass on net8.0: 60 in Geta.NotFoundHandler.Tests and 60 in Geta.NotFoundHandler.Optimizely.Tests.

One thing I need from you to open it: there doesn't seem to be a CMS 12 / 6.x branch in the repo to target as the PR base (the current branches are all on the CMS 13 line or older feature branches; v6.0.0 exists only as a tag). Could you cut a maintenance branch off v6.0.0 — say release/6.x (or whatever you prefer) — for me to PR against? As soon as it exists I'll push and open the backport PR right away.

Thanks again!

[valdisiljuconoks]

@kennygutierrez branch name "release/v6"

[kennygutierrez]

Thanks @valdisiljuconoks! Opened the 6.x backport against release/v6: #193. It's the same src diff that shipped in v7.1.0 applied on top of v6.0.0, built + tested green on .NET 8 (60 + 60 tests, 0 failures); only the CHANGELOG lineage differs (6.x, no 7.0.0 entry). Ready for review whenever you have a moment.