Skip to content

Database: stop leaking query state between statements on a model - #436

Merged
papac merged 1 commit into
5.xfrom
refactor/code-base
Oct 19, 2026
Merged

papac merged 1 commit into
5.xfrom
refactor/code-base

Conversation

@papac

@papac papac commented Oct 19, 2026

Copy link
Copy Markdown
Member

Model::query() memoized ONE process-wide builder (Model::$builder) and handed it back for every query on the same model class, and QueryBuilder only reset its state on the success path. Whatever a failed statement, or a chain that threw while being built, left behind (bindings, where clause, the first() flag, order, locks, eager loads) was replayed into the next query on that model in the same process: a recovery UPDATE dying with HY093, a query for one scope ANDed with the previous scope's where, an UPDATE widened to another scope's rows by a stale OR, a get() returning one row instead of a Collection.

  • Model::query() builds a new builder on every call and caches only each class's default properties.
  • retrieve() and retrieveBy() chain on one builder.
  • get(), the aggregates, update(), delete() and incrementAction() reset the whole builder in a finally (resetState()), so a failure leaves nothing behind. increment()/decrement() never cleared their bindings before, even on success.
  • last() keeps its select, order, limit and locks across the count() it runs, as count() used to leave them.
  • Barry\Builder::get() resets the eager loads in a finally.
  • Model::writeRows() reads the insert id from the builder it inserted with, not from whichever model ran query() last.

BuilderStateIsolationTest covers each case on in-memory SQLite; ten of its fourteen tests fail without this change.

Model::query() memoized ONE process-wide builder (Model::$builder) and
handed it back for every query on the same model class, and QueryBuilder
only reset its state on the success path. Whatever a failed statement, or
a chain that threw while being built, left behind (bindings, where clause,
the first() flag, order, locks, eager loads) was replayed into the next
query on that model in the same process: a recovery UPDATE dying with
HY093, a query for one scope ANDed with the previous scope's where, an
UPDATE widened to another scope's rows by a stale OR, a get() returning
one row instead of a Collection.

- Model::query() builds a new builder on every call and caches only each
  class's default properties.
- retrieve() and retrieveBy() chain on one builder.
- get(), the aggregates, update(), delete() and incrementAction() reset
  the whole builder in a finally (resetState()), so a failure leaves
  nothing behind. increment()/decrement() never cleared their bindings
  before, even on success.
- last() keeps its select, order, limit and locks across the count() it
  runs, as count() used to leave them.
- Barry\Builder::get() resets the eager loads in a finally.
- Model::writeRows() reads the insert id from the builder it inserted
  with, not from whichever model ran query() last.

BuilderStateIsolationTest covers each case on in-memory SQLite; ten of
its fourteen tests fail without this change.
@papac
papac merged commit 193dabe into 5.x Oct 19, 2026
17 checks passed
@papac
papac deleted the refactor/code-base branch October 19, 2026 07:27
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