Skip to content

fix: detect systemd units whose start event sees /init.scope - #374

Merged
blue4209211 merged 2 commits into
mainfrom
fix/systemd-unit-detection
Oct 8, 2026
Merged

blue4209211 merged 2 commits into
mainfrom
fix/systemd-unit-detection

Conversation

@mayankpande88

Copy link
Copy Markdown
Contributor

Summary

New systemd units were sometimes never detected. On a warm agent, 4 of 10 short-lived systemd-run units never appeared. A unit that does nothing after it starts then stays invisible for good.

Cause. systemd forks a unit's process inside its own /init.scope and moves it to the unit's cgroup just before exec. The registry was meant not to cache such a pid as ignored, through the check cg.Id == "/init.scope". But cgroup parsing skips /init.scope and the root cgroup, so for those the Id is "" and that check never matched. When the agent handled the start event before systemd moved the process, it cached the pid as ignored for 15s. It then dropped the exec, the one event a sleeping service produces. #366, which handles process events as soon as they arrive, made that ordering common.

Fix. The check now matches the empty Id, which is what /init.scope actually parses to. pid 1 stays cached as before. A new test in cgroup pins that /init.scope and / parse to an empty Id, so the condition can't go dead again.

The same dead check exists upstream (coroot-node-agent containers/registry.go, since its cgroup parsing started skipping /init.scope in #203).

Engineering detail

How it was found: while testing #371, a few new units never showed up in either build. A diagnostic build that logged every process start and exec event with the cgroup read at handling time showed the missed units:

  • the start event saw an empty cgroup and logged "ignoring";
  • the exec event about 20ms later carried the unit's cgroup, but the pid was in the ignore cache.

With the fix: the same build logged "ignoring without persisting" at start, and detected the unit at exec.

Cost: pids in /init.scope or the root cgroup other than pid 1 are no longer cached as ignored, so each of their events re-reads /proc/<pid>/cgroup. In practice these are systemd's freshly forked children, which move to their unit almost immediately, and kernel threads, which produce no events the registry handles.

CI: gofmt, goimports, vet, golangci-lint, go test (excluding /containers, including the new cgroup test) and the build all pass in a Linux container with Go 1.26.5.

Local e2e: I built agent binaries from this branch and from main and ran them side by side as systemd services on a local Debian 12 VM (kernel 6.1, systemd 252).

  • Start-up bursts: in two rounds of starting 10 units at once, 25s after the agents started, this branch detected 10/10 both times and main detected 9/10 both times.
  • Warm agents: with 10 transient systemd-run units started 3s apart, this branch detected 10/10 and main 6/10.
  • Logs: neither agent logged an error.

systemd forks a unit's process inside its own /init.scope and moves it
to the unit's cgroup before exec. The registry meant not to cache such a
pid as ignored (cg.Id == "/init.scope"), but cgroup parsing skips
/init.scope and the root cgroup, so their Id is "" and the check never
matched. When the start event was handled before the move, the pid was
cached as ignored for 15s, its exec was dropped, and a unit that did
nothing else was never detected. Handling proc events on wakeup (#366)
made that ordering common: 2 of 6 transient units on a warm agent.

Match the empty Id instead, and pin the parsing it relies on in a test.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the container registry to properly handle processes in systemd's /init.scope or the root cgroup, which parse to an empty ID, preventing them from being incorrectly cached as ignored. A new unit test is added to verify this behavior. Feedback suggests using require.Nil instead of assert.Nil in the test to avoid potential nil pointer dereferences if the setup or parsing fails.

Comment thread cgroup/cgroup_test.go Outdated
@blue4209211

Copy link
Copy Markdown
Contributor

Good catch: cgroup.go skips both / and /init.scope, so the old cg.Id == "/init.scope" check could never match. systemd moves the process before exec, and the exec event fires after it, so the cgroup read at exec time already shows the unit. Not caching the start-time miss is enough. The new test pins the empty-Id parse. go test ./cgroup/ passes locally. LGTM.

Two small things for a later pass, neither blocking:

  1. Cost on hosts without systemd. The cost note assumes empty-Id processes are only systemd's freshly forked children and kernel threads. On hosts not running systemd (OpenRC/Alpine VMs, which standalone mode from feat: run without Kubernetes on standalone hosts #341 now covers), host daemons can sit in the root cgroup too. None of their pids are cached any more, so each of their connect, listen and file-open events re-reads /proc/<pid>/cgroup. That's cheap per event but unbounded for a busy daemon. Two options:
    • keep "don't cache" for /init.scope only, for example by recording on Cgroup that /init.scope was skipped while parsing;
    • cache empty-Id pids with a short TTL (1–2 s) instead of not at all, which still catches the exec ~20 ms later.
  2. The inline cleanup in getOrCreateContainer does nothing (predates this PR, ~registry.go:654). It sets containersByPidIgnored[pid] to now and then checks that same entry against IgnoredContainersCacheTTL, so it never deletes anything. It's harmless, since the periodic sweep resets the map, and can be removed.

@blue4209211
blue4209211 merged commit a131bff into main Oct 8, 2026
7 checks passed
@blue4209211
blue4209211 deleted the fix/systemd-unit-detection branch October 8, 2026 07:43
@mayankpande88

Copy link
Copy Markdown
Contributor Author

Thanks. Both follow-ups are in #376.

  • Root-cgroup caching: Cgroup now records whether the process is in /init.scope, and only those pids skip the ignore cache. Root-cgroup daemons are cached as before.
  • Why not a short TTL: systemd's exec arrives about 20ms after the fork, so even a 1–2s TTL would still hit the cached entry and bring the bug back.
  • Dead cleanup: the inline cleanup in getOrCreateContainer is removed.
  • Checked on a VM: a root-cgroup process doing about 20 requests/s was re-evaluated 38 times in 30s on main and once with fix: skip the ignore cache only for /init.scope (follow-up to #374) #376. New-unit detection stayed at 10/10.

mayankpande88 added a commit that referenced this pull request Oct 8, 2026
…s skipped

#374 stopped caching every pid whose cgroup Id is empty, which covers
the root cgroup as well as /init.scope. On hosts without systemd,
daemons can run in the root cgroup, and each of their connect, listen
and file-open events then re-read /proc/<pid>/cgroup. Cgroup now records
whether the process is in /init.scope, and only those pids skip the
ignore cache. Also drop the inline cleanup in getOrCreateContainer: it
checked the entry it had just written, so it never deleted anything.
mayankpande88 added a commit that referenced this pull request Oct 8, 2026
…s skipped (#376)

#374 stopped caching every pid whose cgroup Id is empty, which covers
the root cgroup as well as /init.scope. On hosts without systemd,
daemons can run in the root cgroup, and each of their connect, listen
and file-open events then re-read /proc/<pid>/cgroup. Cgroup now records
whether the process is in /init.scope, and only those pids skip the
ignore cache. Also drop the inline cleanup in getOrCreateContainer: it
checked the entry it had just written, so it never deleted anything.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants