Conversation
…06686364 🤖 Auto-Build: Update Ring Loadables
…13284027 🤖 Auto-Build: Update Ring Loadables
…17978510 🤖 Auto-Build: Update Ring Loadables
…80426873 🤖 Auto-Build: Update Ring Loadables
…87658642 🤖 Auto-Build: Update Ring Loadables
…24194425 🤖 Auto-Build: Update Ring Loadables
…28859204 🤖 Auto-Build: Update Ring Loadables
…33847849 🤖 Auto-Build: Update Ring Loadables
…42019750 🤖 Auto-Build: Update Ring Loadables
…62991812 🤖 Auto-Build: Update Ring Loadables
…66326849 🤖 Auto-Build: Update Ring Loadables
Add halt + multi-parameter sweep functionality
…66954208 🤖 Auto-Build: Update Ring Loadables
|
@gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates forkrun to version v3.4.0, introducing several major features including a live telemetry dashboard (TUI), SLURM preemption support with signal traps, auto-halt thresholds on batch failures, and multi-input parameter sweeps. The review feedback highlights critical safety improvements in the C codebase: preventing potential NULL pointer dereferences of state when updating uma_ingest_offset, avoiding a 32-bit integer overflow in the percentage-based halt calculation, and mitigating stack overflow risks in the TUI by moving a large 1MB array off the stack.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| total_unique_batches += atomic_load_relaxed(&state[i].read_idx); | ||
| } | ||
| if (total_unique_batches >= 100) { | ||
| uint32_t current_pct = (total_poisoned * 100) / total_unique_batches; |
There was a problem hiding this comment.
In large-scale runs, total_poisoned can be very large. Since total_poisoned is a uint32_t, the expression total_poisoned * 100 can overflow 32-bit unsigned integer limits if total_poisoned exceeds 42,949,672. This would result in an incorrect percentage calculation and potentially cause a premature abort or fail to abort. To prevent this, cast total_poisoned to uint64_t before multiplying.
uint32_t current_pct = ((uint64_t)total_poisoned * 100) / total_unique_batches;| sys_write(evfd_ingest_data, &v, 8); | ||
| } | ||
| total_moved += copied_in_chunk; | ||
| atomic_store_relaxed(&state[0].uma_ingest_offset, total_moved); |
There was a problem hiding this comment.
The pointer state is dereferenced here without a NULL check. Since state can be NULL (as seen in other defensive checks in this function, such as on lines 5878, 6002, and 6013), this can lead to a segmentation fault. Please guard this store with a NULL check.
if (state) atomic_store_relaxed(&state[0].uma_ingest_offset, total_moved);| sys_write(evfd_ingest_data, &v, 8); | ||
| } | ||
| total_moved += n; | ||
| atomic_store_relaxed(&state[0].uma_ingest_offset, total_moved); |
There was a problem hiding this comment.
The pointer state is dereferenced here without a NULL check. Please guard this store with a NULL check to prevent potential segmentation faults.
if (state) atomic_store_relaxed(&state[0].uma_ingest_offset, total_moved);| sys_write(evfd_ingest_data, &v, 8); | ||
| } | ||
| total_moved += written; | ||
| atomic_store_relaxed(&state[0].uma_ingest_offset, total_moved); |
There was a problem hiding this comment.
The pointer state is dereferenced here without a NULL check. Please guard this store with a NULL check to prevent potential segmentation faults.
if (state) atomic_store_relaxed(&state[0].uma_ingest_offset, total_moved);| uint8_t node_cpu_map[1024][1024]; | ||
| memset(node_cpu_map, 0, sizeof(node_cpu_map)); |
There was a problem hiding this comment.
Allocating a large array like node_cpu_map[1024][1024] (1 MB) on the stack can easily lead to stack overflow, especially in environments with limited stack sizes (such as custom threads or containerized environments). It is highly recommended to allocate this array on the heap using calloc or declare it as static to move it to the data segment, as ring_tui_main is run once in a background process.
static uint8_t node_cpu_map[1024][1024];
memset(node_cpu_map, 0, sizeof(node_cpu_map));…44624765 🤖 Auto-Build: Update Ring Loadables
…45938774 🤖 Auto-Build: Update Ring Loadables
…69272379 🤖 Auto-Build: Update Ring Loadables
fix resume for multi-parameter sweeps
…71012354 🤖 Auto-Build: Update Ring Loadables
|
1 participant
NEW FEATURES:
--haltflag to auto halt after some failure threshold:::,::::, and--linkflags (same syntax/functionality as GNU parallel)