fix: seventeen devices from the poka-yoke audit (#705–#721)

[rainmanjam]

All seventeen findings from the poka-yoke audit, each with the device its
rung called for.

Closes #705, #706, #707, #708, #709, #710, #711, #712, #713, #714, #715, #716,
#717, #718, #719, #720, #721.

Every guard here has a test that has watched it fail — a planted mutation,
or a fixture that reaches the refusal arm. That is the audit's own standard: a
guard nobody has watched fail is a guard nobody should trust.

#706 — a password change left its tokens' sockets streaming

Revocation had two halves: delete the row, and mark the id in an in-process
set the /ws tick reads. handleRevokeAPIToken did both.
handleChangePassword, which calls DeleteAllAPITokens, marked nothing.

A socket opened with an admin token kept receiving admin-shaped events until it
or the process died — while the response correctly reported zero surviving
tokens. The revoke handler had predicted it word for word:

a second deletion path that did not do this would be a silent hole

The set is gone. The tick asks the store (db.APITokenExists) the way the
session half beside it already asks for users.token_epoch, failing closed on a
read error for the reason stated there. That is Control rather than Warning:
there is no second half left to forget, so resetadmin (#718) and whatever is
added next are honoured without being told to be. One indexed read per socket
per ping period — what the epoch check already costs.

Its test asserted the old premise and closed with "if something else now
writes to it, this test's premise is stale". Something did. It now pins the
stronger property — a deletion made anywhere ends the socket — and keeps the
half that still matters: this check closes sockets early and is not an
authorisation source.

#710 — requireScope passed a request with no principal

Its siblings requireSession and requireCSRF both fail closed on the same
condition. Every group carrying it today also carries requireAuth, so this
changes no live behaviour — it removes the affordance of mounting them apart or
in the wrong order. The p.token == nil arm is a different, deliberate case and
stays: a session principal is not a scoped token.

#714 — stopAux took a slot and a name that had to correspond

Two silent mistakes were available: e.stopAux(&e.preview, "recorder") compiles
and reads correctly while releasing the wrong port and leaking the preview's;
and the switch had no default, so a fourth consumer would skip its release
and leave reconcile believing the child was up.

One auxSlot value per consumer — no second argument to mismatch, no switch to
fall through. It also brings these three into line with every other subscriber
in the package, which stores its name at subscribe time rather than recomputing
a literal at teardown. The test table now names the production slots instead
of carrying its own copy of the same correspondence.

#720 — kill() gained the reaped-check its two siblings carry

killGroup issues a raw syscall.Kill(-pid, SIGKILL), naming a process group by
number and bypassing Go's ErrProcessDone. On a reaped pid that can signal a
group this supervisor never started. The safety argument was correct — and was a
comment spanning three functions. Now it is a guard.

#716 — and the regression it caused

Recorded rather than quietly fixed, because it is the general lesson.

Making the policy switch fail closed dropped every passthrough event — Log,
Status, Levels, Stats, Loudness — because wsPassthrough is iota, the zero
value, and had no case of its own: it had always reached the sender through the
default arm I was replacing. The opening burst on a fresh install delivered
nothing, and TestTheWebSocketOpeningBurstSurvivesZeroSources caught it.

That hazard is now a comment at the case: the arm being replaced has to be
enumerated first, and a zero-valued constant is the member most likely to be
sitting in it unnoticed. wsPassthrough is explicit, and default still fails
closed for a constant added later — which was the point, since passing an
unknown policy through sends a read-scoped socket the admin shape.

#707 and #708 — the engine leaked a relay port per aux child, and Release had no owner

The pool is 500 ports shared across every engine, and Manager.Sync stops
an engine on every source delete while the daemon keeps running — so a port an
engine fails to return is gone for the life of the process. Nothing reports it
until Allocate starts failing, and then it fails everywhere at once and
reads as an unrelated fault.

Four kinds were not returned. Destinations, loudness, clips, captions, feeds,
silence, backup and playlist went through a teardown that released; the
recorder, preview, meters and renditions went through a bare stop helper that
only called Stop. Measured before the fix: three ports plus one per
rendition leaked per shutdown, and the hub kept their subscriptions.

Engine.allocPort/releasePort now keep a per-engine ledger, so a release
names a port this engine holds or is refused and logged (#708). Release
took a bare int and did delete(a.held, p), so releasing twice silently
un-held a port the pool may have since handed to a different engine — two
engines on one UDP port, which is what the allocator's bind probe exists to
prevent. The probe only masks it while the first owner's child is bound; during
restart backoff, or between Allocate and Start, it is open. StopWithin
carries a post-condition on heldPortCount.

The reachable double-release path: stopBackup deliberately does not clear
d.backupPort, so the struct still names a released port and a later teardown
releases it again.

#709 — a mutation whose reconcile failed answered with a plain success

Sixteen handlers called s.reconcile() and two spellings lived side by side
in one package: three turned the error into a 500, twelve logged it at Warn
and returned success. Nothing in the signature said which was right, so a
handler written by copying its nearest neighbour got whichever that neighbour
was.

The silent spelling is worse than it looks. Engine.Reconcile returns early on
a reconcileOutputs error, so preview, clips, captions and loudness are skipped
with it — and reconcile is event-driven with no ticker, so the failure is
never retried. Stored state and the running FFmpeg diverge until the next
successful mutation or a restart, while the response said 200 and the UI raised
a green toast.

The worst case was invisible: handleDeleteDestination returned
{"status":"deleted"}, the row left the list, and the FFmpeg child kept
publishing to a destination the console no longer draws.

Server.reconcileNow is now the only caller of s.reconcile(). writeMutation
folds its sentence into the response through the marshalled form rather than
a field on each of a dozen response types — adding a field per type is the same
per-site decision being removed. expert.go shipped Applied: true as a
literal beside a comment claiming "the command shown back is the one the
reconcile above just started", false on exactly the swallowing branch.

The UI half is in the transport, not at the call sites: api.ts's
request() raises the amber toast on reconcileFailed, so all fifteen routes
and every route added later are covered without being told.

#711 — a relay subscriber name collision silently replaced one consumer

h.subs[name] = &subscriber{...} was a bare map assignment. The replaced
consumer keeps running, keeps a correct command line, keeps a green card — and
receives nothing. The hub logged "relay subscriber added" either way, so the
log positively confirmed the wrong thing.

Three devices already existed to avoid the collision and all three were rung
zero: a naming convention, a lock, and a comment. It had bitten twice.

Subscribe refuses an occupied name. Thirteen production call sites change
signature; each already had a release-and-bail path for the allocator refusing a
port. Unsubscribe was the mirror — delete on an absent key is a no-op and
the line said "removed" regardless.

A test case asserted the hazard: "re-subscribing the same name replaces rather
than duplicates." Replacing is the failure.

#715 — an unreadable hook secret degraded to an unsigned POST

Restore a backup taken on another machine, or rotate secret.key, and every
webhook went out unsigned. scanHook loaded the row with an empty Secret
and said nothing; attempt skips the signature header on an empty secret.

At the far end that is indistinguishable from a forgery. Hook.SecretUnreadable
is the device Destination.KeyUnreadable already is. Three places refuse it,
and attempt — the one place the signature is decided, reached by both delivery
and the operator's Test button — is what makes an unsigned delivery unreachable
rather than merely filtered.

#717 — the census covered one of about twenty-five spawn sites

enrol/discharge were unexported inside internal/supervisor, so one spawner
could use it — and internal/ffmpeg could not have, because supervisor imports
ffmpeg. Its own comment framed it as "WHAT HAVE WE ACTUALLY SPAWNED?", true
only for supervisor children, while the shutdown report said nothing at all on a
clean stop.

internal/childcensus is a leaf package every spawner can reach. Media
transcodes and the transcription and live-caption workers enrol.
TestEverySpawnSiteIsAccountedFor walks the tree and fails the build for a
package that spawns without enrolling and without a stated reason: seven silent
omissions became seven stated decisions.

#719 — the UI reported a failed read as an empty result

lib/readState.ts exists to make .catch(() => setX([])) unwriteable.
DestinationDialog.tsx never imported it and had five. Two drove the
operator to act on a claim the server never made: "No shared encodes yet.
Create one on the Renditions page first." and a Connect account button for
an account already linked, which discards the half-filled dialog.

What enforced the rule was a list of three filenames — training, not a device,
and it did not know this file existed. readState.shape.test.ts walks every
file and matches on the shape, and found nine more the audit never named.
One was a real bug: the Dashboard's metadata composer hides itself on null but
the catch stored [], so a failed read rendered the composer with zero
platforms.

#705 — fsperm's ACL builder stored SIDs as unpinned integers

TrusteeValueFromSID converts a *windows.SID to a uintptr, and x/sys says
it in as many words: "The caller must pin sid using a runtime.Pinner for the
lifetime of the TrusteeValue." Both SIDs are Go-allocated and collectable, and
the EXPLICIT_ACCESS outlives the call.

The pinner is taken in restrict rather than in grantFull — the version that
looks correct and is not, because a pinner there unpins on return, before
ACLFromEntries ever reads the trustee. Consequence was a wrong DACL, not heap
corruption, on the files holding every stream key.

#712 — destWritesAFile failed open on a kind this build does not know

db.DestKind and ffmpeg.DestKind are declared independently and joined by a
conversion that compiles for any string, so a fifth kind reaches the default.
Returning false there was wrong for the reason the function's own comment
gives: without the confinement an audio file target is written relative to the
process working directory, outside the directory every other file destination is
held to. An unknown kind is now treated as a file writer and confined, and
RTMP/SRT are named rather than left to the default so the exhaustiveness is
visible to a linter.

#718 — resetadmin could not say what still had access

It printed "every existing session has been signed out" and stopped. True and
incomplete: the epoch ends sessions, while API tokens are resolved by hash
alone and survive. This is the command an operator reaches for when they cannot
sign in — the compromise case — and it was the one path that could not say what
still reached the install.

It now always lists the surviving tokens and how to end them, mirroring the
disclosure the HTTP handler already performs, and --revoke-api-tokens does the
ending. Opt-in rather than implied, for the reason that handler argues: routine
rotation is the common case and destroying every integration's credential is the
wrong default for it.

Its two failure arms matter most, because they fire exactly when the command
cannot answer the question — and an operator not told that silence means
unknown reads it as nothing. Both are tested, reached by breaking the token
table in two ways that survive db.Open.

#721 — isSetup and setupSlot are two switches over one closed type set

Folded into a single condition in cacheSetup, which made "ordinary media" and
"setup with no slot" indistinguishable. A test now asserts they agree on every
handled type. The runtime log the issue also proposed was not taken:
stream has no logger, threading one in for this is not worth it, and a
build-time comparison catches the disagreement when it is written rather than
when it hangs somebody's ffprobe.

#713 — the code disproved the finding as filed

Recorded rather than quietly fixed. The issue says Rumble, Trovo and Vimeo are
"silently exempt from loudness checking" and should get policy rows. Reading
internal/db's own constants says otherwise: Rumble "exists for CHAT and for
nothing else", Vimeo "for SIGN-IN", and Trovo can fetch a stream key but not
the ingest URL beside it — so all three are pasted by hand and behave as custom
destinations do. They take the custom row deliberately.

The hazard it identified is still real, and it is the other half of its own
sentence: nothing distinguished "custom by choice" from "custom because the
table forgot you". So the device is a test that forces the decision to be
stated — an opt-out list carrying the reason per platform, with a second
test refusing a placeholder reason and refusing to let the list grow past four.
Three silent omissions become three stated decisions, and a fourth fails.

Verification

Mutation-tested, each red before the fix: the socket no longer asking the
store; passthrough falling back to the closed default; the aux ports not being
released; the rendition port not being released; a port not held being released
anyway; a type dropped from either side of the setup switch pair; a platform
removed from the routing decision list.

Coverage for the guards themselves, since a guard nobody has watched fail is
a guard nobody should trust: APITokenExists on a live, deleted, zero and
absent id; tokenRevoked's fail-closed arm, reached by closing the store
underneath it; requireScope with no principal in the context; eventView's
explicit passthrough case and both fail-closed arms — the unclassified type,
and a policy constant with no case, reached by planting one in the table;
destWritesAFile on a kind this build does not know; kill() on a reaped pid,
tested against the guard directly because the supervisor clears p.exited
during teardown and waiting for a real reap races the field the guard reads.

internal/api, internal/engine, internal/supervisor, internal/db and
cmd/polyemesis all green.

https://claude.ai/code/session_01A8N3W5ct9SZtHK9sCDD9cL

[Copilot]

🟡 Changes recommended

A couple of newly introduced/modified documentation comments are now misleading (e.g., token existence and WS revocation tick behavior) and should be corrected to avoid future maintenance mistakes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR implements four targeted fixes from the poka‑yoke audit (plus the noted #716 regression follow-up), tightening correctness in WebSocket revocation/policy handling, auth middleware ordering safety, engine aux teardown safety, and supervisor signalling safety.

Changes:

• WebSocket hardening: replace the process-local revoked set with a store-backed APITokenExists check, and make unknown WS policy values fail closed (with an explicit wsPassthrough case).
• Auth & supervisor safety: make requireScope fail closed when there is no principal, and add a reaped-check guard to Process.kill() before issuing syscall.Kill(-pid, SIGKILL).
• Engine aux teardown safety: replace (slot, name) teardown with a single auxSlot descriptor to prevent mismatched teardown/unsubscribe/release, and update lifecycle tests accordingly.

File summaries

File	Description
internal/supervisor/supervisor.go	Adds a reaped-check guard to kill() to avoid signalling unrelated process groups.
internal/engine/engine.go	Refactors aux teardown to use auxSlot and eliminates name/slot mismatch hazards.
internal/engine/lifecycle_test.go	Updates aux teardown tests to use production auxSlot values and validate isolation.
internal/db/tokens.go	Adds APITokenExists store query to support WS revocation via the DB.
internal/api/ws.go	Switches WS revocation check to tokenRevoked (store-backed).
internal/api/ws_revoke_test.go	Updates revocation tests to assert “deletion anywhere ends the socket” property.
internal/api/ws_policy.go	Makes passthrough explicit and defaults to fail-closed for unknown policies.
internal/api/token_handlers.go	Removes the revoked-set write (set no longer exists).
internal/api/api.go	Removes revoked set, adds tokenRevoked store-backed logic, and makes requireScope fail closed on missing principal.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud