Repository navigation
refactor(storage): poll async job bodies inside one writer job - #4244
Conversation
|
…b-transactions # Conflicts: # crates/tracedecay-rusqlite-runtime/src/exact_sql/tests/async_dispatch.rs
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let deadline = *self.deadline.lock().unwrap_or_else(PoisonError::into_inner); | ||
| if clock::now() >= deadline { | ||
| return Err(ExactSqlError::TransactionExpired); | ||
| } | ||
| verify_write_authority(self.authority, request.intent())?; | ||
| execute_request( | ||
| self.transaction, | ||
| request, | ||
| true, | ||
| Some(Arc::clone(self.shutdown_requested)), | ||
| Some(self.deadline), | ||
| true, | ||
| None, | ||
| ) | ||
| let result = self | ||
| .transaction | ||
| .execute(request, self.shutdown_requested, deadline); | ||
| if result.is_ok() && self.policy == JobPolicy::AuthorizedLongLease { | ||
| *self.deadline.lock().unwrap_or_else(PoisonError::into_inner) = | ||
| clock::now() + EXACT_SQL_TRANSACTION_LIMIT; |
There was a problem hiding this comment.
🔴 Parallel statements inherit stale leases
When a long-lease job runs statements concurrently, dispatch captures a deadline before waiting for the transaction mutex. A waiting statement retains the old deadline after another statement renews it, so an active job can expire.
Learn more
A job transaction is now Sync, so separate threads can call its statements while the writer transaction serializes SQLite access through JobConnection. dispatch snapshots the deadline before acquiring that mutex. A second statement waiting for the first can use a deadline that the first already renewed. Its pre-lock expiry check also does not cover the waiting time.
Example: With a deadline at second 120, statement A starts at second 100 and finishes at second 125, renewing the lease to second 245. Statement B starts at second 110 and waits for A; it executes with deadline 120 and can be interrupted, despite the renewed lease.
Recommended fix: Serialize the lease check, SQLite execution, and lease renewal under the same lock, or read the current deadline only after acquiring the transaction mutex. Cover two concurrent job statements crossing a lease boundary in a test.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if result.is_ok() && self.policy == JobPolicy::AuthorizedLongLease { | ||
| *self.deadline.lock().unwrap_or_else(PoisonError::into_inner) = | ||
| clock::now() + EXACT_SQL_TRANSACTION_LIMIT; |
There was a problem hiding this comment.
🔴 Expired jobs regain their lease
When a statement finishes after its lease expires, dispatch renews the deadline on success without checking expiry. The job can then commit work the interactive transaction would roll back.
Learn more
The execution guard checks a deadline through a SQLite progress callback, not unconditionally after a statement; see with_exact_sql_guard. A fast statement can finish after the lease expires, especially when authority verification or scheduling consumes the remaining time. The interactive dispatch checks expiry after execution before renewing its lease. This new long-lease renewal does not, and run_writer_job does not check expiry at commit.
Example: A job has one second left in its lease. Its authority check takes two seconds, then SELECT 1 succeeds without a progress callback. The job renews its lease and commits, rather than expiring.
Recommended fix: Check the captured deadline after each bounded statement before renewal and reject and roll back on expiry. Check expiry before committing even when the body issues no final statement.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub async fn transact_async<R, E, F>( | ||
| &self, | ||
| policy: JobPolicy, | ||
| body: F, | ||
| ) -> Result<Result<R, E>, ExactSqlError> | ||
| where | ||
| F: AsyncFn(&ExactSqlJobTransaction<'_>) -> Result<R, E> + Send + Sync + 'static, | ||
| R: Send + 'static, | ||
| E: Send + 'static, | ||
| { | ||
| self.run_job(policy, move |transaction| { | ||
| complete_now(body(transaction)).ok_or(ExactSqlError::JobAwaitedForeignFuture) | ||
| }) | ||
| .await |
There was a problem hiding this comment.
🟡 Async job API lacks a production caller
transact_async and the long-lease policy have only test callers. The repository requires production APIs to serve a real caller and user journey.
Learn more
The repository's engineering rules prohibit production ports or features without a real production caller and user journey. All current uses of transact_async and the new long-lease policy are test calls; production still uses transact and interactive transactions.
Example: A release containing this PR exposes transact_async, but no production Work or Workflow operation invokes it.
Recommended fix: Land the primitive with the first production migration, or include a production operation using it in this delivery slice.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Why
Step 5 of the storage redesign (docs/plans/storage/README.md) deletes interactive exact-SQL transactions: a write must be one job that cannot
.await, so it never holds the writer across a round trip. About 150 production sites still open interactive transactions, and they run roughly 500 async helpers written over the executor traits. Rewriting those helpers to sync code has no mechanical lever.This PR adds the primitive that lets those sites become jobs without that rewrite.
ExactSqlHandle::transact_asyncpolls an async body once, with a no-op waker, inside one writer job. The body's statements run synchronously, so a body that awaits only them completes on that poll. A body that awaits anything else is rolled back withExactSqlError::JobAwaitedForeignFuture. The design and its review are recorded in the README entry this PR adds.Scope
ExactSqlJobTransactionisSend + Sync. It holds the writer task's transaction in aMutexbehind a private trait object, which keeps the type covariant, so existing callers such asWorkSqliteJobStoragecompile unchanged. No newunsafe impl; theTaskTransactionsafety contract now rests on the transaction being the only path to its connection andConnection: Send.JobPolicy { Bounded, AuthorizedLongLease }andtransact_with_policy. A long-lease job needs attached write authority and renews its lease after each successful statement, as the interactive long lease does.ExactSqlJobTransaction::execute_batch.transactis unchanged for its callers.Stacked on #4242, which introduces
TaskTransaction.Verification
a_job_body_that_awaits_a_foreign_future_is_refused_and_rolled_back: the body inserts a row, then awaits a pool timer. Mutation: commit on that verdict; the test fails on the row count.an_async_job_body_over_several_statements_commits_and_returns_its_valueanda_long_lease_job_renews_its_lease_after_each_statement. Aconstassertion checks that the job transaction and a reference to it areSend + Sync.//crates/tracedecayunit_test (905), daemon_suite (77), mcp_suite (669) and session_suite (420) pass. Clippy and rustfmt_check_2024 pass on rusqlite-runtime.