fix(sqlite-native): restore kv error hook

[NathanFlurry]

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore(engine): publish engine bases in ci #4649
• chore: lockfile lefthook #4647
• fix(rivetkit): surface native sqlite kv errors to callers #4645
• fix(rivetkit): prevent sleep races during disconnect and db work #4644
• fix(rivetkit): isolate engine envoys and propagate startup failures #4643
• test(rivetkit): remove wasm sqlite-only fixtures and coverage #4642
• chore(sqlite-vfs): remove wasm sqlite packages and workspace wiring #4641
• chore(rivetkit): remove wasm sqlite runtime #4614
• fix(sqlite-native): restore native startup kv preload #4639
• fix(sqlite-native): delete metadata before chunk range #4638
• fix(sqlite-native): keep truncate cache coherent #4637
• fix(sqlite-vfs): use delete range for truncate cleanup #4636
• fix(sqlite-native): restore kv error hook #4635 👈 (View in Graphite)
• perf(sqlite-native): avoid cloning cached read chunks #4634
• perf(sqlite-native): gate kv operation labels #4633
• perf(sqlite-native): reuse read cache for partial writes #4632
• perf(sqlite-native): remove delete file existence probe #4631
• fix(sqlite-native): gate native read cache behind env var #4630
• fix(pegboard): isolate runner config dc lookup failures #4625 : 1 other dependent PR (#4626 )
• docs(pegboard): note runner config upsert split write risk #4624
• fix(pegboard): unpack global runner config keys correctly #4623
• fix(pegboard-gateway): enforce tunnel message state #4622
• fix(pegboard-runner): clear terminal tunnel routes #4621
• fix(pegboard): skip protocol version keys in runner pool backfill #4620
• fix(pegboard): create missing normal runner configs #4619
• fix(epoxy): reuse cached optimistic value #4618
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

PR Review: fix(sqlite-native): restore kv error hook

This PR adds KV error propagation through the native SQLite VFS stack, surfacing errors to JavaScript as user-friendly messages. The overall approach is sound and the main goal is achieved cleanly. A few issues worth addressing before merging.

---

Issues

1. Silent mutex poisoning in database.rs (minor inconsistency)

take_last_kv_error_inner uses .ok() on the mutex lock, silently returning None on poisoning:

fn take_last_kv_error_inner(&self) -> Option<String> {
    self.db
        .lock()
        .ok()  // poisoning silently becomes None
        .and_then(|guard| guard.as_ref().and_then(NativeDatabase::take_last_kv_error))
}

All mutex operations in vfs.rs consistently log a tracing::warn! when poisoned. This should follow the same pattern rather than silently discarding the poison.

---

2. wrapNativeStorageError always throws but callers omit throw (code smell)

try {
    const result = await nativeDb.exec(query);
    return mapRows(result.rows, result.columns);
} catch (error) {
    wrapNativeStorageError(nativeDb, error);  // no throw/return
}
// falls through to implicit undefined return

The function always throws, but because callers omit throw wrapNativeStorageError(...), linters and future readers may assume execution can continue past the catch block. The correct pattern is either:

• Rename to throwNativeStorageError and keep as-is, or
• Have callers write throw wrapNativeStorageError(...) and make the function return the Error instead of throwing

This is a readability issue in the exec path especially, where falling through would return undefined if the function ever stopped throwing.

---

3. report_kv_error stores the error before firing the hook (ordering concern)

fn report_kv_error(&self, err: SqliteKvError) -> String {
    let message = err.to_string();
    self.set_last_error(message.clone());  // stored first
    self.kv.on_error(&self.actor_id, &err); // then hook fired
    message
}

If a future on_error implementation reads last_error (e.g., for correlation), it would see the error already stored before the hook has run. Not an issue with the current EnvoyKv impl (which only logs), but worth a comment or reordering (fire hook first, then store).

---

4. kv_vfs_get_last_error uses clone_last_error without explanation

Using clone_last_error means the error persists in the field after SQLite reads it. This is correct if SQLite may call GetLastError multiple times for the same failure. A brief comment explaining the choice over take_last_error would help future maintainers.

---

5. Bug fix in kv_io_write is undescribed

// before
state.read_cache.get(chunk_key.as_slice()).cloned()

// after
state.read_cache.as_ref().and_then(|read_cache| read_cache.get(chunk_key.as_slice()).cloned())

This looks like a genuine fix (treating read_cache as Option), but it is not mentioned in the PR description and has no comment. A short note explaining why .as_ref().and_then() is needed would help reviewers confirm this was intentional.

---

Suggestions (non-blocking)

• No tests: The error propagation path (KV fails -> VFS stores error -> surfaced via JS takeLastKvError) is uncovered. Given the unsafe FFI involved, an integration test simulating a KV backend failure would meaningfully improve confidence.
• index.d.ts field ordering: notGlobal: boolean was added after metadata?: any. Required fields typically precede optional ones.

---

Positives

• The layered propagation design is clean: VFS stores error -> GetLastError exposes to SQLite -> takeLastKvError exposes to JS -> wrapNativeStorageError gives users an actionable message.
• Using { cause: error } in the JS Error constructor is the correct pattern.
• The c.abortSignal guidance in the error message is specific and actionable.
• clear_last_error on success correctly prevents stale errors from leaking across operations.
• The as_deref() fix in kv_io_write is a correct type coercion improvement.
• All mutex operations in vfs.rs handle poisoning consistently with tracing::warn!.
• sqlite_error_message is retrieved before sqlite3_close in open_database — correct ordering.

---

Generated with Claude Code