{"thread":{"id":"61602","subject":"[PATCH 0/2] commit-graph/server-info: use tempfile.h in more places","startedAt":"2024-06-06T22:19:24Z","lastAt":"2024-06-14T19:35:13Z","messageCount":17,"participants":["Taylor Blau","Junio C Hamano","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"496565","messageId":"cover.1717712358.git.me@ttaylorr.com","threadId":"61602","inReplyTo":null,"subject":"[PATCH 0/2] commit-graph/server-info: use tempfile.h in more places","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-06T22:19:21Z","receivedAt":"2024-06-06T22:19:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This pair of patches addresses two issues (in the commit-graph and\nupdate-server-info areas, respectively), where temporary files are\ncreated outside of the tempfile.h API and thus survive abnormal process\ndeath.\n\nThe commit-graph one is more prevalent, and has been the cause of some\nminor headaches (for e.g. automated tooling to detect repository cruft\nat GitHub complaining about unknown tmp_graph_XXXXXX files left around).\n\nThe fixes in both instances are relatively straightforward conversions\nto use the tempfile.h API.\n\nLooking at the remaining uses of mkstemp(), the remaining class of\ncallers that don't use the tempfile.h API are for creating temporary\n.idx, .rev files, and similar. My personal feeling is that we should\napply similar treatment there, since these files are generated based on\n.pack data, and thus keeping around temporary copies is unnecessary when\nthey can be regenerated.\n\nBut I'd rather keep that part out of this series and clean up a couple\nof more straightforward spots where possible.\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n  commit-graph.c: remove temporary graph layers on exit\n  server-info.c: remove temporary info files on exit\n\n commit-graph.c                | 19 ++++++++-----------\n server-info.c                 | 26 ++++++++++----------------\n t/t5324-split-commit-graph.sh | 26 +++++++++++++++++++++++++-\n 3 files changed, 43 insertions(+), 28 deletions(-)\n\n\nbase-commit: 7b0defb3915eaa0bd118f0996e8c00b4eb2dc1ca\n-- \n2.45.2.411.g2d5a0536af1\n"},{"id":"496566","messageId":"25324fea5b7c7f748d7f4e1e40299c0af04006e8.1717712358.git.me@ttaylorr.com","threadId":"61602","inReplyTo":"cover.1717712358.git.me@ttaylorr.com","subject":"[PATCH 1/2] commit-graph.c: remove temporary graph layers on exit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-06T22:19:25Z","receivedAt":"2024-06-06T22:19:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Since the introduction of split commit graph layers in 92b1ea66b9a\n(Merge branch 'ds/commit-graph-incremental', 2019-07-19), the function\nwrite_commit_graph_file() has done the following when writing an\nincremental commit-graph layer:\n\n  - used a lock_file to control access to the commit-graph-chain file\n\n  - used an auxiliary file (whose descriptor was stored in 'fd') to\n    write the new commit-graph layer itself\n\nUsing a lock_file to control access to the commit-graph-chain is\nsensible, since only one writer may modify it at a time. Likewise, when\nthe commit-graph machinery is writing out a single layer, the lock_file\nstructure is used to modify the commit-graph itself. This is also\nsensible, since the non-incremental commit-graph may also have at most\none writer.\n\nHowever, using an auxiliary temporary file without using the tempfile.h\nAPI means that writes that fail after the temporary graph layer has been\ncreated will leave around a file in\n\n    $GIT_DIR/objects/info/commit-graphs/tmp_graph_XXXXXX\n\nThe commit-graph-chain file and non-incremental commit-graph do not\nsuffer from this problem as the lockfile.h API uses the tempfile.h API\ntransparently, so processes that died before moving those finals into\ntheir final location cleaned up after themselves.\n\nEnsure that the temporary file used to write incremental commit-graphs\nis also managed with the tempfile.h API, to ensure that we do not ever\nleave tmp_graph_XXXXXX files laying around.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c                | 19 ++++++++-----------\n t/t5324-split-commit-graph.sh | 26 +++++++++++++++++++++++++-\n 2 files changed, 33 insertions(+), 12 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex e5dd3553dfe..92c71637142 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2002,8 +2002,8 @@ static int write_graph_chunk_base(struct hashfile *f,\n static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n {\n \tuint32_t i;\n-\tint fd;\n \tstruct hashfile *f;\n+\tstruct tempfile *graph_layer; /* when ctx->split is non-zero */\n \tstruct lock_file lk = LOCK_INIT;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n \tstruct strbuf progress_title = STRBUF_INIT;\n@@ -2035,24 +2035,23 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t\t\t       LOCK_DIE_ON_ERROR, 0444);\n \t\tfree(lock_name);\n \n-\t\tfd = git_mkstemp_mode(ctx->graph_name, 0444);\n-\t\tif (fd < 0) {\n+\t\tgraph_layer = mks_tempfile_m(ctx->graph_name, 0444);\n+\t\tif (!graph_layer) {\n \t\t\terror(_(\"unable to create temporary graph layer\"));\n \t\t\treturn -1;\n \t\t}\n \n-\t\tif (adjust_shared_perm(ctx->graph_name)) {\n+\t\tif (adjust_shared_perm(get_tempfile_path(graph_layer))) {\n \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n-\t\t\t      ctx->graph_name);\n+\t\t\t      get_tempfile_path(graph_layer));\n \t\t\treturn -1;\n \t\t}\n \n-\t\tf = hashfd(fd, ctx->graph_name);\n+\t\tf = hashfd(get_tempfile_fd(graph_layer), get_tempfile_path(graph_layer));\n \t} else {\n \t\thold_lock_file_for_update_mode(&lk, ctx->graph_name,\n \t\t\t\t\t       LOCK_DIE_ON_ERROR, 0444);\n-\t\tfd = get_lock_file_fd(&lk);\n-\t\tf = hashfd(fd, get_lock_file_path(&lk));\n+\t\tf = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));\n \t}\n \n \tcf = init_chunkfile(f);\n@@ -2133,8 +2132,6 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\tchar *final_graph_name;\n \t\tint result;\n \n-\t\tclose(fd);\n-\n \t\tif (!chainf) {\n \t\t\terror(_(\"unable to open commit-graph chain file\"));\n \t\t\treturn -1;\n@@ -2169,7 +2166,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\tfree(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);\n \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n \n-\t\tresult = rename(ctx->graph_name, final_graph_name);\n+\t\tresult = rename_tempfile(&graph_layer, final_graph_name);\n \n \t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++)\n \t\t\tfprintf(get_lock_file_fp(&lk), \"%s\\n\", ctx->commit_graph_hash_after[i]);\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 281266f7883..77e91547ea3 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -13,7 +13,8 @@ test_expect_success 'setup repo' '\n \tgit init &&\n \tgit config core.commitGraph true &&\n \tgit config gc.writeCommitGraph false &&\n-\tinfodir=\".git/objects/info\" &&\n+\tobjdir=\".git/objects\" &&\n+\tinfodir=\"$objdir/info\" &&\n \tgraphdir=\"$infodir/commit-graphs\" &&\n \ttest_oid_cache <<-EOM\n \tshallow sha1:2132\n@@ -718,4 +719,27 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl\n \t)\n '\n \n+test_expect_success 'temporary graph layer is discarded upon failure' '\n+\tgit init layer-discard &&\n+\t(\n+\t\tcd layer-discard &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\n+\t\t# Intentionally remove commit \"A\" from the object store\n+\t\t# so that the commit-graph machinery fails to parse the\n+\t\t# parents of \"B\".\n+\t\t#\n+\t\t# This takes place after the commit-graph machinery has\n+\t\t# initialized a new temporary file to store the contents\n+\t\t# of the new graph layer, so will allow us to ensure\n+\t\t# that the temporary file is discarded upon failure.\n+\t\trm $objdir/$(test_oid_to_path $(git rev-parse HEAD^)) &&\n+\n+\t\ttest_must_fail git commit-graph write --reachable --split &&\n+\t\ttest_dir_is_empty $graphdir\n+\t)\n+'\n+\n test_done\n-- \n2.45.2.411.g2d5a0536af1\n\n"},{"id":"496567","messageId":"2d5a0536af1a6d45835622e2c020266079fa0873.1717712358.git.me@ttaylorr.com","threadId":"61602","inReplyTo":"cover.1717712358.git.me@ttaylorr.com","subject":"[PATCH 2/2] server-info.c: remove temporary info files on exit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-06T22:19:31Z","receivedAt":"2024-06-06T22:19:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The update_info_file() function within server-info.c is responsible for\nmoving the info/refs and info/packs files around when updating server\ninfo.\n\nThese updates are staged into a temporary file and then moved into place\natomically to avoid race conditions when reading those files. However,\nthe temporary file used to stage these changes is managed outside of the\ntempfile.h API, and thus survives process death.\n\nManage these files instead with the tempfile.h API so that they are\nautomatically cleaned up upon abnormal process death.\n\nUnfortunately, and unlike in the previous step, there isn't a\nstraightforward way to inject a failure into the update-server-info step\nthat causes us to die() rather than take the cleanup path in label\n'out', hence the lack of a test here.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n server-info.c | 26 ++++++++++----------------\n 1 file changed, 10 insertions(+), 16 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex 6feaa457c5c..37d1085982e 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -13,6 +13,7 @@\n #include \"object-store-ll.h\"\n #include \"server-info.h\"\n #include \"strbuf.h\"\n+#include \"tempfile.h\"\n \n struct update_info_ctx {\n \tFILE *cur_fp;\n@@ -75,9 +76,8 @@ static int update_info_file(char *path,\n \t\t\tint force)\n {\n \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n+\tstruct tempfile *f = NULL;\n \tint ret = -1;\n-\tint fd = -1;\n-\tFILE *to_close;\n \tstruct update_info_ctx uic = {\n \t\t.cur_fp = NULL,\n \t\t.old_fp = NULL,\n@@ -86,13 +86,12 @@ static int update_info_file(char *path,\n \t};\n \n \tsafe_create_leading_directories(path);\n-\tfd = git_mkstemp_mode(tmp, 0666);\n-\tif (fd < 0)\n+\tf = mks_tempfile_m(tmp, 0666);\n+\tif (!f)\n \t\tgoto out;\n-\tto_close = uic.cur_fp = fdopen(fd, \"w\");\n+\tuic.cur_fp = fdopen_tempfile(f, \"w\");\n \tif (!uic.cur_fp)\n \t\tgoto out;\n-\tfd = -1;\n \n \t/* no problem on ENOENT and old_fp == NULL, it's stale, now */\n \tif (!force)\n@@ -121,27 +120,22 @@ static int update_info_file(char *path,\n \t}\n \n \tuic.cur_fp = NULL;\n-\tif (fclose(to_close))\n-\t\tgoto out;\n \n \tif (uic_is_stale(&uic)) {\n-\t\tif (adjust_shared_perm(tmp) < 0)\n+\t\tif (adjust_shared_perm(get_tempfile_path(f)) < 0)\n \t\t\tgoto out;\n-\t\tif (rename(tmp, path) < 0)\n+\t\tif (rename_tempfile(&f, path) < 0)\n \t\t\tgoto out;\n \t} else {\n-\t\tunlink(tmp);\n+\t\tdelete_tempfile(&f);\n \t}\n \tret = 0;\n \n out:\n \tif (ret) {\n \t\terror_errno(\"unable to update %s\", path);\n-\t\tif (uic.cur_fp)\n-\t\t\tfclose(uic.cur_fp);\n-\t\telse if (fd >= 0)\n-\t\t\tclose(fd);\n-\t\tunlink(tmp);\n+\t\tif (f)\n+\t\t\tdelete_tempfile(&f);\n \t}\n \tfree(tmp);\n \tif (uic.old_fp)\n-- \n2.45.2.411.g2d5a0536af1\n"},{"id":"496649","messageId":"xmqq4ja4hico.fsf@gitster.g","threadId":"61602","inReplyTo":"cover.1717712358.git.me@ttaylorr.com","subject":"Re: [PATCH 0/2] commit-graph/server-info: use tempfile.h in more places","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T15:40:07Z","receivedAt":"2024-06-07T15:40:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> This pair of patches addresses two issues (in the commit-graph and\n> update-server-info areas, respectively), where temporary files are\n> created outside of the tempfile.h API and thus survive abnormal process\n> death.\n>\n> The commit-graph one is more prevalent, and has been the cause of some\n> minor headaches (for e.g. automated tooling to detect repository cruft\n> at GitHub complaining about unknown tmp_graph_XXXXXX files left around).\n\n;-)  \n\nI'd be very surprised if a stale \"update-server-info\" product causes\nany harm, but we unfortunately cannot remove it until we fully\nremove the suport for HTTP walkers.\n\n> The fixes in both instances are relatively straightforward conversions\n> to use the tempfile.h API.\n\n> Looking at the remaining uses of mkstemp(), the remaining class of\n> callers that don't use the tempfile.h API are for creating temporary\n> .idx, .rev files, and similar. My personal feeling is that we should\n> apply similar treatment there, since these files are generated based on\n> .pack data, and thus keeping around temporary copies is unnecessary when\n> they can be regenerated.\n\nAbsolutely.\n\nI wouldn't be surprised by .idx as it and .pack are so old, dating\nback to 2005, but anything newer like .rev I am mildly surprised\nthat we haven't done this conversion.\n"},{"id":"496650","messageId":"xmqqfrtog2rd.fsf@gitster.g","threadId":"61602","inReplyTo":"2d5a0536af1a6d45835622e2c020266079fa0873.1717712358.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] server-info.c: remove temporary info files on exit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T16:02:14Z","receivedAt":"2024-06-07T16:02:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> @@ -121,27 +120,22 @@ static int update_info_file(char *path,\n>  \t}\n>  \n>  \tuic.cur_fp = NULL;\n> -\tif (fclose(to_close))\n> -\t\tgoto out;\n\nWe should fflush() of cur_fp before nuking it, at least, no?\n\nIn the original code, to_close was a mere copy of uic.cur_fp and we\nmade sure that anything buffered at the stdio layer are flushed to\nthe underlying file desciptor (fd that we obtained from\ngit_mkstemp_mode() in the original code) with this fclose() call.\n\nWe no longer do so.  We later call rename_tempfile() to close the\nunderlying file descriptor and move the temporary file to its final\nplace, but I do not see what guarantee we have that we do not lose\nwhat we had buffered in the stdio with the updated code.\n\n>  \tif (uic_is_stale(&uic)) {\n> -\t\tif (adjust_shared_perm(tmp) < 0)\n> +\t\tif (adjust_shared_perm(get_tempfile_path(f)) < 0)\n>  \t\t\tgoto out;\n> -\t\tif (rename(tmp, path) < 0)\n> +\t\tif (rename_tempfile(&f, path) < 0)\n>  \t\t\tgoto out;\n>  \t} else {\n> -\t\tunlink(tmp);\n> +\t\tdelete_tempfile(&f);\n>  \t}\n>  \tret = 0;\n>  \n>  out:\n>  \tif (ret) {\n>  \t\terror_errno(\"unable to update %s\", path);\n> -\t\tif (uic.cur_fp)\n> -\t\t\tfclose(uic.cur_fp);\n> -\t\telse if (fd >= 0)\n> -\t\t\tclose(fd);\n> -\t\tunlink(tmp);\n> +\t\tif (f)\n> +\t\t\tdelete_tempfile(&f);\n>  \t}\n>  \tfree(tmp);\n>  \tif (uic.old_fp)\n"},{"id":"496654","messageId":"xmqqa5jwg1aj.fsf@gitster.g","threadId":"61602","inReplyTo":"25324fea5b7c7f748d7f4e1e40299c0af04006e8.1717712358.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T16:33:56Z","receivedAt":"2024-06-07T16:33:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> @@ -2133,8 +2132,6 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n>  \t\tchar *final_graph_name;\n>  \t\tint result;\n>  \n> -\t\tclose(fd);\n> -\n>  \t\tif (!chainf) {\n>  \t\t\terror(_(\"unable to open commit-graph chain file\"));\n>  \t\t\treturn -1;\n> @@ -2169,7 +2166,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n>  \t\tfree(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);\n>  \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n>  \n> -\t\tresult = rename(ctx->graph_name, final_graph_name);\n> +\t\tresult = rename_tempfile(&graph_layer, final_graph_name);\n\nBefore this rename, after the close(fd) we saw in the previous hunk,\nthere is one early error return when we fail to rename the base\ngraph file.  Do we need to do anything there, or an unfinished\ntempfile getting removed at the process termination is sufficient\nfor cleaning up the mess?\n\nOther than that, this looked quite straight-forward.\n\nThanks.\n\n"},{"id":"496677","messageId":"ZmN+crXyZOze122U@nand.local","threadId":"61602","inReplyTo":"xmqqa5jwg1aj.fsf@gitster.g","subject":"Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-07T21:41:06Z","receivedAt":"2024-06-07T21:41:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jun 07, 2024 at 09:33:56AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > @@ -2133,8 +2132,6 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n> >  \t\tchar *final_graph_name;\n> >  \t\tint result;\n> >\n> > -\t\tclose(fd);\n> > -\n> >  \t\tif (!chainf) {\n> >  \t\t\terror(_(\"unable to open commit-graph chain file\"));\n> >  \t\t\treturn -1;\n> > @@ -2169,7 +2166,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n> >  \t\tfree(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);\n> >  \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n> >\n> > -\t\tresult = rename(ctx->graph_name, final_graph_name);\n> > +\t\tresult = rename_tempfile(&graph_layer, final_graph_name);\n>\n> Before this rename, after the close(fd) we saw in the previous hunk,\n> there is one early error return when we fail to rename the base\n> graph file.  Do we need to do anything there, or an unfinished\n> tempfile getting removed at the process termination is sufficient\n> for cleaning up the mess?\n\nWe could explicitly clean it up, but we'll do so implicitly upon exit,\nso I think it's fine to leave it as-is.\n\nThanks,\nTaylor\n"},{"id":"496678","messageId":"ZmN/NZlriT3gsw7p@nand.local","threadId":"61602","inReplyTo":"xmqqfrtog2rd.fsf@gitster.g","subject":"Re: [PATCH 2/2] server-info.c: remove temporary info files on exit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-07T21:44:21Z","receivedAt":"2024-06-07T21:44:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jun 07, 2024 at 09:02:14AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > @@ -121,27 +120,22 @@ static int update_info_file(char *path,\n> >  \t}\n> >\n> >  \tuic.cur_fp = NULL;\n> > -\tif (fclose(to_close))\n> > -\t\tgoto out;\n>\n> We should fflush() of cur_fp before nuking it, at least, no?\n>\n> In the original code, to_close was a mere copy of uic.cur_fp and we\n> made sure that anything buffered at the stdio layer are flushed to\n> the underlying file desciptor (fd that we obtained from\n> git_mkstemp_mode() in the original code) with this fclose() call.\n>\n> We no longer do so.  We later call rename_tempfile() to close the\n> underlying file descriptor and move the temporary file to its final\n> place, but I do not see what guarantee we have that we do not lose\n> what we had buffered in the stdio with the updated code.\n\nrename_tempfile() first calls close_tempfile_gently() before calling\nrename(). close_tempfile_gently() calls fclose() on temp->fp before\nreturns, so we get our fflush() call implicitly there.\n\nIOW, the tempfile.h API is happy for us to call rename_tempfile()\nwithout having explicitly closed or flushed the underlying file pointer.\n\nThanks,\nTaylor\n"},{"id":"496679","messageId":"xmqqo78ce86p.fsf@gitster.g","threadId":"61602","inReplyTo":"ZmN+crXyZOze122U@nand.local","subject":"Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T21:47:58Z","receivedAt":"2024-06-07T21:48:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> > -\t\tresult = rename(ctx->graph_name, final_graph_name);\n>> > +\t\tresult = rename_tempfile(&graph_layer, final_graph_name);\n>>\n>> Before this rename, after the close(fd) we saw in the previous hunk,\n>> there is one early error return when we fail to rename the base\n>> graph file.  Do we need to do anything there, or an unfinished\n>> tempfile getting removed at the process termination is sufficient\n>> for cleaning up the mess?\n>\n> We could explicitly clean it up, but we'll do so implicitly upon exit,\n> so I think it's fine to leave it as-is.\n\nI am not worried about cleaning it up.  Upon exit, the underlying\nfile descriptor will be closed, but this new code never does\nfclose() on the FILE* that has a buffer around the underlying file\ndescriptor.  How are we guaranteeing that we are not losing anything\nbuffered but not flushed yet?\n"},{"id":"496680","messageId":"xmqqjzj0e84x.fsf@gitster.g","threadId":"61602","inReplyTo":"ZmN/NZlriT3gsw7p@nand.local","subject":"Re: [PATCH 2/2] server-info.c: remove temporary info files on exit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T21:49:02Z","receivedAt":"2024-06-07T21:49:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> We no longer do so.  We later call rename_tempfile() to close the\n>> underlying file descriptor and move the temporary file to its final\n>> place, but I do not see what guarantee we have that we do not lose\n>> what we had buffered in the stdio with the updated code.\n>\n> rename_tempfile() first calls close_tempfile_gently() before calling\n> rename(). close_tempfile_gently() calls fclose() on temp->fp before\n> returns, so we get our fflush() call implicitly there.\n\nOK.  I didn't see that fclose().  Then we are safe.\n\nThanks.\n"},{"id":"496681","messageId":"xmqqfrtoe843.fsf@gitster.g","threadId":"61602","inReplyTo":"ZmN+crXyZOze122U@nand.local","subject":"Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T21:49:32Z","receivedAt":"2024-06-07T21:49:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> Before this rename, after the close(fd) we saw in the previous hunk,\n>> there is one early error return when we fail to rename the base\n>> graph file.  Do we need to do anything there, or an unfinished\n>> tempfile getting removed at the process termination is sufficient\n>> for cleaning up the mess?\n>\n> We could explicitly clean it up, but we'll do so implicitly upon exit,\n> so I think it's fine to leave it as-is.\n\nOK.  That sounds good.\n"},{"id":"496697","messageId":"20240608094258.GA2659849@coredump.intra.peff.net","threadId":"61602","inReplyTo":"xmqqo78ce86p.fsf@gitster.g","subject":"Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T09:42:58Z","receivedAt":"2024-06-08T09:42:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 07, 2024 at 02:47:58PM -0700, Junio C Hamano wrote:\n\n> Taylor Blau <me@ttaylorr.com> writes:\n> \n> >> > -\t\tresult = rename(ctx->graph_name, final_graph_name);\n> >> > +\t\tresult = rename_tempfile(&graph_layer, final_graph_name);\n> >>\n> >> Before this rename, after the close(fd) we saw in the previous hunk,\n> >> there is one early error return when we fail to rename the base\n> >> graph file.  Do we need to do anything there, or an unfinished\n> >> tempfile getting removed at the process termination is sufficient\n> >> for cleaning up the mess?\n> >\n> > We could explicitly clean it up, but we'll do so implicitly upon exit,\n> > so I think it's fine to leave it as-is.\n> \n> I am not worried about cleaning it up.  Upon exit, the underlying\n> file descriptor will be closed, but this new code never does\n> fclose() on the FILE* that has a buffer around the underlying file\n> descriptor.  How are we guaranteeing that we are not losing anything\n> buffered but not flushed yet?\n\nI'm not sure I understand. There is no \"FILE *\" at all in this patch (we\nuse the descriptor directly via the hashfd interface). But even if there\nwere, if we leave without renaming the temp file into place, then that\ntempfile will be deleted. So it wouldn't matter if we flushed to it or\nnot, because all of those byte are destined to be thrown away.\n\n-Peff\n"},{"id":"496699","messageId":"20240608095322.GB2659849@coredump.intra.peff.net","threadId":"61602","inReplyTo":"25324fea5b7c7f748d7f4e1e40299c0af04006e8.1717712358.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T09:53:22Z","receivedAt":"2024-06-08T09:53:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 06, 2024 at 06:19:25PM -0400, Taylor Blau wrote:\n\n> @@ -2035,24 +2035,23 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n>  \t\t\t\t\t       LOCK_DIE_ON_ERROR, 0444);\n>  \t\tfree(lock_name);\n>  \n> -\t\tfd = git_mkstemp_mode(ctx->graph_name, 0444);\n> -\t\tif (fd < 0) {\n> +\t\tgraph_layer = mks_tempfile_m(ctx->graph_name, 0444);\n> +\t\tif (!graph_layer) {\n>  \t\t\terror(_(\"unable to create temporary graph layer\"));\n>  \t\t\treturn -1;\n>  \t\t}\n>  \n> -\t\tif (adjust_shared_perm(ctx->graph_name)) {\n> +\t\tif (adjust_shared_perm(get_tempfile_path(graph_layer))) {\n>  \t\t\terror(_(\"unable to adjust shared permissions for '%s'\"),\n> -\t\t\t      ctx->graph_name);\n> +\t\t\t      get_tempfile_path(graph_layer));\n>  \t\t\treturn -1;\n>  \t\t}\n\nMost errors will cause us to die(), but this \"return\" and the one that\nJunio noted later (when renaming the base graph file fails) mean we'll\nreturn with the tempfile still active. We'll clean it up when the\nprogram exits, but if there were a long-running program that\ncalled write_commit_graph_file(), that tempfile might hang around longer\nthan necessary.\n\nBut that is the same strategy that the existing code uses for the lock\nwe use for the chain filename! And it is much worse there, because now\nit is not just a tempfile hanging around, but we're blocking anybody\nelse from taking the lock.\n\nSo I think it would be OK to punt on this for now. Your patch is not\nmaking the situation worse, and it's all a problem for a hypothetical\nlibification of this function.\n\n> +test_expect_success 'temporary graph layer is discarded upon failure' '\n> +\tgit init layer-discard &&\n> +\t(\n> +\t\tcd layer-discard &&\n> +\n> +\t\ttest_commit A &&\n> +\t\ttest_commit B &&\n> +\n> +\t\t# Intentionally remove commit \"A\" from the object store\n> +\t\t# so that the commit-graph machinery fails to parse the\n> +\t\t# parents of \"B\".\n> +\t\t#\n> +\t\t# This takes place after the commit-graph machinery has\n> +\t\t# initialized a new temporary file to store the contents\n> +\t\t# of the new graph layer, so will allow us to ensure\n> +\t\t# that the temporary file is discarded upon failure.\n> +\t\trm $objdir/$(test_oid_to_path $(git rev-parse HEAD^)) &&\n> +\n> +\t\ttest_must_fail git commit-graph write --reachable --split &&\n> +\t\ttest_dir_is_empty $graphdir\n> +\t)\n> +'\n\nI'm glad you were able to come up with a case that fails cleanly and\nnon-racily. The exit code of rev-parse will be lost. I doubt that it\nmatters in practice, but I wouldn't be surprised if our shell linting\neventually learns to complain about this spot.\n\nLooks like there are a few similar ones in the test suite already, from\nt5329. I'd be content to leave it for now and deal with it later if\nsomebody really wants to go on a crusade against lost exit codes.\n\n-Peff\n"},{"id":"496701","messageId":"20240608102502.GC2659849@coredump.intra.peff.net","threadId":"61602","inReplyTo":"2d5a0536af1a6d45835622e2c020266079fa0873.1717712358.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] server-info.c: remove temporary info files on exit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T10:25:02Z","receivedAt":"2024-06-08T10:25:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 06, 2024 at 06:19:31PM -0400, Taylor Blau wrote:\n\n> The update_info_file() function within server-info.c is responsible for\n> moving the info/refs and info/packs files around when updating server\n> info.\n> \n> These updates are staged into a temporary file and then moved into place\n> atomically to avoid race conditions when reading those files. However,\n> the temporary file used to stage these changes is managed outside of the\n> tempfile.h API, and thus survives process death.\n> \n> Manage these files instead with the tempfile.h API so that they are\n> automatically cleaned up upon abnormal process death.\n\nMakes sense. I was going to suggest that these could even be lockfiles,\nbut it is intentional to let two simultaneous processes race (with an\natomic last-one-wins result). See d38379ece9 (make update-server-info\nmore robust, 2014-09-13).\n\n> Unfortunately, and unlike in the previous step, there isn't a\n> straightforward way to inject a failure into the update-server-info step\n> that causes us to die() rather than take the cleanup path in label\n> 'out', hence the lack of a test here.\n\nThat sounds like a challenge. ;)\n\n  $ echo garbage >.git/packed-refs\n  $ git update-server-info\n  fatal: unexpected line in .git/packed-refs: garbage\n  $ ls .git/info/\n  exclude  refs  refs_QYvQGb\n\nI don't know if it's worth adding such a test. It seems rather brittle\nto assume that we'd die() here (let alone that we are using the files\nbackend at all).\n\n> @@ -86,13 +86,12 @@ static int update_info_file(char *path,\n>  \t};\n>  \n>  \tsafe_create_leading_directories(path);\n> -\tfd = git_mkstemp_mode(tmp, 0666);\n> -\tif (fd < 0)\n> +\tf = mks_tempfile_m(tmp, 0666);\n> +\tif (!f)\n>  \t\tgoto out;\n> -\tto_close = uic.cur_fp = fdopen(fd, \"w\");\n> +\tuic.cur_fp = fdopen_tempfile(f, \"w\");\n\nOK, good, fdopen_tempfile() means that the FILE handle is owned by the\ntempfile, too.\n\n> @@ -121,27 +120,22 @@ static int update_info_file(char *path,\n>  \t}\n>  \n>  \tuic.cur_fp = NULL;\n> -\tif (fclose(to_close))\n> -\t\tgoto out;\n\nAnd we don't need to fclose() anymore since the tempfile code handles\nthat for us. Nice.\n\n>  \tif (uic_is_stale(&uic)) {\n> -\t\tif (adjust_shared_perm(tmp) < 0)\n> +\t\tif (adjust_shared_perm(get_tempfile_path(f)) < 0)\n>  \t\t\tgoto out;\n> -\t\tif (rename(tmp, path) < 0)\n> +\t\tif (rename_tempfile(&f, path) < 0)\n>  \t\t\tgoto out;\n>  \t} else {\n> -\t\tunlink(tmp);\n> +\t\tdelete_tempfile(&f);\n>  \t}\n>  \tret = 0;\n\nOK, so we always rename or delete here, unless we jumped to the error\npath...\n\n>  out:\n>  \tif (ret) {\n>  \t\terror_errno(\"unable to update %s\", path);\n> -\t\tif (uic.cur_fp)\n> -\t\t\tfclose(uic.cur_fp);\n> -\t\telse if (fd >= 0)\n> -\t\t\tclose(fd);\n> -\t\tunlink(tmp);\n> +\t\tif (f)\n> +\t\t\tdelete_tempfile(&f);\n>  \t}\n\nAnd here we do an explicit delete, which is good for a lib-ified world\nwhere the process doesn't just exit immediately.\n\nI think you could actually call delete_tempfile() unconditionally, even\noutside the \"if (ret)\" block. It is a noop for a NULL tempfile (so OK\neven if we jump to \"out\" before opening it). And a renamed tempfile goes\nback to NULL as well.\n\nI.e., one of the advantages to using the tempfile interface is that it's\nalways in a consistent state, and you just use delete() on exit, like we\ndo strbuf_release().\n\nThat said, it's a pretty minor style question, and I don't think is\nworth a re-roll.\n\n-Peff\n"},{"id":"496706","messageId":"20240608104821.GF2659849@coredump.intra.peff.net","threadId":"61602","inReplyTo":"cover.1717712358.git.me@ttaylorr.com","subject":"Re: [PATCH 0/2] commit-graph/server-info: use tempfile.h in more places","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T10:48:21Z","receivedAt":"2024-06-08T10:48:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 06, 2024 at 06:19:21PM -0400, Taylor Blau wrote:\n\n> Looking at the remaining uses of mkstemp(), the remaining class of\n> callers that don't use the tempfile.h API are for creating temporary\n> .idx, .rev files, and similar. My personal feeling is that we should\n> apply similar treatment there, since these files are generated based on\n> .pack data, and thus keeping around temporary copies is unnecessary when\n> they can be regenerated.\n\nAnd actual loose object and pack files themselves, I think. I think it\nwas a deliberate choice long ago to keep those files around, since in\nthe worst case they could be used to recover actual repo content (e.g.,\na failed fetch will leave its tmp_pack_* file around, and you could\nprobably extract _some_ objects from it).\n\nAnd the philosophy is that we'd leave them sitting around until gc ran\nand found tmp_* in objects/, check their mtimes, and remove them then.\n\nIn practice, I don't think I have really seen anybody recover anything\nfrom a temporary file. You're better off looking at whatever was feeding\nthe temporary file (if it was \"git add\", then look at the filesystem,\nand if it was index-pack, look at the fetch/push source, etc).\n\nAnd leaving them around is a minor nuisance, since degenerate cases\n(like a script retrying a failed operation over and over) can let them\npile up.\n\nAnd indeed, for push we don't keep the contents around anymore. Even\nthough we don't clean up the .pack files, we stick them in a quarantine\ndirectory which does get cleaned up. This solved what used to be an\noperational headache for GitHub, and I don't think anybody has ever\ncomplained about not having their partial failed packfile available.\n\nSo I'd argue that we should just treat object/pack tempfiles like the\nrest, and delete them if they don't make it all the way to the rename\nstep. If we really want to support debugging, we could perhaps provide\na run-time knob to leave them in place (and maybe even have it apply to\n_all_ tempfiles).\n\nBut that is all way beyond your series, and I don't think there is any\nurgent need to tackle it. IIRC, GitHub's fork does sprinkle some\nregister_tempfile() calls around the odb code, but I don't have access\nto that code anymore.\n\n-Peff\n"},{"id":"497182","messageId":"CABPp-BFvGa5KhoBs6n+mzka9LoHe3-Gx0JeAYjDQjaB=TyXGkg@mail.gmail.com","threadId":"61602","inReplyTo":"20240608104821.GF2659849@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] commit-graph/server-info: use tempfile.h in more places","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-14T17:41:09Z","receivedAt":"2024-06-14T17:41:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jun 8, 2024 at 3:48 AM Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Jun 06, 2024 at 06:19:21PM -0400, Taylor Blau wrote:\n>\n> > Looking at the remaining uses of mkstemp(), the remaining class of\n> > callers that don't use the tempfile.h API are for creating temporary\n> > .idx, .rev files, and similar. My personal feeling is that we should\n> > apply similar treatment there, since these files are generated based on\n> > .pack data, and thus keeping around temporary copies is unnecessary when\n> > they can be regenerated.\n>\n> And actual loose object and pack files themselves, I think.\n[...]\n> So I'd argue that we should just treat object/pack tempfiles like the\n> rest, and delete them if they don't make it all the way to the rename\n> step. If we really want to support debugging, we could perhaps provide\n> a run-time knob to leave them in place (and maybe even have it apply to\n> _all_ tempfiles).\n>\n> But that is all way beyond your series, and I don't think there is any\n> urgent need to tackle it.\n\nRegardless, it provides more context around the exact questions I had\nwhile reading the series.  Everything in the series looked fine to me,\nbut I wondered about packs and loose objects and why those are\ndifferent.  Anyway, I like your suggestions as a long term goal.\n(Perhaps handling packs and loose objects with tempfiles could serve\nas good microprojects?)\n"},{"id":"497194","messageId":"ZmybaokdVIXOX+6D@nand.local","threadId":"61602","inReplyTo":"20240608104821.GF2659849@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] commit-graph/server-info: use tempfile.h in more places","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-14T19:35:06Z","receivedAt":"2024-06-14T19:35:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Jun 08, 2024 at 06:48:21AM -0400, Jeff King wrote:\n> On Thu, Jun 06, 2024 at 06:19:21PM -0400, Taylor Blau wrote:\n>\n> > Looking at the remaining uses of mkstemp(), the remaining class of\n> > callers that don't use the tempfile.h API are for creating temporary\n> > .idx, .rev files, and similar. My personal feeling is that we should\n> > apply similar treatment there, since these files are generated based on\n> > .pack data, and thus keeping around temporary copies is unnecessary when\n> > they can be regenerated.\n>\n> And actual loose object and pack files themselves, I think. I think it\n> was a deliberate choice long ago to keep those files around, since in\n> the worst case they could be used to recover actual repo content (e.g.,\n> a failed fetch will leave its tmp_pack_* file around, and you could\n> probably extract _some_ objects from it).\n\nHeh, yes. Those were intentionally omitted from this list ;-).\n\nI agree that having the content stick around in failed packfile and\nloose object writes is useful as a last-resort recovery mechanism. I\nsuspect that it is often difficult or impossible to recover the contents\nof an object/pack from a broken write, but I think it's better than the\nalternative of just throwing it away up front.\n\n> And the philosophy is that we'd leave them sitting around until gc ran\n> and found tmp_* in objects/, check their mtimes, and remove them then.\n>\n> In practice, I don't think I have really seen anybody recover anything\n> from a temporary file. You're better off looking at whatever was feeding\n> the temporary file (if it was \"git add\", then look at the filesystem,\n> and if it was index-pack, look at the fetch/push source, etc).\n\nYup.\n\n> So I'd argue that we should just treat object/pack tempfiles like the\n> rest, and delete them if they don't make it all the way to the rename\n> step. If we really want to support debugging, we could perhaps provide\n> a run-time knob to leave them in place (and maybe even have it apply to\n> _all_ tempfiles).\n\nI dunno... I don't disagree with what you're saying, but it does seem a\nlittle scary to delete files containing data that we might have been\nable to recover.\n\nI think the current series ends at a reasonable stopping point... I'd\nhonestly have to think more about whether I agree with what you're\nsaying here or not ;-).\n\nThanks,\nTaylor\n"}]}