Skip to content

Fix memory expiration logic and Makefile warnings #6

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.

Sign up for GitHub

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

Open
fakebizprez wants to merge 2 commits into
base: main
Choose a base branch
from
+151 −16
Open

Fix memory expiration logic and Makefile warnings #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9b8a1e
Fix memory expiration logic and Makefile warnings
fakebizprez Feb 17, 2026
72fdffa
Fix memory expiration logic and Makefile warnings
fakebizprez Feb 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/plugin/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,17 @@ help:
all: build build-mcp

## build: Build the main plugin binary
build: $(BUILD_DIR)
build:
@mkdir -p $(BUILD_DIR)
@echo "Building $(BINARY_NAME) $(VERSION)..."
$(GO) build $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME) ./cmd/
Comment on lines +47 to 50
Copy link
Copy Markdown

@llamapreview llamapreview bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 | Confidence: High

The Makefile changes correctly eliminate duplicate target warnings by replacing directory rule dependencies with explicit mkdir -p in recipes. This simplifies the Makefile and follows best practices. However, it introduces minor redundancy—the mkdir -p command appears in five targets (build, build-mcp, test-coverage, dist, and implicitly in any recipe writing to those directories). This is acceptable but could be centralized using a macro or variable if more directories are added.

Code Suggestion:

define ensure_dir
    @mkdir -p $(1)
endef

build:
    $(call ensure_dir,$(BUILD_DIR))
    @echo "Building $(BINARY_NAME) $(VERSION)..."
    $(GO) build $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME) ./cmd/


## build-mcp: Build the MCP server binary
build-mcp: $(BUILD_DIR)
build-mcp:
@mkdir -p $(BUILD_DIR)
@echo "Building $(MCP_SERVER_BINARY)..."
$(GO) build $(LDFLAGS) -o $(BUILD_DIR)/$(MCP_SERVER_BINARY) ./cmd/mcp-server/

$(BUILD_DIR):
mkdir -p $(BUILD_DIR)

## install: Install binaries to GOBIN
install:
Expand All @@ -79,7 +79,8 @@ test-verbose:
$(GO) test -race -cover -v ./...

## test-coverage: Run tests with coverage report
test-coverage: $(BUILD_DIR)
test-coverage:
@mkdir -p $(BUILD_DIR)
@echo "Running tests with coverage..."
$(GO) test -race -coverprofile=$(BUILD_DIR)/coverage.out ./...
$(GO) tool cover -html=$(BUILD_DIR)/coverage.out -o $(BUILD_DIR)/coverage.html
Expand Down Expand Up @@ -124,7 +125,8 @@ run: build-mcp
./$(BUILD_DIR)/$(MCP_SERVER_BINARY)

## dist: Build distribution binaries for all platforms
dist: $(DIST_DIR)
dist:
@mkdir -p $(DIST_DIR)
@echo "Building distribution binaries..."
GOOS=darwin GOARCH=amd64 $(GO) build $(LDFLAGS) -o $(DIST_DIR)/$(BINARY_NAME)-darwin-amd64 ./cmd/
GOOS=darwin GOARCH=arm64 $(GO) build $(LDFLAGS) -o $(DIST_DIR)/$(BINARY_NAME)-darwin-arm64 ./cmd/
Expand All @@ -133,8 +135,6 @@ dist: $(DIST_DIR)
GOOS=windows GOARCH=amd64 $(GO) build $(LDFLAGS) -o $(DIST_DIR)/$(BINARY_NAME)-windows-amd64.exe ./cmd/
@echo "Distribution binaries created in $(DIST_DIR)/"

$(DIST_DIR):
mkdir -p $(DIST_DIR)

## docker: Build Docker image
docker:
Expand Down
16 changes: 8 additions & 8 deletions src/plugin/internal/db/libsql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ func (d *DB) cleanupExpired(ctx context.Context) error {
d.mu.RUnlock()

_, err := d.db.ExecContext(ctx,
`DELETE FROM memories WHERE expires_at IS NOT NULL AND expires_at < datetime('now')`,
`DELETE FROM memories WHERE expires_at IS NOT NULL AND datetime(expires_at) < datetime('now')`,
)
Comment on lines 321 to 323
Copy link
Copy Markdown

@llamapreview llamapreview bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 | Confidence: High

The change wraps expires_at in datetime() for proper RFC3339 string comparison. This fixes a critical logic bug where expiration comparisons were previously performing lexicographic string comparisons, causing incorrect cleanup and retrieval behavior. The cleanupExpired method is called periodically by StartCleanup() (found in related context), meaning this bug affected automatic expiration of all memories system-wide. Without this fix, memories could remain active past their expiration or be prematurely deleted.

return err
}
Expand Down Expand Up @@ -423,7 +423,7 @@ func (d *DB) Retrieve(ctx context.Context, namespace, key string) (*Memory, erro
SELECT id, namespace, key, value, embedding, metadata, tags, created_at, updated_at, ttl, expires_at
FROM memories
WHERE namespace = ? AND key = ?
AND (expires_at IS NULL OR expires_at > datetime('now'))
AND (expires_at IS NULL OR datetime(expires_at) > datetime('now'))
`

row := d.db.QueryRowContext(ctx, query, namespace, key)
Expand All @@ -444,7 +444,7 @@ func (d *DB) RetrieveByID(ctx context.Context, id string) (*Memory, error) {
SELECT id, namespace, key, value, embedding, metadata, tags, created_at, updated_at, ttl, expires_at
FROM memories
WHERE id = ?
AND (expires_at IS NULL OR expires_at > datetime('now'))
AND (expires_at IS NULL OR datetime(expires_at) > datetime('now'))
`

row := d.db.QueryRowContext(ctx, query, id)
Expand Down Expand Up @@ -588,7 +588,7 @@ func (d *DB) List(ctx context.Context, namespace string, limit, offset int) ([]*
SELECT id, namespace, key, value, embedding, metadata, tags, created_at, updated_at, ttl, expires_at
FROM memories
WHERE namespace = ?
AND (expires_at IS NULL OR expires_at > datetime('now'))
AND (expires_at IS NULL OR datetime(expires_at) > datetime('now'))
ORDER BY created_at DESC
LIMIT ? OFFSET ?
Comment on lines 588 to 593
Copy link
Copy Markdown

@llamapreview llamapreview bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 | Confidence: Medium

Speculative: Applying datetime() to the expires_at column in WHERE clauses prevents any existing index on expires_at from being used efficiently. While necessary for correctness, this could degrade query performance as the table grows, especially for List, Search, and Count operations which may scan large datasets. The related context shows Retrieve is called via store.Retrieve in the memory layer, indicating these queries are on the critical path for user operations.

Code Suggestion:

CREATE INDEX idx_memories_expires_at_datetime ON memories(datetime(expires_at));

Evidence: method:Retrieve

`
Expand Down Expand Up @@ -697,7 +697,7 @@ func (d *DB) Search(ctx context.Context, namespace string, queryEmbedding []floa
FROM memories
WHERE namespace = ?
AND embedding IS NOT NULL
AND (expires_at IS NULL OR expires_at > datetime('now'))
AND (expires_at IS NULL OR datetime(expires_at) > datetime('now'))
`

rows, err := d.db.QueryContext(ctx, query, namespace)
Expand Down Expand Up @@ -766,7 +766,7 @@ func (d *DB) SearchAll(ctx context.Context, queryEmbedding []float32, limit int,
SELECT id, namespace, key, value, embedding, metadata, tags, created_at, updated_at, ttl, expires_at
FROM memories
WHERE embedding IS NOT NULL
AND (expires_at IS NULL OR expires_at > datetime('now'))
AND (expires_at IS NULL OR datetime(expires_at) > datetime('now'))
`

rows, err := d.db.QueryContext(ctx, query)
Expand Down Expand Up @@ -821,7 +821,7 @@ func (d *DB) Count(ctx context.Context, namespace string) (int64, error) {

var count int64
err := d.db.QueryRowContext(ctx,
`SELECT COUNT(*) FROM memories WHERE namespace = ? AND (expires_at IS NULL OR expires_at > datetime('now'))`,
`SELECT COUNT(*) FROM memories WHERE namespace = ? AND (expires_at IS NULL OR datetime(expires_at) > datetime('now'))`,
namespace,
).Scan(&count)
Comment on lines 822 to 826
Copy link
Copy Markdown

@llamapreview llamapreview bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 | Confidence: High

The Count and CountAll methods now use the same datetime() wrapping pattern, ensuring consistency. However, this creates a subtle architectural coupling: all expiration logic now depends on SQLite's datetime() function behavior with RFC3339 strings. If the storage format or database engine changes, all eight query locations must be updated in sync. This is a maintainability risk, though mitigated by the new regression test.

if err != nil {
Expand All @@ -843,7 +843,7 @@ func (d *DB) CountAll(ctx context.Context) (int64, error) {

var count int64
err := d.db.QueryRowContext(ctx,
`SELECT COUNT(*) FROM memories WHERE expires_at IS NULL OR expires_at > datetime('now')`,
`SELECT COUNT(*) FROM memories WHERE expires_at IS NULL OR datetime(expires_at) > datetime('now')`,
).Scan(&count)
if err != nil {
return 0, fmt.Errorf("failed to count memories: %w", err)
Expand Down
135 changes: 135 additions & 0 deletions src/plugin/internal/db/libsql_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"os"
"testing"
"time"
)

func TestDBOperations(t *testing.T) {
Expand Down Expand Up @@ -419,3 +420,137 @@ func TestCount(t *testing.T) {
t.Errorf("expected 5 total memories, got %d", countAll)
}
}

func TestExpirationBehavior(t *testing.T) {
// Create temp database
tmpFile, err := os.CreateTemp("", "test-repro-*.db")
if err != nil {
t.Fatalf("failed to create temp file: %v", err)
}
tmpPath := tmpFile.Name()
tmpFile.Close()
defer os.Remove(tmpPath)
defer os.Remove(tmpPath + "-shm")
defer os.Remove(tmpPath + "-wal")

ctx := context.Background()

// Create DB instance
cfg := Config{
Path: tmpPath,
VectorDimensions: 384,
}
db, err := New(ctx, cfg)
if err != nil {
t.Fatalf("failed to create DB: %v", err)
}
defer db.Close()

t.Run("AlreadyExpired", func(t *testing.T) {
negTTL := -1 * time.Hour
mem := &Memory{
ID: "expired-mem",
Namespace: "default",
Key: "expired-key",
Value: "This should be expired",
TTL: &negTTL,
Embedding: make([]float32, 384),
}

if err := db.Store(ctx, mem); err != nil {
t.Fatalf("store failed: %v", err)
}

// Try to Retrieve it. It should NOT be returned.
_, err = db.Retrieve(ctx, "default", "expired-key")
if err != ErrNotFound {
t.Errorf("expected ErrNotFound for expired memory, got: %v", err)
}

// Verify it exists in the DB if we query without expiration check (manual query)
var count int
err = db.db.QueryRowContext(ctx, "SELECT COUNT(*) FROM memories WHERE key = 'expired-key'").Scan(&count)
if err != nil {
t.Fatalf("manual count failed: %v", err)
}
if count != 1 {
t.Errorf("memory should exist in DB but be hidden, count: %d", count)
}

// Run cleanup
if err := db.cleanupExpired(ctx); err != nil {
t.Fatalf("cleanup failed: %v", err)
}

// Verify it is GONE from the DB
err = db.db.QueryRowContext(ctx, "SELECT COUNT(*) FROM memories WHERE key = 'expired-key'").Scan(&count)
if err != nil {
t.Fatalf("manual count failed: %v", err)
}
if count != 0 {
t.Errorf("memory should have been cleaned up, count: %d", count)
}
})

t.Run("ImmediateExpiration", func(t *testing.T) {
// Test precise expiration boundary (small TTL)
// We use a small TTL instead of 0 because 0 might be interpreted as no TTL in some systems (though not this one)
// And we sleep to ensure it expires.
smallTTL := 10 * time.Millisecond
mem := &Memory{
ID: "immediate-mem",
Namespace: "default",
Key: "immediate-key",
Value: "This should expire immediately",
TTL: &smallTTL,
Embedding: make([]float32, 384),
}

if err := db.Store(ctx, mem); err != nil {
t.Fatalf("store failed: %v", err)
}

// Wait for expiration
time.Sleep(20 * time.Millisecond)

// Should be treated as expired
_, err = db.Retrieve(ctx, "default", "immediate-key")
if err != ErrNotFound {
t.Errorf("expected ErrNotFound for expired memory, got: %v", err)
}
})

t.Run("FutureExpiration", func(t *testing.T) {
// Test memory that expires in the future
futureTTL := 1 * time.Hour
mem := &Memory{
ID: "future-mem",
Namespace: "default",
Key: "future-key",
Value: "This should exist",
TTL: &futureTTL,
Embedding: make([]float32, 384),
}

if err := db.Store(ctx, mem); err != nil {
t.Fatalf("store failed: %v", err)
}

// Should be retrievable
_, err := db.Retrieve(ctx, "default", "future-key")
if err != nil {
t.Errorf("expected memory to be found, got error: %v", err)
}

// Run cleanup
if err := db.cleanupExpired(ctx); err != nil {
t.Fatalf("cleanup failed: %v", err)
}

// Should STILL be retrievable
_, err = db.Retrieve(ctx, "default", "future-key")
if err != nil {
t.Errorf("expected memory to persist after cleanup, got error: %v", err)
}
})
}