Repository navigation
UN-4513 [FIX] Stop execution status updates from blocking file-execution inserts - #2376
muhammad-ali-e wants to merge 6 commits into
Conversation
update_status locked the workflow_execution row twice with FOR UPDATE (once in the view, again inside update_execution) and counted the file rows while holding it. FOR UPDATE conflicts with the FOR KEY SHARE every child-row INSERT takes for its foreign-key check, so file-execution inserts for the same execution queued behind each status write (staging waits up to 8 min). - Lock once: the view hands its locked read to update_execution(locked_row=), and the reaper's recovery does the same. - Lock with FOR NO KEY UPDATE: status writers still exclude each other, child inserts no longer wait. - Count the file rows for the counter floor in one query before the lock. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YNhfqAovVwDvPZDYrPyE3
 [Medium impact] The PR appears safe to merge; no new actionable issues were found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Counting before the lock let a status write that waited behind another writer (e.g. the reaper cascading files to ERROR) floor its counters at a stale count and save failed_files=0. Count after the lock again (still one aggregate query), and run it in a savepoint so a failed count no longer aborts the status transaction. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YNhfqAovVwDvPZDYrPyE3
|
|
muhammad-ali-e
changed the title
… count plumbing - WorkflowExecution.STATUS_LOCK_FIELDS + status_lock_snapshot() replace the hand-built locked_row dicts, so callers and update_execution's own locked read cannot drift. - The reaper no longer passes file_row_counts; the floor recounts as on main. - Test documents that an uncommitted file insert holds no lock on the execution row (deferred FK), under either lock mode. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YNhfqAovVwDvPZDYrPyE3
… main - Drop the savepoint around the file-row count: a failed count aborts the transaction and the request is retried, instead of committing counters the floor never checked (failed_files too low reads as success). - Replace locked_row dicts with a row_locked flag that reads the locked instance's own fields, so a mismatched snapshot cannot be passed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YNhfqAovVwDvPZDYrPyE3
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YNhfqAovVwDvPZDYrPyE3
athul-rs
left a comment
athul-rs
left a comment
There was a problem hiding this comment.
Verdict: APPROVE WITH NITS: Critical 0 · High 0 · Medium 1 · Low 4 · Lenses run: 19/19 (examined 15, N/A 4)
The lock change holds up. FOR NO KEY UPDATE still conflicts with itself, with FOR UPDATE, with plain UPDATEs (including update_execution_queue_message_id) and with DELETE, so every status writer still serializes. Both row_locked=True call sites pass an instance re-read under the lock and not mutated before the call. Every other update_execution() caller keeps the default and re-reads under lock, and the Celery path takes one lock as before.
Security deep-dive: done, no findings at confidence >= 8. Pinned ruff v0.3.4 check and format --check pass on the 3 files. The test suite was not run here (needs Postgres).
Lens checklist
- Spec & intent: Clean
- Architectural fit & precedent: See findings #2
- Correctness & edge cases: See findings #3
- Security: Clean
- Data integrity & migrations: Clean
- Concurrency: Clean
- API & contract compatibility: Clean
- Reliability & resilience: Clean
- Performance & cost: Clean
- Observability: Clean
- Operational safety: Clean
- LLM/agent-specific: N/A (no model or agent code)
- Testing: See findings #1, #4
- Dependencies & build: N/A (no dependency change)
- Code quality: Clean
- Doc & comment accuracy: See findings #2, #3, #5
- Cross-document rule consistency: Clean
- On-prem deployment experience: N/A (no chart, values or on-prem docs)
- Scaling & workload lifecycle: N/A (no worker or pool config)
Assumptions: READ COMMITTED isolation; every FK into workflow_execution was created by Django migrations, so it is DEFERRABLE INITIALLY DEFERRED.
Reviewed with the unstract:review-deep rubric (plugin v2.1.73).
| ) | ||
| try: | ||
| st.join(1.0) | ||
| assert st.is_alive(), f"callback did not wait for the row lock: {sbox}" |
There was a problem hiding this comment.
#1 [Medium] [Lens 13 — Testing] — "Waited for the lock" is proven by a 1 s wall-clock join
st.join(1.0); assert st.is_alive() is also true when the second thread has not reached its select_for_update yet (thread start, DB connect on a loaded CI runner). If it gets there only after release.set(), the holder has already committed: here a count-before-lock regression would read the committed ERROR rows and still pass, and at L451 a lost lock would pass because the straggler sees the committed COMPLETED either way. The failure direction is a false pass, so CI would never show it.
Suggested fix: before releasing, poll pg_stat_activity from the test connection (with a deadline) until a backend shows wait_event_type = 'Lock'. test_file_row_count_runs_once_and_after_the_lock already pins the ordering deterministically, so this tightens the threaded proof rather than filling a gap.
Unverified: does the waiter reliably reach the lock within 1 s on CI? Running the count-before-lock mutation under CPU stress would confirm.
There was a problem hiding this comment.
Agreed, fixed in 4ad9da4. Both sites now poll pg_stat_activity with a deadline until another backend shows wait_event_type = 'Lock', and only then release the holder. They also fail fast if the second thread finishes without ever waiting.
I also tried removing every select_for_update(no_key=True), and both tests still pass. Even then the second writer queues on the holder's own UPDATE row lock, and the DB-level guard still refuses the revert. So these two tests show that a waiter waits and then sees the committed state; they don't pin the explicit lock. As you said, test_file_row_count_runs_once_and_after_the_lock and the single-lock query tests pin that.
Generated by Claude Code
| ) | ||
| try: | ||
| st.join(1.0) | ||
| assert st.is_alive(), f"second writer did not wait for the row lock: {sbox}" |
There was a problem hiding this comment.
#1 (second site): same 1 s is_alive() proof as L388; a lost lock passes here if the second writer reaches the row only after release.
There was a problem hiding this comment.
| # inserts and log writes for this execution stop queueing behind a | ||
| # status update. Nothing here changes the primary key, which is the only | ||
| # thing FOR KEY SHARE protects. | ||
| if row_locked: |
There was a problem hiding this comment.
#2 [Low] [Lens 2 / Lens 16] — row_locked=True vouches for the instance; the comment above says the opposite
The block at L432-L439 still says "Lock the row and read the PERSISTED marker" and "Never trust the (possibly stale) in-memory self.queue_message_id", directly above a branch that does neither. The guard at L423 only checks connection.in_atomic_block, so a future caller passing row_locked=True on an unlocked snapshot inside atomic() passes silently and re-opens the stale-routing race that comment describes. Both current callers are correct; this is about the next copy of the pattern.
Suggested fix: smallest is one clause in the existing comment ("unless the caller passes row_locked=True; see the docstring"). Sturdier: take the locked values instead of a boolean (locked: dict | None = None), so the caller hands over what it read. The docstring's "read with select_for_update()" could also say "either mode", since every caller now uses no_key=True.
There was a problem hiding this comment.
I took the small fix in 4ad9da4. The comment now carves out row_locked=True and points to the docstring, and the docstring says "(either mode)".
I'd rather not switch to locked: dict | None. The caller would still build that dict from the instance it locked, so a caller holding an unlocked snapshot inside atomic() could pass a dict just as wrongly as a boolean. It would widen the signature without closing that gap. Neither form can check that the lock is actually held, short of re-querying it, and re-querying is the round-trip this flag removes. So the guard stays at "a transaction is open", with the docstring stating the contract.
Generated by Claude Code
| execution.save(update_fields=update_fields) | ||
|
|
||
| @staticmethod | ||
| def _count_file_rows(execution) -> "dict | None": |
There was a problem hiding this comment.
#3 [Low] [Lens 3 / Lens 16] — The broad except adds nothing on DB errors and skips the floor on any other error
A DB error is caught without a savepoint, so the transaction is already aborted and the next save() raises: the request does 500, but because of the aborted transaction, not this handler. Any non-DB exception returns None, skips the floor and commits the worker's counters with a 200, which is the under-counted failed_files the docstring says this prevents. Same shape as before this PR, so not a regression.
Suggested fix: drop the try/except and let errors reach the view's handler (same 500 for DB errors). test_count_failure_writes_counters_as_given would need updating. Also drop the two "as on main" clauses: after merge, "main" is this code.
There was a problem hiding this comment.
Agreed, fixed in 4ad9da4. I dropped the try/except, so any count error now fails the request and rolls back instead of committing unfloored counters with a 200. test_count_failure_writes_counters_as_given became test_count_error_fails_the_request_and_commits_nothing, which expects a 500 with nothing written. I also removed the "as on main" wording.
The reaper call site already wraps each execution in its own try/except that logs and returns "failed". An error there rolls back that one recovery, and the next sweep retries it.
Generated by Claude Code
| ] | ||
| for t, _ in runs: | ||
| t.join(WAIT_SECONDS) | ||
| for _, b in runs: |
There was a problem hiding this comment.
#4 [Low] [Lens 13 — Testing] — Racing-writers test does not assert its threads finished
t.join(WAIT_SECONDS) is never followed by assert not t.is_alive(). A hung thread has neither error nor result in its box, so assert "error" not in b passes and only the final status is checked. (The attempts test below fails on a hang via b["result"].) Suggest asserting not t.is_alive() after each join and treating both racing tests as soak checks.
There was a problem hiding this comment.
Fixed in 4ad9da4. Both racing tests now assert not t.is_alive() after each join.
Generated by Claude Code
| @@ -0,0 +1,505 @@ | |||
| """Row locking on the workflow-execution status write. | |||
|
|
|||
| ``update_status`` used to take ``SELECT ... FOR UPDATE`` on the execution row twice | |||
There was a problem hiding this comment.
#5 [Low] [Lens 16] — Test-file prose overstates two things
Here: "used to take SELECT ... FOR UPDATE on the execution row twice" was true only for PG-transport rows; the view locked only when queue_message_id is not None. Suggest "For PG-transport executions, update_status used to take it twice".
L30: "only a broken lock makes the threads wait at all" is not true of this file; two tests assert a thread waits behind a correct lock. Suggest keeping only "a hang must fail the test rather than the CI job".
There was a problem hiding this comment.
… fail the request Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YNhfqAovVwDvPZDYrPyE3
|
Unstract test resultsPer-group results
Critical paths
|
Requested by ali · project thread
What
update_status(internal API) now takes one row lock onworkflow_executioninstead of two, and takes it asFOR NO KEY UPDATEinstead ofFOR UPDATE. The reaper's_recover_one_stuck_pg_executiondoes the same.update_execution()gains a keyword-onlyrow_locked=Trueflag for callers that already locked this instance: it routes and guards on the instance's own locked fields instead of locking the row again.Why
select_for_update(), thenupdate_execution()locked it again.FOR UPDATEconflicts with theFOR KEY SHAREthat every child-row INSERT (file executions, logs) takes for its foreign-key check, so those inserts queued behind each status write.FOR KEY SHAREchecks per file, 5,385 s of total lock wait, and execution selects waiting up to 8 minutes.How
select_for_update(no_key=True).FOR NO KEY UPDATEstill conflicts with itself and withFOR UPDATE, so these still serialize exactly as before:_record_dispatch_handle's UPDATE;select_for_update()on the row.FOR KEY SHARE, which guards the primary key, and nothing here changes the primary key.row_locked=True. Celery path: the view takes no lock andupdate_execution()takes its own. One lock either way.row_locked=Trueraises outside a transaction, since a lock that has already been released proves nothing.failed_files=0, so it was reverted.DEFERRABLE INITIALLY DEFERRED, so an uncommitted file insert holds no lock on the execution row until it commits. Neither lock mode ever waited for an in-flight insert, so the change does not lose any "wait for inserters" barrier. A test pins this.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
main.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
backend/workflow_manager/execution/tests/test_update_status_locking.py(17 tests), run against a real Postgres 16:FOR NO KEY UPDATEand noFOR UPDATEon the PG path, the Celery path, a refused write, the reaper, and plainupdate_execution();row_lockedskips the second lock and is rejected outside a transaction.TransactionTestCase, real threads and connections):lock_timeout = 2scommits while a status write holds the lock. Onmainit fails withcanceling statement due to lock timeout ... FOR KEY SHARE, the staging symptom;FOR UPDATE;failed_filesat the committed error rows;test_pg_finalization_fixes.py(41) pass, and 432 tests pass acrossworkflow_manager,pg_queueandapi_v2. The 3 failures intest_destination_connector_postgres.pyfail identically onmain(the local DB has no schema for the connector).mainbehaviour were left out of scope: view routing on the unlockedget_object()snapshot, andincrement_attemptcomputed before the lock.🤖 Generated with Claude Code
https://claude.ai/code/session_015YNhfqAovVwDvPZDYrPyE3