Implement SQLite workflow persistence - #6
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a SQLite-backed ChangesSQLite Workflow Persistence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/store/file_workflow_store_test.go (1)
9-45: ⚡ Quick winPlease add explicit
GetVersionassertions here.This lifecycle test now exercises context plumbing, but not the newly added
GetVersionAPI. Add an assertion for the intended file-store behavior so interface conformance can’t silently regress.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/file_workflow_store_test.go` around lines 9 - 45, The TestFileWorkflowStorePersistsLifecycle test exercises the Create, Update, and Delete operations but does not test the GetVersion API, which means interface conformance changes could regress silently. Add explicit assertions for the GetVersion API by calling it at key points in the test lifecycle (after the initial Create operation and after the Update operation) and verify that the returned version values are as expected, ensuring the version tracking behavior is properly validated alongside the other store operations.cmd/server/main.go (1)
63-63: ⚡ Quick winCapture and log
Close()errors during shutdown.
defer workflowStore.Close()drops any close error, which can hide SQLite finalization failures.Suggested change
- defer workflowStore.Close() + defer func() { + if err := workflowStore.Close(); err != nil { + log.Printf("close workflow store: %v", err) + } + }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/server/main.go` at line 63, The defer statement for workflowStore.Close() currently ignores any error returned by the Close method, which can hide SQLite finalization failures. Modify the defer statement to capture the error returned by workflowStore.Close() and log it using an appropriate logging mechanism if an error occurs. This ensures that any close-time failures are visible in the logs rather than being silently dropped.internal/store/memory_workflow_store_test.go (1)
11-47: ⚡ Quick winAdd direct coverage for the new versioning contract.
These tests validate CRUD paths, but not
GetVersionor version increments introduced in the memory store. Please add assertions for version 1 after create, version 2 after update, and expected behavior after delete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/memory_workflow_store_test.go` around lines 11 - 47, The tests TestMemoryWorkflowStoreLifecycle and TestMemoryWorkflowStoreUpdateMissing validate CRUD operations but do not assert on the versioning contract. Add assertions in TestMemoryWorkflowStoreLifecycle to verify that GetVersion returns 1 after the initial Create call, returns 2 after the Update call, and verify the expected behavior when calling GetVersion after the Delete call. If versioning applies to the UpdateMissing scenario, add similar version assertions to TestMemoryWorkflowStoreUpdateMissing as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/store/file_workflow_store.go`:
- Around line 45-59: The GetVersion method in FileWorkflowStore incorrectly
returns the current workflow content while claiming it is version 1, which
violates the versioning contract. Since this file-based store cannot maintain
historical versions (it only stores the latest state), modify the method to
return a consistent error (such as ErrWorkflowNotFound) for all version requests
instead of fabricating version data. Remove the logic that reads the current
workflow and returns it as version 1, and ensure the method either accepts only
a specific version number that indicates "current" or rejects all version
requests with an appropriate error that reflects the store's inability to
support versioning.
In `@internal/store/memory_workflow_store.go`:
- Around line 100-109: In the appendVersionLocked method of MemoryWorkflowStore,
the Workflow field is storing a shallow copy of the workflow parameter, which
allows mutations to reference types (maps/slices) within kernel.WorkflowResource
to retroactively modify historical versions. Create a deep copy of the workflow
parameter before storing it in the WorkflowVersion struct to ensure version
history immutability. This deep copy should be created before the append
operation and assigned to the Workflow field in the WorkflowVersion
initialization.
In `@internal/store/sqlite/workflow_store.go`:
- Around line 41-43: The db.Close() call on line 42 in the migration error
handling path is not checking for errors, which violates error handling best
practices and triggers errcheck warnings. Modify the error handling to capture
the error returned by db.Close() when workflowStore.Migrate fails, and either
log the close error or wrap it together with the original migration error before
returning, ensuring cleanup failures are not silently ignored.
---
Nitpick comments:
In `@cmd/server/main.go`:
- Line 63: The defer statement for workflowStore.Close() currently ignores any
error returned by the Close method, which can hide SQLite finalization failures.
Modify the defer statement to capture the error returned by
workflowStore.Close() and log it using an appropriate logging mechanism if an
error occurs. This ensures that any close-time failures are visible in the logs
rather than being silently dropped.
In `@internal/store/file_workflow_store_test.go`:
- Around line 9-45: The TestFileWorkflowStorePersistsLifecycle test exercises
the Create, Update, and Delete operations but does not test the GetVersion API,
which means interface conformance changes could regress silently. Add explicit
assertions for the GetVersion API by calling it at key points in the test
lifecycle (after the initial Create operation and after the Update operation)
and verify that the returned version values are as expected, ensuring the
version tracking behavior is properly validated alongside the other store
operations.
In `@internal/store/memory_workflow_store_test.go`:
- Around line 11-47: The tests TestMemoryWorkflowStoreLifecycle and
TestMemoryWorkflowStoreUpdateMissing validate CRUD operations but do not assert
on the versioning contract. Add assertions in TestMemoryWorkflowStoreLifecycle
to verify that GetVersion returns 1 after the initial Create call, returns 2
after the Update call, and verify the expected behavior when calling GetVersion
after the Delete call. If versioning applies to the UpdateMissing scenario, add
similar version assertions to TestMemoryWorkflowStoreUpdateMissing as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5bebd80-d69e-431d-9395-246e6b070430
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
README.mdcmd/server/main.godocs/architecture.mddocs/development.mdgo.modinternal/api/telegram_webhook.gointernal/api/workflows.gointernal/store/file_workflow_store.gointernal/store/file_workflow_store_test.gointernal/store/memory_workflow_store.gointernal/store/memory_workflow_store_test.gointernal/store/sqlite/doc.gointernal/store/sqlite/workflow_store.gointernal/store/sqlite/workflow_store_test.gointernal/store/workflow_store.go
Comment on lines
+45
to
+59
| func (s *FileWorkflowStore) GetVersion(ctx context.Context, name string, version int) (WorkflowVersion, error) { | ||
| workflow, err := s.Get(ctx, name) | ||
| if err != nil { | ||
| return WorkflowVersion{}, err | ||
| } | ||
| if version != 1 { | ||
| return WorkflowVersion{}, ErrWorkflowNotFound | ||
| } | ||
| return WorkflowVersion{ | ||
| Name: name, | ||
| Version: 1, | ||
| Workflow: workflow, | ||
| Metadata: workflow.Metadata, | ||
| CreatedAt: time.Time{}, | ||
| }, nil |
Contributor
There was a problem hiding this comment.
GetVersion fabricates version semantics for updated workflows.
Line 46 reads the current file, and Lines 55-56 hardcode that payload as version 1. After any Update, GetVersion(..., 1) returns the latest content, not historical version 1, which violates the versioned-store contract. If this backend cannot provide history, return a consistent “not found/unsupported” error instead of incorrect version data.
Suggested minimal safe fallback
-func (s *FileWorkflowStore) GetVersion(ctx context.Context, name string, version int) (WorkflowVersion, error) {
- workflow, err := s.Get(ctx, name)
- if err != nil {
- return WorkflowVersion{}, err
- }
- if version != 1 {
- return WorkflowVersion{}, ErrWorkflowNotFound
- }
- return WorkflowVersion{
- Name: name,
- Version: 1,
- Workflow: workflow,
- Metadata: workflow.Metadata,
- CreatedAt: time.Time{},
- }, nil
-}
+func (s *FileWorkflowStore) GetVersion(_ context.Context, _ string, _ int) (WorkflowVersion, error) {
+ return WorkflowVersion{}, ErrWorkflowNotFound
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *FileWorkflowStore) GetVersion(ctx context.Context, name string, version int) (WorkflowVersion, error) { | |
| workflow, err := s.Get(ctx, name) | |
| if err != nil { | |
| return WorkflowVersion{}, err | |
| } | |
| if version != 1 { | |
| return WorkflowVersion{}, ErrWorkflowNotFound | |
| } | |
| return WorkflowVersion{ | |
| Name: name, | |
| Version: 1, | |
| Workflow: workflow, | |
| Metadata: workflow.Metadata, | |
| CreatedAt: time.Time{}, | |
| }, nil | |
| func (s *FileWorkflowStore) GetVersion(_ context.Context, _ string, _ int) (WorkflowVersion, error) { | |
| return WorkflowVersion{}, ErrWorkflowNotFound | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/file_workflow_store.go` around lines 45 - 59, The GetVersion
method in FileWorkflowStore incorrectly returns the current workflow content
while claiming it is version 1, which violates the versioning contract. Since
this file-based store cannot maintain historical versions (it only stores the
latest state), modify the method to return a consistent error (such as
ErrWorkflowNotFound) for all version requests instead of fabricating version
data. Remove the logic that reads the current workflow and returns it as version
1, and ensure the method either accepts only a specific version number that
indicates "current" or rejects all version requests with an appropriate error
that reflects the store's inability to support versioning.
Comment on lines
+100
to
+109
| func (s *MemoryWorkflowStore) appendVersionLocked(workflow kernel.WorkflowResource) { | ||
| name := workflow.Metadata.Name | ||
| version := len(s.versions[name]) + 1 | ||
| s.versions[name] = append(s.versions[name], WorkflowVersion{ | ||
| Name: name, | ||
| Version: version, | ||
| Workflow: workflow, | ||
| Metadata: workflow.Metadata, | ||
| CreatedAt: time.Now().UTC(), | ||
| }) |
Contributor
There was a problem hiding this comment.
Version snapshots are currently stored as mutable aliases.
On Line 106, Workflow: workflow stores a shallow copy. If kernel.WorkflowResource contains reference fields (maps/slices), later caller-side mutations can retroactively change historical versions returned by GetVersion. Capture a deep copy before persisting both head/version entries to keep version history immutable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/memory_workflow_store.go` around lines 100 - 109, In the
appendVersionLocked method of MemoryWorkflowStore, the Workflow field is storing
a shallow copy of the workflow parameter, which allows mutations to reference
types (maps/slices) within kernel.WorkflowResource to retroactively modify
historical versions. Create a deep copy of the workflow parameter before storing
it in the WorkflowVersion struct to ensure version history immutability. This
deep copy should be created before the append operation and assigned to the
Workflow field in the WorkflowVersion initialization.
Comment on lines
+41
to
+43
| if err := workflowStore.Migrate(ctx); err != nil { | ||
| db.Close() | ||
| return nil, err |
Contributor
There was a problem hiding this comment.
Handle database close errors on migration failure.
Line 42 ignores db.Close() errors, which is flagged by errcheck and can hide cleanup failures on startup error paths.
Suggested fix
workflowStore := &WorkflowStore{db: db}
if err := workflowStore.Migrate(ctx); err != nil {
- db.Close()
- return nil, err
+ if closeErr := db.Close(); closeErr != nil {
+ return nil, fmt.Errorf("migrate sqlite workflow store: %v (close db: %w)", err, closeErr)
+ }
+ return nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := workflowStore.Migrate(ctx); err != nil { | |
| db.Close() | |
| return nil, err | |
| if err := workflowStore.Migrate(ctx); err != nil { | |
| if closeErr := db.Close(); closeErr != nil { | |
| return nil, fmt.Errorf("migrate sqlite workflow store: %v (close db: %w)", err, closeErr) | |
| } | |
| return nil, err | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 42-42: Error return value of db.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/sqlite/workflow_store.go` around lines 41 - 43, The db.Close()
call on line 42 in the migration error handling path is not checking for errors,
which violates error handling best practices and triggers errcheck warnings.
Modify the error handling to capture the error returned by db.Close() when
workflowStore.Migrate fails, and either log the close error or wrap it together
with the original migration error before returning, ensuring cleanup failures
are not silently ignored.
Source: Linters/SAST tools
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
1 participant
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.
Implementation summary
internal/store/sqlite.--data-dir/flowforge.db.context.Context.Architecture decisions
internal/store/sqlite; kernel packages do not import SQLite.store.WorkflowStoreboundary and addedGetVersionplusWorkflowVersionas storage-level concepts.N+1in the same transaction that advances the workflow head.Tests executed
GOCACHE=/tmp/flowforge-go-build go test ./...GOCACHE=/tmp/flowforge-go-build go vet ./...Known limitations
github.com/mattn/go-sqlite3, so builds require CGO support.Migration notes
--data-dir/flowforge.db.--data-dir/workflowsare not migrated automatically.Summary by CodeRabbit
New Features
Documentation