reports are not removed from database after being successfully sent

[freddieleeman]

Tested with MySQL and SQLite database with internal SMTP and smarthost on multiple systems (Jessie / Buster / Bullseye / Perl 5.20 / 5.28 / 5.30) with same result . After a 250 response the report is not cleaned from the database and is being sent over and over again when dmarc_send_report is started.

'dmarc_send_report --verbose' output:

SMTP accepted
        250 OK id=1id9gb-0006hb-QB

Sending error query called by Mail::DMARC::Report::Store::SQL, 146
        DELETE FROM report_record_reason WHERE report_record_id IN (2)
         at /usr/local/share/perl/5.28.1/Mail/DMARC/Report/Store.pm line 12.

[msimerson]

what version of mail::dmarc?

[freddieleeman]

Generated with Mail::DMARC 1.20191025

Something I'm noticing now is that the first ID gives me an error with and SQL query on report_record_spf, all the following errors on report_record_reason.

ID:     1
Domain: <hidden>
rua:    mailto:<hidden>
getting MX for <hidden>
delivering message to <hidden> via <hiddden>
SMTP accepted
        250 2.0.0 OK  1575788031 ec6si13065719ejb.174 - gsmtp

Sending error query called by Mail::DMARC::Report::Store::SQL, 146
        DELETE FROM report_record_spf WHERE report_record_id IN (1)
         at /usr/local/share/perl/5.20.2/Mail/DMARC/Report/Store.pm line 12.

ID:     2
Domain: <hidden>
rua:    mailto:<hidden>
getting MX for <hidden>
delivering message to <hidden> via <hiddden>
SMTP accepted
        250 2.0.0 Ok: queued as 94D16139593

Sending error query called by Mail::DMARC::Report::Store::SQL, 146
        DELETE FROM report_record_reason WHERE report_record_id IN (2)
         at /usr/local/share/perl/5.20.2/Mail/DMARC/Report/Store.pm line 12.

ID:     3
Domain: <hidden>
rua:    mailto:<hidden>
getting MX for <hidden>
server does not support STARTTLS
        SSL connection failed
getting MX for <hidden>
delivering message to <hidden> via <hiddden>
SMTP accepted
        250 OK

Sending error query called by Mail::DMARC::Report::Store::SQL, 146
        DELETE FROM report_record_reason WHERE report_record_id IN (3)
         at /usr/local/share/perl/5.20.2/Mail/DMARC/Report/Store.pm line 12.

ID:     4
Domain: <hidden>
rua:    mailto:<hidden>
getting MX for <hidden>
delivering message to <hidden> via <hiddden>
SMTP accepted
        250 OK id=1idqSA-0006B4-Jj

Sending error query called by Mail::DMARC::Report::Store::SQL, 146
        DELETE FROM report_record_reason WHERE report_record_id IN (4,5,10,13)
         at /usr/local/share/perl/5.20.2/Mail/DMARC/Report/Store.pm line 12.

ID:     5
Domain: <hidden>
rua:    mailto:<hidden>
getting MX for <hidden>
delivering message to <hidden> via <hiddden>
SMTP accepted
        250 OK id=1idqSG-0006BD-8o

Sending error query called by Mail::DMARC::Report::Store::SQL, 146
        DELETE FROM report_record_reason WHERE report_record_id IN (6,9,11,12)
         at /usr/local/share/perl/5.20.2/Mail/DMARC/Report/Store.pm line 12.

ID:     6
Domain: <hidden>
rua:    mailto:<hidden>
getting MX for <hidden>
delivering message to <hidden> via <hiddden>
SMTP accepted
        250 2.0.0 Ok: queued as 037522EE358

Sending error query called by Mail::DMARC::Report::Store::SQL, 146
        DELETE FROM report_record_reason WHERE report_record_id IN (7)
         at /usr/local/share/perl/5.20.2/Mail/DMARC/Report/Store.pm line 12.

ID:     7
Domain: <hidden>
rua:    mailto:<hidden>
getting MX for <hidden>
delivering message to <hidden> via <hiddden>
SMTP accepted
        250 OK id=1idqSO-0006BM-Jf

Sending error query called by Mail::DMARC::Report::Store::SQL, 146
        DELETE FROM report_record_reason WHERE report_record_id IN (8)
         at /usr/local/share/perl/5.20.2/Mail/DMARC/Report/Store.pm line 12.

[msimerson]

For now, I'd recommend reverting to version v1.20191004. There's a bunch of new SQL code that landed recently. I'll look into this further but it may be a few weeks before I have time to identify the exact problem and get a fix published.

[freddieleeman]

I can confirm that version 1.20191004 works like a charm! It might be wise to revert the changes to the master that cause these errors. If a large MTA updates the module they might start spamming DMARC mailboxes and the amount will increase daily.

[msimerson]

Good point. I've just deleted 1.20191024 from PAUSE, so folks that use the normal perl release channels won't get it.

Since you have an environment set up to test, can you try this patch and see if it makes any difference?

diff --git a/lib/Mail/DMARC/Report/Store/SQL.pm b/lib/Mail/DMARC/Report/Store/SQL.pm
index 320061f..2220f40 100644
--- a/lib/Mail/DMARC/Report/Store/SQL.pm
+++ b/lib/Mail/DMARC/Report/Store/SQL.pm
@@ -136,15 +136,12 @@ sub delete_report {
     print "deleting report $report_id\n" if $self->verbose;

     # deletes with FK don't cascade in SQLite? Clean each table manually
-    my $rows = $self->query( $self->grammar->report_record_id,
-        [$report_id] );
+    my $rows = $self->query( $self->grammar->report_record_id, [$report_id] );
     my $row_ids = join( ',', map { $_->{id} } @$rows ) or return 1;
-    foreach my $table (
-        qw/ report_record_spf report_record_dkim report_record_reason /)
-    {
+
+    foreach my $table (qw/ report_record_spf report_record_dkim report_record_reason /) {
         print "deleting $table rows $row_ids\n" if $self->verbose;
-        $self->query(
-            $self->grammar->delete_from_where_record_in($table, $row_ids));
+        $self->query( $self->grammar->delete_from_where_record_in($table), $row_ids);
     }
     foreach my $table (qw/ report_policy_published report_record report_error /) {
         $self->query( $self->grammar->delete_from_where_report($table), [$report_id] );
diff --git a/lib/Mail/DMARC/Report/Store/SQL/Grammars/MySQL.pm b/lib/Mail/DMARC/Report/Store/SQL/Grammars/MySQL.pm
index 7e0a899..55c734d 100644
--- a/lib/Mail/DMARC/Report/Store/SQL/Grammars/MySQL.pm
+++ b/lib/Mail/DMARC/Report/Store/SQL/Grammars/MySQL.pm
@@ -30,8 +30,8 @@ sub report_record_id {
 }

 sub delete_from_where_record_in {
-    my ($self, $table, $row_ids) = @_;
-    return "DELETE FROM $table WHERE report_record_id IN ($row_ids)"
+    my ($self, $table) = @_;
+    return "DELETE FROM $table WHERE report_record_id IN (??)"
 }

 sub delete_from_where_report {
diff --git a/lib/Mail/DMARC/Report/Store/SQL/Grammars/PostgreSQL.pm b/lib/Mail/DMARC/Report/Store/SQL/Grammars/PostgreSQL.pm
index a68fad3..1810785 100644
--- a/lib/Mail/DMARC/Report/Store/SQL/Grammars/PostgreSQL.pm
+++ b/lib/Mail/DMARC/Report/Store/SQL/Grammars/PostgreSQL.pm
@@ -32,7 +32,7 @@ sub report_record_id {

 sub delete_from_where_record_in {
     my ($self, $table, $row_ids) = @_;
-    return "DELETE FROM \"$table\" WHERE \"report_record_id\" IN ($row_ids)"
+    return "DELETE FROM \"$table\" WHERE \"report_record_id\" IN (??)"
 }

 sub delete_from_where_report {
diff --git a/lib/Mail/DMARC/Report/Store/SQL/Grammars/SQLite.pm b/lib/Mail/DMARC/Report/Store/SQL/Grammars/SQLite.pm
index 691b9d4..569a5c7 100644
--- a/lib/Mail/DMARC/Report/Store/SQL/Grammars/SQLite.pm
+++ b/lib/Mail/DMARC/Report/Store/SQL/Grammars/SQLite.pm
@@ -30,8 +30,8 @@ sub report_record_id {
 }

 sub delete_from_where_record_in {
-    my ($self, $table, $row_ids) = @_;
-    return "DELETE FROM $table WHERE report_record_id IN ($row_ids)"
+    my ($self, $table) = @_;
+    return "DELETE FROM $table WHERE report_record_id IN (??)"
 }

 sub delete_from_where_report {

[freddieleeman]

Tried it but didn't fix the issue. Same errors.

[marcbradshaw]

Just a thought, have you considered using DBIx::Class to handle the different database abstractions?

[msimerson]

I've found the issue that @freddieleeman surfaced here, I just haven't had time to write the PR, add tests, and get it published.

DBIx::Class might be a better approach. I used it once a long long time ago. It was huge, which violated some of my sensibilities. However, the alternative is that to support more SQL engines, this module gets larger and more complicated, which isn't a winning strategy.

As I was digging through the guts of Mail::DMARC yesterday, it occurred to me that SQL may no longer be the best approach. SQL is just a bunch of abstraction and complexity that permits us to store and query some XML documents efficiently. A far far simpler, just as performant, and so far as I've thought this through, better approach would be a JSON document store. It can be a standard one like MongoDB or Elasticsearch, or even an RDBMS' (like MySQL) JSON data type.

[msimerson]

FWIW, the code at the heart of this issue is code that I wrote years ago that handles SQLite's lack of cascading foreign key constraints. It was tickled by the recent SQL changes and the tests no longer exercised that deletion code, so the subtle error snuck by.

Regarding SQL, I don't see myself implementing DBIx::Class (short of someone sponsoring the work) but I would welcome such a PR.

[marcbradshaw]

The refactor in #148 will make report sending behavior much easier to test, I'll add something there to test that reports are deleted after being sent.

I have no particular opinion one way or the other on using a JSON document store. For us, it would mean thinking about what the infrastructure for that looks like. That said, the current model does have some race issues with multi master replication that would likely not be an issue with JSON.