ReviewOS

zig-utils/zig-js

GC: roll back partial key-order allocation on OOM

#122
Closed chris opened this 9 days ago ·
9 days ago

Related: #120, #100

Failure

Main CI run https://github.com/zig-utils/zig-js/actions/runs/29460140328 failed only unit (shard 1/4) after all 192 selected tests completed: 190 passed, 2 expected skips, 0 failed, but the shard reported one leak.

The exact witness is Context heap_limit_bytes object side-store pressure fails closed. The newer CI Zig/shard allocation layout exhausts the heap while Object.ensureKeyOrderUnlocked is seeding an unpublished key-order list. Every already-duplicated shape key leaks because the error path neither frees the partial list nor destroys its list header. The local full suite passed because its older Zig/allocation layout reached OOM at a different allocation point.

Scope / acceptance

  • Build key-order lists transactionally: free every duplicated key, list allocation, and newly activated backing flag when initialization fails before publication.
  • Make owned-key append free the just-duplicated key if list growth fails; use the same helper in initialization, incremental record, and replacement paths.
  • Add deterministic all-allocation-failures coverage rather than depending on a particular heap-limit/Zig allocation layout.
  • Preserve property/accessor creation order, GC backing-store accounting, heap-limit failure behavior, and exact teardown.
  • Pass the focused OOM/key-order test, the existing heap-limit witness, and the next batched CI run.
9 days ago

Implemented and pushed as 5cd13df3 (fix(object): roll back key-order OOM (#122)).

The fix makes unpublished key-order construction transactional, deactivates a newly selected backing store even when the list header allocation fails, and uses one owned-key append helper that frees a duplicate if list growth fails. The replacement-list path receives the same pre-header rollback and helper.

Focused validation is green:

  • deterministic checkAllAllocationFailures sweep across key-order initialization and incremental append: 2/2, 0 failed/leaked;
  • original Context heap_limit_bytes object side-store pressure fails closed witness: 2/2, 0 failed/leaked.

Replacement main run: https://github.com/zig-utils/zig-js/actions/runs/29460517377. Keeping this open until that batched run confirms the CI allocation layout.

9 days ago

Confirmed on the replacement full-main CI run at 5cd13df: unit (shard 1/4), which contains the heap-limit/key-order OOM regression, passed. Local allocation-failure sweeps and the pre-existing pressure test were already clean. Closing as fixed by 5cd13df.\n\nCI: https://github.com/zig-utils/zig-js/actions/runs/29460517377

Sign in to comment on this issue.