Key-Value Swallows Write Errors When Backing Impl Fails

[rylev]

The CachingStore completely swallows errors when the inner implementation fails on writes.

https://github.com/fermyon/spin/blob/1f2269b5dd074fe953c1a8e46caf363d713fc984/crates/factor-key-value/src/util.rs#L190

This can lead to writes appearing like they succeed (even being reflected by reads within the same process) but not actually working.

[lann]

FWIW, it looks like this was an intentional tradeoff: https://github.com/fermyon/spin/blob/1f2269b5dd074fe953c1a8e46caf363d713fc984/crates/factor-key-value/src/util.rs#L75-L80

[lann]

After some further investigation, it looks like some of the newer KV implementations (Azure, AWS) are using lazy connections that cannot fail on open, which makes this problem much worse.

[dicej]

I'm the one who implemented CachingStore and wrote the comment @lann referenced above. My intention was that the Spin implementation would support the weakest consistency and durability model allowed by the interface in key-value.wit so as to help the application developer incorporate that model into their design and implementation early on and give them confidence that their app would be portable to any KV implementation.

Unfortunately, the consistency and durability model was apparently never documented in key-value.wit, and that's definitely a problem. I'll open a PR to address that.

FWIW, the wasi-keyvalue docs do address this, although those docs could also be improved by adding an explicit discussion of durability as well as consistency.

[rylev]

@dicej thanks for the context!

The linked wasi-keyvalue docs seem to me to indicate that this is a bug, but perhaps I'm miss understanding the meaning of the docs:

/// Data consistency in a key value store refers to the guarantee that once a write operation
/// completes, all subsequent read operations will return the value that was written.

I assume that these docs are not only talking about subsequent reads in the same component instance but also across component instances. Let me know if this assumption is incorrect. With that assumption, in the current implementation, this guarantee is not upheld. I can do a write operation and then do a later read operation (in another instance) and get an old value.

My understanding of the docs is that all writes must be guaranteed to be reflected eventually. So if I do a write of a value (and no other writes ever happen) all clients will eventually see that write. That's not the case here. I'd love your clarification on this so we can either update the wasi docs to reflect this or update the Spin implementation.

[dicej]

I assume that these docs are not only talking about subsequent reads in the same component instance but also across component instances. Let me know if this assumption is incorrect.

My reading is different, but I think that's because the docs aren't clear enough about the terms "client", "guest", and "caller" in the paragraph following the one you quoted. I've put up a PR that reflects my interpretation; I'm happy to refine and/or rewrite it based on feedback.

I think the key question we need to answer here is whether a host implementation of KV backed by an eventually-consistent distributed system spread across multiple failure domains with asynchronous, "best-effort" durability is (A) valid and (B) normal. For example, if we think it's valid but not normal, we can make the consistency and durability guarantees for both Spin KV and wasi-keyvalue much stronger, and create a new, niche interface which has weaker guarantees in exchange for wider applicability. And regardless of how we answer that question, we should make sure the docs are crystal clear so there are no surprises.

[dicej]

Another way to address the original issue: define set to have blocking durability (i.e. slow and safe) and add a set-async method which returns a resource which may be polled via wasi:io/poll for a result, which the caller can either discard or await as desired.

[ogghead]

Another way to address the original issue: define set to have blocking durability (i.e. slow and safe) and add a set-async method which returns a resource which may be polled via wasi:io/poll for a result, which the caller can either discard or await as desired.

Would it be desirable to implement this as a runtime config (i.e. in spin.toml)? That would allow users to flexibly set strong/weak durability without changing the KV interface. Though perhaps it is better to split the interface as you suggest and encourage users to be explicit (and allow choice of durability for each database call in a component)

[dicej]

I've opened #2995 to address this.