[KVladislav]

With cgroup v2, conmon watches memory.events with inotify and re-reads the oom and oom_kill counters on every IN_MODIFY. check_cgroup2_oom() returns G_SOURCE_REMOVE whenever a read creates no new OOM marker, so the first notification that is not an OOM ends OOM monitoring for the rest of the container's life. A later OOM is then caught only by the exit-time check_cgroup2_oom() call in conmon.c, and that read sees nothing once the cgroup has already been removed (ENOENT). CRI-O then reports Error/137 instead of OOMKilled/137.

This is the normal sequence, not a corner case. The kernel raises MEMCG_MAX and notifies memory.events from try_charge() every time the cgroup hits memory.max, before it reclaims and long before any OOM, and coalesces notifications only within 10 ms (CGROUP_FILE_NOTIFY_MIN_INTV). The first notification after a limit hit is therefore almost always max-only with oom 0 and oom_kill 0, and the watch is gone before the OOM happens.

In the traced run, max increased while oom and oom_kill stayed at zero and conmon dropped the watch on that read; the same cgroup later reached oom 1, oom_kill 3 with a matching kernel OOM message, and conmon's read at container exit found memory.events already gone (ENOENT).

The watch has been dropped on such a read since #371 added if (counter == 0) continue;, which lets a read with both counters at zero fall through to G_SOURCE_REMOVE. #581 (for #426) made it stricter: oom_detected ? G_SOURCE_CONTINUE : G_SOURCE_REMOVE drops the watch even on the first notification after an OOM that was already recorded.

Fix: return G_SOURCE_CONTINUE after any successful read of memory.events. The watch is still removed when the file is gone (ENOENT) and on a non-recoverable read error on the inotify fd, as before. The cost is one open/read/close of memory.events per notification, which the kernel caps at 100 per second.

Test: test/05-oom-watch.bats builds test/oom_watch.c, which #includes the production src/cgroup.c (the callback is static) and defines the four cli.c globals it uses, then drives oom_cb_cgroup_v2() through a GLib main context with a pipe in place of the inotify fd and a temporary memory.events. A real-container OOM cannot be forced deterministically in CI. Eight cases: without the fix max-only, max-then-oom, max-burst, unchanged-positive and marker-retry fail and oom-first, kill-only and removed-file pass; with the fix all eight pass (Ubuntu 24.04, gcc 13.3, GLib 2.80, Bats 1.10). The existing test/05-oom-detection.bats still passes.

[jnovy]

Nice fix - minimal, correct, and well-tested. The root cause analysis is spot on: the kernel notifies memory.events on every memory.max hit (MEMCG_MAX) before reclaim/OOM, so the first notification is almost always max-only with oom=0, oom_kill=0, and the old code dropped the watch.

I built the branch and ran all 8 test cases - they pass with the fix and 5 fail without it, exactly as described.

Two pre-existing issues worth noting (not introduced by this PR, not blocking):

1. oom_cb_cgroup_v2() at line 196: read(fd, &events, ...) executes unconditionally before the G_IO_IN check at line 206. If condition is G_IO_ERR/G_IO_HUP without G_IO_IN, the read consumes queued inotify events but check_cgroup2_oom() is never called - the source is silently removed.

2. Same line: &events has type char (*)[N] rather than char *. The address is identical so it works, but events (without &) would be cleaner.

[jnovy]

Nit (non-blocking): with this change, if memory.events becomes permanently unreadable for a reason other than ENOENT (e.g. EACCES from a security policy change), the watch now returns G_SOURCE_CONTINUE indefinitely, logging a warning on every inotify notification. The old code would have stopped after one such event.

In practice this is unlikely (cgroup files don't normally change permissions mid-life), but a consecutive-error counter with a small threshold (e.g. 5) would be a cheap safety net:

static int consecutive_open_failures = 0;
// ... on open failure:
if (++consecutive_open_failures > 5)
    return G_SOURCE_REMOVE;
return G_SOURCE_CONTINUE;
// ... on success:
consecutive_open_failures = 0;

[KVladislav]

The base revision already returns G_SOURCE_CONTINUE when fopen() fails with an error other than ENOENT. That return happens before the code changed by this PR, so the behavior for EACCES stays the same.

A retry limit would introduce a separate policy for when monitoring should stop. I’d leave that for a follow-up.

[jnovy]

Suggestion (non-blocking): consider adding -fsanitize=address,undefined to the compile flags here. It's a free safety net that would catch use-after-free, buffer overflows, and UB in the test binary.

Also, cgroup.c starts with #define _GNU_SOURCE so -std=gnu99 (or dropping -std=c99 entirely) would better match production semantics. It compiles fine as-is since glib headers pull in the needed extensions, but if new code in cgroup.c ever uses GNU-specific features directly, this test would be the first to break.

[KVladislav]

A sanitizer run would be useful. I’d make it optional so running the regression tests doesn’t require sanitizer support in every build environment.

For the language standard, the production Makefile already uses -std=c99, so the test currently matches it. I’d keep them consistent.

[jnovy]

The test harness is well-designed - pipe-based inotify simulation is clever, alarm(5) as a hang guard is good practice, and the 8 cases cover the important scenarios.

One area that could catch undiscovered crashes: malformed memory.events content. The strtol + strncmp parsing in check_cgroup2_oom() doesn't validate input defensively, so test cases like these would add confidence:

• Empty file (0 bytes)
• Missing fields (e.g. only low 0\nhigh 0\n, no oom line)
• Non-numeric values (oom abc\n)
• Truncated lines (oom\n with no value)
• Very large counter values near LONG_MAX

Non-blocking - the current coverage is solid for the fix being made.

[KVladislav]

Those would be useful additions. The reader already checks line lengths and strtol() conversion errors, but this harness doesn’t exercise those paths.

I’d keep this PR focused on the watch lifetime and cover malformed input in separate parser tests. The key regression here is that processing a notification without an OOM must leave the watch active for a later OOM.

[KVladislav]

Thanks for building the branch and checking both versions.

The read-before-G_IO_IN handling deserves a separate look at the error and hangup paths. Using events instead of &events would also make the code clearer. Both predate this change, so I’d keep that callback cleanup in a separate patch.

[jnovy]

LGTM. The fix is minimal, correct, and well-tested. The root cause analysis is excellent - the kernel's MEMCG_MAX notification arriving before the actual OOM is the normal sequence, and the old code (G_SOURCE_REMOVE on non-OOM read) was fundamentally wrong. Always returning G_SOURCE_CONTINUE from check_cgroup2_oom() is the right fix, with negligible cost (one open/read/close of memory.events per notification, kernel-capped at ~100/s). The test harness is clever - #including production src/cgroup.c to test the static callback directly, using a pipe to simulate inotify deterministically. The two pre-existing issues (unconditional read before G_IO_IN check, &events vs events) should be addressed in a separate follow-up patch.

[KVladislav]

Thanks for merging this, @jnovy! Is there an approximate timeline for the next conmon release that will include this fix?