feat(core): add durable server and indexing activation - #114
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cdd55b192
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
kunkunGames
changed the title
1cdd55b to
5c5fe7a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c5fe7a6a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5c5fe7a to
bebc7b1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bebc7b13ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9d3e222 to
996b6f6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 996b6f63c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
996b6f6 to
8fc4f8e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fc4f8e1a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8fc4f8e to
2232c7c
Compare
…reconciler state Three defects found in a follow-up self-review of this branch. `ProbePort` returned false whenever `FSocket::SetNonBlocking(true)` failed, because the whole result was gated on `bNonBlocking`. Probe *setup* failure was therefore indistinguishable from "port not listening": on a platform that cannot switch the socket to non-blocking, `Start()` would exhaust every attempt against a listener that had actually bound, and the server could never start. The bounded non-blocking path stays as the fast path; a blocking `Connect` — which is what master did — is now the fallback. `HandleReindex` read a bool out of a `ProcessEvent` parameter buffer without checking the reflected signature. `MonolithCore` reaches `MonolithIndex` only through reflection and has no compile-time dependency on it, so if `StartFullIndex` / `StartIncrementalIndex` ever stopped returning bool, the zeroed buffer would read false and the action would report `reindex_not_started` for work that actually started. It now requires an `FBoolProperty` return and reports an explicit module-sync error otherwise, rather than guessing from an untyped buffer. `ReconcileHttpServerActivation` carried an unreachable first-tick branch: `StartupModule` sets `bHasResolvedServerActivation = true` before `AddTicker`, so the ticker could never observe it false. Removed the branch and the flag; the baseline is resolved before the ticker exists, which is what the code already did. Also documents, rather than changes, the fact that `Monolith.StopServer` cannot release the OS listener — UE exposes no per-port teardown — so a port-based liveness check is not a valid readiness signal after a Stop; the sentinel is. Verified at this head on both engines: - UE 5.7: editor build Succeeded, `Monolith.Activation` + `Monolith.Source` 15/15 under `-RenderOffscreen`, 0 failed, exit 0. - UE 5.8: editor build Succeeded, same suite 15/15, 0 failed, exit 0.
|
Self-review pass on this branch before it reaches you. Three defects found and fixed in
Dead state in the reconciler. Documented rather than changed: One known residue I deliberately left alone: sentinel removal is ownership-gated, so a sentinel left behind by a crashed process is not cleaned up by a later editor that starts with activation off, because that editor never takes ownership. The gating is still right — it's what stops one editor deleting another live editor's sentinel — but a stale-sentinel reaper belongs with the startup path, not this change. Happy to add it here if you'd rather it ship together. Verified at head
Full record in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c438e6ffd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ned listener Two defects from the review pass on the previous commit. `ReadConfigFile` collapsed "file absent" and "file exists but unreadable" into the same empty `FConfigFile`, because `FConfigFile::Read` returns void and its result was never probed. Absent correctly means "no user override, inherit the project default" — but the defaults are enabled, so an unreadable file silently re-enabled a server the user had persistently stopped, and the activation ticker would then start it mid-session. Readability is now probed explicitly and the three states are distinct: an unreadable user file fails closed with both services disabled, exactly like a malformed explicit value, and is neither migrated nor deleted. The same conflation was worse on the write path: `SetActivationInFileUnlocked` read, set one key, and wrote. With an unreadable file that produced a config containing only the key being set, dropping the other one and reverting that service to its enabled default. It now refuses the write with an explicit error rather than persisting a file that discards state it could not read. Second: a `MonolithCore` unload/reload left the port unstartable for the rest of the process. Shutdown unbinds routes but deliberately leaves UE's listener up, since UE exposes no per-port teardown, while the replacement `FMonolithHttpServer` has no router ownership — so the pre-bind check saw Monolith's own retained listener and treated it as a foreign owner, refusing persistent activation and `Monolith.StartServer` until the editor exited. `FHttpServerModule` outlives the reload, so its listener map is the authority: `GetHttpRouter` returns the existing in-process listener's router without rebinding, and yields nothing for a port held by another process. Telling those two apart costs one rejected bind on the foreign-owner path, which UE logs at Error level. That only happens where startup was going to fail anyway, so `Monolith.Activation.OccupiedServerPort` now expects it. Adds `Monolith.Activation.ReloadReclaimsRetainedListener`, which starts an instance, destroys it, confirms the UE listener survives, and asserts a fresh instance reclaims that port. Verified at this head on both engines: editor builds succeed and `Monolith.Activation` (6) + `Monolith.Source` (10) report 16/16 Success with 0 failed and exit 0 on UE 5.7 (`-RenderOffscreen`) and UE 5.8.
|
Both new findings were real and are fixed in Unreadable activation file failed open. Confirmed and worse than reported. The write path had the same conflation with a worse outcome, which the report didn't cover: Retained listener after a module reload. Confirmed. Shutdown unbinds routes but deliberately leaves UE's listener up, and the replacement instance has no router ownership, so the pre-bind check treated Monolith's own listener as a foreign owner and refused every start for the rest of the process.
One honest cost: telling the two cases apart takes one rejected bind on the foreign-owner path, and UE logs that at New test Verified at
One environment note worth recording, since it cost me a false failure: an automation host left on the default |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7bf48e000
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…uild a locked DB Two defects surfaced by review of the previous commit. The completion handler captured raw `this`. The indexer broadcasts from its worker thread and the handler hops to the game thread, so `Deinitialize()` could run in between: it closes the database and deletes the indexer, and the queued task then called `ReopenDatabase()` on a torn-down subsystem — reopening a closed database at best, a use-after-free or module-unload crash at worst. This was survivable before only because indexing started on explicit request; this branch starts a catch-up run from `Initialize()`, so a fast editor close now lands squarely in that window. The handler now holds a `TWeakObjectPtr` and re-checks a `bIsShuttingDown` flag set at the top of `Deinitialize()`, and `Deinitialize()` clears `OnComplete` before destroying the indexer so no further broadcast can reach it at all. `StartPreferredIndex` also treated "database file missing" and "database file present but not open" identically, falling through to `TriggerReindex()` on an explicit activation. That is a CLEAN build: it calls `ResetDatabase()` and drops the existing engine index. A transient lock — another editor holding the file — would therefore be silently converted into a destructive multi-minute rebuild, leaving a partial database if interrupted. Full bootstrap is now reserved for a genuinely absent file; an existing-but-unopenable database reports an explicit error and leaves the index intact. Verified at this head on both engines: editor builds succeed and `Monolith.Activation` (6) + `Monolith.Source` (10) report 16/16 with 0 failed and exit 0 on UE 5.7 (`-RenderOffscreen`) and UE 5.8.
|
Both findings from the pass on P1 — completion landing on a torn-down subsystem. Confirmed, and this branch is what made it reachable. The handler captured raw The pre-existing code survived this because indexing only ever started on explicit request. This branch starts a catch-up run from
P2 — destructive rebuild on a locked database. Confirmed. Full bootstrap is now reserved for a genuinely absent file. An existing-but-unopenable database returns an explicit error and leaves the index intact: if (bDatabaseFileExists && Database.IsValid() && Database->IsOpen())
{
return TriggerProjectReindex();
}
if (bDatabaseFileExists)
{
// refuse the clean rebuild, report, keep the index
return false;
}
if (bAllowFullBootstrap) { return TriggerReindex(); }Verified at
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d977baa2df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…on any outcome Startup hard-coded `bExplicitRequest=false` when applying `bDeferFirstTimeIndex`, so a persisted `Monolith.StartIndexing` was treated like an inherited default. An explicitly enabled first-time index that did not finish before the editor exited was therefore re-deferred on every subsequent launch, even though the durable activation stayed enabled. It now passes `Activation.bIndexingUserSet`, matching what the source subsystem already does for its bootstrap decision. Live Asset Registry callbacks were re-armed only when a full index finished successfully. `StartFullIndex` unregisters them for the duration of the run, so a cancelled or failed run left them off while `bAutomaticIndexingEnabled` stayed true: the subsystem reported itself active while silently dropping every later asset change until a successful reindex, another Start command, or an editor restart. They are now re-armed on every outcome. `RegisterLiveCallbacks()` is already self-guarding on effective activation, active-run state, and database readiness, and is idempotent, so a run completing after indexing was genuinely deactivated still leaves them off. Also closes the same use-after-free class that was just fixed in the source subsystem. The index worker queues `OnIndexingFinished` to the game thread capturing raw `this`/`Owner`. `Deinitialize()` joins the worker, which is what makes every worker-thread `Owner->` access safe, but a task queued immediately before that join outlives it and can land on a destroyed subsystem. Both queue sites now resolve the subsystem through a `TWeakObjectPtr` and drop the completion if it is gone. Verified at this head on both engines: editor builds succeed and `Monolith.Activation` + `Monolith.Source` + `Monolith.Index` report 16/16 with 0 failed and exit 0 on UE 5.7 (`-RenderOffscreen`) and UE 5.8.
|
Both findings on Explicit activation lost to Callbacks not restored after an aborted full index. Confirmed and the worse half is the silence: Third, found by looking for the pattern rather than reported: the index worker queues I deliberately did not widen that to the rest of Verified at
|
|
Offering a split before you spend review time on this, since you staged #113 rather than taking it whole and this branch is larger. Four review rounds have now produced ten fixes on this branch. None were in the activation logic itself — they were in the listener lifecycle, the writer gating, and two use-after-free windows on shutdown. That pattern says the breadth is the problem, not any one piece, so here is how it separates. Dependencies are real, not cosmetic: 1. Listener lifecycle — independent, no activation needed.
This is the slice I would land first if you only take one. It fixes shipped behavior and needs nothing else in the branch. 2. Durable activation state — foundation. 3. Server activation + console commands. 4. Indexing activation. The two subsystems, So: 1 alone, then 2 → 3 and 4 in parallel. Say the word and I will open them as separate PRs in that order, each with its own build and automation evidence, and close this one. If you would rather review it whole, it is verified as-is: UE 5.7 and UE 5.8 both build, One scope note either way, since it is a policy call rather than a defect: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6de1d38f40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (bRequestMatches | ||
| && UserStamp == Cache.UserStamp | ||
| && LegacyStamp == Cache.LegacyStamp) | ||
| { | ||
| return Cache.Value; |
There was a problem hiding this comment.
When LoadFileToString fails transiently while the file can still be statted—for example during a Windows sharing lock or temporary permission failure—the new fail-closed result is cached together with the file's existing timestamp. If access later recovers without rewriting the file, this timestamp-only fast path returns the disabled value indefinitely, so the server ticker never restarts the server and indexing remains inactive despite the comment promising recovery once the file is readable. Avoid caching unreadable results as timestamp-stable, or record that state and force another read after the revalidation interval.
Useful? React with 👍 / 👎.
| { | ||
| const FString Path = GetSentinelFilePath(); | ||
| if (FPaths::FileExists(Path)) | ||
| switch (MonolithSentinelFile::RemoveOwned(Path, bOwnsSentinelFile)) |
There was a problem hiding this comment.
When an editor crashes after writing .monolith_running and the next editor starts with server activation or project policy disabled, bOwnsSentinelFile remains false and every stop or shutdown reaches NotOwned here. Unlike the previous unconditional cleanup, no later lifecycle path removes the dead process's sentinel unless this process successfully starts and overwrites it, so the file can continue advertising a nonexistent server indefinitely. Inspect the recorded PID and reclaim the sentinel when that owner is no longer alive while still preserving sentinels belonging to a live editor.
Useful? React with 👍 / 👎.
kunkunGames
changed the title
kunkunGames
changed the title
…ur lifecycle defects Eight defect fixes salvaged from PR #114. The PR's durable-activation feature is NOT taken -- it was declined on review: it persists a server-off state to disk whose only recovery is typing a console command in the editor, while Monolith's primary consumer is an agent driving the editor through the very transport that state disables. These are the genuine bugs underneath it. FHttpServerModule::StopAllListeners() is process-wide. Monolith called it on stop and in the bind-retry loop, so it silently killed every OTHER plugin's HTTP listener in the editor as a side effect of Monolith restarting. Replaced with per-route unbinding; Restart() keeps a listener-recreating path so its stated purpose survives. Four fire-and-forget AsyncTask(GameThread) lambdas in FIndexingTask::Run captured raw this. WaitForCompletion() joins the worker thread but does not drain the game-thread task queue, so an already-queued lambda runs after the task object is destroyed. PR #114 converted two of them -- the completion callbacks -- and left the two OnProgress.Broadcast sites, which fire every batch rather than once, and are therefore the more likely crash. All four now capture a weak pointer, with the counter values copied into locals before the lambda is constructed, since they cannot be read through a dangling this. Live asset-registry callbacks are unregistered for the duration of a full index and were only re-armed on success. After a failed or cancelled run the subsystem reported itself active while silently dropping every subsequent asset change until a successful reindex or an editor restart -- stale data with no error. They are now re-armed on every outcome, the unregister path resets its delegate handles, and register is idempotent so a double-bind cannot accumulate. Two latched-state bugs left indexing permanently refused until an editor restart: a failed FRunnableThread::Create in StartAsync, and a failed OpenForWriting in Run, both returned without broadcasting completion, so bIsIndexing stayed true and every later request got "Indexing already in progress". Reindex results are now honest. TriggerReindex, TriggerProjectReindex, StartFullIndex and StartIncrementalIndex all return bool and the handlers reported unconditional success, so an agent received reindex_started for work that never started. ProbePort no longer blocks: a loopback connect to a closed port stalls the game thread ~2s per attempt, during editor startup. Reported and prototyped by @kunkunGames (#114).
|
Partly shipped in v0.22.0 — about 250 lines of it. I land contributor fixes as my own commits rather than merging the branch — I keep the shipped history single-author for release integrity, and credit you in the release notes instead. No reflection on the patch. The bug fixes buried in this PR are real and I took all of them:
Your The durable-activation feature I have declined, and I want to give you the actual reason rather than a vague one. The disqualifier is the recovery path: Two smaller things that fed into it: the PR describes making None of that reflects on the engineering, which is careful — the sentinel reclaim logic in particular is more thoughtful than most. It is a scope and blast-radius call on the most load-bearing module in the plugin. |
…ve-and-re-add That string shipped in tool output from remove_map_parameter_pin and set_script_parameter_type, and it is now false. Stale prose in a doc is one thing; stale prose emitted to whoever calls the action is worse, because it is read at exactly the moment someone is deciding what to do next. It now points at apply_script_changes, states that it was measured with controls on add, remove and retype, and says the thing that actually matters for the choice: it preserves the caller's other override values, which remove-and-re-add destroys. So re-adding is named as the worse option rather than the only one. It also names the two residuals apply does NOT clean up, because a warning that oversells a fix is the same defect in the other direction: removing an input leaves a dead override pin (tumourlove#114) and retyping one leaves the old override so two same-named inputs exist at different types (tumourlove#115), and a compile shows neither. The block comment above it is corrected too, since it asserted that NO action refreshes a placed caller. That was true when written -- tumourlove#62 tried saving, re-opening the system editor and toggling the enabled flag, and all three failed. They failed because none of them is Apply. Compile is not Apply, which is the whole point. What survives from tumourlove#62 is the static-switch case, still untested, and that is now stated as the remaining scope rather than left implied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Goal
Control MCP-server activation and source/project index-writer activation independently and persistently, while preserving the same fail-closed contract across failures, external edits, restarts, and Editor crashes.
Plain-language explanation
Selections made through
Monolith.StartServer/Monolith.StopServerandMonolith.StartIndexing/Monolith.StopIndexingpersist into the next Editor run and take effect immediately. Server and indexing settings do not overwrite one another, and neither can bypass the project's hard policy gates.Improvements
Before → After
Side-effect analysis
bMcpServerEnabled,bEnableIndex, andbEnableSourcehard gates always take precedence.ServerPortchange is rejected without dropping the working route and returns restart guidance.StopAllListeners()is never called, so listeners owned by other plugins remain untouched.Resolved review findings
Commit
62aba1b1f10948bae0e9cabb7f6c19150f5f5d2baddresses the two remaining review findings:Regression tests deterministically cover unreadable-to-readable recovery with an unchanged timestamp and dead, live, current-process, malformed, and replaced sentinels.
Verification
MonolithSettings.cpp,MonolithCoreModule.cpp, and activation tests were recompiled;UnrealEditor-MonolithCore.dllwas relinked; wrapper exit 0.Monolith.Activation: 6/6 PASS, with 0 test warnings, 0 errors, and process exit 0. Report:D:\P4\MonolithPR114ReviewUE57Host\Saved\Automation\PR114ReviewRound5FinalUE57\index.json.Monolith.Activation: 6/6 PASS, with 0 test warnings, 0 errors, and process exit 0. Report:D:\P4\MonolithPR114ReviewUE58Host\Saved\Automation\PR114ReviewRound5FinalUE58\index.json.git diff --check: PASS. Monolith source contains 0 calls toFHttpServerModule::StopAllListeners(). The branch was 0 commits behind the latesttumourlove/master.Docs/testing/2026-07-26-persistent-service-activation.md.WorkFingerprint
agent: Codexcategory: lifecycle / reliability / activationmodule: MonolithCore, MonolithIndex, MonolithSource, MonolithEditorcomponent/action/helper: durable activation resolver/cache, core lifecycle reconciler, sentinel ownership, project/source writer acceptanceintended files:MonolithSettings.{h,cpp},MonolithCoreModule.{h,cpp},MonolithHttpServer.{h,cpp},MonolithSentinelFile.h,MonolithCoreTools.cpp, Index/Source subsystem and indexer activation surfaces, Settings customization, matching specs/tests/configrisk type: unintended service restart, stale readiness marker, shared-listener damage, false reindex acceptance, writer teardown racepublic API impact: yes, persistent console-command behavior and honest reindex-result contractdocs/spec impact: yesDuplicate check
On 2026-07-27, open PRs #104, #112, #113, and #114, their related remote branches, and their actual changed files were checked again. #104 targets updater selection, #112 targets discovery filtering, and #113 targets project search and FTS repair. #112 overlaps physically in
MonolithCoreTools.cpp, and #113 overlaps in the Index subsystem and shared documentation, but they own discovery/read-search contracts while this PR owns activation and write acceptance. No other open PR provides the same persistent activation and sentinel-reclamation implementation. Common files may require rebasing depending on merge order, and that conflict risk remains explicit.