Skip to content

Native: queue transitions in process before they take a pool connection #1058

Closed

Description

@rmcdaniel

Delivered in Native3.3.0

Implemented by #1083; qualified and published at source 2a15179f29801eec9f2a2d4e6a9c5f93233f104e / index sha256:cb4b11bd6235a551cf51e7dc6342504fbbcef19959618b376fb5648ca00fc388. Acceptance and scope limits. General reconciled this delivered item while the Server agent is Off. No benchmark speedup is claimed.

Original requirement and evidence

External performance report, topic 7. The reported timings and proposed fixes have not been independently reproduced. The supplied patches are reference drafts for review and current-source qualification.

Problem

  • A transition waiting on the database lock holds one of the pool's four
    connections, so the pool's reads queue behind it.
  • On SQLite, the busy handler retries a held lock on a sleep schedule, so
    the lock sits idle between waiters.

Proposal

  • Transitions queue in arrival order on an in-process mutex before
    they take a connection.
  • The next one takes the database lock as soon as the last one releases
    it.
  • A wait as long as the lock timeouts carries on as the database's own
    wait.

With several Server processes, the database lock still orders all of them.
The queue only removes the waiting inside one process.

Reported benchmark evidence

Reporter-supplied cumulative measurements, not an isolated-effect or independently verified performance claim: 200 sequentially started workflows, five sequential 20ms activities or five local activities each, six PHP worker processes, mean of three repetitions, one machine, fresh SQLite/MySQL8.0/PostgreSQL16 databases with default durability. "Five" and "local" below mean last-close wall time in seconds; S/M/P means SQLite/MySQL/PostgreSQL. Preserve regressions and noise in the reported results.

five local
slowest backend 22.46 → 22.60 (noise) 6.28 → 5.47

Acceptance

  • Transitions waiting for the lock hold no pool connection, so pool reads
    proceed.
  • Arrival order is preserved, and no transition starves. A test with
    concurrent transitions checks the order.
  • Lock timeouts behave as before.

Dependencies

No new producer dependency is required for source investigation.

Related existing work: #325.

Draft patches and validation boundary

Current main was independently read as b8f4dd13623ce645de77ba5bd89ae8dd8eea2f7c. The supplied draft series targets 3d4d4afaf2b24aaa5443eac154e253b011924738; patches are cumulative reference material, not a current-main application or test result. Review and rebase on maintained source, preserve all existing tests and published compatibility, then establish causal controls and actual conformance before shipping. No patch was applied or executed during intake.

The following sanitized unified diffs identify proposed reference material:

  • 0006-t07-Queue-transitions-in-process-before-they-take-a-conn.patch — 3,509 bytes, SHA-256 3ed5763b1bb04f2fdc18967d5bc97ca30c788f10a21501d1a9dcbbf970178fd9.

Current implementation: PR #1083 merged as 6bdb2c90f98b3045696cbd844f58cd6762cd235d, tree 4799d05f0f6e8860d1bf6be2b5331c47a33d4ef7. Namespace siblings share one FIFO mutex before pool acquisition. The existing acquisition budget returns the existing typed PoolTimedOut refusal before DB::begin. Read snapshots bypass this queue.

Actual source/main acceptance and cleanup records62 successful PR checks/one declared skip, all48 original queue Shared jobs, the original queue Feature suite, and all five actual-main workflows passing on attempt1. Consumed SQLite, PostgreSQL16/17, MySQL8.0/8.4 and MariaDB10.11 controls cover reader capacity, arrival order, cancellation, a live holder beyond five seconds, no DB::begin after timeout, and failure/drop/rollback recovery. The exact merged remote head independently returns404 and the clean owned worktree/topic is removed.

Next owner/action: Server completes #1059 in draft PR #1084, then the user's ordered remaining P0s and release/published qualification. This issue remains open for that publication acceptance. No timing, installed or release result is claimed. The earlier #1073 exact54af main acquisition failures remain attributed separately; the maintained substantive successor has its own successful qualification.

Activity

  1. added
    kind:featureA public product capability or experience is requested
    priority:P0Immediate release or data-integrity risk
    repo:serverOwned by the standalone server repository
    on Nov 6, 2026
  2. rmcdaniel commented on Nov 6, 2026

    @rmcdaniel
    MemberAuthor

    Sanitized reference diff 0006-t07-Queue-transitions-in-process-before-they-take-a-conn.patch, part 1/1.

    This is an unverified, cumulative draft from the external report. Mail author/date/commit-message metadata was removed; the unified-diff hunks were retained. No patch was applied or executed, and no build/test or compatibility result is implied. The known gaps and acceptance criteria in this issue remain mandatory.

    Complete sanitized patch: 3,509 bytes, SHA-256 3ed5763b1bb04f2fdc18967d5bc97ca30c788f10a21501d1a9dcbbf970178fd9. This part: 3,509 bytes, SHA-256 3ed5763b1bb04f2fdc18967d5bc97ca30c788f10a21501d1a9dcbbf970178fd9. Parts concatenate in order; a part boundary is between complete file diffs.

    diff --git a/rust/src/runtime/store.rs b/rust/src/runtime/store.rs
    index 994b4538..9fa98f6e 100644
    --- a/rust/src/runtime/store.rs
    +++ b/rust/src/runtime/store.rs
    @@ -169,6 +169,8 @@ pub(super) struct Store<DB: Backend> {
         pub(super) repair_pass_at: Arc<std::sync::Mutex<Option<std::time::Instant>>>,
         /// When this server next retires expired poll rows and receipts.
         pub(super) sweeps: Arc<super::sweep_throttle::Sweeps>,
    +    /// This server's transitions in arrival order (`Store::begin`).
    +    pub(super) transitions: Arc<tokio::sync::Mutex<()>>,
     }
     /// The `closed_reason` a terminal history event gives its run, or `None` for
     /// an event that does not close the run.
    @@ -317,6 +319,7 @@ where
                 repair_codec: Arc::new(Semaphore::new(1)),
                 repair_pass_at: Arc::default(),
                 sweeps: Arc::default(),
    +            transitions: Arc::default(),
             }
         }
         pub(super) fn in_namespace(&self, namespace: &str) -> Self {
    @@ -336,6 +339,7 @@ where
                 repair_codec: self.repair_codec.clone(),
                 repair_pass_at: self.repair_pass_at.clone(),
                 sweeps: self.sweeps.clone(),
    +            transitions: self.transitions.clone(),
             }
         }
         pub(super) async fn database_live(&self) -> bool {
    @@ -377,9 +381,21 @@ where
             self.pool.close().await;
         }
         pub(super) async fn begin(&self) -> Result<Transaction<'static, DB>> {
    -        DB::begin(&self.pool)
    +        // This server's transitions queue here before they take a pool
    +        // connection and the database's lock: a transition waiting on the
    +        // database holds a connection the pool's reads then wait for, and
    +        // SQLite's busy handler retries a held lock on a sleep schedule, which
    +        // leaves the lock idle between waiting transitions. The next one in
    +        // line takes the lock as soon as the last one releases it. A wait as
    +        // long as the lock timeouts goes on as the database's own wait.
    +        let queued = tokio::time::timeout(std::time::Duration::from_secs(5), self.transitions.clone().lock_owned())
                 .await
    -            .map(|inner| Transaction::new(inner, self.history_budget))
    +            .ok();
    +        let mut transaction = DB::begin(&self.pool)
    +            .await
    +            .map(|inner| Transaction::new(inner, self.history_budget))?;
    +        transaction.queued = queued;
    +        Ok(transaction)
         }
         // Reads that change nothing use one consistent read-only snapshot instead
         // of the transition lock, so they neither wait for nor delay transitions.
    diff --git a/rust/src/runtime/transaction.rs b/rust/src/runtime/transaction.rs
    index 5ddb1c30..fd5a4c2d 100644
    --- a/rust/src/runtime/transaction.rs
    +++ b/rust/src/runtime/transaction.rs
    @@ -8,6 +8,9 @@ use std::{
     
     pub(super) struct Transaction<'a, DB: Backend> {
         inner: sqlx::Transaction<'a, DB>,
    +    /// This transition's place in the server's queue, held until `inner`
    +    /// has committed or rolled back (`Store::begin`).
    +    pub(super) queued: Option<tokio::sync::OwnedMutexGuard<()>>,
         summaries: BTreeSet<String>,
         pub(super) history_budget: super::history_budget::Config,
         /// A workflow task completion's history meter (Workflow #930).
    @@ -26,6 +29,7 @@ impl<'a, DB: Backend> Transaction<'a, DB> {
         ) -> Self {
             Self {
                 inner,
    +            queued: None,
                 summaries: BTreeSet::new(),
                 history_budget,
                 meter: None,
  3. rmcdaniel commented on Nov 6, 2026

    @rmcdaniel
    MemberAuthor

    Security static review of the attached reference diff found a blocking acceptance mismatch.

    Store::begin wraps transitions.lock_owned() in a fixed five-second timeout, discards timeout with .ok(), and then always calls DB::begin. If an earlier transition holds the queue for more than five seconds, the waiter proceeds with queued = None and takes a pool connection while still waiting on the database lock. Multiple waiters can do the same. This recreates the pool-pressure problem and abandons FIFO/no-starvation exactly under sustained contention; it can also add a database lock wait after the queue wait.

    Do not silently bypass the queue. Either retain the queue position through the backend-derived transaction deadline, or return a typed bounded refusal before acquiring a connection. Add a causal control with the holder exceeding five seconds that proves waiting transitions acquire no connection, remain FIFO, and cannot starve.

    This is a static finding only. The supplied patch was not applied or executed.

  4. rmcdaniel commented on Nov 6, 2026

    @rmcdaniel
    MemberAuthor

    Sanitized rebased reference for report topic 7: 0006-t07-Queue-transitions-in-process-before-they-take-a-conn.patch.

    The supplied revision targets Server b8f4dd13623ce645de77ba5bd89ae8dd8eea2f7c. It is cumulative external reference material, not independently qualified against today's maintained source. Mail headers and commit-message/application instructions were removed; the unified diff is retained. The reporter now claims every rebased commit builds and the tip passes SQLite/MySQL/PostgreSQL tests. Those are attributed claims, not a new maintainer test result. Existing acceptance criteria, security findings and completed implementation receipts remain authoritative. Earlier attached patches are retained as historical references.

    Sanitized patch: 3,401 bytes, SHA-256 5a00d720fec65c3c046f6da2cecef72724c0057b2c6cd63abdaa515e8c98bdb2. Original-base sanitized identity: 3ed5763b1bb04f2fdc18967d5bc97ca30c788f10a21501d1a9dcbbf970178fd9. No patch was applied or executed during this update.

    diff --git a/rust/src/runtime/store.rs b/rust/src/runtime/store.rs
    index e6660620..4d81e7dd 100644
    --- a/rust/src/runtime/store.rs
    +++ b/rust/src/runtime/store.rs
    @@ -173,6 +173,8 @@ pub(super) struct Store<DB: Backend> {
         pub(super) test_claim_at: Option<DateTime<Utc>>,
         /// When this server next retires expired poll rows and receipts.
         pub(super) sweeps: Arc<super::sweep_throttle::Sweeps>,
    +    /// This server's transitions in arrival order (`Store::begin`).
    +    pub(super) transitions: Arc<tokio::sync::Mutex<()>>,
     }
     /// The `closed_reason` a terminal history event gives its run, or `None` for
     /// an event that does not close the run.
    @@ -323,6 +325,7 @@ where
                 #[cfg(test)]
                 test_claim_at: None,
                 sweeps: Arc::default(),
    +            transitions: Arc::default(),
             }
         }
         pub(super) fn in_namespace(&self, namespace: &str) -> Self {
    @@ -344,6 +347,7 @@ where
                 #[cfg(test)]
                 test_claim_at: self.test_claim_at,
                 sweeps: self.sweeps.clone(),
    +            transitions: self.transitions.clone(),
             }
         }
         pub(super) async fn database_live(&self) -> bool {
    @@ -385,9 +389,21 @@ where
             self.pool.close().await;
         }
         pub(super) async fn begin(&self) -> Result<Transaction<'static, DB>> {
    -        DB::begin(&self.pool)
    +        // This server's transitions queue here before they take a pool
    +        // connection and the database's lock: a transition waiting on the
    +        // database holds a connection the pool's reads then wait for, and
    +        // SQLite's busy handler retries a held lock on a sleep schedule, which
    +        // leaves the lock idle between waiting transitions. The next one in
    +        // line takes the lock as soon as the last one releases it. A wait as
    +        // long as the lock timeouts goes on as the database's own wait.
    +        let queued = tokio::time::timeout(std::time::Duration::from_secs(5), self.transitions.clone().lock_owned())
                 .await
    -            .map(|inner| Transaction::new(inner, self.history_budget))
    +            .ok();
    +        let mut transaction = DB::begin(&self.pool)
    +            .await
    +            .map(|inner| Transaction::new(inner, self.history_budget))?;
    +        transaction.queued = queued;
    +        Ok(transaction)
         }
         // Reads that change nothing use one consistent read-only snapshot instead
         // of the transition lock, so they neither wait for nor delay transitions.
    diff --git a/rust/src/runtime/transaction.rs b/rust/src/runtime/transaction.rs
    index 5ddb1c30..fd5a4c2d 100644
    --- a/rust/src/runtime/transaction.rs
    +++ b/rust/src/runtime/transaction.rs
    @@ -8,6 +8,9 @@ use std::{
     
     pub(super) struct Transaction<'a, DB: Backend> {
         inner: sqlx::Transaction<'a, DB>,
    +    /// This transition's place in the server's queue, held until `inner`
    +    /// has committed or rolled back (`Store::begin`).
    +    pub(super) queued: Option<tokio::sync::OwnedMutexGuard<()>>,
         summaries: BTreeSet<String>,
         pub(super) history_budget: super::history_budget::Config,
         /// A workflow task completion's history meter (Workflow #930).
    @@ -26,6 +29,7 @@ impl<'a, DB: Backend> Transaction<'a, DB> {
         ) -> Self {
             Self {
                 inner,
    +            queued: None,
                 summaries: BTreeSet::new(),
                 history_budget,
                 meter: None,
  5. rmcdaniel commented on Nov 6, 2026

    @rmcdaniel
    MemberAuthor

    Security follow-up on the updated rebased reference 0006-t07-Queue-transitions-in-process-before-they-take-a-conn.patch (3,401 B / SHA-256 5a00d720fec65c3c046f6da2cecef72724c0057b2c6cd63abdaa515e8c98bdb2): the blocking finding in 6692710516 remains unchanged.

    The rebase adjusts surrounding maintained-source fields, but the operative code still wraps transitions.lock_owned() in a fixed five-second timeout, converts timeout to None with .ok(), and then unconditionally calls DB::begin. A holder exceeding five seconds therefore still lets each waiter abandon FIFO, acquire a pool connection, and wait on the backend lock. The patch still lacks the required >5-second causal control and typed refusal/retained-queue behavior.

    This is static review of the sanitized rebased diff only. No patch was applied or executed, and the reporter-attributed build/benchmark claims are not independently accepted.

  6. rmcdaniel commented on Nov 7, 2026

    @rmcdaniel
    MemberAuthor

    Implementation is now draftPR1083, exact dfb4278, on maintained54af. This is an independently authored implementation; the reference patch was not applied.

    The runtime's namespace clones share one FIFO mutex acquired before pool acquisition. Queue expiry returns the existing typed PoolTimedOut/transient storage-unavailable path before DB::begin, using the existing pool-acquisition budget. The transaction owns the guard through its lifetime, including explicit commit/rollback, while read snapshots bypass it. Backend lock algorithms and their timeout configuration are unchanged.

    Consumed shared controls warm the real four-slot pool, prove three simultaneous readers keep capacity behind queued namespace writers, poll later arrivals first, cancel a waiter, and require committed FIFO progress. A live holder exceeds five seconds while waiters refuse without reaching DB::begin or consuming reader connections. Failure/drop/rollback recovery checks guard release. The existing SQLite all-targets and PostgreSQL/MySQL-MariaDB explicit qualification selectors consume these methods.

    Normal exact-head qualification is active: native-debug-build and published-sdk-adapters have passed; the test/backend matrix and PHP suite are pending. No assertion, performance or published result is inferred from compilation. All previous1073 runs were terminal before this new train began, and its failed54af main archive-acquisition attempts remain attributed at6700994337. Server owns the new draft's review/checks/fixes and ordinary merge/release follow-through.

  7. rmcdaniel commented on Nov 8, 2026

    @rmcdaniel
    MemberAuthor

    Implemented by #1083 and delivered in Native3.3.0 at protected source 2a15179f29801eec9f2a2d4e6a9c5f93233f104e / index sha256:cb4b11bd6235a551cf51e7dc6342504fbbcef19959618b376fb5648ca00fc388. The accepted source head dfb4278d2db43ad65c26ab7c0404c845c50a9f76 independently still shows62successful checks and1intended skip. Its merge is an ancestor of the release. The later integrated source head 5f937ec8f385c71dc781875f2f38026b63277faa has a byte-identical rust/ tree to that release and passed the registered all-targets/ignored suites across the backend lanes.

    Transition admission queues before acquiring a pool connection. Registered SQLite, PostgreSQL16/17, MySQL8.0/8.4 and MariaDB10.11 controls preserve reader capacity, FIFO order after cancellation, starvation freedom, the existing five-second typed PoolTimedOut boundary and recovery after rollback/failure/drop.

    Release provenance, published SQLite corpus, four-cell MySQL Compose acceptance, default-channel acceptance and the release's three-backend startup/identity smokes establish publication follow-through. The issue's functional/source controls and release delivery are complete. Closing with no benchmark reproduction, isolated speedup or installed contention-benchmark claim. Earlier NONPASSs remain attributed. This does not close the remaining P0 work or #1099.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent:generalkind:featureA public product capability or experience is requestedpriority:P0Immediate release or data-integrity riskrepo:serverOwned by the standalone server repository

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions