fix(demo): the janitor re-entry guard actually works now (#902)

[byrongamatos]

Closes #902.

The guard in startup_events() read:

if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
        and not _DEMO_JANITOR_STARTED:

and binds tighter than or, so that's A or (B and C). The not-already-started half never ran when the env var was truthy — the only case that reaches it at all.

A second startup started a second janitor thread, overwrote the handle, and shutdown then joined only the last: the first leaked and kept firing registered hooks hourly, forever.

The guard now lives inside start_janitor(). A caller cannot get operator precedence wrong if there's nothing left for it to get wrong.

Three ways to write this guard wrong. I hit all three.

guard	what breaks
none (the original bug)	double-start; orphaned, unjoinable thread
on the flag — if _DEMO_JANITOR_STARTED: return	stop_janitor() deliberately leaves that flag True when a hook outruns its join timeout, so a later startup can't spawn one beside a live thread. But the hook usually finishes a moment later — thread exits, flag is stale, and a flag-keyed guard then refuses to start a replacement for the rest of the process. Demo cleanup silently dead. (The original bug accidentally masked this by always starting.)
on liveness alone — if thread.is_alive(): return	a timed-out stop leaves the old thread alive but doomed — its stop event is set and it exits as soon as its current hook returns. Treating that as a running janitor skips the replacement, and we're back at the row above a second later.

Both of the last two were Codex [P2]s, on successive passes. The answer: a janitor counts as running only if its thread is alive and it hasn't been told to stop.

And each janitor now owns its stop event

start_janitor() used to _DEMO_JANITOR_STOP.clear() a single shared Event.

Start a replacement while a doomed thread is still finishing a hook, and that clear() resurrects it — it loops back to wait(), sees the flag cleared, and carries on. Two janitors, which is the exact bug we started from.

A fresh Event per janitor makes it impossible: the old thread waits on its own event, which stays set, so it can only exit.

Behaviour

Env semantics unchanged, verified across every value ("", "1", "0", "true", "false", "off") — the old expression and demo_mode_enabled() agree on all of them. The only behavioural change is the idempotency fix.

Four tests, and each wrong guard fails a different subset:

no guard                 -> 2 fail   (double start; orphaned thread)
guard on the flag        -> 2 fail   (never restarts after a timed-out stop)
liveness alone           -> 1 fail   (no replacement for a doomed janitor)
liveness + not-stopping  -> all pass

pytest 2416 · pyflakes 0 · Codex 0.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved demo-mode cleanup reliability during repeated server startups.
  • Prevented duplicate janitor processes and stale cleanup tasks from interfering with future cleanup.
  • Ensured cleanup can recover after a delayed or incomplete shutdown.

• Tests

  • Added coverage for repeated startups, shutdown timeouts, replacement cleanup, and concurrent cleanup activity.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0d462d82-7f60-4b56-9911-c540ca5a08f8

📥 Commits

Reviewing files that changed from the base of the PR and between db3ca34 and f77ed54.

📒 Files selected for processing (3)

• lib/demo_mode.py
• server.py
• tests/test_demo_mode.py

---

📝 Walkthrough

Walkthrough

Demo janitor startup now prevents duplicate live threads, replaces stale or stopped janitors with fresh stop events, and avoids startup re-entry issues. Regression tests cover repeated starts, timed-out stops, and overlapping replacement shutdown.

Changes

Demo janitor lifecycle

Layer / File(s)	Summary
Guard janitor startup and isolate stop events lib/demo_mode.py	start_janitor() skips healthy existing janitors, creates a new stop event for replacements, and binds each thread to its own event.
Use the demo-mode startup gate server.py	startup_events() starts the janitor only when demo mode is enabled, delegating re-entry protection to start_janitor().
Validate duplicate and replacement behavior tests/test_demo_mode.py	Tests cover idempotent starts, repeated startup, timed-out stops, replacement threads, and per-thread stop-event ownership.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the demo janitor re-entry guard fix tied to issue #902.
Linked Issues check	✅ Passed	The changes address #902 by preventing duplicate janitor threads and preserving shutdown/join behavior, with regression tests covering the edge cases.
Out of Scope Changes check	✅ Passed	The PR stays focused on the demo janitor guard fix and its tests, with no obvious unrelated code changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/902-janitor-guard

---

_{Comment @coderabbitai help to get the list of available commands.}