fix: Prevent possible infinite loop with invalid smtp row

[Hocuri]

If Message::load_from_db_optional() or set_msg_failed() fails, we shouldn't early-return. Because it's important that the line execute("DELETE FROM smtp WHERE id=?", (rowid,)) is executed in order to prevent an infinite loop, if one of these functions fails.

[link2xt]

Can we just move the DELETE FROM smtp above?

[Hocuri]

We can, but why? It seems weird to abort sending messages, early-return from send_smtp_messages(), and wait 30s, because we didn't manage to mark a message as failed.

Or do you mean, in addition to what I already did in the PR here?

[link2xt]

It seems weird to abort sending messages, early-return from send_smtp_messages(), and wait 30s, because we didn't manage to mark a message as failed.

If we fail to mark the message as failed there is something really wrong with the database or the filesystem or hardware. Ignoring errors everywhere just adds more untested codepaths.

We also just got another one case in another PR, also making more codepaths:
#7738 (comment)

[iequidoo]

Can we just move the DELETE FROM smtp above?

I agree that in this particular function it's better to move DELETE FROM smtp above. If we want to ignore some errors in the SMTP loop, then better to do that directly in that loop.

EDIT: And send_msg_to_smtp() is a quite complex function, so we can't predict all scenarios when it can fail and it's better to ignore the error if it fails for a particular message.

[Hocuri]

Early-returning with ? also adds another code path, but it's not really important. I just moved the DELETE, so, the infinite loop is definitely fixed now.

[iequidoo]

I think that the real problem isn't "new code paths", but rather a state when a message remains in InPending without any SMTP rows, but this shouldn't happen unless there's an app crash or so.

[link2xt]

Early-returning with ? also adds another code path

If you have a code that looks like this:

foo()?;
bar()?;
baz()?;

then you have 4 possible paths: foo fails, bar fails, baz fails or everything succeeds.

If you ignore errors like this, you get 8 possible code paths:

foo().log_err().ok();
bar().log_err().ok();
baz().log_err().ok();

[iequidoo]

If the "failed" branches are independent from each other (there's no correlation probabilistically), it's enough to have 4 tests -- everything succeeds, foo() fails, bar() fails, baz() fails -- to cover the code. The same amount of tests is needed if we early return with ?. Ok, skipped errors may add side effects, but they also happen if other async tasks are run in parallel. So i think there's no generic rule, reducing number of code paths may or may not make sense depending on the case.