Skip to content

Commit a37613d

Browse files
committed
fix(writer): atomic rename in updateUnit prevents hard-link corruption
updateUnit() previously used os.WriteFile(filePath) which writes directly to the file's inode. When filePath is a hard link (e.g. a test fixture isolation strategy on same-filesystem setups), this overwrites the shared inode and silently corrupts the source fixture — a critical latent bug on single-partition CI environments where /tmp and testdata share a device. Fix: write to a sibling .tmp file then os.Rename() into place. Rename replaces the directory entry atomically, leaving the linked inode intact. This is the same pattern already used by WriteTransaction.WriteUnit(). Also: - Merge testopen_linux_test.go + testopen_other_test.go → testopen_test.go (both were functionally identical; dead platform split adds maintenance overhead with no current benefit) - Fix waitForAlive() comment: the daemon now binds the socket BEFORE building the index (lazy init), so IsAlive() returns in ~20 ms, not 5 s - Add TEST_TIMEOUT (default 180s) to make test and pre-commit hook so the suite fails fast on hangs; add -p flag to cap concurrent packages Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent 0c20f6d commit a37613d

6 files changed

Lines changed: 31 additions & 46 deletions

File tree

.githooks/checks/02-unit-tests.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ LOG_FILE="$(git rev-parse --show-toplevel)/.test-fail.log"
44
rm -f "$LOG_FILE"
55

66
echo "pre-commit: running unit tests..."
7-
if ! CGO_ENABLED=0 go test -timeout 120s -parallel "$(nproc)" ./... > "$LOG_FILE" 2>&1; then
7+
if ! CGO_ENABLED=0 go test -timeout 180s -p "$(nproc)" -parallel "$(nproc)" ./... > "$LOG_FILE" 2>&1; then
88
echo "" >&2
99
echo "COMMIT BLOCKED: unit tests failed." >&2
1010
echo "" >&2

Makefile

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,14 @@ DAEMON_NAME = mxcli-daemon
3434
# Clean version for VS Code extension (must be valid semver: major.minor.patch)
3535
VSCE_VERSION = $(shell echo "$(VERSION)" | sed 's/^v//; s/-.*//' | grep -E '^[0-9]+\.[0-9]+\.[0-9]+$$' || echo "0.0.0")
3636

37-
# Max parallel test packages (lower = less memory; override with: make report TEST_P=8)
38-
TEST_P ?= 4
37+
# Max parallel test packages (lower = less memory; override with: make test TEST_P=8)
38+
TEST_P ?= $(shell nproc)
3939
# Max concurrent tests within a package (default: GOMAXPROCS). Lower values
4040
# reduce peak memory and I/O pressure. Override: make test TEST_PARALLEL=4
4141
TEST_PARALLEL ?= $(shell nproc)
42+
# Hard ceiling on how long the full test suite may run. Fail fast rather
43+
# than hang CI. Override: make test TEST_TIMEOUT=300s
44+
TEST_TIMEOUT ?= 180s
4245

4346
.PHONY: build build-debug release clean test test-mdl report report-bench report-reset-baseline grammar sync-skills sync-commands sync-lint-rules sync-changelog sync-examples sync-all docs documentation docs-site docs-serve source-tree sbom sbom-report lint lint-go fmt vet update-helpdesk-golden test-helpdesk-regression setup
4447

@@ -167,9 +170,10 @@ release: clean sync-all
167170
@echo " Launchers: mxcli-{os}-{arch}"
168171
@echo " Daemons: mxcli-daemon-{os}-{arch}.tar.zst (or .zip)"
169172

170-
# Run tests
173+
# Run tests. TEST_TIMEOUT guards against hangs; TEST_P caps concurrent packages
174+
# to prevent I/O storms; TEST_PARALLEL caps concurrent tests within a package.
171175
test:
172-
CGO_ENABLED=0 go test -parallel $(TEST_PARALLEL) ./...
176+
CGO_ENABLED=0 go test -timeout $(TEST_TIMEOUT) -p $(TEST_P) -parallel $(TEST_PARALLEL) ./...
173177

174178
# Run full test suite and generate layered report (terminal + HTML)
175179
# Output: coverage/report.html, coverage/bench-baseline.json

internal/expr/daemon/daemon_test.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,11 @@ import (
1616
"github.com/stretchr/testify/require"
1717
)
1818

19-
// waitForAlive polls the socket until it is alive or a 6-second deadline
20-
// expires. 6 s covers the corpus-a index build (~5 s) plus headroom.
19+
// waitForAlive polls until the socket is alive or the deadline expires.
20+
// With the lazy-init design, Serve() binds the socket FIRST and builds the
21+
// index afterwards, so IsAlive() returns true within ~20 ms of the goroutine
22+
// being scheduled. The 6-second deadline is a safety net for cases where
23+
// Serve() fails to bind at all (returns error before binding).
2124
func waitForAlive(t *testing.T, sockPath string) {
2225
t.Helper()
2326
deadline := time.Now().Add(6 * time.Second)

mdl/executor/testopen_other_test.go

Lines changed: 0 additions & 30 deletions
This file was deleted.
Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
// SPDX-License-Identifier: Apache-2.0
22

3-
//go:build linux
4-
53
package executor
64

75
import (
@@ -15,10 +13,12 @@ const fixtureMprPath = "../../testdata/expr-checker/minimal.mpr"
1513

1614
var openFixtureSem = make(chan struct{}, runtime.GOMAXPROCS(0))
1715

18-
// openMprWriterForTest copies (or hard-links) the fixture into a per-test
19-
// temp dir and returns an mmpr.Writer. parallelOnce() ensures t.Parallel()
20-
// fires exactly once even when this helper is invoked multiple times.
21-
// openFixtureSem throttles concurrent I/O to GOMAXPROCS.
16+
// openMprWriterForTest copies the fixture into a per-test temp dir
17+
// (hard-linking mprcontents/ when source and /tmp share a filesystem)
18+
// and returns an mmpr.Writer. parallelOnce() ensures t.Parallel() fires
19+
// exactly once even when this helper is invoked multiple times in the
20+
// same test (e.g. via helper chains). openFixtureSem throttles concurrent
21+
// I/O to GOMAXPROCS to prevent I/O storms.
2222
func openMprWriterForTest(t *testing.T) *mmpr.Writer {
2323
t.Helper()
2424
parallelOnce(t)

modelsdk/mpr/writer_core.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -477,9 +477,17 @@ func (w *Writer) updateUnit(unitID string, contents []byte) error {
477477
swappedUUID+".mxunit",
478478
)
479479

480-
// Write updated content
481-
if err := os.WriteFile(filePath, contents, 0644); err != nil {
482-
return fmt.Errorf("failed to write unit file: %w", err)
480+
// Write to a temp file then atomically rename into place.
481+
// Direct WriteFile would overwrite the shared inode when the target
482+
// is a hard link (e.g. in test fixtures), corrupting the source file.
483+
// Rename replaces the directory entry, leaving the linked inode intact.
484+
tmpPath := filePath + ".tmp"
485+
if err := os.WriteFile(tmpPath, contents, 0644); err != nil {
486+
return fmt.Errorf("failed to write unit temp file: %w", err)
487+
}
488+
if err := os.Rename(tmpPath, filePath); err != nil {
489+
os.Remove(tmpPath)
490+
return fmt.Errorf("failed to rename unit file: %w", err)
483491
}
484492

485493
// Update ContentsHash in database

0 commit comments

Comments
 (0)