Skip to content

volmgr: move ten groups out of executor.rs (pure move) - #564

Merged
ac-pplx merged 1 commit into
mainfrom
ac/split-volmgr-executor-1
Oct 16, 2026
Merged

ac-pplx merged 1 commit into
mainfrom
ac/split-volmgr-executor-1

Conversation

@ac-pplx

@ac-pplx ac-pplx commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

crates/libs/vol/mgr/server/src/executor.rs (4,862 code lines) is the largest production file in the workspace. Besides the transfer executor it holds:

  • the page-cache writeback coordinator;
  • the decode and decrypt sinks;
  • sparse-shadow comparison and replica provenance;
  • cgroup-based codec sizing;
  • the pull path.

This moves ten of those groups into child modules. It avoids every item an open PR edits, and the items next to them, so the payload, crypto staging, artifact job and push stay for a later step. executor.rs drops to 1,706 code lines; the largest new file is pull.rs at 662. Eleven TransferExecutor methods move into impl TransferExecutor blocks in the modules that own them.

This is a pure move, generated by scripts/split_rust_module.py (#562) from the spec below and proved by its verify. It is part of splitting the largest production modules into smaller ones with one job each.

What moved where

Module Holds Code lines
codec_concurrency How many codec workers to run, from the configured CPU request and the cgroup CPU quota. 52
artifact_role Which snapshot artifact a transfer carries, and the materialization mode and block size it uses. 87
transfer_metrics Transfer phases and the duration, throughput and decompression-allocation metrics they emit. 139
sparse_shadow The sparse-shadow rollout rung: compare a sparse materialization with a dense one, and schedule those comparisons. 603
replica_provenance The provenance recorded next to a pulled file, and whether an existing replica can be reused. 176
stream_pull_timing Per-stage timing of a streamed pull and the summary it logs. 112
writeback_coordinator Page-cache writeback pacing shared by every streamed pull: ranges, the byte budget and the coordinator. 620
stream_writeback One stream's writeback state: dirty-range accumulation, reservations and flushes. 294
decode_sinks The writers a pulled stream decodes into: optional zstd, decryption, and the streamed-pull admission. 477
pull Pull jobs: destination locks, the existing-file policy, cancellation cleanup and the pull itself. 662
executor.rs (parent) the transfer executor, its config and codec admission (with the frame-size check it uses), the job entry points, payload and crypto staging, the artifact job, push, the destination pull locks, and the cancellation helper push and pull share 1706

How to check it

The spec (item names, not lines):

file = "crates/libs/vol/mgr/server/src/executor.rs"

[[module]]
name = "codec_concurrency"
allow_unused_glob = true
doc = "How many codec workers to run, from the configured CPU request and the cgroup CPU quota."
items = [
  "fn resolved_codec_concurrency", "fn cpu_request_cores_from_env",
  "fn parse_cpu_request_cores", "fn cores_from_cfs_quota", "fn detect_cgroup_cpu_quota_cores",
]

[[module]]
name = "artifact_role"
doc = "Which snapshot artifact a transfer carries, and the materialization mode and block size it uses."
items = [
  "const MEMORY_SPARSE_BLOCK_BYTES", "const ROOTFS_SPARSE_BLOCK_BYTES", "fn transfer_artifact_name", "fn snapshot_artifact_from_local_path", "fn snapshot_artifact_from_entry_role",
  "fn transfer_artifact_tag", "fn effective_materialization_mode", "fn execution_materialization_mode",
  "fn materialization_block_bytes",
]

[[module]]
name = "transfer_metrics"
doc = "Transfer phases and the duration, throughput and decompression-allocation metrics they emit."
items = [
  "enum TransferPhase", "impl TransferPhase", "impl std::fmt::Display for TransferPhase",
  "fn emit_transfer_phase_duration", "fn emit_transfer_throughput_mib_s", "fn observe_decompression_allocation",
]

[[module]]
name = "sparse_shadow"
doc = "The sparse-shadow rollout rung: compare a sparse materialization with a dense one, and schedule those comparisons."
items = [
  "enum ShadowComparisonOutcome", "impl ShadowComparisonOutcome", "impl std::fmt::Display for ShadowComparisonOutcome",
  "struct ShadowComparisonObservation", "fn sparse_shadow_path", "fn sparse_shadow_source_path",
  "enum SparseShadowCmpError", "impl SparseShadowCmpError", "fn compare_completed_files",
  "fn advise_shadow_read_range", "fn compare_sparse_shadow", "struct ShadowComparisonLifecycle",
  "struct ShadowComparisonController", "impl ShadowComparisonController", "struct SparseShadowTask",
  "struct SparseShadowAdmission", "fn sparse_shadow_disk_floor_bytes", "fn sparse_shadow_has_disk_headroom",
  "fn remove_sparse_shadow_snapshot",
  "impl TransferExecutor :: fn try_acquire_sparse_shadow_admission",
  "impl TransferExecutor :: fn schedule_sparse_shadow_comparison",
  "impl TransferExecutor :: fn spawn_sparse_shadow_comparison",
]

[[module]]
name = "replica_provenance"
doc = "The provenance recorded next to a pulled file, and whether an existing replica can be reused."
items = [
  "struct MaterializationObservation", "struct FileReplicaProvenance", "impl FileReplicaProvenance",
  "fn file_replica_provenance", "fn read_materialization_observation", "fn materialization_is_compatible",
  "fn requested_file_mode_bits",
  "impl TransferExecutor :: fn validate_existing_file_replica",
  "impl TransferExecutor :: fn write_file_replica_provenance",
  "impl TransferExecutor :: fn invalidate_file_replica_provenance",
]

[[module]]
name = "stream_pull_timing"
doc = "Per-stage timing of a streamed pull and the summary it logs."
items = [
  "struct StreamPullTiming", "fn current_thread_cpu_us", "impl StreamPullTiming",
  "fn log_streamed_pull_pipeline_timing",
]

[[module]]
name = "writeback_coordinator"
doc = "Page-cache writeback pacing shared by every streamed pull: ranges, the byte budget and the coordinator."
items = [
  "const STREAM_WRITEBACK_*", "const DEFAULT_STREAM_WRITEBACK_BUDGET_BYTES",
  "struct StreamWritebackRange", "enum StreamWritebackCommand", "trait StreamPageCacheOperations",
  "struct SystemStreamPageCacheOperations", "impl StreamPageCacheOperations for SystemStreamPageCacheOperations",
  "struct StreamWritebackBudget", "struct StreamWritebackCoordinator", "enum StreamWritebackFailureOperation",
  "impl StreamWritebackFailureOperation", "impl std::fmt::Display for StreamWritebackFailureOperation",
  "enum StreamWritebackPriorityClass", "impl StreamWritebackPriorityClass",
  "impl std::fmt::Display for StreamWritebackPriorityClass", "fn fail_stream_writeback_budget",
  "fn release_stream_writeback_budget", "fn emit_stream_writeback_outstanding_if_due",
  "fn should_emit_stream_writeback_outstanding", "fn retire_stream_writeback_range",
  "fn stream_writeback_outstanding_bytes", "fn pending_stream_writeback_debt",
  "fn drain_stream_writeback_ranges_to", "fn drain_all_stream_writeback_ranges",
  "fn queue_stream_writeback_range", "fn start_pending_stream_writeback_ranges",
  "impl StreamWritebackCoordinator",
]

[[module]]
name = "stream_writeback"
doc = "One stream's writeback state: dirty-range accumulation, reservations and flushes."
items = ["struct StreamWritebackAccumulator", "struct StreamWriteback", "impl StreamWriteback", "impl Drop for StreamWriteback"]

[[module]]
name = "decode_sinks"
doc = "The writers a pulled stream decodes into: optional zstd, decryption, and the streamed-pull admission."
items = [
  "const DECRYPT_PULL_WRITE_BUF_BYTES", "trait CompletingWrite", "impl<W: Write + ?Sized> CompletingWrite for &mut W", "struct MaybeZstdDecodingWriter",
  "enum MaybeZstdState", "struct BufferedFlushError", "impl BufferedFlushError", "fn identity_decoding_sink",
  "impl<W: Write> MaybeZstdDecodingWriter<*", "impl<Input, Output, Select> MaybeZstdDecodingWriter<*",
  "impl<Input, Output, Select> Write for MaybeZstdDecodingWriter<*", "enum DecryptedFileSinkInner",
  "struct DecryptedFileSink", "impl DecryptedFileSink", "impl CompletingWrite for DecryptedFileSink",
  "impl Write for DecryptedFileSink", "type StreamSinkSelector", "struct StreamDecodeSink",
  "impl StreamDecodeSink", "impl Write for StreamDecodeSink",
  "impl pplx_space_object_store::FinishingWrite for StreamDecodeSink", "struct StreamedPullAdmission",
  "impl pplx_space_object_store::DecodeAdmission for StreamedPullAdmission<'_>",
]

[[module]]
name = "pull"
doc = "Pull jobs: destination locks, the existing-file policy, cancellation cleanup and the pull itself."
items = [
  "struct DestinationPullGuard", "impl Drop for DestinationPullGuard", "enum PullExistingPolicy",
  "impl PullExistingPolicy", "struct PullExecutionOptions", "fn unlink_partial",
  "fn cleanup_cancelled_pull_temp_paths",
  "impl TransferExecutor :: fn lock_pull_destination", "impl TransferExecutor :: fn cleanup_cancelled_pull_path",
  "impl TransferExecutor :: fn execute_pull_cancellable", "impl TransferExecutor :: fn execute_pull",
  "impl TransferExecutor :: fn execute_pull_locked",
]

The tool is scripts/split_rust_module.py from #562 (not on main yet).

  • Regenerate: on main at b9fc395e0, run split_rust_module.py apply spec.toml && cargo fmt --all. This reproduces the first commit.
  • Proof: split_rust_module.py verify origin/main → regenerated and compared 11 files from 1 split parents: identical. verify derives the spec from the diff, regenerates the split from the merge base, and requires HEAD's Rust files to equal the rustfmt'd result byte for byte.
  • Edits inside moved text: 210 private items, fields or methods → pub(super); 11 methods moved into impl blocks with the same header. Each keeps the reach the item had: one module deeper, pub(super) reaches exactly the old module.

Observable changes

  • Log targets: 28 tracing events without an explicit target: now log under the new module paths (pplx_space_volmgr::executor::<module> instead of pplx_space_volmgr::executor). Nothing in the repository selects on these targets exactly.

  • Smoke harness: space_smoke matches the log message "transfer job completed", which execute_single emits; it stays in executor.rs.

  • Diagnostics name new locations:

    • panic and backtrace locations of the 13 panic sites (expect, unwrap, unreachable!, asserts) in moved code name the new file and line;
    • the 5 panic sites left in the parent shift line numbers;
    • the 28 tracing callsites in moved code report the new file and line in their metadata, including OpenTelemetry's code.filepath, code.lineno and code.namespace;
    • std::any::type_name of a moved type names its new module;

    The reviews found nothing in the repository that depends on these.

Validation

Run on a Linux development machine at the PR head:

  • cargo check --workspace --all-targets --all-features --locked: pass, no warnings.
  • Every clippy invocation in run_clippy_checks with -D warnings: pass. codec_concurrency uses nothing from its parent, so it keeps use super::*; under #[allow(unused_imports)] (the spec's allow_unused_glob).
  • cargo check -p pplx-space-volmgr --no-default-features --all-targets: pass.
  • cargo nextest run -p pplx-space-volmgr --all-features --run-ignored all with Postgres and SPACE_CI_REQUIRE_TEST_DEPS=1: 219 of 220 passed, including all 116 executor::tests.
  • The 220th is the privileged staging::tests::nocow_upload_and_shadow_staging_keep_reflinks. It needs a loop-backed btrfs, which this revision's machine lacked, so it did not run. It passed as root on the first version of this PR, and staging is untouched; CI's privileged job runs it.
  • archcheck as CI runs it: no required findings. The only new advisory finding is the stale file_size entry below.

Interactions

Open PRs touching executor.rs: #346, #351 (draft) and #139. Their hunks were mapped onto current main by content. Every item they edit stays in executor.rs, so they rebase without conflicts in moved code: the payload, TransferEncryption, crypto staging, the artifact job, push, with_store/new, TransferExecutorConfig and the JobExecutor impl.

Follow-ups, kept out of this PR because verify allows no other Rust edits: imports in executor.rs that only the child modules use could move to them.

The archcheck file_size baseline entry for this file is now stale (an advisory finding). Split PRs leave that table alone, so they don't conflict with each other there; one follow-up prunes the stale entries after they land.

Rollback

Revert the commit(s). No data, configuration, wire or public API change.

🤖 Generated with Claude Code

@ac-pplx
ac-pplx force-pushed the ac/split-volmgr-executor-1 branch from b660681 to b7f2a9f Compare October 16, 2026 06:47
executor.rs is the largest production file in the workspace. Besides the
transfer executor it holds the page-cache writeback coordinator, the
decode and decrypt sinks, sparse-shadow comparison, replica provenance,
cgroup codec sizing and the pull path. Move those, which no open pull
request edits, into child modules: codec_concurrency, artifact_role,
transfer_metrics, sparse_shadow, replica_provenance, stream_pull_timing,
writeback_coordinator, stream_writeback, decode_sinks and pull. Payload,
crypto staging, the artifact job and push stay for a later step.

Generated by scripts/split_rust_module.py (#562) from the spec in the
pull request. Items move verbatim. The only edits keep each item's
reach: 210 private items, fields or methods become pub(super). The
parent re-exports the moved items, so no caller changes.
@ac-pplx
ac-pplx force-pushed the ac/split-volmgr-executor-1 branch from b7f2a9f to 2302bb2 Compare October 16, 2026 07:11
@ac-pplx
ac-pplx marked this pull request as ready for review October 16, 2026 14:49
@the-gigi-pplx

Copy link
Copy Markdown
Collaborator

Review

No findings.

Verified against 2302bb201827948207298b55902599c74ab7d596.

What I ran:

  • scripts/split_rust_module.py verify origin/main, using the script from PR scripts: add split_rust_module.py for mechanical, verifiable Rust module splits #562's head: identical, exit 0.
  • apply with the spec in this PR's body, run on the merge base and rustfmt'd: it reproduces every .rs file at the head byte for byte. So the glob patterns in the published spec select the same items as the expanded spec verify --print-spec derives.
  • Each moved child file, grepped for self::/super:: in doc comments. The tool doesn't rewrite comments, so a link like that would point one module off after the move. There are none.
  • The non-split files, grepped for comments that name the old file next to a moved item.
  • Whether main has touched the split file since the merge base: it hasn't.

What I didn't check: whether the grouping is the right one. That's a design call, and I took the body's word for it. I also didn't rebuild or retest the crate; CI is green on this head.

@the-gigi-pplx the-gigi-pplx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved. No findings; details in the review comment above.

@ac-pplx
ac-pplx added this pull request to the merge queue Oct 16, 2026
Merged via the queue into main with commit f918274 Oct 16, 2026
20 checks passed
@ac-pplx
ac-pplx deleted the ac/split-volmgr-executor-1 branch October 16, 2026 16:35
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.

3 participants