fix(sidecar): remove spurious deletion of runtimes on set_session_config - #2277
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: 03fafcbba6
ℹ️ 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".
| for runtime in runtimes { | ||
| runtime.shutdown(); |
There was a problem hiding this comment.
When shutdown() runs while a RuntimeLease is still alive—a case explicitly supported by release_runtime—the lease retains another RuntimeInfo clone and therefore the shared applications Arc. Consuming the drained clone here does not drop the applications or their remote-config guards because RuntimeInfo::shutdown() only logs, so session teardown leaves subscriptions and stale application state active until every lease eventually disappears. Explicitly clear the applications during shutdown rather than relying on the RuntimeInfo clone being the last owner.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
under the intended lifecycle, stop_session is only called when session_counter for the session_id reaches 0. So the session should not be reclaimed until all the connections, and consequently all the runtime leases on the session are gone. Perhaps this could be made less fragile though -- it is true that after this patch, with both the session and the connections holding strong references to the runtimes, it becomes more likely that some logic error has runtimes outliving their session (and consequently not dropping applications and their resources). Although I'm not sure that the consequences of this on master would be that benign either
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: cd97f7c | Docs | Datadog PR Page | Give us feedback! |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
d6363b1 to
cd97f7c
Compare
cataphract
changed the title
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
f2010b6
into
main
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
2 participants
EDIT: After discussion, pending deeper refactoring, fix just the spurious runtime deletion on set_session_config.
What does this PR do?
HashSet<InstanceId>with countedRuntimeLeases.set_session_config()aas a reason to destroy all existing runtimes (when is_fork=false).First, this replaces runtime teardown based on per-connection recorded IDs and explicit shutdown RPCs with lease-based shared ownership. Each connection holds at most one RuntimeLease per InstanceId. Acquiring the first lease creates a RuntimeEntry with one owner; subsequent acquisitions increment its semantic owner count. Dropping a lease decrements that count, and dropping the final lease removes and shuts down the runtime. Non-owning runtime lookups are available where needed (peek_runtime).
Before, when a connection closed, it would calls shutdown_runtime() for every recorded id. Consequently, if two connections use the same runtime, the first one to disconnect can remove it even while the second connection remains alive.
Secondly,
set_session_configcalledsession.shutdown_running_instances()for every non-fork connection, which drained all runtimes of the session. Anenqueue_actionsthat was concurrently in flight then re-created an emptyRuntimeInfo, logged "No application found" and dropped the telemetry payload. So we cannot allow session reconfiguration to bypass the lease ownership model.Notes:
Because reclamation now happens in
Drop,RuntimeInfo::shutdownandSessionInfo::shutdownmust stay synchronous; they are, since neither awaits.With ownership tracked by leases, the
shutdown_runtimeandshutdown_sessionRPCs and theis_forkargument ofset_session_configbecome redundant, so they are removed from the wire interface.Motivation
We're seeing flaky tests in the AppSec integration tests in dd-trace-php that are very likely caused by this and also possible system-tests telemetry tests flakiness that is possibly also related (I have not investigated)
BREAKING CHANGE: removes the shutdown_runtime and shutdown_session sidecar RPCs and removes the is_fork parameter from set_session_config and ddog_sidecar_session_set_config.