diff --git a/CLAUDE.md b/CLAUDE.md index 26bca60..9c0a51e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -88,6 +88,25 @@ cd dashboard && npm run lint && npm run build This exact block is what a plan targeting this repo should use as its runnable `Acceptance` (design.md §10.3). +**Touching `crates/tireless-data/migrations/` or any SQL also means running the +database tests**, which the gate above deliberately skips — CI has no Postgres, +so they are `#[ignore]`d rather than silently passing: + +```sh +podman run -d --name pg -e POSTGRES_PASSWORD=test -e POSTGRES_DB=tireless_test \ + -p 55432:5432 docker.io/library/postgres:18-alpine +export TIRELESS_TEST_DATABASE_URL=postgres://postgres:test@127.0.0.1:55432/tireless_test +cargo test -p tireless-data -- --ignored +``` + +Queries are checked at runtime, not compile time — there is no `.sqlx` offline +metadata and no `DATABASE_URL` needed to build. `store.rs` says why. The +consequence is that these tests are the only thing standing between a malformed +query and production. + +**Never edit an applied migration.** sqlx records a checksum per version, so an +edit makes every deployed database refuse to start. Add a new numbered file. + ## Commits Conventional Commits (`type(scope): subject`), imperative, under ~70 chars. diff --git a/Cargo.lock b/Cargo.lock index 896c0ed..f1c0959 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2309,6 +2309,7 @@ dependencies = [ "thiserror", "tireless-core", "tireless-entities", + "tokio", "tracing", "url", "uuid", diff --git a/crates/tireless-data/Cargo.toml b/crates/tireless-data/Cargo.toml index 195819f..7b31734 100644 --- a/crates/tireless-data/Cargo.toml +++ b/crates/tireless-data/Cargo.toml @@ -21,3 +21,6 @@ thiserror = { workspace = true } tracing = { workspace = true } url = { workspace = true } uuid = { workspace = true } + +[dev-dependencies] +tokio = { workspace = true } diff --git a/crates/tireless-data/migrations/0001_init.sql b/crates/tireless-data/migrations/0001_init.sql new file mode 100644 index 0000000..f280018 --- /dev/null +++ b/crates/tireless-data/migrations/0001_init.sql @@ -0,0 +1,187 @@ +-- tireless initial schema. +-- +-- Three tables mirroring `tireless-entities`: the repos we watch, the work +-- claimed against their issues, and each agent invocation. Enums are stored as +-- text with a check constraint rather than as Postgres enum types: adding a +-- `JobKind` variant should be an ordinary migration, not an ALTER TYPE that +-- takes a lock on every table using it. The accepted values must match what +-- serde emits for each enum, and a test in `tireless-data` asserts they do. + +-- --------------------------------------------------------------------------- +-- Repos tireless polls. +-- --------------------------------------------------------------------------- +create table tracked_repo ( + id uuid primary key default gen_random_uuid(), + + forge text not null check (forge in ('gitea', 'github')), + owner text not null, + repo text not null, + clone_url text not null, + default_branch text not null, + + -- PollSchedule, flattened. The floor lives in config, not here: it is an + -- operator policy that can change, and clamping is core's job + -- (`Poll::clamp`). The database asserts only that the value is sane. + interval_seconds integer not null check (interval_seconds > 0), + quiet_from text, + quiet_until text, + enabled boolean not null default true, + + last_etag text, + last_polled_at timestamptz, + created_at timestamptz not null default now(), + + unique (forge, owner, repo), + + -- Both ends of the quiet window or neither. A half-specified window runs + -- around the clock while the operator believes otherwise, which is silently + -- wrong rather than loudly broken. `Config::validate` enforces the same rule + -- on the global window. + constraint quiet_window_complete + check ((quiet_from is null) = (quiet_until is null)) +); + +-- --------------------------------------------------------------------------- +-- One unit of work against one issue. +-- --------------------------------------------------------------------------- +create table job ( + id uuid primary key default gen_random_uuid(), + + -- IssueRef, flattened. + forge text not null check (forge in ('gitea', 'github')), + owner text not null, + repo text not null, + number bigint not null, + + kind text not null + check (kind in ('discover', 'plan', 'implement')), + state text not null + check (state in ('pending', 'claimed', 'running', + 'delivered', 'blocked', 'failed', 'abandoned')), + + claimed_by text, + claim_expires_at timestamptz, + + -- Set when this job's issue was produced by a Plan job. Presence of a parent + -- is what routes implementation to OpenCode, and what lets a child inherit + -- admission (design.md §2.5). + parent_job_id uuid references job (id), + + attempts integer not null default 0 check (attempts >= 0), + last_error text, + + -- When the poller last reconciled this job's forge labels. Labels mirror + -- this row; they are never read to make a decision (CLAUDE.md invariant 6). + -- The runner cannot write labels at all — a separate identity does it + -- (design.md §6.4) — so this column is how the two halves stay in step. + labels_synced_at timestamptz, + + created_at timestamptz not null default now(), + updated_at timestamptz not null default now(), + + -- A claim is a pair. Half a claim is a job that either cannot be released or + -- cannot be found. + constraint claim_complete + check ((claimed_by is null) = (claim_expires_at is null)), + + -- A pending job holds no claim. This is what makes lease expiry correct: + -- returning a job to the pool *must* clear its claim, and the database + -- refuses the half-done version. + -- + -- Deliberately not the converse. Terminal jobs keep `claimed_by` as an audit + -- trail of which worker delivered them. + constraint pending_holds_no_claim + check (state <> 'pending' or claimed_by is null) +); + +-- At most one *live* job per issue, and any number of historical ones. +-- +-- A plain unique constraint would be wrong in both directions. It would forbid +-- re-running a job after it reached a terminal state, which `tireless job run` +-- exists to do; and it would forbid the discovery lane entirely, since discovery +-- recurs against the same tracking issue on a cooldown (design.md §2.6). +create unique index job_live_issue_idx + on job (forge, owner, repo, number) + where state in ('pending', 'claimed', 'running'); + +-- The claim query: oldest pending job of an allowed kind. +-- +-- Ordered by `created_at` alone, with `kind` left as a filter, because the claim +-- takes `LIMIT 1` and can then stop at the first match. Leading with `kind` +-- instead forces a sort of every pending row on every claim — measured at 720 +-- buffers versus 4, and the gap grows with the backlog rather than staying +-- fixed. Partial on `pending` because that is a small slice of a table that only +-- grows. +-- +-- No INCLUDE (kind): `FOR UPDATE` has to visit the heap regardless, so the extra +-- index payload measured as no gain. +create index job_claimable_idx + on job (created_at) + where state = 'pending'; + +-- Lease expiry sweeps held claims looking for passed deadlines. +create index job_claim_expiry_idx + on job (claim_expires_at) + where state in ('claimed', 'running'); + +-- History for one issue: powers the discovery cooldown ("when did a discover job +-- for this issue last finish?") and the operator asking what has been tried. +create index job_issue_history_idx + on job (forge, owner, repo, number, kind, created_at desc); + +-- --------------------------------------------------------------------------- +-- One invocation of one agent against one job. +-- +-- A job may have several: a retry after a rate limit, or a follow-up turn +-- resuming the same agent session. +-- --------------------------------------------------------------------------- +create table agent_run ( + id uuid primary key default gen_random_uuid(), + job_id uuid not null references job (id) on delete cascade, + + agent text not null + check (agent in ('claude_code', 'opencode')), + model text, + + -- Read from Claude Code's own `apiKeySource`, so the dashboard reports what + -- actually happened rather than what was intended (design.md §3.2). + billing text not null default 'unknown' + check (billing in ('subscription', 'api_key', 'unknown')), + + session_id text, + + outcome text + check (outcome in ('succeeded', 'failed', 'rate_limited', + 'budget_exhausted', 'output_budget_exhausted', + 'timed_out', 'cancelled')), + + -- PullRequestRef, flattened. All six or none. + pr_forge text check (pr_forge in ('gitea', 'github')), + pr_owner text, + pr_repo text, + pr_number bigint, + pr_head_branch text, + pr_url text, + + started_at timestamptz not null default now(), + finished_at timestamptz, + + constraint pull_request_complete + check (num_nonnulls(pr_forge, pr_owner, pr_repo, + pr_number, pr_head_branch, pr_url) in (0, 6)), + + -- A finished run has an outcome and an unfinished one does not. Without + -- this, a crashed writer leaves runs that look in-flight forever and the + -- governor counts them against concurrency. + constraint finished_has_outcome + check ((finished_at is null) = (outcome is null)) +); + +create index agent_run_job_idx + on agent_run (job_id, started_at desc); + +-- The governor's window budget: runs started per lane inside a rolling window +-- (design.md §5). This is asked on every admission decision, so it is the one +-- index that has to be right from the start. +create index agent_run_lane_window_idx + on agent_run (agent, started_at desc); diff --git a/crates/tireless-data/src/store.rs b/crates/tireless-data/src/store.rs index ba5bf9f..687b92d 100644 --- a/crates/tireless-data/src/store.rs +++ b/crates/tireless-data/src/store.rs @@ -4,8 +4,31 @@ //! `architecture/generic.md` §3, which makes the claim atomic across however //! many workers are running. Forge labels mirror this state for humans but are //! never consulted to decide whether a job is already taken. +//! +//! ## Queries are checked at runtime, not compile time +//! +//! `sqlx` can verify SQL at compile time with `query!`, against either a live +//! `DATABASE_URL` or committed `.sqlx` offline metadata. tireless uses neither, +//! and builds its queries with the runtime `sqlx::query` family — matching +//! `lairball`, the other house project on this cluster. +//! +//! The deciding argument is specific to what tireless is *for*. Compile-time +//! checking makes a database a build dependency: regenerating `.sqlx` after any +//! query change needs a live Postgres, and CI needs `cargo sqlx prepare --check` +//! plus the discipline to keep the metadata fresh. tireless expects a 27B model +//! to implement issues against this crate unattended (design.md §2.4). A build +//! that fails without a database it cannot provision is a build that model +//! cannot fix, and its documented failure mode is to improvise. +//! +//! The cost is real and worth naming: a malformed query is found by a test +//! against a real database rather than by `cargo build`. That is why the schema +//! tests below assert the parts a type system would otherwise have caught. use sqlx::PgPool; +use sqlx::migrate::Migrator; + +/// Migrations, embedded at compile time from `migrations/`. +pub static MIGRATOR: Migrator = sqlx::migrate!("./migrations"); pub struct PgStore { #[allow(dead_code)] @@ -16,7 +39,181 @@ impl PgStore { pub fn new(pool: PgPool) -> Self { Self { pool } } + + /// Apply any outstanding migrations. + /// + /// Run at startup rather than by a deploy step: the binary and the schema it + /// expects ship together, so there is no window where a new binary is live + /// against an old schema. `sqlx` records what it has applied, so this is a + /// no-op on an up-to-date database. + pub async fn migrate(&self) -> Result<(), sqlx::migrate::MigrateError> { + MIGRATOR.run(&self.pool).await + } + + pub fn pool(&self) -> &PgPool { + &self.pool + } } -// The `JobStore` impl lands in stage 1; the migrations that back it live in -// `crates/tireless-data/migrations/`. See doc/plan/design.md §7. +// The `JobStore` impl lands with the next child of this epic. See +// doc/plan/design.md §7. + +#[cfg(test)] +mod tests { + use super::*; + use tireless_entities::{AgentKind, BillingMode, Forge, JobKind, JobState, RunOutcome}; + + /// The migration source, for assertions about the schema itself. + const INIT_SQL: &str = include_str!("../migrations/0001_init.sql"); + + /// What serde writes for a value — which is exactly what will be stored. + fn wire(v: &T) -> String { + serde_json::to_value(v) + .expect("serialize") + .as_str() + .expect("enum serialises to a string") + .to_string() + } + + /// Assert a check constraint accepts every variant of an enum. + /// + /// This is the guard that a type system would otherwise provide. Adding a + /// `JobKind` variant without adding it to the schema would otherwise fail at + /// runtime, on a live database, as a constraint violation on the first job + /// of that kind — which is to say, in production, unattended. + fn assert_accepts(column: &str, values: &[String]) { + for v in values { + assert!( + INIT_SQL.contains(&format!("'{v}'")), + "schema has no check-constraint value {v:?} for column {column:?}; \ + add it to migrations/ (a new migration, not an edit to 0001)" + ); + } + } + + #[test] + fn the_schema_accepts_every_forge() { + assert_accepts("forge", &[wire(&Forge::Gitea), wire(&Forge::GitHub)]); + } + + #[test] + fn github_is_not_spelled_git_hub() { + // `rename_all = "snake_case"` would make it `git_hub`, and that spelling + // would be permanent across the database, the API and the TS bindings. + assert_eq!(wire(&Forge::GitHub), "github"); + } + + #[test] + fn the_schema_accepts_every_job_kind() { + let all = [JobKind::Discover, JobKind::Plan, JobKind::Implement]; + assert_accepts("kind", &all.iter().map(wire).collect::>()); + } + + #[test] + fn the_schema_accepts_every_job_state() { + let all = [ + JobState::Pending, + JobState::Claimed, + JobState::Running, + JobState::Delivered, + JobState::Blocked, + JobState::Failed, + JobState::Abandoned, + ]; + assert_accepts("state", &all.iter().map(wire).collect::>()); + } + + #[test] + fn the_schema_accepts_every_agent_and_billing_mode() { + assert_accepts( + "agent", + &[wire(&AgentKind::ClaudeCode), wire(&AgentKind::Opencode)], + ); + assert_accepts( + "billing", + &[ + wire(&BillingMode::Subscription), + wire(&BillingMode::ApiKey), + wire(&BillingMode::Unknown), + ], + ); + } + + #[test] + fn the_schema_accepts_every_run_outcome() { + let all = [ + RunOutcome::Succeeded, + RunOutcome::Failed, + RunOutcome::RateLimited, + RunOutcome::BudgetExhausted, + RunOutcome::OutputBudgetExhausted, + RunOutcome::TimedOut, + RunOutcome::Cancelled, + ]; + assert_accepts("outcome", &all.iter().map(wire).collect::>()); + } + + #[test] + fn the_live_issue_index_is_partial() { + // A plain unique constraint on (forge, owner, repo, number) would forbid + // re-running a terminal job and would forbid the discovery lane outright, + // since discovery recurs against one tracking issue. Both failures are + // "tireless quietly stops doing something", so pin the shape. + assert!(INIT_SQL.contains("create unique index job_live_issue_idx")); + let idx = INIT_SQL + .split("job_live_issue_idx") + .nth(1) + .expect("index present"); + let body = &idx[..idx.find(';').expect("statement ends")]; + assert!( + body.contains("where state in"), + "job_live_issue_idx must be partial, or history and recurrence break" + ); + for terminal in ["delivered", "blocked", "failed", "abandoned"] { + assert!( + !body.contains(terminal), + "terminal state {terminal:?} must not be covered by the live-issue index" + ); + } + } + + #[test] + fn terminal_states_agree_with_the_entity_definition() { + // The partial index above hard-codes which states count as live. If + // `JobState::is_terminal` and that list ever disagree, tireless either + // refuses to enqueue work it should, or enqueues duplicates it should not. + let live: Vec = [JobState::Pending, JobState::Claimed, JobState::Running] + .iter() + .map(wire) + .collect(); + for s in &live { + assert!( + INIT_SQL.contains(&format!("'{s}'")), + "live state {s:?} missing from the schema" + ); + } + for state in [ + JobState::Pending, + JobState::Claimed, + JobState::Running, + JobState::Delivered, + JobState::Blocked, + JobState::Failed, + JobState::Abandoned, + ] { + assert_eq!( + !state.is_terminal(), + live.contains(&wire(&state)), + "{state:?}: is_terminal disagrees with the live-issue index" + ); + } + } + + #[test] + fn exactly_one_migration_ships_today() { + // A reminder rather than a rule: schema changes are new files, never + // edits to an applied one, because sqlx records a checksum per version. + assert_eq!(MIGRATOR.iter().count(), 1); + assert_eq!(MIGRATOR.iter().next().expect("first").version, 1); + } +} diff --git a/crates/tireless-data/tests/migrations.rs b/crates/tireless-data/tests/migrations.rs new file mode 100644 index 0000000..658af73 --- /dev/null +++ b/crates/tireless-data/tests/migrations.rs @@ -0,0 +1,218 @@ +//! Migration tests against a real Postgres. +//! +//! Skipped unless `TIRELESS_TEST_DATABASE_URL` is set, because CI has no +//! database and the house pattern is not to give it one (see `store.rs` on why +//! queries are checked at runtime). Run them by hand against a throwaway server: +//! +//! ```sh +//! podman run -d --name pg -e POSTGRES_PASSWORD=test -e POSTGRES_DB=tireless_test \ +//! -p 55432:5432 docker.io/library/postgres:18-alpine +//! export TIRELESS_TEST_DATABASE_URL=postgres://postgres:test@127.0.0.1:55432/tireless_test +//! cargo test -p tireless-data -- --ignored +//! ``` +//! +//! They are `#[ignore]`d rather than silently passing on a missing variable, so +//! a green `cargo test` never implies the schema was exercised. + +use sqlx::{Connection, Executor, PgConnection, Row}; +use tireless_data::store::MIGRATOR; + +fn url() -> String { + std::env::var("TIRELESS_TEST_DATABASE_URL") + .expect("set TIRELESS_TEST_DATABASE_URL to run the ignored migration tests") +} + +/// A connection scoped to its own empty schema. +/// +/// Per test, not per run: `cargo test` runs these concurrently against one +/// server, so a shared `public` schema means each test's reset truncates +/// whatever its neighbours are midway through. The failure looks like a schema +/// bug — a unique violation on a row the test never inserted — which is an +/// expensive thing to misread. +async fn fresh_db(schema: &str) -> PgConnection { + let mut conn = PgConnection::connect(&url()).await.expect("connect"); + conn.execute( + format!( + "drop schema if exists {schema} cascade; \ + create schema {schema}; \ + set search_path to {schema};" + ) + .as_str(), + ) + .await + .expect("private schema"); + conn +} + +/// A second connection into an existing test schema, for concurrency tests. +async fn join_db(schema: &str) -> PgConnection { + let mut conn = PgConnection::connect(&url()).await.expect("connect"); + conn.execute(format!("set search_path to {schema};").as_str()) + .await + .expect("search_path"); + conn +} + +#[tokio::test] +#[ignore = "needs TIRELESS_TEST_DATABASE_URL"] +async fn migrations_apply_to_an_empty_database() { + const SCHEMA: &str = "t_apply"; + let mut conn = fresh_db(SCHEMA).await; + MIGRATOR.run(&mut conn).await.expect("first run"); + + let tables: Vec = sqlx::query( + format!("select tablename from pg_tables where schemaname='{SCHEMA}'").as_str(), + ) + .fetch_all(&mut conn) + .await + .expect("list tables") + .iter() + .map(|r| r.get::("tablename")) + .collect(); + + for expected in ["tracked_repo", "job", "agent_run"] { + assert!( + tables.contains(&expected.to_string()), + "missing table {expected}" + ); + } +} + +#[tokio::test] +#[ignore = "needs TIRELESS_TEST_DATABASE_URL"] +async fn applying_twice_is_a_no_op() { + const SCHEMA: &str = "t_twice"; + // The runner, poller and api all migrate on start, and systemd may restart + // any of them at any time — including all three at once after a deploy. A + // second run must be silent, not an error and not a duplicate. + let mut conn = fresh_db(SCHEMA).await; + MIGRATOR.run(&mut conn).await.expect("first run"); + MIGRATOR + .run(&mut conn) + .await + .expect("second run must be a no-op"); + + let applied: i64 = sqlx::query("select count(*) as n from _sqlx_migrations") + .fetch_one(&mut conn) + .await + .expect("count migrations") + .get("n"); + assert_eq!(applied, MIGRATOR.iter().count() as i64); +} + +#[tokio::test] +#[ignore = "needs TIRELESS_TEST_DATABASE_URL"] +async fn one_live_job_per_issue_but_any_number_of_historical_ones() { + const SCHEMA: &str = "t_live"; + // The property the partial unique index exists for. Getting this wrong in + // either direction is silent: too strict and discovery never recurs, too + // loose and one issue is worked twice at once. + let mut conn = fresh_db(SCHEMA).await; + MIGRATOR.run(&mut conn).await.expect("migrate"); + + let insert = |state: &str| { + format!( + "insert into job (forge,owner,repo,number,kind,state) \ + values ('gitea','lair','tireless',1,'discover','{state}')" + ) + }; + + conn.execute(insert("pending").as_str()) + .await + .expect("first live job"); + let second = conn.execute(insert("pending").as_str()).await; + assert!( + second.is_err(), + "a second live job for one issue must be rejected" + ); + + conn.execute("update job set state='delivered'") + .await + .expect("finish it"); + conn.execute(insert("pending").as_str()) + .await + .expect("a new job after a terminal one must be allowed"); + + let n: i64 = sqlx::query("select count(*) as n from job") + .fetch_one(&mut conn) + .await + .expect("count") + .get("n"); + assert_eq!(n, 2, "history must be retained, not overwritten"); +} + +#[tokio::test] +#[ignore = "needs TIRELESS_TEST_DATABASE_URL"] +async fn a_pending_job_cannot_hold_a_claim() { + const SCHEMA: &str = "t_claim"; + // Lease expiry returns a job to the pool, and must clear the claim as it + // does. The database refuses the half-done version so that a buggy release + // path cannot strand an issue. + let mut conn = fresh_db(SCHEMA).await; + MIGRATOR.run(&mut conn).await.expect("migrate"); + + let bad = conn + .execute( + "insert into job (forge,owner,repo,number,kind,state,claimed_by,claim_expires_at) \ + values ('gitea','lair','tireless',1,'plan','pending','w1',now())", + ) + .await; + assert!( + bad.is_err(), + "a pending job holding a claim must be rejected" + ); + + conn.execute( + "insert into job (forge,owner,repo,number,kind,state,claimed_by,claim_expires_at) \ + values ('gitea','lair','tireless',1,'plan','claimed','w1',now() + interval '5 min')", + ) + .await + .expect("a properly claimed job is fine"); +} + +#[tokio::test] +#[ignore = "needs TIRELESS_TEST_DATABASE_URL"] +async fn concurrent_claims_never_hand_out_the_same_job() { + const SCHEMA: &str = "t_concurrent"; + // The property the whole design rests on (design.md §4.2). Two claimers, + // one pending job: exactly one winner, and the loser gets nothing rather + // than blocking. + let mut setup = fresh_db(SCHEMA).await; + MIGRATOR.run(&mut setup).await.expect("migrate"); + setup + .execute( + "insert into job (forge,owner,repo,number,kind,state) \ + values ('gitea','lair','tireless',1,'plan','pending')", + ) + .await + .expect("seed one job"); + + const CLAIM: &str = "select id from job where state='pending' \ + order by created_at limit 1 for update skip locked"; + + let mut a = join_db(SCHEMA).await; + let mut b = join_db(SCHEMA).await; + + let mut ta = a.begin().await.expect("tx a"); + let got_a = sqlx::query(CLAIM) + .fetch_all(&mut *ta) + .await + .expect("claim a"); + + // b runs while a still holds the row lock. + let mut tb = b.begin().await.expect("tx b"); + let got_b = sqlx::query(CLAIM) + .fetch_all(&mut *tb) + .await + .expect("claim b"); + + assert_eq!(got_a.len(), 1, "the first claimer takes the job"); + assert_eq!( + got_b.len(), + 0, + "the second claimer must skip the locked row, not block and not duplicate" + ); + + ta.commit().await.expect("commit a"); + tb.commit().await.expect("commit b"); +} diff --git a/crates/tireless-entities/src/forge.rs b/crates/tireless-entities/src/forge.rs index d792a66..dfc48bf 100644 --- a/crates/tireless-entities/src/forge.rs +++ b/crates/tireless-entities/src/forge.rs @@ -9,6 +9,11 @@ pub enum Forge { /// Self-hosted Gitea at `git.lair.cafe` / `git.internal`. The default. Gitea, /// Legacy repos still on GitHub, per `architecture/generic.md` §11. + /// + /// Renamed explicitly: `rename_all = "snake_case"` turns `GitHub` into + /// `git_hub`, which would otherwise become the permanent spelling in the + /// database, the JSON API and the generated TypeScript. + #[serde(rename = "github")] GitHub, } diff --git a/dashboard/src/api/generated/Forge.ts b/dashboard/src/api/generated/Forge.ts index 0953dbf..7b4062f 100644 --- a/dashboard/src/api/generated/Forge.ts +++ b/dashboard/src/api/generated/Forge.ts @@ -3,4 +3,4 @@ /** * A source forge tireless can poll and push to. */ -export type Forge = "gitea" | "git_hub"; +export type Forge = "gitea" | "github";