Skip to content

control-plane(settlement): a redelivered payout can double-decrement a pool after reversal #9612

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

control-plane/src/settlement-backend-driver.ts defines the SettlementBackendDriver contract and its
in-memory fake. The contract states two idempotency requirements:

  • recordPayoutEligibleEvent: "MUST be idempotent per event (same repo+PR+pool+contributor) —
    re-recording a settled event is a no-op, so a redelivered webhook never double-pays."
  • reversePayout: "reverse a previously-recorded payout, crediting event.amount back to the pool.
    MUST be idempotent — reversing an unrecorded or already-reversed event is a no-op, never a throw."

The fake implements both using presence/absence of a key in a single Set<string>
(payoutEventKey = poolId|repoFullName|prNumber|gittensorContributor). Two defects follow:

1. A reversal re-arms the double-pay it was supposed to guard. reversePayout calls
settledEventKeys.delete(key). Since that same set is the only record that the event was ever
processed, a subsequent recordPayoutEligibleEvent(sameEvent) sees !settledEventKeys.has(key) and
decrements the pool a second time. The realistic sequence is:

fundPool("pool-1", 500)
recordPayoutEligibleEvent(e)          // balance 400
reversePayout(e, "refund")            // balance 500
recordPayoutEligibleEvent(e)          // balance 400  <- must be a no-op; the event is not new

GitHub redelivers pull_request webhooks, which is precisely the scenario the "redelivered webhook
never double-pays" clause exists for. Once a refund/dispute has been processed, a redelivery silently
re-debits the pool.

2. A reversal credits back whatever amount the caller hands in, not what was decremented.
amount is deliberately not part of payoutEventKey, so
reversePayout({ ...e, amount: 1_000_000 }, "refund") after recordPayoutEligibleEvent(e) with
amount: 100 matches the key and credits 1,000,000 into the pool. "Reverse a previously-recorded
payout" is not satisfied by crediting back an unrelated number; the fake has no memory of what it
actually took.

The existing suite (control-plane/test/settlement-backend-driver.test.ts) tests record-then-record
and reverse-then-reverse, but never record -> reverse -> record, and never a reversal whose amount
differs from the recorded one — which is why both defects are green today.

This is a correctness bug in the already-shipped fake and its stated interface contract. It is not
a settlement policy change: what a refund means, how pricing/rounding works, and which rail wins all
remain out of scope and untouched.

Requirements

  • The fake MUST replace settledEventKeys: Set<string> with a per-event record that stores both the
    amount that was actually decremented and whether the event is currently reversed. The public
    settledEventKeys: ReadonlySet<string> property MUST remain on FakeSettlementBackendDriver with
    unchanged semantics — "the keys of payout events currently recorded-and-not-reversed" — derived
    from the new internal state.
  • recordPayoutEligibleEvent MUST be a no-op (no balance change, no state change) for any event key it
    has ever recorded, including one that has since been reversed. It MUST still push a
    recordPayoutEligibleEvent entry onto calls on every invocation, as it does today.
  • reversePayout MUST credit back the amount that was recorded for that key, ignoring event.amount.
    It MUST remain a no-op for an unrecorded key and for an already-reversed key, and MUST still push a
    reversePayout entry onto calls on every invocation.
  • fundPool, getPoolBalance, payoutEventKey, the calls log shape (FakeSettlementCall keeps
    amount: event.amount as passed), and the exported SettlementBackendDriver /
    FakeSettlementBackendDriver type surfaces MUST be unchanged.
  • The two JSDoc blocks on recordPayoutEligibleEvent and reversePayout MUST be updated to state the
    ever-recorded rule and the recorded-amount rule explicitly, since the current wording is what the
    fake is failing to honour.

⚠️ Required pattern: keep the fake's shape — a module-local Map/Set closed over by
createFakeSettlementBackendDriver, exposed through get accessors, mirroring
createFakeTenantProvisioningDriver in control-plane/src/tenant-provisioning-driver.ts:157-233.
What does NOT satisfy this issue: adding a real ledger abstraction or persistence; changing
payoutEventKey to include amount (that would make a redelivery with a different amount look like
a new event, which is the same bug in another shape); removing or renaming the public
settledEventKeys property; making either method throw; or adding balance/overdraft policy.

Deliverables

  • A new named regression test in control-plane/test/settlement-backend-driver.test.ts asserting
    the record -> reverse -> record sequence above leaves getPoolBalance("pool-1") === 500, and that
    driver.calls.map(c => c.step) is
    ["fundPool", "recordPayoutEligibleEvent", "reversePayout", "recordPayoutEligibleEvent"].
  • A new named regression test asserting that reversePayout({ ...event, amount: 1_000_000 }, "refund")
    after recordPayoutEligibleEvent({ ...event, amount: 100 }) restores the balance to exactly its
    pre-record value, not to pre + 1_000_000.
  • driver.settledEventKeys still contains the key after a record and no longer contains it after a
    reversal, asserted by the existing tests, unmodified.
  • A test asserting recordPayoutEligibleEvent on a reversed event does not re-add the key to
    settledEventKeys.
  • The JSDoc on both interface methods states the ever-recorded and recorded-amount rules.
  • Every existing test in control-plane/test/settlement-backend-driver.test.ts still passes without
    modification.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
fixing the re-record double-decrement but still crediting back event.amount on reversal — does not
resolve this issue.

Test Coverage Requirements

control-plane/ is not in codecov.yml's ignore list and has its own control-plane Codecov
flag, so the 99%+ branch-counted codecov/patch gate applies to this diff. Both arms of every changed
conditional need a test: record on a never-seen key, record on a currently-settled key, record on a
previously-reversed key; reverse on an unrecorded key, reverse on a settled key, reverse on an
already-reversed key. The two sequences named above are the required named regression tests.

Expected Outcome

The fake settlement backend actually honours the two idempotency clauses its interface documents: a
webhook redelivered after a refund is a no-op rather than a second debit, and a reversal restores
exactly the amount that was taken. Any adapter written against this contract inherits a scenario suite
that catches the same class of bug.

Links & Resources

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions