docs: propose admin purge queue (DLQ-aware) for Web Console#757
Conversation
User feature request: clear DLQ contents from the Web Console without
dropping the queue itself, so the source queue's RedrivePolicy ARN
reference stays valid through the operation.
Doc proposes:
- AdminPurgeQueue admin RPC (mirrors AdminDeleteQueue's auth/audit
pattern, wraps the existing purgeQueueWithRetry primitive)
- DELETE /admin/api/v1/sqs/queues/{name}/messages HTTP endpoint
(RESTful 'delete messages, not queue', distinct from the existing
endpoint that deletes the queue itself)
- AdminQueueSummary.IsDLQ + DLQSources for DLQ-aware UI labeling
(reverse-scan the catalog; reverse-index deferred to a follow-up
if profiling shows the Describe path is hot)
- SPA button on SqsDetailPage with name-confirmation modal,
labelled 'Purge messages' or 'Purge DLQ' depending on IsDLQ
Non-goals:
- Redrive (StartMessageMoveTask) — separate feature with different
semantics (per-message dispatch, partial success)
- Filtered purge — AWS PurgeQueue is all-or-nothing and we mirror it
- Cross-queue bulk purge
The 60-second meta-stored rate limit applies uniformly to SigV4 and
admin paths since both wrap the same tryPurgeQueueOnce. The admin
wrapper maps the SQS PurgeQueueInProgress code to a structured
ErrAdminSQSPurgeInProgress sentinel rendered as HTTP 429 with a
Retry-After header.
Rollout: doc lands first (this PR), then backend RPC + sentinel
errors, then HTTP handler + bridge, then SPA button. Each phase
reviews small; the user-visible feature ships in the final phase.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a design proposal for DLQ-aware SQS admin message peeking and generation-bump purging, new admin HTTP endpoints under /admin/api/v1/sqs/queues/{name}/messages, frontend Messages tab and client wiring, audit/metrics, auth rules, testing plan, and rollout guidance. ChangesSQS Admin Purge Queue Feature Design
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request proposes a design for an 'Admin Purge Queue' feature in the SQS Web Console, allowing operators to clear queue contents without deleting the queue itself. The design includes a new backend RPC, a RESTful HTTP endpoint, and a DLQ-aware UI. Feedback focused on performance concerns regarding the O(N) catalog scan for DLQ identification, specifically its impact on the AdminDescribeQueue path and the audit logging overhead for write operations. Additionally, a potential routing issue with trailing slashes in the new endpoint was identified.
| } | ||
| ``` | ||
|
|
||
| `AdminDescribeQueue` populates `IsDLQ` / `DLQSources` by reverse-scanning the catalog: walk every queue's meta, parse its `RedrivePolicy` via `parseRedrivePolicy` (already in `adapter/sqs_redrive.go:43`), match the resolved DLQ target against `name`. The scan is bounded by the catalog cap that `scanApproxCounters` already enforces, so cost is `O(known queues)` per Describe call — the same order the existing counter scan pays. |
There was a problem hiding this comment.
The design mentions that the O(N) scan is "bounded by the catalog cap that scanApproxCounters already enforces". However, scanApproxCounters (as seen in adapter/sqs_admin.go:67) typically performs a prefix scan on the message keyspace of a single queue, which is efficient. \n\nIn contrast, populating IsDLQ by iterating over every queue's metadata could be expensive if implemented as N individual KV lookups. If the catalog metadata is stored under a common prefix (e.g., !sqs|catalog|meta|), a single prefix scan would be much more efficient. Consider specifying a prefix scan for this discovery or implementing the reverse-index optimization earlier to avoid performance degradation as the number of queues grows, especially since the SPA likely polls this endpoint.
| | `DELETE` | `/admin/api/v1/sqs/queues/{name}` *(existing)* | Delete the queue itself | | ||
| | `DELETE` | `/admin/api/v1/sqs/queues/{name}/messages` *(new)*| Purge the queue's messages, keep meta | | ||
|
|
||
| The router in `internal/admin/sqs_handler.go` already path-prefixes on `pathSqsQueues = "/admin/api/v1/sqs/queues"`. The new endpoint is dispatched by suffix match: `strings.HasSuffix(trailing, "/messages")` after the `{name}` segment is parsed. A new handler `handlePurge` lives in `sqs_handler.go` parallel to `handleDelete`, calling `source.PurgeQueue(ctx, name)` against the `QueuesSource` interface (which is implemented by `sqsQueuesBridge.PurgeQueue` in `main_admin.go`, which itself wraps `AdminPurgeQueue`). |
There was a problem hiding this comment.
The proposed routing logic using strings.HasSuffix(trailing, "/messages") may fail if the request URL contains a trailing slash (e.g., /admin/api/v1/sqs/queues/foo/messages/). Consider using path.Clean or explicitly handling the trailing slash to ensure the endpoint is robust.
| is_dlq=<bool> | ||
| dlq_sources=<csv> |
There was a problem hiding this comment.
Including is_dlq and dlq_sources in the audit log for AdminPurgeQueue implies that the purge operation itself must also perform the O(N) catalog scan to gather this metadata. This adds significant overhead to a write operation that is otherwise O(1) (just a generation bump).\n\nIf the audit log requires this information, it might be more efficient to have the AdminPurgeQueue RPC return the metadata it discovered, or reconsider if the full list of sources is necessary for the audit trail given the cost of acquisition.
Two updates in one commit per the user's request to extend the proposal:
(1) Gemini r1 medium x3:
- doc line ~90 (IsDLQ scan cost): clarify the reverse-scan is ONE
Pebble prefix scan + O(N) in-memory deserialize, not N point
lookups. Note that the cap (sqsQueueScanPageLimit, paged) is the
same bound scanQueueNames already pays for AdminListQueues. The
reverse-index optimisation is acknowledged in-line as the
documented next step.
- doc line ~101 (trailing-slash routing): replace suffix match with
path-segment match using path.Clean. Reject /messages/ (extra
empty segment after Clean), /messages/foo (undefined
sub-resource), and /Messages (case mismatch). Same shape Dynamo
handler already uses for its sub-resources.
- doc line ~137 (audit-log overhead): drop is_dlq / dlq_sources
from the audit line. Computing those would force every purge
(O(1) generation bump) to pay the O(N) reverse-scan. DLQ
context is observable separately via Describe at audit-review
time. Explain the cost trade-off in-line so a future reader
doesn't add the fields back without thinking.
(2) New scope from user follow-up: add Peek for queue contents.
Operators clearing a DLQ need to triage first (which messages
are stuck, are they still useful, are they safe to drop) before
purging. SigV4 ReceiveMessage is destructive (bumps receive
count, starts visibility timer, mints receipt handles) and using
it from the AWS CLI just to glance at DLQ contents can advance
messages toward purge eligibility under the very RedrivePolicy
that put them there.
Added to the proposal:
- AdminPeekQueue RPC: read-only, non-destructive sample of
currently-visible messages. NO receive-count bump, NO visibility
timer, NO receipt handles minted. Cursor-paginated (opaque
base64 encoding-json of {Generation, LastKey}). Limit clamped
to [1, 100], BodyMaxBytes clamped to [256, 65536]. Leader-only
because the visibility index is leader-written and a follower
read could see an index entry whose underlying message record
was already deleted (apply-vs-lease-read race).
- AdminPeekedMessage struct: MessageID, Body (truncated),
BodyTruncated, BodyOriginalSize, SentTimestamp, ReceiveCount,
GroupID, DeduplicationID, Attributes.
- GET /admin/api/v1/sqs/queues/{name}/messages?limit=N&cursor=C
HTTP endpoint, dispatched by method on the same /messages
sub-resource that DELETE owns for purge.
- SPA Messages tab on SqsDetailPage: paginated table with row-
click detail modal, page-size selector (20/50/100), Next/Prev
cursor controls, 'Copy as JSON' on the detail modal so the
Peek-and-export recovery workflow is one click per message.
- Per-queue read-side throttling integration: peek shares the
ReceiveMessage budget so a tight peek loop cannot starve real
consumers.
Non-goals updated:
- per-message delete from the Peek view (receipt-handle interplay
diverges from AWS; future RFC if operators ask)
- in-flight messages in Peek (rented out to consumers; future PR
with ?include_in_flight=true)
Test plan extended with peek-specific cases (Limit clamping,
cursor round-trip, BodyMaxBytes truncation, no-side-effects
verification, FIFO vs standard column rendering) and an
integration test that pins peek-after-purge returns zero rows.
Rollout phases re-numbered: 2=purge, 3=peek, 4=HTTP handlers
(both verbs land together since they share the path-segment
router), 5=SPA Messages tab + Purge button, 6=doc rename.
|
@claude review |
|
Claude finished @bootjp's task in 4m 41s —— View job Claude's ReviewTasks
Overall AssessmentThe design is well-structured and clearly motivated. Reusing Gemini's points about the O(N) audit log scan are already resolved in §3.6 (lean audit log, DLQ context excluded from the write path). The trailing-slash routing concern is resolved by the There are several gaps that need resolution before implementation begins. Design Gaps (should be addressed before Phase 2 starts)1. Partitioned FIFO queues not addressed in The design says peek "walks the visibility index for the queue" but there are two visibility keyspaces:
The simpler option (b) matches how 2.
Options:
Option 1 (typed error) matches how existing Go APIs propagate rate-limit metadata and keeps 3. The design says "Peek is gated by 4. Audit log The audit line requires
Option (b) is the cleanest — 5. The current name := strings.TrimPrefix(r.URL.Path, pathPrefixSqsQueues)
// For /admin/api/v1/sqs/queues/foo/messages → name = "foo/messages"
h.handleDescribe(w, r, name) // currently receives "foo/messages"Adding 6. The design defines Minor Issues7. The API client spec shows 8. Peek throttle budget shared with §3.1 says peek "shares the per-queue read-side budget the SigV4 9. Multi-partition cursor size bound If the Phase 3 implementation follows the partitioned-fanout approach for the cursor, the cursor's encoded size will grow with partition count. Set and validate a maximum cursor byte length (e.g., 512 bytes) to prevent clients from crafting pathologically large cursor inputs. 10. "Copy as JSON" payload schema undefined The detail modal's "Copy as JSON" action is mentioned in §3.5 but the payload schema isn't specified. Operators will export this to downstream systems. Define the schema in the design so the frontend and any operator tooling can agree on the shape before Phase 5. 11. Messages tab: visible vs. total message count The Messages tab shows only Test Plan Gaps (§6)
Rollout NotePhase ordering is correct — Purge before Peek makes sense because Purge reuses existing primitives while Peek introduces the new pagination/cursor infrastructure. The note that Phase 5 (SPA) is the user-visible gate is the right framing for the change-management risk. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcf189e88b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | `GET` | `/admin/api/v1/sqs/queues/{name}/messages?limit=N&cursor=C` *(new)* | Peek a sample of visible messages | | ||
| | `DELETE` | `/admin/api/v1/sqs/queues/{name}/messages` *(new)* | Purge all messages, keep meta intact | | ||
|
|
||
| Method-based dispatch on the same `/messages` sub-resource: `GET` → peek, `DELETE` → purge. The router in `internal/admin/sqs_handler.go` already path-prefixes on `pathSqsQueues = "/admin/api/v1/sqs/queues"`. The new endpoint is dispatched by **path-segment match, not suffix match**: after parsing the `{name}` segment, the handler splits the remaining trailing path into segments (using `path.Clean` first to normalise away leading / trailing / duplicated slashes), and accepts the path iff the trailing segments are exactly `["messages"]`. This rejects `/messages/` (one extra empty segment after Clean), `/messages/foo` (sub-resource we don't define), and `/Messages` (case-sensitive AWS naming) without relying on suffix heuristics. Two new handlers `handlePeek` (GET) and `handlePurge` (DELETE) live in `sqs_handler.go` parallel to `handleDelete`, calling `source.PeekQueue(ctx, name, opts)` / `source.PurgeQueue(ctx, name)` against the `QueuesSource` interface (implemented by `sqsQueuesBridge` in `main_admin.go`, which wraps `AdminPeekQueue` / `AdminPurgeQueue`). The path-segment matcher is the same shape DynamoHandler already uses for its sub-resource routing — keeping the two parallel reduces the cognitive load of operators who reason about both surfaces. |
There was a problem hiding this comment.
The routing rule here says path.Clean is applied before segment matching and still claims /messages/ will be rejected, but path.Clean removes trailing slashes, so /messages/ normalizes to /messages and would be accepted by an exact ["messages"] segment check. That contradicts the stated contract and test plan (which expects /messages/ to return 404), so implementers following this text will likely ship behavior that differs from the documented API shape.
Useful? React with 👍 / 👎.
Claude r2 review on PR #757 listed 6 design gaps + 3 minor + 2 test-plan gaps. All are legitimate; doc updated. Design gaps (commit-by-commit): 1. Partitioned FIFO in Peek (§3.1): two visibility keyspaces exist (standard prefix + 'p|' prefix). Peek now commits to sequential partition scanning (scan partition 0 to exhaustion, then 1, …) matching receiveMessage's dispatch. Cursor is a versioned JSON envelope {v=1, gen, partition, last_key} base64url-encoded with a 512-byte cap; validation returns ErrAdminSQSValidation on overflow or stale-generation cursors. 2. Retry-After propagation (§3.2): AdminPurgeQueue now returns (AdminPurgeResult, error). Rate-limit path returns *PurgeInProgressError{RetryAfter: time.Duration} that wraps the ErrAdminSQSPurgeInProgress sentinel via errors.Is so the handler can branch on the sentinel while the duration travels in the typed payload. Admin-package mirror struct rewraps it across the bridge so admin stays free of the adapter package. 3. principalForRead (§2.1 goal 8): the helper does not exist today. Doc now says peek uses the same session-auth gate that List/Describe use; no additional live-role re-check. Purge alone runs principalForWrite. Earlier wording implied a non-existent helper. 4. Audit log generations (§3.6): purgeQueueWithRetry's signature changes to (oldGen, newGen uint64, err error) so values come from the committed OCC round, not racy pre/post meta reads. Caller audit per the standing rule: one external caller (purgeQueue in sqs_purge.go) that currently ignores success values and will discard them via _, _, err := …. Spelled out so the SigV4 path is not silently broken when the signature changes. 5. ServeHTTP restructure (§3.4): the current dispatch hands the full suffix after /queues/ to handleDescribe/handleDelete as the queue name. Adding /messages sub-resources requires a three-level restructure: collection root → named queue → named queue sub-resource. Existing handlers receive bare {name} after the change. Doc calls this out so the implementation PR plans for it instead of stacking elif branches. 6. ErrQueuesPurgeInProgress + writeQueuesError 429 (§3.4): admin package gains the sentinel + PurgeInProgressError struct + 429 branch with Retry-After header in writeQueuesError. JSON body gets retry_after_seconds field. Minor: 7. body_max_bytes URL param uses snake_case (matches the rest of the admin API). TypeScript client field stays bodyMaxBytes (matches JS convention); client adapter does case translation at request boundary. 8. Peek throttle (§3.1): explicitly uses a distinct per-queue admin-peek bucket (adminPeekRPS=5, adminPeekBurst=20), NOT the ReceiveMessage budget. Operator paginating through a 10k-message DLQ would otherwise starve real consumers. Bucket key 'admin_peek:<queue>' so it cannot collide with wire-side buckets. 9. Cursor size cap (§3.1): 512-byte cap on encoded cursor; pathological client-supplied cursors get ErrAdminSQSValidation. 10. Copy as JSON schema (§3.5): wrapper {schema_version:1, queue, exported_at, message:{…AdminPeekedMessage…}} so downstream tooling pins the version. Detail modal re-peeks at body_max_bytes=65536 if the row was truncated. 11. Messages tab UX (§3.5): tab header shows 'Showing N visible messages (M currently in-flight)' so empty peek on busy queue isn't mistaken for empty queue. N=peek result, M=counters.not_visible. Test plan extended (§6): - TestAdminPeekQueue_PartitionedFIFO_Pagination: PartitionCount=4 with messages across all partitions; cursor pagination yields every message exactly once in (partition, visible_at) order. - TestAdminPurgeQueue audit-generation test: arranges queue at gen N, runs purge, asserts result has GenerationBefore=N / After=N+1, runs concurrent same-tick purge to validate the rate-limit returns the same generation pair (i.e., values logged are the ones that actually landed, not stale intermediates). - TestPurgeQueueSigV4_StillCompiles: exercises the SigV4 wire path through the modified purgeQueueWithRetry signature to prove the change does not regress. No code in this PR; doc-only update.
|
@claude review |
|
Claude finished @bootjp's task in 4m 18s —— View job Claude's Review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 045cc403e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Below the table: a page-size selector (20 / 50 / 100), a Refresh button, and Next / Previous controls driven by the cursor. Detail modal shows full body + every attribute + the timestamps; a "Copy as JSON" button copies the row's full record to the clipboard for manual export. | ||
|
|
||
| **Copy as JSON payload schema.** The clipboard payload is the exact wire shape of a single `AdminPeekedMessage` entry (top-level keys: `message_id`, `body`, `body_truncated`, `body_original_size`, `sent_timestamp`, `receive_count`, `group_id`, `deduplication_id`, `attributes`) plus a wrapper `{"schema_version": 1, "queue": "<name>", "exported_at": "<ISO8601>", "message": { … }}`. The `schema_version` is what downstream tooling pins so a future change to the export format (e.g. multi-message JSONL bundle) does not silently break exporters. Operator workflows that pipe this into a recovery tool can rely on the schema not shifting under them. The full-body modal shows the FULL body — not the truncated one — by issuing a single-message peek with `body_max_bytes=65536` if the row's `body_truncated` was true on the list response. |
There was a problem hiding this comment.
The proposal says the modal can fetch the FULL body by calling peek with body_max_bytes=65536, but this same doc caps BodyMaxBytes at 65,536 while SQS queue message size in this repo is up to 262,144 bytes (adapter/sqs_catalog.go). For any message larger than 64 KiB, operators would still see truncated content in the modal and could purge without actually seeing the full payload, which breaks the triage guarantee this feature is meant to provide.
Useful? React with 👍 / 👎.
| outcome ∈ {"ok", "forbidden", "not_leader", "not_found", "internal_error"} | ||
| ``` |
There was a problem hiding this comment.
This outcome set omits a throttling state even though §3.1 introduces a dedicated admin-peek bucket that returns 429 on exhaustion. If implementers follow this enum literally, throttled peek requests have no correct label and will be either dropped or misclassified as internal_error, which makes the new metric unable to distinguish real backend faults from expected rate limiting.
Useful? React with 👍 / 👎.
…757) Two Codex r3 findings on the round-3 doc: 1. P1 (line 329, Copy-as-JSON full body): the doc claimed the detail modal could fetch the full body by re-peeking with body_max_bytes=65536, but SQS stored-message size caps at 262144 (= 256 KiB, sqsMaximumAllowedMaximumMessageSize in adapter/sqs_catalog.go:38). The 64-KiB ceiling on BodyMaxBytes broke the triage guarantee for messages between 64 KiB and 256 KiB — operators would still see truncated payloads in the modal and could purge without knowing what they were dropping. Fix: raise the BodyMaxBytes upper clamp to sqsMaximumAllowedMaximumMessageSize (= 262144). The detail modal passes body_max_bytes=262144 on the single-message re-peek; guaranteed full body for any legitimately stored message. 2. P2 (line 379, peek metric outcomes): the elastickv_sqs_admin_peek_queue_total enum omitted a 'throttled' outcome even though §3.1 defines a dedicated admin-peek bucket that returns 429 on exhaustion. Implementers following the enum literally would have classified expected rate-limit rejections as internal_error, making the metric unable to distinguish backend faults from operator pagination loops. Fix: add 'throttled' to the outcome enum. Doc explains why peek needs the label but not 'purge_in_progress' (the 60s rate-limit is purge-specific; peek is read-only and doesn't share that state). No code in this PR; doc-only update.
|
@claude review |
|
Claude finished @bootjp's task in 5m 54s —— View job Tasks
Claude's Review — r4Addressed since r3
Both Codex r3 findings are cleanly resolved. Four findings from Claude r3 remain open, plus one new finding surfaced by reading the actual implementation code. Still Open from r31. RetryAfter duration propagation — contradictory phrasing (§3.2) The document now says two things that cannot both be true:
Reading The implementation will follow one path; the doc needs to commit to which one. The correct path — and the one that avoids the race — is: 3. The design shows the bridge interface with "existing" methods type QueuesSource interface {
AdminListQueues(ctx context.Context) ([]string, error)
AdminDescribeQueue(ctx context.Context, name string) (*QueueSummary, bool, error)
AdminDeleteQueue(ctx context.Context, principal AuthPrincipal, name string) error
}If Phase 4 adds new methods as
4. Admin-peek throttle incarnation keying — key format conflicts with The doc says the admin-peek bucket is "referenced by name from If the admin-peek bucket uses the existing s.throttle.charge(adminPeekThrottle, queueName, "admin_peek", meta.Incarnation, 1)This participates in incarnation keying automatically — re-creating the queue resets the bucket via the existing If the admin-peek bucket is a separate, simpler mechanism (a dedicated Pick one and specify it explicitly. The 6. Trailing slash contradiction — still unresolved (§3.4 + §6.4) The doc says both:
After The contradiction:
New Finding12. "Single-message re-peek" mechanism is undefined (§3.5) The detail modal is described as triggering "a single-message re-peek with The most practical interpretation is that the SPA re-issues the current page's request (same cursor, same limit) with
Define the mechanism before Phase 5 so the frontend and backend implementations agree on the full-body fetch contract. Overall AssessmentThe two Codex findings (body cap, throttled metric) are cleanly resolved. The four open r3 findings (#1, #3, #4, #6) are all clarification/precision issues; none blocks Phase 2 (Purge) which doesn't touch Peek routing or the throttle bucket design. Phase 2 can proceed once finding #1 (RetryAfter propagation) is resolved in the doc — it's the only one that directly affects the Phase 2 implementation code. Findings #3, #4, and #6 must be resolved before Phase 3 (Peek) and Phase 4 (HTTP handlers) respectively. Finding #12 must be resolved before Phase 5 (SPA). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55db4de62c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| ``` | ||
|
|
||
| `AdminDescribeQueue` populates `IsDLQ` / `DLQSources` by reverse-scanning the catalog. Implementation: a single `ScanAt` over the queue-meta prefix (`SqsQueueMetaPrefix`, the prefix `scanQueueNames` already walks) returns all queue meta records in one storage round-trip; the handler then unmarshals each, runs `parseRedrivePolicy` (already in `adapter/sqs_redrive.go:43`) against the queue's `RedrivePolicy` attribute, and matches the resolved DLQ target against `name`. Cost is **one Pebble prefix scan + O(N) unmarshal-and-string-compare in memory** — not O(N) point lookups. The catalog cap (`sqsQueueScanPageLimit`, paged) already bounds the scan exactly as `scanQueueNames` does today, so the worst-case load is the same one the existing admin list endpoint pays per request. |
There was a problem hiding this comment.
The design says AdminDescribeQueue can detect DLQ sources with a single ScanAt over SqsQueueMetaPrefix, but that will silently miss sources once the catalog exceeds one page (sqsQueueScanPageLimit is 1024 in adapter/sqs_catalog.go). In deployments with >1024 queues, IsDLQ/DLQSources can be false or incomplete for queues whose source links live past the first page, which would mislabel the purge UI and weaken the DLQ-specific warning this feature is meant to enforce.
Useful? React with 👍 / 👎.
Codex P1 round 4: the previous wording ("a single ScanAt ... returns
all queue meta records in one storage round-trip") would silently miss
DLQ source links in deployments with >1024 queues because
sqsQueueScanPageLimit = 1024 caps each ScanAt page. Queues whose
RedrivePolicy lived past the first page would be invisible to the
IsDLQ / DLQSources detection — the purge UI would render the wrong
framing (no "this is a DLQ" warning, no source-chip list) for the
exact deployments that need the warning most.
Rewrite the §3.3 paragraph to describe the actual reuse: the same
paginated loop scanQueueNames runs today (cursor + page-limit +
prefix-bound-exit). Make the pagination contract explicit so the
implementer cannot ship the bug Codex caught:
- "paginated prefix scan ... reusing the exact loop shape from
scanQueueNames"
- cite the loop exit conditions (empty page / partial page /
cursor past prefix end)
- cost stated as ceil(N/1024) round-trips + O(N) in-memory work,
not "one round-trip"
- call out the bug-shape that the wording fixes so a future
refactor doesn't regress to the single-ScanAt form
No code changes — proposal-only PR.
|
@claude review Round 4 — Codex P1 (line 240): doc claimed "single Rewrote §3.3 to describe the actual reuse: the same paginated loop Cost restated as Proposal-only — no code changes. |
|
Claude finished @bootjp's task in 3m 51s —— View job Tasks
Claude's Review — r5Addressed since r4 ✅ Codex P1 (paginated DLQ source scan, §3.3) — Clean fix. The doc now correctly describes the paginated loop matching Still Open from r3/r4 — Resolution RequiredThe five findings flagged in r3 and confirmed still-open in r4 are all still present in this revision. None of the five findings requires new code archaeology — they are precision gaps in the doc text that must be committed to before implementation begins. 1. RetryAfter duration sourcing — two contradictory sentences remain (§3.2) The paragraph on
These two phrases cannot both be true. Reading The correct mechanism is "at the rejection point": 3. The bridge interface in the doc still shows: // existing
List(ctx context.Context) ([]string, error)
Describe(ctx context.Context, name string) (*QueueSummary, bool, error)
Delete(ctx context.Context, p AuthPrincipal, name string) error
// new
Peek(...)
Purge(...)The actual interface in
Blocks Phase 4. 4. Admin-peek throttle bucket API incompatibility (§3.1) The doc describes the admin-peek bucket as "referenced by name from func (b *bucketStore) charge(cfg *sqsQueueThrottle, queue, action string, incarnation uint64, count int) chargeOutcomeThe key is a s.throttle.charge(adminPeekThrottle, queueName, "admin_peek", meta.Incarnation, 1)The 6. Trailing slash contradiction (§3.4 + §6.4) The doc says both:
After
Blocks Phase 4. 12. "Single-message re-peek" mechanism undefined (§3.5) The doc says "opening the detail modal triggers a single-message re-peek with The three options are the same as r3/r4: (a) re-peek the current page with Minor Finding (New)13. Line number drift in §3.3 §3.3 says "the exact loop shape from Overall AssessmentThe Codex P1 pagination fix is clean and correct — the |
|
@claude review |
|
Claude finished @bootjp's task in 3m 28s —— View job Tasks
Claude's Review — r5Addressed since r4 ✅ Codex P1 (paginated DLQ source scan): §3.3 was rewritten to describe the paginated prefix scan. The code in New Finding from r4 Fix13. §10 Summary still says "one storage round-trip" — inconsistent with §3.3 fix §3.3 (the section that was just fixed) now correctly says
This is the exact bug Codex P1 caught in §3.3 — now it survives in the summary. An implementer reading only §10 (the TL;DR) would think the reverse-scan is a single round-trip and could write Still Open from r41. RetryAfter duration propagation — §3.2 still self-contradictory §3.2, line 220 of the current doc:
These two phrases cannot both be true. "From the meta record" → the admin wrapper reads the meta a second time after the rejection. "At the rejection point" → The two implementation paths are mutually exclusive:
Path (b) is the correct choice. The doc must remove the phrase "from the meta record" and fully commit to the typed-error description. Until this is resolved, Phase 2 implementers face a real decision point with two conflicting approaches in the doc. 3. The design's §3.4 bridge interface shows: type QueuesSource interface {
// existing
List(ctx context.Context) ([]string, error)
Describe(ctx context.Context, name string) (*QueueSummary, bool, error)
Delete(ctx context.Context, p AuthPrincipal, name string) error
// new
Peek(...) (PeekResult, error)
Purge(...) (PurgeResult, error)
}The actual interface at type QueuesSource interface {
AdminListQueues(ctx context.Context) ([]string, error)
AdminDescribeQueue(ctx context.Context, name string) (*QueueSummary, bool, error)
AdminDeleteQueue(ctx context.Context, principal AuthPrincipal, name string) error
}The existing method names in the doc (
4. Admin-peek throttle bucket — The doc says the admin-peek bucket is "referenced by name from func (b *bucketStore) charge(cfg *sqsQueueThrottle, queue, action string, incarnation uint64, count int) chargeOutcomeThe
Option (a) is the right default — it re-uses existing infrastructure and gets incarnation reset for free. 6. Trailing slash contradiction — §3.4 and §6.4 still conflict §3.4 says: " §6.4 test plan: " Commit to one:
12. Single-message re-peek mechanism — still undefined §3.5: "opening the detail modal triggers a single-message re-peek with
|
|
@claude review |
Codex r4 P1 (new):
- IsDLQ scan paginated: single ScanAt would silently miss sources
past the 1024-page limit, leaving IsDLQ/DLQSources incomplete in
>1024-queue deployments. §3.3 now commits to a paginated loop
matching scanQueueNames's internal shape. Cost stated as
O(ceil(N/1024)) round-trips + O(N) in-memory comparisons.
Claude r4 four findings still open from r3:
phrasing was ambiguous — could mean a second loadQueueMetaAt call,
which races a concurrent purge resetting LastPurgedAtMillis in
the 60-second window. Doc now commits explicitly to the
typed-error path: tryPurgeQueueOnce computes remaining inside
the OCC read at rejection and returns purgeRateLimitedError{
remaining time.Duration}; admin wrapper extracts via errors.As.
No second meta read.
AdminListQueues / AdminDescribeQueue / AdminDeleteQueue (verified
at internal/admin/sqs_handler.go:62-65). Earlier draft showed
them as List/Describe/Delete; new methods follow the existing
convention (AdminPeekQueue, AdminPurgeQueue) so the interface
doesn't end up half-prefixed.
bucketKey{queue, action, incarnation}, not strings. Doc now
uses bucketStore.charge() with action='admin_peek' and the
queue's current incarnation, participating in invalidateQueue()
automatically. Earlier draft's free-standing string-keyed map
would have built parallel rate-limiter infrastructure.
slashes, so /messages/ normalises to /messages and the request
succeeds. Earlier draft asserted 404 on /messages/ in the test
plan — contradicted path.Clean's normalisation. Doc now commits
to (a) accept trailing slashes; test plan §6.4 updated to
assert 204 on /messages/.
Claude r4 one new finding:
targeted fetch; the modal re-issues the current page's request
with body_max_bytes=262144 and locates the target message by
ID. Doc adds mitigation: default page size 20, warn before
re-peek if cumulative size > 20 MiB, cache result so other
rows on the same page don't re-fetch. Future PR can add a
?message_id=… filter if production needs more targeted fetch.
No code in this PR; doc-only update.
|
Claude finished @bootjp's task in 4m 45s —— View job Tasks
Claude's Review — r5Addressed since r4 The latest commit (Codex P1 fix — §3.3 DLQ pagination) is accurate: All five r4 open findings are resolved. Summary of verification:
New Findings13.
When a queue has an operator-configured Concrete scenario: queue Fix options:
Option (a) is the smaller change and keeps all throttle state in one place. The 14.
func writeSQSErrorFromErr(w http.ResponseWriter, err error) {
var apiErr *sqsAPIError
if errors.As(err, &apiErr) {
writeSQSError(w, apiErr.status, apiErr.errorType, apiErr.message)
return
}
// falls through to generic 500
}The design says The design says "the wire-side var purgeErr *purgeRateLimitedError
if errors.As(err, &purgeErr) {
writeSQSError(w, http.StatusBadRequest, sqsErrPurgeInProgress,
fmt.Sprintf("…retry after %.0f seconds", purgeErr.remaining.Seconds()))
return
}The doc should say explicitly that 15. The expression remaining := time.Duration(sqsPurgeRateLimitMillis-(now-meta.LastPurgedAtMillis)) * time.MillisecondAn implementer reading Rollout Impact
Phase 2 cannot proceed until finding #14 is addressed in the doc (the SigV4 wire response would regress). Phase 3 needs #13 resolved before the The pagination fix (Codex P1 r4) is confirmed correct against the actual |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/design/2026_05_16_proposed_admin_purge_queue.md (1)
256-256: 💤 Low valueOptional: Add language specifiers to code fences for better rendering.
The code fences at these lines are missing language identifiers, which can affect syntax highlighting in rendered Markdown. Consider adding specifiers:
- Line 256: routing/path examples → no language needed, or use
text- Line 369: structured log format → could use
textorlog- Line 384: Prometheus metric format → could use
textorprometheusThis is purely cosmetic and does not affect the technical content.
✨ Proposed fixes
Line 256:
-``` +```text /admin/api/v1/sqs/queues → handleList (existing)Line 369:
-``` +```text admin.sqs.purge_queueLine 384:
-``` +```text elastickv_sqs_admin_purge_queue_total{queue, outcome}As per coding guidelines, this addresses the markdownlint warning about fenced code blocks requiring language specifiers.
Also applies to: 369-369, 384-384
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/2026_05_16_proposed_admin_purge_queue.md` at line 256, Add language specifiers to the three Markdown fenced code blocks referenced: update the block showing the routing/path example that contains "/admin/api/v1/sqs/queues → handleList" to use a text fence (e.g., ```text), update the structured log example containing "admin.sqs.purge_queue" to use a text/log fence (e.g., ```text or ```log), and update the Prometheus metric example containing "elastickv_sqs_admin_purge_queue_total{queue, outcome}" to use a text/prometheus fence (e.g., ```text or ```prometheus); this fixes the markdownlint warning by ensuring each ``` block has an appropriate language specifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/design/2026_05_16_proposed_admin_purge_queue.md`:
- Line 256: Add language specifiers to the three Markdown fenced code blocks
referenced: update the block showing the routing/path example that contains
"/admin/api/v1/sqs/queues → handleList" to use a text fence (e.g., ```text),
update the structured log example containing "admin.sqs.purge_queue" to use a
text/log fence (e.g., ```text or ```log), and update the Prometheus metric
example containing "elastickv_sqs_admin_purge_queue_total{queue, outcome}" to
use a text/prometheus fence (e.g., ```text or ```prometheus); this fixes the
markdownlint warning by ensuring each ``` block has an appropriate language
specifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6b8d205-e54c-4714-b800-88ce0c3340a9
📒 Files selected for processing (1)
docs/design/2026_05_16_proposed_admin_purge_queue.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbd4237533
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Path segments are extracted with `path.Clean` first (normalising leading / trailing / duplicated slashes and any `.` / `..` traversal), then split on `/`. The first segment after `queues/` is always the queue name; the rest (zero or more segments) is the sub-resource path. Sub-resource path matches: `[]` → name-only routes; `["messages"]` → messages sub-resource. Any other shape (`["messages", "x"]`, `["Messages"]`) returns 404. | ||
|
|
There was a problem hiding this comment.
Cleaning the full request path before segment validation can rewrite malformed targets into valid but different resources: for example, /admin/api/v1/sqs/queues//messages becomes /admin/api/v1/sqs/queues/messages. With this routing rule, a request that omitted the queue name could be interpreted as acting on a queue literally named messages (or even hit delete/describe routes), which risks purging/deleting the wrong queue instead of returning a validation error.
Useful? React with 👍 / 👎.
Codex P1 round 5 (line 275): `/admin/api/v1/sqs/queues//messages`
(omitted queue name, double slash) is normalised by path.Clean to
`/admin/api/v1/sqs/queues/messages`, at which point the first
post-`queues/` segment is the literal string "messages" and the
router would route the request as if the operator asked to act on
a queue NAMED "messages". That is a wrong-queue
purge/peek/delete hazard: a script bug that omits the ${QUEUE}
interpolation silently targets whichever queue is named "messages"
in the cluster.
Add an explicit empty-segment rejection BEFORE path.Clean: the
router inspects r.URL.EscapedPath() (raw, pre-Clean) and rejects
with 400 + {"code":"ValidationError", "message":"empty queue name
segment"} when any consecutive `/` pair appears inside the
`/queues/.../messages` segment range. The check fires pre-Clean so
the duplicate-slash signal is still visible (Clean collapses it).
Test plan §6.4 adds two cases pinning the new 400 paths.
The legal-but-quirky case "queue literally named messages" (the
operator did intentionally call `CreateQueue` with name=messages)
stays allowed — that's a real queue name and a Describe/Delete of
it should succeed.
Proposal-only — no code in this PR.
|
@claude review Round 5 — Codex P1 (line 275): Fix (doc-only): router inspects Legal-but-quirky case "queue literally named |
|
@claude review |
|
Claude finished @bootjp's task in 6m 24s —— View job Tasks
Claude's Review — r6Addressed since r5 ✅ Codex r5 P1 (empty queue segment, §3.4) — The fix is present: §3.4 now describes a pre-Clean check on Issues with the Codex r5 P1 Fix16. Doc claims the The paragraph says: "The same check rejects
More importantly, Remove the 17. Two promised 400 test cases not actually added to §6.4 (item 4 of §6) The fix paragraph ends with "Test plan §6.4 adds two 400 cases." But the actual §6 item 4 (Integration: The two cases must be explicitly enumerated in §6 item 4:
18. Check scope "inside the The doc says the The correct description: the router checks the raw Still Open from r5 — Phase-Blocking#13 (r5 second review):
func resolveActionConfig(cfg *sqsQueueThrottle, action string) (string, float64, float64) {
switch action {
case bucketActionSend: // "send"
if cfg.SendCapacity > 0 { return bucketActionSend, … }
case bucketActionReceive: // "recv"
if cfg.RecvCapacity > 0 { return bucketActionReceive, … }
}
if cfg.DefaultCapacity > 0 {
return bucketActionAny, cfg.DefaultCapacity, cfg.DefaultRefillPerSecond
// ^^^^^^^^^^^^^^^ = "*"
}
return action, 0, 0
}
The fix (add an explicit #14 (r5 second review): The doc (§3.2, line ~228) says The doc says var purgeErr *purgeRateLimitedError
if errors.As(err, &purgeErr) {
writeSQSError(w, http.StatusBadRequest, sqsErrPurgeInProgress,
fmt.Sprintf("…retry after %.0f seconds", purgeErr.remaining.Seconds()))
return
}The doc must state this explicitly in §3.2, not just say the handler "wraps" the error. #15 (r5 second review): Line ~228:
remaining := sqsPurgeRateLimitMillis * time.Millisecond // = 60_000 × 1_000_000 ns = 60_000 seconds, not 60The correct expression is: remaining := time.Duration(sqsPurgeRateLimitMillis-(now-meta.LastPurgedAtMillis)) * time.MillisecondReplace Cosmetic Finding (Still Open)§10 Summary still says "one storage round-trip" (§3.3 fix not mirrored) Line 542: "reverse-scan the catalog (prefix scan over Phase-Blocking Summary
Phase 2 can proceed once #14 and #15 are resolved in the doc. The SigV4 regression (#14) is the only finding that directly breaks existing behavior. Findings #13 and #16–#18 must be resolved before Phase 3 and Phase 4 respectively. |
|
Claude finished @bootjp's task in 6m 3s —— View job Tasks
Claude's Review — r6Addressed since r5 ✅ Codex P1 (empty queue segment, §3.4) — The design now specifies inspecting Still Open from r514. §3.2, line 228:
The "wraps that" claim is unresolvable by reading the code: there is no wrapping site. Commit to one of:
15. §3.2, line 228:
The correct Go expression is: remaining := time.Duration(sqsPurgeRateLimitMillis-(now-meta.LastPurgedAtMillis)) * time.Millisecond§10 Summary still says "one storage round-trip" — contradicts §3.3 fix §10, line 542:
§3.3 was fixed (Codex r4 P1) to say Stale line number in §3.3 — still points to §3.3 (line 252): "reusing the exact loop shape from Line 782 in the current file is #13 admin-peek bucket collision with consumer Default bucket — blocks Phase 3 The doc says:
The fix requires adding an explicit case to const bucketActionAdminPeek = "admin_peek"
case bucketActionAdminPeek:
if cfg.DefaultCapacity > 0 {
return bucketActionAdminPeek, cfg.DefaultCapacity, cfg.DefaultRefillPerSecond
}This ensures admin-peek maps to New Findings (r6)16. Empty-segment check ( §3.4 claims: "The same check rejects But the check is defined as: "any consecutive Either:
Commit to one and update the §3.4 description to match. 17. §6.4 missing the two promised 400 test cases The commit message for
The test plan at line 483 ends with the trailing-slash 18. §3.4 grammar in empty-segment paragraph is self-contradictory The sentence "The same check rejects Rollout Impact
Phase 2 cannot safely proceed until #14 (SigV4 regression) is resolved in the doc. #13 must be resolved before Phase 3. #16 and #17 must be resolved before Phase 4. Findings §10, stale line number, #15, and #18 are cheap fixes that should land in the next commit. |
Two Codex r9 findings:
1. Codex r9 P1 (line 45, peek confidentiality): the r9 doc said
peek uses session-auth only with no live-role re-check, but
peek returns FULL message bodies and attributes (not just the
metadata that List / Describe surface). A session JWT remains
valid for the configured TTL (1 hour default), so a key that is
revoked or downgraded after login can keep reading DLQ payloads
via peek until the token expires. The confidentiality gap is
unacceptable for DLQ triage.
§2.1 Goal 8 now requires a new principalForRead helper parallel
to principalForWrite: same RoleStore liveness check, just a
lower role threshold (RoleReadOnly vs RoleFull). nil RoleStore
(test fixtures) falls back to session-auth-only, same shape
principalForWrite already uses. Phase 4 ships both helpers
together. Existing List / Describe handlers are NOT changed in
this PR — tightening those is a separate scope.
2. Codex r9 P2 (line 285, collection-root dispatch corner case):
the routing procedure tried to detect the bare collection root
/admin/api/v1/sqs/queues via 'len(segments) == 0' after a
strings.TrimPrefix + strings.Split round-trip. Neither call
produces that outcome for the bare-root input: TrimPrefix on a
non-matching prefix returns the input unchanged, and Split('')
returns [''], a one-element slice — so the empty-slice test
never fires and the bare-root request would have flowed through
to the queue-name validator and been rejected as 'empty queue
name segment'. Adding a new step 2 that dispatches the exact
collection root (with or without trailing slash) to handleList
before any splitting sidesteps the corner case entirely.
Renumber: previous steps 1-5 become 1-6 (insertion at position 2).
All cross-references in the bad-inputs and good-inputs tables and
in the dot-segment / path.Clean clauses updated to match the new
numbering.
No code in this PR; doc-only update.
|
@claude review |
|
Claude finished @bootjp's task in 4m 33s —— View job Tasks
Claude's Review — r11Addressed since r10 ✅ Codex r10 P1 (live RoleStore re-check for AdminPeekQueue, §2.1 goal 8) — §2.1 goal 8 now correctly specifies a new ✅ Codex r10 P2 (collection root before splitting, §3.4) — Step 2 of the routing procedure now explicitly dispatches the collection root ( ✅ r10 finding #1 ( Still Open — Phase 4 Blockingr10 finding #2 — Error message Step 4 (line 284):
The message is accurate for only ONE of the four conditions:
§6.4 test plan (lines 540–545) pins An operator receiving a 400 for Fix: Replace New Findings#23 — §2.1 goal 8 describes the new helper as: "performs the same revocation check Reading if h.roles != nil {
live, exists := h.roles.LookupRole(principal.AccessKey)
if !exists { … 403 }
if !live.AllowsWrite() { … 403 } // ← this is THE role requirement
principal.Role = live
} else if !principal.Role.AllowsWrite() { … 403 }The phrase "same revocation check" in §2.1 is ambiguous: does "same check" include the The design's intent is unambiguous from §2.1: "Read-only role can peek but not purge." The
The difference from #24 — §6.4 missing revocation test for §6.4 integration tests include: "bridge bubbles This is exactly the security gap Codex r9 P1 flagged: without the live-role check, a revoked session JWT valid until its 1-hour TTL expiry can still read DLQ payloads. Without a test pinning the revocation rejection, a Phase 4 implementation that accidentally omits the Add one test case to §6.4:
The parallel test for purge already exists via Confirmed Correct (from source review)
Phase Gate Summary
Phase 2 (Purge) and Phase 3 (Peek backend) remain unblocked. The |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/2026_05_16_proposed_admin_purge_queue.md`:
- Line 300: Update the table row for the route
`/admin/api/v1/sqs/queues/orders/messages/` to make the success code
method-specific: indicate that after normalization via path.Clean to
`/orders/messages` the GET method returns 200 and the DELETE method returns 204
(rather than a single 204 for both); explicitly annotate the methods (GET: 200,
DELETE: 204) in that cell so the dual-method endpoint is unambiguous.
- Around line 272-277: The fenced code blocks in this markdown are missing
language identifiers and triggering MD040; update each triple-backtick fence
around the route list and the three metric/event blocks to include a language
tag (e.g., change ``` to ```text) so the blocks containing
"/admin/api/v1/sqs/queues…", "admin.sqs.purge_queue",
"elastickv_sqs_admin_purge_queue_total{queue, outcome}" and
"elastickv_sqs_admin_peek_queue_total{queue, outcome}" (and the other similar
fences referenced at lines ~450-457 and ~465-471) all start with ```text.
- Line 311: The note about trailing-slash normalization references the wrong
step: update the sentence that currently says the path "Cleans ... in step 4" to
instead reference step 5, since path.Clean is defined in step 5; locate the
paragraph mentioning path.Clean and change the step number accordingly so the
document's ordered procedure correctly points to path.Clean in step 5.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a08cec3-4e9c-4672-8233-7afb262a7233
📒 Files selected for processing (1)
docs/design/2026_05_16_proposed_admin_purge_queue.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0036dedcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ReceiveCount int32 // ApproximateReceiveCount | ||
| GroupID string // FIFO MessageGroupId, empty for standard | ||
| DeduplicationID string // FIFO MessageDeduplicationId, empty for standard | ||
| Attributes map[string]string // SQS message attributes |
There was a problem hiding this comment.
Define AdminPeekedMessage.Attributes with the same structured shape used by stored SQS message attributes (data type plus string/binary value), not map[string]string. In this repo, message records carry typed attributes (sqsMessageAttributeValue in adapter/sqs_messages.go), so flattening to plain strings would lose DataType and binary payloads for any non-string attribute. That breaks the document’s own “full body + every attribute” triage goal and can cause operators to purge messages without seeing the real attribute content.
Useful? React with 👍 / 👎.
Three CodeRabbit r10 minor findings: 1. Line 277 fence language (MD040): the routing-table fenced block lacked a language identifier. Add 'text' so markdown lint stays clean. 2. Line 300 method-specific status: the bad-inputs table row for '/queues/orders/messages/' (trailing slash) pinned '204' as the outcome, but only DELETE returns 204; GET (peek) returns 200. Row now reads '200 (GET peek) or 204 (DELETE purge)' so the method distinction is explicit. 3. Line 311 stale step reference: after the collection-root pre- split insertion (r10), path.Clean moved from step 4 to step 5. The trailing-slash note on line 311 still cited 'step 4'; updated to 'step 5'. All three are documentation hygiene; no semantic change. No code in this PR; doc-only update.
|
@claude review |
|
Claude finished @bootjp's task in 4m 31s —— View job Tasks
Claude's Review — r12Addressed since r11 ✅ r11 #1 (step 3 ✅ CodeRabbit r10 (method-specific success codes) — Table row for ✅ CodeRabbit r10 (step reference mismatch) — Line 311 now correctly says "Cleans to … in step 5". ✓ ✅ CodeRabbit r10 (fence language at line 272) — The routing-table fence is now Still Open — Phase-Blocking1. Error message Step 4 (line 284) fires
An operator or integration tool receiving Fix: Change the message in step 4 (line 284) and in all six §6.4 assertions from New Findings2. The latest Codex P2 comment (on line 97) flagged this but it has not been addressed. The actual SQS attribute type in type sqsMessageAttributeValue struct {
DataType string `json:"DataType"`
StringValue string `json:"StringValue,omitempty"`
BinaryValue []byte `json:"BinaryValue,omitempty"`
}The design's Attributes map[string]string // SQS message attributesA The Peek adapter at Phase 3 must reconstruct Fix: Define a typed attribute representation alongside type AdminPeekedMessageAttribute struct {
DataType string // "String", "Number", "Binary", or custom types
StringValue string // set when DataType is String or Number
BinaryValue []byte // set when DataType is Binary; base64-encoded on the JSON wire
}Change This change is Phase 3 scope but must be committed in the design before Phase 3 begins — the bridge layer's translation from Minor Finding3. Code fences at lines 450 and 465 still missing
Both trigger MD040 ( -```
+```text
admin.sqs.purge_queueand the same for line 465. The CodeRabbit r10 AI-agent prompt listed these two blocks under "Also applies to: 450-457, 465-471" but the commit only addressed line 272. Confirmed Correct (verified against source)
Phase Gate Summary
Phase 2 (Purge) remains fully unblocked — no Phase 2 findings exist. Finding #2 must be resolved before Phase 3 begins; finding #1 before Phase 4. Finding #3 is a cheap one-line fix alongside either. |
Codex r11 P2 (line 97): the prior draft typed AdminPeekedMessage.Attributes as map[string]string, which would have flattened the typed attribute set the storage layer carries in sqsMessageRecord.MessageAttributes (adapter/sqs_messages.go:86) — silently dropping the DataType discriminator and any binary payloads. A DLQ-triage operator would then look at peek output and have no way to tell that a message was routed there because of a binary or numeric attribute mismatch; the 'full body + every attribute' goal from §1 would be undermined exactly where it matters most. Add a typed struct AdminPeekedAttribute with DataType + StringValue + BinaryValue, mirroring the wire shape of sqsMessageAttributeValue. Update the peek-result JSON wire example in §3.4 to show the new attribute shape with a String, Number, and Binary entry so the SPA / downstream tooling can pin the schema. Binary uses Go's standard encoding/json behaviour ([]byte → base64), so binary payloads survive the SPA round-trip unchanged. No code in this PR; doc-only update.
|
@claude review |
|
Claude finished @bootjp's task in 4m 8s —— View job Tasks
Claude's Review — r12Addressed since r11 ✅ CodeRabbit r10/r11 minor cleanups (
✅ Codex r11 P2 (typed SQS attribute shape, "Source": { "data_type": "String", "string_value": "checkout" },
"Retry": { "data_type": "Number", "string_value": "2" },
"Payload": { "data_type": "Binary", "binary_value": "AAECAwQF" }The Number-as- Still Open — Phase 4 Blocking1. Error message Step 4 (line 304) pins:
And §6.4 (line 565–569) asserts every bad-input case returns the same body as line 564's The message is correct for exactly one case:
An operator receiving Fix: replace 2. §2.1 goal 8 (line 45):
Reading
The phrase "same revocation check" is intended to mean only step (1), but an implementer who copies There is also no
The difference from 3. §6.4 missing revocation test for §6.4 integration tests (line 560):
This tests:
It does not test the specific case This is the same gap Codex r9 P1 flagged in the auth design, now surfacing at the test-plan level. Add one test case to §6.4:
The parallel test for purge already exists via Confirmed Correct (verified against HEAD)
Phase Gate Summary
Phase 2 (Purge) and Phase 3 (Peek) remain fully unblocked. All prior Phase-2/3-blocking findings have been resolved across r1–r11. The typed attribute fix is clean and correct. Findings #1–#3 are all Phase 4 concerns; they are cheap to fix (one-paragraph edits for #2 and #3, one string substitution for #1) and should land before Phase 4 begins. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a493b0014
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - Standard / non-partitioned: `!sqs|msg|vis|<queue>|<gen>|<visible_at>|<msg_id>` | ||
| - Partitioned FIFO (`PartitionCount > 1`): `!sqs|msg|vis|p|<queue>|<partition>|<gen>|<visible_at>|<msg_id>` | ||
|
|
||
| For non-partitioned queues the walk is a single prefix scan over the first keyspace. For partitioned FIFO queues peek does **sequential partition scanning**: scan partition 0 to exhaustion (or until `Limit` is reached), then continue with partition 1, etc. The choice of sequential-rather-than-fanout matches `receiveMessage`'s existing dispatch and makes the cursor format trivial. A per-partition fanout would force the cursor to carry one offset per partition — Claude r2 flagged this as a concrete pagination concern and sequential scanning is the simpler resolution. |
There was a problem hiding this comment.
The partitioned-FIFO peek plan says to always scan partition 0 to exhaustion before moving on, which creates a strong sampling bias when low-numbered partitions stay hot: operators can page repeatedly and still never see messages from higher partitions without traversing a huge backlog. This contradicts the existing receive-path fairness guard in adapter/sqs_messages.go (lines 875-887), which explicitly rotates the starting partition to prevent starvation. Because this feature is for DLQ triage, hiding entire partitions can lead to incorrect purge decisions based on an unrepresentative view of queue contents.
Useful? React with 👍 / 👎.
Codex r12 P2 (line 146): the partitioned-FIFO peek plan said 'scan partition 0 to exhaustion, then partition 1, etc.'. With hot low-numbered partitions this creates a sampling bias — operators paginating through peek would never see high-numbered partitions until they traversed the entire backlog of every preceding one. For DLQ triage this is precisely the failure mode the feature exists to prevent: a misrepresentative sample could lead an operator to purge based on what they could see, not what is actually in the queue. receiveMessage (adapter/sqs_messages.go:875-887) already solves the same fairness problem via receiveFanoutCounters: a per-queue atomic counter that rotates the starting partition each call. Peek reuses the same counter on the first page (empty cursor), encoding the rotated start partition into the cursor's StartPartition field. Subsequent pages continue deterministically from the cursor; the walk terminates when Partition advances back around to StartPartition (full circle, all partitions traversed) or Limit is hit. Cursor schema extended (still 512-byte hard-cap): peekCursor.StartPartition uint32 - the rotated start peekCursor.Partition uint32 - current advance position Single-partition queues encode StartPartition=Partition=0 and skip the counter (no partition choice to make). Also collapsed the duplicate peekCursor struct definition the r10 edit left behind — there was only one cursor type, but two copies of its Go declaration. Single source of truth restored. No code in this PR; doc-only update.
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 883c01c055
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 2. **Dispatch the collection root before any splitting.** If the escaped path equals `/admin/api/v1/sqs/queues` (with or without trailing slash — i.e. equals the prefix exactly, or equals the prefix + `"/"`), the request is a list call; the handler returns immediately into `handleList`. Codex r9 P2 flagged that the earlier draft tried to detect this case via `len(segments) == 0` after trimming the prefix, but `strings.TrimPrefix` returns the input unchanged when the prefix does not match — and `strings.Split("", "/")` returns `[""]`, a one-element slice, not the empty slice — so the `len(segments) == 0` test never fired and a bare collection-root request would have flowed through to the queue-name validator and been rejected as "empty queue name segment". The explicit pre-split branch sidesteps the entire `Split` / `TrimPrefix` corner case. | ||
| 3. **Trim the route prefix** (`/admin/api/v1/sqs/queues/`) — the trailing slash is now part of the prefix so a match guarantees there is at least one further path segment — and split the remainder on `/`. The result is a slice of **raw** (still-escaped) segments. (A failed trim — i.e. the path neither equals the collection root from step 2 nor starts with the prefix-with-slash — falls through to the admin router's default 404 handler; the SQS sub-router never sees it.) | ||
| 4. **Validate the queue-name segment AND every other segment.** Reject with `400 Bad Request` + `{"code":"ValidationError","message":"empty queue name segment"}` when ANY of these hold: | ||
| - `segments[0] == ""` — the raw URL had `…/queues//…` (the empty string between two consecutive slashes ends up as an empty segment after split). |
There was a problem hiding this comment.
Step 4 only rejects an empty first segment (segments[0] == ""), so malformed paths like /admin/api/v1/sqs/queues/orders//messages pass validation and are normalized by path.Clean into a valid destructive route (/orders/messages). That means a structurally invalid URL can still trigger peek/purge instead of returning a validation error, which weakens the safety guarantees this routing hardening is trying to provide.
Useful? React with 👍 / 👎.
| 4. **Validate the queue-name segment AND every other segment.** Reject with `400 Bad Request` + `{"code":"ValidationError","message":"empty queue name segment"}` when ANY of these hold: | ||
| - `segments[0] == ""` — the raw URL had `…/queues//…` (the empty string between two consecutive slashes ends up as an empty segment after split). | ||
| - `strings.ContainsRune(segments[0], '%')` — any percent sign in the queue-name segment is illegal regardless of what it would decode to. AWS queue names match `sqsQueueNamePattern` (`^[a-zA-Z0-9_-]{1,80}(\.fifo)?$`, `adapter/sqs_catalog.go:65`); none of those characters need percent-encoding, so a queue-name segment containing `%` could only be an attempt to bypass the pre-`path.Clean` empty-segment check via a single-decode (`%2F`), double-decode (`%252F`), or arbitrary-depth-nested decode (`%25252F`, …) round-trip. Rejecting all `%`-containing segments at the source forecloses the entire decoding-collision class without iterating `url.PathUnescape` and without enumerating the encoded variants. Codex r6 P1 round-2 flagged that the previous "single `PathUnescape` decodes to empty" check would have missed `%252F` (one decode yields the literal string `%2F`, not the empty string); the contains-`%` rule sidesteps the bypass entirely. | ||
| - **Any segment equals `"."` or `".."`** — dot-segment traversal. `path.Clean` collapses `..` against the preceding segment, so an input like `/admin/api/v1/sqs/queues/orders/../messages` would pass an "only-validate-segments[0]" check (`segments[0] == "orders"`, a legal name), then `path.Clean` rewrites the trimmed path to `/messages` and step 6 dispatches against a queue literally named `messages` — a wrong-queue operation reachable through a single crafted URL. Codex r7 P1 flagged this as the next confused-deputy vector after the `%`-decoding class was closed. The fix is structural: ban `.` and `..` in **every** segment (queue name and sub-resource alike) at step 4, before `path.Clean` ever runs. AWS queue names cannot equal `.` or `..` anyway (the legal-character set excludes both), and the only sub-resource segment we recognise is the literal string `messages`, so the prohibition costs nothing for legitimate traffic. |
There was a problem hiding this comment.
The dot-segment guard checks only literal "." / ".." on raw escaped segments, so %2e/%2e%2e variants bypass step 4 and are decoded only later in step 6. This leaves a traversal-equivalent class unblocked by the documented validator (e.g. /admin/api/v1/sqs/queues/orders/%2E%2E/messages), so the procedure does not fully enforce the “ban dot segments in every segment” invariant it claims.
Useful? React with 👍 / 👎.
Two Codex r13 P2 findings, both about routing pre-check coverage: 1. Empty interior segment (line 308): the earlier draft only validated segments[0] for emptiness, so /admin/api/v1/sqs/queues/orders//messages would pass — then path.Clean would normalise it to /orders/messages and the structurally invalid URL would silently succeed as a peek/purge against a real queue. Step 4 now rejects empty segments in any position. 2. Percent-encoded dot-segments (line 310): the literal '.'/'..' check missed %2e / %2E / %2e%2e and similar — those would stay opaque past step 4 and only become traversal sequences at step 6 (decode), by which point path.Clean had already collapsed them and dispatched on the wrong queue. Step 4's '%-containing' rule now applies to every segment (not just segments[0]), catching all encoded dot-segment variants alongside the existing %2F / %252F bypass class. Both rules are a natural generalisation of what step 4 already did — same structural constraint (queue names can't contain percent signs, sub-resource is only ever the literal 'messages', neither needs percent-encoding), just applied uniformly. §3.4 bad-inputs table gains three rows; §6.4 test plan gains the matching 400 cases. Empty-interior is the structural-URL case; %2E%2E / %2e are the encoded-dot-segment cases. No code in this PR; doc-only update.
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af367446f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 4. **Validate every segment** (queue-name AND sub-resource). The rules apply uniformly to all elements of `segments`, not just `segments[0]` — Codex r13 P2 caught two cases where the earlier "queue-name-only" wording let bypass classes slip through. Reject with `400 Bad Request` + `{"code":"ValidationError","message":"invalid path segment"}` when ANY segment violates ANY of: | ||
| - `segment == ""` — empty segment, including the **interior** case `/admin/api/v1/sqs/queues/orders//messages` (Codex r13 P2 first finding). The earlier draft only checked `segments[0]`, so `path.Clean` would normalise `orders//messages` to `orders/messages` and the malformed URL would silently succeed as a peek/purge — exactly the structural-invalid-URL-with-destructive-routing failure the routing hardening exists to prevent. Empty segments in any position are rejected at step 4. | ||
| - `strings.ContainsRune(segment, '%')` — any percent sign in any segment is illegal. The original motivation was the `%2F` confused-deputy class on the queue-name position, but extending the same rule to sub-resource segments (Codex r13 P2 second finding) also catches **percent-encoded dot-segments** (`%2e`, `%2E`, `%2e%2e`, `%2E%2E`, etc.) that would otherwise slip past the literal `.`/`..` check below and only become traversal sequences at step 6 (decode) — by which time `path.Clean` has already run on the cleaned path. AWS queue names match `sqsQueueNamePattern` (`^[a-zA-Z0-9_-]{1,80}(\.fifo)?$`, `adapter/sqs_catalog.go:65`); the only legal sub-resource segment we recognise is the literal string `messages`. Neither needs percent-encoding for anything, so banning `%` in every segment forecloses the entire decoding-collision class — single-decode (`%2F`, `%2E`), double-decode (`%252F`, `%252E`), arbitrary-depth-nested decode (`%25252F`, …) — without iterating `url.PathUnescape` and without enumerating the encoded variants. |
There was a problem hiding this comment.
The ordered router flow rejects segment == "" in step 4 before path.Clean runs, so a valid trailing-slash request like /admin/api/v1/sqs/queues/orders/messages/ is split into [..., ""] and returned as 400 instead of being normalized to /orders/messages. This directly conflicts with the document’s stated behavior and tests that expect trailing-slash acceptance, so implementers following these steps will ship a handler that rejects the canonical peek/purge trailing-slash form.
Useful? React with 👍 / 👎.
| outcome ∈ {"ok", "forbidden", "not_leader", "not_found", "purge_in_progress", "internal_error"} | ||
|
|
||
| elastickv_sqs_admin_peek_queue_total{queue, outcome} | ||
| outcome ∈ {"ok", "forbidden", "not_leader", "not_found", "throttled", "internal_error"} |
There was a problem hiding this comment.
Both admin operations explicitly return validation errors (ErrAdminSQSValidation) for malformed names/cursors, but the proposed metric outcome enums omit any validation label. As written, those expected 400-class failures have no dedicated bucket and will be lumped into internal_error or dropped, which distorts alerting and makes operator mistakes indistinguishable from backend faults.
Useful? React with 👍 / 👎.
Codex r14 P1 (line 309): step 4's any-segment-empty rule fires on /admin/api/v1/sqs/queues/orders/messages/ because the split produces ['orders', 'messages', ''] and the trailing empty counts as an empty segment. This directly contradicts the documented trailing-slash-accepted policy (§3.4 trailing-slash note, §6.4 test plan) — implementers following the procedure literally would 400 the canonical trailing-slash peek/purge form. Fix: step 3 now strips a SINGLE trailing empty segment after the split, before step 4 validates. The convention matches HTTP-mux trailing-slash normalisation. Interior empties (/orders//messages splits into ['orders', '', 'messages'], trailing element is 'messages', no strip) still fall into step 4's empty-segment check and 400. Only the trailing empty is dropped, never an interior one. Codex r14 P2 (line 500): the metric outcome enums for both elastickv_sqs_admin_purge_queue_total and elastickv_sqs_admin_peek_queue_total omitted a 'validation' label, even though both endpoints explicitly return ErrAdminSQSValidation for 400-class rejections (empty / %-containing / dot-segment paths from §3.4 step 4; invalid cursor / oversize cursor / wrong-generation cursor from §3.1; empty / malformed queue name). Bucketing those into internal_error would distort alerting — operators paginating with a malformed cursor would look like a backend fault. Add 'validation' to both counters' outcome set, with a paragraph explaining why the four expected- fault classes (forbidden / not_leader / not_found / validation) stay separate from the unexpected internal_error class. No code in this PR; doc-only update.
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@claude review Round 10 — Codex P1 (line 45): peek exposes full message bodies/attributes (not just metadata like List/Describe), so the session-auth-only gate left a confidentiality gap — a revoked/downgraded session JWT could still read DLQ payloads via peek until the 1-hour token TTL elapsed. Fix: introduce Implementation-side caller audit fits in the code PR (new helper, no existing callers can be misaligned). Doc-only here. |
|
Claude finished @bootjp's task in 5m 53s —— View job Tasks
Claude's Review — r16Addressed since r12 (commits r13 through r15) ✅ Codex r12 P2 (partition rotation to avoid starvation, §3.1) — The design now commits to rotated sequential scanning ( ✅ Codex r13 P2 (extend step-4 validation to every segment, §3.4) — Step 4 now validates every element of ✅ Codex r13 P1 (trailing-slash split before step-4 validation, §3.4 r15) — Step 3 now drops the very last empty element before step 4 runs, so ✅ Codex r14 P2 (add Still Open — Phase 4 Blocking1. Error message inconsistency between step 4 spec and §6.4 test assertions — blocks Phase 4 Commit r14 updated the step 4 rejection message from Step 4 (line 307) now says: §6.4 (line 572) still asserts: and all subsequent test cases assert "the same body" — i.e., An implementation that returns Fix: update §6.4 to assert 2. §2.1 goal 8 (line 45):
"The same revocation check" is intended to mean only step (1). But a developer who copies There is also no
Still Open — Minor / Phase 43. §6.4 missing revocation test for §6.4 line 568: "bridge bubbles This tests nil session (wiring bug) and read-only-on-purge (role gate). It does NOT test the confidentiality gap that Add one test case: New Findings (r13–r15)4. Bad-inputs table trailing-slash row references wrong step after r15 fix — minor (§3.4) Line 327 (bad-inputs table):
After the r15 fix, the trailing slash is handled in step 3 (drop the last empty element: 5. §2.1 goal 1 states: peek "does NOT bump receive counts, does NOT start a visibility timer, and does NOT issue receipt handles — it is a pure read over the message keyspace." §3.1 (line 150) says: "peek reads-and-increments
Two options:
Either is acceptable; option (b) is the smaller change. The doc should either commit to (a) or add the §2.1 caveat. Phase Gate Summary
Phase 2 (Purge) and Phase 3 (Peek backend) remain fully unblocked. Finding #5 is minor enough that it does not block Phase 3 implementation — the behavior is correct, only the §2.1 description needs a caveat. Findings #1 and #2 must be resolved before Phase 4 (HTTP handlers) begins; #1 is a one-line change and #2 is a one-paragraph clarification. |
1 participant
Summary
AdminPurgeQueueadmin RPC +DELETE /admin/api/v1/sqs/queues/{name}/messagesHTTP endpoint + Web Console "Purge DLQ" button so operators can clear DLQ contents without dropping the queue.AdminQueueSummarygainsIsDLQ/DLQSourcesso the SPA labels the confirmation appropriately ("Purge DLQ" vs "Purge messages").purgeQueueWithRetryprimitive — same generation-bump semantics as the SigV4 wire path, same 60-second meta-stored rate limit.Per CLAUDE.md design-doc-first: this PR lands the doc; implementation ships in follow-up PRs phased per §7. No code in this PR.
Risk
Doc-only. No runtime change.
Five-lens self-review
IsDLQreverse-scan cost and the deferred reverse-index path.IsDLQstaleness is a display concern, not a correctness concern.Test plan
Summary by CodeRabbit