feat(robot-server): limit the maximum number of analyses stored per protocol

[sanni-t]

Closes AUTH-317

Overview

Adds a max-analyses-per-protocol limit of 5 analyses. Auto-deletes the oldest analysis (es) before adding a new one if the number of analyses stored exceeds the max.

Test Plan

• Tested on dev server and robot that number of analyses don't exceed 5 and the oldest analysis gets deleted first.
• Also tested that if a dev server already had more than 5 analyses in the DB, then the first analyses addition since updating to this branch deletes all excess analyses.

Changelog

• added analyses auto-deletion before new addition in CompletedAnalysisStore
• also added a way to remove the excess analyses from memory cache

Review requests

Usual code review. I've added in-line comments for things that need extra attention.

Risk assessment

Low. Well tested and pretty isolated feature.

[SyntaxColoring]

Purely stylistic, no opinion from me either way, but you can also write this as analyses_to_delete = analyses_ids[:-MAX_ANALYSES_TO_STORE] if you think that's clearer.

[SyntaxColoring]

Can we do all these deletions inside a single transaction?

And ideally, we'd actually go even further and have one transaction for deleting the old entries and inserting the new entry. That doesn't have to happen in this PR if it's a nontrivial refactor, but it is something that we should do, so can we think through what it would take? Like, does the AnalysisStore/CompletedAnalysisStore split get in the way of that?

[sanni-t]

I did try exactly this. Maybe I am not yet familiar with the full range of capabilities of sqlalchemy yet but the code was getting a bit too clunky for my liking, esp when trying to delete all old entries in one transaction.

This was what I was going for:

delete_statement = analysis_table.delete()
insert_statement = analysis_table.insert().values(
    await completed_analysis_resource.to_sql_values()
)
for analysis_id in analyses_to_delete:
    self._memcache.remove(analysis_id)
    delete_statement = delete_statement.where(
        analysis_table.c.id == analysis_id
    )
# <add something to make sure we don't end up executing `analysis_table.delete()` >

with self._sql_engine.begin() as transaction:
    transaction.execute(delete_statement)
    transaction.execute(insert_statement)

self._memcache.insert(
    completed_analysis_resource.id, completed_analysis_resource
)

(needed to have a few more checks in there but that was basically the idea)
Do you know of a better way of composing statements with multiple conditions?

[sanni-t]

Hmm.. it just occurred to me that I could have tried this-

  insert_statement = analysis_table.insert().values(
      await completed_analysis_resource.to_sql_values()
  )
  delete_statements = []
  for analysis_id in analyses_to_delete:
      self._memcache.remove(analysis_id)
      delete_statements.append(analysis_table.delete().where(
          analysis_table.c.id == analysis_id
      ))
  with self._sql_engine.begin() as transaction:
      for delete_statement in delete_statements:
          transaction.execute(delete_statement)
      transaction.execute(insert_statement)
  self._memcache.insert(
      completed_analysis_resource.id, completed_analysis_resource
  )

[sanni-t]

But is this^ any better than what is implemented?
It looks like there is a special engine configuration for executing multiple statements in a single transaction which I don't think we have enabled.

[SyntaxColoring]

  [...]
  for analysis_id in analyses_to_delete:
      self._memcache.remove(analysis_id)
      delete_statement = delete_statement.where(
         analysis_table.c.id == analysis_id
      )
  [...]

Do you know of a better way of composing statements with multiple conditions?

Yeah. There are a few ways to skin this cat, but I think the most direct translation of what you're trying to do is:

delete_statement = analysis_table.delete().where(analysis_table.c.id.in_(analyses_to_delete))

Which will generate a SQL statement like:

DELETE FROM analysis WHERE id IN ("abc", "def", "ghi")

---

Hmm.. it just occurred to me that I could have tried this-

[...]
with self._sql_engine.begin() as transaction:
   for delete_statement in delete_statements:
        transaction.execute(delete_statement)
[...]

Yep, that also works.

But is this^ any better than what is implemented?

Definitely. It's faster, safer in the face of concurrency that might be added in the future, and more semantically "correct."

It looks like there is a special engine configuration for executing multiple statements in a single transaction which I don't think we have enabled.

Hm, what do you mean? Executing multiple statements in a single transaction is a normal thing to do and we do it routinely. Where are you seeing that that needs special configuration?

[SyntaxColoring]

TY!

[jbleon95]

Nice! Looks good!