Keep track when we try and fail to process a pulled event

[MadLittleMods]

Keep track when we try and fail to process a pulled event.

We can follow-up this PR with:

1. Only try to backfill from an event if we haven't tried recently -> Only try to backfill event if we haven't tried before recently #13622
2. When we decide to backfill that event again, process it in the background so it doesn't block and make /messages slow when we know it will probably fail again -> Process previously failed backfill events in the background #13623
3. Generally track failures everywhere we try and fail to pull an event over federation -> Generalize tracking whenever we fail to fetch or process a federated event #13700

Fix #13621

Part of #13356

Mentioned in internal doc

Dev notes

• Last contact time
• Number of attempts

---

• Custom upsert example in set_destination_retry_timings: synapse/storage/databases/main/transactions.py#L207-L330
• simple_upsert ends up in the following SQL query:
  • INSERT INTO event_backfill_attempts (event_id, num_attempts, last_attempt_ts) VALUES (?, ?, ?) ON CONFLICT (event_id) DO UPDATE SET event_id=EXCLUDED.event_id, num_attempts=EXCLUDED.num_attempts, last_attempt_ts=EXCLUDED.last_attempt_ts
  • args=['$kfuSEOfoHjRUVZrJczZkms8rvFmVBZCceu-4TMIfqZ8', 1, 1661462351963]

---

What if the event is considered spam at the moment? What if it isn't considered spam in the future?

Can we ingest the event but add a rejection_reason for spam? Do we only use it for auth based rejections?

We have a rejections table:

$ psql synapse
synapse=# \d+ rejections
                                 Table "public.rejections"
   Column   | Type | Collation | Nullable | Default | Storage  | Stats target | Description
------------+------+-----------+----------+---------+----------+--------------+-------------
 event_id   | text |           | not null |         | extended |              |
 reason     | text |           | not null |         | extended |              |
 last_check | text |           | not null |         | extended |              |

---

backfill
_process_pulled_events
_process_pulled_event
_process_received_pdu
_run_push_actions_and_persist_event
persist_events_and_notify
persist_events
_event_persist_queue
_persist_event_batch
_persist_events_and_state_updates
_persist_events_txn
_store_event_txn
_update_metadata_tables_txn

on_receive_pdu
_process_received_pdu
_run_push_actions_and_persist_event
persist_events_and_notify
persist_events
...

---

    @trace
    async def clear_event_backfill_attempts(self, event_id: str) -> None:
        await self.db_pool.simple_delete(
            table="event_backfill_attempts",
            keyvalues={"event_id": event_id},
            desc="clear_event_backfill_attempts",
        )

    @trace
    async def clear_event_backfill_attempts_txn(
        self, txn: LoggingTransaction, event_id: str
    ) -> None:
        await self.db_pool.simple_delete_one_txn(
            txn,
            table="event_backfill_attempts",
            keyvalues={"event_id": event_id},
        )
        

# We've successfully persisted the event, no need to backfill it anymore
self.store.clear_event_backfill_attempts_txn(event.event_id)

Todo

• Remove event from the table when we have successfully persisted it as a non-outlier

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[erikjohnston]

I'm not sure if this is the right place to record this? What is the failure mode you're trying to catch here?

[MadLittleMods]

I assume this will catch the FederationError where We can't get valid state history,

---

But perhaps you also mean that we should also generally look for any exceptions and record there as well? That sounds reasonable for recording whatever failure that happens 👍:

try:
    ...
except FederationError as e:
    record_event_failed_pull_attempt(...)
    ...
except Exception as e:
    record_event_failed_pull_attempt(...)
    raise e

[erikjohnston]

Sorry, I was mostly just thinking that its not obvious that this particular spot is the right place in the handling. For example, I think by this point we've already attempted to try and refetch events that have bad signatures returned by /backfill.

I guess this is a good first step, and we can always add it the checks in more places later.

[erikjohnston]

LGTM if you remove the if backfilled: check

[MadLittleMods]

Thanks for the review @reivilibre, @richvdh, @DMRobertson, and @erikjohnston 🐮