5 people 471fd8d1dd Bound document content extraction time and decouple it from uploads (MM-69098) (#36856) ...

* Bound document content extraction to prevent upload DoS (MM-69098)

Extraction of uploaded Office documents ran with no time or size bound and
acquired a shared worker-pool slot on the request goroutine, so a low-privilege
user could keep cheap-to-upload but expensive-to-extract documents in flight and
degrade file uploads for every user on the server.

- Enforce a configurable per-extraction timeout (FileSettings.ExtractContentTimeout,
  default 10s) and honor the previously-ignored size argument in the document and
  PDF extractors to bound the work performed.
- Run extraction on a dedicated, bounded worker pool with non-blocking submit
  (GoExtraction) so saturating it can never block the request goroutines that
  dispatch uploads; overflow is skipped and backfilled by the ExtractContent job.
- Route the file and resumable-upload extraction dispatch sites through the new
  pool instead of the shared GoBuffered/Go pools.

Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>

* Use require.Fail instead of t.Fatal in extraction pool test

The mattermost-govet tFatal analyzer (run by check-style) forbids t.Fatal
in tests in favor of testify assertions.

Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>

* ci: retrigger workflows (transient container init failure)

Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>

* Use sync.WaitGroup.Go for extraction workers

Satisfies the golangci-lint waitgroupgo/modernize analyzer flagged by
check-style at goroutines.go:58.

Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>

* Fix file close race with detached extraction goroutine

On timeout, extractWithTimeout returns while the converter may still be
reading the input on a detached goroutine. ExtractContentFromFileInfo
previously closed the file via defer as soon as Extract returned, racing
with (and closing the file out from under) that goroutine.

Transfer close ownership to docextractor via ExtractSettings.ReaderCloser:
the reader is now closed only after extraction actually finishes - in the
detached goroutine on the timeout path, or after Extract returns on the
synchronous path. ExtractContentFromFileInfo no longer closes the file
itself.

Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>

* Document that extraction timeout bounds wait time, not CPU

Clarify in ExtractSettings.Timeout and extractWithTimeout that on timeout
the docconv converter keeps running on a detached goroutine (docconv is
not context-aware), so the timeout bounds how long an extraction occupies
a worker slot, not the CPU it consumes. MaxFileSize is the primary bound
on per-extraction work. Note load-shedding as a possible future
improvement.

Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>

* Add tests for PDF size cap and ExtractContentTimeout validation

- TestPdfMaxFileSize exercises the LimitedReaderWithError branch in
  pdf.go: a tight MaxFileSize errors out before extraction, while zero
  and generous limits extract normally.
- TestFileSettingsExtractContentTimeout asserts ExtractContentTimeout=-1
  fails IsValid while 0 (disabled) and 10 (default) pass.

Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>

* Harden extraction shutdown, panic safety, and timeout input validation

- stopExtractionWorkers now drains the queue after signaling stop so a
  worker cannot dequeue and run buffered tasks during shutdown.
- The detached extraction goroutine recovers panics and converts them to
  errors, so a panic in an extractor cannot crash the server.
- Admin console enforces a client-side minimum of 0 for
  ExtractContentTimeout (validators.minValue).
- Test hardening: bounded wait for the detached extractor to start, plus
  a test that a panic is surfaced as an error.

Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>

* ci: retrigger workflows (transient Docker Hub image pull timeout)

Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>

* Correct misleading extraction-skip logs and comments

The queue-full warning logs and GoExtraction doc comments claimed skipped
content would be backfilled by a 'periodic ExtractContent job'. That job is
registered with a nil scheduler, so it never runs automatically -- it only
runs when an admin manually triggers a content extraction job (mmctl extract).

Reword the logs and comments to state that skipped files stay unsearchable
until an admin runs a content extraction job, so we don't imply automatic
recovery that doesn't exist.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Julien Tant <JulienTant@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

2026-06-10 23:12:51 +00:00

..

services

Bound document content extraction time and decouple it from uploads (MM-69098) (#36856)

2026-06-10 23:12:51 +00:00

shared

MM-69175: Fix broken CI steps (#36989)

2026-06-10 13:54:10 +02:00