feat(platform): add heartbeat module for replica health monitoring#78
feat(platform): add heartbeat module for replica health monitoring#78lterrac wants to merge 14 commits into
Conversation
…Deployment, and service modules Ports the configuration layer (Deployment, Partition, Replica, Task, Edge, RequestManager) and three modules from the upstream reference implementation, renaming the namespace to serving throughout. Wires the new module examples into the meson build and verifies the full build is clean. - configuration/: JSON-serializable deployment graph types; Deployment.verify() checks edge connectivity, producer/consumer symmetry, and request-manager boundary constraints - channelController: reconciliation-loop module that diffs desired vs actual Input/Output channels and drives exchange/fence/initialize in one pass - broadcastDeployment: RPC-based module that distributes the serialized Deployment from the deployer instance to all workers at startup - service: thin Module wrapper around taskr::Runtime for cooperative-polling services; addService() registers external taskr::Service pointers - examples/include/: deployment helpers (readAndParseConfiguration, buildLocalChannelsFromDeployment, assignEdgeManagers) and channel helpers (createDesiredSingleLocalChannels, waitUntilReady, edge name builders) - examples/modules/: broadcastDeployment, channelController, and service example programs with policy.json fixtures and meson test targets Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add mutex locks to hasProducer/hasConsumer to prevent data race with the reconcile service thread modifying _actual* maps - Fix format string typo '%''' → '%s' in deployment verify() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The coordinator and its first replica share the same instance, so additional replicas (i > 0) each need their own distinct instance ID. The required instance count is adjusted accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- telephoneGame: call popMessage() after non-root branch consumes message - channelController: return -1 after abort() so argv[1] is never read on bad argc - service: guard memSpaces and computeResources iterators before dereferencing - partition: default _coordinatorInstanceId to 0 (safe for single-node deployments) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ort interface docs - architecture.md: instance roles, module lifecycle, channel model, deferred scope - simpler-integration.md: replica device assignment, RuntimeBinaries, processFc contract, hot-path staging and future in-device tensor channels - model-support-interface.md: what Model Support provides vs owns, RuntimePlan future contract - README.md: updated index linking all new and existing docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ica example Ports channelDispatcher, taskScheduler, roles (coordinator, replica, dependency, job, jobFactory, taskContext), and requestManager from upstream. Adds a coordinatorReplica example that demonstrates the co-located topology where a single MPI rank runs both coordinator and replica roles simultaneously. Role detection is driven by the deployment config rather than instance ID ranges, which is what makes co-location possible: the same instance can appear as coordinator AND as its first replica in the partition list. Internal coordinator-replica channels work for co-located ranks because HiCR's SPSC channels handle same-process source/target transparently. The 2-partition ring policy (2 MPI ranks) exercises the full coordinator → replica → coordinator pipeline end-to-end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- replica: copy message data before acquiring _activeJobsMutex to avoid serializing blocking memcpy/fence under lock contention - jobFactory: rename instanceId parameter to partitionIndex and use .at() for bounds-checked access; instance IDs don't map 1:1 to partition indices - job: pass dependency_t by const reference to avoid string copies on lookup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
channelDispatcher/module.hpp: - Add RAII EdgeUnlockGuard so edge->unlock() fires even if handler throws - Pop ignored messages before continuing to prevent infinite retry on same head requestManager/module.hpp: - Guard Prompt constructor against nullptr+zero-size pointer arithmetic roles/replica/module.hpp: - Add CleanupGuard RAII so context.freeOwnedOutputs() and freeInputSlots() always run on exception from _processFc or output validation; guard is dismissed only after all validation passes, before the output push loop roles/taskContext.hpp: - Return a static zero-length empty slot instead of throwing for null inputs, preserving the present-but-empty dependency contract Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…el-support quickstart Platform Python bindings (serving_platform C++ extension via pybind11): - Bridge class: co-located coordinator+replica with Python processFc callback - submit(bytes)/getResult(ms) replace mp.Queue + subprocess for Python workers - GIL released while waiting; MPI_Finalize registered with atexit - python/core/platform_bridge.py: pickle-based wrapper over the C++ Bridge - tests/test_platform_bridge.py: 4 integration tests (no NPU required), run with `mpirun -np 1 python3 -m pytest` singlePartition benchmark (C++ only, examples/modules/singlePartition/): - Coordinator + replica co-located on rank 0 - Measures direct-call baseline vs platform path to quantify framework overhead - Overhead: ~1 ms/job (channelDispatcher poll interval), negligible at ≥100 ms compute Supporting modules: - include/modules/router/: router module and defaultProcessFunction (serving:: namespace) - examples/include/runtime/moduleDeploymentRunner.hpp: deployment runner helper Docs: - docs/model-support-quickstart.md: processFc interface, deployment construction, job submission, thread safety, and benchmark interpretation - docs/README.md: link to new quickstart Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduces serving::system::RuntimePlan, an immutable value type that captures the deployment runtime state (version + per-partition replica statuses) at a point in time. Coordinator now manages Active → Draining → Removed transitions: - drainReplica(): stops dispatching new jobs to a replica - removeReplica(): marks a drained replica as fully removed - getRuntimePlan(): returns a consistent snapshot under lock dispatchReadyJobs() discards non-Active entries when dequeuing, so Draining/Removed replicas are never given new work. replicaResponseHandler() does not re-queue a replica after job completion unless it is still Active. Locks acquired sequentially (status lock released before ready-replicas lock) to avoid deadlock. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Coordinator now fires a drainCallback exactly once per replica when it goes Draining → idle (no more in-flight work, no new jobs will be assigned). Model support registers the callback via setDrainCallback() and can safely call removeReplica() and tear down channels from inside it. Two paths trigger the notification: - dispatchReadyJobs: a Draining replica popped from _readyReplicas that had no in-flight job at drain time. - replicaResponseHandler: a Draining replica whose last in-flight job just completed and is not re-queued. Exactly-once delivery is enforced by an idleNotified flag in ReplicaEntry (set under _replicaStatusMutex before the callback fires outside the lock). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_replicas map Coordinator's _replicas map is now protected by a shared_mutex so that addReplicaLive() can safely insert a new replica while the service loop is running without invalidating references held by dispatch and response handlers. Lock semantics: - unique_lock: addReplica, addReplicaLive, drainReplica, removeReplica, idleNotified writes in dispatch/response paths - shared_lock: getRuntimePlan, channel-map reads in dispatchReadyJobs - No lock inversion: dispatch holds _readyReplicasMutex then _replicasMutex; response handler and addReplicaLive never hold both simultaneously addReplicaLive() returns the new channelDispatcher subscriptions so the caller can register them before the replica starts receiving work. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…check Add three-phase co-located example demonstrating add/remove replicas at runtime using the coordinator's drain/remove/addReplicaLive lifecycle. drainReplica now fires the drain callback synchronously when the replica has no in-flight job, avoiding a deadlock where the callback could never fire if _pendingJobs was empty at drain time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Monitors replica liveness via periodic heartbeat messages. health_t tracks three states: unknown (no heartbeat received yet), healthy (recent heartbeat), unhealthy (tolerance window exceeded). A healthChangeCallback fires on every state transition so the coordinator or autoscaler can react immediately. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces the host-side control plane for PyPTO Serving, adding documentation, examples, and core platform modules (such as broadcast deployment, channel controller, service, and heartbeat), alongside Python bindings for the coordinator-replica bridge. The code review identified critical concurrency and correctness issues, including data races in the heartbeat module's shared maps and a race condition in the coordinator's response handler when processing multiple outputs concurrently. Additionally, feedback was provided to address potential undefined behavior from unchecked topology collections in runtime initialization and a resource leak vulnerability in the replica module's cleanup guard.
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.
| __INLINE__ void heartbeatMessageHandler(const instanceId_t instanceId, const message_t &message) | ||
| { | ||
| if (message.getMetadata().type != _messageType) HICR_THROW_RUNTIME("[Heartbeat] unexpected message type %lu.", message.getMetadata().type); | ||
| const auto now = std::chrono::steady_clock::now(); | ||
| const auto previous = _health[instanceId]; | ||
| _lastSeen[instanceId] = now; | ||
| _health[instanceId] = health_t::healthy; | ||
| emitHealthEvent(instanceId, previous, _health[instanceId], now); | ||
| } | ||
|
|
||
| __INLINE__ void checkTimeouts() | ||
| { | ||
| const auto now = std::chrono::steady_clock::now(); | ||
| const auto timeout = std::chrono::milliseconds(_toleranceMs); | ||
| for (const auto &[instanceId, _] : _inputs) | ||
| { | ||
| if (_lastSeen[instanceId] == std::chrono::steady_clock::time_point::min()) continue; | ||
| if (now - _lastSeen[instanceId] > timeout) | ||
| { | ||
| const auto previous = _health[instanceId]; | ||
| _health[instanceId] = health_t::unhealthy; | ||
| emitHealthEvent(instanceId, previous, _health[instanceId], now); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Data race vulnerability: heartbeatMessageHandler and checkTimeouts run concurrently on different TaskR worker threads and both read/write to _health and _lastSeen maps without any synchronization. Additionally, dynamic calls to addInput or removeInput can modify these maps and cause undefined behavior or crashes due to concurrent map rehashing. A mutex must be introduced to protect all accesses to these shared maps, and getHealthSnapshot should return the map by value under the lock to prevent data races on the returned reference.
| std::shared_ptr<Job> job = nullptr; | ||
| { | ||
| std::lock_guard lock(_jobsMutex); | ||
| job = _jobs.at(jobId); | ||
| } | ||
|
|
||
| auto &dependency = job->getOutputDependency(outputName); | ||
| if (dependency.isSatisfied()) HICR_THROW_RUNTIME("Output dependency '%s' for job %lu is already satisfied.", outputName.c_str(), jobId); | ||
| dependency.storeData(message.getData(), message.getSize()); | ||
| dependency.setSatisfied(true); | ||
|
|
||
| if (job->isFinished() == false) return; | ||
|
|
||
| std::vector<JobOutput> outputs; | ||
| outputs.reserve(job->getOutputDependencies().size()); | ||
| for (auto &[_, outputDependency] : job->getOutputDependencies()) | ||
| { | ||
| outputs.push_back(JobOutput{ | ||
| .name = outputDependency.getName(), | ||
| .data = outputDependency.getData(), | ||
| }); | ||
| } | ||
|
|
||
| { | ||
| std::lock_guard lock(_jobsMutex); | ||
| _jobs.erase(jobId); | ||
| } | ||
|
|
||
| { | ||
| std::lock_guard lock(_replicaJobsMutex); | ||
| _replicaJobs.erase(replicaId); | ||
| } |
There was a problem hiding this comment.
Race condition and correctness bug: In replicaResponseHandler, _jobsMutex is released before satisfying the job's output dependencies and checking job->isFinished(). If a task has multiple outputs, the replica sends separate messages for each output, which can be processed concurrently on different threads. This leads to concurrent writes to the same Job object and a race where both threads can see isFinished() == true, causing the completion callback to be called multiple times and _jobs / _replicaJobs to be erased multiple times. To prevent this, the dependency satisfaction, finished check, and map erasure must be performed atomically under the _jobsMutex lock, while releasing the lock before invoking the user completion callback to avoid deadlocks.
std::shared_ptr<Job> job = nullptr;
bool isFinished = false;
std::vector<JobOutput> outputs;
{
std::lock_guard lock(_jobsMutex);
if (!_jobs.contains(jobId)) return;
job = _jobs.at(jobId);
auto &dependency = job->getOutputDependency(outputName);
if (dependency.isSatisfied()) HICR_THROW_RUNTIME("Output dependency '%s' for job %lu is already satisfied.", outputName.c_str(), jobId);
dependency.storeData(message.getData(), message.getSize());
dependency.setSatisfied(true);
if (job->isFinished())
{
isFinished = true;
outputs.reserve(job->getOutputDependencies().size());
for (auto &[_, outputDependency] : job->getOutputDependencies())
{
outputs.push_back(JobOutput{
.name = outputDependency.getName(),
.data = outputDependency.getData(),
});
}
_jobs.erase(jobId);
std::lock_guard lockReplica(_replicaJobsMutex);
_replicaJobs.erase(replicaId);
}
}
if (!isFinished) return;| const auto topology = hwlocTopologyManager.queryTopology(); | ||
| auto device = *topology.getDevices().begin(); | ||
| auto memorySpaces = device->getMemorySpaceList(); | ||
| runtime.bufferMemorySpace = *memorySpaces.begin(); |
There was a problem hiding this comment.
Defensive programming check: topology.getDevices() and device->getMemorySpaceList() can be empty, which would cause undefined behavior when dereferencing .begin(). Add explicit checks to ensure these collections are not empty before accessing their elements.
const auto topology = hwlocTopologyManager.queryTopology();
if (topology.getDevices().empty()) { HICR_THROW_RUNTIME("No devices found in topology."); }
auto device = *topology.getDevices().begin();
auto memorySpaces = device->getMemorySpaceList();
if (memorySpaces.empty()) { HICR_THROW_RUNTIME("No memory spaces found on device."); }
runtime.bufferMemorySpace = *memorySpaces.begin();| // All validation passed. Dismiss the guard so outputs stay alive for the | ||
| // coordinator completion callback, which is responsible for freeing them. | ||
| guard.dismissed = true; | ||
|
|
||
| for (const auto &output : context.getOutputs()) | ||
| { | ||
| const auto response = message_t( | ||
| output.data == nullptr ? nullptr : static_cast<const uint8_t *>(output.data->getPointer()), output.data == nullptr ? 0 : output.data->getSize(), activeJob.metadata); | ||
| _outputChannels.at(output.name)->pushMessageLocking(response); | ||
| } | ||
|
|
||
| // Input slots are no longer needed once outputs have been dispatched. | ||
| freeInputSlots(activeJob); |
There was a problem hiding this comment.
Resource leak vulnerability: guard.dismissed = true; is set before pushing the response messages to the output channels. If pushMessageLocking throws an exception (e.g., due to a closed channel or network error), the CleanupGuard destructor will not free the allocated output memory slots, leading to a memory leak. The guard should only be dismissed after all output messages have been successfully dispatched.
for (const auto &output : context.getOutputs())
{
const auto response = message_t(
output.data == nullptr ? nullptr : static_cast<const uint8_t *>(output.data->getPointer()), output.data == nullptr ? 0 : output.data->getSize(), activeJob.metadata);
_outputChannels.at(output.name)->pushMessageLocking(response);
}
// All validation and dispatch passed. Dismiss the guard so outputs stay alive.
guard.dismissed = true;
// Input slots are no longer needed once outputs have been dispatched.
freeInputSlots(activeJob);
What this adds
platform/include/modules/heartbeat/module.hpp— a module that monitors replica liveness via periodic heartbeat messages.addOutputand the module'sservice()loop sends a heartbeat pulse at every intervaladdInputand the dispatcher routes incoming heartbeats toheartbeatMessageHandlerunknown(no heartbeat yet) →healthy(recent pulse received) →unhealthy(tolerance window exceeded without a pulse)healthChangeCallbackfires on every state transition, giving the coordinator or autoscaler an immediate signal to reactDependencies
Test plan
mainninja -C buildcompiles cleanunhealthy🤖 Generated with Claude Code