002 phase 1.1+1.2: Claim carries WorkerID; fencing calls take *Claim - #37
Merged
Conversation
002-worker-store-improvements.md captures the 7-phase plan to reduce the consumer glue-code tax on experimental/worker and experimental/store/postgres. Tracks the work through tagging at v0.1.0. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…es 1.1+1.2) Claim now records the worker that holds the lease, and QueueStore fencing calls (Heartbeat, Complete) take *Claim directly instead of a separate Lease value. Deletes the Lease type entirely, since it added nothing once *Claim became the unit of currency. This lets store implementations access the full run metadata (OrgID, WorkflowType, CreditCost, CallbackURL) during fencing operations without maintaining a separate runID -> metadata cache. Postgres NewCheckpointer also takes *Claim now. Pre-existing: TestStore_* in the postgres submodule were passing NewRun with nil Spec, which violates the NOT NULL spec column. Those tests were latent-broken because CI skips postgres tests without a DSN. Fix them here by supplying an empty JSON spec blob. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaced use of worker.Lease with *worker.Claim across worker and store code paths, added WorkerID to Claim, removed Lease type, and updated checkpointers, queue heartbeat/complete, store implementations (Postgres/SQLite/memstore), tests, Makefile, and added a planning spec. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Worker
end
rect rgba(200,255,200,0.5)
participant QueueStore
end
rect rgba(255,200,200,0.5)
participant Database
end
Worker->>QueueStore: Heartbeat(ctx, claim *Claim)
QueueStore->>Database: UPDATE runs SET claimed_by=claim.ID, attempt=claim.Attempt WHERE id=claim.ID AND claimed_by=claim.WorkerID AND attempt=claim.Attempt
Database-->>QueueStore: RowsAffected (0 => ErrLeaseLost / >0 => OK)
QueueStore-->>Worker: return nil or ErrLeaseLost
Worker->>QueueStore: Complete(ctx, claim *Claim, outcome)
QueueStore->>Database: UPDATE runs SET status=..., claimed_by=claim.ID, attempt=claim.Attempt WHERE id=claim.ID AND claimed_by=claim.WorkerID AND attempt=claim.Attempt
Database-->>QueueStore: RowsAffected (0 => ErrLeaseLost / >0 => OK)
QueueStore-->>Worker: return nil or ErrLeaseLost
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…tal submodules The sqlite submodule (experimental/store/sqlite) was still referencing worker.Lease, which is now deleted. Port its QueueStore impl and NewCheckpointer to the *Claim shape so the module builds again. Also extends make test-all to build/vet/test each experimental submodule. The previous target only covered the root module, so cross-submodule build breaks like this one were only visible once you cd'd into the affected submodule. The postgres target auto-skips its integration tests when WORKFLOW_PG_DSN is unset, so running test-all in an unconfigured environment stays clean. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
planning/specs/002-worker-store-improvements.md (1)
542-546: Add language specifier to fenced code block.The code block for git tag commands is missing a language specifier. Consider adding
bashorshellfor consistency with other code blocks in the document.📝 Suggested fix
-``` +```bash git tag experimental/worker/v0.1.0 git tag experimental/store/postgres/v0.1.0 git push --tags</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@planning/specs/002-worker-store-improvements.mdaround lines 542 - 546, The
fenced code block containing the git commands (lines with "git tag
experimental/worker/v0.1.0", "git tag experimental/store/postgres/v0.1.0", and
"git push --tags") lacks a language specifier; update the opening fence to
include a shell language (e.g., add "bash" or "shell" after the backticks) so
the block becomes a labeled fenced code block for consistent syntax highlighting
with other snippets in the document.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@planning/specs/002-worker-store-improvements.md:
- Around line 542-546: The fenced code block containing the git commands (lines
with "git tag experimental/worker/v0.1.0", "git tag
experimental/store/postgres/v0.1.0", and "git push --tags") lacks a language
specifier; update the opening fence to include a shell language (e.g., add
"bash" or "shell" after the backticks) so the block becomes a labeled fenced
code block for consistent syntax highlighting with other snippets in the
document.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `0b6d4a17-ed85-407c-8c20-8d9505088a1e` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b0300aa3bd49cac2b34e6a850292daadf7988172 and 8aa128429b274ce735762b4119f694de122c88b6. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `experimental/store/postgres/checkpointer.go` * `experimental/store/postgres/queue.go` * `experimental/store/postgres/store_test.go` * `experimental/worker/memstore/memstore.go` * `experimental/worker/queue_store.go` * `experimental/worker/worker.go` * `planning/specs/002-worker-store-improvements.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
experimental/store/sqlite/checkpointer.go (1)
77-86:⚠️ Potential issue | 🟠 MajorUse
executionIDinDeleteCheckpoint(or reject mismatch).
DeleteCheckpointcurrently ignores itsexecutionIDargument and always operates onc.claim.ID, which can hide caller mistakes.Proposed fix
func (c *leasedCheckpointer) DeleteCheckpoint(ctx context.Context, executionID string) error { + if executionID != c.claim.ID { + return fmt.Errorf("sqlite: checkpoint execution ID %q does not match claim run ID %q", + executionID, c.claim.ID) + } result, err := c.store.db.ExecContext(ctx, ` UPDATE workflow_runs SET checkpoint = NULL WHERE id = ? AND claimed_by = ? AND attempt = ? - `, c.claim.ID, c.claim.WorkerID, c.claim.Attempt) + `, executionID, c.claim.WorkerID, c.claim.Attempt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/store/sqlite/checkpointer.go` around lines 77 - 86, DeleteCheckpoint ignores its executionID parameter and always uses c.claim.ID; change DeleteCheckpoint (method on leasedCheckpointer) to use the provided executionID in the SQL WHERE clause (or at minimum validate that executionID == c.claim.ID and return an error if they differ) and update associated error messages to reference the executionID for clarity; locate the method DeleteCheckpoint and adjust the ExecContext parameter and fmt.Errorf call to use executionID (or add an explicit mismatch check at the start of DeleteCheckpoint that returns a clear error when executionID != c.claim.ID).
🧹 Nitpick comments (1)
Makefile (1)
23-25: Consider addingallandcleanphony targets if checkmake is CI-gated.Static analysis reports
minphonywarnings for missing required phony targets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 23 - 25, The Makefile is missing required phony targets causing checkmake minphony warnings; add explicit .PHONY entries for the missing targets (e.g., add "all" and "clean") and implement minimal "all" and "clean" targets (for example, make "all" depend on "test-all" or desired default and "clean" remove build artifacts) and update the existing .PHONY line to include all phony names (include test-all, test, test-experimental, all, clean) so checkmake no longer reports minphony issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@experimental/store/sqlite/store.go`:
- Around line 62-63: NewCheckpointer currently returns a leasedCheckpointer with
a possibly nil claim, causing panics when SaveCheckpoint or DeleteCheckpoint
dereference c.claim; add a nil guard in NewCheckpointer: if claim == nil then
panic (matching New()'s invalid-input behavior) with a clear message, otherwise
return &leasedCheckpointer{store: s, claim: claim}; reference NewCheckpointer
and leasedCheckpointer (and the callers SaveCheckpoint/DeleteCheckpoint) when
applying the change.
---
Outside diff comments:
In `@experimental/store/sqlite/checkpointer.go`:
- Around line 77-86: DeleteCheckpoint ignores its executionID parameter and
always uses c.claim.ID; change DeleteCheckpoint (method on leasedCheckpointer)
to use the provided executionID in the SQL WHERE clause (or at minimum validate
that executionID == c.claim.ID and return an error if they differ) and update
associated error messages to reference the executionID for clarity; locate the
method DeleteCheckpoint and adjust the ExecContext parameter and fmt.Errorf call
to use executionID (or add an explicit mismatch check at the start of
DeleteCheckpoint that returns a clear error when executionID != c.claim.ID).
---
Nitpick comments:
In `@Makefile`:
- Around line 23-25: The Makefile is missing required phony targets causing
checkmake minphony warnings; add explicit .PHONY entries for the missing targets
(e.g., add "all" and "clean") and implement minimal "all" and "clean" targets
(for example, make "all" depend on "test-all" or desired default and "clean"
remove build artifacts) and update the existing .PHONY line to include all phony
names (include test-all, test, test-experimental, all, clean) so checkmake no
longer reports minphony issues.
🪄 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: 0236b2a9-834e-4d84-8e08-dbca8bb3975e
📒 Files selected for processing (4)
Makefileexperimental/store/sqlite/checkpointer.goexperimental/store/sqlite/queue.goexperimental/store/sqlite/store.go
NewCheckpointer now panics on a nil claim instead of deferring the nil-deref to the first Save/DeleteCheckpoint call. DeleteCheckpoint validates that its executionID argument matches the claim's run ID and uses it in both the SQL binding and error messages. Makefile gains conventional all/clean targets with a consolidated .PHONY list. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9 tasks
myzie
added a commit
that referenced
this pull request
Apr 13, 2026
Reflect the changes from PRs #37-#39: richer Claim fields, HandlerContext with the only-Checkpointer-is-fenced caveat, the runquery subpackage, configurable Postgres schema via WithSchema, atomic DeleteRun, the v0.0.3 single-tenant upgrade carry-forward, and the SQLite coexistence note. Lighter tone throughout; replaced the "Going to production" section with a friendlier "Bring your own storage" framing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
myzie
added a commit
that referenced
this pull request
Apr 13, 2026
…40) Reflect the changes from PRs #37-#39: richer Claim fields, HandlerContext with the only-Checkpointer-is-fenced caveat, the runquery subpackage, configurable Postgres schema via WithSchema, atomic DeleteRun, the v0.0.3 single-tenant upgrade carry-forward, and the SQLite coexistence note. Lighter tone throughout; replaced the "Going to production" section with a friendlier "Bring your own storage" framing. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
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.
Summary
Implements Phase 1.1 + 1.2 of
planning/specs/002-worker-store-improvements.md. Also lands the spec itself so subsequent PRs in this chain can reference it on main.Claimgains aWorkerIDfield populated byClaimQueued.QueueStore.HeartbeatandQueueStore.Completetake*Claiminstead of a separateLease. TheLeasetype is deleted.postgres.Store.NewCheckpointernow takes*Claim.Fencing is still
(WorkerID, Attempt)— the change is purely about giving stores full access to run metadata (OrgID, WorkflowType, CreditCost, CallbackURL) without an out-of-band cache. This eliminates a common adapter pattern in consumer code.Open question decisions
Leaseinternally anymore.Drive-by fix
TestStore_*in the postgres submodule were passingworker.NewRun{ID: "..."}with a nilSpec, which violates theNOT NULL speccolumn. The tests were latent-broken because CI skips postgres tests without a DSN. Fixed by supplying an empty JSON spec blob in the affected tests.Test plan
make test-all(root module)go test ./...inexperimental/workerWORKFLOW_PG_DSN=... go test ./...inexperimental/store/postgresagainst a real Postgres🤖 Generated with Claude Code
Summary by CodeRabbit