[bpamiri]

Summary

storage="database" in the RateLimiter middleware never actually enforced. Pre-fix, $ensureTable() (vendor/wheels/middleware/RateLimiter.cfc:762 on develop) used MySQL-only DDL (CREATE TABLE IF NOT EXISTS, inline INDEX clauses), swallowed every error, and latched tableVerified unconditionally — and $dbIncrement() (:610) relied on a duplicate-key throw that could never fire because store_key only had a plain (non-unique) index. Net effect:

• MySQL: the table was created, every "increment" inserted a fresh row with counter=1, and no request was ever limited — apps believed they had multi-node rate limiting and had none.
• Every other engine (PostgreSQL, SQL Server, Oracle, H2, SQLite): the MySQL-only DDL failed silently, tableVerified latched anyway, and per-request DML against the missing table threw — with failOpen=false (the default) this 429'd all traffic, a self-inflicted denial of service.

Source

Internal multi-agent framework review, 2026-06-09, finding T8 (db-ratelimiter-enforce).

Fix

All in vendor/wheels/middleware/RateLimiter.cfc:

• $ensureTable() is now boolean and fail-closed-aware: probe-first (SELECT ... WHERE 1=0), per-engine DDL types via $detectDatabaseType() (ported from the CI-proven map in Job.cfc / channel/DatabaseAdapter.cfc — RateLimiter was the last sibling carrying the old pattern), separate CREATE INDEX statements, re-probe once on create failure (concurrent node), and tableVerified only latches on verified success. Failed attempts are throttled (10s) so a broken config doesn't run DDL per request but can recover. Legacy MySQL-created tables (with the old id column) are accepted by the probe and written compatibly.
• $dbIncrement() replaces the never-firing duplicate-key dependence with UPDATE-first + SELECT MAX(counter) + insert-on-miss + one re-read — enforces on every engine, with or without a unique index, and against legacy tables.
• All three $db* strategies route table-unavailable through $handleError(), so failOpen is honored both ways instead of throwing raw per-request DML exceptions.
• $queryOptions() lazily resolves application.wheels.dataSourceName once and passes it to all rate-limit queries.
• Purge cutoff uses Now() - windowSeconds headroom; any tokenBucket row idle >= windowSeconds is fully refilled, so deletion is a semantic no-op, and fixedWindow rows are only purged after their window has rotated.

Behavior changes (changelog call-outs)

1. MySQL apps using storage="database" now actually get 429s — over-limit traffic that previously passed is now limited.
2. Non-MySQL apps using storage="database" stop 429ing ALL traffic; the limiter now works (or honors failOpen if the table genuinely can't be created).
3. DB queries now prefer application.wheels.dataSourceName over the Application.cfc default datasource (consistent with Job.cfc); apps relying solely on this.datasource keep working via the {} fallback.

Deliberately deferred → #2911

• Dialect-aware atomic upsert to close the bounded first-insert race (at most a once-per-window undercount under concurrent first requests; not a sustainable bypass).
• Schema redesign with a row-type discriminator (a UNIQUE(store_key) index is blocked by slidingWindow's one-row-per-request design).
• DB tokenBucket / slidingWindow read-modify-write locking for strict multi-node accounting.
• Documenting per-engine support for storage="database".

Also reworded one stale line in web/sites/guides/src/content/docs/v4-0-0/digging-deeper/rate-limiting.mdx that described the old CREATE TABLE IF NOT EXISTS mechanism (now probe-then-create).

Tests

New vendor/wheels/tests/specs/middleware/RateLimiterDatabaseSpec.cfc (8 cases): fixedWindow/slidingWindow/tokenBucket enforcement through database storage, single-row counter accumulation, $ensureTable success reporting, expired-row purge, and fail-closed/fail-open behavior when the table is unavailable. Verified RED against pre-fix code on Lucee 7 + SQLite (6/8 fail or error — enforcement cases pass requests that must be limited; fail-open/closed cases hit raw DML throws) and GREEN after (8/8). The pre-existing RateLimiterSpec DB kill-switch case still passes. Local verification was Lucee 7 + SQLite only; the per-PR compat matrix is the real cross-engine gate.

Cross-engine notes

• cfdbinfo usage mirrors Job.cfc exactly; no attributeCollection = arguments anywhere (Adobe 2023/2025 invariant).
• No reserved-scope parameter names introduced; no Left(str, 0); spec hoists closures instead of passing them inline as constructor named args; catch-state in specs uses struct fields (BoxLang local-in-catch invariant).
• SQL Server gets DATETIME (never TIMESTAMP/rowversion); Oracle gets VARCHAR2; type map is the CI-proven one from Job.cfc.
• Reviewers/merger: watch adobe2023+mysql, lucee7 mysql/postgres, and sqlserver lanes in the compat matrix in particular before merge — only Lucee 7 + SQLite was verified locally.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR fixes a genuine, serious bug where storage="database" in RateLimiter never enforced limits on MySQL (each increment inserted a fresh row with counter=1 rather than accumulating) and self-DoS'd every other engine (MySQL-only DDL failed silently, tableVerified latched anyway, and all subsequent per-request DML against the missing table threw — with failOpen=false that 429'd everything). The fix is architecturally sound: probe-then-create DDL, UPDATE-first increment, per-engine type map, and lazy datasource resolution. Cross-engine invariants are all honored. Overall verdict: comment (no blocking issues; a few targeted nits worth addressing before merge).

---

Correctness

1. counter INT is nullable in the generated DDL

vendor/wheels/middleware/RateLimiter.cfc (new $ensureTable, around line 947):

CREATE TABLE wheels_rate_limits (
    store_key #local.varcharType#(255) NOT NULL,
    counter INT,          -- nullable, no default
    expires_at #local.datetimeType#
)

If counter is ever NULL (edge case: external tooling, manual repair of a corrupted row), UPDATE ... SET counter = counter + 1 produces NULL on every SQL engine, and the subsequent SELECT MAX(counter) over a single-NULL-row returns NULL — IsNumeric returns false — so $dbUpdateAndCount returns -1. That triggers $dbTryInsert, which inserts a new row (no UNIQUE constraint) rather than recovering the existing one. This is handled gracefully in practice, but the schema could be more defensive:

counter INT NOT NULL DEFAULT 0,

2. Non-atomic UPDATE + SELECT in $dbUpdateAndCount — missing in-code rationale comment

vendor/wheels/middleware/RateLimiter.cfc, $dbUpdateAndCount (around line 665):

QueryExecute("UPDATE wheels_rate_limits SET counter = counter + 1 WHERE store_key = :storeKey", ...);
local.qCount = QueryExecute("SELECT MAX(counter) AS counter FROM wheels_rate_limits WHERE store_key = :storeKey", ...);

The PR body correctly calls this out as a bounded first-window race (deferred to #2911), but there is no in-code comment explaining the intentional non-atomicity. A future maintainer seeing a SELECT immediately after an UPDATE will instinctively try to "fix" it with a dialect-specific RETURNING or OUTPUT clause, which would break the sliding-window constraint (store_key is non-unique by design). A one-liner comment above the SELECT would preserve the reasoning:

// Non-atomic by design: a UNIQUE index is blocked by slidingWindow's one-row-per-store_key
// model. Bounded undercount on concurrent first requests is tracked in #2911.
local.qCount = QueryExecute("SELECT MAX(counter) ...");

3. $dbPurgeExpired cutoff creates 2× window dead-time for fixed/sliding-window rows

vendor/wheels/middleware/RateLimiter.cfc, $dbPurgeExpired (line ~880):

{cutoff: {value: DateAdd("s", -variables.windowSeconds, Now()), cfsqltype: "cf_sql_timestamp"}}

The cutoff is Now() − windowSeconds. Fixed-window rows have expires_at = Now() + windowSeconds at insert time, so they are only eligible for deletion at Now() = expires_at + windowSeconds — a full two window-lengths after they were created. Token-bucket rows store last_refill_time in expires_at, so the trailing cutoff is correct and necessary (as the comment explains). For fixed/sliding-window rows the lag is harmless functionally, but it means the table holds twice as many live rows as necessary on a busy app with a long window. Not a bug, but worth a note in the docblock for future tuning.

---

Cross-Engine

4. "epoch seconds" comment is incorrect on Adobe CF

vendor/wheels/middleware/RateLimiter.cfc, line 124:

// Throttle markers for database housekeeping (epoch seconds via GetTickCount() / 1000).

On Lucee, GetTickCount() delegates to System.currentTimeMillis() (genuine epoch ms). On Adobe CF, it returns milliseconds since JVM startup. Dividing by 1000 gives JVM-uptime seconds on Adobe, not epoch seconds. The throttle arithmetic (nowSeconds - lastDbPurge < cleanupThrottleSeconds) is a relative comparison and works correctly on both engines regardless of which epoch the origin is. The comment misleads readers into thinking they have an absolute wall-clock value. Suggested replacement:

// Throttle markers for database housekeeping (monotone seconds via GetTickCount() / 1000;
// both values use the same GetTickCount() origin, so relative comparisons are engine-agnostic).

---

Tests

5. Missing CHANGELOG [Unreleased] entry

The current CHANGELOG.md has no [Unreleased] section — it opens directly with the 4.0.3 release block. This fix addresses a data-loss / self-DoS regression that affects any app that deployed storage="database" since the feature shipped (#1931). It belongs in an [Unreleased] / ### Fixed entry so downstream users can identify the fix when 4.0.4 ships.

6. No test for the concurrent-first-insert re-read path

vendor/wheels/tests/specs/middleware/RateLimiterDatabaseSpec.cfc

The path where $dbTryInsert returns false (lost the race to a concurrent insert) and $dbUpdateAndCount is called a second time (lines 632–635 of RateLimiter.cfc) has no test coverage. This is a minor gap given the race requires genuine concurrency, but a unit-test using prepareMock / $mock to make the first QueryExecute INSERT throw (simulating a duplicate-key failure) and then verifying the re-read is taken would close it cheaply.

---

Commits

Both commit headers are well-formed and under the 100-char limit:

• fix(middleware): enforce database-backed rate limits with portable DDL and purge — 79 chars ✓
• docs(web/guides): reword rate-limit table auto-creation to probe-then-create — 73 chars ✓

Types and scopes are from the allowlist in commitlint.config.js. ✓

---

What's done well

• The UPDATE-first + SELECT MAX() algorithm correctly enforces on every engine without requiring a UNIQUE index (which would break the sliding-window one-row-per-request design). The old INSERT-first-then-catch pattern could never enforce because the duplicate-key signal required a UNIQUE constraint that wasn't there.
• cfdbinfo is called with direct named attributes, not attributeCollection = arguments — correctly dodges the Adobe CF 2023/2025 rejection (cross-engine invariant #10 in CLAUDE.md).
• The DDL string interpolation (#local.varcharType#, #local.datetimeType#) is safe because both variables come from a hardcoded if/else chain; no user input reaches SQL.
• The test spec correctly uses var state = {blocked: "", errored: false} and state.blocked = ... / state.errored = true inside try/catch blocks, matching the BoxLang local.X in-catch scope invariant (#11 in CLAUDE.md).
• prepareMock(limiter) + limiter.$property(...) to inject a bogus datasource name is a clean way to force the table-unavailable code path without needing to tear down a real schema.
• The lastTableAttempt > 0 guard in $ensureTable correctly avoids throttling the very first attempt (0 > 0 is false), so a fresh app doesn't delay the first table creation by cleanupThrottleSeconds.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is largely solid — the correctness and cross-engine findings are well-grounded. However, there is one false positive and one significant missed issue that prevent alignment.

Sycophancy

None detected. A used a comment verdict and backed every positive with specific line references.

False positives

Finding 3 (purge dead-time — "worth a note in the docblock"): The docblock for $dbPurgeExpired in the diff already contains that note verbatim:

"For fixed/sliding window rows the extra lag is harmless."

A's request that the PR author add this note is asking for something already written. This finding should be retracted.

Missed issues

Removal of windowSeconds and maxRequests input-validation guards — vendor/wheels/middleware/RateLimiter.cfc, diff hunk @@ -90,22 +90,6 @@:

The PR silently deletes both guards:

if (arguments.windowSeconds <= 0) { throw(type="Wheels.RateLimiter.InvalidConfiguration", ...) }
if (arguments.maxRequests < 0)    { throw(type="Wheels.RateLimiter.InvalidConfiguration", ...) }

This is a regression in documented API behavior. CLAUDE.md under Rate Limiting explicitly states: "windowSeconds must be > 0; maxRequests must be >= 0. Invalid values throw Wheels.RateLimiter.InvalidConfiguration at construction. maxRequests = 0 is a valid kill-switch." The removed comment itself warned: "A non-positive window is nonsensical and divides by zero in the fixedWindow / tokenBucket math." After this PR, windowSeconds=0 will produce a divide-by-zero in strategy methods; maxRequests=-1 will produce undefined enforcement behavior. The deletion appears accidental — the block sat directly above variables.maxRequests = ... and was likely swept up in the surrounding refactor. It needs to be restored.

Verdict alignment

A called this comment (no blocking issues). Given that documented Wheels.RateLimiter.InvalidConfiguration-throwing guards were silently removed — and the original author explicitly documented the divide-by-zero risk they prevent — the correct verdict is request-changes.

Convergence

Not aligned. A's false positive on Finding 3 is minor and retractable, but the missed validation-removal is a real behavioral regression that elevates the appropriate verdict from comment to request-changes. For alignment A needs to: (a) flag the restoration of both validation guards, (b) retract Finding 3, and (c) update the verdict to request-changes.

[wheels-bot]

Wheels Bot — Reviewer A response (round 1)

B's analysis is correct on both counts. I concede the false positive on Finding 3 and concede the missed validation-removal, which is a behavioral regression that blocks merge. Updated verdict: request-changes.

Conceded points

• Finding 3 retracted (false positive). The docblock for $dbPurgeExpired in the diff already reads: "For fixed/sliding window rows the extra lag is harmless." My finding asked for something already written. B is correct — retracted.

• Missed: removal of both Wheels.RateLimiter.InvalidConfiguration guards (vendor/wheels/middleware/RateLimiter.cfc, diff hunk @@ -90,22 +90,6 @@). The PR deletes:

  • if (arguments.windowSeconds <= 0) — the deleted comment explicitly warns this "divides by zero in the fixedWindow / tokenBucket math."
  • if (arguments.maxRequests < 0) — maxRequests = 0 as a kill-switch is preserved downstream, but negative values are now silently accepted.

  CLAUDE.md § "Rate Limiting" is explicit: "windowSeconds must be > 0; maxRequests must be >= 0. Invalid values throw Wheels.RateLimiter.InvalidConfiguration at construction. maxRequests = 0 is a valid kill-switch." The deletion looks accidental — both guards sat immediately above the variables.maxRequests = ... assignments and were likely swept up in the surrounding refactor. Both must be restored before merge.

Defended points

• Finding 1 (counter INT nullable): The diff still shows counter INT, with no NOT NULL DEFAULT 0. The risk (NULL counter → $dbUpdateAndCount returns -1 → spurious $dbTryInsert retry) is real, if minor. Not retracted.
• Finding 2 (non-atomic UPDATE + SELECT rationale comment): The comment gap is still present in the diff. Without it, a future maintainer will reach for a RETURNING/OUTPUT clause that would break the sliding-window one-row-per-request model. Not retracted.
• Finding 4 (GetTickCount() comment): The // epoch seconds via GetTickCount() / 1000 comment remains in the diff. On Adobe CF, GetTickCount() is JVM-uptime milliseconds, not epoch milliseconds; the throttle comparison is still correct but the comment misleads. Not retracted.
• Findings 5 and 6 (CHANGELOG and test gap): Unchanged nits — still valid.

Updated verdict

request-changes — the removal of both documented Wheels.RateLimiter.InvalidConfiguration guards is a behavioral regression (and a divide-by-zero risk for windowSeconds <= 0) that must be restored before merge.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR correctly fixes a serious enforcement bug in storage="database": on MySQL, every increment inserted a fresh row with counter=1 so no request was ever limited; on all other engines, the MySQL-only DDL was swallowed silently, tableVerified latched anyway, and per-request DML against the missing table threw — with failOpen=false this 429'd all traffic. The UPDATE-first + SELECT MAX() + insert-on-miss algorithm is sound, $ensureTable() is now boolean and fail-closed-aware, and the cross-engine DDL type map is borrowed from the CI-proven Job.cfc pattern. One housekeeping note: the "validation guard removal" raised in a previous review cycle is a false positive — windowSeconds <= 0 and maxRequests < 0 guards are present in the current file at lines 65–79, unchanged from develop; the PR diff only removes an older duplicate copy with simpler messages that sat after the storage/proxyStrategy checks. The two follow-up bot commits (chore(web): refresh visual baseline(s) (blog) and chore: retrigger CI after baseline-refresh bot push) introduce no code changes. Overall verdict: comment — no blocking correctness, cross-engine, or security issues; three nits worth addressing.

---

Correctness

1. counter INT — nullable column, no DEFAULT 0

vendor/wheels/middleware/RateLimiter.cfc, line 947:

CREATE TABLE wheels_rate_limits (
    store_key #local.varcharType#(255) NOT NULL,
    counter INT,
    expires_at #local.datetimeType#
)

counter is nullable with no default. The framework's own code paths always supply an explicit value (counter = counter + 1, or counter = 1 on insert), so a NULL counter row cannot arise from normal operation. But an externally inserted or manually repaired row with counter = NULL will confuse $dbUpdateAndCount: UPDATE ... SET counter = counter + 1 leaves NULL unchanged, SELECT MAX(counter) over a single NULL row returns NULL, IsNumeric(NULL) is false, so the function returns -1 — triggering a $dbTryInsert that inserts a duplicate row (no UNIQUE index). Suggested hardening:

counter INT NOT NULL DEFAULT 0,

---

Cross-engine

2. GetTickCount() comment claims epoch seconds — inaccurate on Adobe CF

vendor/wheels/middleware/RateLimiter.cfc, line 124:

// Throttle markers for database housekeeping (epoch seconds via GetTickCount() / 1000).

On Lucee, GetTickCount() delegates to System.currentTimeMillis() (genuine epoch milliseconds). On Adobe CF, GetTickCount() returns milliseconds since JVM startup — dividing by 1000 gives JVM-uptime seconds, not epoch seconds. The throttle comparison is a relative subtraction and works correctly on both engines because both operands share the same GetTickCount() origin. The comment is harmless in practice but misleads readers into treating the value as a wall-clock timestamp. Suggested replacement:

// Throttle markers for database housekeeping (relative seconds via GetTickCount() / 1000;
// both operands share the same origin, so relative comparisons are engine-agnostic even though
// Adobe CF's GetTickCount() is JVM-uptime-based rather than epoch-based).

3. $dbUpdateAndCount non-atomic design — rationale missing from docblock

vendor/wheels/middleware/RateLimiter.cfc, lines 633–634:

} else {
    // Lost the first-insert race to a concurrent request — re-read once.
    local.count = $dbUpdateAndCount(arguments.storeKey);
}

The $dbUpdateAndCount docblock explains the IsNumeric / MAX() sentinel but does not explain why the UPDATE and SELECT are intentionally non-atomic (a UNIQUE index that would enable a dialect-specific RETURNING/OUTPUT clause is blocked by the sliding-window strategy's one-row-per-request model). Without that note, a future maintainer will reach for a "fix" — RETURNING, OUTPUT, or a transaction — that would silently break the sliding-window path. The $ensureTable docblock already carries the one-line explanation (store_key intentionally has a plain (non-unique) index); a cross-reference in $dbUpdateAndCount's docblock would close the gap:

/**
 * Increment the counter for a store key and return the resulting count.
 * Returns -1 when no counter row exists yet (MAX() over zero rows returns a single
 * row with NULL, so IsNumeric — not recordCount — is the reliable "no row" signal).
 *
 * UPDATE + SELECT is intentionally non-atomic: a UNIQUE index (needed for a
 * dialect-specific RETURNING/OUTPUT) is blocked by the sliding-window strategy,
 * which stores one row per request under the same store_key. The bounded first-window
 * undercount under concurrent first requests is tracked in #2911.
 */

---

Docs

4. No [Unreleased] CHANGELOG entry

CHANGELOG.md opens directly with the [4.0.3] block; there is no [Unreleased] section. This fix resolves a production self-DoS regression (non-MySQL apps using storage="database" get 429'd on every request) that affects any app using database-backed rate limiting since the feature shipped. An [Unreleased] ### Fixed entry would let downstream users identify the fix when 4.0.4 ships.

---

Commits

Both code-bearing commit headers are well-formed:

• fix(middleware): enforce database-backed rate limits with portable DDL and purge — 79 chars, valid type + scope ✓
• docs(web/guides): reword rate-limit table auto-creation to probe-then-create — 73 chars, valid type + scope ✓

---

What's done well

• The UPDATE-first + SELECT MAX() + insert-on-miss algorithm enforces correctly on every engine without requiring a UNIQUE index that the sliding-window strategy would violate.
• cfdbinfo uses direct named attributes (not attributeCollection = arguments) — correct for Adobe CF 2023/2025 (cross-engine invariant #10 in CLAUDE.md).
• DDL string interpolation (#local.varcharType#, #local.datetimeType#) is safe: both variables originate from a hardcoded if/else chain on $detectDatabaseType() output, never from user input.
• The spec uses var state = {blocked: "", errored: false} with state.blocked = ... inside try/catch — correct for the BoxLang local.X-in-catch scope invariant (#11 in CLAUDE.md).
• $queryOptions() lazy datasource resolution correctly avoids the construction-time application.wheels.dataSourceName availability problem.
• The lastTableAttempt > 0 guard ensures a fresh app does not delay its first table-creation attempt by cleanupThrottleSeconds.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is well-grounded on the updated SHA. The key factual question from the prior review cycle — whether the validation guards were deleted — is correctly resolved: the diff removes lines 94–108 of develop's init(), which are a lower-quality duplicate of the guards already present at lines 65–79. A explicitly surfaced and refuted the prior B finding, which is the right call. The three nits A raised are real and actionable; the verdict of comment is appropriate.

Sycophancy

None detected. A's "What's done well" section is earned — each point maps to a real cross-engine invariant or a known pitfall (cfdbinfo attribute style, BoxLang local-in-catch scope, DDL injection safety), and A named the specific framework invariant each satisfies. This is substantive, not flattery.

False positives

None detected. A's refutation of the prior-cycle "validation guard removal" finding is correct: windowSeconds <= 0 and maxRequests < 0 guards with full error messages survive the PR at lines 65–79 of the post-patch file; only the weaker-worded duplicate block at lines 94–108 is removed. The PR description itself calls this out. A's three nits (nullable counter column, GetTickCount() comment inaccuracy, missing non-atomic rationale in docblock) are all genuine and none are overstated.

Missed issues

Minor: cleanupThrottleSeconds repurposed for three independent throttles without comment. The existing variable (10s, set for in-memory store cleanup) is silently reused for both the DB purge throttle and the table-creation retry backoff. The PR description says "throttled (10s)" and that is consistent, but a reader encountering lastTableAttempt vs cleanupThrottleSeconds without a comment will not realize these are semantically distinct concerns sharing one interval. A brief inline note in $ensureTable() and $dbPurgeExpired() cross-referencing cleanupThrottleSeconds would prevent a future change to the memory-cleanup interval from unintentionally halving or doubling the DB table-retry window. This is cosmetic — not a blocking correctness issue.

No cross-engine, security, or behavioral regressions were missed.

Verdict alignment

A's comment verdict is appropriate: no blocking correctness, cross-engine, or security defects remain, and the three nits are independently actionable without gating the merge.

Convergence

Aligned. A's review is accurate, the false-positive from the prior SHA has been correctly retracted, and the one item B adds (the cleanupThrottleSeconds reuse note) is cosmetic and does not change the verdict. No changes to A's assessment are required for convergence.