fix(emqx_management): remove trace files after zip download

We only deleted the resulting zip after a trace file download, not the
actual trace files. This adds a deletion of the uncompressed trace files
as well. It also creates unique directories when collecting trace files
so that concurrent downloads doesn't overwrite files in transit.

apps/emqx_management/src/emqx_mgmt_api_trace.erl

@@ -20,6 +20,7 @@
 -include_lib("kernel/include/file.hrl").
 -include_lib("typerefl/include/types.hrl").
 -include_lib("emqx/include/logger.hrl").
+-include_lib("snabbkaffe/include/snabbkaffe.hrl").
 
 -export([
     api_spec/0,
@@ -461,16 +462,26 @@ download_trace_log(get, #{bindings := #{name := Name}, query_string := Query}) -
             case parse_node(Query, undefined) of
                 {ok, Node} ->
                     TraceFiles = collect_trace_file(Node, TraceLog),
-                    ZipDir = emqx_trace:zip_dir(),
+                    %% We generate a session ID so that we name files
+                    %% with unique names. Then we won't cause
+                    %% overwrites for concurrent requests.
+                    SessionId = emqx_misc:gen_id(),
+                    ZipDir = filename:join([emqx_trace:zip_dir(), SessionId]),
+                    ok = file:make_dir(ZipDir),
+                    %% Write files to ZipDir and create an in-memory zip file
                     Zips = group_trace_file(ZipDir, TraceLog, TraceFiles),
-                    FileName = binary_to_list(Name) ++ ".zip",
+                    ZipName = binary_to_list(Name) ++ ".zip",
-                    ZipFileName = filename:join([ZipDir, FileName]),
+                    {ok, {ZipName, Binary}} = zip:zip(ZipName, Zips, [memory, {cwd, ZipDir}]),
-                    {ok, ZipFile} = zip:zip(ZipFileName, Zips, [{cwd, ZipDir}]),
                     %% emqx_trace:delete_files_after_send(ZipFileName, Zips),
                     %% TODO use file replace file_binary.(delete file after send is not ready now).
-                    {ok, Binary} = file:read_file(ZipFile),
+                    ok = file:del_dir_r(ZipDir),
-                    ZipName = filename:basename(ZipFile),
+                    ?tp(trace_api_download_trace_log, #{
-                    _ = file:delete(ZipFile),
+                        files => Zips,
+                        name => Name,
+                        session_id => SessionId,
+                        zip_dir => ZipDir,
+                        zip_name => ZipName
+                    }),
                     Headers = #{
                         <<"content-type">> => <<"application/x-zip">>,
                         <<"content-disposition">> => iolist_to_binary(

apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl

@@ -168,7 +168,8 @@ t_create_failed(_Config) ->
     ),
     %% clear
     ?assertMatch({ok, _}, request_api(delete, api_path("trace"), [])),
-    {ok, Create} = request_api(post, api_path("trace"), [GoodName | Trace]),
+    {ok, Create1} = request_api(post, api_path("trace"), [GoodName | Trace]),
+    ?assertMatch(#{<<"name">> := <<"test-name-0">>}, json(Create1)),
     %% new name but same trace
     GoodName2 = {<<"name">>, <<"test-name-1">>},
     ?assertMatch(
@@ -314,6 +315,47 @@ t_stream_log(_Config) ->
     unload(),
     ok.
 
+t_trace_files_are_deleted_after_download(_Config) ->
+    ClientId = <<"client-test-delete-after-download">>,
+    Now = erlang:system_time(second),
+    Name = <<"test_client_id">>,
+    load(),
+    create_trace(Name, ClientId, Now),
+    {ok, Client} = emqtt:start_link([{clean_start, true}, {clientid, ClientId}]),
+    {ok, _} = emqtt:connect(Client),
+    [
+        begin
+            _ = emqtt:ping(Client)
+        end
+     || _ <- lists:seq(1, 5)
+    ],
+    ok = emqtt:disconnect(Client),
+    ok = emqx_trace_handler_SUITE:filesync(Name, clientid),
+
+    %% Check that files have been removed after download and that zip
+    %% directories uses unique session ids
+    ?check_trace(
+        begin
+            %% Download two zip files
+            Path = api_path(["trace/", binary_to_list(Name), "/download"]),
+            {ok, Binary1} = request_api(get, Path),
+            {ok, Binary2} = request_api(get, Path),
+            ?assertMatch({ok, _}, zip:table(Binary1)),
+            ?assertMatch({ok, _}, zip:table(Binary2))
+        end,
+        fun(Trace) ->
+            [
+                #{session_id := SessionId1, zip_dir := ZipDir1},
+                #{session_id := SessionId2, zip_dir := ZipDir2}
+            ] = ?of_kind(trace_api_download_trace_log, Trace),
+            ?assertEqual({error, enoent}, file:list_dir(ZipDir1)),
+            ?assertEqual({error, enoent}, file:list_dir(ZipDir2)),
+            ?assertNotEqual(SessionId1, SessionId2),
+            ?assertNotEqual(ZipDir1, ZipDir2)
+        end
+    ),
+    ok.
+
 to_rfc3339(Second) ->
     list_to_binary(calendar:system_time_to_rfc3339(Second)).