fix(envoy-client): fail sent vfs sqlite requests on disconnect instead of stalling until expiry

[MasterPtato]

No description provided.

[MasterPtato]

Stack for rivet-dev/actors

• #5670 fix(rivetkit): surface native runtime load failures instead …
• #5667 fix(envoy-client): fail sent vfs sqlite requests on disconne … 👈
• #5666 fix(rivetkit-core): report actor crashes as a sleep intent i …
• #5668 fix(envoy-client): ack start commands immediately instead of …
• main

Get stack: forklift get 5667
Push local edits: forklift submit
Merge when ready: forklift merge 5667

_{change snwrsykw}

[claude]

Review

Solid, well-targeted fix. fail_sent_sqlite_requests_with_indeterminate_result closes a real gap: ConnClose previously only failed sent remote requests via fail_sent_remote_sqlite_requests_with_indeterminate_result, leaving sent VFS get_pages/commit requests parked until the 30s KV_EXPIRE_MS cleanup tick, which stalls the SQLite worker thread for the whole window. The new function mirrors the existing remote variant almost exactly (filter by sent, remove, dec metric, warn, resolve oneshot), which keeps the two paths consistent and matches the "sent requests fail on disconnect, unsent requests resend on reconnect" invariant described in docs-internal/engine/sqlite-vfs.md/CLAUDE.md. The commit-is-ambiguous caveat is correctly called out and delegated to the head-fence check rather than silently papered over.

Issues

1. Stale doc comment on sent_vfs_request_fails_immediately_on_disconnect (engine/sdks/rust/envoy-client/tests/sqlite_vfs_disconnect_stall.rs, the /// block directly above the test, roughly lines 191-204). The module-level //! header at the top of the file was rewritten to describe the fix, but this function-level doc block was left as-is from the old bug-repro test. It still says step 5 "the VFS get_pages oneshot is STILL pending" and step 6 "we backdate its timestamp by KV_EXPIRE_MS + 1ms and call cleanup_old_sqlite_requests... resolves with the sqlite request timed out error" — none of which the test body does anymore (it now asserts immediate resolution with RemoteSqliteIndeterminateResultError right after the disconnect handlers run, and the final cleanup_old_sqlite_requests call is just there to prove the map is already empty). This directly contradicts the surrounding accurate module doc and will mislead the next reader. Worth updating to match the new step-by-step behavior.

2. Minor: reused error type/message says "remote" for the local VFS path (engine/sdks/rust/envoy-client/src/sqlite.rs:545-547, utils.rs:37-49). RemoteSqliteIndeterminateResultError's Display impl hard-codes "remote sqlite {operation} result is indeterminate after envoy disconnect". It's now also used for local VFS requests (get_pages, commit, etc.), so a failed get_pages will log/return a message calling itself "remote sqlite get_pages" even though get_pages isn't part of the RemoteSqliteRequest enum — it's the VFS-local path. Not a functional bug (nothing currently pattern-matches on the message text, only on the type via downcast_ref), but it's a naming mismatch that could confuse anyone reading logs or the returned error. Consider a wording tweak (e.g. "sqlite {operation} result is indeterminate...") or a short doc comment clarifying the type is intentionally shared across both request kinds.

3. Negative-control test doesn't call the new function (unsent_vfs_request_quickly_resubmitted_after_reconnect, same file). It only invokes fail_sent_remote_sqlite_requests_with_indeterminate_result, not fail_sent_sqlite_requests_with_indeterminate_result, even though production ConnClose now runs both. Since the new function also filters on sent, this doesn't change the test's outcome, but the test's comment claims it's "run[ning] the same disconnect handler" that ConnClose runs, which is now only half true. Consider adding the second call for full parity with the production code path.

Other notes

• envoy.rs wiring is minimal and correct — the new call is added right after the existing remote-fail call in ConnClose, before handle_conn_close/lost-timeout logic, so ordering looks right.
• Metrics handling (sqlite_requests_inflight.dec()) and the sent-only filter (leaving unsent requests for process_unsent_sqlite_requests to resend) both look correct and match the invariant in CLAUDE.md's SQLite Package section.
• No test coverage regression; the rewritten test is arguably clearer than the old repro once the stale doc block above is fixed.

Overall: functionally correct and appropriately scoped fix with good test coverage; the main actionable item is the stale test doc-comment (#1).

Re-reviewed against the current diff (commit 3adb1e9b1d9235ece479a189fc73e1b9ce3dcfbc, unchanged since the prior pass); findings above still stand.