Repository navigation
remove ctags temporary files when ctags is killed - #33
Merged
Merged
Conversation
In interactive mode Universal Ctags writes each reply to $TMPDIR/tags.XXXXXX and removes it once printed. When the process is killed during a request, the file stays behind. go-ctags kills ctags in Close, and Zoekt calls Close when a Parse takes more than a minute, so the leftovers are the largest replies. On Sourcegraph they land on the index volume (the indexserver points TMPDIR there) until the indexserver restarts. New now creates a private directory with os.MkdirTemp and passes it to ctags as TMPDIR, and as TMP, which ctags reads on Windows. Close removes it after Wait, once ctags can no longer write to it, and every error path in New removes it too. The reply file is the only file ctags creates in interactive mode, so nothing else ends up there. Two consequences for maintainers: - ctags reads TMPDIR again for every request and crashes if the directory is gone, so it must live as long as the process. - If $TMPDIR is unusable, New now fails with "creating temporary directory for <bin>: ..." instead of ctags crashing on its first request. Not covered: if the calling process itself is killed, Close never runs; a busy ctags still leaves its file, and an idle parser leaves an empty go-ctags-* directory. ctags has no signal handlers, so SIGTERM before SIGKILL would not help. The tests point TMPDIR at a test directory and check that it ends up empty: after killing ctags mid-reply and closing the parser, and after New fails to start ctags. Fixes sourcegraph#29 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #29.
In interactive mode, Universal Ctags writes each reply to
$TMPDIR/tags.XXXXXXand removes it once it has printed it. When the process is killed during a request, the file stays behind. go-ctags kills ctags inClose, and Zoekt callsClosewhen aParsetakes more than a minute, so the files that stay are the large ones.On Sourcegraph this lands on the index volume: zoekt-sourcegraph-indexserver sets
TMPDIRto<index>/.indexserver.tmpand only empties it when it restarts. Timeouts do happen there (zoekt#1095), and that directory has filled up before (zoekt#646). I found this by reproduction, not from a user report.This PR gives each ctags process its own directory:
Newcreates it withos.MkdirTemp("", "go-ctags-")and passes it to ctags asTMPDIR(andTMP, which ctags reads on Windows).Closeremoves it afterWait, so a killed ctags no longer leaves anything behind. Every error path inNewremoves it too.No API change. Independent of #32:
ctags.gomerges cleanly with it, andctags_test.goonly conflicts because both add imports and tests at the end of the file (keep both).Two side effects:
$TMPDIRis unusable,Newnow fails withcreating temporary directory for <bin>: …. Before, ctags started and then crashed on its first request.TMPDIRon every request and crashes if it is gone. Zoekt's parsers live for one shard, so this only matters for a long-idle parser on a host whose tmp cleaner deletes old directories.What it does not fix: if the calling process itself is killed (for example zoekt-git-index on
INDEXING_TIMEOUTor OOM),Closenever runs. A busy ctags then still leaves its file, as today, and an idle parser now leaves an emptygo-ctags-*directory. The prefix makes those easy to sweep. Ctags has no signal handlers, so sending SIGTERM before SIGKILL would not help. The complete fix would be upstream: unlinking the file right aftermkstemp.Test Plan:
TestCloseRemovesTempDir: real ctags withTMPDIRpointed at an empty test directory; kill ctags while it writes its reply, thenClose; the test directory is empty again. It fails ifClosedoes not remove the directory, or if ctags does not get the privateTMPDIR.TestNewFailureLeavesNoTempDir: when ctags cannot start,Newreturns an error and leaves the test directory empty.go test -race ./...passes on ctags 6.2.1 and on 6.0.0.2783f009 (the CI build, darwin)../gen.shleaves no diff.tags.Ufwjwu: 68947968 byteson main and nothing on this branch.replaceto this branch:go test ./internal/ctags/... ./index/... ./cmd/zoekt-index/...passes.🤖 Generated with Claude Code