Skip to content

fix(admin): always send dry_run on storage-GC requests - #176

Merged
brandonrc merged 1 commit into
mainfrom
fix/storage-gc-send-dry-run-explicitly
Sep 10, 2026
Merged

fix(admin): always send dry_run on storage-GC requests#176
brandonrc merged 1 commit into
mainfrom
fix/storage-gc-send-dry-run-explicitly

Conversation

@brandonrc

Copy link
Copy Markdown
Contributor

Closes #175

Companion to artifact-keeper/artifact-keeper#3619 — that PR should not merge before this one is ready.

What

ak admin storage-gc run built its request body with dry_run.then_some(true):

let body = artifact_keeper_sdk::types::StorageGcRequest {
    dry_run: dry_run.then_some(true),
};

The generated field is Option<bool> carrying #[serde(default, skip_serializing_if = "::std::option::Option::is_none")] (sdk/src/generated_sdk.rs), so on a live run (no --dry-run) the None drops the key and the CLI posts {} to POST /api/v1/admin/storage-gc. The --dry-run path always sent {"dry_run": true} and is unaffected.

That has worked so far only because the backend defaulted the missing field to false. #3619 removes that: dry_run becomes required and the request gains deny_unknown_fields, so an empty body is refused with 422 (missing field `dry_run` ) and no GC runs. Against a backend carrying #3619, ak admin storage-gc run without --dry-run stops working entirely.

The fix

Send the value unconditionally, through a small storage_gc_body(dry_run) helper so the body can be asserted without standing up a client:

fn storage_gc_body(dry_run: bool) -> artifact_keeper_sdk::types::StorageGcRequest {
    artifact_keeper_sdk::types::StorageGcRequest {
        dry_run: Some(dry_run),
    }
}

{"dry_run": false} is exactly what today's backend already infers from {}, so this is safe to merge before #3619 and required after it. No SDK regeneration is involved — the type is unchanged; only what we put in it changes. (When the spec is next regenerated, dry_run becomes required and the field will lose its Option, at which point this line becomes dry_run, — another reason to have the call site sending a real value already.)

Scope

run_storage_gc is the CLI's only constructor of StorageGcRequest, and #3619 touches only backend/src/api/handlers/storage_gc.rs, so no other call site is affected. The repo-scoped POST /api/v1/repositories/{key}/storage-gc handler that #3619 also hardens is not wired into the CLI (absent from the generated SDK), so nothing to fix there.

I checked the other 14 then_some(true) body fields in src/commands/ (backup restore, cleanup, user create/update, promotion, approval, format-handler toggles). They have the same "omit when false" shape, but none of their request types are validated by #3619, so they are out of scope here — they stay correct as long as the backend keeps defaulting those fields.

Testing

New unit test storage_gc_body_always_sends_dry_run asserts both bodies serialize with the key present:

assert_eq!(
    serde_json::to_value(storage_gc_body(false)).unwrap(),
    json!({ "dry_run": false })
);

Revert-proofed: restoring dry_run.then_some(true) fails it with left: Object {} / right: Object {"dry_run": false} — i.e. the test reproduces the empty body the backend would 422 on.

  • cargo nextest run --workspace2025 passed, 0 failed (2024 before, +1 new)
  • cargo clippy --workspace -- -D warnings clean
  • cargo fmt --check clean

CHANGELOG updated under Unreleased / Fixed.

`run_storage_gc` built its body with `dry_run.then_some(true)`. The
generated SDK field is `Option<bool>` carrying
`#[serde(skip_serializing_if = "Option::is_none")]`, so on a live run
(no `--dry-run`) the `None` dropped the key entirely and the CLI posted
`{}` to `POST /api/v1/admin/storage-gc`.

That relied on the server defaulting the field. artifact-keeper#3619
removes that default: `dry_run` becomes required and unknown fields are
rejected, so `{}` is refused with 422 and no GC runs. `ak admin
storage-gc run --dry-run` was always fine — it sent `{"dry_run": true}`.

Send the value unconditionally instead, via a small `storage_gc_body`
helper so the body is testable without a client. A live run now posts
`{"dry_run": false}`, which is what the 1.8.x backend already means by
an empty body, so this is safe to merge before #3619 and required
after it.

The added test asserts both bodies; restoring `then_some(true)` fails
it with `left: Object {}`.
@brandonrc

Copy link
Copy Markdown
Contributor Author

The red Test check (build_client_with_valid_token) is a pre-existing flake, not this PR: filed as #177.

build_client reads the process-global AK_CA_CERT via transport::apply_custom_ca, and transport::tests set that variable to non-existent paths under ENV_LOCK — which the three client::tests do not take. Under cargo test (threads in one process) they can be co-scheduled; under nextest (process per test) they cannot, which is why it is invisible locally.

Reproduced on unmodified main @ 393be2d: running just transport::tests client::tests in one process failed 6 of 60 times with the identical panic. Deterministically, AK_CA_CERT=/some/ca.pem <test-bin> build_client_with_valid_token fails on its own.

This PR touches only run_storage_gc and the CHANGELOG. Re-running the failed job.

@sonarqubecloud

Copy link
Copy Markdown

@brandonrc
brandonrc merged commit 79573e6 into main Sep 10, 2026
25 of 26 checks passed
@brandonrc
brandonrc deleted the fix/storage-gc-send-dry-run-explicitly branch September 10, 2026 05:36
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.

ak admin storage-gc run posts an empty body on a live run

1 participant