Conversation
`fastly::cache::core::replace()` was the last unimplemented piece of the
core cache API: every `replace*` hostcall returned `NotAvailable`/
`Unsupported`, so `ReplaceBuilder::begin()` failed immediately and
read-modify-write patterns such as a cached counter could not be tested
locally at all.
Implement the whole family in both the witx and component host paths. A
replace reads the current object for the request's variant and then writes
a new one, which maps onto the existing `CacheValue`/`Obligation`
machinery; the strategy decides how it interacts with the obligation slot:
* `Immediate` (the default) neither waits nor takes the obligation, so
the existing object stays visible to lookups until the replacement is
provided, and the insert is last-writer-wins.
* `ImmediateForceMiss` clears the existing object under the write lock
and takes the obligation, so concurrent lookups wait for the
replacement rather than being served the old object.
* `Wait` blocks until no matching variant is obligated before taking the
slot, which serializes concurrent replaces and is what makes the
counter pattern correct.
The existing object is returned regardless of freshness, as documented;
freshness only shows up in the lookup state, where `MUST_INSERT_OR_UPDATE`
is always set. Note that `replace_get_state` must always succeed, because
the SDK calls it unconditionally from `begin()` and panics on any status
other than OK, including NONE.
Also implement `Found::hits()` and `Found::stale_while_revalidate()`,
which `Replace::existing_object()` exposes. These were unimplemented for
plain lookups too, and the SDK panics rather than returning an error when
the host reports `unsupported`. Hits are counted on delivery via a
counter shared between the cached object and the `Found` handed to the
guest; a replace reading the object it is about to replace does not count
as a hit.
No `.witx`, `.wit`, or adapter changes: the ABI surface was already
declared and adapted.
Closes fastly#495
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This addresses #495, and closes it.
fastly::cache::core::replace()was the last unimplemented piece of the core cache API. Everyreplace*hostcall returnedNotAvailable("Cache API primitives")(witx) orUnsupported(component), soReplaceBuilder::begin()failed immediately and read-modify-write patterns such as a cached counter could not be tested locally at all.This implements the whole family in both host paths. No
.witx,.wit, or adapter changes were needed — that surface was already declared and adapted.Semantics
A replace reads the current object for the request's variant and then writes a new one, which maps onto the existing
CacheValue/Obligationmachinery. The strategy decides how it interacts with the obligation slot:replace()replace_insert()Immediate(default)insert.ImmediateForceMisspresentfor the matching variants under the write lock and take the obligation, so concurrent lookups wait for the replacement instead of being served the old object.WaitThe existing object is returned regardless of freshness (the docs note it "may be stale"); freshness only shows up in the lookup state, where
MUST_INSERT_OR_UPDATEis always set.Obligation's existingDropimpl already gives the right "replace abandoned" behavior, so closing or dropping a replace handle needs nothing new.Two SDK behaviors worth calling out, since they turn host-side "not supported" into guest panics rather than errors:
ReplaceBuilder::begin()callsreplace_get_stateunconditionally and panics on any status other than OK, includingNONE— soreplace_get_statealways succeeds, returning empty flags when there is no existing object.Found::hits()andFound::stale_while_revalidate()panic when the host reportsunsupported, andReplace::existing_object()returns exactly thatFound. Both were unimplemented for plain lookups too, so this closes those gaps as well. Hits are counted on delivery through a counter shared between the cached object and theFoundhanded to the guest; a replace reading the object it is about to replace does not count as a hit.Known deviations
Both are commented at their definitions:
ImmediateForceMiss, if another party already holds the obligation we clearpresentbut cannot take a second one (the store's invariant is oneObligationper obligatedCacheValue), so we fall back to a racing plain insert.Waitreplace while itself holding an unfulfilled obligation blocks forever, where Compute would eventually time out. No timeout was added.Tests
Rust unit tests in
src/cache.rscover each strategy, an abandoned replace releasing its obligation, a replacement inserted under a different vary rule, and the hits/stale_while_revalidateround-trip. Four concurrentWaitreplaces starting from "1" produce "5", which is the property the counter pattern depends on.Eight guest tests were added to
test-fixtures/src/bin/cache.rs, so they run against both ABI paths via the existingviceroy_test!configurations. Since this fixture is also meant to pass on real Compute, assertions stick to documented behavior — e.g. hit counts are asserted monotonic rather than exact, and the force-miss test uses a transactional lookup, because "requests wait for the replacement" is the documented behavior and is well-defined on both.cargo test --allpasses (176 integration + 132 lib tests).cargo clippy --all-targets --all-featuresreports only pre-existing warnings inhttp_downstream.rs/backend.rs. The onlyUnsupportedreturns left in the cache hostcalls are the deliberateon_behalf_of/service-ID ones, matching the existing lookup and insert paths.🤖 Generated with Claude Code