From 6808da98d7052fe18649e1e8b65797d7f54ecd2c Mon Sep 17 00:00:00 2001 From: rsnetworkinginc Date: Wed, 29 Jul 2026 03:36:55 +0300 Subject: [PATCH] fix(control-plane): remember reversed payout events so a redelivery never re-debits and a reversal credits the recorded amount The fake settlement backend tracked settled events with a single Set, so reversePayout deleted the only proof an event was ever processed: a webhook redelivered after a refund/dispute re-decremented the pool, and a reversal credited back the caller-supplied amount instead of what was actually taken. Track every ever-recorded event in a Map keyed by payoutEventKey with the recorded amount and a reversed flag; recordPayoutEligibleEvent is a lifetime no-op for any ever-recorded key (including reversed ones), reversePayout credits back the recorded amount and stays idempotent, and the public settledEventKeys ReadonlySet keeps its documented recorded-and-not-reversed semantics as a derived view. JSDoc on both interface methods now states the ever-recorded and recorded-amount rules. Closes #9612 --- .../src/settlement-backend-driver.ts | 63 +++++++++++++------ .../test/settlement-backend-driver.test.ts | 56 +++++++++++++++++ 2 files changed, 99 insertions(+), 20 deletions(-) diff --git a/control-plane/src/settlement-backend-driver.ts b/control-plane/src/settlement-backend-driver.ts index b61b6d801a..f357fd0459 100644 --- a/control-plane/src/settlement-backend-driver.ts +++ b/control-plane/src/settlement-backend-driver.ts @@ -47,12 +47,15 @@ export interface SettlementBackendDriver { /** Read side of the balance contract: a pool's remaining allocation. A pool never funded reads as 0. */ getPoolBalance(poolId: PoolId): Promise; /** "Payout owed" intake: record a payout-eligible event and decrement its pool's balance by `event.amount`. - * 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. */ + * MUST be idempotent per event (same repo+PR+pool+contributor) for the event's whole lifetime: re-recording + * an event that has EVER been recorded — including one that has since been reversed — is a no-op, so a + * redelivered webhook never double-pays, even after a refund/dispute already reversed that payout. */ recordPayoutEligibleEvent(event: PayoutEligibleEvent): Promise; - /** Refund/dispute/partial-completion hook: 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 reason is threaded through for #4791's policy/audit; this contract does not interpret it. */ + /** Refund/dispute/partial-completion hook: reverse a previously-recorded payout, crediting back the amount + * that was actually recorded (decremented) for that event — the `event.amount` passed on the reversal call + * is NOT trusted for the credit. MUST be idempotent — reversing an unrecorded or already-reversed event is a + * no-op, never a throw. The reason is threaded through for #4791's policy/audit; this contract does not + * interpret it. */ reversePayout(event: PayoutEligibleEvent, reason: SettlementReversalReason): Promise; } @@ -81,23 +84,41 @@ function payoutEventKey(event: PayoutEligibleEvent): string { return `${event.poolId}|${event.repoFullName}|${event.prNumber}|${event.gittensorContributor}`; } +/** What the fake remembers for every event it has ever recorded: the amount that was actually decremented and + * whether the event is currently reversed. Entries are never deleted — a reversed event stays known, which is + * what keeps a redelivered webhook from re-settling it after a refund/dispute. */ +type SettledEventRecord = { amount: number; reversed: boolean }; + /** - * Minimal in-memory fake for contract/scenario tests — a balances map stands in for a real ledger, a set of - * settled event keys enforces per-event idempotency, and an ordered call log records every step. NO real funds, - * credentials, or IO. Mirrors `createFakeTenantProvisioningDriver`: implements the interface and exposes its - * recorded state as extra introspection surface beyond the contract. + * Minimal in-memory fake for contract/scenario tests — a balances map stands in for a real ledger, a map of + * ever-recorded payout events (recorded amount + reversed flag) enforces lifetime per-event idempotency, and an + * ordered call log records every step. NO real funds, credentials, or IO. Mirrors + * `createFakeTenantProvisioningDriver`: implements the interface and exposes its recorded state as extra + * introspection surface beyond the contract. */ export function createFakeSettlementBackendDriver(): FakeSettlementBackendDriver { const balances = new Map(); - const settledEventKeys = new Set(); + const settledEvents = new Map(); const calls: FakeSettlementCall[] = []; + /** Apply a settlement delta to a pool's balance (a pool never funded starts from 0). */ + function addToPoolBalance(poolId: PoolId, delta: number): void { + balances.set(poolId, (balances.get(poolId) ?? 0) + delta); + } + return { get balances() { return balances; }, get settledEventKeys() { - return settledEventKeys; + // Derived view with the documented semantics preserved: the keys currently recorded-and-not-reversed. + const keys = new Set(); + for (const [key, record] of settledEvents) { + if (!record.reversed) { + keys.add(key); + } + } + return keys; }, get calls() { return calls; @@ -111,22 +132,24 @@ export function createFakeSettlementBackendDriver(): FakeSettlementBackendDriver }, async recordPayoutEligibleEvent(event) { calls.push({ step: "recordPayoutEligibleEvent", poolId: event.poolId, amount: event.amount }); - // Idempotent intake: a redelivered event (already settled) neither re-decrements the balance nor - // double-records — the else-path is the "already settled" no-op. + // Lifetime idempotency: an event that has EVER been recorded — currently settled OR since reversed — is a + // no-op, so a redelivered webhook never re-decrements the pool, even after a refund/dispute reversal. const key = payoutEventKey(event); - if (!settledEventKeys.has(key)) { - settledEventKeys.add(key); - balances.set(event.poolId, (balances.get(event.poolId) ?? 0) - event.amount); + if (!settledEvents.has(key)) { + settledEvents.set(key, { amount: event.amount, reversed: false }); + addToPoolBalance(event.poolId, -event.amount); } }, async reversePayout(event, _reason) { calls.push({ step: "reversePayout", poolId: event.poolId, amount: event.amount }); - // Idempotent reversal: only a currently-settled event credits back; reversing an unrecorded or + // Idempotent reversal: only a currently-settled event credits back — and it credits the amount that was + // recorded for the event, ignoring the reversal caller's `event.amount`. Reversing an unrecorded or // already-reversed event is a no-op, never a throw. const key = payoutEventKey(event); - if (settledEventKeys.has(key)) { - settledEventKeys.delete(key); - balances.set(event.poolId, (balances.get(event.poolId) ?? 0) + event.amount); + const record = settledEvents.get(key); + if (record !== undefined && !record.reversed) { + record.reversed = true; + addToPoolBalance(event.poolId, record.amount); } }, }; diff --git a/control-plane/test/settlement-backend-driver.test.ts b/control-plane/test/settlement-backend-driver.test.ts index 44d8209c5e..84c765d20f 100644 --- a/control-plane/test/settlement-backend-driver.test.ts +++ b/control-plane/test/settlement-backend-driver.test.ts @@ -99,3 +99,59 @@ test("every step is recorded in call order for white-box assertions", async () = ); assert.deepEqual(driver.calls[0], { step: "fundPool", poolId: "pool-1", amount: 500 }); }); + +test("record -> reverse -> record: a redelivery after a reversal is a no-op, never a second debit", async () => { + const driver = createFakeSettlementBackendDriver(); + const event = eventFor({ amount: 100 }); + await driver.fundPool("pool-1", 500); + + await driver.recordPayoutEligibleEvent(event); // balance 400 + await driver.reversePayout(event, "refund"); // balance 500 + // The event was ever-recorded, so a redelivered webhook must not re-decrement the pool. + await driver.recordPayoutEligibleEvent(event); + assert.equal(await driver.getPoolBalance("pool-1"), 500); + + // The public balances view mirrors what getPoolBalance reports. + assert.equal(driver.balances.get("pool-1"), 500); + // Every invocation is still logged, including the no-op redelivery. + assert.deepEqual( + driver.calls.map((c) => c.step), + ["fundPool", "recordPayoutEligibleEvent", "reversePayout", "recordPayoutEligibleEvent"], + ); +}); + +test("reversePayout credits back the recorded amount, not the amount on the reversal call", async () => { + const driver = createFakeSettlementBackendDriver(); + await driver.fundPool("pool-1", 500); + + await driver.recordPayoutEligibleEvent(eventFor({ amount: 100 })); + assert.equal(await driver.getPoolBalance("pool-1"), 400); + + // Same event key (amount is deliberately not part of it) but a wildly different amount: the credit must be + // the 100 that was actually decremented, restoring exactly the pre-record balance — not pre + 1,000,000. + await driver.reversePayout(eventFor({ amount: 1_000_000 }), "refund"); + assert.equal(await driver.getPoolBalance("pool-1"), 500); + + // The call log still records the reversal's amount exactly as passed by the caller. + assert.deepEqual(driver.calls.at(-1), { step: "reversePayout", poolId: "pool-1", amount: 1_000_000 }); +}); + +test("recordPayoutEligibleEvent on a reversed event does not re-add its key to settledEventKeys", async () => { + const driver = createFakeSettlementBackendDriver(); + const event = eventFor({ amount: 100 }); + await driver.fundPool("pool-1", 500); + + await driver.recordPayoutEligibleEvent(event); + await driver.reversePayout(event, "dispute"); + await driver.recordPayoutEligibleEvent(event); + assert.equal(driver.settledEventKeys.has("pool-1|acme/widgets|42|dev"), false); + assert.equal(driver.settledEventKeys.size, 0); +}); + +test("recordPayoutEligibleEvent on a never-funded pool decrements from the 0 an unfunded pool reads as", async () => { + const driver = createFakeSettlementBackendDriver(); + + await driver.recordPayoutEligibleEvent(eventFor({ amount: 100 })); + assert.equal(await driver.getPoolBalance("pool-1"), -100); + assert.ok(driver.settledEventKeys.has("pool-1|acme/widgets|42|dev")); +});