Scripting: resource hot-reload (dev mode)#220
Open
Segfaultd wants to merge 11 commits into
Open
Conversation
ReloadResource previously did Stop+Start without clearing any module cache, so re-executing the entry point returned stale require() exports and edits to non-entry files were silently ignored. Add Engine::EvictModulesUnderPath(rootPath): - V8Engine erases _moduleCache and _requireDataStore entries inside the resource directory (scoped, unlike global ClearModuleCache). - NodeEngine installs a privileged __fw_evictModulesUnderPath hook in the bootstrap that captures publicRequire.cache before sandboxing (which hides require.cache), and deletes CJS cache keys under the root. - ReloadResource captures the resource root, stops, evicts, then starts.
Manual hot-reload triggers (MTASA-style) on top of module eviction. - ResourceManager::RefreshResource re-parses package.json from disk, evicts cached modules, and restarts the resource (and any dependents that cascaded down) if it was running; stopped resources stay stopped. - ResourceManager::RefreshAll stops running resources, re-parses every known resource, picks up newly added directories (RescanResources), then restarts exactly those that were running. - Server Instance registers 'refresh <resource>' and 'refreshall' console commands wired to the above.
Auto-trigger RefreshResource when a running resource's files change on disk, so editing a script reloads it without a console command. - ResourceManagerConfig gains devMode + fileWatchIntervalMs. - ProcessFileWatch (called from the scripting update tick) polls each running resource's newest file mtime, throttled by the interval, and hot-reloads on change. Portable mtime polling (no OS-specific watch API); skips node_modules/.git so big trees aren't walked every tick. - devMode plumbed via InstanceOptions.developmentMode -> ServerScriptingModule::SetDevMode -> ResourceManagerConfig. Off by default; production unaffected.
In the shared runtime, re-running a resource on hot-reload left its old setTimeout/setInterval callbacks firing and duplicated them each reload. Add Engine::ClearResourceTimers(name), called from StopResource after event cleanup: - V8Engine tags each timer with the owning resource (resolved like event handlers: callback script origin, then context, then stack) and cancels matching timers. - NodeEngine wraps the global setTimeout/setInterval/clear* in a bootstrap shim that tracks handles per resource (attributed via privileged __fw_ownerOf) and cancels them via __fw_clearResourceTimers. The real timer handle is returned unchanged so clearTimeout/unref/promisify still work; one-shot timers untrack themselves on fire. Framework event listeners were already cleaned via Events::CleanupResource; this closes the raw-timer leak. Arbitrary EventEmitter listeners still need manual cleanup via resourceStop (documented limitation).
Server half of client-side hot-reload propagation. - New ResourceRefresh RPC (server -> client) carrying changed client resource names/versions, mirroring ServerResources serialization. - ResourceManager gains SetOnResourceReloaded, fired by RefreshResource and RefreshAll (distinct from start/stop so integrations react only to reloads, not normal startup). - Server Instance hooks it: rebuilds the asset streamer's upload list (ClearUploads + InitAssetStreamer) so the changed files get fresh hashes -- DirectoryDeltaTransfer compares stored hashes, so without this the edited file would look up-to-date and never re-send -- then BroadcastRPC(ResourceRefresh) for resources with a client entry point. Reuses existing MafiaNet streamer (ClearUploads/AddFile/delta transfer), no new transport.
Client half of hot-reload propagation. - Register the ResourceRefresh RPC handler. On receipt, store the changed resource names and trigger a targeted delta re-sync that, unlike the connect-time download, does NOT stop all resources or tear down web views. - When that download completes and the scripting module is already initialized, OnAssetsDownloaded refreshes just the flagged resources via the client ResourceManager's RefreshResource (module eviction + timer cancel + manifest re-parse + restart). A refresh racing an initial connect falls through to the normal full init. MAJOR: netcode change, requires matching client+server build. Not yet verified on a live 2-machine connection.
Cover enabling (developmentMode watcher + refresh/refreshall commands),
the reload sequence (stop -> timer cancel -> module eviction -> manifest
re-parse -> restart), client propagation via ResourceRefresh + asset
streamer rebuild, and limitations (CJS-only eviction, EventEmitter/
require('timers') leaks, new-resource propagation, MAJOR netcode).
Refinements from comparing to CitizenFX FiveM's resource model. - RestartResource now evicts the resource's modules between stop and start. In our shared runtime a plain stop+start re-executes against stale module caches and does NOT reload code; eviction makes restart reload code, matching per-resource-runtime engines (FiveM/MTASA) and giving error auto-restart a clean slate. ReloadResource collapses to RestartResource. - Add the 'ensure <resource>' console command (FiveM's canonical start-or-reload verb). - Document the shared-runtime vs per-resource-runtime tradeoff and recommend per-resource isolation as the long-term direction.
Generalize client propagation so it also covers resources started at runtime, not just reloads (FiveM does this; we previously only pushed changed existing resources). - Server broadcasts on any post-boot StartResource via SetOnResourceStarted (gated by a _resourcesBooted latch so boot-time StartAll doesn't push; clients get the full list on connect). This uniformly covers reload restarts, error auto-restarts, and newly started resources. Removes the now-redundant OnResourceReloaded callback. - Client handler discovers a resource it doesn't know yet from the synced cache and starts it; otherwise reloads if running / starts if stopped. Known edge: a brand-new client resource with unmet dependencies isn't graph-resolved on the client (rare hot-add case).
Complete the FiveM-style hot-reload surface. Commands (server Instance): - start / stop / restart / ensure <resource>, refresh (rescan only), refreshall (rescan + reload running). 'quit' shuts down the server; 'stop' with no arg still shuts down the server (back-compat), 'stop <resource>' stops that resource. Shared helper removes the per-verb boilerplate. ResourceManager::Rescan() backs the rescan-only refresh. Client propagation: - New ResourceStop RPC: when a client resource stops at runtime (operator stop, error-stop, or the transient stop of a reload) the server broadcasts it and clients stop the resource (no file transfer). Gated, like the start broadcast, on booted && !shuttingDown. - Fix: the client now ACCUMULATES (deduped) pending refresh resources and coalesces them into a single in-flight delta download. A reload of a resource with dependents arrives as several RPCs; the previous overwrite dropped all but the last, leaving dependents stopped.
Condense the multi-line rationale comments added across the hot-reload work to terse one/two-liners; detail lives in docs/resource_hot_reload.md. No code changes.
WalkthroughAdds end-to-end resource hot-reload support: new ChangesResource Hot-Reload System
Sequence Diagram(s)sequenceDiagram
participant DevOp as Developer/Console
participant ServerInstance
participant ResourceManager
participant AssetStreamer
participant Client
DevOp->>ServerInstance: refresh/restart/ensure command
ServerInstance->>ResourceManager: RefreshResource(name)
ResourceManager->>ResourceManager: StopResource → ClearResourceTimers → EvictModulesUnderPath
ResourceManager->>ResourceManager: re-parse package.json, rebuild dep graph, StartResource
ResourceManager->>ServerInstance: OnResourceStarted(name)
ServerInstance->>AssetStreamer: clear uploads, re-initialize
ServerInstance->>Client: ResourceRefresh RPC
Client->>Client: deduplicate into _pendingRefreshResources
Client->>Client: SyncResourceUpdatesFromServer (cancel in-flight, start delta download)
Client->>Client: OnAssetsDownloaded → RefreshResource / StartResource per pending entry
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/framework/src/integrations/client/instance.cpp`:
- Around line 397-403: The resource stop handling in the client instance path
leaves matching entries in the pending refresh queue, so a later refresh can
call StartResource and restart a server-stopped resource. Update the
ResourceStop flow in instance.cpp to remove any queued refresh/download entries
for each resource in payload.resources before calling rm->StopResource, and keep
the logic aligned with the later StartResource check so stopped resources are
not resurrected.
- Around line 597-598: The early return in the hot-reload path of the instance
cleanup flow skips the shared completion steps after
_pendingRefreshResources.clear(), so move or duplicate the common cleanup in the
same control path: ensure _downloadStatus is reset and
OnAssetsDownloadFinished(success) is invoked before returning from this branch
in the instance hot-reload handling. Use the surrounding logic in the instance
cleanup/download completion method to keep the hot-reload path consistent with
the normal completion path.
In `@code/framework/src/integrations/server/instance.cpp`:
- Around line 479-481: The RPC sends in the resource refresh paths bypass the
framework abstraction by calling net->BroadcastRPC directly. Update the relevant
send sites in Instance to route the ResourceRefresh RPC through the framework
macros instead, using FW_SEND_COMPONENT_RPC or FW_SEND_COMPONENT_RPC_TO as
appropriate, and keep the RPC construction logic intact while replacing the
direct network call.
- Around line 486-514: Stopped client resources are still being included in the
initial sync because the server-side resource state is only broadcast to
existing clients and the startup sync helpers still use the discovered client
list. Update the resource sync flow so GetClientResourceList and
InitAssetStreamer build from the currently running resources instead of all
discovered client resources, and make Instance::BroadcastResourceStop trigger a
refresh/rebuild of that running-resource state whenever a client resource is
stopped.
In `@code/framework/src/scripting/node_engine.cpp`:
- Around line 466-470: The owner lookup in the Node callback cleanup path only
uses GetResourceNameFromFunction and then falls back to an empty name, which
misses callbacks with unmappable origins. Update the owner resolution logic in
this cleanup code to follow the same fallback chain used by the V8 timer path:
try the function mapping first, then fall back to the current resource context,
and finally use the stack-based lookup before giving up. Keep the change
localized around the GetResourceManager/GetResourceNameFromFunction lookup so
both timer and cleanup paths resolve owners consistently.
- Around line 456-458: The __fw_ownerOf install in NodeEngine currently exposes
a writable, configurable global that scripts can replace, breaking timer
ownership tracking. Update the global property definition in
NodeEngine::install/__fw_ownerOf setup so it is created as read-only and
non-configurable when assigned to context->Global(), preserving the existing
function reference while preventing reassignment or deletion.
In `@code/framework/src/scripting/resource/resource_manager.cpp`:
- Around line 497-513: The hot-reload restart flow in
ResourceManager::StartResource (and the related refresh path that rebuilds the
graph then restarts dependents) bypasses the same dependency-validation gate
used by ResourceManager::StartAll, so edited manifests with cycles can recurse
indefinitely. Update these restart paths to run the dependency validation/cycle
check before calling StartResource recursively, and fail cleanly with an error
result instead of continuing into the restart loop.
- Around line 488-496: Reject manifest renames that would overwrite an existing
registered resource by adding an explicit collision check in the refresh path
before updating _resources. In ResourceManager::refresh and the other matching
refresh block, compare reparsed->GetName() against the oldName under
_resourcesMutex; if the new name already exists for a different resource, abort
the rename/update instead of erasing the old key and assigning to
_resources[newName]. Make the check cover both refresh paths so a package.json
rename cannot silently replace another resource’s entry or orphan its state.
- Around line 517-518: `RefreshAll()` is preserving the pre-reload names in
`running`, but after the manifests are reparsed and `_resources` is rebuilt
those names may be stale, so renamed resources can be missed. Update the restart
logic in `RefreshAll()` (and the related restart loop around the referenced
block) to resolve each previously running resource against the new `_resources`
state, using stable identity or manifest-derived matching rather than the old
name, so resources that were renamed during reparse are restarted correctly.
- Around line 425-449: RestartResource currently stops with the default cascade
behavior but only restarts the named resource, so any dependents stopped by
StopResource remain offline. Update ResourceManager::RestartResource to either
stop without cascading when a single-resource restart is intended, or collect
and restart the full set of affected dependents after the stop. Keep the module
eviction around the same restart flow, and use the existing RestartResource,
StopResource, and StartResource paths to ensure all resources that were brought
down are brought back up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 735ea1f7-3c8f-43c1-8dd7-44f85ff82e84
📒 Files selected for processing (16)
code/framework/src/integrations/client/instance.cppcode/framework/src/integrations/client/instance.hcode/framework/src/integrations/server/instance.cppcode/framework/src/integrations/server/instance.hcode/framework/src/integrations/server/scripting/module.cppcode/framework/src/integrations/server/scripting/module.hcode/framework/src/networking/rpc/resource_refresh.hcode/framework/src/scripting/engine.hcode/framework/src/scripting/node_engine.cppcode/framework/src/scripting/node_engine.hcode/framework/src/scripting/resource/resource_manager.cppcode/framework/src/scripting/resource/resource_manager.hcode/framework/src/scripting/v8_engine.cppcode/framework/src/scripting/v8_engine.hcode/framework/src/scripting/v8_engine_callbacks.hdocs/resource_hot_reload.md
Comment on lines
+397
to
+403
| for (const auto &res : payload.resources) { | ||
| if (rm->IsResourceRunning(res.name)) { | ||
| auto result = rm->StopResource(res.name); | ||
| if (!result) { | ||
| Logging::GetLogger(FRAMEWORK_INNER_CLIENT)->warn("Failed to stop client resource '{}': {}", res.name, result.GetError()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove stopped resources from the pending refresh queue.
If ResourceStop arrives while a refresh download is pending, Line 591 later sees the resource as stopped and calls StartResource, resurrecting a server-stopped resource. Drop matching pending refresh entries before stopping.
🐛 Proposed fix
for (const auto &res : payload.resources) {
+ for (auto it = _pendingRefreshResources.begin(); it != _pendingRefreshResources.end();) {
+ if (it->name == res.name) {
+ it = _pendingRefreshResources.erase(it);
+ }
+ else {
+ ++it;
+ }
+ }
if (rm->IsResourceRunning(res.name)) {
auto result = rm->StopResource(res.name);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const auto &res : payload.resources) { | |
| if (rm->IsResourceRunning(res.name)) { | |
| auto result = rm->StopResource(res.name); | |
| if (!result) { | |
| Logging::GetLogger(FRAMEWORK_INNER_CLIENT)->warn("Failed to stop client resource '{}': {}", res.name, result.GetError()); | |
| } | |
| } | |
| for (const auto &res : payload.resources) { | |
| for (auto it = _pendingRefreshResources.begin(); it != _pendingRefreshResources.end();) { | |
| if (it->name == res.name) { | |
| it = _pendingRefreshResources.erase(it); | |
| } | |
| else { | |
| +it; | |
| } | |
| } | |
| if (rm->IsResourceRunning(res.name)) { | |
| auto result = rm->StopResource(res.name); | |
| if (!result) { | |
| Logging::GetLogger(FRAMEWORK_INNER_CLIENT)->warn("Failed to stop client resource '{}': {}", res.name, result.GetError()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/integrations/client/instance.cpp` around lines 397 - 403,
The resource stop handling in the client instance path leaves matching entries
in the pending refresh queue, so a later refresh can call StartResource and
restart a server-stopped resource. Update the ResourceStop flow in instance.cpp
to remove any queued refresh/download entries for each resource in
payload.resources before calling rm->StopResource, and keep the logic aligned
with the later StartResource check so stopped resources are not resurrected.
Comment on lines
+597
to
+598
| _pendingRefreshResources.clear(); | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run completion cleanup before returning from hot reloads.
This early return skips the common _downloadStatus = {} reset and OnAssetsDownloadFinished(success) callback below, leaving mod-level code uninformed after hot-reload downloads.
🐛 Proposed fix
}
_pendingRefreshResources.clear();
+ Logging::GetLogger(FRAMEWORK_INNER_CLIENT)->flush();
+ _downloadStatus = {};
+ OnAssetsDownloadFinished(success);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _pendingRefreshResources.clear(); | |
| return; | |
| _pendingRefreshResources.clear(); | |
| Logging::GetLogger(FRAMEWORK_INNER_CLIENT)->flush(); | |
| _downloadStatus = {}; | |
| OnAssetsDownloadFinished(success); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/integrations/client/instance.cpp` around lines 597 - 598,
The early return in the hot-reload path of the instance cleanup flow skips the
shared completion steps after _pendingRefreshResources.clear(), so move or
duplicate the common cleanup in the same control path: ensure _downloadStatus is
reset and OnAssetsDownloadFinished(success) is invoked before returning from
this branch in the instance hot-reload handling. Use the surrounding logic in
the instance cleanup/download completion method to keep the hot-reload path
consistent with the normal completion path.
Comment on lines
+479
to
+481
| Framework::Networking::RPC::ResourceRefresh refresh; | ||
| refresh.resources.push_back({resource->GetName(), resource->GetVersion()}); | ||
| net->BroadcastRPC(refresh); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the framework RPC send macros here.
These new RPC sends bypass the required framework abstraction. Please route them through the RPC macros instead of calling BroadcastRPC(...) directly. As per coding guidelines, code/**/*.{cpp,hpp,h}: Use FW_SEND_COMPONENT_RPC(rpc, ...) and FW_SEND_COMPONENT_RPC_TO(rpc, guid, ...) macros for network communication in the RPC system.
Also applies to: 509-511
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/integrations/server/instance.cpp` around lines 479 - 481,
The RPC sends in the resource refresh paths bypass the framework abstraction by
calling net->BroadcastRPC directly. Update the relevant send sites in Instance
to route the ResourceRefresh RPC through the framework macros instead, using
FW_SEND_COMPONENT_RPC or FW_SEND_COMPONENT_RPC_TO as appropriate, and keep the
RPC construction logic intact while replacing the direct network call.
Source: Coding guidelines
Comment on lines
+486
to
+514
| void Instance::BroadcastResourceStop(const std::string &name) { | ||
| if (!_scriptingModule || !_networkingEngine) { | ||
| return; | ||
| } | ||
| auto *rm = _scriptingModule->GetResourceManager(); | ||
| if (!rm) { | ||
| return; | ||
| } | ||
| const auto *resource = rm->GetResource(name); | ||
| if (!resource) { | ||
| return; | ||
| } | ||
| // Only client resources need a client-side stop. | ||
| if (resource->GetManifest().GetMafiaHubConfig().client.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| const auto net = _networkingEngine->GetNetworkServer(); | ||
| if (!net) { | ||
| return; | ||
| } | ||
|
|
||
| // No streamer rebuild: stopping ships no files. | ||
| Framework::Networking::RPC::ResourceStop stop; | ||
| stop.resources.push_back({resource->GetName(), resource->GetVersion()}); | ||
| net->BroadcastRPC(stop); | ||
|
|
||
| Logging::GetLogger(FRAMEWORK_INNER_SERVER)->info("Broadcasting stop of resource '{}' to clients", name); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Stopped resources still remain in the initial sync path.
BroadcastResourceStop() only notifies current clients. New clients still get the stopped resource afterward, because the initial sync is built from GetClientResourceList() and InitAssetStreamer(), and both helpers shown in this PR still enumerate discovered client resources instead of the running set. After stop <resource>, reconnecting clients can re-download/restart a resource the server has already stopped. Please make the initial resource list and streamer contents derive from running resources, and rebuild that state when a client resource stops.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/integrations/server/instance.cpp` around lines 486 - 514,
Stopped client resources are still being included in the initial sync because
the server-side resource state is only broadcast to existing clients and the
startup sync helpers still use the discovered client list. Update the resource
sync flow so GetClientResourceList and InitAssetStreamer build from the
currently running resources instead of all discovered client resources, and make
Instance::BroadcastResourceStop trigger a refresh/rebuild of that
running-resource state whenever a client resource is stopped.
Comment on lines
+456
to
+458
| v8::Local<v8::String> key = v8::String::NewFromUtf8( | ||
| _isolate, "__fw_ownerOf").ToLocalChecked(); | ||
| context->Global()->Set(context, key, fn).Check(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 '__fw_ownerOf|InstallResourceTimerTracking|DefineOwnProperty' code/framework/src/scriptingRepository: MafiaHub/Framework
Length of output: 5372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any vendored V8 headers or docs that define the property-install APIs.
fd -a 'v8.h' . || true
fd -a 'object.h' . || true
fd -a 'property-attributes.h' . || true
# Search for the exact APIs and any existing usage of immutable globals in the repo.
rg -n -C3 'DefineOwnProperty\(|Set\(context, key, fn\)|ReadOnly \| DontEnum \| DontDelete|DontDelete|DontEnum|ReadOnly' .
# Inspect the relevant source slice around the timer helper installation.
sed -n '446,462p' code/framework/src/scripting/node_engine.cppRepository: MafiaHub/Framework
Length of output: 38730
🌐 Web query:
V8 Object::DefineOwnProperty PropertyAttribute ReadOnly DontDelete DontEnum API
💡 Result:
In the V8 C++ API, the v8::Object::DefineOwnProperty method is used to implement the [[DefineOwnProperty]] internal method for data properties [1][2]. This method allows you to define a property on an object with specific attributes by passing a v8::PropertyAttribute enum value [1][3]. The v8::PropertyAttribute enum defines the following flags, which can be combined using bitwise OR operations [1][4]: None (0): No special attributes; the property is writable, enumerable, and configurable [1][3]. ReadOnly (1 << 0): The property value cannot be changed (not writable) [1][3]. DontEnum (1 << 1): The property will not appear in property enumerations like for...in loops (not enumerable) [1][3]. DontDelete (1 << 2): The property cannot be deleted and its attributes cannot be changed (not configurable) [1][3]. The method signature is defined as [1][2]: v8::Maybe DefineOwnProperty( v8::Localv8::Context context, v8::Localv8::Name key, v8::Localv8::Value value, v8::PropertyAttribute attributes = v8::None); While DefineOwnProperty allows for attribute specification, V8 documentation notes that CreateDataProperty is generally faster when you do not need to specify custom attributes [1][4]. If you need to define more complex properties (such as accessors), you should use the DefineProperty method, which accepts a v8::PropertyDescriptor object [2][4].
Citations:
- 1: https://chromium.googlesource.com/v8/v8/+/refs/heads/main/include/v8-object.h
- 2: https://v8.github.io/api/head/classv8_1_1Object.html
- 3: https://chromium.googlesource.com/v8/v8/+/af1d043c21512eab93ea973148972ce87149fdad/include/v8-object.h
- 4: https://github.com/nodejs/node/blob/main/deps/v8/include/v8-object.h
Make __fw_ownerOf read-only and non-configurable on install. globalThis.__fw_ownerOf is consulted for every timer wrapper call, so a script can overwrite it and break owner tracking and timer cleanup on stop.
Proposed hardening
v8::Local<v8::String> key = v8::String::NewFromUtf8(
_isolate, "__fw_ownerOf").ToLocalChecked();
- context->Global()->Set(context, key, fn).Check();
+ context->Global()
+ ->DefineOwnProperty(
+ context,
+ key,
+ fn,
+ static_cast<v8::PropertyAttribute>(
+ v8::ReadOnly | v8::DontEnum | v8::DontDelete))
+ .Check();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| v8::Local<v8::String> key = v8::String::NewFromUtf8( | |
| _isolate, "__fw_ownerOf").ToLocalChecked(); | |
| context->Global()->Set(context, key, fn).Check(); | |
| v8::Local<v8::String> key = v8::String::NewFromUtf8( | |
| _isolate, "__fw_ownerOf").ToLocalChecked(); | |
| context->Global() | |
| ->DefineOwnProperty( | |
| context, | |
| key, | |
| fn, | |
| static_cast<v8::PropertyAttribute>( | |
| v8::ReadOnly | v8::DontEnum | v8::DontDelete)) | |
| .Check(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/scripting/node_engine.cpp` around lines 456 - 458, The
__fw_ownerOf install in NodeEngine currently exposes a writable, configurable
global that scripts can replace, breaking timer ownership tracking. Update the
global property definition in NodeEngine::install/__fw_ownerOf setup so it is
created as read-only and non-configurable when assigned to context->Global(),
preserving the existing function reference while preventing reassignment or
deletion.
Comment on lines
+466
to
+470
| std::string name; | ||
| if (info.Length() > 0 && info[0]->IsFunction() && engine->GetResourceManager()) { | ||
| name = engine->GetResourceManager()->GetResourceNameFromFunction( | ||
| isolate, info[0].As<v8::Function>()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the same owner fallback chain as the V8 timer path.
Node timer cleanup misses callbacks whose function origin cannot be mapped, such as bound/wrapped callbacks. Fall back to current resource context and stack lookup before returning an empty owner.
Proposed fix
std::string name;
- if (info.Length() > 0 && info[0]->IsFunction() && engine->GetResourceManager()) {
- name = engine->GetResourceManager()->GetResourceNameFromFunction(
- isolate, info[0].As<v8::Function>());
+ if (auto *mgr = engine->GetResourceManager()) {
+ if (info.Length() > 0 && info[0]->IsFunction()) {
+ name = mgr->GetResourceNameFromFunction(
+ isolate, info[0].As<v8::Function>());
+ }
+ if (name.empty()) {
+ name = mgr->GetCurrentResourceContext();
+ }
+ if (name.empty()) {
+ name = mgr->GetResourceContextFromStack(isolate);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| std::string name; | |
| if (info.Length() > 0 && info[0]->IsFunction() && engine->GetResourceManager()) { | |
| name = engine->GetResourceManager()->GetResourceNameFromFunction( | |
| isolate, info[0].As<v8::Function>()); | |
| } | |
| std::string name; | |
| if (auto *mgr = engine->GetResourceManager()) { | |
| if (info.Length() > 0 && info[0]->IsFunction()) { | |
| name = mgr->GetResourceNameFromFunction( | |
| isolate, info[0].As<v8::Function>()); | |
| } | |
| if (name.empty()) { | |
| name = mgr->GetCurrentResourceContext(); | |
| } | |
| if (name.empty()) { | |
| name = mgr->GetResourceContextFromStack(isolate); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/scripting/node_engine.cpp` around lines 466 - 470, The
owner lookup in the Node callback cleanup path only uses
GetResourceNameFromFunction and then falls back to an empty name, which misses
callbacks with unmappable origins. Update the owner resolution logic in this
cleanup code to follow the same fallback chain used by the V8 timer path: try
the function mapping first, then fall back to the current resource context, and
finally use the stack-based lookup before giving up. Keep the change localized
around the GetResourceManager/GetResourceNameFromFunction lookup so both timer
and cleanup paths resolve owners consistently.
Comment on lines
425
to
449
| ResourceOperationResult ResourceManager::RestartResource(std::string_view name) { | ||
| // Evict the resource's modules between stop and start: in a shared | ||
| // runtime a plain stop+start would re-run against stale caches and not | ||
| // reload code. See docs/resource_hot_reload.md. | ||
| std::string resourceRoot; | ||
| { | ||
| std::scoped_lock lock(_resourcesMutex); | ||
| auto it = _resources.find(name); | ||
| if (it != _resources.end() && it->second) { | ||
| resourceRoot = it->second->GetPath(); | ||
| } | ||
| } | ||
|
|
||
| auto stopResult = StopResource(name); | ||
| if (!stopResult) { | ||
| return stopResult; | ||
| } | ||
|
|
||
| // Evict only after stop (engine cache contract). | ||
| if (_jsEngine && !resourceRoot.empty()) { | ||
| _jsEngine->EvictModulesUnderPath(resourceRoot); | ||
| } | ||
|
|
||
| return StartResource(name); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restarting one resource can leave cascaded dependents offline.
StopResource(name) cascades by default, but this path only starts name again. Any running dependent stopped by that cascade stays down after restart.
Possible fix
auto stopResult = StopResource(name);
if (!stopResult) {
return stopResult;
}
// Evict only after stop (engine cache contract).
if (_jsEngine && !resourceRoot.empty()) {
_jsEngine->EvictModulesUnderPath(resourceRoot);
}
- return StartResource(name);
+ auto toRestart = stopResult.GetValue();
+ if (toRestart.empty()) {
+ toRestart.push_back(std::string(name));
+ }
+
+ std::vector<std::string> restarted;
+ for (const auto &stoppedName : toRestart) {
+ auto result = StartResource(stoppedName);
+ if (!result) {
+ return result;
+ }
+ const auto &names = result.GetValue();
+ restarted.insert(restarted.end(), names.begin(), names.end());
+ }
+ return ResourceOperationResult::Ok(restarted);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ResourceOperationResult ResourceManager::RestartResource(std::string_view name) { | |
| // Evict the resource's modules between stop and start: in a shared | |
| // runtime a plain stop+start would re-run against stale caches and not | |
| // reload code. See docs/resource_hot_reload.md. | |
| std::string resourceRoot; | |
| { | |
| std::scoped_lock lock(_resourcesMutex); | |
| auto it = _resources.find(name); | |
| if (it != _resources.end() && it->second) { | |
| resourceRoot = it->second->GetPath(); | |
| } | |
| } | |
| auto stopResult = StopResource(name); | |
| if (!stopResult) { | |
| return stopResult; | |
| } | |
| // Evict only after stop (engine cache contract). | |
| if (_jsEngine && !resourceRoot.empty()) { | |
| _jsEngine->EvictModulesUnderPath(resourceRoot); | |
| } | |
| return StartResource(name); | |
| } | |
| ResourceOperationResult ResourceManager::RestartResource(std::string_view name) { | |
| // Evict the resource's modules between stop and start: in a shared | |
| // runtime a plain stop+start would re-run against stale caches and not | |
| // reload code. See docs/resource_hot_reload.md. | |
| std::string resourceRoot; | |
| { | |
| std::scoped_lock lock(_resourcesMutex); | |
| auto it = _resources.find(name); | |
| if (it != _resources.end() && it->second) { | |
| resourceRoot = it->second->GetPath(); | |
| } | |
| } | |
| auto stopResult = StopResource(name); | |
| if (!stopResult) { | |
| return stopResult; | |
| } | |
| // Evict only after stop (engine cache contract). | |
| if (_jsEngine && !resourceRoot.empty()) { | |
| _jsEngine->EvictModulesUnderPath(resourceRoot); | |
| } | |
| auto toRestart = stopResult.GetValue(); | |
| if (toRestart.empty()) { | |
| toRestart.push_back(std::string(name)); | |
| } | |
| std::vector<std::string> restarted; | |
| for (const auto &stoppedName : toRestart) { | |
| auto result = StartResource(stoppedName); | |
| if (!result) { | |
| return result; | |
| } | |
| const auto &names = result.GetValue(); | |
| restarted.insert(restarted.end(), names.begin(), names.end()); | |
| } | |
| return ResourceOperationResult::Ok(restarted); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/scripting/resource/resource_manager.cpp` around lines 425
- 449, RestartResource currently stops with the default cascade behavior but
only restarts the named resource, so any dependents stopped by StopResource
remain offline. Update ResourceManager::RestartResource to either stop without
cascading when a single-resource restart is intended, or collect and restart the
full set of affected dependents after the stop. Keep the module eviction around
the same restart flow, and use the existing RestartResource, StopResource, and
StartResource paths to ensure all resources that were brought down are brought
back up.
Comment on lines
+488
to
+496
| const std::string oldName = std::string(name); | ||
| std::string newName = reparsed->GetName(); | ||
| { | ||
| std::scoped_lock lock(_resourcesMutex); | ||
| if (newName != oldName) { | ||
| _resources.erase(oldName); | ||
| } | ||
| _resources[newName] = std::move(reparsed); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject manifest renames that collide with another registered resource.
Both refresh paths erase the old key and then assign into _resources[newName] without checking whether newName already belongs to a different resource. A package.json rename from a to b will silently replace the existing b entry and orphan its state/path.
Also applies to: 545-550
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/scripting/resource/resource_manager.cpp` around lines 488
- 496, Reject manifest renames that would overwrite an existing registered
resource by adding an explicit collision check in the refresh path before
updating _resources. In ResourceManager::refresh and the other matching refresh
block, compare reparsed->GetName() against the oldName under _resourcesMutex; if
the new name already exists for a different resource, abort the rename/update
instead of erasing the old key and assigning to _resources[newName]. Make the
check cover both refresh paths so a package.json rename cannot silently replace
another resource’s entry or orphan its state.
Comment on lines
+497
to
+513
| BuildDependencyGraph(); | ||
|
|
||
| if (!wasRunning) { | ||
| return ResourceOperationResult::Ok({newName}); | ||
| } | ||
|
|
||
| // Restart the resource plus the dependents that cascaded down. | ||
| std::vector<std::string> affected; | ||
| for (const auto &stoppedName : toRestart) { | ||
| const std::string startName = (stoppedName == oldName) ? newName : stoppedName; | ||
| auto result = StartResource(startName); | ||
| if (result) { | ||
| const auto &a = result.GetValue(); | ||
| affected.insert(affected.end(), a.begin(), a.end()); | ||
| } | ||
| } | ||
| return ResourceOperationResult::Ok(affected); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
These hot-reload restart paths skip the dependency-validation gate.
StartAll() validates dependencies and detects cycles before starting, but both refresh paths rebuild the graph and then recurse straight through StartResource(). If an edited manifest introduces a dependency cycle, this can recurse until stack overflow instead of failing cleanly.
Also applies to: 553-566
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/scripting/resource/resource_manager.cpp` around lines 497
- 513, The hot-reload restart flow in ResourceManager::StartResource (and the
related refresh path that rebuilds the graph then restarts dependents) bypasses
the same dependency-validation gate used by ResourceManager::StartAll, so edited
manifests with cycles can recurse indefinitely. Update these restart paths to
run the dependency validation/cycle check before calling StartResource
recursively, and fail cleanly with an error result instead of continuing into
the restart loop.
Comment on lines
+517
to
+518
| // Remember who was running so we restart exactly them. | ||
| auto running = GetRunningResourceNames(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
RefreshAll() drops running resources that were renamed during the reparse.
running is captured before the manifests are reloaded, but the restart loop still uses those old names after _resources has been rewritten. A previously running resource that renamed itself will never be started again here.
Also applies to: 545-560
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/scripting/resource/resource_manager.cpp` around lines 517
- 518, `RefreshAll()` is preserving the pre-reload names in `running`, but after
the manifests are reparsed and `_resources` is rebuilt those names may be stale,
so renamed resources can be missed. Update the restart logic in `RefreshAll()`
(and the related restart loop around the referenced block) to resolve each
previously running resource against the new `_resources` state, using stable
identity or manifest-derived matching rather than the old name, so resources
that were renamed during reparse are restarted correctly.
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
2 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.
Resource hot-reload
Adds development-mode hot-reload for JavaScript resources, end to end: edit a resource on the server and it reloads in place — and propagates to connected clients — without a server restart. Modelled on FiveM/MTASA's resource lifecycle (verified against CitizenFX's
citizen-server-impl).See
docs/resource_hot_reload.mdfor the full write-up.Why it's more than "stop + start"
FiveM and MTASA run each resource in its own runtime (Lua state / V8 isolate / Mono domain), so reload just drops the runtime. This framework shares one Node runtime across all resources, so a plain stop+start re-executes against stale module caches and leaks timers/listeners. Most of this PR is the machinery to compensate for that shared runtime.
What's included
Reload core (server-local)
Engine::EvictModulesUnderPath).Engine::ClearResourceTimers) — V8 timers tagged by owner; Node globalsetTimeout/setIntervalwrapped by a bootstrap shim that tracks handles per resource (real handle returned, soclearTimeout/unref/util.promisifykeep working).RestartResourcenow evicts modules, so restart actually reloads code.InstanceOptions.developmentMode), portable mtime poll (no OS-specific watch API), throttled, skipsnode_modules/.git.Console commands (FiveM-style):
start/stop/restart/ensure/refresh(rescan) /refreshall.quitshuts down the server; barestopstill shuts down the server (back-compat),stop <resource>stops a resource.Client propagation
ResourceRefresh/ResourceStopRPCs. On any post-boot resource start/stop, the server rebuilds the asset-streamer hashes (so MafiaNet's delta transfer actually re-sends edited files) and broadcasts to clients.Versioning
The client propagation is a netcode change (new RPCs, both client and server) → MAJOR; requires matching client and server builds. The server-only pieces don't touch the wire protocol.
Limitations (documented)
import().EventEmitterlisteners andrequire('timers')aren't auto-cleaned — useresourceStop.Recommended follow-up
Per-resource V8 isolation would eliminate the eviction/timer/listener machinery wholesale — the property FiveM/MTASA rely on. Not in this PR (a larger engine rework); noted in the docs as the strategic direction.
Testing
Warning
Not yet built or run. The server-only pieces need a
FrameworkServer+ libnode build; the client propagation path needs a 2-machine smoke test (matching client+server builds). No automated tests added.Summary by CodeRabbit
New Features
Bug Fixes
Documentation