[Discussion] Wait until widgets are deleted before deleting db source object

[sssoleileraaa]

I have an idea I'd like to get feedback on. Right now we do source deletion in the following order:

1. Delete sources from the local database (this happens in one place: the MetadataSyncJob)
2. Delete source widgets that have a reference to the deleted database object
3. Delete conversation widgets that also have a reference to the deleted database object

This is fine if the user deletes the source locally because of our pending deletion state that makes it so a user cannot star or reply or doing anything with a source that is being deleted. However, if sources are deleted via the Journalist Interface or by another instance of the client then we could get into a situation where we make a mistake like trying to log the uuid on the source db object when we get an sqlalchemy error because its been deleted from the db or forgetting to surround access to the db source object in a try catch.

I think we could avoid these kinds of bugs and code messiness in the gui to avoid them by switching the order of these steps. What if instead we marked sources as "deleted" in the local database during a sync and wait until all the widgets of "deleted" sources are removed. After deleting all the source widgets that belong to "deleted" sources and their conversation widgets, we could emit a signal to the controller to tell it to delete the database entries, like so:

deleted_sources = self.source_list.update(sources)
    for source_uuid in deleted_sources:
        self.delete_conversation(source_uuid)
    self.controller.sources_deleted_from_gui.emit(deleted_sources)

[redshiftzero]

this is a good point, although we'll want to be continue to be careful around jobs that may be queued that will execute after some time, e.g. the following can still arise:

1. job runs, fails with 404 because source is deleted
2. failure callback fires -> if there's any logic in here using the database object (like logging object.uuid as you say above), it'll be gone and will raise an exception

[sssoleileraaa]

I don't see how # 1 can happen, but let me try to understand... if we no longer delete sources from our local database in the MetadataSyncJob and instead delete sources from the Controller then it's possible for another MetadataSyncJob to run while the sources marked as "Deleted" are being actually deleted from the database (because the gui is done updating) by the Controller. I think we could avoid the 404 by only syncing the source db objects that are not marked "Deleted" unless I'm missing something.

[redshiftzero]

Expanding the scenario a bit:

0. Lots of jobs are queued for execution, including many message download jobs corresponding to source A.
1. User deletes source A. Deletion job runs locally, all source A related GUI objects are removed, as are any database objects corresponding to source A (both the Source and related objects that the ORM will cascade delete).
2. The logic on the server side is that any subsequent jobs (e.g. a message download attempt running after the sync that deleted the source objects) for a source A will produce a 404 and thus fail.
3. The failure callback will fire for that job, if it accesses any of these deleted objects, we'll get an exception.

[zenmonkeykstop]

Marking with sync label and deferring for a future epic to fix sync/store issues en masse.