Skip to content

config/remote/service: clarify locking behaviors - #42061

Merged
dd-mergequeue[bot] merged 11 commits into
mainfrom
ajwerner/rc-service-cleanup
Oct 23, 2025
Merged

config/remote/service: clarify locking behaviors#42061
dd-mergequeue[bot] merged 11 commits into
mainfrom
ajwerner/rc-service-cleanup

Conversation

@ajwerner

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR is a bundle of cleanups around the code in pkg/config/remote/service. It fixes some race conditions, and more generally attempts to tame and document the use of mutexes around this service.

Motivation

I'm making changes to this package as part of https://datadoghq.atlassian.net/browse/DEBUG-4590 and was having a hard time getting a handle of the synchronization.

Describe how you validated your changes

All the tests pass, and no behavior should be changed.

Additional Notes

This pattern of using an anonymous struct with an embedded mutex to document what is protected by that mutex is a pattern I've seen widely used in the Go ecosystem and one I've found broadly beneficial.

PLEASE REVIEW COMMIT BY COMMIT

@ajwerner ajwerner added changelog/no-changelog No changelog entry needed qa/done QA done before merge and regressions are covered by tests labels Oct 16, 2025
@ajwerner ajwerner changed the title Ajwerner/rc service cleanup config/remote/service: clarify locking behaviors Oct 16, 2025
@github-actions github-actions Bot added the medium review PR review might take time label Oct 16, 2025
@ajwerner
ajwerner force-pushed the ajwerner/rc-service-cleanup branch from 7c5c597 to 4a0d230 Compare October 16, 2025 19:29
@ajwerner
ajwerner marked this pull request as ready for review October 16, 2025 19:29
@ajwerner
ajwerner requested a review from a team as a code owner October 16, 2025 19:29
@agent-platform-auto-pr

agent-platform-auto-pr Bot commented Oct 16, 2025

Copy link
Copy Markdown
Contributor

Static quality checks

✅ Please find below the results from static quality gates
Comparison made with ancestor 9dc3550

Successful checks

Info

Quality gate Delta On disk size (MiB) Delta On wire size (MiB)
agent_deb_amd64 $${+0.01}$$ $${676.48}$$ < $${702.15}$$ $${+0}$$ $${165.15}$$ < $${177.79}$$
agent_deb_amd64_fips $${+0.01}$$ $${670.99}$$ < $${696.65}$$ $${-0.03}$$ $${164.62}$$ < $${176.53}$$
agent_heroku_amd64 $${+0.01}$$ $${336.49}$$ < $${340.18}$$ $${+0}$$ $${89.81}$$ < $${91.08}$$
agent_msi $${+0.06}$$ $${1013.79}$$ < $${1015.38}$$ $${+0.04}$$ $${148.45}$$ < $${150.78}$$
agent_rpm_amd64 $${+0.01}$$ $${676.47}$$ < $${702.14}$$ $${+0.02}$$ $${167.39}$$ < $${180.53}$$
agent_rpm_amd64_fips $${+0.01}$$ $${670.98}$$ < $${696.64}$$ $${+0.01}$$ $${166.06}$$ < $${178.79}$$
agent_rpm_arm64 $${+0.01}$$ $${666.31}$$ < $${686.31}$$ $${+0}$$ $${153.29}$$ < $${161.22}$$
agent_rpm_arm64_fips $${+0.01}$$ $${661.84}$$ < $${681.84}$$ $${-0}$$ $${152.66}$$ < $${160.18}$$
agent_suse_amd64 $${+0.01}$$ $${676.47}$$ < $${702.14}$$ $${+0.02}$$ $${167.39}$$ < $${180.53}$$
agent_suse_amd64_fips $${+0.01}$$ $${670.98}$$ < $${696.64}$$ $${+0.01}$$ $${166.06}$$ < $${178.79}$$
agent_suse_arm64 $${+0.01}$$ $${666.31}$$ < $${686.31}$$ $${+0}$$ $${153.29}$$ < $${161.22}$$
agent_suse_arm64_fips $${+0.01}$$ $${661.84}$$ < $${681.84}$$ $${-0}$$ $${152.66}$$ < $${160.18}$$
docker_agent_amd64 $${+0.01}$$ $${747.03}$$ < $${773.59}$$ $${-0.01}$$ $${251.82}$$ < $${266.06}$$
docker_agent_arm64 $${+0.01}$$ $${760.82}$$ < $${781.7}$$ $${-0.01}$$ $${242.45}$$ < $${251.85}$$
docker_agent_jmx_amd64 $${+0.01}$$ $${937.9}$$ < $${964.45}$$ $${-0.01}$$ $${320.46}$$ < $${334.68}$$
docker_agent_jmx_arm64 $${+0.01}$$ $${940.28}$$ < $${961.17}$$ $${-0.01}$$ $${307.05}$$ < $${316.44}$$
docker_cluster_agent_amd64 $${+0.02}$$ $${213.05}$$ < $${213.74}$$ $${+0.02}$$ $${72.29}$$ < $${73.14}$$
docker_cluster_agent_arm64 $${-0}$$ $${228.99}$$ < $${229.68}$$ $${+0.02}$$ $${68.54}$$ < $${69.41}$$
docker_cws_instrumentation_amd64 $${0}$$ $${7.07}$$ < $${7.12}$$ $${+0}$$ $${2.95}$$ < $${3.29}$$
docker_cws_instrumentation_arm64 $${+0}$$ $${6.69}$$ < $${6.92}$$ $${+0}$$ $${2.7}$$ < $${3.07}$$
docker_dogstatsd_amd64 $${-0}$$ $${38.45}$$ < $${39.3}$$ $${-0}$$ $${14.84}$$ < $${15.76}$$
docker_dogstatsd_arm64 $${-0}$$ $${37.12}$$ < $${37.94}$$ $${-0}$$ $${14.28}$$ < $${14.83}$$
dogstatsd_deb_amd64 $${0}$$ $${29.67}$$ < $${30.53}$$ $${-0}$$ $${7.82}$$ < $${8.75}$$
dogstatsd_deb_arm64 $${0}$$ $${28.25}$$ < $${29.11}$$ $${+0}$$ $${6.78}$$ < $${7.71}$$
dogstatsd_rpm_amd64 $${0}$$ $${29.67}$$ < $${30.53}$$ $${-0}$$ $${7.83}$$ < $${8.76}$$
dogstatsd_suse_amd64 $${0}$$ $${29.67}$$ < $${30.53}$$ $${-0}$$ $${7.83}$$ < $${8.76}$$
iot_agent_deb_amd64 $${+0.01}$$ $${41.94}$$ < $${54.97}$$ $${+0}$$ $${10.91}$$ < $${14.45}$$
iot_agent_deb_arm64 $${+0.01}$$ $${39.76}$$ < $${51.9}$$ $${+0}$$ $${9.42}$$ < $${12.63}$$
iot_agent_deb_armhf $${+0.01}$$ $${39.63}$$ < $${51.84}$$ $${+0}$$ $${9.5}$$ < $${12.74}$$
iot_agent_rpm_amd64 $${+0.01}$$ $${41.94}$$ < $${54.97}$$ $${+0}$$ $${10.93}$$ < $${14.47}$$
iot_agent_suse_amd64 $${+0.01}$$ $${41.94}$$ < $${54.97}$$ $${+0}$$ $${10.93}$$ < $${14.47}$$

@cit-pr-commenter

cit-pr-commenter Bot commented Oct 16, 2025

Copy link
Copy Markdown

Regression Detector

Regression Detector Results

Metrics dashboard
Target profiles
Run ID: 5db1a52b-766c-41c3-891f-1f37681e46c8

Baseline: 9dc3550
Comparison: d9b07d7
Diff

Optimization Goals: ✅ No significant changes detected

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf experiment goal Δ mean % Δ mean % CI trials links
docker_containers_cpu % cpu utilization -0.38 [-2.27, +1.50] 1 Logs

Fine details of change detection per experiment

perf experiment goal Δ mean % Δ mean % CI trials links
docker_containers_memory memory utilization +1.19 [+0.78, +1.60] 1 Logs
quality_gate_idle_all_features memory utilization +0.21 [+0.17, +0.26] 1 Logs bounds checks dashboard
ddot_metrics_sum_delta memory utilization +0.18 [+0.02, +0.34] 1 Logs
file_to_blackhole_100ms_latency egress throughput +0.16 [-0.45, +0.76] 1 Logs
quality_gate_logs % cpu utilization +0.08 [-2.71, +2.88] 1 Logs bounds checks dashboard
otlp_ingest_metrics memory utilization +0.02 [-0.10, +0.15] 1 Logs
uds_dogstatsd_to_api ingress throughput +0.00 [-0.22, +0.22] 1 Logs
tcp_dd_logs_filter_exclude ingress throughput -0.00 [-0.01, +0.01] 1 Logs
file_to_blackhole_500ms_latency egress throughput -0.02 [-0.63, +0.59] 1 Logs
file_to_blackhole_0ms_latency egress throughput -0.04 [-0.65, +0.56] 1 Logs
file_to_blackhole_1000ms_latency egress throughput -0.21 [-0.82, +0.41] 1 Logs
ddot_logs memory utilization -0.25 [-0.33, -0.17] 1 Logs
ddot_metrics_sum_cumulative memory utilization -0.26 [-0.38, -0.14] 1 Logs
ddot_metrics memory utilization -0.31 [-0.46, -0.15] 1 Logs
quality_gate_idle memory utilization -0.34 [-0.38, -0.30] 1 Logs bounds checks dashboard
uds_dogstatsd_20mb_12k_contexts_20_senders memory utilization -0.34 [-0.39, -0.29] 1 Logs
file_tree memory utilization -0.37 [-0.43, -0.31] 1 Logs
docker_containers_cpu % cpu utilization -0.38 [-2.27, +1.50] 1 Logs
ddot_metrics_sum_cumulativetodelta_exporter memory utilization -0.41 [-0.60, -0.21] 1 Logs
otlp_ingest_logs memory utilization -0.43 [-0.57, -0.30] 1 Logs
quality_gate_metrics_logs memory utilization -0.49 [-0.70, -0.29] 1 Logs bounds checks dashboard
tcp_syslog_to_blackhole ingress throughput -0.91 [-0.98, -0.84] 1 Logs

Bounds Checks: ✅ Passed

perf experiment bounds_check_name replicates_passed links
docker_containers_cpu simple_check_run 10/10
docker_containers_memory memory_usage 10/10
docker_containers_memory simple_check_run 10/10
file_to_blackhole_0ms_latency lost_bytes 10/10
file_to_blackhole_0ms_latency memory_usage 10/10
file_to_blackhole_1000ms_latency memory_usage 10/10
file_to_blackhole_100ms_latency lost_bytes 10/10
file_to_blackhole_100ms_latency memory_usage 10/10
file_to_blackhole_500ms_latency lost_bytes 10/10
file_to_blackhole_500ms_latency memory_usage 10/10
quality_gate_idle intake_connections 10/10 bounds checks dashboard
quality_gate_idle memory_usage 10/10 bounds checks dashboard
quality_gate_idle_all_features intake_connections 10/10 bounds checks dashboard
quality_gate_idle_all_features memory_usage 10/10 bounds checks dashboard
quality_gate_logs intake_connections 10/10 bounds checks dashboard
quality_gate_logs lost_bytes 10/10 bounds checks dashboard
quality_gate_logs memory_usage 10/10 bounds checks dashboard
quality_gate_metrics_logs cpu_usage 10/10 bounds checks dashboard
quality_gate_metrics_logs intake_connections 10/10 bounds checks dashboard
quality_gate_metrics_logs lost_bytes 10/10 bounds checks dashboard
quality_gate_metrics_logs memory_usage 10/10 bounds checks dashboard

Explanation

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

Performance changes are noted in the perf column of each table:

  • ✅ = significantly better comparison variant performance
  • ❌ = significantly worse comparison variant performance
  • ➖ = no significant change in performance

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

  1. Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.

  2. Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.

  3. Its configuration does not mark it "erratic".

CI Pass/Fail Decision

Passed. All Quality Gates passed.

  • quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
  • quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
  • quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
  • quality_gate_metrics_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
  • quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
  • quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
  • quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
  • quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
  • quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
  • quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
  • quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.

Comment thread pkg/config/remote/service/service.go Outdated
response, err := s.api.FetchOrgStatus(context.Background())
response, err := s.getAPI().FetchOrgStatus(context.Background())

s.mu.Lock()

@mellon85 mellon85 Oct 17, 2025

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.

there's no need for this to be protected by a mutex, it's an internal function and is guaranteed by the caller to be called only by one goroutine exclusively, this forces to serialize operations that are not supposed to be.

If you want to change the structure then the various functionality should still be independent as they are now. i.e. separate them in their own struct will show why there was no need for those mutex locks to being with.

As an example: #42072

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I incorporated your commit.

Comment thread pkg/config/remote/service/clients.go Outdated
windowDuration time.Duration
capacity int
allowance int
mu struct {

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.

this struct is never called by multiple goroutines, while this adds close to no overhead it's not necessary

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see that now. I read the code more closely, thanks for the push. After reading more closely it became clearer to me that this thing is just a rate limiter and the channel doesn't need to be coupled to it. I renamed it and added commentary clarifying the usage. See if it pleases oyu./

A better refactor might be to pull the state of the refresh loop out of the service struct, but I'll not do that here.

@ajwerner
ajwerner force-pushed the ajwerner/rc-service-cleanup branch from 4a0d230 to 3b314d6 Compare October 17, 2025 15:54

@ajwerner ajwerner left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mellon85 thanks for the review!

Comment thread pkg/config/remote/service/service.go Outdated
response, err := s.api.FetchOrgStatus(context.Background())
response, err := s.getAPI().FetchOrgStatus(context.Background())

s.mu.Lock()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I incorporated your commit.

Comment thread pkg/config/remote/service/clients.go Outdated
windowDuration time.Duration
capacity int
allowance int
mu struct {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see that now. I read the code more closely, thanks for the push. After reading more closely it became clearer to me that this thing is just a rate limiter and the channel doesn't need to be coupled to it. I renamed it and added commentary clarifying the usage. See if it pleases oyu./

A better refactor might be to pull the state of the refresh loop out of the service struct, but I'll not do that here.

@github-actions github-actions Bot added long review PR is complex, plan time to review it and removed medium review PR review might take time labels Oct 17, 2025
Comment thread pkg/config/remote/service/service.go Outdated
func (s *CoreAgentService) getAPI() api.API {
s.mu.Lock()
defer s.mu.Unlock()
return s.mu.api

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.

this method shows the api shouldn't be in the mutex lock as anyway it removes any protection, which means that also the UpdatePARJWT call doesn't need the mutex

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right. I confused myself at some point into thinking that api could be reset in the way db and uptane could. Fixed.

}

func (s *CoreAgentService) refresh() error {
s.Lock()

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.

this is nice, thanks for removing the exposed mutex!

Comment thread pkg/config/remote/service/service.go Outdated
if err != nil {
s.Unlock()
s.mu.Unlock()

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.

shouldn't this be in a defer? we must guaranteee it's unlocked and it's always locked or in the critical points where we don't need it locked it's unlocked and then locked

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. In this specific case I think it was fine, but in general I agree that having defers is better. I added a commit that handles the unlocking and locking around the Fetch more explicitly and delegates the top-level locking to a defer.

@@ -953,12 +1017,14 @@ func (s *CoreAgentService) ClientGetConfigs(_ context.Context, request *pbgo.Cli
return nil, err
}

targetFiles, err := s.getTargetFiles(s.uptane, neededFiles)
// TODO: Do not hold the mutex while calling getTargetFiles -- it may go to

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.

this is fine, the CDN source has never been used and there's no plan to use it in the future, so this is going to disk exclusively.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ack. There's an argument to be made that going to disk while holding the mutex is also bad, but you're right that in this context relative to other things it doesn't really matter.

Comment thread pkg/config/remote/service/service.go Outdated
Comment on lines +1273 to +1274
rcType string
db *bbolt.DB

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.

it must not have access to the db reference itself, it's owned and contained by uptane

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tacked on a commit to hide this underneath an io.Closer as that's all it's here for. I also made Close idempotent.

@ajwerner
ajwerner force-pushed the ajwerner/rc-service-cleanup branch from f15fff4 to 541b5c0 Compare October 20, 2025 14:25

@ajwerner ajwerner left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mellon85 thank you for the review! I added 3 commits to the end to address the feedback. Let me know if I missed anything

Comment thread pkg/config/remote/service/service.go Outdated
if err != nil {
s.Unlock()
s.mu.Unlock()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. In this specific case I think it was fine, but in general I agree that having defers is better. I added a commit that handles the unlocking and locking around the Fetch more explicitly and delegates the top-level locking to a defer.

Comment thread pkg/config/remote/service/service.go Outdated
Comment on lines +1273 to +1274
rcType string
db *bbolt.DB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tacked on a commit to hide this underneath an io.Closer as that's all it's here for. I also made Close idempotent.

Comment thread pkg/config/remote/service/service.go Outdated
func (s *CoreAgentService) getAPI() api.API {
s.mu.Lock()
defer s.mu.Unlock()
return s.mu.api

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right. I confused myself at some point into thinking that api could be reset in the way db and uptane could. Fixed.

@@ -953,12 +1017,14 @@ func (s *CoreAgentService) ClientGetConfigs(_ context.Context, request *pbgo.Cli
return nil, err
}

targetFiles, err := s.getTargetFiles(s.uptane, neededFiles)
// TODO: Do not hold the mutex while calling getTargetFiles -- it may go to

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ack. There's an argument to be made that going to disk while holding the mutex is also bad, but you're right that in this context relative to other things it doesn't really matter.

@ajwerner

Copy link
Copy Markdown
Contributor Author

/gitlab trigger-ci

@dd-devflow-routing-codex

dd-devflow-routing-codex Bot commented Oct 21, 2025

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2025-10-21 01:04:01 UTC ℹ️ Start processing command /gitlab trigger-ci


2025-10-21 01:04:58 UTC ℹ️ Gitlab pipeline started

Started pipeline #79804923

@ajwerner
ajwerner force-pushed the ajwerner/rc-service-cleanup branch from 541b5c0 to 84cabf7 Compare October 21, 2025 01:31
Comment thread pkg/config/remote/service/service.go Outdated

agentVersion string
db *bbolt.DB

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.

this looks great, thank you!
I have a PR that was supposed to get merged in on Friday but it's been blocked by code freezes 😀 it addresses removing db from CoreAgentService - all db interactions will now be done via the uptaneClient - #40065

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fine by me, I'm happy to rebase your change.

This `Service` structure is exported but with no users and now no methods.
Furthermore, it embeds a mutex and two fields, but the two fields that it
embeds don't actually use that mutex -- they are never modified.

Instead we hoist the fields up to the `CoreAgentService`, the actual user
of these fields.
The embedding before made `Lock()` and `Unlock()` part of the public
API of the `CoreAgentService`. This was not intentional.
The comment said it should be, but nobody ever set this channel to nil,
so subsequent calls would panic.
@ajwerner
ajwerner force-pushed the ajwerner/rc-service-cleanup branch 2 times, most recently from a603de4 to dfbd4fe Compare October 22, 2025 14:34
Comment thread pkg/config/remote/service/service.go Outdated
Comment thread pkg/config/remote/service/service.go Outdated
Isolate the state of the orgpoller to clarify synchronization.
Rename `cacheBypassClients` to `rateLimiter` and extract the bypass request
channel from the rate limiter to CoreAgentService, clarifying the threading
model and separation of concerns.
@ajwerner
ajwerner force-pushed the ajwerner/rc-service-cleanup branch from dfbd4fe to d9b07d7 Compare October 23, 2025 13:56
@ajwerner

Copy link
Copy Markdown
Contributor Author

/merge

@dd-devflow-routing-codex

dd-devflow-routing-codex Bot commented Oct 23, 2025

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2025-10-23 13:57:18 UTC ℹ️ Start processing command /merge


2025-10-23 13:57:25 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2025-10-23 15:19:27 UTC ℹ️ MergeQueue: merge request added to the queue

The expected merge time in main is approximately 40m (p90).


2025-10-23 15:33:32 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue
dd-mergequeue Bot merged commit a0b7a61 into main Oct 23, 2025
270 checks passed
@dd-mergequeue
dd-mergequeue Bot deleted the ajwerner/rc-service-cleanup branch October 23, 2025 15:33
@github-actions github-actions Bot added this to the 7.73.0 milestone Oct 23, 2025
dd-mergequeue Bot pushed a commit that referenced this pull request Oct 23, 2025
### What does this PR do?

Fixes `TestWithDatabaseFileName` to test what it was intended to.

### Motivation

Before #40065, this test was exercising the `NewService` constructor to ensure that the option was respected. In that change, the test was modified to set up a mock that returns the expected value and then to call the mock directly -- it stopped testing anything.

### Additional Notes

Stacked on #42061

Co-authored-by: andrew.werner <andrew.werner@datadoghq.com>
dd-mergequeue Bot pushed a commit that referenced this pull request Oct 23, 2025
### What does this PR do?
Properly use the injected clock everywhere.

### Motivation

It speeds up the tests, and it makes it possible to write tests involving time:

Before:
```
$ go test -tags test ./pkg/config/remote/service --count=1
ok      github.com/DataDog/datadog-agent/pkg/config/remote/service      2.213s
```
After
```
$ go test -tags test ./pkg/config/remote/service --count=1
ok      github.com/DataDog/datadog-agent/pkg/config/remote/service      0.206s
```
### Describe how you validated your changes

### Additional Notes

Stacked on #42061

Co-authored-by: andrew.werner <andrew.werner@datadoghq.com>
Comment on lines +162 to +163
mu struct {
sync.Mutex

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.

This is a cool pattern! I am curious where did you learn it from?

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

Labels

changelog/no-changelog No changelog entry needed long review PR is complex, plan time to review it qa/done QA done before merge and regressions are covered by tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants