Library-mode client (KronosLibraryClient) has drifted from the REST path — 9 gaps
- Dominant language
- Rust
- Stars
- 2
- Forks
- 0
- Avg merge
- 1d 23h
- Merged PRs (30d)
- 10
Description
## Summary
`KronosLibraryClient` (embedded / library mode) has **drifted from the REST API path**. Both modes share the raw DB layer (`crates/common/src/db/*`), but the *orchestration above it* — transaction boundaries, pg_cron registration, idempotency handling, guards, input validation, and workspace provisioning — is **re-implemented independently** in `crates/worker/src/client.rs` (library) and `crates/api/src/handlers/*` (REST).
Because there is no shared service layer, parity between the two modes is a manual discipline rather than an invariant. When the REST CRON path was hardened to be transactional with pg_cron (#33), the library path never received the same treatment and silently fell behind. This issue tracks **9 confirmed gaps** (all verified against the code) and the fix.
> Note: an earlier claim that library mode ignores the `WorkerConfig` cache TTLs is **incorrect** — `poller::run` builds its own `PipelineContext` from the `AppConfig`-derived TTLs (`crates/worker/src/poller.rs`), so configured TTLs are honored. Not a bug.
## Root cause
Three entry points, but only **two** implementations of the business logic:
- `KronosHttpClient` → REST handler → db (rides on the REST handler, cannot drift)
- REST handler → db (implementation A)
- `KronosLibraryClient` → db (implementation B)
Everything below the raw SQL is shared; everything above it is duplicated by hand. A and B have drifted.
`crates/api` and `crates/worker` are siblings that both depend only on `crates/common`, so the natural home for a shared layer is a new `service` module in `common`, above `db::` and below both adapters.
## The gaps
| # | Gap | Severity | Where |
|---|-----|----------|-------|
| 1 | Library CRON jobs are never registered with pg_cron | 🔴 High | `crates/worker/src/client.rs` (Cron branch of `create_job`) |
| 2 | `create_job` is not transactional (job/execution can diverge) | 🔴 High | `crates/worker/src/client.rs` (`scoped_connection`, not a txn) |
| 3 | Missing idempotency key stored as `''` instead of `NULL` | 🟠 Medium | `crates/worker/src/client.rs` (`unwrap_or("")`) |
| 4 | `register_endpoint` cannot set payload-spec / config refs | 🟠 Medium | `crates/worker/src/client.rs` (both library **and** HTTP impls) |
| 5 | `cancel_job` pg_cron unschedule is not atomic with the retire | 🟡 Low | `crates/worker/src/client.rs` |
| 6 | Reaper is not provisioned for library workspaces | 🔴 High (coupled to #1) | `crates/worker/src/client.rs` `provision_workspace` / `crates/common/src/db/workspaces.rs` |
| 7 | No INTERNAL / RETIRED guards in library mode | 🟠 Medium | `crates/worker/src/client.rs` (`create_job`, `cancel_job`) |
| 8 | No idempotency short-circuit (raw DB error on key reuse) | 🟠 Medium | `crates/worker/src/client.rs` |
| 9 | No input validation against the endpoint payload spec | 🟠 Medium | `crates/worker/src/client.rs` |
### 1 & 2 — `create_job` is not transactional and never registers pg_cron
`KronosLibraryClient::create_job` opens a plain `scoped_connection` (autocommit), not a `scoped_transaction`.
- **#2 (durability):** `create_immediate` / `create_delayed` each run **two** INSERTs — one into `jobs`, one into `executions` (`crates/common/src/db/jobs.rs`). On an autocommit connection these commit independently; a crash between them leaves an orphaned `ACTIVE` job with **no execution** (shows in `list_jobs`, never runs). REST wraps both in a `scoped_transaction` (`crates/api/src/handlers/jobs.rs`).
- **#1 (never fires):** the `Cron` branch inserts the job row and returns `job_id` — it **never calls `register_pg_cron`**. So a library-created CRON job has no pg_cron entry: it never ticks and never materializes executions. REST registers pg_cron on the same transaction as the row write.
**Impact:** recurring work scheduled through the library silently never runs.
### 3 — Missing idempotency key stored as `''` instead of `NULL`
`create_job` does `let ikey = idempotency_key.unwrap_or("")` and binds it directly. The jobs uniqueness index is **partial**:
```sql
CREATE UNIQUE INDEX idx_{p}jobs_idempotency
ON {p}jobs (endpoint, idempotency_key)
WHERE idempotency_key IS NOT NULL;
```
`NULL` is exempt (never equal to `NULL`, and excluded by the `WHERE` predicate); `''` is **not** — `'' = ''` is true and `'' IS NOT NULL` is true, so it participates in the constraint. Two jobs created without a key on the **same endpoint** collide on the second insert with an opaque unique-violation — even though no key was ever supplied. REST never stores `''` (it generates a UUID for keyless IMMEDIATE and requires a key for DELAYED).
### 4 — `register_endpoint` cannot set payload-spec / config references
The `KronosClient::register_endpoint` trait method has no parameters for `payload_spec_ref` / `config_ref`, and both the library and the HTTP implementations omit them (library hard-codes `None, None`; the HTTP client never sends them). The REST handler and DB layer already support both refs. So a library- or client-registered endpoint can never reference a config (needed for `{{config.*}}` templating) or a payload spec (input validation).
> Landmine for the fix: `db::endpoints::create(.., payload_spec_ref, config_ref, ..)` and `db::endpoints::update(.., config_ref, payload_spec_ref, ..)` take those two args in **swapped order**. The REST handler already accounts for this; the fix must too.
### 5 — `cancel_job` unschedules pg_cron non-atomically
`cancel_job` marks the job `RETIRED` on a scoped connection, then — after dropping it — calls `unregister_pg_cron` on the pool as a **separate** statement. A crash between the two leaves a `RETIRED` job with a live pg_cron entry — a permanent scheduler leak. REST does both on one transaction.
### 6 — Reaper is not provisioned for library workspaces
The per-workspace **reaper** is kronos's dogfooded GC for CRON jobs: an `INTERNAL` CRON job whose ticks retire expired CRON jobs (past `cron_ends_at`) and unschedule their pg_cron entries (`crates/worker/src/reaper.rs`). It is installed **only** by the API path (`workspaces::create` → `provision_reaper`); the library's `provision_workspace` calls only `provision_schema` and stops.
pg_cron has no concept of `cron_ends_at` — the guard in the pg_cron command stops *new executions* past the window, but the entry keeps ticking and the job stays `ACTIVE` forever. The reaper is what closes the loop.
**Coupling to #1:** today this is masked because library CRON jobs never get a pg_cron entry at all. The moment #1 is fixed, any bounded (with `ends_at`) library CRON job will, on expiry, stay `ACTIVE` forever and **leak its pg_cron entry forever**. So #6 must ship with #1.
> `provision_reaper` is currently hard-wired for the API's unprefixed deployment (bare `endpoints`/`jobs` inserts, prefix `""`). Library mode runs with a table prefix, so it must be made prefix-aware.
### 7 — No INTERNAL / RETIRED guards in library mode
REST blocks user jobs on `INTERNAL` endpoints (job create/update/cancel and endpoint create) and rejects cancelling an already-`RETIRED` job. The library has none of these guards. Once #6 installs a reaper into library workspaces, an un-guarded `cancel_job` called with the reaper's `job_id` would silently kill the workspace's sweep and unschedule its pg_cron entry; `create_job` against `kronos.reaper` would stack extra sweeps. These are cheap one-line checks (`EndpointType::from_str_val(x) == Some(INTERNAL)`) already available in `common`.
### 8 — No idempotency short-circuit (raw DB error on key reuse)
On key reuse, REST returns the existing job (HTTP 200) before inserting; on a race it also maps the unique-violation to a clean 409. The library does neither — it surfaces the raw `sqlx` unique-violation. This matters more after #3, since a collision then means a **deliberate** key reuse — exactly when idempotent behavior is expected.
### 9 — No input validation against the endpoint payload spec
REST validates job `input` against the endpoint's payload spec (JSON Schema) when both are present; the library trusts the caller. With #4 fixed (library can attach a payload spec to an endpoint), a caller could reasonably expect it to be enforced. Adding validation makes a payload spec mean the same thing in both modes.
## Proposed fix
Extract the shared job orchestration into a **single core** in `crates/common/src/service/` that both the REST handler and the library client call. The core owns the workspace-mutation semantics — endpoint fetch, INTERNAL guard, input validation, max-attempts resolution, idempotency short-circuit, transactional inserts, pg_cron registration, and atomic cancel + unschedule — and returns a neutral result/error type. Each adapter stays thin: REST maps to `AppError` + HTTP responses; the library maps to `anyhow`. REST's observable behavior (status codes, error messages, response JSON) is preserved.
This fixes gaps 1, 2, 5, 7, 8, 9 **by construction** (the library runs the same code REST runs), makes future drift structurally impossible for the covered operations, and folds in:
- **#3:** `db::jobs::create_immediate` / `create_delayed` take `Option<&str>` for the idempotency key and bind real `NULL` when absent (REST behavior unchanged — it keeps generating a UUID / requiring a key).
- **#4:** thread `payload_spec_ref` / `config_ref` through a new `register_endpoint_with_refs` on the `KronosClient` trait (non-breaking; existing `register_endpoint` preserved), implemented for both the library and the HTTP client, respecting the create/update arg-order swap.
- **#6:** make `provision_reaper` prefix-aware and call it from the library's `provision_workspace`, so library workspaces get a reaper like the API path.
Tests follow the repo's convention (pure-function unit tests). A full REST-vs-library DB round-trip parity test would require a Postgres + pg_cron harness that does not exist in CI today — tracked as a follow-up.
## Notes
- Confirmed against the code; nothing applied on `main`.
- Prod is live with data — the fix keeps REST byte-identical and is backward-compatible.
- Fix branch: `fix/kronos-library-rest-parity` (PR to follow after review).
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.