fix(#5978,#5999): unique temp names for link-pass atomic writes; cancel heavy batch children by process group - #6020
Merged
Conversation
Both of the package's temp+rename sinks built their temp path by concatenating ".tmp" onto the destination, so every writer aiming at one destination shared a single temp file. Two link passes over the same group therefore interleaved their bytes inside that file — the destination could be left holding a torn mix of two passes' output — and the writer that renamed second failed with ENOENT. The two sinks fail differently, and the difference matters. writeDoc (candidates.go), which writes the cross-repo links themselves, always returned that ENOENT to its caller, so a lost link write was at least reported. The scan cache in string_pass.go discards its rename error outright and still does: it is a best-effort per-file cache keyed on mtime+size, and a miss only costs a rescan. What was NOT tolerable there is the torn write — a half-written cache entry the next pass would read back as truth. Both sinks now go through one writeFileAtomic helper that creates a unique temp file with os.CreateTemp in the destination's own directory (same filesystem, so the rename stays atomic), sets the requested mode, and removes the temp file on every error path. BEHAVIOUR CHANGE — permission bits are no longer umask-masked. os.WriteFile passed perm through open(2), so a restrictive umask narrowed these files; os.CreateTemp makes the file 0600 and the explicit Chmod now sets exactly 0644. Under umask 077 these destinations therefore widen from 0600 to 0644. Deliberate (they are per-user state under the grafel home, and a mode that does not depend on which process ran the pass is easier to reason about), documented on the helper, and pinned by a test — deleting the Chmod leaves the mode at 0600 and fails. The concurrency test drives eight concurrent writers at one destination and asserts every write succeeds and the surviving file is one COMPLETE payload. Against the old deterministic name it fails on the first iteration with a torn destination. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft
The batch children are spawned with Setpgid (applyGroupAlgoNice), so each one already leads its own process group — but no spawn site overrode cmd.Cancel, so cancellation fell through to exec.CommandContext's default: a single-pid SIGKILL. Anything the child had forked survived, kept the inherited stdout/stderr pipes open, and so kept the supervising runner blocked in its drain-to-EOF loop with the daemon's exclusive heavy-stage token still held. TestRunSubprocessLinks_CancelTerminatesChild was failing 2-4 runs in 10 under -race for exactly this reason. applyGroupAlgoNice now also wires cmd.Cancel to SIGKILL the negative pid (the whole group), with a fallback to the single-pid kill if the group signal fails. The two are set together in one hook so no spawn site can take the process group without the matching cancellation. Also applied to the INDEX child, which previously had neither. It is the one that really fans out — the extract coordinator forks a `grafel extract` subprocess per batch — so a cancelled reindex left those grandchildren running. Both of its call sites are inside the daemon, with no controlling terminal, so giving it its own process group costs no signal delivery anything relies on. Windows keeps the os/exec default (no setpgid, no signals; a tree kill there needs a Job object) and nice_windows.go now says so instead of leaving it implied. The new test forks a stand-in child that backgrounds a `sleep`, records its pid and blocks. Without the override it fails deterministically 10/10 for both the links and the group-algo runner: the runner is still blocked 5s after cancel and the grandchild is still alive. Several doc comments that asserted "cancellation is cmd.Process.Kill" (and one that described it as SIGTERM, which was never true) are corrected to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft
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
1 participant
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.
Two independent fixes to the daemon's heavy-batch path, both found to be worse than originally filed.
#5978 — the link pass's atomic writes collide
internal/links/candidates.goandstring_pass.gobuilt temp files aspath + ".tmp": deterministic per destination. Two concurrent writers thereforeO_TRUNCand interleave into the same tmp inode, then each renames a torn file into place. The failure is not a lost write — it is a garbled one that a later pass reads back as truth.Fixed with
writeFileAtomic(os.CreateTempin the destination directory → write → chmod → rename, remove-on-error).Behaviour change, stated deliberately:
os.WriteFile(tmp, b, 0o644)was umask-masked by open(2);os.CreateTemp+os.Chmodis not. Underumask 077these files widen from 0600 to 0644. Restoring masking would mean readingsyscall.Umask, which is process-global and racy; a mode that does not depend on whether the daemon, the CLI, or a child ran the pass is the better property for per-user state under the grafel home. Documented on the helper and here rather than left to be discovered.Note on the sinks, since an earlier draft of this message got it wrong:
writeDoc— the actual cross-repo links output — always returned its error, so a dropped write there was already reported.string_pass.go's scan cache still discards its error deliberately (best-effort, keyed on mtime+size; a miss costs a rescan). What neither could tolerate was the torn entry.#5999 — the heavy batch children were cancelled by pid, not by process group
exec.CommandContext's defaultCancelisProcess.Kill()— a single-pid kill. The child's own children survived.applyGroupAlgoNicenow setsSetpgid: trueand wirescmd.Canceltosyscall.Kill(-pid, SIGKILL).The two halves are load-bearing together, and the doc comment explaining why previously stated the wrong mechanism. Corrected:
kill(-pid)addresses the group whose PGID equals that pid — not the caller's group. WithoutSetpgidthat group normally does not exist, so the kill returns ESRCH and the fallback quietly papers over it, meaning the group kill never binds at all. The real residual hazard is pid reuse aliasing onto an unrelated group's PGID.Verification
Both fixes were confirmed to bind, by mutation rather than inspection:
path + ".tmp"applyProcessGroupCancelSetpgid(keep the group kill)os.Chmod(tmp, perm)That last row is the interesting one. The mode was unbound, so a future edit could have silently dropped these files to 0600 with the suite green.
TestWriteFileAtomicAppliesRequestedModenow assertsos.Stat().Mode().Perm()for 0644 and 0600 through the helper and 0644 through the realwriteDoccaller; the mutation is dead.RunSubprocessIndexwas checked on three axes before extending the group cancel to it: SIGTERM/SIGINT still reach the child, thegrafel extractchildren write nothing to disk (they stream on stdout), and both callers are daemon-side.Local gate:
go build ./...,go vet,gofmt -lclean;GOOS=windowsbuild/vet ofinternal/daemon/schedclean; 1686 tests across 22 packages;internal/links -race -count=2(724) andCancelTerminatesChild -race -count=10(10/10) green.Not done here
The codebase-wide sweep. This is the third independent occurrence of the
.tmpclass —internal/statusfile/statusfile.go:434-441(citing review #5734) andinternal/daemon/watchreg/watchreg.go:255-261each diagnosed and fixed it separately, andinternal/daemon/sched/subprocess_runner.go:775already names it a known hazard and argues its own path is safe via a scheduler invariant. 48 deterministic.tmpsites remain across 40 files. Tracked as #6018, deliberately out of scope here.One unrelated flake observed and not touched:
TestIndexErrorRecorded(internal/daemon/sched/scheduler_test.go:282) failed once under-race -count=4. It asserts after a fixedtime.Sleep(150ms)and injects an in-processIndexfunc, so it never reaches the subprocess path; it passed 20/20 in isolation and passed the identical run before these edits.Independently adversarially reviewed.
Closes #5978
Closes #5999