Fix memory expiration logic and Makefile warnings#6
Fix memory expiration logic and Makefile warnings#6fakebizprez wants to merge 2 commits intomainfrom
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe pull request modifies build configuration, database expiration handling, and adds expiration behavior testing. Build targets now create output directories inline with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Required label not found on this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/plugin/internal/db/libsql_test.go (1)
466-467: Prefererrors.Isfor error comparison.Using
errors.Is(err, ErrNotFound)is more idiomatic and resilient to future wrapping of the error. The same pattern exists at line 146.Suggested fix
- if err != ErrNotFound { - t.Errorf("expected ErrNotFound for expired memory, got: %v", err) + if !errors.Is(err, ErrNotFound) { + t.Errorf("expected ErrNotFound for expired memory, got: %v", err)This requires adding
"errors"to the import block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/internal/db/libsql_test.go` around lines 466 - 467, Replace direct equality checks against ErrNotFound with errors.Is for robust error comparison: add "errors" to the import block, and change the test assertions that currently do `if err != ErrNotFound { t.Errorf(...) }` to use `if !errors.Is(err, ErrNotFound) { t.Errorf(...) }` for the occurrences around the current check and the earlier similar check near line 146 (both referencing ErrNotFound and calling t.Errorf).src/plugin/internal/db/libsql.go (1)
865-865:ListNamespacesdoes not filter out expired memories.
ListNamespacesstill returns namespaces that may only contain expired (but not yet cleaned-up) memories. This is pre-existing behavior, but worth noting for consistency with the other queries that now properly exclude expired entries.Suggested fix
- rows, err := d.db.QueryContext(ctx, `SELECT DISTINCT namespace FROM memories ORDER BY namespace`) + rows, err := d.db.QueryContext(ctx, `SELECT DISTINCT namespace FROM memories WHERE (expires_at IS NULL OR datetime(expires_at) > datetime('now')) ORDER BY namespace`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/internal/db/libsql.go` at line 865, ListNamespaces currently selects DISTINCT namespace from memories without excluding expired rows; update the SQL in the ListNamespaces function to only return namespaces that have at least one non-expired memory by adding a WHERE clause like "WHERE expiration IS NULL OR expiration > CURRENT_TIMESTAMP" (or "WHERE expiration IS NULL OR expiration > ?" and pass time.Now().UTC() as the parameter) so expired-only namespaces are filtered out; modify the d.db.QueryContext call in ListNamespaces to use the new query and provide the current time parameter if you choose the parameterized form.src/plugin/Makefile (1)
128-136: Consider extracting a helper variable or splitting thedisttarget.The
disttarget body is fairly long (8 recipe lines). You could define a list of platform tuples and loop over them to reduce duplication, though this is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/Makefile` around lines 128 - 136, The dist target is repetitive; define a helper variable (e.g., PLATFORMS = darwin/amd64 darwin/arm64 linux/amd64 linux/arm64 windows/amd64) and replace the eight explicit build lines in the dist recipe with a loop (using make's foreach or a shell for) that iterates PLATFORMS, splits each tuple to GOOS/GOARCH, and invokes $(GO) build with $(LDFLAGS) to output to $(DIST_DIR)/$(BINARY_NAME)-<os>-<arch>[.exe]; alternatively create a per-platform helper target (e.g., build-platform) that accepts GOOS/GOARCH and have dist call it for each tuple, keeping references to DIST_DIR, BINARY_NAME, GO and LDFLAGS unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/plugin/internal/db/libsql_test.go`:
- Around line 466-467: Replace direct equality checks against ErrNotFound with
errors.Is for robust error comparison: add "errors" to the import block, and
change the test assertions that currently do `if err != ErrNotFound {
t.Errorf(...) }` to use `if !errors.Is(err, ErrNotFound) { t.Errorf(...) }` for
the occurrences around the current check and the earlier similar check near line
146 (both referencing ErrNotFound and calling t.Errorf).
In `@src/plugin/internal/db/libsql.go`:
- Line 865: ListNamespaces currently selects DISTINCT namespace from memories
without excluding expired rows; update the SQL in the ListNamespaces function to
only return namespaces that have at least one non-expired memory by adding a
WHERE clause like "WHERE expiration IS NULL OR expiration > CURRENT_TIMESTAMP"
(or "WHERE expiration IS NULL OR expiration > ?" and pass time.Now().UTC() as
the parameter) so expired-only namespaces are filtered out; modify the
d.db.QueryContext call in ListNamespaces to use the new query and provide the
current time parameter if you choose the parameterized form.
In `@src/plugin/Makefile`:
- Around line 128-136: The dist target is repetitive; define a helper variable
(e.g., PLATFORMS = darwin/amd64 darwin/arm64 linux/amd64 linux/arm64
windows/amd64) and replace the eight explicit build lines in the dist recipe
with a loop (using make's foreach or a shell for) that iterates PLATFORMS,
splits each tuple to GOOS/GOARCH, and invokes $(GO) build with $(LDFLAGS) to
output to $(DIST_DIR)/$(BINARY_NAME)-<os>-<arch>[.exe]; alternatively create a
per-platform helper target (e.g., build-platform) that accepts GOOS/GOARCH and
have dist call it for each tuple, keeping references to DIST_DIR, BINARY_NAME,
GO and LDFLAGS unchanged.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR fixes a critical bug in memory expiration logic that prevented correct cleanup and retrieval but introduces potential performance degradation and maintainability concerns that should be addressed.
📄 Documentation Diagram
This diagram documents the fixed memory expiration cleanup workflow.
sequenceDiagram
participant User
participant Scheduler as Cleanup Scheduler
participant DB as Database
User->>Scheduler: Start periodic cleanup
Scheduler->>DB: Call cleanupExpired()
DB->>DB: Execute DELETE query with datetime(expires_at)
note over DB: PR #35;6 fixed datetime() wrapping for correct expiration comparison
DB-->>Scheduler: Return success/error
Scheduler-->>User: Cleanup completed
🌟 Strengths
- Addresses a critical system-wide bug ensuring expired memories are properly handled.
- Adds a new regression test to validate the expiration behavior.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | src/plugin/internal/db/libsql.go | Bug | Fixes expiration comparison bug affecting all memories system-wide. | method:cleanupExpired |
| P2 | src/plugin/internal/db/libsql.go | Performance | datetime() wrapping may degrade query performance by preventing index usage. | method:Retrieve |
| P2 | src/plugin/internal/db/libsql.go | Architecture | Creates coupling to SQLite datetime() function, risking maintainability. | - |
| P2 | src/plugin/internal/db/libsql_test.go | Testing | Missing test coverage for boundary conditions and concurrent cleanup. | - |
| P2 | src/plugin/Makefile | Maintainability | Introduces redundant mkdir -p commands across multiple targets. | - |
🔍 Notable Themes
- Expiration Logic Overhaul: The fix corrects a critical bug but trades off query performance and introduces architectural coupling to SQLite's datetime() function.
- Build System Cleanup: Makefile changes eliminate duplicate target warnings but add minor redundancy that could be centralized for better maintainability.
📈 Risk Diagram
This diagram illustrates the critical expiration comparison bug and its fix.
sequenceDiagram
participant Memory as Memory Entry
participant Check as Expiration Check
participant Cleanup as Cleanup Process
Memory->>Check: Check if expired (with bug)
Check->>Check: Compare expires_at string lexicographically
note over Check: R1(P1): Incorrect comparison leads to wrong cleanup
Check-->>Memory: May be incorrectly kept or deleted
Memory->>Check: Check if expired (fixed)
Check->>Check: Compare datetime(expires_at) with datetime('now')
note over Check: Fixed in PR #35;6: Proper RFC3339 comparison
Check-->>Memory: Correctly identified as expired or not
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: src/plugin/internal/db/libsql_test.go
The new regression test correctly validates the fixed expiration behavior. However, it only tests the negative TTL case. Missing test coverage for: 1) exact boundary condition (expires_at = now), 2) future expiration that should be returned, and 3) interaction with cleanupExpired when called concurrently with retrievals. The test also directly queries the database (db.db.QueryRowContext), which couples it to internal implementation.
Suggestion:
// Test exact expiration boundary
mem.TTL = &zeroTTL // time.Duration(0)
if err := db.Store(ctx, mem); err != nil { t.Fatalf("store failed: %v", err) }
// Should be treated as expired immediately
_, err = db.Retrieve(ctx, "default", mem.Key)
if err != ErrNotFound { t.Errorf("expected ErrNotFound for expired memory, got: %v", err) }Related Code:
func TestExpirationBehavior(t *testing.T) {
// Store a memory that has already expired
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)
}
}💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| _, 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')`, | ||
| ) |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
| build: | ||
| @mkdir -p $(BUILD_DIR) | ||
| @echo "Building $(BINARY_NAME) $(VERSION)..." | ||
| $(GO) build $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME) ./cmd/ |
There was a problem hiding this comment.
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/| 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 ? |
There was a problem hiding this comment.
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
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #7 |
1 participant
src/plugin/internal/db/libsql.goto correctly compareexpires_at(RFC3339) withdatetime('now')by wrapping the column indatetime().src/plugin/Makefileby removing directory rules and usingmkdir -pin recipes.TestExpirationBehaviorinsrc/plugin/internal/db/libsql_test.goto verify expiration logic.PR created automatically by Jules for task 7162277752654284590 started by @fakebizprez
Summary by CodeRabbit