Skip to content

Commit 985c579

Browse files
committed
fix: io.zig trace state not thread-local; deterministic ghost trace events from std bootstrap; wire engine-api-test into CI
Three follow-ups to the engine C-API race fix: 1. io.zig's write/read overrides and trace state (write_override, werr_override, read_override, g_trace_fn, g_trace_userdata, g_trace_handle, g_trace_prev_line) had the same non-thread-local pattern as engine.zig/host_abi.zig's already-fixed fields. Made all threadlocal. 2. The pre-existing "engine_set_trace_fn fires per source line" failure was assumed to be order-dependent test pollution, but a debug print showed the exact same {1, 7, 1, 2, 3} line sequence on every run -- fully deterministic, since engine.zig's native test runner (tools/standalone_runner.zig) runs tests in fixed file order with no shuffling. Root cause: vm.zig's dispatchTick fired a trace event for every instruction while active, including the embedded std-library bootstrap bytecode every program executes at startup (defining std.array.count and friends as globals) regardless of whether the script references std. The ghost 1 and 7 were two instructions from that bootstrap's own function definition, using array.gengo's own line numbers colliding with the user script's. Fixed by skipping trace firing for ip < chunk_state.std_script_code_end. 3. engine-api-test (144 tests) was never wired into `zig build test`, so it never ran in CI or the pre-push hook -- including the previous commit's new concurrency regression test. Wired it into the main test step. This surfaced two more tests with heap/object/stack/frame/defer sizes hardcoded to the default preset, exceeding -Dpreset=stress's tighter ceilings; both now scale to whatever ceiling engine_init_with_config actually enforces. Verified under standard, -Dpreset=stress, and -Dgc_stress=true builds, each now including engine-api-test.
1 parent 9cae7b1 commit 985c579

5 files changed

Lines changed: 118 additions & 20 deletions

File tree

CHANGELOG.md

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,53 @@ This changelog tracks notable language/runtime changes by implementation date.
44

55
## 2026-08-21
66

7+
### Fix — the two follow-ups flagged after the engine C-API race fix
8+
9+
The previous fix (below) left two things flagged rather than fixed. Both
10+
addressed now:
11+
12+
1. **`io.zig`'s trace state was the same non-thread-local pattern.** Fixed
13+
by making `write_override`/`werr_override`/`read_override`/`g_trace_fn`/
14+
`g_trace_userdata`/`g_trace_handle`/`g_trace_prev_line` all `threadlocal`,
15+
matching `engine.zig`/`host_abi.zig`.
16+
17+
2. **The pre-existing `engine_set_trace_fn fires per source line` failure
18+
was real, not flaky — and not test-order pollution as first suspected.**
19+
`engine.zig`'s native test runner (`tools/standalone_runner.zig`) runs
20+
`builtin.test_functions` in fixed file-declaration order with no
21+
shuffling, so the failure was 100% deterministic (confirmed via a debug
22+
print showing the exact same `{1, 7, 1, 2, 3}` line sequence on every
23+
run — not `{1, 2, 3}` as the test expected). Root cause: `dispatchTick`
24+
(`vm.zig`) fired a trace event for every instruction while a trace
25+
callback was active, including the embedded std-library bootstrap
26+
bytecode every program executes at startup regardless of whether it
27+
references `std` (defining `std.array.count` and friends as globals) —
28+
the ghost `1` and `7` were two instructions from that bootstrap's own
29+
`count` function definition (line 1 and line 7, `array.gengo`'s own
30+
line numbers, colliding with the user script's line numbers in the
31+
trace stream). Fixed by skipping trace firing for
32+
`ip < chunk_state.std_script_code_end` (0, hence a no-op, when std
33+
scripts weren't compiled in — e.g. the REPL) — a host's line-level
34+
tracer should never see line numbers from a library implementation
35+
detail the embedding user didn't write and can't map back to their own
36+
source, whether that's the once-per-program bootstrap or a genuine call
37+
into a std-library function implemented in Gengoscript itself.
38+
39+
3. **`engine-api-test` (144 tests, including both fixes above and the
40+
previous race-condition regression test) is now wired into `zig build
41+
test`**, so it runs under every CI lane and the pre-push hook instead of
42+
never running at all. This surfaced two more pre-existing tests
43+
(`engine_call converts wires in the selected engine heap`,
44+
`engine_call converts a large host-supplied wire array without
45+
corrupting elements under heap pressure`) that hardcoded heap/object/
46+
stack/frame/defer sizes tuned only to the default preset — both now
47+
scale to whatever preset's ceiling `engine_init_with_config` actually
48+
enforces, verified passing under `-Dpreset=stress`'s much tighter
49+
limits specifically, not just the default preset.
50+
51+
Verified under standard, `-Dpreset=stress`, and `-Dgc_stress=true` builds,
52+
each including the newly-wired `engine-api-test` step.
53+
754
### Fix — cross-thread engine C-API race: wrong callback (and ctx) could serve the wrong engine
855

956
Follow-up to an independent LLM's Tengo-vs-Gengo comparison, which flagged

build.zig

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,13 @@ pub fn build(b: *std.Build) void {
451451
test_step.dependOn(&run_vm_safety.step);
452452
test_step.dependOn(&run_vm_value.step);
453453
test_step.dependOn(&run_embedding.step);
454+
// engine.zig's own native test suite (engine_api_test, ~144 tests)
455+
// was never wired into `test` — same class of gap as src/main.zig's
456+
// tests above (found 2026-08-19). Discovered while adding a
457+
// regression test for a real cross-thread engine C-API race
458+
// (2026-08-21): the test caught the bug, but nothing in `zig build
459+
// test` or the pre-push hook would ever have run it.
460+
test_step.dependOn(&run_engine_api_tests.step);
454461
test_step.dependOn(&run_engine_runner.step);
455462
test_step.dependOn(&run_fuzz_runner.step);
456463
test_step.dependOn(&install_engine_debug.step);

src/engine.zig

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1568,12 +1568,20 @@ test "engine_call rejects deeply nested host-supplied wire arguments instead of
15681568
}
15691569

15701570
test "engine_call converts wires in the selected engine heap" {
1571+
// engine_init_with_config rejects any field above the build's compiled-in
1572+
// preset ceiling (engine_init_with_config's validateCeiling calls, by
1573+
// design — an embedder can't request more than the build allows). Clamp
1574+
// every field to the ambient ceiling so this test passes under every
1575+
// preset (found failing under -Dpreset=stress's 256KB/512-object/
1576+
// 128-stack/16-frame/32-defer ceilings, all below this test's original
1577+
// fixed request) instead of hardcoding values tuned to only the default
1578+
// preset.
15711579
const config: InstanceConfig = .{
1572-
.heap_size_bytes = 1024 * 1024,
1573-
.max_objects = 2048,
1574-
.max_stack = 512,
1575-
.max_frames = 64,
1576-
.max_defers = 128,
1580+
.heap_size_bytes = @min(cfg.heap_size_bytes, 1024 * 1024),
1581+
.max_objects = @min(cfg.max_objects, 2048),
1582+
.max_stack = @min(cfg.max_stack, 512),
1583+
.max_frames = @min(cfg.max_frames, 64),
1584+
.max_defers = @min(cfg.max_defers, 128),
15771585
.max_ops = -1,
15781586
.allow_io = false,
15791587
};
@@ -1592,7 +1600,14 @@ test "engine_call converts wires in the selected engine heap" {
15921600
const second_engine = getEngine(second).?;
15931601
second_engine.runtime.inner.activate();
15941602
const second_ctx = vms.VMContext.fromActive();
1595-
var ballast: [60000]u8 = undefined;
1603+
// Scaled to the actual (possibly preset-clamped) heap size above, not a
1604+
// fixed 60000 — under -Dpreset=stress's 256KB ceiling, 14 * 60000-byte
1605+
// roots alone (840KB) would overflow the heap before the test's own
1606+
// assertions ever ran. /24 leaves comfortable headroom for the compiled
1607+
// function and chunk/globals overhead under every preset while still
1608+
// filling most of the heap (genuine pressure) under the default preset.
1609+
const ballast_size = config.heap_size_bytes / 24;
1610+
var ballast: [ballast_size]u8 = undefined;
15961611
@memset(&ballast, 'b');
15971612
var roots: [14]Value = undefined;
15981613
for (&roots) |*root| {
@@ -1603,7 +1618,7 @@ test "engine_call converts wires in the selected engine heap" {
16031618
for (roots) |_| second_ctx.vs.popTempRoot();
16041619
}
16051620

1606-
var data: [60000]u8 = undefined;
1621+
var data: [ballast_size]u8 = undefined;
16071622
@memset(&data, 'a');
16081623
var arg: ValueWire = .{
16091624
.tag = @intFromEnum(WireTag.string),
@@ -1641,12 +1656,16 @@ test "engine_call converts wires in the selected engine heap" {
16411656
// is primarily a correctness check for large host-supplied wire arrays
16421657
// under a small, tightly configured heap.
16431658
test "engine_call converts a large host-supplied wire array without corrupting elements under heap pressure" {
1659+
// See the previous test's comment: engine_init_with_config rejects any
1660+
// field above the build's compiled-in preset ceiling, and every field
1661+
// here except heap_size_bytes exceeded -Dpreset=stress's ceilings
1662+
// (512/128/16/32 objects/stack/frames/defers).
16441663
const config: InstanceConfig = .{
16451664
.heap_size_bytes = 96 * 1024,
1646-
.max_objects = 2048,
1647-
.max_stack = 256,
1648-
.max_frames = 64,
1649-
.max_defers = 64,
1665+
.max_objects = @min(cfg.max_objects, 2048),
1666+
.max_stack = @min(cfg.max_stack, 256),
1667+
.max_frames = @min(cfg.max_frames, 64),
1668+
.max_defers = @min(cfg.max_defers, 64),
16501669
.max_ops = -1,
16511670
.allow_io = false,
16521671
};
@@ -1669,7 +1688,11 @@ test "engine_call converts a large host-supplied wire array without corrupting e
16691688
;
16701689
try std.testing.expectEqual(0, engine_run(h, @intFromPtr(src.ptr), src.len));
16711690

1672-
const N = 600;
1691+
// Each element becomes one converted managed-heap object, so N must
1692+
// stay well under the clamped max_objects ceiling too (600 exceeds
1693+
// -Dpreset=stress's 512-object ceiling on its own, before even counting
1694+
// the chunk/globals/compiled-function overhead sharing that budget).
1695+
const N = @min(600, config.max_objects / 2);
16731696
var bufs: [N][16]u8 = undefined;
16741697
var elem_wires: [N]ValueWire = undefined;
16751698
for (0..N) |i| {

src/lang/vm.zig

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2532,7 +2532,19 @@ noinline fn dispatchTick(ctx: VMContext) !u64 {
25322532
if (ctx.vs.ops_budget_remaining == 0) return error.InstructionBudgetExceeded;
25332533
ctx.vs.ops_budget_remaining -= 1;
25342534
}
2535-
if (io.traceActive()) io.fireTrace(ctx.cs.lineAt(ctx.vs.ip), ctx.cs.colAt(ctx.vs.ip));
2535+
// std_script_code_end marks where the embedded std-library bootstrap
2536+
// bytecode ends and real user code begins (0 when std scripts weren't
2537+
// compiled in, e.g. the REPL, so this comparison is a no-op there).
2538+
// Every program executes the std-script bootstrap once at startup
2539+
// (defining std.array.count and friends as globals) regardless of
2540+
// whether it references std at all, and a call into a std-library
2541+
// function implemented in Gengoscript itself also runs bytecode in
2542+
// this same range — neither should surface line numbers from a
2543+
// library source the embedding host's user never wrote, and doing so
2544+
// was a real bug: a fresh engine's very first traced run fired two
2545+
// ghost events from the std bootstrap before the user script's own
2546+
// three lines, both silently corrupting the trace sequence.
2547+
if (io.traceActive() and ctx.vs.ip >= ctx.cs.std_script_code_end) io.fireTrace(ctx.cs.lineAt(ctx.vs.ip), ctx.cs.colAt(ctx.vs.ip));
25362548
return dispatchGasInterval(ctx);
25372549
}
25382550

src/runtime/io.zig

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,22 @@ pub const WriteFn = *const fn (s: []const u8) void;
2525
pub const ReadFn = *const fn (buf: []u8, is_line: bool) isize;
2626
pub const TraceFn = *const fn (userdata: ?*anyopaque, handle: i32, line: i32, col: i32) callconv(.c) void;
2727

28-
var write_override: ?WriteFn = null;
29-
var werr_override: ?WriteFn = null;
30-
var read_override: ?ReadFn = null;
31-
var g_trace_fn: ?TraceFn = null;
32-
var g_trace_userdata: ?*anyopaque = null;
33-
var g_trace_handle: i32 = -1;
34-
var g_trace_prev_line: u32 = 0xFFFF_FFFF;
28+
// threadlocal: set for the duration of one engine_run/engine_call by
29+
// engine.zig's pushEngineState/popEngineState, the same as
30+
// g_active_engine/write_callback/read_callback there and
31+
// native_host_call_fn/_ctx in host_abi.zig. As plain (non-threadlocal)
32+
// vars, two threads calling into different engines concurrently could
33+
// interleave these writes so one engine's trace/write/read calls fired
34+
// through the other engine's callback and userdata — see engine.zig's
35+
// g_active_engine doc comment for the full writeup and the two-thread
36+
// repro that caught the analogous bug in those other fields.
37+
threadlocal var write_override: ?WriteFn = null;
38+
threadlocal var werr_override: ?WriteFn = null;
39+
threadlocal var read_override: ?ReadFn = null;
40+
threadlocal var g_trace_fn: ?TraceFn = null;
41+
threadlocal var g_trace_userdata: ?*anyopaque = null;
42+
threadlocal var g_trace_handle: i32 = -1;
43+
threadlocal var g_trace_prev_line: u32 = 0xFFFF_FFFF;
3544

3645
pub fn setWriteOverrides(w: WriteFn, e: WriteFn) void {
3746
write_override = w;

0 commit comments

Comments
 (0)