Lock the cycle per subscription, stop re-enqueuing past-due subscriptions, and refuse a pending charge result without a remote id

Design pass against 1.0.x at 2e2e777. Each of the five gaps in the summary is still in the code as described. Three points the summary left open need a decision: where the lock lives and what a contended caller does, how to dedupe the cycle queue, and how "refuse" is spelled for a pending result without a remote id. Notes first, decisions at the end.

The lock

The lock belongs in CycleEngine, not in the queue workers: Drush, the management page and a connector's webhook handler all reach the engine without passing through a worker, and the engine is the only place that knows which operations touch a subscription's billing state. A small withSubscriptionLock(SubscriptionEntityInterface $subscription, callable $operation): bool helper acquires subscription_manager:cycle:{sid} from core's lock service, runs the operation, and releases in a finally. Core's database backend also releases every lock the request holds when it ends, so a worker that dies mid-item leaves no stale lock; the expiry passed to acquire() (sixty seconds, matching the workers' cron budget) bounds the window where a hard crash could hold it.

Four operations run under it: processDueSubscription(), retryCharge(), the sweep's resubmit(), and upgrade(). The first three are background work and simply return when the lock is held, with an info log line: whoever holds it is doing the same job, and the next cron run picks the subscription up again if it is still due. upgrade() is a person waiting on a page, so it waits (lock->wait(), a few seconds) and tries once more before giving up with an exception; #3630969: Refuse an upgrade while a period charge is open or the subscription is past due, and validate the currency on plan change is where its refusals become typed, so here the exception is a plain \RuntimeException that issue can replace. resolveCharge() stays outside the lock: it never opens a charge, and the race it could have with a concurrent cycle step (cycle loads the subscription, webhook settles the charge and advances the anchor, cycle then finds no open charge) is already closed by paidPeriodEnd(), which catches the anchor up instead of charging. scheduleDowngrade(), cancel(), resume() and end() stay outside too: they only save subscription fields, and the single-active invariant's own race has its own issue (#3630999: Acquire a per-user lock around the single-active check in SubscriptionEntity::preSave()).

The due query and the queue

findDueSubscriptionIds() gains an OR group: billing_state is null (a subscription that has never been in dunning stores nothing; the accessor defaults to current) or is not past_due. isPastDue() inside processDueSubscription() stays as the second line of defence, since Drush and tests call the engine directly.

For dedupe, two options. (A) A cycle_queued timestamp base field on the subscription. Cron excludes rows whose cycle_queued is within the settlement wait (four hours by default) and, after enqueuing, stamps the enqueued ids with one direct UPDATE plus a storage resetCache() for those ids. No entity save, so no presave role logic and no cache-tag invalidation per enqueue. An item that is dropped (lock held, connector missing) is re-enqueued once the stamp ages out, so nothing is lost, only delayed by at most the wait. Needs an update hook and a schema change; costs one query per cron run. (B) Accept duplicates as cheap no-ops: with the past-due exclusion and the lock, a duplicate costs one entity load before the anchor check returns, and the backlog only arises on a billing day the queue cannot drain within one cron interval. No schema change. On a site with thousands of subscriptions sharing a billing day, (B) still writes thousands of queue rows per cron run until the backlog clears, so I recommend (A). The retry queue needs neither: DunningRetryWorker already re-checks next_retry, which beginRetry() clears while an attempt is in flight.

Pending without a remote id

Two layers. At the source, ChargeResult::pending() takes a non-nullable string $remote_charge_id, so a connector that returns pending without one fails in its own test suite with a TypeError; when it happens in production the error is thrown inside submit()'s try block and takes the existing unknown-outcome path, which keeps the record submitted and lets the sweep re-submit it under the same key. As a backstop, applyChargeResult() treats a pending result whose id is an empty string the same way, with its own log message. The interface docblock for charge() says a pending result must carry the connector's identifier for the charge, and the change record says so too. succeeded() and failed() keep their nullable id: a synchronous decline often has none.

The instrument guard

In processDueSubscription(), after the zero-amount branch and the connector check and before paidPeriodEnd(): a due subscription whose instrument_id is empty is logged at error ("due but holds no payment instrument; skipping") and left untouched, anchor included, so it shows up again every run until an operator fixes it, like the missing-currency case. A zero-amount period needs no instrument, which is why the guard sits after that branch. retryCharge() and resubmit() re-submit an existing record and are not guarded: an instrument that vanished since the charge was opened makes the connector throw, which is the unknown-outcome path and ends in dunning after the limit, where it belongs. The test connector's charge() throws a \LogicException when the subscription has no instrument, so no future engine path can lean on the stub's leniency.

Tests

• Lock. A Kernel test acquires subscription_manager:cycle:{sid} itself, then calls processDueSubscription(), retryCharge() and the sweep: no charge record, no connector call, the info line logged once each; upgrade() under a held lock throws after waiting and changes nothing. Releasing the lock and calling again works normally. Two genuinely concurrent processes are out of reach for a Kernel test, so the lock-held path is the contract under test.
• Due query. A past-due subscription with a past anchor is not in findDueSubscriptionIds(); one with a null billing_state is; one explicitly current is.
• Dedupe (A). Two cron runs within the wait enqueue each due subscription once; a stamp older than the wait is re-enqueued; the stamp is written without an entity save (no changed bump, no role hook calls).
• Pending without id. The test connector gains a 'pending_no_id' behaviour that returns ChargeResult::pending('') (the typed signature rules out null); the record stays submitted with no remote id and the unknown-outcome warning, settleOpenCharges() re-submits it under the same attempt, and no second record is ever opened. A unit-level assertion that pending(NULL) is a TypeError.
• Instrument. A due subscription with no instrument: no record, no connector call, anchor unchanged, the error logged; a zero-amount period on the same subscription still advances the anchor.
• Year boundary. In the existing anchor test: next monthly anchor from 2026-12-31 is 2027-01-31; previous monthly anchor from 2026-01-31 is 2025-12-31; previous yearly anchor from 2024-02-29 is 2023-02-28.

Change record and connectors

One change record: ChargeResult::pending() requires a remote id; a due subscription without an instrument is skipped rather than handed to the connector; the cycle, retries, re-submissions and upgrades are serialised per subscription (a connector's webhook handler that calls resolveCharge() is unaffected). The connector catch-up template gains the pending() line. With (A), the record also mentions the new cycle_queued field for sites with custom queries, and the update hook installs it.

Decisions to ratify

1. Lock in the engine around processDueSubscription(), retryCharge(), resubmit() and upgrade(); background callers return when it is held, upgrade() waits briefly then throws; resolveCharge() and the field-only operations stay outside.
2. Lock expiry of sixty seconds, matching the queue workers' cron budget; both read one constant on the engine so they change together.
3. Due query excludes past-due rows with a null-or-not-past-due group; isPastDue() stays in the engine as the second check.
4. Dedupe option (A): a cycle_queued stamp with the settlement wait as its lease, written by direct update. (Alternative: (B), accept duplicates as no-ops.)
5. ChargeResult::pending() requires a string id; an empty id is treated as an unknown outcome in applyChargeResult().
6. Instrument guard placed after the zero-amount branch; retries and re-submissions not guarded; the test connector throws on a missing instrument.
7. Three fixed-date year-boundary assertions in the existing anchor test.
8. One change record covering the pending() contract, the instrument skip, the per-subscription serialisation and (with A) the new field.

Implemented on the MR in eight commits, one per theme, tests and docs apart, following the ratified decisions. One deviation from the design comment and two small additions, noted below.

What changed

• Lock. CycleEngine::withSubscriptionLock() wraps processDueSubscription(), retryCharge(), the sweep's re-submission and upgrade() with core's lock service under subscription_manager:cycle:{sid}. The bodies moved to runCycleStep(), retryLocked(), resubmitLocked() and upgradeLocked(). Background callers log at info and return when the lock is held; upgrade() waits LOCK_WAIT_SECONDS (five) then throws a \RuntimeException, which #3630969: Refuse an upgrade while a period charge is open or the subscription is past due, and validate the currency on plan change can turn into its typed refusal. resolveCharge() and the field-only operations stay outside, as decided.
• One constant for the expiry and the workers' budget. Per the ratification note on decision 2: CycleEngine::WORKER_TIME_BUDGET (sixty seconds) is the lock expiry and the cron: time of both CycleWorker and DunningRetryWorker.
• Due query. A null-or-not-past-due group on billing_state; isPastDue() stays in the engine as the second check.
• Enqueue stamp (option A). New cycle_queued timestamp base field, installed by update 10029. Cron calls findDueSubscriptionIds(), enqueues, then markQueued($ids), which writes the stamp with one direct UPDATE through the storage's table mapping and resets the entity cache for those ids. The due query skips rows stamped within the settlement wait. No entity save, so no role hooks and no cache-tag invalidation per enqueue, and changed is untouched (asserted).
• Pending without an identifier. ChargeResult::pending(string $remote_charge_id, ?string $message = NULL). In applyChargeResult() a blank identifier takes the unknown-outcome path through a new recordUnacknowledged(), which recordUnknownOutcome() now also uses; the charge's last_error stays the bare reason so dunning's failure message is unchanged. Interface docblock and README say the identifier is required and that an unacknowledged connector throws.
• Instrument guard. After the zero-amount branch and the connector check: a due subscription with no instrument is logged at error and left untouched. New SubscriptionEntity::hasInstrument() (trimmed, non-empty) backs it and the test connector's new \LogicException for the same case.

Deviation and additions

• The design said the stamp is written "by direct update"; it is, but through the storage's table mapping rather than a hard-coded table and column, so a future field-storage change cannot silently break it. The maintainers' reservation about direct updates stands; this is the narrowest form of one.
• The unknown-outcome log line now reads Charge N: the connector threw (…); outcome unknown… and Charge N: pending result carried no remote charge ID; outcome unknown…, one method behind both. Not in the design comment; a side effect of sharing the path.
• hasInstrument() was not in the design; it replaced three copies of the same trim-and-compare.

Tests

New CycleConcurrencyTest (nine tests) swaps the kernel test base's null lock backend for the real database one and uses a second backend instance, with its own lock id, as the other process:

• held lock: cycle, retry and re-submission skip with the logged line, nothing opened, the sweep counts the skip as not re-submitted, and the same retry goes through once released;
• held lock on upgrade: waits at least the wait, throws the documented message, plan and charges untouched;
• due query: current and null-state rows due, past-due row excluded;
• stamp: written at request time without bumping changed, hides the row, releases it once older than the wait;
• cron twice with a stuck item: one queue item, not two;
• pending without an identifier: stays submitted with no remote id and the reason, re-submitted by the sweep under the same charge_uuid and attempt, one record throughout, awaiting once the connector finally answers with an identifier; pending(NULL) is a TypeError;
• no instrument: skipped, logged, anchor untouched; a zero-amount period on the same shape still advances.

CycleEngineTest::testAnchorYearBoundary() adds the three fixed-date assertions. UpdatePathTestBase expects cycle_queued. Full Kernel suite: 232 tests green, the one pre-existing deprecation; Functional suite green (6 tests) in the clean codebase.

Change record

Drafted alongside: the pending() contract, the lock and its constant, the due-query exclusion and the stamp field, the instrument skip. The connector catch-up template gains the pending() line.