Skip to content

fix(runner): make managed resource gc reference-safe - #30093

Open
seven332 wants to merge 17 commits into
mainfrom
fix/29907-explicit-runner-gc-ownership
Open

fix(runner): make managed resource gc reference-safe#30093
seven332 wants to merge 17 commits into
mainfrom
fix/29907-explicit-runner-gc-ownership

Conversation

@seven332

@seven332 seven332 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • treat every real, UTF-8 immediate child of the managed bin/ and runners/ roots as an independent GC candidate, with path validation independent of Runner/service creation grammar, while discovering persistent Runner services separately
  • resolve exact managed-resource references from persistent and loaded transient unit commands, activation configs, live Runner records, canonical base-directory locks, and the current GC executable; external executable/config paths remain outside GC ownership, and equal names never create ownership
  • retain activation config snapshots for deterministic service startup, resolve their Runner directory from the existing path layout, and remove the attempted source-config metadata contract
  • uninstall eligible inactive persistent services before deleting now-unreferenced resources; retain exact dependencies on uninstall failure or a persistent unit file after an apparently successful uninstall
  • recheck directory identity, completeness, and recency immediately before deletion, and remove a service lock only after the installed unit file is confirmed absent
  • derive image protection only from the final retained config set and skip image deletion when service/resource inventory or a readable retained config is incomplete
  • declare and document service suffix, binary dirname, Runner dirname, and release artifact identity as independent concepts, while preserving the hidden --protect-version and old-Runner capability probe only for rollout compatibility

Explicit exclusions

  • no deployment registry, manifest, bundle identity, or source-config provenance metadata
  • no semantic-version parsing or equal-name association across service, binary, and Runner namespaces
  • no removal of activation config snapshots or legacy direct-config support
  • no uninstall of loaded transient systemd-run services; they are reference-only inventory
  • no change to the current rendered production selector values

Compatibility

Automation Runner binary Behavior
old old Existing released behavior is unchanged.
old new Hidden --protect-version maps one opaque value into the three independent keep sets.
new old The capability probe skips the old inference-based GC invocation.
new new Automation passes independent service, binary, and Runner keeps; Runner discovers resources and references internally.

Existing direct-config and activation-snapshot units remain readable without migration metadata. The stable /var/lib/vm0-runner/locks/deployment-gc.lock ordering remains the cross-version serialization boundary.

Validation

  • cargo fmt --all -- --check
  • cargo clippy -p runner --all-targets --all-features -- -D warnings
  • cargo doc -p runner --all-features --no-deps
  • cargo test -p runner --bin runner cmd::gc::deployments::tests:: -- --nocapture (32 passed, 1 ignored child harness)
  • cargo test -p runner --all-targets --all-features --quiet (3365 passed, 26 ignored; guest inventory integration passed)
  • pnpm prettier --check --ignore-unknown ../docs/runner-host-configuration.md
  • bash -n scripts/dev-runner.sh .github/scripts/tests/dev-runner-test.sh .github/scripts/tests/runner-promotion-ansible-test.sh .github/scripts/prepare-runner-image.sh .github/scripts/tests/prepare-runner-image-test.sh .github/scripts/tests/runner-attribution-ansible-test.sh
  • bash .github/scripts/tests/dev-runner-test.sh
  • bash .github/scripts/tests/runner-promotion-ansible-test.sh
  • bash .github/scripts/tests/runner-attribution-ansible-test.sh
  • bash .github/scripts/tests/prepare-runner-image-test.sh
  • ./scripts/check-file-size.sh
  • repository pre-commit Rust formatting, documentation, Clippy, file-size, and commit-message hooks

The first full-suite attempt reproduced the known unrelated heartbeat timing race tracked by #30179. Its exact test passed immediately afterward, and the second complete suite passed with the results above.

Closes #29907
Relates to #30043
Follow-up: #30102

@github-project-automation github-project-automation Bot moved this to Backlog in VM0 Kanban Aug 28, 2026
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 06:11 Destroyed
@github-actions

Copy link
Copy Markdown
Contributor

API preview: https://pr-30093-api.vm6.ai

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.82072% with 316 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/runner/src/cmd/gc/deployments.rs 72.29% 266 Missing ⚠️
crates/runner/src/cmd/gc/mod.rs 68.83% 24 Missing ⚠️
crates/runner/src/cmd/service/systemctl.rs 70.42% 21 Missing ⚠️
crates/runner/src/cmd/gc/image_refs.rs 88.88% 2 Missing ⚠️
crates/runner/src/cmd/service/mod.rs 71.42% 2 Missing ⚠️
crates/runner/src/cmd/service/unit_config.rs 98.66% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 06:41 Destroyed
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 06:47 Destroyed
@seven332

Copy link
Copy Markdown
Contributor Author

Code Review: PR #30093

Reviewed head: cb35966efd8ebfa9af2926939845e3aec21c9a82

Summary

The current head replaces inferred version/name ownership with an explicit, fail-closed deployment inventory, preserves mixed-version rollout safety, and keeps directory, config, service, and image authorities separate. Self-review fixes for partial mutation failures and target inventory completeness are present and covered.

Findings

No findings.

Testing

Rust formatting, Clippy, docs, the scoped GC suite (167 passed, 4 ignored), and production promotion/rollback contract tests passed locally. The Ruby-backed Turbo workflow contract test could not run in this container because ruby is unavailable; hosted CI remains authoritative. The unrelated concurrent /proc/<pid>/environ full-suite flake is tracked in #30086.

Verdict

LGTM

@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 07:58 Destroyed
@seven332 seven332 changed the title fix(runner): make deployment gc ownership explicit fix(runner): resolve deployment gc ownership from installed services Aug 28, 2026
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 09:02 Destroyed
@seven332
seven332 force-pushed the fix/29907-explicit-runner-gc-ownership branch from d98136c to fcaf3fe Compare August 28, 2026 09:15
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 09:16 Destroyed
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 09:39 Destroyed
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 10:16 Destroyed
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 10:41 Destroyed
@seven332

Copy link
Copy Markdown
Contributor Author

Code Review: PR #30093

Reviewed head: 5e0f5d5af7ad4e8466303f64ae270d035bd69c9e

Summary

The current head removes name/version-based deployment ownership, discovers persistent installed services internally, keeps service/bin/Runner identities independent, and preserves rollout compatibility. It also distinguishes immutable activation snapshots, exact managed source configs, externally managed configs, and source-unknown legacy snapshots without inferring deletion authority.

Findings

No findings.

Testing

Rust formatting, Clippy, documentation, full Runner tests (3343 passed, 27 ignored, plus guest inventory), focused deployment/parser scenarios, file-size validation, and production/dev/CI shell contract tests passed locally. Required hosted checks and metal behavior lanes are still running on this head.

Verdict

LGTM

@seven332
seven332 force-pushed the fix/29907-explicit-runner-gc-ownership branch from 5e0f5d5 to 5e1b5d2 Compare August 28, 2026 16:13
@seven332 seven332 changed the title fix(runner): resolve deployment gc ownership from installed services fix(runner): make managed resource gc reference-safe Aug 28, 2026
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 16:14 Destroyed
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 16:26 Destroyed
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 16:35 Destroyed
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 17:02 Destroyed
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 17:08 Destroyed
@seven332
seven332 force-pushed the fix/29907-explicit-runner-gc-ownership branch from 5c6e2f4 to 707a129 Compare August 28, 2026 17:24
@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 17:26 Destroyed
@seven332

Copy link
Copy Markdown
Contributor Author

Code Review: PR #30093

Reviewed head: 707a12954ee6ca252c8abaa6fbabdd22f874c078

Summary

The current head makes managed bin/ and runners/ roots explicit GC ownership boundaries,
discovers persistent services independently, and retains resources only through exact unit,
config, runtime, lock, keep, and current-executable references. Service removal precedes resource
deletion, and incomplete or changed filesystem state fails closed. The three automation selectors
remain independent; only the rollout alias tracked by #30102 is retained temporarily.

Findings

No findings. Self-review findings around external executables and symlinks, managed-path grammar,
canonical exact references, activity rechecks, and traversal coverage are fixed in this head.

Testing

After rebasing onto 683db889dd2d31920c6294df75b7904be6db7747, Rust formatting, Clippy,
documentation, the deployment GC suite (31 passed, 1 ignored child harness), and the complete Runner
suite (3364 passed, 26 ignored, plus guest inventory) passed locally. The previous head's Crates,
Runner image, and all four metal behavior lanes passed. New-head hosted CI is running. Two unrelated
Turbo test failures from the previous head are outside this Rust/automation diff; the recurring
workflow-suggestion race was added to reopened #30134.

Verdict

LGTM

@github-actions
github-actions Bot temporarily deployed to api/preview/fix/29907-explicit-runner-gc-ownership August 28, 2026 17:56 Destroyed
@seven332

Copy link
Copy Markdown
Contributor Author

Code Review: PR #30093

Reviewed head: 4089cb52a7c0daadd231878cedf80bfb09f926f7

Summary

The current head uses managed bin/ and runners/ roots as independent resource inventories and persistent, transient, live, lock, keep, and current-executable state as exact references. Persistent services are removed before their dependencies, and GC now verifies that the installed unit file is actually absent before releasing its lock or deleting referenced resources. Automation and host documentation keep service suffix, binary dirname, Runner dirname, and release artifact identity separate while preserving only the rollout compatibility tracked by #30102.

Findings

No findings. The self-review finding around an apparently successful uninstall leaving a persistent unit file, and the stale host selector documentation, are fixed and covered on this head.

Testing

Rust formatting, Clippy, documentation, the managed-resource GC suite (32 passed, 1 ignored child harness), the complete Runner suite (3365 passed, 26 ignored, plus guest inventory), Markdown formatting, and the relevant dev/promotion/attribution/Runner-image shell tests passed locally. The first full-suite attempt reproduced the known unrelated heartbeat timing race in #30179; its exact rerun and the second complete suite passed. Hosted CI remains authoritative for metal behavior.

Verdict

LGTM

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

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

bug(runner): stop GC from inferring ownership from directory and service names

1 participant