Skip to content

feat: add lock(value) to the query builder for SKIP LOCKED claims - #346

Open
claude-tmgbedu wants to merge 2 commits into
mainfrom
task/lock-value-skip-locked
Open

claude-tmgbedu wants to merge 2 commits into
mainfrom
task/lock-value-skip-locked

Conversation

@claude-tmgbedu

Copy link
Copy Markdown

Part 2 of task #3864 (task #3876). Follows #344 (ADR 061) and #345 (ADR 062), both released in v0.74.0.

What

  • QueryBuilder.lock(value=True), as in Laravel. True takes the update lock, False the shared lock, and a string is a trusted, code-literal lock clause rendered verbatim, such as FOR UPDATE SKIP LOCKED or MSSQL WITH(ROWLOCK, UPDLOCK, READPAST). SQLite renders no lock.
  • lock_for_update() and shared_lock() delegate to it, and Model.lock(value) proxies it.
  • BaseGrammar.process_locks() renders an unknown lock string verbatim. SQLiteGrammar.process_locks() renders nothing.
  • ADR 063, lock(value) on the query builder, with its index row after 061 and 062.
  • The stale _compile_update docstring is removed (a fix: compile update() against the updated table #345 review follow-up).

The lock string is SQL, not a binding. ADR 063 requires it to be a literal in application code, never user input.

Not included

Tests

  • tests/masoniteorm/query/grammars/test_lock_value_grammar.py snapshots SQLite, MySQL, Postgres and MSSQL. It covers lock()/lock(True)/lock(False), a verbatim string after ORDER BY/LIMIT (after the table with TOP on MSSQL), a locked where_in subquery, and the last call winning.
  • tests/masoniteorm/postgres/test_postgres_skip_locked.py runs real concurrent transactions:
    • Three workers each read up to 2 pending rows with lock("FOR UPDATE SKIP LOCKED"), update each by id and hold their transaction. They claim 2, 2 and 1 of 5 rows, and no row twice.
    • A plain lock() worker waits for the holder.
    • New: a single builder-built UPDATE ... WHERE "id" = (SELECT "id" ... LIMIT 1 FOR UPDATE SKIP LOCKED) claims a free row while another transaction holds one. Mutation check: with a plain lock() subquery the update blocks and the test fails.

Locally, with a postgres:17 container matching docker-compose (POSTGRES_PORT=15433), the full suite gives 3830 passed, 10 skipped and 380 subtests passed, with coverage at 93.21%. Ruff check and format are clean, and Basedpyright reports 0 errors on the touched modules.

The branch is commit 76bccc5 (the approved lock(value) design) rebased onto v0.74.0, with the ADR index rows 061, 062 and 063 kept in order, plus one follow-up commit.

Not merged or released; the user merges and releases framework PRs.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VCMGYLLSTz3VTHCp6XygtL

tmgbedu and others added 2 commits November 1, 2026 13:42
Why:
- Concurrent workers claiming pending rows waited on each other under FOR UPDATE.
- The builder could not render FOR UPDATE SKIP LOCKED, so claims fell back to raw SQL.

How:
- QueryBuilder.lock(value=True) follows Laravel: True is the update lock, False the shared lock, a string is the dialect clause rendered verbatim.
- lock_for_update() and shared_lock() delegate to it; Model.lock proxies it.
- process_locks() renders unknown lock strings verbatim; SQLite renders no lock.
- Grammar snapshots on four dialects and a Postgres test where concurrent fetch-then-update claims take disjoint rows.
- ADR 063 and its index row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCMGYMYVHdzHAFaDeRWDyD
Why:
- A claim can also be a single builder-built UPDATE ... WHERE id = (SELECT ... FOR UPDATE SKIP LOCKED), with no RETURNING and no raw SQL.
- _compile_update kept a docstring after #345 touched it, against the no-docstring rule.

How:
- A Postgres test holds one row in another transaction and asserts that update() through a locked where subquery claims exactly one other row; with a plain lock() subquery the update blocks and the test fails.
- Remove the _compile_update docstring.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCMGYLLSTz3VTHCp6XygtL
@codecov

codecov Bot commented Nov 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...t/src/fastapi_startkit/masoniteorm/models/model.py 66.66% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@claude-tmgbedu

Copy link
Copy Markdown
Author

Strict review (task #3876): APPROVE at head ec3f0f182ac7729d57a335a9c775959feafb741e

Branch

  • Two commits on v0.74.0 (908dd35b, which is main), with no commits behind.
  • 09cf4faf matches 76bccc5c except for the context of v0.74.0. The ADR index keeps 061, 062 and 063 in order.
  • The stale _compile_update docstring is removed, which follows the AGENTS.md rule against docstrings.

Against AGENTS.md

  • ADR first: ADR 063 covers the problem, the alternatives (a skip_locked flag, UPDATE … RETURNING, Laravel lock($value)), the decision and the validation. Its index row is in docs/adr/AGENTS.md.
  • No comments or docstrings in the source or the tests.
  • No RETURNING and no raw SQL anywhere in the tests. The claim test builds UPDATE … WHERE "id" = (SELECT … FOR UPDATE SKIP LOCKED) through the builder alone.

Correctness

  • lock(value):
    • True sets "update", False sets "share", and a string is stored as given.
    • lock_for_update() and shared_lock() delegate to it, so the 061 behaviour is unchanged. The ADR 061 tests pass.
    • Model.lock proxies it.
  • process_locks(): locks.get(lock, lock or "") maps "update"/"share" to the dialect clause and renders any other string verbatim. False still renders "".
  • SQLiteGrammar.process_locks() returns "", so a dialect string can't break SQLite test runs.
  • The four-dialect snapshots match each grammar's format string.
    • MSSQL puts {lock} after {table} and TOP before the columns.
    • MySQL and Postgres put the lock after LIMIT.
    • The bindings stay ["pending"], and the locked where_in subquery keeps its lock on every dialect.

Lock-string injection surface

  • The string is concatenated into SQL unescaped, which is the same trust model as select_raw.
  • ADR 063 states the rule explicitly: a code literal only, never request input, user-changeable config or any other untrusted value.
  • No API in the framework passes request data to lock(). I accept this as designed and as it is in Laravel.

Verification (postgres:17 throwaway container, POSTGRES_PORT)

  • Postgres and grammar tests: 147 passed, 333 subtests.
  • Full suite with coverage: 3830 passed, 10 skipped and 380 subtests, above the coverage threshold. This matches the PR description.
  • Mutation check: I swapped the subquery's lock("FOR UPDATE SKIP LOCKED") for lock() in a copy of the test. The subquery claim test then fails, so it really proves SKIP LOCKED.
  • ruff check is clean, and ruff format --check is clean on the touched files.

Non-blocking

  1. The MySQL subquery snapshot. It renders IN (SELECT … LIMIT 1 FOR UPDATE SKIP LOCKED), but MySQL rejects LIMIT inside an IN subquery (error 1235). The grammar is right, but the pattern can't run on MySQL. Add one line to ADR 063 so nobody copies it as a MySQL claim recipe. The where("id", subquery) form the Postgres test uses has the same caveat on MySQL, because of error 1093 (the target table appears in the subquery).
  2. ADR 063's Validation list doesn't mention the new test_one_update_through_a_skip_locked_subquery_claims_a_free_row.
  3. Optional hardening. lock() could reject a string containing ; or --. This is cheap defence in depth, and it doesn't restrict the dialect clauses.

Not merged or released by me; the user merges framework PRs.

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.

2 participants