app/src/main/java/me/capcom/smsgateway/modules/webhooks/workers/WebhookQueueProcessorWorker.kt (1)

303-304: Distinguish missing payload files from retriable send failures.

WebhookPayloadStorage.read() returning null means the referenced file is already gone. Returning false here sends the row through the normal retry path, but retries cannot reconstruct that payload and only burn attempts on an unrecoverable condition. Consider surfacing a terminal failure result and logging it explicitly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/workers/WebhookQueueProcessorWorker.kt`
around lines 303 - 304, WebhookPayloadStorage.read(webhook.payload) returning
null indicates a terminal missing payload; don't treat it as a retriable send
failure. Update the processing in WebhookQueueProcessorWorker (the block using
payloadStorage.read(...) and gson.fromJson(...)) to detect a null payload, log
an explicit error including webhook.id and webhook.payload, and return/emit a
terminal failure result (not false) so the retry mechanism won’t requeue; only
proceed to gson.fromJson(...) and normal retry semantics when payload is
non-null.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt (1)

63-65: ⚠️ Potential issue | 🟠 Major

Delete the stored payload ref after the status update.

As implemented in app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebhookPayloadStorage.kt:15-19, save() persists file:<key>. Lines 64 and 101 delete the raw webhookId before the DAO update, so completed/permanently-failed rows leak their payload files, and a DAO error here would leave a live row pointing at a missing payload. Pass the stored payload ref through these methods and delete it only after the status change succeeds.

🧹 Minimal fix

-    suspend fun completeWebhook(webhookId: String) {
-        payloadStorage.delete(webhookId)
-        dao.markAsCompleted(id = webhookId)
+    suspend fun completeWebhook(webhookId: String, payloadRef: String) {
+        dao.markAsCompleted(id = webhookId)
+        payloadStorage.delete(payloadRef)
     }
@@
-            permanentlyFailWebhook(
-                webhookId = webhookId,
-                error = error ?: "Max retries exceeded",
-            )
+            permanentlyFailWebhook(
+                webhookId = webhookId,
+                payloadRef = webhook.payload,
+                error = error ?: "Max retries exceeded",
+            )
@@
-    suspend fun permanentlyFailWebhook(webhookId: String, error: String) {
-        payloadStorage.delete(webhookId.toString())
-        dao.markAsPermanentlyFailed(
+    suspend fun permanentlyFailWebhook(webhookId: String, payloadRef: String, error: String) {
+        dao.markAsPermanentlyFailed(
             id = webhookId,
             error = error
         )
+        payloadStorage.delete(payloadRef)
     }

You’ll also need to pass webhook.payload from app/src/main/java/me/capcom/smsgateway/modules/webhooks/workers/WebhookQueueProcessorWorker.kt:246-248.

Also applies to: 90-105

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt`
around lines 63 - 65, The code deletes the stored payload by key before updating
DB, causing leaked files or broken rows on DAO errors; modify completeWebhook
(and the analogous failure method used at 90-105) to accept and propagate the
stored payload ref (returned by WebhookPayloadStorage.save, e.g., the
"file:<key>" value) instead of deleting by webhookId, call
dao.markAsCompleted/markAsPermanentlyFailed first, and only call
payloadStorage.delete(payloadRef) after the DAO update succeeds; update callers
(notably WebhookQueueProcessorWorker where you currently pass webhookId at lines
~246-248) to pass webhook.payload (the stored payload ref) through into these
repository methods.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt (1)

65-67: ⚠️ Potential issue | 🟠 Major

Persist the terminal state before deleting the payload file.

Lines 66 and 103 delete first. If the DAO update fails afterward, the queue entry stays live but its backing payload is gone, so retries or stuck-worker recovery cannot re-read it.

🩹 Minimal fix

     suspend fun completeWebhook(webhookId: String) {
-        payloadStorage.delete(webhookId)
         dao.markAsCompleted(id = webhookId)
+        payloadStorage.delete(webhookId)
     }
@@
     suspend fun permanentlyFailWebhook(webhookId: String, error: String) {
-        payloadStorage.delete(webhookId.toString())
         dao.markAsPermanentlyFailed(
             id = webhookId,
             error = error
         )
+        payloadStorage.delete(webhookId)
     }

Also applies to: 102-107

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt`
around lines 65 - 67, completeWebhook currently deletes the payload via
payloadStorage.delete(webhookId) before persisting the terminal state with
dao.markAsCompleted(id = webhookId); change the order so you first call
dao.markAsCompleted(...) and only upon successful completion delete the payload.
Ensure the dao.markAsCompleted call is awaited/checked for success and
handle/propagate errors so payload is not removed on failure; apply the same
change to the other method that also calls payloadStorage.delete before updating
the DAO (i.e., any code paths using payloadStorage.delete + dao.* update).

app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueDao.kt (1)

20-21: ⚠️ Potential issue | 🟠 Major

Make getById() nullable.

Line 21 cannot represent a missing row safely. This lookup is used on recovery paths, so WebhookQueueEntity? is the right contract; otherwise callers can only discover "not found" via a runtime crash.

🩹 Minimal fix

-    suspend fun getById(id: String): WebhookQueueEntity
+    suspend fun getById(id: String): WebhookQueueEntity?

Then update downstream callers to handle the null case explicitly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueDao.kt`
around lines 20 - 21, Change the DAO contract for the lookup to return a
nullable entity so missing rows are represented safely: update the suspend
function WebhookQueueDao.getById(id: String): WebhookQueueEntity to return
WebhookQueueEntity? and then locate and update all downstream callers that
invoke getById to handle the null case explicitly (e.g., recovery path branches
or early returns) instead of assuming a non-null result.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt (1)

24-28: ⚠️ Potential issue | 🔴 Critical

Critical: Duplicate payloadStorage.save() call leaks files.

payloadStorage.save(id, payload) is called twice—once at line 25 (outside try) and again at line 28 (inside try). The outer payloadRef is shadowed and never used, so a file is written and immediately orphaned on every enqueue. Remove lines 25-26.

🐛 Proposed fix

     ): String {
         val id = NanoIdUtils.randomNanoId()
-        val payloadRef = payloadStorage.save(id, payload)
-
         try {
             val payloadRef = payloadStorage.save(id, payload)
             dao.insertWebhook(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt`
around lines 24 - 28, The code calls payloadStorage.save(id, payload) twice
which creates orphaned files; remove the redundant call so payload is saved
exactly once inside the try block. Specifically, delete the outer val payloadRef
= payloadStorage.save(id, payload) and ensure the single payloadRef declared
inside the try (from payloadStorage.save(id, payload)) is the one used/returned
by the enclosing method (e.g., in WebhookQueueRepository functions that call
NanoIdUtils.randomNanoId() and payloadStorage.save). Also ensure you don't
shadow payloadRef and that any subsequent logic references the try-block
payloadRef.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt (2)

38-40: ⚠️ Potential issue | 🟠 Major

Delete the saved payload by payloadRef, not the webhook id.

WebhookQueueEntity.payload stores the reference returned by payloadStorage.save(...). If insertWebhook() throws, delete(id) will miss a file:... ref and leave the file orphaned.

🩹 Minimal fix

         } catch (e: Exception) {
-            payloadStorage.delete(id)
+            payloadStorage.delete(payloadRef)
             throw e
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt`
around lines 38 - 40, In the catch block inside WebhookQueueRepository (around
the insertWebhook flow) replace the call to payloadStorage.delete(id) with
deletion using the saved payload reference (the value returned by
payloadStorage.save, e.g., payloadRef or WebhookQueueEntity.payload) so the
stored file ref (file:...) is removed if insertWebhook() throws; ensure you
reference the same variable used when calling payloadStorage.save(...) rather
than the webhook id.

---

70-72: ⚠️ Potential issue | 🟠 Major

Use the stored payload ref here too, and delete it only after the DAO update succeeds.

These paths currently delete webhookId, which cannot resolve the stored file:... ref. That leaks files today, and the pre-update delete ordering is still risky because a successful cleanup plus failed DAO write would leave a live row without its payload.

🩹 Minimal fix

-    suspend fun completeWebhook(webhookId: String) {
-        payloadStorage.delete(webhookId)
-        dao.markAsCompleted(id = webhookId)
+    suspend fun completeWebhook(webhookId: String, payloadRef: String) {
+        dao.markAsCompleted(id = webhookId)
+        payloadStorage.delete(payloadRef)
     }
@@
             permanentlyFailWebhook(
                 webhookId = webhookId,
+                payloadRef = webhook.payload,
                 error = error ?: "Max retries exceeded",
             )
@@
-    suspend fun permanentlyFailWebhook(webhookId: String, error: String) {
-        payloadStorage.delete(webhookId.toString())
+    suspend fun permanentlyFailWebhook(webhookId: String, payloadRef: String, error: String) {
         dao.markAsPermanentlyFailed(
             id = webhookId,
             error = error
         )
+        payloadStorage.delete(payloadRef)
     }

Also applies to: 97-100, 107-112

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt`
around lines 70 - 72, In completeWebhook (and the other similar methods around
the other occurrences), read the stored payload reference from the DAO/row (the
file:... ref) and use that when deleting from payloadStorage instead of passing
webhookId; also move the delete to after the DAO update so the sequence is:
fetch payloadRef from the record, call dao.markAsCompleted/markAsFailed/etc.
with the id, then if the DAO update succeeded call
payloadStorage.delete(payloadRef); update code paths that currently call
payloadStorage.delete(webhookId) to use the retrieved payloadRef and ensure
deletion happens post-DAO-write to avoid leaks or orphaned rows (refer to
payloadStorage and dao.markAsCompleted/markAsFailed/whatever methods to locate
the spots).

app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt (1)

70-73: ⚠️ Potential issue | 🟠 Major

Delete payload after successful DB update, not before.

If dao.markAsCompleted() throws after the payload is deleted, the queue row remains with status processing but its payload file is gone. Subsequent retry attempts will fail to read the payload, leaving the webhook in a corrupted state until it becomes permanently failed.

🩹 Proposed fix

 suspend fun completeWebhook(webhookId: String) {
-    payloadStorage.delete(webhookId)
     dao.markAsCompleted(id = webhookId)
+    payloadStorage.delete(webhookId)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt`
around lines 70 - 73, The current completeWebhook function deletes the payload
before updating the DB which can leave the queue row stuck in "processing"
without its payload if dao.markAsCompleted(id = webhookId) throws; change the
ordering so dao.markAsCompleted(webhookId) is executed first and only call
payloadStorage.delete(webhookId) after it returns successfully, and optionally
wrap dao.markAsCompleted in a try/catch to avoid deleting on failure (or rethrow
after ensuring payload remains); references: completeWebhook,
payloadStorage.delete, dao.markAsCompleted.

app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt (1)

120-122: Deletion by ID is correct, but consider transaction safety.

The IDs returned by getOldEntryIds are NanoIds (the storage keys), so payloadStorage.delete(it) correctly removes the files. However, if dao.cleanupOldEntries() fails mid-way, some files may be deleted while their DB rows persist, or vice versa if new entries are added between the two calls. For robustness, consider wrapping in a transaction or accepting eventual consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebhookQueueRepository.kt`
around lines 120 - 122, The current sequence in WebhookQueueRepository (calling
dao.getOldEntryIds, payloadStorage.delete for each id, then
dao.cleanupOldEntries) can leave storage and DB out-of-sync if
dao.cleanupOldEntries fails; wrap the delete operations in a DB transaction or
switch to a two-phase approach: begin a transaction in the DAO (or add a
dao.markEntriesForDeletion method), fetch and mark the entries (e.g., set a
pending_delete flag) via WebhookQueueRepository, commit the DB change, then
delete payloadStorage files for the marked NanoId keys and finally remove rows
with dao.cleanupMarkedEntries (or perform deletions inside the same transaction
if your storage supports transactional semantics) so that functions
dao.getOldEntryIds, dao.cleanupOldEntries, and payloadStorage.delete are
coordinated and atomic.

app/src/main/java/me/capcom/smsgateway/modules/webhooks/workers/WebhookQueueProcessorWorker.kt (1)

371-373: HTTP client closed after every request creates unnecessary overhead.

The client.close() in the finally block means a new HttpClient instance is created and destroyed for each webhook. This is inefficient when processing batches. Consider making the client a class-level property with lazy initialization and closing it in a cleanup method, or reusing across the batch.

♻️ Suggested improvement

-    private val client
-        get() = HttpClient(OkHttp) {
+    private val client by lazy {
+        HttpClient(OkHttp) {
             install(HttpTimeout) {
                 ...
             }
             ...
         }
+    }

Then remove the client.close() from the finally block, or close it once at worker completion.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/workers/WebhookQueueProcessorWorker.kt`
around lines 371 - 373, The HttpClient is being closed in the finally block for
each webhook which forces recreation per request; move the client to a
class-level property in WebhookQueueProcessorWorker (e.g., a private lateinit or
lazy val named client) so it is created once and reused across batch processing
(the code that currently constructs the local client and calls client.close() in
the finally should stop doing that), remove the per-request client.close() call,
and add a single cleanup/shutdown method on WebhookQueueProcessorWorker (or hook
into the worker lifecycle) to close the client once when the worker completes or
is disposed.