fix(engine): bound the capture drain held under the state lock

[KassaSana]

Problem

AppState::engine_tick drained the capture ring with while (auto ev = capture_.next_event()) while holding mutex_. That loop exits only when the consumer outruns the producer, so sustained input made the critical section as long as the typing. For its duration:

• session and settings commands blocked on mutex_
• the idle poll, the Pomodoro poll, the persistence flush, and every UI emission — all of which follow the drain — were deferred
• a stop request could not shorten a drain already in progress

Shutdown still terminated (the producer is stopped and joined first), so this is a latency defect during ordinary operation, not a hang.

Change

• Cap one tick at kEngineDrainBudget (2,048) events, plus a 20 ms wall-clock ceiling checked every 128 events for when per-event work (window extraction, ONNX inference) makes even 2,048 too many. The count budget is primary because it needs no clock, which keeps ManualClock-driven ticks deterministic.
• Truncating drops nothing — the rest of the ring stays queued. engine_tick now returns whether it stopped on a budget, and the engine loop re-ticks after 1 ms instead of its usual 100, so throughput stays bound by processing speed rather than budget-per-tick. What changes is that mutex_ is released between slices.
• A saturated drain logs at most once per 30 s, since during a backlog the loop runs every millisecond.

Verification

Four new cases, driven through a tests-only capture-start seam that fills the ring with no engine thread racing the drain:

1. an over-budget burst is bounded and resumable across ticks
2. a truncated drain still persists and emits what it computed
3. an under-budget drain reports no backlog
4. end-to-end: a settings() call is not starved while a producer floods the ring for 3 seconds

Case 4 was validated against the defect, not just the fix: with the budgets raised to effectively infinite (the old unbounded drain) the worst wait is 3,708 ms across 2 samples; with them restored it is a few milliseconds across hundreds. The 1,000 ms threshold sits far from both.

Full suite: 643 cases / 388,368 assertions passing. App target builds clean.

[Copilot]

🟡 Changes recommended

Unresolved critical stale-event handling and moderate shutdown, logging, and throttle-sentinel issues prevent approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR bounds capture-event draining under AppState::mutex_ to reduce lock contention during sustained input.

Changes:

• Adds event-count and time-based drain limits with faster backlog re-ticks.
• Throttles saturated-drain logging.
• Adds test seams and regression coverage for draining, persistence, and starvation.

File summaries

File	Summary
tests/test_app_state.cpp	Adds bounded-drain, persistence, resumability, and starvation tests.
tests/app_state_test_access.hpp	Adds synchronous capture/drain test seams.
src/app/state.hpp	Defines drain limits and updates the tick API. Moderate: using 0 as the log timestamp sentinel defeats throttling when the clock reads zero.
src/app/state.cpp	Implements bounded draining and backlog scheduling. Critical: pre-deletion queued events can be processed after deletion. Moderate: remaining events can be lost at shutdown; synchronous logging can extend lock holding; the zero timestamp sentinel can defeat log throttling.

Review details

Suppressed comments (5)

src/app/state.cpp:1900

• The bounded drain can leave work unprocessed at shutdown. If stop_engine() stops the producer after a saturated tick, the engine exits after that slice while CaptureThread::stop() leaves the remaining ring entries queued; destruction then discards them without persistence, so captured events can be lost. Add a final drain/flush after joining the producer, or explicitly define and handle this shutdown loss.

        // Leftover events stay in the ring (single consumer, so nobody else takes them) and
        // are picked up by the next tick, which follows in kEngineBacklogTickIntervalMs.
        std::size_t drained = 0;

src/app/state.cpp:1917

• Reaching the count boundary does not prove that the ring still has an event: a burst of exactly kEngineDrainBudget entries is fully consumed, but this unconditionally reports a backlog and causes the engine to run an unnecessary 1 ms tick (and log a false backlog). The return contract says true means work remains, so use a non-destructive pending-event check before setting this flag.

        if (drained >= kEngineDrainBudget) drain_truncated = true;

src/app/state.cpp:1926

• This synchronous log call runs while mutex_ is held. Logger::log formats the timestamp and writes to its sink synchronously, so a slow or blocked file/stderr sink can keep the state lock held well beyond the advertised drain bound and reintroduce command starvation. Record the throttling decision under the state lock, but defer the actual log write until after that lock is released.

                log().info("engine: capture backlog, drained " + std::to_string(drained) +
                           " events this tick and yielded the state lock");

src/app/state.cpp:1923

• The throttle uses 0 as the “never logged” sentinel, but Clock::steady_ms() is allowed to return zero and ManualClock can be constructed with a zero origin. With such a clock, every saturated tick stores 0, the condition remains true, and a sustained backlog logs once per millisecond instead of once per 30 seconds. Use a separate boolean/optional state (or another sentinel outside the clock domain) for whether a log has been emitted.

            if (last_drain_backlog_log_ms_ == 0 ||
                log_now_ms - last_drain_backlog_log_ms_ >= kEngineBacklogLogIntervalMs) {

src/app/state.hpp:549

• Clock::steady_ms() is defined relative to an arbitrary epoch, so 0 is a valid reading (and the test ManualClock uses it). With a saturated backlog while the clock remains at 0, this member is assigned 0 every tick and the == 0 check below logs on every 1 ms tick, defeating the 30-second throttle and adding logger/lock contention. Use an optional or a sentinel outside the clock's value domain.

    // Uptime at the last "capture backlog" log line. Guarded by mutex_ (written inside the
    // drain phase); 0 means never logged, and the first saturated drain always reports.
    std::int64_t last_drain_backlog_log_ms_ = 0;

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

---

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

[KassaSana]

Reviewed each finding against the code. Three were valid as stated, one was valid but under-rated, and the "critical" one describes a real defect with the wrong attribution. All four are fixed in 707a1dd.

Deleting activity left it in the queue (flagged critical)

The defect is real: the activity epoch fences the rows a tick was about to write, not what capture has already queued, so pre-deletion events could be filed into a session started after the deletion.

The attribution is not. This window does not appear "now" — it exists without the drain bound, because events accumulate between ticks regardless of how the drain terminates. What the bound changes is its size: from one tick's worth to potentially a full ring. That is enough to be worth fixing on its own terms — captured window titles surviving "delete my activity" is a privacy problem, not a bookkeeping one.

delete_all_activity_data and delete_session now drop the queue via a capacity-bounded CaptureThread::discard_pending_events(). Scoped deliberately: delete_session drops it only when the session being erased is the active one, since deleting some other session must not throw away input from the session the user is still running. Both directions are covered by tests.

Shutdown dropped the tail (flagged moderate)

Under-rated — this was the more serious of the two, and a regression the PR introduced. The unbounded drain always left the ring empty, so a stop persisted everything captured; the bounded one exited after a single slice.

Backlog is now part of the loop's exit condition rather than only its pacing: it keeps ticking while work remains, which terminates because stop_engine stops the producer before joining. Writing the test surfaced a second hole the review did not mention: stop_engine can flip the flag before the engine thread is ever scheduled, so a plain while drained nothing. It is now a do/while, so one tick always runs.

The backlog flag lied at the boundary

Correct. A burst of exactly kEngineDrainBudget events is fully consumed, and reporting a backlog for it bought a needless 1 ms tick and a log line about an empty queue. Added RingBuffer::has_pending() — non-destructive, consumer-side — and gated the flag on it instead of inferring from the counter.

The log write held mutex_

Correct, and squarely the bug class this PR exists to fix: Logger formats and writes to its std::ostream sink synchronously, so a slow sink would extend the critical section being bounded. The throttle decision stays under the lock; the write happens after it is released.

Zero timestamp sentinel

Correct. Clock::steady_ms() counts from an arbitrary epoch, so 0 is a legitimate reading and ManualClock sits there in several tests — the throttle would have logged every tick for as long as the clock held 0. Now std::optional<std::int64_t>.

---

646 cases pass locally (three new: shutdown leaves the ring empty with the whole burst persisted; deleting activity erases what was queued; deleting one session leaves another's queued events alone).

Unrelated red check: Security audit / frontend fails on npm audit --audit-level=high with 5 pre-existing lockfile vulnerabilities (1 high). It fails identically on master and this branch touches no frontend code.