Repository navigation
docs: pin the one-serving-instance deploy invariant (#271) - #484
Conversation
#271 names documenting this as its short-term fix, and the issue is right that it was written down nowhere - not AGENTS.md, not README, not deploy/, not docs/decisions/. Nothing tells an operator that a second API instance silently double-runs the background work: no error, no log, the duplicate is simply invisible. Verified against the current code rather than restated from the issue, which predates two things that changed it: - The worker registration moved out of Program.cs into Hosting/CluckworkJobServiceCollectionExtensions.cs with the #291 DI grouping, so the issue's Program.cs:500 reference is stale. - The durable-job half is still a scaffold - it selects pending rows and logs them, no handlers are registered - so job double-execution is latent, not live. What is live today is the three recurring sweeps that ride the same poll and run unconditionally per instance. Registering the first handler is what makes the job half real, and the section says so. Also recorded: the two purge sweeps are idempotent deletes, so a second runner wastes work rather than corrupting state, while DailyEntryLockSweep reads-then-writes behind only an optimistic Version token and is where concurrent runners actually contend. And that #307 closing does NOT license horizontal scale - #271 is now the sole remaining blocker, which is exactly the misreading its own acceptance criteria warn about. The claim that the run-then-exit verbs are unaffected was checked, not assumed: CliDispatcher returns an exit code before app.Run(), so hosted services never start for them, and healthcheck dispatches before the host is even built. Canonical statement in AGENTS.md under the deployment boundary, where the repo already keeps requirements it imposes on a host; a pointer sits in deploy/docker-compose.yml at the app service, which is where someone would actually add replicas. This is the interim mitigation, not the close - #271 still needs a real single-runner mechanism plus a two-instance test. Refs #271
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 981c168ec3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| half of the multi-instance story is done. **#271 is the sole remaining | ||
| blocker** — do not read #307's closure as permission to scale. |
There was a problem hiding this comment.
Do not call #271 the sole remaining blocker: after its worker fix, scaling would still make IStepUpGrantRegistry process-local. As documented in src/Cluckwork.Infrastructure/Identity/IStepUpGrantRegistry.cs:3-13, each replica maintains independent replay and logout state, so the same step-up grant can be accepted once by each replica and a logout handled by one replica is invisible to another; these grants protect privileged account-control operations. The deployment guidance must continue to block scaling until this state is shared as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d3cc4b9. You are right, and this was the more dangerous of the two findings — the doc would have licensed exactly the thing that breaks.
Verified rather than taken on trust: IStepUpGrantRegistry.cs:12-13 says a multi-instance deployment "would need to move this to a shared store", InMemoryStepUpGrantRegistry is documented as a per-process singleton holding replay tracking and logout epochs, and #338 is open for precisely that migration. So both #308 guarantees degrade per replica — a single-use grant becomes usable once per replica, and a logout honoured by one replica is invisible to the others — on grants that gate privileged account-control operations.
The method was the real error
"Sole remaining blocker" came from a list I assembled from memory, and #338 was in the open-issues list I had already read. So I did not replace it with another hand-made list — I swept for the property instead: source for process-local / per-process / in-memory (store|state) / shared store, and open issues matching replica|instance|scal|shared store.
That turned up a third blocker neither of us had named: the per-IP login limiter runs on AddRateLimiter's in-process partitions, so N replicas allow roughly N times the intended attempts per IP before lockout. No open issue tracks it, so it is now recorded in the section rather than left to be rediscovered.
The section now leads with three independent blockers that must all close, keeps #307's closure explicitly non-licensing, and the heading is (#271, #338).
| # EXACTLY ONE serving instance until #271 closes — no `deploy.replicas`, no | ||
| # second app service, no scaling this on a real host. Every instance runs the | ||
| # DurableJobWorker loop and its three recurring sweeps with nothing claiming | ||
| # the work, so a second one double-runs them silently: no error, no log, just |
There was a problem hiding this comment.
When two replicas overlap while lockable daily entries exist, this is not necessarily silent: both can load the same original Version, the first save wins, and the second SaveChangesAsync raises a concurrency exception. DailyEntryLockSweep.RunAsync catches that exception and emits LogError at src/Cluckwork.Infrastructure/Jobs/DailyEntryLockSweep.cs:51-55. Calling the duplicate execution “no error, no log” gives operators an incorrect expectation about the observable symptom.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d3cc4b9.
Checked the path before rewriting: the losing replica's SaveChangesAsync trips the optimistic Version token, and DailyEntryLockSweep's per-account catch (Exception ex) logs Lock sweep failed for account {AccountId} — so it is emphatically not silent.
"No error, no log" was over-generalised from the two purge sweeps, which are idempotent ExecuteDeletes and genuinely do report nothing. Both the AGENTS.md section and this comment now split the two cases, and keep the part that actually matters for an operator: the lock sweep's error reads as a database fault, not as "a second replica is sweeping", so the duplicate is still invisible as a duplicate even though something is logged. That is a different and more useful warning than the one I wrote.
Two corrections to what this branch first asserted, both of which would have misled an operator in the direction of scaling. #271 is NOT the sole remaining blocker. IStepUpGrantRegistry is a per-process singleton holding step-up replay tracking and logout epochs, and its own header says a multi-instance deployment must move it to a shared store - that is #338, open. Both #308 guarantees degrade per replica: a single-use step-up grant becomes usable once per replica, and a logout honoured by one replica is invisible to the others. Those grants gate privileged account-control operations, so closing #271 alone does not license scaling, and saying it did was the more dangerous half of the error. Enumerating rather than hand-listing then turned up a third: the per-IP login limiter runs on AddRateLimiter's in-process partitions, so N replicas allow roughly N times the intended attempts per IP before lockout. No open issue tracks it; it is recorded here rather than left to be rediscovered. The original claim came from a list assembled by memory, which is exactly how the step-up registry got missed, so the replacement is a swept list with each entry checked against source. The silence claim was also too broad. "No error, no log" holds for the two purge sweeps, which are idempotent deletes. DailyEntryLockSweep reads-then-writes behind an optimistic Version token, so the losing replica throws a concurrency exception and its per-account catch logs "Lock sweep failed for account {AccountId}". That is observable - it just reads as a database fault rather than as a second replica, which is the sense in which the duplicate stays invisible, and the text now says that instead. Found by codex review of PR #484. Refs #271, #338
|
@codex both findings are addressed in d3cc4b9 — replies at #484 (comment) and #484 (comment). The P1 was the dangerous one and you were right: #338 keeps The underlying error was method: I had hand-listed the blockers from memory. Replaced with a sweep of source for process-local/in-memory state and of open issues for replica/instance/scaling, which surfaced a third one neither of us named — the per-IP login limiter uses The P2 is fixed by splitting the cases: the two purge sweeps are silent, Please re-check, particularly whether the per-IP limiter deserves its own issue rather than only a line in AGENTS.md, and whether any multi-instance caveat remains that my sweep still missed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3cc4b9553
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| **Three independent blockers, all of which must close before scaling** — and | ||
| they are not all in #271: |
There was a problem hiding this comment.
The source sweep still misses ReportConcurrencyLimiter: it is registered as a singleton in CluckworkRateLimitingServiceCollectionExtensions.cs:97 and owns a process-local PartitionedRateLimiter<Guid>, so with N serving replicas one account can run up to N × ReportsConcurrency.PermitLimit heavyweight DB/CPU report queries. This makes the claim of exactly three independent blockers incomplete and could license scale-out after only those three are fixed; add this limiter and its shared/global remedy to the invariant.
AGENTS.md reference: AGENTS.md:L100-L103
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ea22c0bd.
Verified in source before changing anything: ReportConcurrencyLimiter (src/Cluckwork.Api/RateLimiting/ReportConcurrencyLimiter.cs:26-46) owns a PartitionedRateLimiter<Guid> partitioned by accountId, registered as a singleton at Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs:97. Nothing about it is shared across processes, so N replicas give one account N x ReportsConcurrency.PermitLimit concurrent heavyweight report queries — exactly the DB/CPU ceiling #311 exists to bound. It is now the fourth blocker in both AGENTS.md and the compose comment.
The more useful part of this finding is that it is the second miss of the same shape — a process-local limiter — and this one was in a file my previous sweep had already opened. Per the repo's own guard doctrine, two misses of one shape mean the method is wrong, not the list. So the section no longer just carries a longer list; it records how to re-derive it:
enumerate every
AddSingleton/AddHostedServiceundersrc/plus every in-memory state primitive (ConcurrentDictionary,IMemoryCache,PartitionedRateLimiter,Channel,SemaphoreSlim, mutable statics), then classify each one as safe or not.
That walk is 13 singletons + 1 hosted service today. The four blockers are what survives it, and the deliberate exclusions are named so the next walk does not re-litigate them: TimeProvider.System, the Serilog diagnostic contexts, IValidateOptions/IAuthorizationMiddlewareResultHandler (stateless), and FirstRunProvisioningLatch — a monotonic one-way cache of "the default account has an Owner", where a per-replica copy costs a few extra reads and cannot go stale in the unsafe direction.
I also widened the #143 wording from "the per-IP login limiter" to "the IP-keyed auth limiters", since the same AddFixedWindowByClientIp helper builds the login, refresh, and client-error policies.
Verification: docker compose -f deploy/docker-compose.yml config -q exits 0 (run against a throwaway .env copied from .env.example, then deleted — the real .env is gitignored and absent here). Docs-only change; no source touched.
`ReportConcurrencyLimiter` is a singleton owning a `PartitionedRateLimiter<Guid>` keyed by account, so with N serving replicas one account can hold up to N x `ReportsConcurrency.PermitLimit` heavyweight report queries in flight — the DB/CPU ceiling the #311 cap exists to bound, multiplied by the replica count. The list said three blockers, which would have licensed scale-out once those three closed. This is the second miss of the same shape (a process-local limiter), and the second one was in a file the first sweep had already opened — so the section now records how to re-derive the list (walk every AddSingleton/AddHostedService plus every in-memory state primitive, classify each) instead of inviting the next reader to extend it from memory. The deliberate exclusions are named too, including `FirstRunProvisioningLatch`, whose per-replica copy is a monotonic cache that cannot go stale in the unsafe direction.
|
@codex the Since this was the second miss of the same shape and the second one sat in a file my first sweep had already opened, I changed the method rather than just the list. The section now states how to re-derive the blockers — walk every Three things worth a skeptical look:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea22c0bd74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| classify each one as safe or not. That walk is currently 13 singletons and 1 | ||
| hosted service; the four above are what survives it. Excluded deliberately, so |
There was a problem hiding this comment.
The prescribed repo-wide walk finds 12 AddSingleton calls and one AddHostedService call under src/, not 13 singletons plus one hosted service. If the hosted worker is being counted by its singleton DI lifetime, it is already the thirteenth singleton rather than an additional service. Because this number is intended as the reproducible baseline for detecting future omissions, state either “12 singleton registrations and 1 hosted service” or explicitly say that the hosted service is included in 13.
AGENTS.md reference: AGENTS.md:L183-L185
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right — fixed in b9bd7eee. Re-ran the exact counts: grep -rn "AddSingleton\|TryAddSingleton" src/ = 12, grep -rn "AddHostedService" src/ = 1. Now states "12 AddSingleton registrations and 1 AddHostedService (13 total, the hosted service is not one of the 12)" — no ambiguity about whether it is included.
|
@codex the singleton-count fix from #484 (review) is addressed in |
|
The singleton-count fix is correct. The repository-wide scan now yields 12 No further changes were necessary, so I did not create an additional commit or pull request. Checks
|
Partial — this is the interim mitigation this issue names as its short term fix, not the close.
What landed
The invariant is now written down, which it genuinely was not: I grepped
AGENTS.md,README.md,deploy/, anddocs/decisions/and got zero hits. Canonical statement inAGENTS.mdunder the deployment boundary (where the repo already keeps requirements it imposes on a host), plus a pointer indeploy/docker-compose.ymlat theappservice — the place someone would actually adddeploy.replicas.Two corrections to this issue, found by checking the code rather than restating the body
The
Program.cs:500reference is stale. The registration moved intoHosting/CluckworkJobServiceCollectionExtensions.cswith the #291 DI grouping.Job double-execution is latent, not live.
ProcessPendingJobsAsyncselects pending rows and logs"no handlers are registered yet"— it executes nothing. What is live today is the three recurring sweeps riding the same poll unconditionally per instance:DailyEntryLockSweep,RefreshTokenPurgeSweep,IdempotencyRecordPurgeSweep. Registering the first job handler is what makes the job half real, and the new section flags that moment explicitly.Also worth recording, since it changes how urgent the remaining work is: the two purge sweeps are idempotent deletes, so a second runner wastes work rather than corrupting state.
DailyEntryLockSweepreads-then-writes behind only an optimisticVersiontoken, so it is the one where concurrent runners actually contend.Confirmed no claim of any kind in the poll — no
FOR UPDATE SKIP LOCKED, no lease, no advisory lock.On the acceptance criteria
CliDispatcher.TryRunAsyncreturns an exit code beforeapp.Run(), so hosted services never start;healthcheckdispatches before the host is even built.Still open
The real close needs a single-runner mechanism — advisory-lock lease or
FOR UPDATE SKIP LOCKEDwith crash recovery — plus the two-instance test proving each job and each sweep executes exactly once. Leaving this issue open for that.