Skip to content

Commit 413d611

Browse files
fix(credits): hold approval credits before delivering, not after (#59)
Approval ingest read the balance, delivered to every enabled channel, then inserted the charge. Two ingests in flight at once — a moshscript firing ask()/notify() in parallel — both read a balance that covers one paid delivery, both deliver, and both charge, driving the balance negative and sending paid notifications (sms, slack, telegram) nobody paid for. Reserve the cost in a single INSERT ... SELECT ... WHERE guarded by the user's summed balance, the same way /cli/token and /webhooks/coinpay claim their rows, then settle the hold down to what actually went out. The ledger still keeps one row per delivery for exactly what was delivered. Co-authored-by: clawedassistant26 <307253840+clawedassistant26@users.noreply.github.com>
1 parent 14410f1 commit 413d611

3 files changed

Lines changed: 228 additions & 6 deletions

File tree

apps/pwa/src/lib/credits.mjs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,3 +33,37 @@ export const charge = (userId, amount, reason, meta) => entry(userId, -Math.abs(
3333
export function costOf(kinds) {
3434
return kinds.reduce((sum, k) => sum + (CHANNEL_COST[k] ?? 0), 0);
3535
}
36+
37+
/**
38+
* Hold `amount` against a user's balance in a single statement, the same way
39+
* /cli/token and /webhooks/coinpay claim their rows. Reading the balance and
40+
* then inserting a charge is not enough on its own: against a remote (network)
41+
* database two concurrent requests both read a sufficient balance before either
42+
* insert lands, and both spend it. The `WHERE` runs inside the insert, so only
43+
* the first reservation a balance can cover is written.
44+
*
45+
* Returns the ledger row id when the hold landed, or null when it did not.
46+
*/
47+
export async function reserve(userId, amount, reason, meta = null) {
48+
const cost = Math.abs(amount);
49+
const rowId = id();
50+
const r = await run(
51+
`INSERT INTO credit_ledger (id, user_id, delta, reason, meta, created_at)
52+
SELECT ?,?,?,?,?,?
53+
WHERE (SELECT COALESCE(SUM(delta),0) FROM credit_ledger WHERE user_id = ?) >= ?`,
54+
[rowId, userId, -cost, reason, meta ? JSON.stringify(meta) : null, Date.now(), userId, cost]
55+
);
56+
return r.rowsAffected ? rowId : null;
57+
}
58+
59+
/**
60+
* Settle a reservation down to what was actually used, so the ledger keeps
61+
* showing one row per delivery for exactly what went out. Settling to 0 releases
62+
* the hold entirely (nothing was delivered, so there is nothing to charge for).
63+
*/
64+
export async function settle(rowId, amount, meta = null) {
65+
const used = Math.abs(amount);
66+
if (!used) return run(`DELETE FROM credit_ledger WHERE id = ?`, [rowId]);
67+
return run(`UPDATE credit_ledger SET delta = ?, meta = ? WHERE id = ?`,
68+
[-used, meta ? JSON.stringify(meta) : null, rowId]);
69+
}

apps/pwa/src/routes/approvals.mjs

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { config } from "../config.mjs";
1010
import { id, token } from "../lib/crypto.mjs";
1111
import { bearer, userForApiKey } from "../lib/apikey.mjs";
1212
import { verifySignature } from "../lib/signature.mjs";
13-
import { balance, charge, costOf } from "../lib/credits.mjs";
13+
import { balance, costOf, reserve, settle } from "../lib/credits.mjs";
1414
import { fanOut } from "../lib/deliver.mjs";
1515
import { page, footer, appBar, esc } from "../lib/html.mjs";
1616
import { csrfInput } from "../lib/session.mjs";
@@ -45,12 +45,15 @@ approvalsRouter.post("/api/approvals", async (req, res) => {
4545
const url = `${config.origin}/approve/${approval.id}?t=${approval.cap_token}`;
4646

4747
// decide which channels we can afford, deliver only to those, then charge for
48-
// what actually went out (so the ledger always matches reality)
48+
// what actually went out (so the ledger always matches reality). The hold goes
49+
// in BEFORE delivery: checking the balance and charging afterwards lets two
50+
// concurrent ingests — a script firing ask()/notify() in parallel, which the
51+
// runtime allows — both pass the check and spend the same credits twice.
4952
const { all } = await import("../db.mjs");
5053
const enabled = (await all(`SELECT kind FROM channels WHERE user_id = ? AND enabled = 1`, [user.id])).map((r) => r.kind);
5154
const fullCost = costOf(enabled);
52-
const bal = await balance(user.id);
53-
const affordable = bal >= fullCost ? enabled : enabled.filter((k) => costOf([k]) === 0);
55+
const held = fullCost > 0 ? await reserve(user.id, fullCost, "approval.delivered", { id: approval.id, channels: enabled }) : null;
56+
const affordable = fullCost === 0 || held ? enabled : enabled.filter((k) => costOf([k]) === 0);
5457

5558
const notified = await fanOut(user, { ...approval, url }, affordable);
5659
const cost = costOf(notified);
@@ -62,15 +65,16 @@ approvalsRouter.post("/api/approvals", async (req, res) => {
6265
"pending", approval.cap_token, JSON.stringify(notified), cost, approval.created_at]
6366
);
6467

65-
if (cost > 0) await charge(user.id, cost, "approval.delivered", { id: approval.id, channels: notified });
68+
// settle the hold down to what actually went out (0 releases it)
69+
if (held) await settle(held, cost, { id: approval.id, channels: notified });
6670

6771
res.status(201).json({
6872
id: approval.id,
6973
url,
7074
status: "pending",
7175
delivered: notified,
7276
charged: cost,
73-
warning: bal < fullCost ? "insufficient credits — only free channels were used" : undefined,
77+
warning: fullCost > 0 && !held ? "insufficient credits — only free channels were used" : undefined,
7478
});
7579
});
7680

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
// Integration tests for credit accounting on approval ingest (POST /api/approvals).
2+
//
3+
// These boot the real router against a throwaway libsql file database. They
4+
// skip cleanly when the PWA dependencies are not installed (a fresh repo
5+
// clone only has the root CLI deps), so the root `npm test` stays green
6+
// either way. Run `npm install` in apps/pwa to enable them.
7+
import assert from "node:assert/strict";
8+
import http from "node:http";
9+
import fs from "node:fs";
10+
import { mkdtempSync } from "node:fs";
11+
import { tmpdir } from "node:os";
12+
import path from "node:path";
13+
import { createRequire } from "node:module";
14+
import test from "node:test";
15+
16+
const require = createRequire(import.meta.url);
17+
let deps = null;
18+
try {
19+
deps = { express: require("express"), cookieParser: require("cookie-parser") };
20+
} catch {
21+
deps = null; // pwa dependencies not installed — tests below skip
22+
}
23+
24+
// Point the app at a throwaway database BEFORE importing its modules (config
25+
// reads the environment once, at import time).
26+
const workdir = mkdtempSync(path.join(tmpdir(), "moshcode-pwa-approvals-test-"));
27+
process.env.DATABASE_URL = `file:${path.join(workdir, "test.db")}`;
28+
process.env.SESSION_SECRET = "test-secret";
29+
30+
async function boot() {
31+
const { migrate } = await import("../src/migrate.mjs");
32+
await migrate();
33+
const { run, all, db } = await import("../src/db.mjs");
34+
// The local libsql driver resolves statements in microtasks, which fully
35+
// serializes concurrent request handlers and hides read-check-write races.
36+
// Production runs against a network database (Turso), where every statement
37+
// is a round trip. Defer each statement to a macrotask so two in-flight
38+
// handlers genuinely interleave, like they would against the remote DB.
39+
const execute = db.execute.bind(db);
40+
db.execute = (stmt) => new Promise((resolve, reject) => {
41+
setTimeout(() => execute(stmt).then(resolve, reject), 2);
42+
});
43+
const { sessionMiddleware, csrfGuard } = await import("../src/lib/session.mjs");
44+
const { approvalsRouter } = await import("../src/routes/approvals.mjs");
45+
const { createApiKey } = await import("../src/lib/apikey.mjs");
46+
const { balance } = await import("../src/lib/credits.mjs");
47+
const { id } = await import("../src/lib/crypto.mjs");
48+
49+
const app = deps.express();
50+
app.use(deps.express.json({ verify: (req, _res, buf) => { req.rawBody = buf.toString("utf8"); } }));
51+
app.use(deps.express.urlencoded({ extended: false }));
52+
app.use(deps.cookieParser());
53+
app.use(sessionMiddleware);
54+
app.use(csrfGuard);
55+
app.use(approvalsRouter);
56+
const server = await new Promise((resolve) => {
57+
const s = app.listen(0, "127.0.0.1", () => resolve(s));
58+
});
59+
const { port } = server.address();
60+
61+
// A user with the given enabled channels and a starting balance. Returns the
62+
// plaintext API key the CLI would send.
63+
const seedUser = async (userId, { credits = 0, channels = [] } = {}) => {
64+
await run(`INSERT INTO users (id, email, display_name, created_at) VALUES (?,?,?,?)`,
65+
[userId, `${userId}@b.c`, "demo", Date.now()]);
66+
for (const [kind, target] of channels) {
67+
await run(`INSERT INTO channels (id,user_id,kind,target,enabled,created_at) VALUES (?,?,?,?,?,?)`,
68+
[id(), userId, kind, target ?? null, 1, Date.now()]);
69+
}
70+
if (credits) {
71+
await run(`INSERT INTO credit_ledger (id,user_id,delta,reason,created_at) VALUES (?,?,?,?,?)`,
72+
[id(), userId, credits, "test.seed", Date.now()]);
73+
}
74+
const { plaintext } = await createApiKey(userId, "test");
75+
return plaintext;
76+
};
77+
78+
const charges = (userId) =>
79+
all(`SELECT delta FROM credit_ledger WHERE user_id = ? AND reason = 'approval.delivered'`, [userId]);
80+
81+
// Raw http with a fresh connection per request: fetch()/undici would reuse a
82+
// keep-alive socket for same-origin calls and serialize the "concurrent"
83+
// ingests, hiding the race this exercises.
84+
const ingest = (key, message) => new Promise((resolve, reject) => {
85+
const req = http.request({
86+
host: "127.0.0.1", port, path: "/api/approvals", method: "POST", agent: false,
87+
headers: { "content-type": "application/json", authorization: `Bearer ${key}` },
88+
}, (res) => {
89+
let data = "";
90+
res.on("data", (chunk) => { data += chunk; });
91+
res.on("end", () => resolve({ status: res.statusCode, body: JSON.parse(data) }));
92+
});
93+
req.on("error", reject);
94+
req.end(JSON.stringify({ message }));
95+
});
96+
97+
return { run, all, db, server, seedUser, charges, ingest, balance };
98+
}
99+
100+
// One shared app/db for the whole file (db.mjs is a module-level singleton —
101+
// closing it between tests would break the next boot).
102+
let booted = null;
103+
const app = () => (booted ||= boot());
104+
105+
test.after(() => {
106+
if (!booted) return;
107+
booted.then(({ server, db }) => { server.close(); db.close?.(); })
108+
.finally(() => { try { fs.rmSync(workdir, { recursive: true, force: true }); } catch { /* noop */ } });
109+
});
110+
111+
test("api/approvals: a paid delivery is charged exactly once", { skip: !deps && "apps/pwa deps not installed" }, async () => {
112+
const { seedUser, charges, ingest, balance } = await app();
113+
114+
const key = await seedUser("u-single", { credits: 12, channels: [["sms", "+15550000"]] });
115+
const res = await ingest(key, "ship it?");
116+
117+
assert.equal(res.status, 201);
118+
assert.deepEqual(res.body.delivered, ["sms"]);
119+
assert.equal(res.body.charged, 12);
120+
assert.deepEqual((await charges("u-single")).map((r) => Number(r.delta)), [-12]);
121+
assert.equal(await balance("u-single"), 0);
122+
});
123+
124+
test("api/approvals: concurrent ingests cannot spend the same credits twice", { skip: !deps && "apps/pwa deps not installed" }, async () => {
125+
const { seedUser, charges, ingest, balance } = await app();
126+
127+
// A moshscript can fire ask()/notify() in parallel, so two ingests for one
128+
// account are in flight at once. Both read a balance that covers one paid
129+
// delivery — only one of them may actually get it.
130+
const key = await seedUser("u-race", { credits: 12, channels: [["sms", "+15550000"]] });
131+
const [a, b] = await Promise.all([ingest(key, "deploy A?"), ingest(key, "deploy B?")]);
132+
133+
assert.equal(a.status, 201);
134+
assert.equal(b.status, 201);
135+
assert.deepEqual((await charges("u-race")).map((r) => Number(r.delta)), [-12]);
136+
assert.equal(await balance("u-race"), 0);
137+
// exactly one of the two got the paid channel; the other was told why not
138+
const paid = [a, b].filter((r) => r.body.delivered.includes("sms"));
139+
assert.equal(paid.length, 1);
140+
const refused = [a, b].find((r) => !r.body.delivered.includes("sms"));
141+
assert.match(refused.body.warning, /insufficient credits/);
142+
});
143+
144+
test("api/approvals: a second sequential ingest falls back to free channels", { skip: !deps && "apps/pwa deps not installed" }, async () => {
145+
const { seedUser, ingest, balance } = await app();
146+
147+
const key = await seedUser("u-seq", { credits: 12, channels: [["sms", "+15550000"], ["webhook", "https://example.test/hook"]] });
148+
const first = await ingest(key, "first");
149+
const second = await ingest(key, "second");
150+
151+
assert.deepEqual(first.body.delivered, ["sms", "webhook"]);
152+
assert.equal(first.body.charged, 12);
153+
assert.deepEqual(second.body.delivered, ["webhook"]); // free channel still goes out
154+
assert.equal(second.body.charged, 0);
155+
assert.match(second.body.warning, /insufficient credits/);
156+
assert.equal(await balance("u-seq"), 0);
157+
});
158+
159+
test("api/approvals: a channel that fails to deliver is not charged for", { skip: !deps && "apps/pwa deps not installed" }, async () => {
160+
const { seedUser, charges, ingest, balance } = await app();
161+
162+
// slack has no webhook target and no configured default, so it refuses the
163+
// send. The ledger must show only what actually went out.
164+
const key = await seedUser("u-partial", { credits: 16, channels: [["sms", "+15550000"], ["slack", null]] });
165+
const res = await ingest(key, "half of this lands");
166+
167+
assert.deepEqual(res.body.delivered, ["sms"]);
168+
assert.equal(res.body.charged, 12);
169+
assert.deepEqual((await charges("u-partial")).map((r) => Number(r.delta)), [-12]);
170+
assert.equal(await balance("u-partial"), 4); // the 4 held for slack is released
171+
});
172+
173+
test("api/approvals: a free-only account is never charged", { skip: !deps && "apps/pwa deps not installed" }, async () => {
174+
const { seedUser, charges, ingest, balance } = await app();
175+
176+
const key = await seedUser("u-free", { credits: 0, channels: [["webhook", "https://example.test/hook"]] });
177+
const res = await ingest(key, "free ping");
178+
179+
assert.deepEqual(res.body.delivered, ["webhook"]);
180+
assert.equal(res.body.charged, 0);
181+
assert.equal(res.body.warning, undefined);
182+
assert.deepEqual(await charges("u-free"), []); // no zero-value ledger noise
183+
assert.equal(await balance("u-free"), 0);
184+
});

0 commit comments

Comments
 (0)