Skip to content

fix: correct four latent defects in negotiation, caching, staging, and routing - #2

Open
ctxswitch wants to merge 1 commit into
mainfrom
cleanup/phase-1-bugs
Open

fix: correct four latent defects in negotiation, caching, staging, and routing#2
ctxswitch wants to merge 1 commit into
mainfrom
cleanup/phase-1-bugs

Conversation

@ctxswitch

Copy link
Copy Markdown
Owner

Summary

First of six stacked PRs from a full review of src/. This one carries only the
bugs; the later phases carry hot-path allocation, deletions, deduplication, config
robustness, and build/docs/test work.

Four defects, each with a regression test written first and confirmed failing:

  1. npm content negotiation served the wrong representation. negotiate_npm
    discarded the full branch's specificity, so Accept: application/json, application/* scored abbreviated (1, 1.0) against full (2, 1.0) and returned
    abbreviated to a client that had asked for full. It now compares both
    specificities, matching negotiate_python.
  2. The cacheprog manifest aged out exactly the entries prefetch predicts best.
    The local-hit branch returned without record_used, and merge_manifest retains
    stored entries against their old last_seen, refreshing only entries present in
    used. A perfectly predicted action therefore expired at MANIFEST_MAX_AGE_SECONDS
    and was evicted first under MANIFEST_MAX_ENTRIES.
  3. A zero-length upload leaked a .part file. Reservation::drop tested
    outstanding before state. Content-Length: 0 short-circuits reserve(0)
    leaving outstanding == 0, but stage_reserved still calls mark_temporary and
    creates the file.
  4. Thundering-herd ring rebuild. RingState::ring took the write lock and
    rebuilt unconditionally; the read fast path checks earliest_retry but nothing
    re-checked after acquiring. Every concurrent request rebuilt in full (160 SHA-256
    hashes per member) under a lock that blocks all routing, exactly when the cluster
    is degraded.

Verification

make ci clean, including clippy --all-targets -- -D warnings. Each regression
test was confirmed to fail before its fix.

  • New: wildcard_accept_ranges_never_outrank_a_named_representation
    (tests/integration/package_proxies.rs) — table driven, seven npm cases and five
    symmetric python cases.

  • New: locally_answered_gets_refresh_their_manifest_entry
    (src/cacheprog/cacheprog_test.rs).

  • New: dropping_a_zero_length_stage_removes_its_temporary_file
    (src/storage/local/artifact_files/tests.rs).

  • New: concurrent_readmission_rebuilds_the_continuum_once
    (src/agent/discovery_test.rs) — bug 4 has no single-threaded signal, so this
    races 8 barrier-released callers and asserts one shared Arc.

  • make ci

  • Documentation and examples are accurate

  • New or changed behavior has test coverage

Operational impact

A warm build that answers every get locally now costs two more upstream requests at
close (a manifest GET and PUT) than it did before, because its used map is no
longer empty and finalize no longer skips the write-back. That write-back is the
fix: without it the manifest cannot carry a correctly predicted action forward.
warm_build_costs_one_manifest_get_plus_one_get_per_distinct_output was updated
from 4 to 6 requests with an explicit breakdown.

npm clients sending a wildcard alongside an explicit type will now receive full
metadata where they previously received abbreviated. No storage format, HTTP route,
configuration, or security boundary changes.

Review notes

Start at src/proxy/mod.rs:245 and compare against negotiate_python:181 — the two
are now structurally identical apart from their media-type tables and their
application/* defaults (python defaults to json, npm to abbreviated). Those
defaults differ deliberately; the new python cases pin them, because a later PR in
this stack merges the two negotiators and the shared parsing is where this bug lived.

The request-count change in tests/integration/build_cache.rs is the one place
reviewing should be deliberate rather than mechanical: it is a real cost increase,
accepted on purpose.

Deliberately excluded: everything from the other five phases — no Durability
threading, no ChannelStoreError merge, no allocation or deduplication cleanups.

…d routing

npm content negotiation discarded the full representation's specificity, so
`Accept: application/json, application/*` served abbreviated metadata to a
client that had asked for full. Compare both specificities, mirroring
negotiate_python.

cacheprog's local-hit path returned without recording the use, so
merge_manifest retained those entries against a stale `last_seen`: the
best-predicted actions were the first to age out and the first evicted at the
entry cap. A wholly local build now pays a manifest read-modify-write at close.

Reservation::drop tested `outstanding` before `state`, so a zero-length body --
which reserves nothing but still stages a file -- leaked its `.part`.

RingState::ring rebuilt the continuum unconditionally after taking the write
lock, so every concurrent caller rehashed the whole membership under a lock
that blocks all routing, precisely when the cluster is degraded.
@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.56%. Comparing base (5764b6d) to head (9ab93e0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #2      +/-   ##
==========================================
+ Coverage   88.12%   88.56%   +0.44%     
==========================================
  Files          35       35              
  Lines        7093     7174      +81     
==========================================
+ Hits         6251     6354     +103     
+ Misses        842      820      -22     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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