feat(support): per-user rate limit on support-request submissions

[pjb157]

Summary

Caps POST /admin/api/v1/support/requests at 5 accepted submissions per hour per user. The next submission inside the window returns 429 Too Many Requests with a Retry-After: 3600 header.

Base branch: feat/email-async-enqueue — sits on top of #1093 (which sits on top of #1092). Merge order: #1092 → #1093 → this PR.

What changed

• New migration 098_support_request_submissions.sql — minimal audit table (user_id, created_at). Stores only metadata; subject/body never persisted here.
• Handler check + insert run in one transaction so concurrent submissions from the same user can't both observe a stale count and slip past the cap.
• Error::TooManyRequests grows a retry_after_seconds: u64 field so the error-response formatter emits an accurate Retry-After header (was hardcoded to 60s). Existing concurrency-based 429s in limits.rs and the batch-capacity rejection in handlers/batches.rs keep their 60s default.
• dwctl_email_send_total (added in feat(email): add HTTP transport abstraction with Resend client #1092) plus the audit-table row count gives operators an at-a-glance read on legit support volume vs rate-limit trips.

Tuning knobs

SUPPORT_REQUEST_LIMIT and SUPPORT_REQUEST_WINDOW_SECS are file-level consts at the top of support.rs. If we want them runtime-configurable later, pull them into LimitsConfig — the change is mechanical.

Test plan

• cargo check --workspace clean
• cargo test --lib support — 4 new tests pass: happy path, 429 after limit (with Retry-After assertion), per-user isolation, sliding-window aging
• Existing limits.rs tests still pass after the TooManyRequests field addition
• cargo sqlx prepare --workspace to refresh offline metadata; new query JSONs committed
• CI: just lint rust, just test rust, just lint ts, just test ts

[cloudflare-workers-and-pages]

Deploying control-layer with    Cloudflare Pages

Latest commit:	effe377
Status:	✅  Deploy successful!
Preview URL:	https://8d96b52b.control-layer.pages.dev
Branch Preview URL:	https://feat-support-request-rate-li.control-layer.pages.dev

View logs

[doubleword-code]

Summary

This PR implements per-user rate limiting on support request submissions (5 per hour) and migrates all transactional email sending to an asynchronous underway job queue with support for HTTP-based email providers (Resend). The changes are well-structured with comprehensive tests, but there are several issues that need addressing before merge.

Verdict: Needs changes — see Blocking findings below.

Research notes

• Fetched OWASP guidance on rate limiting: sliding-window approaches are recommended for per-user limits, and the implementation correctly uses a database-backed audit table with transactional consistency.
• Resend API documentation confirms the JSON payload structure matches what's implemented in email_http.rs (from, to, subject, html/text, reply_to fields).
• The Retry-After header semantics (RFC 7231 §7.1.3) specify it should contain either an HTTP-date or a delay in seconds — the implementation correctly uses seconds.

Suggested next steps

1. Blocking: Fix the SQLx query macro compilation issue — the migration creates support_request_submissions but the query macro references it before the migration exists in the base branch.
2. Blocking: Address the race condition window in the rate limit check — concurrent requests can still slip through between the COUNT and INSERT.
3. Non-blocking: Consider documenting the operational implications of email_workers: 0 more prominently.
4. Non-blocking: Add metrics for rate-limited support requests.

General findings

Architecture observations

• The separation of concerns between email.rs (envelope construction), email_http.rs (provider-specific wire format), and email_jobs.rs (job orchestration) is clean and follows the existing repository patterns.
• The audit table approach for rate limiting is sound and avoids in-memory state issues in multi-instance deployments.
• The TooManyRequests error enhancement with retry_after_seconds is a good backwards-compatible improvement.

Testing coverage

• Support request handler tests cover the main scenarios: successful submission, rejection after limit, per-user isolation, and window expiry.
• Email HTTP client tests use wiremock effectively to verify request payloads and error classification.
• Missing: test for concurrent submissions from the same user (race condition scenario).

---

Key strengths:

• Comprehensive test suite for new functionality
• Clean separation between transport abstraction and provider implementations
• Proper use of transactions for rate limit enforcement
• Good documentation comments explaining design decisions

Key concerns:

• Transaction isolation level may not prevent all race conditions
• No metrics/observability for rate-limited attempts
• Migration ordering could cause CI failures

General findings (auto-demoted from inline due to pre-validation)

• Non-blocking dwctl/src/api/handlers/support.rs:104 — Explicit drop(tx) is unnecessary here.
  • (demoted: code self-check failed at dwctl/src/api/handlers/support.rs:104: diff has // free; the explicit drop is documentation., model claimed drop(tx);)
• Non-blocking dwctl/src/api/handlers/support.rs:107 — Consider adding telemetry for rate-limited submissions.
  • (demoted: code self-check failed at dwctl/src/api/handlers/support.rs:107: diff has message: format!("support request limit reached ({SUPPORT_REQUEST_LIMIT} per hour); please wait befo, model claimed retry_after_seconds: SUPPORT_REQUEST_WINDOW_SECS,)
• Non-blocking dwctl/src/errors.rs:145 — Consider making retry_after_seconds optional for backwards compatibility with callers that don't have a meaningful value.
  • (demoted: code self-check failed at dwctl/src/errors.rs:145: diff has /// for a sliding-window per-user limit)., model claimed TooManyRequests { message: String, retry_after_seconds: u64 },)
• Non-blocking dwctl/src/email_http.rs:166 — Consider treating 408 (Request Timeout) and 425 (Too Early) as transient.
  • (demoted: path "dwctl/src/email_http.rs" is not in the PR diff)
• Nit dwctl/src/email_http.rs:157 — Silent JSON parse failure may hide bugs.
  • (demoted: path "dwctl/src/email_http.rs" is not in the PR diff)
• Non-blocking dwctl/src/email_jobs.rs:159 — Consider including recipient email in the warning log.
  • (demoted: path "dwctl/src/email_jobs.rs" is not in the PR diff)
• Non-blocking dwctl/src/tasks.rs:241 — The .max(1) guard is critical but easy to miss.
  • (demoted: path "dwctl/src/tasks.rs" is not in the PR diff)
• Non-blocking dwctl/src/config.rs:1825 — Config field lacks validation for maximum reasonable value.
  • (demoted: path "dwctl/src/config.rs" is not in the PR diff)

[doubleword-code]

Blocking: Transaction isolation level may not prevent race conditions between concurrent submissions.

Why it matters: The rate limit check (COUNT query at lines 89-99) and the audit row insert (line 101) run in the same transaction, which is correct. However, PostgreSQL's default isolation level (READ COMMITTED) allows non-repeatable reads. Two concurrent transactions could both read recent_count = 4, both decide to proceed, and both insert — resulting in 6 accepted submissions instead of 5.

To fix this, you need either:

1. Serializable isolation: SET TRANSACTION ISOLATION LEVEL SERIALIZABLE — PostgreSQL will detect the conflict and abort one transaction, but this requires retry logic in the handler.
2. Explicit locking: SELECT ... FOR UPDATE on a per-user lock row (requires a separate lock table or using pg_advisory_xact_lock).
3. Unique constraint + retry: Add a unique constraint that enforces the limit at the database level (hard to express for a sliding window).

Suggested fix: Use pg_advisory_xact_lock(hashtext(user_id)) at the start of the transaction to serialize concurrent submissions from the same user. This is lightweight and automatically released on commit/rollback:

sqlx::query!("SELECT pg_advisory_xact_lock(hashtext($1::text))", current_user.id.to_string())
    .execute(&mut *tx)
    .await?;

Then remove the drop(tx) on line 104 — let the transaction commit normally even on rejection (the lock prevents the race).

[doubleword-code]

Non-blocking: Consider adding a comment about data retention.

Why it matters: This table will grow unbounded over time — one row per accepted support request per user. For high-volume support usage, this could become significant. Operators may want to know:

1. How long should rows be retained? (The rate limit only cares about the last hour)
2. Should there be a background job to purge old rows?
3. Is CASCADE delete on user deletion sufficient, or should there be independent TTL?

Suggested fix: Add a comment to the migration or developer documentation:

-- Note: Rows older than the rate-limit window (1 hour) are not queried by the
-- handler. Consider adding a periodic cleanup job if this table grows large:
-- DELETE FROM support_request_submissions WHERE created_at < NOW() - INTERVAL '7 days';

[doubleword-code]

Summary

This PR adds per-user rate limiting to the support request submission endpoint (POST /admin/api/v1/support/requests), capping submissions at 5 per hour per user. The implementation introduces a new support_request_submissions audit table, extends the Error::TooManyRequests variant with a retry_after_seconds field, and migrates the support email sending to the async job queue.

Verdict: Blocked on correctness issue — the rate-limiting logic has a race condition that allows concurrent submissions from the same user to bypass the limit.

Research notes

• Fetched OWASP guidance on rate limiting: emphasizes that rate limits must be enforced atomically to prevent bypass via concurrent requests
• Reviewed existing patterns in the codebase: sla_capacity.rs (lines 205-212) and batches.rs (line 755) both use pg_advisory_xact_lock to serialize concurrent operations that perform read-check-then-write patterns
• PostgreSQL READ COMMITTED isolation (the default) does not prevent TOCTOU races without explicit locking — two transactions can both read the same count before either commits

Suggested next steps

1. Blocking: Add pg_advisory_xact_lock to serialize concurrent submissions per user (see detailed fix in inline comment on line 82)
2. Non-blocking: Fix the hardcoded "per hour" in the error message to use the actual window duration dynamically
3. Non-blocking: Add a concurrency test using tokio::join! to verify the rate limit holds under simultaneous requests
4. Nit: Consider changing SUPPORT_REQUEST_WINDOW_SECS from u64 to i32 since it's always cast down for make_interval

General findings

• The migration is well-designed with appropriate indexes for the query pattern
• The Error::TooManyRequests extension is correctly applied across all call sites (limits.rs, batches.rs)
• Tests cover sequential scenarios but miss the concurrent submission case
• The email job enqueue pattern follows the established async pattern used elsewhere in the codebase

General findings (auto-demoted from inline due to pre-validation)

• Non-blocking dwctl/src/api/handlers/support.rs:88 — Silent narrowing cast from u64 to i32. While safe for the current value of 3600, if someone later changes SUPPORT_REQUEST_WINDOW_SECS to a value larger than i32::MAX (~2.1 billion seconds ≈ 68 years), this cast would overflow silently.
  • (demoted: code self-check failed at dwctl/src/api/handlers/support.rs:88: diff has let mut tx = state.db.write().begin().await.map_err(|e| Error::Database(e.into()))?;, model claimed let window_secs = SUPPORT_REQUEST_WINDOW_SECS as i32;)

[doubleword-code]

Blocking: This comment claims the transaction prevents concurrent submissions from slipping past the cap, but this is incorrect. A transaction alone does NOT prevent the following TOCTOU race under PostgreSQL's default READ COMMITTED isolation:

Transaction A                          Transaction B
-------------                          -------------
BEGIN
SELECT COUNT(*) → returns 4
                                     BEGIN
                                     SELECT COUNT(*) → returns 4
INSERT audit row                     INSERT audit row
COMMIT                               COMMIT

Both transactions see count=4, both pass the check (4 < 5), and both insert — allowing 10 submissions instead of 5.

Why it matters: The entire rate-limiting mechanism can be bypassed by sending concurrent requests. This defeats the stated goal of preventing "one user (script error, automation, or compromised credentials) from draining the shared transactional-email quota."

Suggested fix: Follow the pattern used elsewhere in the codebase for concurrent access control. Add an advisory lock scoped to the user:

let mut tx = state.db.write().begin().await.map_err(|e| Error::Database(e.into()))?;

// Acquire advisory lock to serialize concurrent submissions for this user
sqlx::query!("SELECT pg_advisory_xact_lock(hashtext($1::text))", current_user.id.to_string())
    .execute(&mut *tx)
    .await
    .map_err(|e| Error::Database(e.into()))?;

// ... rest of the rate limit check and insert

See dwctl/src/api/handlers/sla_capacity.rs:205-212 and dwctl/src/api/handlers/batches.rs:755 for the same pattern.

[doubleword-code]

Non-blocking: Hardcoded "per hour" in the error message, but the actual window is determined by SUPPORT_REQUEST_WINDOW_SECS. If someone changes the window to 1800 (30 minutes) or 7200 (2 hours), the error message becomes misleading.

Why it matters: Users receive incorrect information about when they can retry, leading to confusion and support tickets.

Suggested fix: Compute the human-readable duration dynamically:

let window_label = if SUPPORT_REQUEST_WINDOW_SECS == 3600 {
    "hour".to_string()
} else if SUPPORT_REQUEST_WINDOW_SECS >= 86400 {
    format!("{} day(s)", SUPPORT_REQUEST_WINDOW_SECS / 86400)
} else if SUPPORT_REQUEST_WINDOW_SECS >= 60 {
    format!("{} minute(s)", SUPPORT_REQUEST_WINDOW_SECS / 60)
} else {
    format!("{} second(s)", SUPPORT_REQUEST_WINDOW_SECS)
};
return Err(Error::TooManyRequests {
    message: format!("support request limit reached ({SUPPORT_REQUEST_LIMIT} per {window_label}); please wait before submitting again"),
    retry_after_seconds: SUPPORT_REQUEST_WINDOW_SECS,
});

[doubleword-code]

Non-blocking: Consider adding a unique constraint to provide defense-in-depth against the race condition. While an advisory lock is the proper fix, a unique constraint on (user_id, created_at) with a reasonable tolerance could prevent exact duplicates.

That said, this wouldn't fully solve the problem because two submissions milliseconds apart would have different timestamps. The advisory lock approach is still required for correct rate limiting.

[doubleword-code]

Non-blocking: Test coverage gap — the tests verify sequential submissions but don't test concurrent submissions. A proper concurrency test would use tokio::join! or futures::future::join_all to fire multiple requests simultaneously and verify only 5 succeed.

Suggested fix: Add a test like:

#[sqlx::test]
async fn concurrent_submissions_respect_limit(pool: PgPool) {
    let (server, _bg) = create_test_app(pool.clone(), false).await;
    let user = create_test_user(&pool, Role::StandardUser).await;
    let headers = add_auth_headers(&user);

    // Fire 10 concurrent requests
    let futures: Vec<_> = (0..10)
        .map(|_| post_support_request(&server, &headers, "concurrent", "body"))
        .collect();
    let results = futures::future::join_all(futures).await;

    // Exactly 5 should succeed, 5 should get 429
    let ok_count = results.iter().filter(|r| r.status_code() == 200).count();
    let rejected_count = results.iter().filter(|r| r.status_code() == 429).count();
    assert_eq!(ok_count, 5, "only 5 concurrent submissions should succeed");
    assert_eq!(rejected_count, 5, "5 should be rejected");
}

[doubleword-code]

Nit: Well-documented extension to the error type. The docstring clearly explains the purpose of retry_after_seconds and how callers should use it. No changes needed.