[ENH]: limit concurrency on operators spawned by GC #5720

codetheweb · 2025-10-22T18:29:15Z

Description of changes

When running the garbage collector service, we see it sometimes OOM. This is because ListFilesAtVersionsOperator can allocate a lot of memory and currently has unbounded concurrency.

This PR adds a new config parameter max_concurrent_list_files_operations_per_collection to limit the concurrency of this operator. Doing this required a bit of restructuring in the GC orchestrator.

Test plan

How are these changes tested?

Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Migration plan

Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?

Observability plan

What is the plan to instrument and monitor this change?

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?

codetheweb · 2025-10-22T18:29:32Z

[ENH]: limit concurrency on operators spawned by GC #5720 👈 (View in Graphite)
[CLN]: remove v1 of garbage collection #5627 : 1 other dependent PR (#5628 )
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2025-10-22T18:29:35Z

propel-code-bot · 2025-10-22T20:02:59Z

Introduce Concurrency Limit for GC ListFiles Operations to Reduce Memory Usage

This PR introduces a configurable concurrency limit for the ListFilesAtVersionsOperator invoked by the garbage collector (GC) service. A new config parameter max_concurrent_list_files_operations_per_collection is added to restrict the number of concurrent list files operations per collection, targeting observed out-of-memory (OOM) issues in GC runs due to previously unbounded concurrency. Implementation of the concurrency control required notable refactoring of the GC orchestrator logic, particularly consolidating and simplifying the stage that handles marking versions at sysdb and launching list files operators. The code also removes the now-redundant mark_versions_at_sysdb step and its operator, and reorganizes related logic to leverage direct sysdb client calls.

Key Changes

• Added config option max_concurrent_list_files_operations_per_collection to the GC config; default set to 10.
• Refactored GarbageCollectorOrchestrator to coordinate marking versions as deleted in sysdb and then spawning list files tasks with bounded concurrency using futures::stream::buffer_unordered.
• Removed the separate mark_versions_at_sysdb operator, collapsing its logic directly into the orchestrator.
• Updated affected orchestrator handler signatures and streamlined task dispatch for list files and subsequent file deletion steps.
• Modified function/thread signatures and test utility invocations to account for the new concurrency parameter.
• Introduced OneshotMessageReceiver for improved async message handling in Rust's actor-like system.
• Updated multiple sites across CLI tooling, tests, and configuration parsing to pass and utilize the new concurrency limit.

Affected Areas

• rust/garbage_collector/src/garbage_collector_orchestrator_v2.rs - major refactor and logic update
• rust/garbage_collector/src/config.rs - config struct and defaults update
• rust/garbage_collector/src/bin/garbage_collector_tool.rs - CLI updated to accept/use the new param
• rust/system/src/receiver.rs - new message receiver infrastructure for oneshot channels
• rust/garbage_collector/src/operators/mark_versions_at_sysdb.rs - deleted
• Test helpers and orchestrator construction in various test modules

This summary was automatically generated by @propel-code-bot

propel-code-bot · 2025-10-22T20:04:23Z

rust/garbage_collector/src/config.rs

+    #[serde(
+        default = "GarbageCollectorConfig::default_max_concurrent_list_files_operations_per_collection"
+    )]
+    pub(super) max_concurrent_list_files_operations_per_collection: usize,


[BestPractice]

The new configuration field max_concurrent_list_files_operations_per_collection has pub(super) visibility, which is inconsistent with other pub fields like min_versions_to_keep. This prevents garbage_collector_tool.rs from accessing this value from the loaded config, forcing it to use a hardcoded value.

Please consider making this field pub for consistency. This will also allow the tool to use the configured value.

Suggested change

pub(super) max_concurrent_list_files_operations_per_collection: usize,

pub max_concurrent_list_files_operations_per_collection: usize,

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Context for Agents

[**BestPractice**] The new configuration field `max_concurrent_list_files_operations_per_collection` has `pub(super)` visibility, which is inconsistent with other `pub` fields like `min_versions_to_keep`. This prevents `garbage_collector_tool.rs` from accessing this value from the loaded config, forcing it to use a hardcoded value. Please consider making this field `pub` for consistency. This will also allow the tool to use the configured value. ```suggestion pub max_concurrent_list_files_operations_per_collection: usize, ``` ⚡ **Committable suggestion** Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. File: rust/garbage_collector/src/config.rs Line: 40

propel-code-bot · 2025-10-22T20:04:24Z

rust/garbage_collector/src/bin/garbage_collector_tool.rs

                config.min_versions_to_keep,
                enable_log_gc,
                enable_dangerous_option_to_ignore_min_versions_for_wal3,
+                10,


[BestPractice]

This hardcoded value could be replaced by the value from the configuration. I've left a suggestion on rust/garbage_collector/src/config.rs to make the max_concurrent_list_files_operations_per_collection field public. Once that's done, you can use config.max_concurrent_list_files_operations_per_collection here.

Suggested change

10,

config.max_concurrent_list_files_operations_per_collection,

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Context for Agents

[**BestPractice**] This hardcoded value could be replaced by the value from the configuration. I've left a suggestion on `rust/garbage_collector/src/config.rs` to make the `max_concurrent_list_files_operations_per_collection` field public. Once that's done, you can use `config.max_concurrent_list_files_operations_per_collection` here. ```suggestion config.max_concurrent_list_files_operations_per_collection, ``` ⚡ **Committable suggestion** Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. File: rust/garbage_collector/src/bin/garbage_collector_tool.rs Line: 248

propel-code-bot · 2025-10-22T21:19:46Z

rust/garbage_collector/src/garbage_collector_orchestrator_v2.rs

-            None => return,
-        };
-        let res = self.handle_list_files_at_version_output(output, ctx).await;
+        tracing::info!("res: {:?}", res);


[BestPractice]

Debug logging statement left in production code. This appears to be temporary debugging code that should be removed before merging.

Suggested change

tracing::info!("res: {:?}", res);

self.ok_or_terminate(res, ctx).await;

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Context for Agents

[**BestPractice**] Debug logging statement left in production code. This appears to be temporary debugging code that should be removed before merging. ```suggestion self.ok_or_terminate(res, ctx).await; ``` ⚡ **Committable suggestion** Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. File: rust/garbage_collector/src/garbage_collector_orchestrator_v2.rs Line: 1042

propel-code-bot · 2025-10-22T22:42:24Z

rust/garbage_collector/src/garbage_collector_orchestrator_v2.rs

+        let mut stream = futures::stream::iter(output.versions.into_iter().flat_map(
+            |(collection_id, versions)| {
+                versions
+                    .keys()
+                    .map(|version| (collection_id, *version))
+                    .collect::<Vec<_>>()
+            },
+        ))


[PerformanceOptimization]

For a minor performance improvement, you can avoid collecting into a Vec inside the flat_map. The flat_map can directly work with the iterator returned by map, which avoids an intermediate allocation for each collection's versions.

Context for Agents

[**PerformanceOptimization**] For a minor performance improvement, you can avoid collecting into a `Vec` inside the `flat_map`. The `flat_map` can directly work with the iterator returned by `map`, which avoids an intermediate allocation for each collection's versions. File: rust/garbage_collector/src/garbage_collector_orchestrator_v2.rs Line: 409

propel-code-bot · 2025-10-22T22:42:25Z

rust/system/src/receiver.rs

+        sender
+            .send(message)
+            .map_err(|_| ChannelError::SendError("Failed to send message".to_string()))?;


[BestPractice]

The error from oneshot::Sender::send occurs when the receiver has been dropped, indicating the receiver is no longer interested in the response. The current error message is generic. A more specific message would help debugging. The send method returns Result<(), T> where the Err(T) contains the original message on failure. You can ignore this with let _ = tx.send(value); or explicitly acknowledge it with if let Err(_message) = tx.send(value) { /* handle receiver dropped */ } to make the intention clear.

Context for Agents

[**BestPractice**] The error from `oneshot::Sender::send` occurs when the receiver has been dropped, indicating the receiver is no longer interested in the response. The current error message is generic. A more specific message would help debugging. The `send` method returns `Result<(), T>` where the `Err(T)` contains the original message on failure. You can ignore this with `let _ = tx.send(value);` or explicitly acknowledge it with `if let Err(_message) = tx.send(value) { /* handle receiver dropped */ }` to make the intention clear. File: rust/system/src/receiver.rs Line: 95

HammadB · 2025-10-28T09:22:13Z

rust/garbage_collector/src/operators/mark_versions_at_sysdb.rs

@@ -1,163 +0,0 @@
-use async_trait::async_trait;


as this just unused?

yes, inlined into rust/garbage_collector/src/garbage_collector_orchestrator_v2.rs

This was referenced Oct 22, 2025

[CLN]: remove v1 of garbage collection #5627

Merged

[ENH]: use FSTs for garbage collection #5628

Closed

codetheweb force-pushed the feat-gc-limit-operation-concurrency branch 4 times, most recently from 418aef2 to 4380f03 Compare October 22, 2025 19:57

codetheweb marked this pull request as ready for review October 22, 2025 20:02

codetheweb requested a review from sanketkedia October 22, 2025 20:02

propel-code-bot bot reviewed Oct 22, 2025

View reviewed changes

codetheweb force-pushed the feat-gc-limit-operation-concurrency branch from 4380f03 to a9a8189 Compare October 22, 2025 21:17

propel-code-bot bot reviewed Oct 22, 2025

View reviewed changes

codetheweb changed the base branch from cln-remove-gc-v1 to graphite-base/5720 October 22, 2025 22:38

codetheweb force-pushed the graphite-base/5720 branch from 8646eac to 4545ab3 Compare October 22, 2025 22:38

codetheweb force-pushed the feat-gc-limit-operation-concurrency branch from a9a8189 to 468e9cc Compare October 22, 2025 22:38

graphite-app bot changed the base branch from graphite-base/5720 to main October 22, 2025 22:38

[ENH]: limit concurrency on operators spawned by GC

d63ad47

codetheweb force-pushed the feat-gc-limit-operation-concurrency branch from 468e9cc to d63ad47 Compare October 22, 2025 22:38

propel-code-bot bot reviewed Oct 22, 2025

View reviewed changes

HammadB reviewed Oct 28, 2025

View reviewed changes

HammadB approved these changes Oct 28, 2025

View reviewed changes

codetheweb merged commit 0f1292e into main Oct 28, 2025
62 checks passed

codetheweb deleted the feat-gc-limit-operation-concurrency branch October 28, 2025 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ENH]: limit concurrency on operators spawned by GC #5720

[ENH]: limit concurrency on operators spawned by GC #5720

Uh oh!

codetheweb commented Oct 22, 2025 •

edited

Loading

Uh oh!

codetheweb commented Oct 22, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Oct 22, 2025

Uh oh!

propel-code-bot bot commented Oct 22, 2025 •

edited

Loading

Uh oh!

propel-code-bot bot Oct 22, 2025

Uh oh!

propel-code-bot bot Oct 22, 2025

Uh oh!

propel-code-bot bot Oct 22, 2025

Uh oh!

propel-code-bot bot Oct 22, 2025

Uh oh!

propel-code-bot bot Oct 22, 2025

Uh oh!

HammadB Oct 28, 2025

Uh oh!

codetheweb Oct 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	pub(super) max_concurrent_list_files_operations_per_collection: usize,
	pub max_concurrent_list_files_operations_per_collection: usize,

	10,
	config.max_concurrent_list_files_operations_per_collection,

	tracing::info!("res: {:?}", res);
	self.ok_or_terminate(res, ctx).await;

[ENH]: limit concurrency on operators spawned by GC #5720

[ENH]: limit concurrency on operators spawned by GC #5720

Uh oh!

Conversation

codetheweb commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Test plan

Migration plan

Observability plan

Documentation Changes

Uh oh!

codetheweb commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 22, 2025

Reviewer Checklist

Testing, Bugs, Errors, Logs, Documentation

System Compatibility

Quality

Uh oh!

propel-code-bot bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

propel-code-bot bot Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

propel-code-bot bot Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

propel-code-bot bot Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

propel-code-bot bot Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

propel-code-bot bot Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

codetheweb Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codetheweb commented Oct 22, 2025 •

edited

Loading

codetheweb commented Oct 22, 2025 •

edited

Loading

propel-code-bot bot commented Oct 22, 2025 •

edited

Loading