fix(db): revive soft-deleted push configs and checkpoints on re-upsert - #2488
Closed
QuentinBisson wants to merge 1 commit into
Closed
fix(db): revive soft-deleted push configs and checkpoints on re-upsert#2488QuentinBisson wants to merge 1 commit into
QuentinBisson wants to merge 1 commit into
Conversation
Every table with a deleted_at column is read through a deleted_at IS NULL filter, so an upsert that can match a tombstone has to state what it means: either it clears deleted_at and the row comes back, or it refuses to write and the caller is told. agent, tool, toolserver and the CrewAI memory tables clear it; task refuses. push_notification, lg_checkpoint and lg_checkpoint_write did neither, so a re-upsert updated a tombstone that stayed deleted and the write was invisible to every read, silently, with no error for the caller. These three carry writer-owned state rather than an audited identity: a client re-registering a push config, or a checkpointer writing a thread's state again, is describing what should be current, so the row comes back. That matches the other state tables, and the sessions and tasks decision not to resurrect an identity stays as it is. Both paths are reachable through DeletePushNotification and DeleteCheckpoint on the database client. Signed-off-by: QuentinBisson <quentin@giantswarm.io>
Contributor
Author
|
Closing: this fixes an unreachable path. Nothing soft-deletes |
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.
Same class of bug as #2279, on the tables that issue did not cover.
The rule
Every table with a
deleted_atcolumn is read through adeleted_at IS NULLfilter. So an upsert whoseON CONFLICTtarget can match a tombstone has to say what it means:deleted_at, and the row comes back, orDoing neither updates a tombstone that stays deleted, so the write lands where no read can see it and the caller is told nothing.
Where each table stood
agentdeleted_at = NULLtool,toolserverdeleted_at = NULLcrewai_agent_memory,crewai_flow_statedeleted_at = NULLtaskWHERE task.deleted_at IS NULLsessionpush_notificationlg_checkpointlg_checkpoint_writeevent,feedbackagent_instance,agent_instance_taskandagent_instance_task_eventhave nodeleted_atand are hard-deleted with a cascade, so they are outside this.The fix
The three unguarded upserts clear
deleted_at, which is the same choice the other state tables already make.These three hold writer-owned state, not an audited identity. A client re-registering a push notification config under an id it used before, or a checkpointer writing a thread's state again, is describing what should be current, so the right outcome is that the row comes back. Sessions and tasks are the opposite case, an identity that must not be reused, and that decision stays as it is.
Both soft-delete paths are reachable from the database client,
DeletePushNotificationandDeleteCheckpoint, so this is a live trap for anything that calls them rather than a hypothetical one.Tests
TestDeletedPushConfigIsRevivedByReRegistration: register a config, delete the task's configs, register the same id again, and it is readable and listed once.TestDeletedCheckpointRowsAreRevivedByANewWrite: write a checkpoint and its write, delete the thread, write the same checkpoint id again, and both are readable with the new content.Both run against a real Postgres and both fail without the query change: the list comes back empty and the get returns not-found while the write reported success.