PgStore: implement JobStore with FOR UPDATE SKIP LOCKED #3

New Issue

grenade · 2026-08-07T12:38:45Z

grenade commented

2026-08-07 12:38:45 +00:00

Part of #1. Depends on: Postgres schema and migrations

Goal

Implement tireless_core::port::JobStore for PgStore. This is the persistence
half of stage 1 and the foundation of the claim protocol: PgStore currently
holds a pool and nothing else.

Claiming is a row transition under SELECT … FOR UPDATE SKIP LOCKED
(design.md §4.2), which is what makes it atomic across any number of runners.
Labels mirror this for humans but are never consulted to decide whether a job is
taken — that is invariant 6 in CLAUDE.md.

Only the trait methods are in scope. The behaviour that uses them (a runner
loop, lease-expiry timers, label mirroring) is stage 2.

Files

crates/tireless-data/src/store.rs — the JobStore impl, and the SQL behind
it
crates/tireless-data/src/lib.rs — exports, if new types are needed
crates/tireless-data/Cargo.toml — a dev-dependency for integration tests if
one is chosen

Steps

Implement tracked_repos.
Implement enqueue as an upsert on (forge, owner, repo, number) that skips
issues already having a non-terminal job, returning the count newly enqueued.
Implement claim_next with FOR UPDATE SKIP LOCKED, filtered to the lanes
the caller allows, setting claimed_by and claim_expires_at in the same
transaction as the state transition.
Implement transition, rejecting transitions out of a terminal state —
JobState::is_terminal already says which those are.
Implement expire_stale_claims, returning expired claims to Pending and
returning how many moved.
Test concurrent claiming: two claimers against one pending job must yield one
winner and one None, never the same job twice. This is the property the
whole design rests on and the one most likely to be subtly wrong.

Acceptance

cargo test --workspace
cargo clippy --all-targets --all-features -- -D warnings
A test demonstrates that two concurrent claim_next calls against a single
pending job return it exactly once.
A test demonstrates that a job whose lease has passed is returned to the pool
by expire_stale_claims, and that a job with a live lease is not.

Out of scope

The runner loop, the poller loop, or anything that calls these methods on a
schedule. Stage 2.
Writing labels to the forge. Stage 2.
Retry and circuit-breaker policy — that lives in tireless_core::budget and
is already implemented; do not reimplement any part of it here.
Do not add a "claimed" check that reads a forge label. Invariant 6.

Part of #1. Depends on: Postgres schema and migrations ## Goal Implement `tireless_core::port::JobStore` for `PgStore`. This is the persistence half of stage 1 and the foundation of the claim protocol: `PgStore` currently holds a pool and nothing else. Claiming is a row transition under `SELECT … FOR UPDATE SKIP LOCKED` (design.md §4.2), which is what makes it atomic across any number of runners. Labels mirror this for humans but are never consulted to decide whether a job is taken — that is invariant 6 in `CLAUDE.md`. Only the trait methods are in scope. The *behaviour* that uses them (a runner loop, lease-expiry timers, label mirroring) is stage 2. ## Files - `crates/tireless-data/src/store.rs` — the `JobStore` impl, and the SQL behind it - `crates/tireless-data/src/lib.rs` — exports, if new types are needed - `crates/tireless-data/Cargo.toml` — a dev-dependency for integration tests if one is chosen ## Steps 1. Implement `tracked_repos`. 2. Implement `enqueue` as an upsert on `(forge, owner, repo, number)` that skips issues already having a non-terminal job, returning the count newly enqueued. 3. Implement `claim_next` with `FOR UPDATE SKIP LOCKED`, filtered to the lanes the caller allows, setting `claimed_by` and `claim_expires_at` in the same transaction as the state transition. 4. Implement `transition`, rejecting transitions out of a terminal state — `JobState::is_terminal` already says which those are. 5. Implement `expire_stale_claims`, returning expired claims to `Pending` and returning how many moved. 6. Test concurrent claiming: two claimers against one pending job must yield one winner and one `None`, never the same job twice. This is the property the whole design rests on and the one most likely to be subtly wrong. ## Acceptance - `cargo test --workspace` - `cargo clippy --all-targets --all-features -- -D warnings` - A test demonstrates that two concurrent `claim_next` calls against a single pending job return it exactly once. - A test demonstrates that a job whose lease has passed is returned to the pool by `expire_stale_claims`, and that a job with a live lease is not. ## Out of scope - The runner loop, the poller loop, or anything that calls these methods on a schedule. Stage 2. - Writing labels to the forge. Stage 2. - Retry and circuit-breaker policy — that lives in `tireless_core::budget` and is already implemented; do not reimplement any part of it here. - Do not add a "claimed" check that reads a forge label. Invariant 6.

grenade added the tireless/implement label 2026-08-07 12:40:48 +00:00

grenade referenced this issue

2026-08-07 12:41:14 +00:00

Stage 1 — forge ingestion (read-only) #1

grenade referenced this issue

2026-08-07 15:47:42 +00:00

Postgres schema and migrations #2

grenade referenced this issue from a commit

2026-08-07 17:20:13 +00:00

feat(data): implement JobStore and the Gitea read client

grenade closed this issue

2026-08-07 17:20:13 +00:00

grenade commented

2026-08-07 17:20:32 +00:00

Done in 98f193d, together with #4.

Two spec gaps, both only visible on contact

enqueue(&[DiscoveredIssue]) cannot know a JobKind. DiscoveredIssue
carries labels but nothing says which mode they imply, and the trait takes no
protocol. The store now carries the LabelProtocol and core gained
routing::job_kind_for.

That forced a decision the spec did not mention: what happens when an operator
applies several mode labels, which is easy to do by accident. Precedence is
discover, then plan, then implement — planning beats implementing because a plan
produces the implementation children, so running it first loses nothing, while
the reverse silently discards the decomposition that was also requested.

claim_next(worker, allowed_lanes) had no lane to filter on. The #2 schema
stores kind and parent but not the routing result, and the labels that carry the
tireless/agent:* override are not stored at all — so deriving a lane at claim
time would mean a forge request per claim. Migration 0002_job_lane.sql adds a
lane column, recorded at enqueue by a new routing::lane_for, which route
now delegates to so the two cannot drift. There is a test asserting they agree
across every combination.

The consequence is that the lane is a cache of operator intent, so
refresh_lane exists for the case where someone adds tireless/agent:oc to
something already queued. Without it the override would only work if applied
before the poller first saw the issue.

The claim itself

One statement: a CTE takes the row lock with FOR UPDATE SKIP LOCKED, the
update writes the claim. Select and update share a transaction without anyone
managing one by hand.

Returning a job to pending clears the claim — not because the code remembers
to, but because pending_holds_no_claim from #2 refuses the half-done row. That
is what makes lease expiry safe: a buggy release path fails loudly instead of
stranding an issue with a claim nobody holds.

renew_claim is extra, and needed: a run can outlast the 10-minute lease, and
without renewal the sweeper would hand a still-running job to a second worker.
It is guarded on claimed_by, so a worker cannot renew a claim it already lost.

Verified against Postgres 18

Fifteen database tests, covering the things that are the database's to get right:

enqueue skips issues expressing no work (opted in with no mode label; mode
label without opt-in — the state a discovery proposal sits in); ignores closed
issues; and is a no-op on repeated polls, which matters because a repo polled
every five minutes would otherwise accumulate a job every five minutes;
claiming respects the allowed lanes, including the agent:oc override, and
returns nothing when every lane is held;
a claim records the worker, sets a lease, and counts the attempt at claim
rather than at completion;
an expired lease returns the job and clears the claim, and the retry is
visible in the attempt count;
a delivered job refuses to be transitioned — re-running creates a new job so
history stays true (§4.3);
renewal requires still holding the claim;
relabelling a queued job re-routes it.

Enum values round-trip through serde rather than a hand-written match, so the
schema's check constraints and the Rust types are provably the same vocabulary.

One test from #2 replaced

exactly_one_migration_ships_today failed the moment a second migration was
legitimately added. A test that fails on correct behaviour teaches people to
edit the assertion rather than think, so it now asserts what it was reaching
for: versions unique and ascending, starting at 1.

Done in `98f193d`, together with #4. ## Two spec gaps, both only visible on contact **`enqueue(&[DiscoveredIssue])` cannot know a `JobKind`.** `DiscoveredIssue` carries labels but nothing says which mode they imply, and the trait takes no protocol. The store now carries the `LabelProtocol` and core gained `routing::job_kind_for`. That forced a decision the spec did not mention: **what happens when an operator applies several mode labels**, which is easy to do by accident. Precedence is discover, then plan, then implement — planning beats implementing because a plan *produces* the implementation children, so running it first loses nothing, while the reverse silently discards the decomposition that was also requested. **`claim_next(worker, allowed_lanes)` had no lane to filter on.** The #2 schema stores kind and parent but not the routing result, and the labels that carry the `tireless/agent:*` override are not stored at all — so deriving a lane at claim time would mean a forge request per claim. Migration `0002_job_lane.sql` adds a `lane` column, recorded at enqueue by a new `routing::lane_for`, which `route` now delegates to so the two cannot drift. There is a test asserting they agree across every combination. The consequence is that the lane is a *cache* of operator intent, so `refresh_lane` exists for the case where someone adds `tireless/agent:oc` to something already queued. Without it the override would only work if applied before the poller first saw the issue. ## The claim itself One statement: a CTE takes the row lock with `FOR UPDATE SKIP LOCKED`, the update writes the claim. Select and update share a transaction without anyone managing one by hand. Returning a job to `pending` clears the claim — not because the code remembers to, but because `pending_holds_no_claim` from #2 refuses the half-done row. That is what makes lease expiry safe: a buggy release path fails loudly instead of stranding an issue with a claim nobody holds. `renew_claim` is extra, and needed: a run can outlast the 10-minute lease, and without renewal the sweeper would hand a still-running job to a second worker. It is guarded on `claimed_by`, so a worker cannot renew a claim it already lost. ## Verified against Postgres 18 Fifteen database tests, covering the things that are the database's to get right: - enqueue skips issues expressing no work (opted in with no mode label; mode label without opt-in — the state a discovery proposal sits in); ignores closed issues; and is a no-op on repeated polls, which matters because a repo polled every five minutes would otherwise accumulate a job every five minutes; - claiming respects the allowed lanes, including the `agent:oc` override, and returns nothing when every lane is held; - a claim records the worker, sets a lease, and counts the attempt at claim rather than at completion; - an expired lease returns the job *and* clears the claim, and the retry is visible in the attempt count; - a delivered job refuses to be transitioned — re-running creates a new job so history stays true (§4.3); - renewal requires still holding the claim; - relabelling a queued job re-routes it. Enum values round-trip through serde rather than a hand-written `match`, so the schema's check constraints and the Rust types are provably the same vocabulary. ## One test from #2 replaced `exactly_one_migration_ships_today` failed the moment a second migration was legitimately added. A test that fails on correct behaviour teaches people to edit the assertion rather than think, so it now asserts what it was reaching for: versions unique and ascending, starting at 1.

grenade referenced this issue

2026-08-07 17:20:49 +00:00

Gitea client: conditional issue listing #4

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lair/tireless#3