Potential issue with iterator invalidation during overlay code cleanup

[roninjin10]

Summary

The database overlay cleanup code iterates over a HashMap while freeing memory. While Zig's HashMap iterator is generally safe during iteration, the pattern of freeing values while iterating could be problematic if the allocator implementation interacts with the HashMap in unexpected ways.

Priority: LOW

This is a defensive coding concern. The current code likely works correctly, but the pattern is potentially fragile.

Location

File: src/storage/database.zig
Lines: 201-203, 211-213, 235-237

Current Code

// In begin_ephemeral_view (lines 201-203):
var it = self.overlay_code.iterator();
while (it.next()) |entry| self.allocator.free(entry.value_ptr.*);
self.overlay_code.clearRetainingCapacity();

// In discard_ephemeral_view (lines 211-213):
var it = self.overlay_code.iterator();
while (it.next()) |entry| self.allocator.free(entry.value_ptr.*);
self.overlay_code.clearRetainingCapacity();

// In deinit (lines 235-237):
var it = self.overlay_code.iterator();
while (it.next()) |entry| self.allocator.free(entry.value_ptr.*);
self.overlay_code.deinit();

Potential Issues

1. Iterator Stability: While Zig's HashMap iterator doesn't invalidate during value reads, the free() call modifies memory that the HashMap entry points to
2. Allocator Side Effects: Some allocator implementations might have side effects that affect the HashMap (unlikely but possible with custom allocators)
3. Code Duplication: The same pattern is repeated 3 times, which suggests a helper function would be cleaner

Recommended Fix

Create a helper function that makes the intent clear and is easier to audit:

/// Free all code entries in a code HashMap and clear it
fn clearCodeStorage(code_map: *std.HashMap([32]u8, []const u8, ArrayHashContext, std.hash_map.default_max_load_percentage), allocator: std.mem.Allocator) void {
    // Collect keys first to avoid any iterator invalidation concerns
    var keys_to_free = std.ArrayList([32]u8).init(allocator);
    defer keys_to_free.deinit();
    
    var it = code_map.iterator();
    while (it.next()) |entry| {
        keys_to_free.append(entry.key_ptr.*) catch continue;
    }
    
    // Free values using collected keys
    for (keys_to_free.items) |key| {
        if (code_map.get(key)) |code| {
            allocator.free(code);
        }
    }
    
    code_map.clearRetainingCapacity();
}

Or, if we're confident the current pattern is safe (it likely is), at minimum extract to a helper:

/// Free all allocated code slices in the map
fn freeAllCode(self: *Database, code_map: *@TypeOf(self.overlay_code)) void {
    var it = code_map.iterator();
    while (it.next()) |entry| {
        self.allocator.free(entry.value_ptr.*);
    }
}

// Usage:
self.freeAllCode(&self.overlay_code);
self.overlay_code.clearRetainingCapacity();

Steps to Fix

1. Open src/storage/database.zig
2. Add a helper function for freeing code storage entries
3. Replace the 3 duplicate iteration patterns with calls to the helper
4. Add a comment documenting why this pattern is safe (if it is)
5. Run zig build && zig build test to verify
6. Consider adding a test that exercises the overlay code cleanup path

Verification

# Ensure all tests pass, especially storage-related ones
zig build test-unit

# Run with AddressSanitizer if available to detect memory issues
zig build test -Dsanitize=true

Alternative: Use valueIterator Pattern

Zig's HashMap has a valueIterator() that might be cleaner:

// This might be cleaner but check if it has same concerns
var value_it = self.overlay_code.valueIterator();
while (value_it.next()) |value| {
    self.allocator.free(value.*);
}

Context

The overlay system is used for ephemeral state during simulate() operations. The cleanup happens when discarding the overlay, which is a common operation. While the current code likely works, defensive coding practices suggest being explicit about memory operations during iteration.

Related Memory Patterns

Also review these similar patterns in the file:

• code_storage cleanup in deinit() (lines 222-227)
• Snapshot cleanup (lines 229-233)

---

Note: This issue was created by Claude AI assistant during a code review, not by @roninjin10 or @fucory