From d253189089a43d77535bc81dea7ed98bbbbeda59 Mon Sep 17 00:00:00 2001 From: bootjp Date: Tue, 7 Jul 2026 16:16:29 +0900 Subject: [PATCH 01/16] docs: mark SQS throttling implemented --- adapter/sqs_catalog.go | 2 +- adapter/sqs_throttle.go | 2 +- ...6_implemented_sqs_per_queue_throttling.md} | 26 +++++++++---------- .../2026_06_23_proposed_scaling_roadmap.md | 2 +- 4 files changed, 16 insertions(+), 16 deletions(-) rename docs/design/{2026_04_26_proposed_sqs_per_queue_throttling.md => 2026_04_26_implemented_sqs_per_queue_throttling.md} (84%) diff --git a/adapter/sqs_catalog.go b/adapter/sqs_catalog.go index e450cfe9f..5e4f8af53 100644 --- a/adapter/sqs_catalog.go +++ b/adapter/sqs_catalog.go @@ -533,7 +533,7 @@ var sqsAttributeAppliers = map[string]attributeApplier{ "DeduplicationScope must be 'messageGroup' or 'queue'") }, // Throttle* are non-AWS extensions for per-queue rate limiting, - // see docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md. + // see docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md. // Each accepts a non-negative float64; the cross-attribute // validation that enforces both-zero-or-both-positive on each // (capacity, refill) pair, capacity ≥ refill, hard ceiling, and diff --git a/adapter/sqs_throttle.go b/adapter/sqs_throttle.go index a726c7847..1552b8a74 100644 --- a/adapter/sqs_throttle.go +++ b/adapter/sqs_throttle.go @@ -11,7 +11,7 @@ import ( ) // Per-queue throttling — token-bucket store that hangs off *SQSServer. -// See docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md for +// See docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md for // the full design and rollout context. This file implements §3.1 (bucket // store + token bucket), §3.3 (charging model), §3.4 (Throttling // envelope helpers) and the cache-invalidation primitives §3.1 calls diff --git a/docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md similarity index 84% rename from docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md rename to docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md index 91e66be67..3346a4e3d 100644 --- a/docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md +++ b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md @@ -1,6 +1,6 @@ # Per-Queue Throttling and Tenant Fairness for the SQS Adapter -**Status:** Proposed +**Status:** Implemented **Author:** bootjp **Date:** 2026-04-26 @@ -8,7 +8,7 @@ ## 1. Background and Motivation -elastickv's SQS adapter currently has **no per-queue rate limiting**. A single tenant's runaway producer can: +Before this work, elastickv's SQS adapter had **no per-queue rate limiting**. A single tenant's runaway producer could: 1. Saturate the leader's Raft proposal pipeline (one OCC dispatch per `SendMessage`), pushing latency on every other queue's writes through the same shard. 2. Exhaust the receive-path's visibility-index scan budget (`sqsVisScanPageLimit = 1024`), causing other tenants' `ReceiveMessage` calls to time out empty. @@ -16,7 +16,7 @@ elastickv's SQS adapter currently has **no per-queue rate limiting**. A single t Phase 3.C in [`docs/design/2026_04_24_partial_sqs_compatible_adapter.md`](2026_04_24_partial_sqs_compatible_adapter.md) §16.5 marks this as TODO. AWS itself enforces per-account / per-API limits ("standard request throttle of 3000 RPS per region per AWS account" plus per-API limits like 300 TPS for batch APIs); operators running elastickv as a multi-tenant SQS facade need an equivalent control plane. Without it, the only knobs are (a) shard-level capacity (too coarse — adding a shard requires a Raft membership change) and (b) external load-balancer rate limiting (no visibility into per-queue cost). -This document proposes per-queue token-bucket throttling, configured per-queue in queue meta, evaluated at the SQS-adapter layer on the leader, and surfaced as the same `Throttling.Sender` error AWS uses (so existing SDK retry/backoff logic engages naturally). +This document describes per-queue token-bucket throttling, configured per-queue in queue meta, evaluated at the SQS-adapter layer on the leader, and surfaced as the same `Throttling.Sender` error AWS uses (so existing SDK retry/backoff logic engages naturally). --- @@ -83,7 +83,7 @@ type bucketStore struct { // pair. Each bucket's own mutation (charge / refill) is guarded by // a per-bucket sync.Mutex inside *tokenBucket, scoped to one queue, // so cross-queue traffic never serialises on the same lock. - // (Gemini medium on PR #664 flagged a single-mutex bucket store as + // (A medium review finding on PR #664 flagged a single-mutex bucket store as // a hot-path contention point; this design avoids that.) buckets sync.Map // map[bucketKey]*tokenBucket clock func() time.Time @@ -112,13 +112,13 @@ The `charge` operation: No global lock is held during step 3; concurrent traffic on different queues runs in parallel. -**Cache invalidation on `SetQueueAttributes`**: when an operator updates the throttle config via `SetQueueAttributes`, the handler — *after* the Raft commit that persists the new `sqsQueueThrottle` — calls `buckets.invalidateQueue(name)` (the same path described in the `DeleteQueue` / `CreateQueue` paragraph below). `invalidateQueue` ranges the map and drops every entry whose `queue` matches under a lock-then-`CompareAndDelete`-then-`evicted=true` ordering; a raw per-key `buckets.Delete(key)` would reintroduce the orphan-bucket race that ordering closes (a charger holding the old pointer pre-Delete acquires the bucket's mu after the map entry is gone, sees `evicted=false`, spends a token, then later requests mint a fresh full-capacity bucket — a transient double-allotment window). Without this step at all, the in-memory bucket would keep enforcing the old limits until the idle-eviction sweep removes the stale entry (default 1 h window), defeating the operator's intent to throttle a noisy tenant in real time. The handler also gates the invalidation on a real value change — a same-value `SetQueueAttributes` does not reset the bucket — so a caller cannot bypass the rate limit by re-submitting their own current config. Claude P1 on PR #664 caught the gap; round 9 / round 12 refined the race-free semantics and the no-op gate. +**Cache invalidation on `SetQueueAttributes`**: when an operator updates the throttle config via `SetQueueAttributes`, the handler — *after* the Raft commit that persists the new `sqsQueueThrottle` — calls `buckets.invalidateQueue(name)` (the same path described in the `DeleteQueue` / `CreateQueue` paragraph below). `invalidateQueue` ranges the map and drops every entry whose `queue` matches under a lock-then-`CompareAndDelete`-then-`evicted=true` ordering; a raw per-key `buckets.Delete(key)` would reintroduce the orphan-bucket race that ordering closes (a charger holding the old pointer pre-Delete acquires the bucket's mu after the map entry is gone, sees `evicted=false`, spends a token, then later requests mint a fresh full-capacity bucket — a transient double-allotment window). Without this step at all, the in-memory bucket would keep enforcing the old limits until the idle-eviction sweep removes the stale entry (default 1 h window), defeating the operator's intent to throttle a noisy tenant in real time. The handler also gates the invalidation on a real value change — a same-value `SetQueueAttributes` does not reset the bucket — so a caller cannot bypass the rate limit by re-submitting their own current config. A P1 review finding on PR #664 caught the gap; round 9 / round 12 refined the race-free semantics and the no-op gate. **Cache invalidation on `DeleteQueue` / `CreateQueue`**: when a queue is deleted, the handler — *after* the Raft commit that purges the queue meta — calls `buckets.invalidateQueue(name)`, which ranges the map and drops every `bucketKey` whose `queue` matches (regardless of incarnation), mirroring the `SetQueueAttributes` path above. The `CreateQueue` handler invokes the same call after a genuine create commit (the idempotent-return path skips it) so a same-name recreate that races with in-flight stale-meta traffic still resets the bucket. **Incarnation** participates in the key — `sqsQueueMeta.Incarnation` is set to `lastGen + 1` at `CreateQueue` time and is *preserved* across `PurgeQueue` and `SetQueueAttributes` (the read-modify-write on the meta record carries it forward). A `DeleteQueue`+`CreateQueue` cycle therefore lands the new incarnation at a different `bucketKey` and starts from a fresh full bucket regardless of any per-process cache the previous incarnation left behind on this or any other node. The two mechanisms are complementary — `invalidateQueue` is the cheap hot-path optimisation that keeps the in-memory map small, and the incarnation-keyed structure is the cross-leader correctness guarantee. -**Why Incarnation, not Generation** (Codex P2 on PR #664 round 9): an earlier draft of this design used `sqsQueueMeta.Generation` as the bucket-key discriminator. `Generation` bumps on every `CreateQueue` *and* on every `PurgeQueue` (because message keys are prefixed with the generation, so a purge needs a new prefix to make the old data unreachable). Keying the throttle bucket by `Generation` would therefore re-key the bucket on every purge — letting any caller authorised to call `PurgeQueue` reset the rate limiter to a fresh full bucket once per purge. The 60-second AWS-spec rate limit on `PurgeQueue` bounds the bypass but does not eliminate it. `Incarnation` solves this by isolating the "queue identity changed" semantics (DeleteQueue+CreateQueue cycle) from the "data prefix changed" semantics (any of Create / Delete / Purge), and the throttle layer keys only on the former. +**Why Incarnation, not Generation** (P2 review finding on PR #664 round 9): an earlier draft of this design used `sqsQueueMeta.Generation` as the bucket-key discriminator. `Generation` bumps on every `CreateQueue` *and* on every `PurgeQueue` (because message keys are prefixed with the generation, so a purge needs a new prefix to make the old data unreachable). Keying the throttle bucket by `Generation` would therefore re-key the bucket on every purge — letting any caller authorised to call `PurgeQueue` reset the rate limiter to a fresh full bucket once per purge. The 60-second AWS-spec rate limit on `PurgeQueue` bounds the bypass but does not eliminate it. `Incarnation` solves this by isolating the "queue identity changed" semantics (DeleteQueue+CreateQueue cycle) from the "data prefix changed" semantics (any of Create / Delete / Purge), and the throttle layer keys only on the former. -The bucket map is per-process. There is no Raft replication of bucket state. The behaviour at the moment a node assumes leadership of a queue depends on which of three failover paths fires (Claude low on PR #664 round 7 caught the over-broad earlier wording): +The bucket map is per-process. There is no Raft replication of bucket state. The behaviour at the moment a node assumes leadership of a queue depends on which of three failover paths fires (a low-severity review finding on PR #664 round 7 caught the over-broad earlier wording): 1. **First-time leader** (the most common case for a fresh process): no cached bucket, `loadOrInit` misses, the charge path mints a fresh bucket at full capacity. 2. **Re-elected node, throttle config changed during the prior leader's term**: `loadOrInit` finds the cached bucket but its `capacity`/`refillRate` no longer match the freshly-loaded meta, so the reconciliation path evicts the stale bucket and mints a fresh full-capacity replacement. @@ -161,14 +161,14 @@ Setting `SendCapacity = 100, SendRefillPerSecond = 50` means: bursts up to 100 ` `Default*` fields catch any action not covered by an action-specific pair (so a future `PurgeQueue` rate limit costs nothing once defaults are wired). -**Config-field → bucket-action mapping** (Codex P1 on PR #664 sixth-round Codex review): the JSON config field-name prefixes use short forms (`Send*`, `Recv*`, `Default*`) but the in-memory `bucketKey.action` from §3.1 uses the canonical action vocabulary (`"Send"`, `"Receive"`, `"*"`). The mapping is fixed: `Send*` → `bucketKey{action:"Send"}`, `Recv*` → `bucketKey{action:"Receive"}`, `Default*` → `bucketKey{action:"*"}`. Cache invalidation paragraphs in §3.1 use the bucket-action vocabulary (the actual map keys). Use the config-field vocabulary when discussing the JSON contract (`SetQueueAttributes` payload, `GetQueueAttributes` response) and the bucket-action vocabulary when discussing the in-memory map. Implementation must apply this mapping when looking up buckets after a `SetQueueAttributes` commit. +**Config-field → bucket-action mapping** (P1 review finding on PR #664 sixth-round review): the JSON config field-name prefixes use short forms (`Send*`, `Recv*`, `Default*`) but the in-memory `bucketKey.action` from §3.1 uses the canonical action vocabulary (`"Send"`, `"Receive"`, `"*"`). The mapping is fixed: `Send*` → `bucketKey{action:"Send"}`, `Recv*` → `bucketKey{action:"Receive"}`, `Default*` → `bucketKey{action:"*"}`. Cache invalidation paragraphs in §3.1 use the bucket-action vocabulary (the actual map keys). Use the config-field vocabulary when discussing the JSON contract (`SetQueueAttributes` payload, `GetQueueAttributes` response) and the bucket-action vocabulary when discussing the in-memory map. Implementation must apply this mapping when looking up buckets after a `SetQueueAttributes` commit. The `SetQueueAttributes` validator enforces: - All four `Send*` / `Recv*` fields must be either both zero (disabled) or both positive. - Capacity ≥ refill (otherwise the bucket can never burst above the steady state). - A hard ceiling per queue (e.g. 100,000 RPS) so a typo (`SendCapacity = 1e9`) does not silently mean "no limit at all" but rejects with `InvalidAttributeValue`. -- **Capacity ≥ max single-request charge** (Codex P1 on PR #664 sixth-round review). Per the §3.3 charging table, a `SendMessageBatch` charges up to 10 from the Send bucket and `DeleteMessageBatch` charges up to 10 from the Recv bucket (AWS caps both at 10 entries). Therefore: when `SendCapacity > 0` it must also be `≥ 10`, and when `RecvCapacity > 0` it must also be `≥ 10`. Without this rule, a queue configured with `SendCapacity = 5` enters a permanently unserviceable state for full batches — the bucket can never accumulate the 10 tokens a `SendMessageBatch(len=10)` requires, every full batch is rejected with `Throttling`, and `Retry-After` (§3.4) keeps reporting "wait N seconds" forever with no recovery path short of re-running `SetQueueAttributes`. The validator rejects with `InvalidAttributeValue` and an explicit message naming the per-bucket minimum so the operator sees the cause immediately. **`Default*` requires the same floor**: `resolveActionConfig` in `adapter/sqs_throttle.go` falls Send and Receive traffic through to the Default bucket whenever the dedicated `Send*` / `Recv*` pair is unset, so a `SendMessageBatch` or `DeleteMessageBatch` request can charge the Default bucket. A `DefaultCapacity < 10` therefore creates the same permanently-unserviceable-batch trap as `SendCapacity < 10`. (Earlier drafts of this proposal exempted `Default*` on the assumption that the catch-all set had no batch verb in scope; that was incorrect — the fall-through means batch verbs do hit Default*. The implementation passes `requireBatchCapacity = true` to `validateThrottlePair` for all three action sets — see Codex P1 on PR #679 round 5.) +- **Capacity >= max single-request charge** (P1 review finding on PR #664 sixth-round review). Per the §3.3 charging table, a `SendMessageBatch` charges up to 10 from the Send bucket and `DeleteMessageBatch` charges up to 10 from the Recv bucket (AWS caps both at 10 entries). Therefore: when `SendCapacity > 0` it must also be `>= 10`, and when `RecvCapacity > 0` it must also be `>= 10`. Without this rule, a queue configured with `SendCapacity = 5` enters a permanently unserviceable state for full batches — the bucket can never accumulate the 10 tokens a `SendMessageBatch(len=10)` requires, every full batch is rejected with `Throttling`, and `Retry-After` (§3.4) keeps reporting "wait N seconds" forever with no recovery path short of re-running `SetQueueAttributes`. The validator rejects with `InvalidAttributeValue` and an explicit message naming the per-bucket minimum so the operator sees the cause immediately. **`Default*` requires the same floor**: `resolveActionConfig` in `adapter/sqs_throttle.go` falls Send and Receive traffic through to the Default bucket whenever the dedicated `Send*` / `Recv*` pair is unset, so a `SendMessageBatch` or `DeleteMessageBatch` request can charge the Default bucket. A `DefaultCapacity < 10` therefore creates the same permanently-unserviceable-batch trap as `SendCapacity < 10`. (Earlier drafts of this proposal exempted `Default*` on the assumption that the catch-all set had no batch verb in scope; that was incorrect — the fall-through means batch verbs do hit Default*. The implementation passes `requireBatchCapacity = true` to `validateThrottlePair` for all three action sets — see a P1 review finding on PR #679 round 5.) ### 3.3 Charging model @@ -193,7 +193,7 @@ On rejection: | JSON | HTTP 400, body `{"__type":"Throttling","message":"Rate exceeded for queue '' action ''"}`, header `x-amzn-ErrorType: Throttling`, header `Retry-After: ` (computed per below) | | Query | HTTP 400, body `SenderThrottling......`, headers as above | -`Retry-After` is computed from the *actual* refill rate AND the *requested* token count so neither slow refill nor large batches cause a busy-loop of premature retries (two consecutive Claude reviews on PR #664 caught both: first the `Retry-After: 1` constant lying for sub-1-RPS refill — `SendRefillPerSecond = 0.1` needs 10 s for the next token; then the formula's hardcoded numerator `1.0` lying for batch verbs that charge >1 token — a `SendMessageBatch` of 10 against `refillRate = 1.0` and 0 tokens needs 10 s, not 1): +`Retry-After` is computed from the *actual* refill rate AND the *requested* token count so neither slow refill nor large batches cause a busy-loop of premature retries (two consecutive review findings on PR #664 caught both: first the `Retry-After: 1` constant lying for sub-1-RPS refill — `SendRefillPerSecond = 0.1` needs 10 s for the next token; then the formula's hardcoded numerator `1.0` lying for batch verbs that charge >1 token — a `SendMessageBatch` of 10 against `refillRate = 1.0` and 0 tokens needs 10 s, not 1): ```text needed := float64(requestedCount) - currentTokens @@ -218,7 +218,7 @@ The minimum-1 floor matches `Retry-After`'s integer-second granularity (HTTP/1.1 | `adapter/sqs.go` | After `authorizeSQSRequest`, call `bucketStore.charge(queueName, action, count)`. On reject, write the `Throttling` envelope and return. | | `adapter/sqs_throttle_test.go` (new) | Unit tests for bucket math (edge cases: idle drift, burst, partial refill, batch over-charge, default-off). ~300 lines. | | `adapter/sqs_throttle_integration_test.go` (new) | End-to-end: configure a queue with low limits, send N messages back-to-back, confirm the (N+1)th gets `Throttling` with `Retry-After`. ~150 lines. | -| `monitoring/registry.go` | New counter `sqs_throttled_requests_total{queue, action}` and new **gauge** `sqs_throttle_tokens_remaining{queue, action}`. (Codex P2 on PR #664: tokens go up *and* down so a counter is the wrong instrument.) | +| `monitoring/registry.go` | New counter `sqs_throttled_requests_total{queue, action}` and new **gauge** `sqs_throttle_tokens_remaining{queue, action}`. (P2 review finding on PR #664: tokens go up *and* down so a counter is the wrong instrument.) | | `docs/design/2026_04_24_partial_sqs_compatible_adapter.md` §16.5 | Status update once this lands: TODO → Landed. | ### 4.2 OCC interaction @@ -229,7 +229,7 @@ Throttling sits *outside* the OCC transaction — a rejected request never touch Each queue is owned by exactly one shard (queue-per-shard routing in `kv/shard_router.go`). The leader of that shard owns the bucket. A request that lands on a follower is forwarded by `proxyToLeader` *before* the bucket check, so the bucket is always evaluated by the leader that is also doing the OCC dispatch — no risk of a follower checking against a stale bucket and the leader committing without checking. -Once Phase 3.D (split-queue FIFO) lands, a single queue may span multiple shards. At that point each *partition* gets its own bucket, **keyed by `(queueName, partitionID)`** — not by `MessageGroupId`. `MessageGroupId` is the *input* to `partitionFor`; using it directly as the bucket key would create one bucket per unique group value (unbounded, attacker-amplifiable map size, and hot groups would never share a budget). `partitionID` is bounded by `PartitionCount` so the worst-case bucket count per queue is tiny. The throttle proposal is forward-compatible: the bucket lookup key changes from `queueName` to `(queueName, partitionID)`, and the `bucketKey` struct in §3.1 grows a `partition uint32` field. Documented in §11. (Claude P1 on PR #664 caught the misnomer.) +Once Phase 3.D (split-queue FIFO) lands, a single queue may span multiple shards. At that point each *partition* gets its own bucket, **keyed by `(queueName, partitionID)`** — not by `MessageGroupId`. `MessageGroupId` is the *input* to `partitionFor`; using it directly as the bucket key would create one bucket per unique group value (unbounded, attacker-amplifiable map size, and hot groups would never share a budget). `partitionID` is bounded by `PartitionCount` so the worst-case bucket count per queue is tiny. The throttle proposal is forward-compatible: the bucket lookup key changes from `queueName` to `(queueName, partitionID)`, and the `bucketKey` struct in §3.1 grows a `partition uint32` field. Documented in §11. (P1 review finding on PR #664 caught the misnomer.) **Budget semantics per partition:** each partition's bucket gets the *full* configured `SendCapacity` / `RecvCapacity` / `DefaultCapacity`. The effective aggregate throughput of an N-partition queue is therefore N × the configured per-partition limit. This is intentional and analogous to how AWS High Throughput FIFO multiplies throughput by partition count; operators sizing the throttle should treat `SendCapacity` as the *per-partition* budget. A shared queue-level budget (divided across partitions) would require cross-shard coordination on every `SendMessage` — an extra Raft round-trip per call, defeating the point of partitioning. If per-queue aggregate throttling is needed after Phase 3.D lands, a new `SendCapacityTotal` attribute could be added that gets divided by `PartitionCount` at config time and stored as the per-partition capacity; that design is out of scope for this proposal. @@ -287,7 +287,7 @@ No new flags. Limits are per-queue, set via `SetQueueAttributes`. Defaults are z Two new Prometheus instruments (Section 4.1) expose the throttling activity: - `sqs_throttled_requests_total{queue, action}` — **counter**. Use `rate(...)` per queue in Grafana to spot the noisy tenant. -- `sqs_throttle_tokens_remaining{queue, action}` — **gauge** (Codex P2 on PR #664: token budgets go up *and* down over time, so a counter would mask the depletion that operators most need to see). Sample directly; trending toward zero is the early warning sign. +- `sqs_throttle_tokens_remaining{queue, action}` — **gauge** (P2 review finding on PR #664: token budgets go up *and* down over time, so a counter would mask the depletion that operators most need to see). Sample directly; trending toward zero is the early warning sign. --- diff --git a/docs/design/2026_06_23_proposed_scaling_roadmap.md b/docs/design/2026_06_23_proposed_scaling_roadmap.md index 91afcdfcf..5f94e8b74 100644 --- a/docs/design/2026_06_23_proposed_scaling_roadmap.md +++ b/docs/design/2026_06_23_proposed_scaling_roadmap.md @@ -289,7 +289,7 @@ memory each group's private cache/memtable pins. command worker pool, optional Raft-thread pinning, per-client admission, XREAD O(N)→O(new)), S3 PUT admission control (`2026_04_25_proposed_s3_admission_control.md`), SQS per-queue throttling - (`2026_04_26_proposed_sqs_per_queue_throttling.md`). These bound *one + (`2026_04_26_implemented_sqs_per_queue_throttling.md`). These bound *one workload's* impact so it cannot starve the shared runtime / Raft control plane; they scale a deployment by making it predictable under adversarial or unbalanced load rather than by raising raw capacity. From f3eaaa27c4eb76a3390f85c0f172ad03da337bb6 Mon Sep 17 00:00:00 2001 From: bootjp Date: Tue, 7 Jul 2026 16:22:53 +0900 Subject: [PATCH 02/16] docs: update SQS throttle wording --- docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md index 3346a4e3d..0fd23df19 100644 --- a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md +++ b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md @@ -229,7 +229,7 @@ Throttling sits *outside* the OCC transaction — a rejected request never touch Each queue is owned by exactly one shard (queue-per-shard routing in `kv/shard_router.go`). The leader of that shard owns the bucket. A request that lands on a follower is forwarded by `proxyToLeader` *before* the bucket check, so the bucket is always evaluated by the leader that is also doing the OCC dispatch — no risk of a follower checking against a stale bucket and the leader committing without checking. -Once Phase 3.D (split-queue FIFO) lands, a single queue may span multiple shards. At that point each *partition* gets its own bucket, **keyed by `(queueName, partitionID)`** — not by `MessageGroupId`. `MessageGroupId` is the *input* to `partitionFor`; using it directly as the bucket key would create one bucket per unique group value (unbounded, attacker-amplifiable map size, and hot groups would never share a budget). `partitionID` is bounded by `PartitionCount` so the worst-case bucket count per queue is tiny. The throttle proposal is forward-compatible: the bucket lookup key changes from `queueName` to `(queueName, partitionID)`, and the `bucketKey` struct in §3.1 grows a `partition uint32` field. Documented in §11. (P1 review finding on PR #664 caught the misnomer.) +Once Phase 3.D (split-queue FIFO) lands, a single queue may span multiple shards. At that point each *partition* gets its own bucket, **keyed by `(queueName, partitionID)`** — not by `MessageGroupId`. `MessageGroupId` is the *input* to `partitionFor`; using it directly as the bucket key would create one bucket per unique group value (unbounded, attacker-amplifiable map size, and hot groups would never share a budget). `partitionID` is bounded by `PartitionCount` so the worst-case bucket count per queue is tiny. The throttle design is forward-compatible: the bucket lookup key changes from `queueName` to `(queueName, partitionID)`, and the `bucketKey` struct in §3.1 grows a `partition uint32` field. Documented in §11. (P1 review finding on PR #664 caught the misnomer.) **Budget semantics per partition:** each partition's bucket gets the *full* configured `SendCapacity` / `RecvCapacity` / `DefaultCapacity`. The effective aggregate throughput of an N-partition queue is therefore N × the configured per-partition limit. This is intentional and analogous to how AWS High Throughput FIFO multiplies throughput by partition count; operators sizing the throttle should treat `SendCapacity` as the *per-partition* budget. A shared queue-level budget (divided across partitions) would require cross-shard coordination on every `SendMessage` — an extra Raft round-trip per call, defeating the point of partitioning. If per-queue aggregate throttling is needed after Phase 3.D lands, a new `SendCapacityTotal` attribute could be added that gets divided by `PartitionCount` at config time and stored as the per-partition capacity; that design is out of scope for this proposal. From f63ec936742a260f332559064784049ad09e8947 Mon Sep 17 00:00:00 2001 From: bootjp Date: Tue, 7 Jul 2026 16:32:10 +0900 Subject: [PATCH 03/16] docs: narrow SQS throttle query coverage --- .../2026_04_26_implemented_sqs_per_queue_throttling.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md index 0fd23df19..44985f3ae 100644 --- a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md +++ b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md @@ -26,7 +26,7 @@ This document describes per-queue token-bucket throttling, configured per-queue 1. **Per-queue rate limits** that an operator can set via `SetQueueAttributes` and read back via `GetQueueAttributes`. Limits are persisted on the queue meta record (one Raft commit, no separate keyspace). 2. **Per-action granularity** — `SendMessage` and `ReceiveMessage` have independent buckets so a slow consumer cannot pin the producer or vice versa. Batch verbs charge by entry count, not by call count. -3. **AWS-shape errors**: throttled requests return HTTP `400` with the `Throttling` error code in whichever envelope the request protocol uses (`{"__type":"Throttling", ...}` for the JSON path, `Throttling` for the query/XML path — see §3.4 for the exact wire shape per protocol) and a `Retry-After` header. SDKs already special-case this code with exponential backoff; we do not invent a new code. +3. **AWS-shape errors**: throttled JSON-path requests return HTTP `400` with the `Throttling` error code (`{"__type":"Throttling", ...}`) and a `Retry-After` header. SDKs already special-case this code with exponential backoff; we do not invent a new code. Query/XML message verbs are not claimed by this lifecycle update until the query dispatcher wires those verbs into the same throttled handlers. 4. **Default-off**. Queues created before this feature, and queues created without explicit limits, are not throttled. Operators opt in per queue. 5. **No coordination per request**. Token replenishment is local to whichever node owns the bucket (the leader for the queue's shard); there is no Raft round-trip on the throttling check. 6. **Observable**: per-queue throttle counters are exposed via the existing Prometheus registry so dashboards can spot throttling before users do. @@ -66,7 +66,7 @@ The check sits **between SigV4 authorisation and the existing handler dispatch** - the queue name (parsed from the request body's `QueueUrl` / `QueueName`); - this node is the verified leader for the queue's shard (`isVerifiedSQSLeader`); any non-leader has been forwarded by `proxyToLeader` and re-evaluates the limit on landing; -- the action (`X-Amz-Target` for JSON, `Action` form parameter for query). +- the action (`X-Amz-Target` for the implemented JSON path; future query/XML message-verb wiring will use the `Action` form parameter). ### 3.1 Where the bucket lives @@ -191,7 +191,7 @@ On rejection: | Protocol | Response | |---|---| | JSON | HTTP 400, body `{"__type":"Throttling","message":"Rate exceeded for queue '' action ''"}`, header `x-amzn-ErrorType: Throttling`, header `Retry-After: ` (computed per below) | -| Query | HTTP 400, body `SenderThrottling......`, headers as above | +| Query | Not implemented for throttled message verbs in this lifecycle update. The current query dispatcher wires catalog verbs only and returns `NotImplementedYet` for message actions; when query message handlers land, they should mirror the JSON throttling code with `Throttling` in the XML error body. | `Retry-After` is computed from the *actual* refill rate AND the *requested* token count so neither slow refill nor large batches cause a busy-loop of premature retries (two consecutive review findings on PR #664 caught both: first the `Retry-After: 1` constant lying for sub-1-RPS refill — `SendRefillPerSecond = 0.1` needs 10 s for the next token; then the formula's hardcoded numerator `1.0` lying for batch verbs that charge >1 token — a `SendMessageBatch` of 10 against `refillRate = 1.0` and 0 tokens needs 10 s, not 1): @@ -272,7 +272,7 @@ Strict-validation SDKs that reject unknown attribute names will reject `Throttle 4. **Configuration round-trip**: `SetQueueAttributes` with throttle config → `GetQueueAttributes` returns the same values; an unknown `Throttle*` attribute name is rejected with 400 `InvalidAttributeName` (matching AWS behaviour for unrecognised attributes). -5. **Cross-protocol parity**: throttled JSON and Query requests both surface `Throttling` (different envelope, same code). +5. **JSON protocol coverage**: throttled JSON requests surface `Throttling` with `Retry-After`. Query/XML parity is a follow-up gated on wiring the query message handlers to the same throttle path. 6. **Failover behaviour** (3-node cluster): kill the current leader after 3 messages, confirm the next leader starts the bucket fresh and accepts up to capacity again. Log line records the failover so operators can correlate. From 02dcd9f6718d9259570a6bbc63e1ae768c71b35a Mon Sep 17 00:00:00 2001 From: bootjp Date: Tue, 7 Jul 2026 16:36:39 +0900 Subject: [PATCH 04/16] docs: align sqs throttling scope --- ..._04_26_implemented_sqs_per_queue_throttling.md | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md index 44985f3ae..3c2661751 100644 --- a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md +++ b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md @@ -29,7 +29,7 @@ This document describes per-queue token-bucket throttling, configured per-queue 3. **AWS-shape errors**: throttled JSON-path requests return HTTP `400` with the `Throttling` error code (`{"__type":"Throttling", ...}`) and a `Retry-After` header. SDKs already special-case this code with exponential backoff; we do not invent a new code. Query/XML message verbs are not claimed by this lifecycle update until the query dispatcher wires those verbs into the same throttled handlers. 4. **Default-off**. Queues created before this feature, and queues created without explicit limits, are not throttled. Operators opt in per queue. 5. **No coordination per request**. Token replenishment is local to whichever node owns the bucket (the leader for the queue's shard); there is no Raft round-trip on the throttling check. -6. **Observable**: per-queue throttle counters are exposed via the existing Prometheus registry so dashboards can spot throttling before users do. +6. **Observable at the response boundary**: throttled requests use the AWS-shaped `Throttling` error and `Retry-After` header so clients and request logs expose the limit immediately. Prometheus throttle counters/gauges are a follow-up and are not part of this lifecycle update. ### 2.2 Non-Goals @@ -218,7 +218,7 @@ The minimum-1 floor matches `Retry-After`'s integer-second granularity (HTTP/1.1 | `adapter/sqs.go` | After `authorizeSQSRequest`, call `bucketStore.charge(queueName, action, count)`. On reject, write the `Throttling` envelope and return. | | `adapter/sqs_throttle_test.go` (new) | Unit tests for bucket math (edge cases: idle drift, burst, partial refill, batch over-charge, default-off). ~300 lines. | | `adapter/sqs_throttle_integration_test.go` (new) | End-to-end: configure a queue with low limits, send N messages back-to-back, confirm the (N+1)th gets `Throttling` with `Retry-After`. ~150 lines. | -| `monitoring/registry.go` | New counter `sqs_throttled_requests_total{queue, action}` and new **gauge** `sqs_throttle_tokens_remaining{queue, action}`. (P2 review finding on PR #664: tokens go up *and* down so a counter is the wrong instrument.) | +| `monitoring/registry.go` | No throttle-specific Prometheus collector is registered by this lifecycle update. The existing SQS monitoring surface continues to expose partition activity and queue depth; throttle counters/gauges remain future work. | | `docs/design/2026_04_24_partial_sqs_compatible_adapter.md` §16.5 | Status update once this lands: TODO → Landed. | ### 4.2 OCC interaction @@ -229,9 +229,9 @@ Throttling sits *outside* the OCC transaction — a rejected request never touch Each queue is owned by exactly one shard (queue-per-shard routing in `kv/shard_router.go`). The leader of that shard owns the bucket. A request that lands on a follower is forwarded by `proxyToLeader` *before* the bucket check, so the bucket is always evaluated by the leader that is also doing the OCC dispatch — no risk of a follower checking against a stale bucket and the leader committing without checking. -Once Phase 3.D (split-queue FIFO) lands, a single queue may span multiple shards. At that point each *partition* gets its own bucket, **keyed by `(queueName, partitionID)`** — not by `MessageGroupId`. `MessageGroupId` is the *input* to `partitionFor`; using it directly as the bucket key would create one bucket per unique group value (unbounded, attacker-amplifiable map size, and hot groups would never share a budget). `partitionID` is bounded by `PartitionCount` so the worst-case bucket count per queue is tiny. The throttle design is forward-compatible: the bucket lookup key changes from `queueName` to `(queueName, partitionID)`, and the `bucketKey` struct in §3.1 grows a `partition uint32` field. Documented in §11. (P1 review finding on PR #664 caught the misnomer.) +Split-queue FIFO has landed, but the throttle implementation still uses one aggregate bucket per `(queueName, action, incarnation)`. Partitioned queues therefore share the configured `SendCapacity` / `RecvCapacity` / `DefaultCapacity` across the logical queue; the effective queue throughput is the configured capacity, not `PartitionCount` times that value. This matches the current dispatch path: `SendMessage` charges the queue-level bucket after loading queue metadata and before partition-specific routing. -**Budget semantics per partition:** each partition's bucket gets the *full* configured `SendCapacity` / `RecvCapacity` / `DefaultCapacity`. The effective aggregate throughput of an N-partition queue is therefore N × the configured per-partition limit. This is intentional and analogous to how AWS High Throughput FIFO multiplies throughput by partition count; operators sizing the throttle should treat `SendCapacity` as the *per-partition* budget. A shared queue-level budget (divided across partitions) would require cross-shard coordination on every `SendMessage` — an extra Raft round-trip per call, defeating the point of partitioning. If per-queue aggregate throttling is needed after Phase 3.D lands, a new `SendCapacityTotal` attribute could be added that gets divided by `PartitionCount` at config time and stored as the per-partition capacity; that design is out of scope for this proposal. +Per-partition buckets remain future work. That change must move the charge point to a path that already knows `partitionFor(meta, MessageGroupId)`, extend `bucketKey` with a bounded `partition uint32` field, and audit the send/receive/batch callers so every throttled action uses the same bucket semantics. `MessageGroupId` itself must never be used as the bucket key because it is unbounded and caller-controlled. --- @@ -284,10 +284,7 @@ Strict-validation SDKs that reject unknown attribute names will reject `Throttle No new flags. Limits are per-queue, set via `SetQueueAttributes`. Defaults are zero (disabled). -Two new Prometheus instruments (Section 4.1) expose the throttling activity: - -- `sqs_throttled_requests_total{queue, action}` — **counter**. Use `rate(...)` per queue in Grafana to spot the noisy tenant. -- `sqs_throttle_tokens_remaining{queue, action}` — **gauge** (P2 review finding on PR #664: token budgets go up *and* down over time, so a counter would mask the depletion that operators most need to see). Sample directly; trending toward zero is the early warning sign. +No throttle-specific Prometheus instruments ship in this lifecycle update. Operators can observe throttling through the HTTP `Throttling` response, the `Retry-After` header, and request logs. A follow-up monitoring change should add a per-queue/action throttled-request counter and a token-remaining gauge; the token budget must be a gauge because it goes both up and down over time. --- @@ -325,7 +322,7 @@ Every `charge` proposes a bucket update through the FSM. **Rejected**: an extra |---|---| | 1 | Doc lands (this PR). No code yet. Operators have time to comment. | | 2 | Implementation PR per §4.1. Default-off; existing queues unaffected. | -| 3 | Operators opt in per queue via `SetQueueAttributes`. Monitor `sqs_throttled_requests_total` for false positives. | +| 3 | Operators opt in per queue via `SetQueueAttributes`. Watch `Throttling` responses and request logs for false positives. | | 4 | Once stable, the partial doc's TODO list moves 3.C from TODO to Landed. | --- From 2c5cdac2e9b9b2f452a59228ddaa84af87d3ed0d Mon Sep 17 00:00:00 2001 From: bootjp Date: Tue, 7 Jul 2026 17:00:21 +0900 Subject: [PATCH 05/16] docs: align sqs throttling lifecycle --- .../2026_04_24_proposed_sqs_compatible_adapter.md | 2 +- ...026_04_26_implemented_sqs_per_queue_throttling.md | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md b/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md index ed036ba24..8c4c696e9 100644 --- a/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md +++ b/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md @@ -687,7 +687,7 @@ Structured logs include `route`, `queue`, `action`, `remote_ip`, `token_hash_pre 1. Query-protocol full XML fidelity for older SDKs. 2. `ApproximateNumberOfMessagesDelayed` / `NotVisible` accuracy. -3. Per-queue throttling and fairness across tenants. +3. Per-queue throttling and fairness across tenants has landed; see [`2026_04_26_implemented_sqs_per_queue_throttling.md`](2026_04_26_implemented_sqs_per_queue_throttling.md). 4. Split-queue FIFO for very hot queues. 5. Console UI polish: in-flight tab with per-message countdown, filtering, dark mode. diff --git a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md index 3c2661751..f55a9bd9f 100644 --- a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md +++ b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md @@ -14,7 +14,7 @@ Before this work, elastickv's SQS adapter had **no per-queue rate limiting**. A 2. Exhaust the receive-path's visibility-index scan budget (`sqsVisScanPageLimit = 1024`), causing other tenants' `ReceiveMessage` calls to time out empty. 3. Fill the message keyspace fast enough that the retention reaper cannot keep up — the keyspace grows unbounded until the next manual purge. -Phase 3.C in [`docs/design/2026_04_24_partial_sqs_compatible_adapter.md`](2026_04_24_partial_sqs_compatible_adapter.md) §16.5 marks this as TODO. AWS itself enforces per-account / per-API limits ("standard request throttle of 3000 RPS per region per AWS account" plus per-API limits like 300 TPS for batch APIs); operators running elastickv as a multi-tenant SQS facade need an equivalent control plane. Without it, the only knobs are (a) shard-level capacity (too coarse — adding a shard requires a Raft membership change) and (b) external load-balancer rate limiting (no visibility into per-queue cost). +Phase 3.C in [`docs/design/2026_04_24_proposed_sqs_compatible_adapter.md`](2026_04_24_proposed_sqs_compatible_adapter.md) §14 originally listed this as future work. AWS itself enforces per-account / per-API limits ("standard request throttle of 3000 RPS per region per AWS account" plus per-API limits like 300 TPS for batch APIs); operators running elastickv as a multi-tenant SQS facade need an equivalent control plane. Without it, the only knobs are (a) shard-level capacity (too coarse — adding a shard requires a Raft membership change) and (b) external load-balancer rate limiting (no visibility into per-queue cost). This document describes per-queue token-bucket throttling, configured per-queue in queue meta, evaluated at the SQS-adapter layer on the leader, and surfaced as the same `Throttling.Sender` error AWS uses (so existing SDK retry/backoff logic engages naturally). @@ -29,7 +29,7 @@ This document describes per-queue token-bucket throttling, configured per-queue 3. **AWS-shape errors**: throttled JSON-path requests return HTTP `400` with the `Throttling` error code (`{"__type":"Throttling", ...}`) and a `Retry-After` header. SDKs already special-case this code with exponential backoff; we do not invent a new code. Query/XML message verbs are not claimed by this lifecycle update until the query dispatcher wires those verbs into the same throttled handlers. 4. **Default-off**. Queues created before this feature, and queues created without explicit limits, are not throttled. Operators opt in per queue. 5. **No coordination per request**. Token replenishment is local to whichever node owns the bucket (the leader for the queue's shard); there is no Raft round-trip on the throttling check. -6. **Observable at the response boundary**: throttled requests use the AWS-shaped `Throttling` error and `Retry-After` header so clients and request logs expose the limit immediately. Prometheus throttle counters/gauges are a follow-up and are not part of this lifecycle update. +6. **Client-visible throttling outcome**: throttled requests use the AWS-shaped `Throttling` error and `Retry-After` header so clients can observe and back off from the limit immediately. Server-side Prometheus counters/gauges remain a follow-up and are not part of this lifecycle update. ### 2.2 Non-Goals @@ -219,7 +219,7 @@ The minimum-1 floor matches `Retry-After`'s integer-second granularity (HTTP/1.1 | `adapter/sqs_throttle_test.go` (new) | Unit tests for bucket math (edge cases: idle drift, burst, partial refill, batch over-charge, default-off). ~300 lines. | | `adapter/sqs_throttle_integration_test.go` (new) | End-to-end: configure a queue with low limits, send N messages back-to-back, confirm the (N+1)th gets `Throttling` with `Retry-After`. ~150 lines. | | `monitoring/registry.go` | No throttle-specific Prometheus collector is registered by this lifecycle update. The existing SQS monitoring surface continues to expose partition activity and queue depth; throttle counters/gauges remain future work. | -| `docs/design/2026_04_24_partial_sqs_compatible_adapter.md` §16.5 | Status update once this lands: TODO → Landed. | +| `docs/design/2026_04_24_proposed_sqs_compatible_adapter.md` §14 | Phase 3 per-queue throttling item marked landed with a link to this design. | ### 4.2 OCC interaction @@ -284,7 +284,7 @@ Strict-validation SDKs that reject unknown attribute names will reject `Throttle No new flags. Limits are per-queue, set via `SetQueueAttributes`. Defaults are zero (disabled). -No throttle-specific Prometheus instruments ship in this lifecycle update. Operators can observe throttling through the HTTP `Throttling` response, the `Retry-After` header, and request logs. A follow-up monitoring change should add a per-queue/action throttled-request counter and a token-remaining gauge; the token budget must be a gauge because it goes both up and down over time. +No throttle-specific Prometheus instruments or server-side throttle access logs ship in this lifecycle update. The shipped signal is the HTTP `Throttling` response and `Retry-After` header returned to the caller. A follow-up monitoring change should add a per-queue/action throttled-request counter and a token-remaining gauge; the token budget must be a gauge because it goes both up and down over time. --- @@ -322,8 +322,8 @@ Every `charge` proposes a bucket update through the FSM. **Rejected**: an extra |---|---| | 1 | Doc lands (this PR). No code yet. Operators have time to comment. | | 2 | Implementation PR per §4.1. Default-off; existing queues unaffected. | -| 3 | Operators opt in per queue via `SetQueueAttributes`. Watch `Throttling` responses and request logs for false positives. | -| 4 | Once stable, the partial doc's TODO list moves 3.C from TODO to Landed. | +| 3 | Operators opt in per queue via `SetQueueAttributes`. Watch caller-visible `Throttling` responses for false positives. | +| 4 | Parent SQS adapter roadmap marks Phase 3.C as landed with a link to this design. | --- From 091b444628b6b298d7d1cd45d765e41be22d7ca8 Mon Sep 17 00:00:00 2001 From: bootjp Date: Tue, 7 Jul 2026 17:42:24 +0900 Subject: [PATCH 06/16] Add SQS throttle metrics --- adapter/sqs.go | 35 ++++ adapter/sqs_throttle.go | 17 ++ adapter/sqs_throttle_test.go | 44 +++++ ...6_04_24_proposed_sqs_compatible_adapter.md | 10 +- ...26_implemented_sqs_per_queue_throttling.md | 13 +- main_sqs.go | 5 + monitoring/sqs.go | 154 ++++++++++++++++-- monitoring/sqs_test.go | 66 +++++++- 8 files changed, 320 insertions(+), 24 deletions(-) diff --git a/adapter/sqs.go b/adapter/sqs.go index b7da6ddfe..704688519 100644 --- a/adapter/sqs.go +++ b/adapter/sqs.go @@ -208,6 +208,11 @@ type SQSServer struct { // Increment call sites use a nil-receiver-safe call so the // metrics path costs nothing when unwired. partitionObserver SQSPartitionObserver + // throttleObserver records configured per-queue throttle + // outcomes: rejected-request counters and remaining-token gauges. + // nil on non-monitored fixtures; observeThrottleDecision is + // nil-safe so the request path pays one branch when unwired. + throttleObserver SQSThrottleObserver } // SQSPartitionObserver is the metrics-package interface @@ -218,6 +223,13 @@ type SQSPartitionObserver interface { ObservePartitionMessage(queue string, partition uint32, action string) } +// SQSThrottleObserver is the metrics-package interface +// (monitoring.SQSThrottleObserver) re-declared here so the adapter +// does not import monitoring at the package boundary. +type SQSThrottleObserver interface { + ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) +} + // SQSPartitionAction* mirror the action label values from // monitoring.SQSPartitionAction*. Re-declared so adapter call // sites do not need a monitoring import; the observer interface @@ -253,6 +265,14 @@ func WithSQSPartitionObserver(o SQSPartitionObserver) SQSServerOption { return func(s *SQSServer) { s.partitionObserver = o } } +// WithSQSThrottleObserver installs the +// elastickv_sqs_throttled_requests_total and +// elastickv_sqs_throttle_tokens_remaining observer on the SQS +// server. Pass nil (the default) on non-monitored fixtures. +func WithSQSThrottleObserver(o SQSThrottleObserver) SQSServerOption { + return func(s *SQSServer) { s.throttleObserver = o } +} + // observePartitionMessage is a nil-receiver-safe wrapper around // the configured observer. Pulled into a helper so the call // sites in send / receive / delete each cost one branch instead @@ -264,6 +284,21 @@ func (s *SQSServer) observePartitionMessage(queue string, partition uint32, acti s.partitionObserver.ObservePartitionMessage(queue, partition, action) } +func (s *SQSServer) observeThrottleDecision(queue string, outcome chargeOutcome) { + if s == nil || s.throttleObserver == nil { + return + } + if !outcome.bucketPresent && outcome.allowed { + return + } + s.throttleObserver.ObserveThrottleDecision( + queue, + sqsThrottleMetricAction(outcome.action), + outcome.tokensAfter, + !outcome.allowed, + ) +} + // WithSQSPartitionResolver installs the cluster's partition // resolver on the SQS server so the CreateQueue capability gate // (validateHTFIFOCapability) can verify routing coverage before diff --git a/adapter/sqs_throttle.go b/adapter/sqs_throttle.go index 1552b8a74..c8ed87039 100644 --- a/adapter/sqs_throttle.go +++ b/adapter/sqs_throttle.go @@ -167,6 +167,7 @@ type chargeOutcome struct { retryAfter time.Duration tokensAfter float64 bucketPresent bool + action string } // charge takes count tokens from the bucket identified by (queue, @@ -212,6 +213,7 @@ func (b *bucketStore) charge(cfg *sqsQueueThrottle, queue, action string, incarn bucket := b.loadOrInit(queue, resolvedAction, incarnation, capacity, refill) outcome, retry := chargeBucket(bucket, b.clock(), count) if !retry { + outcome.action = resolvedAction return outcome } } @@ -230,6 +232,7 @@ func (b *bucketStore) charge(cfg *sqsQueueThrottle, queue, action string, incarn allowed: false, retryAfter: time.Second, bucketPresent: false, + action: resolvedAction, } } @@ -498,6 +501,19 @@ func resolveActionConfig(cfg *sqsQueueThrottle, action string) (string, float64, return action, 0, 0 } +func sqsThrottleMetricAction(action string) string { + switch action { + case bucketActionSend: + return "send" + case bucketActionReceive: + return "receive" + case bucketActionAny: + return "default" + default: + return "default" + } +} + // throttleRetryAfterCap bounds the Retry-After value the client sees. // Without a cap, a tiny refillRate plus // a large requested count would compute a multi-day wait — and @@ -617,6 +633,7 @@ func (s *SQSServer) chargeQueueWithThrottle(w http.ResponseWriter, queueName, ac return true } outcome := s.throttle.charge(throttle, queueName, action, incarnation, count) + s.observeThrottleDecision(queueName, outcome) if outcome.allowed { return true } diff --git a/adapter/sqs_throttle_test.go b/adapter/sqs_throttle_test.go index eddc732d4..cd7107c66 100644 --- a/adapter/sqs_throttle_test.go +++ b/adapter/sqs_throttle_test.go @@ -1,6 +1,8 @@ package adapter import ( + "net/http" + "net/http/httptest" "sync" "sync/atomic" "testing" @@ -9,6 +11,26 @@ import ( "github.com/stretchr/testify/require" ) +type throttleObserveReport struct { + queue string + action string + tokensRemaining float64 + throttled bool +} + +type recordingSQSThrottleObserver struct { + reports []throttleObserveReport +} + +func (r *recordingSQSThrottleObserver) ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) { + r.reports = append(r.reports, throttleObserveReport{ + queue: queue, + action: action, + tokensRemaining: tokensRemaining, + throttled: throttled, + }) +} + // TestBucketStore_DefaultOff_ShortCircuit pins the contract that a // nil throttle config never allocates a bucket and never rejects. // This is the hot path for unconfigured queues — every nil-check that @@ -24,6 +46,28 @@ func TestBucketStore_DefaultOff_ShortCircuit(t *testing.T) { } } +func TestSQSServer_ChargeQueueWithThrottleObservesMetrics(t *testing.T) { + t.Parallel() + observer := &recordingSQSThrottleObserver{} + srv := NewSQSServer(nil, nil, nil, WithSQSThrottleObserver(observer)) + cfg := &sqsQueueThrottle{SendCapacity: 10, SendRefillPerSecond: 1} + + for range 10 { + rec := httptest.NewRecorder() + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1)) + } + rec := httptest.NewRecorder() + require.False(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1)) + require.Equal(t, http.StatusBadRequest, rec.Code) + + require.Len(t, observer.reports, 11) + last := observer.reports[len(observer.reports)-1] + require.Equal(t, "orders.fifo", last.queue) + require.Equal(t, "send", last.action) + require.True(t, last.throttled) + require.InDelta(t, 0.0, last.tokensRemaining, 0.001) +} + // TestBucketStore_Empty_ShortCircuit covers the post-validator // canonicalisation path: an all-zero sqsQueueThrottle is equivalent // to nil. Without this branch, a queue whose operator wrote diff --git a/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md b/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md index 8c4c696e9..d905a88cf 100644 --- a/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md +++ b/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md @@ -460,10 +460,12 @@ New metrics (prefixed `sqs_`): 5. `sqs_messages_received_total{queue, is_fifo}` 6. `sqs_messages_deleted_total{queue}` 7. `sqs_messages_in_flight{queue}` -8. `sqs_longpoll_waiters{queue}` -9. `sqs_longpoll_wake_latency_seconds` -10. `sqs_dlq_redrive_total{queue}` -11. `sqs_proxy_to_leader_total{op}` +8. `sqs_throttled_requests_total{queue, action}` +9. `sqs_throttle_tokens_remaining{queue, action}` +10. `sqs_longpoll_waiters{queue}` +11. `sqs_longpoll_wake_latency_seconds` +12. `sqs_dlq_redrive_total{queue}` +13. `sqs_proxy_to_leader_total{op}` Structured log fields match the rest of the project: `queue`, `message_id`, `receipt_token_prefix`, `group_id`, `dedup_id`, `commit_ts`, `leader`. diff --git a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md index f55a9bd9f..b6dc278ac 100644 --- a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md +++ b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md @@ -29,7 +29,7 @@ This document describes per-queue token-bucket throttling, configured per-queue 3. **AWS-shape errors**: throttled JSON-path requests return HTTP `400` with the `Throttling` error code (`{"__type":"Throttling", ...}`) and a `Retry-After` header. SDKs already special-case this code with exponential backoff; we do not invent a new code. Query/XML message verbs are not claimed by this lifecycle update until the query dispatcher wires those verbs into the same throttled handlers. 4. **Default-off**. Queues created before this feature, and queues created without explicit limits, are not throttled. Operators opt in per queue. 5. **No coordination per request**. Token replenishment is local to whichever node owns the bucket (the leader for the queue's shard); there is no Raft round-trip on the throttling check. -6. **Client-visible throttling outcome**: throttled requests use the AWS-shaped `Throttling` error and `Retry-After` header so clients can observe and back off from the limit immediately. Server-side Prometheus counters/gauges remain a follow-up and are not part of this lifecycle update. +6. **Observable throttling outcome**: throttled requests use the AWS-shaped `Throttling` error and `Retry-After` header so clients can observe and back off from the limit immediately. The server also emits `elastickv_sqs_throttled_requests_total{queue, action}` and `elastickv_sqs_throttle_tokens_remaining{queue, action}` so operators can alert on rejects and inspect the current bucket balance. ### 2.2 Non-Goals @@ -215,10 +215,10 @@ The minimum-1 floor matches `Retry-After`'s integer-second granularity (HTTP/1.1 |---|---| | `adapter/sqs_throttle.go` (new) | `bucketStore`, `tokenBucket`, charging helper. ~250 lines. | | `adapter/sqs_catalog.go` | Add `Throttle` field to `sqsQueueMeta`. Extend `applyAttributes` with the new `Throttle*` attribute names. Render the four Throttle fields in `queueMetaToAttributes` so `GetQueueAttributes("All")` surfaces them. | -| `adapter/sqs.go` | After `authorizeSQSRequest`, call `bucketStore.charge(queueName, action, count)`. On reject, write the `Throttling` envelope and return. | +| `adapter/sqs.go` | After `authorizeSQSRequest`, call `bucketStore.charge(queueName, action, count)`. On reject, write the `Throttling` envelope and return. Also forwards each configured bucket decision to the SQS throttle metrics observer. | | `adapter/sqs_throttle_test.go` (new) | Unit tests for bucket math (edge cases: idle drift, burst, partial refill, batch over-charge, default-off). ~300 lines. | | `adapter/sqs_throttle_integration_test.go` (new) | End-to-end: configure a queue with low limits, send N messages back-to-back, confirm the (N+1)th gets `Throttling` with `Retry-After`. ~150 lines. | -| `monitoring/registry.go` | No throttle-specific Prometheus collector is registered by this lifecycle update. The existing SQS monitoring surface continues to expose partition activity and queue depth; throttle counters/gauges remain future work. | +| `monitoring/sqs.go`, `monitoring/registry.go` | Register the `elastickv_sqs_throttled_requests_total{queue, action}` counter and `elastickv_sqs_throttle_tokens_remaining{queue, action}` gauge. Queue-label cardinality uses the same capped `_other` fallback as the existing SQS metrics. | | `docs/design/2026_04_24_proposed_sqs_compatible_adapter.md` §14 | Phase 3 per-queue throttling item marked landed with a link to this design. | ### 4.2 OCC interaction @@ -284,7 +284,12 @@ Strict-validation SDKs that reject unknown attribute names will reject `Throttle No new flags. Limits are per-queue, set via `SetQueueAttributes`. Defaults are zero (disabled). -No throttle-specific Prometheus instruments or server-side throttle access logs ship in this lifecycle update. The shipped signal is the HTTP `Throttling` response and `Retry-After` header returned to the caller. A follow-up monitoring change should add a per-queue/action throttled-request counter and a token-remaining gauge; the token budget must be a gauge because it goes both up and down over time. +Throttle outcomes are visible in two places: + +1. The HTTP response: rejected JSON-path message requests return `Throttling` with `Retry-After`. +2. Prometheus: `elastickv_sqs_throttled_requests_total{queue, action}` increments for rejected requests, and `elastickv_sqs_throttle_tokens_remaining{queue, action}` records the remaining token balance after each configured bucket decision. `action` is the effective bucket action: `send`, `receive`, or `default`. + +No server-side throttle access log is required for this lifecycle; deployments that need request-level forensic logs can add an HTTP access/audit layer independently of the rate-limit correctness surface. --- diff --git a/main_sqs.go b/main_sqs.go index 85b9fd488..fc2a43fcd 100644 --- a/main_sqs.go +++ b/main_sqs.go @@ -46,6 +46,10 @@ func startSQSServer( closeSQSListenerOnError(sqsL, sqsAddr) return nil, err } + var throttleObserver adapter.SQSThrottleObserver + if o, ok := partitionObserver.(adapter.SQSThrottleObserver); ok { + throttleObserver = o + } sqsServer := adapter.NewSQSServer( sqsL, shardStore, @@ -55,6 +59,7 @@ func startSQSServer( adapter.WithSQSStaticCredentials(staticCreds), adapter.WithSQSPartitionResolver(partitionResolver), adapter.WithSQSPartitionObserver(partitionObserver), + adapter.WithSQSThrottleObserver(throttleObserver), ) // Two-goroutine shutdown pattern mirrors startS3Server: one goroutine waits // on either ctx.Done() or Run completion to call Stop, the other runs the diff --git a/monitoring/sqs.go b/monitoring/sqs.go index aa1d0a2b4..1f7539a62 100644 --- a/monitoring/sqs.go +++ b/monitoring/sqs.go @@ -16,6 +16,10 @@ const ( SQSPartitionActionReceive = "receive" SQSPartitionActionDelete = "delete" + SQSThrottleActionSend = "send" + SQSThrottleActionReceive = "receive" + SQSThrottleActionDefault = "default" + // sqsMaxTrackedQueues caps the number of distinct queue names // the metrics layer will emit a per-(queue, partition, action) // series for. Any queue beyond this cap collapses to the @@ -45,6 +49,13 @@ type SQSPartitionObserver interface { ObservePartitionMessage(queue string, partition uint32, action string) } +// SQSThrottleObserver records per-queue throttle decisions. The SQS +// adapter calls it after a configured token-bucket charge so operators +// can observe both rejected requests and the live token balance. +type SQSThrottleObserver interface { + ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) +} + // SQSDepthSource is the contract a per-tick queue-depth source must // satisfy. Implemented by *adapter.SQSServer; SQSObserver.Start // calls SnapshotQueueDepths on every interval and writes the @@ -120,10 +131,13 @@ type SQSQueueDepth struct { type SQSMetrics struct { partitionMessages *prometheus.CounterVec queueDepth *prometheus.GaugeVec + throttledRequests *prometheus.CounterVec + throttleTokens *prometheus.GaugeVec - mu sync.Mutex - trackedCounterQueues map[string]struct{} - trackedDepthQueues map[string]struct{} + mu sync.Mutex + trackedCounterQueues map[string]struct{} + trackedDepthQueues map[string]struct{} + trackedThrottleGaugeQueues map[string]struct{} // overflowDepthQueues is the set of queue names whose depth // gauge is currently collapsed onto the shared sqsQueueOverflow // label. Tracked separately from trackedDepthQueues (which @@ -138,7 +152,8 @@ type SQSMetrics struct { // produce phantom data — it correctly reflects the cumulative // count of all overflow operations, even from queues that no // longer exist. - overflowDepthQueues map[string]struct{} + overflowDepthQueues map[string]struct{} + overflowThrottleGaugeQueues map[string]struct{} } // SQS depth gauge state-label values. Stable so dashboards / alerts @@ -165,12 +180,30 @@ func newSQSMetrics(registerer prometheus.Registerer) *SQSMetrics { }, []string{"queue", "state"}, ), - trackedCounterQueues: map[string]struct{}{}, - trackedDepthQueues: map[string]struct{}{}, - overflowDepthQueues: map[string]struct{}{}, + throttledRequests: prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "elastickv_sqs_throttled_requests_total", + Help: "Total SQS requests rejected by per-queue throttling, labelled by queue and effective throttle bucket action.", + }, + []string{"queue", "action"}, + ), + throttleTokens: prometheus.NewGaugeVec( + prometheus.GaugeOpts{ + Name: "elastickv_sqs_throttle_tokens_remaining", + Help: "Remaining tokens after the most recent SQS per-queue throttle decision, labelled by queue and effective throttle bucket action.", + }, + []string{"queue", "action"}, + ), + trackedCounterQueues: map[string]struct{}{}, + trackedDepthQueues: map[string]struct{}{}, + trackedThrottleGaugeQueues: map[string]struct{}{}, + overflowDepthQueues: map[string]struct{}{}, + overflowThrottleGaugeQueues: map[string]struct{}{}, } registerer.MustRegister(m.partitionMessages) registerer.MustRegister(m.queueDepth) + registerer.MustRegister(m.throttledRequests) + registerer.MustRegister(m.throttleTokens) return m } @@ -220,6 +253,27 @@ func (m *SQSMetrics) ObserveQueueDepth(queue string, visible, notVisible, delaye m.queueDepth.WithLabelValues(queueLabel, sqsQueueStateDelayed).Set(float64(max(int64(0), delayed))) } +// ObserveThrottleDecision implements SQSThrottleObserver. It updates +// the current token-balance gauge for every configured bucket decision +// and increments the rejected-request counter only when throttled=true. +func (m *SQSMetrics) ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) { + if m == nil || queue == "" { + return + } + if !sqsValidThrottleAction(action) { + return + } + if tokensRemaining < 0 { + tokensRemaining = 0 + } + queueCounterLabel := m.admitForCounterBudget(queue) + if throttled { + m.throttledRequests.WithLabelValues(queueCounterLabel, action).Inc() + } + queueGaugeLabel := m.admitForThrottleGaugeBudget(queue) + m.throttleTokens.WithLabelValues(queueGaugeLabel, action).Set(tokensRemaining) +} + // ForgetQueue drops the three gauge series for a queue and frees // its slot in the depth-side cardinality budget so a long-running // deployment that regularly creates and deletes queues (CI @@ -270,6 +324,19 @@ func (m *SQSMetrics) ForgetQueue(queue string) { return } m.mu.Lock() + depthForget := m.forgetDepthQueueLocked(queue) + throttleForget := m.forgetThrottleQueueLocked(queue) + m.mu.Unlock() + m.dropForgottenDepthQueue(queue, depthForget) + m.dropForgottenThrottleQueue(queue, throttleForget) +} + +type sqsGaugeForget struct { + tracked bool + overflowSetEmpty bool +} + +func (m *SQSMetrics) forgetDepthQueueLocked(queue string) sqsGaugeForget { _, tracked := m.trackedDepthQueues[queue] if tracked { delete(m.trackedDepthQueues, queue) @@ -282,12 +349,26 @@ func (m *SQSMetrics) ForgetQueue(queue string) { if overflow { delete(m.overflowDepthQueues, queue) } - overflowSetEmpty := overflow && len(m.overflowDepthQueues) == 0 - m.mu.Unlock() - if tracked { + return sqsGaugeForget{tracked: tracked, overflowSetEmpty: overflow && len(m.overflowDepthQueues) == 0} +} + +func (m *SQSMetrics) forgetThrottleQueueLocked(queue string) sqsGaugeForget { + _, throttleTracked := m.trackedThrottleGaugeQueues[queue] + if throttleTracked { + delete(m.trackedThrottleGaugeQueues, queue) + } + _, throttleOverflow := m.overflowThrottleGaugeQueues[queue] + if throttleOverflow { + delete(m.overflowThrottleGaugeQueues, queue) + } + return sqsGaugeForget{tracked: throttleTracked, overflowSetEmpty: throttleOverflow && len(m.overflowThrottleGaugeQueues) == 0} +} + +func (m *SQSMetrics) dropForgottenDepthQueue(queue string, forget sqsGaugeForget) { + if forget.tracked { m.dropGaugeStatesFor(queue) } - if overflowSetEmpty { + if forget.overflowSetEmpty { // Last overflow queue forgotten — drop the shared _other // series so dashboards stop showing phantom backlog for // queues that no longer exist. Safe because no remaining @@ -297,6 +378,15 @@ func (m *SQSMetrics) ForgetQueue(queue string) { } } +func (m *SQSMetrics) dropForgottenThrottleQueue(queue string, forget sqsGaugeForget) { + if forget.tracked { + m.dropThrottleGaugeActionsFor(queue) + } + if forget.overflowSetEmpty { + m.dropThrottleGaugeActionsFor(sqsQueueOverflow) + } +} + // admitForCounterBudget returns the canonical label for queue: the // real name when the counter budget has room, sqsQueueOverflow once // it is saturated. The counter budget never shrinks (counter series @@ -360,6 +450,34 @@ func (m *SQSMetrics) admitForDepthBudget(queue string) string { return queue } +// admitForThrottleGaugeBudget mirrors admitForDepthBudget for the +// event-driven throttle-token gauge. It is separate from queue depth +// because a queue can emit throttle decisions without being present in +// the next depth scrape yet. +func (m *SQSMetrics) admitForThrottleGaugeBudget(queue string) string { + m.mu.Lock() + if _, ok := m.trackedThrottleGaugeQueues[queue]; ok { + m.mu.Unlock() + return queue + } + if len(m.trackedThrottleGaugeQueues) >= sqsMaxTrackedQueues { + m.overflowThrottleGaugeQueues[queue] = struct{}{} + m.mu.Unlock() + return sqsQueueOverflow + } + m.trackedThrottleGaugeQueues[queue] = struct{}{} + _, wasOverflow := m.overflowThrottleGaugeQueues[queue] + if wasOverflow { + delete(m.overflowThrottleGaugeQueues, queue) + } + overflowSetEmpty := wasOverflow && len(m.overflowThrottleGaugeQueues) == 0 + m.mu.Unlock() + if overflowSetEmpty { + m.dropThrottleGaugeActionsFor(sqsQueueOverflow) + } + return queue +} + // dropGaugeStatesFor removes the three state-labelled series for // label (a real queue name or sqsQueueOverflow) from the depth // gauge. Single-line caller-readability wrapper — prometheus @@ -371,6 +489,12 @@ func (m *SQSMetrics) dropGaugeStatesFor(label string) { m.queueDepth.DeleteLabelValues(label, sqsQueueStateDelayed) } +func (m *SQSMetrics) dropThrottleGaugeActionsFor(label string) { + m.throttleTokens.DeleteLabelValues(label, SQSThrottleActionSend) + m.throttleTokens.DeleteLabelValues(label, SQSThrottleActionReceive) + m.throttleTokens.DeleteLabelValues(label, SQSThrottleActionDefault) +} + // sqsValidPartitionAction returns true iff action is one of the // stable label values. Keeps a typo at the call site (e.g. // "Send" vs "send") from polluting the metric. @@ -382,6 +506,14 @@ func sqsValidPartitionAction(action string) bool { return false } +func sqsValidThrottleAction(action string) bool { + switch action { + case SQSThrottleActionSend, SQSThrottleActionReceive, SQSThrottleActionDefault: + return true + } + return false +} + // sqsDepthObserveInterval is the default tick cadence for // SQSObserver. 30 s mirrors sqsReaperInterval — dashboards are // rarely refreshed faster, and tighter ticks just add catalog-scan diff --git a/monitoring/sqs_test.go b/monitoring/sqs_test.go index 65be82202..ea7e12a53 100644 --- a/monitoring/sqs_test.go +++ b/monitoring/sqs_test.go @@ -83,6 +83,7 @@ func TestSQSMetrics_NilReceiverIsSafe(t *testing.T) { var m *SQSMetrics require.NotPanics(t, func() { m.ObservePartitionMessage("q.fifo", 0, SQSPartitionActionSend) + m.ObserveThrottleDecision("q.fifo", SQSThrottleActionSend, 1, true) }) } @@ -127,19 +128,66 @@ func TestSQSMetrics_RegistryWiring(t *testing.T) { require.NotNil(t, obs) obs.ObservePartitionMessage("q.fifo", 3, SQSPartitionActionReceive) + throttleObs, ok := obs.(SQSThrottleObserver) + require.True(t, ok) + throttleObs.ObserveThrottleDecision("q.fifo", SQSThrottleActionSend, 4, true) mfs, err := r.Gatherer().Gather() require.NoError(t, err) - var found bool + var foundPartition bool + var foundThrottleCounter bool + var foundThrottleGauge bool for _, mf := range mfs { - if !strings.HasPrefix(mf.GetName(), "elastickv_sqs_partition_messages_total") { + switch { + case strings.HasPrefix(mf.GetName(), "elastickv_sqs_partition_messages_total"): + foundPartition = true + require.Len(t, mf.GetMetric(), 1) + case strings.HasPrefix(mf.GetName(), "elastickv_sqs_throttled_requests_total"): + foundThrottleCounter = true + require.Len(t, mf.GetMetric(), 1) + case strings.HasPrefix(mf.GetName(), "elastickv_sqs_throttle_tokens_remaining"): + foundThrottleGauge = true + require.Len(t, mf.GetMetric(), 1) + default: continue } - found = true - require.Len(t, mf.GetMetric(), 1) } - require.True(t, found, + require.True(t, foundPartition, "elastickv_sqs_partition_messages_total must be registered on the public Registry") + require.True(t, foundThrottleCounter, + "elastickv_sqs_throttled_requests_total must be registered on the public Registry") + require.True(t, foundThrottleGauge, + "elastickv_sqs_throttle_tokens_remaining must be registered on the public Registry") +} + +func TestSQSMetrics_ObserveThrottleDecision_EmitsCounterAndGauge(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 9, false) + require.InDelta(t, 9.0, testutil.ToFloat64(m.throttleTokens.WithLabelValues("orders.fifo", SQSThrottleActionSend)), 0.001) + require.InDelta(t, 0.0, testutil.ToFloat64(m.throttledRequests.WithLabelValues("orders.fifo", SQSThrottleActionSend)), 0.001) + + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 0, true) + require.InDelta(t, 0.0, testutil.ToFloat64(m.throttleTokens.WithLabelValues("orders.fifo", SQSThrottleActionSend)), 0.001) + require.InDelta(t, 1.0, testutil.ToFloat64(m.throttledRequests.WithLabelValues("orders.fifo", SQSThrottleActionSend)), 0.001) +} + +func TestSQSMetrics_ObserveThrottleDecision_DropsInvalidLabels(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + m.ObserveThrottleDecision("", SQSThrottleActionSend, 1, true) + m.ObserveThrottleDecision("orders.fifo", "Send", 1, true) + + counterCount, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttled_requests_total") + require.NoError(t, err) + require.Equal(t, 0, counterCount) + gaugeCount, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 0, gaugeCount) } // TestSQSMetrics_ObserveQueueDepth_EmitsThreeStates pins that one @@ -201,8 +249,10 @@ func TestSQSMetrics_ForgetQueue_DropsThreeSeries(t *testing.T) { m := newSQSMetrics(reg) m.ObserveQueueDepth("orders.fifo", 3, 0, 0) + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 4, true) m.ObservePartitionMessage("orders.fifo", 0, SQSPartitionActionSend) require.Len(t, m.trackedDepthQueues, 1, "queue must have been admitted to the depth budget on ObserveQueueDepth") + require.Len(t, m.trackedThrottleGaugeQueues, 1, "queue must have been admitted to the throttle gauge budget on ObserveThrottleDecision") require.Len(t, m.trackedCounterQueues, 1, "queue must have been admitted to the counter budget on ObservePartitionMessage") m.ForgetQueue("orders.fifo") @@ -210,10 +260,16 @@ func TestSQSMetrics_ForgetQueue_DropsThreeSeries(t *testing.T) { count, err := testutil.GatherAndCount(reg, "elastickv_sqs_queue_messages") require.NoError(t, err) require.Equal(t, 0, count, "ForgetQueue must drop every state series for the queue") + throttleGaugeCount, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 0, throttleGaugeCount, "ForgetQueue must drop every throttle-token gauge for the queue") require.Empty(t, m.trackedDepthQueues, "ForgetQueue must free the depth-side cardinality slot") + require.Empty(t, m.trackedThrottleGaugeQueues, "ForgetQueue must free the throttle-gauge cardinality slot") require.Len(t, m.trackedCounterQueues, 1, "ForgetQueue must NOT free the counter-side slot (counters are cumulative)") require.InDelta(t, 1.0, testutil.ToFloat64(m.partitionMessages.WithLabelValues("orders.fifo", "0", SQSPartitionActionSend)), 0.001, "counter must survive ForgetQueue (cumulative metric)") + require.InDelta(t, 1.0, testutil.ToFloat64(m.throttledRequests.WithLabelValues("orders.fifo", SQSThrottleActionSend)), 0.001, + "throttled-request counter must survive ForgetQueue (cumulative metric)") // Observe again post-forget: the new series must be keyed on // the real name (slot was freed), NOT on the _other overflow From 34c26baf77708cfb12219685b4497aa30ad122e1 Mon Sep 17 00:00:00 2001 From: bootjp Date: Tue, 7 Jul 2026 18:21:07 +0900 Subject: [PATCH 07/16] Tighten SQS throttle metric cleanup --- monitoring/sqs.go | 37 +++++++++++++++++++++++++++++++------ monitoring/sqs_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/monitoring/sqs.go b/monitoring/sqs.go index 1f7539a62..81e4f552e 100644 --- a/monitoring/sqs.go +++ b/monitoring/sqs.go @@ -266,8 +266,8 @@ func (m *SQSMetrics) ObserveThrottleDecision(queue string, action string, tokens if tokensRemaining < 0 { tokensRemaining = 0 } - queueCounterLabel := m.admitForCounterBudget(queue) if throttled { + queueCounterLabel := m.admitForCounterBudget(queue) m.throttledRequests.WithLabelValues(queueCounterLabel, action).Inc() } queueGaugeLabel := m.admitForThrottleGaugeBudget(queue) @@ -478,6 +478,22 @@ func (m *SQSMetrics) admitForThrottleGaugeBudget(queue string) string { return queue } +func (m *SQSMetrics) snapshotThrottleGaugeQueues() []string { + if m == nil { + return nil + } + m.mu.Lock() + defer m.mu.Unlock() + queues := make([]string, 0, len(m.trackedThrottleGaugeQueues)+len(m.overflowThrottleGaugeQueues)) + for queue := range m.trackedThrottleGaugeQueues { + queues = append(queues, queue) + } + for queue := range m.overflowThrottleGaugeQueues { + queues = append(queues, queue) + } + return queues +} + // dropGaugeStatesFor removes the three state-labelled series for // label (a real queue name or sqsQueueOverflow) from the depth // gauge. Single-line caller-readability wrapper — prometheus @@ -612,11 +628,7 @@ func (o *SQSObserver) observeOnce(ctx context.Context, source SQSDepthSource) { // tick. Reclaiming first lets phase 2's admissions reuse the // freed slots immediately. o.mu.Lock() - for prev := range o.lastSeen { - if _, ok := current[prev]; !ok { - o.metrics.ForgetQueue(prev) - } - } + o.forgetMissingQueuesLocked(current) o.lastSeen = current o.mu.Unlock() // Phase 2: emit gauges for the current tick. Slots freed in @@ -628,3 +640,16 @@ func (o *SQSObserver) observeOnce(ctx context.Context, source SQSDepthSource) { o.metrics.ObserveQueueDepth(snap.Queue, snap.Visible, snap.NotVisible, snap.Delayed) } } + +func (o *SQSObserver) forgetMissingQueuesLocked(current map[string]struct{}) { + for prev := range o.lastSeen { + if _, ok := current[prev]; !ok { + o.metrics.ForgetQueue(prev) + } + } + for _, prev := range o.metrics.snapshotThrottleGaugeQueues() { + if _, ok := current[prev]; !ok { + o.metrics.ForgetQueue(prev) + } + } +} diff --git a/monitoring/sqs_test.go b/monitoring/sqs_test.go index ea7e12a53..fc62bcaa4 100644 --- a/monitoring/sqs_test.go +++ b/monitoring/sqs_test.go @@ -174,6 +174,23 @@ func TestSQSMetrics_ObserveThrottleDecision_EmitsCounterAndGauge(t *testing.T) { require.InDelta(t, 1.0, testutil.ToFloat64(m.throttledRequests.WithLabelValues("orders.fifo", SQSThrottleActionSend)), 0.001) } +func TestSQSMetrics_ObserveThrottleDecision_AllowedDoesNotConsumeCounterBudget(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + for i := 0; i < sqsMaxTrackedQueues; i++ { + m.ObserveThrottleDecision("allow-"+strconv.Itoa(i)+".fifo", SQSThrottleActionSend, 1, false) + } + require.Empty(t, m.trackedCounterQueues, "allowed-only throttle decisions must not consume counter label budget") + + m.ObserveThrottleDecision("blocked.fifo", SQSThrottleActionSend, 0, true) + require.InDelta(t, 1.0, + testutil.ToFloat64(m.throttledRequests.WithLabelValues("blocked.fifo", SQSThrottleActionSend)), + 0.001, + "first actual reject must keep its real queue label instead of overflowing after allowed-only decisions") +} + func TestSQSMetrics_ObserveThrottleDecision_DropsInvalidLabels(t *testing.T) { t.Parallel() reg := prometheus.NewRegistry() @@ -661,6 +678,27 @@ func TestSQSObserver_ObserveOnce_LeaderStepDownClearsAll(t *testing.T) { require.Equal(t, 0, count, "tick 2 (leader step-down): all gauges cleared") } +func TestSQSObserver_ObserveOnce_ForgetsThrottleOnlyQueues(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + obs := newSQSObserver(m) + + m.ObserveThrottleDecision("short-lived.fifo", SQSThrottleActionSend, 4, false) + count, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 1, count, "sanity: throttle-only queue emitted a token gauge before the depth tick") + + obs.ObserveOnce(&fakeDepthSource{ + ticks: []fakeDepthTick{{snaps: nil, ok: true}}, + }) + + count, err = testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 0, count, "successful depth tick must clear throttle-only queues absent from the snapshot") + require.Empty(t, m.trackedThrottleGaugeQueues, "throttle gauge budget slot must be reclaimed for the short-lived queue") +} + // TestSQSObserver_ObserveOnce_TransientScanErrorPreservesGauges // pins the P2 fix from PR #743 r6: when the source returns // ok=false (transient catalog-scan failure on the leader, ctx From 5ed967bd4df42b03ac34fb7c9676c4f8b41634a8 Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 8 Jul 2026 16:21:37 +0900 Subject: [PATCH 08/16] monitoring: isolate SQS throttle metrics --- adapter/sqs.go | 44 +++- adapter/sqs_throttle.go | 2 +- adapter/sqs_throttle_test.go | 33 ++- ...26_implemented_sqs_per_queue_throttling.md | 15 +- monitoring/sqs.go | 202 +++++++++++++----- monitoring/sqs_test.go | 158 ++++++++++++++ 6 files changed, 390 insertions(+), 64 deletions(-) diff --git a/adapter/sqs.go b/adapter/sqs.go index 704688519..645c0bbaf 100644 --- a/adapter/sqs.go +++ b/adapter/sqs.go @@ -228,20 +228,30 @@ type SQSPartitionObserver interface { // does not import monitoring at the package boundary. type SQSThrottleObserver interface { ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) + ForgetThrottleAction(queue string, action string) } -// SQSPartitionAction* mirror the action label values from -// monitoring.SQSPartitionAction*. Re-declared so adapter call -// sites do not need a monitoring import; the observer interface -// validates the value at runtime so a drift between these -// constants and the monitoring side surfaces as a dropped -// observation rather than a wedge. +// SQS metric action labels mirror the values from monitoring/sqs.go. +// Re-declared so adapter call sites do not need a monitoring import; the +// observer interface validates the value at runtime so a drift between these +// constants and the monitoring side surfaces as a dropped observation rather +// than a wedge. const ( SQSPartitionActionSend = "send" SQSPartitionActionReceive = "receive" SQSPartitionActionDelete = "delete" + + SQSThrottleActionSend = "send" + SQSThrottleActionReceive = "receive" + SQSThrottleActionDefault = "default" ) +var sqsThrottleMetricActions = [...]string{ + SQSThrottleActionSend, + SQSThrottleActionReceive, + SQSThrottleActionDefault, +} + // WithSQSLeaderMap configures the Raft-address-to-SQS-address mapping used to // forward requests from followers to the current leader. Format mirrors // WithDynamoDBLeaderMap / WithS3LeaderMap. @@ -284,10 +294,13 @@ func (s *SQSServer) observePartitionMessage(queue string, partition uint32, acti s.partitionObserver.ObservePartitionMessage(queue, partition, action) } -func (s *SQSServer) observeThrottleDecision(queue string, outcome chargeOutcome) { +func (s *SQSServer) observeThrottleDecision(queue string, throttle *sqsQueueThrottle, outcome chargeOutcome) { if s == nil || s.throttleObserver == nil { return } + for _, action := range disabledThrottleMetricActions(throttle) { + s.throttleObserver.ForgetThrottleAction(queue, action) + } if !outcome.bucketPresent && outcome.allowed { return } @@ -299,6 +312,23 @@ func (s *SQSServer) observeThrottleDecision(queue string, outcome chargeOutcome) ) } +func disabledThrottleMetricActions(throttle *sqsQueueThrottle) []string { + disabled := make([]string, 0, len(sqsThrottleMetricActions)) + if throttle == nil || throttle.IsEmpty() { + return append(disabled, sqsThrottleMetricActions[:]...) + } + if throttle.SendCapacity == 0 { + disabled = append(disabled, SQSThrottleActionSend) + } + if throttle.RecvCapacity == 0 { + disabled = append(disabled, SQSThrottleActionReceive) + } + if throttle.DefaultCapacity == 0 { + disabled = append(disabled, SQSThrottleActionDefault) + } + return disabled +} + // WithSQSPartitionResolver installs the cluster's partition // resolver on the SQS server so the CreateQueue capability gate // (validateHTFIFOCapability) can verify routing coverage before diff --git a/adapter/sqs_throttle.go b/adapter/sqs_throttle.go index c8ed87039..19750bcd7 100644 --- a/adapter/sqs_throttle.go +++ b/adapter/sqs_throttle.go @@ -633,7 +633,7 @@ func (s *SQSServer) chargeQueueWithThrottle(w http.ResponseWriter, queueName, ac return true } outcome := s.throttle.charge(throttle, queueName, action, incarnation, count) - s.observeThrottleDecision(queueName, outcome) + s.observeThrottleDecision(queueName, throttle, outcome) if outcome.allowed { return true } diff --git a/adapter/sqs_throttle_test.go b/adapter/sqs_throttle_test.go index cd7107c66..ecfad627c 100644 --- a/adapter/sqs_throttle_test.go +++ b/adapter/sqs_throttle_test.go @@ -18,8 +18,14 @@ type throttleObserveReport struct { throttled bool } +type throttleForgetReport struct { + queue string + action string +} + type recordingSQSThrottleObserver struct { - reports []throttleObserveReport + reports []throttleObserveReport + forgotten []throttleForgetReport } func (r *recordingSQSThrottleObserver) ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) { @@ -31,6 +37,10 @@ func (r *recordingSQSThrottleObserver) ObserveThrottleDecision(queue string, act }) } +func (r *recordingSQSThrottleObserver) ForgetThrottleAction(queue string, action string) { + r.forgotten = append(r.forgotten, throttleForgetReport{queue: queue, action: action}) +} + // TestBucketStore_DefaultOff_ShortCircuit pins the contract that a // nil throttle config never allocates a bucket and never rejects. // This is the hot path for unconfigured queues — every nil-check that @@ -68,6 +78,27 @@ func TestSQSServer_ChargeQueueWithThrottleObservesMetrics(t *testing.T) { require.InDelta(t, 0.0, last.tokensRemaining, 0.001) } +func TestSQSServer_ChargeQueueWithThrottleForgetsDisabledMetrics(t *testing.T) { + t.Parallel() + observer := &recordingSQSThrottleObserver{} + srv := NewSQSServer(nil, nil, nil, WithSQSThrottleObserver(observer)) + cfg := &sqsQueueThrottle{SendCapacity: 10, SendRefillPerSecond: 1} + + rec := httptest.NewRecorder() + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1)) + require.Contains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionReceive}) + require.Contains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionDefault}) + + observer.forgotten = nil + rec = httptest.NewRecorder() + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, nil, 1)) + require.ElementsMatch(t, []throttleForgetReport{ + {queue: "orders.fifo", action: SQSThrottleActionSend}, + {queue: "orders.fifo", action: SQSThrottleActionReceive}, + {queue: "orders.fifo", action: SQSThrottleActionDefault}, + }, observer.forgotten) +} + // TestBucketStore_Empty_ShortCircuit covers the post-validator // canonicalisation path: an all-zero sqsQueueThrottle is equivalent // to nil. Without this branch, a queue whose operator wrote diff --git a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md index b6dc278ac..2595236a5 100644 --- a/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md +++ b/docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md @@ -73,7 +73,7 @@ The check sits **between SigV4 authorisation and the existing handler dispatch** A `bucketStore` instance hangs off `*SQSServer`. Internally: ```go -// adapter/sqs_throttle.go (new in implementation PR) +// adapter/sqs_throttle.go type bucketStore struct { // sync.Map rather than a single mu+map so the hot SendMessage / // ReceiveMessage path does not contend on a process-wide lock. @@ -213,12 +213,13 @@ The minimum-1 floor matches `Retry-After`'s integer-second granularity (HTTP/1.1 | File | Change | |---|---| -| `adapter/sqs_throttle.go` (new) | `bucketStore`, `tokenBucket`, charging helper. ~250 lines. | +| `adapter/sqs_throttle.go` | `bucketStore`, `tokenBucket`, charging helper. | | `adapter/sqs_catalog.go` | Add `Throttle` field to `sqsQueueMeta`. Extend `applyAttributes` with the new `Throttle*` attribute names. Render the four Throttle fields in `queueMetaToAttributes` so `GetQueueAttributes("All")` surfaces them. | | `adapter/sqs.go` | After `authorizeSQSRequest`, call `bucketStore.charge(queueName, action, count)`. On reject, write the `Throttling` envelope and return. Also forwards each configured bucket decision to the SQS throttle metrics observer. | -| `adapter/sqs_throttle_test.go` (new) | Unit tests for bucket math (edge cases: idle drift, burst, partial refill, batch over-charge, default-off). ~300 lines. | -| `adapter/sqs_throttle_integration_test.go` (new) | End-to-end: configure a queue with low limits, send N messages back-to-back, confirm the (N+1)th gets `Throttling` with `Retry-After`. ~150 lines. | +| `adapter/sqs_throttle_test.go` | Unit tests for bucket math, observer emission, disabled-bucket metric cleanup, idle drift, burst, partial refill, batch over-charge, and default-off. | +| `adapter/sqs_throttle_integration_test.go` | End-to-end: configure a queue with low limits, send N messages back-to-back, confirm the (N+1)th gets `Throttling` with `Retry-After`. | | `monitoring/sqs.go`, `monitoring/registry.go` | Register the `elastickv_sqs_throttled_requests_total{queue, action}` counter and `elastickv_sqs_throttle_tokens_remaining{queue, action}` gauge. Queue-label cardinality uses the same capped `_other` fallback as the existing SQS metrics. | +| `monitoring/sqs_test.go` | Pins public registration of both throttle metrics, independent queue-label budgets for throttle counters vs partition counters, short-lived queue cleanup, disabled action cleanup, and action-aware `_other` gauge cleanup. | | `docs/design/2026_04_24_proposed_sqs_compatible_adapter.md` §14 | Phase 3 per-queue throttling item marked landed with a link to this design. | ### 4.2 OCC interaction @@ -325,9 +326,9 @@ Every `charge` proposes a bucket update through the FSM. **Rejected**: an extra | Phase | Content | |---|---| -| 1 | Doc lands (this PR). No code yet. Operators have time to comment. | -| 2 | Implementation PR per §4.1. Default-off; existing queues unaffected. | -| 3 | Operators opt in per queue via `SetQueueAttributes`. Watch caller-visible `Throttling` responses for false positives. | +| 1 | Default-off implementation and lifecycle documentation have landed; existing queues remain unthrottled until configured. | +| 2 | Operators opt in per queue via `SetQueueAttributes`. Watch caller-visible `Throttling` responses and `Retry-After` for false positives. | +| 3 | Prometheus exports `elastickv_sqs_throttled_requests_total{queue, action}` and `elastickv_sqs_throttle_tokens_remaining{queue, action}` with bounded queue-label cardinality. | | 4 | Parent SQS adapter roadmap marks Phase 3.C as landed with a link to this design. | --- diff --git a/monitoring/sqs.go b/monitoring/sqs.go index 81e4f552e..1a97e5435 100644 --- a/monitoring/sqs.go +++ b/monitoring/sqs.go @@ -54,6 +54,7 @@ type SQSPartitionObserver interface { // can observe both rejected requests and the live token balance. type SQSThrottleObserver interface { ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) + ForgetThrottleAction(queue string, action string) } // SQSDepthSource is the contract a per-tick queue-depth source must @@ -103,11 +104,11 @@ type SQSQueueDepth struct { // SQSMetrics owns the Prometheus collectors for the SQS adapter. // Mirrors DynamoDBMetrics' shape: per-Registry instance, label- -// cardinality-bounded by sqsMaxTrackedQueues, and split between -// counters (HT-FIFO partition activity) and gauges (queue depth). +// cardinality-bounded by sqsMaxTrackedQueues, and split by metric +// family so unrelated SQS signals cannot consume each other's +// queue-label budget. // -// The cardinality budget is split into two independent maps because -// the two metrics have different deletion semantics: +// Counter and gauge families have different deletion semantics: // // - partitionMessages is a CounterVec. Counters are cumulative; // deleting a series throws away its observed-since-process-start @@ -115,14 +116,24 @@ type SQSQueueDepth struct { // budget therefore only ever grows — once a queue is admitted to // trackedCounterQueues it stays admitted, and ForgetQueue does // NOT touch this map. +// - throttledRequests is also cumulative, but uses its own +// trackedThrottleCounterQueues admission map. A burst of +// rejected non-partitioned queues must not force later HT-FIFO +// partition counters into _other, and partition traffic must not +// hide throttle rejects. // - queueDepth is a GaugeVec. Gauges have no cumulative state, so // DeleteLabelValues is safe (and necessary, otherwise a deleted // queue keeps reporting a frozen backlog on the dashboard). // ForgetQueue both removes the gauge series and frees the // queue's slot in trackedDepthQueues so a churn-heavy deployment // can reuse the budget. +// - throttleTokens is also a GaugeVec, but decisions are event- +// driven and action-labelled. It tracks the active actions per +// queue so disabling one bucket can drop just that action's +// gauge while preserving other configured actions for the same +// queue. // -// Sharing one map across the two metrics regresses the counter cap: +// Sharing one map across counters and gauges regresses the counter cap: // ForgetQueue would free a slot, a new queue would be admitted, and // the previous queue's counter series would still occupy a real-name // label in Prometheus — letting cardinality grow without bound under @@ -134,10 +145,11 @@ type SQSMetrics struct { throttledRequests *prometheus.CounterVec throttleTokens *prometheus.GaugeVec - mu sync.Mutex - trackedCounterQueues map[string]struct{} - trackedDepthQueues map[string]struct{} - trackedThrottleGaugeQueues map[string]struct{} + mu sync.Mutex + trackedCounterQueues map[string]struct{} + trackedThrottleCounterQueues map[string]struct{} + trackedDepthQueues map[string]struct{} + trackedThrottleGaugeQueues map[string]map[string]struct{} // overflowDepthQueues is the set of queue names whose depth // gauge is currently collapsed onto the shared sqsQueueOverflow // label. Tracked separately from trackedDepthQueues (which @@ -153,7 +165,7 @@ type SQSMetrics struct { // count of all overflow operations, even from queues that no // longer exist. overflowDepthQueues map[string]struct{} - overflowThrottleGaugeQueues map[string]struct{} + overflowThrottleGaugeQueues map[string]map[string]struct{} } // SQS depth gauge state-label values. Stable so dashboards / alerts @@ -194,11 +206,12 @@ func newSQSMetrics(registerer prometheus.Registerer) *SQSMetrics { }, []string{"queue", "action"}, ), - trackedCounterQueues: map[string]struct{}{}, - trackedDepthQueues: map[string]struct{}{}, - trackedThrottleGaugeQueues: map[string]struct{}{}, - overflowDepthQueues: map[string]struct{}{}, - overflowThrottleGaugeQueues: map[string]struct{}{}, + trackedCounterQueues: map[string]struct{}{}, + trackedThrottleCounterQueues: map[string]struct{}{}, + trackedDepthQueues: map[string]struct{}{}, + trackedThrottleGaugeQueues: map[string]map[string]struct{}{}, + overflowDepthQueues: map[string]struct{}{}, + overflowThrottleGaugeQueues: map[string]map[string]struct{}{}, } registerer.MustRegister(m.partitionMessages) registerer.MustRegister(m.queueDepth) @@ -267,13 +280,37 @@ func (m *SQSMetrics) ObserveThrottleDecision(queue string, action string, tokens tokensRemaining = 0 } if throttled { - queueCounterLabel := m.admitForCounterBudget(queue) + queueCounterLabel := m.admitForThrottleCounterBudget(queue) m.throttledRequests.WithLabelValues(queueCounterLabel, action).Inc() } - queueGaugeLabel := m.admitForThrottleGaugeBudget(queue) + queueGaugeLabel, overflowActionsToDrop := m.admitForThrottleGaugeBudget(queue, action) + for _, action := range overflowActionsToDrop { + m.dropThrottleGaugeActionFor(sqsQueueOverflow, action) + } m.throttleTokens.WithLabelValues(queueGaugeLabel, action).Set(tokensRemaining) } +// ForgetThrottleAction removes the token-balance gauge for one queue/action +// whose throttle bucket is no longer configured. Throttled-request counters +// are cumulative and intentionally survive. +func (m *SQSMetrics) ForgetThrottleAction(queue string, action string) { + if m == nil || queue == "" { + return + } + if !sqsValidThrottleAction(action) { + return + } + m.mu.Lock() + forget := m.forgetThrottleActionLocked(queue, action) + m.mu.Unlock() + if forget.tracked { + m.dropThrottleGaugeActionFor(queue, action) + } + if forget.overflowSetEmpty { + m.dropThrottleGaugeActionFor(sqsQueueOverflow, action) + } +} + // ForgetQueue drops the three gauge series for a queue and frees // its slot in the depth-side cardinality budget so a long-running // deployment that regularly creates and deletes queues (CI @@ -336,6 +373,16 @@ type sqsGaugeForget struct { overflowSetEmpty bool } +type sqsThrottleGaugeForget struct { + tracked bool + overflowActions []string +} + +type sqsThrottleActionForget struct { + tracked bool + overflowSetEmpty bool +} + func (m *SQSMetrics) forgetDepthQueueLocked(queue string) sqsGaugeForget { _, tracked := m.trackedDepthQueues[queue] if tracked { @@ -352,16 +399,53 @@ func (m *SQSMetrics) forgetDepthQueueLocked(queue string) sqsGaugeForget { return sqsGaugeForget{tracked: tracked, overflowSetEmpty: overflow && len(m.overflowDepthQueues) == 0} } -func (m *SQSMetrics) forgetThrottleQueueLocked(queue string) sqsGaugeForget { +func (m *SQSMetrics) forgetThrottleQueueLocked(queue string) sqsThrottleGaugeForget { _, throttleTracked := m.trackedThrottleGaugeQueues[queue] if throttleTracked { delete(m.trackedThrottleGaugeQueues, queue) } - _, throttleOverflow := m.overflowThrottleGaugeQueues[queue] - if throttleOverflow { - delete(m.overflowThrottleGaugeQueues, queue) + return sqsThrottleGaugeForget{ + tracked: throttleTracked, + overflowActions: m.removeThrottleOverflowQueueLocked(queue), } - return sqsGaugeForget{tracked: throttleTracked, overflowSetEmpty: throttleOverflow && len(m.overflowThrottleGaugeQueues) == 0} +} + +func (m *SQSMetrics) forgetThrottleActionLocked(queue string, action string) sqsThrottleActionForget { + var forget sqsThrottleActionForget + if actions, ok := m.trackedThrottleGaugeQueues[queue]; ok { + if _, hasAction := actions[action]; hasAction { + delete(actions, action) + forget.tracked = true + } + if len(actions) == 0 { + delete(m.trackedThrottleGaugeQueues, queue) + } + } + if queues, ok := m.overflowThrottleGaugeQueues[action]; ok { + if _, hasQueue := queues[queue]; hasQueue { + delete(queues, queue) + if len(queues) == 0 { + delete(m.overflowThrottleGaugeQueues, action) + forget.overflowSetEmpty = true + } + } + } + return forget +} + +func (m *SQSMetrics) removeThrottleOverflowQueueLocked(queue string) []string { + var emptyActions []string + for action, queues := range m.overflowThrottleGaugeQueues { + if _, ok := queues[queue]; !ok { + continue + } + delete(queues, queue) + if len(queues) == 0 { + delete(m.overflowThrottleGaugeQueues, action) + emptyActions = append(emptyActions, action) + } + } + return emptyActions } func (m *SQSMetrics) dropForgottenDepthQueue(queue string, forget sqsGaugeForget) { @@ -378,12 +462,12 @@ func (m *SQSMetrics) dropForgottenDepthQueue(queue string, forget sqsGaugeForget } } -func (m *SQSMetrics) dropForgottenThrottleQueue(queue string, forget sqsGaugeForget) { +func (m *SQSMetrics) dropForgottenThrottleQueue(queue string, forget sqsThrottleGaugeForget) { if forget.tracked { m.dropThrottleGaugeActionsFor(queue) } - if forget.overflowSetEmpty { - m.dropThrottleGaugeActionsFor(sqsQueueOverflow) + for _, action := range forget.overflowActions { + m.dropThrottleGaugeActionFor(sqsQueueOverflow, action) } } @@ -396,13 +480,23 @@ func (m *SQSMetrics) dropForgottenThrottleQueue(queue string, forget sqsGaugeFor func (m *SQSMetrics) admitForCounterBudget(queue string) string { m.mu.Lock() defer m.mu.Unlock() - if _, ok := m.trackedCounterQueues[queue]; ok { + return admitCounterQueueLocked(queue, m.trackedCounterQueues) +} + +func (m *SQSMetrics) admitForThrottleCounterBudget(queue string) string { + m.mu.Lock() + defer m.mu.Unlock() + return admitCounterQueueLocked(queue, m.trackedThrottleCounterQueues) +} + +func admitCounterQueueLocked(queue string, tracked map[string]struct{}) string { + if _, ok := tracked[queue]; ok { return queue } - if len(m.trackedCounterQueues) >= sqsMaxTrackedQueues { + if len(tracked) >= sqsMaxTrackedQueues { return sqsQueueOverflow } - m.trackedCounterQueues[queue] = struct{}{} + tracked[queue] = struct{}{} return queue } @@ -454,28 +548,28 @@ func (m *SQSMetrics) admitForDepthBudget(queue string) string { // event-driven throttle-token gauge. It is separate from queue depth // because a queue can emit throttle decisions without being present in // the next depth scrape yet. -func (m *SQSMetrics) admitForThrottleGaugeBudget(queue string) string { +func (m *SQSMetrics) admitForThrottleGaugeBudget(queue string, action string) (string, []string) { m.mu.Lock() - if _, ok := m.trackedThrottleGaugeQueues[queue]; ok { + if actions, ok := m.trackedThrottleGaugeQueues[queue]; ok { + actions[action] = struct{}{} + overflowActionsToDrop := m.removeThrottleOverflowQueueLocked(queue) m.mu.Unlock() - return queue + return queue, overflowActionsToDrop } if len(m.trackedThrottleGaugeQueues) >= sqsMaxTrackedQueues { - m.overflowThrottleGaugeQueues[queue] = struct{}{} + queues := m.overflowThrottleGaugeQueues[action] + if queues == nil { + queues = map[string]struct{}{} + m.overflowThrottleGaugeQueues[action] = queues + } + queues[queue] = struct{}{} m.mu.Unlock() - return sqsQueueOverflow - } - m.trackedThrottleGaugeQueues[queue] = struct{}{} - _, wasOverflow := m.overflowThrottleGaugeQueues[queue] - if wasOverflow { - delete(m.overflowThrottleGaugeQueues, queue) + return sqsQueueOverflow, nil } - overflowSetEmpty := wasOverflow && len(m.overflowThrottleGaugeQueues) == 0 + m.trackedThrottleGaugeQueues[queue] = map[string]struct{}{action: {}} + overflowActionsToDrop := m.removeThrottleOverflowQueueLocked(queue) m.mu.Unlock() - if overflowSetEmpty { - m.dropThrottleGaugeActionsFor(sqsQueueOverflow) - } - return queue + return queue, overflowActionsToDrop } func (m *SQSMetrics) snapshotThrottleGaugeQueues() []string { @@ -484,12 +578,20 @@ func (m *SQSMetrics) snapshotThrottleGaugeQueues() []string { } m.mu.Lock() defer m.mu.Unlock() + seen := make(map[string]struct{}, len(m.trackedThrottleGaugeQueues)) queues := make([]string, 0, len(m.trackedThrottleGaugeQueues)+len(m.overflowThrottleGaugeQueues)) for queue := range m.trackedThrottleGaugeQueues { + seen[queue] = struct{}{} queues = append(queues, queue) } - for queue := range m.overflowThrottleGaugeQueues { - queues = append(queues, queue) + for _, overflowQueues := range m.overflowThrottleGaugeQueues { + for queue := range overflowQueues { + if _, ok := seen[queue]; ok { + continue + } + seen[queue] = struct{}{} + queues = append(queues, queue) + } } return queues } @@ -506,9 +608,13 @@ func (m *SQSMetrics) dropGaugeStatesFor(label string) { } func (m *SQSMetrics) dropThrottleGaugeActionsFor(label string) { - m.throttleTokens.DeleteLabelValues(label, SQSThrottleActionSend) - m.throttleTokens.DeleteLabelValues(label, SQSThrottleActionReceive) - m.throttleTokens.DeleteLabelValues(label, SQSThrottleActionDefault) + for _, action := range []string{SQSThrottleActionSend, SQSThrottleActionReceive, SQSThrottleActionDefault} { + m.dropThrottleGaugeActionFor(label, action) + } +} + +func (m *SQSMetrics) dropThrottleGaugeActionFor(label string, action string) { + m.throttleTokens.DeleteLabelValues(label, action) } // sqsValidPartitionAction returns true iff action is one of the diff --git a/monitoring/sqs_test.go b/monitoring/sqs_test.go index fc62bcaa4..2a5ca9921 100644 --- a/monitoring/sqs_test.go +++ b/monitoring/sqs_test.go @@ -9,9 +9,48 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/testutil" + dto "github.com/prometheus/client_model/go" "github.com/stretchr/testify/require" ) +func gatheredThrottleTokenValue(t *testing.T, reg *prometheus.Registry, labels map[string]string) (float64, bool) { + t.Helper() + mfs, err := reg.Gather() + require.NoError(t, err) + for _, mf := range mfs { + if mf.GetName() != "elastickv_sqs_throttle_tokens_remaining" { + continue + } + for _, metric := range mf.GetMetric() { + if !metricLabelsMatch(metric.GetLabel(), labels) { + continue + } + switch { + case metric.GetGauge() != nil: + return metric.GetGauge().GetValue(), true + case metric.GetCounter() != nil: + return metric.GetCounter().GetValue(), true + default: + return 0, true + } + } + } + return 0, false +} + +func metricLabelsMatch(labels []*dto.LabelPair, want map[string]string) bool { + if len(labels) != len(want) { + return false + } + for _, label := range labels { + got, ok := want[label.GetName()] + if !ok || got != label.GetValue() { + return false + } + } + return true +} + // TestSQSMetrics_ObservePartitionMessage_IncrementsByLabelTriple // pins the basic counter contract: each (queue, partition, // action) tuple gets its own series and only the matching @@ -185,12 +224,54 @@ func TestSQSMetrics_ObserveThrottleDecision_AllowedDoesNotConsumeCounterBudget(t require.Empty(t, m.trackedCounterQueues, "allowed-only throttle decisions must not consume counter label budget") m.ObserveThrottleDecision("blocked.fifo", SQSThrottleActionSend, 0, true) + require.Empty(t, m.trackedCounterQueues, "throttle rejects must not consume the partition counter label budget") + require.Len(t, m.trackedThrottleCounterQueues, 1, "throttle rejects consume the throttle counter label budget") require.InDelta(t, 1.0, testutil.ToFloat64(m.throttledRequests.WithLabelValues("blocked.fifo", SQSThrottleActionSend)), 0.001, "first actual reject must keep its real queue label instead of overflowing after allowed-only decisions") } +func TestSQSMetrics_ThrottleCounterBudgetIsIndependent(t *testing.T) { + t.Parallel() + + t.Run("throttle rejects do not hide later partition counters", func(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + for i := 0; i < sqsMaxTrackedQueues; i++ { + m.ObserveThrottleDecision("throttle-"+strconv.Itoa(i)+".fifo", SQSThrottleActionSend, 0, true) + } + require.Len(t, m.trackedThrottleCounterQueues, sqsMaxTrackedQueues) + require.Empty(t, m.trackedCounterQueues) + + m.ObservePartitionMessage("partition.fifo", 0, SQSPartitionActionSend) + require.InDelta(t, 1.0, + testutil.ToFloat64(m.partitionMessages.WithLabelValues("partition.fifo", "0", SQSPartitionActionSend)), + 0.001, + "partition counter must still get a real queue label after throttle counter budget fills") + }) + + t.Run("partition counters do not hide later throttle rejects", func(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + for i := 0; i < sqsMaxTrackedQueues; i++ { + m.ObservePartitionMessage("partition-"+strconv.Itoa(i)+".fifo", 0, SQSPartitionActionSend) + } + require.Len(t, m.trackedCounterQueues, sqsMaxTrackedQueues) + require.Empty(t, m.trackedThrottleCounterQueues) + + m.ObserveThrottleDecision("blocked.fifo", SQSThrottleActionSend, 0, true) + require.InDelta(t, 1.0, + testutil.ToFloat64(m.throttledRequests.WithLabelValues("blocked.fifo", SQSThrottleActionSend)), + 0.001, + "throttle counter must still get a real queue label after partition counter budget fills") + }) +} + func TestSQSMetrics_ObserveThrottleDecision_DropsInvalidLabels(t *testing.T) { t.Parallel() reg := prometheus.NewRegistry() @@ -207,6 +288,83 @@ func TestSQSMetrics_ObserveThrottleDecision_DropsInvalidLabels(t *testing.T) { require.Equal(t, 0, gaugeCount) } +func TestSQSMetrics_ForgetThrottleAction_DropsGaugeAndFreesSlot(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 5, false) + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionReceive, 7, false) + require.Len(t, m.trackedThrottleGaugeQueues, 1) + + m.ForgetThrottleAction("orders.fifo", SQSThrottleActionSend) + _, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "orders.fifo", + "action": SQSThrottleActionSend, + }) + require.False(t, found, "disabled send bucket must drop the stale token gauge") + remaining, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "orders.fifo", + "action": SQSThrottleActionReceive, + }) + require.True(t, found, "other configured actions for the same queue must survive") + require.InDelta(t, 7.0, remaining, 0.001) + require.Len(t, m.trackedThrottleGaugeQueues, 1, "queue stays admitted while another action remains") + + m.ForgetThrottleAction("orders.fifo", SQSThrottleActionReceive) + count, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 0, count) + require.Empty(t, m.trackedThrottleGaugeQueues, "last disabled action frees the throttle gauge queue slot") +} + +func TestSQSMetrics_ForgetQueue_OverflowThrottleGaugeClearsPerAction(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + for i := 0; i < sqsMaxTrackedQueues; i++ { + m.ObserveThrottleDecision("real-"+strconv.Itoa(i)+".fifo", SQSThrottleActionSend, 1, false) + } + require.Len(t, m.trackedThrottleGaugeQueues, sqsMaxTrackedQueues) + + m.ObserveThrottleDecision("overflow-a.fifo", SQSThrottleActionSend, 10, false) + m.ObserveThrottleDecision("overflow-b.fifo", SQSThrottleActionReceive, 20, false) + + send, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": sqsQueueOverflow, + "action": SQSThrottleActionSend, + }) + require.True(t, found) + require.InDelta(t, 10.0, send, 0.001) + receive, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": sqsQueueOverflow, + "action": SQSThrottleActionReceive, + }) + require.True(t, found) + require.InDelta(t, 20.0, receive, 0.001) + + m.ForgetQueue("overflow-a.fifo") + _, found = gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": sqsQueueOverflow, + "action": SQSThrottleActionSend, + }) + require.False(t, found, "last overflow send queue disappearing must drop only _other/send") + receive, found = gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": sqsQueueOverflow, + "action": SQSThrottleActionReceive, + }) + require.True(t, found, "_other/receive must survive while another overflow receive queue remains") + require.InDelta(t, 20.0, receive, 0.001) + + m.ForgetQueue("overflow-b.fifo") + _, found = gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": sqsQueueOverflow, + "action": SQSThrottleActionReceive, + }) + require.False(t, found, "last overflow receive queue disappearing must drop _other/receive") +} + // TestSQSMetrics_ObserveQueueDepth_EmitsThreeStates pins that one // ObserveQueueDepth call produces three series (visible / not_visible // / delayed) under elastickv_sqs_queue_messages keyed on queue name. From c7ea8ad434fd7d9621d15680122e89841de3234f Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 8 Jul 2026 16:53:08 +0900 Subject: [PATCH 09/16] monitoring: sync SQS throttle gauges --- adapter/sqs.go | 32 +++-- adapter/sqs_admin.go | 3 +- adapter/sqs_catalog.go | 54 ++++---- adapter/sqs_throttle_integration_test.go | 38 ++++++ adapter/sqs_throttle_test.go | 32 +++++ ...6_04_24_proposed_sqs_compatible_adapter.md | 2 +- monitoring/sqs.go | 119 ++++++++++++++---- monitoring/sqs_test.go | 63 ++++++++++ 8 files changed, 279 insertions(+), 64 deletions(-) diff --git a/adapter/sqs.go b/adapter/sqs.go index 645c0bbaf..8765238f0 100644 --- a/adapter/sqs.go +++ b/adapter/sqs.go @@ -229,6 +229,7 @@ type SQSPartitionObserver interface { type SQSThrottleObserver interface { ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) ForgetThrottleAction(queue string, action string) + SyncThrottleActions(queue string, enabledActions []string) } // SQS metric action labels mirror the values from monitoring/sqs.go. @@ -298,9 +299,7 @@ func (s *SQSServer) observeThrottleDecision(queue string, throttle *sqsQueueThro if s == nil || s.throttleObserver == nil { return } - for _, action := range disabledThrottleMetricActions(throttle) { - s.throttleObserver.ForgetThrottleAction(queue, action) - } + s.observeThrottleConfig(queue, throttle) if !outcome.bucketPresent && outcome.allowed { return } @@ -312,21 +311,28 @@ func (s *SQSServer) observeThrottleDecision(queue string, throttle *sqsQueueThro ) } -func disabledThrottleMetricActions(throttle *sqsQueueThrottle) []string { - disabled := make([]string, 0, len(sqsThrottleMetricActions)) +func (s *SQSServer) observeThrottleConfig(queue string, throttle *sqsQueueThrottle) { + if s == nil || s.throttleObserver == nil { + return + } + s.throttleObserver.SyncThrottleActions(queue, enabledThrottleMetricActions(throttle)) +} + +func enabledThrottleMetricActions(throttle *sqsQueueThrottle) []string { if throttle == nil || throttle.IsEmpty() { - return append(disabled, sqsThrottleMetricActions[:]...) + return nil } - if throttle.SendCapacity == 0 { - disabled = append(disabled, SQSThrottleActionSend) + enabled := make([]string, 0, len(sqsThrottleMetricActions)) + if throttle.SendCapacity > 0 { + enabled = append(enabled, SQSThrottleActionSend) } - if throttle.RecvCapacity == 0 { - disabled = append(disabled, SQSThrottleActionReceive) + if throttle.RecvCapacity > 0 { + enabled = append(enabled, SQSThrottleActionReceive) } - if throttle.DefaultCapacity == 0 { - disabled = append(disabled, SQSThrottleActionDefault) + if throttle.DefaultCapacity > 0 { + enabled = append(enabled, SQSThrottleActionDefault) } - return disabled + return enabled } // WithSQSPartitionResolver installs the cluster's partition diff --git a/adapter/sqs_admin.go b/adapter/sqs_admin.go index 3b809be64..a8513ca67 100644 --- a/adapter/sqs_admin.go +++ b/adapter/sqs_admin.go @@ -353,7 +353,7 @@ func (s *SQSServer) AdminSetQueueAttributes(ctx context.Context, principal Admin if strings.TrimSpace(name) == "" || len(attrs) == 0 { return ErrAdminSQSValidation } - throttleChanged, err := s.setQueueAttributesWithRetry(ctx, name, attrs) + throttleChanged, throttle, err := s.setQueueAttributesWithRetry(ctx, name, attrs) if err != nil { if isSQSAdminQueueDoesNotExist(err) { return ErrAdminSQSNotFound @@ -365,6 +365,7 @@ func (s *SQSServer) AdminSetQueueAttributes(ctx context.Context, principal Admin } if throttleChanged { s.throttle.invalidateQueue(name) + s.observeThrottleConfig(name, throttle) } return nil } diff --git a/adapter/sqs_catalog.go b/adapter/sqs_catalog.go index 5e4f8af53..ddd9ee70b 100644 --- a/adapter/sqs_catalog.go +++ b/adapter/sqs_catalog.go @@ -1509,7 +1509,7 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) { writeSQSError(w, http.StatusBadRequest, sqsErrMissingParameter, "Attributes is required") return } - throttleChanged, err := s.setQueueAttributesWithRetry(r.Context(), name, in.Attributes) + throttleChanged, throttle, err := s.setQueueAttributesWithRetry(r.Context(), name, in.Attributes) if err != nil { writeSQSErrorFromErr(w, err) return @@ -1528,33 +1528,36 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) { // full capacity. trySetQueueAttributesOnce therefore // compares the old and new throttle configs under the same Raft // read snapshot used for the commit and reports whether the values - // actually moved. The bucket reconciliation in loadOrInit also + // actually moved, plus the committed throttle config so the metrics + // layer can clear disabled token gauges even if the queue goes quiet. + // The bucket reconciliation in loadOrInit also // catches a stale bucket if a throttle change slips past this gate // (e.g. via a future admin path), so the gating here is purely a // hot-path optimisation plus a no-op-bypass guard. if throttleChanged { s.throttle.invalidateQueue(name) + s.observeThrottleConfig(name, throttle) } writeSQSJSON(w, map[string]any{}) } -func (s *SQSServer) setQueueAttributesWithRetry(ctx context.Context, queueName string, attrs map[string]string) (bool, error) { +func (s *SQSServer) setQueueAttributesWithRetry(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, error) { backoff := transactRetryInitialBackoff deadline := time.Now().Add(transactRetryMaxDuration) for range transactRetryMaxAttempts { - throttleChanged, done, err := s.trySetQueueAttributesOnce(ctx, queueName, attrs) + throttleChanged, throttle, done, err := s.trySetQueueAttributesOnce(ctx, queueName, attrs) if err == nil && done { - return throttleChanged, nil + return throttleChanged, throttle, nil } if err != nil && !isRetryableTransactWriteError(err) { - return false, err + return false, nil, err } if err := waitRetryWithDeadline(ctx, deadline, backoff); err != nil { - return false, errors.WithStack(err) + return false, nil, errors.WithStack(err) } backoff = nextTransactRetryBackoff(backoff) } - return false, newSQSAPIError(http.StatusInternalServerError, sqsErrInternalFailure, "set queue attributes retry attempts exhausted") + return false, nil, newSQSAPIError(http.StatusInternalServerError, sqsErrInternalFailure, "set queue attributes retry attempts exhausted") } // applyAndValidateSetAttributes runs the apply + cross-validator @@ -1592,23 +1595,23 @@ func applyAndValidateSetAttributes(meta *sqsQueueMeta, attrs map[string]string) return validatePartitionConfig(meta) } -// trySetQueueAttributesOnce is one read-validate-commit pass. The -// returns are (throttleChanged, done, err). done reports whether the -// caller should stop retrying (the attrs are now committed); an error -// means either a non-retryable failure (propagate) or a retryable -// write conflict (retry after backoff). throttleChanged is true iff -// the post-apply meta's Throttle config differs from the pre-apply -// snapshot — the caller uses it to gate the cache invalidation so -// that a no-op same-value SetQueueAttributes does not reset the -// bucket to full capacity. -func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, bool, error) { +// trySetQueueAttributesOnce is one read-validate-commit pass. The returns are +// (throttleChanged, postThrottle, done, err). done reports whether the caller +// should stop retrying (the attrs are now committed); an error means either a +// non-retryable failure (propagate) or a retryable write conflict (retry after +// backoff). throttleChanged is true iff the post-apply meta's Throttle config +// differs from the pre-apply snapshot — the caller uses it to gate the cache +// invalidation and metrics reconciliation so that a no-op same-value +// SetQueueAttributes does not reset the bucket to full capacity or churn token +// gauges. +func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, bool, error) { readTS := s.nextTxnReadTS(ctx) meta, exists, err := s.loadQueueMetaAt(ctx, queueName, readTS) if err != nil { - return false, false, errors.WithStack(err) + return false, nil, false, errors.WithStack(err) } if !exists { - return false, false, newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist") + return false, nil, false, newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist") } // Snapshot the throttle config under the same read TS used for the // commit so the comparison sees the value the writer is racing @@ -1616,16 +1619,17 @@ func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName str // floats wrapped in a struct). preThrottle := snapshotThrottle(meta.Throttle) if err := applyAndValidateSetAttributes(meta, attrs); err != nil { - return false, false, err + return false, nil, false, err } if err := s.validateRedrivePolicyTarget(ctx, meta, readTS); err != nil { - return false, false, err + return false, nil, false, err } throttleChanged := !throttleConfigEqual(preThrottle, meta.Throttle) + postThrottle := snapshotThrottle(meta.Throttle) meta.LastModifiedAtMillis = time.Now().UnixMilli() metaBytes, err := encodeSQSQueueMeta(meta) if err != nil { - return false, false, errors.WithStack(err) + return false, nil, false, errors.WithStack(err) } metaKey := sqsQueueMetaKey(queueName) // StartTS + ReadKeys prevent two concurrent SetQueueAttributes from @@ -1640,9 +1644,9 @@ func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName str }, } if _, err := s.coordinator.Dispatch(ctx, req); err != nil { - return false, false, errors.WithStack(err) + return false, nil, false, errors.WithStack(err) } - return throttleChanged, true, nil + return throttleChanged, postThrottle, true, nil } // snapshotThrottle returns a value-copy of the Throttle config so a diff --git a/adapter/sqs_throttle_integration_test.go b/adapter/sqs_throttle_integration_test.go index 2fd716a27..667c2251b 100644 --- a/adapter/sqs_throttle_integration_test.go +++ b/adapter/sqs_throttle_integration_test.go @@ -4,6 +4,8 @@ import ( "net/http" "strconv" "testing" + + "github.com/stretchr/testify/require" ) // === SQS throttle test refill-rate guardrail ============================ @@ -274,6 +276,42 @@ func TestSQSServer_Throttle_SetQueueAttributesInvalidatesBucket(t *testing.T) { } } +func TestSQSServer_Throttle_SetQueueAttributesSyncsDisabledMetrics(t *testing.T) { + t.Parallel() + nodes, _, _ := createNode(t, 1) + defer shutdown(nodes) + node := sqsLeaderNode(t, nodes) + observer := &recordingSQSThrottleObserver{} + node.sqsServer.throttleObserver = observer + + url := mustCreateQueue(t, node, "throttle-metric-sync") + mustSetQueueAttributes(t, node, url, map[string]string{ + "ThrottleSendCapacity": drainBucketCapacity, + "ThrottleSendRefillPerSecond": slowRefillRate, + "ThrottleRecvCapacity": drainBucketCapacity, + "ThrottleRecvRefillPerSecond": slowRefillRate, + }) + require.Contains(t, observer.syncs, throttleSyncReport{ + queue: "throttle-metric-sync", + enabled: []string{SQSThrottleActionSend, SQSThrottleActionReceive}, + }) + + observer.syncs = nil + observer.forgotten = nil + mustSetQueueAttributes(t, node, url, map[string]string{ + "ThrottleRecvCapacity": "0", + "ThrottleRecvRefillPerSecond": "0", + }) + + require.Contains(t, observer.syncs, throttleSyncReport{ + queue: "throttle-metric-sync", + enabled: []string{SQSThrottleActionSend}, + }) + require.NotContains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionSend}) + require.Contains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionReceive}) + require.Contains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionDefault}) +} + // TestSQSServer_Throttle_DeleteQueueInvalidatesBucket pins the §3.1 // cache-invalidation contract for DeleteQueue: a same-name recreate // gets a fresh bucket, not the stale balance from the previous diff --git a/adapter/sqs_throttle_test.go b/adapter/sqs_throttle_test.go index ecfad627c..829f4da91 100644 --- a/adapter/sqs_throttle_test.go +++ b/adapter/sqs_throttle_test.go @@ -23,9 +23,15 @@ type throttleForgetReport struct { action string } +type throttleSyncReport struct { + queue string + enabled []string +} + type recordingSQSThrottleObserver struct { reports []throttleObserveReport forgotten []throttleForgetReport + syncs []throttleSyncReport } func (r *recordingSQSThrottleObserver) ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) { @@ -41,6 +47,28 @@ func (r *recordingSQSThrottleObserver) ForgetThrottleAction(queue string, action r.forgotten = append(r.forgotten, throttleForgetReport{queue: queue, action: action}) } +func (r *recordingSQSThrottleObserver) SyncThrottleActions(queue string, enabledActions []string) { + enabled := append([]string(nil), enabledActions...) + r.syncs = append(r.syncs, throttleSyncReport{queue: queue, enabled: enabled}) + for _, action := range disabledThrottleMetricActionsFromEnabled(enabled) { + r.forgotten = append(r.forgotten, throttleForgetReport{queue: queue, action: action}) + } +} + +func disabledThrottleMetricActionsFromEnabled(enabledActions []string) []string { + enabled := make(map[string]struct{}, len(enabledActions)) + for _, action := range enabledActions { + enabled[action] = struct{}{} + } + disabled := make([]string, 0, len(sqsThrottleMetricActions)) + for _, action := range sqsThrottleMetricActions { + if _, ok := enabled[action]; !ok { + disabled = append(disabled, action) + } + } + return disabled +} + // TestBucketStore_DefaultOff_ShortCircuit pins the contract that a // nil throttle config never allocates a bucket and never rejects. // This is the hot path for unconfigured queues — every nil-check that @@ -86,12 +114,16 @@ func TestSQSServer_ChargeQueueWithThrottleForgetsDisabledMetrics(t *testing.T) { rec := httptest.NewRecorder() require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1)) + require.Contains(t, observer.syncs, throttleSyncReport{queue: "orders.fifo", enabled: []string{SQSThrottleActionSend}}) + require.NotContains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionSend}) require.Contains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionReceive}) require.Contains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionDefault}) observer.forgotten = nil + observer.syncs = nil rec = httptest.NewRecorder() require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, nil, 1)) + require.Contains(t, observer.syncs, throttleSyncReport{queue: "orders.fifo"}) require.ElementsMatch(t, []throttleForgetReport{ {queue: "orders.fifo", action: SQSThrottleActionSend}, {queue: "orders.fifo", action: SQSThrottleActionReceive}, diff --git a/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md b/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md index d905a88cf..345829dd9 100644 --- a/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md +++ b/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md @@ -689,7 +689,7 @@ Structured logs include `route`, `queue`, `action`, `remote_ip`, `token_hash_pre 1. Query-protocol full XML fidelity for older SDKs. 2. `ApproximateNumberOfMessagesDelayed` / `NotVisible` accuracy. -3. Per-queue throttling and fairness across tenants has landed; see [`2026_04_26_implemented_sqs_per_queue_throttling.md`](2026_04_26_implemented_sqs_per_queue_throttling.md). +3. Per-queue throttling has landed; see [`2026_04_26_implemented_sqs_per_queue_throttling.md`](2026_04_26_implemented_sqs_per_queue_throttling.md). 4. Split-queue FIFO for very hot queues. 5. Console UI polish: in-flight tab with per-message countdown, filtering, dark mode. diff --git a/monitoring/sqs.go b/monitoring/sqs.go index 1a97e5435..79948f20c 100644 --- a/monitoring/sqs.go +++ b/monitoring/sqs.go @@ -55,6 +55,7 @@ type SQSPartitionObserver interface { type SQSThrottleObserver interface { ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) ForgetThrottleAction(queue string, action string) + SyncThrottleActions(queue string, enabledActions []string) } // SQSDepthSource is the contract a per-tick queue-depth source must @@ -150,6 +151,8 @@ type SQSMetrics struct { trackedThrottleCounterQueues map[string]struct{} trackedDepthQueues map[string]struct{} trackedThrottleGaugeQueues map[string]map[string]struct{} + throttleGaugeQueueSeq map[string]uint64 + throttleGaugeSeq uint64 // overflowDepthQueues is the set of queue names whose depth // gauge is currently collapsed onto the shared sqsQueueOverflow // label. Tracked separately from trackedDepthQueues (which @@ -210,6 +213,7 @@ func newSQSMetrics(registerer prometheus.Registerer) *SQSMetrics { trackedThrottleCounterQueues: map[string]struct{}{}, trackedDepthQueues: map[string]struct{}{}, trackedThrottleGaugeQueues: map[string]map[string]struct{}{}, + throttleGaugeQueueSeq: map[string]uint64{}, overflowDepthQueues: map[string]struct{}{}, overflowThrottleGaugeQueues: map[string]map[string]struct{}{}, } @@ -283,11 +287,10 @@ func (m *SQSMetrics) ObserveThrottleDecision(queue string, action string, tokens queueCounterLabel := m.admitForThrottleCounterBudget(queue) m.throttledRequests.WithLabelValues(queueCounterLabel, action).Inc() } - queueGaugeLabel, overflowActionsToDrop := m.admitForThrottleGaugeBudget(queue, action) - for _, action := range overflowActionsToDrop { - m.dropThrottleGaugeActionFor(sqsQueueOverflow, action) - } + m.mu.Lock() + queueGaugeLabel := m.admitForThrottleGaugeBudgetLocked(queue, action) m.throttleTokens.WithLabelValues(queueGaugeLabel, action).Set(tokensRemaining) + m.mu.Unlock() } // ForgetThrottleAction removes the token-balance gauge for one queue/action @@ -302,13 +305,43 @@ func (m *SQSMetrics) ForgetThrottleAction(queue string, action string) { } m.mu.Lock() forget := m.forgetThrottleActionLocked(queue, action) - m.mu.Unlock() if forget.tracked { m.dropThrottleGaugeActionFor(queue, action) } if forget.overflowSetEmpty { m.dropThrottleGaugeActionFor(sqsQueueOverflow, action) } + m.mu.Unlock() +} + +// SyncThrottleActions reconciles token-balance gauges after a queue's throttle +// configuration changes, even when no later request touches the disabled +// bucket. enabledActions is the configured metric-action set; omitted actions +// have their gauges removed. Counters intentionally survive. +func (m *SQSMetrics) SyncThrottleActions(queue string, enabledActions []string) { + if m == nil || queue == "" { + return + } + enabled := make(map[string]struct{}, len(enabledActions)) + for _, action := range enabledActions { + if sqsValidThrottleAction(action) { + enabled[action] = struct{}{} + } + } + m.mu.Lock() + for _, action := range []string{SQSThrottleActionSend, SQSThrottleActionReceive, SQSThrottleActionDefault} { + if _, ok := enabled[action]; ok { + continue + } + forget := m.forgetThrottleActionLocked(queue, action) + if forget.tracked { + m.dropThrottleGaugeActionFor(queue, action) + } + if forget.overflowSetEmpty { + m.dropThrottleGaugeActionFor(sqsQueueOverflow, action) + } + } + m.mu.Unlock() } // ForgetQueue drops the three gauge series for a queue and frees @@ -363,9 +396,9 @@ func (m *SQSMetrics) ForgetQueue(queue string) { m.mu.Lock() depthForget := m.forgetDepthQueueLocked(queue) throttleForget := m.forgetThrottleQueueLocked(queue) + m.dropForgottenThrottleQueue(queue, throttleForget) m.mu.Unlock() m.dropForgottenDepthQueue(queue, depthForget) - m.dropForgottenThrottleQueue(queue, throttleForget) } type sqsGaugeForget struct { @@ -404,6 +437,7 @@ func (m *SQSMetrics) forgetThrottleQueueLocked(queue string) sqsThrottleGaugeFor if throttleTracked { delete(m.trackedThrottleGaugeQueues, queue) } + delete(m.throttleGaugeQueueSeq, queue) return sqsThrottleGaugeForget{ tracked: throttleTracked, overflowActions: m.removeThrottleOverflowQueueLocked(queue), @@ -430,6 +464,9 @@ func (m *SQSMetrics) forgetThrottleActionLocked(queue string, action string) sqs } } } + if !m.throttleGaugeQueueTrackedLocked(queue) { + delete(m.throttleGaugeQueueSeq, queue) + } return forget } @@ -445,9 +482,24 @@ func (m *SQSMetrics) removeThrottleOverflowQueueLocked(queue string) []string { emptyActions = append(emptyActions, action) } } + if !m.throttleGaugeQueueTrackedLocked(queue) { + delete(m.throttleGaugeQueueSeq, queue) + } return emptyActions } +func (m *SQSMetrics) throttleGaugeQueueTrackedLocked(queue string) bool { + if _, ok := m.trackedThrottleGaugeQueues[queue]; ok { + return true + } + for _, queues := range m.overflowThrottleGaugeQueues { + if _, ok := queues[queue]; ok { + return true + } + } + return false +} + func (m *SQSMetrics) dropForgottenDepthQueue(queue string, forget sqsGaugeForget) { if forget.tracked { m.dropGaugeStatesFor(queue) @@ -544,17 +596,20 @@ func (m *SQSMetrics) admitForDepthBudget(queue string) string { return queue } -// admitForThrottleGaugeBudget mirrors admitForDepthBudget for the -// event-driven throttle-token gauge. It is separate from queue depth -// because a queue can emit throttle decisions without being present in -// the next depth scrape yet. -func (m *SQSMetrics) admitForThrottleGaugeBudget(queue string, action string) (string, []string) { - m.mu.Lock() +// admitForThrottleGaugeBudgetLocked mirrors admitForDepthBudget for the +// event-driven throttle-token gauge. It is separate from queue depth because a +// queue can emit throttle decisions without being present in the next depth +// scrape yet. m.mu must be held so _other deletions cannot race a concurrent +// overflow Set for the same action. +func (m *SQSMetrics) admitForThrottleGaugeBudgetLocked(queue string, action string) string { + m.throttleGaugeSeq++ + m.throttleGaugeQueueSeq[queue] = m.throttleGaugeSeq if actions, ok := m.trackedThrottleGaugeQueues[queue]; ok { actions[action] = struct{}{} - overflowActionsToDrop := m.removeThrottleOverflowQueueLocked(queue) - m.mu.Unlock() - return queue, overflowActionsToDrop + for _, overflowAction := range m.removeThrottleOverflowQueueLocked(queue) { + m.dropThrottleGaugeActionFor(sqsQueueOverflow, overflowAction) + } + return queue } if len(m.trackedThrottleGaugeQueues) >= sqsMaxTrackedQueues { queues := m.overflowThrottleGaugeQueues[action] @@ -563,16 +618,25 @@ func (m *SQSMetrics) admitForThrottleGaugeBudget(queue string, action string) (s m.overflowThrottleGaugeQueues[action] = queues } queues[queue] = struct{}{} - m.mu.Unlock() - return sqsQueueOverflow, nil + return sqsQueueOverflow } m.trackedThrottleGaugeQueues[queue] = map[string]struct{}{action: {}} - overflowActionsToDrop := m.removeThrottleOverflowQueueLocked(queue) - m.mu.Unlock() - return queue, overflowActionsToDrop + for _, overflowAction := range m.removeThrottleOverflowQueueLocked(queue) { + m.dropThrottleGaugeActionFor(sqsQueueOverflow, overflowAction) + } + return queue +} + +func (m *SQSMetrics) throttleGaugeSnapshotCutoff() uint64 { + if m == nil { + return 0 + } + m.mu.Lock() + defer m.mu.Unlock() + return m.throttleGaugeSeq } -func (m *SQSMetrics) snapshotThrottleGaugeQueues() []string { +func (m *SQSMetrics) snapshotThrottleGaugeQueues(cutoff uint64) []string { if m == nil { return nil } @@ -581,11 +645,17 @@ func (m *SQSMetrics) snapshotThrottleGaugeQueues() []string { seen := make(map[string]struct{}, len(m.trackedThrottleGaugeQueues)) queues := make([]string, 0, len(m.trackedThrottleGaugeQueues)+len(m.overflowThrottleGaugeQueues)) for queue := range m.trackedThrottleGaugeQueues { + if m.throttleGaugeQueueSeq[queue] > cutoff { + continue + } seen[queue] = struct{}{} queues = append(queues, queue) } for _, overflowQueues := range m.overflowThrottleGaugeQueues { for queue := range overflowQueues { + if m.throttleGaugeQueueSeq[queue] > cutoff { + continue + } if _, ok := seen[queue]; ok { continue } @@ -710,6 +780,7 @@ func (o *SQSObserver) observeOnce(ctx context.Context, source SQSDepthSource) { if o == nil || o.metrics == nil || source == nil { return } + throttleCutoff := o.metrics.throttleGaugeSnapshotCutoff() snaps, ok := source.SnapshotQueueDepths(ctx) if !ok { // Source signalled "skip this tick" (transient catalog-scan @@ -734,7 +805,7 @@ func (o *SQSObserver) observeOnce(ctx context.Context, source SQSDepthSource) { // tick. Reclaiming first lets phase 2's admissions reuse the // freed slots immediately. o.mu.Lock() - o.forgetMissingQueuesLocked(current) + o.forgetMissingQueuesLocked(current, throttleCutoff) o.lastSeen = current o.mu.Unlock() // Phase 2: emit gauges for the current tick. Slots freed in @@ -747,13 +818,13 @@ func (o *SQSObserver) observeOnce(ctx context.Context, source SQSDepthSource) { } } -func (o *SQSObserver) forgetMissingQueuesLocked(current map[string]struct{}) { +func (o *SQSObserver) forgetMissingQueuesLocked(current map[string]struct{}, throttleCutoff uint64) { for prev := range o.lastSeen { if _, ok := current[prev]; !ok { o.metrics.ForgetQueue(prev) } } - for _, prev := range o.metrics.snapshotThrottleGaugeQueues() { + for _, prev := range o.metrics.snapshotThrottleGaugeQueues(throttleCutoff) { if _, ok := current[prev]; !ok { o.metrics.ForgetQueue(prev) } diff --git a/monitoring/sqs_test.go b/monitoring/sqs_test.go index 2a5ca9921..4f7c305e5 100644 --- a/monitoring/sqs_test.go +++ b/monitoring/sqs_test.go @@ -318,6 +318,35 @@ func TestSQSMetrics_ForgetThrottleAction_DropsGaugeAndFreesSlot(t *testing.T) { require.Empty(t, m.trackedThrottleGaugeQueues, "last disabled action frees the throttle gauge queue slot") } +func TestSQSMetrics_SyncThrottleActions_DropsDisabledWithoutTraffic(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 5, false) + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionReceive, 7, false) + + m.SyncThrottleActions("orders.fifo", []string{SQSThrottleActionSend}) + send, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "orders.fifo", + "action": SQSThrottleActionSend, + }) + require.True(t, found, "configured send bucket must keep its token gauge") + require.InDelta(t, 5.0, send, 0.001) + _, found = gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "orders.fifo", + "action": SQSThrottleActionReceive, + }) + require.False(t, found, "disabled receive bucket must be removed without waiting for more traffic") + require.Len(t, m.trackedThrottleGaugeQueues, 1, "queue stays admitted while send remains configured") + + m.SyncThrottleActions("orders.fifo", nil) + count, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 0, count) + require.Empty(t, m.trackedThrottleGaugeQueues, "disabling the last action frees the gauge slot") +} + func TestSQSMetrics_ForgetQueue_OverflowThrottleGaugeClearsPerAction(t *testing.T) { t.Parallel() reg := prometheus.NewRegistry() @@ -763,6 +792,14 @@ func (f *fakeDepthSource) SnapshotQueueDepths(_ context.Context) ([]SQSQueueDept return t.snaps, t.ok } +type callbackDepthSource struct { + fn func() ([]SQSQueueDepth, bool) +} + +func (f callbackDepthSource) SnapshotQueueDepths(context.Context) ([]SQSQueueDepth, bool) { + return f.fn() +} + // TestSQSObserver_ObserveOnce_EmitsAndForgets pins the observer's // state machine: queues present in the current tick get gauges // emitted, queues that disappeared since the last tick get @@ -857,6 +894,32 @@ func TestSQSObserver_ObserveOnce_ForgetsThrottleOnlyQueues(t *testing.T) { require.Empty(t, m.trackedThrottleGaugeQueues, "throttle gauge budget slot must be reclaimed for the short-lived queue") } +func TestSQSObserver_ObserveOnce_PreservesThrottleGaugeNewerThanDepthSnapshot(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + obs := newSQSObserver(m) + + obs.ObserveOnce(callbackDepthSource{fn: func() ([]SQSQueueDepth, bool) { + m.ObserveThrottleDecision("newer.fifo", SQSThrottleActionSend, 4, false) + return nil, true + }}) + + value, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "newer.fifo", + "action": SQSThrottleActionSend, + }) + require.True(t, found, "a throttle gauge emitted after the cleanup cutoff must survive until the next depth tick") + require.InDelta(t, 4.0, value, 0.001) + + obs.ObserveOnce(&fakeDepthSource{ + ticks: []fakeDepthTick{{snaps: nil, ok: true}}, + }) + count, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 0, count, "the next successful depth tick can clear the now-old throttle-only gauge") +} + // TestSQSObserver_ObserveOnce_TransientScanErrorPreservesGauges // pins the P2 fix from PR #743 r6: when the source returns // ok=false (transient catalog-scan failure on the leader, ctx From 90b816c3520c3ef3e4c4a515a4cc4c559697a8f4 Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 8 Jul 2026 17:15:28 +0900 Subject: [PATCH 10/16] test: stabilize SQS throttle metrics check --- adapter/sqs_throttle_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/adapter/sqs_throttle_test.go b/adapter/sqs_throttle_test.go index 829f4da91..4e88505e2 100644 --- a/adapter/sqs_throttle_test.go +++ b/adapter/sqs_throttle_test.go @@ -88,6 +88,8 @@ func TestSQSServer_ChargeQueueWithThrottleObservesMetrics(t *testing.T) { t.Parallel() observer := &recordingSQSThrottleObserver{} srv := NewSQSServer(nil, nil, nil, WithSQSThrottleObserver(observer)) + now := time.Date(2026, 4, 27, 10, 0, 0, 0, time.UTC) + srv.throttle = newBucketStore(func() time.Time { return now }, throttleIdleEvictAfter) cfg := &sqsQueueThrottle{SendCapacity: 10, SendRefillPerSecond: 1} for range 10 { From 9a91a63a5251256b0561bc47273c08a2d0c8904a Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 8 Jul 2026 17:27:25 +0900 Subject: [PATCH 11/16] adapter: reset SQS throttle gauges on config churn --- adapter/sqs.go | 10 +++ adapter/sqs_admin.go | 7 +- adapter/sqs_catalog.go | 95 +++++++++++++++++------- adapter/sqs_throttle_integration_test.go | 48 ++++++++++++ 4 files changed, 131 insertions(+), 29 deletions(-) diff --git a/adapter/sqs.go b/adapter/sqs.go index 8765238f0..e36b984f9 100644 --- a/adapter/sqs.go +++ b/adapter/sqs.go @@ -318,6 +318,16 @@ func (s *SQSServer) observeThrottleConfig(queue string, throttle *sqsQueueThrott s.throttleObserver.SyncThrottleActions(queue, enabledThrottleMetricActions(throttle)) } +func (s *SQSServer) observeThrottleConfigChange(queue string, throttle *sqsQueueThrottle, resetActions []string) { + if s == nil || s.throttleObserver == nil { + return + } + s.throttleObserver.SyncThrottleActions(queue, enabledThrottleMetricActions(throttle)) + for _, action := range resetActions { + s.throttleObserver.ForgetThrottleAction(queue, action) + } +} + func enabledThrottleMetricActions(throttle *sqsQueueThrottle) []string { if throttle == nil || throttle.IsEmpty() { return nil diff --git a/adapter/sqs_admin.go b/adapter/sqs_admin.go index a8513ca67..32a91c1c8 100644 --- a/adapter/sqs_admin.go +++ b/adapter/sqs_admin.go @@ -353,7 +353,7 @@ func (s *SQSServer) AdminSetQueueAttributes(ctx context.Context, principal Admin if strings.TrimSpace(name) == "" || len(attrs) == 0 { return ErrAdminSQSValidation } - throttleChanged, throttle, err := s.setQueueAttributesWithRetry(ctx, name, attrs) + throttleChanged, throttle, resetActions, err := s.setQueueAttributesWithRetry(ctx, name, attrs) if err != nil { if isSQSAdminQueueDoesNotExist(err) { return ErrAdminSQSNotFound @@ -365,7 +365,7 @@ func (s *SQSServer) AdminSetQueueAttributes(ctx context.Context, principal Admin } if throttleChanged { s.throttle.invalidateQueue(name) - s.observeThrottleConfig(name, throttle) + s.observeThrottleConfigChange(name, throttle, resetActions) } return nil } @@ -394,6 +394,9 @@ func (s *SQSServer) AdminDeleteQueue(ctx context.Context, principal AdminPrincip } return errors.Wrap(err, "admin delete queue") } + s.throttle.invalidateQueue(name) + s.observeThrottleConfig(name, nil) + s.dropReceiveFanoutCounter(name) return nil } diff --git a/adapter/sqs_catalog.go b/adapter/sqs_catalog.go index ddd9ee70b..d73dfe973 100644 --- a/adapter/sqs_catalog.go +++ b/adapter/sqs_catalog.go @@ -817,6 +817,42 @@ func throttleConfigEqual(a, b *sqsQueueThrottle) bool { a.DefaultRefillPerSecond == b.DefaultRefillPerSecond } +func changedThrottleMetricActions(before, after *sqsQueueThrottle) []string { + changed := make([]string, 0, len(sqsThrottleMetricActions)) + for _, action := range sqsThrottleMetricActions { + if !throttleMetricActionEnabled(after, action) { + continue + } + beforeCapacity, beforeRefill := throttleMetricActionConfig(before, action) + afterCapacity, afterRefill := throttleMetricActionConfig(after, action) + if beforeCapacity != afterCapacity || beforeRefill != afterRefill { + changed = append(changed, action) + } + } + return changed +} + +func throttleMetricActionEnabled(throttle *sqsQueueThrottle, action string) bool { + capacity, _ := throttleMetricActionConfig(throttle, action) + return capacity > 0 +} + +func throttleMetricActionConfig(throttle *sqsQueueThrottle, action string) (float64, float64) { + if throttle == nil { + return 0, 0 + } + switch action { + case SQSThrottleActionSend: + return throttle.SendCapacity, throttle.SendRefillPerSecond + case SQSThrottleActionReceive: + return throttle.RecvCapacity, throttle.RecvRefillPerSecond + case SQSThrottleActionDefault: + return throttle.DefaultCapacity, throttle.DefaultRefillPerSecond + default: + return 0, 0 + } +} + // htfifoAttributesEqual compares the Phase 3.D HT-FIFO fields. // // PartitionCount normalisation: validatePartitionConfig documents 0 @@ -1050,6 +1086,7 @@ func (s *SQSServer) tryCreateQueueOnce(ctx context.Context, requested *sqsQueueM // the new queue starts with a fresh full-capacity bucket // regardless of in-flight traffic to the prior incarnation. s.throttle.invalidateQueue(requested.Name) + s.observeThrottleConfigChange(requested.Name, requested.Throttle, enabledThrottleMetricActions(requested.Throttle)) // Mirror the throttle invalidate for the per-queue fanout-rotation // counter. A delete-then-create race could otherwise leave the // new queue starting partitioned receives at the previous @@ -1082,6 +1119,7 @@ func (s *SQSServer) deleteQueue(w http.ResponseWriter, r *http.Request) { // surprising operators who use DeleteQueue+CreateQueue to reset // queue state. s.throttle.invalidateQueue(name) + s.observeThrottleConfig(name, nil) // Drop the per-queue fanout-rotation counter as well. Without // this, repeated DeleteQueue of unique queue names retains one // receiveFanoutCounters entry per name for the process lifetime @@ -1509,7 +1547,7 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) { writeSQSError(w, http.StatusBadRequest, sqsErrMissingParameter, "Attributes is required") return } - throttleChanged, throttle, err := s.setQueueAttributesWithRetry(r.Context(), name, in.Attributes) + throttleChanged, throttle, resetActions, err := s.setQueueAttributesWithRetry(r.Context(), name, in.Attributes) if err != nil { writeSQSErrorFromErr(w, err) return @@ -1528,36 +1566,37 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) { // full capacity. trySetQueueAttributesOnce therefore // compares the old and new throttle configs under the same Raft // read snapshot used for the commit and reports whether the values - // actually moved, plus the committed throttle config so the metrics - // layer can clear disabled token gauges even if the queue goes quiet. - // The bucket reconciliation in loadOrInit also + // actually moved, plus the committed throttle config and still-enabled + // actions whose capacity/refill changed so the metrics layer can clear + // disabled token gauges and reset stale token gauges even if the queue + // goes quiet. The bucket reconciliation in loadOrInit also // catches a stale bucket if a throttle change slips past this gate // (e.g. via a future admin path), so the gating here is purely a // hot-path optimisation plus a no-op-bypass guard. if throttleChanged { s.throttle.invalidateQueue(name) - s.observeThrottleConfig(name, throttle) + s.observeThrottleConfigChange(name, throttle, resetActions) } writeSQSJSON(w, map[string]any{}) } -func (s *SQSServer) setQueueAttributesWithRetry(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, error) { +func (s *SQSServer) setQueueAttributesWithRetry(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, []string, error) { backoff := transactRetryInitialBackoff deadline := time.Now().Add(transactRetryMaxDuration) for range transactRetryMaxAttempts { - throttleChanged, throttle, done, err := s.trySetQueueAttributesOnce(ctx, queueName, attrs) + throttleChanged, throttle, resetActions, done, err := s.trySetQueueAttributesOnce(ctx, queueName, attrs) if err == nil && done { - return throttleChanged, throttle, nil + return throttleChanged, throttle, resetActions, nil } if err != nil && !isRetryableTransactWriteError(err) { - return false, nil, err + return false, nil, nil, err } if err := waitRetryWithDeadline(ctx, deadline, backoff); err != nil { - return false, nil, errors.WithStack(err) + return false, nil, nil, errors.WithStack(err) } backoff = nextTransactRetryBackoff(backoff) } - return false, nil, newSQSAPIError(http.StatusInternalServerError, sqsErrInternalFailure, "set queue attributes retry attempts exhausted") + return false, nil, nil, newSQSAPIError(http.StatusInternalServerError, sqsErrInternalFailure, "set queue attributes retry attempts exhausted") } // applyAndValidateSetAttributes runs the apply + cross-validator @@ -1596,22 +1635,23 @@ func applyAndValidateSetAttributes(meta *sqsQueueMeta, attrs map[string]string) } // trySetQueueAttributesOnce is one read-validate-commit pass. The returns are -// (throttleChanged, postThrottle, done, err). done reports whether the caller -// should stop retrying (the attrs are now committed); an error means either a -// non-retryable failure (propagate) or a retryable write conflict (retry after -// backoff). throttleChanged is true iff the post-apply meta's Throttle config -// differs from the pre-apply snapshot — the caller uses it to gate the cache -// invalidation and metrics reconciliation so that a no-op same-value -// SetQueueAttributes does not reset the bucket to full capacity or churn token -// gauges. -func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, bool, error) { +// (throttleChanged, postThrottle, resetActions, done, err). done reports +// whether the caller should stop retrying (the attrs are now committed); an +// error means either a non-retryable failure (propagate) or a retryable write +// conflict (retry after backoff). throttleChanged is true iff the post-apply +// meta's Throttle config differs from the pre-apply snapshot — the caller uses +// it to gate the cache invalidation and metrics reconciliation so that a no-op +// same-value SetQueueAttributes does not reset the bucket to full capacity or +// churn token gauges. resetActions is the still-enabled metric-action subset +// whose capacity/refill changed and whose stale token gauge must be removed. +func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, []string, bool, error) { readTS := s.nextTxnReadTS(ctx) meta, exists, err := s.loadQueueMetaAt(ctx, queueName, readTS) if err != nil { - return false, nil, false, errors.WithStack(err) + return false, nil, nil, false, errors.WithStack(err) } if !exists { - return false, nil, false, newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist") + return false, nil, nil, false, newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist") } // Snapshot the throttle config under the same read TS used for the // commit so the comparison sees the value the writer is racing @@ -1619,17 +1659,18 @@ func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName str // floats wrapped in a struct). preThrottle := snapshotThrottle(meta.Throttle) if err := applyAndValidateSetAttributes(meta, attrs); err != nil { - return false, nil, false, err + return false, nil, nil, false, err } if err := s.validateRedrivePolicyTarget(ctx, meta, readTS); err != nil { - return false, nil, false, err + return false, nil, nil, false, err } throttleChanged := !throttleConfigEqual(preThrottle, meta.Throttle) postThrottle := snapshotThrottle(meta.Throttle) + resetActions := changedThrottleMetricActions(preThrottle, postThrottle) meta.LastModifiedAtMillis = time.Now().UnixMilli() metaBytes, err := encodeSQSQueueMeta(meta) if err != nil { - return false, nil, false, errors.WithStack(err) + return false, nil, nil, false, errors.WithStack(err) } metaKey := sqsQueueMetaKey(queueName) // StartTS + ReadKeys prevent two concurrent SetQueueAttributes from @@ -1644,9 +1685,9 @@ func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName str }, } if _, err := s.coordinator.Dispatch(ctx, req); err != nil { - return false, nil, false, errors.WithStack(err) + return false, nil, nil, false, errors.WithStack(err) } - return throttleChanged, postThrottle, true, nil + return throttleChanged, postThrottle, resetActions, true, nil } // snapshotThrottle returns a value-copy of the Throttle config so a diff --git a/adapter/sqs_throttle_integration_test.go b/adapter/sqs_throttle_integration_test.go index 667c2251b..49e75fbaa 100644 --- a/adapter/sqs_throttle_integration_test.go +++ b/adapter/sqs_throttle_integration_test.go @@ -296,6 +296,19 @@ func TestSQSServer_Throttle_SetQueueAttributesSyncsDisabledMetrics(t *testing.T) enabled: []string{SQSThrottleActionSend, SQSThrottleActionReceive}, }) + observer.syncs = nil + observer.forgotten = nil + mustSetQueueAttributes(t, node, url, map[string]string{ + "ThrottleSendCapacity": "20", + "ThrottleSendRefillPerSecond": slowRefillRate, + }) + require.Contains(t, observer.syncs, throttleSyncReport{ + queue: "throttle-metric-sync", + enabled: []string{SQSThrottleActionSend, SQSThrottleActionReceive}, + }) + require.Contains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionSend}) + require.NotContains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionReceive}) + observer.syncs = nil observer.forgotten = nil mustSetQueueAttributes(t, node, url, map[string]string{ @@ -312,6 +325,41 @@ func TestSQSServer_Throttle_SetQueueAttributesSyncsDisabledMetrics(t *testing.T) require.Contains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionDefault}) } +func TestSQSServer_Throttle_DeleteCreateQueueSyncsMetrics(t *testing.T) { + t.Parallel() + nodes, _, _ := createNode(t, 1) + defer shutdown(nodes) + node := sqsLeaderNode(t, nodes) + observer := &recordingSQSThrottleObserver{} + node.sqsServer.throttleObserver = observer + + queue := "throttle-metric-recreate" + url := mustCreateQueue(t, node, queue) + mustSetQueueAttributes(t, node, url, newSendThrottleAttrs()) + + observer.syncs = nil + observer.forgotten = nil + if status, _ := callSQS(t, node, sqsDeleteQueueTarget, map[string]any{"QueueUrl": url}); status != http.StatusOK { + t.Fatalf("delete: %d", status) + } + require.Contains(t, observer.syncs, throttleSyncReport{queue: queue}) + require.ElementsMatch(t, []throttleForgetReport{ + {queue: queue, action: SQSThrottleActionSend}, + {queue: queue, action: SQSThrottleActionReceive}, + {queue: queue, action: SQSThrottleActionDefault}, + }, observer.forgotten) + + observer.syncs = nil + observer.forgotten = nil + _ = mustCreateQueue(t, node, queue) + require.Contains(t, observer.syncs, throttleSyncReport{queue: queue}) + require.ElementsMatch(t, []throttleForgetReport{ + {queue: queue, action: SQSThrottleActionSend}, + {queue: queue, action: SQSThrottleActionReceive}, + {queue: queue, action: SQSThrottleActionDefault}, + }, observer.forgotten) +} + // TestSQSServer_Throttle_DeleteQueueInvalidatesBucket pins the §3.1 // cache-invalidation contract for DeleteQueue: a same-name recreate // gets a fresh bucket, not the stale balance from the previous From c53b5bdc440bed2253441b372efe79850cb71512 Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 8 Jul 2026 18:35:10 +0900 Subject: [PATCH 12/16] adapter: keep SQS throttle gauges current --- adapter/sqs.go | 3 +- adapter/sqs_catalog.go | 52 ++++-------------------- adapter/sqs_throttle.go | 2 +- adapter/sqs_throttle_integration_test.go | 4 +- adapter/sqs_throttle_test.go | 16 +++----- 5 files changed, 18 insertions(+), 59 deletions(-) diff --git a/adapter/sqs.go b/adapter/sqs.go index e36b984f9..e331ab73d 100644 --- a/adapter/sqs.go +++ b/adapter/sqs.go @@ -295,11 +295,10 @@ func (s *SQSServer) observePartitionMessage(queue string, partition uint32, acti s.partitionObserver.ObservePartitionMessage(queue, partition, action) } -func (s *SQSServer) observeThrottleDecision(queue string, throttle *sqsQueueThrottle, outcome chargeOutcome) { +func (s *SQSServer) observeThrottleDecision(queue string, outcome chargeOutcome) { if s == nil || s.throttleObserver == nil { return } - s.observeThrottleConfig(queue, throttle) if !outcome.bucketPresent && outcome.allowed { return } diff --git a/adapter/sqs_catalog.go b/adapter/sqs_catalog.go index d73dfe973..7ae3c2cb4 100644 --- a/adapter/sqs_catalog.go +++ b/adapter/sqs_catalog.go @@ -817,42 +817,6 @@ func throttleConfigEqual(a, b *sqsQueueThrottle) bool { a.DefaultRefillPerSecond == b.DefaultRefillPerSecond } -func changedThrottleMetricActions(before, after *sqsQueueThrottle) []string { - changed := make([]string, 0, len(sqsThrottleMetricActions)) - for _, action := range sqsThrottleMetricActions { - if !throttleMetricActionEnabled(after, action) { - continue - } - beforeCapacity, beforeRefill := throttleMetricActionConfig(before, action) - afterCapacity, afterRefill := throttleMetricActionConfig(after, action) - if beforeCapacity != afterCapacity || beforeRefill != afterRefill { - changed = append(changed, action) - } - } - return changed -} - -func throttleMetricActionEnabled(throttle *sqsQueueThrottle, action string) bool { - capacity, _ := throttleMetricActionConfig(throttle, action) - return capacity > 0 -} - -func throttleMetricActionConfig(throttle *sqsQueueThrottle, action string) (float64, float64) { - if throttle == nil { - return 0, 0 - } - switch action { - case SQSThrottleActionSend: - return throttle.SendCapacity, throttle.SendRefillPerSecond - case SQSThrottleActionReceive: - return throttle.RecvCapacity, throttle.RecvRefillPerSecond - case SQSThrottleActionDefault: - return throttle.DefaultCapacity, throttle.DefaultRefillPerSecond - default: - return 0, 0 - } -} - // htfifoAttributesEqual compares the Phase 3.D HT-FIFO fields. // // PartitionCount normalisation: validatePartitionConfig documents 0 @@ -1566,10 +1530,11 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) { // full capacity. trySetQueueAttributesOnce therefore // compares the old and new throttle configs under the same Raft // read snapshot used for the commit and reports whether the values - // actually moved, plus the committed throttle config and still-enabled - // actions whose capacity/refill changed so the metrics layer can clear - // disabled token gauges and reset stale token gauges even if the queue - // goes quiet. The bucket reconciliation in loadOrInit also + // actually moved, plus the committed throttle config and post-enabled + // actions whose gauges must be reset because the queue-wide invalidation + // drops every bucket. This lets the metrics layer clear disabled token + // gauges and reset stale enabled-token gauges even if the queue goes + // quiet. The bucket reconciliation in loadOrInit also // catches a stale bucket if a throttle change slips past this gate // (e.g. via a future admin path), so the gating here is purely a // hot-path optimisation plus a no-op-bypass guard. @@ -1642,8 +1607,9 @@ func applyAndValidateSetAttributes(meta *sqsQueueMeta, attrs map[string]string) // meta's Throttle config differs from the pre-apply snapshot — the caller uses // it to gate the cache invalidation and metrics reconciliation so that a no-op // same-value SetQueueAttributes does not reset the bucket to full capacity or -// churn token gauges. resetActions is the still-enabled metric-action subset -// whose capacity/refill changed and whose stale token gauge must be removed. +// churn token gauges. resetActions is the still-enabled metric-action set +// whose in-memory bucket was dropped by the queue-wide invalidation and whose +// stale token gauge must be removed. func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, []string, bool, error) { readTS := s.nextTxnReadTS(ctx) meta, exists, err := s.loadQueueMetaAt(ctx, queueName, readTS) @@ -1666,7 +1632,7 @@ func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName str } throttleChanged := !throttleConfigEqual(preThrottle, meta.Throttle) postThrottle := snapshotThrottle(meta.Throttle) - resetActions := changedThrottleMetricActions(preThrottle, postThrottle) + resetActions := enabledThrottleMetricActions(postThrottle) meta.LastModifiedAtMillis = time.Now().UnixMilli() metaBytes, err := encodeSQSQueueMeta(meta) if err != nil { diff --git a/adapter/sqs_throttle.go b/adapter/sqs_throttle.go index 19750bcd7..c8ed87039 100644 --- a/adapter/sqs_throttle.go +++ b/adapter/sqs_throttle.go @@ -633,7 +633,7 @@ func (s *SQSServer) chargeQueueWithThrottle(w http.ResponseWriter, queueName, ac return true } outcome := s.throttle.charge(throttle, queueName, action, incarnation, count) - s.observeThrottleDecision(queueName, throttle, outcome) + s.observeThrottleDecision(queueName, outcome) if outcome.allowed { return true } diff --git a/adapter/sqs_throttle_integration_test.go b/adapter/sqs_throttle_integration_test.go index 49e75fbaa..2635ea35b 100644 --- a/adapter/sqs_throttle_integration_test.go +++ b/adapter/sqs_throttle_integration_test.go @@ -307,7 +307,7 @@ func TestSQSServer_Throttle_SetQueueAttributesSyncsDisabledMetrics(t *testing.T) enabled: []string{SQSThrottleActionSend, SQSThrottleActionReceive}, }) require.Contains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionSend}) - require.NotContains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionReceive}) + require.Contains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionReceive}) observer.syncs = nil observer.forgotten = nil @@ -320,7 +320,7 @@ func TestSQSServer_Throttle_SetQueueAttributesSyncsDisabledMetrics(t *testing.T) queue: "throttle-metric-sync", enabled: []string{SQSThrottleActionSend}, }) - require.NotContains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionSend}) + require.Contains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionSend}) require.Contains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionReceive}) require.Contains(t, observer.forgotten, throttleForgetReport{queue: "throttle-metric-sync", action: SQSThrottleActionDefault}) } diff --git a/adapter/sqs_throttle_test.go b/adapter/sqs_throttle_test.go index 4e88505e2..3e458a89f 100644 --- a/adapter/sqs_throttle_test.go +++ b/adapter/sqs_throttle_test.go @@ -108,7 +108,7 @@ func TestSQSServer_ChargeQueueWithThrottleObservesMetrics(t *testing.T) { require.InDelta(t, 0.0, last.tokensRemaining, 0.001) } -func TestSQSServer_ChargeQueueWithThrottleForgetsDisabledMetrics(t *testing.T) { +func TestSQSServer_ChargeQueueWithThrottleDoesNotSyncMetricActions(t *testing.T) { t.Parallel() observer := &recordingSQSThrottleObserver{} srv := NewSQSServer(nil, nil, nil, WithSQSThrottleObserver(observer)) @@ -116,21 +116,15 @@ func TestSQSServer_ChargeQueueWithThrottleForgetsDisabledMetrics(t *testing.T) { rec := httptest.NewRecorder() require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1)) - require.Contains(t, observer.syncs, throttleSyncReport{queue: "orders.fifo", enabled: []string{SQSThrottleActionSend}}) - require.NotContains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionSend}) - require.Contains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionReceive}) - require.Contains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionDefault}) + require.Empty(t, observer.syncs, "request-path throttle decisions must not sync enabled actions from a potentially stale meta snapshot") + require.Empty(t, observer.forgotten, "request-path throttle decisions must not forget gauges from a potentially stale meta snapshot") observer.forgotten = nil observer.syncs = nil rec = httptest.NewRecorder() require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, nil, 1)) - require.Contains(t, observer.syncs, throttleSyncReport{queue: "orders.fifo"}) - require.ElementsMatch(t, []throttleForgetReport{ - {queue: "orders.fifo", action: SQSThrottleActionSend}, - {queue: "orders.fifo", action: SQSThrottleActionReceive}, - {queue: "orders.fifo", action: SQSThrottleActionDefault}, - }, observer.forgotten) + require.Empty(t, observer.syncs) + require.Empty(t, observer.forgotten) } // TestBucketStore_Empty_ShortCircuit covers the post-validator From 0c5cc4f8ca331ac9864586e0f273aac8bf5e792d Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 8 Jul 2026 19:09:49 +0900 Subject: [PATCH 13/16] monitoring: guard SQS throttle gauge lifecycle --- adapter/sqs_messages.go | 6 ++-- adapter/sqs_throttle.go | 63 ++++++++++++++++++++++++++------- adapter/sqs_throttle_test.go | 31 ++++++++++++++--- monitoring/sqs.go | 52 ++++++++++++++++------------ monitoring/sqs_test.go | 67 ++++++++++++++++++++++++++++++------ 5 files changed, 168 insertions(+), 51 deletions(-) diff --git a/adapter/sqs_messages.go b/adapter/sqs_messages.go index 086cdb830..0e00c58b9 100644 --- a/adapter/sqs_messages.go +++ b/adapter/sqs_messages.go @@ -474,12 +474,13 @@ func (s *SQSServer) prepareSendMessage(w http.ResponseWriter, r *http.Request) ( // OCC transaction (§4.2): a rejected request never reaches the // coordinator. func (s *SQSServer) validateSend(w http.ResponseWriter, r *http.Request, queueName string, in sqsSendMessageInput) (*sqsQueueMeta, uint64, int64, bool) { + throttleEpoch := s.throttle.queueEpoch(queueName) meta, readTS, apiErr := s.loadQueueMetaForSend(r.Context(), queueName, []byte(in.MessageBody)) if apiErr != nil { writeSQSErrorFromErr(w, apiErr) return nil, 0, 0, false } - if !s.chargeQueueWithThrottle(w, queueName, bucketActionSend, 1, meta.Throttle, meta.Incarnation) { + if !s.chargeQueueWithThrottle(w, queueName, bucketActionSend, 1, meta.Throttle, meta.Incarnation, throttleEpoch) { return nil, 0, 0, false } if apiErr := validateMessageAttributes(in.MessageAttributes); apiErr != nil { @@ -759,6 +760,7 @@ func (s *SQSServer) receiveMessage(w http.ResponseWriter, r *http.Request) { return } + throttleEpoch := s.throttle.queueEpoch(queueName) readTS := s.nextTxnReadTS(ctx) meta, exists, err := s.loadQueueMetaAt(ctx, queueName, readTS) if err != nil { @@ -773,7 +775,7 @@ func (s *SQSServer) receiveMessage(w http.ResponseWriter, r *http.Request) { // don't pay an extra meta read just to discover throttling is // off. Sits AFTER the QueueDoesNotExist branch — a missing queue // should not consume a Recv token. - if !s.chargeQueueWithThrottle(w, queueName, bucketActionReceive, 1, meta.Throttle, meta.Incarnation) { + if !s.chargeQueueWithThrottle(w, queueName, bucketActionReceive, 1, meta.Throttle, meta.Incarnation, throttleEpoch) { return } max, maxErr := resolveReceiveMaxMessages(in.MaxNumberOfMessages) diff --git a/adapter/sqs_throttle.go b/adapter/sqs_throttle.go index c8ed87039..f66d94955 100644 --- a/adapter/sqs_throttle.go +++ b/adapter/sqs_throttle.go @@ -5,6 +5,7 @@ import ( "math" "net/http" "sync" + "sync/atomic" "time" "github.com/cockroachdb/errors" @@ -131,6 +132,7 @@ type tokenBucket struct { // caller of sweep() is the sole goroutine the ticker drives. type bucketStore struct { buckets sync.Map // map[bucketKey]*tokenBucket + queueEpochs sync.Map // map[string]*atomic.Uint64 clock func() time.Time evictedAfter time.Duration sweepEvery time.Duration @@ -159,6 +161,35 @@ func newBucketStoreDefault() *bucketStore { return newBucketStore(time.Now, throttleIdleEvictAfter) } +func (b *bucketStore) queueEpoch(queue string) uint64 { + if b == nil || queue == "" { + return 0 + } + v, ok := b.queueEpochs.Load(queue) + if !ok { + return 0 + } + epoch, _ := v.(*atomic.Uint64) + if epoch == nil { + return 0 + } + return epoch.Load() +} + +func (b *bucketStore) bumpQueueEpoch(queue string) { + if b == nil || queue == "" { + return + } + b.queueEpochCell(queue).Add(1) +} + +func (b *bucketStore) queueEpochCell(queue string) *atomic.Uint64 { + fresh := &atomic.Uint64{} + actual, _ := b.queueEpochs.LoadOrStore(queue, fresh) + epoch, _ := actual.(*atomic.Uint64) + return epoch +} + // chargeOutcome is returned from charge so the caller can build the // Throttling envelope (Retry-After computed from refillRate + // requestedCount, see §3.4) without re-loading the bucket. @@ -387,6 +418,7 @@ func (b *bucketStore) invalidateQueue(queue string) { if b == nil { return } + b.bumpQueueEpoch(queue) // Incarnation participates in the key: we do // not know which incarnations have buckets cached, so range the map // and remove any entry whose queue matches. A SetQueueAttributes @@ -601,6 +633,7 @@ func (s *SQSServer) chargeQueue(w http.ResponseWriter, r *http.Request, queueNam if s.throttle == nil { return true } + throttleEpoch := s.throttle.queueEpoch(queueName) throttle, incarnation, err := s.queueThrottleConfig(r, queueName) if err != nil { // Fail closed on a transient storage error. Earlier code @@ -614,26 +647,30 @@ func (s *SQSServer) chargeQueue(w http.ResponseWriter, r *http.Request, queueNam writeSQSErrorFromErr(w, err) return false } - return s.chargeQueueWithThrottle(w, queueName, action, count, throttle, incarnation) + return s.chargeQueueWithThrottle(w, queueName, action, count, throttle, incarnation, throttleEpoch) } -// chargeQueueWithThrottle is the variant for handlers that already -// have the throttle config in hand from their own meta load. Drops -// the per-request meta load chargeQueue does, avoiding redundant -// storage reads on the hot path. incarnation is -// sqsQueueMeta.Incarnation: it must come from the same meta snapshot -// the throttle config was read from so a recreate committed -// mid-request lands in a fresh bucket on the next call rather than -// mixing tokens with the prior incarnation. NOTE: meta.Incarnation, -// NOT meta.Generation — PurgeQueue bumps Generation but preserves -// Incarnation, so keying the bucket by Generation would let a caller +// chargeQueueWithThrottle is the variant for handlers that already have the +// throttle config in hand from their own meta load. Drops the per-request meta +// load chargeQueue does, avoiding redundant storage reads on the hot path. +// incarnation is sqsQueueMeta.Incarnation: it must come from the same meta +// snapshot the throttle config was read from so a recreate committed mid-request +// lands in a fresh bucket on the next call rather than mixing tokens with the +// prior incarnation. throttleEpoch must be captured before the same meta read; +// if SetQueueAttributes/Delete/Create invalidates the queue after that snapshot, +// the request result is still honoured but the stale token-balance metric is not +// allowed to recreate a gauge the config path already reset. NOTE: +// meta.Incarnation, NOT meta.Generation — PurgeQueue bumps Generation but +// preserves Incarnation, so keying the bucket by Generation would let a caller // bypass the rate limit by repeatedly purging. -func (s *SQSServer) chargeQueueWithThrottle(w http.ResponseWriter, queueName, action string, count int, throttle *sqsQueueThrottle, incarnation uint64) bool { +func (s *SQSServer) chargeQueueWithThrottle(w http.ResponseWriter, queueName, action string, count int, throttle *sqsQueueThrottle, incarnation uint64, throttleEpoch uint64) bool { if s.throttle == nil { return true } outcome := s.throttle.charge(throttle, queueName, action, incarnation, count) - s.observeThrottleDecision(queueName, outcome) + if s.throttle.queueEpoch(queueName) == throttleEpoch { + s.observeThrottleDecision(queueName, outcome) + } if outcome.allowed { return true } diff --git a/adapter/sqs_throttle_test.go b/adapter/sqs_throttle_test.go index 3e458a89f..ec683a948 100644 --- a/adapter/sqs_throttle_test.go +++ b/adapter/sqs_throttle_test.go @@ -94,10 +94,10 @@ func TestSQSServer_ChargeQueueWithThrottleObservesMetrics(t *testing.T) { for range 10 { rec := httptest.NewRecorder() - require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1)) + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1, 0)) } rec := httptest.NewRecorder() - require.False(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1)) + require.False(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1, 0)) require.Equal(t, http.StatusBadRequest, rec.Code) require.Len(t, observer.reports, 11) @@ -115,18 +115,41 @@ func TestSQSServer_ChargeQueueWithThrottleDoesNotSyncMetricActions(t *testing.T) cfg := &sqsQueueThrottle{SendCapacity: 10, SendRefillPerSecond: 1} rec := httptest.NewRecorder() - require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1)) + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1, 0)) require.Empty(t, observer.syncs, "request-path throttle decisions must not sync enabled actions from a potentially stale meta snapshot") require.Empty(t, observer.forgotten, "request-path throttle decisions must not forget gauges from a potentially stale meta snapshot") observer.forgotten = nil observer.syncs = nil rec = httptest.NewRecorder() - require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, nil, 1)) + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, nil, 1, 0)) require.Empty(t, observer.syncs) require.Empty(t, observer.forgotten) } +func TestSQSServer_ChargeQueueWithThrottleSkipsStaleMetricAfterInvalidate(t *testing.T) { + t.Parallel() + observer := &recordingSQSThrottleObserver{} + srv := NewSQSServer(nil, nil, nil, WithSQSThrottleObserver(observer)) + now := time.Date(2026, 4, 27, 10, 0, 0, 0, time.UTC) + srv.throttle = newBucketStore(func() time.Time { return now }, throttleIdleEvictAfter) + cfg := &sqsQueueThrottle{SendCapacity: 10, SendRefillPerSecond: 1} + + staleEpoch := srv.throttle.queueEpoch("orders.fifo") + srv.throttle.invalidateQueue("orders.fifo") + + rec := httptest.NewRecorder() + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1, staleEpoch)) + require.Empty(t, observer.reports, "stale pre-invalidation snapshot must not recreate a reset token gauge") + + freshEpoch := srv.throttle.queueEpoch("orders.fifo") + rec = httptest.NewRecorder() + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1, freshEpoch)) + require.Len(t, observer.reports, 1, "fresh post-invalidation snapshot may publish the new token gauge") + require.Equal(t, "orders.fifo", observer.reports[0].queue) + require.Equal(t, SQSThrottleActionSend, observer.reports[0].action) +} + // TestBucketStore_Empty_ShortCircuit covers the post-validator // canonicalisation path: an all-zero sqsQueueThrottle is equivalent // to nil. Without this branch, a queue whose operator wrote diff --git a/monitoring/sqs.go b/monitoring/sqs.go index 79948f20c..b99cba6a2 100644 --- a/monitoring/sqs.go +++ b/monitoring/sqs.go @@ -129,10 +129,10 @@ type SQSQueueDepth struct { // queue's slot in trackedDepthQueues so a churn-heavy deployment // can reuse the budget. // - throttleTokens is also a GaugeVec, but decisions are event- -// driven and action-labelled. It tracks the active actions per -// queue so disabling one bucket can drop just that action's -// gauge while preserving other configured actions for the same -// queue. +// driven and action-labelled. It has a separate queue/action +// lifetime from the depth scraper: throttle config changes or +// throttle-only cleanup drop token gauges, but transient depth +// scan misses must not delete them. // // Sharing one map across counters and gauges regresses the counter cap: // ForgetQueue would free a slot, a new queue would be admitted, and @@ -344,11 +344,10 @@ func (m *SQSMetrics) SyncThrottleActions(queue string, enabledActions []string) m.mu.Unlock() } -// ForgetQueue drops the three gauge series for a queue and frees -// its slot in the depth-side cardinality budget so a long-running -// deployment that regularly creates and deletes queues (CI -// workloads, ephemeral per-job queues) doesn't permanently wedge -// the 512-entry depth budget. +// ForgetQueue drops the three depth gauge series for a queue and frees its slot +// in the depth-side cardinality budget so a long-running deployment that +// regularly creates and deletes queues (CI workloads, ephemeral per-job queues) +// doesn't permanently wedge the 512-entry depth budget. // // Three cases, by membership at call time: // @@ -378,27 +377,33 @@ func (m *SQSMetrics) SyncThrottleActions(queue string, enabledActions []string) // have since been deleted. // // Caller-audit per the standing semantic-change rule: only -// SQSObserver.observeOnce calls this (registry plumbing aside), -// and it's invoked exactly when a queue is observed in the -// previous tick but not the current one. The caller's contract — -// "drop gauges for a queue that disappeared so dashboards don't -// show frozen backlog" — is preserved AND extended consistently: -// overflow queues, previously a silent no-op, now also stop -// pinning the shared gauge once the last one disappears. The -// narrowed-but-still-correct scope (counter budget never -// reclaimed) is invisible to observeOnce because the observer -// only writes gauges; counters are fed via ObservePartitionMessage -// from a different code path entirely. +// SQSObserver.observeOnce calls this (registry plumbing aside), and it is invoked +// exactly when a queue was observed in the previous tick but not the current one. +// The caller's depth contract is preserved, including overflow ref-counting, but +// throttle token gauges are intentionally out of scope: a per-queue depth scan +// miss is not evidence that throttle config changed. func (m *SQSMetrics) ForgetQueue(queue string) { if m == nil || queue == "" { return } m.mu.Lock() depthForget := m.forgetDepthQueueLocked(queue) + m.mu.Unlock() + m.dropForgottenDepthQueue(queue, depthForget) +} + +// ForgetThrottleQueue drops every token-balance gauge for queue and frees its +// throttle-gauge budget slot. It is intentionally separate from ForgetQueue +// because queue-depth snapshots can omit a live queue on a transient per-queue +// scan error. +func (m *SQSMetrics) ForgetThrottleQueue(queue string) { + if m == nil || queue == "" { + return + } + m.mu.Lock() throttleForget := m.forgetThrottleQueueLocked(queue) m.dropForgottenThrottleQueue(queue, throttleForget) m.mu.Unlock() - m.dropForgottenDepthQueue(queue, depthForget) } type sqsGaugeForget struct { @@ -826,7 +831,10 @@ func (o *SQSObserver) forgetMissingQueuesLocked(current map[string]struct{}, thr } for _, prev := range o.metrics.snapshotThrottleGaugeQueues(throttleCutoff) { if _, ok := current[prev]; !ok { - o.metrics.ForgetQueue(prev) + if _, wasDepthSeen := o.lastSeen[prev]; wasDepthSeen { + continue + } + o.metrics.ForgetThrottleQueue(prev) } } } diff --git a/monitoring/sqs_test.go b/monitoring/sqs_test.go index 4f7c305e5..0f62104fb 100644 --- a/monitoring/sqs_test.go +++ b/monitoring/sqs_test.go @@ -347,7 +347,7 @@ func TestSQSMetrics_SyncThrottleActions_DropsDisabledWithoutTraffic(t *testing.T require.Empty(t, m.trackedThrottleGaugeQueues, "disabling the last action frees the gauge slot") } -func TestSQSMetrics_ForgetQueue_OverflowThrottleGaugeClearsPerAction(t *testing.T) { +func TestSQSMetrics_ForgetThrottleQueue_OverflowThrottleGaugeClearsPerAction(t *testing.T) { t.Parallel() reg := prometheus.NewRegistry() m := newSQSMetrics(reg) @@ -373,7 +373,7 @@ func TestSQSMetrics_ForgetQueue_OverflowThrottleGaugeClearsPerAction(t *testing. require.True(t, found) require.InDelta(t, 20.0, receive, 0.001) - m.ForgetQueue("overflow-a.fifo") + m.ForgetThrottleQueue("overflow-a.fifo") _, found = gatheredThrottleTokenValue(t, reg, map[string]string{ "queue": sqsQueueOverflow, "action": SQSThrottleActionSend, @@ -386,7 +386,7 @@ func TestSQSMetrics_ForgetQueue_OverflowThrottleGaugeClearsPerAction(t *testing. require.True(t, found, "_other/receive must survive while another overflow receive queue remains") require.InDelta(t, 20.0, receive, 0.001) - m.ForgetQueue("overflow-b.fifo") + m.ForgetThrottleQueue("overflow-b.fifo") _, found = gatheredThrottleTokenValue(t, reg, map[string]string{ "queue": sqsQueueOverflow, "action": SQSThrottleActionReceive, @@ -442,11 +442,10 @@ func TestSQSMetrics_ObserveQueueDepth_DropsEmptyQueue(t *testing.T) { } // TestSQSMetrics_ForgetQueue_DropsThreeSeries pins that ForgetQueue -// (a) removes all three state-labelled series so a deleted queue -// stops reporting a frozen backlog, (b) frees the cardinality- -// budget slot so a churn-heavy deployment doesn't permanently -// exhaust the 512-entry budget, and (c) leaves the -// (queue, partition, action) counter alone (cumulative-by-design). +// (a) removes all three state-labelled depth series so a deleted queue stops +// reporting a frozen backlog, (b) frees the depth cardinality-budget slot so a +// churn-heavy deployment doesn't permanently exhaust the 512-entry budget, and +// (c) leaves cumulative counters and throttle-token gauges alone. func TestSQSMetrics_ForgetQueue_DropsThreeSeries(t *testing.T) { t.Parallel() reg := prometheus.NewRegistry() @@ -466,9 +465,9 @@ func TestSQSMetrics_ForgetQueue_DropsThreeSeries(t *testing.T) { require.Equal(t, 0, count, "ForgetQueue must drop every state series for the queue") throttleGaugeCount, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") require.NoError(t, err) - require.Equal(t, 0, throttleGaugeCount, "ForgetQueue must drop every throttle-token gauge for the queue") + require.Equal(t, 1, throttleGaugeCount, "ForgetQueue must not drop throttle-token gauges for a depth-only miss") require.Empty(t, m.trackedDepthQueues, "ForgetQueue must free the depth-side cardinality slot") - require.Empty(t, m.trackedThrottleGaugeQueues, "ForgetQueue must free the throttle-gauge cardinality slot") + require.Len(t, m.trackedThrottleGaugeQueues, 1, "ForgetQueue must not free the throttle-gauge cardinality slot") require.Len(t, m.trackedCounterQueues, 1, "ForgetQueue must NOT free the counter-side slot (counters are cumulative)") require.InDelta(t, 1.0, testutil.ToFloat64(m.partitionMessages.WithLabelValues("orders.fifo", "0", SQSPartitionActionSend)), 0.001, "counter must survive ForgetQueue (cumulative metric)") @@ -484,6 +483,25 @@ func TestSQSMetrics_ForgetQueue_DropsThreeSeries(t *testing.T) { "post-forget Observe must re-emit under the real queue name") } +func TestSQSMetrics_ForgetThrottleQueue_DropsTokenGauges(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 4, true) + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionReceive, 7, false) + require.Len(t, m.trackedThrottleGaugeQueues, 1) + + m.ForgetThrottleQueue("orders.fifo") + + count, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 0, count, "ForgetThrottleQueue must drop every token gauge for the queue") + require.Empty(t, m.trackedThrottleGaugeQueues, "ForgetThrottleQueue must free the throttle-gauge cardinality slot") + require.InDelta(t, 1.0, testutil.ToFloat64(m.throttledRequests.WithLabelValues("orders.fifo", SQSThrottleActionSend)), 0.001, + "throttled-request counter must survive ForgetThrottleQueue (cumulative metric)") +} + // TestSQSMetrics_ForgetQueue_OverflowQueueIsNoOp pins the // overflow-collision safety: a queue that hit the cardinality cap // and got collapsed onto the _other label has no individual series @@ -920,6 +938,35 @@ func TestSQSObserver_ObserveOnce_PreservesThrottleGaugeNewerThanDepthSnapshot(t require.Equal(t, 0, count, "the next successful depth tick can clear the now-old throttle-only gauge") } +func TestSQSObserver_ObserveOnce_DepthMissPreservesThrottleGauge(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + obs := newSQSObserver(m) + + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 4, false) + source := &fakeDepthSource{ + ticks: []fakeDepthTick{ + {snaps: []SQSQueueDepth{{Queue: "orders.fifo", Visible: 1}}, ok: true}, + {snaps: nil, ok: true}, + }, + } + + obs.ObserveOnce(source) + obs.ObserveOnce(source) + + depthCount, err := testutil.GatherAndCount(reg, "elastickv_sqs_queue_messages") + require.NoError(t, err) + require.Equal(t, 0, depthCount, "depth miss still clears depth gauges through ForgetQueue") + value, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "orders.fifo", + "action": SQSThrottleActionSend, + }) + require.True(t, found, "depth-only miss must not delete throttle-token gauges") + require.InDelta(t, 4.0, value, 0.001) + require.Len(t, m.trackedThrottleGaugeQueues, 1, "depth-only miss must not reclaim the throttle-gauge budget slot") +} + // TestSQSObserver_ObserveOnce_TransientScanErrorPreservesGauges // pins the P2 fix from PR #743 r6: when the source returns // ok=false (transient catalog-scan failure on the leader, ctx From 43c5a8a68b794582d23a819a02a2253074e5f022 Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 8 Jul 2026 19:47:30 +0900 Subject: [PATCH 14/16] monitoring: harden SQS throttle gauge cleanup --- adapter/sqs.go | 42 ++++++++- adapter/sqs_admin.go | 3 +- adapter/sqs_catalog.go | 6 +- adapter/sqs_throttle.go | 58 ++++++++++--- adapter/sqs_throttle_test.go | 53 +++++++++++- monitoring/sqs.go | 160 +++++++++++++++++++++++++++++++++-- monitoring/sqs_test.go | 74 ++++++++++++++++ 7 files changed, 368 insertions(+), 28 deletions(-) diff --git a/adapter/sqs.go b/adapter/sqs.go index e331ab73d..9b7e551d2 100644 --- a/adapter/sqs.go +++ b/adapter/sqs.go @@ -232,6 +232,18 @@ type SQSThrottleObserver interface { SyncThrottleActions(queue string, enabledActions []string) } +type sqsThrottleRejectionObserver interface { + ObserveThrottleRejection(queue string, action string) +} + +type sqsThrottleGaugeCutoffObserver interface { + ThrottleGaugeSnapshotCutoff() uint64 +} + +type sqsThrottleGaugeResetObserver interface { + ForgetThrottleActionBefore(queue string, action string, cutoff uint64) +} + // SQS metric action labels mirror the values from monitoring/sqs.go. // Re-declared so adapter call sites do not need a monitoring import; the // observer interface validates the value at runtime so a drift between these @@ -295,16 +307,25 @@ func (s *SQSServer) observePartitionMessage(queue string, partition uint32, acti s.partitionObserver.ObservePartitionMessage(queue, partition, action) } -func (s *SQSServer) observeThrottleDecision(queue string, outcome chargeOutcome) { +func (s *SQSServer) observeThrottleDecision(queue string, outcome chargeOutcome, observeTokens bool) { if s == nil || s.throttleObserver == nil { return } if !outcome.bucketPresent && outcome.allowed { return } + action := sqsThrottleMetricAction(outcome.action) + if !observeTokens { + if !outcome.allowed { + if observer, ok := s.throttleObserver.(sqsThrottleRejectionObserver); ok { + observer.ObserveThrottleRejection(queue, action) + } + } + return + } s.throttleObserver.ObserveThrottleDecision( queue, - sqsThrottleMetricAction(outcome.action), + action, outcome.tokensAfter, !outcome.allowed, ) @@ -317,16 +338,31 @@ func (s *SQSServer) observeThrottleConfig(queue string, throttle *sqsQueueThrott s.throttleObserver.SyncThrottleActions(queue, enabledThrottleMetricActions(throttle)) } -func (s *SQSServer) observeThrottleConfigChange(queue string, throttle *sqsQueueThrottle, resetActions []string) { +func (s *SQSServer) observeThrottleConfigChange(queue string, throttle *sqsQueueThrottle, resetActions []string, resetCutoff uint64) { if s == nil || s.throttleObserver == nil { return } s.throttleObserver.SyncThrottleActions(queue, enabledThrottleMetricActions(throttle)) for _, action := range resetActions { + if observer, ok := s.throttleObserver.(sqsThrottleGaugeResetObserver); ok { + observer.ForgetThrottleActionBefore(queue, action, resetCutoff) + continue + } s.throttleObserver.ForgetThrottleAction(queue, action) } } +func (s *SQSServer) throttleGaugeSnapshotCutoff() uint64 { + if s == nil || s.throttleObserver == nil { + return 0 + } + observer, ok := s.throttleObserver.(sqsThrottleGaugeCutoffObserver) + if !ok { + return 0 + } + return observer.ThrottleGaugeSnapshotCutoff() +} + func enabledThrottleMetricActions(throttle *sqsQueueThrottle) []string { if throttle == nil || throttle.IsEmpty() { return nil diff --git a/adapter/sqs_admin.go b/adapter/sqs_admin.go index 32a91c1c8..94dba14ca 100644 --- a/adapter/sqs_admin.go +++ b/adapter/sqs_admin.go @@ -364,8 +364,9 @@ func (s *SQSServer) AdminSetQueueAttributes(ctx context.Context, principal Admin return errors.Wrap(err, "admin set queue attributes") } if throttleChanged { + throttleResetCutoff := s.throttleGaugeSnapshotCutoff() s.throttle.invalidateQueue(name) - s.observeThrottleConfigChange(name, throttle, resetActions) + s.observeThrottleConfigChange(name, throttle, resetActions, throttleResetCutoff) } return nil } diff --git a/adapter/sqs_catalog.go b/adapter/sqs_catalog.go index 7ae3c2cb4..a44a11c8d 100644 --- a/adapter/sqs_catalog.go +++ b/adapter/sqs_catalog.go @@ -1049,8 +1049,9 @@ func (s *SQSServer) tryCreateQueueOnce(ctx context.Context, requested *sqsQueueM // return path above, which exits before this point) guarantees // the new queue starts with a fresh full-capacity bucket // regardless of in-flight traffic to the prior incarnation. + throttleResetCutoff := s.throttleGaugeSnapshotCutoff() s.throttle.invalidateQueue(requested.Name) - s.observeThrottleConfigChange(requested.Name, requested.Throttle, enabledThrottleMetricActions(requested.Throttle)) + s.observeThrottleConfigChange(requested.Name, requested.Throttle, enabledThrottleMetricActions(requested.Throttle), throttleResetCutoff) // Mirror the throttle invalidate for the per-queue fanout-rotation // counter. A delete-then-create race could otherwise leave the // new queue starting partitioned receives at the previous @@ -1539,8 +1540,9 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) { // (e.g. via a future admin path), so the gating here is purely a // hot-path optimisation plus a no-op-bypass guard. if throttleChanged { + throttleResetCutoff := s.throttleGaugeSnapshotCutoff() s.throttle.invalidateQueue(name) - s.observeThrottleConfigChange(name, throttle, resetActions) + s.observeThrottleConfigChange(name, throttle, resetActions, throttleResetCutoff) } writeSQSJSON(w, map[string]any{}) } diff --git a/adapter/sqs_throttle.go b/adapter/sqs_throttle.go index f66d94955..bcd4cee2a 100644 --- a/adapter/sqs_throttle.go +++ b/adapter/sqs_throttle.go @@ -123,6 +123,11 @@ type tokenBucket struct { evicted bool } +type bucketQueueEpoch struct { + epoch atomic.Uint64 + updatedUnixNano atomic.Int64 +} + // bucketStore holds every active bucket for an SQS server process. // sync.Map matches the read-mostly access pattern: lookups are nearly // always Load hits; LoadOrStore pays the write cost only on first use. @@ -132,7 +137,8 @@ type tokenBucket struct { // caller of sweep() is the sole goroutine the ticker drives. type bucketStore struct { buckets sync.Map // map[bucketKey]*tokenBucket - queueEpochs sync.Map // map[string]*atomic.Uint64 + queueEpochMu sync.Mutex + queueEpochs sync.Map // map[string]*bucketQueueEpoch clock func() time.Time evictedAfter time.Duration sweepEvery time.Duration @@ -169,24 +175,29 @@ func (b *bucketStore) queueEpoch(queue string) uint64 { if !ok { return 0 } - epoch, _ := v.(*atomic.Uint64) + epoch, _ := v.(*bucketQueueEpoch) if epoch == nil { return 0 } - return epoch.Load() + return epoch.epoch.Load() } func (b *bucketStore) bumpQueueEpoch(queue string) { if b == nil || queue == "" { return } - b.queueEpochCell(queue).Add(1) + b.queueEpochMu.Lock() + defer b.queueEpochMu.Unlock() + cell := b.queueEpochCellLocked(queue) + cell.epoch.Add(1) + cell.updatedUnixNano.Store(b.clock().UnixNano()) } -func (b *bucketStore) queueEpochCell(queue string) *atomic.Uint64 { - fresh := &atomic.Uint64{} +func (b *bucketStore) queueEpochCellLocked(queue string) *bucketQueueEpoch { + fresh := &bucketQueueEpoch{} + fresh.updatedUnixNano.Store(b.clock().UnixNano()) actual, _ := b.queueEpochs.LoadOrStore(queue, fresh) - epoch, _ := actual.(*atomic.Uint64) + epoch, _ := actual.(*bucketQueueEpoch) return epoch } @@ -497,17 +508,46 @@ func (b *bucketStore) runSweepLoop(ctx context.Context) { // bucket.mu, so there is no AB-BA cycle with charge(). func (b *bucketStore) sweep() { cutoff := b.clock().Add(-b.evictedAfter) + queuesWithBuckets := map[string]struct{}{} b.buckets.Range(func(k, v any) bool { + key, _ := k.(bucketKey) bucket, _ := v.(*tokenBucket) bucket.mu.Lock() if bucket.lastRefill.Before(cutoff) { if b.buckets.CompareAndDelete(k, v) { bucket.evicted = true } + } else { + queuesWithBuckets[key.queue] = struct{}{} } bucket.mu.Unlock() return true }) + b.sweepQueueEpochs(cutoff, queuesWithBuckets) +} + +func (b *bucketStore) sweepQueueEpochs(cutoff time.Time, queuesWithBuckets map[string]struct{}) { + if b == nil || b.evictedAfter <= 0 { + return + } + b.queueEpochMu.Lock() + defer b.queueEpochMu.Unlock() + b.queueEpochs.Range(func(k, v any) bool { + queue, _ := k.(string) + if _, ok := queuesWithBuckets[queue]; ok { + return true + } + cell, _ := v.(*bucketQueueEpoch) + if cell == nil { + b.queueEpochs.Delete(k) + return true + } + updated := time.Unix(0, cell.updatedUnixNano.Load()) + if updated.Before(cutoff) || updated.Equal(cutoff) { + b.queueEpochs.Delete(k) + } + return true + }) } // resolveActionConfig maps a charge() action to (effective bucket @@ -668,9 +708,7 @@ func (s *SQSServer) chargeQueueWithThrottle(w http.ResponseWriter, queueName, ac return true } outcome := s.throttle.charge(throttle, queueName, action, incarnation, count) - if s.throttle.queueEpoch(queueName) == throttleEpoch { - s.observeThrottleDecision(queueName, outcome) - } + s.observeThrottleDecision(queueName, outcome, s.throttle.queueEpoch(queueName) == throttleEpoch) if outcome.allowed { return true } diff --git a/adapter/sqs_throttle_test.go b/adapter/sqs_throttle_test.go index ec683a948..20d3f2656 100644 --- a/adapter/sqs_throttle_test.go +++ b/adapter/sqs_throttle_test.go @@ -29,9 +29,10 @@ type throttleSyncReport struct { } type recordingSQSThrottleObserver struct { - reports []throttleObserveReport - forgotten []throttleForgetReport - syncs []throttleSyncReport + reports []throttleObserveReport + rejections []throttleForgetReport + forgotten []throttleForgetReport + syncs []throttleSyncReport } func (r *recordingSQSThrottleObserver) ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) { @@ -43,6 +44,10 @@ func (r *recordingSQSThrottleObserver) ObserveThrottleDecision(queue string, act }) } +func (r *recordingSQSThrottleObserver) ObserveThrottleRejection(queue string, action string) { + r.rejections = append(r.rejections, throttleForgetReport{queue: queue, action: action}) +} + func (r *recordingSQSThrottleObserver) ForgetThrottleAction(queue string, action string) { r.forgotten = append(r.forgotten, throttleForgetReport{queue: queue, action: action}) } @@ -150,6 +155,23 @@ func TestSQSServer_ChargeQueueWithThrottleSkipsStaleMetricAfterInvalidate(t *tes require.Equal(t, SQSThrottleActionSend, observer.reports[0].action) } +func TestSQSServer_ChargeQueueWithThrottleCountsStaleRejectionAfterInvalidate(t *testing.T) { + t.Parallel() + observer := &recordingSQSThrottleObserver{} + srv := NewSQSServer(nil, nil, nil, WithSQSThrottleObserver(observer)) + now := time.Date(2026, 4, 27, 10, 0, 0, 0, time.UTC) + srv.throttle = newBucketStore(func() time.Time { return now }, throttleIdleEvictAfter) + cfg := &sqsQueueThrottle{SendCapacity: 1, SendRefillPerSecond: 1} + + staleEpoch := srv.throttle.queueEpoch("orders.fifo") + srv.throttle.invalidateQueue("orders.fifo") + + rec := httptest.NewRecorder() + require.False(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 2, cfg, 1, staleEpoch)) + require.Empty(t, observer.reports, "stale pre-invalidation snapshot must not recreate a reset token gauge") + require.Equal(t, []throttleForgetReport{{queue: "orders.fifo", action: SQSThrottleActionSend}}, observer.rejections) +} + // TestBucketStore_Empty_ShortCircuit covers the post-validator // canonicalisation path: an all-zero sqsQueueThrottle is equivalent // to nil. Without this branch, a queue whose operator wrote @@ -767,6 +789,22 @@ func TestBucketStore_InvalidateQueueClearsAllIncarnations(t *testing.T) { require.True(t, hasEvents, "unrelated queue must not be evicted") } +func TestBucketStore_SweepEvictsIdleQueueEpochs(t *testing.T) { + t.Parallel() + now := time.Date(2026, 4, 27, 10, 0, 0, 0, time.UTC) + store := newBucketStore(func() time.Time { return now }, time.Minute) + + store.invalidateQueue("orders") + require.Equal(t, uint64(1), store.queueEpoch("orders")) + require.Equal(t, 1, countQueueEpochs(store)) + + now = now.Add(2 * time.Minute) + store.sweep() + + require.Equal(t, uint64(0), store.queueEpoch("orders")) + require.Equal(t, 0, countQueueEpochs(store), "idle epoch cells must not grow without bound under queue churn") +} + // TestBucketStore_PurgeQueueDoesNotResetBucket pins the // PurgeQueue-bypass guard: PurgeQueue bumps sqsQueueMeta.Generation // but preserves Incarnation, and the throttle bucket keys by Incarnation. @@ -809,6 +847,15 @@ func countBuckets(b *bucketStore) int { return n } +func countQueueEpochs(b *bucketStore) int { + n := 0 + b.queueEpochs.Range(func(_, _ any) bool { + n++ + return true + }) + return n +} + func TestBucketStore_InvalidateQueueDropsAllActions(t *testing.T) { t.Parallel() cfg := &sqsQueueThrottle{ diff --git a/monitoring/sqs.go b/monitoring/sqs.go index b99cba6a2..cf45648ee 100644 --- a/monitoring/sqs.go +++ b/monitoring/sqs.go @@ -152,6 +152,7 @@ type SQSMetrics struct { trackedDepthQueues map[string]struct{} trackedThrottleGaugeQueues map[string]map[string]struct{} throttleGaugeQueueSeq map[string]uint64 + throttleGaugeActionSeq map[string]map[string]uint64 throttleGaugeSeq uint64 // overflowDepthQueues is the set of queue names whose depth // gauge is currently collapsed onto the shared sqsQueueOverflow @@ -214,6 +215,7 @@ func newSQSMetrics(registerer prometheus.Registerer) *SQSMetrics { trackedDepthQueues: map[string]struct{}{}, trackedThrottleGaugeQueues: map[string]map[string]struct{}{}, throttleGaugeQueueSeq: map[string]uint64{}, + throttleGaugeActionSeq: map[string]map[string]uint64{}, overflowDepthQueues: map[string]struct{}{}, overflowThrottleGaugeQueues: map[string]map[string]struct{}{}, } @@ -293,6 +295,20 @@ func (m *SQSMetrics) ObserveThrottleDecision(queue string, action string, tokens m.mu.Unlock() } +// ObserveThrottleRejection increments only the rejected-request counter. The +// adapter uses it when a stale pre-invalidation request still returns +// Throttling to the client but must not recreate a token-balance gauge. +func (m *SQSMetrics) ObserveThrottleRejection(queue string, action string) { + if m == nil || queue == "" { + return + } + if !sqsValidThrottleAction(action) { + return + } + queueCounterLabel := m.admitForThrottleCounterBudget(queue) + m.throttledRequests.WithLabelValues(queueCounterLabel, action).Inc() +} + // ForgetThrottleAction removes the token-balance gauge for one queue/action // whose throttle bucket is no longer configured. Throttled-request counters // are cumulative and intentionally survive. @@ -314,6 +330,34 @@ func (m *SQSMetrics) ForgetThrottleAction(queue string, action string) { m.mu.Unlock() } +// ForgetThrottleActionBefore removes a token-balance gauge only when that +// specific queue/action was not refreshed after cutoff. It lets config-change +// cleanup reset stale gauges without erasing a fresh post-change request that +// raced between bucket invalidation and metrics reconciliation. +func (m *SQSMetrics) ForgetThrottleActionBefore(queue string, action string, cutoff uint64) { + if m == nil || queue == "" { + return + } + if !sqsValidThrottleAction(action) { + return + } + m.mu.Lock() + if actionSeqs := m.throttleGaugeActionSeq[queue]; actionSeqs != nil { + if seq := actionSeqs[action]; seq > cutoff { + m.mu.Unlock() + return + } + } + forget := m.forgetThrottleActionLocked(queue, action) + if forget.tracked { + m.dropThrottleGaugeActionFor(queue, action) + } + if forget.overflowSetEmpty { + m.dropThrottleGaugeActionFor(sqsQueueOverflow, action) + } + m.mu.Unlock() +} + // SyncThrottleActions reconciles token-balance gauges after a queue's throttle // configuration changes, even when no later request touches the disabled // bucket. enabledActions is the configured metric-action set; omitted actions @@ -443,6 +487,7 @@ func (m *SQSMetrics) forgetThrottleQueueLocked(queue string) sqsThrottleGaugeFor delete(m.trackedThrottleGaugeQueues, queue) } delete(m.throttleGaugeQueueSeq, queue) + delete(m.throttleGaugeActionSeq, queue) return sqsThrottleGaugeForget{ tracked: throttleTracked, overflowActions: m.removeThrottleOverflowQueueLocked(queue), @@ -472,6 +517,9 @@ func (m *SQSMetrics) forgetThrottleActionLocked(queue string, action string) sqs if !m.throttleGaugeQueueTrackedLocked(queue) { delete(m.throttleGaugeQueueSeq, queue) } + if !m.throttleGaugeActionTrackedLocked(queue, action) { + m.deleteThrottleGaugeActionSeqLocked(queue, action) + } return forget } @@ -490,9 +538,32 @@ func (m *SQSMetrics) removeThrottleOverflowQueueLocked(queue string) []string { if !m.throttleGaugeQueueTrackedLocked(queue) { delete(m.throttleGaugeQueueSeq, queue) } + delete(m.throttleGaugeActionSeq, queue) return emptyActions } +func (m *SQSMetrics) removeThrottleOverflowActionLocked(queue string, action string) bool { + queues, ok := m.overflowThrottleGaugeQueues[action] + if !ok { + return false + } + if _, ok := queues[queue]; !ok { + return false + } + delete(queues, queue) + empty := len(queues) == 0 + if empty { + delete(m.overflowThrottleGaugeQueues, action) + } + if !m.throttleGaugeActionTrackedLocked(queue, action) { + m.deleteThrottleGaugeActionSeqLocked(queue, action) + } + if !m.throttleGaugeQueueTrackedLocked(queue) { + delete(m.throttleGaugeQueueSeq, queue) + } + return empty +} + func (m *SQSMetrics) throttleGaugeQueueTrackedLocked(queue string) bool { if _, ok := m.trackedThrottleGaugeQueues[queue]; ok { return true @@ -505,6 +576,31 @@ func (m *SQSMetrics) throttleGaugeQueueTrackedLocked(queue string) bool { return false } +func (m *SQSMetrics) throttleGaugeActionTrackedLocked(queue string, action string) bool { + if actions, ok := m.trackedThrottleGaugeQueues[queue]; ok { + if _, ok := actions[action]; ok { + return true + } + } + if queues, ok := m.overflowThrottleGaugeQueues[action]; ok { + if _, ok := queues[queue]; ok { + return true + } + } + return false +} + +func (m *SQSMetrics) deleteThrottleGaugeActionSeqLocked(queue string, action string) { + actionSeqs := m.throttleGaugeActionSeq[queue] + if actionSeqs == nil { + return + } + delete(actionSeqs, action) + if len(actionSeqs) == 0 { + delete(m.throttleGaugeActionSeq, queue) + } +} + func (m *SQSMetrics) dropForgottenDepthQueue(queue string, forget sqsGaugeForget) { if forget.tracked { m.dropGaugeStatesFor(queue) @@ -609,10 +705,14 @@ func (m *SQSMetrics) admitForDepthBudget(queue string) string { func (m *SQSMetrics) admitForThrottleGaugeBudgetLocked(queue string, action string) string { m.throttleGaugeSeq++ m.throttleGaugeQueueSeq[queue] = m.throttleGaugeSeq + if m.throttleGaugeActionSeq[queue] == nil { + m.throttleGaugeActionSeq[queue] = map[string]uint64{} + } + m.throttleGaugeActionSeq[queue][action] = m.throttleGaugeSeq if actions, ok := m.trackedThrottleGaugeQueues[queue]; ok { actions[action] = struct{}{} - for _, overflowAction := range m.removeThrottleOverflowQueueLocked(queue) { - m.dropThrottleGaugeActionFor(sqsQueueOverflow, overflowAction) + if m.removeThrottleOverflowActionLocked(queue, action) { + m.dropThrottleGaugeActionFor(sqsQueueOverflow, action) } return queue } @@ -626,12 +726,19 @@ func (m *SQSMetrics) admitForThrottleGaugeBudgetLocked(queue string, action stri return sqsQueueOverflow } m.trackedThrottleGaugeQueues[queue] = map[string]struct{}{action: {}} - for _, overflowAction := range m.removeThrottleOverflowQueueLocked(queue) { - m.dropThrottleGaugeActionFor(sqsQueueOverflow, overflowAction) + if m.removeThrottleOverflowActionLocked(queue, action) { + m.dropThrottleGaugeActionFor(sqsQueueOverflow, action) } return queue } +// ThrottleGaugeSnapshotCutoff returns the current token-gauge observation +// sequence. Config-change cleanup can use it to forget only gauges observed +// before a reset point while preserving later request observations. +func (m *SQSMetrics) ThrottleGaugeSnapshotCutoff() uint64 { + return m.throttleGaugeSnapshotCutoff() +} + func (m *SQSMetrics) throttleGaugeSnapshotCutoff() uint64 { if m == nil { return 0 @@ -727,14 +834,16 @@ const sqsDepthObserveInterval = 30 * time.Second type SQSObserver struct { metrics *SQSMetrics - mu sync.Mutex - lastSeen map[string]struct{} + mu sync.Mutex + lastSeen map[string]struct{} + depthSeenThrottleQueues map[string]struct{} } func newSQSObserver(metrics *SQSMetrics) *SQSObserver { return &SQSObserver{ - metrics: metrics, - lastSeen: map[string]struct{}{}, + metrics: metrics, + lastSeen: map[string]struct{}{}, + depthSeenThrottleQueues: map[string]struct{}{}, } } @@ -824,16 +933,49 @@ func (o *SQSObserver) observeOnce(ctx context.Context, source SQSDepthSource) { } func (o *SQSObserver) forgetMissingQueuesLocked(current map[string]struct{}, throttleCutoff uint64) { + throttleQueues, activeThrottleQueues := o.snapshotThrottleQueues(throttleCutoff) + o.forgetDepthSeenThrottleQueuesWithoutGauge(activeThrottleQueues) + o.forgetDepthQueuesMissingFrom(current, activeThrottleQueues) + o.forgetThrottleOnlyQueues(current, throttleQueues) +} + +func (o *SQSObserver) snapshotThrottleQueues(throttleCutoff uint64) ([]string, map[string]struct{}) { + throttleQueues := o.metrics.snapshotThrottleGaugeQueues(throttleCutoff) + activeThrottleQueues := make(map[string]struct{}, len(throttleQueues)) + for _, queue := range throttleQueues { + activeThrottleQueues[queue] = struct{}{} + } + return throttleQueues, activeThrottleQueues +} + +func (o *SQSObserver) forgetDepthSeenThrottleQueuesWithoutGauge(activeThrottleQueues map[string]struct{}) { + for queue := range o.depthSeenThrottleQueues { + if _, ok := activeThrottleQueues[queue]; !ok { + delete(o.depthSeenThrottleQueues, queue) + } + } +} + +func (o *SQSObserver) forgetDepthQueuesMissingFrom(current map[string]struct{}, activeThrottleQueues map[string]struct{}) { for prev := range o.lastSeen { if _, ok := current[prev]; !ok { + if _, hasThrottleGauge := activeThrottleQueues[prev]; hasThrottleGauge { + o.depthSeenThrottleQueues[prev] = struct{}{} + } o.metrics.ForgetQueue(prev) } } - for _, prev := range o.metrics.snapshotThrottleGaugeQueues(throttleCutoff) { +} + +func (o *SQSObserver) forgetThrottleOnlyQueues(current map[string]struct{}, throttleQueues []string) { + for _, prev := range throttleQueues { if _, ok := current[prev]; !ok { if _, wasDepthSeen := o.lastSeen[prev]; wasDepthSeen { continue } + if _, wasDepthSeen := o.depthSeenThrottleQueues[prev]; wasDepthSeen { + continue + } o.metrics.ForgetThrottleQueue(prev) } } diff --git a/monitoring/sqs_test.go b/monitoring/sqs_test.go index 0f62104fb..53a16ef41 100644 --- a/monitoring/sqs_test.go +++ b/monitoring/sqs_test.go @@ -213,6 +213,19 @@ func TestSQSMetrics_ObserveThrottleDecision_EmitsCounterAndGauge(t *testing.T) { require.InDelta(t, 1.0, testutil.ToFloat64(m.throttledRequests.WithLabelValues("orders.fifo", SQSThrottleActionSend)), 0.001) } +func TestSQSMetrics_ObserveThrottleRejection_EmitsCounterOnly(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + m.ObserveThrottleRejection("orders.fifo", SQSThrottleActionSend) + + require.InDelta(t, 1.0, testutil.ToFloat64(m.throttledRequests.WithLabelValues("orders.fifo", SQSThrottleActionSend)), 0.001) + count, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 0, count, "stale rejection accounting must not recreate a token gauge") +} + func TestSQSMetrics_ObserveThrottleDecision_AllowedDoesNotConsumeCounterBudget(t *testing.T) { t.Parallel() reg := prometheus.NewRegistry() @@ -318,6 +331,32 @@ func TestSQSMetrics_ForgetThrottleAction_DropsGaugeAndFreesSlot(t *testing.T) { require.Empty(t, m.trackedThrottleGaugeQueues, "last disabled action frees the throttle gauge queue slot") } +func TestSQSMetrics_ForgetThrottleActionBefore_PreservesFreshActionGauge(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 5, false) + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionReceive, 7, false) + cutoff := m.ThrottleGaugeSnapshotCutoff() + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 3, false) + + m.ForgetThrottleActionBefore("orders.fifo", SQSThrottleActionSend, cutoff) + m.ForgetThrottleActionBefore("orders.fifo", SQSThrottleActionReceive, cutoff) + + send, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "orders.fifo", + "action": SQSThrottleActionSend, + }) + require.True(t, found, "post-cutoff send gauge must survive reset cleanup") + require.InDelta(t, 3.0, send, 0.001) + _, found = gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "orders.fifo", + "action": SQSThrottleActionReceive, + }) + require.False(t, found, "pre-cutoff receive gauge should still be reset") +} + func TestSQSMetrics_SyncThrottleActions_DropsDisabledWithoutTraffic(t *testing.T) { t.Parallel() reg := prometheus.NewRegistry() @@ -394,6 +433,39 @@ func TestSQSMetrics_ForgetThrottleQueue_OverflowThrottleGaugeClearsPerAction(t * require.False(t, found, "last overflow receive queue disappearing must drop _other/receive") } +func TestSQSMetrics_ThrottleOverflowPromotionPreservesOtherActionGauge(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + + for i := 0; i < sqsMaxTrackedQueues; i++ { + m.ObserveThrottleDecision("real-"+strconv.Itoa(i)+".fifo", SQSThrottleActionSend, 1, false) + } + m.ObserveThrottleDecision("overflow.fifo", SQSThrottleActionSend, 10, false) + m.ObserveThrottleDecision("overflow.fifo", SQSThrottleActionReceive, 20, false) + + m.ForgetThrottleQueue("real-0.fifo") + m.ObserveThrottleDecision("overflow.fifo", SQSThrottleActionSend, 8, false) + + send, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "overflow.fifo", + "action": SQSThrottleActionSend, + }) + require.True(t, found, "promoted action should move to the real queue label") + require.InDelta(t, 8.0, send, 0.001) + _, found = gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": sqsQueueOverflow, + "action": SQSThrottleActionSend, + }) + require.False(t, found, "the promoted send overflow gauge should be removed") + receive, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": sqsQueueOverflow, + "action": SQSThrottleActionReceive, + }) + require.True(t, found, "unrelated receive overflow gauge must survive until receive traffic is promoted") + require.InDelta(t, 20.0, receive, 0.001) +} + // TestSQSMetrics_ObserveQueueDepth_EmitsThreeStates pins that one // ObserveQueueDepth call produces three series (visible / not_visible // / delayed) under elastickv_sqs_queue_messages keyed on queue name. @@ -949,9 +1021,11 @@ func TestSQSObserver_ObserveOnce_DepthMissPreservesThrottleGauge(t *testing.T) { ticks: []fakeDepthTick{ {snaps: []SQSQueueDepth{{Queue: "orders.fifo", Visible: 1}}, ok: true}, {snaps: nil, ok: true}, + {snaps: nil, ok: true}, }, } + obs.ObserveOnce(source) obs.ObserveOnce(source) obs.ObserveOnce(source) From 0a81d55673281caab15ec85b3f4e35649a5fd6a4 Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 8 Jul 2026 21:00:21 +0900 Subject: [PATCH 15/16] Fix SQS throttle metric reset races --- adapter/sqs.go | 7 +++++ adapter/sqs_admin.go | 4 +-- adapter/sqs_catalog.go | 8 +++--- adapter/sqs_depth_source.go | 7 ++--- adapter/sqs_throttle.go | 30 ++++++++++++++++++++- adapter/sqs_throttle_test.go | 52 +++++++++++++++++++++++++++++++++++- monitoring/sqs.go | 43 +++++++++++++++++++++-------- monitoring/sqs_test.go | 39 +++++++++++++++++++++++---- 8 files changed, 163 insertions(+), 27 deletions(-) diff --git a/adapter/sqs.go b/adapter/sqs.go index 9b7e551d2..74469c5cb 100644 --- a/adapter/sqs.go +++ b/adapter/sqs.go @@ -363,6 +363,13 @@ func (s *SQSServer) throttleGaugeSnapshotCutoff() uint64 { return observer.ThrottleGaugeSnapshotCutoff() } +func (s *SQSServer) beginThrottleReset(queue string) uint64 { + if s == nil || s.throttle == nil { + return s.throttleGaugeSnapshotCutoff() + } + return s.throttle.beginQueueReset(queue, s.throttleGaugeSnapshotCutoff) +} + func enabledThrottleMetricActions(throttle *sqsQueueThrottle) []string { if throttle == nil || throttle.IsEmpty() { return nil diff --git a/adapter/sqs_admin.go b/adapter/sqs_admin.go index 94dba14ca..41a8bb0ad 100644 --- a/adapter/sqs_admin.go +++ b/adapter/sqs_admin.go @@ -364,8 +364,8 @@ func (s *SQSServer) AdminSetQueueAttributes(ctx context.Context, principal Admin return errors.Wrap(err, "admin set queue attributes") } if throttleChanged { - throttleResetCutoff := s.throttleGaugeSnapshotCutoff() - s.throttle.invalidateQueue(name) + throttleResetCutoff := s.beginThrottleReset(name) + s.throttle.invalidateQueueBuckets(name) s.observeThrottleConfigChange(name, throttle, resetActions, throttleResetCutoff) } return nil diff --git a/adapter/sqs_catalog.go b/adapter/sqs_catalog.go index a44a11c8d..f670d2bc9 100644 --- a/adapter/sqs_catalog.go +++ b/adapter/sqs_catalog.go @@ -1049,8 +1049,8 @@ func (s *SQSServer) tryCreateQueueOnce(ctx context.Context, requested *sqsQueueM // return path above, which exits before this point) guarantees // the new queue starts with a fresh full-capacity bucket // regardless of in-flight traffic to the prior incarnation. - throttleResetCutoff := s.throttleGaugeSnapshotCutoff() - s.throttle.invalidateQueue(requested.Name) + throttleResetCutoff := s.beginThrottleReset(requested.Name) + s.throttle.invalidateQueueBuckets(requested.Name) s.observeThrottleConfigChange(requested.Name, requested.Throttle, enabledThrottleMetricActions(requested.Throttle), throttleResetCutoff) // Mirror the throttle invalidate for the per-queue fanout-rotation // counter. A delete-then-create race could otherwise leave the @@ -1540,8 +1540,8 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) { // (e.g. via a future admin path), so the gating here is purely a // hot-path optimisation plus a no-op-bypass guard. if throttleChanged { - throttleResetCutoff := s.throttleGaugeSnapshotCutoff() - s.throttle.invalidateQueue(name) + throttleResetCutoff := s.beginThrottleReset(name) + s.throttle.invalidateQueueBuckets(name) s.observeThrottleConfigChange(name, throttle, resetActions, throttleResetCutoff) } writeSQSJSON(w, map[string]any{}) diff --git a/adapter/sqs_depth_source.go b/adapter/sqs_depth_source.go index 018f96476..b405b1627 100644 --- a/adapter/sqs_depth_source.go +++ b/adapter/sqs_depth_source.go @@ -22,9 +22,10 @@ type SQSQueueDepth struct { // // Returns: // -// - (snaps, true) — leader, scrape OK. Observer writes snaps to -// the gauges and diffs against the previous tick (forgetting -// any queue that disappeared from this snapshot). +// - (snaps, true) — leader, scrape OK. snaps is non-nil, even when +// there are zero queues. Observer writes snaps to the gauges and +// diffs against the previous tick (forgetting any queue that +// disappeared from this snapshot). // - (nil, true) — this node is a follower (leader-only emission // keeps gauges consistent with AdminListQueues / AdminDescribeQueue // at the same instant — follower scans would race the leader's diff --git a/adapter/sqs_throttle.go b/adapter/sqs_throttle.go index bcd4cee2a..60cd248de 100644 --- a/adapter/sqs_throttle.go +++ b/adapter/sqs_throttle.go @@ -137,7 +137,7 @@ type bucketQueueEpoch struct { // caller of sweep() is the sole goroutine the ticker drives. type bucketStore struct { buckets sync.Map // map[bucketKey]*tokenBucket - queueEpochMu sync.Mutex + queueEpochMu sync.RWMutex queueEpochs sync.Map // map[string]*bucketQueueEpoch clock func() time.Time evictedAfter time.Duration @@ -171,6 +171,8 @@ func (b *bucketStore) queueEpoch(queue string) uint64 { if b == nil || queue == "" { return 0 } + b.queueEpochMu.RLock() + defer b.queueEpochMu.RUnlock() v, ok := b.queueEpochs.Load(queue) if !ok { return 0 @@ -193,6 +195,25 @@ func (b *bucketStore) bumpQueueEpoch(queue string) { cell.updatedUnixNano.Store(b.clock().UnixNano()) } +func (b *bucketStore) beginQueueReset(queue string, cutoff func() uint64) uint64 { + if b == nil || queue == "" { + if cutoff == nil { + return 0 + } + return cutoff() + } + b.queueEpochMu.Lock() + defer b.queueEpochMu.Unlock() + var resetCutoff uint64 + if cutoff != nil { + resetCutoff = cutoff() + } + cell := b.queueEpochCellLocked(queue) + cell.epoch.Add(1) + cell.updatedUnixNano.Store(b.clock().UnixNano()) + return resetCutoff +} + func (b *bucketStore) queueEpochCellLocked(queue string) *bucketQueueEpoch { fresh := &bucketQueueEpoch{} fresh.updatedUnixNano.Store(b.clock().UnixNano()) @@ -430,6 +451,13 @@ func (b *bucketStore) invalidateQueue(queue string) { return } b.bumpQueueEpoch(queue) + b.invalidateQueueBuckets(queue) +} + +func (b *bucketStore) invalidateQueueBuckets(queue string) { + if b == nil { + return + } // Incarnation participates in the key: we do // not know which incarnations have buckets cached, so range the map // and remove any entry whose queue matches. A SetQueueAttributes diff --git a/adapter/sqs_throttle_test.go b/adapter/sqs_throttle_test.go index 20d3f2656..98b7d6f30 100644 --- a/adapter/sqs_throttle_test.go +++ b/adapter/sqs_throttle_test.go @@ -33,9 +33,16 @@ type recordingSQSThrottleObserver struct { rejections []throttleForgetReport forgotten []throttleForgetReport syncs []throttleSyncReport + seq uint64 + actionSeq map[throttleForgetReport]uint64 } func (r *recordingSQSThrottleObserver) ObserveThrottleDecision(queue string, action string, tokensRemaining float64, throttled bool) { + r.seq++ + if r.actionSeq == nil { + r.actionSeq = map[throttleForgetReport]uint64{} + } + r.actionSeq[throttleForgetReport{queue: queue, action: action}] = r.seq r.reports = append(r.reports, throttleObserveReport{ queue: queue, action: action, @@ -49,14 +56,32 @@ func (r *recordingSQSThrottleObserver) ObserveThrottleRejection(queue string, ac } func (r *recordingSQSThrottleObserver) ForgetThrottleAction(queue string, action string) { + if r.actionSeq != nil { + delete(r.actionSeq, throttleForgetReport{queue: queue, action: action}) + } r.forgotten = append(r.forgotten, throttleForgetReport{queue: queue, action: action}) } +func (r *recordingSQSThrottleObserver) ForgetThrottleActionBefore(queue string, action string, cutoff uint64) { + key := throttleForgetReport{queue: queue, action: action} + if r.actionSeq != nil { + if seq := r.actionSeq[key]; seq > cutoff { + return + } + delete(r.actionSeq, key) + } + r.forgotten = append(r.forgotten, key) +} + +func (r *recordingSQSThrottleObserver) ThrottleGaugeSnapshotCutoff() uint64 { + return r.seq +} + func (r *recordingSQSThrottleObserver) SyncThrottleActions(queue string, enabledActions []string) { enabled := append([]string(nil), enabledActions...) r.syncs = append(r.syncs, throttleSyncReport{queue: queue, enabled: enabled}) for _, action := range disabledThrottleMetricActionsFromEnabled(enabled) { - r.forgotten = append(r.forgotten, throttleForgetReport{queue: queue, action: action}) + r.ForgetThrottleAction(queue, action) } } @@ -155,6 +180,31 @@ func TestSQSServer_ChargeQueueWithThrottleSkipsStaleMetricAfterInvalidate(t *tes require.Equal(t, SQSThrottleActionSend, observer.reports[0].action) } +func TestSQSServer_BeginThrottleResetSuppressesStaleMetricBeforeCleanup(t *testing.T) { + t.Parallel() + observer := &recordingSQSThrottleObserver{} + srv := NewSQSServer(nil, nil, nil, WithSQSThrottleObserver(observer)) + now := time.Date(2026, 4, 27, 10, 0, 0, 0, time.UTC) + srv.throttle = newBucketStore(func() time.Time { return now }, throttleIdleEvictAfter) + cfg := &sqsQueueThrottle{SendCapacity: 10, SendRefillPerSecond: 1} + + staleEpoch := srv.throttle.queueEpoch("orders.fifo") + resetCutoff := srv.beginThrottleReset("orders.fifo") + rec := httptest.NewRecorder() + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1, staleEpoch)) + require.Empty(t, observer.reports, "pre-reset epoch request must not publish a token gauge after the reset gate starts") + + srv.throttle.invalidateQueueBuckets("orders.fifo") + freshEpoch := srv.throttle.queueEpoch("orders.fifo") + rec = httptest.NewRecorder() + require.True(t, srv.chargeQueueWithThrottle(rec, "orders.fifo", bucketActionSend, 1, cfg, 1, freshEpoch)) + require.Len(t, observer.reports, 1, "fresh post-reset request may publish a token gauge") + + srv.observeThrottleConfigChange("orders.fifo", cfg, []string{SQSThrottleActionSend}, resetCutoff) + require.NotContains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionSend}, + "reset cleanup must preserve token gauges observed after the reset gate") +} + func TestSQSServer_ChargeQueueWithThrottleCountsStaleRejectionAfterInvalidate(t *testing.T) { t.Parallel() observer := &recordingSQSThrottleObserver{} diff --git a/monitoring/sqs.go b/monitoring/sqs.go index cf45648ee..68d18fb30 100644 --- a/monitoring/sqs.go +++ b/monitoring/sqs.go @@ -70,12 +70,16 @@ type SQSThrottleObserver interface { // // Two distinct empty-snapshot states, signalled by ok: // -// - ok=true with an empty/nil slice — "this source legitimately -// has no queues this tick". Triggers when the node is a -// follower (leader-only emission) or the leader genuinely has -// zero queues configured. The observer diffs against the -// previous tick and ForgetQueue's any queue that disappeared -// so a former leader's gauges are cleared on step-down. +// - ok=true with a nil slice — "this source is not emitting this tick". +// The adapter returns this when the node is a follower (leader-only +// emission). The observer clears both depth and token gauges so a former +// leader does not keep exporting stale SQS series after step-down. +// +// - ok=true with a non-nil slice, possibly empty — leader, scrape OK. +// The observer writes the returned queue gauges and diffs against the +// previous tick. A queue omitted from a non-nil snapshot can be a +// per-queue scan miss, so throttle token gauges for depth-seen queues are +// preserved until a config/delete path explicitly removes them. // // - ok=false (regardless of the slice contents) — "the source // could not produce a snapshot this tick" (transient catalog- @@ -918,8 +922,9 @@ func (o *SQSObserver) observeOnce(ctx context.Context, source SQSDepthSource) { // at least one interval even when capacity opens up the same // tick. Reclaiming first lets phase 2's admissions reuse the // freed slots immediately. + sourceInactive := snaps == nil o.mu.Lock() - o.forgetMissingQueuesLocked(current, throttleCutoff) + o.forgetMissingQueuesLocked(current, throttleCutoff, sourceInactive) o.lastSeen = current o.mu.Unlock() // Phase 2: emit gauges for the current tick. Slots freed in @@ -932,10 +937,17 @@ func (o *SQSObserver) observeOnce(ctx context.Context, source SQSDepthSource) { } } -func (o *SQSObserver) forgetMissingQueuesLocked(current map[string]struct{}, throttleCutoff uint64) { +func (o *SQSObserver) forgetMissingQueuesLocked(current map[string]struct{}, throttleCutoff uint64, sourceInactive bool) { + if sourceInactive { + throttleQueues, _ := o.snapshotThrottleQueues(^uint64(0)) + o.forgetDepthQueuesMissingFrom(current, false) + o.forgetInactiveSourceThrottleQueues(current, throttleQueues) + clear(o.depthSeenThrottleQueues) + return + } throttleQueues, activeThrottleQueues := o.snapshotThrottleQueues(throttleCutoff) o.forgetDepthSeenThrottleQueuesWithoutGauge(activeThrottleQueues) - o.forgetDepthQueuesMissingFrom(current, activeThrottleQueues) + o.forgetDepthQueuesMissingFrom(current, true) o.forgetThrottleOnlyQueues(current, throttleQueues) } @@ -956,10 +968,10 @@ func (o *SQSObserver) forgetDepthSeenThrottleQueuesWithoutGauge(activeThrottleQu } } -func (o *SQSObserver) forgetDepthQueuesMissingFrom(current map[string]struct{}, activeThrottleQueues map[string]struct{}) { +func (o *SQSObserver) forgetDepthQueuesMissingFrom(current map[string]struct{}, preserveThrottle bool) { for prev := range o.lastSeen { if _, ok := current[prev]; !ok { - if _, hasThrottleGauge := activeThrottleQueues[prev]; hasThrottleGauge { + if preserveThrottle { o.depthSeenThrottleQueues[prev] = struct{}{} } o.metrics.ForgetQueue(prev) @@ -967,6 +979,15 @@ func (o *SQSObserver) forgetDepthQueuesMissingFrom(current map[string]struct{}, } } +func (o *SQSObserver) forgetInactiveSourceThrottleQueues(current map[string]struct{}, throttleQueues []string) { + for _, prev := range throttleQueues { + if _, ok := current[prev]; ok { + continue + } + o.metrics.ForgetThrottleQueue(prev) + } +} + func (o *SQSObserver) forgetThrottleOnlyQueues(current map[string]struct{}, throttleQueues []string) { for _, prev := range throttleQueues { if _, ok := current[prev]; !ok { diff --git a/monitoring/sqs_test.go b/monitoring/sqs_test.go index 53a16ef41..2dd511b87 100644 --- a/monitoring/sqs_test.go +++ b/monitoring/sqs_test.go @@ -956,11 +956,15 @@ func TestSQSObserver_ObserveOnce_LeaderStepDownClearsAll(t *testing.T) { count, err := testutil.GatherAndCount(reg, "elastickv_sqs_queue_messages") require.NoError(t, err) require.Equal(t, 6, count, "tick 1: 2 queues × 3 states = 6 series") + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 4, false) obs.ObserveOnce(source) count, err = testutil.GatherAndCount(reg, "elastickv_sqs_queue_messages") require.NoError(t, err) require.Equal(t, 0, count, "tick 2 (leader step-down): all gauges cleared") + throttleCount, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") + require.NoError(t, err) + require.Equal(t, 0, throttleCount, "tick 2 (leader step-down): throttle gauges cleared with depth gauges") } func TestSQSObserver_ObserveOnce_ForgetsThrottleOnlyQueues(t *testing.T) { @@ -975,7 +979,7 @@ func TestSQSObserver_ObserveOnce_ForgetsThrottleOnlyQueues(t *testing.T) { require.Equal(t, 1, count, "sanity: throttle-only queue emitted a token gauge before the depth tick") obs.ObserveOnce(&fakeDepthSource{ - ticks: []fakeDepthTick{{snaps: nil, ok: true}}, + ticks: []fakeDepthTick{{snaps: []SQSQueueDepth{}, ok: true}}, }) count, err = testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") @@ -992,7 +996,7 @@ func TestSQSObserver_ObserveOnce_PreservesThrottleGaugeNewerThanDepthSnapshot(t obs.ObserveOnce(callbackDepthSource{fn: func() ([]SQSQueueDepth, bool) { m.ObserveThrottleDecision("newer.fifo", SQSThrottleActionSend, 4, false) - return nil, true + return []SQSQueueDepth{}, true }}) value, found := gatheredThrottleTokenValue(t, reg, map[string]string{ @@ -1003,7 +1007,7 @@ func TestSQSObserver_ObserveOnce_PreservesThrottleGaugeNewerThanDepthSnapshot(t require.InDelta(t, 4.0, value, 0.001) obs.ObserveOnce(&fakeDepthSource{ - ticks: []fakeDepthTick{{snaps: nil, ok: true}}, + ticks: []fakeDepthTick{{snaps: []SQSQueueDepth{}, ok: true}}, }) count, err := testutil.GatherAndCount(reg, "elastickv_sqs_throttle_tokens_remaining") require.NoError(t, err) @@ -1020,8 +1024,8 @@ func TestSQSObserver_ObserveOnce_DepthMissPreservesThrottleGauge(t *testing.T) { source := &fakeDepthSource{ ticks: []fakeDepthTick{ {snaps: []SQSQueueDepth{{Queue: "orders.fifo", Visible: 1}}, ok: true}, - {snaps: nil, ok: true}, - {snaps: nil, ok: true}, + {snaps: []SQSQueueDepth{}, ok: true}, + {snaps: []SQSQueueDepth{}, ok: true}, }, } @@ -1041,6 +1045,31 @@ func TestSQSObserver_ObserveOnce_DepthMissPreservesThrottleGauge(t *testing.T) { require.Len(t, m.trackedThrottleGaugeQueues, 1, "depth-only miss must not reclaim the throttle-gauge budget slot") } +func TestSQSObserver_ObserveOnce_DepthSeenFreshGaugeAfterCutoffSurvivesMiss(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + obs := newSQSObserver(m) + + obs.ObserveOnce(&fakeDepthSource{ + ticks: []fakeDepthTick{{snaps: []SQSQueueDepth{{Queue: "orders.fifo", Visible: 1}}, ok: true}}, + }) + obs.ObserveOnce(callbackDepthSource{fn: func() ([]SQSQueueDepth, bool) { + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 6, false) + return []SQSQueueDepth{}, true + }}) + obs.ObserveOnce(&fakeDepthSource{ + ticks: []fakeDepthTick{{snaps: []SQSQueueDepth{}, ok: true}}, + }) + + value, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "orders.fifo", + "action": SQSThrottleActionSend, + }) + require.True(t, found, "fresh token gauge for a previously depth-seen queue must survive repeated depth misses") + require.InDelta(t, 6.0, value, 0.001) +} + // TestSQSObserver_ObserveOnce_TransientScanErrorPreservesGauges // pins the P2 fix from PR #743 r6: when the source returns // ok=false (transient catalog-scan failure on the leader, ctx From 07365fcf41d1c0094682d887d6b5b5d670963b88 Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 8 Jul 2026 21:34:32 +0900 Subject: [PATCH 16/16] Fix SQS throttle cleanup races --- adapter/sqs.go | 14 +++++ adapter/sqs_admin.go | 10 +-- adapter/sqs_catalog.go | 80 +++++++++++++++--------- adapter/sqs_throttle_integration_test.go | 3 +- adapter/sqs_throttle_test.go | 17 +++++ main.go | 6 +- main_sqs_depth_observer_test.go | 33 ++++++++++ monitoring/sqs.go | 5 +- monitoring/sqs_test.go | 28 +++++++++ 9 files changed, 155 insertions(+), 41 deletions(-) create mode 100644 main_sqs_depth_observer_test.go diff --git a/adapter/sqs.go b/adapter/sqs.go index 74469c5cb..d8125db31 100644 --- a/adapter/sqs.go +++ b/adapter/sqs.go @@ -352,6 +352,20 @@ func (s *SQSServer) observeThrottleConfigChange(queue string, throttle *sqsQueue } } +func (s *SQSServer) observeThrottleDelete(queue string, resetCutoff uint64) { + if s == nil || s.throttleObserver == nil { + return + } + observer, ok := s.throttleObserver.(sqsThrottleGaugeResetObserver) + if !ok { + s.observeThrottleConfig(queue, nil) + return + } + for _, action := range sqsThrottleMetricActions { + observer.ForgetThrottleActionBefore(queue, action, resetCutoff) + } +} + func (s *SQSServer) throttleGaugeSnapshotCutoff() uint64 { if s == nil || s.throttleObserver == nil { return 0 diff --git a/adapter/sqs_admin.go b/adapter/sqs_admin.go index 41a8bb0ad..e3b205dd6 100644 --- a/adapter/sqs_admin.go +++ b/adapter/sqs_admin.go @@ -353,7 +353,7 @@ func (s *SQSServer) AdminSetQueueAttributes(ctx context.Context, principal Admin if strings.TrimSpace(name) == "" || len(attrs) == 0 { return ErrAdminSQSValidation } - throttleChanged, throttle, resetActions, err := s.setQueueAttributesWithRetry(ctx, name, attrs) + throttleChanged, throttle, resetActions, throttleResetCutoff, err := s.setQueueAttributesWithRetry(ctx, name, attrs) if err != nil { if isSQSAdminQueueDoesNotExist(err) { return ErrAdminSQSNotFound @@ -364,7 +364,6 @@ func (s *SQSServer) AdminSetQueueAttributes(ctx context.Context, principal Admin return errors.Wrap(err, "admin set queue attributes") } if throttleChanged { - throttleResetCutoff := s.beginThrottleReset(name) s.throttle.invalidateQueueBuckets(name) s.observeThrottleConfigChange(name, throttle, resetActions, throttleResetCutoff) } @@ -385,7 +384,8 @@ func (s *SQSServer) AdminDeleteQueue(ctx context.Context, principal AdminPrincip if strings.TrimSpace(name) == "" { return ErrAdminSQSValidation } - if err := s.deleteQueueWithRetry(ctx, name); err != nil { + throttleResetCutoff, err := s.deleteQueueWithRetry(ctx, name) + if err != nil { // deleteQueueWithRetry returns sqsAPIError with // sqsErrQueueDoesNotExist when the queue is missing; map // to the structured ErrAdminSQSNotFound so the admin @@ -395,8 +395,8 @@ func (s *SQSServer) AdminDeleteQueue(ctx context.Context, principal AdminPrincip } return errors.Wrap(err, "admin delete queue") } - s.throttle.invalidateQueue(name) - s.observeThrottleConfig(name, nil) + s.throttle.invalidateQueueBuckets(name) + s.observeThrottleDelete(name, throttleResetCutoff) s.dropReceiveFanoutCounter(name) return nil } diff --git a/adapter/sqs_catalog.go b/adapter/sqs_catalog.go index f670d2bc9..7318c3c80 100644 --- a/adapter/sqs_catalog.go +++ b/adapter/sqs_catalog.go @@ -1038,6 +1038,10 @@ func (s *SQSServer) tryCreateQueueOnce(ctx context.Context, requested *sqsQueueM {Op: kv.Put, Key: genKey, Value: []byte(strconv.FormatUint(requested.Generation, 10))}, }, } + // Open the reset gate before the commit so first requests + // against the newly-created incarnation publish gauges newer + // than the cleanup cutoff below. + throttleResetCutoff := s.beginThrottleReset(requested.Name) if _, err := s.coordinator.Dispatch(ctx, req); err != nil { return false, errors.WithStack(err) } @@ -1049,7 +1053,6 @@ func (s *SQSServer) tryCreateQueueOnce(ctx context.Context, requested *sqsQueueM // return path above, which exits before this point) guarantees // the new queue starts with a fresh full-capacity bucket // regardless of in-flight traffic to the prior incarnation. - throttleResetCutoff := s.beginThrottleReset(requested.Name) s.throttle.invalidateQueueBuckets(requested.Name) s.observeThrottleConfigChange(requested.Name, requested.Throttle, enabledThrottleMetricActions(requested.Throttle), throttleResetCutoff) // Mirror the throttle invalidate for the per-queue fanout-rotation @@ -1072,7 +1075,8 @@ func (s *SQSServer) deleteQueue(w http.ResponseWriter, r *http.Request) { writeSQSErrorFromErr(w, err) return } - if err := s.deleteQueueWithRetry(r.Context(), name); err != nil { + throttleResetCutoff, err := s.deleteQueueWithRetry(r.Context(), name) + if err != nil { writeSQSErrorFromErr(w, err) return } @@ -1083,8 +1087,8 @@ func (s *SQSServer) deleteQueue(w http.ResponseWriter, r *http.Request) { // keep enforcing for up to the idle-evict window (default 1 h), // surprising operators who use DeleteQueue+CreateQueue to reset // queue state. - s.throttle.invalidateQueue(name) - s.observeThrottleConfig(name, nil) + s.throttle.invalidateQueueBuckets(name) + s.observeThrottleDelete(name, throttleResetCutoff) // Drop the per-queue fanout-rotation counter as well. Without // this, repeated DeleteQueue of unique queue names retains one // receiveFanoutCounters entry per name for the process lifetime @@ -1095,17 +1099,17 @@ func (s *SQSServer) deleteQueue(w http.ResponseWriter, r *http.Request) { writeSQSJSON(w, map[string]any{}) } -func (s *SQSServer) deleteQueueWithRetry(ctx context.Context, queueName string) error { +func (s *SQSServer) deleteQueueWithRetry(ctx context.Context, queueName string) (uint64, error) { backoff := transactRetryInitialBackoff deadline := time.Now().Add(transactRetryMaxDuration) for range transactRetryMaxAttempts { readTS := s.nextTxnReadTS(ctx) existing, exists, err := s.loadQueueMetaAt(ctx, queueName, readTS) if err != nil { - return errors.WithStack(err) + return 0, errors.WithStack(err) } if !exists { - return newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist") + return 0, newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist") } // Bump the generation counter so any stragglers under the old @@ -1116,7 +1120,7 @@ func (s *SQSServer) deleteQueueWithRetry(ctx context.Context, queueName string) // keyspace would leak forever. lastGen, err := s.loadQueueGenerationAt(ctx, queueName, readTS) if err != nil { - return errors.WithStack(err) + return 0, errors.WithStack(err) } metaKey := sqsQueueMetaKey(queueName) genKey := sqsQueueGenKey(queueName) @@ -1141,17 +1145,21 @@ func (s *SQSServer) deleteQueueWithRetry(ctx context.Context, queueName string) {Op: kv.Put, Key: tombstoneKey, Value: tombstoneValue}, }, } + // Start the delete cleanup cutoff before the tombstone commit. + // A same-name CreateQueue that commits immediately afterward can + // then publish token gauges newer than this stale delete cleanup. + throttleResetCutoff := s.beginThrottleReset(queueName) if _, err := s.coordinator.Dispatch(ctx, req); err == nil { - return nil + return throttleResetCutoff, nil } else if !isRetryableTransactWriteError(err) { - return errors.WithStack(err) + return 0, errors.WithStack(err) } if err := waitRetryWithDeadline(ctx, deadline, backoff); err != nil { - return errors.WithStack(err) + return 0, errors.WithStack(err) } backoff = nextTransactRetryBackoff(backoff) } - return newSQSAPIError(http.StatusInternalServerError, sqsErrInternalFailure, "delete queue retry attempts exhausted") + return 0, newSQSAPIError(http.StatusInternalServerError, sqsErrInternalFailure, "delete queue retry attempts exhausted") } func (s *SQSServer) listQueues(w http.ResponseWriter, r *http.Request) { @@ -1512,7 +1520,7 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) { writeSQSError(w, http.StatusBadRequest, sqsErrMissingParameter, "Attributes is required") return } - throttleChanged, throttle, resetActions, err := s.setQueueAttributesWithRetry(r.Context(), name, in.Attributes) + throttleChanged, throttle, resetActions, throttleResetCutoff, err := s.setQueueAttributesWithRetry(r.Context(), name, in.Attributes) if err != nil { writeSQSErrorFromErr(w, err) return @@ -1538,32 +1546,33 @@ func (s *SQSServer) setQueueAttributes(w http.ResponseWriter, r *http.Request) { // quiet. The bucket reconciliation in loadOrInit also // catches a stale bucket if a throttle change slips past this gate // (e.g. via a future admin path), so the gating here is purely a - // hot-path optimisation plus a no-op-bypass guard. + // hot-path optimisation plus a no-op-bypass guard. The reset cutoff + // was captured before the successful commit to preserve first + // post-commit token observations. if throttleChanged { - throttleResetCutoff := s.beginThrottleReset(name) s.throttle.invalidateQueueBuckets(name) s.observeThrottleConfigChange(name, throttle, resetActions, throttleResetCutoff) } writeSQSJSON(w, map[string]any{}) } -func (s *SQSServer) setQueueAttributesWithRetry(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, []string, error) { +func (s *SQSServer) setQueueAttributesWithRetry(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, []string, uint64, error) { backoff := transactRetryInitialBackoff deadline := time.Now().Add(transactRetryMaxDuration) for range transactRetryMaxAttempts { - throttleChanged, throttle, resetActions, done, err := s.trySetQueueAttributesOnce(ctx, queueName, attrs) + throttleChanged, throttle, resetActions, throttleResetCutoff, done, err := s.trySetQueueAttributesOnce(ctx, queueName, attrs) if err == nil && done { - return throttleChanged, throttle, resetActions, nil + return throttleChanged, throttle, resetActions, throttleResetCutoff, nil } if err != nil && !isRetryableTransactWriteError(err) { - return false, nil, nil, err + return false, nil, nil, 0, err } if err := waitRetryWithDeadline(ctx, deadline, backoff); err != nil { - return false, nil, nil, errors.WithStack(err) + return false, nil, nil, 0, errors.WithStack(err) } backoff = nextTransactRetryBackoff(backoff) } - return false, nil, nil, newSQSAPIError(http.StatusInternalServerError, sqsErrInternalFailure, "set queue attributes retry attempts exhausted") + return false, nil, nil, 0, newSQSAPIError(http.StatusInternalServerError, sqsErrInternalFailure, "set queue attributes retry attempts exhausted") } // applyAndValidateSetAttributes runs the apply + cross-validator @@ -1602,7 +1611,7 @@ func applyAndValidateSetAttributes(meta *sqsQueueMeta, attrs map[string]string) } // trySetQueueAttributesOnce is one read-validate-commit pass. The returns are -// (throttleChanged, postThrottle, resetActions, done, err). done reports +// (throttleChanged, postThrottle, resetActions, resetCutoff, done, err). done reports // whether the caller should stop retrying (the attrs are now committed); an // error means either a non-retryable failure (propagate) or a retryable write // conflict (retry after backoff). throttleChanged is true iff the post-apply @@ -1611,15 +1620,17 @@ func applyAndValidateSetAttributes(meta *sqsQueueMeta, attrs map[string]string) // same-value SetQueueAttributes does not reset the bucket to full capacity or // churn token gauges. resetActions is the still-enabled metric-action set // whose in-memory bucket was dropped by the queue-wide invalidation and whose -// stale token gauge must be removed. -func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, []string, bool, error) { +// stale token gauge must be removed. resetCutoff is captured before the +// successful Dispatch so post-commit request-path gauges are not removed by +// the caller's cleanup. +func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName string, attrs map[string]string) (bool, *sqsQueueThrottle, []string, uint64, bool, error) { readTS := s.nextTxnReadTS(ctx) meta, exists, err := s.loadQueueMetaAt(ctx, queueName, readTS) if err != nil { - return false, nil, nil, false, errors.WithStack(err) + return false, nil, nil, 0, false, errors.WithStack(err) } if !exists { - return false, nil, nil, false, newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist") + return false, nil, nil, 0, false, newSQSAPIError(http.StatusBadRequest, sqsErrQueueDoesNotExist, "queue does not exist") } // Snapshot the throttle config under the same read TS used for the // commit so the comparison sees the value the writer is racing @@ -1627,10 +1638,10 @@ func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName str // floats wrapped in a struct). preThrottle := snapshotThrottle(meta.Throttle) if err := applyAndValidateSetAttributes(meta, attrs); err != nil { - return false, nil, nil, false, err + return false, nil, nil, 0, false, err } if err := s.validateRedrivePolicyTarget(ctx, meta, readTS); err != nil { - return false, nil, nil, false, err + return false, nil, nil, 0, false, err } throttleChanged := !throttleConfigEqual(preThrottle, meta.Throttle) postThrottle := snapshotThrottle(meta.Throttle) @@ -1638,7 +1649,7 @@ func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName str meta.LastModifiedAtMillis = time.Now().UnixMilli() metaBytes, err := encodeSQSQueueMeta(meta) if err != nil { - return false, nil, nil, false, errors.WithStack(err) + return false, nil, nil, 0, false, errors.WithStack(err) } metaKey := sqsQueueMetaKey(queueName) // StartTS + ReadKeys prevent two concurrent SetQueueAttributes from @@ -1652,10 +1663,17 @@ func (s *SQSServer) trySetQueueAttributesOnce(ctx context.Context, queueName str {Op: kv.Put, Key: metaKey, Value: metaBytes}, }, } + var throttleResetCutoff uint64 + if throttleChanged { + // Start the epoch/cutoff gate before Dispatch. A request that + // reads the newly-committed throttle config immediately after + // this commit must publish a gauge newer than the cleanup cutoff. + throttleResetCutoff = s.beginThrottleReset(queueName) + } if _, err := s.coordinator.Dispatch(ctx, req); err != nil { - return false, nil, nil, false, errors.WithStack(err) + return false, nil, nil, 0, false, errors.WithStack(err) } - return throttleChanged, postThrottle, resetActions, true, nil + return throttleChanged, postThrottle, resetActions, throttleResetCutoff, true, nil } // snapshotThrottle returns a value-copy of the Throttle config so a diff --git a/adapter/sqs_throttle_integration_test.go b/adapter/sqs_throttle_integration_test.go index 2635ea35b..7649988d7 100644 --- a/adapter/sqs_throttle_integration_test.go +++ b/adapter/sqs_throttle_integration_test.go @@ -342,7 +342,8 @@ func TestSQSServer_Throttle_DeleteCreateQueueSyncsMetrics(t *testing.T) { if status, _ := callSQS(t, node, sqsDeleteQueueTarget, map[string]any{"QueueUrl": url}); status != http.StatusOK { t.Fatalf("delete: %d", status) } - require.Contains(t, observer.syncs, throttleSyncReport{queue: queue}) + require.Empty(t, observer.syncs, + "DeleteQueue cleanup must use cutoff-scoped forgets instead of unconditional action sync") require.ElementsMatch(t, []throttleForgetReport{ {queue: queue, action: SQSThrottleActionSend}, {queue: queue, action: SQSThrottleActionReceive}, diff --git a/adapter/sqs_throttle_test.go b/adapter/sqs_throttle_test.go index 98b7d6f30..314c2432b 100644 --- a/adapter/sqs_throttle_test.go +++ b/adapter/sqs_throttle_test.go @@ -205,6 +205,23 @@ func TestSQSServer_BeginThrottleResetSuppressesStaleMetricBeforeCleanup(t *testi "reset cleanup must preserve token gauges observed after the reset gate") } +func TestSQSServer_ObserveThrottleDeletePreservesFreshGaugeAfterCutoff(t *testing.T) { + t.Parallel() + observer := &recordingSQSThrottleObserver{} + srv := NewSQSServer(nil, nil, nil, WithSQSThrottleObserver(observer)) + + observer.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 1, false) + deleteCutoff := observer.ThrottleGaugeSnapshotCutoff() + observer.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 9, false) + + srv.observeThrottleDelete("orders.fifo", deleteCutoff) + + require.NotContains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionSend}, + "stale delete cleanup must not erase a same-name new incarnation gauge observed after the cutoff") + require.Contains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionReceive}) + require.Contains(t, observer.forgotten, throttleForgetReport{queue: "orders.fifo", action: SQSThrottleActionDefault}) +} + func TestSQSServer_ChargeQueueWithThrottleCountsStaleRejectionAfterInvalidate(t *testing.T) { t.Parallel() observer := &recordingSQSThrottleObserver{} diff --git a/main.go b/main.go index 742af112d..1a1098d61 100644 --- a/main.go +++ b/main.go @@ -1503,7 +1503,9 @@ func startSQSDepthObserver(ctx context.Context, reg *monitoring.Registry, sqsSer // expects []monitoring.SQSQueueDepth). Same shape both sides; the // loop is a fixed-size copy. type sqsDepthSourceAdapter struct { - inner *adapter.SQSServer + inner interface { + SnapshotQueueDepths(context.Context) ([]adapter.SQSQueueDepth, bool) + } } func (a sqsDepthSourceAdapter) SnapshotQueueDepths(ctx context.Context) ([]monitoring.SQSQueueDepth, bool) { @@ -1518,7 +1520,7 @@ func (a sqsDepthSourceAdapter) SnapshotQueueDepths(ctx context.Context) ([]monit // existing gauges alone on a transient scan failure. return nil, false } - if len(snaps) == 0 { + if snaps == nil { return nil, true } out := make([]monitoring.SQSQueueDepth, len(snaps)) diff --git a/main_sqs_depth_observer_test.go b/main_sqs_depth_observer_test.go new file mode 100644 index 000000000..f5457cb38 --- /dev/null +++ b/main_sqs_depth_observer_test.go @@ -0,0 +1,33 @@ +package main + +import ( + "context" + "testing" + + "github.com/bootjp/elastickv/adapter" + "github.com/stretchr/testify/require" +) + +type fakeSQSDepthBridgeSource struct { + snaps []adapter.SQSQueueDepth + ok bool +} + +func (f fakeSQSDepthBridgeSource) SnapshotQueueDepths(context.Context) ([]adapter.SQSQueueDepth, bool) { + return f.snaps, f.ok +} + +func TestSQSDepthSourceAdapterPreservesNilVsEmptySnapshots(t *testing.T) { + t.Parallel() + + got, ok := (sqsDepthSourceAdapter{inner: fakeSQSDepthBridgeSource{snaps: nil, ok: true}}). + SnapshotQueueDepths(context.Background()) + require.True(t, ok) + require.Nil(t, got, "nil successful snapshots must remain nil for follower/step-down cleanup") + + got, ok = (sqsDepthSourceAdapter{inner: fakeSQSDepthBridgeSource{snaps: []adapter.SQSQueueDepth{}, ok: true}}). + SnapshotQueueDepths(context.Background()) + require.True(t, ok) + require.NotNil(t, got, "non-nil empty leader snapshots must survive the adapter bridge") + require.Empty(t, got) +} diff --git a/monitoring/sqs.go b/monitoring/sqs.go index 68d18fb30..353b7405f 100644 --- a/monitoring/sqs.go +++ b/monitoring/sqs.go @@ -945,8 +945,9 @@ func (o *SQSObserver) forgetMissingQueuesLocked(current map[string]struct{}, thr clear(o.depthSeenThrottleQueues) return } - throttleQueues, activeThrottleQueues := o.snapshotThrottleQueues(throttleCutoff) - o.forgetDepthSeenThrottleQueuesWithoutGauge(activeThrottleQueues) + throttleQueues, _ := o.snapshotThrottleQueues(throttleCutoff) + _, currentThrottleQueues := o.snapshotThrottleQueues(^uint64(0)) + o.forgetDepthSeenThrottleQueuesWithoutGauge(currentThrottleQueues) o.forgetDepthQueuesMissingFrom(current, true) o.forgetThrottleOnlyQueues(current, throttleQueues) } diff --git a/monitoring/sqs_test.go b/monitoring/sqs_test.go index 2dd511b87..c4e13c915 100644 --- a/monitoring/sqs_test.go +++ b/monitoring/sqs_test.go @@ -1070,6 +1070,34 @@ func TestSQSObserver_ObserveOnce_DepthSeenFreshGaugeAfterCutoffSurvivesMiss(t *t require.InDelta(t, 6.0, value, 0.001) } +func TestSQSObserver_ObserveOnce_DepthSeenMarkerSurvivesFreshGaugeDuringLaterMiss(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + m := newSQSMetrics(reg) + obs := newSQSObserver(m) + + obs.ObserveOnce(&fakeDepthSource{ + ticks: []fakeDepthTick{{snaps: []SQSQueueDepth{{Queue: "orders.fifo", Visible: 1}}, ok: true}}, + }) + obs.ObserveOnce(&fakeDepthSource{ + ticks: []fakeDepthTick{{snaps: []SQSQueueDepth{}, ok: true}}, + }) + obs.ObserveOnce(callbackDepthSource{fn: func() ([]SQSQueueDepth, bool) { + m.ObserveThrottleDecision("orders.fifo", SQSThrottleActionSend, 7, false) + return []SQSQueueDepth{}, true + }}) + obs.ObserveOnce(&fakeDepthSource{ + ticks: []fakeDepthTick{{snaps: []SQSQueueDepth{}, ok: true}}, + }) + + value, found := gatheredThrottleTokenValue(t, reg, map[string]string{ + "queue": "orders.fifo", + "action": SQSThrottleActionSend, + }) + require.True(t, found, "fresh token gauge for a previously depth-seen queue must keep the depth-seen marker") + require.InDelta(t, 7.0, value, 0.001) +} + // TestSQSObserver_ObserveOnce_TransientScanErrorPreservesGauges // pins the P2 fix from PR #743 r6: when the source returns // ok=false (transient catalog-scan failure on the leader, ctx