Repository navigation
fix(triggers): don't fire AFTER COMMIT triggers for aborted txns; don't silently drop queued ones on shutdown - #4565
Merged
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
andrejtonev
changed the title
andrejtonev
added
bug
andrejtonev
changed the title
andrejtonev
force-pushed
the
fix/trigger-fires-for-aborted-txn
branch
from
August 26, 2026 04:56
97e702c to
7fb49f9
Compare
as51340
requested changes
Aug 28, 2026
andrejtonev
added a commit
that referenced
this pull request
Aug 28, 2026
…ger e2e assertion Address as51340's review on #4565: - Drop the TransactionWasCommitted helper (+ its unit tests) and reuse the existing replication_error_committed bool for the txn_committed predicate. - e2e: assert on the TriggersExecuted metric (SHOW METRICS INFO) instead of grepping main's log; the trigger's own CREATE (:Audit) also aborts under STRICT_SYNC, so the metric -- bumped inside Trigger::Execute() before that commit -- is the only queryable signal that separates the two binaries. - Comment why ShutDownDrainsNothingButFinishesTheRunningTask's finished-check is guaranteed (jthread join), not racy.
as51340
approved these changes
Aug 28, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 31, 2026
andrejtonev
removed
CI -build=coverage -test=core
ShutDown() empties task_queue_ rather than draining it, and AddTask() silently returns once stop has been requested. Both losses were invisible: no return value, no counter, no log. unfinished_tasks_num_ was also never decremented for the discarded tasks, leaving the counter permanently dirty. Discard stays the semantics -- the replication client and the coordinator teardown both shut down pools whose queued work would otherwise block process shutdown on an unreachable peer -- but it is now reportable: ShutDown() returns how many queued tasks it threw away and AddTask() returns whether the task was accepted. Neither is [[nodiscard]], so the existing statement-form call sites are unaffected. The discarded tasks are also now destroyed after the workers are joined instead of under pool_lock_. Destroying a task runs arbitrary user destructors -- for the after-commit trigger pool that is a storage accessor's Abort/FinalizeTransaction plus a gatekeeper accessor release -- which has no business running under a thread-pool mutex, nor concurrently with a worker still finishing its in-flight task. Claude-Session: https://claude.ai/code/session_01X8MoZnBB3FvaBgWsHrMDmZ
Database::StopAllBackgroundTasks() shuts down the after-commit trigger pool, discarding every trigger execution still queued for that tenant. Those triggers belong to transactions the client was already told had committed, and nothing replays them: trigger definitions are durable, but pending executions are not -- no WAL record, no resume-time replay, no metric. The loss was completely silent. Reachable on DROP DATABASE ... FORCE, the replica-side apply of DropDatabase, and process shutdown. Plain DROP DATABASE and SUSPEND are not affected: both gate on the gatekeeper's sole-accessor check, and a queued trigger task holds a DatabaseAccess, so they fail with USING / ACTIVE_CONNECTIONS long before reaching this code. Report both loss windows at WARN, named by tenant: the queued executions discarded by the shutdown itself, and -- via the now out-of-line Database::AddTask -- a trigger scheduled after the pool is already down, which a commit racing a FORCE drop on another session can still hit. Draining instead of discarding was rejected. DbmsHandler::Delete() holds its exclusive lock_ across the whole of Delete_, and a drained trigger runs user Cypher bounded only by --query-execution-timeout-sec (600s by default), once per queued task. Stalling every tenant in the process for minutes is worse than the loss, and the same primitive's replication and coordination users would then block shutdown on unreachable peers. Claude-Session: https://claude.ai/code/session_01X8MoZnBB3FvaBgWsHrMDmZ
Four cases: an exact discarded count on a zero-worker pool (deterministic, no latch or sleep), AddTask rejection after shutdown, an in-flight task finishing while the tasks queued behind it are discarded, and zero on an idle pool. The in-flight case asserts discarded + ran == queued rather than an exact split: whether the worker pops one more task before ShutDown takes pool_lock_ is a real race, so only the total is invariant. Claude-Session: https://claude.ai/code/session_01X8MoZnBB3FvaBgWsHrMDmZ
Review follow-ups, both in code this branch introduced. AddTask incremented unfinished_tasks_num_ after releasing pool_lock_. Harmless while nothing ever decremented it for un-run tasks, but this branch's new fetch_sub in ShutDown() made a transient size_t wraparound reachable: a push that has left the lock but not yet incremented can be swapped out and decremented first, so a reader sees SIZE_MAX until the increment lands. Increment under the same lock as the push. The shutdown warning claimed the discarded triggers belong to already-committed transactions. Not universally true: the interpreter schedules the after-commit task without consulting ReplicationError::transaction_committed, and a STRICT_SYNC quorum failure reports false after AbortAndResetCommitTs(). Drop the commit claim and match the wording of the sibling warning. Claude-Session: https://claude.ai/code/session_01X8MoZnBB3FvaBgWsHrMDmZ
Comment-only. Three edits from the comment-pruner pass: - ShutDown()'s declaration no longer enumerates which callers depend on discard semantics. The list was already stale when written -- this same branch added a third caller -- and the primitive's contract is the same regardless of who calls it: draining is avoided because a queued task can block on external I/O. - The concrete example at the `discarded` declaration STAYS. The abstract "arbitrary user destructors" claim is unfalsifiable on its own; naming Abort/FinalizeTransaction and the gatekeeper release is what lets the next reader verify the ordering rationale instead of trusting it. - Both test rationale comments tightened. The in-flight test's said the worker "may pop one" of the queued tasks; it can drain any number of them, which is what ASSERT_LE already allowed. Claude-Session: https://claude.ai/code/session_01X8MoZnBB3FvaBgWsHrMDmZ
Whether a StorageManipulationError leaves the transaction committed on main is a property of the error, but it was open-coded at each consumer (InMemoryAccessor::PeriodicCommit, HandlePeriodicCommitError). A third consumer is about to need it, so give it a name. Only ReplicationError can describe a still-committed transaction, and only when its transaction_committed flag is set. Reaching the flag's false state provably means FinalizeCommitPhase never ran: the 2PC branch gets there only when the prepare phase failed and AbortAndResetCommitTs has already undone the deltas. Claude-Session: https://claude.ai/code/session_01C5tmTDLt6j6NdiW4BMn87o
Interpreter::Commit() enqueued the after-commit trigger task on nothing but the presence of a trigger context, never on whether the commit succeeded. The ReplicationError arm of the error visitor above only records a message -- the throw is further down, past the enqueue -- so a STRICT_SYNC transaction whose 2PC prepare phase failed reaches the enqueue looking exactly like a success and runs the user's trigger for effects that were rolled back. The damage is not merely a spurious run. AdaptForAccessor re-resolves the context against a fresh accessor and prunes what no longer exists, but deleted_vertices_/deleted_edges_ are deliberately left alone, so a DELETE-typed trigger is handed vertices that were never deleted and acts on them. (CREATE/UPDATE-typed triggers are gated off by ShouldEventTrigger once their containers are pruned, so they are already self-limiting; DELETE and ANY are the exposed cases.) Keyed on transaction_committed rather than on replication mode: a SYNC replica failing with no STRICT_SYNC replica registered leaves the transaction committed on main and must still fire its trigger, and suppressing that would be a worse bug than this one. Skipping the enqueue lands on the same accessor teardown path a commit already takes when the database has no after-commit triggers. Claude-Session: https://claude.ai/code/session_01C5tmTDLt6j6NdiW4BMn87o
Unit tests pin TransactionWasCommitted over every StorageManipulationError alternative, including that the transaction_committed flag alone decides the answer even when the failure list is empty. The e2e test asserts on main's log rather than on graph data, which is deliberate: while the STRICT_SYNC replica is down the trigger's own commit takes the same 2PC path and also aborts, so a MATCH (:Audit) count reads 0 with or without the fix and would prove nothing. The trigger is DELETE-typed for the same reason -- a CREATE-typed one never executes its body post-abort because AdaptForAccessor empties created_vertices_ before ShouldEventTrigger looks at it, so that test would pass unfixed. test_after_commit_trigger_fires_for_committed_txn is the opposite guard: on a healthy cluster the trigger must still fire and persist its write. Claude-Session: https://claude.ai/code/session_01C5tmTDLt6j6NdiW4BMn87o
Log the replication failure reason in the skip warning. Two sibling sites already log FormatReplicationError's output at WARN for the same error type, and without it an operator gets no reason at all -- the client-facing ReplicationException text is only logged at trace. Read the after-commit trigger count once. It is an unguarded skiplist read, so taking it twice let a concurrent DROP TRIGGER make the logged count disagree with the condition that selected the branch. TransactionWasCommitted tests membership of one variant alternative, so use std::get_if rather than a five-arm visitor, matching the idiom in commit_args.hpp. Drops the <type_traits> include the visitor needed. Claude-Session: https://claude.ai/code/session_01C5tmTDLt6j6NdiW4BMn87o
The test killed instance_1 and never restarted it. The module-level
kill() defaults to keep_directories=True, and MemgraphInstanceRunner.kill
opens with `if not self.is_running(): return`, so the teardown
kill_all(keep_directories=False) early-returned for the already-dead
instance and never deleted its data directory. The next run booted
instance_1 on a stale memgraph DB whose UUID could not be realigned
("Default storage is not clean, cannot update UUID"), leaving it
permanently out of sync, so the first STRICT_SYNC write on main failed.
That produced a perfectly alternating flake: a failing run never reached
the kill, so instance_1 was still running at teardown and got cleaned,
which made the next run pass, which left the directory behind again.
Measured P,F,P,F,P over eight full-suite runs; the only leftover on disk
after a passing run was instance_1.
Pass keep_directories=False at the kill, since nothing restarts that
instance. 6/6 full-suite runs green afterwards with no leftovers.
The harness's kill() ignoring keep_directories for an already-dead
instance is a pre-existing defect and is left alone here; any future test
that kills an instance without restarting it will hit the same trap.
Also drops the comment pruning pass's leftovers and an execute_until_success
retry helper that treated the symptom -- it would have masked a genuinely
broken replica, and it never worked (failing runs burned its full 20s
budget and still failed).
Claude-Session: https://claude.ai/code/session_01C5tmTDLt6j6NdiW4BMn87o
…ger e2e assertion Address as51340's review on #4565: - Drop the TransactionWasCommitted helper (+ its unit tests) and reuse the existing replication_error_committed bool for the txn_committed predicate. - e2e: assert on the TriggersExecuted metric (SHOW METRICS INFO) instead of grepping main's log; the trigger's own CREATE (:Audit) also aborts under STRICT_SYNC, so the metric -- bumped inside Trigger::Execute() before that commit -- is the only queryable signal that separates the two binaries. - Comment why ShutDownDrainsNothingButFinishesTheRunningTask's finished-check is guaranteed (jthread join), not racy.
The thread-pool ShutDown tests coordinated threads with std::atomic<bool>
flags and while(!flag){sleep(1ms)} spin loops (and one UnfinishedTasksNum()
poll). Replace them with std::latch (C++20, <latch>): wait() blocks lock-free
instead of busy-spinning, and count_down() strongly-happens-before the wait it
releases. Same two-latch ordered-handshake idiom already used in
tests/unit/utils_shared_quota.cpp. Behaviour is unchanged -- this is a
readability/CPU-politeness cleanup, not a bug fix; the previous polling was
correct (a task's ran++ completes inside task(), before the worker decrements
the unfinished counter).
andrejtonev
force-pushed
the
fix/trigger-fires-for-aborted-txn
branch
from
September 9, 2026 10:05
07fb04a to
f0bbc0a
Compare
andrejtonev
enabled auto-merge
September 9, 2026 10:05
|
77 of 87 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AFTER COMMIT triggers fired for STRICT_SYNC transactions that failed at commit, so a trigger acted on rolled-back state; dispatch is now gated on whether the transaction actually committed. A SYNC replica that fails after the main finalized the commit still fires its triggers (the data is on main); a STRICT_SYNC 2PC transaction that aborts does not. Separately, ThreadPool::ShutDown() silently discarded queued triggers on shutdown/DROP — it now returns and logs the discarded count and rejects post-shutdown tasks.