Skip to content

Conversation

@norkans7
Copy link
Contributor

… spool file

@norkans7 norkans7 marked this pull request as ready for review October 10, 2025 09:08
Copilot AI review requested due to automatic review settings October 10, 2025 09:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds duplicate message checking functionality to prevent reprocessing messages that already exist in the database. When message insertion fails during spooling, the system now checks if the message UUID already exists before considering it a failure.

  • Added a checkDuplicate function to query for existing messages by UUID
  • Modified the spool file flushing logic to check for duplicates when message insertion fails

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

if err != nil {
return err // fail? oh well, we'll try again later
id, err := checkDuplicate(ctx, b.rt, m.UUID_)
if id == models.NilMsgID || err != nil {
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is misleading. When id != models.NilMsgID, there's no error in checking for duplicates - a duplicate was successfully found. The error logging should only occur when err != nil.

Suggested change
if id == models.NilMsgID || err != nil {
if err != nil {

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

❌ Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.63%. Comparing base (184e19e) to head (f6247e5).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
backends/rapidpro/msg.go 0.00% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #899      +/-   ##
==========================================
- Coverage   67.72%   67.63%   -0.10%     
==========================================
  Files         118      118              
  Lines       13156    13174      +18     
==========================================
  Hits         8910     8910              
- Misses       3577     3595      +18     
  Partials      669      669              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@norkans7 norkans7 requested a review from Copilot October 10, 2025 12:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 291 to 296
return dupErr // fail? oh well, we'll try again later
}
if id == models.NilMsgID {
return err // fail? oh well, we'll try again later
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment 'fail? oh well, we'll try again later' is inconsistent with the actual behavior. The function returns the error immediately, preventing retry, but the comment suggests it will retry later.

Suggested change
return dupErr // fail? oh well, we'll try again later
}
if id == models.NilMsgID {
return err // fail? oh well, we'll try again later
return dupErr // return error immediately; no retry attempted here
}
if id == models.NilMsgID {
return err // return error immediately; no retry attempted here

Copilot uses AI. Check for mistakes.
@norkans7 norkans7 force-pushed the fix-duplicate-writting branch from 27dbafe to f6247e5 Compare October 10, 2025 12:35
@rowanseymour
Copy link
Member

I feel like this just hides the problem.. I wanna see if updating sqlx or the underlying postgres driver can make that query behave properly when the context has timed out.

@norkans7 norkans7 marked this pull request as draft October 10, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants