steadingskift

Claims and reads select nodes by one filter, across every project

Approach

Build the persisted rotation cursor: a small claim_cursors table keyed by (org_id, principal_id, filter_hash) that ClaimNext advances on every successful claim and Ready reads without mutating, so both present nodes in the same round-robin-across-projects turn order. The alternative considered was deriving “whose turn” from the existing leases table (no new schema, group ready projects by the principal’s most-recently-claimed project) — rejected because leases don’t record which filter selected them, so two independently-rotating claim loops by the same principal with different filters (the exact case the work order calls out: "'these two projects' cannot be said at all" today) would collapse into one shared history. That’s not a smaller version of the same feature, it’s a different, weaker feature that happens to pass the two-project acceptance test by coincidence of tie-breaking. The cursor is the smallest structure that actually satisfies “rotating per principal and filter.”

The other load-bearing decision: round-robin selection is implemented as “pick which project to draw from next” (a small, unlocked, non-ordered query over DISTINCT project_id), and then reuse the existing single-project lockNextReady — unchanged — to lock and take the top node of that one project. This avoids rewriting ReadyQuery.sql into a windowed, multi-project query, which Postgres forbids combining with FOR UPDATE at the same query level. Ready’s presentation reuses the same per-project ordered query (already exists, already tested) once per eligible project and interleaves the pages in Go, rather than trying to express the interleave in SQL. Every project.org boundary here is already org_id-scoped everywhere in the store, and there is no per-project ACL (internal/domain/project.go, internal/server/graph.go), so “every project the principal can read” is simply “every project of the caller’s org” — no new authorization plumbing is needed for the org-wide default.

Steps

  1. Proto: NodeFilter, and the mechanical rewire. proto/skift/v1/service.proto gains one NodeFilter message (projects, kinds, labels, any_labels, assignee_kind, min_priority, parent, status, held); ClaimNextRequest, ReadyRequest, ListNodesRequest drop their own filter fields and project for filter NodeFilter. Regenerate (make generate). Update every Go call site that fails to compile: internal/domain/ready.go (ReadyQuery gains Kinds []string, AnyLabels []string, Parent string, replacing singular Kind), internal/domain/nodes.go (NodeListQuery gets the same plus drops the single-project requirement — ListNodes becomes project_id = ANY($n) when Projects is non-empty, no predicate when empty), internal/domain/lease.go (ClaimRequest gets Projects []string, Kinds, AnyLabels, Parent), internal/server/work.go, internal/server/graph.go. Ready and Claim keep requiring exactly one project for now (ErrOneProject unchanged) — cross-project ordering is step 2, not this one. kinds matches any (OR, n.kind = ANY($n::text[])); any_labels matches any (OR, n.labels && $n::text[]); labels stays AND (@>, unchanged); parent filters n.parent_id = $n, resolved the way NodeListQuery.Parent already is. status/held are rejected by the server (not the domain struct) when set on a ClaimNextRequest or ReadyRequest — reads only, as the work order states. Proof: go build ./...; internal/domain/ready_test.go and the ListNodes specs (wherever they live beside nodes.go) gain cases for kinds OR-match, any_labels OR-match, and parent via the new struct; make test-unit green. This is also where the ListNodes-with-any_labels acceptance criterion is proven.

  2. Cross-project round-robin for Claim/ClaimNext and Ready. New migration internal/store/migrations/00019_claim_cursors.sql: table claim_cursors(org_id, principal_id, filter_hash, project_id, updated_at), primary key (org_id, principal_id, filter_hash), FK on org_id. New internal/store/queries/claim_cursors.sql (GetClaimCursor, UpsertClaimCursor), regenerated via sqlc generate. In internal/domain/lease.go: ErrOneProject’s single-project requirement is dropped from Claim/ClaimNext‘s path (it still applies to a direct Ready call with more than… no — see below); add eligibleProjects(ctx, tx, orgID string, q ReadyQuery) ([]string, error) — a plain SELECT DISTINCT n.project_id FROM nodes n WHERE <readyWhere + filter predicates minus the project restriction>, scoped to q.Projects when given, to every project of the org when not. Add nextProject(last *string, eligible []string) string — a pure function, unit-tested on its own: eligible sorted ascending by project id (ULIDs, so this is also creation order), cyclic-next after last, first element when last is nil or no longer eligible. claimOnce (lease.go:235): resolve req.Projects (already-plural) to the eligible set, compute a filterHash over the scope-defining fields only (Projects, Kinds, Labels, AnyLabels, AssigneeKind, MinPriority, Parent — not Wait/TTL/Harness/AgentRef/SessionRef, mirroring how requestHash already strips Wait), read the cursor row for (orgID, actor.PrincipalID, filterHash), pick the project via nextProject, then call the existing lockNextReady with Projects: [chosen] exactly as today. On a successful claim (lease created, inside the same transaction), upsert the cursor to the chosen project; on a lost race (errClaimLost, isLiveLeaseConflict) the cursor is not advanced for that attempt — the outer retry in Claim recomputes eligibility and the cursor fresh. Service.Ready (service.go:204): drop the project == "" requirement (currentProject-style, org-wide when empty); resolve the eligible project set the same way; read (never write) the cursor to pick a starting rotation point; run the existing per-project ordered Ready query once per eligible project (bounded), interleave the pages round-robin starting from that point, and cap at the page size. held_count on an empty first page sums HeldCount (hold.go:97) over every project in the requested scope (not just the eligible-and-empty set, which by definition is empty) — call it once per scoped project and sum in Go rather than writing a new multi-project SQL aggregate. Proof: db-labeled ginkgo specs (internal/domain/lease_test.go or ready_test.go, whichever holds the existing Claim/Ready specs): two projects each carrying one ready node, ClaimNext with an empty filter twice returns one node from each project on the two consecutive claims (the literal acceptance criterion); projects: [a] claims only from a; Ready with no project lists both projects’ nodes in the same turn order a matching sequence of claims would produce; an empty Ready page’s held_count sums correctly across two held projects. Also a plain unit test for nextProject covering: no cursor, cursor on a project no longer eligible, cursor mid-cycle, single eligible project.

  3. Server, CLI, and MCP plumbing. internal/server/work.go (Ready, ClaimNext) and internal/server/graph.go (ListNodes): map NodeFilter to the domain structs from step 1/2, and reject status/held on ClaimNextRequest/ReadyRequest with a clear invalid-argument error. The root-level -p/--project flag (root.go:81) becomes the one and only --project flag in the CLI, made repeatable: a.flags.Project string → a.flags.Projects []string, bound with pf.StringArrayVarP(&a.flags.Projects, "project", "p", nil, ...). A second, command-local --project on ready/claim/list was considered and rejected: cobra/pflag’s flag-set merge drops the inherited persistent flag once a local one shares its name, which breaks -p’s shorthand on exactly those three commands and the existing spec internal/cli/mcp_test.go:1294 (-p platform ready expecting exit 0) — confirmed by running it, not assumed. One flag, inherited everywhere as today, removes the collision entirely. internal/cli/app.go grows currentProjects() ([]string, error), the plural form of today’s currentProject(): the flag’s values if any were given, else [cfg.Project] if the config/env default is set, else empty — the same resolution chain currentProject() already walks, pluralized. project() (used by every command that resolves a bare #7 reference — get, patch, create, etc.) and currentProject() (used by focus.go) keep their existing single-value contracts by building on currentProjects(): zero values behaves as it does today (required-error for project(), empty for currentProject()), exactly one value unwraps to that string, and more than one is a new, explicit usage error (“only one –project is meaningful here”) rather than silently taking the first — those commands need exactly one project to resolve a bare reference against, and the flag is now shared with commands that don’t. ready/claim/ list alone call currentProjects() directly and pass the slice through as Filter.Projects, zero, one, or many, with no error. internal/server/work.go (Ready, ClaimNext) and internal/server/graph.go (ListNodes): map NodeFilter to the domain structs from step 1/2, and reject status/held on ClaimNextRequest/ReadyRequest with a clear invalid-argument error. internal/cli/mcp_tools.go: the ready, claim, node_list tool schemas take a projects array parameter instead of a single project string, resolved the same way as the CLI flag. Proof: internal/server/work_test.go and graph_test.go gain request-mapping cases (NodeFilter in, correct domain query out; status/held on a claim/ready request rejected); CLI flag-parsing tests confirm repeated --project on ready/claim/list produces Filter.Projects with multiple entries, that omitting it with no config default sends an empty filter, and that two --project values on a single-project command (e.g. skift patch) is the new clear usage error rather than silently picking one; internal/cli/mcp_test.go:1294 (-p platform ready exits 0) keeps passing unchanged, proving the shorthand survived; go build ./... and make test-unit green.

  4. Docs. docs/domain-model.md §Priority (domain-model.md:41): replace “Ordering across projects is the orchestrator’s business…” with the turn rule — round-robin across projects that have ready work, rotating per principal and filter, priority still comparable only within a project. service.proto’s own comments (already rewritten in step 1) are the RPC-level contract doc; there is no separate docs/api.md in this repository (confirmed: find docs -iname api.md is empty), so nothing else needs to follow the proto — recorded under Decisions. Proof: none automated; read the paragraph against the acceptance criteria and the two rejected-approach sentences in dagger#1’s Context to confirm neither claim (“selecting across projects is the orchestrator’s”) survives anywhere in the doc.

  5. Breaking-change and acceptance verification. Extend tests/acceptance (the scenario from 189f9c9, “a clean machine becomes a working skift”) with a step that creates a second project, files ready work in both, and runs skift ready with no project configured to check both projects’ nodes appear, each naming its project, in turn order — the end-to-end form of the acceptance criterion, through the built binary rather than the domain layer. Run buf breaking --against '.git#branch=main' and confirm the report names exactly the fields step 1 removed (project, kind, per-request labels/assignee_kind/min_priority on the three messages) and nothing else. Proof: make lint (includes buf lint, check-generated), make test, make test-db, make test-acceptance all green; the buf breaking command above, read by hand since its pass/fail is “reports only the intended fields,” not a zero exit code (removing fields always non-zero-exits buf breaking by design).

Files

Path What happens
proto/skift/v1/service.proto Add NodeFilter; rewrite ClaimNextRequest, ReadyRequest, ListNodesRequest to carry it, dropping project and their own filter fields
gen/skift/v1/** (Go + TS) Regenerated from the proto change (make generate); committed as usual
internal/protodoc/descriptors.binpb Regenerated alongside the proto
internal/domain/ready.go ReadyQuery gains Kinds, AnyLabels, Parent; sql() grows the OR-match clauses; Ready/ReadySet unchanged in shape, eligibleProjects and nextProject added
internal/domain/nodes.go NodeListQuery gains the same fields; ListNodes/.sql() drop the single-project requirement, widen to = ANY($n) or no predicate
internal/domain/lease.go ClaimRequest.Project → Projects []string (+ Kinds/AnyLabels/Parent); claimOnce resolves the eligible set and the rotation cursor before calling the unchanged lockNextReady
internal/domain/service.go Service.Ready drops the required-project check, resolves the scope, interleaves per-project pages, sums held_count
internal/domain/hold.go No signature change; HeldCount called once per scoped project from Service.Ready
internal/domain/filterhash.go (new, name indicative) The scope-only hash used as claim_cursors.filter_hash, sibling to the existing requestHash
internal/store/migrations/00019_claim_cursors.sql New table: (org_id, principal_id, filter_hash, project_id, updated_at), PK (org_id, principal_id, filter_hash)
internal/store/queries/claim_cursors.sql GetClaimCursor, UpsertClaimCursor
internal/store/*.sql.gen.go, models.gen.go Regenerated by sqlc generate
internal/server/work.go Ready, ClaimNext map NodeFilter ↔ domain structs; reject status/held on these two
internal/server/graph.go ListNodes maps NodeFilter ↔ NodeListQuery
internal/cli/root.go -p/--project becomes StringArrayVarP on a.flags.Projects []string, the one flag used everywhere
internal/cli/work.go readyCommand, claimCommand/claim: call the new currentProjects(), no longer hard-required
internal/cli/node.go list command: same
internal/cli/mcp_tools.go ready, claim, node_list tool schemas: projects array
internal/cli/app.go currentProjects() ([]string, error) added; project() and currentProject() rebuilt on top of it, erroring on more than one value
docs/domain-model.md §Priority: turn rule replaces “selecting across projects is the orchestrator’s”
internal/domain/ready_test.go, lease_test.go (or wherever Claim/ClaimNext specs live), internal/server/work_test.go, internal/server/graph_test.go, internal/domain/nodes_test.go (or equivalent), internal/domain/properties_test.go Updated/extended specs per steps 1–3; properties_test.go’s recomputeReady model-checker reviewed for whether it needs a multi-project mode (see Risks)
tests/acceptance/* Extended per step 5

Proof

  • make lint — go vet, golangci-lint, buf lint, opa check/opa fmt, check-generated (proto/sqlc output matches committed generated code) — all clean.
  • make test-unit — the Ginkgo suite excluding db-labelled specs, including every new spec from steps 1–3.
  • make test-db — the db-labelled specs, including the round-robin claim/ready specs from step 2, against real PostgreSQL.
  • make test-acceptance — the end-to-end scenario extended in step 5, against the built skift binary and a fresh database.
  • buf breaking --against '.git#branch=main' (or the ref CI uses) — read by hand: the report must name exactly the removed fields (project, kind, labels, assignee_kind, min_priority on the three messages) and nothing else. A clean buf breaking (no diff at all) would mean the break didn’t happen; any other reported break means something drifted beyond the intended shape.
  • go build ./... at the end of every step, not just the last.

Risks

  • The FOR UPDATE / window-function conflict was a real risk in the naive design and is designed around, not solved by fighting Postgres. By keeping lockNextReady untouched and single-project, and doing project selection as a separate, unlocked, pre-step, there’s no query that mixes FOR UPDATE with DISTINCT/windowing. If implementation finds a case where this still doesn’t hold (e.g. some future combination), the fallback is a per-candidate-project retry loop (try to lock project A’s top row; if nothing lockable, try B) — more round trips, same correctness. Worth an early spike in step 2 before committing to the query shape.
  • Concurrent claims by the same principal+filter can read the same stale cursor and both pick the same “next” project. This degrades fairness (one claim “wastes” its turn) but never breaks correctness: the actual node lock is still FOR UPDATE ... SKIP LOCKED, so two claimants never take the same node. Accepted as a bounded-imprecision race, not fixed with a cursor-row lock, to avoid serializing unrelated claims on an unrelated project’s contention.
  • claim_cursors cardinality is unbounded in principle — filter_hash varies with Kinds/Labels/AnyLabels/MinPriority/Parent, so a caller that varies its filter per call grows the table forever. No sweep job for this exists in the repo today (the same is true of idempotency_keys, which has a created_at index laid down for a sweep that isn’t implemented here either — see internal/store/migrations/00013_idempotency_keys.sql). This plan follows that existing precedent rather than inventing new retention machinery; if it’s a problem in practice it’s the same problem idempotency_keys already has, one level up.
  • properties_test.go’s recomputeReady models the ready set from scratch for property-based comparison against the real query; it’s unclear from this plan alone whether it needs updating to understand round-robin ordering or only the (order-independent) set of ready nodes, which wouldn’t change. Flagged for the implementer to check at step 2, not resolved here.
  • mill’s existing worker breaking at its first claim (the fifth acceptance criterion) isn’t something this repository’s test suite can directly assert — it’s a statement about a different repository’s client code failing to compile/marshal once project no longer exists on the wire. Step 5’s buf breaking check is the closest verifiable proxy available here; the actual criterion is proven by mill’s own blocked node, not this one.

Decisions

  • Cursor-backed rotation over lease-history-derived rotation (see Approach): the work order’s “per filter” requirement can’t be satisfied without recording which filter a claim used, and leases don’t carry that.
  • Round-robin implemented as project selection + the unchanged single-project locked query, not a rewritten multi-project ordered SQL query, to stay compatible with Postgres’s FOR UPDATE restrictions and to keep the change to lockNextReady at zero.
  • status and held on NodeFilter are rejected by the server, not silently ignored, when set on ClaimNextRequest/ReadyRequest — the work order marks them “reads only”; an error is more legible than a filter field that quietly does nothing on two of the three RPCs that carry it.
  • No separate docs/api.md exists in this repository; the proto file’s own comments are the contract documentation the work order’s “or wherever” resolves to. Only docs/domain-model.md gets an edit.
  • The existing root-level -p/--project is made repeatable and stays the one flag, used both for bare-reference resolution (still exactly one value, or a clear error above one) and as ready/claim/list’s scope filter (zero, one, or many). A second, command-local --project on those three commands was tried and rejected: it breaks -p’s shorthand via cobra/pflag’s flag-set merge and fails the existing mcp_test.go:1294 spec, confirmed by running it (see Steps §3).
  • HeldCount is summed in Go across scoped projects rather than given a new multi-project SQL form, consistent with keeping this change’s SQL surface as small as the work order’s ordering requirement actually demands.
  • Canonical project order for the rotation is ascending project_id (ULIDs, so this is also project-creation order) — arbitrary but deterministic and needs no extra lookup, since every row already carries it.

Edit this page