Skip to content

refactor(storage): run handoff grants as awaited writer jobs - #4133

Merged
ScriptedAlchemy merged 1 commit into
masterfrom
refactor/storage-journal-jobs
Oct 16, 2026
Merged

ScriptedAlchemy merged 1 commit into
masterfrom
refactor/storage-journal-jobs

Conversation

@ScriptedAlchemy

@ScriptedAlchemy ScriptedAlchemy commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

Summary

Step 5 of the storage redesign (docs/plans/storage/README.md in #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. HandoffOpenService is async and runs on a native worker, but it called the synchronous HandoffOpenAuthorityPort. Each issue or consume opened an interactive transaction and waited for every statement's reply by parking the worker, and each list or resolve parked 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, an Fn(&ExactSqlJobTransaction) -> Result<R, E> over owned inputs, and sends it to the writer thread as one WriterCommand::Job. The writer runs begin IMMEDIATE, the job's statements, and then commit on Ok or rollback on Err. 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 (outer Err) from the job's verdict (inner Result). Jobs are Fn so 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.
  • HandoffOpenAuthorityPort is async. issue and consume are writer jobs, list and resolve are 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::transact also 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 with journal.db in step 8. The handoff grant keeps its existing idempotency (issued_request_id UNIQUE and 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 removing off_native_thread for 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. Making transact wait through recv_writer_reply fails it, and so does running query_async inline.
  • a_write_job_commits_on_ok_and_rolls_back_on_err: a job that writes and then returns Err leaves 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 through HandoffOpenService on a native worker. Neither counter moves, and the grant row commits. Both mutations above fail it (writer waits left 2, reader waits left 2).
  • The existing handoff storage and contract suites pass on the async port.
  • bazel test --config=clippy for rusqlite-runtime unit_test and rusqlite_suite, contracts unit_test and contracts_suite, runtime-core, global-db, and daemon-service unit_test
  • rustfmt_check_2024 for each touched crate
  • //crates/tracedecay:daemon_suite, :mcp_suite, :session_suite, :unit_test in CI

Refs #4052


Devin Review

@changeset-bot

changeset-bot Bot commented Oct 16, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 63117a4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment on lines +142 to +145
verify_write_authority(authority, ExactSqlWriteIntent::Commit)?;
let _span = tracing::trace_span!("rusqlite.commit").entered();
transaction
.commit()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

@ScriptedAlchemy
ScriptedAlchemy merged commit 0dcb306 into master Oct 16, 2026
11 of 20 checks passed
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