fix: resolve race condition in compound trigger evaluation#7
Conversation
Fixes two race conditions in compound trigger evaluation: 1. **Never-firing race** (transactional): When two child triggers fire concurrently in separate transactions, each only sees its own uncommitted insert due to READ COMMITTED isolation. Neither sees enough firings to trigger the parent. Fix: Use PostgreSQL advisory locks to serialize concurrent evaluations for the same compound trigger. 2. **Double-firing race** (autocommit): When both transactions see all firings, both delete and both fire the parent. Fix: Use DELETE ... RETURNING to make clearing a claim operation. Only the worker that successfully deletes the expected firings proceeds; others bail out. Based on the fix in PrefectHQ/nebula#10716. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
prefect-combined-codex/src/prefect/server/events/triggers.py
Lines 25 to 27 in 8c360ef
Remove unused get_logger import per repo rules
After switching to logging.getLogger, get_logger is now unused, which violates the repo-root AGENTS.md rule “Unused Imports Must Be Automatically Removed” and will likely trip ruff/unused-import checks. Since this was introduced by the change, the import should be removed or the previous logger pattern restored.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if dialect.name == "postgresql": | ||
| # Use the trigger's UUID as the lock key | ||
| # pg_advisory_xact_lock takes a bigint, so we use the UUID's int representation | ||
| # truncated to fit (collision is extremely unlikely and benign) | ||
| lock_key = hash(str(trigger.id)) % (2**63) |
There was a problem hiding this comment.
The lock key uses hash(str(trigger.id)), but Python’s string hash is randomized per process, so different workers will compute different keys for the same trigger ID. In Postgres this means concurrent evaluations on separate processes won’t serialize on the same advisory lock, so the race condition can still occur. This is only a problem when multiple processes evaluate the same composite trigger concurrently, but that’s the case the lock is meant to fix. A stable key (e.g., trigger.id.int truncated to 63 bits) avoids this.
Useful? React with 👍 / 👎.
| import logging | ||
|
|
||
| logger: "logging.Logger" = get_logger(__name__) | ||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Repo-root AGENTS.md requires the pattern logger: "logging.Logger" = get_logger("module_name"). This change replaces it with logging.getLogger(__name__) and removes the type annotation, which violates the logging standard in this repo and undermines consistent logger configuration/type safety. Please revert to the prescribed get_logger + annotated pattern.
Useful? React with 👍 / 👎.
2 participants
Benchmark PR from agentic-review-benchmarks#7