Skip to content

Bound lock waits for human phrase alignment review decisions (#798, #800) - #864

Merged
WaylandYang merged 3 commits into
deeplethe:devfrom
rollroyces:fix/synchronous-handler-lock-budget
Sep 23, 2026
Merged

WaylandYang merged 3 commits into
deeplethe:devfrom
rollroyces:fix/synchronous-handler-lock-budget

Conversation

@rollroyces

Copy link
Copy Markdown
Contributor

/materials requests review requests — bounded lock waits (#798, #800)

Survey follow-up to #798's closing question — "every synchronous handler that takes a lock a background job can hold".

Handler Outcome
decide_alignment_kind_word Fixed in #828 by decide_and_apply_human. Closed.
decide_alignment_phrase This PR. Calls materialize inline at review_routes.rs:1280. The worker align_phrases holds the same advisory lock, and the wait has no cap. #800 was closed as completed when design landed on #841 — but the code fix never followed. This PR does that.
decide (generic review) Calls merge_entitiestemporal.rs per-timeline advisory locks. Real but invasive (multiple lock sites). Deferred.
decide_mapping, decide_defect, decide_violation, decide_pending, decide_proposal, apply_import Single-row UPDATE or no relevant lock; deferred.

What this PR does

  • materialize_human (utopia-store) — same body as materialize, but sets SET LOCAL lock_timeout = '2s' at the top of the transaction and maps PostgreSQL 55P03 to AppError::CodedConflict { code: "alignment_busy" } with a "please try again" message. Same pattern as #828 did for decide_and_apply_human.
  • materialize (worker path) — untouched. Workers don't have a spinner to look at; their waiting policy is unchanged.
  • decide_alignment_phrase — switched to materialize_human.

Tests

crates/utopia-store/tests/human_materialization_has_a_lock_budget.rs:

  • With the typed_materialize lock held by another connection, materialize_human returns CodedConflict("alignment_busy") well within 10 seconds, with a retry message. Once the gate is released, a worker materialize completes normally and produces the expected Outcome.**
  • With no contention, materialize_human matches materialize. SHOW lock_timeout returns empty after the call, confirming SET LOCAL did not leak to the session.

Validation

  • cargo check --package utopia-store clean
  • cargo check --package utopia-server clean
  • cargo clippy --package utopia-store --all-targets -- -D warnings clean
  • cargo clippy --package utopia-server --all-targets -- -D warnings clean
  • cargo fmt --check clean

Why this and not the bigger cut

decide / merge_entities would catch the same bug pattern at multiple temporal.rs lock sites. Doing it cleanly is a second PR's worth of test surface. The two materialize_human regressions above pin the phrase path; the merge path can land as a follow-up with its own survey note. If you'd rather do both at once, say so and I'll extend the cut — but I'd rather ship the smaller reviewable PR first.

rollroyces and others added 3 commits September 22, 2026 10:04
…he#798, deeplethe#800)

Survey of every synchronous handler that takes a lock a background job can hold
(asked in the second sentence of deeplethe#798):

  - decide_alignment_kind_word — fixed in deeplethe#828 by decide_and_apply_human. Closed.
  - decide_alignment_phrase    — calls materialize inline at
    review_routes.rs:1280. materialize takes pg_advisory_xact_lock on
    'typed_materialize' keyed by kb_id; align_phrases worker holds the same
    lock. Without a budget, a review click can pin a connection for minutes
    on a large base (deeplethe#800 measured 'first run 4.2545363s' at 500 statements,
    observed a third-acquire pool timeout on a two-connection pool).
    deeplethe#800 was closed as completed against design deeplethe#841; the code fix did not
    land. This PR lands it.
  - decide (generic review) — calls merge_entities → temporal.rs per-timeline
    advisory locks. Real but more invasive (multiple lock sites). Deferred to
    a follow-up; this PR is the minimum cut.
  - decide_mapping, decide_defect, decide_violation, decide_pending,
    decide_proposal, apply_import — single-row UPDATE or no relevant lock;
    lower priority, deferred.

This change:

  - Add materialize_human (utopia-store): same body as materialize, but sets
    SET LOCAL lock_timeout = '2s' at the top of the transaction and maps
    PostgreSQL 55P03 to AppError::CodedConflict { code: 'alignment_busy' }
    with a 'please try again' message. Worker path (materialize) is
    untouched — workers don't have a spinner.
  - Switch decide_alignment_phrase to call materialize_human.

Tests in crates/utopia-store/tests/human_materialization_has_a_lock_budget.rs:
  - With the typed_materialize lock held by another connection,
    materialize_human returns CodedConflict("alignment_busy") well within
    10 seconds, with a retry message. Worker materialize completes
    normally once the lock is released.
  - With no contention, materialize_human produces the same Outcome as
    materialize. SHOW lock_timeout returns empty after the call, confirming
    SET LOCAL did not leak to the session.

Signed-off-by: rollroyces <royce@rollroyces.com>
The Postgres session-level default for lock_timeout is '0' (wait forever),
not the empty string. SHOW lock_timeout returns Some('0') on a fresh
session — not Some(''). Comparing to '' failed in CI.

Capture the value before and after a second materialize_human call, and
assert they're equal. This pins the SET LOCAL to the transaction it
opened without depending on what the surrounding session happens to
default to.

Signed-off-by: rollroyces <royce@rollroyces.com>
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