Could we not re-use the active_commands set? Or you avoided that because the entries in that set are cleared on the terminal state transition of those operations and you didn't want these entries cleared so soon?

Looking at the doc comment for active_commands this sounds indeed as the correct place for that fix.

However, this raises an other point: why do we have this issue with duplicated commands while there is already a mechanism supposed to handle that?

I think active_commands is a broken solution to the problem. I think there we are de-duplicating c8y/devicecontrol/notifications, but we are only tracking an active command once we receive the relevant tedge-topic command message, which will happen a short while later. I think this is what leaves open the window for an operation to be duplicated.

Assuming I've understood correctly, that would indicate that the problem could simply be solved by moving the active_commands insertion to where I'm currently inserting to processed_ids, and deleting the processed_ids stuff?

Assuming I've understood correctly, that would indicate that the problem could simply be solved by moving the active_commands insertion to where I'm currently inserting to processed_ids, and deleting the processed_ids stuff?

One point is sure: one should keep only a single de-duplication mechanism. What you propose makes sense: it's better to remove duplicates before any processing.

why do we have this issue with duplicated commands while there is already a mechanism supposed to handle that?

For the cases where the duplicate messages are delivered after a restart, it is the lack of persistence of this set. But for duplicate messages delivered while the mapper is still live, this should have been sufficient.

I've now modified active_commands to insert immediately post-conversion, rather than when we receive our outgoing message. As a result, I've deleted processed_ids.

It would be good to make obvious that original and redelivery only differ on the delivery field`.

I've now removed some of the extraneous fields and changed things so we send the original message twice, since the converter doesn't care about the delivery field.

I struggle to understand what is checked by this test and how.

CumulocityConverter.supported_operations field is patched before the first request then restored before the second. Okay, but why?

This tests is checking what happens if we receive a custom operation that is unrecognised and later get redelivered the custom operation after it is registered with the mapper. If the converter naïvely assumes that the operation is active after we first receive it, the de-duplication mechanism will ignore the redelivery. But since we haven't yet processed this message, we should process such a redelivery. This is obviously dependent on something sending a 500 message once the operation is registered, but that could be the case for some custom operation handling service.

The patching of the supported_operations was intended as an easy way of ensuring the registered operations are made clear, since I'm not trying to test how we update supported_operations in this case.

This is clearer. Might be good to add this response as a comment to the test.

Technically this is fine. But seeing the detailed discussion above, it felt like this case could have been better represented as an integration test in tests.rs, where we can better simulate the dynamic custom operation registration during the execution of the test.

I think I want the opposite of that, @albinsuresh. The point of this being a unit test is I don't also want to test the operation registration logic at the same time. I've added a comment to explain what it is I'm trying to test.

A slightly stricter check that validates at least the cmd topic would have been better than this non-empty output check. To avoid false positives like a converted error message sent to te/errors instead of the expected cmd beating this assertion.

Same comment as above, regarding a stricter check.

Technically this is fine. But seeing the detailed discussion above, it felt like this case could have been better represented as an integration test in tests.rs, where we can better simulate the dynamic custom operation registration during the execution of the test.

Just a matter of taste: I would prefer an elapsed or elapsed_since method on the clock rather than the instant:

PS: dbg! to be removed

I was actually thinking that you'd use the timer actor from the mapper to get timeout notification message at desired intervals. But, it was smart to place the eviction logic here.