fix(seahorse): address 3 blocking bugs from code review
- Fix resequenceContextItemsTx scan error handling (store.go:850) Changed `return err` to `return scanErr` to properly propagate scan errors instead of returning nil (which silently corrupts data) - Fix sql.NullString for INTEGER column (store.go:847) Changed `mid` from sql.NullString to sql.NullInt64 since message_id is INTEGER in schema. Removed unnecessary strconv.ParseInt call. - Fix compactCondensed fallback deleting non-candidate items Added ReplaceContextItemsWithSummary method for per-item deletion when candidates are not contiguous in ordinal space. Optimized to use range deletion when candidates are consecutive.
This commit is contained in:
parent
03e26a6d20
commit
615c617a61
3 changed files with 241 additions and 12 deletions
|
|
@ -387,24 +387,55 @@ func (e *CompactionEngine) compactCondensed(ctx context.Context, convID int64) (
|
||||||
|
|
||||||
startOrd := -1
|
startOrd := -1
|
||||||
endOrd := -1
|
endOrd := -1
|
||||||
|
hasNonCandidate := false
|
||||||
for _, item := range items {
|
for _, item := range items {
|
||||||
if item.ItemType == "summary" && candidateSet[item.SummaryID] {
|
if item.ItemType == "summary" && candidateSet[item.SummaryID] {
|
||||||
if startOrd == -1 || item.Ordinal < startOrd {
|
if startOrd == -1 {
|
||||||
|
startOrd, endOrd = item.Ordinal, item.Ordinal
|
||||||
|
} else {
|
||||||
|
// Check for non-candidate items between endOrd and current ordinal
|
||||||
|
for _, it := range items {
|
||||||
|
if it.Ordinal > endOrd && it.Ordinal <= item.Ordinal {
|
||||||
|
if it.ItemType != "summary" || !candidateSet[it.SummaryID] {
|
||||||
|
hasNonCandidate = true
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if hasNonCandidate {
|
||||||
|
break
|
||||||
|
}
|
||||||
|
if item.Ordinal < startOrd {
|
||||||
startOrd = item.Ordinal
|
startOrd = item.Ordinal
|
||||||
}
|
}
|
||||||
if endOrd == -1 || item.Ordinal > endOrd {
|
if item.Ordinal > endOrd {
|
||||||
endOrd = item.Ordinal
|
endOrd = item.Ordinal
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
if startOrd == -1 || endOrd == -1 {
|
if startOrd == -1 || endOrd == -1 {
|
||||||
return nil, nil
|
return nil, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Collect candidate summary IDs
|
||||||
|
candidateIDs := make([]string, 0, len(candidates))
|
||||||
|
for _, c := range candidates {
|
||||||
|
candidateIDs = append(candidateIDs, c.SummaryID)
|
||||||
|
}
|
||||||
|
|
||||||
|
if hasNonCandidate {
|
||||||
|
// Use safe per-item deletion to avoid deleting non-candidate items
|
||||||
|
if err := e.store.ReplaceContextItemsWithSummary(ctx, convID, candidateIDs, summary.SummaryID); err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
// Candidates are consecutive, use efficient range deletion
|
||||||
if err := e.store.ReplaceContextRangeWithSummary(ctx, convID, startOrd, endOrd, summary.SummaryID); err != nil {
|
if err := e.store.ReplaceContextRangeWithSummary(ctx, convID, startOrd, endOrd, summary.SummaryID); err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
return &summary.SummaryID, nil
|
return &summary.SummaryID, nil
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -4,7 +4,6 @@ import (
|
||||||
"context"
|
"context"
|
||||||
"database/sql"
|
"database/sql"
|
||||||
"fmt"
|
"fmt"
|
||||||
"strconv"
|
|
||||||
"strings"
|
"strings"
|
||||||
"time"
|
"time"
|
||||||
)
|
)
|
||||||
|
|
@ -820,6 +819,101 @@ func (s *Store) ReplaceContextRangeWithSummary(
|
||||||
return tx.Commit()
|
return tx.Commit()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ReplaceContextItemsWithSummary replaces specific context items (by summary_id) with a new summary.
|
||||||
|
// Use this when candidates are not contiguous in ordinal space to avoid deleting non-candidate items.
|
||||||
|
func (s *Store) ReplaceContextItemsWithSummary(
|
||||||
|
ctx context.Context,
|
||||||
|
convID int64,
|
||||||
|
summaryIDs []string,
|
||||||
|
newSummaryID string,
|
||||||
|
) error {
|
||||||
|
if len(summaryIDs) == 0 {
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
tx, err := s.db.BeginTx(ctx, nil)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
defer tx.Rollback()
|
||||||
|
|
||||||
|
// Find the ordinals of items to delete and calculate midpoint
|
||||||
|
placeholders := make([]string, len(summaryIDs))
|
||||||
|
args := make([]any, len(summaryIDs)+1)
|
||||||
|
args[0] = convID
|
||||||
|
for i, sid := range summaryIDs {
|
||||||
|
placeholders[i] = "?"
|
||||||
|
args[i+1] = sid
|
||||||
|
}
|
||||||
|
|
||||||
|
query := fmt.Sprintf(
|
||||||
|
"SELECT ordinal FROM context_items WHERE conversation_id = ? AND summary_id IN (%s) ORDER BY ordinal",
|
||||||
|
strings.Join(placeholders, ","),
|
||||||
|
)
|
||||||
|
rows, err := tx.QueryContext(ctx, query, args...)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
|
||||||
|
var ordinals []int
|
||||||
|
for rows.Next() {
|
||||||
|
var ord int
|
||||||
|
if err := rows.Scan(&ord); err != nil {
|
||||||
|
rows.Close()
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
ordinals = append(ordinals, ord)
|
||||||
|
}
|
||||||
|
rows.Close()
|
||||||
|
|
||||||
|
if len(ordinals) == 0 {
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
midpoint := (ordinals[0] + ordinals[len(ordinals)-1]) / 2
|
||||||
|
|
||||||
|
// Delete the specific items by summary_id
|
||||||
|
deleteQuery := fmt.Sprintf(
|
||||||
|
"DELETE FROM context_items WHERE conversation_id = ? AND summary_id IN (%s)",
|
||||||
|
strings.Join(placeholders, ","),
|
||||||
|
)
|
||||||
|
_, err = tx.ExecContext(ctx, deleteQuery, args...)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check if midpoint conflicts with existing ordinal
|
||||||
|
var conflict bool
|
||||||
|
var existingOrd int
|
||||||
|
err = tx.QueryRowContext(ctx,
|
||||||
|
"SELECT ordinal FROM context_items WHERE conversation_id = ? AND ordinal = ?",
|
||||||
|
convID, midpoint,
|
||||||
|
).Scan(&existingOrd)
|
||||||
|
if err == nil {
|
||||||
|
conflict = true
|
||||||
|
}
|
||||||
|
|
||||||
|
if conflict {
|
||||||
|
// Gap exhausted, need resequence
|
||||||
|
err = s.resequenceContextItemsTx(ctx, tx, convID, newSummaryID)
|
||||||
|
if err != nil {
|
||||||
|
return fmt.Errorf("resequence: %w", err)
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
// Normal insert at midpoint
|
||||||
|
_, err = tx.ExecContext(ctx,
|
||||||
|
`INSERT INTO context_items (conversation_id, ordinal, item_type, summary_id, token_count)
|
||||||
|
SELECT ?, ?, 'summary', ?, token_count FROM summaries WHERE summary_id = ?`,
|
||||||
|
convID, midpoint, newSummaryID, newSummaryID,
|
||||||
|
)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return tx.Commit()
|
||||||
|
}
|
||||||
|
|
||||||
// resequenceContextItemsTx renumbers context_items with fresh OrdinalStep gaps.
|
// resequenceContextItemsTx renumbers context_items with fresh OrdinalStep gaps.
|
||||||
// Uses temp negative ordinals to avoid PRIMARY KEY constraint violations (spec lines 1240-1247).
|
// Uses temp negative ordinals to avoid PRIMARY KEY constraint violations (spec lines 1240-1247).
|
||||||
func (s *Store) resequenceContextItemsTx(ctx context.Context, tx *sql.Tx, convID int64, newSummaryID string) error {
|
func (s *Store) resequenceContextItemsTx(ctx context.Context, tx *sql.Tx, convID int64, newSummaryID string) error {
|
||||||
|
|
@ -844,17 +938,17 @@ func (s *Store) resequenceContextItemsTx(ctx context.Context, tx *sql.Tx, convID
|
||||||
var items []item
|
var items []item
|
||||||
for rows.Next() {
|
for rows.Next() {
|
||||||
var i item
|
var i item
|
||||||
var sid, mid sql.NullString
|
var sid sql.NullString
|
||||||
|
var mid sql.NullInt64
|
||||||
var scanErr error
|
var scanErr error
|
||||||
if scanErr = rows.Scan(&i.ordinal, &i.itemType, &sid, &mid, &i.tokenCount); scanErr != nil {
|
if scanErr = rows.Scan(&i.ordinal, &i.itemType, &sid, &mid, &i.tokenCount); scanErr != nil {
|
||||||
return err
|
return scanErr
|
||||||
}
|
}
|
||||||
if sid.Valid {
|
if sid.Valid {
|
||||||
i.summaryID = sid.String
|
i.summaryID = sid.String
|
||||||
}
|
}
|
||||||
if mid.Valid {
|
if mid.Valid {
|
||||||
id, _ := strconv.ParseInt(mid.String, 10, 64)
|
i.messageID = mid.Int64
|
||||||
i.messageID = id
|
|
||||||
}
|
}
|
||||||
items = append(items, i)
|
items = append(items, i)
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1141,3 +1141,107 @@ func TestStoreSearchSummariesReturnsContent(t *testing.T) {
|
||||||
t.Errorf("SearchResult.Content = %q, want %q", results[0].Content, "This is the summary content for testing")
|
t.Errorf("SearchResult.Content = %q, want %q", results[0].Content, "This is the summary content for testing")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestStoreReplaceContextItemsWithSummary(t *testing.T) {
|
||||||
|
s := openTestStore(t)
|
||||||
|
ctx := context.Background()
|
||||||
|
|
||||||
|
conv, _ := s.GetOrCreateConversation(ctx, "agent:test-replace-items")
|
||||||
|
|
||||||
|
// Create messages
|
||||||
|
msgs := make([]int64, 5)
|
||||||
|
for i := 0; i < 5; i++ {
|
||||||
|
m, _ := s.AddMessage(ctx, conv.ConversationID, "user", fmt.Sprintf("msg%d", i), 2)
|
||||||
|
msgs[i] = m.ID
|
||||||
|
}
|
||||||
|
|
||||||
|
// Create summaries
|
||||||
|
summaries := make([]string, 3)
|
||||||
|
for i := 0; i < 3; i++ {
|
||||||
|
sum, _ := s.CreateSummary(ctx, CreateSummaryInput{
|
||||||
|
ConversationID: conv.ConversationID,
|
||||||
|
Kind: SummaryKindLeaf,
|
||||||
|
Depth: 0,
|
||||||
|
Content: fmt.Sprintf("summary %d", i),
|
||||||
|
TokenCount: 10,
|
||||||
|
})
|
||||||
|
summaries[i] = sum.SummaryID
|
||||||
|
}
|
||||||
|
|
||||||
|
// Insert context items with a message in between summaries:
|
||||||
|
// Ordinals: 100 (summary0), 200 (message), 300 (summary1), 400 (summary2)
|
||||||
|
items := []ContextItem{
|
||||||
|
{Ordinal: 100, ItemType: "summary", SummaryID: summaries[0], TokenCount: 10},
|
||||||
|
{Ordinal: 200, ItemType: "message", MessageID: msgs[1], TokenCount: 2},
|
||||||
|
{Ordinal: 300, ItemType: "summary", SummaryID: summaries[1], TokenCount: 10},
|
||||||
|
{Ordinal: 400, ItemType: "summary", SummaryID: summaries[2], TokenCount: 10},
|
||||||
|
}
|
||||||
|
s.UpsertContextItems(ctx, conv.ConversationID, items)
|
||||||
|
|
||||||
|
// Create a new summary to replace with
|
||||||
|
newSummary, _ := s.CreateSummary(ctx, CreateSummaryInput{
|
||||||
|
ConversationID: conv.ConversationID,
|
||||||
|
Kind: SummaryKindCondensed,
|
||||||
|
Depth: 1,
|
||||||
|
Content: "condensed summary",
|
||||||
|
TokenCount: 15,
|
||||||
|
})
|
||||||
|
|
||||||
|
// Replace summaries 0 and 1 (not 2) using per-item deletion
|
||||||
|
// This should NOT delete the message at ordinal 200
|
||||||
|
err := s.ReplaceContextItemsWithSummary(ctx, conv.ConversationID, []string{summaries[0], summaries[1]}, newSummary.SummaryID)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("ReplaceContextItemsWithSummary: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify result: should have 3 items (message at 200, summary2 at 400, new summary)
|
||||||
|
result, _ := s.GetContextItems(ctx, conv.ConversationID)
|
||||||
|
if len(result) != 3 {
|
||||||
|
t.Fatalf("expected 3 items after replace, got %d", len(result))
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify message at ordinal 200 is preserved
|
||||||
|
messagePreserved := false
|
||||||
|
for _, item := range result {
|
||||||
|
if item.ItemType == "message" && item.MessageID == msgs[1] {
|
||||||
|
messagePreserved = true
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if !messagePreserved {
|
||||||
|
t.Error("message at ordinal 200 should have been preserved")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify summary2 at ordinal 400 is preserved
|
||||||
|
summary2Preserved := false
|
||||||
|
for _, item := range result {
|
||||||
|
if item.ItemType == "summary" && item.SummaryID == summaries[2] {
|
||||||
|
summary2Preserved = true
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if !summary2Preserved {
|
||||||
|
t.Error("summary2 at ordinal 400 should have been preserved")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify new summary exists
|
||||||
|
newSummaryFound := false
|
||||||
|
for _, item := range result {
|
||||||
|
if item.ItemType == "summary" && item.SummaryID == newSummary.SummaryID {
|
||||||
|
newSummaryFound = true
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if !newSummaryFound {
|
||||||
|
t.Error("new summary should exist")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify no duplicate ordinals
|
||||||
|
ordinalSet := make(map[int]bool)
|
||||||
|
for _, item := range result {
|
||||||
|
if ordinalSet[item.Ordinal] {
|
||||||
|
t.Errorf("duplicate ordinal %d detected", item.Ordinal)
|
||||||
|
}
|
||||||
|
ordinalSet[item.Ordinal] = true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue