fix(consolidation): guard observation_history append on 0-row observation UPDATE - #3161
Merged
Conversation
The source-liveness checks in _execute_update_action guard the *source* memories, but the observation row itself (UPDATE ... WHERE id = $5) can be concurrently invalidated/deleted, matching 0 rows. The code then fell through to _append_observation_history, whose INSERT carries an observation_id FK onto memory_units — raising ForeignKeyViolationError, a (correctly) non-retryable integrity failure that marked the whole consolidation op failed for a row that simply no longer exists. Capture the UPDATE status in the SQL branch and bail out (return None) before the history append when 0 rows matched. The store/upsert branch cannot hit the 0-row case, so it needs no guard. The Oracle wrapper reshapes rowcount into the same "UPDATE <n>" form, so the parse is dialect-safe (mirrors config_resolver). Adds mock-level regression tests covering both the 0-row bail and the positive (rowcount==1) control.
…ow guard Move the command-tag rowcount parse out of consolidation business logic and into the pg/oracle connection layer. DatabaseConnection.execute_rows_affected runs a DML statement and returns a plain int, normalizing the dialect-divergent result shape the same way parse_json normalizes JSON columns: asyncpg returns the tag directly, the Oracle connection reshapes cursor.rowcount into the same trailing-count form, so parsing the last token is dialect-safe. _execute_update_action now calls conn.execute_rows_affected(...) and checks the int directly instead of hand-parsing an "UPDATE <n>" string. Adds a parser unit test covering the tag shapes both dialects emit.
nicoloboschi
force-pushed
the
fix/update-action-fk-guard-clean
branch
from
August 4, 2026 14:15
60dd41b to
e51e081
Compare
6 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
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.
Summary
When consolidation updates an existing observation,
_execute_update_actionruns source-liveness checks (a non-locking preflight plus a
FOR SHARErecheck) — but those guard only the source memories. The observation row
itself (
UPDATE memory_units ... WHERE id = $5) can still be concurrentlyinvalidated/deleted, so the UPDATE matches 0 rows. The code then fell
through to
_append_observation_history, whose INSERT carries anobservation_idFK ontomemory_units. Appending history for a now-missingrow raises
ForeignKeyViolationError— a (correctly) non-retryable integrityfailure that marks the whole consolidation operation failed for a row that
simply no longer exists.
This captures the UPDATE command status in the SQL branch and bails out
(
return None) before writing history when 0 rows matched. Thestore/upsert branch is an upsert and cannot hit the 0-row case, so it needs no
guard. The Oracle
DatabaseConnectionwrapper reshapesrowcountinto the same"UPDATE <n>"command-tag form, so theint(status.split()[-1])parse isdialect-safe (same idiom already used in
config_resolver.py).Relationship to #3094
This supersedes #3094, which diagnosed the same FK race correctly but:
unconditionally, after the
if/else; andupdate_status, which is only assigned in theSQL branch. On the upsert/non-SQL path (
writes_memory_rows_in_sql=False)that variable is unbound, so the block raises
UnboundLocalErroron everyupdate through that path — turning a rare, correct-to-skip race into an
unconditional crash. Its tests only drove the SQL branch, so this was not
caught.
This PR keeps a single guard in the only branch that can 0-row, and is written
against the current
mainshape of_execute_update_action(pool-based,with the preflight +
FOR SHAREliveness checks that #3094's base predated).Validation
Mock-level regression tests in
tests/test_integrity_violation_not_retried.py:test_update_action_bails_when_observation_row_missing— UPDATE returns"UPDATE 0"; asserts the function returnsNoneand_append_observation_historyis not called (the FK-violating INSERT is skipped).
test_update_action_writes_history_when_row_present— positive control:"UPDATE 1"; asserts history is appended and the embedding is returned.Mutation-checked: reverting the guard turns the first test red (history append
fires) while the positive control stays green.
ruff,ruff format, andtyall pass.