-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Description
I've written about this before a few times but I will summarize my concerns again here. Apologies for eschewing the issue template and for the length of this post, but I wanted to use this as a place for summarizing all the issues around Sequelize's practice of comparing timestamps for soft deletes.
Use of client time instead of DB time
Before #8485, Sequelize would soft delete records using client's time, but query for them using database's time (by "client's time" I mean client of the database, e.g. an app server, ad hoc script, etc). This lead to inconsistencies that have been well documented; namely if client and DB clocks aren't in sync, you can potentially see a soft deleted record.
#8485 normalized this so that both delete and query operations use the client's time. This is good in that you no longer have to worry about time zone differences or clock drift between the DB and the client.
But the safer choice would arguably be to use the DB's time rather than the client's. This is because a typical app will have a number of clients (load balanced app instances, ad hoc scripts running on dev machines, etc), all of which need to keep their clocks in sync with each other, compared to only a handful of DB instances (and often only 1).
This problem still exists when using DB time if you have multiple database instances in a cluster, but typically the number of DB instances will be far less than the number of clients. Additionally, DB instances tend to be pretty homogeneous and under tight control, so keeping their clocks in sync would tend to be much easier than keeping client clocks in sync.
That being said, I still think it's still dangerous to rely on DB clocks being in sync, especially when you have multiple DB instances and non-monotonic time sources.
Non-monotonic time source
Javascript's Date object is not monotonic, i.e. in the presence of NTP, leap seconds, users manually adjusting the system time, etc, it can tick backwards. When that happens, your previously deleted records are now scheduled to be deleted in the future.
For single node deployments, this can potentially be fixed by using something like process.hrtime as a time source. But clock drift between nodes is still a problem.
Clock drift between nodes
Even if we use a monotonic time source, keeping multiple clocks in sync is really hard. Google resorts to atomic clocks to do it! The linked article mentions that NTP, which I can imagine is more typical than atomic clocks for the majority of Sequelize's users, will typically give you an upper bound for clock offsets at "somewhere between 100ms and 250ms."
Prior art
I haven't researched this too much, but a cursory search reveals that alternatives like ActiveRecord's ActsAsParanoid do not compare timestamps and instead compare to a sentinel value. I'd be curious to see if they're consciously not supporting future deletes or have just never implemented it. Regardless, if Sequelize's behavior in this space really is unique, that seems like more evidence that it's not a safe practice. Especially when it's for a feature (scheduled deletes) that few may use.
Conclusion
I think scheduled soft deletes weaken the consistency of Sequelize. It is very possible to see a soft deleted record 250ms or more after it was soft deleted. Sequelize should consider changing the default behavior to something safer.