Add missing SQS operations and fix message lifecycle#15
Add missing SQS operations and fix message lifecycle#15codesoda wants to merge 3 commits intooneslash:mainfrom
Conversation
Implement DeleteMessage, ChangeMessageVisibility, GetQueueUrl, GetQueueAttributes, and SetQueueAttributes endpoints. Replace destructive pop with visibility-timeout-based receive semantics. Route SendMessage/ReceiveMessage dynamically via QueueUrl instead of hardcoded "myqueue". Add long polling support to ReceiveMessage. Remove tracked binaries and database files from repo, update .gitignore.
|
Hi @codesoda, wow thank you :) It was my small Rust learning project, I will merge after the codex review |
|
@codex review |
![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: ece834c2b0
ℹ️ 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".
README.md
Outdated
|
|
||
| ```bash | ||
| $ git clone git@github.com:oneslash/sqs-navbat.git | ||
| $ git clone git@github.com:TeamCadenceAI/sqs-navbat.git |
There was a problem hiding this comment.
Repository has been changed, could you revert it please?
Review fixes (from oneslash and Codex review): - Revert README git clone URL back to oneslash/sqs-navbat - GetQueueUrl: resolve queue existence from DB instead of in-memory map so queues are discoverable after process restarts (P1) - ReceiveMessage: use stored first_received_at timestamp for ApproximateFirstReceiveTimestamp instead of current time (P2) - GetQueueAttributes: surface DB errors as 500 responses instead of silently returning empty attributes via unwrap_or_default() (P2) Also fix clippy warnings across modified files (needless returns, dead_code annotations, manual clamp, empty string comparison).
|
@oneslash I addressed your feedback and updated the PR. Btw, you should consider using my x-agent skills for running cargo toolchains, uses less tokens and helps make sure clippy warnings and test errors are handled which a coding agent is doing it's thing. |
|
@codex review |
|
thank you @codesoda! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e312473c98
ℹ️ 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".
| let reader = app_state.queues.lock().await; | ||
| if !reader.contains_key(&queue_name) { |
There was a problem hiding this comment.
This endpoint treats a queue as non-existent if it is missing from app_state.queues, but queue metadata is persisted in SQLite while the in-memory map starts empty on process boot. That means after a restart, persisted queues can still exist in the DB yet SetQueueAttributes will always return AWS.SimpleQueueService.NonExistentQueue for them, blocking attribute updates for valid queues. The existence check should come from persistent storage (or the map should be hydrated at startup) instead of only contains_key.
Useful? React with 👍 / 👎.
| if let Some(vt) = attrs.get("VisibilityTimeout") { | ||
| if let Ok(timeout) = vt.parse::<u32>() { | ||
| let mut writer = app_state.queues.lock().await; | ||
| if let Some(queue) = writer.get_mut(&queue_name) { | ||
| queue.default_visibility_timeout = timeout; |
There was a problem hiding this comment.
The handler updates queue.default_visibility_timeout in memory before attempting service.set_queue_attributes(...). If the DB upsert fails, the API returns 500 but the runtime queue has already changed, so subsequent receives use a timeout that was never durably stored. This creates inconsistent state after a failed request; defer the in-memory mutation until after persistence succeeds (or roll it back on error).
Useful? React with 👍 / 👎.
| if let Some(pos) = self | ||
| .messages | ||
| .iter() | ||
| .position(|m| m.receipt_handle.as_deref() == Some(handle)) |
There was a problem hiding this comment.
Message deletion accepts any matching receipt_handle without checking whether the message is still in-flight. When visibility expires, the old handle remains attached to the now-visible message, so a stale consumer can still delete it later and lose work that should have been eligible for redelivery. Handle matching should require visible_at > now (and similarly for visibility changes), or handles should be cleared when messages become visible again.
Useful? React with 👍 / 👎.
|
I will fix later :) |
2 participants
Summary
DeleteMessage,ChangeMessageVisibility,GetQueueUrl,GetQueueAttributes,SetQueueAttributes.gitignoreentries for SQLite DB files, compiled test binaries, and misc artifacts; remove accidentally committed binary/DB filesDetails
New Operations
Message Lifecycle Fix
Previously,
ReceiveMessagedeleted messages from the database on read. Now messages follow the standard SQS lifecycle:visiblestatusin_flightwith a visibility timeoutDeleteMessageOther Changes
.gitignorerules for*.db,*.db-shm,*.db-wal, test binaries, andattributes.jsondatabase.db*,attributes.json, andtest-app/test_kafka_appbinaryTest plan
cargo buildto verify compilationSendMessage→ReceiveMessage→DeleteMessagelifecycleChangeMessageVisibilityextends timeout on in-flight messagesGetQueueUrl/GetQueueAttributes/SetQueueAttributes