ReviewOS

zig-utils/zig-js

GC: keep object metadata insertion transactional under OOM

#94
Closed chris opened this 10 days ago ·
10 days ago

Problem

Fresh main CI run https://github.com/zig-utils/zig-js/actions/runs/29353934622 failed tsan-unit (shard 1/4) in Context heap_limit_bytes object side-store pressure fails closed.

The bounded allocator correctly returns OOM, but Object.setAttrUnlocked calls StringHashMapUnmanaged.getOrPut before duplicating the owned key. If that subsequent key duplication fails, the new map entry remains installed with the caller-borrowed key slice. GC finalization later treats every attrs key as backing-allocator-owned and frees the borrowed slice, causing SafeAllocator to abort with an invalid free at src/gc.zig:322.

Object.setAccessor uses the same insertion-before-key-copy pattern and has the same latent failure mode.

Failed job: https://github.com/zig-utils/zig-js/actions/runs/29353934622/job/87156822946

Scope

  • Duplicate owned metadata keys before publishing new attrs/accessor entries.
  • Free the speculative duplicate when the key already exists or map insertion fails.
  • Preserve current normal-path behavior and allocation accounting.

Acceptance criteria

  • The focused bounded-heap side-store failure test returns OOM and destroys the context without invalid frees or leaks.
  • Add focused coverage for accessor-key OOM rollback as needed.
  • tsan-unit (shard 1/4) and the fresh publication gate are green.
10 days ago

Implemented and pushed in f4602d86.

The attrs and accessors maps now check existing entries first, duplicate a new owned key before map publication, and release that speculative duplicate if insertion itself fails. The bounded-heap fail-closed test now also creates accessor metadata so both ownership paths participate in pressure and destruction.

Focused validation:

zig build test -Dtsan=true -Dtest-filter="Context heap_limit_bytes object side-store pressure"
3 passed; 0 skipped; 0 failed; 0 leaked

zig build test -Dtest-filter="Context heap_limit_bytes object side-store pressure fails closed"
2 passed; 0 skipped; 0 failed; 0 leaked

Leaving open until the fresh tsan-unit (shard 1/4) publication job is green.

10 days ago

Fresh publication run: https://github.com/zig-utils/zig-js/actions/runs/29355087082

10 days ago

Verified on fresh main run 29355087082: tsan-unit shard 1/4 completed successfully after f4602d8. The focused local side-store pressure tests and the full non-TSan unit suite also pass with zero leaks. Closing as fixed.

Sign in to comment on this issue.