tree 2d34606b833b6ad83e893203c6e1ab1d5197cd2f
parent 6c5a43106a1f7d0b0258b943c51b93c52563e9ed
author Gian Merlino <gianmerlino@gmail.com> 1660545276 -0700
committer GitHub <noreply@github.com> 1660545276 -0700
gpgsig -----BEGIN PGP SIGNATURE-----
 
 wsBcBAABCAAQBQJi+ej8CRBK7hj4Ov3rIwAApA8IAD+Nx9/C+VCoHwkAB9XLIt6n
 lb9hH0hsUm90N79X5MSf2iQI3dcr4VAkrKnHdrXMuKb22JDCGNYRqTU8k0V2sTae
 AXckcdfcggdMDAJRGUEk8BsUWnH2ts8Z2UMkhL5eL5h0CXh+HNalYpUESwq7uT7R
 RTrWqMg3n/5AKxi3MbWjOfL398wOXZ0VOyxK3dLMU6T/iAFhIBnjpkKlgKLiTr4t
 2QlnAxnc+EkaXSMjKDIrzNkMWL0ZT/fIs/RqPbCN3g3jGE400B0oq7HTKr1fMa7D
 R0TimNya8HS/MajiznFdL/V0MfTOZTyp1/2FsSbfsg9HxDOz4/LxzIgfU/9YOjA=
 =Ro8M
 -----END PGP SIGNATURE-----
 

Fix race in TaskQueue.notifyStatus. (#12901)

* Fix race in TaskQueue.notifyStatus.

It was possible for manageInternal to relaunch a task while it was
being cleaned up, due to a race that happens when notifyStatus is
called to clean up a successful task:

1) In a critical section, notifyStatus removes the task from "tasks".
2) Outside a critical section, notifyStatus calls taskRunner.shutdown
   to let the task runner know it can clear out its data structures.
3) In a critical section, syncFromStorage adds the task back to "tasks",
   because it is still present in metadata storage.
4) In a critical section, manageInternalCritical notices that the task
   is in "tasks" and is not running in the taskRunner, so it launches
   it again.
5) In a (different) critical section, notifyStatus updates the metadata
   store to set the task status to SUCCESS.
6) The task continues running even though it should not be.

The possibility for this race was introduced in #12099, which shrunk
the critical section in notifyStatus. Prior to that patch, a single
critical section encompassed (1), (2), and (5), so the ordering above
was not possible.

This patch does the following:

1) Fixes the race by adding a recentlyCompletedTasks set that prevents
   the main management loop from doing anything with tasks that are
   currently being cleaned up.
2) Switches the order of the critical sections in notifyStatus, so
   metadata store updates happen first. This is useful in case of
   server failures: it ensures that if the Overlord fails in the midst
   of notifyStatus, then completed-task statuses are still available in
   ZK or on MMs for the next Overlord. (Those are cleaned up by
   taskRunner.shutdown, which formerly ran first.) This isn't related
   to the race described above, but is fixed opportunistically as part
   of the same patch.
3) Changes the "tasks" list to a map. Many operations require retrieval
   or removal of individual tasks; those are now O(1) instead of O(N)
   in the number of running tasks.
4) Changes various log messages to use task ID instead of full task
   payload, to make the logs more readable.

* Fix format string.

* Update comment.