Skip to content

refactor: delete dead code and guards against unreachable states - #4

Open
ctxswitch wants to merge 1 commit into
cleanup/phase-2-perffrom
cleanup/phase-3-deletions
Open

refactor: delete dead code and guards against unreachable states#4
ctxswitch wants to merge 1 commit into
cleanup/phase-2-perffrom
cleanup/phase-3-deletions

Conversation

@ctxswitch

Copy link
Copy Markdown
Owner

Summary

Third of six stacked PRs. Based on cleanup/phase-2-perf (#3) — review the stack
in order. Net −179 lines, no new behaviour.

Dead or vestigial, each confirmed unreferenced before removal: the http-body-util
dependency (declared, used nowhere; still present transitively via axum/reqwest), the
references stripe array, four SpaceLedger accessors, impl FromStr for Digest,
ArtifactId::algorithm(), ChannelService::gate(), the publish_or_reject
passthrough, the prefetch pool's active/peak atomics, empty_output()'s
OnceLock, AwaitedRemoval, deleting_channels, the PredictedObject /
PublishedBody / Fetched wrappers, and session_finished. Mode's re-export
tightened to pub(crate).

The references stripes were 512 mutexes guarding two single atomic RocksDB batch
writes with no read-modify-write; the publish path writes reference rows under the
artifact stripe anyway, so the one race they could have serialized wasn't theirs to
serialize. The artifacts stripes are load-bearing and stay.

The eviction column family's stored_len value was written at two sites, discarded
by its only reader, and little-endian while its key is big-endian. It is now empty.

Also removes guards against states that cannot occur: Ring::owner's checked_rem
after the empty case has already returned, the u32 member index (a fallible
conversion that saved no memory — (u64,u32) and (u64,usize) are both 16 bytes),
ChannelId::new's nil-ULID retry loop, RecentUse::new's .max(1) behind a config
validate that already rejects zero, ensure_default's id check, prefix_end's
carry loop over a base32 prefix whose last byte is never 0xFF,
decode_eviction_key's repeated starts_with and two unreachable map_err arms,
and serve_artifact's unsatisfiable pre-check together with the unreachable!() it
forced and the Eq, PartialEq derives that existed only to serve it.

Verification

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

  • tests/integration/maintenance.rs (8 tests) passes, covering the eviction value
    change. tests/integration/channels.rs explicitly skips the eviction value in its
    on-disk-format assertions, so no format guard moves.

  • tests/integration/domain.rs still asserts 1024 fresh ChannelIds are non-nil
    after the retry loop came out, and now asserts id.to_string() == "sha256:<hex>"
    in place of the deleted algorithm() accessor — Display carries the algorithm,
    so the assertion still fails if it breaks.

  • src/cache/space_test.rs reads through snapshot() instead of the four deleted
    accessors — the same lock, and the path telemetry actually uses.

  • empty_output's new const is covered by
    zero_size_actions_are_elided_and_answered_locally, which fails if the literal is
    wrong.

  • make ci

  • Documentation and examples are accurate

  • New or changed behavior has test coverage

Operational impact

No store-format bump. Eviction rows now carry an empty value, and the only reader
already discarded it, so existing stores stay readable in both directions.

One rare-path narrowing: removing AwaitedRemoval means that if a caller's future is
dropped mid-remove_file inside discard(), the temporary file is no longer retried
asynchronously. Accounting still settles conservatively via PendingSettlement::drop,
the file is cleaned by remove_temporary_files() on the next startup reconcile, and
the over-commit reconciles on the next SpaceLedger::refresh — which is the same
fallback the guard itself relied on.

Prefetch debug logs lose the published_meanwhile and peak_concurrency fields.
Debug telemetry only; no metric or operator-facing contract changes.

Review notes

One plan item was wrong and is reported rather than worked around. Digest::parse
was described as duplicating validation that hex::decode_to_slice already performs.
It mostly does — length and hex-ness — but decode_to_slice accepts uppercase.
tests/integration/domain.rs asserts uppercase is rejected and Display emits
lowercase, so the lowercase requirement is load-bearing canonicalisation: deleting
the whole pre-check would have made sha256/ABC… a second URL form for the same
artifact. The length and is_ascii_hexdigit checks are gone; a one-line uppercase
rejection stays, with a comment saying why.

One item was deliberately skipped. rewrite_metadata's Transform::None arm is
genuinely dead — fetch_url routes None into the streaming branch and
cached_outcome returns before the call — but the match must stay exhaustive over
Transform. The only exits are replacing it with unreachable!(), which is the same
class of dead code this PR is removing, or splitting Transform into None plus a
three-variant MetadataTransform — an added type touching the constructors,
upstream_accept, cache_variant, and both dispatch sites. That is a type-level
refactor rather than a deletion, so it belongs with the error-type merge in the next
PR if it is wanted at all.

Review is probably best spent on src/storage/local/artifact_files.rs (the
AwaitedRemoval path above) and src/storage/metadata/rocksdb.rs (eviction value,
prefix_end, decode_eviction_key). The rest is unreferenced-symbol removal that
the compiler validates.

Removes the http-body-util dependency, the references stripe array (512 mutexes
guarding two single atomic batch writes with no read-modify-write), four
unreferenced SpaceLedger accessors, FromStr for Digest, ArtifactId::algorithm,
ChannelService::gate, the publish_or_reject passthrough, the prefetch pool's
active/peak atomics, empty_output's OnceLock, AwaitedRemoval, deleting_channels,
the PredictedObject/PublishedBody/Fetched wrappers, and session_finished.

The eviction column family's stored_len value was written at two sites,
discarded by its only reader, and little-endian while its key is big-endian; it
is now empty. Old stores stay readable because the reader already ignored it.

Also drops guards against states that cannot occur: Ring::owner's checked_rem
after the empty case returns, the u32 member index that saved no memory but cost
a fallible conversion, ChannelId::new's nil-ULID retry loop, RecentUse::new's
max(1) behind a validated config, ensure_default's id check, prefix_end's carry
loop over a base32 prefix whose last byte is never 0xFF, decode_eviction_key's
repeated starts_with and two unreachable map_err arms, and serve_artifact's
unsatisfiable pre-check along with the unreachable! and the Eq/PartialEq derives
that existed only to serve it.
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