mirror of
https://gh-proxy.org/https://github.com/klinsmaya/multica.git
synced 2026-11-08 01:16:45 +08:00
520359a2c9
* refactor(daemon): give the lock and dir-stat helpers neutral names openEnvRootLockFile, releaseEnvRootLock and codexStoreStat are all generic: they take a path, and nothing in them knows about env roots or Codex stores. The next commit reuses the same execution lock for per-task temp directories, where the old names would read as a bug at the call site. No behaviour change. Co-authored-by: multica-agent <[email protected]> * fix(daemon): reclaim orphaned per-task temp dirs in the GC Every task gets a private temp dir (multica-task-*) under the system temp base, exported to the agent as TMPDIR/TMP/TEMP. Its only exit was the RemoveAll deferred at the end of runTask, which does not run when the daemon is killed and does not succeed while a file inside is still open — the Windows case reported in #7364, where one machine had accumulated 174 leftover directories. The GC never covered them: it scans WorkspacesRoot, and the temp base is a different root, so multica-task- had exactly one create site and zero reclaim sites. Sweep the temp base as a fourth out-of-root pruner. Liveness is decided by the directory's .task_lock — the same OS advisory lock an env root uses — not by age: the sweep removes a directory only once it has itself acquired the lock the owner would still be holding, which the kernel grants only after that owner is gone. Using the lock rather than a marker or a TTL is what makes this correct rather than probabilistic. It is held by an open file description, so it answers for a live task in this very process as well as one owned by a different daemon sharing /tmp, which no in-memory active set could see. And it needs no upper bound on task runtime — there is none to assume, since DefaultAgentTimeout is 0. Directories left by daemons predating the lock carry no lock file, so age is the only signal available for them; they are reclaimed after MULTICA_GC_TASK_TEMP_LEGACY_TTL (72h). That branch exists to clear the existing leftovers and can be dropped in a release or two. The end-of-run RemoveAll stays exactly as it was, and nothing was added to the task-completion path: the run releases the lock before removing, so a removal that fails there costs nothing and the next GC cycle finishes the job. Co-authored-by: multica-agent <[email protected]> * fix(daemon): keep the temp dir marker when cleanup cannot finish Two fixes from review. 1. A failed cleanup destroyed the marker the next sweep needs. os.RemoveAll keeps walking after a failure, so removing a directory whose payload is still open deleted the .task_lock sitting next to it and then returned the error. The directory came back as one with no marker — indistinguishable from a pre-lock leftover, so it was no longer reclaimable on liveness at all. That is exactly #7364's failure path, and it made the claim this change is built on ("a failed removal costs nothing, the next cycle retries") false. RemoveTaskTempDir removes contents first and stops at the first one it cannot remove, leaving the marker behind; the lock file goes last, once everything else is gone. Both the task-completion path and the GC use it. A cleanup that fails now leaves a directory that is still lock-bearing and still unlocked, so the next cycle takes the lock and retries — for as many cycles as the holder needs. 2. The legacy TTL could delete a live task's TMPDIR. Pre-lock directories were reclaimed on age after 72h by default, but age cannot establish liveness: DefaultAgentTimeout is 0, so a task may legitimately run far longer than any TTL; a daemon on another profile can still be running the old binary during an upgrade; and every daemon on the machine shares one temp base. The default is now 0 — leave them alone — and reclaiming them is an operator's explicit opt-in. Each cycle logs how many were left in place so the choice stays visible rather than silent. Classification also fails closed: only os.ErrNotExist means "no lock file". Any other stat error (another user's dir under a shared /tmp) now reads as in-use instead of legacy. Regressions for both: a Windows test that makes a real sharing violation with CreateFile minus FILE_SHARE_DELETE, asserting the marker survives the failed cleanup and the directory goes once the handle closes (wired into the Windows CI job, which runs targeted filters rather than whole packages); and unix equivalents driving the same paths through an unlinkable payload, plus fail-closed coverage for an unreadable directory. Co-authored-by: multica-agent <[email protected]> * fix(daemon): publish the temp dir marker atomically, restore it on rmdir failure Two more windows in the marker's lifecycle, from review. 1. A directory could be swept between marker creation and lock acquisition. LockTaskTempDir created .task_lock and locked it as two steps, so there was an instant where the marker existed unlocked. A sweep landing there reads exactly what a dead owner leaves behind: it takes the lock, concludes the owner is gone, and deletes the directory — after which the starting task's own lock call succeeds and it runs with a TMPDIR that no longer exists. The marker is now locked under .task_lock.claiming and renamed into place once held. Rename within one directory is atomic and the lock rides on the open file description, not the name, so the marker becomes visible and held in the same step. Before that rename the directory carries no marker, which classifies as legacy — and legacy only reclaims what is OLDER than legacyTTL. A directory created microseconds ago is younger than any positive TTL, and a TTL of 0 disables the branch outright, so no configuration can sweep the pre-publish state either. 2. A failed rmdir left a directory with no marker. The marker has to be deleted before the directory can be removed, so a failure at that last step — a Windows process holding a handle to the directory, or content an orphaned child recreated — left a directory nothing could classify again. With the legacy branch disabled by default that is a permanent leak, and it was the one path where cleanup got furthest. The marker is now put back when that removal fails, so every failure mode leaves the retry available. Regressions: the publication order and the mid-claim on-disk state (spared under a disabled TTL and under positive ones), a sweeper running flat out against 200 publish/use/cleanup cycles, and the rmdir-failure branch — which fails without the restore, verified by removing it. Co-authored-by: multica-agent <[email protected]> * fix(daemon): decide the legacy sweep on content, not on age alone The publication window was still open. The claim-then-rename ordering stopped .task_lock from ever existing unlocked, but a directory mid-claim carries no .task_lock at all, so the sweep still reaches it through the legacy branch — and the argument that protected it there was wrong. That argument was "a directory created microseconds ago is younger than any positive TTL". MULTICA_GC_TASK_TEMP_LEGACY_TTL takes any duration, down to a nanosecond, and an owner stalled between MkdirTemp and the rename outlasts a large TTL just as easily. Reproduced at 1ns, 1ms, 1h and 72h alike: the sweep deleted a directory whose claim was held. Age cannot separate a pre-lock leftover from a directory being published, so stop asking it to. Content can: a directory being published holds nothing but its own lock files, because the task has not been handed the path yet and cannot have written anything, while a leftover worth reclaiming is by definition one with something in it. The legacy branch now reclaims only directories that hold task content, which covers both halves of the window — the empty directory before the claim exists, and the claim-only directory after — at every TTL value, with no timing assumption left in the argument. The cost is that a pre-lock daemon which crashed before its task wrote anything leaves an empty directory nothing reclaims. That is a few bytes against deleting the temp dir of a task that is starting. Tests: the mid-claim cases now run at 1ns/1ms/1h/72h with now pushed PAST the TTL, so they fail if anything reintroduces a timing argument; the pre-claim (empty directory) half is covered the same way; and the invariant itself is pinned directly, so relaxing it breaks a test rather than a race. The existing legacy TTL test now models a real leftover by giving it content. Verified all of these fail with the content rule removed. Co-authored-by: multica-agent <[email protected]> * fix(daemon): re-check the lock immediately before removing a legacy dir The legacy branch's decision is several separate observations — classify, then walk for content, then walk for mtime — and a directory can finish publishing in the middle of them. Classified as legacy while its marker was still unpublished, then published and written to by its task, and the removal went ahead without ever looking at the marker that had appeared: the sweep deletes a live task's TMPDIR. What was accidentally holding this together was that the task's own write refreshes the mtime — a timing crutch, in the branch whose whole point is not to rely on one. Re-read the classification immediately before removing, and skip unless it is still legacy. That closes the window rather than narrowing it, because of what content means: a task only writes into the directory after it has been handed the path, which happens only after LockTaskTempDir returns and therefore only after the marker is published and held. So content observed at the top of the branch, plus no marker seen at the bottom, proves no live task owns this directory — and none can arrive later either, since MkdirTemp only ever hands out a fresh empty directory under a new name. The regression drives that exact interleaving through a barrier: the sweep commits to removing the directory, the owner then publishes and its task writes, and the sweep must notice. `now` is far past the TTL so age cannot mask it. Verified it fails without the re-check. Also from review: drop a stale test comment that still claimed a positive TTL can only be configured far above a directory's age, and record in the config and operator docs that a directory holding no task content is never reclaimed on age — including a shell left by a CURRENT daemon that died between creating the directory and publishing its lock, not just an old daemon's empty leftover. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]>
85 lines
2.6 KiB
Go
85 lines
2.6 KiB
Go
//go:build windows
|
|
|
|
package execenv
|
|
|
|
import (
|
|
"os"
|
|
"path/filepath"
|
|
"testing"
|
|
"time"
|
|
|
|
"golang.org/x/sys/windows"
|
|
)
|
|
|
|
// TestPruneTaskTempDirsSurvivesRealSharingViolation is the Windows regression
|
|
// for #7364's actual failure mode, with a real sharing violation rather than a
|
|
// simulated one: a payload opened WITHOUT FILE_SHARE_DELETE cannot be deleted
|
|
// while the handle is open, which is exactly what a task's leftover child
|
|
// process does to its node compile cache.
|
|
//
|
|
// What has to hold: the failed cleanup leaves .task_lock intact, so the
|
|
// directory stays reclaimable on liveness, and the cycle after the handle
|
|
// closes removes it — with the legacy TTL disabled throughout, proving no part
|
|
// of this depends on age.
|
|
func TestPruneTaskTempDirsSurvivesRealSharingViolation(t *testing.T) {
|
|
base := t.TempDir()
|
|
dir := makeTaskTempDir(t, base, "sharing-violation", true)
|
|
|
|
payload := filepath.Join(dir, "node-compile-cache")
|
|
if err := os.WriteFile(payload, make([]byte, 2048), 0o600); err != nil {
|
|
t.Fatalf("write payload: %v", err)
|
|
}
|
|
payloadPtr, err := windows.UTF16PtrFromString(payload)
|
|
if err != nil {
|
|
t.Fatalf("UTF16PtrFromString(): %v", err)
|
|
}
|
|
// No FILE_SHARE_DELETE: Windows now refuses to delete this file.
|
|
handle, err := windows.CreateFile(
|
|
payloadPtr,
|
|
windows.GENERIC_READ,
|
|
windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE,
|
|
nil,
|
|
windows.OPEN_EXISTING,
|
|
windows.FILE_ATTRIBUTE_NORMAL,
|
|
0,
|
|
)
|
|
if err != nil {
|
|
t.Fatalf("CreateFile(): %v", err)
|
|
}
|
|
handleClosed := false
|
|
closeHandle := func() {
|
|
if handleClosed {
|
|
return
|
|
}
|
|
handleClosed = true
|
|
_ = windows.CloseHandle(handle)
|
|
}
|
|
t.Cleanup(closeHandle)
|
|
|
|
// Establish that the sharing violation is real before asserting anything
|
|
// about how the sweep copes with it.
|
|
if err := os.Remove(payload); err == nil {
|
|
t.Skip("this Windows build allowed deleting an open file; nothing to regress against")
|
|
}
|
|
|
|
if removed, _ := PruneTaskTempDirs(base, 0, time.Now(), testLogger()); removed != 0 {
|
|
t.Fatalf("prune reported %d removals while the payload was held, want 0", removed)
|
|
}
|
|
if _, err := os.Stat(filepath.Join(dir, envRootLockFile)); err != nil {
|
|
t.Fatalf("failed prune destroyed the lock marker the next cycle needs: %v", err)
|
|
}
|
|
|
|
closeHandle()
|
|
|
|
removed, bytesFreed := PruneTaskTempDirs(base, 0, time.Now(), testLogger())
|
|
if removed != 1 {
|
|
t.Fatalf("prune removed %d dirs after the handle closed, want 1", removed)
|
|
}
|
|
if bytesFreed < 2048 {
|
|
t.Fatalf("prune reported %d bytes freed, want at least 2048", bytesFreed)
|
|
}
|
|
if _, err := os.Stat(dir); !os.IsNotExist(err) {
|
|
t.Fatalf("task temp dir still present after prune: %v", err)
|
|
}
|
|
}
|