Skip to content

feat(amp-worker-gc): standalone GC job for control-plane scheduling#1989

Closed
mitchhs12 wants to merge 11 commits intomainfrom
mitchhs12/gc-job-extraction
Closed

feat(amp-worker-gc): standalone GC job for control-plane scheduling#1989
mitchhs12 wants to merge 11 commits intomainfrom
mitchhs12/gc-job-extraction

Conversation

@mitchhs12
Copy link
Contributor

@mitchhs12 mitchhs12 commented Mar 17, 2026 •
edited
Loading

Summary

Extracts garbage collection from the worker's compaction task into a standalone job type managed by the controller via the job ledger. Previously, GC only ran as a side effect of compaction — if no materialization jobs were active, no garbage was ever collected. This PR gives GC its own independent lifecycle with scheduling, retries, and observability.

Key design decisions

  • Metadata deleted before physical files: If the process crashes mid-GC, you get orphaned files on disk (harmless) rather than dangling metadata pointing to missing files (bad). This is intentional.
  • Pre-filter by expired gc_manifest entries: The scheduler queries gc_manifest for locations with expired files rather than scanning all revisions. This avoids creating no-op jobs for locations that don't need cleanup.
  • GC scheduling is off by default (enabled = false): Deploying this code changes nothing until the config toggle is flipped. This allows safe rollout — enable the new GC only after disabling the legacy worker-internal GC.
  • Deactivated revisions are GC-eligible: GC is driven by the state of the gc_manifest, not the revision's active status. A deactivated table with expired files still gets cleaned up.
  • Scheduler deduplication and throttling: In-flight jobs are not duplicated, and recently-completed jobs are not immediately rescheduled (respects the gc_interval cooldown).

Changes

  • New crate amp-worker-gc: Job descriptor, idempotency key, error types with retryability classification, OTel metrics, and collection algorithm (stream expired files → delete metadata → delete physical files)
  • Worker dispatch: New Gc variant in JobDescriptor enum with execution wiring in job_impl.rs
  • Controller scheduling: schedule_gc_jobs() method with pre-filter query, in-flight dedup, and completion cooldown. Background task spawned conditionally based on config.
  • Controller metrics: gc_jobs_dispatched_total, gc_jobs_skipped_in_flight_total, gc_jobs_skipped_too_recent_total
  • Config toggle: GcSchedulingConfig with enabled (default false) and interval (default 60s)
  • Metadata DB: New gc::locations_with_expired_entries() query for scheduler pre-filtering
  • Integration tests: 7 tests covering the full GC execution pipeline, scheduler pre-filtering, deactivated revision handling, in-flight dedup, and completion throttling

Follow-up

A follow-up PR will be required to remove the legacy worker-internal GC code from compaction, as per the phased rollout plan.

Test plan

  • GC deletes expired files (metadata + physical) end-to-end
  • GC is a no-op when no files are expired
  • GC returns LocationNotFound for invalid location IDs
  • Scheduler only creates jobs for locations with expired gc_manifest entries
  • Scheduler creates jobs for deactivated revisions
  • Scheduler skips locations with in-flight GC jobs
  • Scheduler skips locations where GC completed within the cooldown interval

@mitchhs12 mitchhs12 force-pushed the mitchhs12/gc-job-extraction branch from c571586 to 74eb098 Compare March 18, 2026 15:13
@mitchhs12 mitchhs12 marked this pull request as ready for review March 18, 2026 16:13
@mitchhs12 mitchhs12 force-pushed the mitchhs12/gc-job-extraction branch from c95850e to ec68363 Compare March 20, 2026 16:21
@mitchhs12 mitchhs12 requested a review from JohnSwan1503 March 20, 2026 19:29
mitchhs12 added 11 commits March 22, 2026 15:32
@mitchhs12
feat(amp-worker-gc): add standalone GC job type for control-plane sch…
8d7c709 
…eduling

Extract garbage collection from the worker's compaction task into a standalone
job type managed by the controller via the job ledger. This decouples GC from
compaction so it can run independently regardless of whether materialization
jobs are active.

New crate: amp-worker-gc with job descriptor, idempotency key, and collection
algorithm. Controller schedules GC jobs per active physical table revision on
a 60s interval. Workers execute them using the same stream-expired → delete-
metadata → delete-files algorithm as the existing Collector.
@mitchhs12
feat(amp-config): add GC scheduling config toggle for controller
91ec827 
Add GcSchedulingConfig with `enabled` (default false) and `interval`
(default 60s) fields so GC scheduling can be toggled without code changes.
The controller only spawns the GC scheduling task when enabled, preventing
unintended GC job creation on deployment.
@mitchhs12
feat(amp-worker-gc): add metrics, last_success_at scheduling, and int…
85257d7 
…egration tests

- Add GcMetrics (expired_files_found, metadata_entries_deleted, files_deleted,
  files_not_found) with OpenTelemetry counters keyed by location_id
- Add last_success_at check in schedule_gc_jobs() to respect the configured
  interval between GC runs per location (RFC compliance)
- Add 3 integration tests in tests/src/tests/it_gc.rs verifying the full
  collection algorithm against real Postgres + filesystem
- Update config.sample.toml with [gc_scheduling] section
@mitchhs12
fix(monitoring): add amp_worker_gc to workspace crates list
963cf0d 
The workspace_crates_match_amp_crates_list test validates that the
hardcoded AMP_CRATES list matches actual workspace members. Adding the
new amp-worker-gc crate to the workspace requires updating this list.
@mitchhs12
feat(controller,amp-worker-gc): add controller-level metrics and tabl…
c315b32 
…e_ref tracing

- Add GcSchedulerMetrics to controller scheduler with gc_jobs_dispatched_total,
  gc_jobs_skipped_in_flight_total, and gc_jobs_skipped_too_recent_total counters
- Add table_ref field to GC job execution tracing span, recorded from the
  revision path after lookup
- Pass Meter to Scheduler for metrics initialization
@mitchhs12
refactor(controller): pre-filter GC scheduling by expired gc_manifest…
d754185 
… entries

Instead of paginating through all active revisions and scheduling no-op
GC jobs, query gc_manifest for distinct location_ids with expired entries.
This reduces unnecessary job ledger writes and worker notifications.

Also removes the active-only filter so deactivated revisions are GC-eligible.
GC is only concerned with the state of the gc_manifest, not the revision's
active status.
@mitchhs12
refactor(controller): pre-filter GC scheduling by expired gc_manifest…
f474641 
… entries

Instead of paginating through all active revisions and scheduling no-op
GC jobs, query gc_manifest for distinct location_ids with expired entries.
This reduces unnecessary job ledger writes and worker notifications.

Also removes the active-only filter so deactivated revisions are GC-eligible.
GC is only concerned with the state of the gc_manifest, not the revision's
active status.

Add gc::locations_with_expired_entries() query to metadata-db and 3 integration
tests verifying pre-filter behavior, empty results, and deactivated revisions.
@mitchhs12
refactor(tests): use MetadataDb fixture and GcTestCtx wrapper in GC t…
f375e7a 
…ests

Replace raw pgtemp usage with the testlib MetadataDbFixture for consistency
with other integration tests. Extract shared setup into a GcTestCtx wrapper
struct with helper methods (register_revision, register_file, file_ids,
gc_context, data_store). Remove pgtemp direct dependency from tests crate.
@mitchhs12
chore: update Cargo.lock after removing pgtemp from tests
516f0ac
@mitchhs12
test(tests): add scheduler dedup and throttle tests for GC jobs
cf110ba 
Add two integration tests covering the scheduler's in-flight skip
and recently-completed cooldown logic in schedule_gc_jobs(), plus
helper methods on GcTestCtx to reduce boilerplate.
@mitchhs12
fix(controller): make scheduler module public for integration tests
1edae45
@mitchhs12 mitchhs12 force-pushed the mitchhs12/gc-job-extraction branch from 821dc2d to 1edae45 Compare March 22, 2026 20:32
LNSD
LNSD requested changes Mar 23, 2026
View reviewed changes
Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

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

Can you chunk this PR down into smaller pieces?

For example, one PR for the amp-worker-gc job crate; then another to wire it to the worker, and after that another to wire it to the scheduler.

@mitchhs12 mitchhs12 closed this Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LNSD LNSD LNSD requested changes

@JohnSwan1503 JohnSwan1503 Awaiting requested review from JohnSwan1503

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mitchhs12 @LNSD