Drop jobs from Offline downstairs.

[leftwo]

If we receive a job ACK from an offline downstairs, and that job has already been moved
back to New, then discard it as we know the replay will resend it when the downstairs
comes back.

This is to fix #1155

[mkeeter]

I'm surprised that this isn't fixed by #1131: once the downstairs IO task stops, I would expect that we would have mismatches between DeferredMessage::connection_id and DownstairsClient::get_connection_id, and would therefore discard messages before hitting this point. We should never be seeing messages from an old connection; that's the whole point of #1131.

Looking into the code, I think the real issue is a race condition in DownstairsClient::get_connection_id. We only increment self.downstairs.clients[client_id].stats.connected after seeing ClientAction::Connected. This means that (terminated) connection N and (brand new) connection N + 1 will return the same value for get_connection_id, until connection N + 1 actually connects to the Downstairs.

I think the correct fix is to fix DownstairsClient::get_connection_id so that it rolls over immediately when a new connection is established (rather than relying on stats.connected). Let me know if that makes sense!

[mkeeter]

Suggested change

	/// Session ID for a clients connection to a downstairs.
	/// Session ID for a clients connection to a downstairs.
	///
	/// This value is incremented whenever we restart the client IO task.

[mkeeter]

This looks much better, thanks!

My one remaining thought is wondering whether we should define a struct ConnectionId(u64), instead of the current usize (in DownstairsClient) → u64 (in get_connection_id)

[leftwo]

This looks much better, thanks!

My one remaining thought is wondering whether we should define a struct ConnectionId(u64), instead of the current usize (in DownstairsClient) → u64 (in get_connection_id)

I've done that in the last commit. I think this should be ready to go now.

[mkeeter]

Sorry to be so pedantic, but now that you've added the new type, we should actually use it – this function should be changed to return an Option<ConnectionId>, and then fix all of the compiler errors and we should be good :)