fix: address remaining review findings — storage bugs, test coverage, and correctness issues#5
Conversation
… and correctness issues Fix bugs across all three storage backends (Memory, SQLite, Postgres): - FindPending now filters by ReadyAt <= now to avoid returning commands not yet ready - SQLite implements QueryableStorage with full Find() method including tag filtering - Postgres Find() supports tag filters via JSONB contains operator - Cleanup includes DEAD_LETTER status in all storage backends - Fix missing argNum++ after CreatedBefore in Postgres Find - Remove duplicate compile-time assertion in sqlite.go Fix correctness issues in core: - CancelByTag now records EventCancelled in command history - ContinueSequence uses deepCopyMap to prevent shared nested references - Stop() returns error on shutdown timeout instead of nil - Workflow example consolidated duplicate Add call Fix documentation: - NewFunc doc corrected to reference executor.Register() Add comprehensive test coverage for previously untested subsystems: - Rate limiter (per-command, global, blocking, stats, concurrency) - HookedStorage (AfterCreate, AfterUpdate, AfterDelete, OnError, nil hooks) - Dead Letter Queue (move to DLQ, FindDeadLettered, ReplayFromDLQ, PurgeDLQ) - Middleware chains (execution order, short-circuit, context passing) - Graceful shutdown (timeout, double-stop, stop before start, add after stop) - AddMany and CancelByTag (partial failure, event recording) - Dashboard API (all HTTP endpoints via httptest) - Stuck command recovery and permanent commands - Polling worker mode (basic execution, shutdown, retry) Update ROADMAP.md to reflect all completed items. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5422b0b921
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if query.Offset > 0 { | ||
| offsetClause = fmt.Sprintf("OFFSET %d", query.Offset) |
There was a problem hiding this comment.
When query.Offset > 0 and query.Limit == 0, this builds SQL with a standalone OFFSET clause (... ORDER BY ... OFFSET n), which SQLite rejects with a syntax error. That makes offset-only pagination fail for the new QueryableStorage implementation, even though the same query shape works in memory/postgres backends. Adding an implicit LIMIT -1 (or requiring Limit when Offset is set) would keep pagination behavior consistent and prevent runtime query failures.
Useful? React with 👍 / 👎.
| for k, v := range i.Data { | ||
| data[k] = v | ||
| // Deep copy data to prevent shared nested references | ||
| data := deepCopyMap(i.Data) |
There was a problem hiding this comment.
Switching ContinueSequence to deepCopyMap causes a JSON marshal/unmarshal round-trip on every chained command payload, which coerces non-JSON-native Go values (for example int64 precision-sensitive numbers, custom types, or time.Time) into different runtime types. This is a regression for in-memory execution paths that previously preserved exact value types across sequence steps, and can break handlers that rely on type assertions or high-precision numeric data.
Useful? React with 👍 / 👎.
…aveat - Guard OFFSET with implicit LIMIT -1 in SQLite Find() to prevent syntax errors when offset is used without an explicit limit - Add doc comment to ContinueSequence noting the JSON round-trip type coercion tradeoff (consistent with Clone() and storage serialization) - Revert ROADMAP.md test coverage checkmarks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Removes the final argNum++ after CreatedBefore in Postgres Find(), which is never read again and flagged by golangci-lint (ineffassign). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
This PR addresses all remaining issues identified in the codebase audit, including critical bugs in storage layer, missing event recording, test coverage gaps, and API correctness issues.
Bugs Fixed
EventCancelledevent in history (executor_api.go)Add(), properly set sequence viaWithSequence()(examples/workflow/main.go)executor.Register()instead of nonexistentexecutor.Handle()(func.go)Stop()now returns error when shutdown times out (executor.go)deepCopyMapfor nested map safety (instance.go)ready_at <= now(storage/*.go)Test Coverage Added
ReplayFromDLQ(),FindDeadLettered(),PurgeDLQ()Refactoring
Impact