feat(discord): add /queue and let an admin re-run a finished session - #49
Merged
Conversation
Every session recorded before the VAD fix on this branch has a document built from an empty or hallucinated transcript, and nothing in the system can produce a better one: JobQueue.claim selects only pending jobs, never done and never dead, so a finished job stays finished no matter how wrong its transcript is. /queue requeue is the explicit write that resurrects one. A re-queue is a state reset and nothing else. It returns the session to exactly the state it was in immediately after close_session and before documentation, and the machinery that already exists -- claim, complete, _create_session_document, announce_ready_sessions -- carries it forward a second time on its own. The decision of which jobs may be reset is a pure function over plain dicts in sturnus.application.requeue, beside sessions_to_announce and expired_jobs and tested the same way; the write is inline ORM in the cog, exactly as audio_cog._erase_audio is and for the same reason -- the selection is used by nothing else. Two rules keep the command from being destructive. A session with any job still pending or running is refused outright: the worker holding a running job will call complete() when it finishes, writing the old run's transcript back over the reset while the admin has already been told the reset happened. And a job whose audio_deleted_at is set is skipped rather than reset, because re-queueing it would only hand a worker a key it cannot download; its done status and its old transcript stay, so the new document is the redone speakers plus the untouched old text of the ones whose audio is gone -- which the reply says in as many words. Both of those are decided inside the same SELECT ... FOR UPDATE the JobQueue.complete takes, ORDER BY id included so the two cannot deadlock, and re-derived there rather than reused from the plan the confirmation was rendered from. Under READ COMMITTED a plan built outside that lock would see a snapshot predating a complete() already in flight. The job resets and the session reset share one transaction and one commit: half of that write landing would strand the session with no document and no error, since complete() and retry_pending_documents both require status == "closed". announced_at is cleared deliberately, and that is the choice to post a second link into the voice channel. mark_documented never touches that column and sessions_to_announce selects only sessions where it is null, so leaving it set would produce a session that transcribes, re-documents with a fresh URL and is then never announced -- nothing logged, nothing raised, the channel simply never told. A corrected transcript nobody hears about is indistinguishable from no transcript. The old document is orphaned rather than updated: DocumentSink exposes create and nothing else, anywhere in the codebase. That is accepted, not stumbled into, so the confirmation names the existing document_url and says it stays and will not be updated -- deleting or cross-linking it remains a human act. Confirmation is a discord.ui.View showing the plan, not a force flag: the three things an admin cannot undo -- discarded transcripts, a second document, a second public post -- are all on screen before a button exists. A second press finds the jobs already pending and is refused on that alone, so it cannot produce a second document or a second announcement. All three subcommands are admin-gated, including the read-only ones: even /queue status reports who was recorded and how much they said. Every query joins to session and filters on guild_id, and a session belonging to another guild gets the same reply as one that exists nowhere, so the command cannot be used to probe ids elsewhere. /queue session reports the length of a stored transcript and never its text -- 24 characters for a 100-minute recording is the tell, and that is all an admin needs to decide.
Section 5 previously had no way to name a job that succeeded end-to-end and was still wrong: `status` done, `transcript` non-NULL, nothing logged. That was every session transcribed before this branch's VAD fix. Add it as a third failure category alongside the existing transcription/document split, with the concrete tell (/queue session's transcript length, e.g. the 19-character " Copyright WDR 2021" fixture) and the fix (/queue requeue), and document the three /queue subcommands themselves, which had no writeup anywhere. Also point the stale "no admin command yet to list dead jobs" line at /queue status and /queue session. No settings table changes: the two commits on this branch touch no Settings/WorkerSettings/LinkSettings field, verified by diffing against the branch's merge-base. first-deployment.md needs no change for the same reason -- no first-deployment step touches any of this.
Three defects found reviewing /queue, all of them in what the command does when the pipeline has not actually finished with a session, plus the three holes in its tests that let them through. **A re-queue may only touch a `documented` session.** Neither `_load_requeue_view` nor `_apply_requeue` looked at `session.status`, and `_apply_requeue` writes `status="closed"` unconditionally -- so a session still inside `RecordingService.close` could be forced into exactly the state `JobQueue.complete`'s Defect 5 guard exists to refuse. That is not an exotic race: `close` uploads and enqueues one speaker at a time, each `enqueue` committing on its own, and calls `close_session` only after the last upload, so a long multi-speaker session stays `open` for minutes with early speakers already `done`; the first job to finish after the forced `closed` would be taken for the session's last, and the document would be built from part of the meeting. A `closed` session is refused for a different reason: `retry_pending_documents` still owns it and may be between its read and its `mark_documented` write, so a re-queue landing in that sweep gets documented from the transcripts it has just discarded, after which neither `complete`'s last-job rule nor the sweep itself ever looks at the session again. `SessionView.is_settled` is the rule, `is_refused` composes it with the two job rules from `sturnus.application.requeue`, and the check is taken inside the row lock as well as in front of the prompt -- the prompt holds nothing but ids while a human reads it, and the session can move in that time. The refusal names the status the session is in and says what to wait for; an administrator who is told only "no" tries again immediately. **Every reply is bounded to Discord's 2000 characters.** `render_session` emitted one ~85-character line per speaker plus `str(exc)` out of `job.error`, unbounded on both counts, and the resulting `HTTPException` from `followup.send` after a `thinking=True` defer is not a truncated answer but no answer at all -- the diagnostic command going silent exactly when the queue is in the state it exists to diagnose. Errors are collapsed to one line and cut at `MAX_ERROR_CHARS`, so one exception cannot swallow the other speakers; the speaker list is filled to the remaining budget and ends in a line stating how many speakers are not shown. Truncation rather than an attachment, because the readout is scanned for one thing -- a transcript length absurd for the length of the session -- and the first speakers sample that as well as any, while a file arrives as a download to open on a command whose value is that the answer is on screen in a second; `docs/operations.md` section 5 already documents the SQL for the rest. `_capped` is the backstop under that for lengths nobody budgeted, and every render function ends inside it, not only `/queue session`. **`mark_announced` is now a compare-and-set.** `announce_ready_sessions` awaits `announcer.post` -- seconds, under rate limiting -- and stamps `announced_at` only afterwards; a re-queue landing in that window clears the column precisely so the redo's link will be posted, and the late unconditional stamp put it straight back, leaving the corrected transcript documented and never announced, with nothing logged and nothing raised. The UPDATE now matches only the state the selection was made on (still `documented`, still unannounced), so the stamp lands only if the session is still the one the post was about. The cost is one duplicate post of the superseded link, which is the side `announce_ready_sessions` already documents itself as erring towards. The tests had three holes of their own, and every one of them is a mutation the old suite passed with. `_press` called `Button.callback` directly, which is not how discord.py dispatches: `View._scheduled_task` runs `item._run_checks(interaction) and self.interaction_check(interaction)` first and drops the press if either is falsy, so the confirm view's author check could be made to deny every press -- locking administrators out of their own Confirm -- with the file still green. `_press` goes through `_scheduled_task` now, with `on_error` replaced by one that re-raises, because the default logs and swallows exactly the assertions these fakes exist to make. `_Followup.send` accepted a followup on an interaction Discord had never been told about, so the `defer` in `RequeueConfirmView.confirm` was deletable with the suite green; it now refuses one, which is the contract the module docstring already claimed to enforce. And the four reply paths that never asserted `ephemeral is True` now do, ephemerality being a contract this cog states in its own docstring because these replies name who was recorded. docs/operations.md section 5 gains the refusal rule with both states and what to wait for, the fact that `/queue session` truncates and where to read the untruncated rows, and the duplicate announcement as something to expect rather than to be alarmed by.
#43 (silent-audio detection) and #48 (transcribe the speech, not the padded track) landed while this waited. Two conflicts, both additive: `tests/infrastructure/test_repositories.py` needed `select` and `update` from sqlalchemy rather than one or the other -- #43's repository test reads a column back, this branch's compare-and-set writes one. `docs/operations.md` section 5 gained two independent troubleshooting entries at the same anchor: this branch's `/queue` walkthrough and #43's 'a speaker's audio arrives with no level'. Both stay; they answer different questions and neither supersedes the other.
TheMeinerLP
added a commit
that referenced
this pull request
Aug 21, 2026
…content
OpenTelemetry traces and metrics, plus structured logging shaped for Loki.
Squashed to a single commit on top of main. Two earlier commits on this
branch carried fixtures with the literal shape of a credential -- an AWS
access key id and a Discord bot token -- which secret scanning detects,
correctly, whether or not either string opens anything. Both are now
assembled from parts at import: identical at runtime, so the tests still
prove the redaction catches those exact shapes, with nothing in the file
for a scanner or a reader to mistake for a credential. Rewriting the tip
alone would not have helped; a scan reads every commit in the pull
request, so the literals had to leave the history.
**Redaction is an allowlist, not a denylist.** Unregistered field names are
dropped, `bytes` is dropped as a class (audio and wrapped data keys are
always bytes, so that closes the highest-value leak by construction),
strings are pattern-scrubbed and capped, and every replacement is visible
(`«redacted:discord_token»`) rather than silent.
**The leak this branch closed.** With `STURNUS_LOG_LEVEL=DEBUG` the Discord
voice `secret_key` reached the logs and would have reached Loki.
Reproduced:
discord/ext/voice_recv/gateway.py:57
log.debug("Received op %s: \n%s", op, pformat(data))
-> {'mode': ..., 'secret_key': [1, 2, ..., 32], 'ssrc': ...}
Not a redaction failure: `extra={"secret_key": ...}` was dropped, and so was
`extra={"voice_ready": {...}}`. The key arrived already formatted into a
third-party logger's *message string*, which a field allowlist cannot
touch. The cause was `root.setLevel(min(resolved_level,
resolved_third_party))`, which put 28 unclamped third-party loggers at
DEBUG -- every logger absent from the enumerated clamp list inherits from
root, and one list cannot be complete about libraries it does not import.
That `min()` was never load-bearing (Python checks the *originating*
logger's effective level on propagation, never root's), so removing it
costs nothing and closes the hole. `THIRD_PARTY_FLOOR` replaces the
enumeration as the structural half of the fix, and
`tests/observability/test_third_party_log_floor.py` asserts the property
over `logging.Logger.manager.loggerDict` rather than over a list.
`discord.voice_state` is pinned at INFO rather than silenced: DEBUG is where
the leak lives, but INFO is the connect narrative -- handshake attempts,
endpoint, close codes, resume -- and it is the evidence base for telling
the three capture failures apart. Pinning needed a `NEVER_ABOVE`
counterpart: `NEVER_BELOW` is applied as `max(level, floor)` and can only
ever make a logger *quieter*, so an INFO entry there was a no-op at the
deployed `WARNING` default -- the logger still ended at WARNING and the
line the entry was written to keep was still gone.
**The metrics answer questions this project actually had.**
`sturnus.transcription.decoded_seconds` divided by wall time is the
real-time factor: a job that "finished" a 100-minute recording in 43
seconds reports an impossible, unmistakable number -- where the symptom
everyone saw, an empty transcript, looked exactly like a participant who
never spoke and was misread as one for a day. `position_seconds`,
`total_seconds` and `seconds_since_progress` are **observable instruments,
not synchronous gauges**: a gauge only changes when a call site sets it, so
a decoder that wedges freezes it and `seconds_since_progress` -- the actual
alert -- could never grow. The SDK calls these callbacks once per export
interval instead, which is also what lets them emit nothing at all while
the worker is idle, so the series goes stale rather than reporting a
finished job's numbers forever. The stall clock starts before the library
call, since the collapse happened inside feature extraction. Labels are
`model` only: no session, job, guild or user id, which would be unbounded
cardinality and a record of who was in a voice channel when, kept for as
long as the metric store keeps anything.
`sturnus.job.outcome` reported `done` for every failed job, because
`process_one` returns True after `queue.fail(...)` exactly as it does after
`queue.complete(...)` -- the boolean means "work was attempted", never
"work succeeded", and a metric that reports failures as successes is worse
than no metric because it will be believed. The label is now recorded by
the transitions that decide a job's terminal state, and `crashed` is the
one the worker loop still owns.
**Rebased onto #48, which rewrote the same method.** The transcription
mechanics are main's: the model is handed the gated speech concatenated,
`clip_timestamps` is re-expressed on that timeline, and every returned
segment goes back onto the recording's through
`_on_the_original_timeline(segment.start, segment.end, segment.seek, ...)`,
where `Segment.seek` -- the encoder window the segment was decoded from --
is what names its clip. The observability is re-expressed on top: the
segment generator is drained by a loop and not a comprehension, so
`TRANSCRIPTION_PROGRESS` sees each segment as it arrives rather than only
after the job has already finished.
Progress is reported on the *concatenated* timeline -- `advance(segment.end)`
and not the restored end -- because the denominator is
`duration_after_vad`, which since #48 is the concatenated speech. Reporting
a restored end against it would put a job that had decoded its first clip
at several hundred percent. `telemetry.TranscriptionProgress` and
`docs/operations.md` § 7.5 say so; they described the whole file before.
Two call sites arrived from main that the merge could not have seen, both
of them what `tests/test_logging_discipline.py` R2 and R6 forbid, and for
the reason R6 exists: `%s` on an exception prints `str(exc)` verbatim into
the message `observability.scrub_event` forwards to Sentry.
`RecordingService._report_silent_audio` (#48) was three `log.warning`
calls, two interpolating an exception and one passing `display_name`; it is
now `speaker.audio_silent`, `speaker.silent_warning_failed` and
`speaker.silent_record_failed` through `log_event`/`log_exception`, with
`display_name` gone -- the channel message renders the mention, and the
operator has the id. `RequeueConfirmView._disable` (#49) is now
`queue.view_disable_failed` the same way.
TheMeinerLP
added a commit
that referenced
this pull request
Aug 21, 2026
…content (#50) OpenTelemetry traces and metrics, plus structured logging shaped for Loki. Squashed to a single commit on top of main. Two earlier commits on this branch carried fixtures with the literal shape of a credential -- an AWS access key id and a Discord bot token -- which secret scanning detects, correctly, whether or not either string opens anything. Both are now assembled from parts at import: identical at runtime, so the tests still prove the redaction catches those exact shapes, with nothing in the file for a scanner or a reader to mistake for a credential. Rewriting the tip alone would not have helped; a scan reads every commit in the pull request, so the literals had to leave the history. **Redaction is an allowlist, not a denylist.** Unregistered field names are dropped, `bytes` is dropped as a class (audio and wrapped data keys are always bytes, so that closes the highest-value leak by construction), strings are pattern-scrubbed and capped, and every replacement is visible (`«redacted:discord_token»`) rather than silent. **The leak this branch closed.** With `STURNUS_LOG_LEVEL=DEBUG` the Discord voice `secret_key` reached the logs and would have reached Loki. Reproduced: discord/ext/voice_recv/gateway.py:57 log.debug("Received op %s: \n%s", op, pformat(data)) -> {'mode': ..., 'secret_key': [1, 2, ..., 32], 'ssrc': ...} Not a redaction failure: `extra={"secret_key": ...}` was dropped, and so was `extra={"voice_ready": {...}}`. The key arrived already formatted into a third-party logger's *message string*, which a field allowlist cannot touch. The cause was `root.setLevel(min(resolved_level, resolved_third_party))`, which put 28 unclamped third-party loggers at DEBUG -- every logger absent from the enumerated clamp list inherits from root, and one list cannot be complete about libraries it does not import. That `min()` was never load-bearing (Python checks the *originating* logger's effective level on propagation, never root's), so removing it costs nothing and closes the hole. `THIRD_PARTY_FLOOR` replaces the enumeration as the structural half of the fix, and `tests/observability/test_third_party_log_floor.py` asserts the property over `logging.Logger.manager.loggerDict` rather than over a list. `discord.voice_state` is pinned at INFO rather than silenced: DEBUG is where the leak lives, but INFO is the connect narrative -- handshake attempts, endpoint, close codes, resume -- and it is the evidence base for telling the three capture failures apart. Pinning needed a `NEVER_ABOVE` counterpart: `NEVER_BELOW` is applied as `max(level, floor)` and can only ever make a logger *quieter*, so an INFO entry there was a no-op at the deployed `WARNING` default -- the logger still ended at WARNING and the line the entry was written to keep was still gone. **The metrics answer questions this project actually had.** `sturnus.transcription.decoded_seconds` divided by wall time is the real-time factor: a job that "finished" a 100-minute recording in 43 seconds reports an impossible, unmistakable number -- where the symptom everyone saw, an empty transcript, looked exactly like a participant who never spoke and was misread as one for a day. `position_seconds`, `total_seconds` and `seconds_since_progress` are **observable instruments, not synchronous gauges**: a gauge only changes when a call site sets it, so a decoder that wedges freezes it and `seconds_since_progress` -- the actual alert -- could never grow. The SDK calls these callbacks once per export interval instead, which is also what lets them emit nothing at all while the worker is idle, so the series goes stale rather than reporting a finished job's numbers forever. The stall clock starts before the library call, since the collapse happened inside feature extraction. Labels are `model` only: no session, job, guild or user id, which would be unbounded cardinality and a record of who was in a voice channel when, kept for as long as the metric store keeps anything. `sturnus.job.outcome` reported `done` for every failed job, because `process_one` returns True after `queue.fail(...)` exactly as it does after `queue.complete(...)` -- the boolean means "work was attempted", never "work succeeded", and a metric that reports failures as successes is worse than no metric because it will be believed. The label is now recorded by the transitions that decide a job's terminal state, and `crashed` is the one the worker loop still owns. **Rebased onto #48, which rewrote the same method.** The transcription mechanics are main's: the model is handed the gated speech concatenated, `clip_timestamps` is re-expressed on that timeline, and every returned segment goes back onto the recording's through `_on_the_original_timeline(segment.start, segment.end, segment.seek, ...)`, where `Segment.seek` -- the encoder window the segment was decoded from -- is what names its clip. The observability is re-expressed on top: the segment generator is drained by a loop and not a comprehension, so `TRANSCRIPTION_PROGRESS` sees each segment as it arrives rather than only after the job has already finished. Progress is reported on the *concatenated* timeline -- `advance(segment.end)` and not the restored end -- because the denominator is `duration_after_vad`, which since #48 is the concatenated speech. Reporting a restored end against it would put a job that had decoded its first clip at several hundred percent. `telemetry.TranscriptionProgress` and `docs/operations.md` § 7.5 say so; they described the whole file before. Two call sites arrived from main that the merge could not have seen, both of them what `tests/test_logging_discipline.py` R2 and R6 forbid, and for the reason R6 exists: `%s` on an exception prints `str(exc)` verbatim into the message `observability.scrub_event` forwards to Sentry. `RecordingService._report_silent_audio` (#48) was three `log.warning` calls, two interpolating an exception and one passing `display_name`; it is now `speaker.audio_silent`, `speaker.silent_warning_failed` and `speaker.silent_record_failed` through `log_event`/`log_exception`, with `display_name` gone -- the channel message renders the mention, and the operator has the id. `RequeueConfirmView._disable` (#49) is now `queue.view_disable_failed` the same way.
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.
Re-transcribing the sessions that the VAD defect ruined had to be done by hand-written SQL against production, twice. This is that capability as a reviewed command instead.
/queue sessionreads a session's jobs;/queue requeueputs them back through transcription behind a confirmation. Admin-gated through the samerequire_admin()the other commands use, every reply ephemeral — these replies name who was recorded.What it refuses, and why each refusal exists
A session that is not
documented. Two different hazards, one rule:openis the windowJobQueue.complete's Defect-5 guard exists for.RecordingService.closeuploads and enqueues one speaker at a time, eachenqueuecommitting separately, and only callsclose_sessionafter the last upload. Forcingstatus="closed"inside that window lets the nextcomplete()call the session finished from whichever speakers happen to exist yet. On a long multi-speaker session that window is wide, and "re-queue the session that just ended" is the obvious thing an impatient admin does.closedis still owned byretry_pending_documents, which can document the session from the transcripts a re-queue just cleared — after which neithercomplete()'s last-job rule nor the sweep ever revisits it.A job whose audio is gone.
audio_deleted_atis set only after a real S3 delete succeeded. Re-queueing those creates jobs that can only fail.The reply names the actual status and what to wait for.
Bounded output, because a diagnostic command must not go silent
render_sessionemits a line per speaker includingjob.error, which holds an arbitrary exception string. With several speakers, or one long error, that exceeds Discord's 2000-character limit; theHTTPExceptionfromfollowup.sendafter athinking=Truedefer means the admin gets no reply at all — precisely when the queue is full of failures.Errors are collapsed to one line and cut at 160 characters so one exception cannot swallow the other speakers; the list fills the remaining budget and ends in "…and N more speakers not shown", pointing at the SQL in operations.md §5. The cap sits under every render function, not just
/queue session: a refusal listing 80 erased speakers has the same failure mode.Truncation rather than an attachment, because the readout is scanned for one thing — a transcript length absurd for the session — and the first speakers sample that fine, whereas a file arrives as a download on a command whose whole value is an answer on screen in a second.
A race that silently lost the announcement
SessionRepository.mark_announcedis now a compare-and-set onstatus = 'documented' AND announced_at IS NULL.announce_ready_sessionsawaitsannouncer.post(...)— a Discord HTTP call that can take seconds under rate limiting — and only then stamps. A sweep already inside that call would re-stamp the column a re-queue had just cleared, and the redo would never be announced. Now the late stamp misses and logs at INFO. Accepted cost: one duplicate post of the superseded link, the sideannounce_ready_sessionsalready errs towards.Tests
695 pass. 13 mutations, each killed by the test that names the behaviour.
Two test defects were found by review and both were confirmed real before being fixed — the old suite was run with the mutations applied and stayed green:
RequeueConfirmView.interaction_checkwas only tested in the deny direction. The_presshelper calleditem.callbackdirectly, bypassing the gate Discord dispatches through (View._scheduled_taskrunsitem._run_checks(...) and self.interaction_check(...)). It now goes through that path, so the allow direction is load-bearing in all ten confirming tests.defercould be deleted with the suite green, though the file docstring claimed the fakes enforce Discord's contract._Followupnow refuses a followup on an unacknowledged interaction.Worth second-guessing
closedremoves a capability. A session genuinely stuck there — the document sweep failing permanently — can no longer be re-queued by command at all; SQL is the only route. operations.md §5 points at the document-failure section, but it is a real narrowing._pressreaches into discord.py privates (View._scheduled_task, assigning overon_error). That is the price of testing the gate Discord actually dispatches through, and it couples the suite to a private API across upgrades.