Close the draft consumer's six review findings, and park a sweep that could only reach what it meant to spare (#765) - #768
Merged
Conversation
… could only reach what it meant to spare (#765) Six confirmed findings from #767's enhanced review. They sat outside that diff's write scope but every one is a consequence of it, so they are fixed here. AN UNASSESSED DRAFT COULD PUBLISH AS NOVEL. The consumer refuses to publish when assessment fails, but one layer down ProposalGate could not tell "no neighbours found" from "the neighbour search was unavailable" - a shed or degraded vector read returns an empty list, which scores as maximally novel. The guard above was therefore defeated by the layer below it. Every assessment now carries comparison: :complete or :unavailable, and the consumer treats :unavailable as NOT ASSESSED, exactly as it already treats :unknown. The CREATE path is deliberately unchanged and still falls open: a write must not 500 because a novelty read shed. The two callers want opposite things from the same failure, which is why the fix is to distinguish rather than to change the default. THE SWEEP WORKER COULD ONLY REACH THE CLASS IT DOCUMENTED IT MUST SPARE. This is the finding worth reading. DraftDuplicateSweepWorker archives drafts whose nearest published neighbour clears a high threshold, and it sweeps only drafts that HAVE an embedding. Embeddings are enqueued at status published only, so every row it could ever archive was published once and then retracted - precisely the class its own "what it must NOT sweep" section says to protect. It spared only the half consolidation wrote, leaving a human retraction reachable. It is now PARKED rather than rewritten. Nothing was swapped in for archive because a draft is already the bottom of the reversible lifecycle - published to draft and back is the pair, archived is the exit from it - so there is no reversible retirement to move to. Parking changes the AUTHORITY instead of the action: a human may take a terminal act, and an operator unparking it is a human asking for exactly that. The design below it is preserved intact for a revive, including the measurement behind the threshold. A DELIBERATELY STAGED DRAFT NOW HAS A VETO, not a 48-hour floor. staged_draft_at is a column stamped programmatically and absent from every cast list, so an ordinary PATCH cannot erase it - the same lesson previous_title and lifecycle_entered_at each paid for. Plus a covering index for the candidate scan, which was reading the tenant's articles on the three-connection admin pool that every authenticated request also checks out of; and the pre-marker retraction residual, verified against the code and documented at the guard site as the deliberate bounded cost of not stranding every pre-marker draft permanently. The CHANGELOG entry that the previous two changes correctly omitted is warranted here, and it names its categories: two migrations, and OBAN_UNPARK_CRONS as the lever that revives the parked worker.
mkreyman
added a commit
that referenced
this pull request
Aug 27, 2026
) * Alarm when the nightly consumers go quiet with work waiting (#765) The last item of #765, and the one that guards the failure that started it. KnowledgeLintWorker died at 600s for six consecutive nights and it read as quiet success, because nothing in the system watched for ABSENCE. Every class now has a consumer - generic_title retitles, duplicate_capture unpublishes, drafts publish and link - so a night where all of them apply zero is indistinguishable from a healthy clean corpus. That is the hole this closes. IT DOES NOT LIVE IN THE PASS IT WATCHES, which is the whole design. #761 killed the job before it wrote its audit event at all, so a detector inside that pass would have been just as dead, and a streak counted over events freezes exactly when the system breaks. It runs from the hourly IngestionHealthWorker instead - different queue, different cadence - reading only the durable rows the pass leaves behind. KnowledgeLintWorker's timeout and reserve arithmetic are untouched; the check costs that job nothing. QUIET AND BLIND ARE DIFFERENT and the distinction is the point. A clean corpus with an empty draft queue and every gate open stays silent forever. A candidate needs zero applications across the window AND one of three things: work actually waiting; a hard blind signal (a failed gate, the -1 step-failed sentinel, or a wall clock that cut in before the first application, which counts alone because a step starved every night is a defect whether or not work happened to be queued); or a paused drain with its work corroborated independently. That corroboration is what keeps the alarm honest - a paused gate short-circuits before the candidate query and reports zero offered whether the queue is full or empty, so without it a keyless tenant with nothing to do would alarm forever, which is how a real switch gets muted. STATE IS DERIVED FROM THE AUDIT EVENTS the pass already writes, not from a new counter, because a counter is one more thing that can be silently wrong. The history window is derived from the two thresholds rather than picked beside them and clamped to the audit retention. Recovery closes on what this run could actually evaluate, never on absence from the candidate list - so an anomaly cannot be auto-closed by its own evidence ageing out. N is 7 runs. A quiet night neither starts nor extends a streak, so 7 means 7 nights on which work was waiting or the step could not act - at ~11 drafts a night that is ~77 held drafts, which is unambiguous. Three knobs, all resolving DB row to app config to default, so an operator can retune without a deploy. ON THE INDEX MIGRATION AND ITS LOCK. audit_log is a RANGE-partitioned parent, where CREATE INDEX CONCURRENTLY is unsupported, so this takes a SHARE lock per partition and blocks audit writes while it builds - and audit writes are on the path of every API call. I measured production before taking it rather than reasoning about it: the current partition is 12 MB over 8,443 rows, July is 10 MB over 8,034, future partitions are 48 kB, and only June is large at 128 MB, which ages out of the 90-day retention shortly. An index build at that size is seconds. The alternative is a PERMANENT hourly sequential scan on the three-connection admin pool that every authenticated request also checks out of, which is the same cost #768 just removed from the draft candidate query. A brief lock beats a standing scan. NO DRAINER WAS BUILT for the 905 proposals sitting in pending, and that half of the item is deliberately not implemented. review_status is vestigial by the schema's own moduledoc - nothing in lib branches on it, the nightly upsert resets it on every re-derivation, and the controller's own OpenAPI text says so. Those rows are historical scan records, not a queue, and a drainer for them would be motion without signal. The half of item 6 that IS real - no state has a human as its only exit - is what the switch encodes: a paused or keyless drain no longer sits silently forever, it surfaces after N nights, and resolve or archive on the anomalies endpoint are its exits. Seventeen mutations, each killed by a named test, covering both directions: a clean corpus never alarms, and work-waiting-with-zero-applied always does. * fix(review): round-1 - stop the consumer switch alarming on healthy installs, and let it see the outages it was built for Fourteen review findings on the nightly consumer dead-man's-switch, all in the same detector. False alarms it could never take back: - consumer_runs had no active-tenant join, the only detector in the module without one. Suspending a tenant stops the pass by design, so 72h later the pass half paged the operator about work they had turned off, and recovery (which IS status-gated) could never close it. - Deliberate refusals counted as work waiting. A duplicate group the corroboration gate withheld, a title the provider abstained on, a curated article the retitle correctly skips: the scan re-proposes each every night, so a healthy corpus alarmed forever and the only remedy muted the consumer. Those counters are now subtracted from the work signal instead of added. - Recovery closed on the stall condition merely lapsing. A dead drain stops being a candidate the moment a sweep archives the drafts behind it, which stamped resolved into the append-only audit log about a consumer that had disposed of nothing. It now needs positive evidence: recovered_keys. Outages it could not see: - The read was bounded by the derived class window (14 days at the defaults), so a pass dead for longer than that had no rows to judge and produced no alarm at all. The read now runs to the audit retention; the derived window bounds only the per-class streak. The docstring claiming 90 days was wrong by 6x and is replaced. - consumer_stall_runs above the retention could never fill a window, silently switching off every per-class check while the run still reported zero candidates. It is now clamped to the retention, with a warning. - The scan_limit cut on a stable tenant_id sort blinded the same tail of tenants on every run. The sort key now rotates hourly. - The retention ceiling was a hardcoded 90 rather than the configurable key AuditPartitionWorker actually drops partitions by. Blast radius: - A globally dead nightly cron stales every tenant in one run, so the pass half now emits ONE system-scope operator alert per run like sweep_stalled does, instead of a page per tenant. The per-tenant anomaly rows and webhooks are unchanged. - window_seconds in the operator alert reported the observed staleness rather than the detection window, unlike every sibling clause. - The draft corroboration read was one query per tenant on the 3-connection admin pool, and an install-wide pause put every tenant on that path. It is one bounded read now, taken only when some run reports a paused drain. Migrations: - The audit_log partial index built on the partitioned parent, blocking every audit write - and so every mutating request - for the whole build. It is now created ON ONLY, with each partition built CONCURRENTLY and attached. - The anomaly-type rollback DELETEd recorded stalls that the audit chain still references. It retags and resolves them instead. Also: the three consumer knobs were documented under the ingestion_health keyword list while tunable/3 reads them as top-level loopctl keys, so an operator override was silently ignored; and source_type_seen? guarded on four hand-copied sentinel strings that already exist in consumer_classes. * fix(review): round-2 - stop the refusal subtraction and the recovery gate from blinding the switch A deliberate refusal is still subtracted from the work signal, but SATURATION is now the opposite reading: refusing every offer while applying none is exactly what a dead extraction provider or a dead vectorisation input looks like, and the counters that carry the benign case carry that one too. Both classes read it as hard-blind, so a 100 percent refusal streak can no longer subtract itself to silence, while a refusal beside a real disposition stays quiet. Recovery gets its second route back. Requiring a disposition left a healthy but idle consumer's anomaly open forever, and the operator's only remedy leaves last_event_at NULL, which suppresses every future stall of that key. A full window with nothing offered, no blind gate and no paused gate now closes and re-arms; a still-paused drain does not. Also: clamp the stall streak to HALF the audit retention, since the derived window is double it and above half a single missed night leaves every tenant permanently short of a window; bound the draft corroboration read to the tenants that actually reported a paused gate, instead of an unqualified DISTINCT over articles on the three-connection admin pool; make the partition-index probe ask for a VALID attached index rather than a matching NAME, and drop and rebuild an interrupted build's invalid orphan; archive the rows the anomaly-type rollback retags, so a rolled-back install does not list a capture_silence anomaly for a consumer; and give the pass system alert the same injectable insert seam its sweep twin has, with the dropped-enqueue test that seam exists for. * Publish the consumer_stalled type in the anomalies contract, not only in the code (#765) The fifth anomaly type shipped with parse_anomaly_type and known_anomaly_type accepting it while the OpenAPI spec still listed four. A caller reading the published contract would conclude consumer_stalled is not a filterable value, and this repo's rule is explicit that a new API constraint belongs in the endpoint's operation spec and not just the controller guard. Both sites now name it: the filter parameter, and the summary that explains what each type means. The summary entry says what the type IS - a dead-man's switch that detects ABSENCE rather than failure - because that is the part a reader cannot infer from the name, and because it is the property that makes silence from this detector meaningful rather than merely unremarkable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The six confirmed findings from #767's enhanced review. They sat outside that diff's write scope, but every one is a consequence of it, so they are fixed rather than filed.
An unassessed draft could publish as
:novelThe consumer refuses to publish when assessment fails. One layer down,
ProposalGatecould not distinguish "no neighbours found" from "the neighbour search was unavailable" — a shed or degraded vector read returns an empty list, which scores as maximally novel. The guard above was defeated by the layer below it.Every assessment now carries
comparison: :complete | :unavailable, and the consumer treats:unavailableas NOT ASSESSED, exactly as it already treats:unknown.The create path is deliberately unchanged and still falls open — a write must not 500 because a novelty read shed. The two callers want opposite things from the same failure, which is why the fix distinguishes rather than changing the default.
The sweep worker could only reach the class it documented it must spare
The finding worth reading.
DraftDuplicateSweepWorkerarchives drafts whose nearest published neighbour clears a high threshold, and it sweeps only drafts that have an embedding. Embeddings are enqueued atstatus: :publishedonly — so every row it could ever archive was published once and then retracted, which is precisely the class its own "what it must NOT sweep" section says to protect. It spared only the half consolidation wrote, leaving a human retraction reachable.It is now parked, not rewritten. Nothing was swapped in for archive because a draft is already the bottom of the reversible lifecycle —
{:published, :draft}and{:draft, :published}are the pair,:archivedis the exit from it — so there is no reversible retirement to move to.Parking changes the authority, not the action: a human may take a terminal act, and an operator unparking this is a human asking for exactly that. The design below it is preserved intact for a revive, including the measurement behind the threshold.
The rest
staged_draft_atis a column, stamped programmatically, absent from everycastlist — the lessonprevious_titleandlifecycle_entered_ateach paid for.articleson the 3-connection admin pool that every authenticated request also checks out of.OBAN_UNPARK_CRONSas the lever that revives the parked worker.Verification
7,970 tests green via the pre-commit hook (credo --strict, dialyzer included). Both migrations run clean on dev and test.
Per the review skill's own rule, an out-of-scope list is fixed on a follow-up branch and not re-reviewed — a full review already produced these findings, and re-reviewing its own output is the unbounded chain the depth counter exists to stop.