Skip to content

fix(sqlite): avoid nested runtime during vfs register#4894

Draft
NathanFlurry wants to merge 1 commit intomainfrom
driver-test-complaints/fix-vfs-register-block-on
Draft

fix(sqlite): avoid nested runtime during vfs register#4894
NathanFlurry wants to merge 1 commit intomainfrom
driver-test-complaints/fix-vfs-register-block-on

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

NathanFlurry commented May 3, 2026

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

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

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 3, 2026

PR Review: fix(sqlite): avoid nested runtime during vfs register

Overview

This PR fixes two related SQLite runtime issues:

  1. vfs.rsfetch_initial_main_page was calling Handle::block_on from within an async context, causing a panic ("cannot call block_on from within an async context"). Fixed by wrapping with block_in_place on multi-thread runtimes, and returning an explicit error on current-thread runtimes.

  2. connection_manager.rsNativeConnection and NativeVfsHandle drops were happening directly on the async executor thread, blocking it during SQLite I/O cleanup. Fixed by routing all drops through spawn_blocking.

Both fixes are correct and address real panics/blocking behavior.


Code Quality

Strengths:

  • The ReadAcquireAction enum cleanly replaces the previous Option<NativeVfsHandle> dual-purpose return, making control flow explicit.
  • Refactoring prune_expired_readers to return Vec<IdleReadConnection> instead of usize enables the caller to drop connections outside the mutex — the right approach.
  • drop_native_connections_blocking and drop_vfs_blocking are well-factored helpers with appropriate early-return guards.
  • The close() fix correctly includes idle_writer in the connections vec. The old code silently dropped the writer inside the lock block without routing it through spawn_blocking.

Issues and Suggestions

Minor: awkward Option::into_iter().collect() pattern

In two places:

drop_native_connections_blocking(idle_connection.into_iter().collect()).await;
drop_native_connections_blocking(close_connection.into_iter().collect()).await;

Both are Option<NativeConnection>. Works correctly but is verbose. Consider making the helper accept impl IntoIterator<Item = NativeConnection> or Option<NativeConnection> directly.

Potential: block_in_place error loses context

The CurrentThread early-return error is descriptive, but doesn't include the actor ID. Since this surfaces as a String propagated up through VFS registration, including the actor_id would help operators diagnose which actor triggered it.

Pre-existing gap: Drop still runs on async thread for leaked leases

If a NativeReadConnectionLease or NativeWriteConnectionLease is leaked via a panic without calling release(), the Drop impl only logs a warning and the NativeConnection drop runs on whatever thread unwinds — which may be the async executor. Pre-existing issue, but worth tracking as a follow-up now that the normal paths are fixed.


Missing Tests

No tests are added. A regression test covering the panic scenario (calling open_database_from_envoy from within a multi-thread async context and asserting no panic) would prevent regressions. Expected given the DRAFT state; noting for when the PR is ready.


Verdict

The core approach is sound. block_in_place in vfs.rs and spawn_blocking-based drop routing in connection_manager.rs are the correct Tokio patterns for these scenarios. Suggestions above are minor polish items for when the PR comes out of draft.

@NathanFlurry NathanFlurry force-pushed the driver-test-complaints/fix-vfs-register-block-on branch from dc5e609 to 215bb00 Compare May 3, 2026 00:54
@NathanFlurry NathanFlurry force-pushed the driver-test-complaints/repair-progress-note branch from 83ff825 to 14cc5db Compare May 3, 2026 00:54
@NathanFlurry NathanFlurry marked this pull request as ready for review May 3, 2026 03:57
@NathanFlurry NathanFlurry marked this pull request as draft May 3, 2026 07:19
@NathanFlurry NathanFlurry changed the base branch from driver-test-complaints/repair-progress-note to graphite-base/4894 May 3, 2026 07:34
@NathanFlurry NathanFlurry force-pushed the driver-test-complaints/fix-vfs-register-block-on branch from 215bb00 to c237935 Compare May 3, 2026 07:34
@NathanFlurry NathanFlurry force-pushed the graphite-base/4894 branch from 14cc5db to 5ffa0d0 Compare May 3, 2026 07:34
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4894 to 05-02-chore_driver-test-runner_add_native_wasm_runtime_matrix_to_skill May 3, 2026 07:34
This was referenced May 4, 2026
Base automatically changed from 05-02-chore_driver-test-runner_add_native_wasm_runtime_matrix_to_skill to main May 4, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant