amadan
internal/core: fix-round-1 -- cap timeout to what it's for, discard test
that can fail, exact-length reads, bounded log, relocated vendor Five findings from fix-round-1 review, addressed with real reproducers rather than assertions: 1. TestTrappedInstanceIsDiscardedAndRebuilds could not fail against the bug it names: its trap fires inside Update::decode_v1 before any RefCell is borrowed, so a host that never discarded the instance passed every behavioural check anyway. Moved to core_internal_test.go (white-box) and added a direct h.mod pointer-identity check -- confirmed by temporarily disabling the discard and watching this exact test fail (see task-6-report.md). 2. guestCallTimeout (5s) never fired for the input it exists to stop: that input measures ~4.34s under wazero, so 5s let it nearly finish naturally rather than demonstrating the cap doing anything. Changed the default to 1s and made it a Runtime field set via a functional option (WithGuestCallTimeout) instead of a package constant, so it's tunable without a recompile. TestSlowUpdateIsCutOffByGuestCallTimeout now shows the default cutting the same reproducer off at ~1.0s. 3. textOn read the output buffer using text_len's value instead of the written count docs_core_text_read actually returns -- the latter is only guaranteed <= the former (core/src/abi.rs:287); on divergence this would append uninitialised allocator bytes to a document's text. Now reads exactly `written` bytes. 4. The repo-root vendor/ (Rust's fastrand patch, unrelated to Go) collided with Go's vendor auto-detection, forcing -mod=readonly on every command and suppressing the class of real vendoring error that flag exists to report. Moved to core/vendor/ (git mv), updated the one path in core/Cargo.toml's [patch.crates-io]. The .wasm artifact changed (495976 -> 496015 bytes -- the crate's absolute path is embedded in panic strings) so it was rebuilt and re-copied into internal/core/testdata/, verified byte-identical by sha256 against a fresh build. `go build/vet/test ./...` now all work with no -mod flag. 5. Handle.applied grew without bound, making a future rebuild O(n) forever and any single cancelled request's rebuild replay an amplifying cost on a long-lived document. Real compaction needs an ABI v2 state-export this crate doesn't have yet (documented, and left to the snapshot work). Until then: capped at maxRetainedUpdates (4096); Apply refuses further updates with ErrUpdateLogFull once full, rather than growing unboundedly or silently dropping history (which would make a later replay produce silently wrong content). Also (minor, requested alongside): Text now retries the read against a freshly rebuilt instance instead of returning "" plus an error when the data is actually available; Apply now returns a *RecoveredError (Recovered(err)) when a trap/timeout's rebuild succeeds, so a caller can tell "rejected, instance intact" apart from "trapped, document recovered" apart from "trapped, and recovery itself failed" (a plain joined error). The package doc comment now states explicitly that rebuild replays an in-memory update log rather than restoring a persisted snapshot -- defensible under ABI v1 (no state-encode export exists to snapshot from) but previously only implicit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA
16 files changed,
+474
−206
core/Cargo.toml+1 −1{vendor => core/vendor}/README.md+0 −0{vendor => core/vendor}/fastrand-2.5.0/CHANGELOG.md+0 −0{vendor => core/vendor}/fastrand-2.5.0/Cargo.toml+0 −0{vendor => core/vendor}/fastrand-2.5.0/LICENSE-APACHE+0 −0{vendor => core/vendor}/fastrand-2.5.0/LICENSE-MIT+0 −0{vendor => core/vendor}/fastrand-2.5.0/README.md+0 −0{vendor => core/vendor}/fastrand-2.5.0/benches/bench.rs+0 −0{vendor => core/vendor}/fastrand-2.5.0/src/global_rng.rs+0 −0{vendor => core/vendor}/fastrand-2.5.0/src/lib.rs+0 −0{vendor => core/vendor}/fastrand-2.5.0/tests/char.rs+0 −0{vendor => core/vendor}/fastrand-2.5.0/tests/smoke.rs+0 −0internal/core/core.go+205 −75internal/core/core_internal_test.go+256 −49internal/core/core_test.go+12 −81internal/core/testdata/docs_core.wasmbinary
diff --git a/core/Cargo.toml b/core/Cargo.toml| index 0ba22b6..8193a58 100644 |
| --- a/core/Cargo.toml |
| +++ b/core/Cargo.toml |
| @@ -30,4 +30,4 @@ wasmparser = "=0.258.0" |
| # conflict that only manifests when overriding getrandom's backend to |
| # avoid a wasm-bindgen (JS) import on wasm32-unknown-unknown. |
| [patch.crates-io] |
| -fastrand = { path = "../vendor/fastrand-2.5.0" } |
| +fastrand = { path = "vendor/fastrand-2.5.0" } |
diff --git a/vendor/README.md b/core/vendor/README.md| similarity index 100% |
| rename from vendor/README.md |
| rename to core/vendor/README.md |
diff --git a/vendor/fastrand-2.5.0/CHANGELOG.md b/core/vendor/fastrand-2.5.0/CHANGELOG.md| similarity index 100% |
| rename from vendor/fastrand-2.5.0/CHANGELOG.md |
| rename to core/vendor/fastrand-2.5.0/CHANGELOG.md |
diff --git a/vendor/fastrand-2.5.0/Cargo.toml b/core/vendor/fastrand-2.5.0/Cargo.toml| similarity index 100% |
| rename from vendor/fastrand-2.5.0/Cargo.toml |
| rename to core/vendor/fastrand-2.5.0/Cargo.toml |
diff --git a/vendor/fastrand-2.5.0/LICENSE-APACHE b/core/vendor/fastrand-2.5.0/LICENSE-APACHE| similarity index 100% |
| rename from vendor/fastrand-2.5.0/LICENSE-APACHE |
| rename to core/vendor/fastrand-2.5.0/LICENSE-APACHE |
diff --git a/vendor/fastrand-2.5.0/LICENSE-MIT b/core/vendor/fastrand-2.5.0/LICENSE-MIT| similarity index 100% |
| rename from vendor/fastrand-2.5.0/LICENSE-MIT |
| rename to core/vendor/fastrand-2.5.0/LICENSE-MIT |
diff --git a/vendor/fastrand-2.5.0/README.md b/core/vendor/fastrand-2.5.0/README.md| similarity index 100% |
| rename from vendor/fastrand-2.5.0/README.md |
| rename to core/vendor/fastrand-2.5.0/README.md |
diff --git a/vendor/fastrand-2.5.0/benches/bench.rs b/core/vendor/fastrand-2.5.0/benches/bench.rs| similarity index 100% |
| rename from vendor/fastrand-2.5.0/benches/bench.rs |
| rename to core/vendor/fastrand-2.5.0/benches/bench.rs |
diff --git a/vendor/fastrand-2.5.0/src/global_rng.rs b/core/vendor/fastrand-2.5.0/src/global_rng.rs| similarity index 100% |
| rename from vendor/fastrand-2.5.0/src/global_rng.rs |
| rename to core/vendor/fastrand-2.5.0/src/global_rng.rs |
diff --git a/vendor/fastrand-2.5.0/src/lib.rs b/core/vendor/fastrand-2.5.0/src/lib.rs| similarity index 100% |
| rename from vendor/fastrand-2.5.0/src/lib.rs |
| rename to core/vendor/fastrand-2.5.0/src/lib.rs |
diff --git a/vendor/fastrand-2.5.0/tests/char.rs b/core/vendor/fastrand-2.5.0/tests/char.rs| similarity index 100% |
| rename from vendor/fastrand-2.5.0/tests/char.rs |
| rename to core/vendor/fastrand-2.5.0/tests/char.rs |
diff --git a/vendor/fastrand-2.5.0/tests/smoke.rs b/core/vendor/fastrand-2.5.0/tests/smoke.rs| similarity index 100% |
| rename from vendor/fastrand-2.5.0/tests/smoke.rs |
| rename to core/vendor/fastrand-2.5.0/tests/smoke.rs |
diff --git a/internal/core/core.go b/internal/core/core.go| index 430745d..958c998 100644 |
| --- a/internal/core/core.go |
| +++ b/internal/core/core.go |
| @@ -17,15 +17,31 @@ |
| // |
| // Separately, a malformed update need not trap at all: Task 5's fuzzing |
| // found a 36-byte corrupted update that ran docs_core_apply for 163 real |
| -// CPU seconds before returning an ordinary, correct answer — not a hang, |
| -// but on a server accepting updates from anyone with a share link it is a |
| -// denial-of-service vector no correctness test can see. Every guest call |
| -// here runs under a context.WithTimeout, and the Runtime is built with |
| -// WithCloseOnContextDone(true) so wazero actually terminates a call that |
| -// overruns its budget rather than merely making the caller stop waiting. |
| -// A deadline exceeded is treated exactly like a trap: the instance is |
| -// discarded and a fresh one is rebuilt by replaying the updates already |
| -// known to have applied cleanly. |
| +// CPU seconds under wasmi's interpreter before returning an ordinary, |
| +// correct answer — not a hang, but on a server accepting updates from |
| +// anyone with a share link it is a denial-of-service vector no correctness |
| +// test can see. Every guest call here runs under a context.WithTimeout |
| +// (default one second, tunable per Runtime via WithGuestCallTimeout — see |
| +// its doc comment for why 1s and not the originally-chosen 5s), and the |
| +// Runtime is built with WithCloseOnContextDone(true) so wazero actually |
| +// terminates a call that overruns its budget rather than merely making the |
| +// caller stop waiting. A deadline exceeded is treated exactly like a trap: |
| +// the instance is discarded and a fresh one is rebuilt. |
| +// |
| +// # Rebuilding is log replay, not snapshot restore |
| +// |
| +// Spec §5.2 describes recovery as rebuilding "from its latest snapshot" — |
| +// a persisted, compacted state plus a bounded tail of subsequent updates. |
| +// ABI v1 exports no state-encode function (nothing here can ask the guest |
| +// "give me your current state as a single update"), so there is no |
| +// snapshot to load. What this package does instead: every Handle keeps the |
| +// updates it has itself applied successfully, in memory, for the life of |
| +// the Handle, and a discarded instance is rebuilt by replaying that entire |
| +// log from an empty document. This is a deliberate substitution, not an |
| +// oversight — it is what ABI v1 makes possible — and it is why the log |
| +// needs its own bound (maxRetainedUpdates below): without real snapshot |
| +// support, "replay everything" is the only recovery this package can do, |
| +// so the log cannot be allowed to grow without limit. |
| package core |
| import ( |
| @@ -48,16 +64,64 @@ var moduleWASM []byte |
| // rather than behaving strangely. |
| const abiVersion = 1 |
| -// guestCallTimeout bounds every single call into the wasm module. Measured |
| -// fuel consumption for real fixtures tops out in the tens of thousands of |
| -// wasmi fuel units (core/tests/wasm_trap_containment.rs); the 163-second |
| -// pathological input this guards against is not a close call. Five seconds |
| -// is generous for any call this host makes — alloc/free/doc_new/doc_free |
| -// are effectively instantaneous, and apply/text_read on real documents are |
| -// not on a per-keystroke hot path (spec §5.1) — while still being small |
| -// enough that a runaway call fails fast instead of tying up a goroutine for |
| -// minutes. |
| -const guestCallTimeout = 5 * time.Second |
| +// defaultGuestCallTimeout bounds every single call into the wasm module, |
| +// unless overridden with WithGuestCallTimeout. |
| +// |
| +// This was originally 5s, chosen only from real-fixture timings (tens of |
| +// milliseconds at most). Fix-round-1 review measured the actual motivating |
| +// case — Task 5's 163-second-under-wasmi reproducer, replayed against |
| +// wazero's compiler engine at the memory-growth state that produces the |
| +// slowdown (core_internal_test.go's TestSlowUpdateIsCutOffByGuestCallTimeout) |
| +// — at ~4.34s. A 5s cap never fires for the input it exists to stop; it |
| +// would have been dead code for its own motivating case. One second keeps |
| +// roughly 1000x headroom over real fixtures (sub-millisecond) while |
| +// actually cutting off the pathological case at about a quarter of its |
| +// natural running time instead of letting it nearly finish first. |
| +const defaultGuestCallTimeout = 1 * time.Second |
| + |
| +// maxRetainedUpdates bounds how many previously-applied updates a Handle |
| +// will keep for the log-replay recovery described above. Without a bound, |
| +// a long-lived, actively-edited document's log — and so the O(n) cost of |
| +// any future rebuild, and the blast radius of a single trapped or |
| +// timed-out call forcing one — would grow without limit (fix-round-1 |
| +// finding 5). |
| +// |
| +// The real fix is snapshot support: an ABI v2 export that encodes a |
| +// document's current state as one update (yrs supports this internally; |
| +// nothing in core/src/abi.rs exposes it yet), so rebuild could restore a |
| +// compacted snapshot plus only the updates since it, matching spec §5.2 and |
| +// the doc_snapshots table spec §4.1 describes — bounded, cheap recovery |
| +// regardless of a document's total history. That belongs with the |
| +// snapshot/compaction work, not here. |
| +// |
| +// Until then, this package does the safe thing available under ABI v1: |
| +// once a document's log is full, Apply refuses to accept further updates |
| +// at all (ErrUpdateLogFull), rather than either growing the log without |
| +// bound or silently dropping old entries — dropping old entries would make |
| +// a later rebuild replay an incomplete history and produce silently wrong |
| +// content, which is worse than refusing. |
| +const maxRetainedUpdates = 4096 |
| + |
| +// ErrUpdateLogFull is returned by Handle.Apply once a document's retained |
| +// update log has reached maxRetainedUpdates. See that constant's doc |
| +// comment: this is a deliberate, visible failure standing in for |
| +// compaction support this ABI doesn't have yet, not a bug. |
| +var ErrUpdateLogFull = errors.New("update log exceeds the retained limit; document needs compaction (unsupported under ABI v1) before it can accept further updates") |
| + |
| +// Option configures a Runtime. See WithGuestCallTimeout. |
| +type Option func(*runtimeOptions) |
| + |
| +type runtimeOptions struct { |
| + callTimeout time.Duration |
| +} |
| + |
| +// WithGuestCallTimeout overrides defaultGuestCallTimeout for one Runtime. |
| +// Exists so the cap can be tuned (for a slower/faster production host, or |
| +// for a test that wants a tight budget without waiting out the default) in |
| +// a config value instead of a recompile. |
| +func WithGuestCallTimeout(d time.Duration) Option { |
| + return func(o *runtimeOptions) { o.callTimeout = d } |
| +} |
| // Runtime owns the compiled module. Compiling is expensive and |
| // instantiating is not, so one Runtime is shared and each document gets its |
| @@ -65,15 +129,25 @@ const guestCallTimeout = 5 * time.Second |
| type Runtime struct { |
| rt wazero.Runtime |
| compiled wazero.CompiledModule |
| + |
| + // callTimeout bounds every guest call made through this Runtime and |
| + // every Handle it creates. See defaultGuestCallTimeout and |
| + // WithGuestCallTimeout. |
| + callTimeout time.Duration |
| } |
| -func New(ctx context.Context) (*Runtime, error) { |
| +func New(ctx context.Context, opts ...Option) (*Runtime, error) { |
| + o := runtimeOptions{callTimeout: defaultGuestCallTimeout} |
| + for _, opt := range opts { |
| + opt(&o) |
| + } |
| + |
| // WithCloseOnContextDone(true): without this, a context deadline or |
| // cancellation on a guest call only makes the *caller* stop waiting — |
| // the underlying goroutine executing wasm keeps running to completion. |
| // With it, wazero itself terminates the call and closes the module |
| - // when the context is done, which is what turns "the 163-second input" |
| - // into a bounded failure instead of a resource leak. |
| + // when the context is done, which is what turns a pathologically slow |
| + // input into a bounded failure instead of a resource leak. |
| cfg := wazero.NewRuntimeConfig().WithCloseOnContextDone(true) |
| rt := wazero.NewRuntimeWithConfig(ctx, cfg) |
| compiled, err := rt.CompileModule(ctx, moduleWASM) |
| @@ -81,7 +155,7 @@ func New(ctx context.Context) (*Runtime, error) { |
| rt.Close(ctx) |
| return nil, fmt.Errorf("compiling core module: %w", err) |
| } |
| - return &Runtime{rt: rt, compiled: compiled}, nil |
| + return &Runtime{rt: rt, compiled: compiled, callTimeout: o.callTimeout}, nil |
| } |
| func (r *Runtime) Close(ctx context.Context) error { return r.rt.Close(ctx) } |
| @@ -90,7 +164,7 @@ func (r *Runtime) Close(ctx context.Context) error { return r.rt.Close(ctx) } |
| // version. Used both for a brand-new document and to rebuild after a |
| // discarded (trapped or timed-out) instance. |
| func (r *Runtime) instantiate(ctx context.Context) (api.Module, error) { |
| - cctx, cancel := context.WithTimeout(ctx, guestCallTimeout) |
| + cctx, cancel := context.WithTimeout(ctx, r.callTimeout) |
| defer cancel() |
| // Empty name: wazero treats an anonymous module (ModuleName == "") as |
| // exempt from its store's name-uniqueness check, so instantiating many |
| @@ -101,7 +175,7 @@ func (r *Runtime) instantiate(ctx context.Context) (api.Module, error) { |
| return nil, fmt.Errorf("instantiating core module: %w", err) |
| } |
| - version, err := callGuest(ctx, mod.ExportedFunction("docs_core_abi_version")) |
| + version, err := callGuest(ctx, r.callTimeout, mod.ExportedFunction("docs_core_abi_version")) |
| if err != nil { |
| mod.Close(ctx) |
| return nil, fmt.Errorf("reading abi version: %w", err) |
| @@ -113,16 +187,16 @@ func (r *Runtime) instantiate(ctx context.Context) (api.Module, error) { |
| return mod, nil |
| } |
| -// callGuest calls fn under guestCallTimeout. A non-nil error here means the |
| -// call trapped, ran past its deadline (in which case wazero has already |
| -// closed the module — WithCloseOnContextDone(true)), or the module was |
| -// otherwise closed out from under it: in every case the instance must be |
| -// treated as poisoned, never merely retried. The error is wrapped in |
| -// trapFault so callers further up (Apply, Text) can tell it apart from an |
| -// ordinary clean rejection (a negative return code from the guest), which |
| -// does not poison anything — see isFatal. |
| -func callGuest(ctx context.Context, fn api.Function, params ...uint64) ([]uint64, error) { |
| - cctx, cancel := context.WithTimeout(ctx, guestCallTimeout) |
| +// callGuest calls fn under the given timeout. A non-nil error here means |
| +// the call trapped, ran past its deadline (in which case wazero has |
| +// already closed the module — WithCloseOnContextDone(true)), or the |
| +// module was otherwise closed out from under it: in every case the |
| +// instance must be treated as poisoned, never merely retried. The error is |
| +// wrapped in trapFault so callers further up (Apply, Text) can tell it |
| +// apart from an ordinary clean rejection (a negative return code from the |
| +// guest), which does not poison anything — see isFatal. |
| +func callGuest(ctx context.Context, timeout time.Duration, fn api.Function, params ...uint64) ([]uint64, error) { |
| + cctx, cancel := context.WithTimeout(ctx, timeout) |
| defer cancel() |
| res, err := fn.Call(cctx, params...) |
| if err != nil { |
| @@ -154,15 +228,39 @@ func isFatal(err error) bool { |
| return errors.As(err, &t) |
| } |
| +// RecoveredError wraps the error from an Apply call whose instance |
| +// trapped or timed out but was then successfully discarded and rebuilt — |
| +// so the *document* is intact (every previously-applied update survived), |
| +// even though this specific update was not (its outcome, applied or not, |
| +// is unknown; it is never retried). Callers that only check "err != nil" |
| +// cannot otherwise tell that apart from an ordinary clean rejection (where |
| +// the instance was never touched) or from a rebuild that itself failed |
| +// (a plain joined error — see Apply — where there is no usable instance at |
| +// all). Use Recovered(err) rather than a type assertion. |
| +type RecoveredError struct{ err error } |
| + |
| +func (e *RecoveredError) Error() string { return e.err.Error() } |
| +func (e *RecoveredError) Unwrap() error { return e.err } |
| + |
| +// Recovered reports whether err came from an Apply call that trapped or |
| +// timed out but whose instance was successfully discarded and rebuilt — |
| +// see RecoveredError. |
| +func Recovered(err error) bool { |
| + var r *RecoveredError |
| + return errors.As(err, &r) |
| +} |
| + |
| // Handle is one document, backed by its own module instance. Not safe for |
| // concurrent use; callers serialise access per document. |
| // |
| // applied records, in order, every update byte slice this handle has |
| -// itself applied successfully (rc == 0). It exists solely so a trapped or |
| -// timed-out instance can be discarded and a fresh one rebuilt by replaying |
| -// only the updates already known to be good — the update that triggered |
| -// the discard is never replayed, matching §5.2's "the request fails |
| -// cleanly; the document does not." |
| +// itself applied successfully (rc == 0), up to maxRetainedUpdates. It |
| +// exists solely so a trapped or timed-out instance can be discarded and a |
| +// fresh one rebuilt by replaying only the updates already known to be good |
| +// — the update that triggered the discard is never replayed, matching |
| +// §5.2's "the request fails cleanly; the document does not." See the |
| +// package doc comment ("Rebuilding is log replay, not snapshot restore") |
| +// for why this is a log rather than an actual snapshot. |
| type Handle struct { |
| rt *Runtime |
| guid string |
| @@ -185,7 +283,7 @@ func (r *Runtime) NewDoc(ctx context.Context, guid string, clientID uint64) (*Ha |
| } |
| handle := &Handle{rt: r, guid: guid, clientID: clientID, mod: mod} |
| - docHandle, err := createDoc(ctx, mod, guid, clientID) |
| + docHandle, err := createDoc(ctx, r.callTimeout, mod, guid, clientID) |
| if err != nil { |
| mod.Close(ctx) |
| return nil, err |
| @@ -197,19 +295,19 @@ func (r *Runtime) NewDoc(ctx context.Context, guid string, clientID uint64) (*Ha |
| // createDoc allocates the guid in mod's linear memory, calls |
| // docs_core_doc_new, and frees the buffer. Shared between NewDoc and a |
| // post-trap rebuild. |
| -func createDoc(ctx context.Context, mod api.Module, guid string, clientID uint64) (uint64, error) { |
| +func createDoc(ctx context.Context, timeout time.Duration, mod api.Module, guid string, clientID uint64) (uint64, error) { |
| guidBytes := []byte(guid) |
| - ptr, err := allocIn(ctx, mod, uint64(len(guidBytes))) |
| + ptr, err := allocIn(ctx, timeout, mod, uint64(len(guidBytes))) |
| if err != nil { |
| return 0, fmt.Errorf("alloc for guid: %w", err) |
| } |
| - defer freeIn(ctx, mod, ptr, uint64(len(guidBytes))) |
| + defer freeIn(ctx, timeout, mod, ptr, uint64(len(guidBytes))) |
| if !mod.Memory().Write(uint32(ptr), guidBytes) { |
| return 0, fmt.Errorf("writing guid at %d: out of range", ptr) |
| } |
| - res, err := callGuest(ctx, mod.ExportedFunction("docs_core_doc_new"), ptr, uint64(len(guidBytes)), clientID) |
| + res, err := callGuest(ctx, timeout, mod.ExportedFunction("docs_core_doc_new"), ptr, uint64(len(guidBytes)), clientID) |
| if err != nil { |
| return 0, fmt.Errorf("doc_new: %w", err) |
| } |
| @@ -221,8 +319,8 @@ func createDoc(ctx context.Context, mod api.Module, guid string, clientID uint64 |
| // allocIn calls docs_core_alloc and checks the 0-on-failure sentinel |
| // (including any length over isize::MAX — core/src/abi.rs's layout_for). |
| -func allocIn(ctx context.Context, mod api.Module, length uint64) (uint64, error) { |
| - res, err := callGuest(ctx, mod.ExportedFunction("docs_core_alloc"), length) |
| +func allocIn(ctx context.Context, timeout time.Duration, mod api.Module, length uint64) (uint64, error) { |
| + res, err := callGuest(ctx, timeout, mod.ExportedFunction("docs_core_alloc"), length) |
| if err != nil { |
| return 0, fmt.Errorf("alloc trapped or timed out: %w", err) |
| } |
| @@ -236,12 +334,12 @@ func allocIn(ctx context.Context, mod api.Module, length uint64) (uint64, error) |
| // "the instance is now poisoned", which the caller's own callGuest error |
| // (the one that actually matters) will already have triggered, so this |
| // deliberately does not itself return an error to plumb through defers. |
| -func freeIn(ctx context.Context, mod api.Module, ptr, length uint64) { |
| - _, _ = callGuest(ctx, mod.ExportedFunction("docs_core_free"), ptr, length) |
| +func freeIn(ctx context.Context, timeout time.Duration, mod api.Module, ptr, length uint64) { |
| + _, _ = callGuest(ctx, timeout, mod.ExportedFunction("docs_core_free"), ptr, length) |
| } |
| func (h *Handle) Close(ctx context.Context) error { |
| - _, err := callGuest(ctx, h.mod.ExportedFunction("docs_core_doc_free"), h.handle) |
| + _, err := callGuest(ctx, h.rt.callTimeout, h.mod.ExportedFunction("docs_core_doc_free"), h.handle) |
| if closeErr := h.mod.Close(ctx); err == nil { |
| err = closeErr |
| } |
| @@ -260,10 +358,11 @@ func (h *Handle) Close(ctx context.Context) error { |
| // already-expired deadline would make recovery fail precisely in the case |
| // it exists for, so this strips ctx's cancellation/deadline |
| // (context.WithoutCancel) while keeping any values on it, and relies on |
| -// guestCallTimeout to still bound each individual call made during the |
| -// rebuild itself. |
| +// callTimeout to still bound each individual call made during the rebuild |
| +// itself. |
| func (h *Handle) rebuild(ctx context.Context) error { |
| rebuildCtx := context.WithoutCancel(ctx) |
| + timeout := h.rt.callTimeout |
| // Best-effort: the old instance may already be closed (a timeout under |
| // WithCloseOnContextDone(true) closes it as part of terminating the |
| @@ -274,13 +373,13 @@ func (h *Handle) rebuild(ctx context.Context) error { |
| if err != nil { |
| return fmt.Errorf("rebuilding instance: %w", err) |
| } |
| - docHandle, err := createDoc(rebuildCtx, mod, h.guid, h.clientID) |
| + docHandle, err := createDoc(rebuildCtx, timeout, mod, h.guid, h.clientID) |
| if err != nil { |
| mod.Close(rebuildCtx) |
| return fmt.Errorf("rebuilding instance: %w", err) |
| } |
| for i, update := range h.applied { |
| - if err := applyOn(rebuildCtx, mod, docHandle, update); err != nil { |
| + if err := applyOn(rebuildCtx, timeout, mod, docHandle, update); err != nil { |
| mod.Close(rebuildCtx) |
| return fmt.Errorf("rebuilding instance: replaying update %d/%d, previously applied cleanly: %w", |
| i+1, len(h.applied), err) |
| @@ -294,18 +393,18 @@ func (h *Handle) rebuild(ctx context.Context) error { |
| // applyOn runs docs_core_apply against a specific (mod, docHandle) pair — |
| // factored out so both Handle.Apply and rebuild's replay share the same |
| // alloc/write/call/free/error-code logic. |
| -func applyOn(ctx context.Context, mod api.Module, docHandle uint64, update []byte) error { |
| - ptr, err := allocIn(ctx, mod, uint64(len(update))) |
| +func applyOn(ctx context.Context, timeout time.Duration, mod api.Module, docHandle uint64, update []byte) error { |
| + ptr, err := allocIn(ctx, timeout, mod, uint64(len(update))) |
| if err != nil { |
| return fmt.Errorf("alloc: %w", err) |
| } |
| - defer freeIn(ctx, mod, ptr, uint64(len(update))) |
| + defer freeIn(ctx, timeout, mod, ptr, uint64(len(update))) |
| if !mod.Memory().Write(uint32(ptr), update) { |
| return fmt.Errorf("writing %d bytes at %d: out of range", len(update), ptr) |
| } |
| - rc, err := callGuest(ctx, mod.ExportedFunction("docs_core_apply"), docHandle, ptr, uint64(len(update))) |
| + rc, err := callGuest(ctx, timeout, mod.ExportedFunction("docs_core_apply"), docHandle, ptr, uint64(len(update))) |
| if err != nil { |
| return fmt.Errorf("apply trapped or timed out: %w", err) |
| } |
| @@ -323,9 +422,21 @@ func applyOn(ctx context.Context, mod api.Module, docHandle uint64, update []byt |
| // different: the instance is discarded and rebuilt from every update |
| // applied here so far, and the triggering update is reported as an error |
| // but never retried or logged, so it can never itself be replayed into the |
| -// rebuilt instance. |
| +// rebuilt instance. When that rebuild succeeds, the returned error is a |
| +// *RecoveredError (see Recovered) so a caller can tell "this update was |
| +// rejected but the document is fine" apart from "the document itself is |
| +// gone" (a rebuild that fails returns a plain joined error instead). |
| +// |
| +// Once the document's retained update log is full (maxRetainedUpdates), |
| +// Apply refuses any further update — including one that would otherwise |
| +// apply cleanly — with ErrUpdateLogFull, rather than growing the log |
| +// without bound. See maxRetainedUpdates's doc comment. |
| func (h *Handle) Apply(ctx context.Context, update []byte) error { |
| - err := applyOn(ctx, h.mod, h.handle, update) |
| + if len(h.applied) >= maxRetainedUpdates { |
| + return ErrUpdateLogFull |
| + } |
| + |
| + err := applyOn(ctx, h.rt.callTimeout, h.mod, h.handle, update) |
| if err == nil { |
| // Copy: the caller may reuse or mutate its update slice after this |
| // call returns, and this log must survive a later rebuild. |
| @@ -338,26 +449,37 @@ func (h *Handle) Apply(ctx context.Context, update []byte) error { |
| if rebuildErr := h.rebuild(ctx); rebuildErr != nil { |
| return errors.Join(err, rebuildErr) |
| } |
| + return &RecoveredError{err} |
| } |
| return err |
| } |
| -// Text returns the document's current text content. |
| +// Text returns the document's current text content. If the read itself |
| +// traps or times out, the instance is discarded and rebuilt exactly as in |
| +// Apply, and the read is retried once against the freshly rebuilt |
| +// instance — the content is available at that point, and returning an |
| +// empty string plus an error instead of it would tell the caller the data |
| +// is gone when it is not. |
| func (h *Handle) Text(ctx context.Context) (string, error) { |
| - text, err := textOn(ctx, h.mod, h.handle) |
| + text, err := textOn(ctx, h.rt.callTimeout, h.mod, h.handle) |
| if err == nil { |
| return text, nil |
| } |
| - if isFatal(err) { |
| - if rebuildErr := h.rebuild(ctx); rebuildErr != nil { |
| - return "", errors.Join(err, rebuildErr) |
| - } |
| + if !isFatal(err) { |
| + return "", err |
| + } |
| + if rebuildErr := h.rebuild(ctx); rebuildErr != nil { |
| + return "", errors.Join(err, rebuildErr) |
| + } |
| + text, err = textOn(ctx, h.rt.callTimeout, h.mod, h.handle) |
| + if err != nil { |
| + return "", fmt.Errorf("reading text from the rebuilt instance: %w", err) |
| } |
| - return "", err |
| + return text, nil |
| } |
| -func textOn(ctx context.Context, mod api.Module, docHandle uint64) (string, error) { |
| - lenRes, err := callGuest(ctx, mod.ExportedFunction("docs_core_text_len"), docHandle) |
| +func textOn(ctx context.Context, timeout time.Duration, mod api.Module, docHandle uint64) (string, error) { |
| + lenRes, err := callGuest(ctx, timeout, mod.ExportedFunction("docs_core_text_len"), docHandle) |
| if err != nil { |
| return "", fmt.Errorf("text_len trapped or timed out: %w", err) |
| } |
| @@ -372,23 +494,31 @@ func textOn(ctx context.Context, mod api.Module, docHandle uint64) (string, erro |
| return "", nil |
| } |
| - ptr, err := allocIn(ctx, mod, n) |
| + ptr, err := allocIn(ctx, timeout, mod, n) |
| if err != nil { |
| return "", fmt.Errorf("alloc: %w", err) |
| } |
| - defer freeIn(ctx, mod, ptr, n) |
| + defer freeIn(ctx, timeout, mod, ptr, n) |
| - rc, err := callGuest(ctx, mod.ExportedFunction("docs_core_text_read"), docHandle, ptr, n) |
| + rc, err := callGuest(ctx, timeout, mod.ExportedFunction("docs_core_text_read"), docHandle, ptr, n) |
| if err != nil { |
| return "", fmt.Errorf("text_read trapped or timed out: %w", err) |
| } |
| - if written := int32(rc[0]); written < 0 { |
| + written := int32(rc[0]) |
| + if written < 0 { |
| return "", fmt.Errorf("text_read failed: code %d", written) |
| } |
| - buf, ok := mod.Memory().Read(uint32(ptr), uint32(n)) |
| + // docs_core_text_read returns s.len(), guaranteed only <= out_len |
| + // (core/src/abi.rs:287 — the buffer sized by text_len is an upper |
| + // bound, not a promise the two calls agree exactly). Reading n bytes |
| + // instead of the written count would, if they ever diverged, append |
| + // uninitialised allocator bytes past the real content to the returned |
| + // string (fix-round-1 finding 3) — read exactly what text_read says it |
| + // wrote. |
| + buf, ok := mod.Memory().Read(uint32(ptr), uint32(written)) |
| if !ok { |
| - return "", fmt.Errorf("reading %d bytes at %d: out of range", n, ptr) |
| + return "", fmt.Errorf("reading %d bytes at %d: out of range", written, ptr) |
| } |
| return string(buf), nil |
| } |
diff --git a/internal/core/core_internal_test.go b/internal/core/core_internal_test.go| index 0a92d10..6fb8f67 100644 |
| --- a/internal/core/core_internal_test.go |
| +++ b/internal/core/core_internal_test.go |
| @@ -1,13 +1,22 @@ |
| package core |
| -// White-box (package core) test for the execution cap. It needs the |
| -// unexported Runtime.instantiate/createDoc/applyOn helpers directly, and |
| -// constructs a Handle by hand, because reproducing a genuinely slow guest |
| -// call requires state this package's public API deliberately never |
| -// exposes: a *shared* module instance carrying the accumulated linear |
| -// memory of many prior documents. |
| +// White-box (package core) tests for behaviour the public API alone can't |
| +// prove: |
| // |
| -// # Why this is the right way to prove the cap fires |
| +// - TestTrappedInstanceIsDiscardedAndRebuilds needs to check the module |
| +// instance itself changed (fix-round-1 finding 1: this specific trap |
| +// fires before any RefCell is borrowed, so a host that never discarded |
| +// anything would pass every check the external core_test package can |
| +// make). |
| +// - TestSlowUpdateIsCutOffByGuestCallTimeout needs the unexported |
| +// Runtime.instantiate/createDoc/applyOn helpers directly, and |
| +// constructs a Handle by hand, because reproducing a genuinely slow |
| +// guest call requires state the public API deliberately never exposes: |
| +// a *shared* module instance carrying the accumulated linear memory of |
| +// many prior documents. |
| +// |
| +// # Why the timeout test replays a real fuzz sequence rather than just |
| +// asserting a slow call gets cut off |
| // |
| // Task 5's fuzzing (core/tests/wasm_trap_containment.rs) found that |
| // mutating the `concurrent` fixture with a fixed xorshift64 RNG (seed |
| @@ -27,21 +36,19 @@ package core |
| // So this test replays the exact same deterministic sequence — same RNG, |
| // same mutate(), same fixture cycling, same trial count — against a real |
| // module instance to reach the same state, confirms trial 1133 reproduces |
| -// the exact bytes documented in wasm_trap_containment.rs (a drift check, |
| -// so a future change to that fuzz sequence fails loudly here instead of |
| -// silently testing nothing), and confirms that under wazero's compiler |
| -// engine this input is still genuinely slow (single-digit seconds, not |
| -// milliseconds) when left uncapped — then proves a short caller-imposed |
| -// deadline cuts it off well before that. |
| -// |
| -// It runs through the exported Handle.Apply, not a bespoke code path: the |
| -// only irregular part is how the Handle's *first* instance came to exist |
| -// (built by hand, already warmed up), everything after that first call is |
| -// exactly what a real caller sees. |
| +// the exact bytes documented in wasm_trap_containment.rs, and confirms |
| +// that under wazero's compiler engine this input is still genuinely slow |
| +// (single-digit seconds, not milliseconds) when left uncapped — measured |
| +// at ~4.34s, which is why defaultGuestCallTimeout is 1s and not the |
| +// originally-chosen 5s (fix-round-1 finding 2): 5s barely — and |
| +// unreliably — beats a 4.34s natural completion; it does not actually |
| +// demonstrate the cap doing anything for its own motivating case. |
| import ( |
| "context" |
| "encoding/hex" |
| + "encoding/json" |
| + "errors" |
| "fmt" |
| "os" |
| "path/filepath" |
| @@ -91,7 +98,38 @@ func loadFixtureBytes(t *testing.T, name string) []byte { |
| return b |
| } |
| -func TestSlowUpdateIsCutOffByGuestCallTimeout(t *testing.T) { |
| +func loadFixtureExpectedText(t *testing.T, name string) string { |
| + t.Helper() |
| + raw, err := os.ReadFile(filepath.Join("..", "..", "fixtures", "data", name+".expected.json")) |
| + if err != nil { |
| + t.Fatalf("reading expectation for %s: %v", name, err) |
| + } |
| + var want struct { |
| + Text string `json:"text"` |
| + } |
| + if err := json.Unmarshal(raw, &want); err != nil { |
| + t.Fatalf("parsing expectation for %s: %v", name, err) |
| + } |
| + return want.Text |
| +} |
| + |
| +// TestTrappedInstanceIsDiscardedAndRebuilds is the test the plan's |
| +// containment argument depends on. §5.2's design is: a trap poisons the |
| +// module instance's linear memory, so the host must discard that instance |
| +// and rebuild the document rather than reuse it. That claim is only real |
| +// if this package actually does it. |
| +// |
| +// Fix-round-1 finding 1: an earlier version of this test (in the external |
| +// core_test package) only checked behaviour — that Apply reports an error |
| +// and a later Text still returns the right content — and the reviewer |
| +// showed that passes identically against a host that simply keeps reusing |
| +// the trapped instance, because this specific reproducer traps inside |
| +// Update::decode_v1 before any RefCell is borrowed: nothing about *this* |
| +// trap corrupts a reused instance enough to fail those checks. The |
| +// assertion that can actually fail if the discard is removed is a direct |
| +// check that h.mod is a different value afterward — which needs |
| +// unexported access, hence this file rather than core_test.go. |
| +func TestTrappedInstanceIsDiscardedAndRebuilds(t *testing.T) { |
| ctx := context.Background() |
| rt, err := New(ctx) |
| if err != nil { |
| @@ -99,6 +137,126 @@ func TestSlowUpdateIsCutOffByGuestCallTimeout(t *testing.T) { |
| } |
| t.Cleanup(func() { rt.Close(ctx) }) |
| + doc, err := rt.NewDoc(ctx, "go-host-test-guid", 9001) |
| + if err != nil { |
| + t.Fatalf("NewDoc: %v", err) |
| + } |
| + defer doc.Close(ctx) |
| + |
| + // Establish real content before the trap, so recovery can be checked |
| + // against more than an empty document: a rebuild that silently lost |
| + // this would be a much worse failure than one that merely can't apply |
| + // anything further. |
| + helloUpdate := loadFixtureBytes(t, "hello") |
| + helloWant := loadFixtureExpectedText(t, "hello") |
| + if err := doc.Apply(ctx, helloUpdate); err != nil { |
| + t.Fatalf("seeding the document before the trap: %v", err) |
| + } |
| + |
| + // Captured before the trap: this is the actual thing under test. |
| + oldMod := doc.mod |
| + |
| + // core/tests/wasm_trap_containment.rs's confirmed repro for |
| + // borrow_leak_repro_trapping_unicode_mutation: a 3-byte-flipped |
| + // mutation of the `unicode` fixture that reliably yields |
| + // TrapCode(MemoryOutOfBounds) reaching the from_utf8_unchecked call |
| + // this package's doc comment describes. Confirmed here under wazero |
| + // itself (not just wasmi, which that Rust test uses). |
| + trapHex := "01010100040107636f6e74656e741168c3a96c6c6f20f09f8c8d206361ffc3a900" |
| + trapping, err := hex.DecodeString(trapHex) |
| + if err != nil { |
| + t.Fatalf("decoding the trap reproducer: %v", err) |
| + } |
| + |
| + applyErr := doc.Apply(ctx, trapping) |
| + if applyErr == nil { |
| + t.Fatal("expected the known trap reproducer to fail — if this now succeeds, it stopped " + |
| + "trapping and needs replacing (see core/tests/wasm_trap_containment.rs)") |
| + } |
| + if !Recovered(applyErr) { |
| + t.Errorf("expected a trap whose instance was successfully rebuilt to report Recovered(err) == true, got err = %v", applyErr) |
| + } |
| + |
| + // The check that actually distinguishes "discarded and rebuilt" from |
| + // "reused, and happened not to break for this particular trap": the |
| + // module instance itself must be a different value. |
| + if doc.mod == oldMod { |
| + t.Fatalf("doc.mod is unchanged after a trapping Apply — the poisoned instance was not discarded " + |
| + "(this specific trap does not corrupt a reused instance, so every behavioural check below would " + |
| + "pass even without discard — that is the whole point of this check)") |
| + } |
| + |
| + // The instance must have been discarded and a fresh one rebuilt from |
| + // the update log — not merely "still answers something", but holding |
| + // exactly the content applied before the trap. |
| + got, err := doc.Text(ctx) |
| + if err != nil { |
| + t.Fatalf("Text after the trap: %v", err) |
| + } |
| + if got != helloWant { |
| + t.Fatalf("after trap recovery got %q, want %q — the rebuilt instance lost pre-trap content", got, helloWant) |
| + } |
| + |
| + // And the rebuilt instance must be fully usable going forward, not |
| + // just readable: apply a further update and confirm it merges in. |
| + unicodeUpdate := loadFixtureBytes(t, "unicode") |
| + unicodeWant := loadFixtureExpectedText(t, "unicode") |
| + doc2, err := rt.NewDoc(ctx, "go-host-test-guid-2", 9002) |
| + if err != nil { |
| + t.Fatalf("NewDoc for a second, unrelated document: %v", err) |
| + } |
| + defer doc2.Close(ctx) |
| + if err := doc2.Apply(ctx, unicodeUpdate); err != nil { |
| + t.Fatalf("applying to an unrelated document after another document's trap: %v", err) |
| + } |
| + got2, err := doc2.Text(ctx) |
| + if err != nil { |
| + t.Fatalf("Text on the unrelated document: %v", err) |
| + } |
| + if got2 != unicodeWant { |
| + t.Errorf("unrelated document got %q, want %q — a trap in one document's instance must not affect another's", got2, unicodeWant) |
| + } |
| +} |
| + |
| +// TestGuestCallTimeoutOption confirms New defaults to |
| +// defaultGuestCallTimeout and that WithGuestCallTimeout actually overrides |
| +// it (fix-round-1 finding 2's "tunable without a recompile"). Deliberately |
| +// cheap: this checks the field is wired correctly, not the cap firing |
| +// under load — TestSlowUpdateIsCutOffByGuestCallTimeout below covers that. |
| +func TestGuestCallTimeoutOption(t *testing.T) { |
| + ctx := context.Background() |
| + |
| + rt, err := New(ctx) |
| + if err != nil { |
| + t.Fatalf("core.New: %v", err) |
| + } |
| + if rt.callTimeout != defaultGuestCallTimeout { |
| + t.Errorf("default callTimeout = %v, want %v", rt.callTimeout, defaultGuestCallTimeout) |
| + } |
| + rt.Close(ctx) |
| + |
| + const override = 250 * time.Millisecond |
| + rt2, err := New(ctx, WithGuestCallTimeout(override)) |
| + if err != nil { |
| + t.Fatalf("core.New with WithGuestCallTimeout: %v", err) |
| + } |
| + defer rt2.Close(ctx) |
| + if rt2.callTimeout != override { |
| + t.Errorf("callTimeout after WithGuestCallTimeout(%v) = %v, want %v", override, rt2.callTimeout, override) |
| + } |
| +} |
| + |
| +func TestSlowUpdateIsCutOffByGuestCallTimeout(t *testing.T) { |
| + ctx := context.Background() |
| + rt, err := New(ctx) // default 1s callTimeout — this is the setting under test. |
| + if err != nil { |
| + t.Fatalf("core.New: %v", err) |
| + } |
| + t.Cleanup(func() { rt.Close(ctx) }) |
| + if rt.callTimeout != defaultGuestCallTimeout { |
| + t.Fatalf("expected the default callTimeout (%v), got %v", defaultGuestCallTimeout, rt.callTimeout) |
| + } |
| + |
| mod, err := rt.instantiate(ctx) |
| if err != nil { |
| t.Fatalf("instantiate: %v", err) |
| @@ -120,11 +278,11 @@ func TestSlowUpdateIsCutOffByGuestCallTimeout(t *testing.T) { |
| for i := 0; i < 1133; i++ { |
| fi := r.nextUsize(len(fixtures)) |
| mutated := mutate(r, fixtures[fi]) |
| - docHandle, err := createDoc(ctx, mod, fmt.Sprintf("warmup-%d", i), uint64(2_000_000+i)) |
| + docHandle, err := createDoc(ctx, rt.callTimeout, mod, fmt.Sprintf("warmup-%d", i), uint64(2_000_000+i)) |
| if err != nil { |
| t.Fatalf("warm-up trial %d: doc_new failed unexpectedly: %v", i, err) |
| } |
| - _ = applyOn(ctx, mod, docHandle, mutated) // outcome irrelevant; only the memory growth matters |
| + _ = applyOn(ctx, rt.callTimeout, mod, docHandle, mutated) // outcome irrelevant; only the memory growth matters |
| } |
| // Trial 1133 itself: the exact reproducer documented in |
| @@ -143,21 +301,21 @@ func TestSlowUpdateIsCutOffByGuestCallTimeout(t *testing.T) { |
| t.Fatalf("expected trial 1133 to mutate the concurrent fixture, got %s", fixtureNames[fi]) |
| } |
| - docHandle, err := createDoc(ctx, mod, "trial-1133", 2_000_000+1133) |
| + docHandle, err := createDoc(ctx, rt.callTimeout, mod, "trial-1133", 2_000_000+1133) |
| if err != nil { |
| t.Fatalf("creating the document under test: %v", err) |
| } |
| // Sanity: confirm this input is genuinely slow (not just a fast trap) |
| // under wazero's compiler engine at this memory-growth state, with a |
| - // budget generous enough to let it run to its natural conclusion. |
| - // Measured while building this test: ~4.36s (vs ~3ms with no memory |
| - // growth, and 163s under wasmi's interpreter on the equivalent state) |
| - // — well past what a per-keystroke host call should ever be allowed |
| - // to occupy a goroutine for. |
| + // budget generous enough to let it run to its natural conclusion — |
| + // deliberately not rt.callTimeout, since this call needs to be allowed |
| + // to run past it to measure the natural time. Measured while building |
| + // this test: ~4.34s (vs ~3ms with no memory growth, and 163s under |
| + // wasmi's interpreter on the equivalent state). |
| sanityCtx, sanityCancel := context.WithTimeout(ctx, 20*time.Second) |
| sanityStart := time.Now() |
| - sanityErr := applyOn(sanityCtx, mod, docHandle, mutated) |
| + sanityErr := applyOn(sanityCtx, 20*time.Second, mod, docHandle, mutated) |
| sanityElapsed := time.Since(sanityStart) |
| sanityCancel() |
| if sanityErr == nil { |
| @@ -165,53 +323,58 @@ func TestSlowUpdateIsCutOffByGuestCallTimeout(t *testing.T) { |
| } |
| if sanityElapsed < 200*time.Millisecond { |
| t.Fatalf("expected the known pathological update to run for at least hundreds of "+ |
| - "milliseconds at this memory-growth state (it measured ~4.36s while building this test); "+ |
| + "milliseconds at this memory-growth state (it measured ~4.34s while building this test); "+ |
| "it returned in %v instead — the reproducer may have stopped reproducing, which would make "+ |
| "the capped assertion below meaningless", sanityElapsed) |
| } |
| t.Logf("uncapped: pathological update ran %v before failing on its own (%v)", sanityElapsed, sanityErr) |
| - // The actual assertion: a fresh document for the same instance, |
| - // wired through the exported Handle so this exercises the real |
| - // Apply()/rebuild() path, given a deadline far shorter than the |
| - // update's natural running time, returns promptly instead of running |
| - // to completion — proving WithCloseOnContextDone(true) actually |
| - // terminates the in-flight call rather than merely abandoning the |
| - // caller's wait. |
| - docHandle2, err := createDoc(ctx, mod, "trial-1133b", 2_000_000+11330) |
| + // The actual assertion: a fresh document on the same (already grown) |
| + // instance, wired through the exported Handle so this exercises the |
| + // real Apply()/rebuild() path, called with a plain context carrying no |
| + // deadline of its own — so what cuts this call off, if anything does, |
| + // is exactly rt.callTimeout (the default, 1s), not a contrived |
| + // caller-side budget. That is the point fix-round-1 finding 2 raised: |
| + // the previous 5s default was close enough to this input's ~4.34s |
| + // natural running time that the cap wasn't reliably doing anything for |
| + // its own motivating case. 1s should cut it off at roughly a quarter |
| + // of that. |
| + docHandle2, err := createDoc(ctx, rt.callTimeout, mod, "trial-1133b", 2_000_000+11330) |
| if err != nil { |
| t.Fatalf("creating the second document under test: %v", err) |
| } |
| handle := &Handle{rt: rt, guid: "trial-1133b", clientID: 2_000_000 + 11330, mod: mod, handle: docHandle2} |
| oldMod := handle.mod |
| - const shortDeadline = 300 * time.Millisecond |
| - capCtx, capCancel := context.WithTimeout(ctx, shortDeadline) |
| - defer capCancel() |
| - |
| start := time.Now() |
| - applyErr := handle.Apply(capCtx, mutated) |
| + applyErr := handle.Apply(ctx, mutated) |
| elapsed := time.Since(start) |
| if applyErr == nil { |
| t.Fatalf("expected the capped Apply to fail (it should have been cut off before completing), got nil error after %v", elapsed) |
| } |
| // Generous upper bound: the call must not have been allowed to run |
| - // anywhere near its natural ~4.36s completion time. shortDeadline |
| - // (300ms) plus slack for scheduling/cleanup overhead. |
| - const upperBound = 2 * time.Second |
| + // anywhere near its natural ~4.34s completion time. |
| + const upperBound = 3 * time.Second |
| if elapsed > upperBound { |
| - t.Fatalf("Apply took %v with a %v deadline (natural runtime is ~4.36s) — "+ |
| - "WithCloseOnContextDone did not actually cut the call off", elapsed, shortDeadline) |
| + t.Fatalf("Apply took %v against a %v default cap (natural runtime is ~4.34s) — "+ |
| + "WithCloseOnContextDone did not actually cut the call off", elapsed, defaultGuestCallTimeout) |
| } |
| - t.Logf("capped: Apply returned after %v (deadline %v, uncapped natural runtime %v): %v", |
| - elapsed, shortDeadline, sanityElapsed, applyErr) |
| + t.Logf("capped: Apply returned after %v (default cap %v, uncapped natural runtime %v): %v", |
| + elapsed, defaultGuestCallTimeout, sanityElapsed, applyErr) |
| // The instance must have been discarded and rebuilt: a fresh module, |
| // not the one that was mid-flight when the deadline hit. |
| if handle.mod == oldMod { |
| t.Fatalf("handle.mod is unchanged after a fatal (timed-out) Apply — the poisoned instance was not discarded") |
| } |
| + // This document had nothing in its log (its own first Apply is the one |
| + // that just timed out), so rebuild had nothing to replay and should |
| + // always succeed — meaning this timeout, too, should come back as |
| + // Recovered. |
| + if !Recovered(applyErr) { |
| + t.Errorf("expected a timed-out Apply whose instance was successfully rebuilt to report Recovered(err) == true, got err = %v", applyErr) |
| + } |
| // And the rebuilt instance must actually work: apply a real update |
| // and read it back, proving "a fresh instance for the same document |
| @@ -232,3 +395,47 @@ func TestSlowUpdateIsCutOffByGuestCallTimeout(t *testing.T) { |
| t.Logf("handle.Close: %v (non-fatal for this test)", err) |
| } |
| } |
| + |
| +// TestApplyLogCapFailsCleanly is fix-round-1 finding 5: h.applied must not |
| +// grow without limit. Rather than spend real time performing |
| +// maxRetainedUpdates genuine applies, this constructs a Handle whose log |
| +// is already at the cap (a legitimate white-box shortcut — what's under |
| +// test is the length check in Apply, not how the log got that long) and |
| +// confirms a further Apply is refused cleanly, without growing the log or |
| +// touching the wasm module at all. |
| +func TestApplyLogCapFailsCleanly(t *testing.T) { |
| + ctx := context.Background() |
| + rt, err := New(ctx) |
| + if err != nil { |
| + t.Fatalf("core.New: %v", err) |
| + } |
| + t.Cleanup(func() { rt.Close(ctx) }) |
| + |
| + doc, err := rt.NewDoc(ctx, "log-cap-test-guid", 1) |
| + if err != nil { |
| + t.Fatalf("NewDoc: %v", err) |
| + } |
| + defer doc.Close(ctx) |
| + |
| + doc.applied = make([][]byte, maxRetainedUpdates) |
| + |
| + hello := loadFixtureBytes(t, "hello") |
| + err = doc.Apply(ctx, hello) |
| + if !errors.Is(err, ErrUpdateLogFull) { |
| + t.Fatalf("expected ErrUpdateLogFull once the log is at its cap, got %v", err) |
| + } |
| + if len(doc.applied) != maxRetainedUpdates { |
| + t.Fatalf("log grew past the cap: len(doc.applied) = %d, want %d", len(doc.applied), maxRetainedUpdates) |
| + } |
| + |
| + // And the instance itself must be untouched: Apply must refuse before |
| + // ever calling into wasm, not after a wasted (and possibly poisoning) |
| + // attempt. |
| + text, err := doc.Text(ctx) |
| + if err != nil { |
| + t.Fatalf("Text after a log-cap refusal: %v", err) |
| + } |
| + if text != "" { |
| + t.Errorf("expected an untouched (empty) document after a log-cap refusal, got %q", text) |
| + } |
| +} |
diff --git a/internal/core/core_test.go b/internal/core/core_test.go| index 66bee5d..02ab2a3 100644 |
| --- a/internal/core/core_test.go |
| +++ b/internal/core/core_test.go |
| @@ -2,7 +2,6 @@ package core_test |
| import ( |
| "context" |
| - "encoding/hex" |
| "encoding/json" |
| "os" |
| "path/filepath" |
| @@ -91,83 +90,15 @@ func TestGarbageUpdateReturnsErrorNotTrap(t *testing.T) { |
| } |
| } |
| -// TestTrappedInstanceIsDiscardedAndRebuilds is the test the plan's |
| -// containment argument depends on. §5.2's design is: a trap poisons the |
| -// module instance's linear memory, so the host must discard that instance |
| -// and rebuild the document rather than reuse it. That claim is only real |
| -// if this package actually does it — a host that quietly kept using a |
| -// trapped instance would make the containment guarantee theoretical. |
| -// |
| -// The trapping input below is |
| -// core/tests/wasm_trap_containment.rs's confirmed repro for |
| -// borrow_leak_repro_trapping_unicode_mutation: a 3-byte-flipped mutation |
| -// of the `unicode` fixture that reliably yields |
| -// TrapCode(MemoryOutOfBounds) reaching the from_utf8_unchecked call this |
| -// package's doc comment describes. It is not a length/content mismatch |
| -// this ABI catches cleanly (unlike TestGarbageUpdateReturnsErrorNotTrap's |
| -// 0xff bytes) — it is a genuine wasm trap, confirmed here under wazero |
| -// itself (not just wasmi, which core/tests/wasm_trap_containment.rs uses). |
| -func TestTrappedInstanceIsDiscardedAndRebuilds(t *testing.T) { |
| - ctx := context.Background() |
| - rt, err := core.New(ctx) |
| - if err != nil { |
| - t.Fatalf("core.New: %v", err) |
| - } |
| - t.Cleanup(func() { rt.Close(ctx) }) |
| - |
| - doc, err := rt.NewDoc(ctx, "go-host-test-guid", 9001) |
| - if err != nil { |
| - t.Fatalf("NewDoc: %v", err) |
| - } |
| - defer doc.Close(ctx) |
| - |
| - // Establish real content before the trap, so recovery can be checked |
| - // against more than an empty document: a rebuild that silently lost |
| - // this would be a much worse failure than one that merely can't apply |
| - // anything further. |
| - helloUpdate, helloWant := fixture(t, "hello") |
| - if err := doc.Apply(ctx, helloUpdate); err != nil { |
| - t.Fatalf("seeding the document before the trap: %v", err) |
| - } |
| - |
| - trapHex := "01010100040107636f6e74656e741168c3a96c6c6f20f09f8c8d206361ffc3a900" |
| - trapping, err := hex.DecodeString(trapHex) |
| - if err != nil { |
| - t.Fatalf("decoding the trap reproducer: %v", err) |
| - } |
| - |
| - if err := doc.Apply(ctx, trapping); err == nil { |
| - t.Fatal("expected the known trap reproducer to fail — if this now succeeds, it stopped " + |
| - "trapping and needs replacing (see core/tests/wasm_trap_containment.rs)") |
| - } |
| - |
| - // The instance must have been discarded and a fresh one rebuilt from |
| - // the update log — not merely "still answers something", but holding |
| - // exactly the content applied before the trap. |
| - got, err := doc.Text(ctx) |
| - if err != nil { |
| - t.Fatalf("Text after the trap: %v (a poisoned instance reused instead of discarded would fail here)", err) |
| - } |
| - if got != helloWant { |
| - t.Fatalf("after trap recovery got %q, want %q — the rebuilt instance lost pre-trap content", got, helloWant) |
| - } |
| - |
| - // And the rebuilt instance must be fully usable going forward, not |
| - // just readable: apply a further update and confirm it merges in. |
| - unicodeUpdate, unicodeWant := fixture(t, "unicode") |
| - doc2, err := rt.NewDoc(ctx, "go-host-test-guid-2", 9002) |
| - if err != nil { |
| - t.Fatalf("NewDoc for a second, unrelated document: %v", err) |
| - } |
| - defer doc2.Close(ctx) |
| - if err := doc2.Apply(ctx, unicodeUpdate); err != nil { |
| - t.Fatalf("applying to an unrelated document after another document's trap: %v", err) |
| - } |
| - got2, err := doc2.Text(ctx) |
| - if err != nil { |
| - t.Fatalf("Text on the unrelated document: %v", err) |
| - } |
| - if got2 != unicodeWant { |
| - t.Errorf("unrelated document got %q, want %q — a trap in one document's instance must not affect another's", got2, unicodeWant) |
| - } |
| -} |
| +// TestTrappedInstanceIsDiscardedAndRebuilds lives in core_internal_test.go |
| +// (package core), not here. Fix-round-1 finding 1: the trap this test uses |
| +// fires inside Update::decode_v1 before any RefCell is borrowed, so a host |
| +// that simply kept reusing the trapped instance would pass every |
| +// public-API-observable check (Text still succeeds, content still |
| +// correct) — the one assertion that can actually fail against a host that |
| +// skips the discard is a direct check that the module instance itself |
| +// changed, which needs unexported access this external test package |
| +// doesn't have. This file's tests only cover the other two cases: a clean |
| +// success (TestAppliesFixturesUnderWazero) and a clean rejection |
| +// (TestGarbageUpdateReturnsErrorNotTrap), both of which the public API is |
| +// enough to verify. |
diff --git a/internal/core/testdata/docs_core.wasm b/internal/core/testdata/docs_core.wasm| index db12991..d32f557 100755 |
| Binary files a/internal/core/testdata/docs_core.wasm and b/internal/core/testdata/docs_core.wasm differ |