Use the same file write/sync/rename path where needed

When we want to be sure a file has been written before proceeding - we need a safer (that `file:write_file/2`) mechanism to be sure that it is written before proceeding.

This will:
open, write, sync, rename and then optionally read-back.

Changed so that manifest writing uses the safest form (including read back), and that sst writing uses a slightly looser form (with no read back to avoid performance issues).

src/leveled_imanifest.erl

@@ -173,8 +173,7 @@ writer(Manifest, ManSQN, RootPath) ->
                             integer_to_list(ManSQN) ++ "." ++ ?PENDING_FILEX),
     MBin = term_to_binary(to_list(Manifest), [compressed]),
     leveled_log:log("I0016", [ManSQN]),
-    ok = file:write_file(TmpFN, MBin),
+    ok = leveled_util:safe_rename(TmpFN, NewFN, MBin, true),
-    ok = file:rename(TmpFN, NewFN),
     GC_SQN = ManSQN - ?MANIFESTS_TO_RETAIN,
     GC_Man = filename:join(ManPath,
                             integer_to_list(GC_SQN) ++ "." ++ ?MANIFEST_FILEX),

src/leveled_pmanifest.erl

@@ -53,6 +53,7 @@
 -include_lib("eunit/include/eunit.hrl").
 
 -define(MANIFEST_FILEX, "man").
+-define(PENDING_FILEX, "pnd").
 -define(MANIFEST_FP, "ledger_manifest").
 -define(MAX_LEVELS, 8).
 -define(TREE_TYPE, idxt).
@@ -201,14 +202,15 @@ close_manifest(Manifest, CloseEntryFun) ->
 %% @doc
 %% Save the manifest to file (with a checksum)
 save_manifest(Manifest, RootPath) ->
-    FP = filepath(RootPath, Manifest#manifest.manifest_sqn, current_manifest),
+    TFP = filepath(RootPath, Manifest#manifest.manifest_sqn, pending_manifest),
+    AFP = filepath(RootPath, Manifest#manifest.manifest_sqn, current_manifest),
     ManBin = term_to_binary(Manifest#manifest{snapshots = [],
                                                 pending_deletes = dict:new(),
                                                 min_snapshot_sqn = 0,
                                                 blooms = dict:new()}),
     CRC = erlang:crc32(ManBin),
-    ok = file:write_file(FP, <<CRC:32/integer, ManBin/binary>>),
+    ToPersist = <<CRC:32/integer, ManBin/binary>>,
-    {ok, <<CRC:32/integer, ManBin/binary>>} = file:read_file(FP),
+    ok = leveled_util:safe_rename(TFP, AFP, ToPersist, true),
     GC_SQN = Manifest#manifest.manifest_sqn - ?MANIFESTS_TO_RETAIN,
         % If a manifest is corrupted the previous one will be tried, so don't
         % delete the previous one straight away.  Retain until enough have been
@@ -906,7 +908,12 @@ filepath(RootPath, manifest) ->
 
 filepath(RootPath, NewMSN, current_manifest) ->
     filepath(RootPath, manifest)  ++ "nonzero_"
-                ++ integer_to_list(NewMSN) ++ "." ++ ?MANIFEST_FILEX.
+                ++ integer_to_list(NewMSN) ++ "." ++ ?MANIFEST_FILEX;
+filepath(RootPath, NewMSN, pending_manifest) ->
+    filepath(RootPath, manifest)  ++ "nonzero_"
+                ++ integer_to_list(NewMSN) ++ "." ++ ?PENDING_FILEX.
+
+
 
 
 open_manifestfile(_RootPath, L) when L == [] orelse L == [0] ->

src/leveled_sst.erl

@@ -1201,13 +1201,6 @@ write_file(RootPath, Filename, SummaryBin, SlotsBin,
     SlotsLength = byte_size(SlotsBin),
     {PendingName, FinalName} = generate_filenames(Filename),
     FileVersion = gen_fileversion(PressMethod, IdxModDate),
-    ok = file:write_file(filename:join(RootPath, PendingName),
-                            <<FileVersion:8/integer,
-                                SlotsLength:32/integer,
-                                SummaryLength:32/integer,    
-                                SlotsBin/binary,
-                                SummaryBin/binary>>,
-                            [raw]),
     case filelib:is_file(filename:join(RootPath, FinalName)) of
         true ->
             AltName = filename:join(RootPath, filename:basename(FinalName))
@@ -1217,8 +1210,14 @@ write_file(RootPath, Filename, SummaryBin, SlotsBin,
         false ->
             ok
     end,
-    file:rename(filename:join(RootPath, PendingName),
+    ok = leveled_util:safe_rename(filename:join(RootPath, PendingName),
-                filename:join(RootPath, FinalName)),
+                                    filename:join(RootPath, FinalName),
+                                    <<FileVersion:8/integer,
+                                        SlotsLength:32/integer,
+                                        SummaryLength:32/integer,
+                                        SlotsBin/binary,
+                                        SummaryBin/binary>>,
+                                        false),
     FinalName.
 
 read_file(Filename, State, LoadPageCache) ->

src/leveled_util.erl

@@ -14,7 +14,10 @@
 -export([generate_uuid/0,
             integer_now/0,
             integer_time/1,
-            magic_hash/1]).
+            magic_hash/1,
+            safe_rename/4]).
+
+-define(WRITE_OPS, [binary, raw, read, write]).
 
 
 -spec generate_uuid() -> list().
@@ -65,12 +68,33 @@ hash1(H, <<B:8/integer, Rest/bytes>>) ->
     hash1(H2, Rest).
 
 
+-spec safe_rename(string(), string(), binary(), boolean()) -> ok.
+%% @doc
+%% Write a file, sync it and rename it (and for super-safe mode read it back)
+%% An attempt to prevent crashes leaving files with empty or partially written
+%% values
+safe_rename(TempFN, RealFN, BinData, ReadCheck) ->
+    {ok, TempFH} = file:open(TempFN, ?WRITE_OPS),
+    ok = file:write(TempFH, BinData),
+    ok = file:sync(TempFH),
+    ok = file:close(TempFH),
+    ok = file:rename(TempFN, RealFN),
+    case ReadCheck of
+        true ->
+            {ok, ReadBack} = file:read_file(RealFN),
+            true = (ReadBack == BinData),
+            ok;
+        false ->
+            ok
+    end.
+
 %%%============================================================================
 %%% Test
 %%%============================================================================
 
 -ifdef(TEST).
 
+-define(TEST_AREA, "test/test_area/util/").
 
 magichashperf_test() ->
     KeyFun =
@@ -86,4 +110,17 @@ magichashperf_test() ->
     {TimeMH2, _HL1} = timer:tc(lists, map, [fun(K) -> magic_hash(K) end, KL]),
     io:format(user, "1000 keys magic hashed in ~w microseconds~n", [TimeMH2]).
 
+
+safe_rename_test() ->
+    ok = filelib:ensure_dir(?TEST_AREA),
+    TempFN = filename:join(?TEST_AREA, "test_manifest0.pnd"),
+    RealFN = filename:join(?TEST_AREA, "test_manifest0.man"),
+    ok = safe_rename(TempFN, RealFN, <<1:128/integer>>, false),
+    ?assertMatch({ok, <<1:128/integer>>}, file:read_file(RealFN)),
+    TempFN1 = filename:join(?TEST_AREA, "test_manifest1.pnd"),
+    RealFN1 = filename:join(?TEST_AREA, "test_manifest1.man"),
+    ok = safe_rename(TempFN1, RealFN1, <<2:128/integer>>, true),
+    ?assertMatch({ok, <<2:128/integer>>}, file:read_file(RealFN1)).
+
+
 -endif.