Fix/new entry module logging

[ChrisCrosley]

Two fixes to the new C# session loader's entry module.

1. C# loader logs never reached runtime.log or the output window.

Logging gets wired up by ScriptOutput.ConfigureLogging(), which was only called from Python's _setup_output(), and only on first load. Under the C# loader that gate is never true — checking first_load builds the output window it's testing for, so it always answers "not first load."

Fix: the loader wires up logging itself, as soon as the runtime assembly is available. It also tells the entry script directly whether this is a startup or a reload, instead of leaving it to guess. Load timings now travel through the environment dictionary, since preload and postload each run in their own engine and can't share variables.

2. Reloading pyRevit from a button crashed that button.

Reload died with AttributeError: 'NoneType' object has no attribute 'Add' as soon as the session came back. On reload pyRevit shuts down cached script engines, skipping one so it doesn't kill the caller. But the load now runs in its own entry script, so the engine being skipped was the entry script's — not Reload's. Reload's engine was shut down while Reload was still paused inside it, and when it resumed, its script runtime and output stream were gone.

Fix: track which engines are actually mid-execution and never shut one of those down.

[devloai]

PR Summary:

• Fix 1 (logging): The C# session loader now wires up NLog → runtime logging itself (ConfigureRuntimeLogging), instead of relying on Python's first-load-only _setup_output(). The broken first_load inference (checking window_handle built the window it tested for, so it always answered "not first load") is replaced by an explicit __sessionfirstload__ builtin threaded through ScriptRuntimeConfigs.Variables. Load timings now travel through the AppDomain env-var dict since preload and postload run in separate engines.
• Fix 2 (reload crash): Tracks mid-execution engines via a depth-counted ActiveEngineDict (EnterEngine/ExitEngine in a try/finally around ExecuteManagedScript). ClearEngines and SetCachedEngine now skip active engines, so a paused command's engine (e.g. Reload's) survives a session reload instead of being shut down out from under it.

Review Summary:

Both fixes are well-reasoned and the core logic is sound. I verified the ActiveEngineDict depth bookkeeping unwinds correctly for nested executions, confirmed EnvDictionary.Seed merges (not replaces) so env-var timing state survives across preload/postload engines, and traced the __sessionfirstload__ flow through ScriptRuntimeConfigs.Variables → engine SetupBuiltins → EXEC_PARAMS.first_load. The old first_load window-handle inference bug and the excludeEngine mismatch on reload are both correctly addressed. Two minor follow-ups noted below (one convention, one defensive). No blocking issues found.

Suggestions

• Add a unit test verifying ActiveEngineDict depth unwinds correctly when EnterEngine/ExitEngine bracket a nested ClearEngines call. Apply
• Audit other C# loader early-startup catch blocks for Debug.WriteLine vs Trace.WriteLine consistency. Apply

[devloai]

The repository convention for early-startup diagnostics in SessionManagerService / EnvDictionarySeeder is System.Diagnostics.Trace.WriteLine(...) (see EnvDictionarySeeder.ReadIPYVersion and ExecuteExtensionStartupScript). The two diagnostic lines added here use Debug.WriteLine instead. Debug.WriteLine is compiled out under RELEASE builds, so these messages — which are exactly the kind of "logging infrastructure not ready yet" trace the review guidelines call out — vanish in a signed/release installer where they are most needed when troubleshooting why loader logs never appear.

Use Trace.WriteLine for consistency with the surrounding early-startup diagnostics and to keep the messages in release builds:

Trace.WriteLine(
    "pyRevit: ScriptOutput.ConfigureLogging not found; loader logging is discarded.");

and

Trace.WriteLine($"pyRevit: failed to route loader logging into the runtime: {ex}");

actions

Feedback: Rate this comment to help me improve future code reviews:

• 👍 Good - Helpful and accurate
• 👎 Poor - Wrong, unclear, or unhelpful
• Skip if you don't have any strong opinions either way.

[devloai]

ActiveEngineDict / EnterEngine / ExitEngine mutate a plain Dictionary<string, int> with no locking. Today every call site (command execute, reload entry scripts) runs on the Revit main thread, so this is safe. However the existing ClearEngines already documents a single-handler concurrency caveat in ScriptExecutor.Initialize, and RequestExecuteScript exists for off-main-thread execution. If a future change ever drives ExecuteScript from a non-main thread (or nests an ExternalEvent raise that re-enters ExecuteManagedScript on the idle handler while a main-thread command is still in its try block), the depth++/depth-- bookkeeping here will race and can leave an engine permanently marked active (so it never gets shut down) or permanently removable (re-introducing the original crash).

Consider either a brief note that these helpers are main-thread-only (like the one on ScriptExecutor.Initialize), or a lock around the read-modify-write in EnterEngine/ExitEngine and the enumeration in ClearEngines so the invariant is preserved if the threading assumptions change.

actions

Feedback: Rate this comment to help me improve future code reviews:

• 👍 Good - Helpful and accurate
• 👎 Poor - Wrong, unclear, or unhelpful
• Skip if you don't have any strong opinions either way.

[Copilot]

Pull request overview

This PR fixes two defects in the new C# session loader's Python entry module (perform_preload/perform_postload), which replaced the old single-function load_session flow.

1. Loader logs never reached runtime.log or the output window. ScriptOutput.ConfigureLogging() was only invoked from Python's _setup_output(), which only ran on first load and was gated by a first_load check that could never be true under the C# loader. The fix has the C# SessionManagerService call ConfigureLogging() directly once the runtime assembly is available, and passes an explicit firstLoad flag to the entry scripts (exposed as the __sessionfirstload__ builtin) instead of inferring it from the output-window handle. Load timings are now carried across the two separate entry-script engines via new environment-dictionary keys.

2. Reloading pyRevit from a button crashed the button. During a reload, cached engines are shut down except the one being skipped; that skip targeted the entry script's engine, not the still-suspended caller's engine, so the caller resumed into a disposed runtime. The fix ref-counts engines that are mid-execution (ActiveEngineDict + Enter/ExitEngine) and never shuts one of those down.

Changes:

• Wire up runtime logging from the C# loader and pass an explicit first-load/reload flag to entry scripts.
• Carry session-start and output-setup timings through the environment dictionary instead of engine-module globals.
• Track in-flight engines by depth and suppress their shutdown during a session reload.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pyrevitlib/pyrevit/loader/sessionmgr.py	Replaces module-level timing/output state with env-var-carried timings; always self-destructs the session output singleton.
pyrevitlib/pyrevit/coreutils/envvars.py	Adds SESSIONSTARTTIME/OUTPUTSETUPTIME env-var keys.
pyrevitlib/pyrevit/__init__.py	first_load now reads the __sessionfirstload__ builtin, falling back to the output-window inference.
dev/pyRevitLoader/.../SessionManagerService.cs	Adds ConfigureRuntimeLogging(), plumbs firstLoad into entry scripts, and publishes __sessionfirstload__ via Variables.
dev/pyRevitLabs.PyRevit.Runtime/ScriptExecutor.cs	Wraps script execution in EnterEngine/ExitEngine (try/finally).
dev/pyRevitLabs.PyRevit.Runtime/ScriptEngineManager.cs	Adds ActiveEngineDict depth tracking and skips active engines in ClearEngines/SetCachedEngine.
dev/pyRevitLabs.PyRevit.Runtime/EnvVariables.cs	Adds the ActiveEngines AppDomain storage key.

I reviewed the timing computation (Timer.start is a time.time() value, so time.time() - starttime is correct), the first_load builtin pattern (consistent with existing __cachedengine__/__scriptruntime__ properties), the firstLoad plumbing (LoadSession() reload passes firstLoad: false), the idempotent ConfigureLogging guard, and the reload engine-lifecycle scenario (depth counting correctly handles a caller and entry script sharing an engine TypeId). I did not find concrete defects. The changes are subtle and touch core session-loading, engine-lifecycle, and logging behavior, so final human verification in a running Revit session is warranted.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.