Repository navigation
refactor(storage): run handoff grants as awaited writer jobs - #4133
Conversation
|
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| verify_write_authority(authority, ExactSqlWriteIntent::Commit)?; | ||
| let _span = tracing::trace_span!("rusqlite.commit").entered(); | ||
| transaction | ||
| .commit() |
There was a problem hiding this comment.
🔴 Expired writer jobs still commit
When a job runs past its lease after its last statement, run_writer_job still commits. ExactSqlJobTransaction::dispatch checks the deadline only before statements, leaving the commit unbounded.
Learn more
A writer job holds the sole SQLite writer for an ordinary 120-second transaction lease. The new job transaction checks its deadline only when dispatch starts a statement. An expensive job closure, or a statement that finishes after the deadline without hitting SQLite's progress handler, can return Ok after that lease. The commit then succeeds without another clock check. The interactive transaction expires and rolls back instead of committing past its lease.
Example: A job inserts one row, spends 121 seconds computing its return value, then returns Ok. The row commits even though its transaction lease expired during the computation.
Recommended fix: Check the job deadline immediately before commit and roll back with TransactionExpired when it elapsed. Keep the check after the job returns so elapsed time inside a final statement also counts.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Step 5 of the storage redesign (
docs/plans/storage/README.mdin #4118), first slice: the async journal shape, proven end to end on handoff-open grants.The handoff authority was the one Work-adjacent path that parked native threads.
HandoffOpenServiceis async and runs on a native worker, but it called the synchronousHandoffOpenAuthorityPort. Eachissueorconsumeopened an interactive transaction and waited for every statement's reply by parking the worker, and eachlistorresolveparked it on reader admission. Building the authority ran a schema probe the same way on every request.ExactSqlHandle::transact(rusqlite-runtime). It takes a job, anFn(&ExactSqlJobTransaction) -> Result<R, E>over owned inputs, and sends it to the writer thread as oneWriterCommand::Job. The writer runs begin IMMEDIATE, the job's statements, and then commit onOkor rollback onErr. The caller awaits a single reply. Each statement keeps the guard, authority check and deadline of an interactive transaction statement. The return value separates the transaction's own failure (outerErr) from the job's verdict (innerResult). Jobs areFnso the step 7 executor can rerun one after a fence interrupt.ExactSqlHandle::query_async. It runs reader admission and the query as one blocking-pool job, the same interim hop step 1 uses for typed reads. Step 7 replaces it.HandoffOpenAuthorityPortis async.issueandconsumeare writer jobs,listandresolveare awaited reads, and the build-time schema probe is awaited.handoff_open_storage()is async in runtime-core and global-db, and the daemon handler awaits it.The design's
WorkJournal::transactalso looks up an idempotency ledger by operation id first. That ledger is new schema on a store that has no migration path yet, so it ships withjournal.dbin step 8. The handoff grant keeps its existing idempotency (issued_request_id UNIQUEand the same-identity replay).Next slices of step 5, each its own PR: placement and the leaf Work ports; the three actor threads; the attempt chain together with run control and Workflow (entangled through
work_run_control.rs); then removingoff_native_threadfor Work and the native wait counters.Test plan
a_write_job_and_an_awaited_read_on_a_native_worker_park_no_native_thread: a job and an awaited read on a native worker leave both native wait counters unchanged. Makingtransactwait throughrecv_writer_replyfails it, and so does runningquery_asyncinline.a_write_job_commits_on_ok_and_rolls_back_on_err: a job that writes and then returnsErrleaves no row. Committing regardless of the verdict fails it.handoff_calls_awaited_on_a_native_worker_park_no_native_thread(rusqlite_suite) issues and opens a handoff throughHandoffOpenServiceon a native worker. Neither counter moves, and the grant row commits. Both mutations above fail it (writer waits left 2, reader waits left 2).bazel test --config=clippyfor rusqlite-runtimeunit_testandrusqlite_suite, contractsunit_testandcontracts_suite, runtime-core, global-db, and daemon-serviceunit_testrustfmt_check_2024for each touched crate//crates/tracedecay:daemon_suite,:mcp_suite,:session_suite,:unit_testin CIRefs #4052