Skip to content

Commit 236b8f6

Browse files
nullstyleclaude
andcommitted
test: isolate the OOM harness per iteration; re-land the ReleaseSafe promotion
aaabb6a promoted CI's ReleaseSafe lane to the full suite and b4873d8 backed it out, because `L4 Join proxy relay rolls back state under OOM injection` failed under ReleaseSafe on both amd64 tiers. Root-caused: THE ROLLBACK CODE WAS NEVER AT FAULT. All 34 injected failures rolled back cleanly, no leak, no swallowed OOM. The harness was unsound. `std.testing.checkAllAllocationFailures` shares ONE backing allocator across every fail-index iteration, coupling the runs through residual heap state. `FailingAllocator` bumps `alloc_index` only in `alloc`; `resize`/`remap` bump a separate counter it never fails. So a buffer that needs a fresh `alloc` on a cold heap can grow IN PLACE on a warm one -- one allocation disappears, the last fail index is never reached, and a deterministic function is reported as `NondeterministicMemoryUsage`. Measured on ubuntu-latest under ReleaseSafe, via a throwaway branch because the CI log printed no verdict: reference allocs = 35 ANOMALY fail_index=34 induced=false alloc_index=34 resize_index=6 anomalies = 1 Exactly one anomaly, at the LAST index only -- for every earlier index the injected failure fires before that allocation is reached. Six identical plain runs all allocated exactly 35, so the impl itself is deterministic. It is platform-specific because whether growth happens in place is an allocator and heap-layout decision: linux-aarch64 (verified in a container) and macOS never reproduce it. `harness.checkAllAllocationFailuresIsolated` gives every iteration a pristine DebugAllocator. That removes the coupling and keeps per-iteration leak detection; with a fresh heap each time an unreached fail index is a real signal rather than an artifact. The previous arrangement could silently under-test rollback paths on any platform where growth happens in place -- which is the more important half of this finding. With that fixed the promotion returns: CI runs `zig build test -Doptimize=ReleaseSafe`, green on all three tiers. A correction to the record while diagnosing this: I claimed an emulated amd64 container reproduced the failure. It did not -- that container died with `rosetta error: bss_size overflow`, i.e. Apple's x86 emulation failing to run the Zig compiler, and I read exit 133 as a reproduction. The architecture correlation is real, but it rests on the CI runs and the aarch64 container, not on that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent b4873d8 commit 236b8f6

4 files changed

Lines changed: 111 additions & 3 deletions

File tree

.github/workflows/ci.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -442,8 +442,8 @@ jobs:
442442
# fuzz-smoke and two transport files, so most of the RPC runtime -- peer,
443443
# caps, promises, vat, integration -- is still never executed with safety
444444
# checks on in an optimized build.
445-
- name: Run ReleaseSafe hardening tests
446-
run: zig build test-release-safe --summary all
445+
- name: Run the full suite under ReleaseSafe
446+
run: zig build test -Doptimize=ReleaseSafe --summary all
447447

448448
# ReleaseFast, not for speed but because it is the only mode that does NOT
449449
# poison a freed pointer. A use-after-free reached from a destructor is

CHANGELOG.md

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,57 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
104104
that can carry RPC today — see the Evented note above. The compile check is
105105
kept as a cheap cross-check that the evented selector still builds.
106106

107+
- **CI runs the full suite under ReleaseSafe**, not the ten-binary
108+
`test-release-safe` subset. That subset covered message, codegen, framing,
109+
fuzz-smoke and two transport files — leaving most of the RPC runtime (peer,
110+
caps, promises, vat, integration) never executed with safety checks on in an
111+
optimized build. Ablation-verified both ways: a deliberately failing assertion
112+
in `tests/rpc/peer/rpc_peer_test.zig` leaves `zig build test-release-safe`
113+
**green** and turns `zig build test -Doptimize=ReleaseSafe` red. The narrow
114+
step remains for a quick local pass and for the nightly job.
115+
116+
- **The OOM-injection harness was unsound, and the ReleaseSafe lane found it on
117+
its first run.** `L4 Join proxy relay rolls back state under OOM injection`
118+
failed under ReleaseSafe on both amd64 tiers while passing on macOS and
119+
linux-aarch64. The rollback code was never at fault — all 34 injected failures
120+
rolled back cleanly, with no leak and no swallowed OOM.
121+
122+
`std.testing.checkAllAllocationFailures` shares **one** backing allocator
123+
across every fail-index iteration, which couples the runs through residual
124+
heap state. `FailingAllocator` bumps `alloc_index` only in `alloc`, while
125+
`resize`/`remap` bump a separate counter it never fails — so a buffer that
126+
needs a fresh `alloc` on a cold heap can grow *in place* on a warm one. One
127+
allocation disappears, the last fail index is never reached, and a perfectly
128+
deterministic function is reported as `NondeterministicMemoryUsage`. Measured
129+
on ubuntu-latest: reference run 35 allocations; iteration 34 made 34
130+
allocations and 6 resizes with no failure induced — exactly one anomaly, at
131+
the last index only, because for every earlier index the injected failure
132+
fires first. It is platform-dependent because in-place growth is an
133+
allocator/heap-layout decision.
134+
135+
`harness.checkAllAllocationFailuresIsolated` gives each iteration a pristine
136+
`DebugAllocator`, which removes the coupling and keeps per-iteration leak
137+
detection. With a fresh heap every time, an unreached fail index becomes a
138+
real signal instead of an artifact. Worth stating plainly: the previous
139+
arrangement could silently under-test rollback paths on any platform where
140+
growth happens in place.
141+
142+
- **A lane that actually executes an `std.Io` backend selector.** The
143+
`evented-check` job ran `zig build -Dio-backend=evented check`, which verified
144+
nothing that plain `zig build check` did not already: `-Dio-backend` is a
145+
`[]const u8` compared at *runtime* by `io_backend.parseKind`, so all three
146+
arms of `Backend.init` are semantically analysed in every configuration.
147+
Ablation-proven — breaking the `.evented` arm turns plain `zig build check`
148+
red with no flag passed at all.
149+
150+
What was missing was any lane that *executes* a selector. `just
151+
check-selector` / `zig build -Dio-backend=threaded e2e-self` now runs the RPC
152+
e2e over an explicitly chosen backend, in CI and in `just ci`. Its own
153+
ablation: making `parseKind("threaded")` return null leaves the old compile
154+
check **green** and turns the new lane red. `.threaded` is the only selector
155+
that can carry RPC today — see the Evented note above. The compile check is
156+
kept as a cheap cross-check that the evented selector still builds.
157+
107158
### Known issues
108159

109160
- **`L4 Join proxy relay rolls back state under OOM injection` fails under

tests/rpc/peer/rpc_join_readiness_test.zig

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2857,7 +2857,12 @@ fn crossPeerJoinRelayOomImpl(allocator: std.mem.Allocator) !void {
28572857
}
28582858

28592859
test "L4 Join proxy relay rolls back state under OOM injection" {
2860-
try std.testing.checkAllAllocationFailures(std.testing.allocator, crossPeerJoinRelayOomImpl, .{});
2860+
// Isolated backing allocator per iteration. `std.testing.
2861+
// checkAllAllocationFailures` shares one across every fail index, and that
2862+
// coupling reported this deterministic impl as NondeterministicMemoryUsage
2863+
// under ReleaseSafe on both amd64 tiers while passing on macOS and
2864+
// linux-aarch64. See the helper for the measured evidence.
2865+
try harness.checkAllAllocationFailuresIsolated(crossPeerJoinRelayOomImpl, .{});
28612866
}
28622867

28632868
test "L4 JoinResult send failure drains pending direct Accept state" {

tests/rpc/peer/three_party_handoff_harness.zig

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,3 +78,55 @@ pub fn expectNoCrossPeerProxyLinks(peer: *const Peer) !void {
7878
try std.testing.expectEqual(@as(usize, 0), peer.cross_peer_proxy_links.items.len);
7979
try std.testing.expectEqual(@as(usize, 0), peer.cross_peer_join_relay_links.items.len);
8080
}
81+
82+
/// `std.testing.checkAllAllocationFailures`, but with a pristine backing
83+
/// allocator per iteration.
84+
///
85+
/// The std version shares ONE backing allocator across every fail-index
86+
/// iteration, which couples the runs together through residual heap state.
87+
/// That is enough to make it report `NondeterministicMemoryUsage` for code
88+
/// that is in fact perfectly deterministic: `FailingAllocator` bumps
89+
/// `alloc_index` only in `alloc`, while `resize`/`remap` bump a separate
90+
/// counter it never fails, so a buffer that needs a fresh `alloc` on a cold
91+
/// heap can grow in place on a warm one. One allocation disappears, the final
92+
/// fail index is never reached, and the run is reported as nondeterministic.
93+
///
94+
/// Measured on `crossPeerJoinRelayOomImpl` (ubuntu-latest, ReleaseSafe): the
95+
/// reference run made 35 allocations; iteration 34 made 34 allocations and 6
96+
/// resizes, with no failure induced. Every one of the 34 injected failures
97+
/// rolled back correctly — the rollback code was never the problem. macOS and
98+
/// linux-aarch64 did not reproduce it, because whether growth can happen in
99+
/// place is allocator- and platform-dependent.
100+
///
101+
/// Giving each iteration its own allocator removes the coupling, and keeps
102+
/// per-iteration leak detection.
103+
pub fn checkAllAllocationFailuresIsolated(
104+
comptime test_fn: anytype,
105+
extra_args: anytype,
106+
) !void {
107+
const needed = blk: {
108+
var dbg: std.heap.DebugAllocator(.{}) = .init;
109+
defer std.debug.assert(dbg.deinit() == .ok);
110+
var fa = std.testing.FailingAllocator.init(dbg.allocator(), .{});
111+
try @call(.auto, test_fn, .{fa.allocator()} ++ extra_args);
112+
break :blk fa.alloc_index;
113+
};
114+
115+
for (0..needed) |fail_index| {
116+
var dbg: std.heap.DebugAllocator(.{}) = .init;
117+
defer std.debug.assert(dbg.deinit() == .ok);
118+
var fa = std.testing.FailingAllocator.init(dbg.allocator(), .{ .fail_index = fail_index });
119+
if (@call(.auto, test_fn, .{fa.allocator()} ++ extra_args)) |_| {
120+
// A fresh heap every iteration means the alloc sequence is the
121+
// same one the reference run took, so an unreached fail index is
122+
// a real signal rather than an artifact.
123+
if (fa.has_induced_failure) return error.SwallowedOutOfMemoryError;
124+
return error.NondeterministicMemoryUsage;
125+
} else |err| switch (err) {
126+
error.OutOfMemory => {
127+
if (fa.allocated_bytes != fa.freed_bytes) return error.MemoryLeakDetected;
128+
},
129+
else => |e| return e,
130+
}
131+
}
132+
}

0 commit comments

Comments
 (0)