Skip to content

refactor(git-index): make transaction store async, drop actor - #4140

Merged
ScriptedAlchemy merged 1 commit into
masterfrom
refactor/storage-git-index-async
Oct 16, 2026
Merged

ScriptedAlchemy merged 1 commit into
masterfrom
refactor/storage-git-index-async

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), third slice. This deletes the first of the three storage actor threads the design removes.

GitIndexTransactionStore was a synchronous trait. The daemon implemented it with a dedicated tracedecay-git-index-store thread: callers sent a StoreCommand (10 variants, capacity 64), and the thread settled each one against the async global-db store with native_runtime::block_on. The trait is now async, and DaemonGitIndexTransactionStore awaits the global-db store directly. The thread, its command protocol and its block_on path are deleted.

  • Preview-input GC. It moved from the actor thread to a NativeTask started with the store, on the same schedule. Open reads the next expiry, a save with an earlier expiry wakes the task, the task purges in batches of 256 and retries after 5 s on failure. A failed purge no longer fails the next unrelated command. Expired inputs still never read as available, because the read query checks expiry itself.
  • Invariants. Each mutation stays one IMMEDIATE transaction with the journal compare-and-swap inside it. The steps of one git transaction stay in order because each caller awaits them in turn. Mutations of one repository are still serialized by the repository mutation queue, which keeps its 64-entry cap and is now async.
  • Callers. GitIndexTransactionPort and GitIndexTransactionService (contracts) and the daemon's git handlers are async. Preview and apply still do synchronous Git work and call the configuration authority, which still uses block_on. So they run through native_runtime::on_blocking_pool, which polls a future on one blocking-pool thread wherever its caller runs. The removed code used spawn_blocking for the same reason.
  • DaemonGitIndexShutdownReceiptV1.store_actors_joined keeps its name for its reader in daemon shutdown. It now counts closed stores.

Test plan

  • store_awaited_on_a_native_worker_never_parks_it: open, save and read on a native worker complete, and neither native wait counter moves. Putting back the block_on settle in read_preview_input fails it.
  • failed_gc_retries_after_its_delay_without_failing_commands, restart_reconstructs_expired_cleanup_deadline, same_repository_mutations_never_enter_the_native_section_together and the shutdown test each fail under a targeted mutation: no retry, no deadline reload, four concurrent mutations, and a lease kept past shutdown.
  • on_blocking_pool_never_polls_on_a_tokio_caller_thread
  • //crates/tracedecay-code-index-runtime:unit_test (639 passed; the git_transactions filter passes 10 of 10 runs), //crates/tracedecay-daemon-service:unit_test, the global-db, store and contracts targets, //crates/tracedecay:unit_test --test_arg=git_, and runtime_acceptance_suite --test_arg=git_preview_and_apply_have_real_cli_mcp_runtime_parity
  • bazel build --config=clippy on each touched crate, and rustfmt_check_2024
  • //crates/tracedecay:daemon_suite, :mcp_suite, :session_suite in CI

Refs #4052


Devin Review

@changeset-bot

changeset-bot Bot commented Oct 16, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 9c7b351

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.

Devin Review

Comment on lines +54 to +61
fn close(&self) -> bool {
self.gc_task.abort();
self.shared
.database
.lock()
.unwrap_or_else(PoisonError::into_inner)
.take()
.is_some()

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.

🔴 Store shutdown leaves cleanup writes running

When preview-input GC is writing, shutdown returns before its aborted task and accepted SQL work finish. Project retirement can then find the session database still leased and fail to remove its shard.

Learn more

A store's background GC task borrows a database lease while purging expired preview inputs. StoreInner::close only requests abort and immediately reports that it closed the store. A purge already admitted to the SQL runtime can outlive that abort; the database retirement reservation requires all clients to finish before removing the shard. remove_remote_deleted_project attempts that reservation after Git store retirement. The previous actor shutdown joined its worker before returning.

Example: A purge starts writing at the same time a project deletion retires its Git store. Shutdown reports success while the write still holds its database client; the destructive reservation refuses deletion, although Git retirement succeeded.

Recommended fix: Make store shutdown asynchronous and await GC termination and any accepted purge settlement before reporting closure. Keep the database lease until that join completes, and propagate failures to the retirement caller.

Devin Review


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

@ScriptedAlchemy
ScriptedAlchemy merged commit 900ccf2 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