Quick succession of saving script leads to broken history state

[shulhi]

If you save the script quickly and rapidly, you will encounter TryingToAppendToNonHead exception.

The script history is also broken (see screenshot above). It thinks that the HEAD is the second latest one. If you pick the actual latest version, you won't be able to save the script as it throws TryingToAppendToNonHead exception.

I think we need some sort of lock while saving the script.

[siddharth-krishna]

I've been looking into the code of storeVCObject to see if there could be any concurrency bugs due to two racing stores that could cause issues like the above, and I think there shouldn't be.

inferno/inferno-vc/src/Inferno/VersionControl/Operations.hs

Lines 145 to 166 in 552fd3f

	obj_h <- case maybeCurrentHead of
	Just pred_hash -> do
	let head_fp = storePath </> "heads" </> show pred_hash
	-- check to see if pred_hash exists in '<storePath>/heads`
	exists_head <- liftIO $ doesFileExist head_fp
	if exists_head
	then do
	-- we know that pred_h is currently HEAD, we can therefore store the object and metadata in the store
	obj_h <- writeHashedJSON storePath obj
	-- next we make the newly added object the HEAD
	let new_head_fp = storePath </> "heads" </> show obj_h
	liftIO $ renameFile head_fp new_head_fp
	-- we append the previous head hash to the file (this serves as lookup for all the predecessors)
	appendBS new_head_fp $ BL.fromStrict $ vcObjectHashToByteString pred_hash <> "\n"
	-- now we need to change all the predecessor mappings in '<storePath>/to_head' to point to the new HEAD
	-- we also include the new head pointing to itself
	preds <- readVCObjectHashTxt new_head_fp
	let obj_h_bs = BL.fromStrict $ vcObjectHashToByteString obj_h
	forM_ (obj_h : preds) (\pred_h -> writeBS (storePath </> "to_head" </> show pred_h) obj_h_bs)
	pure obj_h
	else throwError $ TryingToAppendToNonHead pred_hash

Let's say that object o1 is currently the head of a script history, and there are two calls: store(o2) and store(o3), both with pred = o1. If these calls race and one of them (say o3) goes first, then since it will rename the heads/o1 file to heads/o3 on line 156, store(o2) will either fail at the check to see if file heads/o1 exists on line 150 or when it tries to rename the file on line 156. Both store operations cannot succeed because the rename on line 156 acts as an atomic compare-and-swap (CAS), a common primitive used in lock-free concurrent data structures. This means the append on line 158 will only be executed by the successful store (in our example, o3).

There are still a few potential issues with the current code, however:

We don't catch the IO error thrown by renameFile if it fails due to a race. We should catch that, return a TryingToAppendToNonHead error and clean up the object file that was written in line 153.

Let's say we have two successive but concurrent calls to store, store(o2, pred=o1) and store(o3, pred=o2) but within a very short duration so that their updating of the to_head mappings on lines 161-163 overlap. This could lead to a situation where store(o3) finishes first, and then store(o2) comes along and incorrectly updates a bunch of to_head mappings to point to o2 instead of o3. One way to fix this might be to check at each loop iteration that the head hasn't changed, and simply break out of the loop if it has. The operation that changed the head can be responsible for updating the rest of the to_head pointers.

(cc @goodlyrottenapple @smurphy8 )