fix: PR #38 follow-ups — upgrade path, DeleteRun race, runquery lift

[myzie]

Summary

Addresses the critical review on #38. Stacked on top of feat/worker-pgstore-v2 so it merges once the base PR ships.

Must-fix

• schema.sql upgrade path: add UPDATE workflow_runs SET org_id/initiated_by = NULL WHERE = '' so pre-existing v0.0.3 single-tenant rows stay visible to the new read API after Migrate(). Also drop NOT NULL on initiated_by (was only dropped on org_id), and rewrite the upgrade comment to name the v0.0.3 → v0.0.4 boundary explicitly.
• DeleteRun TOCTOU: collapse into a single DELETE … WHERE status <> 'running' RETURNING status. The destructive path is now one atomic statement; a follow-up probe only fires to pick between ErrRunNotFound and ErrCannotDeleteRunning for the error message, never to gate the delete.

Should-fix

• Lift run types to a neutral package: new experimental/worker/runquery (stdlib-only) holds Run, RunFilter, RunCursor, ErrRunNotFound, ErrCannotDeleteRunning, and the Store interface. Postgres aliases the types and satisfies runquery.Store at compile time. Dashboards can now depend on runquery instead of importing experimental/store/postgres.
• sqlite ReclaimStale parity: also reset started_at = NULL, matching Postgres.
• HandlerStores / HandlerContext godoc: make it explicit that only Checkpointer is lease-fenced. ProgressStore / ActivityLogger accept *Claim for API symmetry and typically ignore it; SignalStore is shared across claims and intentionally lives on Config, not the factory.

Nice-to-have

• Rename QueueStore.ListFailedWithCredits → ListRefundPending across worker, memstore, postgres, sqlite, and the integration test.
• sqlite coexistence doc: spell out that SQLite has no schema namespacing escape hatch; consumers who need coexistence should hand the library a dedicated *sql.DB.

New tests

• TestStore_UpgradeFromV003 — seeds a v0.0.3-shaped workflow_runs table (org_id NOT NULL DEFAULT '', no project_id / parent_run_id / metadata), inserts a populated row, runs Migrate, and asserts the row is still findable via GetRun / ListRuns. This catches the exact regression the review flagged.
• TestStore_ClaimQueuedConcurrent — 8 goroutines race to claim a 20-run queue; every run must be claimed exactly once across all workers.

Test plan

• make test-all at repo root — all modules build, vet clean, existing tests pass
• go vet cleanly across root, experimental/worker, experimental/store/postgres, experimental/store/sqlite
• Run postgres integration tests against a real DB: WORKFLOW_PG_DSN=… go test ./experimental/store/postgres/… — new TestStore_UpgradeFromV003 and TestStore_ClaimQueuedConcurrent in particular

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• New Features

  • Introduced a new workflow run query package providing a unified, backend-neutral API for accessing run data across different storage implementations.

• Improvements

  • Enhanced refund handling with improved method naming and consistency across storage backends.
  • Improved schema migration support for multi-tenant compatibility.
  • Added clarification on SQLite table coexistence strategies and handler store lease-fencing behavior.

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

This PR introduces a backend-neutral read API package (runquery) with types for querying workflow runs, refactors PostgreSQL stores to use these types via aliasing, renames the ListFailedWithCredits method to ListRefundPending across all store implementations, updates the v0.0.3 → v0.0.4 schema migration to handle initiated_by field nullification, and adds integration tests for concurrent claiming and schema upgrades.

Changes

Cohort / File(s)	Summary
New runquery package experimental/worker/runquery/runquery.go	Introduces backend-neutral read API: exported sentinel errors (ErrRunNotFound, ErrCannotDeleteRunning), data types (Run, RunFilter, RunCursor), and Store interface with GetRun, ListRuns, CountRuns, DeleteRun methods.
Queue method renaming experimental/store/postgres/queue.go, experimental/store/sqlite/queue.go, experimental/worker/memstore/memstore.go, experimental/worker/queue_store.go	Renamed ListFailedWithCredits to ListRefundPending with updated documentation; memstore implementation changed sorting/limiting logic to sort by completedAt ascending before applying limit.
Postgres runs refactoring experimental/store/postgres/runs.go	Converted local Run, RunFilter, RunCursor types and ErrRunNotFound, ErrCannotDeleteRunning errors to aliases of runquery types; refactored DeleteRun to use atomic DELETE ... RETURNING pattern; extracted common SQL column projection into runProjection constant.
Postgres integration tests experimental/store/postgres/integration_test.go	Renamed TestStore_ListFailedWithCredits to TestStore_ListRefundPending; added TestStore_UpgradeFromV003 to verify schema migration preserves legacy single-tenant runs; added TestStore_ClaimQueuedConcurrent to assert concurrent claim exclusivity.
Schema migration experimental/store/postgres/schema.sql	Updated v0.0.3 → v0.0.4 migration to drop NOT NULL constraint and default for initiated_by, with UPDATE statements converting empty string rows to NULL.
Worker subsystems experimental/worker/subsystems.go, experimental/worker/credits.go	Updated reconcileCredits to call ListRefundPending instead of ListFailedWithCredits; updated CreditStore documentation comment.
Documentation and configuration experimental/worker/handler.go, experimental/store/sqlite/store.go	Clarified lease-fencing behavior for handler stores; documented SQLite coexistence strategy with consumer tables and ATTACH DATABASE guidance.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat: worker + postgres submodules and user guides #33: Introduces the initial ListRefundPending method rename across queue store implementations, which this PR extends to the full system refactor including runquery integration.
• feat: richer NewRun/Claim, HandlerContext, Run API, configurable schema #38: Likely builds on or precedes runquery package integration, involving parallel updates to postgres run storage logic.

Poem

🐰 A rabbit hops through schemas neat,
Refactoring runs, a tidy feat!
From credits lost to pending refunds bright,
New queries shine with runquery light. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the PR's main changes: addressing follow-ups from PR #38 with three key areas (upgrade path, DeleteRun race condition fix, and runquery abstraction).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pr38-followups

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}