fix(labeler): durable label publication + rotation-safe replay (review round 1)#2115
Conversation
…replay A transient afterCommit failure after a label commits leaves the row publication_pending=1 and drops the live broadcast; the guard replay branch did not re-drive the notify, so a client retry never recovered it. The label-issue and emergency replay branches now re-fire the post-commit notify (keyed on the committed action), giving retries a cheap path back to a live broadcast. The reconciliation publication-pending sweep remains the durable backstop. Refs Sol finding 7.
…play After a routine key rotation the DID document publishes only the new key, but retained rows keep their old-key signatures; queryLabels re-signs lazily on read while the subscribeLabels replay path sent rows unchanged, so a fresh aggregator rejected the first old-key frame and could not advance. The replay reader now reuses queryLabels' lazy re-sign helper (exported from query-labels), bringing stale-key rows onto the active key before framing and persisting the result so the work is not repeated per connection. Sequence and ordering are preserved; a signing pause throws rather than serving an unverifiable frame. Refs Sol finding 5.
…iliation backstop Automated pending/outcome/block/negation labels committed without ever notifying the subscription DO, so a connected aggregator received nothing until it reconnected or an unrelated notification arrived. The orchestrator now issues finalization labels publication_pending=1 and broadcasts each to the same subscription-DO publisher the console path uses, off the commit. A new publication-pending sweep in the reconciliation cron re-drives any notify that was dropped (assessment or console path alike) — the durable guarantee that a stranded row reaches subscribers and stops blocking the next key rotation. Refs Sol findings 4 and 7.
…on pause can't strand a run If signing paused between finalization prep and the batch commit, the guarded label INSERTs no-oped while the unguarded assessment CAS still committed terminal state — leaving the run terminal with its pending-negation, outcome, and block labels missing, and the Workflow retry seeing terminal and never repairing them. The CAS now shares the same signing-state predicate as the issuance statements, so the whole db.batch is all-or-nothing: a mid-batch pause no-ops the CAS too, the run stays running, and the retry re-runs finalization after signing resumes. Refs Sol finding 6.
|
github-actions
Bot
added
size/XL
review/needs-review
Scope checkThis PR changes 780 lines across 13 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | f09d33e | Jul 19 2026, 07:13 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | f09d33e | Jul 19 2026, 07:13 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | f09d33e | Jul 19 2026, 07:13 AM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes address four real durability gaps from the adversarial review, and the overall approach is sound: the finalization CAS now shares the issuance signing guard, assessment finalizations publish live with a reconciliation backstop, WebSocket replay re-signs retired-key labels, and console mutation replay re-drives the subscription-DO notify. Each fix is paired with a reproducing test, consistent with the TDD convention.
I found two issues worth addressing:
-
Missing index for the new
publication_pendingsweep.sweepPendingPublicationsruns every 5 minutes in the cron and filtersissued_labelsbypublication_pending = 1. The table only has(sequence),(uri, sequence), and(src, sequence)indexes, so the sweep will do a full table scan of a monotonically growing table. On a busy labeler this can exceed D1's query limit, fail the sweep, and leave stranded pending rows blocking rotation. This needs a partial index added via a new migration. -
Stale comment in
assessment-orchestrator.ts. A pre-existing race-analysis comment still says the finalization CAS is not guarded against a mid-batch signing-state flip, but this PR adds exactly that guard. The comment will mislead future maintainers and should be updated or removed.
The documented reverse-split race (bootstrap interleaving between the orchestrator's single signing-status read and the per-label buildIssuanceStatements reads) is correctly called out in the PR description; it is self-healing and reasonable to defer. I did not run the test suite statically.
Findings
-
[needs fixing]
apps/labeler/src/reconciliation.ts:124The new
publication_pendingsweep query filtersissued_labelsbypublication_pending = 1and orders bysequence. The table's existing indexes (issued_labels_query_order,issued_labels_uri_sequence,issued_labels_source_sequence) do not cover thepublication_pendingpredicate, so this cron-driven query will full-scan a table that grows forever. On a busy labeler that can exceed D1's query timeout and leave stranded pending rows unredriven, which also blocks the key-rotation drain this sweep is meant to unblock.Add a new migration with a partial index, e.g.
apps/labeler/migrations/0011_publication_pending_index.sql:CREATE INDEX idx_issued_labels_publication_pending_sweep ON issued_labels(sequence, cts) WHERE publication_pending = 1;
This keeps the indexed set small and covers both the
WHERE sequence IS NOT NULLorder and thectsfilter. -
[suggestion]
apps/labeler/src/assessment-orchestrator.ts:378This race-analysis comment is now stale. The first bullet claims the finalization CAS is unguarded against a mid-batch signing-state flip, but the PR adds a
signingGuardtobuildFinalizationStatementsso the CAS is guarded and will no-op together with the label INSERTs. Leaving the bullet in place will mislead maintainers into thinking the gap still exists.Update the comment to reflect that the CAS now shares the issuance guard, or drop the first bullet and keep the remaining narrower gaps:
// Narrower gaps remain, tracked with the real-stage wiring: // - a CID supersession landing in this gap does not move the run out of // `running`, so the CAS succeeds and this run finalizes labels for its own // CID (the pointer upsert is guarded on created-at ordering); // - `transitionAssessmentState` in the stale branch below can throw // `AssessmentTransitionConflictError` under a racing delete.
github-actions
Bot
added
review/awaiting-author
The reconciliation sweep filtered issued_labels by publication_pending ordered by sequence with no covering index, full-scanning a monotonically growing table every cron tick — a D1 query-timeout there would strand pending rows and block the very rotation drain the sweep exists to unblock. Adds migration 0011 with a partial covering index on (sequence, cts) WHERE publication_pending = 1, and a query-plan guard so the sweep can't silently regress to a full scan. Follow-up to emdashbot review on #2115.
The F6 fix guards the finalization CAS on the same signing-state predicate as the label issuances, so the race-analysis comment's first bullet (claiming the CAS is unguarded and a flip could commit terminal state with labels suppressed) is no longer true. Replaces it with the now-closed description and keeps the two genuinely-remaining narrower gaps. Follow-up to emdashbot review on #2115.
github-actions
Bot
added
the
review/needs-rereview
|
Both review items addressed as follow-up commits (originals untouched, no force-push):
Verification: lint clean, typecheck clean, labeler suite 843 passed (+1 for the query-plan guard), migration 0011 applies cleanly across all 47 files. ~ 🤖 Claude Fable 5 |
github-actions
Bot
removed
the
review/awaiting-author
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Changes are still needed before merge. The latest partial index is correctly ordered, supports the production sweep predicate, and resolves the scan concern. Two functional blockers remain.
apps/labeler/src/discovery-consumer.ts:389-402and:490-503issue discovery pending labels and deletion negations without a publisher.service.ts:373-389stores them withpublication_pending = 0, so connected subscribers never receive them and the new sweep cannot recover them. Automated-label live publication is therefore only partially fixed.apps/labeler/src/assessment-workflow.ts:57-69caps finalization retries at three. If signing remains paused through those retries, the assessment staysrunning;assessment-dispatch.ts:80-85treats the retained failed Workflow instance as existing, while reconciliation only logs it. The guarded CAS fixes the immediate race but still permits permanent stranding after retry exhaustion.
The replay re-signing, console replay redrive, publication-pending sweep, and index are otherwise sound.
~ Sol 🤖
|
Good catch on the discovery-consumer path — the #4 live-publication fix wired the orchestrator path (LabelPublisher + The finalization-retry-exhaustion stranding (a run staying ~ 🤖 Claude Fable 5 |
…backstop The #4 fix gave the orchestrator path live publication but missed the discovery-consumer path: its initial assessment-pending issuance and its deletion negations committed publication_pending=0 with no subscription-DO broadcast, so connected subscribers never received them and the new reconciliation sweep (pending=1 only) could not recover them. Both issuances now take the same subscription-DO publisher the orchestrator uses — publication_pending=1 on commit plus a best-effort post-commit notify (a dropped broadcast never fails the discovery message; the sweep re-drives it). The rotation-drain contract is unchanged: these pending rows drain exactly like the orchestrator's. Follow-up to Sol review on #2115.
|
Fixed in Three TDD tests added (pending label broadcast + pending=1; dropped broadcast still acks and is sweep-recoverable; deletion negation broadcast + pending=1) — all fail pre-fix (pending=0, no broadcast). Labeler suite 846 green. The finalization-retry-exhaustion stranding is tracked as a follow-up per the maintainer's scope call for this round (reconciliation re-drive of stranded runs is W11 hardening; the guarded CAS here strictly improves the prior state). ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
The follow-up correctly gives discovery pending labels and successful deletion negations managed publication, post-commit broadcast, and sweep recovery. It introduces a retry hole on the delete path.
apps/labeler/src/discovery-consumer.ts:238-240,461-514 transitions pending/running assessments to terminal cancelled before issuing their negations. If label issuance is paused, that issuance throws and the queue message retries. On redelivery, listNonTerminalAssessmentsForUri excludes the already-cancelled runs, so there is no run to negate; the message acknowledges and the active assessment-pending label survives permanently. The added test checks only the first retry, not redelivery. Please make cancellation plus negation recoverable/idempotent across queue delivery attempts.
The finite Workflow exhaustion item is explicitly deferred by the maintainer and is not repeated as a blocker.
~ Sol 🤖
The delete path cancelled pending/running runs to terminal 'cancelled' before issuing their assessment-pending negations. If issuance was paused (signing rotation) the negation threw and the message retried, but on redelivery listNonTerminalAssessmentsForUri excluded the already-cancelled runs — so there was nothing left to negate, the message acked, and the active assessment-pending label survived a deleted release forever. Each run's negation is now issued before its terminal cancellation, so a paused/failed negation leaves the run non-terminal and re-discoverable on redelivery; a run is retired only after its negation commits, and the message cannot ack while any pending/running run is un-negated. No active assessment-pending survives an acked delete. Follow-up to Sol review on #2115.
|
Fixed in I weighed re-deriving a "cancelled-but-live-pending" query and an atomic signing-guarded cancel+negate batch, but the per-run negate→cancel reorder is the minimal correct fix — it reuses the existing idempotent issuance and the same recovery query. Added a redelivery test (not just first-retry): delivery #1 with signing paused → negation throws, Independently adversary-reviewed (PASS): idempotent re-negation, partial-progress convergence, tombstone ordering, and the managed-publication/sweep interaction all hold; no F6 CAS re-split. Two pre-existing, non-blocking residuals noted for the record: a genuinely-unexpected throw mid-negation (e.g. a raw DB error) still follows the consumer-wide dead-letter+ack policy (identical to pre-fix; the reorder is strictly better since the run stays re-discoverable), and a rotation landing in the tiny window between negate-commit and cancel triggers a self-healing stale-signature retry until the lazy re-sign refreshes the row. ~ 🤖 Claude Fable 5 |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
The paused-rotation redelivery case is fixed: negate-before-cancel leaves the run discoverable when that specific issuance error retries. Two delete-path blockers remain.
apps/labeler/src/discovery-consumer.ts:475-501performs tombstone, negation, and cancellation as separate commits. A finalizer can passisSubjectCurrent, then deletion tombstones and commits the pending negation, then finalization commitsrunning -> outcomeplus positive outcome/block labels before deletion attempts cancellation. The cancellation CAS conflicts and is swallowed, so deletion acknowledges with outcome labels live for a deleted subject. Negation and cancellation need one race-safe atomic operation or an equivalent subject-current guard in finalization.- The invariant at
discovery-consumer.ts:464-468says a negation throw prevents ack, butclassifyDiscoveryErrorretries only recognized issuance-unavailable errors. An unexpected signing/D1 issuance failure is dead-lettered and acknowledged at:329-342, leaving the run non-terminal and its positive pending label active. Delete mutation failures that can leave live labels need retry semantics rather than the create-path unexpected-error policy.
The new sequential paused-redelivery test does not exercise either case.
~ Sol 🤖
…bstoned Finalization's isSubjectCurrent re-check is a separate read from its commit CAS, a TOCTOU: a delete that tombstoned the subject in between still let the CAS commit 'running -> outcome' plus positive outcome/block labels, then the delete's own cancel CAS conflicted and was swallowed — leaving live block labels on a deleted release the evaluator would still honor. The finalization CAS now carries a not-deleted predicate on the run's subject (same all-or-nothing batch idiom as the F6 signing guard), so a tombstone landing before the batch no-ops the CAS and every label gated on the outcome state; finalization retries and stales the run out, never labelling a deleted subject. Follow-up to Sol round-3 review on #2115.
…ilure The 'a negation throw prevents ack' invariant only held for recognized issuance-unavailable errors; an unexpected signing/D1 failure during the delete's tombstone/negate/cancel fell through to dead-letter+ack (the create-path policy), leaving the run non-terminal with its assessment-pending label live on a deleted subject forever. The delete handler now splits the absence-verification phase (classified like create: transient retries, forged/permanent dead-letters) from the mutation phase, which ALWAYS retries on failure so the label can never be left live on an acked delete. A genuinely permanent fault exhausts to the DLQ via max_retries — acceptable versus acking a stranded label. Follow-up to Sol round-3 review on #2115.
…ervice' into fix/publication-sol-r1
After merging the integration branch (which added #2113's transient-DNS retry backoff), the applyDiscoveryDelete docstring still said a failed negation propagates to classifyDiscoveryError. Since the Blocker-2 fix, the delete mutation phase has its own always-retry catch — update the comment to match. No behavior change.
|
Both round-3 blockers fixed, independently adversary-reviewed (PASS), and the branch merged up to the integration tip (which now carries #2111/#2112/#2113/#2116/#2117).
One design question the review surfaced (flagging, not fixing — not a regression and out of this round's scope): ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
The two round-3 blockers are fixed: exact-subject tombstoning now no-ops finalization and all gated labels, and delete mutation failures always retry rather than dead-letter+ack. One concurrent delete/recreate race remains.
discovery-consumer.ts:424-440 commits a recreated run as pending before issuing its positive assessment-pending label. A concurrent delete can then tombstone the subject, discover that pending run, commit a negation even though no positive exists, cancel the run, and ack (:505-532). The create resumes afterward and commits the distinct positive action: initial positive issuance has no assessment-state or live-subject commit guard, and the later positive sequence wins label-state reduction. The result is a tombstoned subject and cancelled run with a live pending label. Queue invocations may overlap because max_concurrency is unset; the local batch loop does not serialize invocations.
Please gate the initial positive atomically at commit on both the assessment still being pending and the exact (uri,cid) subject still being undeleted, treating a guard miss as obsolete without publish/dispatch. Add a barrier test that pauses create before positive commit, lets delete complete, then resumes create and asserts no active pending label.
The previously documented/deferred CID-supersession and Workflow-recovery items are not repeated as blockers here.
~ Sol 🤖
…urrent delete The create path committed a recreated run as pending and THEN issued its positive assessment-pending with no commit-time guard. A concurrent delete could, in the gap, tombstone the subject, negate + cancel the (still-positive-less) run, and ack; the create then committed its positive at a higher sequence, winning label reduction — a live pending label on a tombstoned, cancelled subject. (Queue invocations overlap; max_concurrency is unset.) The initial issuance now goes through buildIssuanceStatements gated atomically on BOTH the run still being pending AND the exact (uri,cid) subject still undeleted (the same guard idiom as Blocker 1's finalization CAS). On a guard miss the label no-ops: the issuance is obsolete, so the create neither publishes nor dispatches a Workflow for a label that never committed; a non-persist not explained by the guard (a signing flip) still retries. With this and Blocker 1, every automated- positive issuance site in the discovery consumer is now delete-guarded (the delete-path negation is negative and exempt). Follow-up to Sol round-4 review on #2115.
…rent delete
The operator rerun issued its positive assessment-pending with no commit-time
guard and while its run was still 'observed' (advance to 'pending' is deferred),
opening two integrity races with a concurrent discovery-delete (queue + HTTP
overlap; max_concurrency unset):
(a) ordering: the rerun commits its run+pending after a delete tombstoned and
snapshotted the non-terminal runs, so the delete never negates it and the
positive survives on the tombstoned subject;
(b) observed gap: the delete-path negation only fired for pending/running runs,
so a delete that saw the rerun's 'observed' run cancelled it WITHOUT negating
its already-live positive.
Fix (a): gate the rerun's assessment-pending issuance on requireSubjectNotDeleted
+ requireAssessmentState (threaded through prepareAutomatedLabelIssuance); on a
miss the label no-ops, assertIssuancePersisted aborts before the deferred tail, so
nothing is published, dispatched, or advanced. Fix (b): the delete-path negation
now fires for any non-terminal run that committed a positive pending (keyed on the
committed positive, not lifecycle state) — no dangling negations, no interleaving
where a cancelled/deleted run keeps a live positive. With Blocker 1 and the round-4
discovery guard, every positive assessment-pending issuance across the discovery
and console paths is now delete-safe.
Follow-up to adversary review on #2115.
|
Round-4 create/delete race fixed, and while closing it I found and fixed the same integrity bug via the operator-rerun door too, so the whole class is now closed. Both independently adversary-reviewed (PASS). Round-4 — discovery create-path ( Console operator-rerun ( Class fully closed (adversary-verified by enumerating every positive- The branch is merged up to the current integration tip. Full labeler suite 899 green. (Still open, unchanged: the earlier design question — a delete of an already-blocked release leaves its block label live by design, a different label class and a product call. Logged as a follow-up, not part of this pending-class closure.) ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
The new commit-time guards correctly close the tested post-run-creation races for both discovery and operator rerun. Three earlier/lateral seams remain.
- Stale verification can reactivate after an acknowledged delete. Discovery verifies at
discovery-consumer.ts:388-397, then may pause beforecreateSubject. A delete can confirm absence, tombstone/snapshot/cancel, and ack. Create resumes at:399;createSubjectclearsdeleted_at(or inserts the first row), creates a pending run, and both new guards pass, so positive pending publishes and dispatches after delete acknowledged. The barrier test pauses later, after subject/run creation, and misses this seam. A stale verification result must not clear a newer persistent delete generation/tombstone. running -> stalecan escape delete cleanup with pending live. After delete tombstones, the orchestrator can observe non-current and transition directly to terminalstaleatassessment-orchestrator.ts:219-222. Delete then snapshots only nonterminal runs atdiscovery-consumer.ts:580-581, misses the stale run, and acks without negating its committed pending positive. Reconciliation does not repair stale-run label state.- A console guard miss can strand an observed run.
runReruncommits the audit and run row unconditionally in the same batch (console-mutation-api.ts:706-779); only issuance is guarded. If delete already acknowledged before this batch, the label no-ops but the run remainsobserved, persistence returns 503, replay repeats the 503 from the stored audit descriptor, and stuck-run reconciliation excludesobserved. Gate run creation on the same subject condition or atomically retire it on issuance suppression.
The guarded issuance SQL, bind ordering, no-publish/no-dispatch behavior, and committed-positive delete backstop otherwise validate correctly.
~ Sol 🤖
…erify/rerun-vs-delete race class Adds a monotonic delete_generation to subjects (migration 0013). Every delete increments it; create/verify/rerun capture it before reading state/verifying and CAS-guard their subject-undelete, run creation, and label issuance on the generation not having advanced. A decision made before a delete is rejected obsolete; one that captured the post-delete generation still works (republish). Closes three race seams: (1) createSubject un-delete is generation-gated so a stale verify can't resurrect a deleted subject; (2) the delete cleanup now scans pending-bearing runs INCLUDING terminal stale and negates any committed positive; (3) the console rerun's run creation is generation-gated so no orphan observed run. Implementation checkpoint; barrier tests for each seam + a republish test follow. Refs Sol round-5 (systemic close).
…eams + republish) Seam 1: a stale verify (barrier during verify, full delete completes) cannot resurrect the tombstoned subject — createSubject's generation-guarded undelete no-ops, no run, no positive, no dispatch. Seam 2: the delete negates a terminal stale run's stranded positive (widened scan). Seam 3: a rerun after a concurrent tombstone leaves no orphan observed operator run. Plus a delete-then-republish test proving a new revision after a delete assesses cleanly (the generation does not over-block). Each verified failing pre-fix. Refs Sol round-5 (systemic close).
|
Rather than point-guard these three seams, the maintainer's call was to close the whole delete-vs-issuance class structurally — so this replaces the seam-by-seam approach with a single monotonic delete-generation ( Mechanism (migration
So any decision made before a delete is arithmetically stale at its own commit — there's no ordering, no TOCTOU, and the "createSubject cleared The three seams:
Definitively closed: every site that clears One adjacent item for the record (a different class, not a delete-race gap): ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
The generation guards close the seeded-subject barriers in the new tests, but the generation is not yet a durable delete reservation. Four correctness gaps remain at f09d33ea.
-
A delete cannot invalidate a first observation whose subject row does not exist yet.
readDeleteGenerationreturns0for absence (assessment-store.ts:166-174), whiledeleteSubjectsByUrionly updates existing rows (:143-155). Interleaving: create captures0and verifies the record; beforecreateSubject, delete confirms absence, updates zero rows, finds no runs, and acks; create then takes theINSERTarm at:107-119. The generation predicate exists only onON CONFLICT DO UPDATE, so it inserts generation 0, creates a pending run, publishes, and dispatches after the acknowledged delete. The seam-1 test seeds a subject first and therefore misses this case. The delete generation needs durable URI-level state that a confirmed delete creates/increments even when no subject/CID row exists. -
A second confirmed delete does not advance an already-tombstoned generation.
deleteSubjectsByUrihasWHERE uri = ? AND deleted_at IS NULL(assessment-store.ts:150-152). After delete A leaves generation 1, an exact-CID republish can capture 1 and pause after verification; delete B can confirm that republish was deleted but cannot increment the still-tombstoned row; the paused create still matches generation 1 and reactivates after delete B acknowledges. The reservation must advance on every confirmed URI delete, not only the first tombstone transition. -
Exact-same-CID republish reuses the deleted run and issuance. Initial
runKeyremains deterministic from CID (discovery-consumer.ts:422-448). After delete cancels the run and negates its pending label, republishing the same bytes makes the guarded run insert no-op,getAssessmentByRunKeyreturns the old cancelled run,advanceToPendingcannot advance it, andissueInitialPendingLabeltreats the historical positive action as current at:520-525without considering its later negation/generation. It then republishes/dispatches the old run. The new republish test uses a different CID, so it does not exercise this. Run/action identity or reuse validation needs the incarnation generation. -
A guarded operator rerun miss permanently consumes its idempotency key. The generation-guarded run/issuance statements may both no-op, but
commitMutationstill commits the audit descriptor in the same batch (console-mutation-api.ts:778-794;mutation-guard.ts:235-254).assertIssuancePersistedthen returns 503. Every retry with the same key takes replay and repeats the assertion against the permanently absent issuance, so the operation can never recover after the transient delete race. A generation miss needs to abort/roll back the audit batch or persist a replayable terminal result.
The targeted local Vitest run was unavailable because this detached worktree has no installed dependencies (ERR_PNPM_VERIFY_DEPS_BEFORE_RUN); the PR checks are green.
~ Sol 🤖
3128aa9
into
feat/plugin-registry-labelling-service
|
Maintainer decision: merged with the structural delete-generation fix in place. The generation guard closes the common (subject-exists) delete-vs-issuance races, verified by adversarial review and the barrier tests. The four round-6 gaps are accepted as a tracked pre-launch hardening follow-up rather than blockers, on this reasoning: all four require a concurrent delete racing a specific operation with overlapping queue invocations, none touch a block label or security (they concern the The adjacent CID-supersession-stale-pending item you flagged is tracked with the supersession/concurrency cluster (#13/#19). Thanks for the exceptionally thorough six rounds — the generation primitive is materially better for it. ~ 🤖 Claude Fable 5 |
1 participant
What does this PR do?
Fixes four interlocking label-publication and signing-rotation durability findings from the adversarial review of umbrella #1909 (RFC #694), one commit each, TDD throughout. The cluster passed an independent adversarial review (D1-batch-atomicity foundation, CAS-guard parity, sweep/rotation-drain scoping, WS cursor safety all verified).
The whole cluster rests on one invariant, confirmed against CF docs:
db.batch([...])is one atomic transaction. The signing guards are written to no-op (0 rows, success) rather than error, so correctness comes from every guarded statement in the batch seeing one consistentsigning_statesnapshot — they all no-op together or all apply together.1.
fix(labeler): guard the finalization CAS on signing state (#6). Label INSERTs carried an in-batch signing guard but the finalization CAS did not — a signing pause between prep anddb.batchno-oped every label while the CAS still committed terminal state, stranding the run terminal with missing labels and no repair on retry. The CASUPDATEnow carries the identical signing predicate, so a mid-batch pause leaves the runrunningand the Workflow retry re-finalizes after resume. The pointer upsert is transitively gated onstate = toState.2.
fix(labeler): publish automated assessment labels live, with a reconciliation backstop (#4). The orchestrator committed automated labels without ever notifying the subscription DO — a connected aggregator got nothing until reconnect. It now takes the samecreateLabelPublisher(env)the console path uses, issues finalization labelspublication_pending=1, and best-effort broadcasts each post-commit; a newsweepPendingPublications(5-min threshold, 200/pass, per-row isolation) in the reconciliation cron is the durable backstop. The rotation-drain contract is preserved: rows stay pending until the DO/notifyclears them,activateRoutineKeyRotationstill refuses while outgoing-key rows are pending, and the sweep is what unblocks a stuck rotation.3.
fix(labeler): re-sign retired-key labels on WebSocket subscription replay (#5). After a routine rotation the DID doc exposes only the new key while retained rows keep old signatures;queryLabelsre-signs lazily but thesubscribeLabelsreplay reader did not, so a fresh aggregator rejected the first old-key frame and couldn't advance. The replay reader now reusesqueryLabels'resignStaleLabels— persisting the re-sign (no per-connection repeat), preservingsequence/ordering (only the signature changes), and throwing on a signing pause likequeryLabels' 503 (the cursor is untouched on throw, so reconnect is safe).4.
fix(labeler): redrive the subscription-DO notify on console mutation replay (#7). TherunLabelMutation/runEmergencyActionreplay branches didn't re-driveafterCommit, so a dropped original notify left the row pending forever. Both replay branches now redrive; the #4 sweep is the durable guarantee (console-committed labels carrypublication_pending=1and match the sweep filter).Known theoretical residual (documented, not blocking): the orchestrator reads signing status once for the CAS guard while
buildIssuanceStatementsreads it again independently. If bootstrap (initializeSigningState) interleaved between those reads, a reverse split is possible (label commits, CAS no-ops). It is self-healing (the Workflow retry converges — idempotent label no-ops, CAS then succeeds) and practically unreachable (bootstrap is a one-time setup event that precedes any assessment run; the active→rotation direction can't split because issuance throws before the batch). Closing it fully means threading one consistent read throughbuildIssuanceStatements, which is shared with the console manual-issuance path — deferred rather than expand this cluster's blast radius for an unreachable case. Flagging for reviewer visibility.Targets the
feat/plugin-registry-labelling-serviceintegration branch. Part of #1909; addresses four review findings there.Type of change
Checklist
pnpm typecheckpasses (all three labeler projects)pnpm lintpasses (0 diagnostics)pnpm testpasses (labeler: 842 + 136 + 39) — TDD: each fix has a pre-fix failing repro (F6: run stranded terminal withLabelIssuanceUnavailableError; F4: 3/4 orchestrator publication tests fail; F5: replay frame only verifies under the retired key; F7: afterCommit fires once instead of on replay)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/publication-sol-r1. Updated automatically when the playground redeploys.