From 7053ac857904723bcf0d43ba40ca8fd2e3ffea82 Mon Sep 17 00:00:00 2001 From: Saty <5752427+SatyPardus@users.noreply.github.com> Date: Sun, 25 Oct 2026 17:24:43 +0100 Subject: [PATCH] Peer review queue uses the same rules as the decision The queue judged "decided" by weight alone, while a decision also takes two reviewers on the winning side. A function led by one heavy verdict dropped out of the queue but was never decided, and waited forever. Co-Authored-By: Claude Opus 5.5 --- src/server/peer.js | 42 +++++++++++++++++++++++------------------- test/server.test.js | 8 +++++++- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/server/peer.js b/src/server/peer.js index f237750..cee0928 100644 --- a/src/server/peer.js +++ b/src/server/peer.js @@ -94,27 +94,24 @@ export class PeerReviews { // Room for this reviewer is part of the query, so the limit counts functions they can take // (filtering after the limit returned an empty list while the first rows were fully leased). const rows = this.store.all( - `SELECT * FROM ( - SELECT a.id AS attempt_id, a.batch_id, a.task_id, a.state, t.symbol_id, s.name, s.unit, a.finished_at, - (SELECT COALESCE(SUM(p.weight), 0) FROM peer_reviews p WHERE p.attempt_id = a.id AND p.verdict = 'same') AS confirmations, - (SELECT COALESCE(SUM(p.weight), 0) FROM peer_reviews p WHERE p.attempt_id = a.id AND p.verdict = 'different') AS objections, - (SELECT COUNT(*) FROM peer_reviews p WHERE p.attempt_id = a.id) AS reviewers, - (SELECT COUNT(*) FROM peer_leases l WHERE l.attempt_id = a.id AND l.expires_at > ?) AS in_progress - FROM attempts a JOIN tasks t ON t.id = a.task_id JOIN symbols s ON s.id = t.symbol_id - WHERE a.state IN ('peer_review', 'in_review') AND a.user_id != ? - AND NOT EXISTS (SELECT 1 FROM users b WHERE b.id = ? AND b.peer_review_blocked = 1) - AND NOT EXISTS (SELECT 1 FROM peer_reviews p WHERE p.attempt_id = a.id AND p.reviewer_id = ?) - ) - WHERE NOT (objections >= ? AND objections - confirmations >= ?) AND confirmations + in_progress < objections + ? AND reviewers + in_progress < ?`, + `SELECT a.id AS attempt_id, a.batch_id, a.task_id, a.state, t.symbol_id, s.name, s.unit, a.finished_at, + (SELECT COALESCE(SUM(p.weight), 0) FROM peer_reviews p WHERE p.attempt_id = a.id AND p.verdict = 'same') AS confirmations, + (SELECT COALESCE(SUM(p.weight), 0) FROM peer_reviews p WHERE p.attempt_id = a.id AND p.verdict = 'different') AS objections, + (SELECT COUNT(*) FROM peer_reviews p WHERE p.attempt_id = a.id AND p.verdict = 'same') AS same_reviewers, + (SELECT COUNT(*) FROM peer_reviews p WHERE p.attempt_id = a.id AND p.verdict = 'different') AS different_reviewers, + (SELECT COUNT(*) FROM peer_leases l WHERE l.attempt_id = a.id AND l.expires_at > ?) AS in_progress + FROM attempts a JOIN tasks t ON t.id = a.task_id JOIN symbols s ON s.id = t.symbol_id + WHERE a.state IN ('peer_review', 'in_review') AND a.user_id != ? + AND NOT EXISTS (SELECT 1 FROM users b WHERE b.id = ? AND b.peer_review_blocked = 1) + AND NOT EXISTS (SELECT 1 FROM peer_reviews p WHERE p.attempt_id = a.id AND p.reviewer_id = ?)`, now(), user.id, user.id, user.id, - this.required, - this.rejectMargin, - this.margin, - this.maxReviewers, - ).filter((r) => r.confirmations + r.in_progress < this.needed(r.objections)); + ).filter((r) => { + r.reviewers = r.same_reviewers + r.different_reviewers; + return !this.full({ same: r.confirmations, different: r.objections, sameReviewers: r.same_reviewers, differentReviewers: r.different_reviewers }, r.in_progress); + }); // Random by batch, so a batch stays together. const key = new Map(); for (const r of rows) { @@ -231,8 +228,7 @@ export class PeerReviews { const leased = this.store.get('SELECT 1 FROM peer_leases WHERE attempt_id = ? AND reviewer_id = ? AND expires_at > ?', attemptId, user.id, now()); const counts = this.counts(attemptId); const others = this.store.get('SELECT COUNT(*) AS n FROM peer_leases WHERE attempt_id = ? AND reviewer_id != ? AND expires_at > ?', attemptId, user.id, now()).n; - const full = counts.same + others >= this.needed(counts.different) || counts.sameReviewers + counts.differentReviewers + others >= this.maxReviewers; - if (!leased && full) throw new HttpError(409, 'enough reviewers are already on this attempt; claim another'); + if (!leased && this.full(counts, others)) throw new HttpError(409, 'enough reviewers are already on this attempt; claim another'); const weight = this.trust?.reviewer(user.id, user.tokenId ?? null).weight ?? 1; this.store.run( @@ -357,6 +353,14 @@ export class PeerReviews { return c.different >= this.required && c.different - c.same >= this.rejectMargin && c.differentReviewers >= 2; } + // No room for another reviewer: decided, or decided once the `inProgress` leased reviews come + // back "same", or at `maxReviewers`. The same rules as decide(), reviewer counts included, so a + // function one heavy verdict leads is still offered until a second reviewer settles it. + full(c, inProgress = 0) { + const hoped = { ...c, same: c.same + inProgress, sameReviewers: c.sameReviewers + inProgress }; + return this.rejects(c) || this.accepts(hoped) || c.sameReviewers + c.differentReviewers + inProgress >= this.maxReviewers; + } + /** Passed peer review, or a trusted author's function that did not need it (trust.js). */ confirmed(attemptId) { if (this.store.get('SELECT 1 FROM attempts WHERE id = ? AND peer_skipped = 1', attemptId)) return true; diff --git a/test/server.test.js b/test/server.test.js index ccde157..a10a9f4 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -1525,8 +1525,14 @@ test('an admin sets a reviewer weight: it counts at once, also on verdicts alrea const alone = open(); assert.equal(ctx.peers.submit(trusted, alone, 'same', why, null).weight, 3); assert.equal(ctx.store.get('SELECT state FROM attempts WHERE id = ?', alone).state, 'peer_review', 'one reviewer never decides'); + assert.ok(ctx.peers.available(other, 1000).some((o) => o.attempt_id === alone), 'still offered: a second reviewer settles it'); + ctx.peers.submit(other, alone, 'same', why, null); + assert.equal(ctx.store.get('SELECT state FROM attempts WHERE id = ?', alone).state, 'approved'); + const objected = open(); + ctx.peers.submit(trusted, objected, 'different', 'the loop bound is off by one against the original', null); + assert.ok(ctx.peers.available(other, 1000).some((o) => o.attempt_id === objected), 'one heavy "different" alone is offered too'); ctx.peers.setReviewWeight(trusted.id, null); assert.equal(ctx.trust.reviewer(trusted.id).weight, 1, 'auto: back to the earned weight'); - ctx.store.run(`UPDATE attempts SET state = 'superseded' WHERE id IN (?, ?)`, waiting, alone); + ctx.store.run(`UPDATE attempts SET state = 'superseded' WHERE id IN (?, ?, ?)`, waiting, alone, objected); ctx.store.run('UPDATE tasks SET state = ? WHERE id = ?', saved.state, saved.id); });