devnw-thread-pool
fork of gitlab.com/devnw/zig/thread-pool for zig 0.17
Star
Fork
Watch
devnw-thread-pool docs security REV-001.md
3.8 kB · 80 lines
Markdown
at main

REV-001 — Task Panic Counter Skew Contract (ADR) #

Status: Accepted (contract + observability) Severity: Medium GitLab issue: #35 Closed: 2026-04-16

Problem #

Core.runTask increments counters.active_tasks and (optionally) sets descriptor.claimed_by / descriptor.current_task_started_ns before invoking user code, and decrements / clears them after the invocation returns. If the user's task_fn panics between those two points, Zig's default panic handler aborts the process. That happens to be the desired outcome for a production pool — a panicking task leaves the whole process in an undefined state — but there is a narrow window where a caller with a custom panic handler (one that catches the panic and returns to the scheduler) would observe skewed counters and a descriptor slot that never gets released:

  • counters.active_tasks stays incremented → shutdown(.drain, …) spins until its timeout.
  • descriptor.claimed_by stays set → the next acquireDescriptor that hits this slot would see a stale worker index and (in Debug /ReleaseSafe) panic the double-dequeue sentinel.
  • WaitGroup.pending stays positive → a caller's wg.wait() hangs.

Why not a try/catch? #

Zig panics are not catchable by design. The language deliberately does not provide a try/catch equivalent for @panic / integer overflow / OOB access — those signal a bug in the program, not an error value. Wrapping task_fn in try is a no-op because user code returns void, not an error union. The only way to "recover" from a panic is to install a custom panic handler, which is a process-global concern and explicitly out of scope for a thread-pool library.

Any hot-path instrumentation we could add (errdefer-style cleanup around task_fn.?(…)) would add a write to a cacheline the hot path already dominates — the 5–12% throughput regression budget for SEC-018 and F-002 is already spent, and a recover() shim that never fires in production is a bad trade.

Decision #

  1. Contract. The pool's public contract is "a panicking task_fn is a caller bug that aborts the process." Callers must not install a custom panic handler that attempts to recover and resume into pool state. This is documented on Config.timestamp_mode and reinforced in docs/DESIGN.md (see "Task panic behaviour").

  2. Observability. statsSnapshot() computes a non-authoritative accounting_skew_detected flag that is true when active_tasks > submitted - completed - cancelled at snapshot time. This is a best-effort divergence signal — counters are read with .acquire but not atomically as a group, so a snapshot taken mid- submit will naturally show active_tasks > submitted - completed for a brief window. Operators should treat a persistent skew (multiple snapshots 100ms apart showing the same inequality) as evidence of a panic-recovered task.

  3. No hot-path code change. The fix is contract-only. Adding a panic_observed_count counter was considered and rejected — there is no way to reach it from inside a panicking frame without a custom panic handler, which we do not own.

Non-goals #

  • Surviving a panicking task. A panic is terminal; the pool aborts.
  • Guaranteeing WaitGroup.pending decrements under panic. Callers who want that must wrap their task body in a defer.
  • Catching integer-overflow / OOB / unreachable panics. Same reasoning.
  • src/types.zig: TimestampMode, Config, WaitGroup all gain /// # Task panic behaviour doc sections pointing back to this file.
  • src/pool.zig: Core.runTask has a top-of-function comment describing the skew hazard and the contract.
  • docs/DESIGN.md: high-level contract section.