Compare commits

..
27 Commits
Author SHA1 Message Date
mathias e037c6b683 Merge pull request 'v0.31.1: cross-origin POST protection, ffprobe/ffmpeg limits, receive cap, SIGTERM, lint gate (#41 #47 #48 #49 #50)' (#55) from sprint/audio-followups into main
CI / Deploy via GitOps (push) Successful in 6s
CI / Lint / Test / Vet (push) Successful in 30s
CI / Build & Import (push) Successful in 10s
2026-10-03 19:19:55 +00:00
mathiasandClaude Sonnet 5.5 5baffd0189 fix(test): strip colour codes before looking for the lint skip notice (CI run 8937)
CI / Lint / Test / Vet (pull_request) Successful in 30s
CI / Build & Import (pull_request) Skipped
CI / Deploy via GitOps (pull_request) Skipped
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:18:05 +02:00
mathiasandClaude Sonnet 5.5 228bcc77da test(lint): the skip notice is found behind CI's colour codes, red
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:18:01 +02:00
mathiasandClaude Sonnet 5.5 4ef446548d docs(adr): spool arithmetic and the unbounded slow-drip limitation for the receive cap (#47)
CI / Lint / Test / Vet (pull_request) Failing after 30s
CI / Build & Import (pull_request) Skipped
CI / Deploy via GitOps (pull_request) Skipped
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:12:48 +02:00
mathiasandClaude Sonnet 5.5 145949875e fix(audio): keep a bounded head of ffmpeg's output instead of buffering all of it (#49)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:12:48 +02:00
mathiasandClaude Sonnet 5.5 82aeac7c19 fix(store): FailUpload only fails a queued or transcribing upload (review of #55)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:12:48 +02:00
mathiasandClaude Sonnet 5.5 1b4747ed03 test: FailUpload must not overwrite a finished upload; ffmpeg stderr is bounded, red (review of #55)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:12:34 +02:00
mathiasandClaude Sonnet 5.5 9939afb90c test: pin the production receive limits, split the vacuous drain test, harden the lint-gate test (review of #55)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:12:34 +02:00
mathiasandClaude Sonnet 5.5 3e69c00a64 docs(adr): ADR-033 amended for SIGTERM handling and the recorded deploy limitation (#50, #47)
CI / Lint / Test / Vet (pull_request) Failing after 31s
CI / Build & Import (pull_request) Skipped
CI / Deploy via GitOps (pull_request) Skipped
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:00:55 +02:00
mathiasandClaude Sonnet 5.5 253aa567aa fix(serve): handle SIGTERM and wait for in-flight requests and the worker before exiting (#50)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:00:55 +02:00
mathiasandClaude Sonnet 5.5 5644dc3fc8 fix(uploads): record a job interrupted by shutdown at once, on its own short context (#50)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 21:00:55 +02:00
mathiasandClaude Sonnet 5.5 7df1de6628 test(serve): shutdown waits for in-flight requests and the worker, bounded (#50), red
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:59:57 +02:00
mathiasandClaude Sonnet 5.5 7e920102f6 test: SIGTERM cancels the process context; a shutdown mid-job records the row interrupted at once (#50), red
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:59:40 +02:00
mathiasandClaude Sonnet 5.5 480c7227b5 fix(uploads): bound concurrent request-body receives, 4 total and 2 per user (#47)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:59:09 +02:00
mathiasandClaude Sonnet 5.5 0eb3be9631 test(uploads): concurrent receives are bounded in total and per user (#47), red
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:57:31 +02:00
mathiasandClaude Sonnet 5.5 e8c5ac50bf fix(audio): give ffprobe its own 45s deadline, a timeout is the file's fault (#49)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:56:58 +02:00
mathiasandClaude Sonnet 5.5 6101dd4e2f test(audio): a hung ffprobe times out on its own deadline as unreadable (#49), red
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:56:47 +02:00
mathiasandClaude Sonnet 5.5 089444c19f fix(audio): stop reading ffprobe output past a cap and call the file unreadable (#49)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:56:47 +02:00
mathiasandClaude Sonnet 5.5 d44db88787 test(audio): ffprobe output past a cap is unreadable, not buffered (#49), red
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:56:37 +02:00
mathiasandClaude Sonnet 5.5 1f8bd849fe fix(audio): cap ffprobe/ffmpeg streams at 32 and narrow the probe output (#49)
A 900-stream file cost ~570 MB RSS in each tool; with the cap the same file fails
in ~18 MB and is reported as unreadable.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:56:28 +02:00
mathiasandClaude Sonnet 5.5 7b19801bfa test(audio): ffprobe/ffmpeg must cap streams, ffprobe must narrow its output (#49), red
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:55:54 +02:00
mathiasandClaude Sonnet 5.5 1260500d32 fix(web): refuse cross-origin non-safe requests at the router (#48)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:55:19 +02:00
mathiasandClaude Sonnet 5.5 10b4c43bd2 test(web): foreign-origin POSTs must be refused by the router (#48), red
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:55:00 +02:00
mathiasandClaude Sonnet 5.5 f1f8b6d02e fix(task): lint step fails on findings instead of printing 'not installed' (#41)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:54:20 +02:00
mathiasandClaude Sonnet 5.5 0d4c5c9d1d test(lint): task lint must exit non-zero on a finding (#41), red
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:53:50 +02:00
mathiasandClaude Sonnet 5.5 24d65b2085 docs(sprint): audio upload follow-ups sweep sprint plan, oath and decision list
CI / Lint / Test / Vet (push) Successful in 27s
CI / Build & Import (push) Successful in 22s
CI / Deploy via GitOps (push) Successful in 5s
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 20:50:34 +02:00
mathiasandClaude Sonnet 5.5 df7bd459ed docs(adr): the ingress body-size item is resolved, and uploads are on in production (infra#520)
CI / Build & Import (push) Successful in 13s
CI / Deploy via GitOps (push) Successful in 5s
CI / Lint / Test / Vet (push) Successful in 36s
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-03 16:29:32 +02:00
20 changed files with 1080 additions and 44 deletions
+20 -1
View File
@@ -1447,6 +1447,25 @@ an hour of Swedish *was* an unknown here; it is now measured, see decision 8.)
re-uploads. The single-replica assumption gains a second dependent (the semaphore is re-uploads. The single-replica assumption gains a second dependent (the semaphore is
process-local) and is recorded here. process-local) and is recorded here.
*Amended 2026-10-03 (#50, #47).* The process now handles SIGTERM (what Kubernetes sends on
every deploy; only SIGINT was handled, so shutdown code never ran). On shutdown the running job
is recorded `interrupted` at once and the process waits, bounded to 10 s each, for in-flight
requests and for that write; the start-up sweep stays the backstop for a kill. **Known
limitation, deliberately not built: a deploy still loses a job in flight** (finishing it needs
a drain of up to the job time, a ~330 s termination grace and `strategy: Recreate`, i.e.
downtime on every deploy; a 90-minute file takes ~4.5 min and a deploy lands on a running job
perhaps one time in five at current volume). Jobs still queued at shutdown stay `queued` until
the next start's sweep. The user re-uploads, with an honest message.
*Receive limits (#47).* Request bodies are received at most 4 at a time, 2 per user, answered
as busy beyond that. Together with the 20-job queue and the running job that bounds the spool
at about (4 + 20 + 1) x 200 MiB = 5 GiB of the 6 Gi `emptyDir` (infra): raising any of those or
`TAPIR_UPLOAD_MAX_BYTES` must revisit that sum (pinned by `TestNewIntake_Defaults...`, not
enforced against infra). **A slow-drip body holds its slot without a time bound**: the server
sets `ReadHeaderTimeout` only, because a `ReadTimeout` would cut legitimate 200 MiB uploads on
slow links; two slow bodies take a user's two slots and two such users make everyone's upload
busy. It needs a valid session; recorded as a limitation with a follow-up issue.
8. **Chat reuses `chat.Service`; it is not forked.** The uploaded-transcript chat handlers 8. **Chat reuses `chat.Service`; it is not forked.** The uploaded-transcript chat handlers
look the row up through the RLS-scoped store, so a non-owner gets 404 by the same mechanism look the row up through the RLS-scoped store, so a non-owner gets 404 by the same mechanism
as ADR-027's summary gate, **before any model is called**. ADR-027's decisions (ephemeral, as ADR-027's summary gate, **before any model is called**. ADR-027's decisions (ephemeral,
@@ -1531,7 +1550,7 @@ deferred. The scope cut — chat first, no rich analysis, no video — is what k
#33 specifies, with the serving endpoint shown **(recommended, matches the issue)**; (B) #33 specifies, with the serving endpoint shown **(recommended, matches the issue)**; (B)
disabled by default, a failed iguana job fails honestly and the user retries. *Blocks:* disabled by default, a failed iguana job fails honestly and the user retries. *Blocks:*
the production default only; the adapter ships either way. the production default only; the adapter ships either way.
- Verify Tapir's ingress `proxy-body-size` against the 200 MB cap (`infra`). - ~~Verify Tapir's ingress `proxy-body-size` against the 200 MB cap (`infra`).~~ **Resolved 2026-10-03:** real unauthenticated bodies of 2, 30, 120 and 210 MiB all pass NPM (TLS, piguard) and Traefik to tapir (a 302 to login); there is no body limit in that range. Uploads were enabled in production the same day, local engines only (infra#520).
- Per-transcript deletion is not built; account deletion cascades. A private-content store - Per-transcript deletion is not built; account deletion cascades. A private-content store
without a delete button is a gap worth a ticket, not a silent omission. without a delete button is a gap worth a ticket, not a silent omission.
+1 -1
View File
@@ -34,7 +34,7 @@ tasks:
lint: lint:
desc: golangci-lint (installed by CI; skipped locally if absent). desc: golangci-lint (installed by CI; skipped locally if absent).
cmds: cmds:
- 'command -v golangci-lint >/dev/null 2>&1 && golangci-lint run ./... || echo "golangci-lint not installed — skipping (CI installs it)"' - 'if command -v golangci-lint >/dev/null 2>&1; then golangci-lint run ./...; else echo "golangci-lint not installed — skipping (CI installs it)"; fi'
test: test:
desc: Run all tests. -p 1 serializes packages so the embedded-postgres tests desc: Run all tests. -p 1 serializes packages so the embedded-postgres tests
+17 -16
View File
@@ -17,9 +17,9 @@ import (
"errors" "errors"
"fmt" "fmt"
"log/slog" "log/slog"
"net"
"net/http" "net/http"
"os" "os"
"os/signal"
"sync" "sync"
"time" "time"
@@ -41,7 +41,7 @@ func main() {
} }
log := slog.New(slog.NewTextHandler(os.Stderr, nil)) log := slog.New(slog.NewTextHandler(os.Stderr, nil))
ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) ctx, stop := shutdownContext(context.Background())
defer stop() defer stop()
var err error var err error
@@ -215,9 +215,11 @@ func cmdServe(ctx context.Context, log *slog.Logger) error {
if err != nil { if err != nil {
return err return err
} }
var workerDone chan struct{} // nil without uploads: nothing to wait for at shutdown
wireUploads(app, cfg, intake) wireUploads(app, cfg, intake)
if intake != nil { if intake != nil {
go worker.Run(ctx) workerDone = make(chan struct{})
go func() { worker.Run(ctx); close(workerDone) }()
log.Info("audio upload enabled", "model", cfg.TranscribeModel, "fallback", cfg.TranscribeFallbackModel, log.Info("audio upload enabled", "model", cfg.TranscribeModel, "fallback", cfg.TranscribeFallbackModel,
"max_mb", cfg.UploadMaxBytes>>20, "max_minutes", int(cfg.UploadMaxDuration.Minutes())) "max_mb", cfg.UploadMaxBytes>>20, "max_minutes", int(cfg.UploadMaxDuration.Minutes()))
log.Warn("audio upload assumes a SINGLE replica: the transcription queue is process-local (ADR-033, ADR-018)") log.Warn("audio upload assumes a SINGLE replica: the transcription queue is process-local (ADR-033, ADR-018)")
@@ -350,20 +352,19 @@ func cmdServe(ctx context.Context, log *slog.Logger) error {
}() }()
} }
// Graceful shutdown on signal: stop accepting, drain in-flight requests. // Graceful shutdown on signal: stop accepting, drain in-flight requests, and let the
go func() { // worker record the job it was running as interrupted (#50).
<-ctx.Done() var extra []*http.Server
shutdownCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) if metricsSrv != nil {
defer cancel() extra = append(extra, metricsSrv)
_ = srv.Shutdown(shutdownCtx) }
if metricsSrv != nil { ln, err := net.Listen("tcp", srv.Addr)
_ = metricsSrv.Shutdown(shutdownCtx) if err != nil {
} return fmt.Errorf("serve: listen: %w", err)
}() }
log.Info("serving web ui", "addr", cfg.HTTPAddr, "user", cfg.UserID) log.Info("serving web ui", "addr", cfg.HTTPAddr, "user", cfg.UserID)
if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { if err := serveHTTP(ctx, srv, ln, extra, workerDone, shutdownGrace); err != nil {
return fmt.Errorf("serve: %w", err) return err
} }
return nil return nil
} }
+55
View File
@@ -0,0 +1,55 @@
package main
import (
"context"
"errors"
"fmt"
"net"
"net/http"
"os"
"os/signal"
"syscall"
"time"
)
// shutdownGrace bounds each wait at shutdown (in-flight requests, then the worker). Two of
// them stay inside Kubernetes' default 30 s termination grace.
const shutdownGrace = 10 * time.Second
// shutdownContext is cancelled by SIGINT (a terminal) or SIGTERM (what Kubernetes sends
// on every deploy). Handling only SIGINT meant a deploy killed the process before any
// shutdown code ran (#50).
func shutdownContext(parent context.Context) (context.Context, context.CancelFunc) {
return signal.NotifyContext(parent, os.Interrupt, syscall.SIGTERM)
}
// serveHTTP serves srv on ln until it fails or ctx ends, then shuts every server down and
// waits for in-flight requests and for workerDone (nil: no worker) to finish, each for at
// most grace. It must not return earlier: Serve returns http.ErrServerClosed as soon as
// Shutdown STARTS, and returning from main kills the drain and the worker's last write.
func serveHTTP(ctx context.Context, srv *http.Server, ln net.Listener, extra []*http.Server, workerDone <-chan struct{}, grace time.Duration) error {
errc := make(chan error, 1)
go func() { errc <- srv.Serve(ln) }()
select {
case err := <-errc:
if !errors.Is(err, http.ErrServerClosed) {
return fmt.Errorf("serve: %w", err)
}
return nil
case <-ctx.Done():
}
shutdownCtx, cancel := context.WithTimeout(context.Background(), grace)
defer cancel()
for _, s := range append([]*http.Server{srv}, extra...) {
_ = s.Shutdown(shutdownCtx)
}
if workerDone != nil {
select {
case <-workerDone:
case <-time.After(grace):
}
}
return nil
}
+124
View File
@@ -0,0 +1,124 @@
package main
import (
"context"
"net/http"
"net/http/httptest"
"os"
"sync"
"syscall"
"testing"
"time"
)
// Kubernetes stops a pod with SIGTERM; only SIGINT was handled, so a deploy killed the
// process without running any shutdown code (#50).
func TestShutdownContext_IsCancelledBySIGTERMAndSIGINT(t *testing.T) {
for _, sig := range []syscall.Signal{syscall.SIGTERM, syscall.SIGINT} {
t.Run(sig.String(), func(t *testing.T) {
ctx, stop := shutdownContext(context.Background())
defer stop()
if err := syscall.Kill(os.Getpid(), sig); err != nil {
t.Fatalf("send %v: %v", sig, err)
}
select {
case <-ctx.Done():
case <-time.After(2 * time.Second):
t.Fatalf("context not cancelled by %v", sig)
}
})
}
}
// serveHTTP must not return while a request is in flight or the worker is still recording
// its job: Serve returns http.ErrServerClosed as soon as Shutdown STARTS, and returning
// from it let the process exit before either finished (#50). Each half is tested with the
// other already satisfied, so neither can hold the call open on the other's behalf.
func pendingFor(t *testing.T, returned <-chan error, why string) {
t.Helper()
select {
case err := <-returned:
t.Fatalf("serveHTTP returned (%v) %s", err, why)
case <-time.After(150 * time.Millisecond):
}
}
func TestServeHTTP_DrainsAnInFlightRequestBeforeReturning(t *testing.T) {
release := make(chan struct{})
entered := make(chan struct{})
srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
close(entered)
<-release
_, _ = w.Write([]byte("ok"))
}))
srv.Start()
defer srv.Close()
var once sync.Once
releaseHandler := func() { once.Do(func() { close(release) }) }
defer releaseHandler() // runs before srv.Close: a failing test must not hang on the blocked handler
ctx, cancel := context.WithCancel(context.Background())
workerDone := make(chan struct{})
close(workerDone) // the worker is already done: only the request can hold serveHTTP open
returned := make(chan error, 1)
go func() { returned <- serveHTTP(ctx, srv.Config, srv.Listener, nil, workerDone, 5*time.Second) }()
go func() { _, _ = http.Get(srv.URL) }()
<-entered
cancel()
pendingFor(t, returned, "while a request was still in flight")
releaseHandler()
select {
case err := <-returned:
if err != nil {
t.Fatalf("serveHTTP: %v", err)
}
case <-time.After(2 * time.Second):
t.Fatal("serveHTTP did not return once the request finished")
}
}
func TestServeHTTP_WaitsForTheWorkerBeforeReturning(t *testing.T) {
srv := httptest.NewUnstartedServer(http.NotFoundHandler())
srv.Start()
defer srv.Close()
ctx, cancel := context.WithCancel(context.Background())
workerDone := make(chan struct{})
returned := make(chan error, 1)
go func() { returned <- serveHTTP(ctx, srv.Config, srv.Listener, nil, workerDone, 5*time.Second) }() // no request in flight
cancel()
pendingFor(t, returned, "before the worker finished")
close(workerDone)
select {
case err := <-returned:
if err != nil {
t.Fatalf("serveHTTP: %v", err)
}
case <-time.After(2 * time.Second):
t.Fatal("serveHTTP did not return once the worker finished")
}
}
func TestServeHTTP_GivesUpOnAWorkerThatNeverFinishesAfterTheGrace(t *testing.T) {
srv := httptest.NewUnstartedServer(http.NotFoundHandler())
srv.Start()
defer srv.Close()
ctx, cancel := context.WithCancel(context.Background())
returned := make(chan error, 1)
go func() {
returned <- serveHTTP(ctx, srv.Config, srv.Listener, nil, make(chan struct{}), 200*time.Millisecond)
}()
cancel()
select {
case <-returned:
case <-time.After(3 * time.Second):
t.Fatal("a stuck worker must not hold the process past the grace period")
}
}
@@ -0,0 +1,88 @@
# Sprint: audio upload follow-ups (Sweep Sprint, 2026-10)
Method: Sweep Sprint (phase 1 parallel read-only research per issue: **done**; phase 2 sequential
execution, one issue per commit: **not started, waits for the owner's go**). Closing oath below.
Milestones: [v0.31.1 - security and robustness](https://git.d-ma.be/mathias/tapir/milestone/10),
[v0.32.0 - audio upload follow-ups](https://git.d-ma.be/mathias/tapir/milestone/11).
Context: v0.31.0 shipped audio upload (ADR-033/034/035) and is live in production behind auth at
`/uploads`, with the Berget cloud fallback on for audio (infra#522). A real 63.7 min upload
transcribed in 2m53s with no errors. The phase-1 investigation found two pre-existing or new
weaknesses an authenticated user can reach, so the first release is a hotfix.
## Oath (closing gates, every clause must hold; blocked = descoped and tracked, never "done")
| # | Clause | Evidence |
|---|--------|----------|
| SG1 | Every issue in the sprint is closed with a linked commit, or moved out with a stated reason | issue list of both milestones |
| SG2 | Each fix has a test that was seen red on main first, named after the mechanism it observes | red/green output in the issue |
| SG3 | `task check` AND `golangci-lint run ./...` exit 0 (checked by `$?`, not by grep); `task check` does not gate lint until #41 | gate output |
| SG4 | Control code (#48, #49, the upload cap in #47) has a recorded adversarial review verified by execution, verdict in the issue | gatekeeper verdict comment |
| SG5 | Docs current in the same commit: ADR-033/035 text, feature file, use-case coverage test | diff |
| SG6 | Each release tagged (v0.31.1, v0.32.0), pushed, deployed, and observed live (pod running the new image, no restart loop, a real upload still works) | `kubectl`/log + one upload |
| SG7 | Learnings written to the brain; deferred work filed as issues; decisions that are the owner's recorded as DECISION NEEDED | brain ids + issue numbers |
## Work order
### v0.31.1 (security and robustness; hotfix, do first)
1. **#41 lint gate.** `task check` swallows lint findings (`|| echo`). Make the gate fail on findings,
fix what it surfaces. First, so every later gate means something. (Decision D1.)
2. **#48 cross-origin POST protection.** `http.NewCrossOriginProtection()` around `Router()`.
Red: foreign-origin POST to `/account/delete`, `/uploads`, `/uploads/{id}/chat` is refused.
3. **#49 ffprobe/ffmpeg limits.** `-max_streams 32` + narrower probe output; probe stdout cap;
probe deadline. Three commits.
4. **#47 concurrent-upload cap (remaining item).** Bound the spool: reservation per accepted upload
against the 6Gi emptyDir so concurrent uploads cannot evict the pod.
5. **#50 SIGTERM interim.** Handle SIGTERM; record the interrupted job at once; fix the false
comment. Full drain recorded as a limitation. (Decision D2.)
Release v0.31.1: one adversarial review round over 2-5, then tag, push, observe.
### v0.32.0 (follow-ups)
6. **#52** label a cloud-served transcript in plain words; document the empty-transcript trigger.
7. **#53** bound the LLM client's response body read.
8. **#51** tracker: save timeout, then app-clock heartbeat.
9. **#37** nav link to uploads and per-recording delete (queued/transcribing: refuse until it
ends). (Decision D3.)
10. **#36** chat default model after measurement. (Decision D4.)
11. **#38** measurements (Berget length limit, large models on production iguana). (D5, D6.)
12. **#46** remainder of the hardening list after the items above are split out.
13. **#45** evaluate Berget as the reconciler: a question, not work. (D8.)
## DECISION NEEDED (owner's call; recommendation first)
| # | Question | Recommendation | Blocks |
|---|----------|----------------|--------|
| D1 | #41: should any linter be allowed to opt out of the gate? | No opt-out; fix findings | #41 |
| D2 | #50: full SIGTERM drain, or record as a limitation and ship the interim fix? | Limitation + interim; a drain costs 4-6 h and downtime on every deploy at current volume | #50 |
| D3 | #37: delete of a queued/transcribing recording | Refuse until it ends | #37 |
| D4 | #36: make a 31B model the chat default? | Only after measurement, conditional | #36 |
| D5 | #38: Berget length measurement needs a throwaway key | Yes, throwaway key | #38 |
| D6 | #38: load large models on production iguana | Yes, in a quiet window with the owner present | #38 |
| D7 | #46: where the ffmpeg tarball lives | Do nothing now; file an issue | none |
| D8 | #45: evaluate Berget as the reconciler | No; stay local-only | #45 |
| D9 | Who judges ADR-034's second blind check | The owner; do not reuse the one-off Opus authorisation | ADR-034 |
| D10 | When to enable reconcile in production | Needs `iguana/pianissimo` in the key pin and env vars; owner present | ADR-034 |
## Dropped or reduced, with reasons
- **#46 item 1 (`alis`/`dref` external reference):** checked, not a problem. Under the application's
flags ffmpeg skips the external track and writes a 78-byte WAV.
- **#46 item 2 / #47 item 2:** merged into #49.
- **#47 item 3 (full drain):** reduced to #50 plus a recorded limitation.
- **#46 `Beat` taking the tracker mutex:** incorrect in #46; only `logStill` does. Corrected in #51.
## Human-present items (do not run unattended)
- Production iguana model loads (#38, D6).
- Enabling reconcile in production (D10).
- Any key change or `apply` in infra (infra#523, infra#524 are open and not part of this sprint).
## Release plan
SemVer: v0.31.1 patch (fixes, no new behaviour); v0.32.0 minor (nav link, delete, cloud labelling).
Every push to tapir main redeploys the pod and fails any job in flight: push when no upload is
running (check the pod log first).
+6 -2
View File
@@ -47,11 +47,15 @@ func (f *FFmpeg) ToWAV(ctx context.Context, src, dst string) error {
args = append(args, "-f", "wav", dst) args = append(args, "-f", "wav", dst)
cmd := exec.CommandContext(ctx, bin, args...) cmd := exec.CommandContext(ctx, bin, args...)
cmd.WaitDelay = killGrace cmd.WaitDelay = killGrace
if out, err := cmd.CombinedOutput(); err != nil { // Only a bounded head of ffmpeg's output is kept: a hostile file made it print ~0.4
// bytes per input byte, all of which CombinedOutput would buffer and copy (#49).
out := &headWriter{max: maxStderr}
cmd.Stdout, cmd.Stderr = out, out
if err := cmd.Run(); err != nil {
if ctxErr := ctx.Err(); ctxErr != nil { if ctxErr := ctx.Err(); ctxErr != nil {
return fmt.Errorf("audio: ffmpeg: %w", ctxErr) return fmt.Errorf("audio: ffmpeg: %w", ctxErr)
} }
return fmt.Errorf("audio: ffmpeg: %w: %s", err, strings.TrimSpace(string(out))) return fmt.Errorf("audio: ffmpeg: %w: %s", err, strings.TrimSpace(out.buf.String()))
} }
// A zero exit that wrote nothing is still a failure: never hand whisper a // A zero exit that wrote nothing is still a failure: never hand whisper a
// missing file and let the error surface three layers later. // missing file and let the error surface three layers later.
+29
View File
@@ -109,3 +109,32 @@ printf data > "$last"`)
require.NoError(t, err) require.NoError(t, err)
assert.NotContains(t, " "+strings.ReplaceAll(string(raw), "\n", " ")+" ", " -t ") assert.NotContains(t, " "+strings.ReplaceAll(string(raw), "\n", " ")+" ", " -t ")
} }
func TestFFmpegToWAV_CapsTheStreamCountBeforeTheInput(t *testing.T) {
// ffmpeg also allocated ~570 MB opening the 900-stream file (#49).
rec := filepath.Join(t.TempDir(), "argv")
bin := stub(t, `printf '%s\n' "$@" > `+rec+`
for a; do last="$a"; done
printf data > "$last"`)
err := (&audio.FFmpeg{Bin: bin}).ToWAV(context.Background(), "/spool/in", filepath.Join(t.TempDir(), "o.wav"))
require.NoError(t, err)
args := argvOf(t, rec)
ms, in := indexOf(args, "-max_streams"), indexOf(args, "-i")
require.GreaterOrEqual(t, ms, 0, "argv: %v", args)
assert.Equal(t, "32", args[ms+1])
assert.Less(t, ms, in, "an input option placed after -i is applied to the output")
}
func TestFFmpegToWAV_ABoundedPartOfAFloodingStderrIsKept(t *testing.T) {
// A hostile mp3 made ffmpeg print ~0.4 bytes of stderr per input byte (83 MB for a
// 209 MB file), all of it buffered, then copied twice into the error (#49 review).
bin := stub(t, `head -c 5000000 /dev/zero | tr '\0' 'e' >&2; exit 1`)
err := (&audio.FFmpeg{Bin: bin}).ToWAV(context.Background(), "/spool/in", filepath.Join(t.TempDir(), "o.wav"))
require.Error(t, err)
assert.Less(t, len(err.Error()), 16*1024, "the error carries a bounded head of stderr, not all of it")
assert.Contains(t, err.Error(), "eeee", "and still carries some of it")
}
+85 -9
View File
@@ -4,6 +4,7 @@
package audio package audio
import ( import (
"bytes"
"context" "context"
"encoding/json" "encoding/json"
"errors" "errors"
@@ -19,6 +20,51 @@ import (
type FFprobe struct { type FFprobe struct {
// Bin is the ffprobe executable ("ffprobe" resolves via PATH). // Bin is the ffprobe executable ("ffprobe" resolves via PATH).
Bin string Bin string
// MaxOutput bounds how much of ffprobe's stdout is read; zero means DefaultMaxOutput.
MaxOutput int
// Timeout bounds one probe; zero means DefaultProbeTimeout. A probe is milliseconds of
// work on a real file, so one that runs this long is the FILE's doing (#49).
Timeout time.Duration
}
// DefaultProbeTimeout is the probe deadline when FFprobe.Timeout is unset.
const DefaultProbeTimeout = 45 * time.Second
// DefaultMaxOutput is far above the narrowed probe output of a legitimate file (a few
// hundred bytes) and far below what a hostile one can make ffprobe print.
const DefaultMaxOutput = 1 << 20
// maxStderr is how much of ffprobe's stderr is kept for the error message.
const maxStderr = 4096
// errOutputCap is the write error that stops ffprobe once it prints past the cap.
var errOutputCap = errors.New("audio: ffprobe output exceeds the cap")
// capWriter collects up to max bytes and then refuses further writes, which closes the
// pipe and ends the process instead of buffering whatever it prints.
type capWriter struct {
buf bytes.Buffer
max int
}
func (w *capWriter) Write(p []byte) (int, error) {
if w.buf.Len()+len(p) > w.max {
return 0, errOutputCap
}
return w.buf.Write(p)
}
// headWriter keeps the first max bytes and silently drops the rest.
type headWriter struct {
buf bytes.Buffer
max int
}
func (w *headWriter) Write(p []byte) (int, error) {
if room := w.max - w.buf.Len(); room > 0 {
w.buf.Write(p[:min(room, len(p))])
}
return len(p), nil
} }
// AllowedDemuxers are the input formats an upload may be. The input is // AllowedDemuxers are the input formats an upload may be. The input is
@@ -30,9 +76,22 @@ type FFprobe struct {
// formats. "mov" covers .mp4/.m4a/.3gp; "matroska" covers .webm. // formats. "mov" covers .mp4/.m4a/.3gp; "matroska" covers .webm.
const AllowedDemuxers = "wav,mov,mp4,m4a,3gp,mp3,ogg,matroska,webm,flac,aac,opus,aiff,amr,asf" const AllowedDemuxers = "wav,mov,mp4,m4a,3gp,mp3,ogg,matroska,webm,flac,aac,opus,aiff,amr,asf"
// MaxStreams bounds how many streams a file may declare. A 1.3 MB file declaring 900 of
// them cost ~570 MB of RSS in each of ffprobe and ffmpeg (#49), against a 512 Mi pod;
// real recordings have a handful.
const MaxStreams = "32"
// inputRestrictions are ffmpeg/ffprobe INPUT options and must precede the input. // inputRestrictions are ffmpeg/ffprobe INPUT options and must precede the input.
// Local files and pipes only: no network, no other protocols. // Local files and pipes only: no network, no other protocols, and a bounded stream count.
var inputRestrictions = []string{"-protocol_whitelist", "file,pipe", "-format_whitelist", AllowedDemuxers} var inputRestrictions = []string{
"-protocol_whitelist", "file,pipe",
"-format_whitelist", AllowedDemuxers,
"-max_streams", MaxStreams,
}
// probeEntries is exactly what ParseProbe reads. -show_format/-show_streams print every
// tag too, and a 120 MB WAV made 491 MB of JSON that Output() buffers.
const probeEntries = "format=duration:stream=codec_type:stream_disposition=attached_pic"
// killGrace bounds how long Probe waits for a cancelled ffprobe's pipes to close. // killGrace bounds how long Probe waits for a cancelled ffprobe's pipes to close.
const killGrace = 2 * time.Second const killGrace = 2 * time.Second
@@ -48,21 +107,38 @@ func (f *FFprobe) Probe(ctx context.Context, path string) (uploads.Probe, error)
bin = "ffprobe" bin = "ffprobe"
} }
args := append([]string{"-v", "error"}, inputRestrictions...) args := append([]string{"-v", "error"}, inputRestrictions...)
args = append(args, "-print_format", "json", "-show_format", "-show_streams", path) args = append(args, "-print_format", "json", "-show_entries", probeEntries, path)
cmd := exec.CommandContext(ctx, bin, args...) timeout := f.Timeout
if timeout <= 0 {
timeout = DefaultProbeTimeout
}
probeCtx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
cmd := exec.CommandContext(probeCtx, bin, args...)
cmd.WaitDelay = killGrace cmd.WaitDelay = killGrace
out, err := cmd.Output() maxOut := f.MaxOutput
if err != nil { if maxOut <= 0 {
if ctxErr := ctx.Err(); ctxErr != nil { maxOut = DefaultMaxOutput
}
stdout, stderr := &capWriter{max: maxOut}, &headWriter{max: maxStderr}
cmd.Stdout, cmd.Stderr = stdout, stderr
if err := cmd.Run(); err != nil {
if ctxErr := ctx.Err(); ctxErr != nil { // the caller gave up: not a verdict on the file
return uploads.Probe{}, fmt.Errorf("audio: ffprobe: %w", ctxErr) return uploads.Probe{}, fmt.Errorf("audio: ffprobe: %w", ctxErr)
} }
if probeCtx.Err() != nil { // our own deadline passed while the caller was still waiting
return uploads.Probe{}, fmt.Errorf("%w: ffprobe did not finish within %s", uploads.ErrUnreadable, timeout)
}
if errors.Is(err, errOutputCap) {
return uploads.Probe{}, fmt.Errorf("%w: ffprobe printed more than %d bytes", uploads.ErrUnreadable, maxOut)
}
var exit *exec.ExitError var exit *exec.ExitError
if errors.As(err, &exit) { if errors.As(err, &exit) {
return uploads.Probe{}, fmt.Errorf("%w: ffprobe: %s", uploads.ErrUnreadable, exit.Stderr) return uploads.Probe{}, fmt.Errorf("%w: ffprobe: %s", uploads.ErrUnreadable, stderr.buf.String())
} }
return uploads.Probe{}, fmt.Errorf("audio: run ffprobe: %w", err) return uploads.Probe{}, fmt.Errorf("audio: run ffprobe: %w", err)
} }
p, err := ParseProbe(out) p, err := ParseProbe(stdout.buf.Bytes())
if err != nil { if err != nil {
return uploads.Probe{}, fmt.Errorf("audio: ffprobe output: %w", err) return uploads.Probe{}, fmt.Errorf("audio: ffprobe output: %w", err)
} }
+75
View File
@@ -184,3 +184,78 @@ func TestAllowedDemuxers_ExcludeThePlaylistFormats(t *testing.T) {
assert.Contains(t, strings.Split(audio.AllowedDemuxers, ","), good) assert.Contains(t, strings.Split(audio.AllowedDemuxers, ","), good)
} }
} }
func TestFFprobeProbe_CapsTheStreamCountBeforeTheInput(t *testing.T) {
// A 1.3 MB file declaring 900 streams cost ~570 MB of RSS in ffprobe (#49) against
// a 512 Mi pod. -max_streams is an input option: it must precede the path.
bin, argvFile := argvStub(t)
_, err := (&audio.FFprobe{Bin: bin}).Probe(context.Background(), "/spool/upload-1")
require.NoError(t, err)
args := argvOf(t, argvFile)
ms := indexOf(args, "-max_streams")
require.GreaterOrEqual(t, ms, 0, "argv: %v", args)
assert.Equal(t, "32", args[ms+1])
assert.Less(t, ms, len(args)-1, "must come before the path, which stays last")
}
func TestFFprobeProbe_AsksOnlyForTheFieldsParseProbeReads(t *testing.T) {
// -show_format/-show_streams print every tag: a 120 MB WAV produced 491 MB of JSON
// that cmd.Output() buffers in Go (#49).
bin, argvFile := argvStub(t)
_, err := (&audio.FFprobe{Bin: bin}).Probe(context.Background(), "/spool/upload-1")
require.NoError(t, err)
args := argvOf(t, argvFile)
assert.Equal(t, -1, indexOf(args, "-show_format"), "argv: %v", args)
assert.Equal(t, -1, indexOf(args, "-show_streams"), "argv: %v", args)
se := indexOf(args, "-show_entries")
require.GreaterOrEqual(t, se, 0, "argv: %v", args)
assert.Equal(t, "format=duration:stream=codec_type:stream_disposition=attached_pic", args[se+1])
}
func TestFFprobeProbe_OutputPastTheCapIsUnreadableNotBuffered(t *testing.T) {
// ffprobe's stdout is attacker-influenced (tags, 491 MB observed). Past the cap the
// probe is stopped and the file is reported as unreadable (#49).
bin := stub(t, `head -c 200000 /dev/zero | tr '\0' 'a'`)
_, err := (&audio.FFprobe{Bin: bin, MaxOutput: 4096}).Probe(context.Background(), "/x")
assert.ErrorIs(t, err, uploads.ErrUnreadable)
}
func TestFFprobeProbe_OutputWithinTheCapIsParsed(t *testing.T) {
bin := stub(t, `echo '{"streams":[{"codec_type":"audio"}],"format":{"duration":"90"}}'`)
got, err := (&audio.FFprobe{Bin: bin, MaxOutput: 4096}).Probe(context.Background(), "/x")
require.NoError(t, err)
assert.True(t, got.HasAudio)
}
func TestFFprobeProbe_AHungProbeTimesOutOnItsOwnDeadlineAsUnreadableNotAsOurFault(t *testing.T) {
// Accept runs the probe on the request context, which has no deadline of its own;
// a probe that never returns would pin a request (#49). `exec` so the sleep is the
// process itself and not a child holding the pipe open.
bin := stub(t, `exec sleep 30`)
start := time.Now()
_, err := (&audio.FFprobe{Bin: bin, Timeout: 200 * time.Millisecond}).Probe(context.Background(), "/x")
assert.ErrorIs(t, err, uploads.ErrUnreadable)
assert.Less(t, time.Since(start), 3*time.Second)
}
func TestFFprobeProbe_ParentCancellationIsStillNotUnreadableWhenATimeoutIsSet(t *testing.T) {
bin := stub(t, `exec sleep 30`)
ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
defer cancel()
_, err := (&audio.FFprobe{Bin: bin, Timeout: time.Minute}).Probe(ctx, "/x")
require.Error(t, err)
assert.False(t, errors.Is(err, uploads.ErrUnreadable), "the caller giving up is not a verdict on the file")
assert.ErrorIs(t, err, context.DeadlineExceeded)
}
@@ -88,12 +88,16 @@ func (s *Store) Heartbeat(ctx context.Context, userID, id string) error {
WHERE id = $1 AND user_id = $2`) WHERE id = $1 AND user_id = $2`)
} }
// FailUpload moves an upload to failed with a typed reason. // FailUpload moves a queued or transcribing upload to failed with a typed reason. A
// finished upload is left as it was: a shutdown can record a job as failed after its
// transcript was already committed (the commit lands, the caller sees a cancelled
// context), and that must not hide the text. Such a call touches no row and returns
// ErrUploadNotFound.
func (s *Store) FailUpload(ctx context.Context, userID, id string, kind domain.UploadErrorKind) error { func (s *Store) FailUpload(ctx context.Context, userID, id string, kind domain.UploadErrorKind) error {
return s.updateUpload(ctx, userID, id, "fail upload", return s.updateUpload(ctx, userID, id, "fail upload",
`UPDATE uploaded_transcripts `UPDATE uploaded_transcripts
SET status = 'failed', error_kind = $3, finished_at = NOW() SET status = 'failed', error_kind = $3, finished_at = NOW()
WHERE id = $1 AND user_id = $2`, WHERE id = $1 AND user_id = $2 AND status IN ('queued', 'transcribing')`,
string(kind)) string(kind))
} }
@@ -0,0 +1,47 @@
package store_test
// A shutdown can record a job as failed after its transcript was already committed (the
// commit lands, the caller sees a cancelled context). FailUpload must not be able to turn a
// finished row into a failed one and hide the text (#50 review).
import (
"context"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"git.d-ma.be/mathias/tapir/internal/domain"
)
func TestFailUpload_LeavesAFinishedUploadAsItWas(t *testing.T) {
s := newStore(t)
ctx := context.Background()
id := startedUpload(t, s)
require.NoError(t, s.StartTranscribing(ctx, userA, id))
require.NoError(t, s.CompleteUpload(ctx, userA, id, domain.Transcription{Text: "synthetic text", Endpoint: "iguana/kb-whisper"}))
_ = s.FailUpload(ctx, userA, id, domain.UploadErrInterrupted)
got, err := s.GetUpload(ctx, userA, id)
require.NoError(t, err)
assert.Equal(t, domain.UploadDone, got.Status)
assert.Equal(t, "synthetic text", got.Content)
assert.Empty(t, got.ErrorKind)
}
func TestFailUpload_StillFailsAQueuedOrTranscribingUpload(t *testing.T) {
s := newStore(t)
ctx := context.Background()
queued := startedUpload(t, s)
running, err := s.CreateUpload(ctx, userA, "r", false)
require.NoError(t, err)
require.NoError(t, s.StartTranscribing(ctx, userA, running))
for _, id := range []string{queued, running} {
require.NoError(t, s.FailUpload(ctx, userA, id, domain.UploadErrBusy))
got, err := s.GetUpload(ctx, userA, id)
require.NoError(t, err)
assert.Equal(t, domain.UploadFailed, got.Status)
}
}
+53
View File
@@ -6,6 +6,7 @@ import (
"fmt" "fmt"
"io" "io"
"os" "os"
"sync"
"time" "time"
"git.d-ma.be/mathias/tapir/internal/domain" "git.d-ma.be/mathias/tapir/internal/domain"
@@ -51,6 +52,53 @@ type Intake struct {
sub Submitter sub Submitter
limits Limits limits Limits
tmpDir string tmpDir string
// MaxReceives and MaxReceivesPerUser bound how many request bodies Accept reads at
// once; zero means the defaults. Without them N parallel POSTs each spool up to
// MaxBytes while the queue still looks empty, and the spool volume fills (#47).
MaxReceives int
MaxReceivesPerUser int
mu sync.Mutex
receives int
byUser map[string]int
}
const (
defaultMaxReceives = 4
defaultMaxReceivesPerUser = 2
)
// acquireReceive reserves a receive slot for userID, or reports there is none. Every
// successful call must be paired with releaseReceive.
func (in *Intake) acquireReceive(userID string) bool {
maxAll, maxUser := in.MaxReceives, in.MaxReceivesPerUser
if maxAll <= 0 {
maxAll = defaultMaxReceives
}
if maxUser <= 0 {
maxUser = defaultMaxReceivesPerUser
}
in.mu.Lock()
defer in.mu.Unlock()
if in.receives >= maxAll || in.byUser[userID] >= maxUser {
return false
}
if in.byUser == nil {
in.byUser = map[string]int{}
}
in.receives++
in.byUser[userID]++
return true
}
func (in *Intake) releaseReceive(userID string) {
in.mu.Lock()
defer in.mu.Unlock()
in.receives--
if in.byUser[userID]--; in.byUser[userID] <= 0 {
delete(in.byUser, userID)
}
} }
// NewIntake wires an Intake. tmpDir is where uploads are spooled; it must exist. // NewIntake wires an Intake. tmpDir is where uploads are spooled; it must exist.
@@ -94,6 +142,11 @@ func (in *Intake) Accept(ctx context.Context, userID, title string, src io.Reade
if rc, ok := in.sub.(interface{ HasRoom(userID string) bool }); ok && !rc.HasRoom(userID) { if rc, ok := in.sub.(interface{ HasRoom(userID string) bool }); ok && !rc.HasRoom(userID) {
return "", &RejectError{Reason: ReasonBusy, Message: busyMessage} return "", &RejectError{Reason: ReasonBusy, Message: busyMessage}
} }
// HasRoom cannot see bodies still being received; bound those separately.
if !in.acquireReceive(userID) {
return "", &RejectError{Reason: ReasonBusy, Message: busyMessage}
}
defer in.releaseReceive(userID)
f, err := os.CreateTemp(in.tmpDir, "upload-*") f, err := os.CreateTemp(in.tmpDir, "upload-*")
if err != nil { if err != nil {
return "", fmt.Errorf("uploads: spool: %w", err) return "", fmt.Errorf("uploads: spool: %w", err)
+131
View File
@@ -0,0 +1,131 @@
package uploads_test
// Observes how many request bodies Accept will read at once (#47). HasRoom and the
// per-user queue cap act before and after the receive; N parallel POSTs all pass
// HasRoom while the queue is still empty, so the receive itself is bounded.
import (
"context"
"errors"
"io"
"strings"
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"git.d-ma.be/mathias/tapir/internal/uploads"
)
// slowReceive starts an Accept whose body stays open until the returned writer is closed,
// and waits until it holds a spool file, which proves it is inside the receive.
type slowReceive struct {
w *io.PipeWriter
done chan error
finished bool
}
func startSlowReceive(t *testing.T, r *rig, in *uploads.Intake, user string, wantFiles int) *slowReceive {
t.Helper()
pr, pw := io.Pipe()
s := &slowReceive{w: pw, done: make(chan error, 1)}
go func() {
_, err := in.Accept(context.Background(), user, "t", pr)
s.done <- err
}()
require.Eventually(t, func() bool { return len(r.tmpFiles(t)) == wantFiles }, 2*time.Second, 5*time.Millisecond,
"the receive for %s should have started", user)
// An abandoned receive ends in a read error, so it never reaches the rig's fakes, which
// are not safe for concurrent use. A receive that finish() already ended is a no-op.
t.Cleanup(func() {
if s.finished {
return
}
_ = pw.CloseWithError(errors.New("test over"))
select {
case <-s.done:
case <-time.After(2 * time.Second):
}
})
return s
}
func (s *slowReceive) finish(t *testing.T) error {
t.Helper()
_, _ = s.w.Write([]byte("data"))
require.NoError(t, s.w.Close())
s.finished = true
select {
case err := <-s.done:
return err
case <-time.After(2 * time.Second):
t.Fatal("Accept did not return after its body ended")
return nil
}
}
func requireBusy(t *testing.T, err error) {
t.Helper()
var rej *uploads.RejectError
require.True(t, errors.As(err, &rej), "want a busy rejection, got %v", err)
assert.Equal(t, uploads.ReasonBusy, rej.Reason)
}
func TestAccept_APerUserLimitOnConcurrentReceivesRefusesTheNextBodyBeforeSpoolingIt(t *testing.T) {
r := newRig(t)
in := uploads.NewIntake(r.st, r.pr, r.sub, uploads.Limits{MaxBytes: 1024, MaxDuration: time.Hour}, r.tmp)
in.MaxReceivesPerUser = 2
in.MaxReceives = 10
startSlowReceive(t, r, in, "user-1", 1)
startSlowReceive(t, r, in, "user-1", 2)
_, err := in.Accept(context.Background(), "user-1", "t", strings.NewReader("data"))
requireBusy(t, err)
assert.Len(t, r.tmpFiles(t), 2, "the refused body was never spooled")
// Another user is not blocked by user-1's two receives.
other := startSlowReceive(t, r, in, "user-2", 3)
require.NoError(t, other.finish(t))
}
func TestAccept_AnOverallLimitOnConcurrentReceivesRefusesTheNextUser(t *testing.T) {
r := newRig(t)
in := uploads.NewIntake(r.st, r.pr, r.sub, uploads.Limits{MaxBytes: 1024, MaxDuration: time.Hour}, r.tmp)
in.MaxReceivesPerUser = 5
in.MaxReceives = 3
for i, u := range []string{"a", "b", "c"} {
startSlowReceive(t, r, in, u, i+1)
}
_, err := in.Accept(context.Background(), "d", "t", strings.NewReader("data"))
requireBusy(t, err)
assert.Len(t, r.tmpFiles(t), 3)
}
func TestAccept_AFinishedReceiveFreesItsSlot(t *testing.T) {
r := newRig(t)
in := uploads.NewIntake(r.st, r.pr, r.sub, uploads.Limits{MaxBytes: 1024, MaxDuration: time.Hour}, r.tmp)
in.MaxReceivesPerUser = 1
in.MaxReceives = 5
first := startSlowReceive(t, r, in, "user-1", 1)
require.NoError(t, first.finish(t))
_, err := in.Accept(context.Background(), "user-1", "t", strings.NewReader("data"))
require.NoError(t, err, "the slot is released when Accept returns")
}
func TestAccept_AFailedReceiveFreesItsSlot(t *testing.T) {
r := newRig(t)
in := uploads.NewIntake(r.st, r.pr, r.sub, uploads.Limits{MaxBytes: 1024, MaxDuration: time.Hour}, r.tmp)
in.MaxReceivesPerUser = 1
in.MaxReceives = 5
_, err := in.Accept(context.Background(), "user-1", "t", &brokenBody{n: 1})
require.ErrorIs(t, err, uploads.ErrClientRead)
_, err = in.Accept(context.Background(), "user-1", "t", strings.NewReader("data"))
require.NoError(t, err, "a dropped client must not leak a slot")
}
+31
View File
@@ -0,0 +1,31 @@
package uploads_test
// The limits production runs on are the DEFAULTS: nothing outside tests sets
// MaxReceives or MaxReceivesPerUser. Pinned here with the literal numbers, so changing a
// default is a visible decision (#47 review). With 4 receives, 20 queued and 1 running at
// the 200 MiB cap, the spool needs ~5 GiB of the 6 Gi emptyDir (infra); raising any of
// them or TAPIR_UPLOAD_MAX_BYTES must revisit that arithmetic.
import (
"context"
"strings"
"testing"
"time"
"git.d-ma.be/mathias/tapir/internal/uploads"
)
func TestNewIntake_DefaultsAllowTwoReceivesPerUserAndFourOverall(t *testing.T) {
r := newRig(t)
in := uploads.NewIntake(r.st, r.pr, r.sub, uploads.Limits{MaxBytes: 1024, MaxDuration: time.Hour}, r.tmp)
startSlowReceive(t, r, in, "a", 1)
startSlowReceive(t, r, in, "a", 2)
_, err := in.Accept(context.Background(), "a", "t", strings.NewReader("x"))
requireBusy(t, err) // a third body from one user
startSlowReceive(t, r, in, "b", 3)
startSlowReceive(t, r, in, "b", 4)
_, err = in.Accept(context.Background(), "c", "t", strings.NewReader("x"))
requireBusy(t, err) // a fifth body overall
}
+17 -8
View File
@@ -176,9 +176,10 @@ func (w *Worker) Submit(j Job) error {
} }
// Run processes queued jobs until ctx ends, then deletes the audio of anything // Run processes queued jobs until ctx ends, then deletes the audio of anything
// still queued and refuses new jobs. The rows of those jobs stay queued or // still queued and refuses new jobs. The job running at shutdown is recorded interrupted
// transcribing and are failed as interrupted by the start-up sweep // by Process; the rows of jobs still queued stay queued and are failed as interrupted by
// (store.FailInterruptedUploads): the audio is not retained, so they cannot resume. // the start-up sweep (store.FailInterruptedUploads): the audio is not retained, so they
// cannot resume.
func (w *Worker) Run(ctx context.Context) { func (w *Worker) Run(ctx context.Context) {
defer w.stop() defer w.stop()
for { for {
@@ -227,7 +228,7 @@ func (w *Worker) stop() {
// Process runs one job: row -> transcribing, convert, transcribe, store. The // Process runs one job: row -> transcribing, convert, transcribe, store. The
// uploaded file and the converted WAV are deleted on every path — the audio is // uploaded file and the converted WAV are deleted on every path — the audio is
// never retained (ADR-033). A failure is recorded with a TYPED kind; a shutdown // never retained (ADR-033). A failure is recorded with a TYPED kind; a shutdown
// mid-job is not a failure and leaves the row for the start-up sweep. // mid-job is recorded as interrupted at once (see fail).
// //
// Every phase is reported through one progress.Tracker, which saves it to the row for // Every phase is reported through one progress.Tracker, which saves it to the row for
// the page and logs it, and a heartbeat stamps the row on its own while the job runs // the page and logs it, and a heartbeat stamps the row on its own while the job runs
@@ -269,7 +270,7 @@ func (w *Worker) Process(ctx context.Context, job Job) {
defer func() { stopBeat(); <-hbDone }() // no heartbeat outlives its job defer func() { stopBeat(); <-hbDone }() // no heartbeat outlives its job
// abort records where the job stopped. A shutdown (the parent context ended) is not // abort records where the job stopped. A shutdown (the parent context ended) is not
// a failure: the row is left for the start-up sweep, so the step stays as it was. // a failure of the step: it stays as it was, and fail records the row as interrupted.
abort := func(kind domain.UploadErrorKind) { abort := func(kind domain.UploadErrorKind) {
if ctx.Err() == nil { if ctx.Err() == nil {
tr.FailRunning() tr.FailRunning()
@@ -332,11 +333,19 @@ func (w *Worker) Process(ctx context.Context, job Job) {
"segments", len(t.Segments), "elapsed", time.Since(start).Round(time.Second)) "segments", len(t.Segments), "elapsed", time.Since(start).Round(time.Second))
} }
// fail records a transcription failure — unless the PARENT context ended, which // interruptedWriteTimeout bounds the one write a dying process makes to record its job.
// is a shutdown, not a failure: the row is left for the start-up sweep. const interruptedWriteTimeout = 3 * time.Second
// fail records a transcription failure. When the PARENT context ended the process is
// shutting down: the job is recorded interrupted at once, on a short context of its own,
// because the process may exit before any other pod's start-up sweep would do it (#50).
// The sweep remains the backstop for a process that is killed outright.
func (w *Worker) fail(ctx context.Context, job Job, log *slog.Logger, kind domain.UploadErrorKind) { func (w *Worker) fail(ctx context.Context, job Job, log *slog.Logger, kind domain.UploadErrorKind) {
if ctx.Err() != nil { if ctx.Err() != nil {
return kind = domain.UploadErrInterrupted
var cancel context.CancelFunc
ctx, cancel = context.WithTimeout(context.WithoutCancel(ctx), interruptedWriteTimeout)
defer cancel()
} }
if err := w.store.FailUpload(ctx, job.UserID, job.UploadID, kind); err != nil { if err := w.store.FailUpload(ctx, job.UserID, job.UploadID, kind); err != nil {
log.Error("upload: record failure", "kind", errkind.Of(err)) log.Error("upload: record failure", "kind", errkind.Of(err))
+9 -4
View File
@@ -53,7 +53,10 @@ func (s *wStore) CompleteUpload(_ context.Context, _, id string, t domain.Transc
return nil return nil
} }
func (s *wStore) FailUpload(_ context.Context, _, id string, k domain.UploadErrorKind) error { func (s *wStore) FailUpload(ctx context.Context, _, id string, k domain.UploadErrorKind) error {
if err := ctx.Err(); err != nil { // like the real store: a cancelled context cannot write
return err
}
s.mu.Lock() s.mu.Lock()
defer s.mu.Unlock() defer s.mu.Unlock()
s.failed[id] = k s.failed[id] = k
@@ -242,7 +245,9 @@ func TestWorkerProcess_FailuresAreTypedAndStillDeleteTheAudio(t *testing.T) {
}) })
} }
func TestWorkerProcess_ShutdownMidJobLeavesTheRowForTheStartupSweep(t *testing.T) { func TestWorkerProcess_ShutdownMidJobRecordsTheRowInterruptedAtOnce(t *testing.T) {
// SIGTERM on a deploy used to leave the row "transcribing" until the NEXT pod's
// start-up sweep; the process may exit before any sweep runs (#50).
r := newWRig(t, time.Minute) r := newWRig(t, time.Minute)
r.tr.block = make(chan struct{}) r.tr.block = make(chan struct{})
ctx, cancel := context.WithCancel(context.Background()) ctx, cancel := context.WithCancel(context.Background())
@@ -255,8 +260,8 @@ func TestWorkerProcess_ShutdownMidJobLeavesTheRowForTheStartupSweep(t *testing.T
<-done <-done
_, f := r.st.snapshot() _, f := r.st.snapshot()
assert.Empty(t, f, "a shutdown is not a transcription failure: the row stays for FailInterruptedUploads to mark interrupted") assert.Equal(t, domain.UploadErrInterrupted, f["a"], "the row is recorded interrupted by the dying process, not left for a sweep")
assert.Empty(t, r.leftovers(t), "but the audio is still deleted") assert.Empty(t, r.leftovers(t), "and the audio is still deleted")
} }
func TestWorkerProcess_AJobThatOutlivesItsTimeoutIsFailedNotStuck(t *testing.T) { func TestWorkerProcess_AJobThatOutlivesItsTimeoutIsFailedNotStuck(t *testing.T) {
+137
View File
@@ -0,0 +1,137 @@
package web_test
// Observes the cross-origin gate around Router (#48): SameSite=Lax does not stop a POST
// from a sibling *.d-ma.be origin, so the router itself must refuse a non-safe request
// whose Origin or Sec-Fetch-Site says it did not come from this site's own pages.
import (
"context"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// foreign is a header set a browser sends for a form post made by another page.
var foreign = map[string]map[string]string{
"sibling subdomain, Sec-Fetch-Site": {"Origin": "https://evil.d-ma.be", "Sec-Fetch-Site": "same-site"},
"other site, Sec-Fetch-Site": {"Origin": "https://evil.example", "Sec-Fetch-Site": "cross-site"},
"sibling subdomain, Origin only": {"Origin": "https://evil.d-ma.be"},
"opaque origin": {"Origin": "null"},
}
// own is a header set for the app's own pages (or a non-browser client): must pass.
var own = map[string]map[string]string{
"same-origin Sec-Fetch-Site": {"Sec-Fetch-Site": "same-origin", "Origin": "http://example.com"},
"matching Origin only": {"Origin": "http://example.com"},
"no browser headers": {},
}
func withHeaders(req *http.Request, h map[string]string) *http.Request {
for k, v := range h {
req.Header.Set(k, v)
}
return req
}
func userCount(t *testing.T) int {
t.Helper()
var n int
require.NoError(t, rawPool(t).QueryRow(context.Background(), `SELECT count(*) FROM users WHERE id = $1`, userID).Scan(&n))
return n
}
func TestCrossOrigin_ARefusedDeleteAccountPostLeavesTheAccountInPlace(t *testing.T) {
for name, h := range foreign {
t.Run(name, func(t *testing.T) {
app, _ := newAccountApp(t)
resetDB(t, rawPool(t))
seedConnection(t, app, "youtube", "ada@channel", "ref")
rec := do(t, app, withHeaders(httptest.NewRequest(http.MethodPost, "/account/delete", nil), h))
assert.Equal(t, http.StatusForbidden, rec.Code)
assert.Equal(t, 1, userCount(t), "the account must survive a refused post")
})
}
}
func TestCrossOrigin_ASameOriginOrHeaderlessDeleteAccountPostIsServed(t *testing.T) {
for name, h := range own {
t.Run(name, func(t *testing.T) {
app, _ := newAccountApp(t)
resetDB(t, rawPool(t))
seedConnection(t, app, "youtube", "ada@channel", "ref")
rec := do(t, app, withHeaders(httptest.NewRequest(http.MethodPost, "/account/delete", nil), h))
assert.Equal(t, http.StatusSeeOther, rec.Code)
assert.Equal(t, "/auth/logout", rec.Header().Get("Location"))
})
}
}
func TestCrossOrigin_ARefusedUploadPostSubmitsNoJob(t *testing.T) {
for name, h := range foreign {
t.Run(name, func(t *testing.T) {
app, sub := newUploadApp(t, audioOK, 200<<20)
rec := do(t, app, withHeaders(uploadReq(t, "m", "m.wav", []byte("RIFF....WAVE")), h))
assert.Equal(t, http.StatusForbidden, rec.Code)
assert.Empty(t, sub.jobs, "a refused post must not enqueue transcription")
})
}
}
func TestCrossOrigin_ARefusedUploadChatPostCallsNoModel(t *testing.T) {
for name, h := range foreign {
t.Run(name, func(t *testing.T) {
app, fc := newUploadChatApp(t)
id := seedUpload(t, userID, "Roadmap sync", "done", meetingText)
req := chatPost("/uploads/"+id+"/chat", url.Values{"message": {"hej"}})
rec := do(t, app, withHeaders(req, h))
assert.Equal(t, http.StatusForbidden, rec.Code)
assert.Zero(t, fc.callCount)
})
}
}
func TestCrossOrigin_AForeignPostIsRefusedOnEachListedStateChangingPath(t *testing.T) {
routes := []string{
"/account/disconnect/youtube",
"/account/summarize-mode",
"/register",
"/paste",
"/v/abc/action",
"/v/abc/summarize",
"/v/abc/retry-now",
"/v/abc/chat",
}
for _, path := range routes {
t.Run(path, func(t *testing.T) {
app := newApp(t)
req := httptest.NewRequest(http.MethodPost, path, strings.NewReader(""))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
rec := do(t, app, withHeaders(req, foreign["sibling subdomain, Sec-Fetch-Site"]))
assert.Equal(t, http.StatusForbidden, rec.Code)
})
}
}
// A login redirect comes back to /auth/callback as a cross-site GET navigation, and
// health probes carry no headers: safe methods must never be refused on origin grounds.
func TestCrossOrigin_ACrossSiteGETIsNotRefused(t *testing.T) {
app := newApp(t)
for _, path := range []string{"/healthz", "/welcome"} {
req := httptest.NewRequest(http.MethodGet, path, nil)
req.Header.Set("Sec-Fetch-Site", "cross-site")
assert.Equal(t, http.StatusOK, do(t, app, req).Code, path)
}
}
+6 -1
View File
@@ -229,7 +229,12 @@ func (a *App) Router() http.Handler {
// resolved user_id. /register lives inside the auth guard but is exempt from // resolved user_id. /register lives inside the auth guard but is exempt from
// the registration gate (you must be able to reach it before you have a user). // the registration gate (you must be able to reach it before you have a user).
root.Handle("/", a.Auth.Middleware(a.registrationGate(app))) root.Handle("/", a.Auth.Middleware(a.registrationGate(app)))
return root
// The session cookie is SameSite=Lax, which still travels on a POST made from a
// sibling *.d-ma.be page (same site, different origin). Refuse any non-safe request
// whose Origin or Sec-Fetch-Site says it did not come from this origin (#48). It wraps
// everything, so a route added later is covered without remembering to ask.
return http.NewCrossOriginProtection().Handler(root)
} }
// handleWelcome renders the public landing page (/welcome). It is mounted outside // handleWelcome renders the public landing page (/welcome). It is mounted outside
+143
View File
@@ -0,0 +1,143 @@
package lintgate
// Observes the exit status of `task lint` (the lint step of `task check`), not the lint
// rules themselves. It runs the repo's real Taskfile in a scratch module so a change to the
// lint command that stops it failing on findings (#41) turns this red.
import (
"bytes"
"errors"
"os"
"os/exec"
"path/filepath"
"regexp"
"strings"
"testing"
)
const scratchWithFinding = `package main
import "os"
func main() { os.Remove("x") }
`
func scratchModule(t *testing.T) string {
t.Helper()
taskfile, err := os.ReadFile(filepath.Join("..", "..", "Taskfile.yml"))
if err != nil {
t.Fatalf("read Taskfile.yml: %v", err)
}
dir := t.TempDir()
for name, body := range map[string]string{
"Taskfile.yml": string(taskfile),
"go.mod": "module scratch\n\ngo 1.21\n",
"main.go": scratchWithFinding,
} {
if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil {
t.Fatalf("write %s: %v", name, err)
}
}
return dir
}
// ansi matches the colour escape sequences task prints when it thinks it has a terminal.
var ansi = regexp.MustCompile("\x1b\\[[0-9;]*m")
// hasSkipNotice reports whether the skip notice was printed as output. Task echoes the command
// line first, and that line contains the same words, so a substring match would be vacuous.
func hasSkipNotice(out string) bool {
for _, line := range strings.Split(ansi.ReplaceAllString(out, ""), "\n") {
if strings.HasPrefix(line, "golangci-lint not installed") {
return true
}
}
return false
}
// runLint runs `task lint` in dir with the given PATH and returns its combined output and exit code.
func runLint(t *testing.T, dir, path string) (string, int) {
t.Helper()
return runTask(t, dir, path, "lint")
}
func runTask(t *testing.T, dir, path string, args ...string) (string, int) {
t.Helper()
taskBin, err := exec.LookPath("task")
if err != nil {
t.Skip("task not installed")
}
cmd := exec.Command(taskBin, args...)
cmd.Dir = dir
cmd.Env = append(os.Environ(), "PATH="+path)
var out bytes.Buffer
cmd.Stdout, cmd.Stderr = &out, &out
err = cmd.Run()
var exitErr *exec.ExitError
switch {
case err == nil:
return out.String(), 0
case errors.As(err, &exitErr):
return out.String(), exitErr.ExitCode()
default:
t.Fatalf("run task lint: %v", err)
return "", -1
}
}
func TestTaskLint_ExitsNonZeroWhenGolangciLintReportsAFinding(t *testing.T) {
if _, err := exec.LookPath("golangci-lint"); err != nil {
t.Skip("golangci-lint not installed")
}
out, code := runLint(t, scratchModule(t), os.Getenv("PATH"))
if code == 0 {
t.Fatalf("task lint exited 0 on a module with an unchecked error; output:\n%s", out)
}
if hasSkipNotice(out) {
t.Fatalf("task lint blamed a missing binary for a lint failure; output:\n%s", out)
}
if !strings.Contains(out, "errcheck") {
t.Fatalf("a non-zero exit without the finding means the linter crashed, not that it found something; output:\n%s", out)
}
}
// The lint step only gates anything if `task check` runs it.
func TestTaskCheck_RunsTheLintStep(t *testing.T) {
out, code := runTask(t, scratchModule(t), os.Getenv("PATH"), "--dry", "check")
if code != 0 {
t.Fatalf("task --dry check exited %d; output:\n%s", code, out)
}
if !strings.Contains(out, "task: [lint]") {
t.Fatalf("task check no longer runs the lint step; output:\n%s", out)
}
}
func TestTaskLint_SkipsWithANoticeWhenGolangciLintIsAbsent(t *testing.T) {
taskBin, err := exec.LookPath("task")
if err != nil {
t.Skip("task not installed")
}
bin := t.TempDir() // a PATH holding task and nothing else
if err := os.Symlink(taskBin, filepath.Join(bin, "task")); err != nil {
t.Fatalf("symlink task: %v", err)
}
out, code := runLint(t, scratchModule(t), bin)
if code != 0 {
t.Fatalf("task lint exited %d without golangci-lint on PATH; output:\n%s", code, out)
}
if !hasSkipNotice(out) {
t.Fatalf("expected the skip notice; output:\n%s", out)
}
}
// CI runs task with colour on, so the notice line starts with an escape sequence; a bare
// prefix match missed it there while passing on a terminal-less local run.
func TestHasSkipNotice_SeesTheNoticeBehindColourCodes(t *testing.T) {
out := "\x1b[32mtask: [lint] ...\n\x1b[0mgolangci-lint not installed — skipping (CI installs it)\n"
if !hasSkipNotice(out) {
t.Fatalf("notice not found in coloured output: %q", out)
}
if hasSkipNotice("\x1b[32mtask: [lint] if command -v golangci-lint ...; echo \"golangci-lint not installed\"\n") {
t.Fatal("the echoed command line must not count as the notice")
}
}