Skip to content

Preserve RegistrationContext in telemetry worker threads - #6960

Open
masenf wants to merge 4 commits into
mainfrom
claude/flaky-config-load-windows-dl7pg9
Open

Preserve RegistrationContext in telemetry worker threads#6960
masenf wants to merge 4 commits into
mainfrom
claude/flaky-config-load-windows-dl7pg9

Conversation

@masenf

@masenf masenf commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Description

This PR fixes an issue where telemetry event collection in worker threads would lose the submitting thread's RegistrationContext, causing the worker to create a throwaway context and re-import rxconfig.py unnecessarily.

The Problem:
When telemetry jobs are submitted via _submit(), they run on a background executor thread that has no RegistrationContext attached. During event collection (e.g., get_bun_path()), a config lookup would trigger ensure_context(), which attaches a new context to the worker thread and re-imports rxconfig.py. This is wasteful since the app's config is already loaded in the submitting thread's context.

The Solution:

  • Capture the submitting thread's RegistrationContext (if any) at submission time via new _current_registration_context() helper
  • Pass this context to _run_suppressed() in the worker thread
  • Attach the context for the duration of the job execution, then detach it to keep the worker thread clean for subsequent jobs
  • This allows event collection to reuse the already-loaded config instead of re-importing

Changes:

  • reflex/utils/telemetry.py:
    • Added _current_registration_context() to safely retrieve the caller's context without creating one
    • Modified _run_suppressed() to accept and manage the registration context around job execution
    • Updated _submit() to capture and forward the context
  • tests/units/test_telemetry.py: Added two new tests verifying context preservation and cleanup
  • tests/units/test_config.py: Updated existing test to avoid counting unrelated background thread loads

Testing

  • Added test_submit_runs_job_in_callers_registration_context() to verify the submitting thread's context is available during job execution
  • Added test_submit_leaves_worker_context_clean_between_jobs() to verify the context is properly detached after each job
  • Updated test_get_config_loads_once_for_shared_context() to exclude unrelated background threads from load counting
  • All existing telemetry tests continue to pass

https://claude.ai/code/session_01Ex7f8t8niToPKPyqXh4Eeq

Review in cubic

claude added 2 commits August 27, 2026 20:51
…hreads

test_get_config_loads_once_for_shared_context patches
reflex_base.config._load_config and asserts it ran exactly once. The counter
was process-wide, so any background thread that called get_config() inside the
patched window was counted as a duplicate load of the shared context.

The telemetry worker thread does exactly that: _process_event -> _send ->
_prepare_event -> get_event_defaults -> get_bun_version ->
path_ops.get_bun_path -> get_config(). That thread carries no
RegistrationContext, so ensure_context() creates a fresh one and loads the
config for it. When an event queued by an earlier test drains during this
test, load_count reaches 2 and the assert fails (seen on Windows CI).

Count only loads made from the test's worker threads, via a thread-local
marker, so an unserialized load path for the shared context still fails the
test while unrelated background loads are ignored.
The telemetry worker thread carries no RegistrationContext, so a config
lookup during event collection (_prepare_event -> get_event_defaults ->
get_bun_version -> path_ops.get_bun_path -> get_config()) attached a
throwaway context to the worker and re-imported rxconfig.py off-thread —
duplicating work the caller had already done, against a config the app is not
using, and mutating sys.path from a background thread while doing it.

_submit now captures the caller's RegistrationContext (without attaching one
to a caller that has none) and _run_suppressed attaches it for the duration of
the job, resetting it afterwards so a context never leaks into the next job on
the reused worker thread. Collection therefore reuses the already-loaded
Config instead of loading its own.

Verified across the unit suite: instrumenting _load_config to log calls from
non-main threads reported one such load before this change and none after.
@masenf
masenf requested a review from a team as a code owner August 27, 2026 21:26
@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR propagates the submitting thread’s RegistrationContext into telemetry jobs and resets that temporary attachment after execution.

  • Adds non-creating context lookup and propagation around telemetry worker calls.
  • Adds tests for caller-context reuse and detachment after context-bearing jobs.
  • Narrows shared-config load counting to the test’s own worker threads.

Confidence Score: 4/5

The PR is not yet safe to merge because context-free telemetry jobs can still leak worker-local configuration and registration state into later jobs.

The single telemetry worker is reused, while the new cleanup resets only a propagated submitter context; a context created by get_config() during a context-free job remains attached and is visible to subsequent context-free work.

Files Needing Attention: reflex/utils/telemetry.py

Important Files Changed

Filename Overview
reflex/utils/telemetry.py Adds submitter-context propagation and token-based restoration, but the previously reported context-free worker state leak remains.
tests/units/test_telemetry.py Covers propagation and cleanup after a context-bearing job, but does not exercise a context-free job that creates its own worker context.
tests/units/test_config.py Restricts load counting to the test’s participating threads to avoid unrelated telemetry activity.
news/6960.bugfix.md Documents the intended telemetry context-preservation behavior.

Reviews (4): Last reviewed commit: "telemetry: document the context-less-job..." | Re-trigger Greptile

Comment thread reflex/utils/telemetry.py
@codspeed-hq

codspeed-hq Bot commented Aug 27, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing claude/flaky-config-load-windows-dl7pg9 (b048aab) with main (a33e02a)

Open in CodSpeed

Footnotes

  1. 8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed against the latest diff

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread reflex/utils/telemetry.py
@masenf

masenf commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

resolved issues via documenting the edge case instead of adding a bunch of complexity for a case which is rarely hit and doesn't really cause significant issue when it is hit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants