Fork Sync: Update from parent repository - #176
Merged
Merged
Conversation
The PRRTE AGENTS.md and its PMIx counterpart share the same authorship and much of the same guidance, but the PMIx file had accumulated several general, project-agnostic rules that PRRTE's copy was missing. Carrying them over keeps the two orientation guides consistent so an agent moving between the code bases meets the same expectations, and it fills real gaps in the PRRTE guidance around style, hygiene, and process. Added: the conditional-spacing style rule; a directive to update .gitignore for any build product a change introduces; the "never bend a test to accommodate a bug" testing rule; a shared-repository/worktree section warning against repo-wide git commands; the preference for C++-style comments; commit-message guidance (body line wrapping, no AI tooling attribution, incidental fixes as standalone commits); and a maintainer-mode caution for build-system edits. While porting the build-system guidance, correct an over-broad claim in the existing text: because PRRTE builds in maintainer mode, editing a Makefile.am does not require the full autogen.pl plus configure cycle — a plain make regenerates the affected Makefiles. Also repair a garbled duplicate line in the copyright-header rule. Signed-off-by: Ralph Castain <rhc@pmix.org>
The m4-configure component list for each framework is ordered from highest to lowest priority so that STOP_AT_FIRST, STOP_AT_FIRST_PRIORITY, and PRIORITY frameworks configure their components in the intended order. Until now that ordering was produced by shelling out to config/prte_mca_priority_sort.pl through esyscmd during configure, which forks an external Perl interpreter at autogen time and adds Perl as an implicit build dependency for this one small task. Do the sort entirely in m4 instead. MCA_ORDER_COMPONENT_LIST now scans the framework's components to find the highest and lowest priorities, then walks the priorities from high to low, emitting the components that carry each priority. Walking the priorities rather than sorting the components preserves the original relative order among components that share a priority, matching the previous behavior. The Perl script and its EXTRA_DIST entry are removed. Signed-off-by: Ralph Castain <rhc@pmix.org>
PRRTE and PMIx are developed in lockstep and most contributors work in both trees, yet the two AGENTS.md guides had drifted into different section orders and different levels of coverage. A rule learned in one project was hard to relocate in the other, and PRRTE's guide had fallen behind the code. Reorder the page to follow the companion PMIx AGENTS.md so each topic sits in the same place in both. The thread model, caddy pattern, and blocking/non-blocking examples move out of the Coding Rules grab-bag into their own "Thread Safety & the Progress Thread" section, and the build and testing material is consolidated into a single "Build & Test Procedures" section rather than being split between a Build section and a stray Testing subsection under Contributing. Bring the content current. The Testing text still claimed PRRTE had no standalone unit tests; it now documents the test/unit suite wired into make check and the offline mapper harness driven by make check-offline over the shared synthetic topologies. Add the two procedures the old guide lacked -- how to test-build a change and a "did I break it" verification checklist -- and import the portable rules the PMIx guide carried that apply equally here: PRTE_EXPORT symbol visibility, unique numeric values for status and state codes, a Performance Considerations section, and a closing General Guidance section. Finally, repoint the stale cross-reference in the rmaps unit-test Makefile.am at the section that now describes the offline harness. Signed-off-by: Ralph Castain <rhc@pmix.org>
Give every MCA framework under src/mca a developer-level AGENTS.md that orients a contributor to what the framework does, where it runs in the DVM and job lifecycle, the module/component vtable contract, and a detailed walk of the functions its base/ provides. Each component directory gains its own AGENTS.md describing that component's files, selection gates, and the specifics of how it implements the framework interface. Every AGENTS.md carries a CLAUDE.md symlink so either name resolves to the same guide. These follow the model already established by the rmaps guides and let an AI agent or a human new to a subsystem come up to speed from the code itself rather than by reverse-engineering it each time. The companion rmaps framework guide is refreshed to match the current source (the map_job size note and a ranking-wording correction). The empty common framework gets a short guide explaining why its Makefile.am placeholder exists. Documentation only; no source or build files are changed. Signed-off-by: Ralph Castain <rhc@pmix.org>
The DVM error manager's job- and process-error handlers carried two latent defects that this commit repairs. First, the `prte_finalizing` early return in `job_errors` returned without releasing the state caddy it had just acquired, leaking the object on every job-error activation seen while the DVM is shutting down. It now releases the caddy before returning, matching the discipline used on every other exit path in the file. Second, the FAILED_TO_START / FAILED_TO_LAUNCH arm of `proc_errors` selected the reported job state with `if (PRTE_PROC_STATE_FAILED_TO_START)` — a bare constant that is always true — so a genuine FAILED_TO_LAUNCH proc was always recorded as FAILED_TO_START. The test now compares the actual `state`, so each failure is reported as itself. Also refresh the component guide to describe the corrected code rather than the former smell. Signed-off-by: Ralph Castain <rhc@pmix.org>
The HNP I/O-forwarding component gated its query on `!PRTE_PROC_IS_MASTER && !PRTE_PROC_IS_MASTER` — the same predicate tested twice, a harmless copy-paste artifact. Collapse it to a single `!PRTE_PROC_IS_MASTER` test and update the component guide to match. Signed-off-by: Ralph Castain <rhc@pmix.org>
`plm_pals_init` printed "OOPS prte_plm_base_comm_start returned error" straight to stderr on the comm-start failure path — a leftover debug artifact that bypasses the project's error-reporting conventions. The failure is already recorded with `PRTE_ERROR_LOG`, so drop the raw stderr write and refresh the component guide. Signed-off-by: Ralph Castain <rhc@pmix.org>
`grpcomm_base_stubs.c` was never listed in `base/Makefile.am` and so was never compiled. It described a retired collective-tracking API (`prte_grpcomm_signature_t`, `prte_grpcomm_base.actives`/`sig_table`, `prte_grpcomm_base_get_tracker`, a `grp_construct` vtable entry, and `prte_pmix_grp_caddy_t` fields that no longer exist), so it served only to mislead a reader into thinking the base still owns collective state. Delete it — the live tracking model lives entirely in the `direct` component — and drop the guides' warnings about it. Signed-off-by: Ralph Castain <rhc@pmix.org>
`ras_base_close.c` was not listed in `base/Makefile.am` and so was never compiled; the framework's real close hook, `prte_ras_base_close`, is defined in `ras_base_frame.c` and wired into the framework DECLARE. The orphaned copy still referenced `active_module`/`ras_opened` fields that no longer exist on `prte_ras_base`, so it would not even build. Delete it and adjust the framework guide accordingly. Signed-off-by: Ralph Castain <rhc@pmix.org>
`ess/base/base.h` declared four symbols — `prte_ess_env_get`, `prte_ess_env_put`, `prte_ess_base_proc_binding`, and the `prte_ess_base_std_buffering` variable — that have no definition anywhere in the tree. They are remnants of long-removed functionality and only invite a reader to wire new code to something that will not link. Drop the declarations and update the framework guide. Signed-off-by: Ralph Castain <rhc@pmix.org>
PRRTE carried a full prtedl (dlopen/dlsym/dlclose) framework, but it is dead weight: nothing selects a prtedl module or calls the prte_dl_* wrapper API anywhere in the tree. PRRTE has no MCA base of its own — the plugin/component system comes from the installed PMIx — so the code that actually dlopen()s PRRTE's DSO components is PMIx's component repository, which loads through PMIx's own pdl framework. prtedl is a leftover from when the MCA base was embedded in-tree and has been unused for a long time (its libltdl component would not even compile). Delete the whole src/mca/prtedl tree. The generated framework list (config/autogen_found_items.m4, src/include/prte_frameworks.*) is rebuilt by autogen.pl from the directory layout, so it drops prtedl automatically. Remove the now-dead references that fall out of the deletion: - schizo no longer rewrites a generic "--mca dl_foo" into the PRRTE "prtedl_foo" namespace (in both the parse_prte scanner and the ompi personality's check_prte_overlap). The PMIx-side mapping to pdl is retained, so those directives now correctly route to PMIx's pdl instead of a framework that no longer exists. - prte_info reported dynamic-loading support via PRTE_HAVE_DL_SUPPORT, which prtedl's configure.m4 defined. It now reports PMIX_HAVE_PDL_SUPPORT, which reflects the loader PRRTE actually uses. Verified with a clean --enable-debug build, make check, and a live prterun -n 2 hostname launch. Signed-off-by: Ralph Castain <rhc@pmix.org>
GitHub will be charging for its use Signed-off-by: Ralph Castain <rhc@pmix.org>
Reflect changes to the mapby, rankby, and bindto options. Signed-off-by: Ralph Castain <rhc@pmix.org>
Update to reflect change in those cmd line options Signed-off-by: Ralph Castain <rhc@pmix.org>
The odls child code that runs between fork() and execve() rendered show_help messages, closed file descriptors by scanning /proc/self/fd with opendir/readdir, allocated argv, and called getcwd/stat/asprintf/strerror before reporting an execve failure. The binding code reached through the same path allocated error strings with asprintf/strdup and even terminated the child with exit() rather than _exit(). None of that is legal after fork(). fork() copies only the calling thread and leaves any lock another thread held frozen in the child, so calling into the allocator, stdio, opendir, or show_help can deadlock the child intermittently under load - which then blocks the parent that is waiting on the error pipe. This mirrors the problem fixed on the PMIx side in openpmix/openpmix#4005. Restrict the child to async-signal-safe operations and move all rendering to the parent. Replace the variable-length show_help-string pipe protocol with a fixed-size record carrying a failure code (prte_odls_child_err_t) plus errno; the child writes only that record via the new prte_odls_base_child_fail (fatal, _exit) and prte_odls_base_child_warn (non-fatal, returns), shared by both the component's do_child and the base binding routine so the two writers cannot drift apart. do_parent reads the record and renders the human-readable diagnostic - including the bad-interpreter stat() check and the working-directory reconstruction - where allocation and show_help are safe. Close file descriptors with a plain close() loop instead of the /proc scan, and default argv in the parent before the fork so the child never allocates it. While adding the parent-side rendering, register help topics for the three binding-warning messages (not bound, memory not bound, incorrectly bound) that the binding code referenced but that were absent from every help file, so those warnings rendered as "help not found" rather than the intended text. Signed-off-by: Ralph Castain <rhc@pmix.org>
prte_odls_base_set() ran entirely in the forked child before execve() and, to bind the process, called hwloc_bitmap_alloc, parsed the mapper's cpuset string with hwloc_bitmap_list_sscanf, invoked hwloc_set_cpubind and hwloc_set_membind, and - for --report-bindings - rendered a string and printed it. hwloc allocates, and on Linux hwloc_set_cpubind CPU_ALLOCs a mask once the logical-CPU count exceeds the fixed cpu_set_t size (exactly the large-node case). None of that is legal after fork(): the fork happens on an odls spawn-pool worker thread while other spawn threads and the PMIx progress thread are allocating, so a frozen malloc lock can deadlock the child. Split the work. prte_odls_base_prepare_binding() now runs in the parent, before the fork, from spawn_proc: it parses the cpuset, classifies the binding, precomputes the memory-binding policy, emits the --report-bindings output and the "incorrectly bound" warning, and stashes a ready-to-apply hwloc bitmap on the caddy. prte_odls_base_set() then runs in the child and only issues the bind syscalls. For CPU affinity the child now issues a bare sched_setaffinity() using a raw cpu_set_t that the parent precomputed from the hwloc bitmap - a syscall is async-signal-safe. This is gated on a new PRTE_HAVE_SCHED_SETAFFINITY configure check (config/prte_check_setaffinity.m4); where the function is absent, e.g. macOS, an #else clause falls back to hwloc_set_cpubind as before. Memory binding still goes through hwloc_set_membind in the child on all platforms; turning that into a bare set_mempolicy/mbind syscall would mean reproducing hwloc's NUMA nodeset handling and is left for a follow-up. Because the child no longer reads the applied binding, --report-bindings now renders the binding the mapper requested rather than the value read back after the bind. For a successful bind the two are identical, and this keeps the allocating cset2str/pmix_output out of the async-signal-safe child. Signed-off-by: Ralph Castain <rhc@pmix.org>
Add Slurm-private tracker objects for release requests that start a DVM shrink before Slurm resources are returned. Each tracker records its PRRTE shrink campaign and owns per-Slurm-job release actions. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Add init/finalize helpers for the Slurm release shrink tracker list and wire them into the Slurm RAS module lifecycle. Finalization drains any remaining trackers so their nested release actions are also released. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Update count-based Slurm release to skip allocations that already have an active release shrink tracker. The request is accepted only if enough non-conflicting sessions remain, and the mutation path uses the same newest releasable session selection. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Treat the Slurm job that issued a count-based release as removable only down to one surviving node. This lets count release use spare nodes from the requesting allocation without ever cancelling that allocation or removing the current Slurm daemon node. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Extend Slurm release request parsing to accept PMIX_ALLOC_NODE_LIST and PMIX_ALLOC_ID in addition to PMIX_ALLOC_NUM_NODES. Node-list releases resolve exact target nodes, group them by Slurm allocation, and resize partial allocations with an explicit survivor list. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Start Slurm release requests by planning the resources to remove and launching a DVM shrink campaign. Defer Slurm-side cancellation or resize until the shrink completion callback reports that the targeted daemons have left. Track pending release actions privately in the Slurm release component so overlapping requests for the same Slurm job are rejected while unrelated release requests can proceed. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Reject Slurm release requests before parsing or planning when the DVM is not running in elastic mode. Print a hint telling users to enable the prte_elastic_mode MCA parameter. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Track whether Slurm sessions were created dynamically by PRRTE and only allow full RM-side termination for those sessions. Sessions discovered from the initial allocation may still be partially shrunk, but explicit full release requests for them are rejected. When static sessions are destroyed locally, return success without asking Slurm to terminate the underlying job. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Keep the local helper declarations in ras_slurm_modify_release.c on single lines for a more uniform declaration block. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Break prte_ras_base_complete_request() into smaller grow and shrink helpers while preserving the existing public entry point. The split keeps the shared DVM orchestration in RAS base, but exposes helpers that can be called independently by other RAS components. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Use the shared DVM shrink helper for Slurm release requests after Slurm has planned the scheduler-side release actions. Keep Slurm-specific tracking and RM-side cancellation or resize work in the Slurm release component. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Route Slurm extend completion through the shared RAS base helper that marks the DVM for growth and activates daemon launch. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
During elastic shrink, Slurm may terminate the srun launcher for a released dynamic allocation. It can also terminate an srun step whose session remains but has shrunk down to the HNP node. Treat these non-zero srun exits as expected when elastic mode is active. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Return PRTE_ERR_OP_IN_PROGRESS after accepting a Slurm release request so the module-level modify path can avoid touching the request after the callback may have released it. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Add test/unit/errmgr, wired into make check, covering the parts of the errmgr framework's structural contract that are reachable without a live DVM. It asserts that every module struct -- the log-only default, the live global, and the dvm and prted role modules -- carries a non-NULL logfn, the load-bearing invariant that lets prte_errmgr.logfn be safely called during a very early failure before the framework is even opened, and that prte_errmgr_base_log survives a normal code, PRTE_SUCCESS, and an out-of-range "silent" code without dereferencing a NULL string. The handlers themselves mutate global runtime state and drive termination through plm and grpcomm, so they still require a live DVM and remain covered by the integration and dockerswarm harnesses. Register the new Makefile in the config file list and .gitignore and add a Testing section to the framework guide. Adding the config file list entry required regenerating configure via autogen.pl. Signed-off-by: Ralph Castain <rhc@pmix.org>
The remaining early returns in proc_errors -- the pack-failure branches inside the CALLED_ABORT, TERM_NON_ZERO, keep_going abnormal-termination report, and all-terminated blocks -- bare-returned after releasing the data buffer, leaking the state caddy the handler had acquired. Each of these eleven paths now routes through the cleanup label so the caddy is released. These are rare (they fire only when packing an alert to the HNP fails), which is why they lingered after the non-error leaks fixed in the previous commit, but there is no reason to treat them differently: every exit from the handler owns the caddy release. The send-failure paths, which release the alert and fall through to cleanup rather than jumping, are left as-is, and the component guide is updated to state the invariant that every proc_errors exit reaches cleanup. Signed-off-by: Ralph Castain <rhc@pmix.org>
The signal-forwarding callback packs a PRTE_DAEMON_SIGNAL_LOCAL_PROCS command and sends it to the daemon itself, which unpacks a jobid followed by an int32 signal number and delivers that signal to the local application processes. The 2021 conversion of PRRTE process names to PMIx process names replaced the packed signal value with PRTE_JOBID_WILDCARD, a zero-filled nspace, while leaving the pack type as PMIX_INT32. The receiver therefore always unpacked signal 0, so every signal a user asked to forward via ess_base_forward_signals was delivered as the null signal -- a silent no-op. Pack the caught signal number (signum) as the code originally did so the forwarded signal actually reaches the application. Signed-off-by: Ralph Castain <rhc@pmix.org>
prte_ess_base_setup_signals rejects a signal the system cannot forward (SIGKILL, SIGTERM, and friends) when the user names it, but the branch that parses a signal given as an integer never consulted the can_forward flag. A request such as "ess_base_forward_signals=15" thus slipped a non-forwardable signal onto the forward list, installing a spurious handler that the by-name path would have refused -- for a catchable signal like SIGTERM that means a second handler competing with the daemon's own shutdown handler. Apply the same can_forward gate in the numeric branch so both input forms behave identically. Signed-off-by: Ralph Castain <rhc@pmix.org>
The env, slurm, pals, and lsf daemon modules each call their <rm>_set_name() helper and then proceed straight into prte_ess_base_prted_setup(), discarding the helper's return code. A missing nspace, vpid, or node-offset environment variable therefore left the daemon with a half-built identity and only surfaced later as a confusing failure deep in the shared bring-up. Check the return and route a failure through the module's error path so it reports the actual failing step. The slurm and lsf helpers additionally passed the result of getenv() straight to atoi() when computing the per-node vpid offset (SLURM_NODEID, LSF_PM_TASKID). The launchers always set these, so the normal path is unaffected, but atoi(NULL) is undefined behavior and crashes on glibc. Guard the lookups and return PRTE_ERR_NOT_FOUND when the variable is absent, matching what pals already did -- now that the caller checks the return, this becomes a clean error instead of a crash. While here, drop a stray empty statement after the return in the lsf module's rte_finalize. The component AGENTS.md notes are updated to describe the guarded, return-checked behavior. Signed-off-by: Ralph Castain <rhc@pmix.org>
Almost all of the ess framework -- daemon and HNP bring-up -- needs a live DVM and is exercised by the integration harnesses. The one piece that is pure input parsing, prte_ess_base_setup_signals(), can be tested in isolation, and it produces the very signal numbers the forwarding callback delivers, so it is worth pinning down. Add test/unit/ess/test_ess.c, wired into make check. It drives the "none" no-op case and a mixed name/duplicate parse, and asserts each accepted entry carries a real, non-zero signal number -- the field a recent regression had corrupted. Because setup_signals latches after its first non-"none" call, a process can drive only one substantive parse; the test is structured around that. The framework AGENTS.md gains a Testing section describing what is and is not unit-testable here. Signed-off-by: Ralph Castain <rhc@pmix.org>
The routing layer notifies every runtime consumer of a daemon fault by calling prte_grpcomm.fault_handler(), prte_filem.fault_handler(), and prte_relm.fault_handler() unconditionally from routed_radix.c, with no NULL guard. The filem framework's default "none" module -- the module that stays installed whenever no filem component is selected, which is a supported configuration -- never initialized its fault_handler slot, so it was NULL. The first daemon fault in such a build dereferenced a NULL function pointer and crashed the DVM. Add a prte_filem_base_none_fault_handler no-op and wire it into the "none" module so the slot is always safe to call. Signed-off-by: Ralph Castain <rhc@pmix.org>
The base filem RML query service (filem_base_receive.c) is scaffolding that nothing currently starts, but it carries two latent bugs that would bite whenever it is revived. prte_filem_base_comm_stop() inverted its "already stopped?" test -- it read "if (recv_issued) return" instead of "if (!recv_issued) return", so it bailed out without ever cancelling the receive. And filem_base_process_get_remote_path_cmd() leaked the unpacked filename on the getcwd() failure path by returning directly instead of branching to its CLEANUP label. Correct the guard and route the error through CLEANUP. Signed-off-by: Ralph Castain <rhc@pmix.org>
Several receive-side error paths in the raw component mishandled the objects they had already placed on a file-scoped list. recv_ack() freed the unpacked filename only when it matched an in-flight transfer, leaking it whenever an ack arrived for an already-positioned file. In recv_files(), the chunk-0 dirpath-create failure released the incoming object without first removing it from incoming_files -- leaving a dangling pointer the next chunk would walk -- while the two fd-open failures leaked the incoming object entirely; make all three remove-then-release consistently. write_handler() leaked the zero-byte EOF marker output on every completed transfer and leaked the tar command and dirname strings on the archive error paths (and left the daemon's cwd changed after a failed extraction); release the marker and free the strings, restoring the cwd on failure. None of these are hot-path, but the dangling incoming pointer is a latent use-after-free. Signed-off-by: Ralph Castain <rhc@pmix.org>
Preload staging was broken end to end because create_link() and its caller pointed the symlinks in the wrong places. create_link() left rc holding the PMIX_ERR_EXISTS (-11) that pmix_os_dirpath_create() returns whenever the per-proc session directory already exists -- the normal case. The guard tolerated that value but never reset rc before the symlink, so a link that was created perfectly still returned -11 (surfacing as PRTE_ERR_IN_ERRNO) and raw_link_local_files() aborted the launch. Reset rc to PRTE_SUCCESS once the directory is known to exist. The symlink source was also wrong: recv_files() writes staged bytes under the node's top_session_dir, but create_link() was handed the job session_dir as the source base, so the link pointed one level too deep at a path that does not exist. Pass top_session_dir as the source. Finally, the link target: a preloaded data file belongs in each proc's own session directory, but a preloaded binary does not. --preload-binary sets PRTE_APP_SSNDIR_CWD, which makes every proc's cwd the job session directory (see setup_path in odls), so "./<binary>" only resolves if the executable is linked there. Select the target by inbnd->type: the job session dir for PRTE_FILEM_TYPE_EXE, the per-proc dir for everything else. Also free the file list with PMIx_Argv_free rather than a bare free on the create_link error path. Signed-off-by: Ralph Castain <rhc@pmix.org>
… open raw_preposition_files() threadshifts each file to send_chunk() as it walks the list, so an already-queued transfer has a live libevent event and an open fd. When a *later* file failed to open, the function released the outbound -- whose destructor drains the queued transfers -- and returned PRTE_ERROR. That freed the transfers out from under their pending events (a use-after-free libevent flags as "event_queue_remove_active ... not on queue"), leaked their fds, and double-drove PRTE_JOB_STATE_FILES_POSN_FAILED because the caller treats the error return as a failure while the eventual completion callback fires as well. Instead, record PRTE_ERR_FILE_OPEN_FAILURE in outbound->status and stop queueing: the already-queued transfers complete normally and the completion callback delivers that status, or -- if nothing was queued -- the empty list path fires the callback with it. The loop can now leave entries on the temporary fileset, so drain it with PMIX_LIST_DESTRUCT. Signed-off-by: Ralph Castain <rhc@pmix.org>
setup_path() honored PRTE_APP_SSNDIR_CWD by reading the session directory from app->job. That back-pointer is only ever set on the HNP (in the ess/hnp and dynamic-spawn paths); it is not serialized into the launch message, so on any daemon that rebuilt the job from that message it is NULL. --preload-binary sets PRTE_APP_SSNDIR_CWD, so launching a preloaded binary dereferenced a NULL app->job and segfaulted the daemon. Pass the job into setup_path() explicitly. Both call sites already hold it -- launch_local from jobdat, restart_proc from prte_get_job_data_object() -- so the back-pointer is never needed. Signed-off-by: Ralph Castain <rhc@pmix.org>
prte_app_parse.c deliberately leaves app->app.cwd NULL when the user gives --set-cwd-to-session-dir: the working directory is chosen on the backend from the PMIx_SET_SESSION_CWD directive rather than fixed at parse time. The spawn path in prte() then handed that NULL straight to strdup() while building the PMIx app array, crashing the launcher before the job was even submitted. Guard the copy so a NULL cwd stays NULL in the PMIx app, which is exactly what the session-cwd directive expects. Signed-off-by: Ralph Castain <rhc@pmix.org>
The staging machinery needs a live DVM, but two pieces of the framework can be exercised in isolation and are worth pinning down. test_filem checks that the process-set, file-set, and request classes construct to their documented defaults and tear down cleanly, and that the default "none" module is a set of safe no-ops -- every slot returns success, preposition_files still fires its completion callback, and fault_handler is non-NULL and callable (the regression pin for the routed_radix crash). Wire it into make check alongside the other test/unit suites. Signed-off-by: Ralph Castain <rhc@pmix.org>
A single-host run cannot prove file staging works, because the source file is already present in the launch directory -- the app runs even if filem does nothing. The multi-node swarm can: compile a marker binary on node1 only, then run it under --preload-binary on node2 and node3, where it can only execute if the bytes were actually staged across daemons and linked into the job session directory the session cwd points at. Add that test to run-tests.sh and describe it in the README. Signed-off-by: Ralph Castain <rhc@pmix.org>
Every xcast builds an op_t in xcast_nb() to carry the (possibly compressed) payload, thread-shifts it to begin_xcast(), which packs it and relays it to the HNP. The op the master actually tracks and completes is a fresh one built on receipt, so the initiating op is meant to be discarded once relayed -- but begin_xcast() never released it, and xcast_nb() also dropped it on the copy-payload failure path. Because xcast underlies every launch, wireup, and daemon command, this leaked an op_t plus its payload on the hottest collective path. Release the op on all of begin_xcast()'s exits and on the xcast_nb() error path. Signed-off-by: Ralph Castain <rhc@pmix.org>
The group() handler allocates a prte_pmix_grp_caddy_t, thread-shifts it, and caches only its callback pointers into the long-lived tracker. The caddy is therefore consumed by the handler, but its two success returns (the bootstrap-to-HNP path and the send-to-self path) never released it, leaking the caddy and its strdup'd groupID on every successful group construct/destruct. Release it on both. While here, close the matching gap on the error label, which leaked the open grpinfo/endpts info-lists on every jump to it; NULL them once they are released on the success path so the post-release send-failure jumps do not double-release. Signed-off-by: Ralph Castain <rhc@pmix.org>
When the PMIX_GROUP_BOOTSTRAP value failed to convert, group() logged the error and PMIX_DESTRUCT'd the signature but then fell through and kept using it -- get_tracker(), pack_signature(), and the relay all ran against a destructed signature whose groupID/members/addmembers had already been freed, a use-after-free. Every other directive-parse failure in the same loop jumps to the error label; make the bootstrap case do the same. Signed-off-by: Ralph Castain <rhc@pmix.org>
fence_recv() unpacks the incoming info array but only freed it on the non-HNP rollup path. On the HNP-complete path (which broadcasts the release via xcast) and on every intermediate contribution that does not yet complete the rollup, the array was leaked whenever the fence carried info (e.g. a PMIX_TIMEOUT). Free it once at the end of the handler; the non-HNP path already frees and NULLs it before forwarding, and PMIX_INFO_FREE is a no-op on NULL, so all paths are covered without a double free. Signed-off-by: Ralph Castain <rhc@pmix.org>
fence_recv() and grp_recv() both start by unpacking a signature into a NULL-initialized pointer. On an unpack failure the helper leaves that pointer NULL, but both handlers only logged the error and fell through to code that immediately dereferences the signature -- get_tracker(sig, ...) in the fence case, sig->op in the group case -- so a truncated or corrupt RML message crashed the daemon. Return on the failure, matching the fence_release/grp_release handlers that already do. Signed-off-by: Ralph Castain <rhc@pmix.org>
The group-signature constructor's definition carried a trailing backslash after the parameter list, splicing the opening brace onto the signature line. It was harmless but confusing; remove it. Signed-off-by: Ralph Castain <rhc@pmix.org>
Wire a new test/unit/grpcomm into make check. The collective algorithms need a live DVM and are covered by the integration/dockerswarm harnesses, but the framework's DVM-independent contract can be checked in isolation: the direct module's vtable is fully wired (every entry point is a load-bearing "MUST be provided" slot), the component is named "direct", prte_grpcomm_base.context_id starts at UINT32_MAX (the group context-id pool counts down from the top), and every signature/tracker/caddy class constructs with its documented defaults -- all rollup counters zero so a collective cannot complete before anyone reports, the group tracker's grpinfo/endpts info-lists opened -- and destructs without leaking. Signed-off-by: Ralph Castain <rhc@pmix.org>
Record the two invariants a deep review found violated: an entry-point handler owns the caddy/op it was thread-shifted and must release it on every exit (the tracker caches only the callbacks, never the caddy), and a recv handler must return on a signature unpack failure because the code below dereferences the NULL signature. Also note the new test/unit/grpcomm coverage. Signed-off-by: Ralph Castain <rhc@pmix.org>
Several error and cleanup paths in the ODLS base leaked resources or dereferenced invalid pointers. While building the launch message, get_add_procs_data leaked the PMIX_INFO_LIST and the procs argv when regex generation failed, and leaked the info array when PMIx_server_setup_application failed synchronously (its callback, which frees the array, never fires in that case). While parsing the message, construct_child_list leaked the prior-jobs data buffers when a nested load failed. spawn_proc leaked the per-child caddy (and its wdir string) on the PRTE_JOB_DO_NOT_SPAWN mapping-only path. setup_path overwrote app->cwd without freeing the previous string. Finally, if getcwd() failed at entry to launch_local, the code fell through to the shared cleanup that chdir()s to the uninitialized basedir buffer. Free or release each resource on the failing path, and bail directly from the getcwd() failure instead of using the uninitialized buffer. Signed-off-by: Ralph Castain <rhc@pmix.org>
When a per-app setup step failed during local launch - setup_fork, file pre-positioning, the executable/cwd check, or the resource-limit setup - launch_local flagged only that app's procs as failed and then jumped out of the launch loop. Any procs belonging to apps it had not yet reached were left in the INIT state. Because a daemon's errmgr only reports FAILED_TO_LAUNCH once every local proc has been accounted for (num_terminated == num_local_procs), those stranded procs prevented the report from ever being sent, hanging a multi-app launch. The per-child IOF-setup failure had the same shape, and could strand even a single-app job. Give every fatal launch error the same handling the setup_path and working-directory cases already used: flag the affected procs with PRTE_ODLS_SET_ERROR and then activate PRTE_JOB_STATE_FAILED_TO_LAUNCH for the whole job, so the job is torn down promptly and uniformly regardless of which app or proc tripped the error. This also corrects setup_fork, which recorded a proc-state constant in exit_code instead of the error code, and setup_path, which reported a file-limit error code for an unrelated failure. Signed-off-by: Ralph Castain <rhc@pmix.org>
The fork/exec/waitpid/kill lifecycle only runs against real processes inside a live DVM, but the framework's structural contract can be checked in isolation. Add test/unit/odls/test_odls.c, wired into make check, asserting that the pdefault module vtable is fully wired and reuses the base get_add_procs_data, that the component names itself "pdefault", that the PRTE_DAEMON_* command bytes are pairwise unique, that the child-error enum keeps NONE at zero with every warn code sorted after every fatal code, and that the two caddy classes construct with their documented defaults and destruct cleanly. Register the new test directory in test/unit/Makefile.am and config/prte_config_files.m4, and ignore its build products. Signed-off-by: Ralph Castain <rhc@pmix.org>
Record, in the framework guide, the gotchas behind the accompanying fixes: that every fatal launch error must abort the whole job via PRTE_ACTIVATE_JOB_STATE rather than failing only one app's procs; that the child loop variable is invalid outside the per-child loop; that every spawn_proc exit must release the caddy; that setup_path writes through to app->cwd; that the getcwd baseline must not be used before it is established; and that the blocking base functions own their lock and must not bare-return. Add a Testing section pointing at the new structural unit test and the integration smoke tests. Signed-off-by: Ralph Castain <rhc@pmix.org>
The offline mapper harness crashed with an uncaught ZeroDivisionError when a case produced output it could not parse into a map: parse_map returned an empty map, and the ranking checker then divided by the (zero) node count, aborting the entire multi-thousand-case run with a cryptic traceback instead of failing the single case. Raise a ParseError when a JOB MAP block yields no parseable nodes so the existing per-case parse-error path reports it cleanly, guard the rank-expectation helper against an empty node set, and wrap the per-case checker so any unexpected exception fails only that case and the run continues. Signed-off-by: Ralph Castain <rhc@pmix.org>
prte_event_alloc() returned a bare malloc'd, uninitialized event, but every caddy that owns one frees it in its destructor with prte_event_free() -- libevent's event_free(), which calls event_del() and dereferences the event's internal fields (ev_base et al.). On an uninitialized event those fields are garbage, so the free crashes. In normal operation this never bites because the owning objects always event_assign() the event before they are released: PRTE_ACTIVATE_LOCAL_LAUNCH assigns the odls launch-local caddy's event, and prte_timer_t assigns its event when the timer is armed. But an object that is constructed and then destructed without ever being activated -- as the odls unit test does when it exercises the launch-local caddy's destructor -- frees an uninitialized event and segfaults inside event_del(). It went unnoticed on platforms where a fresh malloc page happens to be zeroed (a zeroed event has a NULL base, which event_del() tolerates), and surfaced as a CI segfault on Linux once the allocation landed on recycled, non-zero heap. Zero-initialize the event at allocation. A zeroed event is always safe to event_free(), and a later event_assign() overwrites the zeros, so armed events are unaffected. This fixes the launch-local caddy, hardens prte_timer_t against the same latent hazard, and is strictly safer for every other caller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ralph Castain <rhc@pmix.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.