-
Notifications
You must be signed in to change notification settings - Fork 0
fix: resolve race condition in compound trigger evaluation #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: coderabbit_full_base_fix_resolve_race_condition_in_compound_trigger_evaluation_pr7
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,12 +7,46 @@ | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| from prefect.server.database import PrefectDBInterface, db_injector | ||||||||||||||||||||||||||||||||||||||
| from prefect.server.events.schemas.automations import CompositeTrigger, Firing | ||||||||||||||||||||||||||||||||||||||
| from prefect.server.utilities.database import get_dialect | ||||||||||||||||||||||||||||||||||||||
| from prefect.types._datetime import DateTime, now | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if TYPE_CHECKING: | ||||||||||||||||||||||||||||||||||||||
| from prefect.server.database.orm_models import ORMCompositeTriggerChildFiring | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| async def acquire_composite_trigger_lock( | ||||||||||||||||||||||||||||||||||||||
| session: AsyncSession, | ||||||||||||||||||||||||||||||||||||||
| trigger: CompositeTrigger, | ||||||||||||||||||||||||||||||||||||||
| ) -> None: | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
| Acquire a transaction-scoped advisory lock for the given composite trigger. | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| This serializes concurrent child trigger evaluations for the same compound | ||||||||||||||||||||||||||||||||||||||
| trigger, preventing a race condition where multiple transactions each see | ||||||||||||||||||||||||||||||||||||||
| only their own child firing and neither fires the parent. | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| The lock is automatically released when the transaction commits or rolls back. | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
| bind = session.get_bind() | ||||||||||||||||||||||||||||||||||||||
| if bind is None: | ||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # Get the engine from either an Engine or Connection | ||||||||||||||||||||||||||||||||||||||
| engine: sa.Engine = bind if isinstance(bind, sa.Engine) else bind.engine # type: ignore[union-attr] | ||||||||||||||||||||||||||||||||||||||
| dialect = get_dialect(engine) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||
| await session.execute( | ||||||||||||||||||||||||||||||||||||||
| sa.text("SELECT pg_advisory_xact_lock(:key)"), {"key": lock_key} | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
| # SQLite doesn't support advisory locks, but SQLite also serializes writes | ||||||||||||||||||||||||||||||||||||||
| # at the database level, so the race condition is less likely to occur | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| @db_injector | ||||||||||||||||||||||||||||||||||||||
| async def upsert_child_firing( | ||||||||||||||||||||||||||||||||||||||
| db: PrefectDBInterface, | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -102,11 +136,22 @@ async def clear_child_firings( | |||||||||||||||||||||||||||||||||||||
| session: AsyncSession, | ||||||||||||||||||||||||||||||||||||||
| trigger: CompositeTrigger, | ||||||||||||||||||||||||||||||||||||||
| firing_ids: Sequence[UUID], | ||||||||||||||||||||||||||||||||||||||
| ) -> None: | ||||||||||||||||||||||||||||||||||||||
| await session.execute( | ||||||||||||||||||||||||||||||||||||||
| sa.delete(db.CompositeTriggerChildFiring).filter( | ||||||||||||||||||||||||||||||||||||||
| ) -> set[UUID]: | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
| Delete the specified child firings and return the IDs that were actually deleted. | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Returns the set of child_firing_ids that were successfully deleted. Callers can | ||||||||||||||||||||||||||||||||||||||
| compare this to the expected firing_ids to detect races and avoid double-firing | ||||||||||||||||||||||||||||||||||||||
| composite triggers. | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
| result = await session.execute( | ||||||||||||||||||||||||||||||||||||||
| sa.delete(db.CompositeTriggerChildFiring) | ||||||||||||||||||||||||||||||||||||||
| .filter( | ||||||||||||||||||||||||||||||||||||||
| db.CompositeTriggerChildFiring.automation_id == trigger.automation.id, | ||||||||||||||||||||||||||||||||||||||
| db.CompositeTriggerChildFiring.parent_trigger_id == trigger.id, | ||||||||||||||||||||||||||||||||||||||
| db.CompositeTriggerChildFiring.child_firing_id.in_(firing_ids), | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
| .returning(db.CompositeTriggerChildFiring.child_trigger_id) | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+147
to
155
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# First, read the specific file and lines mentioned
echo "=== Reading the file at lines 147-155 ==="
sed -n '140,160p' src/prefect/server/events/models/composite_trigger_child_firing.py
echo -e "\n=== Full file to understand context ==="
wc -l src/prefect/server/events/models/composite_trigger_child_firing.pyRepository: qodo-benchmark/prefect-combined-coderabbit Length of output: 996 🏁 Script executed: #!/bin/bash
# Look at the ORM model definition
echo "=== Finding CompositeTriggerChildFiring model ==="
rg -n "class CompositeTriggerChildFiring" -A 20 src/Repository: qodo-benchmark/prefect-combined-coderabbit Length of output: 1771 🏁 Script executed: #!/bin/bash
# Search for the function containing lines 147-155 to understand docstring and intent
echo "=== Finding the function context ==="
sed -n '130,175p' src/prefect/server/events/models/composite_trigger_child_firing.pyRepository: qodo-benchmark/prefect-combined-coderabbit Length of output: 1065 🏁 Script executed: #!/bin/bash
# Search for callers of this function to see what they expect
echo "=== Finding callers of the function ==="
rg -n "\.child_firing_id\|\.child_trigger_id" src/prefect/server/events/ -A 2 -B 2Repository: qodo-benchmark/prefect-combined-coderabbit Length of output: 125 Return deleted child_firing_id values, not child_trigger_id. The docstring explicitly states the function returns 🧩 Proposed fix- .returning(db.CompositeTriggerChildFiring.child_trigger_id)
+ .returning(db.CompositeTriggerChildFiring.child_firing_id)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| return set(result.scalars().all()) | ||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import asyncio | ||
| import datetime | ||
| from datetime import timedelta | ||
| from typing import List | ||
|
|
@@ -1624,3 +1625,123 @@ async def test_sequence_trigger_identical_event_triggers_only_one_fired_does_not | |
| await triggers.reactive_evaluation(ingredients_buy) | ||
|
|
||
| act.assert_not_called() | ||
|
|
||
|
|
||
| class TestCompoundTriggerConcurrency: | ||
| """Tests for concurrent child trigger evaluation race condition fix.""" | ||
|
|
||
| @pytest.fixture | ||
| async def compound_automation_concurrent( | ||
| self, | ||
| automations_session: AsyncSession, | ||
| cleared_buckets: None, | ||
| cleared_automations: None, | ||
| ) -> Automation: | ||
|
Comment on lines
+1634
to
+1639
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, locate the test file and check if it exists
fd "test_composite_triggers.py" --type fRepository: qodo-benchmark/prefect-combined-coderabbit Length of output: 141 🏁 Script executed: # Get line count to understand file size
wc -l tests/events/server/triggers/test_composite_triggers.pyRepository: qodo-benchmark/prefect-combined-coderabbit Length of output: 146 🏁 Script executed: # Read the specific lines mentioned in the review (1634-1639)
sed -n '1634,1650p' tests/events/server/triggers/test_composite_triggers.pyRepository: qodo-benchmark/prefect-combined-coderabbit Length of output: 771 🌐 Web query:
💡 Result: If Ruff is flagging a pytest-injected fixture parameter as unused (often in class-based tests/fixtures, showing up as
class TestX:
def test_something(self, _unused_fixture): ...(Only works if pytest will still resolve the fixture name as written.)
import pytest
`@pytest.mark.usefixtures`("db")
def test_something():
...
# local
def test_something(self, db): # noqa: ARG002
...
# pyproject.toml / ruff.toml
[tool.ruff.lint.per-file-ignores]
"tests/**/*.py" = ["ARG001", "ARG002"]Notes
Sources: Ruff 🏁 Script executed: # Read more lines to see complete method body and verify fixtures aren't used
sed -n '1634,1680p' tests/events/server/triggers/test_composite_triggers.pyRepository: qodo-benchmark/prefect-combined-coderabbit Length of output: 1766 🌐 Web query:
💡 Result: In If you’re adding a leading underscore only to silence a linter warning like Ruff ARG002 (“unused argument”), note that Ruff treats leading-underscore names as intentionally unused. [2] But that conflicts with pytest’s “inject-by-name” behavior unless your fixture is also named with the underscore. Practical options:
Sources: pytest fixture injection & 🏁 Script executed: # Search for `@pytest.mark.usefixtures` usage in the test file
rg "@pytest.mark.usefixtures" tests/events/server/triggers/test_composite_triggers.py -A 2Repository: qodo-benchmark/prefect-combined-coderabbit Length of output: 68 🏁 Script executed: # Also search for fixtures decorator definitions to understand fixture naming conventions
rg "@pytest.fixture" tests/events/server/triggers/test_composite_triggers.py -A 1 | head -30Repository: qodo-benchmark/prefect-combined-coderabbit Length of output: 821 🏁 Script executed: # Check if cleared_buckets or cleared_automations are defined/used elsewhere in the test file
rg "cleared_buckets|cleared_automations" tests/events/server/triggers/test_composite_triggers.pyRepository: qodo-benchmark/prefect-combined-coderabbit Length of output: 985 🏁 Script executed: # Also check the broader context - search for similar fixture cleanup patterns in the file
rg "@pytest.fixture.*cleared" tests/events/server/triggers/test_composite_triggers.py -B 1 -A 3Repository: qodo-benchmark/prefect-combined-coderabbit Length of output: 68 🏁 Script executed: # Search for fixture definitions in conftest.py files
fd "conftest.py" --type f | xargs grep -l "cleared_buckets\|cleared_automations" 2>/dev/nullRepository: qodo-benchmark/prefect-combined-coderabbit Length of output: 158 🏁 Script executed: # Search the entire test directory for fixture definitions
rg "def cleared_buckets|def cleared_automations" tests/ -A 3Repository: qodo-benchmark/prefect-combined-coderabbit Length of output: 648 🏁 Script executed: # Check if there's a conftest in the test directory or parent
find tests/events/server/triggers -name "conftest.py" -exec cat {} \;Repository: qodo-benchmark/prefect-combined-coderabbit Length of output: 2112 Use
The correct approach for side-effect-only fixtures is to use the `@pytest.mark.usefixtures`("cleared_buckets", "cleared_automations")
async def compound_automation_concurrent(
self,
automations_session: AsyncSession,
) -> Automation:This is the idiomatic pytest pattern and aligns with Ruff's PT019 recommendation. 🧰 Tools🪛 Ruff (0.14.14)1637-1637: Unused method argument: (ARG002) 1638-1638: Unused method argument: (ARG002) 🤖 Prompt for AI Agents |
||
| """Compound trigger requiring all child triggers to fire.""" | ||
| compound_automation = Automation( | ||
| name="Compound Automation Concurrency Test", | ||
| trigger=CompoundTrigger( | ||
| require="all", | ||
| within=timedelta(minutes=5), | ||
| triggers=[ | ||
| EventTrigger( | ||
| expect={"event.A"}, | ||
| match={"prefect.resource.id": "*"}, | ||
| posture=Posture.Reactive, | ||
| threshold=1, | ||
| ), | ||
| EventTrigger( | ||
| expect={"event.B"}, | ||
| match={"prefect.resource.id": "*"}, | ||
| posture=Posture.Reactive, | ||
| threshold=1, | ||
| ), | ||
| ], | ||
| ), | ||
| actions=[actions.DoNothing()], | ||
| ) | ||
|
|
||
| persisted = await automations.create_automation( | ||
| session=automations_session, automation=compound_automation | ||
| ) | ||
| compound_automation.created = persisted.created | ||
| compound_automation.updated = persisted.updated | ||
| triggers.load_automation(persisted) | ||
| await automations_session.commit() | ||
|
|
||
| return compound_automation | ||
|
|
||
| async def test_compound_trigger_does_not_double_fire_when_children_race( | ||
| self, | ||
| act: mock.AsyncMock, | ||
| compound_automation_concurrent: Automation, | ||
| start_of_test: DateTime, | ||
| ): | ||
| """ | ||
| Regression test for compound trigger double-firing when child firings race. | ||
|
|
||
| Verifies that when two child trigger events are processed concurrently, | ||
| the compound trigger fires exactly once. The DELETE ... RETURNING fix | ||
| ensures only one worker proceeds to fire the parent trigger. | ||
| """ | ||
| event_a = ReceivedEvent( | ||
| occurred=start_of_test + timedelta(microseconds=1), | ||
| event="event.A", | ||
| resource={"prefect.resource.id": "test.resource"}, | ||
| id=uuid4(), | ||
| ) | ||
| event_b = ReceivedEvent( | ||
| occurred=start_of_test + timedelta(microseconds=2), | ||
| event="event.B", | ||
| resource={"prefect.resource.id": "test.resource"}, | ||
| id=uuid4(), | ||
| ) | ||
|
|
||
| # Process both events concurrently | ||
| await asyncio.gather( | ||
| triggers.reactive_evaluation(event_a), | ||
| triggers.reactive_evaluation(event_b), | ||
| ) | ||
|
|
||
| # The compound trigger should fire exactly once | ||
| act.assert_called_once() | ||
|
|
||
| firing: Firing = act.call_args.args[0] | ||
| assert isinstance(firing.trigger, CompoundTrigger) | ||
| assert firing.trigger.id == compound_automation_concurrent.trigger.id | ||
|
|
||
| async def test_concurrent_child_firings_still_triggers_parent( | ||
| self, | ||
| act: mock.AsyncMock, | ||
| compound_automation_concurrent: Automation, | ||
| start_of_test: DateTime, | ||
| ): | ||
| """ | ||
| Verify that when two child trigger events arrive nearly simultaneously, | ||
| the compound trigger still fires. This tests that the race condition fix | ||
| doesn't prevent legitimate firings. | ||
| """ | ||
| event_a = ReceivedEvent( | ||
| occurred=start_of_test + timedelta(microseconds=1), | ||
| event="event.A", | ||
| resource={"prefect.resource.id": "test.resource"}, | ||
| id=uuid4(), | ||
| ) | ||
| event_b = ReceivedEvent( | ||
| occurred=start_of_test + timedelta(microseconds=2), | ||
| event="event.B", | ||
| resource={"prefect.resource.id": "test.resource"}, | ||
| id=uuid4(), | ||
| ) | ||
|
|
||
| # Process both events concurrently to simulate the race condition | ||
| await asyncio.gather( | ||
| triggers.reactive_evaluation(event_a), | ||
| triggers.reactive_evaluation(event_b), | ||
| ) | ||
|
|
||
| # The compound trigger should fire exactly once | ||
| act.assert_called_once() | ||
|
|
||
| firing: Firing = act.call_args.args[0] | ||
| assert firing.trigger.id == compound_automation_concurrent.trigger.id | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cat -n src/prefect/server/events/models/composite_trigger_child_firing.py | head -80Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 3630
🏁 Script executed:
Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 2265
🏁 Script executed:
Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 292
🏁 Script executed:
# Check the trigger model definition to see what type trigger.id is fd composite_trigger -t fRepository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 361
🌐 Web query:
Python hash randomization PYTHONHASHSEED how does it affect hash(str()) consistency across processes💡 Result:
hash(str_obj)in CPython is stable only within a single interpreter process unless you control the hash seed.What hash randomization does
PYTHONHASHSEEDunset or set torandom), CPython chooses a different per-process secret seed at startup, sohash("same text")will generally differ across separate runs / processes. (docs.python.org)strandbytes(and, at the implementation level, also covers the same hashing machinery used formemoryview). (docs.python.org)How
PYTHONHASHSEEDaffects cross-process consistencyFrom the Python docs: (docs.python.org)
random→ different hash seed each process →hash("x")not consistent across processes.N(0…4294967295) → fixed seed →hash("x")becomes repeatable across processes that use the same seed.0→ “disable hash randomization” (i.e., no per-process random seed), giving repeatable results across runs. (docs.python.org)Example (Unix-like shells):
Important caveats
PYTHONHASHSEED, don’t treathash()as a stable persistent identifier: results can differ across Python versions/build options/word size/platforms (the hash algorithm is an internal detail exposed viasys.hash_info.algorithm). (docs.python.org)PYTHONHASHSEEDmust be set before the interpreter starts; changingos.environ["PYTHONHASHSEED"]inside a running process won’t retroactively changehash()behavior for that process. (stackoverflow.com)Citations:
🏁 Script executed:
Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 1950
🏁 Script executed:
Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 250
🏁 Script executed:
Repository: qodo-benchmark/prefect-combined-coderabbit
Length of output: 1909
Use a deterministic advisory-lock key across processes.
hash(str(trigger.id))is randomized per process by default (controlled byPYTHONHASHSEED), so different workers compute different keys and skip mutual exclusion. Use the UUID's stable integer representation instead, which is identical across all processes and aligns with the comment's intent.🔒 Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents