[ccf-lisa]

Task: IMPROVEMENTS item 5(b), stack 5 PR 5 of 8: fx/imp5b-5-serve-state-url on fx/imp5b-4-readyz-contract. Design: P3 (#223).

Merge with squash (Gustavo, 2026-10-28): a stray build output was committed and removed in 31df42e, so squash keeps it out of main. .gitignore now covers the repo-root /factory and /factory-agent (536bd87).

BREAKING CHANGE: factory serve needs a durable store. Without --state-dir or --state-url it refuses to start, unless it is given --state-memory (tests and quick local runs, where every work item starts fresh on a restart) or --dev-fake-providers. The run kit already passes --state-dir.

Workstream RT (internal/store/, internal/controllers/, internal/scheduler/, internal/server/, internal/auth/, cmd/factory serve and factory state), plus the design docs the spec names.

Summary

This PR implements P3 §Rollout PR 5:

• §Several dispatchers: who runs what, including per-process state, OAuth, /stream, LinkObservations and recovery;
• §Locks and leases: the sched lock;
• §Reads and freshness: workItemFor's empty lookup, tested across two stores;
• §Two releases on one log: readiness, and the 503 on writes;
• §No rebuild from markers, except Conn.List and Observe's parsing, which are PR 6's;
• §Backends: the serve flags and the refusal;
• the known limit, and Decision 7 (the reset messages).

Flags and readiness

• serve --state-url gs://…|s3://…|file:///… opens the bucket with objectstore.ParseURL and the store with objectstore.Open. It registers internalKinds and --state-sync-interval (default 2 s).
• --state-url is exclusive with --state-dir. The URL is never logged: the start-up line names its scheme only.
• A durable store is required. Without --state-dir or --state-url, serve refuses to start, unless it has --state-memory or --dev-fake-providers. A memory store logs at warn that every work item starts fresh on a restart, with no state and no run counters.
• Session keys. --state-url without FACTORY_SESSION_KEY is refused, and the error says why. FACTORY_SESSION_KEY_PREVIOUS (at least 32 bytes) is accepted for verifying.
• /readyz and factory_ready follow PR 4's contract. The server checks any store that has Ready() (server.StoreReadiness), and server.Options.Ready (the rebuild's wait) is gone. A store that stopped writing answers 503 store-read-only; one not synced, or past MaxLag, answers 503 unavailable. A catalog that isn't loaded is now catalog-not-loaded, the code the contract names; it was unavailable.
• CodeStoreReadOnly = "store-read-only": toProblem maps store.ErrReadOnly to a 503 with no Retry-After.

The leader

• controllers.Leader holds the leader lock: TTL 3 min, renewed every minute, and tried at start and every minute with a 5 s context.
• Held() is local. It is true from a successful Lock or Renew until the lease that call set ends on this process's clock. So a holder paused past its lease stops before another instance can take over. A failed Renew ends the term.
• Controller reconciles (provider, rule, debounce, work-item, run, and the scheduler's lease controller) are gated with the new Controller.SetGate(leader), which is checked before each reconcile. Informers run everywhere, so the queues are full at a takeover. Controller.Run can be called only once, so gating is used instead of restarting controllers.
• The sweeper, the other leader-only loop, runs per term through Leader.While. The scheduler's Refresh is skipped by the tick on non-leaders (scheduler.Options.Leader); the tick's wake of parked polls runs everywhere.
• Every instance serves the API, runs the tick, checks the App installations (reads only; each instance's getCatalog shows its own result, Decision 2), and runs Recover at its start with since = process start − 10 s.
• Release. At shutdown, the lock is released once the loops have stopped and before the store closes, so another instance takes over at its next try.

Per-process state

• OAuth state is self-contained: <payload>.<HMAC(session key, "factory-oauth-state\0" + payload)>, where the payload is {id, exp, rt}.
  • It is single-use through a UsedLoginState create named uls_<sha256(id)>. A second use, on any instance, is 400 oauth-state-mismatch.
  • The leader's sweep deletes the record once the state has expired.
  • MaxPendingLogins and the pending map are gone.
  • Session cookies verify under the previous key too.
• /stream: a Last-Event-ID ahead of the store syncs and is tried once more, then starts from now.
• Resync baselines are stored in LinkObservations, one record per work item: {digests: {"<provider>/<ref>": sha256}}.
  • A record is written only when the digests change: a create, or a CAS.
  • It is deleted when the item goes (found gone, or retired). A removed link's entry goes at the next resync, and the leader's sweep deletes orphans.
  • A new leader's first resync of an item that has stored baselines is jittered uniformly over the interval.
  • observationDigest drops the projections JSON key rather than naming the Go field, so PR 6 can delete Observation.Projections without touching RT files. Kept, with a comment (536bd87): until PR 6 merges, Observe still reports the factory's own markers, and the digest must leave them out. After PR 6 the key is never there and the deletion is a no-op. No deployed baseline depends on the encoding, since LinkObservations is new in this release.
• New internal kinds LinkObservations and UsedLoginState are in:
  • internalKinds();
  • storeaudit (kinds, audit lines and fixtures, which the file store's fixtures_test.go audit runs over);
  • store.md;
  • the schema golden, regenerated at readableSchema 1. SchemaVersion and ReadableSchema stay 1, as decided on feat(store): an object store that keeps the state as a log of create-only objects #238.

Scheduler: the sched lock (P3 §Locks and leases)

• Mint before the lock. The credentials are minted first. Then the commit takes sched with a 2 s context (Options.SchedWait) and a 30 s TTL (SchedTTL).
• Busy lock. A timeout is errSchedBusy. It ends the pass with no grant and sets schedBusyUntil = now + 2 s. A pass that starts before then returns at once, with no wait and no mint.
• The section runs under a 15 s deadline. It reloads the snapshot and re-checks the candidate: still pending, its agent still there, and every matching row passing (check). Then it runs the writes (commitLocked). A candidate that fails the re-check is skipped, and its credential is left to expire.

Rebuild removal

• wic.Rebuild, RebuildResult, mergeProjections, RebuildTimeout, the call in serve and /readyz's wait are all gone. Conn.List stays in the interface; PR 6 removes it.
• TestRestartRebuildsFromProjections is gone, and TestServeNormalisesAfterRebuild became TestServeRecoversFromBeforeItsStart.
• The RT and cmd tests that read Observation.Projections (rule_test.go, serve_store_test.go, workitem_test.go) now use the fake's log (gh.Projections()).

Reset (Decision 7) and factory state --state-url

• factory state reset's prompt and output, its golden and README §6 and §11 now say what is lost and how to recover:
  • open items restart with no state and no run counters (the run cap included), and no stored event survives to replay;
  • to recover, redeliver from the App's delivery log (about 3 days), reopen issues or re-apply labels.
• factory state --state-url URL is read-only, as --state-dir is. It uses the new objectstore.Read (a View: no sync, no lock, no write), and every read subcommand works on it. reset --state-url is refused, and says how a bucket is reset.

Shutdown

• Connections that never sent a request. Once a listener stops (Serve returns ErrServerClosed), its connections still in StateNew are closed. They are tracked with ConnState. TestServeShutdownWithIdleConnection covers it and fails without the fix ("drain requests: context deadline exceeded"). feat(agent): supervise can poll briefly instead of long-polling #237's test-side workaround is untouched.
• The bucket is closed after the store (sharedStore.Close), when it is an io.Closer.

Audit: every store write and provider call a leader-only loop or the recovery makes

Every row is a create, a compare-and-swap (update() and LinkObject re-read and retry), a conditional delete at the version read (store.DeleteIfUnchanged), or runs under the store-wide wi/<id> lock, except the deletes marked delete. A second delete of those finds nothing (ErrNotFound ignored), and each removes a record whose purpose is over. Four gaps were fixed in this PR (fixed).

Loop	Write or provider call	Guard
Recovery (every instance, at start)	Conn.Recover: lists the provider's deliveries	read
	Accept: Create Delivery named by the delivery id	create (a duplicate is ErrDuplicateDelivery)
Provider controller (normalise)	Conn.Normalise	provider read, no write
	Create Event (deterministic id)	create
	Update Delivery (normalised / failed)	CAS
	Delete Delivery with no stored event; delete after its retention	Fixed (Decision 1): conditional delete at the version read (store.DeleteIfUnchanged); a redelivery under the same id since is kept and normalised (TestIntakeRedeliveryRacesTheDelete)
Rule controller	Create WorkItem (deterministic id), Create EventStatus	create
	Delete EventStatus of an aged-out Event	delete
	Conn.Observe (observeItem)	provider read
	Update WorkItem (decisions, state, links), Create Task/TaskPrompt (deterministic, looked up first), Create DebounceRecord	wi lock + CAS / create. Fixed: EventStatus is read again under the lock, so a second leader whose reconcile saw the event unprocessed applies nothing (TestOverlapRuleEventProcessedOnce)
	Conn.Apply (notify, effects), then Create Effect	wi lock + the provider's idempotency key (IdempotencyKey)
Debounce controller	Update WorkItem, Apply, tasks (as the rule controller)	wi lock. Fixed: the record is read again and deleted under the lock; it was deleted after the lock was released, so a second leader could fire it twice (TestOverlapDebounceFiresOnce)
Work-item controller	Conn.Project	Fixed: under the wi lock, on the item read again there, so an overlapping leader can't write an older projection over a newer one (TestOverlapProjectionUnderTheLock)
	Conn.Observe (cache Refresh)	provider read
	Create factory.workitem.resynced Event (key resync/<wi>/<digest>)	create
	Create / Update LinkObservations	create / CAS
	Delete LinkObservations (item gone or retired)	delete (the next resync records a fresh baseline)
	Retire: Delete WorkItem after a re-check	wi lock
Run controller	Create Run (named per attempt)	create (ErrAlreadyExists reads the existing one)
	Update Run, Task; count (WorkItem counters)	CAS; a finish holds the wi lock, and a retry that finds the run ended skips the count
	Create factory.run.completed/failed Event (key run/<run>)	create
Scheduler lease controller	expire (Update Task), endLease (Update Lease), AckCancel	CAS (each re-checks its condition)
	Delete an ended Lease after LeaseRetention	delete
Scheduler Refresh	Delete expired AgentToken	delete
	syncAgent (Update Agent), setReason (Update Task)	CAS
Sweeper	store Sweep: retention deletes	delete (by retention rule)
	object store: compaction, lock records, log and snapshot retention, transcript orphans	create (snapshot, create-if-absent) / conditional delete (deleteAt) / bucket deletes
	Delete expired UsedLoginState, orphan LinkObservations	delete
Installation check (every instance, Decision 2)	the providers' installation lookups	provider read; each instance keeps its own result

Lease matching isn't leader-only: it runs in the agent's request on any instance, and its commits now serialise on sched.

Verification

• PATH=<scratchpad>/bin:$PATH make reviewable: passes at the head. golangci-lint (go1.26 build) reports 0 issues, and every package passes.
• After the review fixes (9787478): the race suites were re-run and pass:
  • the serve tests (shared state, idle connection, refusals, installations, connection grace, TestStateURL*), 10 times each;
  • the scheduler and controllers suites, 10 times;
  • internal/store/..., internal/auth and internal/server, 5 times.
• At the head (536bd87): make reviewable passes, and git status stays clean after building ./cmd/factory and ./cmd/factory-agent at the repo root.
• The two-serve tests. go test -race -count=10 -run 'TestServeSharedState|TestServeShutdownWithIdleConnection|TestServeStateURLRefusals|TestServeInstallationsOnEveryInstance' ./cmd/factory/ passes.
• The scheduler, leader, overlap and intake tests. go test -race -count=10 -run 'TestSched|TestLeader|TestOverlap|TestWorkItemFor|TestResync|TestIntake' ./internal/scheduler/ ./internal/controllers/ passes. Both packages also passed whole, 10 times, with -race, before the decisions' changes.
• The stores. go test -race -count=5 ./internal/store/... passes, with storetest's new CRUD/DeleteIfUnchanged on memory, the file store and the object store (fake and file:// buckets), and TestTwoStoresDeleteIfUnchanged (a stale store's delete of a recreated object is ErrConflict).
• Mutation checks. Each of these fails when its fix is reverted:
  • the sched lock's re-check: TestSchedLockTwoInstancesConcurrencyOne "leased 2 tasks";
  • linkedWorkItem's sync: TestWorkItemForSyncsOnAnEmptyLookup;
  • the recovery slack: TestServeRecoversFromBeforeItsStart;
  • closing the connections that never sent a request: TestServeShutdownWithIdleConnection;
  • the stream's sync: TestServeSharedState;
  • the rule and debounce re-checks: TestOverlap*;
  • the conditional delivery delete: TestIntakeRedeliveryRacesTheDelete ("the redelivery after the stale delete: ");
  • the installation check on every instance: TestServeInstallationsOnEveryInstance (B's warnings never appear behind the leader gate).
• TestServeSharedState runs two serves on one file:// bucket, plus a third that lags (sync 1 h):
  • one leader at a time;
  • a webhook on A, then a lease, heartbeat and complete on B, with the task Succeeded seen from A, and only the leader normalising;
  • a /stream resumed on the lagging one with A's Last-Event-ID;
  • a login started on A, finished on B with the shared key, the session used on A, and the state's second use refused;
  • a change on GitHub while A leads, then A stops and releases the lock, B takes over, and B's first resync reports the change.
• Scheduler, two object stores on one bucket (sched_test.go):
  • two schedulers with concurrency: 1, each behind, lease exactly one task, including a second round past the busy window;
  • a dead holder: one mint and one wait in the first pass; passes within 2 s neither mint nor wait; after the TTL, the task is leased once;
  • Refresh runs on the leader only.
• Refusals:
  • TestServeStateURLRefusals: --state-url file:// on a P2 dir, --state-dir on a bucket, and an unusable URL, none printing the URL;
  • TestParseServeConfig: no store, exclusive flags, --state-url without a session key, and the previous key.

Only a real cloud bucket can check: the leader lock's and the sched lock's timings against GCS's and S3's latencies, which are tested here on file:// and the fake bucket only. The emulator job from PR 2 doesn't run serve. Two serves on Cloud Run or Lambda, with CPU throttled between requests, are IMPROVEMENTS item 9's.

Size

This PR is over the 1,000-line rule, and the spec says it would be: +3,728 / −608 across 72 files. That is code +1,633 / −407, tests and goldens +2,006 / −153, and docs +89 / −48 (.gitignore counted as code). It carries the size-exception label: P3 §Rollout PR 5 is one checklist, and its pieces (the leader, the gate, the per-process state and the rebuild's removal) only make a working multi-dispatcher serve together. Over half of the added lines are tests.

Review fixes (ccf-review 5681190143, 9787478)

1. Must-fix: a stale pass minted once per pending task (internal/scheduler/lease.go). A candidate that fails the re-check under sched now ends the pass with a new sentinel, errSchedStale. It is handled like errSchedBusy, but sets no busy window, and the lock's sync has already woken the parked poll. So a pass mints at most once and takes the lock at most once. TestSchedLockStalePassMintsOnce (concurrency 1, 5 pending tasks, a stale instance polling once) asserts 1 mint and 1 lock round trip, then nothing on the next pass. With the old behaviour it fails at 5 mints and 10 writes.
2. factory state --state-url closed the bucket before reading through it (cmd/factory/state.go). The bucket now stays open on the stateRun, and runState closes it after the subcommand. TestStateURLBucketLifetime uses a bucket that fails on use after Close.
3. The OAuth callback wrote before checking the code (internal/auth/oauth.go). FinishLogin now checks the code and exchanges it before it creates the UsedLoginState. TestFinishLoginWritesNothingWithoutACode checks that an empty or refused code writes nothing.
4. A While term outlived the local lease (internal/controllers/leader.go). A watcher now cancels the term's context at the local lease end, with a timer re-armed on each renewal. TestLeaderPausedPastItsLease is extended to cover it.
5. Nits:
   • A connection that hasn't sent a request gets a 1 s grace before shutdown closes it (TestConnTrackerGrace).
   • returnTo is capped at 1 KB (auth.MaxReturnTo), so the state cookie stays small (TestSafeReturnTo).

Decisions (Gustavo, 2026-10-28)

1. The delivery delete and a redelivery: fixed here. A provider-controller reconcile on an overlapping (paused) leader could delete a Delivery that a redelivery re-created with the same id after the other leader had deleted it. The provider controller now deletes a Delivery only at the resourceVersion it read (internal/controllers/provider.go, deleteDelivery). The store gains an optional store.UnchangedDeleter (DeleteIfUnchanged(ctx, k, resourceVersion)) with the helper store.DeleteIfUnchanged, which falls back to Delete. Memory implements it over its existing DeleteAt, the file store through its journal, and the object store through the log (its internal deleteAt). The Store interface doesn't change. On ErrConflict the delivery is kept and enqueued. Tests: storetest CRUD/DeleteIfUnchanged on all three stores, TestTwoStoresDeleteIfUnchanged, and TestIntakeRedeliveryRacesTheDelete (the redelivery survives the stale delete and is normalised by its own reconcile).
2. The installation check runs on every instance. It only reads GitHub, and each instance keeps its own result, so it is off the leader gate. getCatalog's installation warnings (Installations.Gaps) are now the same on every instance. Test: TestServeInstallationsOnEveryInstance (a non-leader's getCatalog shows them), using a new test-only serveDeps.WrapAdapter.
3. The title is breaking: feat(serve)!: …, with the BREAKING CHANGE paragraph at the top.
4. Rebuild mentions outside the spec's list are updated here: design/README.md, 06-technology-options.md, 09-open-questions.md, 10-poc.md, rules-engine.md and rollout.md. github-provider.md stays for PR 6.
5. objectstore.Read/View for factory state --state-url is accepted, and kept. It is the only change to internal/store/objectstore beyond Open, apart from decision 1's exported DeleteIfUnchanged: view.go with its tests, plus an unexported collectLog flag (log.go, objectstore.go) that keeps the log entries a load applies.
6. Merge with squash, so no history rewrite: see the note at the top.
7. PR 6's verification grep (\.Projections\b) matches the fake's accessor gh.Projections(), which RT's tests now use and PR 6 keeps. Those hits are expected.

P3 on the design branch records decisions 1 and 2.

Deviations from the spec

• --state-memory in TestParseServeConfig. Each case without a store flag gets --state-memory from the test loop (a bare field opts a case out), rather than being edited case by case. Each case keeps its own expected error, and new bare cases cover the refusal.
• "serve starts without calling Conn.List" is shown by the code's removal: the call is gone. There is no runtime assertion, because one would need fake.MethodList, which PR 6 removes from FND's fake.
• /readyz with a catalog not loaded answers catalog-not-loaded, the code PR 4's contract names, instead of unavailable.
• --state-sync-interval is a new flag. P3 §Reads names it, and the objectstore package docs refer to it.

Open questions for reviewers

None: Gustavo's decisions above settle them.

🤖 Generated with Claude Code

[ccf-lisa]

size-exception (Gustavo, 2026-10-28): P3's PR 5 is specified as one change: running several dispatchers safely needs the leader, the scheduler's sched lock, the per-process state moved into the store or signed tokens, and the rebuild's removal together, and the two-serve test exercises them together. About half the diff is tests and goldens.

Reading order: cmd/factory/serve*.go (flags, store opening, readiness, shutdown) → internal/controllers/leader.go and the gates → internal/scheduler (the sched lock) → internal/auth (signed OAuth state, UsedLoginState) → internal/server/stream.go → internal/controllers/workitem.go (LinkObservations, no Rebuild) → the reset messages → the tests (serve_shared_test.go, twostore_test.go, overlap_test.go, sched_test.go).

[gusfcarvalho]

ccf-review: REQUEST_CHANGES

1 Must-fix, 3 Should-fix.

Stack (base-chain): #223 → #239 → #238 → #244 → #236 → #234 → #246

[gusfcarvalho]

ccf-review: COMMENT

no findings.

Round 1 (review 5681190143) is fixed at 536bd87: the one-mint-per-pass re-check (errSchedStale), the state bucket's lifetime, the code exchange before the UsedLoginState write, the While term ending at the local lease end, and both nits (the 1 s connection grace, returnTo capped at 1 KB). Each has a test. The scheduler, leader, two-serve, shutdown and state-URL tests pass with -race -count=10; the full suite (-race) and golangci-lint are clean.

Stack (base-chain): #223 → #239 → #238 → #244 → #236 → #234 → #246 → #247

[gusfcarvalho]

ccf-review: REQUEST_CHANGES

1 Must-fix.

Stack (gh stack 245): #223 → #239 → #238 → #244 → #236 → #234 → #246 → #247 → #237

[gusfcarvalho]

ccf-review: REQUEST_CHANGES

1 Must-fix.

Stack (gh stack 245): #223 → #239 → #238 → #244 → #236 → #234 → #246 → #247 → #237

[gusfcarvalho]

[Must-fix] CORE-TEST-001 · New behaviour without a test

TestFinishAfterTheLeaseMovedOn passes when either check is removed, because its scenario (an attempt spent, -1 closed lost, a new lease) trips both checks. Each check also covers a case of its own, and neither case has a test:

• The endTask attempt/agent check (line 700). Suppose the task is leased again before any reconcile closes -1. LeaseIssued's ensureRun then reuses the open -1, so endRun ends it byThis and the finishLocked check never runs. With line 700 replaced by if false {, a hookStore probe gets Complete = Succeeded, <nil>. The probe's beforeLock sets the task Pending, Attempts++, then e.lease(id), with no Reconcile. The probe then shows the re-leased task Succeeded and one run.completed.
• The finishLocked check (line 605). Suppose a throttled Abandon, which spends no attempt and closes -1 as abandoned, and then a new lease for the same agent. Attempt and agent both match, so endTask passes. With line 605 replaced by if false {, the probe (beforeLock: e.run.Abandon(.., Abandonment{Reason: AbandonThrottled}), then e.lease(id)) gets Complete = Succeeded, <nil> and one run.completed for the abandoned -1.

At da79c38 both probes return a conflict and leave the task Leased.

Why: Untested behaviour regresses silently and the next refactor removes it without anyone noticing.
Fix: Add both probes as cases next to TestFinishAfterTheLeaseMovedOn in run_interleave_test.go, for example as a table over the beforeLock hook. The cases are (a) re-leased before any reconcile: want a conflict and the task Leased on attempt 1; (b) a throttled abandon, then leased again to the same agent: want a conflict, the task Leased on attempt 0, -1 abandoned with no Result, and no run.completed. Check that (a) fails with line 700 disabled and (b) fails with line 605 disabled.

_{ccf-review · 4bf7e7b9777a · rules@fe7cb9dc86ce}