Skip to content

refactor: make the CLI-to-config mapping total and fix three error-handling gaps - #7

Open
ctxswitch wants to merge 1 commit into
cleanup/phase-4b-extractor-negotiatorfrom
cleanup/phase-5-config
Open

refactor: make the CLI-to-config mapping total and fix three error-handling gaps#7
ctxswitch wants to merge 1 commit into
cleanup/phase-4b-extractor-negotiatorfrom
cleanup/phase-5-config

Conversation

@ctxswitch

Copy link
Copy Markdown
Owner

Summary

Fifth of six phases. Based on cleanup/phase-4b-extractor-negotiator (#6).

ServeArgs::config() assigned twenty fields over the defaults Config::new had just
written, so a newly added Config field was silently left at its default with no
compile error
. It now returns a Config { … } literal naming all 21 fields in
declaration order, which makes an omission a compile failure and turns the six
clone_from calls and the data_dir copy into moves. main.rs captures data_dir
and foreground_concurrency before the move, matching the existing listen capture.

The 18 defaults still exist twice — once in #[arg(default_value_t)] and once in
Config::new — which is left alone as agreed. Config::new gains a doc comment
recording that clap owns the operator-facing defaults, that ServeArgs::config
bypasses Config::new entirely, and that the two sets must change together.

Three error-handling fixes:

  • The elided empty object in the prefetch pass propagated its write failure with ?,
    aborting the entire pass and contradicting the per-object tolerance documented three
    lines below. Now tolerated per object.
  • The ephemeral cache's remove_dir_all is best-effort like prune_stale_files, so a
    cleanup failure after a fully successful build no longer exits non-zero — which Go
    surfaces as a build-cache error.
  • A match with an empty _ => {} arm becomes the let-chain form used elsewhere in
    this codebase.

Verification

make ci clean, including clippy --all-targets -- -D warnings.

New: serve_arguments_map_onto_the_configuration (tests/integration/cli.rs). The
struct literal makes a missing field a compile error but not a crossed one — low
and high watermark swapped would still compile. The test parses a serve invocation
with a distinct value per flag and asserts each lands on the right Config field, so
a crossed mapping fails. No default value appears in it; it is not a restatement of
the defaults.

The cacheprog changes are covered by src/cacheprog/cacheprog_test.rs and
tests/integration/build_cache.rs.

  • make ci
  • Documentation and examples are accurate
  • New or changed behavior has test coverage

Operational impact

The build-cache helper no longer exits non-zero when post-build cleanup of the
ephemeral cache directory fails. Previously a successful build could still be reported
to Go as a build-cache error. Cleanup failures are now silent, matching
prune_stale_files.

A prefetch pass no longer aborts because one elided empty object failed to write. The
remaining objects in the pass now proceed, which is what the surrounding code already
documented.

No configuration, storage format, route, metric, or security boundary changes. The
CLI accepts exactly the same arguments and produces the same Config for them.

Review notes

The config() rewrite is the substance; the rest is small. Worth confirming the field
mapping by eye against Config's declaration order, since that is the one thing the
compiler cannot check and the new test exists precisely to cover.

One plan item was declined because its premise does not hold. The plan carried
forward a report that put_reference and put_artifact are "sibling routes with
inconsistent error-response policy" — api_error JSON bodies versus bare status
codes. On reading, the split is not per-handler inconsistency but a consistent
per-route-family policy: every channel-management and every reference handler returns
api_error JSON (put_reference, get_reference, and delete_reference all return
invalid_reference JSON on the same parse failure, and get_reference returns a JSON
body on success), while every byte-stream route — put_artifact, get_artifact, the
build-cache and Bazel routes — returns bare status codes to clients that never parse
an envelope.

docs/operations.md:211-213 codifies exactly that: "Errors from channel management
use a JSON body with code and message fields. Data and protocol routes may return
an empty body when their client protocol does not use that error envelope."
No test
pins these bodies, so the change was available — it was declined because the current
state is the coherent one, and either direction would have broken a documented
contract.

Incidental, not changed: ServeArgs still derives Clone, which has no caller now
that config() consumes self. Clippy does not flag it and it is a public type.

…ndling gaps

ServeArgs::config assigned twenty fields over the defaults Config::new had just
written, so a newly added Config field was silently left at its default with no
compile error. It now returns a struct literal naming every field, which makes an
omission a compile failure and turns the clone_from calls into moves. Config::new
gains a comment recording that clap owns the operator-facing defaults.

The elided empty object in the prefetch pass propagated its write failure with ?,
aborting the whole pass and contradicting the per-object tolerance documented
three lines below; it is now tolerated per object, without counting a failed
write as a local hit. The ephemeral cache's remove_dir_all is best-effort like
prune_stale_files, so cleanup failure after a fully successful build no longer
exits non-zero into Go's build cache. A match with an empty catch-all arm becomes
the let-chain used elsewhere in the codebase.
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.

1 participant