fix(ingestor): refuse a hard-linked stats tmp, and harden the #160/#161 tests (#228)

[dborup-agent]

Relates to #228

Also covers the three follow-ups from the round-2 re-review of #216 listed in the issue comment.

Changes

1. Hard-linked tmp is refused. writeStatsAtomic now checks the link count from the open descriptor's Fstat, after the type and owner checks and before the chmod/truncate. A tmp with nlink > 1 is closed, left in place, and reported with the same statsWriteError shape:
   <tmp>: hard-linked (nlink N); remove it.
   The link target keeps its content and mode, and nothing is renamed or published. fileLinkCount sits next to fileOwnerUID (stats_file_owner_unix.go). On Windows it reports no count, so the check is skipped there, the same way as the owner check.
2. FIFO tests can no longer hang.
   • The FIFO subtest of TestStatsWriteErrorNamesThePathOnce_160 now goes through writeStatsAtomicOrRelease.
   • TestStatsFileWriterFailureLineNamesThePathOnce_160 stops the writer through a new stopStatsWriterOrRelease, both in the test body and in its cleanup. The cleanup matters because the test can end early on t.Fatal.
   • stopStatsWriterOrRelease is the guard that TestStatsFileWriterStopsWithFIFOAtTmp_161 already had inline, moved into a helper and reused.
3. Rename failure is tested. A new rename failure case in TestStatsWriteErrorNamesThePathOnce_160 puts a non-empty directory at the stats path. The case checks that the *os.LinkError is stripped, so the error names the path only once.
4. DRY. The owner detail and hint are now built in one place, (*statsWriteError).setForeignOwner, which both setPermissionHint and checkStatsTmpOwner use.

There is no other behaviour change. The stats file's content, format, interval and path are unchanged.

Tests

• New: TestWriteStatsAtomicRefusesHardLinkedTmp_228 checks the exact error, that the link target's content and mode are unchanged, that nothing is published, and that the tmp is left in place.
• New subtests in TestStatsWriteErrorNamesThePathOnce_160: rename failure and hard link.
• Both hard-link tests fail on master's code (hard-linked tmp accepted).
• All mutants were run as a non-root user. The results table is in the report comment.

Perf

There is one extra comparison on a Stat_t that the writer already fetched, once per tick (1 Hz). It adds no syscall.

🤖 Generated with Claude Code

[dborup-agent]

Rapport — CS-pve-agent2 PR#240 #228 — head 2acd924

Status: All four acceptance points are done and verified by tests and mutants. Go Build & Test is green. Playwright E2E is red because of a failure that already exists on master (aff158c7), which this PR does not touch.

Evidence tags: [T] test run locally · [A] CI (GitHub Actions) · [K] code / diff inspection.

All local runs were done as a non-root user (uid 1000), with TMPDIR on tmpfs. Each mutant was applied to stats_file.go alone and then restored.

1. Hard-linked tmp is refused (#228)

Check	Test	Mutant	Result
A hard link at the tmp gives exactly <tmp>: hard-linked (nlink 2); remove it. The link target keeps its content and mode 0640. Nothing is published, and the tmp is left in place.	TestWriteStatsAtomicRefusesHardLinkedTmp_228	—	Red on master's code, at the test-only commit 06f2a62d (hard-linked tmp accepted). Green at head. [T]
The error names the path once and has the statsWriteError shape	TestStatsWriteErrorNamesThePathOnce_160/hard_link	—	Red at 06f2a62d, green at head. [T]
The check is needed	both tests above	M1a nlink check disabled	Killed: both tests fail with accepted. [T]
The check runs before chmod and truncate	…_228	M1b check moved to after the truncate	Killed: link target changed: "" and mode changed: -rw-------. [T]
The tmp is left in place	…_228	M1c os.Remove(tmp) on refusal	Killed: hard-linked tmp not left in place. [T]

On Windows, fileLinkCount reports no count, so the check is skipped there, like the owner check. Cross-builds pass: GOOS=windows go build, GOOS=freebsd go build and GOOS=darwin go vet. [T][K]

2. FIFO tests fail fast instead of hanging (#161 guard)

Check	Test	Mutant	Result
The FIFO subtest goes through writeStatsAtomicOrRelease	TestStatsWriteErrorNamesThePathOnce_160/FIFO_without_a_reader	M2 O_NONBLOCK dropped from the open	Killed in 2.0s: writeStatsAtomic blocked on a FIFO at the tmp path. [T]
The writer stop is guarded, in the test body and in the cleanup	TestStatsFileWriterFailureLineNamesThePathOnce_160	M2	Killed in 6.3s: no failure line, then stop hung: the writer is blocked on the FIFO. [T]
The shared stopStatsWriterOrRelease still guards the original test	TestStatsFileWriterStopsWithFIFOAtTmp_161	M2	Killed in 3.4s. [T]
Contrast: master's versions of these tests	the same two tests from origin/master	M2	Both hang until panic: test timed out after 30s. [T]

3. Rename failure path

Check	Test	Mutant	Result
A non-empty directory at the stats path makes the rename fail. The error is <tmp>: rename to stats.json: file exists, with the path named once.	TestStatsWriteErrorNamesThePathOnce_160/rename_failure	M3 *os.LinkError not stripped	Killed: path named 3 times. [T]
Contrast: master's version of the test	TestStatsWriteErrorNamesThePathOnce_160 from origin/master	M3	Survives (ok). [T]

4. DRY: one owner helper

Check	Test	Mutant	Result
setPermissionHint and checkStatsTmpOwner both use (*statsWriteError).setForeignOwner	—	—	[K]
One hint string, used by both paths	…NamesThePathOnce_160/foreign_owner and /foreign_owner,_unopenable	M4a hint changed in the helper	Killed: both the openable and the unopenable path fail. [T]
One detail string, used by both paths	TestWriteStatsAtomicUnopenableForeignTmpNamesTheFix_160	M4b detail emptied in the helper	Killed: lacks "uid 1000". [T]

Requirements

Requirement	Result
No behaviour change other than point 1. The stats content, format, interval and path are unchanged.	[K] The only production change outside the new check is the helper extraction. The output strings are identical.
go test -race -count=1 -timeout 20m ./... in cmd/ingestor	ok in 772s. [T]
Affected tests with -race -count=5 (45 tests matching _160|_161|_228|_118|Symlink|StatsFile|WriteStats)	ok. [T]
go vet ./...	Clean. [T]
gofmt -l on the touched files	Empty. Other files that are unformatted on master are untouched. [T]
Fork guards	Unchanged: 9 in deploy.yml, 1 in release-fast-path.yml. No .github/ changes. [K]
New map[string]interface{}	0. [K]
Commits	06f2a62d adds the tests and is red on master's code. 2acd9249 is the fix. [T][K]

CI per job (run for 2acd9249)

Job	Result
✅ Go Build & Test	success [A]
🎭 Playwright E2E Tests	failure [A]: 2 steps in test-issue-1122-details-row-clamp-e2e.js, [desktop-1200] and [tablet-900]: advert links in Details stay visible and clickable: advert link in Details is not hit-testable.
📦 Release Artifacts, 🏗️ Docker, 🚀 Deploy Staging, 📝 Badges	skipped (depends on E2E, or not on a PR) [A]

The Playwright failure already exists on master and is not caused by this PR:

• The master run for aff158c7 fails with the same 2 steps and the same output. [A]
• The previous master run, 341a1961, passed Playwright. [A]
• An unrelated open PR branch that is based on aff158c7 fails the same way. [A]
• This PR changes only Go files under cmd/ingestor/. [K]

Remaining

• The master Playwright regression in test-issue-1122-details-row-clamp-e2e.js, which started between 341a1961 and aff158c7, needs its own issue. It is not addressed here.
• The hard-link check runs after the owner check. A hard link to another user's file is therefore still reported as owned by uid … (as on master), not as hard-linked. Either way it is refused before anything changes.
• No browser validation: this change is backend-only and not visible in the UI.

[dborup-agent]

Review — CS-pve-agent1 PR#240 stats-hardlink — head 2acd924

Dom: APPROVE med nits. The hard-link refusal is correct, fd-based and TOCTOU-free for the target. The #216 follow-ups do what they claim. The nits below are optional and none of them blocks.

Evidence legend: [T] = test or reproduction I ran myself · [A] = my analysis of code, diffs or CI logs · [K] = known from the issue, the PR or the author's report, not re-verified.

Read-only review: git archive of head and of the merged tree (git merge-tree --write-tree origin/master 2acd9249, master 3878d7ea) into a scratch directory. No worktree and no push. git ls-remote showed head 2acd9249 and master 3878d7ea both before and after the review.

Findings

#	Severity	Finding	Evidence
1	nit (optional)	The check correctly uses the descriptor's Fstat (fi from f.Stat()), but no test pins that. My mutant R6 replaces it with a name-based os.Lstat(tmp) (a TOCTOU-prone variant), and it survives. A deterministic test is hard to write without a hook, so a comment next to the check ("must use the fd's stat, not the name") would be enough.	[T] R6 survives; [A]
2	nit (optional)	No test checks that the refusal path closes the fd. Mutant R7 drops the f.Close() and survives. At 1 Hz that would leak one fd per second for as long as the link stays in place. The code is correct today. A cheap guard is to count /proc/self/fd (Linux) across, say, 50 refusals. The same applies to the other refusal branches, which predate this PR.	[T] R7 survives; [A]
3	info, pre-existing	os.Rename(tmp, path) acts on the name after f.Close(). In a stats directory that is writable by others and not sticky, an attacker could swap the tmp between close and rename and get a different inode published at the stats path. The target is not modified, only published. With the default /tmp (sticky), another user cannot replace the ingestor's own tmp. This is unchanged from master and out of scope for #228.	[A]
4	info, report accuracy	The author's report calls the test-issue-1122-details-row-clamp-e2e.js failure a master regression "between 341a1961 and aff158c7". It is intermittent: in another PR's rerun on an aff158c7-based tree, all three "advert links in Details stay visible and clickable" steps passed (CI job 111662539602). It is unrelated to this PR either way (see CI below).	[A]

Point 1 — Hard link

• Refused after open via Fstat, the target is unchanged and nothing is published. The check runs on fi from f.Stat() on the opened descriptor. It comes after the type and owner checks and before f.Chmod, f.Truncate, f.Write and os.Rename. On refusal the fd is closed and the tmp is left in place. [A]
• No TOCTOU window for the target. Everything after the check (fchmod, ftruncate, write) goes through the same fd whose inode was checked. Swapping the tmp name after the open cannot redirect these writes to another inode. A link added to our own inode after the check would only give a second name to the ingestor's own file, so there is no victim. The remaining name-based step (the rename) is pre-existing; see finding 3. [A]
• Error shape. newStatsWriteError(tmp, "", nil) with detail hard-linked (nlink N) and hint remove it gives <tmp>: hard-linked (nlink 2); remove it. That matches the statsWriteError format of setNotRegular, and the path is named once. [A][T]
• Real reproduction with the actual ingestor binary, as a normal user (uid 1000), fs.protected_hardlinks=1. I built cmd/ingestor from master and from head. Each ran for 5 s against a minimal MQTT broker, with CORESCOPE_INGESTOR_STATS=<dir>/stats.json, a victim file precious ("precious content", mode 0640) and ln precious stats.json.tmp. [T]

Binary	Victim after the run	stats.json	stats.json.tmp	Log
master 3878d7ea	clobbered: content replaced by the stats JSON (16 → 2851 bytes), mode 640 → 600, nlink 2 → 1 (its inode was published at stats.json and then replaced on the next tick)	present	absent	no stats error
head 2acd9249	unchanged: same inode, content, mode 640, nlink 2	absent	left in place (the victim's inode)	[stats-file] write failed: <dir>/stats.json.tmp: hard-linked (nlink 2); remove it

Point 2 — FIFO tests with a release guard

With O_NONBLOCK dropped from the open (mutant R4), the -run '_160|_161' set, timeout 90 s: [T]

Tests	Result
head's tests	fail fast, whole run 15.5 s. FIFO without a reader subtest: writeStatsAtomic blocked on a FIFO at the tmp path (2.0 s). TestStatsFileWriterFailureLineNamesThePathOnce_160: no failure line + stop hung: … (6.0 s). …FailsFast_161 (2.0 s) and …StopsWithFIFOAtTmp_161 (3.1 s) also fail.
master's tests	hang: panic: test timed out after 1m30s in TestStatsWriteErrorNamesThePathOnce_160/FIFO_without_a_reader. Run alone, master's TestStatsFileWriterFailureLineNamesThePathOnce_160 also hangs until a 45 s timeout.

The stop function is called twice in that test (body and cleanup). This is safe because stop is sync.Once-guarded and waits on a closed done. [A]

Point 3 — Rename path

Mutant R3 keeps the *os.LinkError (it no longer strips le.Err). The new rename failure subtest kills it: path named 3 times in "…/stats.json.tmp: rename to stats.json: rename …/stats.json.tmp …/stats.json: file exists", want once. [T] On master's code the subtest passes, as expected: it adds coverage and does not fix anything. [T]

Point 4 — DRY

setPermissionHint and checkStatsTmpOwner now share (*statsWriteError).setForeignOwner, which carries one detail string and one hint string. [A]

Behaviour is unchanged. Master's versions of stats_file_errtext_160_unix_test.go and stats_file_fifo_161_unix_test.go, run against head's production code, pass on the full stats set. [T]

Mutant R5 makes setPermissionHint ignore the helper's result and fall through to the mode hint. It is killed by …/foreign_owner,_unopenable and by TestWriteStatsAtomicUnopenableForeignTmpNamesTheFix_160. [T]

Point 5 — No other behaviour change

The production diff consists of the new check, the helper extraction, and fileLinkCount (Unix: Stat_t.Nlink; Windows: 0, false). Nothing changes in the JSON encoding, the snapshot fields, the tick interval (StartStatsFileWriter(store, time.Second)), statsFilePath(), the open flags, the mode (0o600) or the rename target. [A]

Head's tests run against master's code fail only the two hard-link cases (…/hard_link: accepted; …_228: hard-linked tmp accepted). Everything else passes, including rename failure. [T]

Point 6 — Rules

• Files: only cmd/ingestor/ (6 files; 0 outside). [A]
• New map[string]interface{}: 0. [A]
• Fork guards in the merged tree: deploy.yml 9, release-fast-path.yml 1. [T]
• Closing keywords in the title, body or commits: none. [T]
• Both commits (06f2a62d tests, 2acd9249 fix): author and committer are dborup <kontakt@meshview.dk>. [T]
• Draft, open. [A]

Tests

• Merged tree (3878d7ea + PR): cd cmd/ingestor && TMPDIR=<tmpfs> go test -race -count=1 -timeout 20m ./... gives ok, 748 s (774 s wall), no DATA RACE, no FAIL. [T]
• Head, stats set (Stats|_160|_161|_228|_118|Symlink|WriteStats, 51 top-level tests): green. [T]
• go vet (linux, darwin): clean. GOOS=windows go build: ok. gofmt -l on the 6 touched files: empty. [T]

Mutants (mine, each on a copy of head's tree; stats set unless noted)

Mutant	Change	Result
R1	n > 1 → n > 2	killed: …/hard_link: accepted, …_228: hard-linked tmp accepted [T]
R2	fileLinkCount (Unix) returns ok=false	killed: same two tests [T]
R3	*os.LinkError not stripped	killed: rename failure, path named 3 times [T]
R4	O_NONBLOCK dropped	killed fast on head (15.5 s); hangs to timeout with master's tests [T]
R5	setPermissionHint ignores setForeignOwner's result	killed: 2 tests [T]
R6	check via name-based os.Lstat(tmp) instead of the fd's Fstat	survives (finding 1) [T]
R7	no f.Close() on the hard-link refusal	survives (finding 2) [T]

CI

• The run for 2acd9249 (37277054612) has ✅ Go Build & Test: success, and 🎭 Playwright E2E Tests: failure. [A]
• The failure is exactly 2 steps in test-issue-1122-details-row-clamp-e2e.js, [desktop-1200] and [tablet-900]: advert links in Details stay visible and clickable: advert link in Details is not hit-testable: {"found":true,"hitIsLink":false,"text":"R5-D4 300D Rak"}. [A]
• Master aff158c7 (run 37274272380, attempt 1) fails with the identical two steps and output. That run's attempt 2 was still in progress when I checked. [A]
• The failure is a frontend hit-test on the packets Details pane. This PR changes only Go files in cmd/ingestor/, so it is outside this PR. [A]
• The other jobs were skipped. [A]

Not verified

• I did not run the Playwright suite or the bug(packets): filter UX disaster — help panel overlaps table, toolbar chaotic, path chips spill rows Kpa-clawbot/CoreScope#1122 E2E myself. The CI conclusion rests on the logs. [A]
• No real Windows/macOS/FreeBSD run: Windows only cross-builds, and darwin is only vetted. [T]
• No test with a hard-linked tmp and a foreign owner. By reading, it is reported as the owner error and refused before any change, as the author notes. [A]
• I did not run the author's own mutants M1b, M1c, M4a or M4b. [K]
• No root run. All runs were as uid 1000.

[dborup]

Merged as e0bfe955 with one red CI check. Go Build & Test and the Docker build were green. The only failing step was the Playwright Details-clamp test, "advert links in Details stay visible and clickable" (#244). That test is time-dependent and fails the same way on master. This PR changes only cmd/ingestor and cannot affect that frontend check.