feat(runtime): ADR 0012 concurrency — debounce, concurrent, lockScope + lease-loss hardening - #38
Conversation
|
@codex review |
|
@codex security review |
![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: 85659f6600
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 85659f6600
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7fefe7af3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: a7fefe7af3
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
@codex review |
|
@codex security review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6884d2bbb
ℹ️ 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".
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f480b7a53
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 0f480b7a53
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
Implement the remaining ADR 0012 surface: ConcurrencyDebounce, ConcurrencyBurst, ConcurrencyConcurrent, a configurable LockScope (thread default, channel opt-in), and handler preemption/steerability via RuntimeOptions.OnLockConflict with force-release-lock coordination through the new chat.LockForcer optional State capability (ForceReleaseLock on memory/redis/postgres/nats states + conformance). Drop stays the default; existing drop/queue behavior is unchanged and the public API is additive. Closes #7
- preempt: verify the waiter is still pending before destroying in-flight work; wait (ctx-bounded) for the local victim to finish before force releasing so drop/queue serialization holds locally; bound the force release by the Detached Work Context instead of WithoutCancel - outcome telemetry: record OutcomePreempted by cancellation cause even when the handler returns nil, and close acquire-failure waits (queue/ debounce/preempt/burst) as OutcomeError instead of ignored - validation: reject DetachTimeout <= DebounceInterval under debounce and burst, which would abandon every event before dispatch - lock scope: length-prefix channel key fields so the mapping is injective across adapter/tenant/channel combinations - debounce: superseded waiters exit promptly on a displacement signal instead of parking through the full interval holding their payloads
- preempt: re-validate pending ownership after waiting for the local victim, so a displaced preemptor cannot force-release a lease a newer waiter already holds; residual instruction-scale races stay fenced by lease refresh cancellation - lock waits: re-check pending ownership after AcquireLock returns held, releasing the fresh lease when the waiter was displaced mid-acquire (debounce/queue newest-waiter-only guarantee) - lock scope: namespace channel-scope fallback thread keys (thread-scope/<len>:<id>) so an opaque Thread ID can never collide with a synthesized channel key - burst: document join-order (not arrival-order) batch semantics
A local preemption victim releases its own lease after cancellation, so the preemptor now simply waits for it and acquires normally - a lease acquired by a fresh unrelated handler after the victim's release can no longer be destroyed. Force release through LockForcer is reserved for leases with no locally registered holder (remote instance or orphaned), where lease-refresh cancellation remains the documented fence. The preemption log/observation now carries forced=true/false.
- burst: the batch dispatch now runs under a fresh DetachTimeout that starts when the collection window closes, so collection time can no longer consume accepted batch members' execution budget - nats: gate ForceReleaseLock's delete by the observed revision so only the lease seen in-call is invalidated (parity with the atomic Redis DEL / Postgres DELETE), treating a mid-call handover as no-op - docs: state explicitly that queue/debounce supersession and burst windows coalesce per runtime instance (cross-instance coalescing is the ADR 0012 wait/coalesce State extension, tracked in #50)
9e5537a to
022d1f8
Compare
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 022d1f817e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 022d1f817e
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
…newal retry - preempt: local lease ownership is now reserved the moment a lease is acquired (prelude and lock-wait acquisitions), not when the handler starts, so a preemptor arriving while the owner is still routing or deduping can never mistake the fresh local lease for a remote one and force-release it; a reservation preempted before the handler starts prevents the handler from running at all (span outcome preempted, started=false) - nats: a revision mismatch during ForceReleaseLock re-reads the entry and retries while the token is unchanged, so an ordinary same-token ExtendLock renewal is no longer mistaken for a lease handover
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebe1ab8f03
ℹ️ 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".
An ExtendLock error exits the refresh loop, after which the lease expires at TTL while the handler (or remaining burst members) would keep running unserialized. Treat a refresh error like a lost lease: mark it lost (benign release) and cancel the handler with ErrPreempted so no holder outlives a lease the runtime can no longer maintain.
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0892821b41
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 0892821b41
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
… design track Per maintainer decision on the review convergence bound: burst and the OnLockConflict force/steerability hook (plus the LockForcer State capability that exists solely for it) move to a draft branch (feat/adr-0012-preempt-burst-draft) pending the deferred-dispatch admission-bound and fenced-coordination design work (#44, #50). This PR retains debounce, concurrent, LockScope, and the universal lease-loss hardening: a deferred handler whose lease vanishes or can no longer be refreshed is cancelled with chat.ErrPreempted instead of running unserialized (OutcomePreempted telemetry retained for that path).
ThomasK33
changed the title
|
@codex review |
|
@codex security review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91751e4cb1
ℹ️ 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".
A delivery delayed in its prelude (validation, dedupe, routing) could register after — and displace — a waiter admitted later, making an older message dispatch as the debounce 'final' event (and inverting queue newest-wins). Deliveries now carry a dispatch admission sequence; registerPending rejects registrations older than the current waiter, which resolve as superseded instead.
|
@codex review |
|
@codex security review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a01d2d3a22
ℹ️ 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".
Snapshot the cancellation cause before draining the refresh loop, so a lease-loss result that lands only after the handler already returned cannot reclassify the completed run as preempted or suppress a real handler error. Also document that the debounce quiet period is measured over registered waiters: a delivery stalled in its prelude longer than the interval dispatches separately (serialized, never lost) rather than resetting a timer it never joined.
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…ts + deferred protocol design (non-converging surface, per the #38 precedent)
…ce coalescing rejected for now (#54) * docs(adr): ADR 0015 — deferred-dispatch admission bound and fenced cross-instance coalescing (design for #44 + #50) * docs(adr): honest preemption-overlap contract (cooperative cancellation) and explicit sync-dispatch serving-layer non-goal * docs(adr): close coalescing protocol holes — global fence sequence, register-all-routed, deferred-only fencing, abandonment vs degradation * docs(adr): commit-point degradation rule, admission-anchored register TTL, NATS uniform-TTL provision, reject preemption under concurrent * docs(adr): bounded allocation-to-registration window with TTL arithmetic covering it * docs(adr): concrete fence timing bounds, uniform-fleet caveat, LockForcer commit-point rule, bounded refresh calls, burst requires deferred * docs(adr): full-park check horizon, fence as single ordering source, idempotent takeover reconciliation, shape-aware admission rejection * docs(adr): bounded idempotent takeover reconciliation; fleet-uniform timing for TTL arithmetic across all backends * docs(adr): scope-wide degradation ordering, pre-RPC window anchor, expired register-entry purge * docs(adr): universal SHA-256 holder identity; reject burst + preemption hook * docs(adr): uniform pre/post-mark degradation rule; concrete refresh RPC bound (one refresh interval) * docs(adr): takeover + reconciliation execute in the detached tail under the DetachTimeout budget * docs(adr): extend-as-reconciliation-probe with TTL horizon; ack-budget bounds on prelude fence calls * docs(adr): conflict-time observation binding, hook in tail, extend-only probe, read-completion validity, allocation before all prelude State reads, spec-level scoping * docs(adr): descope preemption to rejected shapes + binding requirements + deferred protocol design (non-converging surface, per the #38 precedent) * docs(adr): re-scope per maintainer directive — admission bound kept, cross-instance coalescing rejected for now with formal-design reopening bar * docs(adr): prelude-return slot release, ADR 0003 acceptance qualification, explicit ADR 0012 supersessions for burst gating * docs(adr): slot release at tail-goroutine return, duplicate fast path at the cap, optional per-tenant sublimit * docs(adr): drop unimplementable duplicate fast path (qualify ADR 0002, prohibit mark-as-probe), composite tenant key + empty-tenant bucket + validation, burst final-member slot through cleanup * docs(adr): consistent slot-lifetime wording; burst lock sequencing per shipped debounce precedent with explicit ADR 0002 qualification * docs(adr): batch FIFO seal-order dispatch, batch-derived member retention bound (ADR 0002 qualification), budgeted busy-response for ack-separated interactions * docs(adr): burst window = DebounceInterval, bounded batch coordination with observable abandonment, lease-loss batch disposition * docs(adr): cap-reaching member ownership, cooperative-bound residual, no-response-url fallback, no-undocumented-loss invariant * docs(adr): fixed burst window anchor, OutcomeSkippedLeaseLoss closed-set value, member reference clearing before slot release * docs(adr): shutdown drains open windows, handler-error continuation, detached busy-post context, index ADR 0015 in explanation.md * docs(adr): ADR 0004 burst lock qualification, atomic shutdown admission close, tenant counter cleanup * docs(adr): idle scope-coordinator removal; busy-post drain before adapter cleanup * docs(adr): decision-only re-scope per maintainer ruling — decisions/invariants/non-goals; all mechanism prose cut; burst admission deferred to #53 revival
1 participant
Implements the ADR 0012 concurrency surface that converged cleanly in review:
ConcurrencyDebounce,ConcurrencyConcurrent, a configurableLockScope(thread default, channel opt-in), and universal lease-loss cancellation for deferred handlers. Contributes to #7 (partial delivery — see scope note).Scope note (maintainer-directed split)
Nine codex review rounds kept converging everywhere except two areas — the force/steerability (
OnLockConflict) preemption path and the burst lifecycle — whose P1 findings interact with the upcoming deferred-dispatch admission-bound design (#44) and fenced cross-instance coordination design (#50). Per maintainer decision, those two features (plus theLockForcerState capability that exists solely for preemption) are staged onfeat/adr-0012-preempt-burst-draft(draft PR) and their names stay reserved; this PR ships the surface that was consistently clean across rounds.What ships
ConcurrencyDebounce— every routed event registers as its scope's pending event (superseding the previous waiter, reusing the queue supersession machinery) and its detached tail waitsDebounceIntervalfrom its own arrival; only the final event of a quiet period dispatches. Superseded waiters exit promptly on a displacement signal (no parked payloads), supersession is observable (chat debounce superseded+superseded_by), and post-acquire ownership re-checks close the displaced-mid-acquire race. RequiresDispatchDeferredwithDetachTimeout > DebounceInterval(validated).ConcurrencyConcurrent— the explicit opt-out of per-scope serialization: no Thread Lock, every routed event runs in its own execution bounded by a requiredMaxConcurrentsemaphore. Works under sync and deferred dispatch.LockScope(LockScopeThreaddefault /LockScopeChannel) — chooses the Thread Lock key. Channel keys are namespaced and length-prefixed (injective; a fallback opaque Thread ID can never collide with a synthesized channel key); the prelude validates the Thread ID before acquiring so the key comes from the adapter-validated ref.chat.ErrPreemptedand its span closes aspreempted, instead of running on without serialization. Lock-wait state failures close aserror, never silentlyignored.Design interpretation notes
DispatchDeferred: a synchronous webhook cannot park an event through a quiet period without blowing platform ack deadlines.MaxConcurrentbounds the whole runtime (one semaphore) and must be positive: an explicit bound beats a silently unlimited default.ValidateThreadIDnow runs beforeAcquireLock(the channel scope key needs the validated ref). An invalid-thread-id event no longer touches the lock — same terminal outcome, less lock traffic.New public surface (additive)
ConcurrencyDebounce,ConcurrencyConcurrent,LockScope/LockScopeThread/LockScopeChannel,RuntimeOptions.{LockScope,DebounceInterval,MaxConcurrent},ErrPreempted,OutcomePreempted. Existing drop/queue behavior is unchanged apart from note 6 (regression-covered by the existing queue/drop suites).Testing
debounce_test.go,concurrent_test.go,lockscope_test.go— lifecycle, coalescing/supersession observability (incl. deterministic displaced-mid-acquire coverage via a gated-acquire fake), abandonment, shutdown drain, bounded concurrency, no-lock proof, key-collision proof, and construction validation for every new option; plusTestDeferredHandlerCancelledOnLeaseLoss.fakeStatemints unique lock tokens per acquisition (the deterministickey + "-token"scheme violated the token-owned lease invariant).mise run vetandGOFLAGS=-race mise run testpass across all workspace modules (root, adapters, nats/postgres/redis states, examples).Review history
24+ findings across nine codex review rounds (three clean security verdicts on interim heads) were fixed in-branch before the split; the preemption/burst-specific fixes travel with the draft branch. Related issues filed during review: #39 (pre-existing slack test flake, since fixed on main by #41), #44 (deferred-dispatch admission bound), #50 (fenced cross-instance coordination: coalescing + token-bound force).
Generated with
mux• Model:anthropic:claude-fable-5• Thinking:xhigh