perf(channel): bound cleanup candidate query in SQL instead of driver maxrows

[bpamiri]

Summary

DatabaseAdapter.cleanup() bounded its per-pass candidate SELECT with the driver-level maxrows option only. That caps the resultset client-side: the database still materializes the entire expired backlog before truncation — worst exactly in the large-backlog scenario the bound exists for.

The bound now lives in dialect SQL via a new unit-testable helper $applyRowBound(sqlText, dbType, maxRows):

• sqlserver → SELECT TOP n …
• oracle → … FETCH FIRST n ROWS ONLY
• everything else (mysql/postgresql/sqlite/h2/default) → … LIMIT n

The bound is hardened with Int() before interpolation; non-positive bounds leave the statement unchanged. The two-step SELECT-then-DELETE-by-id shape is unchanged (single-statement bounded DELETE is not portable), and the driver maxrows option stays on as belt-and-braces. The DB can now do an index-assisted top-n read (idx_wevents_cleanup).

Verification

• Full core suite (Lucee 7 + SQLite): 4670 passed, 0 failed (develop baseline 4665 + 5 new $applyRowBound dialect specs; 2 pre-existing local-env errors unrelated).
• Existing behavior spec "cleanup with maxRows bounds the number of rows deleted per pass" stays green.
• Integration branch: adobe2023+MySQL and lucee7+MySQL failure sets identical to or better than develop's.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer

TL;DR — This PR pushes the cleanup() candidate-SELECT row bound from the driver-level maxrows option into dialect SQL (SELECT TOP n / FETCH FIRST n ROWS ONLY / LIMIT n) via a new unit-tested $applyRowBound() helper, so bounded retention passes do an index-assisted top-n read instead of materializing the whole expired backlog. The performance intent is sound, the helper is well-tested, and the changelog fragment + commit are correct. One correctness edge case blocks: the "default" fallback dialect appends LIMIT n, which is invalid on SQL Server and Oracle — the exact engines that need dialect-specific handling. Verdict: request changes (one-line fix).

Correctness

vendor/wheels/channel/DatabaseAdapter.cfc:367 — "default" fallback appends LIMIT n, which is invalid on SQL Server / Oracle.

if (arguments.dbType == "oracle") {
    return arguments.sqlText & " FETCH FIRST #local.bound# ROWS ONLY";
}
return arguments.sqlText & " LIMIT #local.bound#";   // also the "default" branch

$detectDatabaseType() (line 319) returns "default" whenever cfdbinfo throws or the product name is unrecognized — it wraps the call in try/catch specifically because the metadata call can be unavailable. Before this PR the candidate SELECT carried no dialect clause and relied only on the driver maxrows option, so it was database-agnostic and executed correctly on every engine regardless of detection. Now, if detection falls through to "default" on a SQL Server (or Oracle) instance, the statement becomes ... ORDER BY createdAt ASC LIMIT n, a syntax error on those engines. cleanup() then throws, the catch (any e) at line 209 swallows it, logs to wheels_channels, and returns 0.

This is reachable on the hot path: $maybeCleanup() (line 225) calls cleanup(maxRows = variables.cleanupBatchSize) (throttled) on every publish(). Under detection failure on SQL Server, cleanup would silently stop working and the backlog would never drain — the opposite of this PR's goal.

Note this is a new failure mode, not a pre-existing one: the table-creation "default" branch (line 271, VARCHAR/TEXT/DATETIME) is itself valid T-SQL, so the adapter currently tolerates cfdbinfo failure on SQL Server. This PR regresses that tolerance for cleanup.

Suggested fix — leave the "default" case unchanged and rely on the retained driver maxrows (safe on every engine), applying LIMIT only to the explicitly-detected LIMIT dialects:

if (arguments.dbType == "mysql"
    || arguments.dbType == "postgresql"
    || arguments.dbType == "sqlite"
    || arguments.dbType == "h2") {
    return arguments.sqlText & " LIMIT #local.bound#";
}
return arguments.sqlText;   // unknown dialect: driver maxrows still bounds it

Update the "every other dialect" spec accordingly (drop "default" from the LIMIT list, or add a case asserting "default" returns the statement unchanged).

Tests

Coverage of the helper is good — DatabaseAdapterSpec.cfc:225-280 exercises the TOP / FETCH FIRST / LIMIT branches plus the Int() hardening (7.9 -> 7) and the non-positive (0 -> unchanged) edge, and the existing "cleanup with maxRows bounds..." behavior spec is retained. If you adopt the fix above, the "default" assertion in the "appends LIMIT for every other dialect" test (line 251) must move to assert the unchanged-statement contract instead.

Commits

Clean. PR title perf(channel): bound cleanup candidate query in SQL instead of driver maxrows is a valid conventional-commit header (type perf, scope channel, ~78 chars, sentence case) — correct since the repo squash-merges. Changelog fragment changelog.d/channel-cleanup-sql-row-bound.performance.md uses the valid performance type and a complete bullet, matching the fragment convention (no direct CHANGELOG.md edit).

Cross-engine

The helper itself is cross-engine clean — ReplaceNoCase / Int / string concat only, no closures, struct member functions, or reserved-scope names. SELECT TOP n ... ORDER BY (SQL Server) and ... FETCH FIRST n ROWS ONLY (Oracle 12c+, soft-fail in CI) are valid for their dialects. The one cross-engine gap is the "default"->LIMIT issue above.

[wheels-bot]

Wheels Bot — Reviewer

TL;DR — This PR pushes the cleanup() candidate-SELECT row bound from the driver-level maxrows option into dialect SQL (SELECT TOP n / FETCH FIRST n ROWS ONLY / LIMIT n) via the unit-tested $applyRowBound() helper, so bounded retention passes do an index-assisted top-n read instead of materializing the whole expired backlog. My prior review (--request-changes on 2f19f9b) flagged one correctness edge case; commit e23f55e resolves it exactly as suggested. No blocking finding remains. Verdict: approve (superseding the stale CHANGES_REQUESTED per #3048).

Resolved since last review

vendor/wheels/channel/DatabaseAdapter.cfc:371-376 — the "default" dialect no longer appends LIMIT.

The previous head appended LIMIT n in the catch-all branch, so a cfdbinfo failure on SQL Server / Oracle (where $detectDatabaseType() at line 319-333 falls back to "default") would have produced ... ORDER BY createdAt ASC LIMIT n, a syntax error swallowed by the catch (any e) at line 209 — silently killing the throttled cleanup on the publish hot path. Now LIMIT is gated behind an explicit dialect allowlist and unknown/"default" dialects return the statement unchanged, leaning on the retained driver maxrows (line 180) which bounds the resultset on every engine:

if (ListFindNoCase("mysql,postgresql,sqlite,h2", arguments.dbType)) {
    return arguments.sqlText & " LIMIT #local.bound#";
}
// Unknown dialect (incl. the "default" cfdbinfo-failure fallback): leave the
// statement alone rather than risk invalid syntax; driver maxrows bounds it.
return arguments.sqlText;

This matches the fix I proposed line-for-line.

Tests

The requested spec update landed. DatabaseAdapterSpec.cfc:258-273 now asserts that both "default" and "informix" return the statement unchanged, with a comment explaining why ("default" is the cfdbinfo-failure fallback). The TOP / FETCH FIRST / LIMIT branches, the Int() hardening (7.9 -> 7), and the non-positive (0 -> unchanged) edge remain covered, and the existing "cleanup with maxRows bounds..." behavior spec is retained.

Cross-engine

Clean. $applyRowBound uses only Int / ReplaceNoCase / ListFindNoCase / string concat — no closures, struct member functions, or reserved-scope names. It is public with the $ prefix (Cross-Engine Invariant #7), keeping it mixable and unit-testable. SELECT TOP n ... ORDER BY (SQL Server) and ... FETCH FIRST n ROWS ONLY (Oracle 12c+) are valid for their dialects, and a bare SELECT ... ORDER BY with no top-n clause (the unknown-dialect path) is valid standalone SQL on every supported engine.

Commits

Both commits are conventional-commit compliant: perf(channel): ... and fix(channel): leave unknown dialects unbounded in SQL so cleanup survives cfdbinfo failure (valid types perf/fix, scope channel, subjects under 100 chars, sentence case). The changelog fragment changelog.d/channel-cleanup-sql-row-bound.performance.md uses the valid performance type with a complete bullet — no direct CHANGELOG.md edit.