fix(labeler): keep assessment-pending live until the last concurrent run resolves (held review finding #13)#2168
Conversation
…run resolves Concurrent assessment runs for the same (uri,cid) all target the single assessment-pending label, so one run's finalization negation cleared the shared gate while a sibling run was still active — briefly exposing a stale pass. Gate the pending negation, in-batch, on no other non-terminal run for the subject (requireNoOtherActiveRun), so only the last run to resolve clears it. Make the delete cleanup's pending scan label-driven (any run holding a committed un-negated positive, at any state) so a decision run that now finalizes while still holding a positive is still negated on delete.
|
github-actions
Bot
added
review/needs-review
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | 8322358 | Jul 21 2026, 07:06 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 8322358 | Jul 21 2026, 07:04 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 8322358 | Jul 21 2026, 07:06 PM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for finding #13. Gating the assessment-pending negation on “no other non-terminal run for the same (uri, cid)” inside the same db.batch as the running → terminal CAS is a clean projection that preserves the deliberate “rerun mints a fresh run” semantics. Making the delete cleanup’s scan label-driven (all non-terminal runs plus any terminal run still holding a live positive) correctly closes the delete-vs-issuance gap created when a terminal run suppresses its own pending negation because a sibling is still active.
I traced the SQL bind ordering in buildIssuanceStatements, the placement of pendingNegation.statements in the finalization batch versus the assessment-update CAS, the new listPendingBearingAssessmentsForUri predicate, the widened terminal-state cancel-skip in the discovery consumer, and the new TDD tests. I found no logic bug, SQL-injection risk, authorization gap, i18n/RTL issue, missing locale filter, or logged-out query-count impact. One inline comment in the delete handler is now stale after the scan widened beyond terminal stale.
Findings
-
[suggestion]
apps/labeler/src/discovery-consumer.ts:625This inline comment is stale after the cleanup scan/skip were widened.
listPendingBearingAssessmentsForUrinow returns all non-terminal runs plus any terminal run holding a live, un-negated positive — not only terminalstaleruns — and the skip below isTERMINAL_STATES.has(run.state), not juststale.// Negate (before any cancel) any run — non-terminal or terminal — that // committed a positive pending and has not already been negated.
github-actions
Bot
added
needs-rebase
review/awaiting-author
…ervice' into fix/labeler-sol-13-concurrency # Conflicts: # apps/labeler/test/assessment-orchestrator.test.ts
|
Fixed the stale comment in That commit is also the reconciliation merge with the integration branch after #2166 (the 9/14/19 batch) merged: the finalization CAS now carries the automation-not-paused guard (14), the subject-not-deleted guard (#2115), and the signing guard together with this PR's ~ 🤖 Claude Fable 5 |
abe4d72
into
feat/plugin-registry-labelling-service
1 participant
What does this PR do?
Closes held finding #13 from the adversarial review of the labeler (RFC #694, umbrella #1909) — parked during the earlier fix PRs, now addressed off the merged tip. TDD'd and independently adversary-reviewed (PASS, no #2115 regression). Targets the
feat/plugin-registry-labelling-serviceintegration branch.The race.
assessment-pendingis a real atproto label whose subject state is the highest-sequenceop per(src,uri,val). Concurrent assessment runs for the same(uri,cid)(different run keys → separate Workflow instances, by design so an operator rerun mints a fresh run) each issue their own positive/negation, but all target the sameval— so any run's negation becomes the stream head and clears the gate for the whole subject. If run A finalizespassedwhile run B is still active, A's negation clears the pending gate, briefly flipping the release frompendingto eligible (exposing A's stale pass) until B finalizes and re-blocks.The fix — a pending projection via last-run-clears. The
assessment-pendingnegation is now gated, in the samedb.batch, on no other non-terminal run existing for the same(uri,cid)(a newrequireNoOtherActiveRunguard onbuildIssuanceStatements, alongside the existing signing /requireAssessmentState/ subject-not-deleted guards). D1 serializes batch transactions and each run'srunning→terminalCAS commits in the same batch, so the last run to resolve is the only one that sees no non-terminal sibling and negates — the gate clears exactly once, when the last run finishes. Self-exclusion is by run id, so it holds regardless of batch ordering. Keyed on(uri,cid), so distinct CID versions never entangle. Serialization was rejected in favor of the projection because it would regress the deliberate rerun-mints-a-fresh-run semantics.Because a run can now finalize
passed/blockedwhile still holding a live positive (its own negation suppressed), the delete cleanup's pending scan (listPendingBearingAssessmentsForUri) is made label-driven — it now negates any run of any state that holds a committed, un-negated positive, plus all non-terminal runs — so the #2115 delete-vs-issuance closure is preserved (the adversary confirmed it's a strict superset of the old state-based scan). The cleanup's cancel-skip widens fromstate==="stale"to all terminal states (a terminal run is negated but not fed an illegal→cancelledtransition).Known follow-up (deferred, not a regression): the pure CID-supersession case — a run self-
staled because its CID was superseded (no delete event) — leaves an old-CID positive that this fix doesn't negate. Negating it would clobber the new CID's legitimate pending (the negation would be the CID-mismatched stream head). Both real consumers (enforcement andgetActiveLabelState) are CID-scoped, and the new-CID run's own pending is the stream winner, so the lingering old-CID positive is a stale audit signal on a superseded CID, not a false-eligibility signal. Tracked as a CID-aware pending-reduction follow-up if the audit-view wart matters.Type of change
Checklist
pnpm typecheckpasses (all three labeler projects)pnpm lintpasses (0 diagnostics)pnpm testpasses (labeler: 904 + 136 + 39) — TDD: the pending-gate test asserts A's negation is suppressed while a sibling is active (fails pre-fix, where finalize unconditionally negated); the delete test asserts a terminal decision run's live positive is negated on delete (fails pre-fix, where the state-based scan excludedpassed)pnpm formathas been run@emdash-cms/labeleris private.AI-generated code disclosure
Screenshots / test output
Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
fix/labeler-sol-13-concurrency. Updated automatically when the playground redeploys.