Repository navigation
feat(golang)!: the guest steps instead of blocking on the network, and the client drives it outside its lock - #1265
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Mutation testing (cargo-mutants,
|
This comment has been minimized.
This comment has been minimized.
6927805 to
76285d6
Compare
|
CI fix, force-pushed: the token is now fetched once per operation, before its first step and outside the lock, and every step of that operation hands it over. Fetching before each step asked an OIDC provider twice for one init ( |
7682a7f to
2659536
Compare
76285d6 to
5135edf
Compare
2659536 to
91b40eb
Compare
5135edf to
5fd317d
Compare
coderdan
left a comment
There was a problem hiding this comment.
Review of 76285d6 (this PR's diff only, against feat/remove-ffivalue). I read #1064 and its decisions comment, #1263, and the Rust and Go on both sides of the boundary. No earlier review comments exist on this PR.
Findings, most severe first
-
High. The handle space runs out in a long-lived process.
step::NEXTis au32that only increases and is never reused, andissue()fails withSTATUS_STATEonce it would overflow. EverySealandOpenpauses at least once (generate_keys/retrieve_keysalways go to ZeroKMS), so every one of those calls spends a handle. After 2^32 of them, every ZeroKMS-reaching call on thatClientreturnsErrState("this instance has issued every handle") until the process restarts; the client is not closed, so nothing on the Go side notices or recovers. At 1,000 seals a second that is about 50 days of uptime, at 5,000 about ten. Suggest au64handle (wasm passes i64 scalars natively, and the Go side already hands the handle over as auint64throughscalar), or wrap-around with a "not currently open" check, which keeps the stale-resume guarantee because a stale handle is never in the table. Better to fix while the ABI has one host. Inline onstep.rs. -
High. The 64-open cap is a hard failure for the 65th concurrent Go caller.
MAX_OPEN = 64is the right guard against a host that forgetsse_abort, but the Go client does nothing to stay under it. Before this PR, 65 goroutines callingSealqueued onc.mu; now the first 64 pause across the network and the 65thse_encrypt_recordreturnsSTATUS_STATE, whichdrivehands back asErrState("too many operations are paused: resume or abort one first"). An HTTP server with more than 64 in-flight handlers under ordinary ZeroKMS latency hits this under load, andErrStateis documented as "the client is closed", so a caller will treat the shared client as dead. Suggest a bound indrive(a semaphore ofMAX_OPENacquired before the first step and released when the operation ends or is aborted), so excess callers wait as they did before, plus a test for the 65th caller. Inline onclient.go. -
Low (behaviour). A token is fetched for every
callKMSoperation, including ones that never ask for it. A query term, aderived term or a keyset lookup on a cached keyset never reaches ZeroKMS, so the guest never callstoken_get; before this PRStrategy.Tokenwas not called for them. Now every such call takesStrategy.mu, runs a credential-guest export, and for a device session whose token has lapsed performs a refresh (file lock and network) on behalf of an operation that would never have used it. The failure semantics are preserved (a source that fails only surfaces if the guest asks), so this is cost and a doc accuracy point: the comment ondrivesays the source is asked "once per operation, as it was when the guest asked for it", which is not true for those operations. Either fetch only when the operation's first step needs it (not possible under decision 4 as written) or say so in the comment and CHANGELOG. Inline onclient.go. -
Low. Marshalled responses are not wiped when the next step short-circuits. The resume closure owns
defer wipe(responses), butc.stepreturnsErrStatebefore calling it when the client was closed while the request was in flight (Closefrom another goroutine, or a trap on another operation's step). The responses, which can carry wrapped keys, then wait for the collector. Wipe on that path too. Inline onclient.go. -
Low.
takeIOcallsse_ioandse_deallocoutside the allocator'sEnter/Exitbracket. Every other guest entry goes throughguest.Call, which keeps the memory mapped for the whole call so aFreethat arrives mid-call is deferred (the contract onAllocator.Free).takeIOcallsinst.io.Callandinst.exports.Freedirectly. Nothing can close the module during it today (the context isWithoutCancel, the lock is held, the cleanup only fires on an unreachableClient), so it is safe, but it is the one entry that does not keep the discipline. Suggest bracketing it. Inline onstep.go. -
Note, until #1263 and this are merged together. A step runs under
context.WithoutCancelwith no backstop. Before this PR a guest export that never returned at least died with its caller's deadline (WithCloseOnContextDone); now it holdsc.muforever, andClosetakesc.mutoo, so the process can only exit. It needs a guest bug to reach, since no step does I/O any more, and the PR's merge plan (take #1263'sDetach(ctx, c.stuck)instep) covers it; just make sure whichever lands second keeps the backstop. Related: the comment onTestInterruptedCallClosesTheClient("An interrupted call closes the module (WithCloseOnContextDone)") is stale, its first subtest now exercises the abort-between-steps path and the module stays open.
What I checked and found sound
- Locks across I/O.
c.muis held per step only;fetchTokenruns outside it, so the oldc.muthenStrategy.munesting is gone andClose(which takesc.muthen releases the credentials) cannot invert against it.transport.held,fetchErrandtokenErrare written and read only underc.muon the stepping goroutine;sendsis atomic.go vetandgo test -raceonencryptandinternal/guestreport no race. - Interleaving inside the guest. With two operations paused, a std
Mutexheld across an.awaitwould panic on wasm32 (cannot recursively acquire mutex) and take the instance down. I checked the two:StackCipher::keysettakeskeysets()before and afterload_keyset(..).await, never across it (cipher.rs:587-617), andWasiHostConnection::senddrops thebase()guard at the end of thelet urlstatement, beforestep::request(..).await.StackKmsholds no lock of its own on wasm32;HostTokenStrategyis synchronous. - Handle lifecycle. Issued only on the first pause;
resumeremoves the entry first, so a wrong response count, a pause without a request, or a cap refusal all drop the work; abort and a second abort/resume on a spent handle areSTATUS_STATE;se_shutdownrunsdrop_allbefore taking the cipher, so theRccount makes the cipher's wipe the last owner's;clear_pendingwipes a step the host never took;wipe_allforgets the pending slot before freeing. In Go, every error after a pause (performStepfailure, context ended) aborts the handle, andabortchecksclosedbefore calling in. - Inputs across a pause. Every stepping export copies its inputs before building the future (
take_plaintextorto_vec, options parsed up front), so the host freeing its buffers when the export returns is safe; the aborted-init memory scan passes locally. TheInitialisingguard is moved into the future and so clears on done, failure, abort, cap refusal and shutdown. - ABI boundary.
se_resumevalidates the responses buffer throughtake_plaintext(range check, wiped in place),decode_responsesisdeny_unknown_fieldsand refuses a non-bytesheaderswithout echoing it, the handle is checked against the table before any response is placed, andse_iois handed over once. Miri onstack-guest-abiis green in CI and the step tests run under it. - Decisions in #1064 honoured: init and keyset loading on the loop with an in-progress state; a request list with one entry per step; several handles open at once with the cap and no-reuse rule; the last-error slot read under the same lock as the failing step; token refresh on the host; the credential guest out of scope; wakers stored per request and woken before the poll; pending-with-no-request is
STATUS_INTERNAL. The one disclosed divergence (a status plusse_ioinstead of aStep::Done | NeedsIoframe) keeps every output format unchanged and copies no plaintext into a wrapper; it reads as the better shape. Not done, as the PR says: a Go test of two interleaved handles and a plaintext-needle scan of an aborted encrypt. - Behavioural equivalence. The stored-rows fixture and the whole hermetic suite pass;
max_concurrent_reqs(1)and the 500-key split are unchanged; stack-kms has no retry, so none is lost; the request runs under the caller's context as before. The accepted change is one token per operation rather than per request, which the decisions comment calls for. - CHANGELOG and the
!. The Go API is unchanged, so the entry belongs under Changed, as it is; the!is for the guest ABI, which has no consumer outside this module. Worth keeping in mind when the Go release is cut by hand so it is not read as a major.
Checks run
cargo test -p stack-guest-abi --all-features(root workspace): 42 passed.cargo testinlanguages/golang/encrypt/guest(native): all passed.- Built the default, deterministic and EQL guests for wasm32-wasip1 with the import-surface check (
token_getis the only transport import). go vetandgo test -race -count=1 ./encrypt/ ./internal/guest/(no credentials; the live tests skip): all passed once the EQL guest was built, no race reports.- mise is untrusted in the review worktree, so the tasks were run by hand with the pinned 1.94.1 toolchain and Go 1.26; trust settings were not changed.
- The failing "fmt, clippy, nextest, doctests, rustdoc" job is the inherited
cargo publish --dry-runfailure from #1264 (vitaminc-aead-framehas noserdefeature on crates.io), not this PR's.
91b40eb to
28f0c33
Compare
5fd317d to
9e0556d
Compare
28f0c33 to
2aaab14
Compare
9e0556d to
7aa625d
Compare
2aaab14 to
63d2183
Compare
7aa625d to
31a0607
Compare
…s HTTP requests instead of blocking A guest export that reaches ZeroKMS blocked on the host's synchronous transport_send import. A host whose HTTP client cannot block — fetch in a JavaScript edge runtime — cannot satisfy that import, and in Go the client held its lock for the whole round trip. The new `step` module runs an operation's work as a future polled once. A request the work makes is recorded and the future stays pending; the operation is kept in a table under a handle and its requests are handed to the host. The host performs them and resumes the operation with the responses, in order, until it is done; or it aborts it, which drops the future and wipes everything it held. Each request stores the waker it was polled with and resuming wakes it first, so combinators that poll an inner future only when woken (buffered, as the ZeroKMS client's 500-key chunking uses) do not stall. Handles are issued in increasing order and never reused, at most 64 operations may be paused, an unknown handle is STATUS_STATE, and work that pauses with nothing to wait on is STATUS_INTERNAL. STATUS_NEEDS_IO (29) is how an export says it paused. It is not a failure: call::run records no error for it. The module holds no encoding; each guest decides how a step's requests and responses cross. Refs #1064.
…t_send, and the client drives it outside its lock The crypto guest no longer imports transport_send. An export that can reach ZeroKMS (se_cipher_init, se_keyset, se_term, se_encrypt_record, se_decrypt_record, se_query_term) runs its work through the step loop: done, it returns its output as before; waiting on a request, it returns STATUS_NEEDS_IO, the host takes the handle and requests with se_io, performs them and calls se_resume with the responses, or gives up with se_abort. A step and the responses are transport frames read and written with serde (the guest's io module). Each paused operation owns its inputs, plaintext included, and shares the cipher through an Rc, so se_shutdown drops every paused operation before the cipher and its wipe is the last owner's. A second se_cipher_init is refused while one is paused. The Go client drives the loop. It holds its lock for each step only, and performs the requests with the lock free, so other callers run meanwhile instead of queuing behind a round trip. Each step runs under a context that cannot be cancelled, so the runtime never closes the module under a caller who gave up; the context is checked between steps, and an operation whose caller has gone is aborted in the guest. The token is fetched before each step, outside the lock, and token_get only hands it over. Steps of different operations may interleave; a step is read from the guest before the lock is released. Tests: the import surface is token_get alone; one load-keyset request is one step and spends its handle; a call runs while another's request is in flight; a caller that gives up mid-request gets its context's error, the operation is gone and the client still works; an ended context never starts a call; the token is fetched outside the lock; an init paused and aborted leaves neither the client key nor the bearer token in guest memory (and without the abort, the bearer remains, so the test can fail). Refs #1064.
31a0607 to
7fcb8f0
Compare
|
Rebased onto How they were resolved (the rewrite changes more than hashes):
Verified locally:
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fcb8f08bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… handles 64 bits Review follow-up on the step loop (#1064). A Go caller past the guest's MAX_OPEN of 64 paused operations had its first step refused with STATUS_STATE, which surfaced as ErrState, the error that means the client is closed. Before the step loop such a caller queued on the client's lock. The client now holds a semaphore of the same size: an operation that can reach ZeroKMS takes a slot before its first step and gives it back when it ends, so callers past the cap wait (and leave the queue when their context ends) instead of failing. A test pins maxPaused to the guest's MAX_OPEN. Handles were u32, issued once each and never reused, so a long-lived instance spent them: every Seal and Open pauses at least once, and after 2^32 pauses every ZeroKMS-reaching call failed with ErrState. They are u64 now, through se_resume, se_abort and the step frame. Also: responses for a resume step that never ran (the client closed while the request was in flight) are wiped rather than left to the collector; takeIO keeps the allocator's Enter/Exit bracket like every other guest entry; and the docs and CHANGELOG say the token source is asked once per call that can reach ZeroKMS, including calls the guest answers from cache.
Summary
The guest is Stack Encrypt compiled to WebAssembly. Today, whenever the guest needs ZeroKMS (our key service), it calls a host function,
transport_send, and waits until the HTTP response comes back. A JavaScript edge runtime (Cloudflare Workers, Deno, a browser) can't do that: itsfetchonly returns a promise, so the guest can't run there. In Go it costs something too: the client holds its lock for the whole network round trip, so every caller queues behind the slowest request.This PR is the step loop from #1064, as amended by the decisions comment there. An export that needs ZeroKMS no longer waits. Instead it pauses: it hands the host a handle and the HTTP requests it needs. The host performs them however it likes and resumes the operation with the responses, until it is done. The Go client now drives that loop and holds its lock only while the guest is actually running. There is no change for Go users' code.
Changes
Shared guest ABI (
packages/stack-guest-abi), first commitstepmodule: an operation's work runs as a future, polled once.resumedelivers the responses in order. Each request stores its waker, andresumewakes it first, so the 500-key chunking (buffered) never stalls.abortdrops the operation, which wipes everything it held.resumecan't reach another operation.STATUS_STATE.STATUS_INTERNAL.STATUS_NEEDS_IO(29) is how an export says it paused. It is not a failure:call::runrecords no error for it.Crypto guest and Go client, second commit (breaking for anything that embeds the guest directly)
transport_send; its only import istoken_get.se_cipher_init,se_keyset,se_term,se_encrypt_record,se_decrypt_recordandse_query_term.se_io: takes the paused step, handed over once.se_resume(handle, responses): hands back the responses and runs the next step.se_abort(handle): drops the operation.guest/src/io.rs):{handle, requests: [{method, url, headers, body}]}out, and[{status, headers, body}]back. That's field for field whattransport_sendcarried, including the negative-status rule.Rc, sose_shutdowndrops every paused operation before the cipher, and the cipher's wipe is the last owner's.se_cipher_initis refused while one is paused.encrypt/step.go,client.go):Client.calland the newClient.callKMSdrive the loop.context.WithoutCancel, so the runtime never closes the module under a caller who gave up. The context is checked between steps, and an operation whose caller has gone is aborted in the guest.token_getonly hands it over. Fetching before every step would ask an OIDC provider once per round trip.callnow receives the step's context.mise.tomlno longer requirestransport_sendon the crypto guest's import surface. The credential guest still requires it.encrypt/README.md, theClientdoc comment, and a Go CHANGELOG entry.Verification
stack-guest-abi:buffered;call::runtest that a pause records no error.mise run miri:stack-guest-abipasses, 41 tests.cargo testpasses, including the step encoding's round trip and refusals that never echo a value.-D warningsis clean for wasm32 with default features,eqlanddeterministic-kms, and natively.mise run wasm:guest:testandwasm:no-http-testpass.token_getalone.--locked.go test ./...passes.go test -racepasses onencrypt(524 s) andinternal/guest.encrypt/step_test.go, against a stub ZeroKMS that holds the init's request:context.Canceled, the operation's handle is gone, the client stays open, and a new init runs;TryLock);TestImportSurfaceIsWASIPlusTokenGet(was…PlusTransport);TestEachRequestIsOneStep(was thesends == 1counter test), which also checks the handle is spent;TestClientKeyDoesNotRemainInGuestMemorynow drives the loop.Related
next, which now carries refactor: read control messages from frames with serde, not FfiValue (part 1: options and stats) #1262, fix(golang): a call whose context ends fails on its own, and leaves the client open #1263 and refactor(stack-encrypt)!: remove FfiValue #1264.Review notes
packages/stack-guest-abi/src/step.rs;guest/src/abi.rs(deliver,run,se_io,se_resume,se_abort);encrypt/client.go(drive,step,abort).Step::Done | NeedsIoframe as every stepping export's output.STATUS_NEEDS_IO) plusse_ioto take the step.c.mu.Acquire(ctx); an abort takesLock(), because a paused operation must always be dropped.guest.Detach(ctx, c.stuck)in place ofcontext.WithoutCancel, so the 30-second backstop for a stuck guest still applies.guest.Callerintoken_getandtransport_sendis gone: the token is fetched before the first step, and requests run between steps under the caller's context.ErrTransporttoo, as fix(golang): a call whose context ends fails on its own, and leaves the client open #1263 documents. A step that succeeds after its context ended has its output wiped and fails the same way.load-keyset, which the hermetic stubs don't fake. The Rust step tests cover interleaving, never-reused handles and the cap, and the Go lock test shows a second call running while one is paused.Zeroizingplaintext on abort; that drop is tested natively (the abort test), but there's no linear-memory scan for it.{"any"}.