Errors in `merge_single_tombstone` and `open_file` `append` mode [JIRA: RIAK-2733]
Closed this issue · 2 comments
tl;dr
If a merge occurs with an active tombstone, bitcask:merge_single_tombstone/6 will attempt to append that tombstone to the .data file that originally owned the tombstone'd data. If that .data file does not have an associated .hint file, the operation will fail, and a {generic_failure,error,{badmatch,{error,enoent}} will be logged.
full error
yyyy-mm-dd hh:mm:ss.mil [error] <P.I.D> Failed to merge
{["/var/lib/riak/bitcask/1090417237227353349128530480659414284196418420736/97218.bitcask.data",
"/var/lib/riak/bitcask/1090417237227353349128530480659414284196418420736/97217.bitcask.data",
"/var/lib/riak/bitcask/1090417237227353349128530480659414284196418420736/97216.bitcask.data",
"/var/lib/riak/bitcask/1090417237227353349128530480659414284196418420736/97015.bitcask.data",
"/var/lib/riak/bitcask/1090417237227353349128530480659414284196418420736/477.bitcask.data"],
["/var/lib/riak/bitcask/1090417237227353349128530480659414284196418420736/477.bitcask.data"]}:
{generic_failure,error,{badmatch,{error,enoent}},[{bitcask_fileops,reopen_hintfile,1,[{file,"src/bitcask_fileops.erl"},{line,182}]},
{bitcask_fileops,open_file,2,[{file,"src/bitcask_fileops.erl"},{line,141}]},
{bitcask,get_filestate,4,[{file,"src/bitcask.erl"},{line,1356}]},
{bitcask,get_filestate,2,[{file,"src/bitcask.erl"},{line,1343}]},
{bitcask,merge_single_tombstone,6,[{file,"src/bitcask.erl"},{line,1496}]},
{bitcask_fileops,fold_int_loop,5,[{file,"src/bitcask_fileops.erl"},{line,554}]},
{bitcask_fileops,fold_file_loop,8,[{file,"src/bitcask_fileops.erl"},{line,712}]},
{bitcask_fileops,fold,3,[{file,"src/bitcask_fileops.erl"},{line,391}]}]}
Full description
The interesting logic starts in bitcask:merge_single_tombstone/6 when the tombstone context has information on a deleted value, and calls get_filestate/2 in preparation to append the active tombstone to the to the old .data file.
When get_filestate/2 is called with a merge state , it will use the append file mode, and call into bitcask_fileops:open_file(Filename, append).
As part of bitcask_fileops:open_file(Filename, append), we attempt to "reopen" the .data file's .hint file with bitcask_fileops:reopen_hintfile/1. (I'm not sure why this is a 'reopen' call; in the context of this error there is no .hint file for us to have opened in the first place.)
reopen_hintfile wraps a call to bitcask_fileops:open_hint_file/1 in a case (catch . . .) of statement -- this turns out to be a problem. Note that 1) we're explicitly passing [] as the file options so, they will default to [read, write, raw, binary] , and 2) we have an unguarded clause in the in the case.
bitcask_fileops:open_hintfile/3 knows how to deal with two states; either the files is successfully opened and {ok,FD} is returned, or the OS returns eexists and the operation is retried. If the file does not exist, and we're not passing create as one of the file options, the OS will return enoent.
As there's no matching case clause in open_hintfile, an error will be thrown and immediately caught by reopen_hintfile. Remember that unguarded case clause?
In the HintFD -> branch of reopen_hintfile the assumption is made that HintFilename will be pointing to a valid file that actually exists. Because that's not the case, read_file_info(HintFilename), will return {error,enoent}, which does not match {ok, _}, which makes that function error out with {badmatch,{error,enoent}}.
From there, it's elephants all the way down.
But wait, there's more
If we add a quick and dirty {error, Error} -> {error, Error}; clause to bitcask_fileops:reopen_hintfile, we're still going to have a semantic error in this code. bitcask_fileops:open_file(Filename, append) already has a case clause for when reopen_hintfile returns {error,enoent}. If that case is ever hit (which it's currently not) the .data file is closed, and {error,enoent} is returned. That error will bubble up through get_filestate to merge_single_tombstone, at which point merge_single_tombstone will gracefully consume the {error,enoent} and not append the tombstone.
What a kick ass bug report. 👏👏👏
Underlying exception which causes crash is now handled by a1568ae. With that said, this code should be reworked at some point as it is very confusing and overly complex for what it is doing.