chore(remoteconfig): refresh client identity on runtime refresh - #19818
litianningdatadog wants to merge 1 commit into
Conversation
litianningdatadog
added
changelog/no-changelog
Codeowners resolved asResolved from the full PR diff against |
Circular import analysis
|
Dependency direction analysis
|
litianningdatadog
changed the title
🎉 All green!🧪 All tests passed 🔗 Commit SHA: e9a58bc | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-08-23 22:23:08 Comparing candidate commit 3e763c0 in PR branch Found 0 performance improvements and 10 performance regressions! Performance is the same for 612 metrics, 10 unstable metrics.
|
d59e112 to
16a5332
Compare
3e763c0 to
ff3e857
Compare
16a5332 to
8dd7e8e
Compare
ff3e857 to
25a94b2
Compare
cde3045 to
a0e3c42
Compare
25a94b2 to
997466f
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the internal Remote Config client to respond to runtime-identity refreshes (e.g., AWS Lambda MicroVM restore) by regenerating its client ID and forcing a rebuild of the native Remote Config client so subsequent polls use fresh identity.
Changes:
- Subscribe
RemoteConfigClientto runtime ID change notifications and renew its client ID on refresh. - Drop the cached native Remote Config client on identity refresh so
ensure_native()rebuilds it with refreshed runtime identity. - Add native Remote Config tests validating client-id renewal, native client dropping/rebuild, and wiring to
runtime.refresh_identity().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ddtrace/internal/remoteconfig/client.py |
Subscribes to runtime ID changes and drops/rebuilds native Remote Config client identity on refresh. |
tests/internal/remoteconfig/test_remoteconfig_native.py |
Adds coverage for client-id renewal and native client rebuild behavior on identity refresh. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
997466f to
6e15d59
Compare
6e15d59 to
e9a58bc
Compare
![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: e9a58bc42f
ℹ️ 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".
| log.debug("failed to create remote config reader after fork", exc_info=True) | ||
| else: | ||
| self._reader = None | ||
| self.renew_id() |
There was a problem hiding this comment.
When runtime.refresh_identity() runs while AppSec Remote Configuration is enabled, this renews the client ID, but enable_appsec_rc() cached the previous value in asm_config._rc_client_id only once (ddtrace/appsec/_remoteconfiguration.py:83). finalize_asm_env() continues tagging every AppSec span with that stale value (ddtrace/appsec/_asm_request_context.py:403-404), while subsequent RC requests use the new ID, breaking correlation between those spans and the RC client after an identity refresh. Propagate the renewed ID to this cache or make the span tag read the current client ID.
Useful? React with 👍 / 👎.
| self.renew_id() | ||
| self._native = None |
There was a problem hiding this comment.
When refresh_identity() runs in an origin process after it has forked workers, dropping this native client also drops the writer for the shared-memory segments inherited by those workers. The replacement client starts with local storage, and existing children cannot inherit its new handles—the native protocol requires enable_shared_memory() before the fork (src/native/remote_config.rs:9-12)—so they remain attached to the frozen old mappings and silently stop receiving future Remote Configuration updates, including security configuration. The refresh path needs to preserve or migrate the existing broadcast publisher rather than simply replacing it in an already-forked origin.
AGENTS.md reference: AGENTS.md:L135-L136
Useful? React with 👍 / 👎.
| self.renew_id() | ||
| self._native = None |
There was a problem hiding this comment.
When identity refresh overlaps the first ensure_native() call while _native is still None—for example, one thread enables RC while a restore hook refreshes identity on another—the constructor can already have read the old self.id but not yet assigned its result. These lines then renew the ID and clear _native, after which the racing constructor assigns the old-identity native client back into _native; it remains cached, so every later poll continues using the stale client identity despite the completed refresh. Native construction/assignment and identity invalidation need a shared synchronization or generation check.
Useful? React with 👍 / 👎.
![datadog-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
A runtime identity refresh changes the Remote Config client ID. AppSec keeps the old ID and adds it to later spans, so Remote Config data cannot match those spans.
🤖 Datadog Autotest · Commit e9a58bc · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| log.debug("failed to create remote config reader after fork", exc_info=True) | ||
| else: | ||
| self._reader = None | ||
| self.renew_id() |
There was a problem hiding this comment.
Remote Config data cannot match AppSec spans from a restored runtime.
Assertion details
- Input: Enable AppSec Remote Config. Then refresh the runtime identity and finalize a later AppSec span.
- Expected:
AppSec spans must use the new Remote Config client ID after a runtime identity refresh. - Actual:
The refresh handler renews RemoteConfigClient.id. AppSec still keeps the old value in asm_config._rc_client_id and adds it to later spans.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| client_ref = weakref.ref(self) | ||
|
|
||
| # Runtime keeps callbacks in a module-level set, so registering a bound | ||
| # method would keep this client alive after callers drop their reference. | ||
| def _on_identity_refresh(new_runtime_id: str) -> None: | ||
| client = client_ref() | ||
| if client is not None: | ||
| client._on_identity_refresh(new_runtime_id) | ||
|
|
||
| on_runtime_id_change(_on_identity_refresh) |
There was a problem hiding this comment.
we should probably just add a remove_on_runtime_id_change(func) method, then this can all go away and we get:
def __init__(self):
on_runtime_id_change(self._on_identity_refresh)
def __del__(self):
remove_on_runtime_id_change(self._on_identity_refresh)| # as immutable constructor arguments (get_client_id() is documented "stable for | ||
| # the process lifetime"). The next ensure_native() call rebuilds it bound to the |
There was a problem hiding this comment.
Given this definition of "stable for the process lifetime", the new changes to refresh the runtime id at runtime seem to go against this expectation.
Should MicroVM be using a different identifier for this instead of runtime-id?
Do we need to change all expectations in shared components in libdatadog and all repos to no longer assume runtime-id is process stable? which is mostly the changes we are making here
emmettbutler
left a comment
emmettbutler
left a comment
There was a problem hiding this comment.
Marked as draft because the base branch is not main
|
close it as it is out of the scope |
Stacked PRs:
Description
Split from #19780.
Remote Config clients include process/runtime identity in their generated client id, and the native Remote Config client keeps that id in its own state. When runtime identity is refreshed for a restored MicroVM instance, rebuild the Remote Config client identity and drop the stale native client so future requests use the refreshed identity.
Testing
scripts/lint format_check ddtrace/internal/remoteconfig/client.py tests/internal/remoteconfig/test_remoteconfig_native.pygit diff --checkStack
Depends on #19816.